On 02/10/2011 01:13 AM, Endi Sukma Dewata wrote:
On 2/9/2011 7:06 PM, Adam Young wrote:



A few comments:

1. The functionality seems to be working, but the layout is a bit different. Previously the label (e.g. Filter) and the widget (e.g. text field) occupy the same line. Right now they occupy different lines and not aligned with the labels & widgets above it (e.g. Permission name). I'd like the UXD team to review this change.

I had mIssed the classes that these things needed.  Added them back in.


2. The jQuery selectors on lines 427, 462, 472 in aci.js are not qualified, so they will be doing a global search. I'd rather store the object reference somewhere and use it directly without searching for it again. For example, line 411 can be changed as follows:

  target_type.container = $('<dl/>', {

Then line 427 can be changed as follows:

  target_type.container.css('display', 'block');

Done.  Good idea/


3. The indentation of the target_types array in aci.js is inconsistent.
Fixed

4. The IPA.hidden_widget doesn't seem to be used. Should this be removed?
Gone baby gone

5. For the changes in dialog.js, it's not necessary to check section.reset()'s presence before calling it. All sections will have a reset() function because it's inherited from the base class.

Removed

6. For the changes in widget.js, let's do this in a separate patch. We'll combine the create/setup in a more consistent way.

Agreed. This was actually part of trial and error to get it to work, and it didn't need to be there. Gone.

7. There are some jslint warnings.

Fixed
From c88f50789f8ae94e852b15aaf8970f5c506554f9 Mon Sep 17 00:00:00 2001
From: Adam Young <ayo...@redhat.com>
Date: Mon, 7 Feb 2011 23:02:43 -0500
Subject: [PATCH] target section without radio buttons
 ACI target section refactored into an array of widget-like objects.
 The radio buttons have been replaced by a select box.  THe select is not visible on the details page.

https://fedorahosted.org/freeipa/ticket/924
---
 install/ui/aci.js            |  519 +++++++++++++++++++++---------------------
 install/ui/dialog.js         |    3 +
 install/ui/test/aci_tests.js |   43 +++-
 install/ui/widget.js         |    6 +-
 4 files changed, 293 insertions(+), 278 deletions(-)

diff --git a/install/ui/aci.js b/install/ui/aci.js
index e515902c5c83451389b5c9dde8115e087f9686f3..fce6846dc56ec4722239673f6b9fc9ec2c939aa9 100644
--- a/install/ui/aci.js
+++ b/install/ui/aci.js
@@ -36,6 +36,8 @@ IPA.attributes_widget = function(spec) {
 
     that.create = function(container) {
 
+
+
         that.table = $('<table/>', {
             id:id,
             'class':'search-table aci-attribute-table'
@@ -129,7 +131,8 @@ IPA.attributes_widget = function(spec) {
         var unmatched = [];
 
         for (var i=0; i<that.values.length; i++) {
-            var input = $('input[name="'+that.name+'"][value="'+that.values[i]+'"]', that.container);
+            var input = $('input[name="'+that.name+'"]'+
+                          '[value="'+that.values[i]+'"]', that.container);
             if (!input.length) {
                 unmatched.push(that.values[i]);
             }
@@ -180,29 +183,6 @@ IPA.rights_widget = function(spec) {
     return that;
 };
 
-IPA.hidden_widget = function(spec) {
-    spec.label = '';
-    var that = IPA.widget(spec);
-    that.id = spec.id;
-    var value = spec.value || '';
-    that.create = function(container){
-        $('<input/>',{
-            type:'hidden',
-            'id':that.id,
-            value: value
-        }).
-            appendTo(container);
-    };
-
-    that.save = function(){
-        return [value];
-    };
-    that.reset = function(){
-
-    };
-    return that;
-};
-
 
 IPA.rights_section = function() {
     var spec =  {
@@ -210,7 +190,8 @@ IPA.rights_section = function() {
         'label': 'Rights'
     };
     var that = IPA.details_section(spec);
-    that.add_field(IPA.rights_widget({name: 'permissions', label: 'Permissions', join: true}));
+    that.add_field(IPA.rights_widget(
+        {name: 'permissions', label: 'Permissions', join: true}));
 
     return that;
 };
@@ -221,265 +202,268 @@ IPA.target_section = function(spec) {
     spec = spec || {};
 
     var that = IPA.details_section(spec);
-
     that.undo = typeof spec.undo == 'undefined' ? true : spec.undo;
 
-    var groupings = ['aci_by_type',  'aci_by_query', 'aci_by_group',
-                     'aci_by_filter' ];
-    var inputs = ['input', 'select', 'textarea'];
-
-    function disable_inputs() {
-        for (var g = 0; g < groupings.length; g += 1 ){
-            for (var t = 0 ; t < inputs.length; t += 1){
-                $('.' + groupings[g] + ' '+ inputs[t]).
-                    attr('disabled', 'disabled');
+    that.filter_text = IPA.text_widget({name: 'filter', undo: that.undo});
+    that.subtree_textarea = IPA.textarea_widget({
+        name: 'subtree',
+        cols: 30, rows: 1,
+        undo: that.undo
+    });
+    that.group_select = IPA.entity_select_widget(
+        {name: 'targetgroup', entity:'group', undo: that.undo});
+    that.type_select = IPA.select_widget({name: 'type', undo: that.undo});
+    that.attribute_table = IPA.attributes_widget({
+        name: 'attrs', undo: that.undo});
+
+    that.add_field(that.filter_text);
+    that.add_field(that.subtree_textarea);
+    that.add_field(that.group_select );
+    that.add_field(that.type_select);
+    that.add_field(that.attribute_table);
+
+    var target_types = [
+        {
+            name:'filter',
+            create: function(dl){
+
+                $('<dt/>').
+                    append($('<label/>', {
+                        text: 'Filter'
+                    })).
+                    appendTo(dl);
+
+                var dd = $('<dd/>', {
+                    'class': 'aci_by_filter first'
+                }).appendTo(dl);
+
+                var span = $('<span/>', {
+                    name: 'filter'
+                }).appendTo(dd);
+
+                that.filter_text.create(span);
+            },
+            load: function(record){
+                that.filter_text.load(record);
+            },
+            save: function(record){
+                record.filter = that.filter_text.save()[0];
+            }
+        },
+        {
+            name:'subtree',
+            create:function(dl) {
+                $('<dt/>').
+                    append($('<label/>', {
+                        text: 'By Subtree'
+                    })).
+                    appendTo(dl);
+                var dd = $('<dd/>', {
+                    'class': 'aci_by_query first'
+                }).appendTo(dl);
+                var span = $('<span/>', {
+                    name: 'subtree'
+                }).appendTo(dd);
+                that.subtree_textarea.create(span);
+            },
+            load: function(record){
+                that.subtree_textarea.load(record);
+            },
+            save: function(record){
+                record.subtree = that.subtree_textarea.save()[0];
+            }
+        },
+        {
+            name:'targetgroup',
+            create:  function (dl) {
+                $('<dt/>').
+                    append($('<label/>', {
+                        text: 'Target Group'
+                    })).
+                    appendTo(dl);
+                var dd = $('<dd/>', {
+                    'class': 'aci_by_group first'
+                }).appendTo(dl);
+                var span = $('<span/>', {
+                    name: 'targetgroup'
+                }).appendTo(dd);
+                that.group_select.create(span);
+            },
+            load: function(record){
+                that.group_select.entity_select.val(record.targetgroup);
+            },
+            save: function(record){
+                record.targetgroup = that.group_select.save()[0];
+            }
+        },
+        {
+            name:'type',
+            create:   function(dl) {
+                $('<dt/>').
+                    append($('<label/>', {
+                        text: 'Object By Type'
+                    })).
+                    appendTo(dl);
+                var dd = $('<dd/>', {
+                    'class': 'aci_by_type first'
+                }).appendTo(dl);
+                var span = $('<span/>', {
+                    name: 'type'
+                }).appendTo(dd);
+                that.type_select.create(span);
+                span = $('<span/>', {
+                    name: 'attrs'
+                }).appendTo(dl);
+
+                that.attribute_table.create(span);
+
+                var select = that.type_select.select;
+
+                select.change(function() {
+                    that.attribute_table.object_type =
+                        that.type_select.save()[0];
+                    that.attribute_table.reset();
+                });
+                select.append($('<option/>', {
+                    value: '',
+                    text: ''
+                }));
+                var type_params = IPA.get_param_info('permission', 'type');
+                for (var i=0; i<type_params.values.length; i++){
+                    select.append($('<option/>', {
+                        value: type_params.values[i],
+                        text: type_params.values[i]
+                    }));
+                }
+                that.type_select.update = function() {
+                    that.type_select.select_update();
+                    that.attribute_table.object_type =
+                        that.type_select.save()[0];
+                 that.attribute_table.reset();
+             };
+            },
+            load: function(record){
+                that.type_select.load(record);
+                that.attribute_table.object_type = record.type;
+                that.attribute_table.reset();
+            },
+            save: function(record){
+                record.type = that.type_select.save()[0];
+                record.attrs =  that.attribute_table.save().join(',');
+            }
+        }] ;
+
+    var target_type = target_types[0];
+    var class_prefix = '#aci-target-';
+
+    function show_target_type(type_to_show){
+        for (var i =0 ; i < target_types.length; i +=1){
+            if ( target_types[i].name === type_to_show){
+                target_type = target_types[i];
+                target_type.container.css('display', 'block');
+            }else{
+                target_types[i].container.css('display', 'none');
             }
         }
-    }
-    function enable_by(grouping) {
-        for (var t = 0 ; t < inputs.length; t += 1){
-            $('.' + grouping + ' '+ inputs[t]).
-                attr('disabled', '');
-        }
-    }
-
-    function display_filter_target(dl) {
-        $('<dt/>').
-        append($('<input/>', {
-            type: 'radio',
-            name: 'aci_type',
-            checked: 'true',
-            id: 'aci_by_filter'
-        })).
-        append($('<label/>', {
-            text: 'Filter'
-        })).
-        appendTo(dl);
-
-        var dd = $('<dd/>', {
-            'class': 'aci_by_filter first'
-        }).appendTo(dl);
-
-        var span = $('<span/>', {
-            name: 'filter'
-        }).appendTo(dd);
-
-        that.filter_text.create(span);
-    }
-
-
-    function display_type_target(dl) {
-        $('<dt/>').
-        append($('<input/>', {
-            type: 'radio',
-            name: 'aci_type',
-            checked: 'true',
-            id: 'aci_by_type'
-        })).
-        append($('<label/>', {
-            text: 'Object By Type'
-        })).
-        appendTo(dl);
 
-        var dd = $('<dd/>', {
-            'class': 'aci_by_type first'
-        }).appendTo(dl);
-
-        var span = $('<span/>', {
-            name: 'type'
-        }).appendTo(dd);
-
-        that.type_select.create(span);
-
-        span = $('<span/>', {
-            name: 'attrs'
-        }).appendTo(dl);
-
-        that.attribute_table.create(span);
     }
-
-    function display_query_target(dl) {
-        $('<dt/>').
-        append($('<input/>', {
-            type: 'radio',
-            name: 'aci_type',
-            id: 'aci_by_query'
-        })).
-        append($('<label/>', {
-            text: 'By Subtree'
-        })).
-        appendTo(dl);
-
-        var dd = $('<dd/>', {
-            'class': 'aci_by_query first'
-        }).appendTo(dl);
-
-        var span = $('<span/>', {
-            name: 'subtree'
-        }).appendTo(dd);
-
-        that.subtree_textarea.create(span);
-    }
-
-    function display_group_target(dl) {
-        $('<dt/>').
-            append($('<input />', {
-                type: 'radio',
-                name: 'aci_type',
-                id: 'aci_by_group'
-            })).
-            append($('<label/>', {
-                text: 'Target Group'
-            })).
-            appendTo(dl);
-
-        var dd = $('<dd/>', {
-            'class': 'aci_by_group first'
-        }).appendTo(dl);
-
-        var span = $('<span/>', {
-            name: 'targetgroup'
-        }).appendTo(dd);
-
-        that.group_select.create(span);
-    }
-
     that.create = function(container) {
+
         var dl =  $('<dl/>', {
             'class': 'aci-target'
         }).appendTo(container);
-
-        display_filter_target(dl);
-        display_query_target(dl);
-        display_group_target(dl);
-        display_type_target(dl);
-
-        $('#aci_by_filter', dl).click(function() {
-            disable_inputs();
-            enable_by(groupings[3]);
-        });
-
-        $('#aci_by_type', dl).click(function() {
-            disable_inputs();
-            enable_by(groupings[0]);
-        });
-
-        $('#aci_by_query', dl).click(function() {
-            disable_inputs();
-            enable_by(groupings[1]);
-        });
-
-        $('#aci_by_group', dl).click(function() {
-            disable_inputs();
-            enable_by(groupings[2]);
-        });
-
-        $('#aci_by_type', dl).click();
-    };
-
-    that.setup = function(container) {
-        that.section_setup(container);
-
-        var select = that.type_select.select;
-
-        select.change(function() {
-            that.attribute_table.object_type = that.type_select.save()[0];
-            that.attribute_table.reset();
-        });
-
-        select.append($('<option/>', {
-            value: '',
-            text: ''
-        }));
-
-        var type_params = IPA.get_param_info('permission', 'type');
-        for (var i=0; i<type_params.values.length; i++){
-            select.append($('<option/>', {
-                value: type_params.values[i],
-                text: type_params.values[i]
+        $('<dt>Target:</dt>').appendTo(dl);
+
+        if (!that.undo){
+            dl.css('display','block');
+        }
+        that.target_type_select =  $('<select></select>',{
+            change:function(){
+                show_target_type(this.value);
+            }});
+
+        $('<dd/>',
+          {"class":"first"}).
+            append(that.target_type_select).appendTo(dl);
+
+        for (var i = 0 ; i < target_types.length; i += 1){
+            target_type = target_types[i];
+            dl =  $('<dl/>', {
+                'class': 'aci-target' ,
+                id:+ target_type.name,
+                style: 'display:none'
+            }).appendTo(container);
+
+            that.target_type_select.append($('<option/>',{
+                text: target_type.name,
+                value : target_type.name
             }));
+            target_type.create(dl);
+            target_type.container = dl;
         }
-
-        that.type_select.update = function() {
-            that.type_select.select_update();
-            that.attribute_table.object_type = that.type_select.save()[0];
-            that.attribute_table.reset();
-        };
+        /*
+           default for the add dialog
+        */
+        target_type = target_types[0];
+        that.target_type_select.val( target_type.name);
+        target_type.container.css('display', 'block');
     };
 
-    function set_aci_type(record) {
-        if (record.filter) {
-            $('#aci_by_filter').click();
+    function reset_target_widgets(){
+        that.filter_text.record = null;
+        that.subtree_textarea.record = null;
+        that.group_select.record = null;
+        that.type_select.record = null;
+        that.attribute_table.record = null;
 
-        } else if (record.subtree) {
-            $('#aci_by_query').click();
+        that.filter_text.reset();
+        that.subtree_textarea.reset();
+        that.group_select.reset();
+        that.type_select.reset();
+        that.attribute_table.reset();
+    }
 
-        } else if (record.targetgroup) {
-            $('#aci_by_group').click();
+    function set_target_type(record) {
 
-        } else if (record.type) {
-            $('#aci_by_type').click();
+        reset_target_widgets();
 
-        } else {
+        var target_type_name ;
+        for (var i = 0 ; i < target_types.length; i += 1){
+            target_type = target_types[i];
+            if (record[target_type.name]){
+                target_type_name = target_type.name;
+                break;
+            }
+        }
+        if (!target_type_name){
             alert('permission with invalid target specification');
+            return;
         }
+
+        target_type.container.css('display', 'block');
+        that.target_type_select.val( target_type_name);
+        target_type.load(record);
     }
-
-    that.load = function(record) {
-
-        set_aci_type(record);
-        that.attribute_table.object_type = record.type;
-
+    that.load = function(record){
         that.section_load(record);
+        that.reset();
     };
-
     that.reset = function() {
-
-        set_aci_type(that.record);
-        that.attribute_table.object_type = that.record.type;
-
+        for (var i = 0 ; i < target_types.length ; i +=1 ){
+            target_types[i].container.css('display', 'none');
+        }
+        if (that.record){
+            set_target_type(that.record);
+            that.attribute_table.object_type = that.record.type;
+        }else{
+            reset_target_widgets();
+        }
         that.section_reset();
-    };
 
-    that.init = function() {
-        that.filter_text = IPA.text_widget({name: 'filter', undo: that.undo});
-        that.add_field(that.filter_text);
-
-        that.subtree_textarea = IPA.textarea_widget({
-            name: 'subtree',
-            cols: 30, rows: 1,
-            undo: that.undo
-        });
-        that.add_field(that.subtree_textarea);
-
-        that.group_select = IPA.entity_select_widget(
-            {name: 'targetgroup', entity:'group', undo: that.undo});
-        that.add_field(that.group_select);
-
-        that.type_select = IPA.select_widget({name: 'type', undo: that.undo});
-        that.add_field(that.type_select);
-
-        that.attribute_table = IPA.attributes_widget({name: 'attrs', undo: that.undo});
-        that.add_field(that.attribute_table);
     };
 
     that.save = function(record) {
-
-        var record_type = $("input[name='aci_type']:checked").attr('id');
-
-        if (record_type === 'aci_by_group') {
-            record.targetgroup = that.group_select.save()[0];
-
-        } else if (record_type === 'aci_by_type') {
-            record.type = that.type_select.save()[0];
-            record.attrs =   that.attribute_table.save().join(',');
-
-        } else if (record_type === 'aci_by_query') {
-            record.subtree = that.subtree_textarea.save([0]);
-
-        } else if (record_type === 'aci_by_filter') {
-            record.filter = that.filter_text.save()[0];
-        }
+        target_type.save(record);
     };
 
     return that;
@@ -534,8 +518,11 @@ IPA.entity_factories.permission = function() {
                         width: '700px'
                     }).
                         field(IPA.text_widget({name: 'cn', undo: false})).
-                        field(IPA.rights_widget({name: 'permissions', label: 'Permissions', join: true, undo: false})).
-                        section(IPA.target_section({name: 'target', label: 'Target', undo: false})))).
+                        field(IPA.rights_widget(
+                            {name: 'permissions', label: 'Permissions', 
+                             join: true, undo: false})).
+                        section(IPA.target_section(
+                            {name: 'target', label: 'Target', undo: false})))).
         facet(
             IPA.permission_details_facet({ name: 'details' }).
                 section(
@@ -561,7 +548,8 @@ IPA.entity_factories.privilege = function() {
                         title: 'Add Privilege'
                     }).
                         field(IPA.text_widget({ name: 'cn', undo: false})).
-                        field(IPA.text_widget({ name: 'description', undo: false})))).
+                        field(IPA.text_widget(
+                            { name: 'description', undo: false})))).
         facet(
             IPA.details_facet({name:'details'}).
                 section(
@@ -596,7 +584,8 @@ IPA.entity_factories.role = function() {
                         title: 'Add Role'
                     }).
                         field(IPA.text_widget({ name: 'cn', undo: false})).
-                        field(IPA.text_widget({ name: 'description', undo: false})))).
+                        field(IPA.text_widget(
+                            { name: 'description', undo: false})))).
         facet(
             IPA.details_facet({name:'details'}).
                 section(
@@ -657,9 +646,11 @@ IPA.entity_factories.delegation = function() {
                         field(IPA.text_widget({name: 'aciname', undo: false})).
                         field(IPA.entity_select_widget({name: 'group',
                             entity: 'group', undo: false})).
-                        field(IPA.entity_select_widget({name: 'memberof', entity: 'group',
+                        field(IPA.entity_select_widget(
+                            {name: 'memberof', entity: 'group',
                             join: true, undo: false})).
-                        field(IPA.attributes_widget({name: 'attrs', object_type: 'user',
+                        field(IPA.attributes_widget(
+                            {name: 'attrs', object_type: 'user',
                             join: true, undo: false})))).
         facet(
             IPA.details_facet().
@@ -672,11 +663,13 @@ IPA.entity_factories.delegation = function() {
                             {name:'memberof', label: 'Member Group',
                              entity:'group', join: true})).
                         custom_input(
-                            IPA.rights_widget({name: 'permissions', label: 'Permissions',
+                            IPA.rights_widget(
+                                {name: 'permissions', label: 'Permissions',
                                 direction: 'horizontal', join: true})).
                         custom_input(
                             IPA.attributes_widget({
-                                name:'attrs', object_type:'user', join: true})))).
+                                name:'attrs', object_type:'user', 
+                                join: true})))).
         standard_associations();
     return that;
 
diff --git a/install/ui/dialog.js b/install/ui/dialog.js
index b1f84a98bbd11a827415664d1357cdf90cb90fb3..f8eaf21215500cf83d35a39d558a380412dfdf53 100644
--- a/install/ui/dialog.js
+++ b/install/ui/dialog.js
@@ -234,6 +234,9 @@ IPA.dialog = function(spec) {
             var field = that.fields[i];
             field.reset();
         }
+        for (var j=0; j<that.sections.length; j++) {
+            that.sections[j].reset();
+        }
     };
 
     that.dialog_init = that.init;
diff --git a/install/ui/test/aci_tests.js b/install/ui/test/aci_tests.js
index d1a5cb6bfa586a58c2e2cea609fd7169e9f7b6e5..ff8b5de04fdef6582f2a5ad02f4d709cadaddc54 100644
--- a/install/ui/test/aci_tests.js
+++ b/install/ui/test/aci_tests.js
@@ -130,33 +130,54 @@ test("IPA.rights_widget.", function() {
 test("Testing aci grouptarget.", function() {
     var sample_data_filter_only = {"targetgroup":"ipausers"};
     target_section.load(sample_data_filter_only);
-    ok($('#aci_by_group')[0].checked, 'aci_by_group control selected');
-    ok ($('#targetgroup-entity-select option').length > 2,'group select populated');
+
+    var selected = $(target_section.type_select+":selected");
+
+    same(selected.val(), 'targetgroup' , 'group control selected');
+    ok ($('#targetgroup-entity-select option').length > 2,
+        'group select populated');
 
 });
 
-
-
-test("Testing aci object type.", function() {
+test("Testing type target.", function() {
     var sample_data_filter_only = {"type":"hostgroup"};
+
     target_section.load(sample_data_filter_only);
-    ok($('.aci-attribute', target_container).length > 4);
-    ok($('#aci_by_type')[0].checked, 'aci_by_type control selected');
+    var selected = $(target_section.type_select+":selected");
+    same(selected.val(), 'type', 'type selected');
+
+    $("input[type=checkbox]").attr("checked",true);
+    var response_record = {};
+    target_section.save(response_record);
+    same(response_record.type, sample_data_filter_only.type,
+         "saved type matches sample data");
+    ok((response_record.attrs.length > 10),
+       "response length shows some attrs set");
 
 });
 
 
-test("Testing aci filter only.", function() {
+test("Testing filter target.", function() {
 
     var sample_data_filter_only = {"filter":"somevalue"};
 
     target_section.load(sample_data_filter_only);
 
-    var filter_radio = $('#aci_by_filter');
+    var selected = $(target_section.type_select+":selected");
+    same(selected.val(), 'filter', 'filter selected');
+});
+
+
+
+test("Testing subtree target.", function() {
 
-    ok(filter_radio.length,'find "filter_only_radio" control');
-    ok(filter_radio[0].checked,'filter_only_radio control is checked');
+    var sample_data = {
+        subtree:"ldap:///cn=*,cn=roles,cn=accounts,dc=example,dc=co"};
 
+    target_section.load(sample_data);
+    var record = {};
+    target_section.save(record);
+    same(record.subtree, sample_data.subtree, 'subtree set correctly');
 });
 
 
diff --git a/install/ui/widget.js b/install/ui/widget.js
index eb2f70cfc0407d36fa6dd04ef7a4e9acb2f82853..dad35ecfa55d390ad6d5bd501e85f7cfe069b16e 100644
--- a/install/ui/widget.js
+++ b/install/ui/widget.js
@@ -59,6 +59,7 @@ IPA.widget = function(spec) {
     that.validate_input = spec.validate_input || validate_input;
     that.valid = true;
     that.param_info = spec.param_info;
+    that.values = [];
 
     that.__defineGetter__("entity_name", function(){
         return that._entity_name;
@@ -115,7 +116,7 @@ IPA.widget = function(spec) {
     function create(container) {
     }
 
-    function setup(container) {
+    function setup(container){
         that.container = container;
     }
 
@@ -875,9 +876,6 @@ IPA.select_widget = function(spec) {
             container.append(' ');
             that.create_undo(container);
         }
-    };
-
-    that.setup = function(container) {
 
         that.widget_setup(container);
 
-- 
1.7.3.5

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

Reply via email to