launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02406
[Merge] lp:~sinzui/launchpad/getmembershipinformation-0 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/getmembershipinformation-0 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#675835 MailingListApplication:MailingListAPIView (getMembershipInformation) timeout
https://bugs.launchpad.net/bugs/675835
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/getmembershipinformation-0/+merge/47275
MailingListApplication:MailingListAPIView (getMembershipInformation) timeout.
Launchpad bug: https://bugs.launchpad.net/bugs/675835
Pre-implementation: lifeless
Test command: ./bin/test -vv \
-t TestMailinglistSet -t doc/mailinglist-subscriptions
The view times out calling two model methods on MailingListSet:
getSubscribedAddresses() and getSenderAddresses(). Both methods return
a simple dict of strings {team_name: [(person_displayname, email) ...]}
The internals of both methods are working with 4 columns in the database,
but are constructing the objects for teams, persons, mailing lists and
subscriptions.
--------------------------------------------------------------------
RULES
* Add tests for getSubscribedAddresses() and getSenderAddresses() to
document what they do *before* the fix.
* Update getSubscribedAddresses() and getSenderAddresses() to work
with just the required columns in storm/sql. The queries are currently
getting the objects for email address and person. They are also getting
MailingListSubscription and TeamParticpation, but those objects are
not used.
QA
* Watch staging's oops reports and verify there are 0 oopses for
MailingListApplication:MailingListAPIView
LINT
lib/lp/registry/model/mailinglist.py
lib/lp/registry/tests/test_mailinglist.py
IMPLEMENTATION
I Added unit tests for getSubscribedAddresses() and getSenderAddresses().
The methods are indirectly tested in doc/mailinglist-subscriptions.*
but I was not comfortable refactoring the methods without direct tests.
I Extracted duplicate logic to get team and list ids and revised the
SQL to get both in a single query. Updated getSubscribedAddresses() and
getSenderAddresses() to use the new helper. I Updated both methods to work
with just the columns. Note that part of getSenderAddresses already did
this.
lib/lp/registry/model/mailinglist.py
lib/lp/registry/tests/test_mailinglist.py
--
https://code.launchpad.net/~sinzui/launchpad/getmembershipinformation-0/+merge/47275
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/getmembershipinformation-0 into lp:launchpad.
=== modified file 'lib/lp/registry/model/mailinglist.py'
--- lib/lp/registry/model/mailinglist.py 2010-12-11 03:48:21 +0000
+++ lib/lp/registry/model/mailinglist.py 2011-01-24 17:17:34 +0000
@@ -662,6 +662,20 @@
""" % sqlvalues(team_name),
clauseTables=['Person'])
+ def _getTeamIdsAndMailingListIds(self, team_names):
+ """Return a tuple of team and mailing list Ids for the team names."""
+ store = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR)
+ tables = (
+ Person,
+ Join(MailingList, MailingList.team == Person.id))
+ results = set(store.using(*tables).find(
+ (Person.id, MailingList.id),
+ And(Person.name.is_in(team_names),
+ Person.teamowner != None)))
+ team_ids = [result[0] for result in results]
+ list_ids = [result[1] for result in results]
+ return team_ids, list_ids
+
def getSubscribedAddresses(self, team_names):
"""See `IMailingListSet`."""
store = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR)
@@ -688,19 +702,10 @@
LeftJoin(Team,
Team.id == MailingList.teamID),
)
- team_ids = set(
- team.id for team in store.find(
- Person,
- And(Person.name.is_in(team_names),
- Person.teamowner != None)))
- list_ids = set(
- mailing_list.id for mailing_list in store.find(
- MailingList,
- MailingList.teamID.is_in(team_ids)))
+ team_ids, list_ids = self._getTeamIdsAndMailingListIds(team_names)
# Find all the people who are subscribed with their preferred address.
preferred = store.using(*tables).find(
- (EmailAddress, MailingListSubscription, TeamParticipation,
- Person, Team),
+ (EmailAddress.email, Person.displayname, Team.name),
And(MailingListSubscription.mailing_listID.is_in(list_ids),
TeamParticipation.teamID.is_in(team_ids),
MailingList.teamID == TeamParticipation.teamID,
@@ -710,11 +715,11 @@
Account.status == AccountStatus.ACTIVE))
# Sort by team name.
by_team = {}
- for address, subscription, participation, person, team in preferred:
- assert team.name in team_names, (
- 'Unexpected team name in results: %s' % team.name)
- value = (person.displayname, address.email)
- by_team.setdefault(team.name, set()).add(value)
+ for email, display_name, team_name in preferred:
+ assert team_name in team_names, (
+ 'Unexpected team name in results: %s' % team_name)
+ value = (display_name, email)
+ by_team.setdefault(team_name, set()).add(value)
tables = (
EmailAddress,
LeftJoin(Account, Account.id == EmailAddress.accountID),
@@ -734,16 +739,16 @@
Team.id == MailingList.teamID),
)
explicit = store.using(*tables).find(
- (EmailAddress, MailingList, Person, Team),
+ (EmailAddress.email, Person.displayname, Team.name),
And(MailingListSubscription.mailing_listID.is_in(list_ids),
TeamParticipation.teamID.is_in(team_ids),
MailingList.status != MailingListStatus.INACTIVE,
Account.status == AccountStatus.ACTIVE))
- for address, mailing_list, person, team in explicit:
- assert team.name in team_names, (
- 'Unexpected team name in results: %s' % team.name)
- value = (person.displayname, address.email)
- by_team.setdefault(team.name, set()).add(value)
+ for email, display_name, team_name in explicit:
+ assert team_name in team_names, (
+ 'Unexpected team name in results: %s' % team_name)
+ value = (display_name, email)
+ by_team.setdefault(team_name, set()).add(value)
# Turn the results into a mapping of lists.
results = {}
for team_name, address_set in by_team.items():
@@ -766,11 +771,7 @@
Join(MailingList, MailingList.teamID == TeamParticipation.teamID),
Join(Team, Team.id == MailingList.teamID),
)
- team_ids = set(
- team.id for team in store.find(
- Person,
- And(Person.name.is_in(team_names),
- Person.teamowner != None)))
+ team_ids, list_ids = self._getTeamIdsAndMailingListIds(team_names)
team_members = store.using(*tables).find(
(Team.name, Person.displayname, EmailAddress.email),
And(TeamParticipation.teamID.is_in(team_ids),
@@ -793,10 +794,6 @@
MailingList.id == MessageApproval.mailing_listID),
Join(Team, Team.id == MailingList.teamID),
)
- list_ids = set(
- mailing_list.id for mailing_list in store.find(
- MailingList,
- MailingList.teamID.is_in(team_ids)))
approved_posters = store.using(*tables).find(
(Team.name, Person.displayname, EmailAddress.email),
And(MessageApproval.mailing_listID.is_in(list_ids),
=== modified file 'lib/lp/registry/tests/test_mailinglist.py'
--- lib/lp/registry/tests/test_mailinglist.py 2010-10-26 15:47:24 +0000
+++ lib/lp/registry/tests/test_mailinglist.py 2011-01-24 17:17:34 +0000
@@ -4,12 +4,26 @@
__metaclass__ = type
__all__ = []
-from canonical.testing.layers import DatabaseFunctionalLayer
+from textwrap import dedent
+import transaction
+
+from zope.component import getUtility
+
+from canonical.launchpad.interfaces.message import IMessageSet
+from canonical.testing.layers import (
+ DatabaseFunctionalLayer,
+ LaunchpadFunctionalLayer,
+ )
from lp.registry.interfaces.mailinglistsubscription import (
MailingListAutoSubscribePolicy,
)
from lp.registry.interfaces.person import TeamSubscriptionPolicy
-from lp.testing import login_celebrity, person_logged_in, TestCaseWithFactory
+from lp.registry.interfaces.mailinglist import IMailingListSet
+from lp.testing import (
+ login_celebrity,
+ person_logged_in,
+ TestCaseWithFactory,
+ )
class MailingList_getSubscribers_TestCase(TestCaseWithFactory):
@@ -60,3 +74,147 @@
self.assertEqual(2, subscribers.count())
self.assertEqual(
['pa2', 'pb1'], [person.name for person in subscribers])
+
+
+class MailingListSetMixin:
+ """Helpers for testing the MailingListSet class."""
+
+ def makeTeamWithMailingListSubscribers(self, team_name, super_team=None,
+ auto_subscribe=True):
+ """Create a team, mailing list, and subcribers."""
+ team = self.factory.makeTeam(name=team_name)
+ if super_team is None:
+ mailing_list = self.factory.makeMailingList(team, team.teamowner)
+ else:
+ super_team.addMember(
+ team, reviewer=team.teamowner, force_team_add=True)
+ mailing_list = super_team.mailing_list
+ member = self.factory.makePerson()
+ team.addMember(member, reviewer=team.teamowner)
+ if auto_subscribe:
+ mailing_list.subscribe(member)
+ transaction.commit()
+ return team, member
+
+
+class TestMailinglistSet(TestCaseWithFactory, MailingListSetMixin):
+ """Test the mailing list set class."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestMailinglistSet, self).setUp()
+ self.mailing_list_set = getUtility(IMailingListSet)
+ login_celebrity('admin')
+
+ def test_getSenderAddresses_dict_keys(self):
+ # getSenderAddresses() returns a dict of teams names
+ # {team_name: [(member_displayname, member_email) ...]}
+ team1, member1 = self.makeTeamWithMailingListSubscribers(
+ 'team1', auto_subscribe=False)
+ team2, member2 = self.makeTeamWithMailingListSubscribers(
+ 'team2', auto_subscribe=False)
+ team_names = [team1.name, team2.name]
+ result = self.mailing_list_set.getSenderAddresses(team_names)
+ self.assertEqual(team_names, sorted(result.keys()))
+
+ def test_getSenderAddresses_dict_values(self):
+ # getSenderAddresses() returns a dict of team namess with a list of
+ # all membera display names and email addresses.
+ # {team_name: [(member_displayname, member_email) ...]}
+ team1, member1 = self.makeTeamWithMailingListSubscribers(
+ 'team1', auto_subscribe=False)
+ result = self.mailing_list_set.getSenderAddresses([team1.name])
+ list_senders = sorted([
+ (m.displayname, m.preferredemail.email)
+ for m in team1.allmembers])
+ self.assertEqual(list_senders, sorted(result[team1.name]))
+
+ def test_getSenderAddresses_participation_dict_values(self):
+ # getSenderAddresses() dict values includes indirect participants.
+ team1, member1 = self.makeTeamWithMailingListSubscribers(
+ 'team1', auto_subscribe=False)
+ result = self.mailing_list_set.getSenderAddresses([team1.name])
+ list_senders = sorted([
+ (m.displayname, m.preferredemail.email)
+ for m in team1.allmembers if m.preferredemail])
+ self.assertEqual(list_senders, sorted(result[team1.name]))
+
+ def test_getSubscribedAddresses_dict_keys(self):
+ # getSubscribedAddresses() returns a dict of team names.
+ # {team_name: [(subscriber_displayname, subscriber_email) ...]}
+ team1, member1 = self.makeTeamWithMailingListSubscribers('team1')
+ team2, member2 = self.makeTeamWithMailingListSubscribers('team2')
+ team_names = [team1.name, team2.name]
+ result = self.mailing_list_set.getSubscribedAddresses(team_names)
+ self.assertEqual(team_names, sorted(result.keys()))
+
+ def test_getSubscribedAddresses_dict_values(self):
+ # getSubscribedAddresses() returns a dict of teams names with a list
+ # of subscriber tuples.
+ # {team_name: [(subscriber_displayname, subscriber_email) ...]}
+ team1, member1 = self.makeTeamWithMailingListSubscribers('team1')
+ result = self.mailing_list_set.getSubscribedAddresses([team1.name])
+ list_subscribers = [
+ (member1.displayname, member1.preferredemail.email)]
+ self.assertEqual(list_subscribers, result[team1.name])
+
+ def test_getSubscribedAddresses_participation_dict_values(self):
+ # getSubscribedAddresses() dict values includes indirect participants.
+ team1, member1 = self.makeTeamWithMailingListSubscribers('team1')
+ team2, member2 = self.makeTeamWithMailingListSubscribers(
+ 'team2', super_team=team1)
+ result = self.mailing_list_set.getSubscribedAddresses([team1.name])
+ list_subscribers = sorted([
+ (member1.displayname, member1.preferredemail.email),
+ (member2.displayname, member2.preferredemail.email)])
+ self.assertEqual(list_subscribers, sorted(result[team1.name]))
+
+ def test_getSubscribedAddresses_preferredemail_dict_values(self):
+ # getSubscribedAddresses() dict values include users who want email to
+ # go to their preferred address.
+ team1, member1 = self.makeTeamWithMailingListSubscribers(
+ 'team1', auto_subscribe=False)
+ team1.mailing_list.subscribe(member1)
+ transaction.commit()
+ result = self.mailing_list_set.getSubscribedAddresses([team1.name])
+ list_subscribers = [
+ (member1.displayname, member1.preferredemail.email)]
+ self.assertEqual(list_subscribers, result[team1.name])
+
+
+class TestMailinglistSetMessages(TestCaseWithFactory, MailingListSetMixin):
+ """Test the mailing list set class message rules."""
+
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ super(TestMailinglistSetMessages, self).setUp()
+ self.mailing_list_set = getUtility(IMailingListSet)
+ login_celebrity('admin')
+
+ def test_getSenderAddresses_approved_dict_values(self):
+ # getSenderAddresses() dict values includes senders where were
+ # approved in the list moderation queue.
+ team1, member1 = self.makeTeamWithMailingListSubscribers(
+ 'team1', auto_subscribe=False)
+ owner1 = team1.teamowner
+ sender = self.factory.makePerson()
+ email = dedent(str("""\
+ From: %s
+ To: %s
+ Subject: A question
+ Message-ID: <first-post>
+ Date: Fri, 01 Aug 2000 01:08:59 -0000\n
+ I have a question about this team.
+ """ % (sender.preferredemail.email, team1.mailing_list.address)))
+ message = getUtility(IMessageSet).fromEmail(email)
+ held_message = team1.mailing_list.holdMessage(message)
+ held_message.approve(owner1)
+ transaction.commit()
+ result = self.mailing_list_set.getSenderAddresses([team1.name])
+ list_senders = sorted([
+ (owner1.displayname, owner1.preferredemail.email),
+ (member1.displayname, member1.preferredemail.email),
+ (sender.displayname, sender.preferredemail.email)])
+ self.assertEqual(list_senders, sorted(result[team1.name]))
