Code review comment for lp:~bac/launchpad/bug-5927

Nice change. I wonder how we ever managed without this!

[1]

- # This is a private bug. Only explicit subscribers may view it.
+ # This is a private bug. Explicit subscribers may view it.
             for subscription in self.subscriptions:
                 if user.inTeam(subscription.person):
                     self._known_viewers.add(user.id)
                     return True
+ # Assignees to bugtasks can also see the private bug.
+ for bugtask in self.bugtasks:
+ if user.inTeam(bugtask.assignee):
+ self._known_viewers.add(user.id)
+ return True

As there are almost always fewer bugtasks than subscriptions, and
assignees are often not set and not usually teams, might this work out
more efficient to loop through bugtask assigness before looping
through subscriptions?

Mmm, I guess it's not that clear-cut. Anyway, just a thought.

[2]

+ def test_privateBugOwner(self):
+ # A regular (non-privileged) user can not view a private bug.
+ self.assertTrue(self.bug.userCanView(self.owner))

I think the comment here is wrong.

[3]

+ def test_privateBugSupervisorPrivateBugsByDefault(self):
+ # A member of the bug supervisor team can see a private bug if the
+ # product is set to have private bugs by default.

I think the reason for this might be that the bug supervisor is
subscribed to new bugs for private-by-default projects. There isn't
any code in userCanView() that provides this behaviour.

If this is the case, the question is what to do with this test. It
probably makes sense to change it into a test for subscriber
visibility.

[4]

+ def test_privateBugAssignee(self):
+ # The bug assignee can see the private bug.
+ bug_assignee = self.factory.makePerson(name="bugassignee")
+ with person_logged_in(self.product.owner):
+ self.bug.default_bugtask.transitionToAssignee(bug_assignee)
+ self.assertTrue(self.bug.userCanView(bug_assignee))

Is it worth asserting that userCanView() returns False before setting
the assignee?

[5]

There might be some gnarled doctest somewhere that does bug visibility
testing. Could you try and remove some of that, or file a bug to trim
it. Here are a few promising looking places:

  lib/lp/bugs/doc/bug.txt
  lib/lp/bugs/stories/bug-privacy
  lib/lp/bugs/stories/upstream-bugprivacy ?