Merge lp:~danilo/launchpad/bug-720826-links into lp:launchpad/db-devel

[Данило Шеган]

[r=gary][bug=720826] Populate BugNotificationFilter table with links between BugNotifications and BugSubscriptionFilters to finally close off 720826.

= 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.

[Gary Poster (gary)]

Let's remove the distinct=True in the structural subscription code, as we discussed on IRC. It might have made some sense when we were not getting a result per filter, but does not now.

I asked for a test showing multiple filters and you said they existed in unit tests. I'm mostly OK with that as being sufficient here. :-)

Yay, thank you!

review: Approve

[Данило Шеган (danilo)]

Yep, unit tests are available in "bin/test -cvvt test_header_multiple -t test_body_multiple".

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2011-03-02 23:08:54 +0000
+++ lib/lp/bugs/configure.zcml	2011-03-05 17:49:56 +0000
@@ -1056,6 +1056,10 @@
         class="lp.bugs.mail.bugnotificationrecipients.BugNotificationRecipients">
         <allow
             interface="canonical.launchpad.interfaces.launchpad.INotificationRecipientSet"/>
+        <!-- BugNotificationRecipients provides the following
+             attributes/methods in addition. -->
+        <allow
+            attributes="subscription_filters addFilter"/>
     </class>
     <securedutility
         provides="lp.bugs.interfaces.bugnotification.IBugNotificationSet"

=== 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-05 17:49:56 +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)
@@ -676,7 +678,6 @@
 been set properly.
 
     >>> from lp.bugs.model.bugnotification import BugNotification
-    >>> from lp.bugs.enum import BugNotificationStatus
     >>> for notification in BugNotification.select(orderBy='id')[-8:]:
     ...     if notification.is_comment:
     ...         identifier = 'comment'
@@ -1247,6 +1248,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 +1281,14 @@
     From: Sample Person <[e-mail]>
     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: [e-mail]
@@ -1390,6 +1394,7 @@
     From: Sample Person <[e-mail]>
     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 +1405,7 @@
     --
     You received this bug notification because you are subscribed to Mozilla
     Firefox.
+    Matching filters: Allow-comments filter
     ...
     ----------------------------------------------------------------------
     To: [e-mail]

=== 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-05 17:49:56 +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-05 17:49:56 +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 02:29:09 +0000
+++ lib/lp/bugs/model/structuralsubscription.py	2011-03-05 17:49:56 +0000
@@ -585,14 +585,15 @@
     else:
         subscribers = []
         query_results = source.find(
-            (Person, StructuralSubscription),
-            *constraints).config(distinct=True)
-        for person, subscription in query_results:
+            (Person, StructuralSubscription, BugSubscriptionFilter),
+            *constraints)
+        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