launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04628
[Merge] lp:~bac/launchpad/bug-813322-2 into lp:launchpad
Brad Crittenden has proposed merging lp:~bac/launchpad/bug-813322-2 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-813322-2/+merge/71733
= Summary =
Marking a bug with many duplicates as a duplicate of another bug is
very expensive because lots of mail notifications are sent for each
bug that is a duplicate of the original bug. The calculation of the
set of recipients is time consuming and involves many rules and
separate queries. Attempts to optimize the queries to allow it to
happen during the web request were not fruitful.
== Proposed fix ==
The notification needs to be done outside of the web request or we'll
continue to have time outs. Ideally this notification should be moved
to the job system we have but it was deemed to be too big of a job at
this time. Instead, I have taken advantage of the fact the
notifications are sent via a cronjob. A new type of notification
(DEFERRED) was introduced. Deferred notifications do not have any
recipients though do maintain all of the data necessary to calculate
the recipient list. The cron script has been changed to process all
of the deferred notifications, creating the recipient list, and
creating corresponding fully-formed PENDING notifications that are
then handled as usual.
== Pre-implementation notes ==
Lots of talks with Gary. Significant help from Benji in tracking down
some inefficiencies.
== Implementation details ==
Note that the Zope event model is used to trigger the creation of
notifications for the original bug and it is invoked in the browser
code and in the API code when a duplicate change is made. I chose to
not modify those call sites so the effect is notifications will be
generated during the web request for the subscribers to that bug being
marked but not for its children. The number of queries using this
scheme has been shown to be 20% of the previous.
== Tests ==
bin/test -vvm lp.bugs -t '(test_bugnotification|tests/bug|test_bug)'
== Demo and Q/A ==
Create a bug with duplicates. Make that bug a duplicate of another
bug. Ensure that email is sent to subscribers for the original bug's
duplicates.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py
lib/lp/bugs/adapters/bugchange.py
lib/lp/bugs/model/bugnotification.py
lib/lp/bugs/configure.zcml
lib/lp/bugs/model/tests/test_bug.py
lib/lp/bugs/tests/bug.py
lib/lp/bugs/enum.py
lib/lp/bugs/tests/test_bugnotification.py
database/schema/security.cfg
lib/lp/bugs/browser/bug.py
cronscripts/send-bug-notifications.py
lib/lp/bugs/scripts/tests/test_bugnotification.py
lib/lp/bugs/model/bug.py
lib/lp/bugs/scripts/bugnotification.py
lib/lp/bugs/interfaces/bugnotification.py
./cronscripts/send-bug-notifications.py
16: '_pythonpath' imported but unused
--
https://code.launchpad.net/~bac/launchpad/bug-813322-2/+merge/71733
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-813322-2 into lp:launchpad.
=== modified file 'cronscripts/send-bug-notifications.py'
--- cronscripts/send-bug-notifications.py 2011-08-12 14:49:34 +0000
+++ cronscripts/send-bug-notifications.py 2011-08-16 16:28:25 +0000
@@ -22,15 +22,22 @@
from lp.services.mail.sendmail import sendmail
from lp.bugs.enum import BugNotificationStatus
from lp.bugs.interfaces.bugnotification import IBugNotificationSet
-from lp.bugs.scripts.bugnotification import get_email_notifications
+from lp.bugs.scripts.bugnotification import (
+ get_email_notifications,
+ process_deferred_notifications,
+ )
from lp.services.scripts.base import LaunchpadCronScript
class SendBugNotifications(LaunchpadCronScript):
def main(self):
notifications_sent = False
- pending_notifications = get_email_notifications(getUtility(
- IBugNotificationSet).getNotificationsToSend())
+ bug_notification_set = getUtility(IBugNotificationSet)
+ deferred_notifications = \
+ bug_notification_set.getDeferredNotifications()
+ process_deferred_notifications(deferred_notifications)
+ pending_notifications = get_email_notifications(
+ bug_notification_set.getNotificationsToSend())
for (bug_notifications,
omitted_notifications,
messages) in pending_notifications:
@@ -59,4 +66,3 @@
script = SendBugNotifications('send-bug-notifications',
dbuser=config.malone.bugnotification_dbuser)
script.lock_and_run()
-
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2011-08-10 15:57:35 +0000
+++ database/schema/security.cfg 2011-08-16 16:28:25 +0000
@@ -1497,7 +1497,7 @@
public.bugmessage = SELECT, INSERT
public.bugmute = SELECT
public.bugnomination = SELECT
-public.bugnotification = SELECT, INSERT, UPDATE
+public.bugnotification = SELECT, INSERT, UPDATE, DELETE
public.bugnotificationfilter = SELECT, INSERT
public.bugnotificationrecipient = SELECT, INSERT, UPDATE
public.bugsubscription = SELECT, INSERT
=== modified file 'lib/lp/bugs/adapters/bugchange.py'
--- lib/lp/bugs/adapters/bugchange.py 2011-07-18 03:05:04 +0000
+++ lib/lp/bugs/adapters/bugchange.py 2011-08-16 16:28:25 +0000
@@ -36,6 +36,7 @@
'BugWatchRemoved',
'CveLinkedToBug',
'CveUnlinkedFromBug',
+ 'DeferredBugDuplicateChange',
'SeriesNominated',
'UnsubscribedFromBug',
'get_bug_change_class',
@@ -489,6 +490,91 @@
return {'text': text}
+class DeferredBugDuplicateChange(AttributeChange):
+ """Describes a change to a bug's duplicate marker.
+
+ This change is merely queued up without a recipients list given. The
+ post-processing job is responsible for computing the recipients list and
+ then sending the notifications.
+
+ The deferral may be fragile if multiple changes are made to the bug's
+ duplicate status before the post-processor runs, so checks must be
+ performed to ensure the state is still as expected.
+ """
+ def __init__(self, when, person, what_changed,
+ old_value, new_value):
+ super(DeferredBugDuplicateChange, self).__init__(
+ when, person, what_changed, old_value, new_value)
+
+ def getBugActivity(self):
+ if self.old_value is not None and self.new_value is not None:
+ return {
+ 'whatchanged': CHANGED_DUPLICATE_MARKER,
+ 'oldvalue': str(self.old_value.id),
+ 'newvalue': str(self.new_value.id),
+ }
+ elif self.old_value is None:
+ return {
+ 'whatchanged': MARKED_AS_DUPLICATE,
+ 'newvalue': str(self.new_value.id),
+ }
+ elif self.new_value is None:
+ return {
+ 'whatchanged': REMOVED_DUPLICATE_MARKER,
+ 'oldvalue': str(self.old_value.id),
+ }
+ else:
+ raise AssertionError(
+ "There is no change: both the old bug and new bug are None.")
+
+ def getBugNotification(self):
+ if self.old_value is not None and self.new_value is not None:
+ if self.old_value.private:
+ old_value_text = (
+ "** This bug is no longer a duplicate of private bug "
+ "%d" % self.old_value.id)
+ else:
+ old_value_text = (
+ "** This bug is no longer a duplicate of bug %d\n"
+ " %s" % (self.old_value.id, self.old_value.title))
+ if self.new_value.private:
+ new_value_text = (
+ "** This bug has been marked a duplicate of private bug "
+ "%d" % self.new_value.id)
+ else:
+ new_value_text = (
+ "** This bug has been marked a duplicate of bug %d\n"
+ " %s" % (self.new_value.id, self.new_value.title))
+
+ text = "\n".join((old_value_text, new_value_text))
+
+ elif self.old_value is None:
+ if self.new_value.private:
+ text = (
+ "** This bug has been marked a duplicate of private bug "
+ "%d" % self.new_value.id)
+ else:
+ text = (
+ "** This bug has been marked a duplicate of bug %d\n"
+ " %s" % (self.new_value.id, self.new_value.title))
+
+ elif self.new_value is None:
+ if self.old_value.private:
+ text = (
+ "** This bug is no longer a duplicate of private bug "
+ "%d" % self.old_value.id)
+ else:
+ text = (
+ "** This bug is no longer a duplicate of bug %d\n"
+ " %s" % (self.old_value.id, self.old_value.title))
+
+ else:
+ raise AssertionError(
+ "There is no change: both the old bug and new bug are None.")
+
+ return {'text': text}
+
+
class BugTitleChange(AttributeChange):
"""Describes a change to a bug's title, aka summary."""
@@ -622,7 +708,6 @@
download_url_of_bugattachment(self.new_value))
else:
what_changed = ATTACHMENT_REMOVED
- attachment = self.new_value
old_value = "%s %s" % (
self.old_value.title,
download_url_of_bugattachment(self.old_value))
=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py 2011-06-16 13:50:58 +0000
+++ lib/lp/bugs/browser/bug.py 2011-08-16 16:28:25 +0000
@@ -453,19 +453,19 @@
def subscription_info(self):
return IBug(self.context).getSubscriptionInfo()
- @property
+ @cachedproperty
def direct_subscribers(self):
"""Return the list of direct subscribers."""
- return self.subscription_info.direct_subscriptions.subscribers
+ return self.subscription_info.direct_subscribers
- @property
+ @cachedproperty
def duplicate_subscribers(self):
"""Return the list of subscribers from duplicates.
This includes all subscribers who are also direct or indirect
subscribers.
"""
- return self.subscription_info.duplicate_subscriptions.subscribers
+ return self.subscription_info.duplicate_subscribers
def getSubscriptionClassForUser(self, subscribed_person):
"""Return a set of CSS class names based on subscription status.
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2011-08-01 06:59:40 +0000
+++ lib/lp/bugs/configure.zcml 2011-08-16 16:28:25 +0000
@@ -1111,6 +1111,9 @@
<require
permission="launchpad.AnyPerson"
set_schema="lp.bugs.interfaces.bugnotification.IBugNotification"/>
+ <require
+ permission="launchpad.Edit"
+ attributes="destroySelf" />
</class>
<class
class="lp.bugs.model.bugnotification.BugNotificationRecipient">
=== modified file 'lib/lp/bugs/enum.py'
--- lib/lp/bugs/enum.py 2011-05-16 16:57:55 +0000
+++ lib/lp/bugs/enum.py 2011-08-16 16:28:25 +0000
@@ -68,3 +68,10 @@
The notification has been sent.
""")
+
+ DEFERRED = DBItem(40, """
+ Deferred
+
+ The notification is deferred. The recipient list was not calculated
+ at creation time but is done when processed.
+ """)
=== modified file 'lib/lp/bugs/interfaces/bugnotification.py'
--- lib/lp/bugs/interfaces/bugnotification.py 2011-05-22 23:43:39 +0000
+++ lib/lp/bugs/interfaces/bugnotification.py 2011-08-16 16:28:25 +0000
@@ -73,6 +73,12 @@
def getNotificationsToSend():
"""Returns the notifications pending to be sent."""
+ def getDeferredNotifications():
+ """Returns the deferred notifications.
+
+ A deferred noticiation is one that is pending but has no recipients.
+ """
+
def addNotification(self, bug, is_comment, message, recipients, activity):
"""Create a new `BugNotification`.
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2011-08-01 05:56:43 +0000
+++ lib/lp/bugs/model/bug.py 2011-08-16 16:28:25 +0000
@@ -106,6 +106,7 @@
from canonical.launchpad.webapp.authorization import check_permission
from canonical.launchpad.webapp.interfaces import (
DEFAULT_FLAVOR,
+ ILaunchBag,
IStoreSelector,
MAIN_STORE,
)
@@ -123,6 +124,7 @@
BugConvertedToQuestion,
BugWatchAdded,
BugWatchRemoved,
+ DeferredBugDuplicateChange,
SeriesNominated,
UnsubscribedFromBug,
)
@@ -929,11 +931,12 @@
"""
if level is None:
level = BugNotificationLevel.LIFECYCLE
- subscriptions = self.getSubscriptionInfo(level).direct_subscriptions
+ direct_subscribers = (
+ self.getSubscriptionInfo(level).direct_subscribers)
if recipients is not None:
- for subscriber in subscriptions.subscribers:
+ for subscriber in direct_subscribers:
recipients.addDirectSubscriber(subscriber)
- return subscriptions.subscribers.sorted
+ return direct_subscribers.sorted
def getDirectSubscribersWithDetails(self):
"""See `IBug`."""
@@ -999,7 +1002,6 @@
for subscription in info.duplicate_only_subscriptions:
recipients.addDupeSubscriber(
subscription.person, subscription.bug)
-
return info.duplicate_only_subscriptions.subscribers.sorted
def getSubscribersForPerson(self, person):
@@ -1099,7 +1101,7 @@
bug=self, is_comment=True,
message=message, recipients=recipients, activity=activity)
- def addChange(self, change, recipients=None):
+ def addChange(self, change, recipients=None, deferred=False):
"""See `IBug`."""
when = change.when
if when is None:
@@ -1128,7 +1130,8 @@
level=BugNotificationLevel.METADATA)
getUtility(IBugNotificationSet).addNotification(
bug=self, is_comment=False, message=message,
- recipients=recipients, activity=activity)
+ recipients=recipients, activity=activity,
+ deferred=deferred)
self.updateHeat()
@@ -1411,7 +1414,6 @@
question = question_target.createQuestionFromBug(self)
self.addChange(BugConvertedToQuestion(UTC_NOW, person, question))
get_property_cache(self)._question_from_bug = question
-
notify(BugBecameQuestionEvent(self, question, person))
return question
@@ -1823,16 +1825,23 @@
if duplicate_of is not None:
field._validate(duplicate_of)
if self.duplicates:
+ user = getUtility(ILaunchBag).user
for duplicate in self.duplicates:
- # Fire a notify event in model code since moving
- # duplicates of a duplicate does not normally fire an
- # event.
- dupe_before = Snapshot(
- duplicate, providing=providedBy(duplicate))
+ old_value = duplicate.duplicateof
affected_targets.update(
duplicate._markAsDuplicate(duplicate_of))
- notify(ObjectModifiedEvent(
- duplicate, dupe_before, 'duplicateof'))
+
+ # Put an entry into the BugNotification table for
+ # later processing.
+ change = DeferredBugDuplicateChange(
+ when=None, person=user,
+ what_changed='duplicateof',
+ old_value=old_value,
+ new_value=duplicate_of)
+ empty_recipients = BugNotificationRecipients()
+ duplicate.addChange(
+ change, empty_recipients, deferred=True)
+
self.duplicateof = duplicate_of
except LaunchpadValidationError, validation_error:
raise InvalidDuplicateValue(validation_error)
@@ -2287,7 +2296,7 @@
@cachedproperty
@freeze(BugSubscriptionSet)
- def direct_subscriptions(self):
+ def old_direct_subscriptions(self):
"""The bug's direct subscriptions."""
return IStore(BugSubscription).find(
BugSubscription,
@@ -2297,19 +2306,57 @@
Select(BugMute.person_id, BugMute.bug_id == self.bug.id))))
@cachedproperty
+ def direct_subscriptions_and_subscribers(self):
+ """The bug's direct subscriptions."""
+ res = IStore(BugSubscription).find(
+ (BugSubscription, Person),
+ BugSubscription.bug_notification_level >= self.level,
+ BugSubscription.bug == self.bug,
+ BugSubscription.person_id == Person.id,
+ Not(In(BugSubscription.person_id,
+ Select(BugMute.person_id,
+ BugMute.bug_id == self.bug.id))))
+ # Here we could test for res.count() but that will execute another
+ # query. This structure avoids the extra query.
+ return zip(*res) or ((), ())
+
+ @cachedproperty
@freeze(BugSubscriptionSet)
- def duplicate_subscriptions(self):
+ def direct_subscriptions(self):
+ return self.direct_subscriptions_and_subscribers[0]
+
+ @cachedproperty
+ @freeze(BugSubscriberSet)
+ def direct_subscribers(self):
+ return self.direct_subscriptions_and_subscribers[1]
+
+ @cachedproperty
+ def duplicate_subscriptions_and_subscribers(self):
"""Subscriptions to duplicates of the bug."""
if self.bug.private:
- return ()
+ return ((), ())
else:
- return IStore(BugSubscription).find(
- BugSubscription,
+ res = IStore(BugSubscription).find(
+ (BugSubscription, Person),
BugSubscription.bug_notification_level >= self.level,
BugSubscription.bug_id == Bug.id,
+ BugSubscription.person_id == Person.id,
Bug.duplicateof == self.bug,
Not(In(BugSubscription.person_id,
Select(BugMute.person_id, BugMute.bug_id == Bug.id))))
+ # Here we could test for res.count() but that will execute another
+ # query. This structure avoids the extra query.
+ return zip(*res) or ((), ())
+
+ @cachedproperty
+ @freeze(BugSubscriptionSet)
+ def duplicate_subscriptions(self):
+ return self.duplicate_subscriptions_and_subscribers[0]
+
+ @cachedproperty
+ @freeze(BugSubscriberSet)
+ def duplicate_subscribers(self):
+ return self.duplicate_subscriptions_and_subscribers[1]
@cachedproperty
@freeze(BugSubscriptionSet)
@@ -2319,9 +2366,9 @@
Excludes subscriptions for people who have a direct subscription or
are also notified for another reason.
"""
- self.duplicate_subscriptions.subscribers # Pre-load subscribers.
+ self.duplicate_subscribers # Pre-load subscribers.
higher_precedence = (
- self.direct_subscriptions.subscribers.union(
+ self.direct_subscribers.union(
self.also_notified_subscribers))
return (
subscription for subscription in self.duplicate_subscriptions
@@ -2363,13 +2410,13 @@
self.structural_subscriptions.subscribers,
self.all_pillar_owners_without_bug_supervisors,
self.all_assignees).difference(
- self.direct_subscriptions.subscribers).difference(muted)
+ self.direct_subscribers).difference(muted)
@cachedproperty
def indirect_subscribers(self):
"""All subscribers except direct subscribers."""
return self.also_notified_subscribers.union(
- self.duplicate_subscriptions.subscribers)
+ self.duplicate_subscribers)
class BugSet:
=== modified file 'lib/lp/bugs/model/bugnotification.py'
--- lib/lp/bugs/model/bugnotification.py 2011-05-23 09:13:32 +0000
+++ lib/lp/bugs/model/bugnotification.py 2011-08-16 16:28:25 +0000
@@ -110,18 +110,19 @@
store = IStore(BugNotification)
source = store.using(BugNotification,
Join(Message,
- BugNotification.message==Message.id),
+ BugNotification.message == Message.id),
LeftJoin(
BugActivity,
- BugNotification.activity==BugActivity.id))
+ BugNotification.activity == BugActivity.id))
results = list(source.find(
(BugNotification, BugActivity, Message),
+ BugNotification.status == BugNotificationStatus.PENDING,
BugNotification.date_emailed == None).order_by(
'BugNotification.bug', '-BugNotification.id'))
interval = timedelta(
minutes=int(config.malone.bugnotification_interval))
time_limit = (
- datetime.now(pytz.timezone('UTC')) - interval)
+ datetime.now(pytz.UTC) - interval)
last_omitted_notification = None
pending_notifications = []
people_ids = set()
@@ -141,7 +142,7 @@
pending_notifications.append(notification)
people_ids.add(notification.message.ownerID)
bug_ids.add(notification.bugID)
- # Now we do some calls that are purely for cacheing.
+ # Now we do some calls that are purely for caching.
# Converting these into lists forces the queries to execute.
if pending_notifications:
list(
@@ -154,13 +155,29 @@
pending_notifications.reverse()
return pending_notifications
- def addNotification(self, bug, is_comment, message, recipients, activity):
+ def getDeferredNotifications(self):
+ """See `IBugNoticationSet`."""
+ store = IStore(BugNotification)
+ results = store.find(
+ BugNotification,
+ BugNotification.date_emailed == None,
+ BugNotification.status == BugNotificationStatus.DEFERRED)
+ return results
+
+ def addNotification(self, bug, is_comment, message, recipients, activity,
+ deferred=False):
"""See `IBugNotificationSet`."""
- if not recipients:
- return
+ if deferred:
+ status = BugNotificationStatus.DEFERRED
+ else:
+ if not recipients:
+ return
+ status = BugNotificationStatus.PENDING
+
bug_notification = BugNotification(
bug=bug, is_comment=is_comment,
- message=message, date_emailed=None, activity=activity)
+ message=message, date_emailed=None, activity=activity,
+ status=status)
store = Store.of(bug_notification)
# XXX jamesh 2008-05-21: these flushes are to fix ordering
# problems in the bugnotification-sending.txt tests.
@@ -173,19 +190,20 @@
# We add all the recipients in a single SQL statement to make
# this a bit more efficient for bugs with many subscribers.
- store.execute("""
- INSERT INTO BugNotificationRecipient
- (bug_notification, person, reason_header, reason_body)
- VALUES %s;""" % ', '.join(sql_values))
-
- if len(recipients.subscription_filters) > 0:
- filter_link_sql = [
- "(%s, %s)" % sqlvalues(bug_notification, filter.id)
- for filter in recipients.subscription_filters]
+ if len(sql_values) > 0:
store.execute("""
- INSERT INTO BugNotificationFilter
- (bug_notification, bug_subscription_filter)
- VALUES %s;""" % ", ".join(filter_link_sql))
+ INSERT INTO BugNotificationRecipient
+ (bug_notification, person, reason_header, reason_body)
+ VALUES %s;""" % ', '.join(sql_values))
+
+ if len(recipients.subscription_filters) > 0:
+ filter_link_sql = [
+ "(%s, %s)" % sqlvalues(bug_notification, filter.id)
+ for filter in recipients.subscription_filters]
+ store.execute("""
+ INSERT INTO BugNotificationFilter
+ (bug_notification, bug_subscription_filter)
+ VALUES %s;""" % ", ".join(filter_link_sql))
return bug_notification
@@ -257,9 +275,12 @@
source_person_id_map[source_person_id]['filters'][filter_id] = (
filter_description)
filter_ids.append(filter_id)
- no_filter_marker = -1 # This is only necessary while production and
- # sample data have structural subscriptions without filters.
- # Assign the filters to each recipient.
+
+ # This is only necessary while production and sample data have
+ # structural subscriptions without filters. Assign the filters to
+ # each recipient.
+ no_filter_marker = -1
+
for recipient_data in recipient_id_map.values():
for source_person_id in recipient_data['source person ids']:
recipient_data['filters'].update(
@@ -292,7 +313,7 @@
filter_descriptions = [
description for description
in recipient_data['filters'].values() if description]
- filter_descriptions.sort() # This is good for tests.
+ filter_descriptions.sort() # This is good for tests.
result[recipient_data['principal']] = {
'sources': recipient_data['sources'],
'filter descriptions': filter_descriptions}
=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py 2011-07-29 18:49:10 +0000
+++ lib/lp/bugs/model/tests/test_bug.py 2011-08-16 16:28:25 +0000
@@ -1,14 +1,23 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
+from storm.store import Store
from testtools.testcase import ExpectedException
+from zope.component import getUtility
from canonical.testing.layers import DatabaseFunctionalLayer
-from lp.bugs.enum import BugNotificationLevel
+from lp.bugs.enum import (
+ BugNotificationLevel,
+ BugNotificationStatus,
+ )
+from lp.bugs.interfaces.bugnotification import IBugNotificationSet
from lp.bugs.interfaces.bugtask import BugTaskStatus
-from lp.bugs.model.bug import BugSubscriptionInfo
+from lp.bugs.model.bug import (
+ BugNotification,
+ BugSubscriptionInfo,
+ )
from lp.registry.interfaces.person import PersonVisibility
from lp.testing import (
feature_flags,
@@ -19,6 +28,7 @@
TestCaseWithFactory,
)
from lp.testing.matchers import (
+ Equals,
HasQueryCount,
LessThan,
)
@@ -132,6 +142,101 @@
bug.subscribe(team, member)
self.assertTrue(team in bug.getDirectSubscribers())
+ def test_get_direct_subscribers_query_count(self):
+ bug = self.factory.makeBug()
+ # Make lots of subscribers.
+ for i in xrange(10):
+ subscriber = self.factory.makePerson()
+ with person_logged_in(subscriber):
+ bug.subscribe(subscriber, subscriber)
+ Store.of(bug).flush()
+ with StormStatementRecorder() as recorder:
+ subscribers = list(bug.getDirectSubscribers())
+ self.assertThat(len(subscribers), Equals(10 + 1))
+ self.assertThat(recorder, HasQueryCount(Equals(1)))
+
+ def test_mark_as_duplicate_query_count(self):
+ bug = self.factory.makeBug()
+ # Make lots of duplicate bugs.
+ previous_dup = None
+ for i in xrange(10):
+ dup = self.factory.makeBug()
+ # Make lots of subscribers.
+ for j in xrange(10):
+ subscriber = self.factory.makePerson()
+ with person_logged_in(subscriber):
+ dup.subscribe(subscriber, subscriber)
+ if previous_dup is not None:
+ with person_logged_in(previous_dup.owner):
+ previous_dup.markAsDuplicate(dup)
+ previous_dup = dup
+ with person_logged_in(bug.owner):
+ Store.of(bug).flush()
+ with StormStatementRecorder() as recorder:
+ previous_dup.markAsDuplicate(bug)
+ self.assertThat(recorder, HasQueryCount(LessThan(95)))
+
+ def _get_notifications(self, status):
+ return self.store.find(
+ BugNotification,
+ BugNotification.date_emailed == None,
+ BugNotification.status == status)
+
+ def _get_pending(self):
+ return self._get_notifications(BugNotificationStatus.PENDING)
+
+ def _get_deferred(self):
+ return self._get_notifications(BugNotificationStatus.DEFERRED)
+
+ def _add_subscribers(self, bug, number):
+ for i in xrange(number):
+ subscriber = self.factory.makePerson()
+ with person_logged_in(subscriber):
+ bug.subscribe(subscriber, subscriber)
+
+ def test_duplicate_subscriber_notifications(self):
+ # Notifications for duplicate bugs are deferred where notifications
+ # for direct subscribers of the original bug are pending.
+ bug = self.factory.makeBug(title="bug-0")
+ self._add_subscribers(bug, 3)
+ self.store = Store.of(bug)
+ duplicates = []
+ # Make a few duplicate bugs.
+ for i in xrange(3):
+ duplicates.append(self.factory.makeBug(title="bug-%d" % (i + 1)))
+
+ # Pending messages exist for the bug creation.
+ self.assertEqual(4, self._get_pending().count())
+ self.assertEqual(0, self._get_deferred().count())
+
+ previous_dup = None
+ for dup in duplicates:
+ # Make a few subscribers.
+ self._add_subscribers(dup, 3)
+ if previous_dup is not None:
+ with person_logged_in(previous_dup.owner):
+ previous_dup.markAsDuplicate(dup)
+ previous_dup = dup
+
+ # Pending messages are still all from bug creation.
+ # Only one deferred notification has been created, since notices for
+ # the primary bug are not deferred and are created by the calling
+ # process (browser or API).
+ self.assertEqual(4, self._get_pending().count())
+ self.assertEqual(1, self._get_deferred().count())
+
+ with person_logged_in(bug.owner):
+ previous_dup.markAsDuplicate(bug)
+
+ # Now there are two new deferred messages, for the duplicates to the
+ # last bug.
+ self.assertEqual(4, self._get_pending().count())
+ self.assertEqual(3, self._get_deferred().count())
+
+ # The method for retrieving deferred notification reports them all.
+ deferred = getUtility(IBugNotificationSet).getDeferredNotifications()
+ self.assertEqual(3, deferred.count())
+
def test_get_subscribers_from_duplicates_with_private_team(self):
product = self.factory.makeProduct()
bug = self.factory.makeBug(product=product)
@@ -350,7 +455,7 @@
bug.setPrivate(True, person)
self.assertFalse(bug.personIsDirectSubscriber(person))
- def test_getVisibleLinkedBranches_doesnt_return_inaccessible_branches(self):
+ def test_getVisibleLinkedBranches_doesnt_rtn_inaccessible_branches(self):
# If a Bug has branches linked to it that the current user
# cannot access, those branches will not be returned in its
# linked_branches property.
=== modified file 'lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py'
--- lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py 2011-06-07 06:24:04 +0000
+++ lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py 2011-08-16 16:28:25 +0000
@@ -527,7 +527,7 @@
self.info.all_pillar_owners_without_bug_supervisors
def test_also_notified_subscribers(self):
- with self.exactly_x_queries(6):
+ with self.exactly_x_queries(5):
self.info.also_notified_subscribers
def test_also_notified_subscribers_later(self):
@@ -541,7 +541,7 @@
self.info.also_notified_subscribers
def test_indirect_subscribers(self):
- with self.exactly_x_queries(7):
+ with self.exactly_x_queries(6):
self.info.indirect_subscribers
def test_indirect_subscribers_later(self):
=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py 2011-06-15 11:19:21 +0000
+++ lib/lp/bugs/scripts/bugnotification.py 2011-08-16 16:28:25 +0000
@@ -10,11 +10,13 @@
__all__ = [
"construct_email_notifications",
"get_email_notifications",
+ "process_deferred_notifications",
]
from itertools import groupby
from operator import itemgetter
+from storm.store import Store
import transaction
from zope.component import getUtility
@@ -25,6 +27,7 @@
BugNotificationBuilder,
get_bugmail_from_address,
)
+from lp.bugs.enum import BugNotificationLevel
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
@@ -296,3 +299,32 @@
log.exception("Error while building email notifications.")
transaction.abort()
transaction.begin()
+
+
+def process_deferred_notifications(bug_notifications):
+ """Transform deferred notifications into real ones.
+
+ Deferred notifications must have their recipients list calculated and then
+ re-inserted as real notifications.
+ """
+ bug_notification_set = getUtility(IBugNotificationSet)
+ for notification in bug_notifications:
+ # Construct the real notification with recipients.
+ bug = notification.bug
+ recipients = bug.getBugNotificationRecipients(
+ level=BugNotificationLevel.LIFECYCLE,
+ include_master_dupe_subscribers=False)
+ message = notification.message
+ is_comment = notification.is_comment
+ activity = notification.activity
+ # Remove the deferred notification.
+ # Is activity affected?
+ store = Store.of(notification)
+ notification.destroySelf()
+ store.flush()
+ bug_notification_set.addNotification(
+ bug=bug,
+ is_comment=is_comment,
+ message=message,
+ recipients=recipients,
+ activity=activity)
=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
--- lib/lp/bugs/scripts/tests/test_bugnotification.py 2011-06-15 11:19:21 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py 2011-08-16 16:28:25 +0000
@@ -66,6 +66,7 @@
get_activity_key,
notification_batches,
notification_comment_batches,
+ process_deferred_notifications,
)
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.product import IProductSet
@@ -600,7 +601,7 @@
commit()
login('test@xxxxxxxxxxxxx')
self.layer.switchDbUser(config.malone.bugnotification_dbuser)
- self.now = datetime.now(pytz.timezone('UTC'))
+ self.now = datetime.now(pytz.UTC)
self.ten_minutes_ago = self.now - timedelta(minutes=10)
self.notification_set = getUtility(IBugNotificationSet)
for notification in self.notification_set.getNotificationsToSend():
@@ -1190,3 +1191,41 @@
for name in names:
template = get_email_template(name, 'bugs')
self.assertTrue(re.search('^-- $', template, re.MULTILINE))
+
+
+class TestDeferredNotifications(TestCaseWithFactory):
+
+ layer = LaunchpadZopelessLayer
+
+ def setUp(self):
+ super(TestDeferredNotifications, self).setUp()
+ self.notification_set = getUtility(IBugNotificationSet)
+ # Ensure there are no outstanding notifications.
+ for notification in self.notification_set.getNotificationsToSend():
+ notification.destroySelf()
+ self.ten_minutes_ago = datetime.now(pytz.UTC) - timedelta(minutes=10)
+
+ def _make_deferred_notification(self):
+ bug = self.factory.makeBug()
+ empty_recipients = BugNotificationRecipients()
+ message = getUtility(IMessageSet).fromText(
+ 'subject', 'a comment.', bug.owner,
+ datecreated=self.ten_minutes_ago)
+ self.notification_set.addNotification(
+ bug, False, message, empty_recipients, None, deferred=True)
+
+ def test_deferred_notifications(self):
+ # Create some deferred notifications and show that processing them
+ # puts then in the state where they are ready to send.
+ num = 5
+ for i in xrange(num):
+ self._make_deferred_notification()
+ deferred = self.notification_set.getDeferredNotifications()
+ self.assertEqual(num, deferred.count())
+ process_deferred_notifications(deferred)
+ # Now that are all in the PENDING state.
+ ready_to_send = self.notification_set.getNotificationsToSend()
+ self.assertEqual(num, len(ready_to_send))
+ # And there are no longer any deferred.
+ deferred = self.notification_set.getDeferredNotifications()
+ self.assertEqual(0, deferred.count())
=== modified file 'lib/lp/bugs/tests/bug.py'
--- lib/lp/bugs/tests/bug.py 2011-08-01 05:56:43 +0000
+++ lib/lp/bugs/tests/bug.py 2011-08-16 16:28:25 +0000
@@ -200,7 +200,7 @@
:title: A string. The bug title for testing.
:days_old: An int. The bug's age in days.
- :target: A BugTarkget. The bug's target.
+ :target: A BugTarget. The bug's target.
:status: A BugTaskStatus. The status of the bug's single bugtask.
:with_message: A Bool. Whether to create a reply message.
:external_bugtracker: An external bug tracker which is watched for this
=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
--- lib/lp/bugs/tests/test_bugnotification.py 2011-07-22 00:07:01 +0000
+++ lib/lp/bugs/tests/test_bugnotification.py 2011-08-16 16:28:25 +0000
@@ -6,6 +6,8 @@
__metaclass__ = type
from itertools import chain
+from datetime import datetime
+import pytz
import transaction
import unittest
@@ -13,6 +15,7 @@
from lazr.lifecycle.snapshot import Snapshot
from storm.store import Store
from testtools.matchers import Not
+from zope.component import getUtility
from zope.event import notify
from zope.interface import providedBy
@@ -30,12 +33,14 @@
BugTaskStatus,
IBugTask,
)
+from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients
from lp.bugs.model.bugnotification import (
BugNotification,
BugNotificationFilter,
BugNotificationSet,
)
from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilterMute
+from lp.services.messages.interfaces.message import IMessageSet
from lp.testing import (
TestCaseWithFactory,
person_logged_in,
@@ -640,3 +645,47 @@
{team.teamowner: [notification.recipients[0]],
team: [notification.recipients[1]]},
[notification]))
+
+
+class TestGetDeferredNotifications(TestCaseWithFactory):
+ """Test the getDeferredNotifications method."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestGetDeferredNotifications, self).setUp()
+ self.bns = BugNotificationSet()
+
+ def test_no_deferred_notifications(self):
+ results = self.bns.getDeferredNotifications()
+ self.assertEqual(0, results.count())
+
+ def _make_deferred_notification(self):
+ bug = self.factory.makeBug()
+ empty_recipients = BugNotificationRecipients()
+ message = getUtility(IMessageSet).fromText(
+ 'subject', 'a comment.', bug.owner,
+ datecreated=datetime.now(pytz.UTC))
+ self.bns.addNotification(
+ bug, False, message, empty_recipients, None, deferred=True)
+
+ def test_one_deferred_notification(self):
+ self._make_deferred_notification()
+ results = self.bns.getDeferredNotifications()
+ self.assertEqual(1, results.count())
+
+ def test_many_deferred_notification(self):
+ num = 5
+ for i in xrange(num):
+ self._make_deferred_notification()
+ results = self.bns.getDeferredNotifications()
+ self.assertEqual(num, results.count())
+
+ def test_destroy_notifications(self):
+ self._make_deferred_notification()
+ results = self.bns.getDeferredNotifications()
+ self.assertEqual(1, results.count())
+ notification = results[0]
+ notification.destroySelf()
+ results = self.bns.getDeferredNotifications()
+ self.assertEqual(0, results.count())
