launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/merge-request-url-refresh-929422 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #929422 in Launchpad itself: "Fails to refresh the URL when making a merge request"
  https://bugs.launchpad.net/launchpad/+bug/929422

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/merge-request-url-refresh-929422/+merge/92901

== Implementation ==

If a XHR request is used to submit a mp registration, and there's no confirmation prompt required, the browser needs to redirect to the new mp url. So we return a status of 201 and the location. The browser then does the redirect.

== Tests ==

Add a new test to TestRegisterBranchMergeProposalView to test the redirect:
- test_register_ajax_request_with_no_confirmation

Modify the yui tests also.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/branch.py
  lib/lp/code/browser/tests/test_branchmergeproposal.py
  lib/lp/code/javascript/branchmergeproposal.nominate.js
  lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js


-- 
https://code.launchpad.net/~wallyworld/launchpad/merge-request-url-refresh-929422/+merge/92901
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/merge-request-url-refresh-929422 into lp:launchpad.

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2012-02-07 11:51:59 +0000
+++ lib/lp/code/browser/branch.py	2012-02-14 03:56:19 +0000
@@ -1372,7 +1372,6 @@
                 description=data.get('comment'),
                 review_requests=review_requests,
                 commit_message=data.get('commit_message'))
-            self.next_url = canonical_url(proposal)
             if len(visible_branches) < 2:
                 invisible_branches = [branch.unique_name
                             for branch in [source_branch, target_branch]
@@ -1381,6 +1380,14 @@
                     'To ensure visibility, %s is now subscribed to: %s'
                     % (visibility_info['person_name'],
                        english_list(invisible_branches)))
+            # Success so we do a client redirect to the new mp page.
+            if self.request.is_ajax:
+                self.request.response.setStatus(201)
+                self.request.response.setHeader(
+                    'Location', canonical_url(proposal))
+                return None
+            else:
+                self.next_url = canonical_url(proposal)
         except InvalidBranchMergeProposal, error:
             self.addError(str(error))
 

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2012-02-06 12:23:47 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2012-02-14 03:56:19 +0000
@@ -465,7 +465,7 @@
         self.assertOnePendingReview(proposal, target_branch.owner)
         self.assertIs(None, proposal.description)
 
-    def test_register_ajax_request(self):
+    def test_register_ajax_request_with_confirmation(self):
         # Ajax submits return json data containing info about what the visible
         # branches are if they are not all visible to the reviewer.
 
@@ -490,6 +490,27 @@
                  'needs_review': True})
         self.assertEqual(expected_data, simplejson.loads(result_data))
 
+    def test_register_ajax_request_with_no_confirmation(self):
+        # Ajax submits where there is no confirmation required return a 201
+        # with the new location.
+        owner = self.factory.makePerson()
+        target_branch = self._makeTargetBranch()
+        reviewer = self.factory.makePerson()
+        extra = {'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest'}
+        request = LaunchpadTestRequest(
+            method='POST', principal=owner, **extra)
+        view = self._createView(request=request)
+        with person_logged_in(owner):
+            result_data = view.register_action.success(
+                {'target_branch': target_branch,
+                 'reviewer': reviewer,
+                 'needs_review': True})
+        self.assertEqual(None, result_data)
+        self.assertEqual(201, view.request.response.getStatus())
+        mp = target_branch.getMergeProposals()[0]
+        self.assertEqual(
+            canonical_url(mp), view.request.response.getHeader('Location'))
+
     def test_register_work_in_progress(self):
         # The needs review checkbox can be unchecked to create a work in
         # progress proposal.

=== modified file 'lib/lp/code/javascript/branchmergeproposal.nominate.js'
--- lib/lp/code/javascript/branchmergeproposal.nominate.js	2012-02-07 22:12:34 +0000
+++ lib/lp/code/javascript/branchmergeproposal.nominate.js	2012-02-14 03:56:19 +0000
@@ -161,13 +161,13 @@
 };
 
 /**
- * Replace the entire document contents with the new html.
- * @param html
+ * Redirect to a new URL. We need to break this out to allow testing.
+ *
+ * @method _redirect
+ * @param url
  */
-var render_new_page = function(html) {
-    document.open();
-    document.write(html);
-    document.close();
+var _redirect = function(url) {
+    window.location.replace(url);
 };
 
 /**
@@ -199,15 +199,12 @@
                 failure: error_handler.getFailureHandler(),
                 success:
                     function(id, response) {
-                        var content_type
-                            = response.getResponseHeader('Content-Type');
-                        // If we get html back, that means there's been an
-                        // error detected on the server and so we need to
-                        // display the error page.
-                        if (content_type.indexOf('application/json') < 0) {
-                            namespace.render_new_page(response.responseText);
+                        if (response.status === 201) {
+                            namespace._redirect(
+                                response.getResponseHeader("Location"));
                         } else {
-                            var branch_info = Y.JSON.parse(response.responseText);
+                            var branch_info =
+                                Y.JSON.parse(response.responseText);
                             namespace.confirm_reviewer_nomination(branch_info);
                         }
                     }
@@ -238,7 +235,7 @@
 namespace.confirm_reviewer = confirm_reviewer;
 namespace.confirm_reviewer_nomination = confirm_reviewer_nomination;
 namespace.setup_nominate_submit = setup_nominate_submit;
-namespace.render_new_page = render_new_page;
+namespace._redirect = _redirect;
 
 // We want to disable the review_type field if no reviewer is
 // specified. In such cases, the reviewer will be set by the back end

=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js	2012-02-07 11:51:59 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js	2012-02-14 03:56:19 +0000
@@ -256,15 +256,15 @@
         form.submit = orig_submit;
     },
 
-    // Test that when an validation error occurs during the io call, the html
-    // returned by the call is properly rendered.
-    test_confirm_reviewer_nomination_error: function() {
-        var orig_render_new_page
-            = module.render_new_page;
-        var render_page_called = false;
-        module.render_new_page = function(html) {
-            Y.Assert.areEqual('<html>Hello World</html>', html);
-            render_page_called = true;
+    // Test that when a mp is submitted without any confirmation prompt being
+    // required, the response is used to redirect to the new page.
+    test_merge_proposal_submission: function() {
+        var orig_redirect
+            = module._redirect;
+        var redirect_called = false;
+        module._redirect = function(url) {
+            Y.Assert.areEqual('http://foo', url);
+            redirect_called = true;
         };
 
         module.setup_nominate_submit(this.mockio);
@@ -275,10 +275,10 @@
         var submit_button = Y.one("[name='field.actions.register']");
         submit_button.simulate('click');
         this.mockio.success({
-            responseText: '<html>Hello World</html>',
-            responseHeaders: {'Content-Type': 'text/html'}});
-        Y.Assert.isTrue(render_page_called);
-        module.render_new_page = orig_render_new_page;
+            status: 201,
+            responseHeaders: {'Location': 'http://foo'}});
+        Y.Assert.isTrue(redirect_called);
+        module._redirect = orig_redirect;
     }
 })));