launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03133
[Merge] lp:~gary/launchpad/bug741684-2 into lp:launchpad
Gary Poster has proposed merging lp:~gary/launchpad/bug741684-2 into lp:launchpad with lp:~gary/launchpad/bug741684 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #741684 in Launchpad itself: "bug notification script needs to be optimized"
https://bugs.launchpad.net/launchpad/+bug/741684
Bug #742230 in Launchpad itself: "Too much repeated SQL in send-bug-notifications job"
https://bugs.launchpad.net/launchpad/+bug/742230
For more details, see:
https://code.launchpad.net/~gary/launchpad/bug741684-2/+merge/55657
This branch is the second of two that I have in mind to address the linked bugs. The other branch is the one on which this one depends, lp:~gary/launchpad/bug741684. The only additional change I know of at the moment may be to increase the Storm cache size, as discussed in the MP for the other branch.
This branch replaces canonical.launchpad.helpers.emailPeople with lib.lp.registry.model.person.get_recipients. The change in code is to make fewer passes through the transitive search, and to take advantage of the IPersonSet.getPrecachedPersonsFromIDs method to early-load email information that we need. The change in module is because the new code uses database tools that are not allowed to be imported from the scripts location. The change in name was because I already had to change all the call sites, and I thought it was an improved name, since I thought it more clearly described the function's purpose.
I briefly toyed with the idea of making this a method on the Person class. I really have wanted to see a move to tightly-focused data-based models, though, so I preferred to make this a function. That so, if you, the reviewer, request that it be a method, I will make it so with nary a complaint.
When migrating the tests of the old function to the new location, I expanded them. The test_get_recipients_complex_indirect in particular I used in experiments to show that even in a relatively simple case like that I was reducing database calls by half. I'm hopeful that in real-world usage, the change will be significantly more dramatic.
I incorporated the new version of the function everywhere the old one was used; I am running tests now to see if this causes any unpleasant hiccups. As you might expect, the usage I'm most interested in right now is the one in lib/lp/bugs/scripts/bugnotification.py.
Thank you for your time.
Gary
--
https://code.launchpad.net/~gary/launchpad/bug741684-2/+merge/55657
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug741684-2 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/helpers.py'
--- lib/canonical/launchpad/helpers.py 2011-02-17 16:15:50 +0000
+++ lib/canonical/launchpad/helpers.py 2011-03-31 00:59:57 +0000
@@ -200,43 +200,20 @@
return output
-def emailPeople(person):
- """Return a set of people to who receive email for this Person.
-
- If <person> has a preferred email, the set will contain only that
- person. If <person> doesn't have a preferred email but is a team,
- the set will contain the preferred email address of each member of
- <person>, including indirect members.
-
- Finally, if <person> doesn't have a preferred email and is not a team,
- the set will be empty.
- """
- pending_people = [person]
- people = set()
- seen = set()
- while len(pending_people) > 0:
- person = pending_people.pop()
- if person in seen:
- continue
- seen.add(person)
- if person.preferredemail is not None:
- people.add(person)
- elif person.isTeam():
- pending_people.extend(person.activemembers)
- return people
-
-
def get_contact_email_addresses(person):
"""Return a set of email addresses to contact this Person.
- In general, it is better to use emailPeople instead.
+ In general, it is better to use lp.registry.model.person.get_recipients
+ instead.
"""
# Need to remove the security proxy of the email address because the
# logged in user may not have permission to see it.
from zope.security.proxy import removeSecurityProxy
+ # Circular imports force this import.
+ from lp.registry.model.person import get_recipients
return set(
str(removeSecurityProxy(mail_person.preferredemail).email)
- for mail_person in emailPeople(person))
+ for mail_person in get_recipients(person))
replacements = {0: {'.': ' |dot| ',
=== modified file 'lib/canonical/launchpad/tests/test_helpers.py'
--- lib/canonical/launchpad/tests/test_helpers.py 2010-10-21 04:19:36 +0000
+++ lib/canonical/launchpad/tests/test_helpers.py 2011-03-31 00:59:57 +0000
@@ -8,12 +8,9 @@
from zope.publisher.interfaces.browser import IBrowserRequest
from canonical.launchpad import helpers
-from canonical.launchpad.ftests import login
from canonical.launchpad.webapp.interfaces import ILaunchBag
-from canonical.testing.layers import LaunchpadFunctionalLayer
from lp.registry.interfaces.person import IPerson
from lp.services.worlddata.interfaces.language import ILanguageSet
-from lp.testing.factory import LaunchpadObjectFactory
from lp.translations.utilities.translation_export import LaunchpadWriteTarFile
@@ -298,41 +295,10 @@
self.assertEqual(text, helpers.truncate_text(text, len(text)))
-class TestEmailPeople(unittest.TestCase):
- """Tests for emailPeople"""
-
- layer = LaunchpadFunctionalLayer
-
- def setUp(self):
- unittest.TestCase.setUp(self)
- login('foo.bar@xxxxxxxxxxxxx')
- self.factory = LaunchpadObjectFactory()
-
- def test_emailPeopleIndirect(self):
- """Ensure emailPeople uses indirect memberships."""
- owner = self.factory.makePerson(
- displayname='Foo Bar', email='foo@xxxxxxx', password='password')
- team = self.factory.makeTeam(owner)
- super_team = self.factory.makeTeam(team)
- recipients = helpers.emailPeople(super_team)
- self.assertEqual(set([owner]), recipients)
-
- def test_emailPeopleTeam(self):
- """Ensure emailPeople uses teams with preferredemail."""
- owner = self.factory.makePerson(
- displayname='Foo Bar', email='foo@xxxxxxx', password='password')
- team = self.factory.makeTeam(owner, email='team@xxxxxxx')
- super_team = self.factory.makeTeam(team)
- recipients = helpers.emailPeople(super_team)
- self.assertEqual(set([team]), recipients)
-
-
def test_suite():
suite = unittest.TestSuite()
suite.addTest(DocTestSuite())
suite.addTest(DocTestSuite(helpers))
suite.addTest(
unittest.TestLoader().loadTestsFromTestCase(TruncateTextTest))
- suite.addTest(
- unittest.TestLoader().loadTestsFromTestCase(TestEmailPeople))
return suite
=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py 2011-03-31 00:59:44 +0000
+++ lib/lp/bugs/scripts/bugnotification.py 2011-03-31 00:59:57 +0000
@@ -18,10 +18,7 @@
import transaction
from zope.component import getUtility
-from canonical.launchpad.helpers import (
- emailPeople,
- get_email_template,
- )
+from canonical.launchpad.helpers import get_email_template
from canonical.launchpad.scripts.logger import log
from canonical.launchpad.webapp import canonical_url
from lp.bugs.mail.bugnotificationbuilder import (
@@ -30,6 +27,7 @@
)
from lp.bugs.mail.newbug import generate_bug_add_email
from lp.bugs.interfaces.bugnotification import IBugNotificationSet
+from lp.registry.model.person import get_recipients
from lp.services.mail.mailwrapper import MailWrapper
@@ -108,7 +106,7 @@
# We will report this notification.
filtered_notifications.append(notification)
for recipient in notification.recipients:
- email_people = emailPeople(recipient.person)
+ email_people = get_recipients(recipient.person)
for email_person in email_people:
recipients_for_person = recipients.get(email_person)
if recipients_for_person is None:
@@ -169,6 +167,7 @@
cached_filters = {}
bug_notification_set = getUtility(IBugNotificationSet)
+
def get_filter_descriptions(recipients):
"Given a list of recipients, return the filter descriptions for them."
descriptions = set()
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2011-03-29 03:13:40 +0000
+++ lib/lp/registry/model/person.py 2011-03-31 00:59:57 +0000
@@ -7,6 +7,7 @@
__metaclass__ = type
__all__ = [
+ 'get_recipients',
'generate_nick',
'IrcID',
'IrcIDSet',
@@ -57,6 +58,7 @@
And,
Desc,
Exists,
+ In,
Join,
LeftJoin,
Min,
@@ -4561,3 +4563,65 @@
if person is None:
raise ComponentLookupError()
return person
+
+
+@ProxyFactory
+def get_recipients(person):
+ """Return a set of people who receive email for this Person (person/team).
+
+ If <person> has a preferred email, the set will contain only that
+ person. If <person> doesn't have a preferred email but is a team,
+ the set will contain the preferred email address of each member of
+ <person>, including indirect members.
+
+ Finally, if <person> doesn't have a preferred email and is not a team,
+ the set will be empty.
+ """
+ if person.preferredemail:
+ return [person]
+ elif person.is_team:
+ # Get transitive members of team with a preferred email.
+ return _get_recipients_for_team(person)
+ else:
+ return []
+
+
+def _get_recipients_for_team(team):
+ """Given a team without a preferred email, return recipients.
+
+ Helper for get_recipients, divided out simply to make get_recipients
+ easier to understand in broad brush."""
+ store = IStore(Person)
+ source = store.using(TeamMembership,
+ Join(Person,
+ TeamMembership.personID==Person.id),
+ LeftJoin(EmailAddress,
+ EmailAddress.person == Person.id))
+ pending_team_ids = [team.id]
+ recipient_ids = set()
+ seen = set()
+ while pending_team_ids:
+ intermediate_transitive_results = source.find(
+ (TeamMembership.personID, EmailAddress.personID),
+ In(TeamMembership.status,
+ [TeamMembershipStatus.ADMIN.value,
+ TeamMembershipStatus.APPROVED.value]),
+ In(TeamMembership.teamID, pending_team_ids),
+ Or(EmailAddress.status == EmailAddressStatus.PREFERRED,
+ And(Not(Person.teamownerID == None),
+ EmailAddress.status == None))).config(distinct=True)
+ next_ids = []
+ for (person_id,
+ preferred_email_marker) in intermediate_transitive_results:
+ if preferred_email_marker is None:
+ # This is a team without a preferred email address.
+ if person_id not in seen:
+ next_ids.append(person_id)
+ seen.add(person_id)
+ else:
+ recipient_ids.add(person_id)
+ pending_team_ids = next_ids
+ return getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+ recipient_ids,
+ need_validity=True,
+ need_preferred_email=True)
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2011-03-26 16:28:39 +0000
+++ lib/lp/registry/tests/test_person.py 2011-03-31 00:59:57 +0000
@@ -35,6 +35,7 @@
from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
from canonical.testing.layers import (
DatabaseFunctionalLayer,
+ LaunchpadFunctionalLayer,
reconnect_stores,
)
from lp.answers.model.answercontact import AnswerContact
@@ -61,7 +62,10 @@
KarmaCategory,
KarmaTotalCache,
)
-from lp.registry.model.person import Person
+from lp.registry.model.person import (
+ get_recipients,
+ Person,
+ )
from lp.services.openid.model.openididentifier import OpenIdIdentifier
from lp.soyuz.enums import (
ArchivePurpose,
@@ -1293,3 +1297,67 @@
# https://bugs.launchpad.net/storm/+bug/619017 which is adding 3
# queries to the test.
self.assertThat(collector, HasQueryCount(LessThan(14)))
+
+
+class TestGetRecipients(TestCaseWithFactory):
+ """Tests for get_recipients"""
+
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ super(TestGetRecipients, self).setUp()
+ login('foo.bar@xxxxxxxxxxxxx')
+
+ def test_get_recipients_indirect(self):
+ """Ensure get_recipients uses indirect memberships."""
+ owner = self.factory.makePerson(
+ displayname='Foo Bar', email='foo@xxxxxxx', password='password')
+ team = self.factory.makeTeam(owner)
+ super_team = self.factory.makeTeam(team)
+ recipients = get_recipients(super_team)
+ self.assertEqual(set([owner]), set(recipients))
+
+ def test_get_recipients_team(self):
+ """Ensure get_recipients uses teams with preferredemail."""
+ owner = self.factory.makePerson(
+ displayname='Foo Bar', email='foo@xxxxxxx', password='password')
+ team = self.factory.makeTeam(owner, email='team@xxxxxxx')
+ super_team = self.factory.makeTeam(team)
+ recipients = get_recipients(super_team)
+ self.assertEqual(set([team]), set(recipients))
+
+ def makePersonWithNoPreferredEmail(self, **kwargs):
+ kwargs['email_address_status'] = EmailAddressStatus.NEW
+ return self.factory.makePerson(**kwargs)
+
+ def get_test_recipients_person(self):
+ person = self.factory.makePerson()
+ recipients = get_recipients(person)
+ self.assertEqual(set([person]), set(recipients))
+
+ def test_get_recipients_empty(self):
+ """get_recipients returns empty set for person with no preferredemail.
+ """
+ recipients = get_recipients(self.makePersonWithNoPreferredEmail())
+ self.assertEqual(set(), set(recipients))
+
+ def test_get_recipients_complex_indirect(self):
+ """Ensure get_recipients uses indirect memberships."""
+ owner = self.factory.makePerson(
+ displayname='Foo Bar', email='foo@xxxxxxx', password='password')
+ team = self.factory.makeTeam(owner)
+ super_team_member_person = self.factory.makePerson(
+ displayname='Bing Bar', email='bing@xxxxxxx')
+ super_team_member_team = self.factory.makeTeam(
+ email='baz@xxxxxxx')
+ super_team = self.factory.makeTeam(
+ team, members=[super_team_member_person,
+ super_team_member_team,
+ self.makePersonWithNoPreferredEmail()])
+ super_team_member_team.acceptInvitationToBeMemberOf(
+ super_team, u'Go Team!')
+ recipients = list(get_recipients(super_team))
+ self.assertEqual(set([owner,
+ super_team_member_person,
+ super_team_member_team]),
+ set(recipients))
=== modified file 'lib/lp/services/mail/notificationrecipientset.py'
--- lib/lp/services/mail/notificationrecipientset.py 2010-12-02 16:13:51 +0000
+++ lib/lp/services/mail/notificationrecipientset.py 2011-03-31 00:59:57 +0000
@@ -14,7 +14,6 @@
from zope.interface import implements
from zope.security.proxy import isinstance as zope_isinstance
-from canonical.launchpad.helpers import emailPeople
from canonical.launchpad.interfaces.launchpad import (
INotificationRecipientSet,
UnknownRecipientError,
@@ -45,7 +44,7 @@
def getRecipients(self):
"""See `INotificationRecipientSet`."""
return sorted(
- self._personToRationale.keys(), key=attrgetter('displayname'))
+ self._personToRationale.keys(), key=attrgetter('displayname'))
def getRecipientPersons(self):
"""See `INotificationRecipientSet`."""
@@ -88,6 +87,7 @@
def add(self, persons, reason, header):
"""See `INotificationRecipientSet`."""
from zope.security.proxy import removeSecurityProxy
+ from lp.registry.model.person import get_recipients
if IPerson.providedBy(persons):
persons = [persons]
@@ -98,7 +98,7 @@
if person in self._personToRationale:
continue
self._personToRationale[person] = reason, header
- for receiving_person in emailPeople(person):
+ for receiving_person in get_recipients(person):
# Bypass zope's security because IEmailAddress.email is not
# public.
preferred_email = removeSecurityProxy(
@@ -116,6 +116,7 @@
def remove(self, persons):
"""See `INotificationRecipientSet`."""
from zope.security.proxy import removeSecurityProxy
+ from lp.registry.model.person import get_recipients
if IPerson.providedBy(persons):
persons = [persons]
for person in persons:
@@ -123,7 +124,7 @@
'You can only remove() an IPerson: %r' % person)
if person in self._personToRationale:
del self._personToRationale[person]
- for removed_person in emailPeople(person):
+ for removed_person in get_recipients(person):
# Bypass zope's security because IEmailAddress.email is
# not public.
preferred_email = removeSecurityProxy(
=== modified file 'lib/lp/soyuz/scripts/ppareport.py'
--- lib/lp/soyuz/scripts/ppareport.py 2010-08-31 11:11:09 +0000
+++ lib/lp/soyuz/scripts/ppareport.py 2011-03-31 00:59:57 +0000
@@ -20,9 +20,9 @@
from zope.component import getUtility
from canonical.config import config
-from canonical.launchpad.helpers import emailPeople
from canonical.launchpad.webapp import canonical_url
from lp.registry.interfaces.distribution import IDistributionSet
+from lp.registry.model.person import get_recipients
from lp.services.propertycache import cachedproperty
from lp.services.scripts.base import (
LaunchpadScript,
@@ -181,7 +181,7 @@
self.output.write('= PPA user emails =\n')
people_to_email = set()
for ppa in self.ppas:
- people_to_email.update(emailPeople(ppa.owner))
+ people_to_email.update(get_recipients(ppa.owner))
sorted_people_to_email = sorted(
people_to_email, key=operator.attrgetter('name'))
for user in sorted_people_to_email:
