launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06768
[Merge] lp:~linaro-infrastructure/launchpad/notify-workitems-changes into lp:launchpad
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."""
Follow ups