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

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

Reply via email to