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