On 03/31/2011 04:29 PM, Adam Young wrote:
On 03/31/2011 03:27 PM, Adam Young wrote:
On 03/31/2011 03:12 PM, Adam Young wrote:
On 03/31/2011 02:09 PM, Adam Young wrote:
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.
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Including Automount
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Hadn't merged in the changes. Updated patch attached.
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
This version uses a spec for the details facet, IAW code review feedback
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
ACKed after lengthy discussion and minor fix up in IRC
Patches 217 and 219 pushed to master
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel