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

[Ana Krivokapic]

On 07/23/2013 06:09 PM, Petr Vobornik wrote:
> 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.
>

All fixed. Updated patch attached.

Thanks for the review.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0047-03-Honor-enabled-option-for-widgets.patch
Type: text/x-patch
Size: 14130 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130724/f47b0f01/attachment.bin>