launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-785190 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #785190 in Launchpad itself: "Oops during async package sync"
  https://bugs.launchpad.net/launchpad/+bug/785190

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-785190/+merge/61594

= Summary =

I made a silly little mistake in a last-minute update of the view code that generates asynchronous package sync requests on the +localpackagediffs page.


== Proposed fix ==

I had replaced a "spn" (SourcePackageName) which had to be wrapped in a list as [spn] with an actual list of SourcePackageNames, spns.  But accidentally passed that on as [spns].  All that's needed to fix this is to remove the brackets.  This broke a permissions check.


== Pre-implementation notes ==

Didn't have a proper pre-imp for this small fix, but did learn that an uploader needs an ArchivePermission.  Soyuz test setup is costly!


== Implementation details ==

You'll see the actual fix at the very top of the diff.

The tests didn't previously tickle this problem because I only tested this code for the case where the permissions check failed very early on.  I needed it to pass, which turned out to be nontrivial.  Some tests had to move from the lightweight, factory-less, layerless helper test case to the heavier view test case.


== Tests ==

{{{
./bin/test -vvc lp.soyuz.browser.tests.test_package_copying_mixin
}}}


== Demo and Q/A ==

Set feature flag soyuz.derived_series.max_synchronous_syncs to a very low number, e.g. 3.  Then find a nonempty +localpackagediffs page for a derived distroseries (you must have Soyuz super powers), select more than this number of differences, and hit the Sync button.  It should tell you that it requested this number of syncs which will be processed in due course.

For good measure, try the same with fewer differences selected (no more than max_synchronous_syncs).  This should still work as normal; the message will be slightly different because it can tell you exactly which syncs it has performed.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/browser/tests/test_package_copying_mixin.py
  lib/lp/soyuz/browser/archive.py
-- 
https://code.launchpad.net/~jtv/launchpad/bug-785190/+merge/61594
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-785190 into lp:launchpad.

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2011-05-11 11:06:59 +0000
+++ lib/lp/soyuz/browser/archive.py	2011-05-19 15:23:32 +0000
@@ -1322,7 +1322,7 @@
             spph.sourcepackagerelease.sourcepackagename
             for spph in source_pubs]
         check_copy_permissions(
-            person, dest_archive, dest_series, dest_pocket, [spns])
+            person, dest_archive, dest_series, dest_pocket, spns)
 
     job_source = getUtility(IPackageCopyJobSource)
     archive_pubs = partition_pubs_by_archive(source_pubs)

=== modified file 'lib/lp/soyuz/browser/tests/test_package_copying_mixin.py'
--- lib/lp/soyuz/browser/tests/test_package_copying_mixin.py	2011-05-11 11:09:43 +0000
+++ lib/lp/soyuz/browser/tests/test_package_copying_mixin.py	2011-05-19 15:23:32 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.testing.layers import LaunchpadFunctionalLayer
 from lp.registry.interfaces.pocket import PackagePublishingPocket
@@ -168,24 +169,6 @@
         self.assertNotIn("x<y", html_text)
         self.assertIn("x&lt;y", html_text)
 
-    def test_copy_synchronously_checks_permissions(self):
-        # Unless told not to, copy_synchronously does a permissions
-        # check.
-        pocket = self.getPocket()
-        self.assertRaises(
-            CannotCopy,
-            copy_synchronously,
-            [FakeSPPH()], FakeArchive(), FakeDistroSeries(), pocket, False)
-
-    def test_copy_asynchronously_checks_permissions(self):
-        # Unless told not to, copy_asynchronously does a permissions
-        # check.
-        pocket = self.getPocket()
-        self.assertRaises(
-            CannotCopy,
-            copy_asynchronously,
-            [FakeSPPH()], FakeArchive(), FakeDistroSeries(), pocket, False)
-
 
 class TestPackageCopyingMixinIntegration(TestCaseWithFactory):
     """Integration tests for `PackageCopyingMixin`."""
@@ -233,6 +216,12 @@
         return create_initialized_view(
             self.makeDerivedSeries(), "+localpackagediffs")
 
+    def getUploader(self, archive, spn):
+        """Get person with upload rights for the given package and archive."""
+        uploader = archive.owner
+        removeSecurityProxy(archive).newPackageUploader(uploader, spn)
+        return uploader
+
     def test_canCopySynchronously_obeys_feature_flag(self):
         packages = [self.getUniqueString() for counter in range(3)]
         mixin = PackageCopyingMixin()
@@ -307,3 +296,51 @@
             False, check_permissions=False)
         jobs = list(getUtility(IPackageCopyJobSource).getActiveJobs(archive))
         self.assertNotEqual([], jobs)
+
+    def test_copy_synchronously_may_allow_copy(self):
+        # In a normal working situation, copy_synchronously allows a
+        # copy.
+        spph = self.makeSPPH()
+        pocket = PackagePublishingPocket.RELEASE
+        dest_series = self.makeDerivedSeries()
+        dest_archive = dest_series.main_archive
+        spn = spph.sourcepackagerelease.sourcepackagename
+        notification = copy_synchronously(
+            [spph], dest_archive, dest_series, pocket, False,
+            person=self.getUploader(dest_archive, spn))
+        self.assertIn("Packages copied", notification.escapedtext)
+
+    def test_copy_synchronously_checks_permissions(self):
+        # Unless told not to, copy_synchronously does a permissions
+        # check.
+        spph = self.makeSPPH()
+        pocket = self.factory.getAnyPocket()
+        dest_series = self.makeDistroSeries()
+        self.assertRaises(
+            CannotCopy,
+            copy_synchronously,
+            [spph], dest_series.main_archive, dest_series, pocket, False)
+
+    def test_copy_asynchronously_may_allow_copy(self):
+        # In a normal working situation, copy_asynchronously allows a
+        # copy.
+        spph = self.makeSPPH()
+        pocket = PackagePublishingPocket.RELEASE
+        dest_series = self.makeDerivedSeries()
+        dest_archive = dest_series.main_archive
+        spn = spph.sourcepackagerelease.sourcepackagename
+        notification = copy_asynchronously(
+            [spph], dest_archive, dest_series, pocket, False,
+            person=self.getUploader(dest_archive, spn))
+        self.assertIn("Requested", notification.escapedtext)
+
+    def test_copy_asynchronously_checks_permissions(self):
+        # Unless told not to, copy_asynchronously does a permissions
+        # check.
+        spph = self.makeSPPH()
+        pocket = self.factory.getAnyPocket()
+        dest_series = self.makeDistroSeries()
+        self.assertRaises(
+            CannotCopy,
+            copy_asynchronously,
+            [spph], dest_series.main_archive, dest_series, pocket, False)