launchpad-reviewers team mailing list archive

[Mattias Backman <mattias.backman@xxxxxxxxxx>]

Mattias Backman has proposed merging lp:~linaro-infrastructure/launchpad/notify-workitems-changes into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #956940 in Launchpad itself: "Regression - work items notifications lost since the split from whiteboard"
  https://bugs.launchpad.net/launchpad/+bug/956940

For more details, see:
https://code.launchpad.net/~linaro-infrastructure/launchpad/notify-workitems-changes/+merge/98231

This branch adds email notifications for Work Items changes on Blueprints. To get the same format users are used to from the Whiteboard we use the same mechanisms to send a diff of the workitems_text property.

I've had to add an extra transaction.commit() to the production code in updateWorkItems() to be able to get notifications for just adding Work Items. Also changing order of entire blocks of Work Items sometimes produce weird diffs without it. Any tips about how to get rid of that commit() is appreciated.
-- 
https://code.launchpad.net/~linaro-infrastructure/launchpad/notify-workitems-changes/+merge/98231
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~linaro-infrastructure/launchpad/notify-workitems-changes into lp:launchpad.

=== modified file 'lib/lp/blueprints/mail/notifications.py'
--- lib/lp/blueprints/mail/notifications.py	2011-12-30 06:14:56 +0000
+++ lib/lp/blueprints/mail/notifications.py	2012-03-19 16:39:43 +0000
@@ -78,6 +78,18 @@
                 whiteboard_delta['old'], whiteboard_delta['new'], 72)
             info_lines.append('Whiteboard changed:')
             info_lines.append(whiteboard_diff)
+    if spec_delta.workitems_text is not None:
+        if info_lines:
+            info_lines.append('')
+        workitems_delta = spec_delta.workitems_text
+        if workitems_delta['old'] is '':
+            info_lines.append('Work items set to:')
+            info_lines.append(mail_wrapper.format(workitems_delta['new']))
+        else:
+            workitems_diff = get_unified_diff(
+                workitems_delta['old'], workitems_delta['new'], 72)
+            info_lines.append('Work items changed:')
+            info_lines.append(workitems_diff)
 
     if not info_lines:
         # The specification was modified, but we don't yet support

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2012-03-08 16:01:35 +0000
+++ lib/lp/blueprints/model/specification.py	2012-03-19 16:39:43 +0000
@@ -30,6 +30,7 @@
     SQL,
     )
 from storm.store import Store
+import transaction
 from zope.component import getUtility
 from zope.event import notify
 from zope.interface import implements
@@ -338,6 +339,10 @@
         for sequence, item in to_insert:
             self.newWorkItem(item['title'], sequence, item['status'],
                              item['assignee'], item['milestone'])
+        # XXX: without this commit, we won't get notifications for just
+        # adding work items. In complicated edits such as reordering and
+        # switching entire work items blocks we get weird diffs.
+        transaction.commit()
 
     def setTarget(self, target):
         """See ISpecification."""
@@ -612,7 +617,6 @@
         delta.recordListAddedAndRemoved("bugs",
                                         "bugs_linked",
                                         "bugs_unlinked")
-
         if delta.changes:
             changes = delta.changes
             changes["specification"] = self

=== modified file 'lib/lp/blueprints/model/tests/test_specification.py'
--- lib/lp/blueprints/model/tests/test_specification.py	2012-03-08 16:01:35 +0000
+++ lib/lp/blueprints/model/tests/test_specification.py	2012-03-19 16:39:43 +0000
@@ -5,10 +5,15 @@
 
 __metaclass__ = type
 
+from lazr.lifecycle.event import ObjectModifiedEvent
+from lazr.lifecycle.snapshot import Snapshot
 from testtools.matchers import (
     Equals,
     MatchesStructure,
     )
+import transaction
+from zope.event import notify
+from zope.interface import providedBy
 from zope.security.interfaces import Unauthorized
 
 from lp.app.validators import LaunchpadValidationError
@@ -18,6 +23,7 @@
     )
 from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
 from lp.registry.model.milestone import Milestone
+from lp.services.mail import stub
 from lp.services.webapp import canonical_url
 from lp.testing import (
     ANONYMOUS,
@@ -148,6 +154,95 @@
             % (u'http://ubuntu.com/foo', url, cleaned_title), str(e))
 
 
+class TestSpecificationWorkItemsNotifications(TestCaseWithFactory):
+    """ Test the notification related to SpecificationWorkItems on
+    ISpecification."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_workitems_added_notification_message(self):
+        stub.test_emails = []
+        spec = self.factory.makeSpecification()
+        old_spec = Snapshot(spec, providing=providedBy(spec))
+        status = SpecificationWorkItemStatus.TODO
+        new_work_item = {'title': u'A work item',
+                         'status': status,
+                         'assignee': None,
+                         'milestone': None,
+                         'sequence': 0
+                         }
+
+        login_person(spec.owner)
+        spec.updateWorkItems([new_work_item])
+        notify(ObjectModifiedEvent(spec, old_spec,
+                                   edited_fields=['workitems_text']))
+        transaction.commit()
+
+        self.assertEqual(1, len(stub.test_emails))
+        rationale = 'Work items set to:\nWork items:\n%s: %s' % (
+            new_work_item['title'],
+            new_work_item['status'].name)
+        [email] = stub.test_emails
+        # Actual message is part 2 of the e-mail.
+        msg = email[2]
+        self.assertIn(rationale, msg)
+
+    def test_workitems_deleted_notification_message(self):
+        stub.test_emails = []
+        wi = self.factory.makeSpecificationWorkItem()
+        spec = wi.specification
+        old_spec = Snapshot(spec, providing=providedBy(spec))
+        login_person(spec.owner)
+        spec.updateWorkItems([])
+        notify(ObjectModifiedEvent(spec, old_spec,
+                                   edited_fields=['workitems_text']))
+        transaction.commit()
+
+        self.assertEqual(1, len(stub.test_emails))
+        rationale = '- %s: %s' % (wi.title, wi.status.name)
+        [email] = stub.test_emails
+        # Actual message is part 2 of the e-mail.
+        msg = email[2]
+        self.assertIn(rationale, msg)
+
+    def test_workitems_changed_notification_message(self):
+        spec = self.factory.makeSpecification()
+        original_status = SpecificationWorkItemStatus.TODO
+        new_status = SpecificationWorkItemStatus.DONE
+        original_work_item = {'title': u'The same work item',
+                               'status': original_status,
+                               'assignee': None,
+                               'milestone': None,
+                               'sequence': 0
+                               }
+        new_work_item = {'title': u'The same work item',
+                          'status': new_status,
+                          'assignee': None,
+                          'milestone': None,
+                          'sequence': 0
+                          }
+        login_person(spec.owner)
+        spec.updateWorkItems([original_work_item])
+        old_spec = Snapshot(spec, providing=providedBy(spec))
+
+        stub.test_emails = []
+        spec.updateWorkItems([new_work_item])
+        notify(ObjectModifiedEvent(spec, old_spec,
+                                   edited_fields=['workitems_text']))
+        transaction.commit()
+
+        self.assertEqual(1, len(stub.test_emails))
+        rationale_removed = '- %s: %s' % (original_work_item['title'],
+                                          original_work_item['status'].name)
+        rationale_added = '+ %s: %s' % (new_work_item['title'],
+                                        new_work_item['status'].name)
+        [email] = stub.test_emails
+        # Actual message is part 2 of the e-mail.
+        msg = email[2]
+        self.assertIn(rationale_removed, msg)
+        self.assertIn(rationale_added, msg)
+
+
 class TestSpecificationWorkItems(TestCaseWithFactory):
     """Test the Workitem-related methods of ISpecification."""