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

[Petr Vobornik]

On 07/26/2013 01:56 PM, Ana Krivokapic wrote:
> 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
>

ACK and pushed to master.
-- 
Petr Vobornik