Merge lp:~abentley/launchpad/resubmit-change-branch into lp:launchpad

[Aaron Bentley]

Allow changing branches, description when resubmitting merge proposal.

= Summary =
Fix bug #504369: Resubmit should allow changing the branches
Screenshot: http://people.canonical.com/~abentley/resubmit-change-branch.png

== Proposed fix ==
Add fields for the branches to the resubmit page. Also allow changing the
description. Also allow the user to break the link between the old and new
proposals.

== Pre-implementation notes ==
None

== Implementation details ==
Changed the schema to ResubmitSchema to add the break_link boolean.
Changed from MergeProposalEditView to LaunchpadFormView to avoid attempting to
adapt BranchMergeProposal to ResubmitSchema.
Changed the text to reflect the fact that users now control the branches.
Changed the text to refer to the current merge proposal as "this proposal",
since the new one doesn't exist yet.

== Tests ==
bin/test -vt proposal code

== Demo and Q/A ==
Create a merge proposal. Resubmit it, changing all branches, description, and
selecting "break link". A new proposal should be created with the specified
branches and description, and with no link to the previous one. The previous
one should be marked "superseded".

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/templates/branchmergeproposal-resubmit.pt
  lib/lp/code/interfaces/branchmergeproposal.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/code/browser/branchmergeproposal.py

./lib/lp/code/browser/branchmergeproposal.py
     986: E202 whitespace before ']'
     988: E301 expected 1 blank line, found 0

[Brad Crittenden (bac)]

Thanks for the very nice cover letter Aaron.

The lint issues both look real and need to be fixed. The list for field_names should be multi-line with one entry per line ending with a trailing comma.

In 'initial_values' the list 'values' is really key, value pairs so calling it 'items' would make more sense.

In 'test_resubmit_text' your use of string concatenation the call to 'assertTextMatches...' is hard to read. Using a variable like
  expected = '...'
would be more readable.

s/If this is the same as the target branch/
  If this branch is the same as the target branch

Please provide a 'cancel_url' so that you get a cancel link.

Finally, it is clear you thought it out since you test for it explicitly, but I question the need to create a new merge proposal when nothing has changed. Do you have a good use case for that?

This new functionality is a vast improvement. Thanks for recognizing the need and providing the solution.

review: Approve (code)

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/05/2010 01:42 AM, Brad Crittenden wrote:
> Finally, it is clear you thought it out since you test for it explicitly, but I question the need to create a new merge proposal when nothing has changed. Do you have a good use case for that?

Actually, that was the existing functionality. It is for cases where
the proposal is marked "resubmit", many changes are made, and finally
the proposal is resubmitted.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzUJpAACgkQ0F+nu1YWqI1WDwCdEKhJpAiuZXSzxf6/PK5gmzGO
wUUAnjmKdw60t5erpfaDAegkFkU22zuN
=houq
-----END PGP SIGNATURE-----

[Paul Hummer (rockstar)]

As discussed on the phone, I'm of the opinion that we should ensure that both the source and the target aren't changed. In cases like that, I think that we should encouraging people to create a whole new merge proposal. I've been outvoted in this case, but I thought I'd mention it in the MP just for history's sake.

review: Approve (ui)

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2010-09-29 07:09:03 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2010-11-11 02:54:49 +0000
@@ -57,6 +57,7 @@
     Interface,
     )
 from zope.schema import (
+    Bool,
     Choice,
     Int,
     Text,
@@ -965,20 +966,51 @@
                         % revision_name)
 
 
-class BranchMergeProposalResubmitView(MergeProposalEditView,
+class ResubmitSchema(IBranchMergeProposal):
+
+    break_link = Bool(
+        title=u'Start afresh',
+        description=(
+            u'Do not show old conversation and do not link to superseded'
+            ' proposal.'),
+        default=False,)
+
+
+class BranchMergeProposalResubmitView(LaunchpadFormView,
                                       UnmergedRevisionsMixin):
     """The view to resubmit a proposal to merge."""
 
-    schema = IBranchMergeProposal
+    schema = ResubmitSchema
+    for_input = True
     page_title = label = "Resubmit proposal to merge"
-    field_names = []
+    field_names = [
+        'source_branch',
+        'target_branch',
+        'prerequisite_branch',
+        'description',
+        'break_link',
+        ]
+
+    def initialize(self):
+        self.cancel_url = canonical_url(self.context)
+        super(BranchMergeProposalResubmitView, self).initialize()
+
+    @property
+    def initial_values(self):
+        UNSET = object()
+        items = ((key, getattr(self.context, key, UNSET)) for key in
+                  self.field_names if key != 'break_link')
+        return dict(item for item in items if item[1] is not UNSET)
 
     @action('Resubmit', name='resubmit')
-    @update_and_notify
     def resubmit_action(self, action, data):
         """Resubmit this proposal."""
-        proposal = self.context.resubmit(self.user)
+        proposal = self.context.resubmit(
+            self.user, data['source_branch'], data['target_branch'],
+            data['prerequisite_branch'], data['description'],
+            data['break_link'])
         self.next_url = canonical_url(proposal)
+        return proposal
 
 
 class BranchMergeProposalEditView(MergeProposalEditView):

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2010-10-25 05:33:20 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2010-11-11 02:54:49 +0000
@@ -34,6 +34,7 @@
     BranchMergeProposalChangeStatusView,
     BranchMergeProposalContextMenu,
     BranchMergeProposalMergedView,
+    BranchMergeProposalResubmitView,
     BranchMergeProposalVoteView,
     DecoratedCodeReviewVoteReference,
     ICodeReviewNewRevisions,
@@ -53,7 +54,9 @@
     make_merge_proposal_without_reviewers,
     )
 from lp.testing import (
+    BrowserTestCase,
     login_person,
+    person_logged_in,
     TestCaseWithFactory,
     time_counter,
     )
@@ -501,7 +504,7 @@
         self.assertOnePendingReview(proposal, reviewer)
         self.assertIs(None, proposal.description)
 
-    def test_register_request_review_type(self):
+    def test_register_request_review_type_branch_reviewer(self):
         # We can ask for a specific review type. The target branch has a
         # reviewer so that reviewer should be used.
         target_branch, reviewer = self._makeTargetBranchWithReviewer()
@@ -515,6 +518,96 @@
         self.assertIs(None, proposal.description)
 
 
+class TestBranchMergeProposalResubmitView(TestCaseWithFactory):
+    """Test BranchMergeProposalResubmitView."""
+
+    layer = DatabaseFunctionalLayer
+
+    def createView(self):
+        """Create the required view."""
+        context = self.factory.makeBranchMergeProposal()
+        self.useContext(person_logged_in(context.registrant))
+        view = BranchMergeProposalResubmitView(
+            context, LaunchpadTestRequest())
+        view.initialize()
+        return view
+
+    def test_resubmit_action(self):
+        """resubmit_action resubmits the proposal."""
+        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': False,
+            })
+        self.assertEqual(new_proposal.supersedes, context)
+        self.assertEqual(new_proposal.source_branch, context.source_branch)
+        self.assertEqual(new_proposal.target_branch, context.target_branch)
+        self.assertEqual(
+            new_proposal.prerequisite_branch, context.prerequisite_branch)
+
+    def test_resubmit_action_change_branches(self):
+        """Changing the branches changes the branches in the new proposal."""
+        view = self.createView()
+        target = view.context.source_branch.target
+        new_source = self.factory.makeBranchTargetBranch(target)
+        new_target = self.factory.makeBranchTargetBranch(target)
+        new_prerequisite = self.factory.makeBranchTargetBranch(target)
+        new_proposal = view.resubmit_action.success(
+            {'source_branch': new_source, 'target_branch': new_target,
+             'prerequisite_branch': new_prerequisite,
+             'description': 'description',
+             'break_link': False,
+             })
+        self.assertEqual(new_proposal.supersedes, view.context)
+        self.assertEqual(new_proposal.source_branch, new_source)
+        self.assertEqual(new_proposal.target_branch, new_target)
+        self.assertEqual(new_proposal.prerequisite_branch, new_prerequisite)
+
+    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,
+            })
+        self.assertIs(None, new_proposal.supersedes)
+
+
+class TestResubmitBrowser(BrowserTestCase):
+    """Browser tests for resubmitting branch merge proposals."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_resubmit_text(self):
+        """The text of the resubmit page is as expected."""
+        bmp = self.factory.makeBranchMergeProposal(registrant=self.user)
+        text = self.getMainText(bmp, '+resubmit')
+        expected = (
+            'Resubmit proposal to merge.*'
+            'Source Branch:.*'
+            'Target Branch:.*'
+            'Prerequisite Branch:.*'
+            'Description.*'
+            'Start afresh.*')
+        self.assertTextMatchesExpressionIgnoreWhitespace(expected, text)
+
+    def test_resubmit_controls(self):
+        """Proposals can be resubmitted using the browser."""
+        bmp = self.factory.makeBranchMergeProposal(registrant=self.user)
+        browser = self.getViewBrowser(bmp, '+resubmit')
+        browser.getControl('Description').value = 'flibble'
+        browser.getControl('Resubmit').click()
+        self.assertEqual('flibble', bmp.superseded_by.description)
+
+
 class TestBranchMergeProposalView(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer

=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py	2010-10-25 05:33:20 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py	2010-11-11 02:54:49 +0000
@@ -45,6 +45,7 @@
 from lazr.restful.fields import (
     CollectionField,
     Reference,
+    ReferenceChoice,
     )
 from zope.event import notify
 from zope.interface import (
@@ -62,6 +63,7 @@
     TextLine,
     )
 
+from canonical.database.constants import DEFAULT
 from canonical.launchpad import _
 from canonical.launchpad.interfaces.launchpad import IPrivacy
 from canonical.launchpad.webapp.interfaces import ITableBatchNavigator
@@ -112,25 +114,26 @@
             description=_('The person who registered the landing target.')))
 
     source_branch = exported(
-        Reference(
-            title=_('Source Branch'), schema=IBranch,
+        ReferenceChoice(
+            title=_('Source Branch'), schema=IBranch, vocabulary='Branch',
             required=True, readonly=True,
             description=_("The branch that has code to land.")))
 
     target_branch = exported(
-        Reference(
+        ReferenceChoice(
             title=_('Target Branch'),
-            schema=IBranch, required=True, readonly=True,
+            schema=IBranch, vocabulary='Branch', required=True, readonly=True,
             description=_(
                 "The branch that the source branch will be merged into.")))
 
     prerequisite_branch = exported(
-        Reference(
-            title=_('Dependent Branch'),
-            schema=IBranch, required=False, readonly=True,
-            description=_("The branch that the source branch branched from. "
-                          "If this is the same as the target branch, then "
-                          "leave this field blank.")))
+        ReferenceChoice(
+            title=_('Prerequisite Branch'),
+            schema=IBranch, vocabulary='Branch', required=False,
+            readonly=True, description=_(
+                "The branch that the source branch branched from. "
+                "If this branch is the same as the target branch, then "
+                "leave this field blank.")))
 
     # This is redefined from IPrivacy.private because the attribute is
     # read-only. The value is determined by the involved branches.
@@ -430,13 +433,24 @@
         :type merge_reporter: ``Person``
         """
 
-    def resubmit(registrant):
+    def resubmit(registrant, source_branch=None, target_branch=None,
+                 prerequisite_branch=DEFAULT):
         """Mark the branch merge proposal as superseded and return a new one.
 
         The new proposal is created as work-in-progress, and copies across
         user-entered data like the whiteboard.  All the current proposal's
         reviewers, including those who have only been nominated, are requested
         to review the new proposal.
+
+        :param registrant: The person registering the new proposal.
+        :param source_branch: The source_branch for the new proposal (defaults
+            to the current source_branch).
+        :param target_branch: The target_branch for the new proposal (defaults
+            to the current target_branch).
+        :param prerequisite_branch: The prerequisite_branch for the new
+            proposal (defaults to the current prerequisite_branch).
+        :param description: The description for the new proposal (defaults to
+            the current description).
         """
 
     def isMergable():

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2010-10-19 00:44:24 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2010-11-11 02:54:49 +0000
@@ -543,8 +543,19 @@
             date_merged = UTC_NOW
         self.date_merged = date_merged
 
-    def resubmit(self, registrant):
+    def resubmit(self, registrant, source_branch=None, target_branch=None,
+                 prerequisite_branch=DEFAULT, description=None,
+                 break_link=False):
         """See `IBranchMergeProposal`."""
+        if source_branch is None:
+            source_branch = self.source_branch
+        if target_branch is None:
+            target_branch = self.target_branch
+        # DEFAULT instead of None, because None is a valid value.
+        if prerequisite_branch is DEFAULT:
+            prerequisite_branch = self.prerequisite_branch
+        if description is None:
+            description = self.description
         # You can transition from REJECTED to SUPERSEDED, but
         # not from MERGED or SUPERSEDED.
         self._transitionToState(
@@ -553,15 +564,16 @@
         # a database query to identify if there are any active proposals
         # with the same source and target branches.
         self.syncUpdate()
-        review_requests = set(
-            (vote.reviewer, vote.review_type) for vote in self.votes)
-        proposal = self.source_branch.addLandingTarget(
+        review_requests = list(set(
+            (vote.reviewer, vote.review_type) for vote in self.votes))
+        proposal = source_branch.addLandingTarget(
             registrant=registrant,
-            target_branch=self.target_branch,
-            prerequisite_branch=self.prerequisite_branch,
-            description=self.description,
+            target_branch=target_branch,
+            prerequisite_branch=prerequisite_branch,
+            description=description,
             needs_review=True, review_requests=review_requests)
-        self.superseded_by = proposal
+        if not break_link:
+            self.superseded_by = proposal
         # This sync update is needed to ensure that the transitive
         # properties of supersedes and superseded_by are visible to
         # the old and the new proposal.

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2010-11-03 04:27:13 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2010-11-11 02:54:49 +0000
@@ -1659,6 +1659,9 @@
             bmp2.queue_status, BranchMergeProposalStatus.NEEDS_REVIEW)
         self.assertEqual(
             bmp2, bmp1.superseded_by)
+        self.assertEqual(bmp1.source_branch, bmp2.source_branch)
+        self.assertEqual(bmp1.target_branch, bmp2.target_branch)
+        self.assertEqual(bmp1.prerequisite_branch, bmp2.prerequisite_branch)
 
     def test_resubmit_re_requests_review(self):
         """Resubmit should request new reviews.
@@ -1680,6 +1683,41 @@
                  (reviewer, 'specious')]),
             set((vote.reviewer, vote.review_type) for vote in bmp2.votes))
 
+    def test_resubmit_no_reviewers(self):
+        """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)
+
+    def test_resubmit_changes_branches(self):
+        """Resubmit changes branches, if specified."""
+        original = self.factory.makeBranchMergeProposal()
+        self.useContext(person_logged_in(original.registrant))
+        branch_target = original.source_branch.target
+        new_source = self.factory.makeBranchTargetBranch(branch_target)
+        new_target = self.factory.makeBranchTargetBranch(branch_target)
+        new_prerequisite = self.factory.makeBranchTargetBranch(branch_target)
+        revised = original.resubmit(original.registrant, new_source,
+                new_target, new_prerequisite)
+        self.assertEqual(new_source, revised.source_branch)
+        self.assertEqual(new_target, revised.target_branch)
+        self.assertEqual(new_prerequisite, revised.prerequisite_branch)
+
+    def test_resubmit_changes_description(self):
+        """Resubmit changes description, if specified."""
+        original = self.factory.makeBranchMergeProposal()
+        self.useContext(person_logged_in(original.registrant))
+        revised = original.resubmit(original.registrant, description='foo')
+        self.assertEqual('foo', revised.description)
+
+    def test_resubmit_breaks_link(self):
+        """Resubmit breaks link, if specified."""
+        original = self.factory.makeBranchMergeProposal()
+        self.useContext(person_logged_in(original.registrant))
+        revised = original.resubmit(
+            original.registrant, break_link=True)
+        self.assertIs(None, original.superseded_by)
+
 
 class TestCreateMergeProposalJob(TestCaseWithFactory):
     """Tests for CreateMergeProposalJob."""

=== modified file 'lib/lp/code/templates/branchmergeproposal-resubmit.pt'
--- lib/lp/code/templates/branchmergeproposal-resubmit.pt	2009-10-29 13:58:43 +0000
+++ lib/lp/code/templates/branchmergeproposal-resubmit.pt	2010-11-11 02:54:49 +0000
@@ -14,14 +14,12 @@
     <div metal:fill-slot="extra_info">
       <p>
         Resubmitting this proposal to merge will cause this proposal to be
-        marked as <strong>superseded</strong>.  Another merge proposal will
-        be created, with the same source, target and prerequisite branch
-        (if any).
+        marked as <strong>superseded</strong>.  A new proposal will
+        be created.
       </p>
       <p>
-        Everyone who has reviewed the previous proposal or was requested to
-        review the previous proposal will be requested to review the new
-        proposal.
+        Everyone who has reviewed this proposal or was requested to review this
+        proposal will be requested to review the new proposal.
       </p>
     </div>
   </div>