[Freeipa-devel] [PATCH] 0047 Honor 'enabled' option for widgets

[Petr Vobornik]

On 07/22/2013 04:46 PM, Ana Krivokapic wrote:
> On 07/18/2013 09:47 AM, Petr Vobornik wrote:
>> On 07/17/2013 09:18 PM, Ana Krivokapic wrote:
>>> Hello,
>>>
>>> This patch addresses ticket https://fedorahosted.org/freeipa/ticket/3793.
>>>
>>
>> Hello,
>>
>> 1) IMO  we should not create attribute which is just a negation of another.
>>
>> 2) We should add set_enabled method to base widget. Existing set_enabled
>> methods should use it and maintain widget output consistent with the attribute
>> (ie. one should not directly set the attr and should use set_enabled instead).
>> The method should be also callable when content is not yet created.
>> get_enabled methods might become unnecessary - one can get the state form
>> 'enabled' attribute.
>>
>
> The attached updated patch implements the following changes:
>
> 1) set_enabled method has been added to the base widget class.
> 2) get_enabled/is_enabled methods have been removed.
> 3) Widget classes that inherit from the base widget class override the
> set_enabled method where necessary.
> 4) Using 'enabled: true/false' in the widget definition should now work
> correctly for all types of widgets.
>
>

Thanks.

1. set_enabled method in input_widget uses `that.input`. Input widget is 
a base class which doesn't set the property and therefore we can't be 
certain that the descendant will set it. Also it may break when you call 
set_enabled(val) before create() .

We should test for `that.input` presence.

Same content-created test should be perform on other places: 
widget.js:1017,1147,2006

2. The changes in option_widget_base break disabling if user doesn't 
have write-rights. (can be reproduced when navigated (by manual change 
of url) to service in self-service.

Note the differences between read_only, writable and enabled:
* read_only - reflects metadata
* writable - reflects ACL
* enabled - context specific

read_only and writable don't offer edit interface (label instead of 
textbox). Enabled controls disabled state of textbox. For some widgets 
the result might be the same (radios, checkboxes).

option_widget_base.set_enabled should be changed. The mixin overwrites 
the original method and therefore doesn't set 'enabled' property.

3. multiple_choice_section.set_enabled should be renamed. It's related 
to individual choices and not the widget itself. And then new 
set_enabled should be added which would call the old one for each choice.

4. widget.js:3870 - not sure if it's needed but if so, one should also 
change `action_clicked` method.

-- 
Petr Vobornik