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