On 07/29/2013 10:31 AM, Ana Krivokapic wrote:
On 07/29/2013 10:08 AM, Petr Vobornik wrote:
On 07/26/2013 05:54 PM, Ana Krivokapic wrote:
On 07/17/2013 01:53 PM, Petr Vobornik wrote:
On 07/16/2013 06:46 PM, Ana Krivokapic wrote:
Hello,

This patch addresses ticket https://fedorahosted.org/freeipa/ticket/3759.


Hello,

Thanks for the patch, some comments:

1) idrange.js:183 I would avoid modifying widget html output in form methods.
In this case you can simply add `layout: 'vertical'` to 'iparangetype' field
definition.

2) idrange.js:187 Can be replaced by adding `enabled: false` to
'ipanttrusteddomainsid' field definition

3) I would rather see the switching logic encapsulated in a policy object than
in a dialog. The main reason is to avoid using init() call in the factory.
Most code other than method definitions in factory methods create mess in
inheritance chain. Long term plan is to remove most of these calls. In this
case, you can define public init method in the policy which will be
automatically called after dialog instantiation.

4) IIUIC 'ipabaserid' have to be set together with 'ipanttrusteddomainsid' ->
'ipabaserid' should be made required when is_ad_trust is true.

5) As I read plugins/idrange.py:487-530, the logic for enabling/making
required 'ipabaserid' and 'ipasecondarybaserid' is quite more complex than
implemented.

IIUIC 'ipasecondarybaserid' should be required and enabled only when
'ipabaserid' is set. Additionally, both should be required and enabled if
adtrust_is_enabled (in UI: `IPA.trust_enabled`).

All fixed, updated patch attached.

Hello,

thanks for the fix.

1. a little issue: There is invalid state when you open the dialog, change
type to 'AD domain', close dialog and open it again. Fields are reseted, so
'iparangetype' is defaulted to 'local domain' but all other have the state
from 'AD Domain'. Possible fix (init method):
     type_f.widget.updated.attach(that.on_input_change);


Thanks for the catch. Updated patch attached.


ACK and pushed to master.
--
Petr Vobornik

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to