launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

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

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


Summary

Partial fix for timeout OOPses on +participation by reducing lookups for mailing list information and making indirect_teams a more efficient query process.

Proposed fix

Replace an expensive SQLObject query with a better Storm query, and improve the template so we don't keep looking up subscription data.

Pre-implementation notes

Spoke with Curtis Hovey (sinzui) about the causes of the timeouts

Implementation details

Person.teams_indirectly_participated_in now uses a Storm query structured to avoid the need for subselects. The callsite has been updated to use the new returned data and go with sane defaults for indirect teams (e.g. member role cannot be admin, so don't check).

The template also uses direct content rather than checking conditions, where possible.

Tests

bin/test -t test_teams_indirect
bin/test -t test_active_participations

Demo and Q/A

Membership data on launchpad.dev should show sanely.

lint

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/person.py
  lib/lp/registry/browser/tests/test_person_view.py
  lib/lp/registry/model/person.py
  lib/lp/registry/templates/person-participation.pt
  lib/lp/registry/tests/test_person.py

-- 
https://code.launchpad.net/~jcsackett/launchpad/plus-participation-timeouts/+merge/32268
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/plus-participation-timeouts into lp:launchpad/devel.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-08-05 22:12:44 +0000
+++ lib/lp/registry/browser/person.py	2010-08-10 21:52:46 +0000
@@ -3073,19 +3073,25 @@
     def label(self):
         return 'Team participation for ' + self.context.displayname
 
-    def _asParticipation(self, membership, team_path=None):
+    def _asParticipation(self, membership, team_path=None, team=None):
         """Return a dict of participation information for the membership."""
         if team_path is None:
             via = None
         else:
             via = COMMASPACE.join(team.displayname for team in team_path)
-        team = membership.team
-        if membership.person == team.teamowner:
-            role = 'Owner'
-        elif membership.status == TeamMembershipStatus.ADMIN:
-            role = 'Admin'
-        else:
+        if membership is None:
             role = 'Member'
+            datejoined = None
+        else:
+            if team is None:
+                team = membership.team
+            datejoined = membership.datejoined
+            if membership.person == team.teamowner:
+                role = 'Owner'
+            elif membership.status == TeamMembershipStatus.ADMIN:
+                role = 'Admin'
+            else:
+                role = 'Member'
         if team.mailing_list is not None and team.mailing_list.is_usable:
             subscription = team.mailing_list.getSubscription(self.context)
             if subscription is None:
@@ -3093,9 +3099,9 @@
             else:
                 subscribed = 'Subscribed'
         else:
-            subscribed = None
+            subscribed = '—'
         return dict(
-            displayname=team.displayname, team=team, membership=membership,
+            displayname=team.displayname, team=team, datejoined=datejoined,
             role=role, via=via, subscribed=subscribed)
 
     @cachedproperty
@@ -3105,16 +3111,15 @@
                 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:
+        for team, membership 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)
-            membership = membership_set.getByPersonAndTeam(
-                team_path[-2], team)
             participations.append(
-                self._asParticipation(membership, team_path=team_path[:-1]))
+                self._asParticipation(
+                    membership, team_path=team_path[:-1], team=team))
         return sorted(participations, key=itemgetter('displayname'))
 
     @cachedproperty
@@ -3600,8 +3605,8 @@
 
     def add_ssh(self):
         sshkey = self.request.form.get('sshkey')
-	try:
-      	    getUtility(ISSHKeySet).new(self.user, sshkey)
+        try:
+            getUtility(ISSHKeySet).new(self.user, sshkey)
         except SSHKeyAdditionError:
             self.error_message = structured('Invalid public key')
         except SSHKeyCompromisedError:

=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py	2010-08-02 02:13:52 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py	2010-08-10 21:52:46 +0000
@@ -274,7 +274,7 @@
         login_person(team.teamowner)
         team.addMember(self.user, team.teamowner)
         [participation] = self.view.active_participations
-        self.assertEqual(None, participation['subscribed'])
+        self.assertEqual('—', participation['subscribed'])
 
     def test__asParticpation_unsubscribed_to_mailing_list(self):
         # The default team role is 'Member'.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-07-23 12:24:55 +0000
+++ lib/lp/registry/model/person.py	2010-08-10 21:52:46 +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, Lower, Not, Or, SQL
+from storm.expr import And, In, Join, LeftJoin, Lower, Not, Or, SQL
 from storm.info import ClassAlias
 
 from canonical.config import config
@@ -1956,27 +1956,23 @@
     @property
     def teams_indirectly_participated_in(self):
         """See `IPerson`."""
-        return Person.select("""
-              -- we are looking for teams, so we want "people" that are on the
-              -- teamparticipation.team side of teamparticipation
-            Person.id = TeamParticipation.team AND
-              -- where this person participates in the team
-            TeamParticipation.person = %s AND
-              -- but not the teamparticipation for "this person in himself"
-              -- which exists for every person
-            TeamParticipation.team != %s AND
-              -- nor do we want teams in which the person is a direct
-              -- participant, so we exclude the teams in which there is
-              -- a teammembership for this person
-            TeamParticipation.team NOT IN
-              (SELECT TeamMembership.team FROM TeamMembership WHERE
-                      TeamMembership.person = %s AND
-                      TeamMembership.status IN (%s, %s))
-            """ % sqlvalues(self.id, self.id, self.id,
-                            TeamMembershipStatus.APPROVED,
-                            TeamMembershipStatus.ADMIN),
-            clauseTables=['TeamParticipation'],
-            orderBy=Person.sortingColumns)
+        Team = ClassAlias(Person, "Team")
+        store = Store.of(self)
+        origin = [
+            Team,
+            Join(TeamParticipation, Team.id == TeamParticipation.teamID),
+            LeftJoin(TeamMembership,
+                And(TeamMembership.person == self.id,
+                    TeamMembership.teamID == TeamParticipation.teamID,
+                    TeamMembership.status.is_in([
+                        TeamMembershipStatus.APPROVED,
+                        TeamMembershipStatus.ADMIN])))]
+        find_objects = (Team, TeamMembership)
+        return store.using(*origin).find(find_objects,
+            And(
+                TeamParticipation.person == self.id,
+                TeamParticipation.person != TeamParticipation.teamID,
+                TeamMembership.id == None))
 
     @property
     def teams_with_icons(self):

=== modified file 'lib/lp/registry/templates/person-participation.pt'
--- lib/lp/registry/templates/person-participation.pt	2010-06-24 18:27:11 +0000
+++ lib/lp/registry/templates/person-participation.pt	2010-08-10 21:52:46 +0000
@@ -39,7 +39,7 @@
           </td>
           <td>
             <tal:date condition="not: participation/via"
-              tal:replace="participation/membership/datejoined/fmt:date">
+              tal:replace="participation/datejoined/fmt:date">
               2005-06-17
             </tal:date>
             <tal:no-date condition="participation/via">
@@ -60,13 +60,9 @@
           </td>
           <td
             tal:condition="not: context/teamowner">
-            <tal:subscribed condition="participation/team/mailing_list"
-              replace="participation/subscribed">
+            <tal:subscribed replace="structure participation/subscribed">
               yes
             </tal:subscribed>
-            <tal:no-list condition="not: participation/team/mailing_list">
-              &mdash;
-            </tal:no-list>
           </td>
         </tr>
       </tbody>

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-08-04 23:27:49 +0000
+++ lib/lp/registry/tests/test_person.py	2010-08-10 21:52:46 +0000
@@ -40,6 +40,24 @@
 from canonical.testing.layers import DatabaseFunctionalLayer, reconnect_stores
 
 
+class TestPersonTeams(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    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]
+        test_teams = sorted(indirect_teams,
+            key=lambda team: team[0].displayname)
+        self.assertEqual(expected_teams, test_teams)
+
+
 class TestPerson(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer