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

[Petr Vobornik]

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`).
-- 
Petr Vobornik