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

Hi JC,

This looks good. BTW, it is customary to add a comment with the incremental diff of changes. This way the reviewer gets all the changes they need to look at in an email. I've included them below in case anybody else is following the progress on this.

-Edwin

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2010-08-19 14:52:19 +0000
+++ lib/lp/registry/browser/person.py 2010-08-19 17:39:30 +0000
@@ -3101,7 +3101,7 @@
             # Membership is via an indirect team so sane defaults exist.
             # An indirect member cannot be an Owner or Admin of a team.
             role = 'Member'
- # The Person joined, and can't have a join date.
+ # The Person never joined, and can't have a join date.
             datejoined = None
         else:
             # The member is a direct member; use the membership data.
@@ -3133,8 +3133,8 @@
         paths, memberships = self.context.getPathsToTeams()
         direct_teams = [membership.team for membership in memberships]
         indirect_teams = [
- team for team in paths.keys() if
- team not in direct_teams]
+ team for team in paths.keys()
+ if team not in direct_teams]
         participations = []

         # First, create a participation for all direct memberships.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2010-08-19 17:29:38 +0000
+++ lib/lp/registry/model/person.py 2010-08-19 17:39:30 +0000
@@ -1712,7 +1712,6 @@
         store = Store.of(self)
         return store.find(TeamMembership,
             And(TeamMembership.personID == self.id,
- Team.id == TeamMembership.teamID,
                 TeamMembership.status.is_in(
                 [TeamMembershipStatus.APPROVED, TeamMembershipStatus.ADMIN])))

@@ -2080,7 +2079,7 @@

         # Get all of the memberships for any of the teams this person is
         # a participant of. This must be ordered by date and id because
- # because the graph the results will create needs to contain
+ # because the graph of the results will create needs to contain
         # the oldest path information to be consistent with results from
         # IPerson.findPathToTeam.
         store = Store.of(self)
@@ -2114,7 +2113,7 @@
                 step = graph[step]
                 path.append(step)
             paths[team] = path
- return (paths, user_memberships)
+ return (paths, user_memberships)

     @property
     def teams_participated_in(self):

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2010-08-19 14:48:21 +0000
+++ lib/lp/registry/tests/test_person.py 2010-08-19 17:39:30 +0000
@@ -75,7 +75,7 @@
         memberships = [(m.person, m.team) for m in memberships]
         self.assertEqual([(self.user, self.a_team)], memberships)

- def test_path_to_team_no_limit(self):
+ def test_path_to_team(self):
         path_to_a = self.user.findPathToTeam(self.a_team)
         path_to_b = self.user.findPathToTeam(self.b_team)
         path_to_c = self.user.findPathToTeam(self.c_team)
@@ -84,15 +84,6 @@
         self.assertEqual([self.a_team, self.b_team], path_to_b)
         self.assertEqual([self.a_team, self.b_team, self.c_team], path_to_c)

- def test_path_to_team_with_limit(self):
- path_to_c_0 = self.user.findPathToTeam(self.c_team, limit=0)
- path_to_c_1 = self.user.findPathToTeam(self.c_team, limit=1)
- path_to_c_2 = self.user.findPathToTeam(self.c_team, limit=2)
-
- self.assertEqual([self.c_team], path_to_c_0)
- self.assertEqual([self.b_team, self.c_team], path_to_c_1)
- self.assertEqual([self.a_team, self.b_team, self.c_team], path_to_c_2)
-
     def test_teams_participated_in(self):
         teams = self.user.teams_participated_in
         teams = sorted(list(teams), key=lambda x: x.displayname)
@@ -133,9 +124,9 @@
         self.assertEqual(expected_paths, paths)

         expected_memberships = [
+ (e_team, self.user),
+ (d_team, self.user),
             (self.a_team, self.user),
- (d_team, self.user),
- (e_team, self.user),
             ]
         memberships = [
             (membership.team, membership.person) for membership