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

[Ana Krivokapic]

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.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0045-03-Expose-ipaRangeType-in-Web-UI.patch
Type: text/x-patch
Size: 10091 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130729/9e1f83fa/attachment.bin>