launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/archive-subscriber-refactor-cancel into lp:launchpad with lp:~cjwatson/launchpad/close-account-more-2 as a prerequisite.

Commit message:
Factor out some duplication in ArchiveSubscriber cancellation.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/archive-subscriber-refactor-cancel/+merge/364169

See comments in https://code.launchpad.net/~cjwatson/launchpad/close-account-more-2/+merge/363259.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/archive-subscriber-refactor-cancel into lp:launchpad.

=== modified file 'lib/lp/registry/scripts/closeaccount.py'
--- lib/lp/registry/scripts/closeaccount.py	2019-03-08 17:05:56 +0000
+++ lib/lp/registry/scripts/closeaccount.py	2019-03-08 17:05:57 +0000
@@ -24,10 +24,7 @@
     PersonSettings,
     )
 from lp.services.database import postgresql
-from lp.services.database.constants import (
-    DEFAULT,
-    UTC_NOW,
-    )
+from lp.services.database.constants import DEFAULT
 from lp.services.database.interfaces import IMasterStore
 from lp.services.database.sqlbase import cursor
 from lp.services.identity.interfaces.account import (
@@ -41,6 +38,7 @@
     LaunchpadScriptFailure,
     )
 from lp.soyuz.enums import ArchiveSubscriberStatus
+from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriberSet
 from lp.soyuz.model.archivesubscriber import ArchiveSubscriber
 
 
@@ -322,13 +320,13 @@
     # corresponding ArchiveSubscriber rows; but even once PPA authorisation
     # is handled dynamically, we probably still want to have the per-person
     # audit trail here.
-    store.find(
-        ArchiveSubscriber,
+    archive_subscriber_ids = set(store.find(
+        ArchiveSubscriber.id,
         ArchiveSubscriber.subscriber_id == person.id,
-        ArchiveSubscriber.status == ArchiveSubscriberStatus.CURRENT).set(
-            date_cancelled=UTC_NOW,
-            cancelled_by_id=janitor.id,
-            status=ArchiveSubscriberStatus.CANCELLED)
+        ArchiveSubscriber.status == ArchiveSubscriberStatus.CURRENT))
+    if archive_subscriber_ids:
+        getUtility(IArchiveSubscriberSet).cancel(
+            archive_subscriber_ids, janitor)
     skip.add(('archivesubscriber', 'subscriber'))
     skip.add(('archiveauthtoken', 'person'))
 

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2019-02-07 10:17:43 +0000
+++ lib/lp/security.py	2019-03-08 17:05:57 +0000
@@ -2923,6 +2923,11 @@
                 super(EditArchiveSubscriber, self).checkAuthenticated(user))
 
 
+class AdminArchiveSubscriberSet(AdminByCommercialTeamOrAdmins):
+    """Only (commercial) admins can manipulate archive subscribers in bulk."""
+    usedfor = IArchiveSubscriberSet
+
+
 class ViewSourcePackageRecipe(DelegatedAuthorization):
 
     permission = "launchpad.View"

=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml	2018-11-18 18:30:00 +0000
+++ lib/lp/soyuz/configure.zcml	2019-03-08 17:05:57 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+<!-- Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
      GNU Affero General Public License version 3 (see the file LICENSE).
 -->
 
@@ -515,7 +515,10 @@
         class="lp.soyuz.model.archivesubscriber.ArchiveSubscriberSet"
         provides="lp.soyuz.interfaces.archivesubscriber.IArchiveSubscriberSet">
         <allow
-            interface="lp.soyuz.interfaces.archivesubscriber.IArchiveSubscriberSet"/>
+            interface="lp.soyuz.interfaces.archivesubscriber.IArchiveSubscriberSetView"/>
+        <require
+            permission="launchpad.Admin"
+            interface="lp.soyuz.interfaces.archivesubscriber.IArchiveSubscriberSetAdmin"/>
     </securedutility>
 
     <!-- PersonalArchiveSubscription -->

=== modified file 'lib/lp/soyuz/doc/archivesubscriber.txt'
--- lib/lp/soyuz/doc/archivesubscriber.txt	2018-05-27 18:32:33 +0000
+++ lib/lp/soyuz/doc/archivesubscriber.txt	2019-03-08 17:05:57 +0000
@@ -446,6 +446,26 @@
     >>> login("admin@xxxxxxxxxxxxx")
     >>> new_sub.cancel(cprov)
 
+We can cancel subscriptions in bulk:
+
+    >>> login("celso.providelo@xxxxxxxxxxxxx")
+    >>> subs = [
+    ...     cprov_private_ppa.newSubscription(factory.makePerson(), cprov)
+    ...     for _ in range(3)]
+    >>> sub_set.cancel([subs[0].id, subs[1].id], cprov)
+    Traceback (most recent call last):
+    ...
+    Unauthorized:...
+
+    >>> login("admin@xxxxxxxxxxxxx")
+    >>> sub_set.cancel([subs[0].id, subs[1].id], cprov)
+    >>> print(subs[0].status.name)
+    CANCELLED
+    >>> print(subs[1].status.name)
+    CANCELLED
+    >>> print(subs[2].status.name)
+    CURRENT
+
 
 Finding all non-active subscribers
 ----------------------------------

=== modified file 'lib/lp/soyuz/interfaces/archivesubscriber.py'
--- lib/lp/soyuz/interfaces/archivesubscriber.py	2015-03-06 10:22:08 +0000
+++ lib/lp/soyuz/interfaces/archivesubscriber.py	2019-03-08 17:05:57 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """ArchiveSubscriber interface."""
@@ -127,8 +127,8 @@
     export_as_webservice_entry()
 
 
-class IArchiveSubscriberSet(Interface):
-    """An interface for the set of all archive subscribers."""
+class IArchiveSubscriberSetView(Interface):
+    """An interface for launchpad.View ops on all archive subscribers."""
 
     def getBySubscriber(subscriber, archive=None, current_only=True):
         """Return all the subscriptions for a person.
@@ -154,7 +154,7 @@
         """
 
     def getByArchive(archive, current_only=True):
-        """Return all the subscripions for an archive.
+        """Return all the subscriptions for an archive.
 
         :param archive: An `IArchive` for which to return all
             `ArchiveSubscriber` records.
@@ -163,6 +163,26 @@
         """
 
 
+class IArchiveSubscriberSetAdmin(Interface):
+    """An interface for launchpad.Admin ops on all archive subscribers."""
+
+    def cancel(archive_subscriber_ids, cancelled_by):
+        """Cancel a set of subscriptions.
+
+        :param archive_subscriber_ids: A sequence of `ArchiveSubscriber.id`
+            values.
+        :param cancelled_by: An `IPerson` who is cancelling the subscription.
+
+        Sets cancelled_by to the supplied person and date_cancelled to
+        the current date/time.
+        """
+
+
+class IArchiveSubscriberSet(
+        IArchiveSubscriberSetView, IArchiveSubscriberSetAdmin):
+    """An interface for the set of all archive subscribers."""
+
+
 class IPersonalArchiveSubscription(Interface):
     """An abstract interface representing a subscription for an individual.
 

=== modified file 'lib/lp/soyuz/model/archivesubscriber.py'
--- lib/lp/soyuz/model/archivesubscriber.py	2015-09-23 15:38:13 +0000
+++ lib/lp/soyuz/model/archivesubscriber.py	2019-03-08 17:05:57 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Database class for table ArchiveSubscriber."""
@@ -45,7 +45,10 @@
 from lp.services.webapp.authorization import precache_permission_for_objects
 from lp.soyuz.enums import ArchiveSubscriberStatus
 from lp.soyuz.interfaces.archiveauthtoken import IArchiveAuthTokenSet
-from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriber
+from lp.soyuz.interfaces.archivesubscriber import (
+    IArchiveSubscriber,
+    IArchiveSubscriberSet,
+    )
 from lp.soyuz.model.archive import Archive
 from lp.soyuz.model.archiveauthtoken import ArchiveAuthToken
 
@@ -94,9 +97,7 @@
 
     def cancel(self, cancelled_by):
         """See `IArchiveSubscriber`."""
-        self.date_cancelled = UTC_NOW
-        self.cancelled_by = cancelled_by
-        self.status = ArchiveSubscriberStatus.CANCELLED
+        ArchiveSubscriberSet().cancel([self.id], cancelled_by)
 
     def getNonActiveSubscribers(self):
         """See `IArchiveSubscriber`."""
@@ -149,6 +150,7 @@
                 EmailAddress.status == EmailAddressStatus.PREFERRED)
 
 
+@implementer(IArchiveSubscriberSet)
 class ArchiveSubscriberSet:
     """See `IArchiveSubscriberSet`."""
 
@@ -253,3 +255,12 @@
                 ArchiveSubscriber.status == ArchiveSubscriberStatus.CURRENT)
 
         return extra_exprs
+
+    def cancel(self, archive_subscriber_ids, cancelled_by):
+        """See `IArchiveSubscriberSet`."""
+        Store.of(cancelled_by).find(
+            ArchiveSubscriber,
+            ArchiveSubscriber.id.is_in(archive_subscriber_ids)).set(
+                date_cancelled=UTC_NOW,
+                cancelled_by_id=cancelled_by.id,
+                status=ArchiveSubscriberStatus.CANCELLED)