launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/private-bug-visibility-741234 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #741234 in Launchpad itself: "product:+code-index merge proposal queries do not show private bugs visible by assignment"
  https://bugs.launchpad.net/launchpad/+bug/741234

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/private-bug-visibility-741234/+merge/91975

== Implementation ==

The core issue was that BugBranchSet.getBranchesWithVisibleBugs() executed a Storm query which did not check for bugs assigned to the user. But this code is really just a pseudo duplicate of other bug visibility stuff that lives elsewhere. So I refactored it to use the bug module's get_privacy_filter() method which does the right thing by definition - it's the method used in the security adaptor etc. 

The other issue was that there was duplication in the BranchListingItem class - overridden methods which are never called but also attempt to determine whether badges should be shown. This duplicate code was deleted, and in actual fact produced potentially more sql so was not as efficient anyway.

== QA ==

Link a private bug to a branch. Assign a user to a private bug task (ensuring they can not previously have seen the bug). Log in as that user and ensure there is a bug badge on the code listing page showing the branch to which the bug is linked.

== Tests ==

Added two new tests to TestBugBranchSet:
- test_getBranchesWithVisibleBugs_shows_private_bugs_to_assignee
- test_getBranchesWithVisibleBugs_shows_private_bugs_to_admins

The second test above tests functionality which was previously never tested AFAICT. 

== Lint ==

Bit of drive by lint done also.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/bugbranch.py
  lib/lp/bugs/tests/test_bugbranch.py
  lib/lp/code/browser/branchlisting.py

./lib/lp/bugs/tests/test_bugbranch.py
      50: local variable 'bug_b' is assigned to but never used

-- 
https://code.launchpad.net/~wallyworld/launchpad/private-bug-visibility-741234/+merge/91975
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/private-bug-visibility-741234 into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bugbranch.py'
--- lib/lp/bugs/model/bugbranch.py	2011-12-30 06:14:56 +0000
+++ lib/lp/bugs/model/bugbranch.py	2012-02-08 04:32:18 +0000
@@ -16,23 +16,15 @@
     IntCol,
     StringCol,
     )
-from storm.expr import (
-    And,
-    Exists,
-    Or,
-    Select,
-    )
-from zope.component import getUtility
 from zope.interface import implements
 
-from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.bugs.interfaces.bugbranch import (
     IBugBranch,
     IBugBranchSet,
     )
+from lp.bugs.model.bugtask import get_bug_privacy_filter
 from lp.code.interfaces.branchtarget import IHasBranchTarget
 from lp.registry.interfaces.person import validate_public_person
-from lp.registry.model.teammembership import TeamParticipation
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.datetimecol import UtcDateTimeCol
 from lp.services.database.lpstorm import IStore
@@ -73,52 +65,27 @@
     implements(IBugBranchSet)
 
     def getBugBranch(self, bug, branch):
-        "See `IBugBranchSet`."
+        """See `IBugBranchSet`."""
         return BugBranch.selectOneBy(bugID=bug.id, branchID=branch.id)
 
     def getBranchesWithVisibleBugs(self, branches, user):
         """See `IBugBranchSet`."""
         # Avoid circular imports.
         from lp.bugs.model.bug import Bug
-        from lp.bugs.model.bugsubscription import BugSubscription
 
         branch_ids = [branch.id for branch in branches]
-        if branch_ids == []:
+        if not branch_ids:
             return []
 
-        admins = getUtility(ILaunchpadCelebrities).admin
-        if user is None:
-            # Anonymous visitors only get to know about public bugs.
-            visible = And(
-                Bug.id == BugBranch.bugID,
-                Bug.private == False)
-        elif user.inTeam(admins):
-            # Administrators know about all bugs.
-            visible = True
-        else:
-            # Anyone else can know about public bugs plus any private
-            # ones they may be directly or indirectly subscribed to.
-            subscribed = And(
-                TeamParticipation.teamID == BugSubscription.person_id,
-                TeamParticipation.personID == user.id,
-                Bug.id == BugSubscription.bug_id)
-
-            visible = And(
-                Bug.id == BugBranch.bugID,
-                Or(
-                    Bug.private == False,
-                    Exists(Select(
-                        columns=[True],
-                        tables=[BugSubscription, TeamParticipation],
-                        where=subscribed))))
-
+        visible = get_bug_privacy_filter(user) or True
         return IStore(BugBranch).find(
             BugBranch.branchID,
             BugBranch.branch_id.is_in(branch_ids),
+            Bug.id == BugBranch.bugID,
             visible).config(distinct=True)
 
     def getBugBranchesForBugTasks(self, tasks):
-        "See IBugBranchSet."
+        """See `IBugBranchSet`."""
         bug_ids = [task.bugID for task in tasks]
         if not bug_ids:
             return []

=== modified file 'lib/lp/bugs/tests/test_bugbranch.py'
--- lib/lp/bugs/tests/test_bugbranch.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/tests/test_bugbranch.py	2012-02-08 04:32:18 +0000
@@ -8,6 +8,7 @@
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
 
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.bugs.interfaces.bugbranch import (
     IBugBranch,
     IBugBranchSet,
@@ -130,6 +131,30 @@
         self.assertContentEqual(
             [branch.id], utility.getBranchesWithVisibleBugs([branch], user))
 
+    def test_getBranchesWithVisibleBugs_shows_private_bugs_to_assignee(self):
+        # getBranchesWithVisibleBugs will show private bugs to their
+        # assignees.
+        branch = self.factory.makeBranch()
+        bug = self.factory.makeBug(private=True)
+        user = self.factory.makePerson()
+        with celebrity_logged_in('admin'):
+            bug.default_bugtask.transitionToAssignee(user)
+            bug.linkBranch(branch, self.factory.makePerson())
+        utility = getUtility(IBugBranchSet)
+        self.assertContentEqual(
+            [branch.id], utility.getBranchesWithVisibleBugs([branch], user))
+
+    def test_getBranchesWithVisibleBugs_shows_private_bugs_to_admins(self):
+        # getBranchesWithVisibleBugs will show private bugs to admins.
+        branch = self.factory.makeBranch()
+        bug = self.factory.makeBug(private=True)
+        with celebrity_logged_in('admin'):
+            bug.linkBranch(branch, self.factory.makePerson())
+        utility = getUtility(IBugBranchSet)
+        admin = getUtility(ILaunchpadCelebrities).admin
+        self.assertContentEqual(
+            [branch.id], utility.getBranchesWithVisibleBugs([branch], admin))
+
     def test_getBugBranchesForBugTasks(self):
         # IBugBranchSet.getBugBranchesForBugTasks returns all of the BugBranch
         # objects associated with the given bug tasks.

=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py	2012-01-06 11:08:30 +0000
+++ lib/lp/code/browser/branchlisting.py	2012-02-08 04:32:18 +0000
@@ -147,32 +147,10 @@
 class BranchBadges(HasBadgeBase):
     badges = "private", "bug", "blueprint", "warning", "mergeproposal"
 
-    def isBugBadgeVisible(self):
-        """Show a bug badge if the branch is linked to bugs."""
-        # Only show the badge if at least one bug is visible by the user.
-        for bug in self.context.linked_bugs:
-            # Stop on the first visible one.
-            if check_permission('launchpad.View', bug):
-                return True
-        return False
-
-    def isBlueprintBadgeVisible(self):
-        """Show a blueprint badge if the branch is linked to blueprints."""
-        # When specs get privacy, this will need to be adjusted.
-        return self.context.spec_links.count() > 0
-
     def isWarningBadgeVisible(self):
         """Show a warning badge if there are mirror failures."""
         return self.context.mirror_failures > 0
 
-    def isMergeproposalBadgeVisible(self):
-        """Show a proposal badge if there are any landing targets."""
-        for proposal in self.context.landing_targets:
-            # Stop on the first visible one.
-            if check_permission('launchpad.View', proposal):
-                return True
-        return False
-
     def getBadge(self, badge_name):
         """See `IHasBadges`."""
         if badge_name == "warning":