Edwin-- Thanks for all the comments; I'll make those changes and push them up soon. On Aug 18, 2010, at 9:48 PM, Edwin Grubbs wrote: > Review: Needs Fixing > 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) >> + > -- > https://code.launchpad.net/~jcsackett/launchpad/plus-participation-additional-fixes/+merge/32820 > You are the owner of lp:~jcsackett/launchpad/plus-participation-additional-fixes.