Merge lp:~sinzui/launchpad/mailing-list-subscribers-0 into lp:launchpad

[Curtis Hovey]

This is my branch to walk the list of mailing list subscribers properly.

    lp:~sinzui/launchpad/mailing-list-subscribers-0
    Diff size: 201 (lint demanded a lot of fixes)
    Launchpad bug:
          https://bugs.launchpad.net/bugs/568114
    Test command: ./bin/test -vv -t test_mailinglist
    Pre-implementation: no one
    Target release: 10.09

Walk the list of mailing list subscribers properly
-------------------------------------------------

The table layout is abusing the batch navigator, asking for items by index,
causing a query for each user using the getSubscribers() query as the base,
adding an offset and a limit. Since the getSubscribers intends to return a all
users, sorted by display, asking for OFFSET 1, LIMIT 1 and OFFSET 2, LIMIT 2
are working with a pair of results, not one, and the table builder, expecting
only one, always uses the first one!

Adding the proposed Person.id fixes the issue. Using Person.name is clearer.
But the queries on the page remain outrageous. The table could create a list
of the users in the batch before it starts getting items by index so that a
single query for the user is needed.

Rules
-----

    * Add displayname to the getSubscribers() order by clause to ensure
      only 1 person can match repeated calls to OFFSET and Limit.
    * Listify the current batch before the table starts using indexes to
      get items so that one query is called to get all the subscribers
      for the page.

QA
--

    * Visit https://launchpad.net/~kicad-developers/+mailing-list-subscribers
    * Verify that Alain Portals is listed twice and each links to a different
      user profile.
    * View the source of the page and verify less than 129 queries were
      issues (~79 queries)

Lint
----

Linting changed files:
  lib/lp/registry/browser/team.py
  lib/lp/registry/model/mailinglist.py
  lib/lp/registry/tests/test_mailinglist.py

Test
----

    * lib/lp/registry/tests/test_mailinglist.py
      * Updated an older test for getSubscribers to use current rules
        for unittests.
      * Refactored the test so that another test could share the setup.
      * Added a new test to verify that items are sorted by displayname
        and name.

Implementation
--------------

    * lib/lp/registry/browser/team.py
      * Listified the batch of persons. This is an internal optimisation
        that had not test changes.
    * lib/lp/registry/model/mailinglist.py
      * Added Person.name to the order by clause to ensure a query with
        LIMIT 1 OFFSET X will return a distinct row.

[Aaron Bentley (abentley)]

review: Approve

=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/team.py	2010-08-22 19:16:42 +0000
@@ -150,7 +150,7 @@
         "name", "visibility", "displayname", "contactemail",
         "teamdescription", "subscriptionpolicy",
         "defaultmembershipperiod", "renewal_policy",
-        "defaultrenewalperiod",  "teamowner",
+        "defaultrenewalperiod", "teamowner",
         ]
     private_prefix = PRIVATE_TEAM_PREFIX
 
@@ -767,7 +767,7 @@
 
     def renderTable(self):
         html = ['<table style="max-width: 80em">']
-        items = self.subscribers.currentBatch()
+        items = list(self.subscribers.currentBatch())
         assert len(items) > 0, (
             "Don't call this method if there are no subscribers to show.")
         # When there are more than 10 items, we use multiple columns, but

=== modified file 'lib/lp/registry/model/mailinglist.py'
--- lib/lp/registry/model/mailinglist.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/mailinglist.py	2010-08-22 19:16:42 +0000
@@ -389,7 +389,7 @@
                              TeamParticipation.team == self.team,
                              MailingListSubscription.person == Person.id,
                              MailingListSubscription.mailing_list == self)
-        return results.order_by(Person.displayname)
+        return results.order_by(Person.displayname, Person.name)
 
     def subscribe(self, person, address=None):
         """See `IMailingList`."""
@@ -451,8 +451,9 @@
                      MailingListSubscription.personID
                      == EmailAddress.personID),
             # pylint: disable-msg=C0301
-            LeftJoin(MailingList,
-                     MailingList.id == MailingListSubscription.mailing_listID),
+            LeftJoin(
+                MailingList,
+                MailingList.id == MailingListSubscription.mailing_listID),
             LeftJoin(TeamParticipation,
                      TeamParticipation.personID
                      == MailingListSubscription.personID),
@@ -472,8 +473,9 @@
                      MailingListSubscription.email_addressID
                      == EmailAddress.id),
             # pylint: disable-msg=C0301
-            LeftJoin(MailingList,
-                     MailingList.id == MailingListSubscription.mailing_listID),
+            LeftJoin(
+                MailingList,
+                MailingList.id == MailingListSubscription.mailing_listID),
             LeftJoin(TeamParticipation,
                      TeamParticipation.personID
                      == MailingListSubscription.personID),
@@ -664,8 +666,9 @@
                      MailingListSubscription.personID
                      == EmailAddress.personID),
             # pylint: disable-msg=C0301
-            LeftJoin(MailingList,
-                     MailingList.id == MailingListSubscription.mailing_listID),
+            LeftJoin(
+                MailingList,
+                MailingList.id == MailingListSubscription.mailing_listID),
             LeftJoin(TeamParticipation,
                      TeamParticipation.personID
                      == MailingListSubscription.personID),
@@ -678,8 +681,7 @@
             team.id for team in store.find(
                 Person,
                 And(Person.name.is_in(team_names),
-                    Person.teamowner != None))
-            )
+                    Person.teamowner != None)))
         list_ids = set(
             mailing_list.id for mailing_list in store.find(
                 MailingList,
@@ -709,8 +711,9 @@
                      MailingListSubscription.email_addressID
                      == EmailAddress.id),
             # pylint: disable-msg=C0301
-            LeftJoin(MailingList,
-                     MailingList.id == MailingListSubscription.mailing_listID),
+            LeftJoin(
+                MailingList,
+                MailingList.id == MailingListSubscription.mailing_listID),
             LeftJoin(TeamParticipation,
                      TeamParticipation.personID
                      == MailingListSubscription.personID),
@@ -756,8 +759,7 @@
             team.id for team in store.find(
                 Person,
                 And(Person.name.is_in(team_names),
-                    Person.teamowner != None))
-            )
+                    Person.teamowner != None)))
         team_members = store.using(*tables).find(
             (Team.name, Person.displayname, EmailAddress.email),
             And(TeamParticipation.teamID.is_in(team_ids),

=== modified file 'lib/lp/registry/tests/test_mailinglist.py'
--- lib/lp/registry/tests/test_mailinglist.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/tests/test_mailinglist.py	2010-08-22 19:16:42 +0000
@@ -1,64 +1,64 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from __future__ import with_statement
+
 __metaclass__ = type
 __all__ = []
 
-
-import unittest
-
-from canonical.launchpad.ftests import login
-from canonical.testing import LaunchpadFunctionalLayer
+from canonical.testing import DatabaseFunctionalLayer
 from lp.registry.interfaces.mailinglistsubscription import (
     MailingListAutoSubscribePolicy,
     )
 from lp.registry.interfaces.person import TeamSubscriptionPolicy
-from lp.testing import TestCaseWithFactory
+from lp.testing import login_celebrity, person_logged_in, TestCaseWithFactory
 
 
 class MailingList_getSubscribers_TestCase(TestCaseWithFactory):
     """Tests for `IMailingList`.getSubscribers()."""
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
-        # Create a team (tied to a mailing list) with one former member, one
-        # pending member and one active member.
         TestCaseWithFactory.setUp(self)
-        login('[e-mail]')
+        self.team, self.mailing_list = self.factory.makeTeamAndMailingList(
+            'test-mailinglist', 'team-owner')
+
+    def test_only_active_members_can_be_subscribers(self):
         former_member = self.factory.makePerson()
         pending_member = self.factory.makePerson()
         active_member = self.active_member = self.factory.makePerson()
-        self.team, self.mailing_list = self.factory.makeTeamAndMailingList(
-            'test-mailinglist', 'team-owner')
-        self.team.subscriptionpolicy = TeamSubscriptionPolicy.MODERATED
-
         # Each of our members want to be subscribed to a team's mailing list
         # whenever they join the team.
+        login_celebrity('admin')
         former_member.mailing_list_auto_subscribe_policy = (
             MailingListAutoSubscribePolicy.ALWAYS)
         active_member.mailing_list_auto_subscribe_policy = (
             MailingListAutoSubscribePolicy.ALWAYS)
         pending_member.mailing_list_auto_subscribe_policy = (
             MailingListAutoSubscribePolicy.ALWAYS)
-
+        self.team.subscriptionpolicy = TeamSubscriptionPolicy.MODERATED
         pending_member.join(self.team)
-        self.assertEqual(False, pending_member.inTeam(self.team))
-
         self.team.addMember(former_member, reviewer=self.team.teamowner)
         former_member.leave(self.team)
-        self.assertEqual(False, former_member.inTeam(self.team))
-
         self.team.addMember(active_member, reviewer=self.team.teamowner)
-        self.assertEqual(True, active_member.inTeam(self.team))
-
-    def test_only_active_members_can_be_subscribers(self):
         # Even though our 3 members want to subscribe to the team's mailing
         # list, only the active member is considered a subscriber.
-        subscribers = [self.active_member]
-        self.assertEqual(
-            subscribers, list(self.mailing_list.getSubscribers()))
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+        self.assertEqual(
+            [active_member], list(self.mailing_list.getSubscribers()))
+
+    def test_getSubscribers_order(self):
+        person_1 = self.factory.makePerson(name="pb1", displayname="Me")
+        with person_logged_in(person_1):
+            person_1.mailing_list_auto_subscribe_policy = (
+                MailingListAutoSubscribePolicy.ALWAYS)
+            person_1.join(self.team)
+        person_2 = self.factory.makePerson(name="pa2", displayname="Me")
+        with person_logged_in(person_2):
+            person_2.mailing_list_auto_subscribe_policy = (
+                MailingListAutoSubscribePolicy.ALWAYS)
+            person_2.join(self.team)
+        subscribers = self.mailing_list.getSubscribers()
+        self.assertEqual(2, subscribers.count())
+        self.assertEqual(
+            ['pa2', 'pb1'], [person.name for person in subscribers])