Re: [Freeipa-devel] [PATCH] 0067-72: webui for kerberos aliases
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
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
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
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
