Hi JC, The main thing I learned from reviewing this branch is that we should not give new employees programming tasks concerning graph traversal unless we want to scare them away. I think that is the most confusing code I've ever seen in the registry. I am impressed with all that you have accomplished in this branch. Most of my comments are related to formatting, which is to be expected with new employees. I felt your pain. You really need to run "make lint" against the branch, especially test_person.py. -Edwin >=== modified file 'lib/lp/registry/browser/person.py' >--- lib/lp/registry/browser/person.py 2010-08-18 03:12:35 +0000 >+++ lib/lp/registry/browser/person.py 2010-08-18 21:23:34 +0000 >@@ -3078,31 +3078,34 @@ > def label(self): > return 'Team participation for ' + self.context.displayname > >- def _asParticipation(self, membership=None, team=None, team_path=None): >+ def _asParticipation(self, membership=None, team=None, via=None): > """Return a dict of participation information for the membership. > > Method requires membership or team, not both. >+ :param via: The team through which the membership in the indirect >+ is established. "in the indirect" seems to be missing a noun. > """ > if ((membership is None and team is None) or > (membership is not None and team is not None)): > raise AssertionError( > "The membership or team argument must be provided, not both.") > >- if team_path is None: >- via = None >- else: >- via = COMMASPACE.join(team.displayname for team in team_path) >+ if via is not None: >+ # When showing the path, it's unnecessary to show the team in >+ # question at the beginning of the path, or the user at the >+ # end of the path. >+ via = COMMASPACE.join( >+ [via_team.displayname for via_team in via[1:-1]]) > > if membership is None: >- #we have membership via an indirect team, and can use >- #sane defaults >+ # Membership is via an indirect team; sane defaults exist. > >- #the user can only be a member of this team >+ # The user can only be a member of this team. The space between these two comments makes it look like a line of code disappeared. "sane defaults exist" kinda confuses me. I see what you are going for with a comment for the block and a separate comment for the first statement. It also took a second to understand what "can only be a member" meant. Maybe, the two comments can be combined as: # An indirect member cannot be the Owner or Admin of the team. > role = 'Member' >- #the user never joined, and can't have a join date >+ # The user never joined, and can't have a join date. "user" almost always refers to the logged-in user, so it is better to say "person" if the function is not dealing with the logged-in user. > datejoined = None > else: >- #the member is a direct member, so we can use membership data >+ # The member is a direct member; use the membership data. > team = membership.team > datejoined = membership.datejoined > if membership.person == team.teamowner: >@@ -3128,18 +3131,27 @@ > @cachedproperty > def active_participations(self): > """Return the participation information for active memberships.""" >- participations = [self._asParticipation(membership=membership) >- for membership in self.context.myactivememberships >- if check_permission('launchpad.View', membership.team)] >- membership_set = getUtility(ITeamMembershipSet) >- for team in self.context.teams_indirectly_participated_in: >- if not check_permission('launchpad.View', team): >+ 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] The style guide doesn't explicitely mention this, but I think the formatting choices should be similar to a function call. Each line of the list comprehension should line up so they are easier to read: Either: result = [foo bar baz] Or: result = [ foo bar baz] For actual lists, the ending square bracket has to be on a line by itself. There isn't an exact rule for list comprehensions, but pocketlint will complain if you put the square bracket on a line by itself without a comma on the previous line, and of course, that only works for lists. >+ participations = [] >+ >+ # First, create participation for all direct memberships. s/create participation/create a participation/ >+ for membership in memberships: >+ # Add a participation record for the membership if allowed. >+ if check_permission('launchpad.View', membership.team): >+ participations.append(self._asParticipation( >+ membership=membership)) >+ >+ # Second, create participation for all indirect memberships, >+ # using the remaining paths. Here, too. >+ for indirect_team in indirect_teams: >+ if not check_permission('launchpad.View', indirect_team): > continue >- # The key points of the path for presentation are: >- # [-?] indirect memberships, [-2] direct membership, [-1] team. >- team_path = self.context.findPathToTeam(team) > participations.append( >- self._asParticipation(team_path=team_path[:-1], team=team)) >+ self._asParticipation(via=paths[indirect_team], >+ team=indirect_team)) > return sorted(participations, key=itemgetter('displayname')) > > @cachedproperty > >=== modified file 'lib/lp/registry/interfaces/person.py' >--- lib/lp/registry/interfaces/person.py 2010-08-17 13:58:57 +0000 >+++ lib/lp/registry/interfaces/person.py 2010-08-18 21:23:34 +0000 >@@ -927,7 +925,7 @@ > # @operation_parameters(team=copy_field(ITeamMembership['team'])) > # @operation_returns_collection_of(Interface) # Really IPerson > # @export_read_operation() >- def findPathToTeam(team): >+ def findPathToTeam(team, limit): > """Return the teams that cause this person to be a participant of the > given team. I don't see the limit being used anywhere? Is that feature still needed? If it's needed the docstring should describe what limit is. Look in other docstrings, which use the convention: :param PARAMNAME: DESCRIPTION >Also, the person who originally wrote the docstring didn't follow pep8. >The first sentence should fit on a single line. > > >=== modified file 'lib/lp/registry/model/person.py' >--- lib/lp/registry/model/person.py 2010-08-18 15:52:30 +0000 >+++ lib/lp/registry/model/person.py 2010-08-18 21:23:34 +0000 >@@ -753,8 +753,15 @@ > packages.sort(key=lambda x: x.name) > return packages > >- def findPathToTeam(self, team): >- """See `IPerson`.""" >+ def findPathToTeam(self, team, limit=None): >+ """See `IPerson`. >+ >+ The limit parameter sets the depth of traversal along the path. A >+ limit of 0 returns the passed in team, 1 returns the team >+ providing membership to the passed in team. 2 returns the membership >+ for the team returned with limit 1, and so on. None returns the path >+ all the way to the team Person has direct membership in. >+ """ Oh, you can move this info into IPerson, assuming that you need to keep the limit param. > # This is our guarantee that _getDirectMemberIParticipateIn() will > # never return None > assert self.hasParticipationEntryFor(team), ( >@@ -1687,19 +1699,16 @@ > self.invited_members, > orderBy=self._sortingColumnsForSetOperations) > >- # XXX: kiko 2005-10-07: >- # myactivememberships should be renamed to team_memberships and be >- # described as the set of memberships for the object's teams. > @property >- def myactivememberships(self): >+ def team_memberships(self): > """See `IPerson`.""" >- return TeamMembership.select(""" >- TeamMembership.person = %s AND status in %s AND >- Person.id = TeamMembership.team >- """ % sqlvalues(self.id, [TeamMembershipStatus.APPROVED, >- TeamMembershipStatus.ADMIN]), >- clauseTables=['Person'], >- orderBy=Person.sortingColumns) >+ Team = ClassAlias(Person, "Team") >+ 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]))) I know I looked at this code before with you, but I don't understand why you are joining with the Team here, since you are not actually pulling the Team info out with the TeamMembership. If that is what you are trying to do, then you need something like this: result = store.find((TeamMembership, Team), And(...)) If you don't want team_memberships to return tuples with both items, you do this: def decorate(row): membership, team = row return membership return DecoratedResultSet(result, decorate) > def _getMappedParticipantsLocations(self, limit=None): > """See `IPersonViewRestricted`.""" >@@ -2052,10 +2061,46 @@ > > def getLatestApprovedMembershipsForPerson(self, limit=5): > """See `IPerson`.""" >- result = self.myactivememberships >- result = result.orderBy(['-date_joined', '-id']) >+ result = self.team_memberships >+ result = result.order_by(Desc(TeamMembership.datejoined), >+ Desc(TeamMembership.id)) The style guide dictates arguments on multiple lines as either: result = result.order_by(one, two) Or: result = result.order_by( one, two) Or: result = result.order_by( one, two) > > return result[:limit] > >+ def getPathsToTeams(self): >+ """See `Iperson`.""" >+ # Get all of the teams this person participates in. >+ teams = list(self.teams_participated_in) >+ all_user_memberships = [(self, team) for team in teams] >+ # Get all of the memberships for any of the teams this person is >+ # a participant of. >+ store = Store.of(self) >+ all_direct_memberships = store.find(TeamMembership, >+ TeamMembership.personID.is_in( >+ [team.id for team in teams] + [self.id])) >+ # Pull out the memberships directly used by this person. >+ user_memberships = [membership for membership in >+ all_direct_memberships if membership.person == self] Reformat list comprehension. >+ all_direct_memberships = [(membership.team, membership.person) for >+ membership in all_direct_memberships.order_by( >+ Desc(TeamMembership.datejoined), Desc(TeamMembership.id))] Every time you iterate over a storm result set, it executes the query. To eliminate a second query, you could convert all_direct_memberships to a list. Since you will have no problem creating user_memberships with a sorted all_direct_memberships, you can just move the order_by up before you iterate over it and before you convert it to a list. >+ >+ # Create a graph from the edges provided by the other data sets Comments must end in a period to indicate that the end didn't get deleted accidentally. I think you should also add a comment that the Desc(date) ordering is necessary, so that the dictionary contains the "oldest teams" in the graph as mentioned in the IPerson.findPathToTeam() docstring, since later entries in the list with duplicate keys will overwrite previous items in the list. >+ graph = dict(all_direct_memberships) >+ graph.update(all_user_memberships) There is no point adding all_user_memberships to the graph, since you only look up an item in the graph if it is set to the step, and as soon as the step is set to self, you break out of the loop. Perhaps, you had intended that all_user_memberships contain (team, self) tuples instead of (self, team) tuples, but that isn't necessary, since you have added self.id to your query for all_direct_memberships. You don't even need to create the variable all_user_memberships. >+ # Build the teams paths from that graph. >+ paths = {} >+ for team in teams: >+ path = [team] >+ step = team >+ while path[-1] != self: >+ step = graph[step] >+ path.append(step) >+ paths[team] = path >+ return paths, user_memberships The extra space looks odd. If you want you can use parens to make the separation more prominent. > @property > def teams_participated_in(self): > """See `IPerson`.""" > >=== modified file 'lib/lp/registry/tests/test_person.py' >--- lib/lp/registry/tests/test_person.py 2010-08-17 13:53:04 +0000 >+++ lib/lp/registry/tests/test_person.py 2010-08-18 21:23:34 +0000 >+ def test_getPathsToTeamsCycle(self): Using "Cycle" in the method name is misleading, since team memberships are not allowed to form a cycle, i.e. Team A can't participate in another team that participates in Team A. You could say "MultiplePaths" instead. You should also comment in this test that it always chooses the oldest path, so the result is not random. >+ d_team = self.factory.makeTeam(name='d', owner=self.b_team) >+ login_person(self.a_team.teamowner) >+ self.c_team.addMember(d_team, self.c_team.teamowner) >+ >+ paths = self.user.getPathsToTeams() For all the tests for getPathsToTeams(), it would be clearer to assign the results as paths, memberships = self.user.getPathsToTeams() like you do in the browser code. >+ expected_paths = { >+ self.a_team:[self.a_team, self.user], >+ self.b_team:[self.b_team, self.a_team, self.user], >+ self.c_team:[self.c_team, self.b_team, self.a_team, self.user], >+ d_team:[d_team, self.b_team, self.a_team, self.user]} >+ self.assertEqual(expected_paths, paths[0]) >+ >+ expected_memberships = [(self.a_team, self.user)] >+ memberships = [(membership.team, membership.person) for membership >+ in paths[1]] Reformat list comprehension. >+ self.assertEqual(expected_memberships, memberships) >+