launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04584
[Merge] lp:~danilo/launchpad/bug-690568 into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/bug-690568 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #690568 in Launchpad itself: "Timeout when adding ~canonical (400 people) to private PPA"
https://bugs.launchpad.net/launchpad/+bug/690568
For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-690568/+merge/71394
= Bug 690568 =
Subscribing a big team to a private PPA results in a timeout because we fetch preferred email address one by one. Fetch them all in one go.
Archive.getNonActiveSubscribers is used solely for this purpose so we can simply add in the EmailAddress into it. Make it slightly prettier at that.
== Implementation details ==
I haven't fixed lint in the doctest, nor have I bothered with moving the test to a proper unit test.
== Tests ==
bin/test -vvt archivesubscriber.txt -t archive_sub
== Demo and Q/A ==
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/soyuz/doc/archivesubscriber.txt
lib/lp/soyuz/model/archivesubscriber.py
lib/canonical/launchpad/mailnotification.py
lib/lp/soyuz/interfaces/archivesubscriber.py
./lib/lp/soyuz/doc/archivesubscriber.txt
1: narrative uses a moin header.
35: narrative uses a moin header.
232: narrative uses a moin header.
326: source exceeds 78 characters.
372: narrative uses a moin header.
411: narrative uses a moin header.
445: narrative uses a moin header.
--
https://code.launchpad.net/~danilo/launchpad/bug-690568/+merge/71394
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-690568 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/mailnotification.py'
--- lib/canonical/launchpad/mailnotification.py 2011-07-01 09:54:54 +0000
+++ lib/canonical/launchpad/mailnotification.py 2011-08-12 16:09:24 +0000
@@ -285,13 +285,8 @@
template = get_email_template('ppa-subscription-new.txt')
- for person in non_active_subscribers:
-
- if person.preferredemail is None:
- # Don't send to people without a preferred email.
- continue
-
- to_address = [person.preferredemail.email]
+ for person, preferred_email in non_active_subscribers:
+ to_address = [preferred_email.email]
root = getUtility(ILaunchpadRoot)
recipient_subscriptions_url = "%s~/+archivesubscriptions" % (
canonical_url(root))
=== modified file 'lib/lp/soyuz/doc/archivesubscriber.txt'
--- lib/lp/soyuz/doc/archivesubscriber.txt 2011-07-26 21:10:24 +0000
+++ lib/lp/soyuz/doc/archivesubscriber.txt 2011-08-12 16:09:24 +0000
@@ -461,18 +461,18 @@
So the getNonActiveSubscribers() method for this team subscription will
currently include Joe:
- >>> for person in subscription.getNonActiveSubscribers():
- ... print person.displayname
- Celso Providelo
- Joe Smith
- John Smith
+ >>> for person, email in subscription.getNonActiveSubscribers():
+ ... print person.displayname, email.email
+ Celso Providelo celso.providelo@xxxxxxxxxxxxx
+ Joe Smith joe@xxxxxxxxxxx
+ John Smith john@xxxxxxxxxxx
But if we create an auth token for joe to the archive (this could be via
a separate subscription), then he will no longer be listed as a non-active
subscriber for this subscription:
>>> joesmith_token = mark_private_ppa.newAuthToken(joesmith)
- >>> for person in subscription.getNonActiveSubscribers():
+ >>> for person, email in subscription.getNonActiveSubscribers():
... print person.displayname
Celso Providelo
John Smith
@@ -487,7 +487,7 @@
... email="harry@xxxxxxxxxxx")
>>> subscription = mark_private_ppa.newSubscription(
... harrysmith, mark, description=u"subscription for joesmith")
- >>> for person in subscription.getNonActiveSubscribers():
+ >>> for person, email in subscription.getNonActiveSubscribers():
... print person.displayname
Harry Smith
@@ -507,7 +507,7 @@
... team_cprov, mark, force_team_add=True)
>>> subscription = mark_private_ppa.newSubscription(
... launchpad_devs, mark, description=u"LP team too")
- >>> for person in subscription.getNonActiveSubscribers():
+ >>> for person, email in subscription.getNonActiveSubscribers():
... print person.displayname
Celso Providelo
John Smith
=== modified file 'lib/lp/soyuz/interfaces/archivesubscriber.py'
--- lib/lp/soyuz/interfaces/archivesubscriber.py 2011-05-06 14:12:11 +0000
+++ lib/lp/soyuz/interfaces/archivesubscriber.py 2011-08-12 16:09:24 +0000
@@ -93,9 +93,11 @@
def getNonActiveSubscribers():
"""Return the people included in this subscription.
- :return: a storm `ResultSet` of all the people who are included in
- this subscription who do not yet have an active token for the
- corresponding archive.
+ :return: a storm `ResultSet` of all the people and their preferred
+ email address who are included in this subscription who do not
+ yet have an active token for the corresponding archive.
+
+ Persons with no preferred email addresses are not included.
:rtype: `storm.store.ResultSet`
"""
=== modified file 'lib/lp/soyuz/model/archivesubscriber.py'
--- lib/lp/soyuz/model/archivesubscriber.py 2011-05-06 14:12:11 +0000
+++ lib/lp/soyuz/model/archivesubscriber.py 2011-08-12 16:09:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Database class for table ArchiveSubscriber."""
@@ -13,8 +13,8 @@
from storm.expr import (
And,
Desc,
+ Join,
LeftJoin,
- Join,
)
from storm.locals import (
DateTime,
@@ -30,6 +30,8 @@
from canonical.database.constants import UTC_NOW
from canonical.database.enumcol import DBEnum
+from canonical.launchpad.database.emailaddress import EmailAddress
+from canonical.launchpad.interfaces.emailaddress import EmailAddressStatus
from lp.registry.interfaces.person import validate_person
from lp.registry.model.teammembership import TeamParticipation
from lp.soyuz.interfaces.archiveauthtoken import IArchiveAuthTokenSet
@@ -96,26 +98,32 @@
store = Store.of(self)
if self.subscriber.is_team:
+ # We get all the people who already have active tokens for
+ # this archive (for example, through separate subscriptions).
+ auth_token = LeftJoin(
+ ArchiveAuthToken,
+ And(ArchiveAuthToken.person_id == Person.id,
+ ArchiveAuthToken.archive_id == self.archive_id,
+ ArchiveAuthToken.date_deactivated == None))
+
+ team_participation = Join(
+ TeamParticipation,
+ TeamParticipation.personID == Person.id)
+
+ # Only return people with preferred email address set.
+ preferred_email = Join(
+ EmailAddress, EmailAddress.personID == Person.id)
+
# We want to get all participants who are themselves
# individuals, not teams:
- all_subscribers = store.find(
- Person,
+ non_active_subscribers = store.using(
+ Person, team_participation, preferred_email, auth_token).find(
+ (Person, EmailAddress),
+ EmailAddress.status == EmailAddressStatus.PREFERRED,
TeamParticipation.teamID == self.subscriber_id,
- TeamParticipation.personID == Person.id,
- Person.teamowner == None)
-
- # Then we get all the people who already have active
- # tokens for this archive (for example, through separate
- # subscriptions).
- active_subscribers = store.find(
- Person,
- Person.id == ArchiveAuthToken.person_id,
- ArchiveAuthToken.archive_id == self.archive_id,
- ArchiveAuthToken.date_deactivated == None)
-
- # And return just the non active subscribers:
- non_active_subscribers = all_subscribers.difference(
- active_subscribers)
+ Person.teamowner == None,
+ # There is no existing archive auth token.
+ ArchiveAuthToken.person_id == None)
non_active_subscribers.order_by(Person.name)
return non_active_subscribers
else:
@@ -128,8 +136,12 @@
return EmptyResultSet()
# Otherwise return a result set containing only the
- # subscriber.
- return store.find(Person, Person.id == self.subscriber_id)
+ # subscriber and their preferred email address.
+ return store.find(
+ (Person, EmailAddress),
+ Person.id == self.subscriber_id,
+ EmailAddress.personID == Person.id,
+ EmailAddress.status == EmailAddressStatus.PREFERRED)
class ArchiveSubscriberSet:
