Merge lp:~cjwatson/launchpad/consistent-bmp-events into lp:launchpad

[Colin Watson]

Remove BranchMergeProposalStatusChangeEvent and consistently generate ObjectModifiedEvent for all changes to merge proposal attributes.

I'd like to hook merge proposal webhooks into the Zope events that are already generated for several kinds of changes to merge proposals, but this is complicated by unnecessarily-chaotic conditions for when those events are generated. This removes the special-purpose BranchMergeProposalStatusChangeEvent, and instead simply consistently generates ObjectModifiedEvent for all changes to merge proposal attributes.

To get this right, I exhaustively checked all the possible calling paths to BranchMergeProposal._transitionToState. A couple of them (BranchMergeProposal:+resubmit and CodeHandler) failed to be consistent with other paths in terms of whether they generated ObjectModifiedEvent, which wasn't a practical problem before but would be now; and most of the rest were untested. I fixed all this up. The one relevant webservice entry point (BranchMergeProposal.setStatus) of course gets this right automatically thanks to lazr.restful.

BranchMergeProposalNeedsReviewEvent continues to exist even though it's sometimes accompanied by an almost equivalent ObjectModifiedEvent, because it can also be generated when a merge proposal is created.

This also has the useful side-effect of sending mail notifications for transitions to SUPERSEDED, per bug 716169. I made this consistent and added tests for it.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2015-07-21 16:30:15 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2015-10-06 15:19:46 +0000
@@ -161,18 +161,6 @@
     return decorator
 
 
-def update_and_notify(func):
-    """Decorate an action to update from a form and send a notification."""
-
-    @notify
-    def decorator(view, action, data):
-        result = func(view, action, data)
-        form.applyChanges(
-            view.context, view.form_fields, data, view.adapters)
-        return result
-    return decorator
-
-
 class BranchMergeCandidateView(LaunchpadView):
     """Provides a small fragment of landing targets"""
 
@@ -1028,6 +1016,7 @@
         return dict(item for item in items if item[1] is not UNSET)
 
     @action('Resubmit', name='resubmit')
+    @notify
     def resubmit_action(self, action, data):
         """Resubmit this proposal."""
         try:

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2015-10-01 10:25:19 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2015-10-06 15:19:46 +0000
@@ -12,6 +12,7 @@
     )
 from difflib import unified_diff
 
+from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.restful.interfaces import IJSONRequestCache
 import pytz
 import simplejson
@@ -20,11 +21,16 @@
     Tag,
     )
 from testtools.matchers import (
+    MatchesListwise,
     MatchesRegex,
+    MatchesStructure,
     Not,
     )
 import transaction
-from zope.component import getMultiAdapter
+from zope.component import (
+    getMultiAdapter,
+    getUtility,
+    )
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
@@ -47,6 +53,11 @@
     BranchMergeProposalStatus,
     CodeReviewVote,
     )
+from lp.code.interfaces.branchmergeproposal import (
+    IBranchMergeProposal,
+    IMergeProposalNeedsReviewEmailJobSource,
+    IMergeProposalUpdatedEmailJobSource,
+    )
 from lp.code.model.diff import PreviewDiff
 from lp.code.tests.helpers import (
     add_revision_to_branch,
@@ -66,6 +77,7 @@
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
     BrowserTestCase,
+    EventRecorder,
     feature_flags,
     login_person,
     monkey_patch,
@@ -168,6 +180,27 @@
         browser = self.getViewBrowser(bmp.merge_source, '+index')
         self.assertThat(browser.contents, HTMLContains(revision_number))
 
+    def test_notifies_modification(self):
+        bmp = self.makeBranchMergeProposal()
+        registrant = bmp.registrant
+        login_person(registrant)
+        browser = self.getViewBrowser(bmp, '+merged', user=bmp.registrant)
+        browser.getControl(self.merged_revision_text).value = str(
+            self.arbitrary_revisions[2])
+        with EventRecorder(propagate=True) as recorder:
+            browser.getControl('Mark as Merged').click()
+        login_person(registrant)
+        events = [
+            event for event in recorder.events
+            if isinstance(event, ObjectModifiedEvent) and
+               IBranchMergeProposal.providedBy(event.object)]
+        self.assertThat(events, MatchesListwise([
+            MatchesStructure(
+                object_before_modification=MatchesStructure.byEquality(
+                    queue_status=BranchMergeProposalStatus.WORK_IN_PROGRESS),
+                object=MatchesStructure.byEquality(
+                    queue_status=BranchMergeProposalStatus.MERGED))]))
+
 
 class TestBranchMergeProposalMergedViewBzr(
     TestBranchMergeProposalMergedViewMixin, BrowserTestCase):
@@ -918,6 +951,50 @@
             simplejson.loads(view.form_result))
 
 
+class TestBranchMergeProposalRequestReviewViewMixin:
+    """Test `BranchMergeProposalRequestReviewView`."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_notifies_modification(self):
+        bmp = self.makeBranchMergeProposal()
+        registrant = bmp.registrant
+        self.factory.makePerson(name='test-reviewer')
+        login_person(registrant)
+        browser = self.getViewBrowser(
+            bmp, '+request-review', user=bmp.registrant)
+        browser.getControl('Reviewer').value = 'test-reviewer'
+        with EventRecorder(propagate=True) as recorder:
+            browser.getControl('Request Review').click()
+        login_person(registrant)
+        events = [
+            event for event in recorder.events
+            if isinstance(event, ObjectModifiedEvent) and
+               IBranchMergeProposal.providedBy(event.object)]
+        self.assertThat(events, MatchesListwise([
+            MatchesStructure(
+                object_before_modification=MatchesStructure.byEquality(
+                    queue_status=BranchMergeProposalStatus.WORK_IN_PROGRESS),
+                object=MatchesStructure.byEquality(
+                    queue_status=BranchMergeProposalStatus.NEEDS_REVIEW))]))
+
+
+class TestBranchMergeProposalRequestReviewViewBzr(
+    TestBranchMergeProposalRequestReviewViewMixin, BrowserTestCase):
+    """Test `BranchMergeProposalRequestReviewView` for Bazaar."""
+
+    def makeBranchMergeProposal(self):
+        return self.factory.makeBranchMergeProposal()
+
+
+class TestBranchMergeProposalRequestReviewViewGit(
+    TestBranchMergeProposalRequestReviewViewMixin, BrowserTestCase):
+    """Test `BranchMergeProposalRequestReviewView` for Git."""
+
+    def makeBranchMergeProposal(self):
+        return self.factory.makeBranchMergeProposalForGit()
+
+
 class TestBranchMergeProposalResubmitViewMixin:
     """Test BranchMergeProposalResubmitView."""
 
@@ -998,13 +1075,54 @@
             ' <a href=.*>a similar merge proposal</a> is already active.'))
         self.assertEqual(BrowserNotificationLevel.ERROR, notification.level)
 
+    def test_notifies_modification(self):
+        view = self.createView()
+        with EventRecorder(propagate=True) as recorder:
+            self.resubmitDefault(view)
+        events = [
+            event for event in recorder.events
+            if isinstance(event, ObjectModifiedEvent) and
+               IBranchMergeProposal.providedBy(event.object)]
+        self.assertThat(events, MatchesListwise([
+            MatchesStructure(
+                object_before_modification=MatchesStructure.byEquality(
+                    queue_status=BranchMergeProposalStatus.WORK_IN_PROGRESS),
+                object=MatchesStructure.byEquality(
+                    queue_status=BranchMergeProposalStatus.SUPERSEDED))]))
+
+    def test_mail_jobs(self):
+        """Resubmitting sends mail about the changed old MP and the new MP."""
+        view = self.createView()
+        new_proposal = self.resubmitDefault(view)
+        updated_job_source = getUtility(IMergeProposalUpdatedEmailJobSource)
+        [updated_job] = list(
+            removeSecurityProxy(updated_job_source).iterReady())
+        self.assertThat(updated_job, MatchesStructure.byEquality(
+            branch_merge_proposal=view.context,
+            metadata={
+                'delta_text': '    Status: Work in progress => Superseded',
+                'editor': view.context.registrant.name,
+                }))
+        needs_review_job_source = getUtility(
+            IMergeProposalNeedsReviewEmailJobSource)
+        [needs_review_job] = list(
+            removeSecurityProxy(needs_review_job_source).iterReady())
+        self.assertThat(needs_review_job, MatchesStructure.byEquality(
+            branch_merge_proposal=new_proposal,
+            metadata={}))
+
 
 class TestBranchMergeProposalResubmitViewBzr(
     TestBranchMergeProposalResubmitViewMixin, TestCaseWithFactory):
     """Test BranchMergeProposalResubmitView for Bazaar."""
 
     def _makeBranchMergeProposal(self):
-        return self.factory.makeBranchMergeProposal()
+        bmp = self.factory.makeBranchMergeProposal()
+        # Pretend to have a revision so that
+        # BranchMergeProposalJobDerived.iterReady will consider this
+        # proposal.
+        removeSecurityProxy(bmp.source_branch).revision_count = 1
+        return bmp
 
     @staticmethod
     def _getFormValues(source_branch, target_branch, prerequisite_branch,
@@ -1366,8 +1484,8 @@
         self.assertThat(browser.contents, HTMLContains(expected_meta))
 
 
-class TestBranchMergeProposalChangeStatusOptions(TestCaseWithFactory):
-    """Test the status vocabulary generated for then +edit-status view."""
+class TestBranchMergeProposalChangeStatusView(TestCaseWithFactory):
+    """Test the +edit-status view."""
 
     layer = DatabaseFunctionalLayer
 
@@ -1378,10 +1496,10 @@
         self.proposal = self.factory.makeBranchMergeProposal(
             registrant=self.user)
 
-    def _createView(self):
+    def _createView(self, form=None):
         # Construct the view and initialize it.
         view = BranchMergeProposalChangeStatusView(
-            self.proposal, LaunchpadTestRequest())
+            self.proposal, LaunchpadTestRequest(form=form))
         view.initialize()
         return view
 
@@ -1454,6 +1572,22 @@
         self.assertAllStatusesAvailable(
             user=self.proposal.target_branch.owner)
 
+    def test_notifies_modification(self):
+        view = self._createView(form={'revno': '1'})
+        with EventRecorder(propagate=True) as recorder:
+            view.update_action.success(
+                {'queue_status': BranchMergeProposalStatus.NEEDS_REVIEW})
+        events = [
+            event for event in recorder.events
+            if isinstance(event, ObjectModifiedEvent) and
+               IBranchMergeProposal.providedBy(event.object)]
+        self.assertThat(events, MatchesListwise([
+            MatchesStructure(
+                object_before_modification=MatchesStructure.byEquality(
+                    queue_status=BranchMergeProposalStatus.WORK_IN_PROGRESS),
+                object=MatchesStructure.byEquality(
+                    queue_status=BranchMergeProposalStatus.NEEDS_REVIEW))]))
+
 
 class TestCommentAttachmentRendering(TestCaseWithFactory):
     """Test diff attachments are rendered correctly."""

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2015-09-24 13:44:02 +0000
+++ lib/lp/code/configure.zcml	2015-10-06 15:19:46 +0000
@@ -295,8 +295,8 @@
       handler="lp.code.subscribers.karma.branch_merge_proposed"/>
   <subscriber
       for="lp.code.interfaces.branchmergeproposal.IBranchMergeProposal
-           lp.code.interfaces.event.IBranchMergeProposalStatusChangeEvent"
-      handler="lp.code.subscribers.karma.branch_merge_status_changed"/>
+           lazr.lifecycle.interfaces.IObjectModifiedEvent"
+      handler="lp.code.subscribers.karma.branch_merge_modified"/>
 
   <!-- hierarchy -->
 

=== modified file 'lib/lp/code/event/branchmergeproposal.py'
--- lib/lp/code/event/branchmergeproposal.py	2015-07-08 16:05:11 +0000
+++ lib/lp/code/event/branchmergeproposal.py	2015-10-06 15:19:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Event implementation classes for branch merge proposal events."""
@@ -6,7 +6,6 @@
 __metaclass__ = type
 __all__ = [
     'BranchMergeProposalNeedsReviewEvent',
-    'BranchMergeProposalStatusChangeEvent',
     'NewBranchMergeProposalEvent',
     'NewCodeReviewCommentEvent',
     'ReviewerNominatedEvent',
@@ -17,24 +16,12 @@
 
 from lp.code.interfaces.event import (
     IBranchMergeProposalNeedsReviewEvent,
-    IBranchMergeProposalStatusChangeEvent,
     INewBranchMergeProposalEvent,
     INewCodeReviewCommentEvent,
     IReviewerNominatedEvent,
     )
 
 
-@implementer(IBranchMergeProposalStatusChangeEvent)
-class BranchMergeProposalStatusChangeEvent(ObjectEvent):
-    """See `IBranchMergeProposalStatusChangeEvent`."""
-
-    def __init__(self, proposal, user, from_state, to_state):
-        ObjectEvent.__init__(self, proposal)
-        self.user = user
-        self.from_state = from_state
-        self.to_state = to_state
-
-
 @implementer(INewBranchMergeProposalEvent)
 class NewBranchMergeProposalEvent(ObjectEvent):
     """A new merge has been proposed."""

=== modified file 'lib/lp/code/interfaces/event.py'
--- lib/lp/code/interfaces/event.py	2015-06-06 08:49:54 +0000
+++ lib/lp/code/interfaces/event.py	2015-10-06 15:19:46 +0000
@@ -5,7 +5,6 @@
 
 __metaclass__ = type
 __all__ = [
-    'IBranchMergeProposalStatusChangeEvent',
     'IBranchMergeProposalNeedsReviewEvent',
     'IGitRefsUpdatedEvent',
     'INewBranchMergeProposalEvent',
@@ -15,15 +14,6 @@
 
 
 from zope.component.interfaces import IObjectEvent
-from zope.interface import Attribute
-
-
-class IBranchMergeProposalStatusChangeEvent(IObjectEvent):
-    """A merge proposal has changed state."""
-
-    user = Attribute("The user who updated the proposal.")
-    from_state = Attribute("The previous queue_status.")
-    to_state = Attribute("The updated queue_status.")
 
 
 class INewBranchMergeProposalEvent(IObjectEvent):

=== modified file 'lib/lp/code/mail/codehandler.py'
--- lib/lp/code/mail/codehandler.py	2015-09-28 17:38:45 +0000
+++ lib/lp/code/mail/codehandler.py	2015-10-06 15:19:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -14,6 +14,7 @@
 from zope.interface import implementer
 from zope.security.interfaces import Unauthorized
 
+from lp.code.adapters.branch import BranchMergeProposalNoPreviewDiffDelta
 from lp.code.enums import CodeReviewVote
 from lp.code.errors import UserNotBranchReviewer
 from lp.code.interfaces.branchmergeproposal import IBranchMergeProposalGetter
@@ -258,12 +259,13 @@
     def processCommands(self, context, commands):
         """Process the various merge proposal commands against the context."""
         processing_errors = []
-
-        for command in commands:
-            try:
-                command.execute(context)
-            except EmailProcessingError as error:
-                processing_errors.append((error, command))
+        with BranchMergeProposalNoPreviewDiffDelta.monitor(
+                context.merge_proposal):
+            for command in commands:
+                try:
+                    command.execute(context)
+                except EmailProcessingError as error:
+                    processing_errors.append((error, command))
 
         if len(processing_errors) > 0:
             errors, commands = zip(*processing_errors)

=== modified file 'lib/lp/code/mail/tests/test_codehandler.py'
--- lib/lp/code/mail/tests/test_codehandler.py	2015-07-21 09:04:01 +0000
+++ lib/lp/code/mail/tests/test_codehandler.py	2015-10-06 15:19:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Testing the CodeHandler."""
@@ -7,6 +7,7 @@
 
 from textwrap import dedent
 
+from lazr.lifecycle.event import ObjectModifiedEvent
 from storm.store import Store
 import transaction
 from zope.security.management import setSecurityPolicy
@@ -17,6 +18,7 @@
     CodeReviewNotificationLevel,
     CodeReviewVote,
     )
+from lp.code.event.branchmergeproposal import NewCodeReviewCommentEvent
 from lp.code.mail.codehandler import (
     AddReviewerEmailCommand,
     CodeEmailCommands,
@@ -425,6 +427,24 @@
             mail['from'])
         self.assertEqual(0, bmp.all_comments.count())
 
+    def test_notifies_modification(self):
+        """Changes to the merge proposal itself trigger events."""
+        mail = self.factory.makeSignedMessage(body=' merge approved')
+        bmp = self.factory.makeBranchMergeProposal()
+        email_addr = bmp.address
+        switch_dbuser(config.processmail.dbuser)
+        login_person(bmp.merge_target.owner)
+        _, events = self.assertNotifies(
+            [ObjectModifiedEvent, NewCodeReviewCommentEvent], False,
+            self.code_handler.process, mail, email_addr, None)
+        self.assertEqual(bmp, events[0].object)
+        self.assertEqual(
+            BranchMergeProposalStatus.WORK_IN_PROGRESS,
+            events[0].object_before_modification.queue_status)
+        self.assertEqual(
+            BranchMergeProposalStatus.CODE_APPROVED,
+            events[0].object.queue_status)
+
 
 class TestVoteEmailCommand(TestCase):
     """Test the vote and tag processing of the VoteEmailCommand."""

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2015-07-08 16:05:11 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2015-10-06 15:19:46 +0000
@@ -58,7 +58,6 @@
     )
 from lp.code.event.branchmergeproposal import (
     BranchMergeProposalNeedsReviewEvent,
-    BranchMergeProposalStatusChangeEvent,
     NewCodeReviewCommentEvent,
     ReviewerNominatedEvent,
     )
@@ -577,7 +576,6 @@
                         _date_reviewed=None):
         """Set the proposal to next_state."""
         # Check the reviewer can review the code for the target branch.
-        old_state = self.queue_status
         if not self.merge_target.isPersonTrustedReviewer(reviewer):
             raise UserNotBranchReviewer
         # Check the current state of the proposal.
@@ -589,8 +587,6 @@
         self.date_reviewed = _date_reviewed
         # Record the reviewed revision id
         self.reviewed_revision_id = revision_id
-        notify(BranchMergeProposalStatusChangeEvent(
-                self, reviewer, old_state, next_state))
 
     def approveBranch(self, reviewer, revision_id, _date_reviewed=None):
         """See `IBranchMergeProposal`."""

=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
--- lib/lp/code/model/branchmergeproposaljob.py	2015-08-03 09:52:29 +0000
+++ lib/lp/code/model/branchmergeproposaljob.py	2015-10-06 15:19:46 +0000
@@ -284,6 +284,7 @@
                        Or(Branch.next_mirror_time == None,
                           Branch.branch_type != BranchType.HOSTED)),
                    BranchMergeProposal.source_git_repository != None)))
+        jobs = jobs.config(distinct=True)
         return (klass(job) for job in jobs)
 
     def getOopsVars(self):

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2015-09-28 17:38:45 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2015-10-06 15:19:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for BranchMergeProposals."""
@@ -514,7 +514,7 @@
         commenter = self.factory.makePerson()
         login_person(commenter)
         result, events = self.assertNotifies(
-            NewCodeReviewCommentEvent,
+            NewCodeReviewCommentEvent, False,
             merge_proposal.createComment,
             owner=commenter,
             subject='A review.')
@@ -661,7 +661,7 @@
         registrant = self.factory.makePerson()
         result, events = self.assertNotifies(
             [NewBranchMergeProposalEvent,
-             BranchMergeProposalNeedsReviewEvent],
+             BranchMergeProposalNeedsReviewEvent], False,
             source_branch.addLandingTarget, registrant, target_branch,
             needs_review=True)
         self.assertEqual(result, events[0].object)
@@ -674,7 +674,7 @@
             product=source_branch.product)
         registrant = self.factory.makePerson()
         result, events = self.assertNotifies(
-            [NewBranchMergeProposalEvent],
+            [NewBranchMergeProposalEvent], False,
             source_branch.addLandingTarget, registrant, target_branch)
         self.assertEqual(result, events[0].object)
 
@@ -685,7 +685,7 @@
             set_state=BranchMergeProposalStatus.WORK_IN_PROGRESS)
         with person_logged_in(bmp.registrant):
             self.assertNotifies(
-                [BranchMergeProposalNeedsReviewEvent],
+                [BranchMergeProposalNeedsReviewEvent], False,
                 bmp.setStatus, BranchMergeProposalStatus.NEEDS_REVIEW)
 
     def test_needs_review_no_op(self):
@@ -1285,7 +1285,7 @@
         bmp = self.factory.makeBranchMergeProposal()
         login_person(bmp.target_branch.owner)
         self.assertNotifies(
-            ObjectModifiedEvent, notify_modified, bmp, bmp.markAsMerged,
+            ObjectModifiedEvent, False, notify_modified, bmp, bmp.markAsMerged,
             merge_reporter=bmp.target_branch.owner)
         self.assertEqual(BranchMergeProposalStatus.MERGED, bmp.queue_status)
         self.assertEqual(bmp.target_branch.owner, bmp.merge_reporter)
@@ -1305,7 +1305,7 @@
         login_person(merge_proposal.source_branch.owner)
         reviewer = self.factory.makePerson()
         result, events = self.assertNotifies(
-            ReviewerNominatedEvent,
+            ReviewerNominatedEvent, False,
             merge_proposal.nominateReviewer,
             reviewer=reviewer,
             registrant=merge_proposal.source_branch.owner)

=== modified file 'lib/lp/code/model/tests/test_codereviewcomment.py'
--- lib/lp/code/model/tests/test_codereviewcomment.py	2012-01-15 10:43:27 +0000
+++ lib/lp/code/model/tests/test_codereviewcomment.py	2015-10-06 15:19:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for CodeReviewComment"""
@@ -107,7 +107,8 @@
         """Creating a CodeReviewComment should trigger a notification."""
         message = self.factory.makeMessage()
         self.assertNotifies(
-            NewCodeReviewCommentEvent, self.bmp.createCommentFromMessage,
+            NewCodeReviewCommentEvent, False,
+            self.bmp.createCommentFromMessage,
             message, None, None, original_email=None, _validate=False)
 
 

=== modified file 'lib/lp/code/model/tests/test_codereviewkarma.py'
--- lib/lp/code/model/tests/test_codereviewkarma.py	2011-12-30 07:38:46 +0000
+++ lib/lp/code/model/tests/test_codereviewkarma.py	2015-10-06 15:19:46 +0000
@@ -1,10 +1,11 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for karma allocated for code reviews."""
 
 __metaclass__ = type
 
+from lp.code.interfaces.branchmergeproposal import notify_modified
 from lp.registry.interfaces.karma import IKarmaAssignedEvent
 from lp.registry.interfaces.person import IPerson
 from lp.testing import (
@@ -113,7 +114,9 @@
         proposal = self.factory.makeBranchMergeProposal()
         reviewer = proposal.target_branch.owner
         self.karma_events = []
-        proposal.approveBranch(reviewer, "A rev id.")
+        login_person(reviewer)
+        notify_modified(
+            proposal, proposal.approveBranch, reviewer, "A rev id.")
         self.assertOneKarmaEvent(reviewer, 'branchmergeapproved')
 
     def test_approvingOwnCodeReview(self):
@@ -123,7 +126,9 @@
         proposal = self.factory.makeBranchMergeProposal(
             target_branch=target_branch, registrant=reviewer)
         self.karma_events = []
-        proposal.approveBranch(reviewer, "A rev id.")
+        login_person(reviewer)
+        notify_modified(
+            proposal, proposal.approveBranch, reviewer, "A rev id.")
         self.assertOneKarmaEvent(reviewer, 'branchmergeapprovedown')
 
     def test_rejectedCodeReview(self):
@@ -132,7 +137,8 @@
         proposal = self.factory.makeBranchMergeProposal()
         reviewer = proposal.target_branch.owner
         self.karma_events = []
-        proposal.rejectBranch(reviewer, "A rev id.")
+        login_person(reviewer)
+        notify_modified(proposal, proposal.rejectBranch, reviewer, "A rev id.")
         self.assertOneKarmaEvent(reviewer, 'branchmergerejected')
 
     def test_rejectedOwnCodeReview(self):
@@ -144,5 +150,6 @@
         proposal = self.factory.makeBranchMergeProposal(
             target_branch=target_branch, registrant=reviewer)
         self.karma_events = []
-        proposal.rejectBranch(reviewer, "A rev id.")
+        login_person(reviewer)
+        notify_modified(proposal, proposal.rejectBranch, reviewer, "A rev id.")
         self.assertOneKarmaEvent(reviewer, 'branchmergerejectedown')

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2015-09-02 16:54:24 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2015-10-06 15:19:46 +0000
@@ -51,6 +51,7 @@
     GitRepositoryExists,
     GitTargetError,
     )
+from lp.code.event.git import GitRefsUpdatedEvent
 from lp.code.interfaces.branchmergeproposal import (
     BRANCH_MERGE_PROPOSAL_FINAL_STATES as FINAL_STATES,
     )
@@ -1864,7 +1865,7 @@
         hosting_client.detectMerges = FakeMethod(
             result={source_1.commit_sha1: u"0" * 40})
         self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
-        repository.createOrUpdateRefs({
+        refs_info = {
             u"refs/heads/target-1": {
                 u"sha1": u"0" * 40,
                 u"type": GitObjectType.COMMIT,
@@ -1872,8 +1873,12 @@
             u"refs/heads/target-2": {
                 u"sha1": u"1" * 40,
                 u"type": GitObjectType.COMMIT,
-                }
-            })
+                },
+            }
+        expected_events = [
+            ObjectModifiedEvent, ObjectModifiedEvent, GitRefsUpdatedEvent]
+        _, events = self.assertNotifies(
+            expected_events, True, repository.createOrUpdateRefs, refs_info)
         expected_args = [
             (repository.getInternalPath(), target_1.commit_sha1,
              set([source_1.commit_sha1, source_2.commit_sha1])),
@@ -1888,6 +1893,15 @@
             BranchMergeProposalStatus.WORK_IN_PROGRESS, bmp2.queue_status)
         self.assertEqual(BranchMergeProposalStatus.MERGED, bmp3.queue_status)
         self.assertEqual(u"0" * 40, bmp3.merged_revision_id)
+        # The two ObjectModifiedEvents indicate the queue_status changes.
+        self.assertContentEqual(
+            [bmp1, bmp3], [event.object for event in events[:2]])
+        self.assertContentEqual(
+            [(BranchMergeProposalStatus.WORK_IN_PROGRESS,
+              BranchMergeProposalStatus.MERGED)],
+            set((event.object_before_modification.queue_status,
+                 event.object.queue_status)
+                for event in events[:2]))
 
 
 class TestGitRepositorySet(TestCaseWithFactory):

=== modified file 'lib/lp/code/subscribers/branchmergeproposal.py'
--- lib/lp/code/subscribers/branchmergeproposal.py	2013-04-09 09:47:58 +0000
+++ lib/lp/code/subscribers/branchmergeproposal.py	2015-10-06 15:19:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Event subscribers for branch merge proposals."""
@@ -49,14 +49,19 @@
         from_person = None
     else:
         from_person = IPerson(event.user)
-    # If the merge proposal was work in progress, then we don't want to send
-    # out an email as the needs review email will cover that.
+    # If the merge proposal was work in progress and is now needs review,
+    # then we don't want to send out an email as the needs review email will
+    # cover that.
     old_status = event.object_before_modification.queue_status
-    if old_status == BranchMergeProposalStatus.WORK_IN_PROGRESS:
-        # Unless the new status is merged.  If this occurs we really should
-        # send out an email.
-        if merge_proposal.queue_status != BranchMergeProposalStatus.MERGED:
-            return
+    new_status = merge_proposal.queue_status
+
+    in_progress_states = (
+        BranchMergeProposalStatus.WORK_IN_PROGRESS,
+        BranchMergeProposalStatus.NEEDS_REVIEW)
+
+    if (old_status == BranchMergeProposalStatus.WORK_IN_PROGRESS and
+            new_status in in_progress_states):
+        return
     # Create a delta of the changes.  If there are no changes to report, then
     # we're done.
     delta = BranchMergeProposalNoPreviewDiffDelta.construct(

=== modified file 'lib/lp/code/subscribers/karma.py'
--- lib/lp/code/subscribers/karma.py	2015-04-22 16:11:40 +0000
+++ lib/lp/code/subscribers/karma.py	2015-10-06 15:19:46 +0000
@@ -3,6 +3,8 @@
 
 """Assign karma for code domain activity."""
 
+from zope.principalregistry.principalregistry import UnauthenticatedPrincipal
+
 from lp.code.enums import BranchMergeProposalStatus
 from lp.registry.interfaces.person import IPerson
 from lp.services.database.sqlbase import block_implicit_flushes
@@ -43,26 +45,33 @@
 
 
 @block_implicit_flushes
-def branch_merge_status_changed(proposal, event):
-    """Assign karma to the user who approved the merge."""
+def branch_merge_modified(proposal, event):
+    """Assign karma to the user who approved or rejected the merge."""
+    if event.user is None or isinstance(event.user, UnauthenticatedPrincipal):
+        # Some modification events have no associated user context.  In
+        # these cases there's no karma to assign.
+        return
+
     if proposal.source_git_repository is not None:
         target = proposal.source_git_repository.namespace
     else:
         target = proposal.source_branch.target
     user = IPerson(event.user)
+    old_status = event.object_before_modification.queue_status
+    new_status = proposal.queue_status
 
     in_progress_states = (
         BranchMergeProposalStatus.WORK_IN_PROGRESS,
         BranchMergeProposalStatus.NEEDS_REVIEW)
 
-    if ((event.to_state == BranchMergeProposalStatus.CODE_APPROVED) and
-        (event.from_state in (in_progress_states))):
+    if ((new_status == BranchMergeProposalStatus.CODE_APPROVED) and
+        (old_status in (in_progress_states))):
         if user == proposal.registrant:
             target.assignKarma(user, 'branchmergeapprovedown')
         else:
             target.assignKarma(user, 'branchmergeapproved')
-    elif ((event.to_state == BranchMergeProposalStatus.REJECTED) and
-          (event.from_state in (in_progress_states))):
+    elif ((new_status == BranchMergeProposalStatus.REJECTED) and
+          (old_status in (in_progress_states))):
         if user == proposal.registrant:
             target.assignKarma(user, 'branchmergerejectedown')
         else:

=== modified file 'lib/lp/codehosting/scanner/tests/test_mergedetection.py'
--- lib/lp/codehosting/scanner/tests/test_mergedetection.py	2013-06-20 05:50:00 +0000
+++ lib/lp/codehosting/scanner/tests/test_mergedetection.py	2015-10-06 15:19:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the scanner's merge detection."""
@@ -8,6 +8,7 @@
 import logging
 
 from bzrlib.revision import NULL_REVISION
+from lazr.lifecycle.event import ObjectModifiedEvent
 import transaction
 from zope.component import getUtility
 from zope.event import notify
@@ -271,7 +272,8 @@
         self.assertNotEqual(
             BranchLifecycleStatus.MERGED,
             proposal.source_branch.lifecycle_status)
-        mergedetection.merge_detected(
+        _, [event] = self.assertNotifies(
+            [ObjectModifiedEvent], True, mergedetection.merge_detected,
             logging.getLogger(),
             proposal.source_branch, proposal.target_branch, proposal)
         self.assertEqual(
@@ -279,6 +281,12 @@
         self.assertEqual(
             BranchLifecycleStatus.MERGED,
             proposal.source_branch.lifecycle_status)
+        self.assertEqual(proposal, event.object)
+        self.assertEqual(
+            BranchMergeProposalStatus.WORK_IN_PROGRESS,
+            event.object_before_modification.queue_status)
+        self.assertEqual(
+            BranchMergeProposalStatus.MERGED, event.object.queue_status)
         job = IStore(proposal).find(
             BranchMergeProposalJob,
             BranchMergeProposalJob.branch_merge_proposal == proposal,

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2015-09-30 00:38:30 +0000
+++ lib/lp/testing/__init__.py	2015-10-06 15:19:46 +0000
@@ -530,11 +530,14 @@
         from lp.testing.matchers import Provides
         self.assertThat(obj, Provides(interface))
 
-    def assertNotifies(self, event_types, callable_obj, *args, **kwargs):
+    def assertNotifies(self, event_types, propagate, callable_obj,
+                       *args, **kwargs):
         """Assert that a callable performs a given notification.
 
         :param event_type: One or more event types that notification is
             expected for.
+        :param propagate: If True, propagate events to their normal
+            subscribers.
         :param callable_obj: The callable to call.
         :param *args: The arguments to pass to the callable.
         :param **kwargs: The keyword arguments to pass to the callable.
@@ -543,7 +546,7 @@
         """
         if not isinstance(event_types, (list, tuple)):
             event_types = [event_types]
-        with EventRecorder() as recorder:
+        with EventRecorder(propagate=propagate) as recorder:
             result = callable_obj(*args, **kwargs)
         if len(recorder.events) == 0:
             raise AssertionError('No notification was performed.')
@@ -1241,21 +1244,27 @@
 class EventRecorder:
     """Intercept and record Zope events.
 
-    This prevents the events from propagating to their normal subscribers.
-    The recorded events can be accessed via the 'events' list.
+    This prevents the events from propagating to their normal subscribers,
+    unless `propagate=True` is passed to the constructor.  The recorded
+    events can be accessed via the 'events' list.
     """
 
-    def __init__(self):
+    def __init__(self, propagate=False):
+        self.propagate = propagate
         self.events = []
         self.old_subscribers = None
+        self.new_subscribers = None
 
     def __enter__(self):
         self.old_subscribers = zope.event.subscribers[:]
-        zope.event.subscribers[:] = [self.events.append]
+        if not self.propagate:
+            zope.event.subscribers[:] = []
+        zope.event.subscribers.append(self.events.append)
+        self.new_subscribers = zope.event.subscribers[:]
         return self
 
     def __exit__(self, exc_type, exc_value, traceback):
-        assert zope.event.subscribers == [self.events.append], (
+        assert zope.event.subscribers == self.new_subscribers, (
             'Subscriber list has been changed while running!')
         zope.event.subscribers[:] = self.old_subscribers