launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/defer-wip-email into lp:launchpad/devel with lp:~thumper/launchpad/needs-review-event as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


Change the addLandingTarget method to only raise the needs review event if the proposal is actually created in needs review state.

Also raise the event when moving from work in progress to needs review.

tests:
  TestMergeProposalNotification

-- 
https://code.launchpad.net/~thumper/launchpad/defer-wip-email/+merge/38791
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/defer-wip-email into lp:launchpad/devel.

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2010-10-19 00:49:45 +0000
+++ lib/lp/code/model/branch.py	2010-10-19 00:49:48 +0000
@@ -437,7 +437,8 @@
                 reviewer, registrant, review_type, _notify_listeners=False)
 
         notify(NewBranchMergeProposalEvent(bmp))
-        notify(BranchMergeProposalNeedsReviewEvent(bmp))
+        if needs_review:
+            notify(BranchMergeProposalNeedsReviewEvent(bmp))
 
         return bmp
 

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2010-10-07 22:46:08 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2010-10-19 00:49:48 +0000
@@ -61,6 +61,7 @@
     WrongBranchMergeProposal,
     )
 from lp.code.event.branchmergeproposal import (
+    BranchMergeProposalNeedsReviewEvent,
     BranchMergeProposalStatusChangeEvent,
     NewCodeReviewCommentEvent,
     ReviewerNominatedEvent,
@@ -412,6 +413,11 @@
         # review state.
         if _date_requested is None:
             _date_requested = UTC_NOW
+        # If we are going from work in progress to needs review, then reset
+        # the root message id and trigger a job to send out the email.
+        if self.queue_status == BranchMergeProposalStatus.WORK_IN_PROGRESS:
+            self.root_message_id = None
+            notify(BranchMergeProposalNeedsReviewEvent(self))
         if self.queue_status != BranchMergeProposalStatus.NEEDS_REVIEW:
             self._transitionToState(BranchMergeProposalStatus.NEEDS_REVIEW)
             self.date_review_requested = _date_requested

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2010-10-19 00:49:45 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2010-10-19 00:49:48 +0000
@@ -3,6 +3,8 @@
 
 # pylint: disable-msg=F0401
 
+from __future__ import with_statement
+
 """Tests for BranchMergeProposals."""
 
 __metaclass__ = type
@@ -24,9 +26,7 @@
 
 from canonical.database.constants import UTC_NOW
 from canonical.launchpad.ftests import (
-    ANONYMOUS,
     import_secret_test_key,
-    login,
     syncUpdate,
     )
 from canonical.launchpad.interfaces.launchpad import IPrivacy
@@ -77,7 +77,10 @@
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
 from lp.testing import (
+    ANONYMOUS,
+    login,
     login_person,
+    person_logged_in,
     TestCaseWithFactory,
     )
 from lp.testing.factory import (
@@ -753,8 +756,10 @@
     def setUp(self):
         TestCaseWithFactory.setUp(self, user='test@xxxxxxxxxxxxx')
 
-    def test_notifyOnCreate(self):
-        """Ensure that a notification is emitted on creation"""
+    def test_notifyOnCreate_needs_review(self):
+        # When a merge proposal is created needing review, the
+        # BranchMergeProposalNeedsReviewEvent is raised as well as the usual
+        # NewBranchMergeProposalEvent.
         source_branch = self.factory.makeProductBranch()
         target_branch = self.factory.makeProductBranch(
             product=source_branch.product)
@@ -762,9 +767,48 @@
         result, events = self.assertNotifies(
             [NewBranchMergeProposalEvent,
              BranchMergeProposalNeedsReviewEvent],
+            source_branch.addLandingTarget, registrant, target_branch,
+            needs_review=True)
+        self.assertEqual(result, events[0].object)
+
+    def test_notifyOnCreate_work_in_progress(self):
+        # When a merge proposal is created as work in progress, the
+        # BranchMergeProposalNeedsReviewEvent is not raised.
+        source_branch = self.factory.makeProductBranch()
+        target_branch = self.factory.makeProductBranch(
+            product=source_branch.product)
+        registrant = self.factory.makePerson()
+        result, events = self.assertNotifies(
+            [NewBranchMergeProposalEvent],
             source_branch.addLandingTarget, registrant, target_branch)
         self.assertEqual(result, events[0].object)
 
+    def test_needs_review_from_work_in_progress(self):
+        # Transitioning from work in progress to needs review raises the
+        # BranchMergeProposalNeedsReviewEvent event.
+        bmp = self.factory.makeBranchMergeProposal(
+            set_state=BranchMergeProposalStatus.WORK_IN_PROGRESS)
+        with person_logged_in(bmp.registrant):
+            self.assertNotifies(
+                [BranchMergeProposalNeedsReviewEvent],
+                bmp.setStatus, BranchMergeProposalStatus.NEEDS_REVIEW)
+
+    def test_needs_review_no_op(self):
+        # Calling needs review when in needs review does not notify.
+        bmp = self.factory.makeBranchMergeProposal(
+            set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+        with person_logged_in(bmp.registrant):
+            self.assertNoNotification(
+                bmp.setStatus, BranchMergeProposalStatus.NEEDS_REVIEW)
+
+    def test_needs_review_from_approved(self):
+        # Calling needs review when approved does not notify either.
+        bmp = self.factory.makeBranchMergeProposal(
+            set_state=BranchMergeProposalStatus.CODE_APPROVED)
+        with person_logged_in(bmp.registrant):
+            self.assertNoNotification(
+                bmp.setStatus, BranchMergeProposalStatus.NEEDS_REVIEW)
+
     def test_getNotificationRecipients(self):
         """Ensure that recipients can be added/removed with subscribe"""
         bmp = self.factory.makeBranchMergeProposal()