launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/bug-722626 into lp:launchpad with lp:~danilo/launchpad/devel-bug-720826 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #722626 Removing a product series doesn't remove BugSubscriptionFilters for any StructuralSubscription
  https://bugs.launchpad.net/bugs/722626

For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-722626/+merge/50618

= Bug 722626 =

ProductSeries removal view directly removes any relevant StructuralSubscriptions.  With BugSubscriptionFilters which can be attached to them, removal will fail.

I've found that this is breaking a test from a failing test in my branch which automatically creates a BugSubscriptionFilter for each StructuralSubscription.

== Proposed fix ==

We should instead call subscription.delete() method which will then remove all linked BSFs as well.

== Implementation details ==

 * Add a filter to lib/lp/registry/browser/tests/productseries-views.txt and see how removal actually fails
 * Use subscription.delete() in lib/lp/bugs/browser/__init__.py RegistryDeleteViewMixin._unsubscribe_structure()
 * Fix up permissions (i.e. allow target owners to delete structural subscriptions as well): this doesn't affect actual user experience (i.e. firefox owner can't modify my firefox subscriptions) because that's still controlled through StructuralSubscriptionTarget.userCanAlterSubscription()

== Tests ==

bin/test -cvvt registry.*productseries-views.txt

== Demo and Q/A ==

1. Add a structural subscription to a product series for someone
2. Add a subscription filter for that subscription
2. As a product non-owner (i.e. registry admin/admin) remove a series

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/security.py
  lib/lp/registry/browser/tests/productseries-views.txt
  lib/lp/registry/browser/__init__.py
  lib/lp/bugs/model/structuralsubscription.py
-- 
https://code.launchpad.net/~danilo/launchpad/bug-722626/+merge/50618
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-722626 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2011-01-05 18:46:40 +0000
+++ lib/canonical/launchpad/security.py	2011-02-21 15:38:16 +0000
@@ -56,6 +56,8 @@
 from lp.blueprints.interfaces.sprint import ISprint
 from lp.blueprints.interfaces.sprintspecification import ISprintSpecification
 from lp.bugs.interfaces.bugtarget import IOfficialBugTagTargetRestricted
+from lp.bugs.interfaces.structuralsubscription import (
+    IStructuralSubscription)
 from lp.buildmaster.interfaces.builder import (
     IBuilder,
     IBuilderSet,
@@ -1091,6 +1093,27 @@
                 user.in_admin)
 
 
+class EditStructuralSubscription(AuthorizationBase):
+    permission = 'launchpad.Edit'
+    usedfor = IStructuralSubscription
+
+    def checkAuthenticated(self, user):
+        """Who can edit StructuralSubscriptions."""
+
+        assert self.obj.target
+
+        # Removal of a target cascades removals to StructuralSubscriptions,
+        # so we need to allow editing to those who can edit the target itself.
+        can_edit_target = check_permission('launchpad.Edit', self.obj.target)
+
+        # Who is actually allowed to edit a subscription is determined by
+        # a helper method on the model.
+        can_edit_subscription = self.obj.target.userCanAlterSubscription(
+            self.obj.subscriber, user.person)
+
+        return (can_edit_target or can_edit_subscription)
+
+
 class UseApiDoc(AuthorizationBase):
     """This is just to please apidoc.launchpad.dev."""
     permission = 'zope.app.apidoc.UseAPIDoc'

=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
--- lib/lp/bugs/model/structuralsubscription.py	2011-02-01 19:06:18 +0000
+++ lib/lp/bugs/model/structuralsubscription.py	2011-02-21 15:38:16 +0000
@@ -172,11 +172,10 @@
 
     def delete(self):
         store = Store.of(self)
-        store.find(
-            BugSubscriptionFilter,
-            BugSubscriptionFilter.structural_subscription == self).remove()
+        self.bug_filters.remove()
         store.remove(self)
 
+
 class DistroSeriesTargetHelper:
     """A helper for `IDistroSeries`s."""
 
@@ -353,10 +352,11 @@
             if subscribed_by.inTeam(self.distribution.owner):
                 return True
 
-        admins = getUtility(ILaunchpadCelebrities).admin
+        celebrities = getUtility(ILaunchpadCelebrities)
         return (subscriber == subscribed_by or
                 subscriber in subscribed_by.getAdministratedTeams() or
-                subscribed_by.inTeam(admins))
+                subscribed_by.inTeam(celebrities.registry_experts) or
+                subscribed_by.inTeam(celebrities.admins))
 
     def addSubscription(self, subscriber, subscribed_by):
         """See `IStructuralSubscriptionTarget`."""

=== modified file 'lib/lp/registry/browser/__init__.py'
--- lib/lp/registry/browser/__init__.py	2011-02-03 21:14:05 +0000
+++ lib/lp/registry/browser/__init__.py	2011-02-21 15:38:16 +0000
@@ -187,7 +187,7 @@
             # The owner of the subscription or an admin are the only users
             # that can destroy a subscription, but this rule cannot prevent
             # the owner from removing the structure.
-            Store.of(subscription).remove(subscription)
+            subscription.delete()
 
     def _remove_series_bugs_and_specifications(self, series):
         """Untarget the associated bugs and subscriptions."""

=== modified file 'lib/lp/registry/browser/tests/productseries-views.txt'
--- lib/lp/registry/browser/tests/productseries-views.txt	2011-02-14 19:14:15 +0000
+++ lib/lp/registry/browser/tests/productseries-views.txt	2011-02-21 15:38:16 +0000
@@ -339,6 +339,7 @@
     >>> series_specification.proposeGoal(productseries, owner)
     >>> series_bugtask = factory.makeBugTask(bug=bug, target=productseries)
     >>> subscription = productseries.addSubscription(owner, owner)
+    >>> filter = subscription.newBugFilter()
     >>> productseries.branch = factory.makeBranch()
 
     >>> view = create_view(productseries, name='+delete')