Merge lp:~thumper/launchpad/propose-merging-extra-options into lp:launchpad

[Tim Penhey]

Make prerequisite branch and reviewer fields extra options in the propose for merging page.

[Tim Penhey (thumper)]

Makes prerequisite branch, reviewer and review type extra options when proposing a merge.

Also adds in commit message and needs review options.

[Paul Hummer (rockstar)]

Thanks for taking noodles and I through this branch's UI. The flow is more similar to the new bug UI as well now, so it looks good. Thanks for doing this.

review: Approve (code ui*)

[Michael Nelson (michael.nelson)]

review: Approve (ui)

=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css	2010-01-06 14:14:35 +0000
+++ lib/canonical/launchpad/icing/style.css	2010-01-17 06:03:17 +0000
@@ -241,9 +241,11 @@
 textarea {width: 90%; max-width: 60em;}
 
 /* Many forms are laid out using tables, with appropriate spacing: */
-table.form {margin: 1em 0; width: 100%;} /* http://launchpad.dev/firefox/+edit */
+table.form, table.extra-options {margin: 1em 0; width: 100%;} /* http://launchpad.dev/firefox/+edit */
 table.form th {font-weight: normal;}
-table.form th, table.form td {padding-bottom: 1em;}
+table.form th, table.form td,
+table.form table.extra-options td,
+table.form table.extra-options th {padding-bottom: 1em;}
 table.form td td {padding-bottom: 0;}
 
 /* === Tables === */

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2010-01-07 05:03:46 +0000
+++ lib/lp/code/browser/branch.py	2010-01-17 06:03:17 +0000
@@ -41,7 +41,7 @@
 from zope.formlib import form
 from zope.interface import Interface, implements, providedBy
 from zope.publisher.interfaces import NotFound
-from zope.schema import Choice, Text
+from zope.schema import Bool, Choice, Text
 from lazr.delegates import delegates
 from lazr.enum import EnumeratedType, Item
 from lazr.lifecycle.event import ObjectModifiedEvent
@@ -85,6 +85,7 @@
 from lp.code.interfaces.branch import (
     BranchCreationForbidden, BranchExists, IBranch,
     user_has_special_branch_access)
+from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
 from lp.code.interfaces.branchtarget import IBranchTarget
 from lp.code.interfaces.codeimportjob import ICodeImportJobWorkflow
 from lp.code.interfaces.branchnamespace import IBranchNamespacePolicy
@@ -1156,6 +1157,13 @@
         description=u'Lowercase keywords describing the type of review you '
                      'would like to be performed.')
 
+    commit_message = IBranchMergeProposal['commit_message']
+
+    needs_review = Bool(
+        title=_("Needs review"), required=True, default=True,
+        description=_(
+            "Is the proposal ready for review now?"))
+
 
 class RegisterBranchMergeProposalView(LaunchpadFormView):
     """The view to register new branch merge proposals."""
@@ -1205,14 +1213,13 @@
             review_requests.append((reviewer, data.get('review_type')))
 
         try:
-            # Always default to needs review until we have the wonder of AJAX
-            # and an advanced expandable section.
             proposal = source_branch.addLandingTarget(
                 registrant=registrant, target_branch=target_branch,
-                prerequisite_branch=prerequisite_branch, needs_review=True,
+                prerequisite_branch=prerequisite_branch,
+                needs_review=data['needs_review'],
                 initial_comment=data.get('comment'),
-                review_requests=review_requests)
-
+                review_requests=review_requests,
+                commit_message=data.get('commit_message'))
             self.next_url = canonical_url(proposal)
         except InvalidBranchMergeProposal, error:
             self.addError(str(error))

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2010-01-14 01:48:19 +0000
+++ lib/lp/code/browser/configure.zcml	2010-01-17 06:03:17 +0000
@@ -519,7 +519,7 @@
         class="lp.code.browser.branch.RegisterBranchMergeProposalView"
         facet="branches"
         permission="launchpad.AnyPerson"
-        template="../../app/templates/generic-edit.pt"/>
+        template="../templates/branch-register-merge.pt"/>
     <browser:page
         name="+linkbug"
         for="lp.code.interfaces.branch.IBranch"

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2010-01-06 17:25:04 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2010-01-17 06:03:17 +0000
@@ -351,14 +351,11 @@
         return view
 
     def _getSourceProposal(self):
-        # There will only be one proposal and it will be in needs review
-        # state.
+        # There will only be one proposal.
         landing_targets = list(self.source_branch.landing_targets)
         self.assertEqual(1, len(landing_targets))
         proposal = landing_targets[0]
         self.assertEqual(self.target_branch, proposal.target_branch)
-        self.assertEqual(BranchMergeProposalStatus.NEEDS_REVIEW,
-                         proposal.queue_status)
         return proposal
 
     def assertNoComments(self, proposal):
@@ -393,18 +390,43 @@
         # This simplest case is where the user only specifies the target
         # branch, and not an initial comment or reviewer.
         view = self._createView()
-        view.register_action.success({'target_branch': self.target_branch})
+        view.register_action.success(
+            {'target_branch': self.target_branch,
+             'needs_review': True})
         proposal = self._getSourceProposal()
         self.assertNoPendingReviews(proposal)
         self.assertNoComments(proposal)
 
+    def test_register_work_in_progress(self):
+        # The needs review checkbox can be unchecked to create a work in
+        # progress proposal.
+        view = self._createView()
+        view.register_action.success(
+            {'target_branch': self.target_branch,
+             'needs_review': False})
+        proposal = self._getSourceProposal()
+        self.assertEqual(
+            BranchMergeProposalStatus.WORK_IN_PROGRESS,
+            proposal.queue_status)
+
+    def test_register_with_commit_message(self):
+        # A commit message can also be set during the register process.
+        view = self._createView()
+        view.register_action.success(
+            {'target_branch': self.target_branch,
+             'needs_review': True,
+             'commit_message': 'Fixed the bug!'})
+        proposal = self._getSourceProposal()
+        self.assertEqual('Fixed the bug!', proposal.commit_message)
+
     def test_register_initial_comment(self):
         # If the user specifies an initial comment, this is added to the
         # proposal.
         view = self._createView()
         view.register_action.success(
             {'target_branch': self.target_branch,
-             'comment': "This is the first comment."})
+             'comment': "This is the first comment.",
+             'needs_review': True})
 
         proposal = self._getSourceProposal()
         self.assertNoPendingReviews(proposal)
@@ -417,7 +439,8 @@
         view = self._createView()
         view.register_action.success(
             {'target_branch': self.target_branch,
-             'reviewer': reviewer})
+             'reviewer': reviewer,
+             'needs_review': True})
 
         proposal = self._getSourceProposal()
         self.assertOnePendingReview(proposal, reviewer)
@@ -431,7 +454,8 @@
         view.register_action.success(
             {'target_branch': self.target_branch,
              'reviewer': reviewer,
-             'review_type': 'god-like'})
+             'review_type': 'god-like',
+             'needs_review': True})
 
         proposal = self._getSourceProposal()
         self.assertOnePendingReview(proposal, reviewer, 'god-like')
@@ -446,7 +470,8 @@
             {'target_branch': self.target_branch,
              'reviewer': reviewer,
              'review_type': 'god-like',
-             'comment': "This is the first comment."})
+             'comment': "This is the first comment.",
+             'needs_review': True})
 
         proposal = self._getSourceProposal()
         self.assertOnePendingReview(proposal, reviewer, 'god-like')

=== added file 'lib/lp/code/stories/branches/xx-propose-for-merging.txt'
--- lib/lp/code/stories/branches/xx-propose-for-merging.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/code/stories/branches/xx-propose-for-merging.txt	2010-01-17 06:03:17 +0000
@@ -0,0 +1,59 @@
+Propose a branch for merging
+============================
+
+    >>> login(ANONYMOUS)
+    >>> from lp.code.tests.helpers import make_erics_fooix_project
+    >>> locals().update(make_erics_fooix_project(factory))
+    >>> branch = factory.makeProductBranch(product=fooix)
+    >>> url = canonical_url(branch)
+    >>> logout()
+
+When proposing a branch for merging, the minimum that is needed is a target
+branch.
+
+    >>> browser = setupBrowser(auth='Basic [e-mail]:test')
+    >>> browser.open(url)
+    >>> browser.getLink('Propose for merging').click()
+
+    >>> def print_radio_buttons(browser):
+    ...     main = find_main_content(browser.contents)
+    ...     for button in main.findAll('input', attrs={'type': 'radio'}):
+    ...         try:
+    ...             if button['checked']:
+    ...                 checked = '(*)'
+    ...             else:
+    ...                 checked = '( )'
+    ...         except KeyError:
+    ...             checked = '( )'
+    ...         print checked, button['value']
+
+    >>> print_radio_buttons(browser)
+    (*) ~eric/fooix/trunk
+    ( ) other
+    >>> browser.getControl('Propose Merge').click()
+    >>> print_tag_with_id(browser.contents, 'proposal-summary')
+    Status: Needs review
+    Proposed branch: ...
+    Merge into: lp://dev/fooix
+
+
+Work in progress
+----------------
+
+Sometimes a proposal is wanted for the diff against the target, but the code
+is not yet ready for review.  In these situations the user can uncheck the
+needs review checkbox in the extra options.
+
+    >>> login(ANONYMOUS)
+    >>> branch = factory.makeProductBranch(product=fooix)
+    >>> url = canonical_url(branch)
+    >>> logout()
+
+    >>> browser.open(url)
+    >>> browser.getLink('Propose for merging').click()
+    >>> browser.getControl('Needs review').click()
+    >>> browser.getControl('Propose Merge').click()
+    >>> print_tag_with_id(browser.contents, 'proposal-summary')
+    Status: Work in progress
+    Proposed branch: ...
+    Merge into: lp://dev/fooix

=== added file 'lib/lp/code/templates/branch-register-merge.pt'
--- lib/lp/code/templates/branch-register-merge.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/code/templates/branch-register-merge.pt	2010-01-17 06:03:17 +0000
@@ -0,0 +1,60 @@
+<html
+  xmlns="http://www.w3.org/1999/xhtml"
+  xmlns:tal="http://xml.zope.org/namespaces/tal"
+  xmlns:metal="http://xml.zope.org/namespaces/metal"
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
+  metal:use-macro="view/macro:page/main_only"
+  i18n:domain="launchpad">
+  <body>
+    <div metal:fill-slot="main">
+      <div metal:use-macro="context/@@launchpad_form/form">
+
+        <metal:formbody fill-slot="widgets">
+          <table class="form">
+
+            <tal:widget define="widget nocall:view/widgets/target_branch">
+              <metal:block use-macro="context/@@launchpad_form/widget_row" />
+            </tal:widget>
+
+            <tal:widget define="widget nocall:view/widgets/comment">
+              <metal:block use-macro="context/@@launchpad_form/widget_row" />
+            </tal:widget>
+
+            <td colspan="2">
+              <fieldset id="mergeproposal-extra-options"
+                        class="collapsible collapsed">
+                <legend>Extra options</legend>
+                <table class="extra-options">
+
+                  <tal:widget define="widget nocall:view/widgets/commit_message">
+                    <metal:block use-macro="context/@@launchpad_form/widget_row" />
+                  </tal:widget>
+
+                  <tal:widget define="widget nocall:view/widgets/reviewer">
+                    <metal:block use-macro="context/@@launchpad_form/widget_row" />
+                  </tal:widget>
+
+                  <tal:widget define="widget nocall:view/widgets/review_type">
+                    <metal:block use-macro="context/@@launchpad_form/widget_row" />
+                  </tal:widget>
+
+                  <tal:widget define="widget nocall:view/widgets/needs_review">
+                    <metal:block use-macro="context/@@launchpad_form/widget_row" />
+                  </tal:widget>
+
+                  <tal:widget define="widget nocall:view/widgets/prerequisite_branch">
+                    <metal:block use-macro="context/@@launchpad_form/widget_row" />
+                  </tal:widget>
+
+
+
+                </table>
+              </fieldset>
+            </td>
+
+          </table>
+        </metal:formbody>
+      </div>
+    </div>
+  </body>
+</html>