[Freeipa-devel] [PATCH] admiyo-0217-define-entities-using-builder-and-more-declarative

[Adam Young]

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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0219-code-review-fixes.patch
Type: text/x-patch
Size: 13675 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110331/9b77831f/attachment.bin>