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.

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

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

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

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

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.

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.

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.

9. The IPA.entity_builder could be a singleton because it doesn't take
   any parameters and there's no multi-threading issue.

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.

--
Endi S. Dewata

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

Reply via email to