Code review comment for lp:~jcsackett/launchpad/plus-participation-additional-fixes

Hi Jonathan,

I reviewed the branch and both the test and implementation of the changes look good. However, I did come upon one large question: did the IPerson.findPathToTeam() interface definition change with this patch, or does this patch only change the Person class itself? If so, then the IPerson interface class will have to be updated. If you want to verify this, I suggest writing a test in test_person.py that calls verifyObject() on a Person instance - that will immediately reveal any interface errors.

Besides that, I only found minor points in the diff:

 • Does the new 'via' argument to _asParticipation() need a epydoc stanza in the method's docstring? Otherwise the argument type is not obvious.

 • Should there be a comment on the line 53 of the diff, before calling findPathToTeam(limit=1), to make the purpose of the limit argument clear? Without that 'limit=1' appears to be a magic number.

 • It looks like the 'from storm.expr' import in model/person.py was ordered alphabetically, but Desc isn't in order.

 • You probably want to use an epydoc :param: stanza for the new limit argument in the IPerson.findPathToTeam() docstring.

I am marking this as Needs Information for now. When the IPerson interface questions are answered then we can approve this to land.

Maris