Merge lp:~jml/launchpad/narrow-commercial-celebrity into lp:launchpad

[Jonathan Lange]

Remove IDistribution.getCommercialPPAs and IArchiveSet.getCommercialPPAs

We don't need getCommercialPPAs at all. It was added for us, we are the only users, and we don't use it. Therefore, it should be deleted. Thus let it be written, thus let it be done.

This branch deletes them both. It also deletes a doctest, archive-commercial.txt, which doesn't really do much once getCommercialPPAs is deleted. The sole remaining test went into test_archive_privacy, which I also went to work on with a pair of pliers and a blowtorch, moving stuff out of setUp, using simpler layers and hopefully making tests more clear.

I've updated the documentation in IArchive for commercial to match our current understanding.

If it lands as-is, this branch will leave me 113 deletions in credit (by diffstat's measure). I'm going to need them.

[Jonathan Lange (jml)]

Fresh revisions make better use of the new 'commercial' parameter on LaunchpadObjectFactory.makeArchive. I also did to test_archive_agent what I did to the other test modules: kill unnecessary helpers, use better helpers from the common library, stop using setUp.

[Richard Harding (rharding)]

Looks good, updated your negative LoC count to 113 after revisions.

review: Approve

=== modified file 'lib/lp/_schema_circular_imports.py'
--- lib/lp/_schema_circular_imports.py	2012-03-13 00:45:33 +0000
+++ lib/lp/_schema_circular_imports.py	2012-05-01 12:28:21 +0000
@@ -470,8 +470,6 @@
     IDistribution, 'getSourcePackage', IDistributionSourcePackage)
 patch_collection_return_type(
     IDistribution, 'searchSourcePackages', IDistributionSourcePackage)
-patch_collection_return_type(
-    IDistribution, 'getCommercialPPAs', IArchive)
 patch_reference_property(
     IDistribution, 'main_archive', IArchive)
 IDistribution['all_distro_archives'].value_type.schema = IArchive
@@ -861,9 +859,9 @@
 
 # IDistribution
 patch_operations_explicit_version(
-    IDistribution, 'beta', "getArchive", "getCommercialPPAs",
-    "getCountryMirror", "getDevelopmentSeries", "getMirrorByName",
-    "getSeries", "getSourcePackage", "searchSourcePackages")
+    IDistribution, 'beta', "getArchive", "getCountryMirror",
+    "getDevelopmentSeries", "getMirrorByName", "getSeries",
+    "getSourcePackage", "searchSourcePackages")
 
 # IDistributionMirror
 patch_entry_explicit_version(IDistributionMirror, 'beta')

=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py	2011-12-24 16:54:44 +0000
+++ lib/lp/registry/interfaces/distribution.py	2012-05-01 12:28:21 +0000
@@ -570,18 +570,6 @@
     def getAllPPAs():
         """Return all PPAs for this distribution."""
 
-    # Really returns IArchive, see
-    # _schema_circular_imports.py.
-    @operation_returns_collection_of(Interface)
-    @export_read_operation()
-    def getCommercialPPAs():
-        """Return all commercial PPAs.
-
-        Commercial PPAs are private, but explicitly flagged up as commercial
-        so that they are discoverable by people who wish to buy items
-        from them.
-        """
-
     def searchPPAs(text=None, show_inactive=False):
         """Return all PPAs matching the given text in this distribution.
 

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2012-03-08 01:07:47 +0000
+++ lib/lp/registry/model/distribution.py	2012-05-01 12:28:21 +0000
@@ -1316,12 +1316,6 @@
             distribution=self,
             purpose=ArchivePurpose.PPA).order_by('id')
 
-    def getCommercialPPAs(self):
-        """See `IDistribution`."""
-        # If we ever see non-Ubuntu PPAs, this will return more than
-        # just the PPAs for the Ubuntu context.
-        return getUtility(IArchiveSet).getCommercialPPAs()
-
     def searchPPAs(self, text=None, show_inactive=False, user=None):
         """See `IDistribution`."""
         clauses = ["""

=== removed file 'lib/lp/soyuz/doc/archive-commercial.txt'
--- lib/lp/soyuz/doc/archive-commercial.txt	2010-06-30 13:50:25 +0000
+++ lib/lp/soyuz/doc/archive-commercial.txt	1970-01-01 00:00:00 +0000
@@ -1,56 +0,0 @@
-===============
-Commercial PPAs
-===============
-
-Commercial PPAs are to all intents and purposes identical to private PPAs.
-They contain a 'commercial' flag which indicates that the PPA is actively
-selling software and should be more discoverable.
-
-We have a private PPA for demonstration purposes:
-
-    >>> ppa = factory.makeArchive(private=True, name="pppa")
-
-
-Discovering Commercial PPAs
----------------------------
-
-IArchive.getCommercialPPAs() retrieves the subset of all private PPAs that are
-marked for commercial usage.
-
-    >>> from lp.soyuz.interfaces.archive import IArchiveSet
-    >>> archive_set = getUtility(IArchiveSet)
-    >>> for commercial_ppa in archive_set.getCommercialPPAs():
-    ...     print commercial_ppa.name
-
-There are none yet - if we mark "ppa" as commercial it will be
-returned:
-
-    >>> # Setting 'commercial' is restricted to admins but we don't want to
-    >>> # worry about that here. See lib/lp/soyuz/tests/test_archive.py
-    >>> from zope.security.proxy import removeSecurityProxy
-    >>> removeSecurityProxy(ppa).commercial = True
-    >>> for commercial_ppa in archive_set.getCommercialPPAs():
-    ...     print commercial_ppa.name
-    pppa
-
-
-Security
---------
-
-Anyone is allowed to retrieve the list of commercial PPAs.  Normal security
-restrictions apply to individual PPA access though.
-
-    >>> login(ANONYMOUS)
-    >>> ppas = archive_set.getCommercialPPAs()
-
-'name is a public attribute:
-
-    >>> print ppas[0].name
-    pppa
-
-'description' is not, however.
-
-    >>> ppas[0].description
-    Traceback (most recent call last):
-    ...
-    Unauthorized:...

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2012-04-03 15:34:50 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2012-05-01 12:28:21 +0000
@@ -324,8 +324,9 @@
             title=_("Commercial"),
             required=True,
             description=_(
-                "Display the archive in Software Center's commercial "
-                "listings. Only private archives can be commercial.")))
+                "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.")))
 
     def checkArchivePermission(person, component_or_package=None):
         """Check to see if person is allowed to upload to component.
@@ -1886,14 +1887,6 @@
     def getPrivatePPAs():
         """Return a result set containing all private PPAs."""
 
-    def getCommercialPPAs():
-        """Return a result set containing all commercial PPAs.
-
-        Commercial PPAs are private, but explicitly flagged up as commercial
-        so that they are discoverable by people who wish to buy items
-        from them.
-        """
-
     def getPublicationsInArchives(source_package_name, archive_list,
                                   distribution):
         """Return a result set of publishing records for the source package.

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-04-04 13:36:29 +0000
+++ lib/lp/soyuz/model/archive.py	2012-05-01 12:28:21 +0000
@@ -2404,14 +2404,6 @@
             Archive._private == True,
             Archive.purpose == ArchivePurpose.PPA)
 
-    def getCommercialPPAs(self):
-        """See `IArchiveSet`."""
-        store = IStore(Archive)
-        return store.find(
-            Archive,
-            Archive.commercial == True,
-            Archive.purpose == ArchivePurpose.PPA)
-
     def getArchivesForDistribution(self, distribution, name=None,
                                    purposes=None, user=None,
                                    exclude_disabled=True):

=== modified file 'lib/lp/soyuz/stories/webservice/xx-archive-commercial.txt'
--- lib/lp/soyuz/stories/webservice/xx-archive-commercial.txt	2011-12-24 17:49:30 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive-commercial.txt	2012-05-01 12:28:21 +0000
@@ -8,24 +8,11 @@
 Start by making a commercial PPA that we can fetch via the webservice:
 
     >>> login("[e-mail]")
-    >>> ppa = factory.makeArchive(private=True, name="cpppa")
-    >>> from zope.security.proxy import removeSecurityProxy
-    >>> removeSecurityProxy(ppa).commercial = True
+    >>> ppa = factory.makeArchive(private=True, name="cpppa", commercial=True)
     >>> import transaction
     >>> transaction.commit()
     >>> logout()
 
-Commercial archives can be found with a custom method 'getCommercialPPAs'
-that lives on the distribution object.
-
-Using webservice directly:
-
-    >>> ubuntu = webservice.get("/ubuntu").jsonBody()
-    >>> cppas = webservice.named_get(
-    ...     ubuntu['self_link'], 'getCommercialPPAs').jsonBody()
-    >>> for entry in cppas['entries']:
-    ...     print entry['name']
-    cpppa
 
 == Software Center Agent ==
 
@@ -35,8 +22,8 @@
     >>> from lp.app.interfaces.launchpad import ILaunchpadCelebrities
     >>> login('[e-mail]')
     >>> celebrity = getUtility(ILaunchpadCelebrities).software_center_agent
-    >>> archive = factory.makeArchive(name='commercial', private=True)
-    >>> archive.commercial = True
+    >>> archive = factory.makeArchive(
+    ...     name='commercial', private=True, commercial=True)
     >>> url = "/~%s/+archive/commercial" % archive.owner.name
     >>> person = factory.makePerson(name='joe')
     >>> logout()

=== modified file 'lib/lp/soyuz/tests/test_archive_agent.py'
--- lib/lp/soyuz/tests/test_archive_agent.py	2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/tests/test_archive_agent.py	2012-05-01 12:28:21 +0000
@@ -1,67 +1,54 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
+# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the GNU
+# Affero General Public License version 3 (see the file LICENSE).
 
 """Test Archive software center agent celebrity."""
 
 from zope.component import getUtility
 
-from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.services.webapp.authorization import check_permission
 from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriberSet
 from lp.testing import (
-    login,
-    login_person,
+    celebrity_logged_in,
     TestCaseWithFactory,
     )
-from lp.testing.layers import LaunchpadFunctionalLayer
+from lp.testing.layers import DatabaseFunctionalLayer
 
 
 class TestArchivePrivacy(TestCaseWithFactory):
-    layer = LaunchpadFunctionalLayer
-
-    def setUp(self):
-        super(TestArchivePrivacy, self).setUp()
-        self.ppa = self._makePrivateArchive()
-        self.ppa.commercial = True
-        self.agent = getUtility(ILaunchpadCelebrities).software_center_agent
-        self.joe = self.factory.makePerson(name='joe')
-
-    def _makePrivateArchive(self):
-        ppa = self.factory.makeArchive()
-        login('[e-mail]')
-        ppa.buildd_secret = 'blah'
-        ppa.private = True
-        return ppa
+
+    layer = DatabaseFunctionalLayer
 
     def test_check_permission(self):
         """The software center agent has the relevant permissions for a
         commercial archive, but not a private one.
         """
-        login_person(self.agent)
-        self.assertEqual(
-            check_permission('launchpad.View', self.ppa), True)
-        self.assertEqual(
-            check_permission('launchpad.Append', self.ppa), True)
+        ppa = self.factory.makeArchive(private=True, commercial=True)
+        with celebrity_logged_in('software_center_agent'):
+            self.assertEqual(check_permission('launchpad.View', ppa), True)
+            self.assertEqual(check_permission('launchpad.Append', ppa), True)
 
     def test_check_permission_private(self):
-        ppa = self._makePrivateArchive()
-        login_person(self.agent)
-        self.assertEqual(check_permission('launchpad.View', ppa), False)
-        self.assertEqual(check_permission('launchpad.Append', ppa), False)
+        ppa = self.factory.makeArchive(private=True, commercial=False)
+        with celebrity_logged_in('software_center_agent'):
+            self.assertEqual(check_permission('launchpad.View', ppa), False)
+            self.assertEqual(check_permission('launchpad.Append', ppa), False)
 
     def test_add_subscription(self):
-        login_person(self.agent)
-        self.ppa.newSubscription(self.joe, self.agent)
-        subscription = getUtility(
-            IArchiveSubscriberSet).getBySubscriber(
-            self.joe, archive=self.ppa).one()
-        self.assertEqual(subscription.registrant, self.agent)
-        self.assertEqual(subscription.subscriber, self.joe)
+        person = self.factory.makePerson()
+        ppa = self.factory.makeArchive(private=True, commercial=True)
+        with celebrity_logged_in('software_center_agent') as agent:
+            ppa.newSubscription(person, agent)
+            subscription = getUtility(IArchiveSubscriberSet).getBySubscriber(
+                person, archive=ppa).one()
+            self.assertEqual(subscription.registrant, agent)
+            self.assertEqual(subscription.subscriber, person)
 
     def test_getArchiveSubscriptionURL(self):
-        login_person(self.agent)
-        sources = self.joe.getArchiveSubscriptionURL(self.agent, self.ppa)
-        authtoken = self.ppa.getAuthToken(self.joe).token
-        url = self.ppa.archive_url.split('http://')[1]
-        new_url = "http://joe:%s@%s" % (authtoken, url)
+        ppa = self.factory.makeArchive(private=True, commercial=True)
+        person = self.factory.makePerson()
+        with celebrity_logged_in('software_center_agent') as agent:
+            sources = person.getArchiveSubscriptionURL(agent, ppa)
+            authtoken = ppa.getAuthToken(person).token
+            url = ppa.archive_url.split('http://')[1]
+        new_url = "http://%s:%s@%s" % (person.name, authtoken, url)
         self.assertEqual(sources, new_url)

=== modified file 'lib/lp/soyuz/tests/test_archive_privacy.py'
--- lib/lp/soyuz/tests/test_archive_privacy.py	2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/tests/test_archive_privacy.py	2012-05-01 12:28:21 +0000
@@ -3,98 +3,95 @@
 
 """Test Archive privacy features."""
 
-from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
 
-from lp.soyuz.interfaces.archive import (
-    CannotSwitchPrivacy,
-    IArchiveSet,
-    )
+from lp.soyuz.interfaces.archive import CannotSwitchPrivacy
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import (
-    login,
-    login_person,
+    person_logged_in,
     TestCaseWithFactory,
     )
 from lp.testing.layers import (
-    LaunchpadFunctionalLayer,
+    DatabaseFunctionalLayer,
     LaunchpadZopelessLayer,
     )
 
 
 class TestArchivePrivacy(TestCaseWithFactory):
-    layer = LaunchpadFunctionalLayer
-
-    def setUp(self):
-        super(TestArchivePrivacy, self).setUp()
-        self.private_ppa = self.factory.makeArchive(description='Foo')
-        login('[e-mail]')
-        self.private_ppa.private = True
-        self.joe = self.factory.makePerson(name='joe')
-        self.fred = self.factory.makePerson(name='fred')
-        login_person(self.private_ppa.owner)
-        self.private_ppa.newSubscription(self.joe, self.private_ppa.owner)
-
-    def _getDescription(self, p3a):
-        return p3a.description
+
+    layer = DatabaseFunctionalLayer
 
     def test_no_subscription(self):
-        login_person(self.fred)
-        p3a = getUtility(IArchiveSet).get(self.private_ppa.id)
-        self.assertRaises(Unauthorized, self._getDescription, p3a)
+        # You cannot access private PPAs without a subscription.
+        ppa = self.factory.makeArchive(private=True)
+        non_subscriber = self.factory.makePerson()
+        with person_logged_in(non_subscriber):
+            self.assertRaises(Unauthorized, getattr, ppa, 'description')
 
     def test_subscription(self):
-        login_person(self.joe)
-        p3a = getUtility(IArchiveSet).get(self.private_ppa.id)
-        self.assertEqual(self._getDescription(p3a), "Foo")
+        # Once you have a subscription, you can access private PPAs.
+        ppa = self.factory.makeArchive(private=True, description="Foo")
+        subscriber = self.factory.makePerson()
+        with person_logged_in(ppa.owner):
+            ppa.newSubscription(subscriber, ppa.owner)
+        with person_logged_in(subscriber):
+            self.assertEqual(ppa.description, "Foo")
+
+    def test_commercial_security(self):
+        # 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)
+        non_subscriber = self.factory.makePerson()
+        with person_logged_in(non_subscriber):
+            self.assertEqual(ppa_name, ppa.name)
+            self.assertRaises(Unauthorized, getattr, ppa, 'description')
 
 
 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.private = True
-
-    def make_ppa_private(self, ppa):
+    def set_ppa_privacy(self, ppa, private):
         """Helper method to privatise a ppa."""
-        ppa.private = True
-
-    def make_ppa_public(self, ppa):
-        """Helper method to make a PPA public (and use for assertRaises)."""
-        ppa.private = False
+        ppa.private = private
 
     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)
+        public_ppa = self.factory.makeArchive()
+        self.set_ppa_privacy(public_ppa, private=True)
+        self.assertTrue(public_ppa.private)
 
-        self.private_ppa.private = False
-        self.assertFalse(self.private_ppa.private)
+        private_ppa = self.factory.makeArchive(private=True)
+        self.set_ppa_privacy(private_ppa, private=False)
+        self.assertFalse(private_ppa.private)
 
     def test_switch_privacy_with_pubs_fails(self):
         # Changing the privacy is not possible when the archive already
         # has published sources.
+        public_ppa = self.factory.makeArchive(private=False)
         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)
+
+        private_ppa = self.factory.makeArchive(private=True)
+        publisher.getPubSource(archive=public_ppa)
+        publisher.getPubSource(archive=private_ppa)
+
+        self.assertRaises(
+            CannotSwitchPrivacy,
+            self.set_ppa_privacy, public_ppa, private=True)
+
+        self.assertRaises(
+            CannotSwitchPrivacy,
+            self.set_ppa_privacy, private_ppa, private=False)
 
     def test_buildd_secret_was_generated(self):
-        self.make_ppa_private(self.public_ppa)
-        self.assertNotEqual(self.public_ppa.buildd_secret, None)
+        public_ppa = self.factory.makeArchive()
+        self.set_ppa_privacy(public_ppa, private=True)
+        self.assertNotEqual(public_ppa.buildd_secret, None)
 
     def test_discard_buildd_was_discarded(self):
-        self.make_ppa_public(self.private_ppa)
-        self.assertEqual(self.private_ppa.buildd_secret, None)
+        private_ppa = self.factory.makeArchive(private=True)
+        self.set_ppa_privacy(private_ppa, private=False)
+        self.assertEqual(private_ppa.buildd_secret, None)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-04-27 18:50:49 +0000
+++ lib/lp/testing/factory.py	2012-05-01 12:28:21 +0000
@@ -2663,7 +2663,8 @@
 
     def makeArchive(self, distribution=None, owner=None, name=None,
                     purpose=None, enabled=True, private=False,
-                    virtualized=True, description=None, displayname=None):
+                    virtualized=True, description=None, displayname=None,
+                    commercial=False):
         """Create and return a new arbitrary archive.
 
         :param distribution: Supply IDistribution, defaults to a new one
@@ -2676,6 +2677,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.
         """
         if purpose is None:
             purpose = ArchivePurpose.PPA
@@ -2714,6 +2717,10 @@
             naked_archive.private = True
             naked_archive.buildd_secret = "sekrit"
 
+        if commercial:
+            naked_archive = removeSecurityProxy(archive)
+            naked_archive.commercial = True
+
         return archive
 
     def makeArchiveAdmin(self, archive=None):