launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04052
[Merge] lp:~abentley/launchpad/resubmit-inactive into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/resubmit-inactive into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #618866 in Launchpad itself: "OOPS when trying to create a merge proposal to a branch that already has an active one"
https://bugs.launchpad.net/launchpad/+bug/618866
For more details, see:
https://code.launchpad.net/~abentley/launchpad/resubmit-inactive/+merge/65820
= Summary =
Fix bug #618866: OOPS when trying to create a merge proposal to a branch that already has an active one
== Proposed fix ==
Catch the exception and report it as a user error. Screenshot: http://people.canonical.com/~abentley/resubmit-error.png
== Pre-implementation notes ==
None
== Implementation details ==
With the current codebase, the context merge proposal has its status set to SUPERSEDED even when an exception is raised. This changes it so that the exception is raised before the context merge proposal is changed. Also, the exception has a reference to the existing merge proposal that was found.
testing.ExpectedException extends the testtools version to include the caught exception.
Also fixed some lint.
== Tests ==
bin/test -t test_resumit_on_inactive_retains_state -t test_resubmit_existing -t test_existingMergeProposal
== Demo and Q/A ==
Create a merge proposal. Open it in a second tab. In the second tab, resubmit it. In the first tab, resubmit it. You should get an error as shown in the screenshot.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/errors.py
lib/lp/code/model/branchmergeproposal.py
lib/lp/code/model/tests/test_branchmergeproposal.py
lib/lp/code/browser/tests/test_branchmergeproposal.py
lib/lp/testing/__init__.py
lib/lp/code/model/branch.py
lib/lp/code/browser/branchmergeproposal.py
--
https://code.launchpad.net/~abentley/launchpad/resubmit-inactive/+merge/65820
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/resubmit-inactive into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2011-05-12 21:33:10 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2011-06-24 18:22:13 +0000
@@ -80,7 +80,7 @@
from canonical.launchpad.webapp.authorization import check_permission
from canonical.launchpad.webapp.breadcrumb import Breadcrumb
from canonical.launchpad.webapp.interfaces import IPrimaryContext
-from canonical.launchpad.webapp.menu import NavigationMenu
+from canonical.launchpad.webapp.menu import NavigationMenu, structured
from lp.app.browser.launchpadform import (
action,
custom_widget,
@@ -102,6 +102,7 @@
CodeReviewVote,
)
from lp.code.errors import (
+ BranchMergeProposalExists,
ClaimReviewFailed,
WrongBranchMergeProposal,
)
@@ -1015,10 +1016,19 @@
@action('Resubmit', name='resubmit')
def resubmit_action(self, action, data):
"""Resubmit this proposal."""
- proposal = self.context.resubmit(
- self.user, data['source_branch'], data['target_branch'],
- data['prerequisite_branch'], data['description'],
- data['break_link'])
+ try:
+ proposal = self.context.resubmit(
+ self.user, data['source_branch'], data['target_branch'],
+ data['prerequisite_branch'], data['description'],
+ data['break_link'])
+ except BranchMergeProposalExists as e:
+ message = structured(
+ 'Cannot resubmit because <a href="%(url)s">a similar merge'
+ ' proposal</a> is already active.',
+ url=canonical_url(e.existing_proposal))
+ self.request.response.addErrorNotification(message)
+ self.next_url = canonical_url(self.context)
+ return None
self.next_url = canonical_url(proposal)
return proposal
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2011-05-12 21:33:10 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2011-06-24 18:22:13 +0000
@@ -18,14 +18,20 @@
import pytz
from soupmatchers import HTMLContains, Tag
-from testtools.matchers import Not
+from testtools.matchers import (
+ MatchesRegex,
+ Not,
+ )
import transaction
from zope.component import getMultiAdapter
from zope.security.interfaces import Unauthorized
from zope.security.proxy import removeSecurityProxy
from lp.services.messages.model.message import MessageSet
-from canonical.launchpad.webapp.interfaces import IPrimaryContext
+from canonical.launchpad.webapp.interfaces import (
+ BrowserNotificationLevel,
+ IPrimaryContext,
+ )
from canonical.launchpad.webapp.servers import LaunchpadTestRequest
from canonical.launchpad.webapp.testing import verifyObject
from canonical.testing.layers import (
@@ -585,16 +591,33 @@
def test_resubmit_action_break_link(self):
"""Enabling break_link prevents linking the old and new proposals."""
view = self.createView()
- context = view.context
- new_proposal = view.resubmit_action.success(
- {'source_branch': context.source_branch,
- 'target_branch': context.target_branch,
- 'prerequisite_branch': context.prerequisite_branch,
- 'description': None,
- 'break_link': True,
- })
+ new_proposal = self.resubmitDefault(view, break_link=True)
self.assertIs(None, new_proposal.supersedes)
+ @staticmethod
+ def resubmitDefault(view, break_link=False):
+ context = view.context
+ return view.resubmit_action.success(
+ {'source_branch': context.source_branch,
+ 'target_branch': context.target_branch,
+ 'prerequisite_branch': context.prerequisite_branch,
+ 'description': None,
+ 'break_link': break_link,
+ })
+
+ def test_resubmit_existing(self):
+ """Resubmitting a proposal when another is active is a user error."""
+ view = self.createView()
+ first_bmp = view.context
+ with person_logged_in(first_bmp.target_branch.owner):
+ first_bmp.resubmit(first_bmp.registrant)
+ self.resubmitDefault(view)
+ (notification,) = view.request.response.notifications
+ self.assertThat(
+ notification.message, MatchesRegex('Cannot resubmit because'
+ ' <a href=.*>a similar merge proposal</a> is already active.'))
+ self.assertEqual(BrowserNotificationLevel.ERROR, notification.level)
+
class TestResubmitBrowser(BrowserTestCase):
"""Browser tests for resubmitting branch merge proposals."""
@@ -1004,7 +1027,7 @@
parent = add_revision_to_branch(self.factory, source_branch,
self.factory.getUniqueDate()).revision
bmp = self.factory.makeBranchMergeProposal(registrant=self.user,
- date_created = self.factory.getUniqueDate(),
+ date_created=self.factory.getUniqueDate(),
source_branch=source_branch)
revision = add_revision_to_branch(self.factory, bmp.source_branch,
self.factory.getUniqueDate()).revision
@@ -1051,7 +1074,7 @@
date_created=(
datetime(year=2008, month=9, day=10, tzinfo=pytz.UTC)))
bmp2 = self.factory.makeBranchMergeProposal(
- target_branch = bmp1.target_branch,
+ target_branch=bmp1.target_branch,
date_created=(
datetime(year=2008, month=10, day=10, tzinfo=pytz.UTC)))
self.assertEqual(
=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py 2011-06-16 18:58:45 +0000
+++ lib/lp/code/errors.py 2011-06-24 18:22:13 +0000
@@ -182,6 +182,14 @@
class BranchMergeProposalExists(InvalidBranchMergeProposal):
"""Raised if there is already a matching BranchMergeProposal."""
+ def __init__(self, existing_proposal):
+ super(BranchMergeProposalExists, self).__init__(
+ 'There is already a branch merge proposal registered for '
+ 'branch %s to land on %s that is still active.' %
+ (existing_proposal.source_branch.displayname,
+ existing_proposal.target_branch.displayname))
+ self.existing_proposal = existing_proposal
+
class InvalidNamespace(Exception):
"""Raised when someone tries to lookup a namespace with a bad name.
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2011-06-16 18:52:42 +0000
+++ lib/lp/code/model/branch.py 2011-06-24 18:22:13 +0000
@@ -421,11 +421,8 @@
target = BranchMergeProposalGetter.activeProposalsForBranches(
self, target_branch)
- if target.count() > 0:
- raise BranchMergeProposalExists(
- 'There is already a branch merge proposal registered for '
- 'branch %s to land on %s that is still active.'
- % (self.displayname, target_branch.displayname))
+ for existing_proposal in target:
+ raise BranchMergeProposalExists(existing_proposal)
if date_created is None:
date_created = UTC_NOW
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2011-05-23 11:06:50 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2011-06-24 18:22:13 +0000
@@ -29,7 +29,6 @@
Select,
SQL,
)
-from storm.info import ClassAlias
from storm.locals import (
Int,
Reference,
@@ -59,6 +58,7 @@
from lp.code.errors import (
BadBranchMergeProposalSearchContext,
BadStateTransition,
+ BranchMergeProposalExists,
UserNotBranchReviewer,
WrongBranchMergeProposal,
)
@@ -560,6 +560,11 @@
if target_branch is None:
target_branch = self.target_branch
# DEFAULT instead of None, because None is a valid value.
+ proposals = BranchMergeProposalGetter.activeProposalsForBranches(
+ source_branch, target_branch)
+ for proposal in proposals:
+ if proposal is not self:
+ raise BranchMergeProposalExists(proposal)
if prerequisite_branch is DEFAULT:
prerequisite_branch = self.prerequisite_branch
if description is None:
@@ -656,9 +661,11 @@
LIMIT 10)""" % self.source_branch.id)
where = SQL("""BranchRevision.revision NOT IN (SELECT revision from
BranchRevision AS target where target.branch = %s and
- BranchRevision.revision = target.revision)""" % self.target_branch.id)
+ BranchRevision.revision = target.revision)""" %
+ self.target_branch.id)
using = SQL("""source as BranchRevision""")
- revisions = store.with_(source).using(using).find(BranchRevision, where)
+ revisions = store.with_(source).using(using).find(
+ BranchRevision, where)
return list(revisions.order_by(
Desc(BranchRevision.sequence)).config(limit=10))
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py 2011-05-12 21:33:10 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2011-06-24 18:22:13 +0000
@@ -45,6 +45,7 @@
)
from lp.code.errors import (
BadStateTransition,
+ BranchMergeProposalExists,
WrongBranchMergeProposal,
)
from lp.code.event.branchmergeproposal import (
@@ -78,6 +79,7 @@
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.product import IProductSet
from lp.testing import (
+ ExpectedException,
launchpadlib_for,
login,
login_person,
@@ -1627,7 +1629,7 @@
"""Resubmitting a proposal with no reviewers should work."""
bmp = make_merge_proposal_without_reviewers(self.factory)
with person_logged_in(bmp.registrant):
- bmp2 = bmp.resubmit(bmp.registrant)
+ bmp.resubmit(bmp.registrant)
def test_resubmit_changes_branches(self):
"""Resubmit changes branches, if specified."""
@@ -1654,10 +1656,42 @@
"""Resubmit breaks link, if specified."""
original = self.factory.makeBranchMergeProposal()
self.useContext(person_logged_in(original.registrant))
- revised = original.resubmit(
+ original.resubmit(
original.registrant, break_link=True)
self.assertIs(None, original.superseded_by)
+ def test_resumit_with_active_retains_state(self):
+ """Resubmit does not change proposal if an active proposal exists."""
+ first_mp = self.factory.makeBranchMergeProposal()
+ with person_logged_in(first_mp.registrant):
+ first_mp.rejectBranch(first_mp.target_branch.owner, 'a')
+ second_mp = self.factory.makeBranchMergeProposal(
+ source_branch=first_mp.source_branch,
+ target_branch=first_mp.target_branch)
+ expected_exc = ExpectedException(
+ BranchMergeProposalExists, 'There is already a branch merge'
+ ' proposal registered for branch .* to land on .* that is'
+ ' still active.')
+ with expected_exc:
+ first_mp.resubmit(first_mp.registrant)
+ self.assertEqual(
+ second_mp, expected_exc.caught_exc.existing_proposal)
+ self.assertEqual(
+ BranchMergeProposalStatus.REJECTED, first_mp.queue_status)
+
+ def test_resumit_on_inactive_retains_state_new_branches(self):
+ """Resubmit with branches doesn't change proposal."""
+ first_mp = self.factory.makeBranchMergeProposal()
+ with person_logged_in(first_mp.registrant):
+ first_mp.rejectBranch(first_mp.target_branch.owner, 'a')
+ second_mp = self.factory.makeBranchMergeProposal()
+ with ExpectedException(BranchMergeProposalExists, ''):
+ first_mp.resubmit(
+ first_mp.registrant, second_mp.source_branch,
+ second_mp.target_branch)
+ self.assertEqual(
+ BranchMergeProposalStatus.REJECTED, first_mp.queue_status)
+
class TestCreateMergeProposalJob(TestCaseWithFactory):
"""Tests for CreateMergeProposalJob."""
=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py 2011-06-24 11:10:43 +0000
+++ lib/lp/testing/__init__.py 2011-06-24 18:22:13 +0000
@@ -15,6 +15,7 @@
'BrowserTestCase',
'build_yui_unittest_suite',
'celebrity_logged_in',
+ 'ExpectedException',
'FakeTime',
'get_lsb_information',
'is_logged_in',
@@ -95,6 +96,7 @@
from testtools.content import Content
from testtools.content_type import UTF8_TEXT
from testtools.matchers import MatchesRegex
+from testtools.testcase import ExpectedException as TTExpectedException
import transaction
from windmill.authoring import WindmillTestClient
from zope.component import (
@@ -1350,3 +1352,16 @@
source_package.productseries,
source_package.sourcepackagename,
source_package.distroseries)
+
+
+class ExpectedException(TTExpectedException):
+ """An ExpectedException that provides access to the caught exception."""
+
+ def __init__(self, exc_type, value_re):
+ super(ExpectedException, self).__init__(exc_type, value_re)
+ self.caught_exc = None
+
+ def __exit__(self, exc_type, exc_value, traceback):
+ self.caught_exc = exc_value
+ return super(ExpectedException, self).__exit__(
+ exc_type, exc_value, traceback)
