Launchpad itself

Merge lp:~thumper/launchpad/bmp-notification-recipients into lp:launchpad

  1. bmp-notification-recipients
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Merged at revision: not available
Proposed branch: lp:~thumper/launchpad/bmp-notification-recipients
Merge into: lp:launchpad
Diff against target: 200 lines
6 files modified
lib/canonical/launchpad/security.py (+5/-35)
lib/lp/code/configure.zcml (+1/-0)
lib/lp/code/interfaces/branch.py (+3/-0)
lib/lp/code/model/branch.py (+32/-2)
lib/lp/code/model/branchmergeproposal.py (+5/-0)
lib/lp/code/model/tests/test_branchmergeproposals.py (+31/-0)
To merge this branch: bzr merge lp:~thumper/launchpad/bmp-notification-recipients
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Review via email: mp+12399@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

Moves the permission logic for viewing a branch into the model code.

test: TestAccessBranch

Checks this method inside the getNotificationRecipients method of the merge proposal.

test: test_getNotificationRecipients_privacy

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Looks fine.

The "potato programming" aspect upsets me slightly -- surely you can get all subscribers that can see the merge proposal in one database query! -- but at least it's not in the webapp and it's a very important bug to fix, so let's land it.

review: Approve
Revision history for this message
Tim Penhey (thumper) wrote :

On Fri, 25 Sep 2009 13:52:24 Michael Hudson wrote:
> Review: Approve
> Looks fine.
>
> The "potato programming" aspect upsets me slightly -- surely you can get
> all subscribers that can see the merge proposal in one database query! --
> but at least it's not in the webapp and it's a very important bug to fix,
> so let's land it.

"potato programming"? Can you explain a bit more?

Yes we probably should add a query that gets all the people that can see a
merge proposal, although it is likely to be a chunky query.

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Tim Penhey wrote:
> On Fri, 25 Sep 2009 13:52:24 Michael Hudson wrote:
>> Review: Approve
>> Looks fine.
>>
>> The "potato programming" aspect upsets me slightly -- surely you can get
>> all subscribers that can see the merge proposal in one database query! --
>> but at least it's not in the webapp and it's a very important bug to fix,
>> so let's land it.
>
> "potato programming"? Can you explain a bit more?

http://www.divmod.org/trac/wiki/PotatoProgramming

The usual kind of programming using an ORM that results in pages doing
500 queries.

> Yes we probably should add a query that gets all the people that can see a
> merge proposal, although it is likely to be a chunky query.

I don't think it's a real priority unless the script starts running slowly.

Cheers,
mwh

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/security.py'
2--- lib/canonical/launchpad/security.py 2009-08-31 03:03:00 +0000
3+++ lib/canonical/launchpad/security.py 2009-10-01 11:38:12 +0000
4@@ -1577,41 +1577,11 @@
5 permission = 'launchpad.View'
6 usedfor = IBranch
7
8- def _checkBranchAuthenticated(self, branch, user):
9- if not branch.private:
10- return True
11- if user.inTeam(branch.owner):
12- return True
13- for subscriber in branch.subscribers:
14- if user.inTeam(subscriber):
15- return True
16- return user_has_special_branch_access(user)
17-
18- def checkAuthenticated(self, user, checked_branches=None):
19- if checked_branches is None:
20- checked_branches = []
21- if self.obj in checked_branches:
22- return True
23- can_access = self._checkBranchAuthenticated(self.obj, user)
24- if can_access and self.obj.stacked_on is not None:
25- checked_branches.append(self.obj)
26- access = getAdapter(
27- self.obj.stacked_on, IAuthorization, name='launchpad.View')
28- can_access = access.checkAuthenticated(user, checked_branches)
29- return can_access
30-
31- def checkUnauthenticated(self, checked_branches=None):
32- if checked_branches is None:
33- checked_branches = []
34- if self.obj in checked_branches:
35- return True
36- can_access = not self.obj.private
37- if can_access and self.obj.stacked_on is not None:
38- checked_branches.append(self.obj)
39- access = getAdapter(
40- self.obj.stacked_on, IAuthorization, name='launchpad.View')
41- can_access = access.checkUnauthenticated(checked_branches)
42- return can_access
43+ def checkAuthenticated(self, user):
44+ return self.obj.visibleByUser(user)
45+
46+ def checkUnauthenticated(self):
47+ return self.obj.visibleByUser(None)
48
49
50 class EditBranch(AuthorizationBase):
51
52=== modified file 'lib/lp/code/configure.zcml'
53--- lib/lp/code/configure.zcml 2009-09-17 23:08:09 +0000
54+++ lib/lp/code/configure.zcml 2009-10-01 11:38:12 +0000
55@@ -476,6 +476,7 @@
56 needs_upgrading
57 getUpgradeFormat
58 isBranchMergeable
59+ visibleByUser
60 "/>
61 <require
62 permission="launchpad.Edit"
63
64=== modified file 'lib/lp/code/interfaces/branch.py'
65--- lib/lp/code/interfaces/branch.py 2009-09-17 03:10:34 +0000
66+++ lib/lp/code/interfaces/branch.py 2009-10-01 11:38:12 +0000
67@@ -1008,6 +1008,9 @@
68
69 needs_upgrading = Attribute("Whether the branch needs to be upgraded.")
70
71+ def visibleByUser(user):
72+ """Can the specified user see this branch?"""
73+
74
75 class IBranchSet(Interface):
76 """Interface representing the set of branches."""
77
78=== modified file 'lib/lp/code/model/branch.py'
79--- lib/lp/code/model/branch.py 2009-09-16 05:18:20 +0000
80+++ lib/lp/code/model/branch.py 2009-10-01 11:38:12 +0000
81@@ -37,7 +37,6 @@
82 from canonical.launchpad import _
83 from lp.services.job.model.job import Job
84 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
85-from canonical.launchpad.mailnotification import NotificationRecipientSet
86 from canonical.launchpad.webapp import urlappend
87 from canonical.launchpad.webapp.interfaces import (
88 IStoreSelector, MAIN_STORE, SLAVE_FLAVOR)
89@@ -60,7 +59,7 @@
90 bazaar_identity, BranchCannotBePrivate, BranchCannotBePublic,
91 BranchTargetError, BranchTypeError, CannotDeleteBranch,
92 DEFAULT_BRANCH_STATUS_IN_LISTING, IBranch,
93- IBranchNavigationMenu, IBranchSet)
94+ IBranchNavigationMenu, IBranchSet, user_has_special_branch_access)
95 from lp.code.interfaces.branchcollection import IAllBranches
96 from lp.code.interfaces.branchlookup import IBranchLookup
97 from lp.code.interfaces.branchmergeproposal import (
98@@ -73,6 +72,8 @@
99 IFindOfficialBranchLinks)
100 from lp.registry.interfaces.person import (
101 validate_person_not_private_membership, validate_public_person)
102+from lp.services.mail.notificationrecipientset import (
103+ NotificationRecipientSet)
104
105
106 class Branch(SQLBase):
107@@ -943,6 +944,35 @@
108 return True
109 return False
110
111+ def _checkBranchVisibleByUser(self, user):
112+ """Is *this* branch visible by the user.
113+
114+ This method doesn't check the stacked upon branch. That is handled by
115+ the `visibleByUser` method.
116+ """
117+ if not self.private:
118+ return True
119+ if user is None:
120+ return False
121+ if user.inTeam(self.owner):
122+ return True
123+ for subscriber in self.subscribers:
124+ if user.inTeam(subscriber):
125+ return True
126+ return user_has_special_branch_access(user)
127+
128+ def visibleByUser(self, user, checked_branches=None):
129+ """See `IBranch`."""
130+ if checked_branches is None:
131+ checked_branches = []
132+ can_access = self._checkBranchVisibleByUser(user)
133+ if can_access and self.stacked_on is not None:
134+ checked_branches.append(self)
135+ if self.stacked_on not in checked_branches:
136+ can_access = self.stacked_on.visibleByUser(
137+ user, checked_branches)
138+ return can_access
139+
140
141 class DeletionOperation:
142 """Represent an operation to perform as part of branch deletion."""
143
144=== modified file 'lib/lp/code/model/branchmergeproposal.py'
145--- lib/lp/code/model/branchmergeproposal.py 2009-09-09 15:16:45 +0000
146+++ lib/lp/code/model/branchmergeproposal.py 2009-10-01 11:38:12 +0000
147@@ -243,6 +243,11 @@
148 for branch in branches:
149 branch_recipients = branch.getNotificationRecipients()
150 for recipient in branch_recipients:
151+ # If the recipient cannot see either of the branches, skip
152+ # them.
153+ if (not self.source_branch.visibleByUser(recipient) or
154+ not self.target_branch.visibleByUser(recipient)):
155+ continue
156 subscription, rationale = branch_recipients.getReason(
157 recipient)
158 if (subscription.review_level < min_level):
159
160=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
161--- lib/lp/code/model/tests/test_branchmergeproposals.py 2009-09-19 14:18:09 +0000
162+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2009-10-01 11:38:12 +0000
163@@ -837,6 +837,37 @@
164 CodeReviewNotificationLevel.STATUS)
165 self.assertFalse(owner in recipients)
166
167+ def test_getNotificationRecipients_privacy(self):
168+ # If a user can see only one of the source and target branches, then
169+ # they do not get email about the proposal.
170+ bmp = self.factory.makeBranchMergeProposal()
171+ # Subscribe eric to the source branch only.
172+ eric = self.factory.makePerson()
173+ bmp.source_branch.subscribe(
174+ eric, BranchSubscriptionNotificationLevel.NOEMAIL, None,
175+ CodeReviewNotificationLevel.FULL)
176+ # Subscribe bob to the target branch only.
177+ bob = self.factory.makePerson()
178+ bmp.target_branch.subscribe(
179+ bob, BranchSubscriptionNotificationLevel.NOEMAIL, None,
180+ CodeReviewNotificationLevel.FULL)
181+ # Subscribe charlie to both.
182+ charlie = self.factory.makePerson()
183+ bmp.source_branch.subscribe(
184+ charlie, BranchSubscriptionNotificationLevel.NOEMAIL, None,
185+ CodeReviewNotificationLevel.FULL)
186+ bmp.target_branch.subscribe(
187+ charlie, BranchSubscriptionNotificationLevel.NOEMAIL, None,
188+ CodeReviewNotificationLevel.FULL)
189+ # Make both branches private.
190+ removeSecurityProxy(bmp.source_branch).private = True
191+ removeSecurityProxy(bmp.target_branch).private = True
192+ recipients = bmp.getNotificationRecipients(
193+ CodeReviewNotificationLevel.FULL)
194+ self.assertFalse(bob in recipients)
195+ self.assertFalse(eric in recipients)
196+ self.assertTrue(charlie in recipients)
197+
198
199 class TestGetAddress(TestCaseWithFactory):
200 """Test that the address property gives expected results."""