launchpad-reviewers team mailing list archive

[James Westby <jw+debian@xxxxxxxxxxxxxxx>]

James Westby has proposed merging lp:~james-w/launchpad/suppress-notifications-for-all into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~james-w/launchpad/suppress-notifications-for-all/+merge/107560

= Summary =

Archives have a "suppress_subscription_notifications" attribute, that for
historical reasons is only settable by commercial admins. There's nothing
sensitive about the attribute, so we can allow it to be set by any owner.

== Proposed fix ==

Change the permissions for the attribute to require Launchpad.Edit rather
than Launchpad.Commercial.

== Pre-implementation notes ==

I didn't have a pre-implementation call with anyone on the LP team.

== LOC Rationale ==

38 lines in credit, bringing us to 140 total for this arc of work.

== Implementation details ==

Changing the permission required is pretty easy, and most of the
other changes are test changes to test the new behaviour rather
than the old.

This change does drop the (partially implemented) restriction that
only private archives can have suppress_subscription_notifications.
I decided this was best as there would be a lot of lines of code
required to enforce this, even leaving aside presenting that
restriction well in the UI.

This change does not make the attribute setable on the +edit page
of the archive, leaving it on +admin for now. I'm not sure that
it should be moved to +edit, but it can be done in a followup
branch if needed.

There are also a couple of changes that seem unrelated, but they
are cleaning up debt in this area (changes to the commercial
archives doctest file so that it no longer refers to a file
that doesn't exist, and doesn't create an archive that isn't
used anywhere else in the file.)

== Tests ==

There are new tests that check that owners can set and change
the attribute, and a tests that a non-owner can't. Tests
were removed that checked that commercial admins can set
the attribute.

== Demo and Q/A ==

I believe that testing that the attribute can still be
toggled on the +admin form should be sufficient for
allowing this to rollout. Checking that owners can now
change the attribute will be necessary for closing the
bug.

I don't think that testing that the attribute still causes
emails to be suppressed is necessary as none of that code
changed.

= Launchpad lint =

I don't think these reported issues should be addressed
as they would harm readability:

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/tests/test_archive_subscriptions.py
  lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt
  lib/lp/soyuz/stories/webservice/xx-archive.txt
  lib/lp/soyuz/tests/test_archive.py
  lib/lp/soyuz/interfaces/archive.py
  lib/lp/soyuz/configure.zcml
  lib/lp/soyuz/tests/test_person_createppa.py
  lib/lp/soyuz/tests/test_archive_privacy.py
  lib/lp/soyuz/model/archive.py
  lib/lp/soyuz/stories/webservice/xx-archive-commercial.txt
  lib/lp/registry/model/person.py
  lib/lp/soyuz/browser/archive.py

./lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt
     278: want exceeds 78 characters.
./lib/lp/soyuz/stories/webservice/xx-archive.txt
      42: want exceeds 78 characters.
      46: want exceeds 78 characters.
     169: want exceeds 78 characters.
     185: want exceeds 78 characters.
     201: want exceeds 78 characters.
     217: want exceeds 78 characters.
     361: want exceeds 78 characters.
     422: want exceeds 78 characters.
     553: want exceeds 78 characters.
./lib/lp/soyuz/interfaces/archive.py
     913: E303 too many blank lines (2)
./lib/lp/soyuz/model/archive.py
     346: redefinition of function 'suppress_subscription_notifications' from line 342
     357: redefinition of function 'private' from line 353
./lib/lp/registry/model/person.py
    2990: E501 line too long (84 characters)
-- 
https://code.launchpad.net/~james-w/launchpad/suppress-notifications-for-all/+merge/107560
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~james-w/launchpad/suppress-notifications-for-all into lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-05-14 10:56:16 +0000
+++ lib/lp/registry/model/person.py	2012-05-27 17:32:22 +0000
@@ -2979,8 +2979,7 @@
         # 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, suppress_subscription_notifications)
+        errors = Archive.validatePPA(self, name, private)
         if errors:
             raise PPACreationError(errors)
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2012-05-04 12:24:11 +0000
+++ lib/lp/soyuz/browser/archive.py	2012-05-27 17:32:22 +0000
@@ -2172,13 +2172,6 @@
                 error_text = "\n".join(errors)
                 self.setFieldError('external_dependencies', error_text)
 
-        if (data.get('suppress_subscription_notifications') is True
-            and not data['private']):
-            self.setFieldError(
-                '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')
         proc_family_set = getUtility(IProcessorFamilySet)

=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml	2012-05-21 17:29:19 +0000
+++ lib/lp/soyuz/configure.zcml	2012-05-27 17:32:22 +0000
@@ -402,7 +402,8 @@
         <require
             permission="launchpad.Edit"
             interface="lp.soyuz.interfaces.archive.IArchiveEdit"
-            set_attributes="description displayname publish status"/>
+            set_attributes="description displayname publish status
+                            suppress_subscription_notifications"/>
         <!--
            NOTE: The 'private' permission controls who can turn a public
            archive into a private one, and vice versa. The logic that says this
@@ -415,8 +416,12 @@
             set_attributes="authorized_size build_debug_symbols buildd_secret
                             enabled_restricted_families
                             external_dependencies private
+<<<<<<< TREE
                             require_virtualized
                             suppress_subscription_notifications"/>
+=======
+                            require_virtualized relative_build_score"/>
+>>>>>>> MERGE-SOURCE
         <require
             permission="launchpad.Moderate"
             set_schema="lp.soyuz.interfaces.archive.IArchiveRestricted"/>

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2012-05-22 10:50:19 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2012-05-27 17:32:22 +0000
@@ -333,7 +333,7 @@
             required=True,
             description=_(
                 "Whether subscribers to private PPAs get emails about their "
-                "subscriptions.")))
+                "subscriptions. Has no effect on a public PPA.")))
 
     def checkArchivePermission(person, component_or_package=None):
         """Check to see if person is allowed to upload to component.

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-05-14 01:29:38 +0000
+++ lib/lp/soyuz/model/archive.py	2012-05-27 17:32:22 +0000
@@ -80,7 +80,6 @@
 from lp.services.database.enumcol import EnumCol
 from lp.services.database.lpstorm import (
     ISlaveStore,
-    IStore,
     )
 from lp.services.database.sqlbase import (
     cursor,
@@ -1985,10 +1984,9 @@
         self.enabled_restricted_families = restricted
 
     @classmethod
-    def validatePPA(self, person, proposed_name, private=False,
-                    suppress_subscription_notifications=False):
+    def validatePPA(self, person, proposed_name, private=False):
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
-        if private or suppress_subscription_notifications:
+        if private:
             # 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`
@@ -1996,13 +1994,8 @@
             # permissions.
             role = IPersonRoles(person)
             if not (role.in_admin or role.in_commercial_admin):
-                if private:
-                    return (
-                        '%s is not allowed to make private PPAs' % person.name)
-                if suppress_subscription_notifications:
-                    return (
-                        '%s is not allowed to make PPAs that suppress '
-                        'subscription notifications' % person.name)
+                return (
+                    '%s is not allowed to make private PPAs' % person.name)
         if person.is_team and (
             person.subscriptionpolicy in OPEN_TEAM_POLICY):
             return "Open teams cannot have PPAs."

=== modified file 'lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt'
--- lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt	2012-05-04 12:24:11 +0000
+++ lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt	2012-05-27 17:32:22 +0000
@@ -419,10 +419,7 @@
     '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 "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:
+Setting the buildd secret for non-private archives also generates an error:
 
     >>> admin_browser.getControl(
     ...     name="field.external_dependencies").value = ""
@@ -432,8 +429,7 @@
 
     >>> for error in get_feedback_messages(admin_browser.contents):
     ...     print error
-    There are 2 errors.
-    Can only suppress subscription notifications for private archives.
+    There is 1 error.
     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 12:02:44 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive-commercial.txt	2012-05-27 17:32:22 +0000
@@ -2,20 +2,10 @@
 Using the webservice with commercial archives
 =============================================
 
-(See also soyuz/stories/webservice/xx-archive.txt and
-soyuz/doc/archive-commercial.txt)
-
-Start by making a commercial PPA that we can fetch via the webservice:
-
-    >>> login("admin@xxxxxxxxxxxxx")
-    >>> ppa = factory.makeArchive(private=True, name="cpppa",
-    ...     suppress_subscription_notifications=True)
-    >>> import transaction
-    >>> transaction.commit()
-    >>> logout()
-
-
-== Software Center Agent ==
+(See also soyuz/stories/webservice/xx-archive.txt)
+
+Software Center Agent
+---------------------
 
 Create the P3A where software_center_agent is an owner.
 
@@ -26,7 +16,7 @@
     >>> owner = factory.makePerson()
     >>> ppa_owner = factory.makeTeam(members=[celebrity, owner])
     >>> archive = factory.makeArchive(name='commercial', private=True,
-    ...     owner=ppa_owner)
+    ...     owner=ppa_owner, suppress_subscription_notifications=True)
     >>> url = "/~%s/+archive/commercial" % archive.owner.name
     >>> person = factory.makePerson(name='joe')
     >>> logout()

=== modified file 'lib/lp/soyuz/stories/webservice/xx-archive.txt'
--- lib/lp/soyuz/stories/webservice/xx-archive.txt	2012-05-23 05:42:24 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive.txt	2012-05-27 17:32:22 +0000
@@ -1177,6 +1177,18 @@
     foocomm 1.0-2 in hoary
     (same version already uploaded and waiting in ACCEPTED queue)
 
+Suppressing notifications
+-------------------------
+
+The owner of the archive can suppress notifications on subscription
+changes over the API.
+
+    >>> private_archive = cprov_webservice.get(
+    ...     cprov_private_ppa['self_link']).jsonBody()
+    >>> private_archive['suppress_subscription_notifications'] = True
+    >>> print modify_archive(cprov_webservice, private_archive)
+    HTTP/1.1 209 ...
+    ...
 
 Archive dependencies
 ====================

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2012-05-04 14:49:26 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2012-05-27 17:32:22 +0000
@@ -1288,22 +1288,25 @@
     layer = DatabaseFunctionalLayer
 
     def test_set_and_get_suppress(self):
-        # Basic set and get of the commercial property.  Anyone can read
-        # it and it defaults to False.
+        # Basic set and get of the suppress_subscription_notifications
+        # property.  Anyone can read it and it defaults to False.
         archive = self.factory.makeArchive()
         with person_logged_in(archive.owner):
             self.assertFalse(archive.suppress_subscription_notifications)
 
-            # The archive owner can't change the value.
-            self.assertRaises(
-                Unauthorized,
-                setattr, archive, 'suppress_subscription_notifications', True)
-
-        # Commercial admins can change it.
-        with celebrity_logged_in('commercial_admin'):
+            # The archive owner can change the value.
             archive.suppress_subscription_notifications = True
             self.assertTrue(archive.suppress_subscription_notifications)
 
+    def test_most_users_cant_set_suppress(self):
+        # Basic set and get of the suppress_subscription_notifications
+        # property.  Anyone can read it and it defaults to False.
+        archive = self.factory.makeArchive()
+        with person_logged_in(self.factory.makePerson()):
+            self.assertFalse(archive.suppress_subscription_notifications)
+            self.assertRaises(Unauthorized,
+                setattr, archive, 'suppress_subscription_notifications', True)
+
 
 class TestBuildDebugSymbols(TestCaseWithFactory):
     """Tests relating to the build_debug_symbols flag."""

=== modified file 'lib/lp/soyuz/tests/test_archive_privacy.py'
--- lib/lp/soyuz/tests/test_archive_privacy.py	2012-05-04 12:02:44 +0000
+++ lib/lp/soyuz/tests/test_archive_privacy.py	2012-05-27 17:32:22 +0000
@@ -37,17 +37,6 @@
         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, suppress_subscription_notifications=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):
 

=== modified file 'lib/lp/soyuz/tests/test_archive_subscriptions.py'
--- lib/lp/soyuz/tests/test_archive_subscriptions.py	2012-05-04 11:44:05 +0000
+++ lib/lp/soyuz/tests/test_archive_subscriptions.py	2012-05-27 17:32:22 +0000
@@ -9,7 +9,6 @@
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
     BrowserTestCase,
-    celebrity_logged_in,
     login_person,
     person_logged_in,
     TestCaseWithFactory,
@@ -107,14 +106,9 @@
             notifications[0]['to'])
 
     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.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
-        # needs to be cleared out before calling newSubscription().
-        pop_notifications()
+        # As per bug 611568, an email is not sent for
+        # suppress_subscription_notifications PPAs.
+        self.archive.suppress_subscription_notifications = True
 
         self.archive.newSubscription(
             self.subscriber, registrant=self.archive.owner)

=== modified file 'lib/lp/soyuz/tests/test_person_createppa.py'
--- lib/lp/soyuz/tests/test_person_createppa.py	2012-05-04 12:13:17 +0000
+++ lib/lp/soyuz/tests/test_person_createppa.py	2012-05-27 17:32:22 +0000
@@ -36,13 +36,6 @@
                 PPACreationError, person.createPPA, private=True)
 
     def test_suppress_subscription_notifications(self):
-        with celebrity_logged_in('commercial_admin') as person:
-            ppa = person.createPPA(suppress_subscription_notifications=True)
-            self.assertEqual(True, ppa.suppress_subscription_notifications)
-
-    def test_suppress_without_permission(self):
         person = self.factory.makePerson()
-        with person_logged_in(person):
-            self.assertRaises(
-                PPACreationError, person.createPPA,
-                suppress_subscription_notifications=True)
+        ppa = person.createPPA(suppress_subscription_notifications=True)
+        self.assertEqual(True, ppa.suppress_subscription_notifications)