Merge lp:~allenap/launchpad/dsd-batch-position-reset-bug-822781 into lp:launchpad

[Gavin Panella]

[r=abentley][bug=822781] After a successful sync request via DistroSeries:+localpackagediffs and its siblings, redirect to the page from where the request was made with the same batch and filtering parameters.

When redirecting the user after a sync on +localpackagediffs (and its
siblings), keep all the same batch and filtering options in the query
string.

[Aaron Bentley (abentley)]

This looks good, but please clean up the comment "...forms should post to themselves, including GET params..."

review: Approve

=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
--- lib/canonical/launchpad/webapp/interfaces.py	2011-08-01 15:40:06 +0000
+++ lib/canonical/launchpad/webapp/interfaces.py	2011-08-11 20:06:37 +0000
@@ -347,6 +347,13 @@
         If no matching object is found, the tuple (None, None) is returned.
         """
 
+    def getURL(level=0, path_only=False, include_query=False):
+        """See `IHTTPApplicationRequest`.
+
+        Additionally, if `include_query` is `True`, the query string is
+        included in the returned URL.
+        """
+
 
 class IBrowserFormNG(Interface):
     """Interface to manipulate submitted form data."""

=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py	2011-05-27 17:09:42 +0000
+++ lib/canonical/launchpad/webapp/servers.py	2011-08-11 20:06:37 +0000
@@ -542,6 +542,16 @@
         """See `IBasicLaunchpadRequest`."""
         return 'XMLHttpRequest' == self.getHeader('HTTP_X_REQUESTED_WITH')
 
+    def getURL(self, level=0, path_only=False, include_query=False):
+        """See `IBasicLaunchpadRequest`."""
+        sup = super(LaunchpadBrowserRequestMixin, self)
+        url = sup.getURL(level, path_only)
+        if include_query:
+            query_string = self.get('QUERY_STRING')
+            if query_string is not None and len(query_string) > 0:
+                url = "%s?%s" % (url, query_string)
+        return url
+
 
 class BasicLaunchpadRequest(LaunchpadBrowserRequestMixin):
     """Mixin request class to provide stepstogo."""
@@ -798,8 +808,8 @@
     return request.response
 
 
-class LaunchpadTestRequest(TestRequest, ErrorReportRequest,
-                           LaunchpadBrowserRequestMixin):
+class LaunchpadTestRequest(LaunchpadBrowserRequestMixin,
+                           TestRequest, ErrorReportRequest):
     """Mock request for use in unit and functional tests.
 
     >>> request = LaunchpadTestRequest(SERVER_URL='http://127.0.0.1/foo/bar')

=== modified file 'lib/canonical/launchpad/webapp/tests/test_servers.py'
--- lib/canonical/launchpad/webapp/tests/test_servers.py	2010-10-25 13:16:10 +0000
+++ lib/canonical/launchpad/webapp/tests/test_servers.py	2011-08-11 20:06:37 +0000
@@ -35,6 +35,7 @@
 from canonical.launchpad.webapp.servers import (
     ApplicationServerSettingRequestFactory,
     LaunchpadBrowserRequest,
+    LaunchpadTestRequest,
     VHostWebServiceRequestPublicationFactory,
     VirtualHostRequestPublicationFactory,
     WebServiceClientRequest,
@@ -358,20 +359,90 @@
             retried_request.response.getHeader('Vary'),
             'Cookie, Authorization')
 
+
+class TestLaunchpadBrowserRequestMixin:
+    """Tests for `LaunchpadBrowserRequestMixin`.
+
+    As `LaunchpadBrowserRequestMixin` is a mixin, it needs to be tested when
+    mixed into another class, hence why this does not inherit from `TestCase`.
+    """
+
+    request_factory = None  # Specify in subclasses.
+
     def test_is_ajax_false(self):
         """Normal requests do not define HTTP_X_REQUESTED_WITH."""
-        request = LaunchpadBrowserRequest(StringIO.StringIO(''), {})
+        request = self.request_factory(StringIO.StringIO(''), {})
 
         self.assertFalse(request.is_ajax)
 
     def test_is_ajax_true(self):
         """Requests with HTTP_X_REQUESTED_WITH set are ajax requests."""
-        request = LaunchpadBrowserRequest(StringIO.StringIO(''), {
+        request = self.request_factory(StringIO.StringIO(''), {
             'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest',
             })
 
         self.assertTrue(request.is_ajax)
 
+    def test_getURL(self):
+        """
+        getURL() overrides HTTPRequest.getURL(), but behaves identically by
+        default.
+        """
+        environ = {
+            "SERVER_URL": "http://geturl.example.com",
+            "SCRIPT_NAME": "/sabbra/cadabra",
+            "QUERY_STRING": "tuesday=gone",
+            }
+        request = self.request_factory(StringIO.StringIO(''), environ)
+        self.assertEqual(
+            "http://geturl.example.com/sabbra/cadabra",
+            request.getURL())
+        self.assertEqual(
+            "http://geturl.example.com/sabbra",
+            request.getURL(level=1))
+        self.assertEqual(
+            "/sabbra/cadabra",
+            request.getURL(path_only=True))
+
+    def test_getURL_include_query(self):
+        """
+        getURL() overrides HTTPRequest.getURL(), but appends the query string
+        if include_query=True.
+        """
+        environ = {
+            "SERVER_URL": "http://geturl.example.com",
+            "SCRIPT_NAME": "/sabbra/cadabra",
+            "QUERY_STRING": "tuesday=gone",
+            }
+        request = self.request_factory(StringIO.StringIO(''), environ)
+        self.assertEqual(
+            "http://geturl.example.com/sabbra/cadabra?tuesday=gone",
+            request.getURL(include_query=True))
+        self.assertEqual(
+            "http://geturl.example.com/sabbra?tuesday=gone",
+            request.getURL(include_query=True, level=1))
+        self.assertEqual(
+            "/sabbra/cadabra?tuesday=gone",
+            request.getURL(include_query=True, path_only=True))
+
+
+class TestLaunchpadBrowserRequestMixinWithLaunchpadBrowserRequest(
+    TestLaunchpadBrowserRequestMixin, TestCase):
+    """
+    Tests for `LaunchpadBrowserRequestMixin` as found in
+    `LaunchpadBrowserRequest`.
+    """
+    request_factory = LaunchpadBrowserRequest
+
+
+class TestLaunchpadBrowserRequestMixinWithLaunchpadTestRequest(
+    TestLaunchpadBrowserRequestMixin, TestCase):
+    """
+    Tests for `LaunchpadBrowserRequestMixin` as found in
+    `LaunchpadTestRequest`.
+    """
+    request_factory = LaunchpadTestRequest
+
 
 class IThingSet(Interface):
     """Marker interface for a set of things."""

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-08-09 15:55:15 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-08-11 20:06:37 +0000
@@ -901,16 +901,22 @@
             self.context, destination_pocket, include_binaries=False,
             dest_url=series_url, dest_display_name=series_title,
             person=self.user, force_async=True):
-            # The copy worked so we can redirect back to the page to
-            # show the results.
-            self.next_url = self.request.URL
+            # The copy worked so we redirect back to show the results. Include
+            # the query string so that the user ends up on the same batch page
+            # with the same filtering parameters as before.
+            self.next_url = self.request.getURL(include_query=True)
 
     @property
     def action_url(self):
-        """The forms should post to themselves, including GET params to
-        account for batch parameters.
+        """The request URL including query string.
+
+        Forms should post to the view with a query string containing the
+        active batch and filtering parameters. Actions should then redirect
+        using that information so that the user is left on the same batch
+        page, with the same filtering parameters, as the page from which they
+        submitted the form.
         """
-        return "%s?%s" % (self.request.getURL(), self.request['QUERY_STRING'])
+        return self.request.getURL(include_query=True)
 
     def validate_sync(self, action, data):
         """Validate selected differences."""

=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py	2011-08-09 15:55:57 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-08-11 20:06:37 +0000
@@ -10,6 +10,7 @@
 import re
 from textwrap import TextWrapper
 from urllib import urlencode
+from urlparse import urlparse
 
 from BeautifulSoup import BeautifulSoup
 from lazr.restful.interfaces import IJSONRequestCache
@@ -42,17 +43,17 @@
 from canonical.launchpad.webapp.interaction import get_current_principal
 from canonical.launchpad.webapp.interfaces import BrowserNotificationLevel
 from canonical.launchpad.webapp.publisher import canonical_url
+from canonical.launchpad.webapp.url import urlappend
 from canonical.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
     LaunchpadZopelessLayer,
     )
-from canonical.launchpad.webapp.url import urlappend
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.archivepublisher.debversion import Version
 from lp.registry.browser.distroseries import (
+    ALL,
     HIGHER_VERSION_THAN_PARENT,
-    ALL,
     NON_IGNORED,
     RESOLVED,
     seriesToVocab,
@@ -2067,11 +2068,10 @@
         # 302 is a redirect back to the same page.
         self.assertEqual(302, view.request.response.getStatus())
 
-    def test_sync_notification_on_success(self):
-        # A user with upload rights on the destination archive can
-        # sync packages. Notifications about the synced packages are
-        # displayed and the packages are copied inside the destination
-        # series.
+    def test_sync_success(self):
+        # A user with upload rights on the destination archive can sync
+        # packages. Notifications about the synced packages are displayed and
+        # the packages are copied inside the destination series.
         versions = {
             'base': '1.0',
             'derived': '1.0derived1',
@@ -2094,13 +2094,20 @@
         # Now, sync the source from the parent using the form.
         set_derived_series_sync_feature_flag(self)
         view = self._syncAndGetView(
-            derived_series, person, [diff_id])
+            derived_series, person, [diff_id], query_string=(
+                "batch=12&start=24&my-old-man=dustman"))
 
         # The parent's version should now be in the derived series and
         # the notifications displayed:
         self.assertPackageCopied(
             derived_series, 'my-src-name', versions['parent'], view)
 
+        # The URL to which the browser is redirected has same batch and
+        # filtering options as where the sync request was made.
+        self.assertEqual(
+            "batch=12&start=24&my-old-man=dustman",
+            urlparse(view.next_url).query)
+
     def test_sync_success_not_yet_in_derived_series(self):
         # If the package to sync does not exist yet in the derived series,
         # upload right to any component inside the destination series will be