<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> <html> <head> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#ffffff"> On 03/31/2011 03:27 PM, Adam Young wrote: <blockquote cite="mid:4D94D592.5020608@redhat.com" type="cite"> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> On 03/31/2011 03:12 PM, Adam Young wrote: <blockquote cite="mid:4D94D225.50606@redhat.com" type="cite"> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> On 03/31/2011 02:09 PM, Adam Young wrote: <blockquote cite="mid:4D94C348.9010700@redhat.com" type="cite">On 03/31/2011 12:51 PM, Endi Sukma Dewata wrote: <blockquote type="cite">On 3/31/2011 8:43 AM, Adam Young wrote: <blockquote type="cite">rebased both patches </blockquote> 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. </blockquote> Good to know. <blockquote type="cite"> 2. There's a IPA.metadata assignment and the like in unit tests, this is redundant. </blockquote> yeah, I suspect it was from before I explicitly set async. removed. <blockquote type="cite"> 3. In IPA.entity_builder.section(), the current_section should be added to the current_facet before adding the fields to do incremental construction. </blockquote> Done. <blockquote type="cite"> 4. In IPA.entity_builder the entity_name can be replaced with entity.name to reduce the number of variables. </blockquote> Done <blockquote type="cite"> 5. In IPA.entity_builder the standard_associations() can be replaced standard_association_facets() for consistency. </blockquote> Done <blockquote type="cite"> 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. </blockquote> 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. <blockquote type="cite"> 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. </blockquote> 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. <blockquote type="cite"> 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. </blockquote> Done. <blockquote type="cite"> 9. The IPA.entity_builder could be a singleton because it doesn't take any parameters and there's no multi-threading issue. </blockquote> 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. <blockquote type="cite"> 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. </blockquote> 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. <pre wrap=""><fieldset class="mimeAttachmentHeader"></fieldset> _______________________________________________ Freeipa-devel mailing list <a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a> <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre> </blockquote> Including Automount <pre wrap=""><fieldset class="mimeAttachmentHeader"></fieldset> _______________________________________________ Freeipa-devel mailing list <a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a> <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre> </blockquote> Hadn't merged in the changes. Updated patch attached. <pre wrap=""> <fieldset class="mimeAttachmentHeader"></fieldset> _______________________________________________ Freeipa-devel mailing list <a class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a> <a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre> </blockquote> This version uses a spec for the details facet, IAW code review feedback </body> </html>