launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/bug-720826-links into lp:launchpad/db-devel with lp:~danilo/launchpad/bug-720826-emails as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #720826 Add subscription description header for bug notifications
  https://bugs.launchpad.net/bugs/720826

For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-720826-links/+merge/52226

= Bug 720826 =

This finally ties everything up together: we start adding links to
subscription filters that have caused a certain BugNotification to
go off.

Previously, framework for this was provided, and if these links were
there, appropriate email headers and content was added to outgoing
emails.

== Proposed fix ==

 * Extend BugNotificationRecipients to allow holding relevant filters
   along with recipients and reasons they are getting emails
 * Find out what those filters are and add them to the `recipients`
   in structuralsubscription._get_structural_subscribers
 * In BugNotificationSet.addNotification(), we insert appropriate
   linking rows for all the filters present in `recipients`.

== Implementation details ==

BugNotificationRecipients is a structure that is passed around.  IMHO, it is somewhat nasty that `recipients` is passed around and updated as a side-effect in a few methods which are called get*, but because it already is, I am abusing it to pass the subscription filters list as well (especially since I can easily calculate them at the same time all the other recipients stuff is calculated).

I am only providing the integration test in bugnotification-sending.txt.

== Tests ==

bin/test -cvvt bugnotification-sending.txt

== Demo and Q/A ==

Add a few subscription filters, and run "cronscripts/send-bug-notifications.py -vvv" and watch for the X-Launchpad-Subscription-Filter and "Matching filters" line.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/bugnotification.py
  lib/lp/bugs/mail/bugnotificationrecipients.py
  lib/lp/bugs/model/tests/test_bugtask.py
  lib/lp/bugs/doc/bugnotification-sending.txt
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/tests/test_structuralsubscriptiontarget.py
  lib/lp/bugs/model/structuralsubscription.py
  lib/lp/bugs/model/bugtask.py

./lib/lp/bugs/model/bugnotification.py
     132: E202 whitespace before ']'
./lib/lp/bugs/doc/bugnotification-sending.txt
     433: want has trailing whitespace.
     451: want has trailing whitespace.
-- 
https://code.launchpad.net/~danilo/launchpad/bug-720826-links/+merge/52226
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-720826-links into lp:launchpad/db-devel.

=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
--- lib/lp/bugs/doc/bugnotification-sending.txt	2011-02-22 10:44:48 +0000
+++ lib/lp/bugs/doc/bugnotification-sending.txt	2011-03-04 17:17:08 +0000
@@ -21,8 +21,10 @@
 
     >>> def print_notification_headers(email_notification):
     ...     for header in ['To', 'From', 'Subject',
-    ...                    'X-Launchpad-Message-Rationale']:
-    ...         print "%s: %s" % (header, email_notification[header])
+    ...                    'X-Launchpad-Message-Rationale',
+    ...                    'X-Launchpad-Subscription-Filter']:
+    ...         if email_notification[header]:
+    ...             print "%s: %s" % (header, email_notification[header])
 
     >>> def print_notification(email_notification):
     ...     print_notification_headers(email_notification)
@@ -1247,6 +1249,7 @@
     >>> with lp_dbuser():
     ...     filter = subscription_no_priv.newBugFilter()
     ...     filter.bug_notification_level = BugNotificationLevel.COMMENTS
+    ...     filter.description = u"Allow-comments filter"
 
     >>> comment = getUtility(IMessageSet).fromText(
     ...     'subject', 'another comment.', sample_person,
@@ -1279,12 +1282,14 @@
     From: Sample Person <...@bugs.launchpad.net>
     Subject: [Bug 1] subject
     X-Launchpad-Message-Rationale: Subscriber (Mozilla Firefox)
+    X-Launchpad-Subscription-Filter: Allow-comments filter
     <BLANKLINE>
     another comment.
     <BLANKLINE>
     --
     You received this bug notification because you are subscribed to Mozilla
     Firefox.
+    Matching filters: Allow-comments filter
     ...
     ----------------------------------------------------------------------
     To: support@xxxxxxxxxx
@@ -1390,6 +1395,7 @@
     From: Sample Person <...@bugs.launchpad.net>
     Subject: [Bug 1] subject
     X-Launchpad-Message-Rationale: Subscriber (Mozilla Firefox)
+    X-Launchpad-Subscription-Filter: Allow-comments filter
     <BLANKLINE>
     no comment for no-priv.
     <BLANKLINE>
@@ -1400,6 +1406,7 @@
     --
     You received this bug notification because you are subscribed to Mozilla
     Firefox.
+    Matching filters: Allow-comments filter
     ...
     ----------------------------------------------------------------------
     To: support@xxxxxxxxxx

=== modified file 'lib/lp/bugs/mail/bugnotificationrecipients.py'
--- lib/lp/bugs/mail/bugnotificationrecipients.py	2011-03-02 14:07:23 +0000
+++ lib/lp/bugs/mail/bugnotificationrecipients.py	2011-03-04 17:17:08 +0000
@@ -58,6 +58,7 @@
         """
         NotificationRecipientSet.__init__(self)
         self.duplicateof = duplicateof
+        self.subscription_filters = set()
 
     def _addReason(self, person, reason, header):
         """Adds a reason (text and header) for a person.
@@ -127,3 +128,13 @@
         else:
             text = "are the registrant for %s" % upstream.displayname
         self._addReason(person, text, reason)
+
+    def update(self, recipient_set):
+        """See `INotificationRecipientSet`."""
+        super(BugNotificationRecipients, self).update(recipient_set)
+        self.subscription_filters.update(
+            recipient_set.subscription_filters)
+
+    def addFilter(self, subscription_filter):
+        if subscription_filter is not None:
+            self.subscription_filters.add(subscription_filter)

=== modified file 'lib/lp/bugs/model/bugnotification.py'
--- lib/lp/bugs/model/bugnotification.py	2011-02-25 16:19:58 +0000
+++ lib/lp/bugs/model/bugnotification.py	2011-03-04 17:17:08 +0000
@@ -150,12 +150,23 @@
             reason_body, reason_header = recipients.getReason(recipient)
             sql_values.append('(%s, %s, %s, %s)' % sqlvalues(
                 bug_notification, recipient, reason_header, reason_body))
+
         # 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]
+            store.execute("""
+                INSERT INTO BugNotificationFilter
+                  (bug_notification, bug_subscription_filter)
+                VALUES %s;""" % ", ".join(filter_link_sql))
+
         return bug_notification
 
 

=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
--- lib/lp/bugs/model/structuralsubscription.py	2011-03-04 17:16:44 +0000
+++ lib/lp/bugs/model/structuralsubscription.py	2011-03-04 17:17:08 +0000
@@ -585,14 +585,15 @@
     else:
         subscribers = []
         query_results = source.find(
-            (Person, StructuralSubscription),
+            (Person, StructuralSubscription, BugSubscriptionFilter),
             *constraints).config(distinct=True)
-        for person, subscription in query_results:
+        for person, subscription, filter in query_results:
             # Set up results.
             if person not in recipients:
                 subscribers.append(person)
                 recipients.addStructuralSubscriber(
                     person, subscription.target)
+            recipients.addFilter(filter)
         return subscribers