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
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2009-08-31 03:03:00 +0000
+++ lib/canonical/launchpad/security.py 2009-10-01 11:38:12 +0000
@@ -1577,41 +1577,11 @@
1577 permission = 'launchpad.View'1577 permission = 'launchpad.View'
1578 usedfor = IBranch1578 usedfor = IBranch
15791579
1580 def _checkBranchAuthenticated(self, branch, user):1580 def checkAuthenticated(self, user):
1581 if not branch.private:1581 return self.obj.visibleByUser(user)
1582 return True1582
1583 if user.inTeam(branch.owner):1583 def checkUnauthenticated(self):
1584 return True1584 return self.obj.visibleByUser(None)
1585 for subscriber in branch.subscribers:
1586 if user.inTeam(subscriber):
1587 return True
1588 return user_has_special_branch_access(user)
1589
1590 def checkAuthenticated(self, user, checked_branches=None):
1591 if checked_branches is None:
1592 checked_branches = []
1593 if self.obj in checked_branches:
1594 return True
1595 can_access = self._checkBranchAuthenticated(self.obj, user)
1596 if can_access and self.obj.stacked_on is not None:
1597 checked_branches.append(self.obj)
1598 access = getAdapter(
1599 self.obj.stacked_on, IAuthorization, name='launchpad.View')
1600 can_access = access.checkAuthenticated(user, checked_branches)
1601 return can_access
1602
1603 def checkUnauthenticated(self, checked_branches=None):
1604 if checked_branches is None:
1605 checked_branches = []
1606 if self.obj in checked_branches:
1607 return True
1608 can_access = not self.obj.private
1609 if can_access and self.obj.stacked_on is not None:
1610 checked_branches.append(self.obj)
1611 access = getAdapter(
1612 self.obj.stacked_on, IAuthorization, name='launchpad.View')
1613 can_access = access.checkUnauthenticated(checked_branches)
1614 return can_access
16151585
16161586
1617class EditBranch(AuthorizationBase):1587class EditBranch(AuthorizationBase):
16181588
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml 2009-09-17 23:08:09 +0000
+++ lib/lp/code/configure.zcml 2009-10-01 11:38:12 +0000
@@ -476,6 +476,7 @@
476 needs_upgrading476 needs_upgrading
477 getUpgradeFormat477 getUpgradeFormat
478 isBranchMergeable478 isBranchMergeable
479 visibleByUser
479 "/>480 "/>
480 <require481 <require
481 permission="launchpad.Edit"482 permission="launchpad.Edit"
482483
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2009-09-17 03:10:34 +0000
+++ lib/lp/code/interfaces/branch.py 2009-10-01 11:38:12 +0000
@@ -1008,6 +1008,9 @@
10081008
1009 needs_upgrading = Attribute("Whether the branch needs to be upgraded.")1009 needs_upgrading = Attribute("Whether the branch needs to be upgraded.")
10101010
1011 def visibleByUser(user):
1012 """Can the specified user see this branch?"""
1013
10111014
1012class IBranchSet(Interface):1015class IBranchSet(Interface):
1013 """Interface representing the set of branches."""1016 """Interface representing the set of branches."""
10141017
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2009-09-16 05:18:20 +0000
+++ lib/lp/code/model/branch.py 2009-10-01 11:38:12 +0000
@@ -37,7 +37,6 @@
37from canonical.launchpad import _37from canonical.launchpad import _
38from lp.services.job.model.job import Job38from lp.services.job.model.job import Job
39from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities39from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
40from canonical.launchpad.mailnotification import NotificationRecipientSet
41from canonical.launchpad.webapp import urlappend40from canonical.launchpad.webapp import urlappend
42from canonical.launchpad.webapp.interfaces import (41from canonical.launchpad.webapp.interfaces import (
43 IStoreSelector, MAIN_STORE, SLAVE_FLAVOR)42 IStoreSelector, MAIN_STORE, SLAVE_FLAVOR)
@@ -60,7 +59,7 @@
60 bazaar_identity, BranchCannotBePrivate, BranchCannotBePublic,59 bazaar_identity, BranchCannotBePrivate, BranchCannotBePublic,
61 BranchTargetError, BranchTypeError, CannotDeleteBranch,60 BranchTargetError, BranchTypeError, CannotDeleteBranch,
62 DEFAULT_BRANCH_STATUS_IN_LISTING, IBranch,61 DEFAULT_BRANCH_STATUS_IN_LISTING, IBranch,
63 IBranchNavigationMenu, IBranchSet)62 IBranchNavigationMenu, IBranchSet, user_has_special_branch_access)
64from lp.code.interfaces.branchcollection import IAllBranches63from lp.code.interfaces.branchcollection import IAllBranches
65from lp.code.interfaces.branchlookup import IBranchLookup64from lp.code.interfaces.branchlookup import IBranchLookup
66from lp.code.interfaces.branchmergeproposal import (65from lp.code.interfaces.branchmergeproposal import (
@@ -73,6 +72,8 @@
73 IFindOfficialBranchLinks)72 IFindOfficialBranchLinks)
74from lp.registry.interfaces.person import (73from lp.registry.interfaces.person import (
75 validate_person_not_private_membership, validate_public_person)74 validate_person_not_private_membership, validate_public_person)
75from lp.services.mail.notificationrecipientset import (
76 NotificationRecipientSet)
7677
7778
78class Branch(SQLBase):79class Branch(SQLBase):
@@ -943,6 +944,35 @@
943 return True944 return True
944 return False945 return False
945946
947 def _checkBranchVisibleByUser(self, user):
948 """Is *this* branch visible by the user.
949
950 This method doesn't check the stacked upon branch. That is handled by
951 the `visibleByUser` method.
952 """
953 if not self.private:
954 return True
955 if user is None:
956 return False
957 if user.inTeam(self.owner):
958 return True
959 for subscriber in self.subscribers:
960 if user.inTeam(subscriber):
961 return True
962 return user_has_special_branch_access(user)
963
964 def visibleByUser(self, user, checked_branches=None):
965 """See `IBranch`."""
966 if checked_branches is None:
967 checked_branches = []
968 can_access = self._checkBranchVisibleByUser(user)
969 if can_access and self.stacked_on is not None:
970 checked_branches.append(self)
971 if self.stacked_on not in checked_branches:
972 can_access = self.stacked_on.visibleByUser(
973 user, checked_branches)
974 return can_access
975
946976
947class DeletionOperation:977class DeletionOperation:
948 """Represent an operation to perform as part of branch deletion."""978 """Represent an operation to perform as part of branch deletion."""
949979
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2009-09-09 15:16:45 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2009-10-01 11:38:12 +0000
@@ -243,6 +243,11 @@
243 for branch in branches:243 for branch in branches:
244 branch_recipients = branch.getNotificationRecipients()244 branch_recipients = branch.getNotificationRecipients()
245 for recipient in branch_recipients:245 for recipient in branch_recipients:
246 # If the recipient cannot see either of the branches, skip
247 # them.
248 if (not self.source_branch.visibleByUser(recipient) or
249 not self.target_branch.visibleByUser(recipient)):
250 continue
246 subscription, rationale = branch_recipients.getReason(251 subscription, rationale = branch_recipients.getReason(
247 recipient)252 recipient)
248 if (subscription.review_level < min_level):253 if (subscription.review_level < min_level):
249254
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
--- lib/lp/code/model/tests/test_branchmergeproposals.py 2009-09-19 14:18:09 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2009-10-01 11:38:12 +0000
@@ -837,6 +837,37 @@
837 CodeReviewNotificationLevel.STATUS)837 CodeReviewNotificationLevel.STATUS)
838 self.assertFalse(owner in recipients)838 self.assertFalse(owner in recipients)
839839
840 def test_getNotificationRecipients_privacy(self):
841 # If a user can see only one of the source and target branches, then
842 # they do not get email about the proposal.
843 bmp = self.factory.makeBranchMergeProposal()
844 # Subscribe eric to the source branch only.
845 eric = self.factory.makePerson()
846 bmp.source_branch.subscribe(
847 eric, BranchSubscriptionNotificationLevel.NOEMAIL, None,
848 CodeReviewNotificationLevel.FULL)
849 # Subscribe bob to the target branch only.
850 bob = self.factory.makePerson()
851 bmp.target_branch.subscribe(
852 bob, BranchSubscriptionNotificationLevel.NOEMAIL, None,
853 CodeReviewNotificationLevel.FULL)
854 # Subscribe charlie to both.
855 charlie = self.factory.makePerson()
856 bmp.source_branch.subscribe(
857 charlie, BranchSubscriptionNotificationLevel.NOEMAIL, None,
858 CodeReviewNotificationLevel.FULL)
859 bmp.target_branch.subscribe(
860 charlie, BranchSubscriptionNotificationLevel.NOEMAIL, None,
861 CodeReviewNotificationLevel.FULL)
862 # Make both branches private.
863 removeSecurityProxy(bmp.source_branch).private = True
864 removeSecurityProxy(bmp.target_branch).private = True
865 recipients = bmp.getNotificationRecipients(
866 CodeReviewNotificationLevel.FULL)
867 self.assertFalse(bob in recipients)
868 self.assertFalse(eric in recipients)
869 self.assertTrue(charlie in recipients)
870
840871
841class TestGetAddress(TestCaseWithFactory):872class TestGetAddress(TestCaseWithFactory):
842 """Test that the address property gives expected results."""873 """Test that the address property gives expected results."""