launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/fix-packagediff-private into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1023986 in Launchpad itself: ""Available diffs" are not accessible when publishing private packages via copyPackage()  "
  https://bugs.launchpad.net/launchpad/+bug/1023986

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/fix-packagediff-private/+merge/114759

== Summary ==

Bug 1023986: package diffs for security uploads are sometimes inappropriately private.  I think this is because they've been generated against SPRs that were originally uploaded to a private archive, so PackageDiff incorrectly thinks that diffs against them need to be private even though they've since been unembargoed.

== Proposed fix ==

Check privacy of published_archives rather than of upload_archive.

== LOC Rationale ==

+21.  I have 1625 lines of credit.  Besides, this is probably part of the work to let us remove delayed copies and the ubuntu-security celebrity and suchlike, which should make up for it.

== Tests ==

bin/test -vvct test_packagediff

== Demo and Q/A ==

Upload a package whose ancestry is an SPR that was originally uploaded to a private archive (e.g. a security update) and check that its PackageDiffs are public.
-- 
https://code.launchpad.net/~cjwatson/launchpad/fix-packagediff-private/+merge/114759
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/fix-packagediff-private into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/packagediff.py'
--- lib/lp/soyuz/model/packagediff.py	2012-04-16 23:02:44 +0000
+++ lib/lp/soyuz/model/packagediff.py	2012-07-12 23:54:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -145,7 +145,8 @@
     @property
     def private(self):
         """See `IPackageDiff`."""
-        return self.to_source.upload_archive.private
+        return all(
+            archive.private for archive in self.to_source.published_archives)
 
     def _countDeletedLFAs(self):
         """How many files associated with either source package have been

=== modified file 'lib/lp/soyuz/tests/soyuz.py'
--- lib/lp/soyuz/tests/soyuz.py	2012-01-06 11:08:30 +0000
+++ lib/lp/soyuz/tests/soyuz.py	2012-07-12 23:54:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Helper functions/classes for Soyuz tests."""
@@ -154,6 +154,7 @@
         Store the `FakePackager` object used in the test uploads as `packager`
         so the tests can reuse it if necessary.
         """
+        super(TestPackageDiffsBase, self).setUp()
         with dbuser(LAUNCHPAD_DBUSER_NAME):
             fake_chroot = LibraryFileAlias.get(CHROOT_LIBRARYFILEALIAS)
             ubuntu = getUtility(IDistributionSet).getByName(

=== modified file 'lib/lp/soyuz/tests/test_packagediff.py'
--- lib/lp/soyuz/tests/test_packagediff.py	2011-12-30 06:14:56 +0000
+++ lib/lp/soyuz/tests/test_packagediff.py	2012-07-12 23:54:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test source package diffs."""
@@ -19,11 +19,12 @@
     )
 from lp.soyuz.enums import PackageDiffStatus
 from lp.soyuz.tests.soyuz import TestPackageDiffsBase
+from lp.testing import TestCaseWithFactory
 from lp.testing.dbuser import dbuser
 from lp.testing.layers import LaunchpadZopelessLayer
 
 
-class TestPackageDiffs(TestPackageDiffsBase):
+class TestPackageDiffs(TestPackageDiffsBase, TestCaseWithFactory):
     """Test package diffs."""
     layer = LaunchpadZopelessLayer
     dbuser = config.uploader.dbuser
@@ -100,3 +101,21 @@
         diff.performDiff()
         # The diff fails due to the presence of expired files.
         self.assertEqual(PackageDiffStatus.FAILED, diff.status)
+
+    def test_packagediff_private_with_copied_spr(self):
+        # If an SPR has been copied from a private archive to a public
+        # archive, diffs against it are public.
+        p3a = self.factory.makeArchive(private=True)
+        orig_spr = self.factory.makeSourcePackageRelease(archive=p3a)
+        spph = self.factory.makeSourcePackagePublishingHistory(
+            archive=p3a, sourcepackagerelease=orig_spr)
+        private_spr = self.factory.makeSourcePackageRelease(archive=p3a)
+        private_diff = private_spr.requestDiffTo(p3a.owner, orig_spr)
+        self.assertEqual(1, len(orig_spr.published_archives))
+        self.assertTrue(private_diff.private)
+        ppa = self.factory.makeArchive(owner=p3a.owner)
+        spph.copyTo(spph.distroseries, spph.pocket, ppa)
+        self.assertEqual(2, len(orig_spr.published_archives))
+        public_spr = self.factory.makeSourcePackageRelease(archive=ppa)
+        public_diff = public_spr.requestDiffTo(p3a.owner, orig_spr)
+        self.assertFalse(public_diff.private)