[Freeipa-devel] [PATCH] 0047 Honor 'enabled' option for widgets
Petr Vobornik
pvoborni at redhat.com
Thu Jul 25 11:49:15 UTC 2013
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>
More information about the Freeipa-devel
mailing list