launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00694
[Merge] lp:~sinzui/launchpad/mailing-list-subscribers-0 into lp:launchpad/devel
Curtis Hovey has proposed merging lp:~sinzui/launchpad/mailing-list-subscribers-0 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#568114 User appears twice on +mailing-list-subscribers
https://bugs.launchpad.net/bugs/568114
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.
--
https://code.launchpad.net/~sinzui/launchpad/mailing-list-subscribers-0/+merge/33279
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/mailing-list-subscribers-0 into lp:launchpad/devel.
=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py 2010-08-12 19:55:29 +0000
+++ lib/lp/registry/browser/team.py 2010-08-21 02:05:58 +0000
@@ -125,7 +125,7 @@
"name", "visibility", "displayname", "contactemail",
"teamdescription", "subscriptionpolicy",
"defaultmembershipperiod", "renewal_policy",
- "defaultrenewalperiod", "teamowner",
+ "defaultrenewalperiod", "teamowner",
]
private_prefix = PRIVATE_TEAM_PREFIX
@@ -742,7 +742,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-06-11 18:43:23 +0000
+++ lib/lp/registry/model/mailinglist.py 2010-08-21 02:05:58 +0000
@@ -352,7 +352,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`."""
@@ -414,8 +414,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),
@@ -435,8 +436,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),
@@ -627,8 +629,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),
@@ -641,8 +644,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,
@@ -672,8 +674,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),
@@ -719,8 +722,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 2009-07-17 02:25:09 +0000
+++ lib/lp/registry/tests/test_mailinglist.py 2010-08-21 02:05:58 +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('foo.bar@xxxxxxxxxxxxx')
+ 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])
