launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Graham Binns has proposed merging lp:~gmb/launchpad/private-branches-bug-657004 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~gmb/launchpad/private-branches-bug-657004/+merge/65639

This branch fixes bug 657004 by making Bug.linked_branches return only those branches that the current user can see.

In order to do this I've changed Bug.linked_branches to a @property which finds any linked branches and returns those for which branch.visibleByUser() is True. Unfortunately, I've had to use ILaunchBag.user in order to be able to get the current user; any suggestions as to how I could have done this without using the LaunchBag would be welcome.
-- 
https://code.launchpad.net/~gmb/launchpad/private-branches-bug-657004/+merge/65639
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/private-branches-bug-657004 into lp:launchpad.

=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2011-06-17 18:08:30 +0000
+++ lib/lp/app/browser/tales.py	2011-06-23 11:08:30 +0000
@@ -925,7 +925,7 @@
 
     def _hasBugBranch(self):
         """Return whether the bug has a branch linked to it."""
-        return self._context.bug.linked_branches.count() > 0
+        return len(self._context.bug.linked_branches) > 0
 
     def _hasSpecification(self):
         """Return whether the bug is linked to a specification."""

=== modified file 'lib/lp/bugs/doc/bugtask-search.txt'
--- lib/lp/bugs/doc/bugtask-search.txt	2011-05-13 16:50:50 +0000
+++ lib/lp/bugs/doc/bugtask-search.txt	2011-06-23 11:08:30 +0000
@@ -992,7 +992,7 @@
     >>> search_params = BugTaskSearchParams(
     ...     user=None, linked_branches=BugBranchSearch.BUGS_WITH_BRANCHES)
     >>> for task in firefox.searchTasks(search_params):
-    ...     print task.bug.id, task.bug.linked_branches.count()
+    ...     print task.bug.id, len(task.bug.linked_branches)
     4 2
     5 1
 
@@ -1002,7 +1002,7 @@
     >>> search_params = BugTaskSearchParams(
     ...     user=None, linked_branches=BugBranchSearch.BUGS_WITHOUT_BRANCHES)
     >>> for task in firefox.searchTasks(search_params):
-    ...     print task.bug.id, task.bug.linked_branches.count()
+    ...     print task.bug.id, len(task.bug.linked_branches)
     1 0
     6 0
     18 0

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-06-17 21:13:22 +0000
+++ lib/lp/bugs/model/bug.py	2011-06-23 11:08:30 +0000
@@ -106,6 +106,7 @@
 from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR,
+    ILaunchBag,
     IStoreSelector,
     MAIN_STORE,
     )
@@ -369,8 +370,6 @@
     questions = SQLRelatedJoin('Question', joinColumn='bug',
         otherColumn='question', intermediateTable='QuestionBug',
         orderBy='-datecreated')
-    linked_branches = SQLMultipleJoin(
-        'BugBranch', joinColumn='bug', orderBy='id')
     date_last_message = UtcDateTimeCol(default=None)
     number_of_duplicates = IntCol(notNull=True, default=0)
     message_count = IntCol(notNull=True, default=0)
@@ -1317,6 +1316,17 @@
             notify(ObjectDeletedEvent(bug_branch, user=user))
             bug_branch.destroySelf()
 
+    @property
+    def linked_branches(self):
+        store = Store.of(self)
+        bug_branches = store.find(
+            BugBranch,
+            BugBranch.bug == self)
+        user = getUtility(ILaunchBag).user
+        return [
+            bug_branch for bug_branch in bug_branches
+                if bug_branch.branch.visibleByUser(user)]
+
     @cachedproperty
     def has_cves(self):
         """See `IBug`."""

=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py	2011-06-16 13:50:58 +0000
+++ lib/lp/bugs/model/tests/test_bug.py	2011-06-23 11:08:30 +0000
@@ -313,3 +313,21 @@
             bug.subscribe(team, person)
             bug.setPrivate(True, person)
             self.assertFalse(bug.personIsDirectSubscriber(person))
+
+    def test_linked_branches_doesnt_return_inaccessible_branches(self):
+        # If a Bug has branches linked to it that the current user
+        # cannot access, those branches will not be returned in its
+        # linked_branches property.
+        bug = self.factory.makeBug()
+        private_branch_owner = self.factory.makePerson()
+        private_branch = self.factory.makeBranch(
+            owner=private_branch_owner, private=True)
+        with person_logged_in(private_branch_owner):
+            bug.linkBranch(private_branch, private_branch.registrant)
+        public_branch = self.factory.makeBranch()
+        with person_logged_in(public_branch.registrant):
+            bug.linkBranch(public_branch, public_branch.registrant)
+            linked_branches = [
+                bug_branch.branch for bug_branch in bug.linked_branches]
+            self.assertIn(public_branch, linked_branches)
+            self.assertNotIn(private_branch, linked_branches)