On 09/16/2010 06:53 AM, Pavel Zuna wrote:
On 09/15/2010 03:41 PM, Adam Young wrote:
On 09/15/2010 08:46 AM, Pavel Zůna wrote:
Re-based version of the patch attached, that should apply on the
current master. It doesn't have the Javascript library files (BBQ,
jQ-UI). This makes the patch a lot smaller and easier to process.

I'm going to post another patch that does nothing, but adds the
library files.

Pavel


Things to add to the to do list before this can go in:

Netgroups is missing most of their associations
Fixed.

The menu leading to associations was only generated using the 'memberof' attribute. Now it uses all attributes in LDAPObject.attribute_members.


Ha: You probably just made things both easier and harder in a single stroke. Easier for implementing new associations, but harder in that many of the existing associations had strange rules. We also have several associations that we hadn't implemented yes, like the association between users and roles, as we don't have roles. We'll need a way to hide stuff until we work out the kinks. Endi, take note.



netgroup_show.json has unmerged changes. We should revert to the version
in the top of tree
Fixed.

netgroup_show.json changes got in by mistake.

You have removed the author from several files and replaced it with only
your own.

Only in two files were it made sense:
1) add.js, because I had to rewrite it from scratch completely.
2) details.js, because I started on a reverted version, that didn't have your changes (and name) yet. The changes I'm talking about were the addition of DetailsForm function, about 20 lines of code (out of witch 10 were copy pasted from somewhere else).

I added you as the author on search.js even though it's also pretty much a complete rewrite, but I felt that it was based on your ideas. I also didn't touch authorship info on associations even though I made more or less
significant changes to it.

I just acted naturally without thinking about it when adding the file headers. If you think it's unfair and I forgot to mention you or Endi somewhere, I'm sorry and we can fix it no prob.

No problem.

You should feel comfortable adding your name as an author to associations. You've made significant contributions there.

Details is pretty much your work, so I feel comfortable letting you take complete bla^H^H^Hownership. I suspect that if we look really closely at add.js, we can incorperate it into details, as it really should be a minor variation in functionality. Long term, it would be a logical pattern to have add and edit for things in the same file. We already do that with associations.

The ones that were a refactoring of our work should still contain us as authors. I think that is goingto be a fairly common pattern moving forward: as we get better and better understand of stuff, we should feel comfortable simplifying the implementations. Most refactorings can be automated (at least in som languages) and don't constitute a major change in the conceptual underpinnings. Just like when the the US version of Harry POtter comes out, and they CHange "Jumper" to "Sweater" and "Garden" to "Back-yard." I don't want to make anyone wary of making necessary changes due to authorship issues. Once someone's name goes on a file as author, we should leave it there unless there are legal reasons to remove it.


You have a good point that you make in the comments about the use of stuff from seearch.js inside of associations. PLaces where we come up with our own UI patterns should get pulled out into JQuery plugins, especially in the case where they get reused. The file search.js is really the union of two distinct things: a business object for querying the server for lists of things based on a filter, and a results table. Moving forward, we might want to split it along these fault lines, and then the logic for binding them together goes into the webui.js file.








I posted the diff:


https://fedorahosted.org/freeipa/attachment/ticket/41/pzuna-freeipa-0022-3-BIG.patch

New Diff posted here https://fedorahosted.org/freeipa/attachment/ticket/41/pzuna-freeipa-0022-4-BIG.patch


Pavel

Going through the new patch now. I found one more issue since this review: You need to add new files to Makefile.am in install/static. Specifially, entity.js is not there.

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

Reply via email to