launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #717609 in Launchpad itself: "unassigning a private bug results in a red error box"
  https://bugs.launchpad.net/launchpad/+bug/717609

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/unassign-private-bug/+merge/53945

Add ability to manually cache view access permission for a user on a bug during the processing of object modification events.

== Implementation ==

When a user unassigns or unsubscribes themselves from a private bug, an event listener is used to send out notifications to subscribers etc. However, when this happens, the bug is now not visible to the user since they have already been unassigned/unsubscribed. This branch adds an api to Bug to allow a caller to cache view access permission for a given user so that subsequent calls to Bug.userCanView() succeed. This new API is used by the listener code before it starts to send out notifications, allowing the notification code to run successfully.

== Tests ==

Added new tests to TestPrivateBugVisibility:
  test_privateBugRegularUserWithCachedAssigneePermission
  test_privateBugRegularUserWithCachedSubscriberPermission

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/configure.zcml
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/subscribers/bugtask.py
  lib/lp/bugs/tests/test_bugvisibility.py

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

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2011-03-16 13:26:30 +0000
+++ lib/lp/bugs/configure.zcml	2011-03-18 04:53:30 +0000
@@ -684,6 +684,7 @@
                     who_made_private
                     date_made_private
                     userCanView
+                    _cacheViewPermission
                     personIsDirectSubscriber
                     personIsAlsoNotifiedSubscriber
                     personIsSubscribedToDuplicate

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-03-16 13:26:30 +0000
+++ lib/lp/bugs/model/bug.py	2011-03-18 04:53:30 +0000
@@ -1807,6 +1807,31 @@
         """
         return set()
 
+    def _cacheViewPermission(self, user, assignee=None, subscribers=None):
+        """Cache view permissions for subsequent call to userCanView.
+
+        When a bug is modified by having is assignee or subscribers changed,
+        it could be that the user may not be able to view the bug anymore.
+        But this doesn't account for the fact that at the time the change was
+        made, the user could view the bug. The result is that an error occurs
+        when bug change notifications are attempted to be published. This
+        method allows the bug modified notification handler to cache view
+        permissions for a given user for the original bug assignee and
+        subscriptions as they were before the change was made.
+        """
+
+        # Assignees to bugtasks can see the bug.
+        if assignee is not None and user.inTeam(assignee):
+            self._known_viewers.add(user.id)
+
+        if subscribers is None:
+            return
+        # Explicit subscribers may also view it.
+        for subscriber in subscribers:
+            if user.inTeam(subscriber):
+                self._known_viewers.add(user.id)
+                return
+
     def userCanView(self, user):
         """See `IBug`.
 

=== modified file 'lib/lp/bugs/subscribers/bugtask.py'
--- lib/lp/bugs/subscribers/bugtask.py	2010-08-23 15:10:48 +0000
+++ lib/lp/bugs/subscribers/bugtask.py	2011-03-18 04:53:30 +0000
@@ -54,26 +54,32 @@
     modified_bugtask must be an IBugTask. event must be an
     IObjectModifiedEvent.
     """
-    bugtask_delta = event.object.getDelta(event.object_before_modification)
+    bugtask_before_modification = event.object_before_modification
+    bug = event.object.bug
+
+    bugtask_delta = event.object.getDelta(bugtask_before_modification)
     bug_delta = BugDelta(
-        bug=event.object.bug,
-        bugurl=canonical_url(event.object.bug),
+        bug=bug,
+        bugurl=canonical_url(bug),
         bugtask_deltas=bugtask_delta,
         user=IPerson(event.user))
 
     event_creator = IPerson(event.user)
-    previous_subscribers = event.object_before_modification.bug_subscribers
+    previous_subscribers = bugtask_before_modification.bug_subscribers
     current_subscribers = event.object.bug_subscribers
     prev_subs_set = set(previous_subscribers)
     cur_subs_set = set(current_subscribers)
     new_subs = cur_subs_set.difference(prev_subs_set)
 
+    bug._cacheViewPermission(
+        event.user.person, bugtask_before_modification.assignee,
+        previous_subscribers)
     add_bug_change_notifications(
-        bug_delta, old_bugtask=event.object_before_modification,
+        bug_delta, old_bugtask=bugtask_before_modification,
         new_subscribers=new_subs)
 
     send_bug_details_to_new_bug_subscribers(
-        event.object.bug, previous_subscribers, current_subscribers,
+        bug, previous_subscribers, current_subscribers,
         event_creator=event_creator)
 
     update_security_contact_subscriptions(event)

=== modified file 'lib/lp/bugs/tests/test_bugvisibility.py'
--- lib/lp/bugs/tests/test_bugvisibility.py	2011-01-06 20:59:52 +0000
+++ lib/lp/bugs/tests/test_bugvisibility.py	2011-03-18 04:53:30 +0000
@@ -5,7 +5,6 @@
 
 from canonical.testing.layers import LaunchpadFunctionalLayer
 from lp.testing import (
-    celebrity_logged_in,
     person_logged_in,
     TestCaseWithFactory,
     )
@@ -58,6 +57,20 @@
         user = self.factory.makePerson()
         self.assertFalse(self.bug.userCanView(user))
 
+    def test_privateBugRegularUserWithCachedAssigneePermission(self):
+        # A regular (non-privileged) user can view a private bug if the
+        # permissions have been cached.
+        user = self.factory.makePerson()
+        self.bug._cacheViewPermission(user, assignee=user)
+        self.assertTrue(self.bug.userCanView(user))
+
+    def test_privateBugRegularUserWithCachedSubscriberPermission(self):
+        # A regular (non-privileged) user can view a private bug if the
+        # permissions have been cached.
+        user = self.factory.makePerson()
+        self.bug._cacheViewPermission(user, subscribers=[user])
+        self.assertTrue(self.bug.userCanView(user))
+
     def test_privateBugOwner(self):
         # The bug submitter may view a private bug.
         self.assertTrue(self.bug.userCanView(self.owner))