Merge lp:~abentley/launchpad/move-reviewer-selection into lp:launchpad

[Aaron Bentley]

[r=thumper][ui=none][bug=525424] Move reviewer and review type outside extra options

= Summary =
Fix bug #525424: Moving the reviewer option in to the expander leads to people
requesting more than they want

== Proposed fix ==
Move the reviewer and review type outside the extra options field.

== Pre-implementation notes ==
None

== Implementation details ==
Can't think of any

== Tests ==
bin/test -t register_reviewer_not_hidden test_branchmergeproposal

== Demo and Q/A ==
Go to a branch. Click on "Propose for merging". It should show "Reviewer" and
"Review type" above the "Extra options" expander.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/templates/branch-register-merge.pt
  lib/lp/code/browser/tests/test_branchmergeproposal.py

[Tim Penhey (thumper)]

review: Approve

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2010-12-01 11:26:57 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2010-12-20 17:52:57 +0000
@@ -17,6 +17,8 @@
 import unittest
 
 import pytz
+from soupmatchers import HTMLContains, Tag
+from testtools.matchers import Not
 import transaction
 from zope.component import getMultiAdapter
 from zope.security.interfaces import Unauthorized
@@ -349,10 +351,10 @@
         view.render()
 
 
-class TestRegisterBranchMergeProposalView(TestCaseWithFactory):
+class TestRegisterBranchMergeProposalView(BrowserTestCase):
     """Test the merge proposal registration view."""
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def setUp(self):
         TestCaseWithFactory.setUp(self)
@@ -523,6 +525,15 @@
         self.assertOnePendingReview(proposal, reviewer, 'god-like')
         self.assertIs(None, proposal.description)
 
+    def test_register_reviewer_not_hidden(self):
+        branch = self.factory.makeBranch()
+        browser = self.getViewBrowser(branch, '+register-merge')
+        extra = Tag(
+            'extra', 'fieldset', attrs={'id': 'mergeproposal-extra-options'})
+        reviewer = Tag('reviewer', 'input', attrs={'id': 'field.reviewer'})
+        matcher = Not(HTMLContains(reviewer.within(extra)))
+        self.assertThat(browser.contents, matcher)
+
 
 class TestBranchMergeProposalResubmitView(TestCaseWithFactory):
     """Test BranchMergeProposalResubmitView."""

=== modified file 'lib/lp/code/templates/branch-register-merge.pt'
--- lib/lp/code/templates/branch-register-merge.pt	2010-11-08 05:45:57 +0000
+++ lib/lp/code/templates/branch-register-merge.pt	2010-12-20 17:52:57 +0000
@@ -20,6 +20,14 @@
               <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>
+
             <td colspan="2">
               <fieldset id="mergeproposal-extra-options"
                         class="collapsible collapsed">
@@ -30,14 +38,6 @@
                     <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>