launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/redirect-201 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1080762 in Launchpad itself: "Merge proposal oopses on non-ASCII character in description"
  https://bugs.launchpad.net/launchpad/+bug/1080762

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/redirect-201/+merge/136198

I'm getting oopses while trying to submit a merge proposal where the
description has a non-ASCII character in it. The character in question
was a Unicode emdash.

The traceback shows that the MP was was created, the error happened when
that page is rendeder. While the unicode error is odd, it is equally
odd that the view is rendering HTML for an ajax view with a status
of 201. The work is wasteful.

--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * The action is setting 201 and the location so that the ajax method
      will issue its own redirect.
      * When the HTML for is used, the action sets next_url which sets up
        a redirection view and does not render the template.
    * Add 201 to the list of statuses that the LaunchpadView does not
      render a content.

QA

    * Visit https://code.qastaging.launchpad.net/~sinzui/launchpad/mailman-archive-0/+register-merge
    * Enter "This has a mdash — that I can see" in the comment field.
    * Submit the MP.
    * Verify your browser loads the MP.

LINT

    lib/lp/app/doc/launchpadview.txt
    lib/lp/services/webapp/publisher.py
    lib/lp/services/webapp/tests/test_publisher.py

LoC

    I have a 16,000 credit this week

TEST

    ./bin/test -vvc -t LaunchpadView lp.services.webapp.tests.test_publisher
    ./bin/test -vvc -t launchpadview.txt lp.app.tests.test_doc

IMPLEMENTATION

I added unittests for _isRedirected() and how __call__() uses it. Added
201 to the list of statuses that do not render page content. I removed
a doctest that was replaced by a unittest.
    lib/lp/app/doc/launchpadview.txt
    lib/lp/services/webapp/publisher.py
    lib/lp/services/webapp/tests/test_publisher.py

Do not render html when status is 201.
-- 
https://code.launchpad.net/~sinzui/launchpad/redirect-201/+merge/136198
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/redirect-201 into lp:launchpad.

=== modified file 'lib/lp/app/doc/launchpadview.txt'
--- lib/lp/app/doc/launchpadview.txt	2011-12-24 17:49:30 +0000
+++ lib/lp/app/doc/launchpadview.txt	2012-11-26 15:11:22 +0000
@@ -51,25 +51,6 @@
     >>> print view.user.name
     name16
 
-When the initialize method does a redirection, the render method should
-not be called.
-
-    >>> class MyRedirectView(LaunchpadView):
-    ...
-    ...     def initialize(self):
-    ...         print "Redirecting..."
-    ...         self.request.response.redirect('http://localhost/')
-    ...
-    ...     def render(self):
-    ...         return "Should not be called."
-    ...
-    >>> request = LaunchpadTestRequest()
-    >>> view = MyRedirectView(context, request)
-    >>> result = view()
-    Redirecting...
-    >>> result
-    u''
-
 A view can have `error_message` or `info_message` set for display in
 the template (each template is responsible for including the messages,
 using a `structure` directive). The supplied value must be None or

=== modified file 'lib/lp/services/webapp/publisher.py'
--- lib/lp/services/webapp/publisher.py	2012-10-04 20:38:22 +0000
+++ lib/lp/services/webapp/publisher.py	2012-11-26 15:11:22 +0000
@@ -431,7 +431,7 @@
 
         Check if the response status is one of 301, 302, 303 or 307.
         """
-        return self.request.response.getStatus() in [301, 302, 303, 307]
+        return self.request.response.getStatus() in [201, 301, 302, 303, 307]
 
     def __call__(self):
         self.initialize()

=== modified file 'lib/lp/services/webapp/tests/test_publisher.py'
--- lib/lp/services/webapp/tests/test_publisher.py	2012-09-11 02:01:52 +0000
+++ lib/lp/services/webapp/tests/test_publisher.py	2012-11-26 15:11:22 +0000
@@ -140,6 +140,27 @@
         self.assertIsCanada(json_dict['context'])
         self.assertIn('my_bool', json_dict)
 
+    def test_isRedirected_status_codes(self):
+        request = LaunchpadTestRequest()
+        view = LaunchpadView(object(), request)
+        for code in [201, 301, 302, 303, 307]:
+            request.response.setStatus(code)
+            self.assertTrue(view._isRedirected())
+        for code in [100, 200, 403, 404, 500]:
+            request.response.setStatus(code)
+            self.assertFalse(view._isRedirected())
+
+    def test_call_render_with_isRedirected(self):
+        class TestView(LaunchpadView):
+            def render(self):
+                return u'rendered'
+        request = LaunchpadTestRequest()
+        view = TestView(object(), request)
+        request.response.setStatus(200)
+        self.assertEqual(u'rendered', view())
+        request.response.setStatus(301)
+        self.assertEqual(u'', view())
+
     def test_related_feature_info__default(self):
         # By default, LaunchpadView.related_feature_info is empty.
         request = LaunchpadTestRequest()