Merge lp:~michael.nelson/launchpad/506203-ppa-privatisation-check into lp:launchpad

[Michael Nelson]

[Michael Nelson (michael.nelson)]

= Summary =
This branch ensures that the privacy of a PPA cannot be changed once there are published sources for the PPA. It does so both on the model (ie. API usage) as well as on the view.

There will be a second branch following which will update a gazillion tests that switch the privacy of PPAs during tests. And then a third which will just update the UI to present a disabled checkbox when appropriate.

== Implementation details ==

I'd hoped to find a solution that would enable me to re-use the same validation code on both the model and the view (initially I thought a validation constraint defined on the interface might do the job, but it doesn't).

The only way I could see to reuse the code directly would be to add something like validateArchivePrivacy() on the interface, but that doesn't seem sane either.

== Tests ==
To test:
bin/test -vvt TestArchivePrivacySwitching

== Demo and Q/A ==

Visit:
https://launchpad.dev/~cprov/+archive/ppa

login as <email address hidden> and click on Administer archive, then try changing the checkbox.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/soyuz/browser/tests/test_archive_admin_view.py
  lib/lp/soyuz/tests/test_archive.py
  lib/lp/soyuz/interfaces/archive.py
  lib/lp/soyuz/model/archive.py
  lib/lp/soyuz/browser/archive.py

== Pylint notices ==

lib/lp/soyuz/interfaces/archive.py
    41: [F0401] Unable to import 'lazr.enum' (No module named enum)
    54: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    60: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    457: [C0322, IArchivePublic.newPackagesetUploader] Operator not preceded by a space

    packageset=Reference(
    ^
    Interface, title=_("Package set"), required=True),
    explicit=Bool(
    title=_("Explicit"), required=False))

followed by a few dozen more of the same incorrect lint report?

[Graham Binns (gmb)]

Hi Michael,

Nice branch; thanks for fixing this. A couple of nitpicks, but nothing
major. Otherwise this is r=me.

> === modified file 'lib/lp/soyuz/browser/archive.py'
> --- lib/lp/soyuz/browser/archive.py 2010-02-11 13:26:21 +0000
> +++ lib/lp/soyuz/browser/archive.py 2010-02-16 16:08:18 +0000
> @@ -1802,6 +1802,14 @@
> """
> form.getWidgetsData(self.widgets, 'field', data)
>
> + if data.get('private') != self.context.private:
> + # The privacy is being switched.
> + if self.context.getPublishedSources().count() > 0:

You should use

    if not self.context.getPublishedSources().is_empty():

rather than checking .count() > 0 as it's more efficient.

> + self.setFieldError(
> + 'private',
> + 'This archive already has published sources. It is '
> + 'not possible to switch the privacy.')
> +
> if data.get('buildd_secret') is None and data['private']:
> self.setFieldError(
> 'buildd_secret',
>
> === modified file 'lib/lp/soyuz/model/archive.py'
> --- lib/lp/soyuz/model/archive.py 2010-02-02 12:04:19 +0000
> +++ lib/lp/soyuz/model/archive.py 2010-02-16 16:08:18 +0000
> @@ -112,10 +113,19 @@
> If the owner of the archive is private, then the archive cannot be
> made public.
> """
> - if not value:
> + if value is False:
> # The archive is transitioning from public to private.
> assert self.owner.visibility != PersonVisibility.PRIVATE, (
> "Private teams may not have public PPAs.")
> +
> + # If the privacy is being changed ensure there are no sources
> + # published.
> + sources_count = self.getPublishedSources().count()
> + if sources_count > 0:

You should use .is_empty() here, too.

> + raise CannotSwitchPrivacy(
> + "This archive has had %d sources published and therefore "
> + "cannot have its privacy switched." % sources_count)
> +
> return value
>
> name = StringCol(

review: Approve (code)

[Michael Nelson (michael.nelson)]

17:37 < noodles775> gmb: lol, re. the is_empty()... we hit the same point last week. I tried is_empty first, but it's not part of ISQLObjectResultSet.
17:38 < gmb> noodles775: Oh, sod. I hate that shim. Okay, checking .count() is an acceptable alternative then.

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2010-02-25 16:49:16 +0000
+++ lib/lp/soyuz/browser/archive.py	2010-02-26 15:29:16 +0000
@@ -1802,6 +1802,14 @@
         """
         form.getWidgetsData(self.widgets, 'field', data)
 
+        if data.get('private') != self.context.private:
+            # The privacy is being switched.
+            if self.context.getPublishedSources().count() > 0:
+                self.setFieldError(
+                    'private',
+                    'This archive already has published sources. It is '
+                    'not possible to switch the privacy.')
+
         if data.get('buildd_secret') is None and data['private']:
             self.setFieldError(
                 'buildd_secret',

=== added file 'lib/lp/soyuz/browser/tests/test_archive_admin_view.py'
--- lib/lp/soyuz/browser/tests/test_archive_admin_view.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_admin_view.py	2010-02-26 15:29:16 +0000
@@ -0,0 +1,95 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from canonical.launchpad.ftests import login
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing import LaunchpadFunctionalLayer
+from lp.soyuz.browser.archive import ArchiveAdminView
+from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
+from lp.testing import TestCaseWithFactory
+
+
+class TestArchivePrivacySwitchingView(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        """Create a ppa for the tests and login as an admin."""
+        super(TestArchivePrivacySwitchingView, self).setUp()
+        self.ppa = self.factory.makeArchive()
+        # Login as an admin to ensure access to the view's context
+        # object.
+        login('[e-mail]')
+
+    def initialize_admin_view(self, private=True):
+        """Initialize the admin view to set the privacy.."""
+        method = 'POST'
+        form = {
+            'field.enabled': 'on',
+            'field.actions.save': 'Save',
+            }
+        if private is True:
+            form['field.private'] = 'on'
+            form['field.buildd_secret'] = 'test'
+        else:
+            form['field.private'] = 'off'
+            form['field.buildd_secret'] = ''
+
+        view = ArchiveAdminView(self.ppa, LaunchpadTestRequest(
+            method=method, form=form))
+        view.initialize()
+        return view
+
+    def make_ppa_private(self, ppa):
+        """Helper method to privatise a ppa."""
+        ppa.private = True
+        ppa.buildd_secret = "secret"
+
+    def publish_to_ppa(self, ppa):
+        """Helper method to publish a package in a PPA."""
+        publisher = SoyuzTestPublisher()
+        publisher.prepareBreezyAutotest()
+        publisher.getPubSource(archive=ppa)
+
+    def test_set_private_without_packages(self):
+        # If a ppa does not have packages published, it is possible to
+        # update the private attribute.
+        view = self.initialize_admin_view(private=True)
+        self.assertEqual(0, len(view.errors))
+        self.assertTrue(view.context.private)
+
+    def test_set_public_without_packages(self):
+        # If a ppa does not have packages published, it is possible to
+        # update the private attribute.
+        self.make_ppa_private(self.ppa)
+        self.assertTrue(self.ppa.private)
+
+        view = self.initialize_admin_view(private=False)
+        self.assertEqual(0, len(view.errors))
+        self.assertFalse(view.context.private)
+
+    def test_set_private_with_packages(self):
+        # A PPA that does have packages cannot be privatised.
+        self.publish_to_ppa(self.ppa)
+        view = self.initialize_admin_view(private=True)
+        self.assertEqual(1, len(view.errors))
+        self.assertEqual(
+            'This archive already has published sources. '
+            'It is not possible to switch the privacy.',
+            view.errors[0])
+
+    def test_set_public_with_packages(self):
+        # A PPA that does have (or had) packages published is presented
+        # with a disabled 'private' field.
+        self.make_ppa_private(self.ppa)
+        self.assertTrue(self.ppa.private)
+        self.publish_to_ppa(self.ppa)
+
+        view = self.initialize_admin_view(private=False)
+        self.assertEqual(1, len(view.errors))
+        self.assertEqual(
+            'This archive already has published sources. '
+            'It is not possible to switch the privacy.',
+            view.errors[0])

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2010-02-25 16:49:16 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2010-02-26 15:29:16 +0000
@@ -14,6 +14,7 @@
     'ArchiveNotPrivate',
     'ArchivePurpose',
     'CannotCopy',
+    'CannotSwitchPrivacy',
     'ComponentNotFound',
     'DistroSeriesNotFound',
     'IArchive',
@@ -78,6 +79,12 @@
     webservice_error(400) #Bad request.
 
 
+class CannotSwitchPrivacy(Exception):
+    """Raised when switching the privacy of an archive that has
+    publishing records."""
+    webservice_error(400) # Bad request.
+
+
 class PocketNotFound(Exception):
     """Invalid pocket."""
     webservice_error(400) #Bad request.
@@ -151,7 +158,9 @@
         Bool(
             title=_("Private"), required=False,
             description=_(
-                "Whether the archive is private to the owner or not.")))
+                "Whether the archive is private to the owner or not. "
+                "This can only be changed if the archive has not had "
+                "any sources published.")))
 
     require_virtualized = Bool(
         title=_("Require Virtualized Builder"), required=False,

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2010-02-18 17:05:50 +0000
+++ lib/lp/soyuz/model/archive.py	2010-02-26 15:29:16 +0000
@@ -55,9 +55,10 @@
 from lp.registry.model.teammembership import TeamParticipation
 from lp.soyuz.interfaces.archive import (
     AlreadySubscribed, ArchiveDependencyError, ArchiveNotPrivate,
-    ArchivePurpose, CannotCopy, DistroSeriesNotFound, IArchive, IArchiveSet,
-    IDistributionArchive, InvalidComponent, IPPA, MAIN_ARCHIVE_PURPOSES,
-    NoSuchPPA, PocketNotFound, VersionRequiresName, default_name_by_purpose)
+    ArchivePurpose, CannotCopy, CannotSwitchPrivacy,
+    DistroSeriesNotFound, IArchive, IArchiveSet, IDistributionArchive,
+    InvalidComponent, IPPA, MAIN_ARCHIVE_PURPOSES, NoSuchPPA,
+    PocketNotFound, VersionRequiresName, default_name_by_purpose)
 from lp.soyuz.interfaces.archiveauthtoken import IArchiveAuthTokenSet
 from lp.soyuz.interfaces.archivepermission import (
     ArchivePermissionType, IArchivePermissionSet)
@@ -112,10 +113,19 @@
         If the owner of the archive is private, then the archive cannot be
         made public.
         """
-        if not value:
-            # The archive is transitioning from public to private.
+        if value is False:
+            # The archive is transitioning from private to public.
             assert self.owner.visibility != PersonVisibility.PRIVATE, (
                 "Private teams may not have public PPAs.")
+
+        # If the privacy is being changed ensure there are no sources
+        # published.
+        sources_count = self.getPublishedSources().count()
+        if sources_count > 0:
+            raise CannotSwitchPrivacy(
+                "This archive has had %d sources published and therefore "
+                "cannot have its privacy switched." % sources_count)
+
         return value
 
     name = StringCol(
@@ -1118,7 +1128,7 @@
         self._copySources([source], to_pocket, to_series, include_binaries)
 
     def _collectLatestPublishedSources(self, from_archive, source_names):
-        """Private helper to collect the latest published sources for an 
+        """Private helper to collect the latest published sources for an
         archive.
 
         :raises NoSuchSourcePackageName: If any of the source_names do not
@@ -1250,7 +1260,7 @@
             extra_exprs.append(Build.buildstate == build_status)
 
         result_set = store.find(
-            SourcePackageRelease, 
+            SourcePackageRelease,
             Build.sourcepackagereleaseID == SourcePackageRelease.id,
             Build.archive == self,
             *extra_exprs)

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2010-01-26 11:41:42 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2010-02-26 15:29:16 +0000
@@ -18,7 +18,8 @@
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.person import IPersonSet
 from lp.services.job.interfaces.job import JobStatus
-from lp.soyuz.interfaces.archive import IArchiveSet, ArchivePurpose
+from lp.soyuz.interfaces.archive import (
+    IArchiveSet, ArchivePurpose, CannotSwitchPrivacy)
 from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
 from lp.soyuz.interfaces.build import BuildStatus
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
@@ -529,7 +530,7 @@
         # the private method which is not defined on the interface.
         self.archive = self.factory.makeArchive()
         self.naked_archive = removeSecurityProxy(self.archive)
-        
+
         self.pub_1 = self.publisher.getPubSource(
             version='0.5.11~ppa1', archive=self.archive, sourcename="foo",
             status=PackagePublishingStatus.PUBLISHED)
@@ -558,5 +559,51 @@
         self.assertEqual(1, len(pubs))
         self.assertEqual('0.5.11~ppa1', pubs[0].source_package_version)
 
+
+class TestArchivePrivacySwitching(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        """Create a public and a private PPA."""
+        super(TestArchivePrivacySwitching, self).setUp()
+        self.public_ppa = self.factory.makeArchive()
+        self.private_ppa = self.factory.makeArchive()
+        self.private_ppa.buildd_secret = 'blah'
+        self.private_ppa.private = True
+
+    def make_ppa_private(self, ppa):
+        """Helper method to privatise a ppa."""
+        ppa.private = True
+        ppa.buildd_secret = "secret"
+
+    def make_ppa_public(self, ppa):
+        """Helper method to make a PPA public (and use for assertRaises)."""
+        ppa.private = False
+        ppa.buildd_secret = ''
+
+    def test_switch_privacy_no_pubs_succeeds(self):
+        # Changing the privacy is fine if there are no publishing
+        # records.
+        self.make_ppa_private(self.public_ppa)
+        self.assertTrue(self.public_ppa.private)
+
+        self.private_ppa.private = False
+        self.assertFalse(self.private_ppa.private)
+
+    def test_switch_privacy_with_pubs_fails(self):
+        # Changing the privacy is not possible when the archive already
+        # has published sources.
+        publisher = SoyuzTestPublisher()
+        publisher.prepareBreezyAutotest()
+        publisher.getPubSource(archive=self.public_ppa)
+        publisher.getPubSource(archive=self.private_ppa)
+
+        self.assertRaises(
+            CannotSwitchPrivacy, self.make_ppa_private, self.public_ppa)
+
+        self.assertRaises(
+            CannotSwitchPrivacy, self.make_ppa_public, self.private_ppa)
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)