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
_______________________________________________
Freeipa-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/freeipa-devel
