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.

From d89a8aa747bf4606e5b62b6182ac8b8e9dd0edf6 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic <akriv...@redhat.com>
Date: Wed, 17 Jul 2013 21:13:42 +0200
Subject: [PATCH] Honor 'enabled' option for widgets.

https://fedorahosted.org/freeipa/ticket/3793
---
 install/ui/src/freeipa/association.js |   1 -
 install/ui/src/freeipa/dns.js         |   3 +-
 install/ui/src/freeipa/facet.js       |   2 +-
 install/ui/src/freeipa/rule.js        |   2 -
 install/ui/src/freeipa/widget.js      | 142 ++++++++++++++++++++++------------
 5 files changed, 93 insertions(+), 57 deletions(-)

diff --git a/install/ui/src/freeipa/association.js b/install/ui/src/freeipa/association.js
index c60c7b8afe9c16ae55e5147574664c60afc43d3e..ad427d66b6b98119b2eb577ae98e4b7c2f1a6932 100644
--- a/install/ui/src/freeipa/association.js
+++ b/install/ui/src/freeipa/association.js
@@ -530,7 +530,6 @@ IPA.association_table_widget = function (spec) {
             $('.action-button', that.table).addClass('action-button-disabled');
             that.unselect_all();
         }
-        that.enabled = enabled;
     };
 
     that.select_changed = function() {
diff --git a/install/ui/src/freeipa/dns.js b/install/ui/src/freeipa/dns.js
index b4085fea8b792e7f642a10373207916886ff50be..0a0fd3f85b33f51c474f3e6a47cca00ae9ffcfe9 100644
--- a/install/ui/src/freeipa/dns.js
+++ b/install/ui/src/freeipa/dns.js
@@ -603,7 +603,7 @@ IPA.dnszone_adder_dialog = function(spec) {
         var zone = zone_w.save()[0] || '';
         var ns = ns_w.save()[0] || '';
 
-        var zone_is_reverse = !zone_w.is_enabled() ||
+        var zone_is_reverse = !zone_w.enabled ||
                               ends_with(zone, '.in-addr.arpa.') ||
                               ends_with(zone, '.ip6.arpa.');
         var relative_ns = true;
@@ -1767,7 +1767,6 @@ IPA.dns.record_type_table_widget = function(spec) {
             $('.action-button', that.table).addClass('action-button-disabled');
             that.unselect_all();
         }
-        that.enabled = enabled;
     };
 
     that.select_changed = function() {
diff --git a/install/ui/src/freeipa/facet.js b/install/ui/src/freeipa/facet.js
index 37106e22f44b2fb50fc79b8183cc62e9eb35b6e4..b01452dd718b894ecb66d29f70242779ff75cfa4 100644
--- a/install/ui/src/freeipa/facet.js
+++ b/install/ui/src/freeipa/facet.js
@@ -1940,7 +1940,7 @@ exp.action_button_widget = IPA.action_button_widget = function(spec) {
     };
 
     that.set_enabled = function(enabled) {
-        that.enabled = enabled;
+        that.widget_set_enabled(enabled);
 
         if (that.button_element) {
             if (enabled) {
diff --git a/install/ui/src/freeipa/rule.js b/install/ui/src/freeipa/rule.js
index f573b56d469ac8c6652f8e49c66fd6fd1259db90..332342bf8f7806da1c77a61d2da8677944657296 100644
--- a/install/ui/src/freeipa/rule.js
+++ b/install/ui/src/freeipa/rule.js
@@ -112,8 +112,6 @@ IPA.rule_association_table_widget = function(spec) {
 
     that.external = spec.external;
 
-    that.enabled = spec.enabled !== undefined ? spec.enabled : true;
-
     that.setup_column = function(column, div, record) {
         var suppress_link = false;
         if (that.external) {
diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index 06fcef563ca416e6e3e1cc454f2e1dd665c68f26..d66468ae245945122a1b730d0301d95d0bd5ff51 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -51,6 +51,7 @@ IPA.widget = function(spec) {
     that.measurement_unit = spec.measurement_unit;
     that.entity = IPA.get_entity(spec.entity); //some old widgets still need it
     that.facet = spec.facet;
+    that.enabled = spec.enabled === undefined ? true : spec.enabled;
 
     that.create = function(container) {
         container.addClass('widget');
@@ -60,6 +61,10 @@ IPA.widget = function(spec) {
     that.clear = function() {
     };
 
+    that.set_enabled = function(value) {
+        that.enabled = value;
+    };
+
     that.set_visible = function(visible) {
 
         if (visible) {
@@ -81,6 +86,7 @@ IPA.widget = function(spec) {
     };
 
     that.widget_create = that.create;
+    that.widget_set_enabled = that.set_enabled;
 
     return that;
 };
@@ -200,6 +206,14 @@ IPA.input_widget = function(spec) {
         }
     };
 
+    that.set_enabled = function(value) {
+        that.widget_set_enabled(value);
+
+        if (that.input) {
+            that.input.prop('disabled', !value);
+        }
+    };
+
     that.on_value_changed = function() {
         var value = that.save();
         that.value_changed.notify([value], that);
@@ -257,7 +271,6 @@ IPA.text_widget = function(spec) {
         that.input = $('<input/>', {
             type: that.input_type,
             name: that.name,
-            disabled: that.disabled,
             size: that.size,
             title: that.tooltip,
             keyup: function() {
@@ -274,6 +287,7 @@ IPA.text_widget = function(spec) {
         }
 
         that.create_error_link(container);
+        that.set_enabled(that.enabled);
     };
 
     that.update = function(values) {
@@ -303,15 +317,6 @@ IPA.text_widget = function(spec) {
         }
     };
 
-    that.is_enabled = function(value) {
-        return !that.input.prop('disabled');
-    };
-
-    that.set_enabled = function(value) {
-
-        that.input.prop('disabled', !value);
-    };
-
     that.clear = function() {
         that.input.val('');
         that.display_control.text('');
@@ -474,7 +479,8 @@ IPA.multivalued_widget = function(spec) {
             name: that.name+'-'+row_index,
             undo: that.undo || row.is_new,
             read_only: that.read_only,
-            writable: that.writable
+            writable: that.writable,
+            enabled: that.enabled
         });
 
         row.widget.create(row.container);
@@ -756,6 +762,8 @@ IPA.option_widget_base = function(spec, that) {
                 that._child_widgets.push(option.widget);
             }
         }
+
+        option.enabled = spec.enabled === undefined ? true : spec.enabled;
         option.label = text.get(option.label);
         option.combine_values = option.combine_values === undefined ? true :
                                     !!option.combine_values;
@@ -794,11 +802,13 @@ IPA.option_widget_base = function(spec, that) {
     that._create_option = function(option, container) {
         var input_name = that.get_input_name();
         var id = that._option_next_id + input_name;
+        var enabled = that.enabled && option.enabled;
 
         $('<input/>', {
             id: id,
             type: that.input_type,
             name: input_name,
+            disabled: !enabled,
             value: option.value,
             title: option.tooltip || that.tooltip,
             change: that.on_input_change
@@ -900,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) {
@@ -914,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);
                 }
             }
         }
@@ -927,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);
@@ -958,9 +968,9 @@ IPA.option_widget_base = function(spec, that) {
             // uncheck all inputs
             check(that._selector, true /*uncheck*/);
 
-            var writable = !that.read_only && !!that.writable;
+            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) {
@@ -986,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 {
@@ -1003,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);
             }
         }
@@ -1010,12 +1021,22 @@ IPA.option_widget_base = function(spec, that) {
         that.updated.notify([], that);
     };
 
-    that.set_enabled = function(enabled, clear) {
+    that.set_enabled = function(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);
 
-        $(that._selector, that.container).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);
         }
     };
 
@@ -1074,7 +1095,7 @@ IPA.checkbox_widget = function (spec) {
     spec.input_type = spec.input_type || 'checkbox';
 
     if (!spec.options) {
-        spec.options = [ { value: checked, label: '' } ];
+        spec.options = [ { value: checked, label: '', enabled: spec.enabled } ];
     }
 
     if (spec.checked) spec.default_value = spec.checked;
@@ -1137,6 +1158,15 @@ IPA.select_widget = function(spec) {
         }
 
         that.create_error_link(container);
+        that.set_enabled(that.enabled);
+    };
+
+    that.set_enabled = function(value) {
+        that.widget_set_enabled(value);
+
+        if (that.select) {
+            that.select.prop('disabled', !value);
+        }
     };
 
     that.create_options = function() {
@@ -1231,7 +1261,6 @@ IPA.textarea_widget = function (spec) {
             name: that.name,
             rows: that.rows,
             cols: that.cols,
-            disabled: that.disabled,
             readOnly: !!that.read_only,
             title: that.tooltip,
             keyup: function() {
@@ -1250,6 +1279,7 @@ IPA.textarea_widget = function (spec) {
         }
 
         that.create_error_link(container);
+        that.set_enabled(that.enabled);
     };
 
     that.save = function() {
@@ -1786,6 +1816,8 @@ IPA.table_widget = function (spec) {
                 name: 'total_pages'
             }).appendTo(that.pagination_control);
         }
+
+        that.set_enabled(that.enabled);
     };
 
     that.prev_page = function() {
@@ -1994,7 +2026,11 @@ IPA.table_widget = function (spec) {
     };
 
     that.set_enabled = function(enabled) {
-        $('input[name="'+that.name+'"]', that.table).prop('disabled', !enabled);
+        that.widget_set_enabled(enabled);
+
+        if (that.table) {
+            $('input[name="'+that.name+'"]', that.table).prop('disabled', !enabled);
+        }
     };
 
     that.clear = function() {
@@ -2122,7 +2158,6 @@ IPA.attribute_table_widget = function(spec) {
             $('.action-button', that.table).addClass('action-button-disabled');
             that.unselect_all();
         }
-        that.enabled = enabled;
     };
 
     that.select_changed = function() {
@@ -2489,6 +2524,7 @@ IPA.combobox_widget = function(spec) {
         }
 
         that.create_error_link(container);
+        that.set_enabled(that.enabled);
     };
 
     that.on_no_close = function() {
@@ -2633,7 +2669,7 @@ IPA.combobox_widget = function(spec) {
     };
 
     that.open = function() {
-        if (!that.read_only) {
+        if (!that.read_only && that.enabled) {
             that.list_container.css('visibility', 'visible');
         }
     };
@@ -2989,33 +3025,19 @@ IPA.button_widget = function(spec) {
             style: that.style,
             click: that.on_click
         }).appendTo(container);
-    };
 
-    that.get_enabled = function() {
-
-        var enabled = true;
-
-        if (that.button) {
-            enabled = that.button.hasClass(that.disabled_class);
-        }
-
-        return enabled;
+        that.set_enabled(that.enabled);
     };
 
     that.set_enabled = function(enabled) {
+        that.widget_set_enabled(enabled);
 
-        enabled ? that.enable() : that.disable();
-    };
-
-    that.enable = function() {
-        if (that.button) {
-            that.button.removeClass(that.disabled_class);
-        }
-    };
-
-    that.disable = function() {
         if (that.button) {
-            that.button.addClass(that.disabled_class);
+            if (enabled) {
+                that.button.removeClass(that.disabled_class);
+            } else {
+                that.button.addClass(that.disabled_class);
+            }
         }
     };
 
@@ -3313,6 +3335,8 @@ IPA.multiple_choice_section = function(spec) {
             choice = choices[i];
             that.create_choice(choice);
         }
+
+        that.set_enabled(that.enabled);
     };
 
     that.create_choice = function(choice) {
@@ -3353,6 +3377,7 @@ IPA.multiple_choice_section = function(spec) {
             id: radio_id,
             value: choice.name,
             checked: enabled,
+            disabled: !that.enabled,
             change: function() {
                 that.select_choice(this.value);
             }
@@ -3375,11 +3400,11 @@ IPA.multiple_choice_section = function(spec) {
         for (i=0; i<that.choices.values.length; i++) {
             choice = that.choices.values[i];
             enabled = choice.name === choice_name;
-            that.set_enabled(choice, enabled);
+            that.set_choice_enabled(choice, enabled);
         }
     };
 
-    that.set_enabled = function (choice, enabled) {
+    that.set_choice_enabled = function (choice, enabled) {
 
         var i, field_name, field, fields, required;
 
@@ -3395,7 +3420,21 @@ IPA.multiple_choice_section = function(spec) {
         }
     };
 
+    that.set_enabled = function(value) {
+        var i, choice;
+
+        that.widget_set_enabled(value);
+
+        for (i=0; i<that.choices.values.length; i++) {
+            choice = that.choices.values[i];
+            that.set_choice_enabled(choice, value);
+        }
+    };
+
     that.init_enabled = function() {
+        if (!that.enabled) {
+            return;
+        }
 
         var i, choice;
 
@@ -3790,7 +3829,8 @@ IPA.sshkey_widget = function(spec) {
 
             dialog.textarea = $('<textarea/>', {
                 'class': 'certificate',
-                readonly: that.read_only
+                readonly: that.read_only,
+                disabled: !that.enabled
             }).appendTo(dialog.container);
 
             var key = that.key.key || '';
@@ -3868,7 +3908,7 @@ IPA.action_panel = function(spec) {
         if (!action.visible) return;
 
         classes = ['action'];
-        state = action.enabled ? 'enabled' : 'disabled';
+        state = action.enabled && that.enabled ? 'enabled' : 'disabled';
         classes.push(state);
 
         li = $('<li/>');
@@ -3908,7 +3948,7 @@ IPA.action_panel = function(spec) {
 
     that.action_clicked = function(action) {
 
-        if (!action.enabled || !action.visible) return;
+        if (!that.enabled || !action.enabled || !action.visible) return;
 
         action.execute(that.facet);
     };
-- 
1.8.1.4

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to