Merge lp:~linaro-infrastructure/launchpad/notify-workitems-changes into lp:launchpad

[Mattias Backman]

[r=abentley] Add email notifications for Work Item changes and fix the +workitems page.

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.

[Guilherme Salgado (salgado)]

Unfortunately, I don't think this actually fixes the bug. AIUI, the email notifications were not being sent because nothing emits an ObjectModifiedEvent(spec) after the spec's work items are changed (which is expected because there are no changes to the spec itself, only to things related to it) and your changes doesn't seem to cause that event to be fired either. You have the notify(ObjectModifiedEvent(...)) call in your tests, but in production that event is not going to be fired anywhere since the Specification object itself is not changed. My suggestion was to have updateWorkItems() fire the event, in the same way that Product._setLicenses() (which is very similar to our case in that it changes objects related to the Product but not the Product itself) does.

[Mattias Backman (mabac)]

On Mon, Mar 19, 2012 at 7:22 PM, Guilherme Salgado
<email address hidden> wrote:
> Unfortunately, I don't think this actually fixes the bug. AIUI, the email notifications were not being sent because nothing emits an ObjectModifiedEvent(spec) after the spec's work items are changed (which is expected because there are no changes to the spec itself, only to things related to it) and your changes doesn't seem to cause that event to be fired either.  You have the notify(ObjectModifiedEvent(...)) call in your tests, but in production that event is not going to be fired anywhere since the Specification object itself is not changed.  My suggestion was to have updateWorkItems() fire the event, in the same way that Product._setLicenses() (which is very similar to our case in that it changes objects related to the Product but not the Product itself) does.

That's the mystery here. ;) Since I started writing the tests, I ended
up with a call to notify() in updateWorkItems(). Later when I tried to
run the test Launchpad locally I got two notifications for each change
so I removed my call to notify(). Something else is actually seeing
the change on a Specification property and fires the notification in
the running system.

The reason for not getting emails seems to be that nothing in
notify_specification_modified() added appropriate lines for the
workitems delta.

The automagical notify does not happen when running the tests though,
so I followed the example in
lp.bugs.mail.tests.test_assignee_notification_message() which changes
a bugtask property and then fires it's own notification.

> --
> https://code.launchpad.net/~linaro-infrastructure/launchpad/notify-workitems-changes/+merge/98231
> Your team Linaro Infrastructure is subscribed to branch lp:~linaro-infrastructure/launchpad/notify-workitems-changes.

[Guilherme Salgado (salgado)]

Oh, so I was wrong all along, thinking the event was what we were missing, and you didn't tell me? ;)

So, now that you pointed me in the right direction I can see where this is coming from... The event is fired by lazr.restful whenever someone calls a remote operation (like updateStatus(), which is exported as a write operation), so that's why you don't get the event when you call updateStatus() directly. I suggest you add a comment to the test explaining why you need to fire the event yourself and that in production this is not needed. However, if someone edits the workitems using the +workitems page, no notification will be sent because that code doesn't call updateStatus() via lazr.restful and so no event is fired. We might need to have that page itself fire an ObjectModifiedEvent...

Now, it looks like the work item changes are not always flushed to the DB when notify_specification_modified is fired (as the pdb session below shows), so that would explain why the diff looks wrong in some cases.

(this is within notify_specification_modified)
(Pdb) p spec.workitems_text
''
(Pdb) !from lp.services.database.sqlbase import flush_database_updates
(Pdb) p flush_database_updates()
None
(Pdb) p spec.workitems_text
u'Work items:\nfoo: TODO\nbar: DONE'

I wouldn't expect this tot happen because spec.workitems_text will do a DB query and before executing that storm will flush the updates to the DB. As you've found out, calling transaction.commit() solves that, but that is not something we should be doing (we should commit only at the end of the request processing routine, if everything succeeds). A better way to workaround this is to call flush_database_updates(), probably in updateWorkItems(), but first I'll try to figure out why we need that so that we can at least leave a comment explaining it.

[Guilherme Salgado (salgado)]

<lifeless> salgado: IIRC the only reason a select wouldn't see new objects is if both the select and the store.add for the objects *both* happen within a block_implicit_flushes() decorator

In our case the select runs inside a block_implicit_flushes() but the store.add() happens before that. Still, I think this is probably why we're seeing this behavior

[Guilherme Salgado (salgado)]

<lifeless> you might want to figure out why it has that decorator
<lifeless> this smells to me of some bug patched over
<lifeless> if it was e.g. a truism that all event handlers must not permit implicit flushes, we'd have glued it into the event subscription code to avoid the repetition
<lifeless> salgado: I would look in annotate for the commit adding the decorator and see what it was part of; look at the MP too probably; and definitely try removing the decorator and seeing what happens

[Mattias Backman (mabac)]

Removing @block_implicit_flushes worked just fine, all tests pass and nothing seems broken when running locally. Now the diffs look good and we get notifications for all changes without extra commits.

I added code to send notifications from the +workitems page and in the process fixed it so that is also saves the changes.

[Aaron Bentley (abentley)]

review: Approve

[Aaron Bentley (abentley)]

LOC increased acceptable because this was part of work that had decreased LOC.

=== modified file 'lib/lp/blueprints/browser/specification.py'
--- lib/lp/blueprints/browser/specification.py	2012-03-06 18:13:52 +0000
+++ lib/lp/blueprints/browser/specification.py	2012-03-21 15:37:21 +0000
@@ -45,6 +45,8 @@
     Popen,
     )
 
+from lazr.lifecycle.event import ObjectModifiedEvent
+from lazr.lifecycle.snapshot import Snapshot
 from lazr.restful.interface import use_template
 from lazr.restful.interfaces import (
     IFieldHTMLRenderer,
@@ -58,11 +60,13 @@
 from zope.app.form.browser.itemswidgets import DropdownWidget
 from zope.component import getUtility
 from zope.error.interfaces import IErrorReportingUtility
+from zope.event import notify
 from zope.formlib import form
 from zope.formlib.form import Fields
 from zope.interface import (
     implementer,
     Interface,
+    providedBy,
     )
 from zope.schema import (
     Bool,
@@ -110,6 +114,7 @@
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.product import IProduct
 from lp.services.config import config
+from lp.services.fields import WorkItemsText
 from lp.services.propertycache import cachedproperty
 from lp.services.webapp import (
     canonical_url,
@@ -690,6 +695,12 @@
             "The state of progress being made on the actual "
             "implementation or delivery of this feature."))
 
+    workitems_text = WorkItemsText(
+        title=_('Work Items'), required=True,
+        description=_(
+            "Work items for this specification input in a text format. "
+            "Your changes will override the current work items."))
+
 
 class SpecificationEditView(LaunchpadEditFormView):
 
@@ -729,6 +740,14 @@
     field_names = ['workitems_text']
     custom_widget('workitems_text', TextAreaWidget, height=15)
 
+    @action(_('Change'), name='change')
+    def change_action(self, action, data):
+        old_spec = Snapshot(self.context, providing=providedBy(self.context))
+        self.context.setWorkItems(data['workitems_text'])
+        notify(ObjectModifiedEvent(
+            self.context, old_spec, edited_fields=['workitems_text']))
+        self.next_url = canonical_url(self.context)
+
 
 class SpecificationEditPeopleView(SpecificationEditView):
     label = 'Change the people involved'

=== 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-21 15:37:21 +0000
@@ -22,7 +22,6 @@
     return '[Blueprint %s] %s' % (spec.name, spec.title)
 
 
-@block_implicit_flushes
 def notify_specification_modified(spec, event):
     """Notify the related people that a specification has been modifed."""
     user = IPerson(event.user)
@@ -78,6 +77,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/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-21 15:37:21 +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,111 @@
             % (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):
+        """ Test that we get a notification for setting work items on a new
+        specification."""
+        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])
+        # In production this notification is fired by lazr.restful for changes
+        # in the specification form and notify(ObjectModifiedEvent(...)) for
+        # changes in the +workitems form. We need to do it ourselves in this
+        # test.
+        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):
+        """ Test that we get a notification for deleting a work item."""
+        stub.test_emails = []
+        wi = self.factory.makeSpecificationWorkItem()
+        spec = wi.specification
+        old_spec = Snapshot(spec, providing=providedBy(spec))
+        login_person(spec.owner)
+        spec.updateWorkItems([])
+        # In production this notification is fired by lazr.restful, but we
+        # need to do it ourselves in this test.
+        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):
+        """ Test that we get a notification about a work item status change.
+        This will be in the form of a line added and one deleted."""
+        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])
+        # In production this notification is fired by lazr.restful, but we
+        # need to do it ourselves in this test.
+        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."""
 
@@ -155,7 +266,7 @@
 
     def setUp(self):
         super(TestSpecificationWorkItems, self).setUp()
-        self.wi_header =  self.factory.makeMilestone(
+        self.wi_header = self.factory.makeMilestone(
             name='none-milestone-as-header')
 
     def assertWorkItemsTextContains(self, spec, items):
@@ -260,7 +371,7 @@
         items = [self.wi_header, work_item1, milestone, work_item2]
         self.assertWorkItemsTextContains(spec, items)
 
-    def test_workitems_text_with_implicit_and_explicit_milestone_reversed(self):
+    def test_workitems_text_with_implicit_and_explicit_milestone_reverse(self):
         spec = self.factory.makeSpecification()
         milestone = self.factory.makeMilestone(product=spec.product)
         login_person(spec.owner)
@@ -294,7 +405,8 @@
     def test_workitems_text_with_assignee(self):
         assignee = self.factory.makePerson()
         work_item = self.factory.makeSpecificationWorkItem(assignee=assignee)
-        self.assertWorkItemsTextContains(work_item.specification, [self.wi_header, work_item])
+        self.assertWorkItemsTextContains(
+            work_item.specification, [self.wi_header, work_item])
 
     def test_work_items_property(self):
         spec = self.factory.makeSpecification()