[Freeipa-devel] [PATCH] 0045 Expose ipaRangeType in Web UI

[Petr Vobornik]

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);
-- 
Petr Vobornik