On 03/31/2011 12:51 PM, Endi Sukma Dewata wrote:
On 3/31/2011 8:43 AM, Adam Young wrote:
rebased both patches

I don't see any code change in the rebased patches, only new commit ID's, I hope this is correct.

Some comments (some of which had been discussed over IRC):

1. I ran my Selenium test cases against 215, 216, and 217 together,
   so far there's no failure.
Good to know.


2. There's a IPA.metadata assignment and the like in unit tests, this
   is redundant.

yeah, I suspect it was from before I explicitly set async.  removed.


3. In IPA.entity_builder.section(), the current_section should be added
   to the current_facet before adding the fields to do incremental
   construction.
Done.


4. In IPA.entity_builder the entity_name can be replaced with
   entity.name to reduce the number of variables.

Done

5. In IPA.entity_builder the standard_associations() can be replaced
   standard_association_facets() for consistency.
Done

6. In the permission entity definition, the 'add_fields' is used
   inconsistently to add a section (i.e. IPA.target_section). The
   solution is either adding 'add_sections' or converting
   IPA.target_sections into widgets. I think adding 'add_sections' is
   simpler because widgets is designed to represent a single attribute.

Gonna punt on this for this patch. Not certain on the correct approach, either to make adders have sections, or to convert the one custom section we have to a widget. Either way, beyond the scope of this patch, and it will only affect one entity and the builder when we decide.


7. The IPA.entity_builder.details_facet() takes an array of sections
   instead of a spec object. This limits the expandability of the
   builder interface. It should take a spec object with a 'sections'
   attribute containing the array of sections, this would be consistent
   with the other interfaces.

I thought about it, but there is nothing that we want to customize in the details_facet. We can always do a custom facet to customize. An alternative is to check the type of that is passed in to the details_section method on the builder, check if it is an object or an array, and treat it like a spec or array depending.


8. In IPA.entity_builder.search_facet(), there's no need to call
   current_facet.init() because all facets will be initialized by
   the entity when IPA.start_entities() is invoked.

Done.


9. The IPA.entity_builder could be a singleton because it doesn't take
   any parameters and there's no multi-threading issue.
Going to leave it like this for now. That change would be limited to a single file (entity.js) and can be done if we decide we want to with a very small patch.



10. In IPA.details_refresh() it calls IPA.refresh_devel_hook()
    to execute a code specifically used by testing. I think ideally
    the develop.js should modify the entity_builder instance to generate
    details facet with test-specific code. This requires item #9. But
    for now at least we should rename IPA.refresh_devel_hook() into
    IPA.details_refresh_devel_hook() for clarity.

Good idea.  Done


I had already submitted patch freeipa-admiyo-0218-default-all-false. Some of the fixes here would conflict with that patch if rebased. What I've attached is on top of 217-2 and 218-1.


From 849e9ea5afaf79fef8d438eb18d788239bda9207 Mon Sep 17 00:00:00 2001
From: Adam Young <ayo...@redhat.com>
Date: Thu, 31 Mar 2011 13:57:35 -0400
Subject: [PATCH] code review fixes

---
 install/ui/aci.js                |    8 ++++----
 install/ui/details.js            |    4 ++--
 install/ui/develop.js            |    2 +-
 install/ui/dns.js                |    2 +-
 install/ui/entity.js             |   32 ++++++++++----------------------
 install/ui/entitylist            |   15 +++++++++++++++
 install/ui/group.js              |    2 +-
 install/ui/host.js               |    2 +-
 install/ui/hostgroup.js          |    2 +-
 install/ui/netgroup.js           |    2 +-
 install/ui/policy.js             |    2 +-
 install/ui/service.js            |    3 ++-
 install/ui/test/details_tests.js |    5 -----
 install/ui/test/entity_tests.js  |    5 -----
 install/ui/user.js               |    2 +-
 15 files changed, 41 insertions(+), 47 deletions(-)
 create mode 100644 install/ui/entitylist

diff --git a/install/ui/aci.js b/install/ui/aci.js
index d51741245f4305a1bdc87acb5fc639b30a5fd23e..7d669fc9155584849ae74f5d0a9d4ed336779509 100644
--- a/install/ui/aci.js
+++ b/install/ui/aci.js
@@ -60,7 +60,7 @@ IPA.entity_factories.permission = function() {
                 factory:IPA.target_section,
                 label: IPA.messages.objects.permission.target
             }]).
-        standard_associations().
+        standard_association_facets().
         build();
 };
 
@@ -88,7 +88,7 @@ IPA.entity_factories.privilege = function() {
                 add_method: 'add_permission',
                 remove_method: 'remove_permission'
         }).
-        standard_associations().
+        standard_association_facets().
         build();
 
 };
@@ -110,7 +110,7 @@ IPA.entity_factories.role = function() {
                 add_method: 'add_privilege',
                 remove_method: 'remove_privilege'
         }).
-        standard_associations().
+        standard_association_facets().
         build();
 };
 
@@ -183,7 +183,7 @@ IPA.entity_factories.delegation = function() {
                         name: 'attrs', object_type: 'user',
                         join: true
                     }]}]).
-        standard_associations().
+        standard_association_facets().
         build();
 };
 
diff --git a/install/ui/details.js b/install/ui/details.js
index 1dccb830e240fd7adda2ebc5dd216da4deba617b..40dd6d4f8389531c7caeec98a8d613fb63eb9506 100644
--- a/install/ui/details.js
+++ b/install/ui/details.js
@@ -579,8 +579,8 @@ IPA.details_refresh = function() {
         options: { 'all': true, 'rights': true }
     });
     
-    if (IPA.refresh_devel_hook){
-        IPA.refresh_devel_hook(that.entity_name,command,that.pkey);
+    if (IPA.details_refresh_devel_hook){
+        IPA.details_refresh_devel_hook(that.entity_name,command,that.pkey);
     }
 
 
diff --git a/install/ui/develop.js b/install/ui/develop.js
index 5095905d7d5b5d3a420e1e0b007a69fc633ade93..9f0c8dd5ac4f60fd1fb43f3f7245702fb0b275fb 100644
--- a/install/ui/develop.js
+++ b/install/ui/develop.js
@@ -4,7 +4,7 @@ if (window.location.protocol == 'file:') {
     IPA.json_url = "test/data";
     IPA.use_static_files = true;
 
-    IPA.refresh_devel_hook = function(entity_name,command,pkey){
+    IPA.details_refresh_devel_hook = function(entity_name,command,pkey){
         if ((entity_name === 'host')||(entity_name === 'permission')){
             command.name =   entity_name+'_show_'+pkey;
             command.method = entity_name+'_show';
diff --git a/install/ui/dns.js b/install/ui/dns.js
index e1fce532edb9df73d85b51624b7658c414d0827a..e89a677ec32c349a67462c0b493896d5ad04b25a 100644
--- a/install/ui/dns.js
+++ b/install/ui/dns.js
@@ -52,7 +52,7 @@ IPA.entity_factories.dnszone = function() {
             'name': 'records',
             'label': IPA.metadata.objects.dnsrecord.label
         })).
-        standard_associations().
+        standard_association_facets().
         build();
 };
 
diff --git a/install/ui/entity.js b/install/ui/entity.js
index 31be28600e17b1e231ecebe1a9c547c17d51e56f..8121ee070791b5497cb65022f8f9b9633988daa6 100644
--- a/install/ui/entity.js
+++ b/install/ui/entity.js
@@ -530,20 +530,17 @@ IPA. facet_create_action_panel = function(container) {
 IPA.entity_builder = function(){
 
     var that = {};
-
-    var entity_name ;
     var entity = null;
     var current_facet = null;
 
-
     function section(spec){
         var current_section = null;
-        spec.entity_name = entity_name;
+        spec.entity_name = entity.name;
 
         if (spec.section){
             spec.name = spec.section;
             if (!spec.label){
-                var obj_messages = IPA.messages.objects[entity_name];
+                var obj_messages = IPA.messages.objects[entity.name];
                 spec.label =  obj_messages[spec.section];
             }
         }
@@ -553,7 +550,7 @@ IPA.entity_builder = function(){
         }else{
             current_section = IPA.details_list_section(spec);
         }
-
+        current_facet.add_section(current_section);
         var fields = spec.fields;
         if (fields){
             var i;
@@ -561,22 +558,20 @@ IPA.entity_builder = function(){
             for (i =0; i < fields.length; i += 1){
                 field =  fields[i];
                 if (field instanceof Object){
-                    field.entity_name = entity_name;
+                    field.entity_name = entity.name;
                     current_section.add_field(field.factory(field));
                 }else{
                     field = IPA.text_widget({
                         name:field,
-                        entity_name:entity_name
+                        entity_name:entity.name
                     });
                     current_section.add_field(field);
                 }
             }
         }
-        current_facet.add_section(current_section);
     }
 
     that.entity = function(name){
-        entity_name = name;
         that.entity_name = name;
         entity = IPA.entity({name: name});
         return that;
@@ -588,7 +583,7 @@ IPA.entity_builder = function(){
     };
 
     that.details_facet = function (sections){
-        current_facet =IPA.details_facet({entity_name:entity_name});
+        current_facet =IPA.details_facet({entity_name:entity.name});
         entity.facet(current_facet);
 
         var i;
@@ -599,10 +594,6 @@ IPA.entity_builder = function(){
         return that;
     };
 
-    that.get_current_facet = function(){
-        return current_facet;
-    };
-
     that.facet = function (facet){
         current_facet = facet;
         entity.facet(facet);
@@ -614,8 +605,6 @@ IPA.entity_builder = function(){
             entity_name:that.entity_name,
             search_all: spec.search_all || false
         });
-        //once everything usese this mechanism, inline the init code
-        current_facet.init();
 
         var columns = spec.columns;
         var i;
@@ -630,7 +619,7 @@ IPA.entity_builder = function(){
             IPA.add_dialog({
                 'name': 'add',
                 'title': IPA.messages.objects.user.add,
-                entity_name: entity_name
+                entity_name: entity.name
             });
 
         current_facet.dialog(current_dialog);
@@ -653,7 +642,7 @@ IPA.entity_builder = function(){
                     field.section = null;
                     current_dialog.add_section(factory(field));
                 }else{
-                    field.entity_name = entity_name;
+                    field.entity_name = entity.name;
                     factory = field.factory;
                     current_dialog.field(factory(field));
                 }
@@ -661,19 +650,18 @@ IPA.entity_builder = function(){
                 current_dialog.text(add_fields[i]);
             }
         }
-
         entity.facet(current_facet);
         return that;
     };
 
 
     that.association_facet = function(spec){
-        spec.entity_name = entity_name;
+        spec.entity_name = entity.name;
         entity.facet(IPA.association_facet(spec));
         return that;
     };
 
-    that.standard_associations = function(){
+    that.standard_association_facets = function(){
         entity.standard_associations();
         return that;
     };
diff --git a/install/ui/entitylist b/install/ui/entitylist
new file mode 100644
index 0000000000000000000000000000000000000000..83de69aa86a30195f480a3255509437138976d5c
--- /dev/null
+++ b/install/ui/entitylist
@@ -0,0 +1,15 @@
+aci.js
+automount.js
+certificate.js
+dns.js
+group.js
+hbac.js
+hostgroup.js
+host.js
+netgroup.js
+policy.js
+rule.js
+serverconfig.js
+service.js
+sudo.js
+user.js
diff --git a/install/ui/group.js b/install/ui/group.js
index cc443cc53bffed5b49f2dc0d3da49b43bddd148b..b04808e3d28fc7e37baa7f08c11bfe00d7c21815 100644
--- a/install/ui/group.js
+++ b/install/ui/group.js
@@ -85,6 +85,6 @@ IPA.entity_factories.group =  function () {
             name: 'memberof_role',
             associator: IPA.serial_associator
         }).
-        standard_associations().
+        standard_association_facets().
         build();
 };
diff --git a/install/ui/host.js b/install/ui/host.js
index 3f9c05d1de849fd6686a51729784b7904d54f52a..43821d3e22a05ff1e5114b29c599d67644c27cec 100644
--- a/install/ui/host.js
+++ b/install/ui/host.js
@@ -85,7 +85,7 @@ IPA.entity_factories.host = function () {
             name: 'memberof_role',
             associator: IPA.serial_associator
         }).
-        standard_associations().
+        standard_association_facets().
         build();
 };
 
diff --git a/install/ui/hostgroup.js b/install/ui/hostgroup.js
index 46f2ab68c6dca8ca84e387570083bbf6eb6b5767..58c475c00a8bf1436fead9d785c0b952290e7bdd 100644
--- a/install/ui/hostgroup.js
+++ b/install/ui/hostgroup.js
@@ -38,7 +38,7 @@ IPA.entity_factories.hostgroup = function() {
             name: 'memberof_hostgroup',
             associator: IPA.serial_associator
         }).
-        standard_associations().
+        standard_association_facets().
         build();
 };
 
diff --git a/install/ui/netgroup.js b/install/ui/netgroup.js
index 9389d019704f871fbde2c9777c3d7fdca5bad8b0..993b52d4c1607e391ed356b47455dd23bffd5722 100644
--- a/install/ui/netgroup.js
+++ b/install/ui/netgroup.js
@@ -35,6 +35,6 @@ IPA.entity_factories.netgroup = function() {
             name: 'memberof_netgroup',
             associator: IPA.serial_associator
         }).
-        standard_associations().
+        standard_association_facets().
         build();
 };
diff --git a/install/ui/policy.js b/install/ui/policy.js
index ec202138c42e44a60546127bf38905e35c6fba2d..0ea50f136b6b2ed1de8d467a01182642fa85debe 100644
--- a/install/ui/policy.js
+++ b/install/ui/policy.js
@@ -36,7 +36,7 @@ IPA.entity_factories.pwpolicy = function() {
                 fields:['krbmaxpwdlife','krbminpwdlife','krbpwdhistorylength',
                         'krbpwdmindiffchars','krbpwdminlength']
             }]).
-        standard_associations().
+        standard_association_facets().
         build();
 };
 
diff --git a/install/ui/service.js b/install/ui/service.js
index 8590550aeef2e8454893ebd98c863448bb88556e..646c7796692ebcfe38c777f6de36bfeb098c08ae 100644
--- a/install/ui/service.js
+++ b/install/ui/service.js
@@ -74,7 +74,8 @@ IPA.entity_factories.service = function() {
                 add_method: 'add_host',
                 remove_method: 'remove_host'
         })).
-        standard_associations().build();
+        standard_association_facets().
+        build();
 };
 
 
diff --git a/install/ui/test/details_tests.js b/install/ui/test/details_tests.js
index 6cc8fd72d01a63a088095448db093faa3b299ff1..488ca84b2e9229e01d0b54b5a9e7842536245dcf 100644
--- a/install/ui/test/details_tests.js
+++ b/install/ui/test/details_tests.js
@@ -29,11 +29,6 @@ module('details', {
             "data",
             true,
             function(data, text_status, xhr) {
-                IPA.metadata = data.result.results[0];
-                IPA.messages = data.result.results[1].messages;
-                IPA.whoami  = data.result.results[2].result[0];
-                IPA.env = data.result.results[3].result;
-                IPA.dns_enabled = data.result.results[4].result;
             },
             function(xhr, text_status, error_thrown) {
                 ok(false, "ipa_init() failed: "+error_thrown);
diff --git a/install/ui/test/entity_tests.js b/install/ui/test/entity_tests.js
index 5f7ba9e7398b8ee0eb6f9ec944d1d7a92db06f54..86e44cd6741056c91eddb2cabc3274ffc44167ba 100644
--- a/install/ui/test/entity_tests.js
+++ b/install/ui/test/entity_tests.js
@@ -30,11 +30,6 @@ module('entity',{
             "data",
             true,
             function(data, text_status, xhr) {
-                IPA.metadata = data.result.results[0];
-                IPA.messages = data.result.results[1].messages;
-                IPA.whoami  = data.result.results[2].result[0];
-                IPA.env = data.result.results[3].result;
-                IPA.dns_enabled = data.result.results[4].result;
 
                 IPA.entity_factories.user = function(){
                     return IPA.
diff --git a/install/ui/user.js b/install/ui/user.js
index a922f9f89341419e68d3a03893de7865bd95d672..2bd393200dfc035a1991634478b4a5e2959f35eb 100644
--- a/install/ui/user.js
+++ b/install/ui/user.js
@@ -79,7 +79,7 @@ IPA.entity_factories.user = function() {
             name: 'memberof_role',
             associator: IPA.serial_associator
         }).
-        standard_associations();
+        standard_association_facets();
 
 
     var entity = builder.build();
-- 
1.7.4

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

Reply via email to