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.

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

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


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:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to