Launchpad itself

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

  1. 506203-ppa-privatisation-check
  2. Merge into devel
Proposed by Michael Nelson
Status: Merged
Approved by: Graham Binns
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~michael.nelson/launchpad/506203-ppa-privatisation-check
Merge into: lp:launchpad
Diff against target: 287 lines (+179/-10)
5 files modified
lib/lp/soyuz/browser/archive.py (+8/-0)
lib/lp/soyuz/browser/tests/test_archive_admin_view.py (+95/-0)
lib/lp/soyuz/interfaces/archive.py (+10/-1)
lib/lp/soyuz/model/archive.py (+17/-7)
lib/lp/soyuz/tests/test_archive.py (+49/-2)
To merge this branch: bzr merge lp:~michael.nelson/launchpad/506203-ppa-privatisation-check
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Review via email: mp+19415@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Michael Nelson (michael.nelson) wrote :

= 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?

Revision history for this message
Graham Binns (gmb) wrote :

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)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/soyuz/browser/archive.py'
2--- lib/lp/soyuz/browser/archive.py 2010-02-25 16:49:16 +0000
3+++ lib/lp/soyuz/browser/archive.py 2010-02-26 15:29:16 +0000
4@@ -1802,6 +1802,14 @@
5 """
6 form.getWidgetsData(self.widgets, 'field', data)
7
8+ if data.get('private') != self.context.private:
9+ # The privacy is being switched.
10+ if self.context.getPublishedSources().count() > 0:
11+ self.setFieldError(
12+ 'private',
13+ 'This archive already has published sources. It is '
14+ 'not possible to switch the privacy.')
15+
16 if data.get('buildd_secret') is None and data['private']:
17 self.setFieldError(
18 'buildd_secret',
19
20=== added file 'lib/lp/soyuz/browser/tests/test_archive_admin_view.py'
21--- lib/lp/soyuz/browser/tests/test_archive_admin_view.py 1970-01-01 00:00:00 +0000
22+++ lib/lp/soyuz/browser/tests/test_archive_admin_view.py 2010-02-26 15:29:16 +0000
23@@ -0,0 +1,95 @@
24+# Copyright 2009 Canonical Ltd. This software is licensed under the
25+# GNU Affero General Public License version 3 (see the file LICENSE).
26+
27+__metaclass__ = type
28+
29+from canonical.launchpad.ftests import login
30+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
31+from canonical.testing import LaunchpadFunctionalLayer
32+from lp.soyuz.browser.archive import ArchiveAdminView
33+from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
34+from lp.testing import TestCaseWithFactory
35+
36+
37+class TestArchivePrivacySwitchingView(TestCaseWithFactory):
38+
39+ layer = LaunchpadFunctionalLayer
40+
41+ def setUp(self):
42+ """Create a ppa for the tests and login as an admin."""
43+ super(TestArchivePrivacySwitchingView, self).setUp()
44+ self.ppa = self.factory.makeArchive()
45+ # Login as an admin to ensure access to the view's context
46+ # object.
47+ login('admin@canonical.com')
48+
49+ def initialize_admin_view(self, private=True):
50+ """Initialize the admin view to set the privacy.."""
51+ method = 'POST'
52+ form = {
53+ 'field.enabled': 'on',
54+ 'field.actions.save': 'Save',
55+ }
56+ if private is True:
57+ form['field.private'] = 'on'
58+ form['field.buildd_secret'] = 'test'
59+ else:
60+ form['field.private'] = 'off'
61+ form['field.buildd_secret'] = ''
62+
63+ view = ArchiveAdminView(self.ppa, LaunchpadTestRequest(
64+ method=method, form=form))
65+ view.initialize()
66+ return view
67+
68+ def make_ppa_private(self, ppa):
69+ """Helper method to privatise a ppa."""
70+ ppa.private = True
71+ ppa.buildd_secret = "secret"
72+
73+ def publish_to_ppa(self, ppa):
74+ """Helper method to publish a package in a PPA."""
75+ publisher = SoyuzTestPublisher()
76+ publisher.prepareBreezyAutotest()
77+ publisher.getPubSource(archive=ppa)
78+
79+ def test_set_private_without_packages(self):
80+ # If a ppa does not have packages published, it is possible to
81+ # update the private attribute.
82+ view = self.initialize_admin_view(private=True)
83+ self.assertEqual(0, len(view.errors))
84+ self.assertTrue(view.context.private)
85+
86+ def test_set_public_without_packages(self):
87+ # If a ppa does not have packages published, it is possible to
88+ # update the private attribute.
89+ self.make_ppa_private(self.ppa)
90+ self.assertTrue(self.ppa.private)
91+
92+ view = self.initialize_admin_view(private=False)
93+ self.assertEqual(0, len(view.errors))
94+ self.assertFalse(view.context.private)
95+
96+ def test_set_private_with_packages(self):
97+ # A PPA that does have packages cannot be privatised.
98+ self.publish_to_ppa(self.ppa)
99+ view = self.initialize_admin_view(private=True)
100+ self.assertEqual(1, len(view.errors))
101+ self.assertEqual(
102+ 'This archive already has published sources. '
103+ 'It is not possible to switch the privacy.',
104+ view.errors[0])
105+
106+ def test_set_public_with_packages(self):
107+ # A PPA that does have (or had) packages published is presented
108+ # with a disabled 'private' field.
109+ self.make_ppa_private(self.ppa)
110+ self.assertTrue(self.ppa.private)
111+ self.publish_to_ppa(self.ppa)
112+
113+ view = self.initialize_admin_view(private=False)
114+ self.assertEqual(1, len(view.errors))
115+ self.assertEqual(
116+ 'This archive already has published sources. '
117+ 'It is not possible to switch the privacy.',
118+ view.errors[0])
119
120=== modified file 'lib/lp/soyuz/interfaces/archive.py'
121--- lib/lp/soyuz/interfaces/archive.py 2010-02-25 16:49:16 +0000
122+++ lib/lp/soyuz/interfaces/archive.py 2010-02-26 15:29:16 +0000
123@@ -14,6 +14,7 @@
124 'ArchiveNotPrivate',
125 'ArchivePurpose',
126 'CannotCopy',
127+ 'CannotSwitchPrivacy',
128 'ComponentNotFound',
129 'DistroSeriesNotFound',
130 'IArchive',
131@@ -78,6 +79,12 @@
132 webservice_error(400) #Bad request.
133
134
135+class CannotSwitchPrivacy(Exception):
136+ """Raised when switching the privacy of an archive that has
137+ publishing records."""
138+ webservice_error(400) # Bad request.
139+
140+
141 class PocketNotFound(Exception):
142 """Invalid pocket."""
143 webservice_error(400) #Bad request.
144@@ -151,7 +158,9 @@
145 Bool(
146 title=_("Private"), required=False,
147 description=_(
148- "Whether the archive is private to the owner or not.")))
149+ "Whether the archive is private to the owner or not. "
150+ "This can only be changed if the archive has not had "
151+ "any sources published.")))
152
153 require_virtualized = Bool(
154 title=_("Require Virtualized Builder"), required=False,
155
156=== modified file 'lib/lp/soyuz/model/archive.py'
157--- lib/lp/soyuz/model/archive.py 2010-02-18 17:05:50 +0000
158+++ lib/lp/soyuz/model/archive.py 2010-02-26 15:29:16 +0000
159@@ -55,9 +55,10 @@
160 from lp.registry.model.teammembership import TeamParticipation
161 from lp.soyuz.interfaces.archive import (
162 AlreadySubscribed, ArchiveDependencyError, ArchiveNotPrivate,
163- ArchivePurpose, CannotCopy, DistroSeriesNotFound, IArchive, IArchiveSet,
164- IDistributionArchive, InvalidComponent, IPPA, MAIN_ARCHIVE_PURPOSES,
165- NoSuchPPA, PocketNotFound, VersionRequiresName, default_name_by_purpose)
166+ ArchivePurpose, CannotCopy, CannotSwitchPrivacy,
167+ DistroSeriesNotFound, IArchive, IArchiveSet, IDistributionArchive,
168+ InvalidComponent, IPPA, MAIN_ARCHIVE_PURPOSES, NoSuchPPA,
169+ PocketNotFound, VersionRequiresName, default_name_by_purpose)
170 from lp.soyuz.interfaces.archiveauthtoken import IArchiveAuthTokenSet
171 from lp.soyuz.interfaces.archivepermission import (
172 ArchivePermissionType, IArchivePermissionSet)
173@@ -112,10 +113,19 @@
174 If the owner of the archive is private, then the archive cannot be
175 made public.
176 """
177- if not value:
178- # The archive is transitioning from public to private.
179+ if value is False:
180+ # The archive is transitioning from private to public.
181 assert self.owner.visibility != PersonVisibility.PRIVATE, (
182 "Private teams may not have public PPAs.")
183+
184+ # If the privacy is being changed ensure there are no sources
185+ # published.
186+ sources_count = self.getPublishedSources().count()
187+ if sources_count > 0:
188+ raise CannotSwitchPrivacy(
189+ "This archive has had %d sources published and therefore "
190+ "cannot have its privacy switched." % sources_count)
191+
192 return value
193
194 name = StringCol(
195@@ -1118,7 +1128,7 @@
196 self._copySources([source], to_pocket, to_series, include_binaries)
197
198 def _collectLatestPublishedSources(self, from_archive, source_names):
199- """Private helper to collect the latest published sources for an
200+ """Private helper to collect the latest published sources for an
201 archive.
202
203 :raises NoSuchSourcePackageName: If any of the source_names do not
204@@ -1250,7 +1260,7 @@
205 extra_exprs.append(Build.buildstate == build_status)
206
207 result_set = store.find(
208- SourcePackageRelease,
209+ SourcePackageRelease,
210 Build.sourcepackagereleaseID == SourcePackageRelease.id,
211 Build.archive == self,
212 *extra_exprs)
213
214=== modified file 'lib/lp/soyuz/tests/test_archive.py'
215--- lib/lp/soyuz/tests/test_archive.py 2010-01-26 11:41:42 +0000
216+++ lib/lp/soyuz/tests/test_archive.py 2010-02-26 15:29:16 +0000
217@@ -18,7 +18,8 @@
218 from lp.registry.interfaces.distribution import IDistributionSet
219 from lp.registry.interfaces.person import IPersonSet
220 from lp.services.job.interfaces.job import JobStatus
221-from lp.soyuz.interfaces.archive import IArchiveSet, ArchivePurpose
222+from lp.soyuz.interfaces.archive import (
223+ IArchiveSet, ArchivePurpose, CannotSwitchPrivacy)
224 from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
225 from lp.soyuz.interfaces.build import BuildStatus
226 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
227@@ -529,7 +530,7 @@
228 # the private method which is not defined on the interface.
229 self.archive = self.factory.makeArchive()
230 self.naked_archive = removeSecurityProxy(self.archive)
231-
232+
233 self.pub_1 = self.publisher.getPubSource(
234 version='0.5.11~ppa1', archive=self.archive, sourcename="foo",
235 status=PackagePublishingStatus.PUBLISHED)
236@@ -558,5 +559,51 @@
237 self.assertEqual(1, len(pubs))
238 self.assertEqual('0.5.11~ppa1', pubs[0].source_package_version)
239
240+
241+class TestArchivePrivacySwitching(TestCaseWithFactory):
242+
243+ layer = LaunchpadZopelessLayer
244+
245+ def setUp(self):
246+ """Create a public and a private PPA."""
247+ super(TestArchivePrivacySwitching, self).setUp()
248+ self.public_ppa = self.factory.makeArchive()
249+ self.private_ppa = self.factory.makeArchive()
250+ self.private_ppa.buildd_secret = 'blah'
251+ self.private_ppa.private = True
252+
253+ def make_ppa_private(self, ppa):
254+ """Helper method to privatise a ppa."""
255+ ppa.private = True
256+ ppa.buildd_secret = "secret"
257+
258+ def make_ppa_public(self, ppa):
259+ """Helper method to make a PPA public (and use for assertRaises)."""
260+ ppa.private = False
261+ ppa.buildd_secret = ''
262+
263+ def test_switch_privacy_no_pubs_succeeds(self):
264+ # Changing the privacy is fine if there are no publishing
265+ # records.
266+ self.make_ppa_private(self.public_ppa)
267+ self.assertTrue(self.public_ppa.private)
268+
269+ self.private_ppa.private = False
270+ self.assertFalse(self.private_ppa.private)
271+
272+ def test_switch_privacy_with_pubs_fails(self):
273+ # Changing the privacy is not possible when the archive already
274+ # has published sources.
275+ publisher = SoyuzTestPublisher()
276+ publisher.prepareBreezyAutotest()
277+ publisher.getPubSource(archive=self.public_ppa)
278+ publisher.getPubSource(archive=self.private_ppa)
279+
280+ self.assertRaises(
281+ CannotSwitchPrivacy, self.make_ppa_private, self.public_ppa)
282+
283+ self.assertRaises(
284+ CannotSwitchPrivacy, self.make_ppa_public, self.private_ppa)
285+
286 def test_suite():
287 return unittest.TestLoader().loadTestsFromName(__name__)