launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/launchpad/dsd-batch-position-reset-bug-822781 into lp:launchpad with lp:~allenap/launchpad/phantom-overlay-bug-820828 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #822781 in Launchpad itself: "Batch position is lost after form submission on distroseries diffs pages"
  https://bugs.launchpad.net/launchpad/+bug/822781

For more details, see:
https://code.launchpad.net/~allenap/launchpad/dsd-batch-position-reset-bug-822781/+merge/71069

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

-- 
https://code.launchpad.net/~allenap/launchpad/dsd-batch-position-reset-bug-822781/+merge/71069
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/dsd-batch-position-reset-bug-822781 into lp:launchpad.

=== 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-10 15:44:51 +0000
@@ -903,15 +903,20 @@
             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
+            self.next_url = self.full_url
 
     @property
-    def action_url(self):
-        """The forms should post to themselves, including GET params to
-        account for batch parameters.
+    def full_url(self):
+        """The request URL including query string.
+
+        The forms should post to themselves, including GET params to account
+        for batch parameters, and actions should redirect back to the same
+        batch, with the same filtering, as they were submitted with.
         """
         return "%s?%s" % (self.request.getURL(), self.request['QUERY_STRING'])
 
+    action_url = full_url
+
     def validate_sync(self, action, data):
         """Validate selected differences."""
         form.getWidgetsData(self.widgets, self.prefix, data)

=== 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-10 15:44:51 +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