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

[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