← Back to team overview

launchpad-reviewers team mailing list archive

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

 

Colin Watson has proposed merging lp:~cjwatson/launchpad/consistent-bmp-events into lp:launchpad.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/consistent-bmp-events/+merge/273325

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.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/consistent-bmp-events into lp:launchpad.
=== 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-04 01:43:34 +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-04 01:43:34 +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,7 +21,9 @@
     Tag,
     )
 from testtools.matchers import (
+    MatchesListwise,
     MatchesRegex,
+    MatchesStructure,
     Not,
     )
 import transaction
@@ -47,6 +50,7 @@
     BranchMergeProposalStatus,
     CodeReviewVote,
     )
+from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
 from lp.code.model.diff import PreviewDiff
 from lp.code.tests.helpers import (
     add_revision_to_branch,
@@ -66,6 +70,7 @@
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
     BrowserTestCase,
+    EventRecorder,
     feature_flags,
     login_person,
     monkey_patch,
@@ -168,6 +173,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 +944,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,6 +1068,21 @@
             ' <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))]))
+
 
 class TestBranchMergeProposalResubmitViewBzr(
     TestBranchMergeProposalResubmitViewMixin, TestCaseWithFactory):
@@ -1366,8 +1451,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 +1463,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 +1539,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-04 01:43:34 +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-04 01:43:34 +0000
@@ -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-04 01:43:34 +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-04 01:43:34 +0000
@@ -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-04 01:43:34 +0000
@@ -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-04 01:43:34 +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/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-04 01:43:34 +0000
@@ -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-04 01:43:34 +0000
@@ -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-04 01:43:34 +0000
@@ -5,6 +5,7 @@
 
 __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-04 01:43:34 +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,
     )
@@ -120,6 +121,7 @@
     ANONYMOUS,
     api_url,
     celebrity_logged_in,
+    EventRecorder,
     login_person,
     person_logged_in,
     TestCaseWithFactory,
@@ -1864,7 +1866,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 +1874,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 +1894,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/karma.py'
--- lib/lp/code/subscribers/karma.py	2015-04-22 16:11:40 +0000
+++ lib/lp/code/subscribers/karma.py	2015-10-04 01:43:34 +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-04 01:43:34 +0000
@@ -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-04 01:43:34 +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
 


Follow ups