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

[Petr Vobornik]

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