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

[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