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

[Ana Krivokapic]

On 07/25/2013 01:49 PM, Petr Vobornik wrote:
> 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);

Fixed.

>         }
>
> 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.

Agreed, changes squashed to the patch.

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

Good idea, I opened a ticket: https://fedorahosted.org/freeipa/ticket/3818

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

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