← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~jcsackett/launchpad/plus-participation-additional-fixes into lp:launchpad/devel

 

j.c.sackett has proposed merging lp:~jcsackett/launchpad/plus-participation-additional-fixes into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #607879 ~person/+participation timeouts
  https://bugs.launchpad.net/bugs/607879


Summary

Contributes to the +participation time outs by limiting the team_path lookups to just one level of traversal. Also stormifies the team_membership query, previously myactivememberships

Proposed fix

Stormifying where possible to assist db load, and limiting the team_path look ups for indirect teams, which proved to be a painful set of queries.

Pre-implementation notes

Chatted with Curtis Hovey (sinzui)

Implementation details

Renames myactivememberships to team_memberships, per an XXX from kiko in the codebase. Updates call sites.

Stormifies team_memberships and updates callsites.

The big deal is the change to findPathForTeam, which now accepts a limit argument to cut the depth of traversal; this is used to set the limit to one to just find the next team in the chain to the person for indirect_teams.

Demo and Q/A
bin/test -vvc -t TestPersonTeams
bin/test -vvc -t TestPersonParticipationView

On launchpad.dev, checkout https://launchpad.dev/~mark/+participation. The via column should only show "Membership via [team]" for indirect teams, rather than the "[team1], [team2]..." data previously shown.

lint
= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/templates/person-portlet-team-assignedbugs.pt
  lib/lp/registry/vocabularies.py
  lib/lp/registry/browser/person.py
  lib/lp/registry/browser/tests/test_person_view.py
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_person.py

./lib/lp/registry/vocabularies.py
     180: E302 expected 2 blank lines, found 1
     230: E302 expected 2 blank lines, found 1
     625: E302 expected 2 blank lines, found 1
    1495: E301 expected 1 blank line, found 0
./lib/lp/registry/interfaces/person.py
     448: E302 expected 2 blank lines, found 1

The "expected n blank lines" were explained to me as a factor of lint having issues around comments and class definitions.
-- 
https://code.launchpad.net/~jcsackett/launchpad/plus-participation-additional-fixes/+merge/32820
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/plus-participation-additional-fixes into lp:launchpad/devel.
=== modified file 'lib/lp/bugs/templates/person-portlet-team-assignedbugs.pt'
--- lib/lp/bugs/templates/person-portlet-team-assignedbugs.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/bugs/templates/person-portlet-team-assignedbugs.pt	2010-08-16 21:51:00 +0000
@@ -5,7 +5,7 @@
   omit-tag="">
 
 <div class="portlet" id="portlet-team-assigned-bugs"
-     tal:condition="context/myactivememberships">
+     tal:condition="context/team_memberships">
 
   <h2><span tal:replace="context/title" />'s teams</h2>
 

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-08-13 07:48:54 +0000
+++ lib/lp/registry/browser/person.py	2010-08-16 21:51:00 +0000
@@ -3078,7 +3078,7 @@
     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.
@@ -3088,10 +3088,8 @@
             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:
+            via = "Membership through %s" % via.displayname
 
         if membership is None:
             #we have membership via an indirect team, and can use
@@ -3129,17 +3127,16 @@
     def active_participations(self):
         """Return the participation information for active memberships."""
         participations = [self._asParticipation(membership=membership)
-                for membership in self.context.myactivememberships
+                for membership in self.context.team_memberships
                 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):
                 continue
             # The key points of the path for presentation are:
             # [-?] indirect memberships, [-2] direct membership, [-1] team.
-            team_path = self.context.findPathToTeam(team)
+            team_path = self.context.findPathToTeam(team, limit=1)
             participations.append(
-                self._asParticipation(team_path=team_path[:-1], team=team))
+                self._asParticipation(via=team_path[-2], team=team))
         return sorted(participations, key=itemgetter('displayname'))
 
     @cachedproperty

=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py	2010-08-13 07:48:54 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py	2010-08-16 21:51:00 +0000
@@ -253,7 +253,7 @@
         team = self.factory.makeTeam()
         login_person(team.teamowner)
         team.addMember(self.user, team.teamowner)
-        for membership in self.user.myactivememberships:
+        for membership in self.user.team_memberships:
             membership.setStatus(
                 TeamMembershipStatus.ADMIN, team.teamowner)
         [participation] = self.view.active_participations
@@ -344,8 +344,8 @@
             participation['displayname'] for participation in participations]
         self.assertEqual(['A', 'B', 'C'], display_names)
         self.assertEqual(None, participations[0]['via'])
-        self.assertEqual('A', participations[1]['via'])
-        self.assertEqual('A, B', participations[2]['via'])
+        self.assertEqual('Membership through A', participations[1]['via'])
+        self.assertEqual('Membership through B', participations[2]['via'])
 
     def test_has_participations_false(self):
         participations = self.view.active_participations

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2010-08-03 08:49:19 +0000
+++ lib/lp/registry/interfaces/person.py	2010-08-16 21:51:00 +0000
@@ -131,6 +131,7 @@
 
 def validate_person(obj, attr, value):
     """Validate the person is a real person with no other restrictions."""
+
     def validate(person):
         return IPerson.providedBy(person)
     return validate_person_common(obj, attr, value, validate)
@@ -610,9 +611,9 @@
                         readonly=True, required=False,
                         value_type=Reference(schema=IJabberID)),
         exported_as='jabber_ids')
-    myactivememberships = exported(
+    team_memberships = exported(
         CollectionField(
-            title=_("All TeamMemberships for Teams this Person is an "
+            title=_("All TeamMemberships for Teams this Team or Person is an "
                     "active member of."),
             value_type=Reference(schema=ITeamMembership),
             readonly=True, required=False),

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-08-13 08:26:15 +0000
+++ lib/lp/registry/model/person.py	2010-08-16 21:51:00 +0000
@@ -48,7 +48,7 @@
     StringCol)
 from sqlobject.sqlbuilder import AND, OR, SQLConstant
 from storm.store import EmptyResultSet, Store
-from storm.expr import And, In, Join, LeftJoin, Lower, Not, Or, SQL
+from storm.expr import And, In, Join, LeftJoin, Lower, Not, Or, SQL, Desc
 from storm.info import ClassAlias
 
 from canonical.config import config
@@ -749,8 +749,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.
+        """
         # This is our guarantee that _getDirectMemberIParticipateIn() will
         # never return None
         assert self.hasParticipationEntryFor(team), (
@@ -759,9 +766,13 @@
         assert team.is_team, "You can't pass a person to this method."
         path = [team]
         team = self._getDirectMemberIParticipateIn(team)
+        increment = 0
         while team != self:
+            if increment == limit:
+                break
             path.insert(0, team)
             team = self._getDirectMemberIParticipateIn(team)
+            increment += 1
         return path
 
     def _getDirectMemberIParticipateIn(self, team):
@@ -1557,19 +1568,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])))
 
     def _getMappedParticipantsLocations(self, limit=None):
         """See `IPersonViewRestricted`."""
@@ -1675,7 +1683,7 @@
         assert self.is_valid_person, (
             "You can only deactivate an account of a valid person.")
 
-        for membership in self.myactivememberships:
+        for membership in self.team_memberships:
             self.leave(membership.team)
 
         # Deactivate CoC signatures, invalidate email addresses, unassign bug
@@ -1922,8 +1930,9 @@
 
     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))
         return result[:limit]
 
     @property

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-08-13 07:48:54 +0000
+++ lib/lp/registry/tests/test_person.py	2010-08-16 21:51:00 +0000
@@ -44,19 +44,45 @@
 
     layer = DatabaseFunctionalLayer
 
+    def setUp(self):
+        super(TestPersonTeams, self).setUp()
+        self.user = self.factory.makePerson()
+        self.a_team = self.factory.makeTeam(name='a')
+        self.b_team = self.factory.makeTeam(name='b', owner=self.a_team)
+        self.c_team = self.factory.makeTeam(name='c', owner=self.b_team)
+        login_person(self.a_team.teamowner)
+        self.a_team.addMember(self.user, self.a_team.teamowner)
+
     def test_teams_indirectly_participated_in(self):
-        self.user = self.factory.makePerson()
-        a_team = self.factory.makeTeam(name='a')
-        b_team = self.factory.makeTeam(name='b', owner=a_team)
-        c_team = self.factory.makeTeam(name='c', owner=b_team)
-        login_person(a_team.teamowner)
-        a_team.addMember(self.user, a_team.teamowner)
         indirect_teams = self.user.teams_indirectly_participated_in
-        expected_teams = [b_team, c_team]
+        expected_teams = [self.b_team, self.c_team]
         test_teams = sorted(indirect_teams,
             key=lambda team: team.displayname)
         self.assertEqual(expected_teams, test_teams)
 
+    def test_team_memberships(self):
+        memberships = self.user.team_memberships
+        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):
+        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)
+
+        self.assertEqual([self.a_team], path_to_a)
+        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)
+
 
 class TestPerson(TestCaseWithFactory):
 

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2010-07-29 20:16:23 +0000
+++ lib/lp/registry/vocabularies.py	2010-08-16 21:51:00 +0000
@@ -220,8 +220,7 @@
             like_query = "'%%' || %s || '%%'" % quote_like(query)
             fti_query = quote(query)
             sql = "active = 't' AND (name LIKE %s OR fti @@ ftq(%s))" % (
-                    like_query, fti_query
-                    )
+                    like_query, fti_query)
             return self._table.select(sql, orderBy=self._orderBy)
         return self.emptySelectResults()
 
@@ -268,8 +267,7 @@
             like_query = "'%%' || %s || '%%'" % quote_like(query)
             fti_query = quote(query)
             sql = "active = 't' AND (name LIKE %s OR fti @@ ftq(%s))" % (
-                    like_query, fti_query
-                    )
+                    like_query, fti_query)
             return self._table.select(sql)
         return self.emptySelectResults()
 
@@ -545,11 +543,8 @@
                        # Or a person who has an active account and a working
                        # email address.
                        And(Account.status == AccountStatus.ACTIVE,
-                           EmailAddress.status.is_in(valid_email_statuses))
-                       ),
-                    self.extra_clause
-                    )
-                )
+                           EmailAddress.status.is_in(valid_email_statuses))),
+                    self.extra_clause))
             # The public query doesn't need to be ordered as it will be done
             # at the end.
             public_result.order_by()
@@ -789,7 +784,7 @@
     def _get_teams(self):
         """The teams that the vocabulary is built from."""
         return [membership.team for membership
-                in self.context.myactivememberships
+                in self.context.team_memberships
                 if membership.team.visibility == PersonVisibility.PUBLIC]
 
     def __len__(self):
@@ -980,9 +975,7 @@
                 Milestone.q.productseriesID == ProductSeries.q.id,
                 ProductSeries.q.productID == Product.q.id,
                 Product.q.name == productname,
-                ProductSeries.q.name == productseriesname
-                )
-            )
+                ProductSeries.q.name == productseriesname))
         try:
             return self.toTerm(obj)
         except IndexError:
@@ -1268,8 +1261,7 @@
         return [
             project for project in sorted(projects,
                                           key=attrgetter('displayname'))
-            if not project.qualifies_for_free_hosting
-            ]
+            if not project.qualifies_for_free_hosting]
 
     def _doSearch(self, query=None):
         """Return terms where query is in the text of name
@@ -1407,11 +1399,8 @@
                     Distribution.q.id == DistroSeries.q.distributionID,
                     OR(
                         CONTAINSSTRING(Distribution.q.name, query),
-                        CONTAINSSTRING(DistroSeries.q.name, query)
-                        )
-                    ),
-                orderBy=self._orderBy
-                )
+                        CONTAINSSTRING(DistroSeries.q.name, query))),
+                    orderBy=self._orderBy)
         return objs
 
 
@@ -1425,8 +1414,7 @@
         """See `IVocabulary`."""
         if IPillarName.providedBy(obj):
             assert obj.active, 'Inactive object %s %d' % (
-                    obj.__class__.__name__, obj.id
-                    )
+                    obj.__class__.__name__, obj.id)
             obj = obj.pillar
 
         # It is a hack using the class name here, but it works


Follow ups