launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19524
[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
