Merge lp:~gmb/launchpad/bug-197775 into lp:launchpad

[Graham Binns]

[r=jcsackett,sinzui][ui=none][bug=197775] Submitting the +subscribe form of a StructuralSubscription target will now take you to the context's canonical_url rather than returning you to the form.

This branch fixes bug 197775 by changing the next_url for StructuralSubscriptionView to be the canonical URL of the context rather than the context's +subscribe page.

[j.c.sackett (jcsackett)]

Graham--

I like anything that gets rid of manual url hacking/building in the code. This will be nice.

I have a few comments below.

> === modified file 'lib/canonical/launchpad/ftests/_launchpadformharness.py'
> --- lib/canonical/launchpad/ftests/_launchpadformharness.py 2010-08-20 20:31:18 +0000
> +++ lib/canonical/launchpad/ftests/_launchpadformharness.py 2010-11-18 14:02:07 +0000
> @@ -19,15 +19,17 @@
> class LaunchpadFormHarness:
>
> def __init__(self, context, view_class, form_values=None,
> - request_class=LaunchpadTestRequest):
> + request_class=LaunchpadTestRequest, request_environ=None):
> self.context = context
> self.view_class = view_class
> self.request_class = request_class
> + self.request_environ = request_environ
> self._render(form_values)
>
> def _render(self, form_values=None, method='GET'):
> self.request = self.request_class(
> - method=method, form=form_values, PATH_INFO='/')
> + method=method, form=form_values, PATH_INFO='/',
> + environ=self.request_environ)
> if queryInteraction() is not None:
> self.request.setPrincipal(get_current_principal())
> # Setup a new interaction using self.request, create the view,
> @@ -58,3 +60,4 @@
>
> def redirectionTarget(self):
> return self.request.response.getHeader('location')
> + self.request_environ = request_environ

Doesn't this require passing in request_environ as an argument? Or is it in scope in a way I have missed?

> === modified file 'lib/lp/registry/browser/structuralsubscription.py'
> --- lib/lp/registry/browser/structuralsubscription.py 2010-11-10 11:13:05 +0000
> +++ lib/lp/registry/browser/structuralsubscription.py 2010-11-18 14:02:07 +0000
> @@ -205,7 +205,7 @@
> self._handleUserSubscription(data)
> self._handleTeamSubscriptions(data)
> self._handleDriverChanges(data)
> - self.next_url = canonical_url(self.context) + '/+subscribe'
> + self.next_url = canonical_url(self.context)
>
> def _handleUserSubscription(self, data):
> """Process the subscription for the user."""
>
> === modified file 'lib/lp/registry/browser/tests/test_structuralsubscription.py'
> --- lib/lp/registry/browser/tests/test_structuralsubscription.py 2010-11-10 11:33:45 +0000
> +++ lib/lp/registry/browser/tests/test_structuralsubscription.py 2010-11-18 14:02:07 +0000
> @@ -282,5 +282,35 @@
> self.target = self.factory.makeMilestone()

There are a couple of lint errors in the test file:

> +class TestStructuralSubscriptionView(TestCaseWithFactory):
> + """General tests for the StructuralSubscriptionView."""
> +
> + layer = LaunchpadFunctionalLayer
> +
> + def test_next_url_set_to_context(self):
> + # When the StructuralSubscriptionView form is submitted, the
> + # view's next_url is set to the canonical_url of the current
> + # target.
> + target = self.factory.makeProduct()
> + person = self.factory.makePerson()
> + with person_logged_in(person):
> + harness = LaunchpadFormHarness(
> + target, ...

[Graham Binns (gmb)]

I've made the changes as requested.

[j.c.sackett (jcsackett)]

Thanks for making those changes, Graham.

review: Approve (code*)

[Curtis Hovey (sinzui)]

Does this test really require the librarian (LaunchpadFunctionalLayer)? This looks like it requires DatabaseFunctionalLayer.

Why isn't this tested with lp.testing.views.create_initialized_view?

    view = create_initialized_view(target, name='+subscribe', form=form)
    self.assertEqual(canonical_url(target), view.next_url)

We can remove the loadTestsFromName() stuff. The testrunner will find the test without
it.

review: Needs Information (code)

[Graham Binns (gmb)]

> Does this test really require the librarian (LaunchpadFunctionalLayer)? This
> looks like it requires DatabaseFunctionalLayer.
>

It doesn't. Fixed.

> Why isn't this tested with lp.testing.views.create_initialized_view?
>
> view = create_initialized_view(target, name='+subscribe', form=form)
> self.assertEqual(canonical_url(target), view.next_url)
>

Out of habit. I've been using the harness elsewhere. Fixed.

> We can remove the loadTestsFromName() stuff. The testrunner will find the test
> without
> it.

Done.

[Curtis Hovey (sinzui)]

Thank you for addressing my concerns,

review: Approve (code)

=== modified file 'lib/canonical/launchpad/ftests/_launchpadformharness.py'
--- lib/canonical/launchpad/ftests/_launchpadformharness.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/ftests/_launchpadformharness.py	2010-11-23 13:30:34 +0000
@@ -19,15 +19,17 @@
 class LaunchpadFormHarness:
 
     def __init__(self, context, view_class, form_values=None,
-                 request_class=LaunchpadTestRequest):
+                 request_class=LaunchpadTestRequest, request_environ=None):
         self.context = context
         self.view_class = view_class
         self.request_class = request_class
+        self.request_environ = request_environ
         self._render(form_values)
 
     def _render(self, form_values=None, method='GET'):
         self.request = self.request_class(
-            method=method, form=form_values, PATH_INFO='/')
+            method=method, form=form_values, PATH_INFO='/',
+            environ=self.request_environ)
         if queryInteraction() is not None:
             self.request.setPrincipal(get_current_principal())
         # Setup a new interaction using self.request, create the view,

=== modified file 'lib/lp/bugs/stories/structural-subscriptions/xx-bug-subscriptions.txt'
--- lib/lp/bugs/stories/structural-subscriptions/xx-bug-subscriptions.txt	2010-10-17 15:44:08 +0000
+++ lib/lp/bugs/stories/structural-subscriptions/xx-bug-subscriptions.txt	2010-11-23 13:30:34 +0000
@@ -15,6 +15,8 @@
     >>> subscribe_team.selected = True
     >>> browser.getControl('Save these changes').click()
 
+    >>> browser.open(
+    ... 'http://bugs.launchpad.dev/ubuntu/+subscribe')
     >>> print extract_text(find_portlet(browser.contents, 'Bug subscriptions'))
     Bug subscriptions
       For Ubuntu Linux:
@@ -38,6 +40,8 @@
 
 Sample Person and the Landscape team are now subscribed.
 
+    >>> browser.open(
+    ... 'http://bugs.launchpad.dev/ubuntu/+source/mozilla-firefox/+subscribe')
     >>> print extract_text(find_portlet(
     ...     browser.contents, 'Bug subscriptions')).encode(
     ...     'ascii', 'backslashreplace')
@@ -57,6 +61,8 @@
     >>> subscribe_team = browser.getControl('Landscape')
     >>> subscribe_team.selected = False
     >>> browser.getControl('Save these changes').click()
+    >>> browser.open(
+    ... 'http://bugs.launchpad.dev/ubuntu/+source/mozilla-firefox/+subscribe')
     >>> print extract_text(find_portlet(browser.contents, 'Bug subscriptions'))
     Bug subscriptions
       For...mozilla-firefox...package in Ubuntu:
@@ -102,6 +108,8 @@
     No Privileges Person will now receive an e-mail each time someone reports
     or changes a public bug in "mozilla-firefox in ubuntu".
 
+    >>> browser.open(
+    ... 'http://bugs.launchpad.dev/ubuntu/+source/mozilla-firefox/+subscribe')
     >>> print extract_text(find_portlet(browser.contents, 'Bug subscriptions'))
     Bug subscriptions
       For...mozilla-firefox...package in Ubuntu:
@@ -122,6 +130,8 @@
     ...    browser.contents, 'informational message')[0].contents[0]
     No Privileges Person will no longer automatically receive e-mail about
     public bugs in "mozilla-firefox in ubuntu".
+    >>> browser.open(
+    ... 'http://bugs.launchpad.dev/ubuntu/+source/mozilla-firefox/+subscribe')
     >>> print extract_text(find_portlet(browser.contents, 'Bug subscriptions'))
     Bug subscriptions
       For...mozilla-firefox...package in Ubuntu:
@@ -160,6 +170,8 @@
     >>> subscribe_other = browser.getControl('Subscribe someone else:')
     >>> subscribe_other.value = 'nopriv'
     >>> browser.getControl('Save these changes').click()
+    >>> browser.open(
+    ... 'http://bugs.launchpad.dev/ubuntu/+source/mozilla-firefox/+subscribe')
     >>> print extract_text(find_portlet(browser.contents, 'Bug subscriptions'))
     Bug subscriptions
       For...mozilla-firefox...package in Ubuntu:

=== modified file 'lib/lp/registry/browser/structuralsubscription.py'
--- lib/lp/registry/browser/structuralsubscription.py	2010-11-10 11:13:05 +0000
+++ lib/lp/registry/browser/structuralsubscription.py	2010-11-23 13:30:34 +0000
@@ -80,6 +80,10 @@
     def label(self):
         return self.page_title
 
+    @property
+    def next_url(self):
+        return canonical_url(self.context)
+
     def setUpFields(self):
         """See LaunchpadFormView."""
         LaunchpadFormView.setUpFields(self)
@@ -205,7 +209,6 @@
         self._handleUserSubscription(data)
         self._handleTeamSubscriptions(data)
         self._handleDriverChanges(data)
-        self.next_url = canonical_url(self.context) + '/+subscribe'
 
     def _handleUserSubscription(self, data):
         """Process the subscription for the user."""

=== modified file 'lib/lp/registry/browser/tests/test_structuralsubscription.py'
--- lib/lp/registry/browser/tests/test_structuralsubscription.py	2010-11-18 16:13:19 +0000
+++ lib/lp/registry/browser/tests/test_structuralsubscription.py	2010-11-23 13:30:34 +0000
@@ -37,6 +37,7 @@
     set_feature_flag,
     TestCaseWithFactory,
     )
+from lp.testing.views import create_initialized_view
 
 
 class FakeLaunchpadRequest(FakeRequest):
@@ -293,5 +294,19 @@
         self.target = self.factory.makeMilestone()
 
 
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+class TestStructuralSubscriptionView(TestCaseWithFactory):
+    """General tests for the StructuralSubscriptionView."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_next_url_set_to_context(self):
+        # When the StructuralSubscriptionView form is submitted, the
+        # view's next_url is set to the canonical_url of the current
+        # target.
+        target = self.factory.makeProduct()
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            view = create_initialized_view(target, name='+subscribe')
+            self.assertEqual(
+                canonical_url(target), view.next_url,
+                "Next URL does not match target's canonical_url.")