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
diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index 739abcd..f6bcd64 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -910,7 +910,7 @@ IPA.option_widget_base = function(spec, that) {
var parents_selected = [];
- $(that._selector+':checked', that.container).each(function() {
+ $(that._selector+':checked', that.$node).each(function() {
var value = $(this).val();
var option = that.get_option(value);
if (option && option.nested) {
@@ -924,7 +924,7 @@ IPA.option_widget_base = function(spec, that) {
if (option.nested) {
var selected = parents_selected.indexOf(option.value) > -1;
- option.widget.set_enabled(selected, true);
+ option.widget.update_enabled(selected, true);
}
}
}
@@ -937,7 +937,7 @@ IPA.option_widget_base = function(spec, that) {
if (that.$node) {
- $(that._selector+':checked', that.container).each(function() {
+ $(that._selector+':checked', that.$node).each(function() {
var value = $(this).val();
var child_values = [];
var option = that.get_option(value);
@@ -970,7 +970,7 @@ IPA.option_widget_base = function(spec, that) {
var writable = !that.read_only && !!that.writable && that.enabled;
if (!that.nested) {
- that.set_enabled(writable);
+ that.update_enabled(writable);
}
if (values && values.length > 0) {
@@ -996,7 +996,7 @@ IPA.option_widget_base = function(spec, that) {
check(that._selector+'[value="'+ option.value +'"]');
}
if (option.widget) {
- option.widget.set_enabled(writable && has_opt, false);
+ option.widget.update_enabled(writable && has_opt, false);
}
}
} else {
@@ -1013,6 +1013,7 @@ IPA.option_widget_base = function(spec, that) {
var widget = that._child_widgets[j];
widget.writable = that.writable;
widget.read_only = that.read_only;
+ widget.enabled = that.enabled;
widget.update(values);
}
}
@@ -1020,21 +1021,22 @@ IPA.option_widget_base = function(spec, that) {
that.updated.notify([], that);
};
- that.set_enabled = function(enabled, clear) {
- if (that.widget_set_enabled) {
- that.widget_set_enabled(enabled);
- } else {
- that.enabled = enabled;
- }
+ that.set_enabled = function(enabled) {
- if (that._selector && that.container) {
- $(that._selector, that.container).prop('disabled', !enabled);
- }
+ that.enabled = enabled;
+ that.update_enabled(enabled);
+ };
+
+ that.update_enabled = function(enabled, clear) {
+
+ if (!that.$node) return;
+
+ $(that._selector, that.$node).prop('disabled', !enabled);
if (!enabled && clear) that.clear();
for (var i=0; i<that._child_widgets.length;i++) {
- that._child_widgets[i].set_enabled(enabled, clear);
+ that._child_widgets[i].update_enabled(enabled, clear);
}
};
_______________________________________________
Freeipa-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/freeipa-devel
