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<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)
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
