Launchpad itself

Code review comment for lp:~thumper/launchpad/branch-subscription-subscribed-by

  1. branch-subscription-subscribed-by
  2. Merge into db-devel
Revision history for this message
Tim Penhey (thumper) wrote :

On Fri, 28 May 2010 13:58:22 you wrote:
> Review: Needs Information release-critical
> > === modified file 'lib/lp/code/browser/branchsubscription.py'
> Wrap the code at 78 characters.

Done.

> > === modified file 'lib/lp/testing/factory.py'
> Wrap the code at 78 characters.

Done.

Also added the checking.

bs.diff
1=== modified file 'lib/canonical/launchpad/security.py'
2--- lib/canonical/launchpad/security.py 2010-05-27 04:44:39 +0000
3+++ lib/canonical/launchpad/security.py 2010-05-28 04:18:17 +0000
4@@ -24,8 +24,6 @@
5 IBranch, user_has_special_branch_access)
6 from lp.code.interfaces.branchmergeproposal import (
7 IBranchMergeProposal)
8-from lp.code.interfaces.branchsubscription import (
9- IBranchSubscription)
10 from lp.code.interfaces.sourcepackagerecipe import ISourcePackageRecipe
11 from lp.code.interfaces.sourcepackagerecipebuild import (
12 ISourcePackageRecipeBuild)
13@@ -61,8 +59,7 @@
14 from lp.services.worlddata.interfaces.language import ILanguage, ILanguageSet
15 from lp.translations.interfaces.languagepack import ILanguagePack
16 from canonical.launchpad.interfaces.launchpad import (
17- IBazaarApplication, IHasBug, IHasDrivers, ILaunchpadCelebrities,
18- IPersonRoles)
19+ IHasBug, IHasDrivers, ILaunchpadCelebrities, IPersonRoles)
20 from lp.registry.interfaces.role import IHasOwner
21 from lp.registry.interfaces.location import IPersonLocation
22 from lp.registry.interfaces.mailinglist import IMailingListSet
23@@ -1733,26 +1730,6 @@
24 self.obj.distribution).checkAuthenticated(user))
25
26
27-class BranchSubscriptionEdit(AuthorizationBase):
28- permission = 'launchpad.Edit'
29- usedfor = IBranchSubscription
30-
31- def checkAuthenticated(self, user):
32- """Is the user able to edit a branch subscription?
33-
34- Any team member can edit a branch subscription for their team.
35- Launchpad Admins can also edit any branch subscription.
36- """
37- return (user.inTeam(self.obj.person) or
38- user.inTeam(self.obj.subscribed_by) or
39- user.in_admin or
40- user.in_bazaar_experts)
41-
42-
43-class BranchSubscriptionView(BranchSubscriptionEdit):
44- permission = 'launchpad.View'
45-
46-
47 class BranchMergeProposalView(AuthorizationBase):
48 permission = 'launchpad.View'
49 usedfor = IBranchMergeProposal
50
51=== modified file 'lib/lp/code/browser/branchsubscription.py'
52--- lib/lp/code/browser/branchsubscription.py 2010-05-25 03:13:25 +0000
53+++ lib/lp/code/browser/branchsubscription.py 2010-05-28 09:31:03 +0000
54@@ -12,10 +12,9 @@
55 'BranchSubscriptionPrimaryContext',
56 ]
57
58-from zope.component import getUtility
59+
60 from zope.interface import implements
61
62-from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
63 from canonical.launchpad.webapp import (
64 action, canonical_url, LaunchpadEditFormView, LaunchpadFormView,
65 LaunchpadView)
66@@ -210,7 +209,8 @@
67 subscription = self.context.getSubscription(person)
68 if subscription is None:
69 self.context.subscribe(
70- person, notification_level, max_diff_lines, review_level, self.user)
71+ person, notification_level, max_diff_lines, review_level,
72+ self.user)
73
74 self.add_notification_message(
75 '%s has been subscribed to this branch with: '
76
77=== modified file 'lib/lp/code/configure.zcml'
78--- lib/lp/code/configure.zcml 2010-05-07 04:53:47 +0000
79+++ lib/lp/code/configure.zcml 2010-05-28 04:18:17 +0000
80@@ -11,6 +11,7 @@
81 xmlns:webservice="http://namespaces.canonical.com/webservice"
82 i18n_domain="launchpad">
83 <include package=".browser"/>
84+ <authorizations module="lp.code.security" />
85
86 <!-- Branch Merge Queue -->
87
88
89=== modified file 'lib/lp/code/interfaces/branchsubscription.py'
90--- lib/lp/code/interfaces/branchsubscription.py 2010-05-25 01:24:43 +0000
91+++ lib/lp/code/interfaces/branchsubscription.py 2010-05-28 04:18:17 +0000
92@@ -21,7 +21,8 @@
93 from lp.code.interfaces.branch import IBranch
94 from canonical.launchpad.fields import ParticipatingPersonChoice
95 from lazr.restful.declarations import (
96- export_as_webservice_entry, exported)
97+ REQUEST_USER, call_with, export_as_webservice_entry,
98+ export_read_operation, exported)
99 from lazr.restful.fields import Reference
100
101
102@@ -78,3 +79,8 @@
103 title=_('Subscribed by'), required=True,
104 vocabulary='ValidPersonOrTeam', readonly=True,
105 description=_("The person who created this subscription.")))
106+
107+ @call_with(user=REQUEST_USER)
108+ @export_read_operation()
109+ def canBeUnsubscribedByUser(user):
110+ """Can the user unsubscribe the subscriber from the branch?"""
111
112=== modified file 'lib/lp/code/model/branch.py'
113--- lib/lp/code/model/branch.py 2010-05-25 04:37:11 +0000
114+++ lib/lp/code/model/branch.py 2010-05-28 09:04:16 +0000
115@@ -45,6 +45,7 @@
116 from canonical.launchpad.webapp.interfaces import (
117 IStoreSelector, MAIN_STORE, SLAVE_FLAVOR)
118
119+from lp.app.errors import UserCannotUnsubscribePerson
120 from lp.code.bzr import (
121 BranchFormat, ControlFormat, CURRENT_BRANCH_FORMATS,
122 CURRENT_REPOSITORY_FORMATS, RepositoryFormat)
123@@ -705,9 +706,16 @@
124 def unsubscribe(self, person, unsubscribed_by):
125 """See `IBranch`."""
126 subscription = self.getSubscription(person)
127+ if subscription is None:
128+ # Silent success seems order of the day (like bugs).
129+ return
130+ if not subscription.canBeUnsubscribedByUser(unsubscribed_by):
131+ raise UserCannotUnsubscribePerson(
132+ '%s does not have permission to unsubscribe %s.' % (
133+ unsubscribed_by.displayname,
134+ person.displayname))
135 store = Store.of(subscription)
136- assert subscription is not None, "User is not subscribed."
137- BranchSubscription.delete(subscription.id)
138+ store.remove(subscription)
139 store.flush()
140
141 def getBranchRevision(self, sequence=None, revision=None,
142
143=== modified file 'lib/lp/code/model/branchsubscription.py'
144--- lib/lp/code/model/branchsubscription.py 2010-05-25 04:37:11 +0000
145+++ lib/lp/code/model/branchsubscription.py 2010-05-28 04:18:17 +0000
146@@ -13,13 +13,14 @@
147 from canonical.database.constants import DEFAULT
148 from canonical.database.sqlbase import SQLBase
149 from canonical.database.enumcol import EnumCol
150-
151+from canonical.launchpad.interfaces.launchpad import IPersonRoles
152 from lp.code.enums import (
153 BranchSubscriptionDiffSize, BranchSubscriptionNotificationLevel,
154 CodeReviewNotificationLevel)
155 from lp.code.interfaces.branchsubscription import IBranchSubscription
156 from lp.code.interfaces.branch import IBranchNavigationMenu
157 from lp.code.interfaces.branchtarget import IHasBranchTarget
158+from lp.code.security import BranchSubscriptionEdit
159 from lp.registry.interfaces.person import (
160 validate_person_not_private_membership)
161
162@@ -49,3 +50,10 @@
163 def target(self):
164 """See `IHasBranchTarget`."""
165 return self.branch.target
166+
167+ def canBeUnsubscribedByUser(self, user):
168+ """See `IBranchSubscription`."""
169+ if user is None:
170+ return False
171+ permission_check = BranchSubscriptionEdit(self)
172+ return permission_check.checkAuthenticated(IPersonRoles(user))
173
174=== added file 'lib/lp/code/model/tests/test_branchsubscription.py'
175--- lib/lp/code/model/tests/test_branchsubscription.py 1970-01-01 00:00:00 +0000
176+++ lib/lp/code/model/tests/test_branchsubscription.py 2010-05-28 09:04:16 +0000
177@@ -0,0 +1,115 @@
178+# Copyright 2010 Canonical Ltd. This software is licensed under the
179+# GNU Affero General Public License version 3 (see the file LICENSE).
180+
181+"""Tests for the BranchSubscrptions model object.."""
182+
183+from __future__ import with_statement
184+
185+__metaclass__ = type
186+
187+import unittest
188+
189+from canonical.testing.layers import DatabaseFunctionalLayer
190+from lp.app.errors import UserCannotUnsubscribePerson
191+from lp.code.enums import (
192+ BranchSubscriptionNotificationLevel, CodeReviewNotificationLevel)
193+from lp.testing import TestCaseWithFactory, person_logged_in
194+
195+
196+class TestBranchSubscriptions(TestCaseWithFactory):
197+ """Tests relating to branch subscriptions in general."""
198+
199+ layer = DatabaseFunctionalLayer
200+
201+ def test_subscribed_by_set(self):
202+ """The user subscribing is recorded along the subscriber."""
203+ subscriber = self.factory.makePerson()
204+ subscribed_by = self.factory.makePerson()
205+ branch = self.factory.makeAnyBranch()
206+ subscription = branch.subscribe(
207+ subscriber, BranchSubscriptionNotificationLevel.NOEMAIL, None,
208+ CodeReviewNotificationLevel.NOEMAIL, subscribed_by)
209+ self.assertEqual(subscriber, subscription.person)
210+ self.assertEqual(subscribed_by, subscription.subscribed_by)
211+
212+ def test_unsubscribe(self):
213+ """Test unsubscribing by the subscriber."""
214+ subscription = self.factory.makeBranchSubscription()
215+ subscriber = subscription.person
216+ branch = subscription.branch
217+ branch.unsubscribe(subscriber, subscriber)
218+ self.assertFalse(branch.hasSubscription(subscriber))
219+
220+ def test_unsubscribe_by_subscriber(self):
221+ """Test unsubscribing by the person who subscribed the user."""
222+ subscribed_by = self.factory.makePerson()
223+ subscription = self.factory.makeBranchSubscription(
224+ subscribed_by=subscribed_by)
225+ subscriber = subscription.person
226+ branch = subscription.branch
227+ branch.unsubscribe(subscriber, subscribed_by)
228+ self.assertFalse(branch.hasSubscription(subscriber))
229+
230+ def test_unsubscribe_by_unauthorized(self):
231+ """Test unsubscribing someone you shouldn't be able to."""
232+ subscription = self.factory.makeBranchSubscription()
233+ branch = subscription.branch
234+ self.assertRaises(
235+ UserCannotUnsubscribePerson,
236+ branch.unsubscribe,
237+ subscription.person,
238+ self.factory.makePerson())
239+
240+
241+class TestBranchSubscriptionCanBeUnsubscribedbyUser(TestCaseWithFactory):
242+ """Tests for BranchSubscription.canBeUnsubscribedByUser."""
243+
244+ layer = DatabaseFunctionalLayer
245+
246+ def test_none(self):
247+ """None for a user always returns False."""
248+ subscription = self.factory.makeBranchSubscription()
249+ self.assertFalse(subscription.canBeUnsubscribedByUser(None))
250+
251+ def test_self_subscriber(self):
252+ """The subscriber has permission to unsubscribe."""
253+ subscription = self.factory.makeBranchSubscription()
254+ self.assertTrue(
255+ subscription.canBeUnsubscribedByUser(subscription.person))
256+
257+ def test_non_subscriber_fails(self):
258+ """An unrelated person can't unsubscribe a user."""
259+ subscription = self.factory.makeBranchSubscription()
260+ editor = self.factory.makePerson()
261+ self.assertFalse(subscription.canBeUnsubscribedByUser(editor))
262+
263+ def test_subscribed_by(self):
264+ """If a user subscribes someone else, the user can unsubscribe."""
265+ subscribed_by = self.factory.makePerson()
266+ subscriber = self.factory.makePerson()
267+ subscription = self.factory.makeBranchSubscription(
268+ person=subscriber, subscribed_by=subscribed_by)
269+ self.assertTrue(subscription.canBeUnsubscribedByUser(subscribed_by))
270+
271+ def test_team_member_can_unsubscribe(self):
272+ """Any team member can unsubscribe the team from a branch."""
273+ team = self.factory.makeTeam()
274+ member = self.factory.makePerson()
275+ with person_logged_in(team.teamowner):
276+ team.addMember(member, team.teamowner)
277+ subscription = self.factory.makeBranchSubscription(
278+ person=team, subscribed_by=team.teamowner)
279+ self.assertTrue(subscription.canBeUnsubscribedByUser(member))
280+
281+ def test_team_subscriber_can_unsubscribe(self):
282+ """A team can be unsubscribed by the subscriber even if they are not a
283+ member."""
284+ team = self.factory.makeTeam()
285+ subscribed_by = self.factory.makePerson()
286+ subscription = self.factory.makeBranchSubscription(
287+ person=team, subscribed_by=subscribed_by)
288+ self.assertTrue(subscription.canBeUnsubscribedByUser(subscribed_by))
289+
290+
291+def test_suite():
292+ return unittest.TestLoader().loadTestsFromName(__name__)
293
294=== added file 'lib/lp/code/security.py'
295--- lib/lp/code/security.py 1970-01-01 00:00:00 +0000
296+++ lib/lp/code/security.py 2010-05-28 04:18:17 +0000
297@@ -0,0 +1,37 @@
298+# Copyright 2010 Canonical Ltd. This software is licensed under the
299+# GNU Affero General Public License version 3 (see the file LICENSE).
300+
301+"""Security adapters for the code module."""
302+
303+__metaclass__ = type
304+__all__ = [
305+ 'BranchSubscriptionEdit',
306+ 'BranchSubscriptionView',
307+ ]
308+
309+from canonical.launchpad.security import AuthorizationBase
310+
311+from lp.code.interfaces.branchsubscription import IBranchSubscription
312+
313+
314+
315+class BranchSubscriptionEdit(AuthorizationBase):
316+ permission = 'launchpad.Edit'
317+ usedfor = IBranchSubscription
318+
319+ def checkAuthenticated(self, user):
320+ """Is the user able to edit a branch subscription?
321+
322+ Any team member can edit a branch subscription for their team.
323+ Launchpad Admins can also edit any branch subscription.
324+ """
325+ return (user.inTeam(self.obj.person) or
326+ user.inTeam(self.obj.subscribed_by) or
327+ user.in_admin or
328+ user.in_bazaar_experts)
329+
330+
331+class BranchSubscriptionView(BranchSubscriptionEdit):
332+ permission = 'launchpad.View'
333+
334+
335
336=== modified file 'lib/lp/testing/factory.py'
337--- lib/lp/testing/factory.py 2010-05-25 01:24:43 +0000
338+++ lib/lp/testing/factory.py 2010-05-28 09:31:03 +0000
339@@ -986,7 +986,8 @@
340
341 return proposal
342
343- def makeBranchSubscription(self, branch=None, person=None, subscribed_by=None):
344+ def makeBranchSubscription(self, branch=None, person=None,
345+ subscribed_by=None):
346 """Create a BranchSubscription.
347
348 :param branch_title: The title to use for the created Branch

« Back to merge proposal