On 08/28/2012 04:46 PM, Endi Sukma Dewata wrote:
Found a couple of issues with Undo:


Updated patch attached.


1. Using the scenario described in the ticket, if I undo the Type back
to User Group the Attributes aren't updated, it still shows the Service
attributes.

Fixed. Now after undo it performs the same change as when changing the type. The fix is not very clean because it is dependent on the order of registration of undo_clicked event handlers, it should suffice.


2. After that, if I undo the Attributes it will show the originally
selected attribute (description) but the attribute will appear at the
end of Service attributes (not User Group attributes) and the attributes
are not sorted.

Fixed by #1. When not performing #1 before, it still behaves this way. I think it is correct behavior otherwise it would still show the undo button.


I also have some comments below.

On 8/22/2012 7:17 AM, Petr Vobornik wrote:
Problem:
  When a permission is edited, and Type switched, the attributes
selected for previous Type are still selected, and update fails, if they
are invalid for the new Type. But it should get deselected or not even
listed if Type changes.

Fix:
  When Type is changed, attribute list is refreshed and still applicable
attributes are chosen. If Type is reverted back, previously chosen
attributes are back as chosen.

  If attributes are extended outside Web UI by not listed attr, this
attr is listed at the list end.

To my understanding the list of ACI attributes are obtained from the
LDAP schema, so if a new attribute is added to the object class the UI
will know about it and show it in the attribute list. However, if the
attribute is added using the extensibleObject the UI may not know about
it because there's no schema change, is this what you meant? In that
case the UI won't show a checkbox for the attribute, so we'd probably
have to use the Filter or Subtree permission target that accepts
arbitrary attributes.

On mod or create, validation of attrs is checking presence of attr in schema but not in object type's object classes. So in CLI admin can define attrs which are not offered in UI. It might be wrong, but Web UI should be able to work with it to avoid possible problems with dirty status right after load.


Ideally the server should support a generic LDAP ACI target which would
accept any combination of LDAP filter, subtree, and attributes, but that
probably depends on the actual needs.

Note:
  If user makes change in attribute list before type change, this change
is forgotten.

https://fedorahosted.org/freeipa/ticket/2617



--
Petr Vobornik
From 3a146d9f88003b0b146914ab3afe4adf775fc910 Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Wed, 22 Aug 2012 14:03:20 +0200
Subject: [PATCH] Permissions: select only applicable options on type change

Problem:
 When a permission is edited, and Type switched, the attributes selected for
 previous Type are still selected, and update fails, if they are invalid for the
 new Type. But it should get deselected or not even listed if Type changes.

Fix:
 When Type is changed, attribute list is refreshed and still applicable attributes
 are chosen. If Type is reverted back, previously chosen  attributes are back as chosen.

 If attributes are extended outside Web UI by not listed attr, this attr is listed at
 the list end.

Note:
 If user makes change in attribute list before type change, this change is forgotten.

https://fedorahosted.org/freeipa/ticket/2617
---
 install/ui/aci.js | 90 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 43 insertions(+), 47 deletions(-)

diff --git a/install/ui/aci.js b/install/ui/aci.js
index 039e633234c0aa968dc6f2f81093507aa57f22f2..bd7de19ab0e1d4bd59431637c80b1291fda25c41 100644
--- a/install/ui/aci.js
+++ b/install/ui/aci.js
@@ -467,6 +467,7 @@ IPA.attributes_widget = function(spec) {
     var that = IPA.checkboxes_widget(spec);
 
     that.object_type = spec.object_type;
+    that.skip_unmatched = spec.skip_unmatched === undefined ? false : spec.skip_unmatched;
 
     var id = spec.name;
 
@@ -512,6 +513,30 @@ IPA.attributes_widget = function(spec) {
         that.create_error_link(container);
     };
 
+    that.create_options = function(options) {
+        var tbody = $('tbody', that.table);
+
+        for (var i=0; i<options.length ; i++){
+            var value = options[i].toLowerCase();
+            var tr = $('<tr/>').appendTo(tbody);
+
+            var td =  $('<td/>').appendTo(tr);
+            td.append($('<input/>',{
+                type: 'checkbox',
+                name: that.name,
+                value: value,
+                'class': 'aci-attribute',
+                change: function() {
+                    that.value_changed.notify([], that);
+                }
+            }));
+            td = $('<td/>').appendTo(tr);
+            td.append($('<label/>',{
+                text: value
+            }));
+        }
+    };
+
     that.update = function(values) {
 
         that.values = [];
@@ -543,26 +568,7 @@ IPA.attributes_widget = function(spec) {
 
         var aciattrs = metadata.aciattrs;
 
-        var tbody = $('tbody', that.table);
-
-        for (var i=0; i<aciattrs.length ; i++){
-            var value = aciattrs[i].toLowerCase();
-            var aci_tr = $('<tr/>').appendTo(tbody);
-
-            var td =  $('<td/>').appendTo(aci_tr);
-            td.append($('<input/>',{
-                type: 'checkbox',
-                name: that.name,
-                value: value,
-                'class': 'aci-attribute',
-                click: function() {
-                    that.value_changed.notify([], that);
-                }
-            }));
-            td =  $('<td/>').appendTo(aci_tr);
-            td.append($('<label/>',{
-                text:value}));
-        }
+        that.create_options(aciattrs);
     };
 
     that.append = function() {
@@ -579,29 +585,8 @@ IPA.attributes_widget = function(spec) {
             }
         }
 
-        if (unmatched.length > 0) {
-            var tbody = $('tbody', that.table);
-
-            for (var j=0; j<unmatched.length; j++) {
-                var value = unmatched[j].toLowerCase();
-                var tr = $('<tr/>').appendTo(tbody);
-
-                var td = $('<td/>').appendTo(tr);
-                td.append($('<input/>', {
-                    type: 'checkbox',
-                    name: that.name,
-                    value: value,
-                    'class': 'aci-attribute',
-                    change: function() {
-                        that.value_changed.notify([], that);
-                    }
-                }));
-
-                td = $('<td/>').appendTo(tr);
-                td.append($('<label/>', {
-                    text: value
-                }));
-            }
+        if (unmatched.length > 0 && !that.skip_unmatched) {
+            that.create_options(unmatched);
         }
     };
 
@@ -765,22 +750,33 @@ IPA.permission_target_policy = function (widget_name) {
 
         type_select.value_changed.attach(function() {
             var type = type_select.save()[0];
-            that.set_attrs_type(type);
+            that.set_attrs_type(type, true);
+        });
+
+        type_select.undo_clicked.attach(function() {
+            var type = type_select.save()[0];
+            that.set_attrs_type(type, true);
         });
     };
 
-    that.set_attrs_type = function(type) {
+    that.set_attrs_type = function(type, skip_unmatched) {
         var attribute_field = that.container.fields.get_field('attrs');
         var attribute_table = that.permission_target.widgets.get_widget('attrs');
+        var skip_unmatched_org = attribute_table.skip_unmatched;
         attribute_table.object_type = type;
+        // skip values which don't belong to new type. Bug #2617
+        attribute_table.skip_unmatched =  skip_unmatched || skip_unmatched_org;
         attribute_field.reset();
+        // force value_change to update dirty status if some unmatched values were skipped
+        attribute_table.value_changed.notify([], attribute_table);
+        attribute_table.skip_unmatched = skip_unmatched_org;
     };
 
     that.update_attrs = function() {
 
         var type_select = that.permission_target.widgets.get_widget('type');
         var type = type_select.save()[0];
-        that.set_attrs_type(type);
+        that.set_attrs_type(type, false);
     };
 
     that.post_create = function() {
@@ -868,7 +864,7 @@ IPA.permission_target_policy = function (widget_name) {
                 }
             ],
             action: function() {
-                that.set_attrs_type('group');
+                that.set_attrs_type('group', false);
             }
         },
         type: {
-- 
1.7.11.4

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

Reply via email to