launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07621
[Merge] lp:~jml/launchpad/commercial-means-shut-up into lp:launchpad
Jonathan Lange has proposed merging lp:~jml/launchpad/commercial-means-shut-up into lp:launchpad with lp:~jml/launchpad/drop-special-commercial-permissions as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jml/launchpad/commercial-means-shut-up/+merge/104739
This branch renames the 'commercial' attribute on 'IArchive' to 'suppress_subscription_notifications'. It does this using a property to set the 'commercial' attribute for the database's benefit.
We don't do any queries across the attribute in Launchpad, so we're good there. Ubuntu Software Centre is the only user, and they (currently) don't even use it at an API level, so we're good for backwards compatibility.
Thus, we can un-expose 'commercial' and forbid any access to the attribute in code.
Future patches will:
- rename the column in the db
- change the permissions so that PPA owners (& any PPA creator) can change the attribute
- (possibly) migrate from boolean to an enum
Thanks,
jml
--
https://code.launchpad.net/~jml/launchpad/commercial-means-shut-up/+merge/104739
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jml/launchpad/commercial-means-shut-up into lp:launchpad.
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2012-04-18 13:51:38 +0000
+++ lib/lp/registry/interfaces/person.py 2012-05-04 13:37:33 +0000
@@ -1487,12 +1487,12 @@
displayname=TextLine(required=False),
description=TextLine(required=False),
private=Bool(required=False),
- commercial=Bool(required=False),
+ suppress_subscription_notifications=Bool(required=False),
)
@export_factory_operation(Interface, []) # Really IArchive.
@operation_for_version("beta")
def createPPA(name=None, displayname=None, description=None,
- private=False, commercial=False):
+ private=False, suppress_subscription_notifications=False):
"""Create a PPA.
:param name: A string with the name of the new PPA to create. If
@@ -1501,6 +1501,8 @@
:param description: The description for the new PPA.
:param private: Whether or not to create a private PPA. Defaults to
False, which means the PPA will be public.
+ :param suppress_subscription_notifications: Whether or not to suppress
+ emails to new subscribers about their subscriptions.
:raises: `PPACreationError` if an error is encountered
:return: a PPA `IArchive` record.
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2012-04-20 07:01:18 +0000
+++ lib/lp/registry/model/person.py 2012-05-04 13:37:33 +0000
@@ -2972,14 +2972,15 @@
return getUtility(IArchiveSet).getPPAOwnedByPerson(self, name)
def createPPA(self, name=None, displayname=None, description=None,
- private=False, commercial=False):
+ private=False, suppress_subscription_notifications=False):
"""See `IPerson`."""
# XXX: We pass through the Person on whom the PPA is being created,
# but validatePPA assumes that that Person is also the one creating
# the PPA. This is not true in general, and particularly not for
# teams. Instead, both the acting user and the target of the PPA
# creation ought to be passed through.
- errors = Archive.validatePPA(self, name, private, commercial)
+ errors = Archive.validatePPA(
+ self, name, private, suppress_subscription_notifications)
if errors:
raise PPACreationError(errors)
ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
@@ -2987,7 +2988,7 @@
owner=self, purpose=ArchivePurpose.PPA,
distribution=ubuntu, name=name, displayname=displayname,
description=description, private=private,
- commercial=commercial)
+ suppress_subscription_notifications=suppress_subscription_notifications)
def isBugContributor(self, user=None):
"""See `IPerson`."""
=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py 2012-02-10 10:00:53 +0000
+++ lib/lp/soyuz/browser/archive.py 2012-05-04 13:37:33 +0000
@@ -2107,9 +2107,17 @@
class ArchiveAdminView(BaseArchiveEditView, EnableRestrictedFamiliesMixin):
- field_names = ['enabled', 'private', 'commercial', 'require_virtualized',
- 'build_debug_symbols', 'buildd_secret', 'authorized_size',
- 'relative_build_score', 'external_dependencies']
+ field_names = [
+ 'enabled',
+ 'private',
+ 'suppress_subscription_notifications',
+ 'require_virtualized',
+ 'build_debug_symbols',
+ 'buildd_secret',
+ 'authorized_size',
+ 'relative_build_score',
+ 'external_dependencies',
+ ]
custom_widget('external_dependencies', TextAreaWidget, height=3)
custom_widget('enabled_restricted_families', LabeledMultiCheckBoxWidget)
page_title = 'Administer'
@@ -2164,10 +2172,12 @@
error_text = "\n".join(errors)
self.setFieldError('external_dependencies', error_text)
- if data.get('commercial') is True and not data['private']:
+ if (data.get('suppress_subscription_notifications') is True
+ and not data['private']):
self.setFieldError(
- 'commercial',
- 'Can only set commericial for private archives.')
+ 'suppress_subscription_notifications',
+ 'Can only suppress subscription notifications for private '
+ 'archives.')
enabled_restricted_families = data.get('enabled_restricted_families')
require_virtualized = data.get('require_virtualized')
=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml 2012-04-17 22:10:05 +0000
+++ lib/lp/soyuz/configure.zcml 2012-05-04 13:37:33 +0000
@@ -409,11 +409,12 @@
-->
<require
permission="launchpad.Commercial"
- interface="lp.soyuz.interfaces.archive.IArchiveCommercial"
+ interface="lp.soyuz.interfaces.archive.IArchiveCommercial"
set_attributes="authorized_size build_debug_symbols buildd_secret
- commercial enabled_restricted_families
+ enabled_restricted_families
external_dependencies private
- require_virtualized relative_build_score "/>
+ require_virtualized relative_build_score
+ suppress_subscription_notifications"/>
<require
permission="launchpad.Admin"
set_attributes="distribution name signing_key"/>
=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py 2012-05-01 11:15:58 +0000
+++ lib/lp/soyuz/interfaces/archive.py 2012-05-04 13:37:33 +0000
@@ -319,14 +319,20 @@
is_main = Bool(
title=_("True if archive is a main archive type"), required=False)
- commercial = exported(
+ commercial = Bool(
+ title=_("Commercial"),
+ required=True,
+ description=_(
+ "True if the archive is for commercial applications in the "
+ "Ubuntu Software Centre. Governs whether subscribers or "
+ "uploaders get mail from Launchpad about archive events."))
+
+ suppress_subscription_notifications = exported(
Bool(
- title=_("Commercial"),
+ title=_("Suppress subscription notifications"),
required=True,
description=_(
- "True if the archive is for commercial applications in the "
- "Ubuntu Software Centre. Governs whether subscribers or "
- "uploaders get mail from Launchpad about archive events.")))
+ "Whether subscribers get emails about their subscriptions.")))
def checkArchivePermission(person, component_or_package=None):
"""Check to see if person is allowed to upload to component.
@@ -1743,7 +1749,7 @@
def new(purpose, owner, name=None, displayname=None, distribution=None,
description=None, enabled=True, require_virtualized=True,
- private=False, commercial=False):
+ private=False, suppress_subscription_notifications=False):
"""Create a new archive.
On named-ppa creation, the signing key for the default PPA for the
@@ -1766,6 +1772,8 @@
:param require_virtualized: whether builds for the new archive shall
be carried out on virtual builders
:param private: whether or not to make the PPA private
+ :param suppress_subscription_notifications: whether to suppress
+ emails to subscribers about new subscriptions.
:return: an `IArchive` object.
:raises AssertionError if name is already taken within distribution.
=== modified file 'lib/lp/soyuz/mail/notifications.py'
--- lib/lp/soyuz/mail/notifications.py 2011-12-30 06:14:56 +0000
+++ lib/lp/soyuz/mail/notifications.py 2012-05-04 13:37:33 +0000
@@ -29,9 +29,10 @@
archive = subscription.archive
- # We don't send notification emails for commercial PPAs as these
- # are purchased via software center (and do not mention Launchpad).
- if archive.commercial:
+ # We don't send notification emails for some PPAs, particularly those that
+ # are purchased via the Software Centre, so that its users do not have to
+ # learn about Launchpad.
+ if archive.suppress_subscription_notifications:
return
registrant_name = subscription.registrant.displayname
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py 2012-05-01 10:20:07 +0000
+++ lib/lp/soyuz/model/archive.py 2012-05-04 13:37:33 +0000
@@ -340,6 +340,14 @@
else:
alsoProvides(self, IDistributionArchive)
+ @property
+ def suppress_subscription_notifications(self):
+ return self.commercial
+
+ @suppress_subscription_notifications.setter
+ def suppress_subscription_notifications(self, suppress):
+ self.commercial = suppress
+
# Note: You may safely ignore lint when it complains about this
# declaration. As of Python 2.6, this is a perfectly valid way
# of adding a setter
@@ -1978,9 +1986,9 @@
@classmethod
def validatePPA(self, person, proposed_name, private=False,
- commercial=False):
+ suppress_subscription_notifications=False):
ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
- if private or commercial:
+ if private or suppress_subscription_notifications:
# NOTE: This duplicates the policy in lp/soyuz/configure.zcml
# which says that one needs 'launchpad.Commercial' permission to
# set 'private', and the logic in `AdminByCommercialTeamOrAdmins`
@@ -1991,10 +1999,10 @@
if private:
return (
'%s is not allowed to make private PPAs' % person.name)
- if commercial:
+ if suppress_subscription_notifications:
return (
- '%s is not allowed to make commercial PPAs'
- % person.name)
+ '%s is not allowed to make PPAs that suppress '
+ 'subscription notifications' % person.name)
if person.is_team and (
person.subscriptionpolicy in OPEN_TEAM_POLICY):
return "Open teams cannot have PPAs."
@@ -2129,7 +2137,8 @@
def new(self, purpose, owner, name=None, displayname=None,
distribution=None, description=None, enabled=True,
- require_virtualized=True, private=False, commercial=False):
+ require_virtualized=True, private=False,
+ suppress_subscription_notifications=False):
"""See `IArchiveSet`."""
if distribution is None:
distribution = getUtility(ILaunchpadCelebrities).ubuntu
@@ -2206,7 +2215,8 @@
else:
new_archive.private = private
- new_archive.commercial = commercial
+ new_archive.suppress_subscription_notifications = (
+ suppress_subscription_notifications)
return new_archive
=== modified file 'lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt'
--- lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt 2012-01-15 11:06:57 +0000
+++ lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt 2012-05-04 13:37:33 +0000
@@ -362,7 +362,8 @@
True
>>> admin_browser.getControl(name="field.private").value
False
- >>> admin_browser.getControl(name="field.commercial").value
+ >>> admin_browser.getControl(
+ ... name="field.suppress_subscription_notifications").value
False
>>> admin_browser.getControl(name="field.require_virtualized").value
True
@@ -373,7 +374,8 @@
>>> admin_browser.getControl(name="field.enabled").value = False
>>> admin_browser.getControl(name="field.private").value = True
- >>> admin_browser.getControl(name="field.commercial").value = True
+ >>> admin_browser.getControl(
+ ... name="field.suppress_subscription_notifications").value = True
>>> admin_browser.getControl(name="field.buildd_secret").value = "secret"
>>> admin_browser.getControl(
... name="field.require_virtualized").value = True
@@ -417,10 +419,10 @@
'deb not_a_url' is not a complete and valid sources.list entry
-Setting the buildd secret for non-private archives also generates
-an error. Because the "commercial" flag is also currently set, removing
-privacy will also trigger a validation error because the commercial flag can
-only be set on private archives:
+Setting the buildd secret for non-private archives also generates an error.
+Because the "suppress_subscription_notifications" flag is also currently set,
+removing privacy will also trigger a validation error because the
+suppress_subscription_notifications flag can only be set on private archives:
>>> admin_browser.getControl(
... name="field.external_dependencies").value = ""
@@ -431,7 +433,7 @@
>>> for error in get_feedback_messages(admin_browser.contents):
... print error
There are 2 errors.
- Can only set commericial for private archives.
+ Can only suppress subscription notifications for private archives.
Do not specify for non-private archives
=== modified file 'lib/lp/soyuz/stories/webservice/xx-archive-commercial.txt'
--- lib/lp/soyuz/stories/webservice/xx-archive-commercial.txt 2012-05-04 10:43:42 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive-commercial.txt 2012-05-04 13:37:33 +0000
@@ -8,7 +8,8 @@
Start by making a commercial PPA that we can fetch via the webservice:
>>> login("admin@xxxxxxxxxxxxx")
- >>> ppa = factory.makeArchive(private=True, name="cpppa", commercial=True)
+ >>> ppa = factory.makeArchive(private=True, name="cpppa",
+ ... suppress_subscription_notifications=True)
>>> import transaction
>>> transaction.commit()
>>> logout()
=== modified file 'lib/lp/soyuz/stories/webservice/xx-archive.txt'
--- lib/lp/soyuz/stories/webservice/xx-archive.txt 2012-02-28 05:09:39 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive.txt 2012-05-04 13:37:33 +0000
@@ -17,7 +17,6 @@
>>> from lazr.restful.testing.webservice import pprint_entry
>>> pprint_entry(cprov_archive)
authorized_size: 1024
- commercial: False
dependencies_collection_link:
u'http://.../~cprov/+archive/ppa/dependencies'
description: u'packages to help my friends.'
@@ -31,6 +30,7 @@
resource_type_link: u'http://.../#archive'
self_link: u'http://.../~cprov/+archive/ppa'
signing_key_fingerprint: None
+ suppress_subscription_notifications: False
web_link: u'http://launchpad.../~cprov/+archive/ppa'
For "devel" additional attributes are available.
@@ -39,7 +39,6 @@
... "/~cprov/+archive/ppa", api_version='devel').jsonBody()
>>> pprint_entry(cprov_archive_devel)
authorized_size: 1024
- commercial: False
dependencies_collection_link: u'http://.../~cprov/+archive/ppa/dependencies'
description: u'packages to help my friends.'
displayname: u'PPA for Celso Providelo'
@@ -53,6 +52,7 @@
resource_type_link: u'http://.../#archive'
self_link: u'http://.../~cprov/+archive/ppa'
signing_key_fingerprint: None
+ suppress_subscription_notifications: False
web_link: u'http://launchpad.../~cprov/+archive/ppa'
While the Archive signing key is being generated its
@@ -108,7 +108,6 @@
... ubuntutest['main_archive_link']).jsonBody()
>>> pprint_entry(ubuntu_main_archive)
authorized_size: None
- commercial: False
dependencies_collection_link:
u'http://.../ubuntutest/+archive/primary/dependencies'
description: None
@@ -122,6 +121,7 @@
resource_type_link: u'http://.../#archive'
self_link: u'http://.../ubuntutest/+archive/primary'
signing_key_fingerprint: None
+ suppress_subscription_notifications: False
web_link: u'http://launchpad.../ubuntutest/+archive/primary'
A distribution can also provide a list of all its archives:
@@ -977,7 +977,6 @@
>>> pprint_entry(user_webservice.get("/~cprov/+archive/p3a").jsonBody())
authorized_size: u'tag:launchpad.net:2008:redacted'
- commercial: False
dependencies_collection_link:
u'http://.../~cprov/+archive/p3a/dependencies'
description: u'tag:launchpad.net:2008:redacted'
@@ -991,11 +990,11 @@
resource_type_link: u'http://.../#archive'
self_link: u'http://.../~cprov/+archive/p3a'
signing_key_fingerprint: u'tag:launchpad.net:2008:redacted'
+ suppress_subscription_notifications: False
web_link: u'http://launchpad.../~cprov/+archive/p3a'
>>> pprint_entry(cprov_webservice.get("/~cprov/+archive/p3a").jsonBody())
authorized_size: 2048
- commercial: False
dependencies_collection_link:
u'http://.../~cprov/+archive/p3a/dependencies'
description: u'packages to help my friends.'
@@ -1009,6 +1008,7 @@
resource_type_link: u'http://.../#archive'
self_link: u'http://.../~cprov/+archive/p3a'
signing_key_fingerprint: u'ABCDEF0123456789ABCDDCBA0000111112345678'
+ suppress_subscription_notifications: False
web_link: u'http://launchpad.../~cprov/+archive/p3a'
Creating subscriptions to a (private) archive
=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py 2012-03-28 17:26:49 +0000
+++ lib/lp/soyuz/tests/test_archive.py 2012-05-04 13:37:33 +0000
@@ -1282,32 +1282,30 @@
self.assertEqual(ArchiveStatus.DELETING, self.archive.status)
-class TestCommercialArchive(TestCaseWithFactory):
- """Tests relating to commercial archives."""
+class TestSuppressSubscription(TestCaseWithFactory):
+ """Tests relating to suppressing subscription."""
layer = DatabaseFunctionalLayer
def setUp(self):
- super(TestCommercialArchive, self).setUp()
+ super(TestSuppressSubscription, self).setUp()
self.archive = self.factory.makeArchive()
- def setCommercial(self, archive, commercial):
- """Helper function."""
- archive.commercial = commercial
-
- def test_set_and_get_commercial(self):
+ def test_set_and_get_suppress(self):
# Basic set and get of the commercial property. Anyone can read
# it and it defaults to False.
- login_person(self.archive.owner)
- self.assertFalse(self.archive.commercial)
+ with person_logged_in(self.archive.owner):
+ self.assertFalse(self.archive.suppress_subscription_notifications)
- # The archive owner can't change the value.
- self.assertRaises(Unauthorized, self.setCommercial, self.archive, True)
+ # The archive owner can't change the value.
+ self.assertRaises(
+ Unauthorized, setattr, self.archive,
+ 'suppress_subscription_notifications', True)
# Commercial admins can change it.
- login(COMMERCIAL_ADMIN_EMAIL)
- self.setCommercial(self.archive, True)
- self.assertTrue(self.archive.commercial)
+ with celebrity_logged_in('commercial_admin'):
+ self.setCommercial(self.archive, True)
+ self.assertTrue(self.archive.suppress_subscription_notifications)
class TestBuildDebugSymbols(TestCaseWithFactory):
=== modified file 'lib/lp/soyuz/tests/test_archive_privacy.py'
--- lib/lp/soyuz/tests/test_archive_privacy.py 2012-05-01 11:13:33 +0000
+++ lib/lp/soyuz/tests/test_archive_privacy.py 2012-05-04 13:37:33 +0000
@@ -41,7 +41,8 @@
# Commercial private PPAs cannot be accessed by non-subscribers.
ppa_name = self.factory.getUniqueString()
ppa = self.factory.makeArchive(
- private=True, commercial=True, name=ppa_name)
+ private=True, suppress_subscription_notifications=True,
+ name=ppa_name)
non_subscriber = self.factory.makePerson()
with person_logged_in(non_subscriber):
self.assertEqual(ppa_name, ppa.name)
=== modified file 'lib/lp/soyuz/tests/test_archive_subscriptions.py'
--- lib/lp/soyuz/tests/test_archive_subscriptions.py 2012-02-28 11:14:44 +0000
+++ lib/lp/soyuz/tests/test_archive_subscriptions.py 2012-05-04 13:37:33 +0000
@@ -109,7 +109,7 @@
def test_new_commercial_subscription_no_email(self):
# As per bug 611568, an email is not sent for commercial PPAs.
with celebrity_logged_in('commercial_admin'):
- self.archive.commercial = True
+ self.archive.suppress_subscription_notifications = True
# Logging in as a celebrity team causes an email to be sent
# because a person is added as a member of the team, so this
=== modified file 'lib/lp/soyuz/tests/test_person_createppa.py'
--- lib/lp/soyuz/tests/test_person_createppa.py 2012-04-04 13:37:01 +0000
+++ lib/lp/soyuz/tests/test_person_createppa.py 2012-05-04 13:37:33 +0000
@@ -35,13 +35,14 @@
self.assertRaises(
PPACreationError, person.createPPA, private=True)
- def test_commercial(self):
+ def test_suppress_subscription_notifications(self):
with celebrity_logged_in('commercial_admin') as person:
- ppa = person.createPPA(commercial=True)
- self.assertEqual(True, ppa.commercial)
+ ppa = person.createPPA(suppress_subscription_notifications=True)
+ self.assertEqual(True, ppa.suppress_subscription_notifications)
- def test_commercial_without_permission(self):
+ def test_suppress_without_permission(self):
person = self.factory.makePerson()
with person_logged_in(person):
self.assertRaises(
- PPACreationError, person.createPPA, commercial=True)
+ PPACreationError, person.createPPA,
+ suppress_subscription_notifications=True)
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2012-05-02 05:25:11 +0000
+++ lib/lp/testing/factory.py 2012-05-04 13:37:33 +0000
@@ -2658,7 +2658,7 @@
def makeArchive(self, distribution=None, owner=None, name=None,
purpose=None, enabled=True, private=False,
virtualized=True, description=None, displayname=None,
- commercial=False):
+ suppress_subscription_notifications=False):
"""Create and return a new arbitrary archive.
:param distribution: Supply IDistribution, defaults to a new one
@@ -2671,8 +2671,8 @@
:param private: Whether the archive is created private.
:param virtualized: Whether the archive is virtualized.
:param description: A description of the archive.
- :param commercial: Whether the archive is commercial. Defaults to
- False.
+ :param suppress_subscription_notifications: Whether to suppress
+ subscription notifications, defaults to False.
"""
if purpose is None:
purpose = ArchivePurpose.PPA
@@ -2711,9 +2711,9 @@
naked_archive.private = True
naked_archive.buildd_secret = "sekrit"
- if commercial:
+ if suppress_subscription_notifications:
naked_archive = removeSecurityProxy(archive)
- naked_archive.commercial = True
+ naked_archive.suppress_subscription_notifications = True
return archive
Follow ups
