Re: [Freeipa-devel] [PATCH] 0067-72: webui for kerberos aliases

2016-07-01 Thread Petr Vobornik
On 07/01/2016 09:04 AM, Pavel Vomacka wrote:
> 
> 
> On 06/30/2016 05:27 PM, Petr Vobornik wrote:
>> On 06/30/2016 02:48 PM, Pavel Vomacka wrote:
>>> Hello,
>>>
>>> please review these patches. First two patches fix two minor bugs in
>>> custom_command_multivalued_widget.
>>>
>>> The rest of patches add webui for kerberos aliases.
>>>
>>> https://fedorahosted.org/freeipa/ticket/5927
>>>
>> A preliminary review - I only read the code.
>>
>> Patch 0067: LGTM,
>>
>> btw same wrong interface is in on_error_add, but there it is not use
> fixed for on_error_add as well.
>>
>> Patch 0068: lGTM
>>
>> Patch 0069:
>>
>> 1. A nitpick, not necessarily a NACK.
>> krb_custom_command_multivalued_widget should be named e.g.
>> krb_principal_multivalued_widget.
>>
>> 2. Doc texts for the new widges are missing. This can be added later.
>>
>>
>> 3. `!principal_name || principal_name === '')` is the same as
>> `!principal_name`
>>
>> so
>>
>> var principal_name = value[0];
>>
>>  if (!principal_name || principal_name === '') {
>>  principal_name = {};
>>  }
>>
>> could be simplified into
>>var principal_name = value[0] || {};
>>
>> but why is an object set into that.principal_name when it is later used
>> as a text: `that.principal_text.text(that.principal_name);`
>>
> fixed
>> Patch 0070: LGTM
>>
>> Patch 0071: LGTM
>>
>> Patch 0072: LGTM if the change of krbprincipalname to krbcanonicalname
>> is good.
>>
>>
> Updated patches attached.
> 

Focusing on element was returned to patch 68 as discussed offline.

ACK for all.

master:
* df56fd3371bd20a2ce8f5d0097e05437b7827e29 Change error handling in
custom_command_multivalued_widget
* 2232a5bb09b3e99d10598ab64d0bf5d8ef006df4 Set default confirmation
button label to 'Remove'
* 4bc2e3164fbc4fdbbd4ecd1d26001a5d4671dd94 Add widgets for kerberos aliases
* 2da3090a9716bc47e9cf29329ac9bdb734376cb6 Add widget for kerberos
aliases to user page
* 62c4e15d16cf1b29d4a23db146c774ba01bf5935 Add widget for kerberos
aliases to hosts page
* 2ec59b7f232d9119d32d7a5574efba8965904ee8 Add widget for kerberos
aliases to service page

-- 
Petr Vobornik

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCH] 0067-72: webui for kerberos aliases

2016-07-01 Thread Pavel Vomacka



On 06/30/2016 05:27 PM, Petr Vobornik wrote:

On 06/30/2016 02:48 PM, Pavel Vomacka wrote:

Hello,

please review these patches. First two patches fix two minor bugs in
custom_command_multivalued_widget.

The rest of patches add webui for kerberos aliases.

https://fedorahosted.org/freeipa/ticket/5927


A preliminary review - I only read the code.

Patch 0067: LGTM,

btw same wrong interface is in on_error_add, but there it is not use

fixed for on_error_add as well.


Patch 0068: lGTM

Patch 0069:

1. A nitpick, not necessarily a NACK.
krb_custom_command_multivalued_widget should be named e.g.
krb_principal_multivalued_widget.

2. Doc texts for the new widges are missing. This can be added later.


3. `!principal_name || principal_name === '')` is the same as
`!principal_name`

so

var principal_name = value[0];

 if (!principal_name || principal_name === '') {
 principal_name = {};
 }

could be simplified into
   var principal_name = value[0] || {};

but why is an object set into that.principal_name when it is later used
as a text: `that.principal_text.text(that.principal_name);`


fixed

Patch 0070: LGTM

Patch 0071: LGTM

Patch 0072: LGTM if the change of krbprincipalname to krbcanonicalname
is good.



Updated patches attached.

--
Pavel^3 Vomacka

From 27d3d1b23a58de9fb8133ea7ad7102dc8f4118df Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Thu, 30 Jun 2016 14:32:27 +0200
Subject: [PATCH 1/6] Change error handling in
 custom_command_multivalued_widget

The custom_command_multivalued_widget now handles remove and add commands errors
correctly and shows error message.

Part of: https://fedorahosted.org/freeipa/ticket/5381

add_error
---
 install/ui/src/freeipa/widget.js | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index 0972efadb46ac7b5811f4e072121a448618fe434..e3c2dc555df9dbebdea62e19ce445cae4d60c8f7 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -1533,8 +1533,11 @@ IPA.custom_command_multivalued_widget = function(spec) {
 /**
  * Called on error of add command. Override point.
  */
-that.on_error_add = function(data) {
-that.adder_dialog.focus_first_element();
+that.on_error_add = function(xhr, text_status, error_thrown) {
+if (error_thrown.message) {
+var msg = error_thrown.message;
+IPA.notify(msg, 'error');
+}
 };
 
 /**
@@ -1548,8 +1551,11 @@ IPA.custom_command_multivalued_widget = function(spec) {
 /**
  * Called on error of remove command. Override point.
  */
-that.on_error_remove = function(data) {
-IPA.notify(data.result.summary, 'error');
+that.on_error_remove = function(xhr, text_status, error_thrown) {
+if (error_thrown.message) {
+var msg = error_thrown.message;
+IPA.notify(msg, 'error');
+}
 };
 
 /**
-- 
2.5.5

From f52a0e5561497e0a86baa149eb7aa949f7b04c07 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Thu, 30 Jun 2016 14:34:33 +0200
Subject: [PATCH 3/6] Add widgets for kerberos aliases

Create own custom_command_multivalued_widget for kerberos aliases.

https://fedorahosted.org/freeipa/ticket/5927
---
 install/ui/src/freeipa/widget.js   | 108 +
 install/ui/test/data/ipa_init.json |   6 +++
 ipaserver/plugins/internal.py  |   6 +++
 3 files changed, 120 insertions(+)

diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index d6c1672ad614d13642fe4c9e3dcef55d458bcd7d..c365a8c4f83a039f0e9b2d454e2d9ab1d8fc0772 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -1796,6 +1796,110 @@ IPA.custom_command_multivalued_widget = function(spec) {
 return that;
 };
 
+/**
+ * Multivalued widget which is used for working with kerberos principal aliases.
+ *
+ * @class
+ * @extends IPA.custom_command_multivalued_widget
+ */
+IPA.krb_principal_multivalued_widget = function (spec) {
+
+spec = spec || {};
+
+spec.adder_dialog_spec = spec.adder_dialog_spec || {
+title: '@i18n:krbaliases.adder_title',
+fields: [
+{
+$type: 'text',
+name: 'krbprincalname',
+label: '@i18n:krbaliases.add_krbal_label'
+}
+]
+};
+
+var that = IPA.custom_command_multivalued_widget(spec);
+
+that.create_remove_dialog_title = function(row) {
+return text.get('@i18n:krbaliases.remove_title');
+};
+
+that.create_remove_dialog_message = function(row) {
+var message = text.get('@i18n:krbaliases.remove_message');
+message = message.replace('${alias}', row.widget.principal_name);
+
+return message;
+};
+
+
+that.create_remove_args = function(row) {
+var pkey = that.facet.get_pkey();
+var krbprincipalname = row.widget.pri

Re: [Freeipa-devel] [PATCH] 0067-72: webui for kerberos aliases

2016-06-30 Thread Petr Vobornik
On 06/30/2016 02:48 PM, Pavel Vomacka wrote:
> Hello,
> 
> please review these patches. First two patches fix two minor bugs in
> custom_command_multivalued_widget.
> 
> The rest of patches add webui for kerberos aliases.
> 
> https://fedorahosted.org/freeipa/ticket/5927
> 

A preliminary review - I only read the code.

Patch 0067: LGTM,

btw same wrong interface is in on_error_add, but there it is not use


Patch 0068: lGTM

Patch 0069:

1. A nitpick, not necessarily a NACK.
krb_custom_command_multivalued_widget should be named e.g.
krb_principal_multivalued_widget.

2. Doc texts for the new widges are missing. This can be added later.


3. `!principal_name || principal_name === '')` is the same as
`!principal_name`

so

var principal_name = value[0];

if (!principal_name || principal_name === '') {
principal_name = {};
}

could be simplified into
  var principal_name = value[0] || {};

but why is an object set into that.principal_name when it is later used
as a text: `that.principal_text.text(that.principal_name);`


Patch 0070: LGTM

Patch 0071: LGTM

Patch 0072: LGTM if the change of krbprincipalname to krbcanonicalname
is good.


-- 
Petr Vobornik

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


[Freeipa-devel] [PATCH] 0067-72: webui for kerberos aliases

2016-06-30 Thread Pavel Vomacka

Hello,

please review these patches. First two patches fix two minor bugs in 
custom_command_multivalued_widget.


The rest of patches add webui for kerberos aliases.

https://fedorahosted.org/freeipa/ticket/5927

--
Pavel^3 Vomacka

From d6e0337fd83a4e337c429ecc23038e7af754312e Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Thu, 30 Jun 2016 14:32:27 +0200
Subject: [PATCH 1/6] Change error handling of remove command

The custom_command_multivalued_widget handles remove command error correctly and shows
errror message.

Part of: https://fedorahosted.org/freeipa/ticket/5381
---
 install/ui/src/freeipa/widget.js | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index 0972efadb46ac7b5811f4e072121a448618fe434..1f767ed14342bd3bb5c8c7ac5384c34a9b2c3abe 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -1548,8 +1548,11 @@ IPA.custom_command_multivalued_widget = function(spec) {
 /**
  * Called on error of remove command. Override point.
  */
-that.on_error_remove = function(data) {
-IPA.notify(data.result.summary, 'error');
+that.on_error_remove = function(xhr, text_status, error_thrown) {
+if (error_thrown.message) {
+var msg = error_thrown.message;
+IPA.notify(msg, 'error');
+}
 };
 
 /**
-- 
2.5.5

From a132113a19d92c7c3bb3a6f0fdc3b922009c8f0e Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Thu, 30 Jun 2016 14:34:10 +0200
Subject: [PATCH 2/6] Set default confirmation button label to 'Remove'

Part of: https://fedorahosted.org/freeipa/ticket/5831
---
 install/ui/src/freeipa/widget.js | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index 1f767ed14342bd3bb5c8c7ac5384c34a9b2c3abe..c422f8aa9aefdf67a5770ff0b58754e0d3eb9e04 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -1672,7 +1672,8 @@ IPA.custom_command_multivalued_widget = function(spec) {
 var spec = that.remove_dialog_spec || {
 title: title,
 message: message,
-on_ok: perform_remove
+on_ok: perform_remove,
+ok_label: '@i18n:buttons.remove'
 };
 
 that.remove_dialog = IPA.confirm_dialog(spec);
-- 
2.5.5

From d7d6a29361b30e09e4c58451ea3bf21c1bd0168d Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Thu, 30 Jun 2016 14:34:33 +0200
Subject: [PATCH 3/6] Add widgets for kerberos aliases

Create own custom_command_multivalued_widget for kerberos aliases.

https://fedorahosted.org/freeipa/ticket/5927
---
 install/ui/src/freeipa/widget.js   | 99 ++
 install/ui/test/data/ipa_init.json |  6 +++
 ipaserver/plugins/internal.py  |  6 +++
 3 files changed, 111 insertions(+)

diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index c422f8aa9aefdf67a5770ff0b58754e0d3eb9e04..96a1643bb87980cce674ef97de45d3f2823cfa3e 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -1793,6 +1793,101 @@ IPA.custom_command_multivalued_widget = function(spec) {
 return that;
 };
 
+IPA.krb_custom_command_multivalued_widget = function (spec) {
+
+spec = spec || {};
+
+spec.adder_dialog_spec = spec.adder_dialog_spec || {
+title: '@i18n:krbaliases.adder_title',
+fields: [
+{
+$type: 'text',
+name: 'krbprincalname',
+label: '@i18n:krbaliases.add_krbal_label'
+}
+]
+};
+
+var that = IPA.custom_command_multivalued_widget(spec);
+
+that.create_remove_dialog_title = function(row) {
+return text.get('@i18n:krbaliases.remove_title');
+};
+
+that.create_remove_dialog_message = function(row) {
+var message = text.get('@i18n:krbaliases.remove_message');
+message = message.replace('${alias}', row.widget.principal_name);
+
+return message;
+};
+
+
+that.create_remove_args = function(row) {
+var pkey = that.facet.get_pkey();
+var krbprincipalname = row.widget.principal_name;
+krbprincipalname = [ krbprincipalname ];
+
+var args = [
+pkey,
+krbprincipalname
+];
+
+return args;
+};
+
+that.create_add_args = function(row) {
+var pkey = that.facet.get_pkey();
+var krbprincipalname = that.adder_dialog.get_field('krbprincalname').value;
+
+var args = [
+pkey,
+krbprincipalname
+];
+
+return args;
+};
+
+return that;
+};
+
+IPA.krb_principal_widget = function(spec) {
+spec = spec || {};
+
+var that = IPA.input_widget();
+
+that.create = function(container) {
+that.widget_create(container);
+
+that.principal_text = $('', {
+'class': 'kr