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

[Petr Vobornik]

On 07/24/2013 03:52 PM, Ana Krivokapic wrote:
> 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.
>

1. Following code is incorrect (line 2030):

         var input = $('input[name="'+that.name+'"]', that.table);
         if (input) {
             input.prop('disabled', !enabled);
         }

It will perform document wide search, when that.table is not set, and 
find all inputs with that.name. The test should be:

         if (that.table) {
             $('input[name="'+that.name+'"]', 
that.table).prop('disabled', !enabled);
         }

2. There are issues with option_widget_base. This widget is really a beast.

a) There is an existing issue - that.container is not set - which 
revealed itself in this patch. The result is that nested options are not 
enabled. Attaching a fix.

b) I have doubts about option_widget_base changes on line ~1023. It will 
most-likely work for our cases, but has unwanted behavior. It will set 
enabled to false if read_only or writable is false. So enabled will 
remain false even when writable/read_only changes back to true. It won't 
probably happen, but it might if user would somehow obtain new rights 
during Web UI lifetime.

It may be better if original set_enabled method would be renamed and it 
would only reflect the desired state in UI.

Proposed changes are also in attached diff.


I'm thinking about developing a testing page with various widgets where 
one could test how they behave with various setting.

-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: set_enabled.diff
Type: text/x-patch
Size: 3476 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130725/1f7605e8/attachment.bin>