Launchpad itself

Merge lp:~wallyworld/launchpad/unassign-private-bug into lp:launchpad

  1. unassign-private-bug
  2. Merge into devel
Proposed by Ian Booth
Status: Merged
Approved by: Robert Collins
Approved revision: no longer in the source branch.
Merged at revision: 12650
Proposed branch: lp:~wallyworld/launchpad/unassign-private-bug
Merge into: lp:launchpad
Diff against target: 182 lines (+87/-36)
4 files modified
lib/lp/bugs/model/bug.py (+3/-0)
lib/lp/bugs/model/bugtask.py (+5/-2)
lib/lp/bugs/tests/test_bugchanges.py (+79/-33)
lib/lp/bugs/tests/test_bugvisibility.py (+0/-1)
To merge this branch: bzr merge lp:~wallyworld/launchpad/unassign-private-bug
Reviewer Review Type Date Requested Status
Robert Collins (community) Approve
Review via email: mp+53945@code.launchpad.net

Commit message

[r=lifeless][bug=717609] Add ability to manually cache view access permission for a user on a bug during the processing of object modification events.

Description of the change

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

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

This seems much more complex than needed.

When unassigning, you know the current /user/ has permission

All you need to do is unconditionally add that user to the cache. I couldn't really make heads or tails of your logic - and it looks to do db lookups that aren't needed.

Sorry to be a wet blanket, but I think simpler is better here.

review: Needs Fixing
Revision history for this message
Ian Booth (wallyworld) wrote :

> This seems much more complex than needed.
>
> When unassigning, you know the current /user/ has permission
>
> All you need to do is unconditionally add that user to the cache. I couldn't
> really make heads or tails of your logic - and it looks to do db lookups that
> aren't needed.
>
> Sorry to be a wet blanket, but I think simpler is better here.

I've fixed the implementation as requested and written new tests to compliment the existing ones I found in lp.bugs.tests.test_bugchanges. I refactored the existing test_unassign_bugtask to become a helper _test_unassign_bugtask and created new test_unassign_bugtask and test_unassign_private_bugtask tests to call it.

Revision history for this message
Robert Collins (lifeless) wrote :

This is much cleaner - thanks.

Uhm, its not clear to me why the person unassigning themselves wouldn't get a notification if they would have on a public bug. Is that just the way it works, or deliberate?

Your test doesn't make it clear that the case under test is 'the assignee unassigns *themself* and *can only see the bug due to being assigned to it*. Both those conditions must be true to trigger the fault you are fixing.

If your test already does test this, consider tweaking the explanation in it to make this clear (and why it matters).

review: Approve
Revision history for this message
Ian Booth (wallyworld) wrote :

>
> Uhm, its not clear to me why the person unassigning themselves wouldn't get a notification if they would have on a public bug. Is that just the way it works, or deliberate?
>

Good question. That's the way it works. It sort of makes sense - once
you are unassigned, the bug is invisible to you since it is private.

> Your test doesn't make it clear that the case under test is 'the assignee unassigns *themself* and *can only see the bug due to being assigned to it*. Both those conditions must be true to trigger the fault you are fixing.
>
> If your test already does test this, consider tweaking the explanation in it to make this clear (and why it matters).

I'll fix the explanation.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/model/bug.py'
2--- lib/lp/bugs/model/bug.py 2011-03-16 13:26:30 +0000
3+++ lib/lp/bugs/model/bug.py 2011-03-23 03:23:40 +0000
4@@ -780,6 +780,9 @@
5 """See `IBug`."""
6 # Drop cached subscription info.
7 clear_property_cache(self)
8+ # Ensure the unsubscriber is in the _known_viewer cache for the bug so
9+ # that the permissions are such that the operation can succeed.
10+ get_property_cache(self)._known_viewers = set([unsubscribed_by.id])
11 if person is None:
12 person = unsubscribed_by
13
14
15=== modified file 'lib/lp/bugs/model/bugtask.py'
16--- lib/lp/bugs/model/bugtask.py 2011-03-20 22:39:45 +0000
17+++ lib/lp/bugs/model/bugtask.py 2011-03-23 03:23:40 +0000
18@@ -1015,8 +1015,11 @@
19 # value.
20 self.date_assigned = None
21 # The bugtask is unassigned, so clear the _known_viewer cached
22- # property for the bug.
23- get_property_cache(self.bug)._known_viewers = set()
24+ # property for the bug. Retain the current assignee as a viewer so
25+ # that they are able to unassign themselves and get confirmation
26+ # that that worked.
27+ get_property_cache(self.bug)._known_viewers = set(
28+ [self.assignee.id])
29 if not self.assignee and assignee:
30 # The task is going from not having an assignee to having
31 # one, so record when this happened
32
33=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
34--- lib/lp/bugs/tests/test_bugchanges.py 2011-02-22 10:44:48 +0000
35+++ lib/lp/bugs/tests/test_bugchanges.py 2011-03-23 03:23:40 +0000
36@@ -217,6 +217,22 @@
37 person=self.user)
38 self.assertRecordedChange(expected_activity=unsubscribe_activity)
39
40+ def test_unsubscribe_private_bug(self):
41+ # Test that a person can unsubscribe themselves from a private bug
42+ # that they are not assigned to.
43+ subscriber = self.factory.makePerson(displayname='Mom')
44+ # Create the private bug.
45+ bug = self.factory.makeBug(
46+ product=self.product, owner=self.user, private=True)
47+ bug.subscribe(subscriber, self.user)
48+ self.saveOldChanges(bug=bug)
49+ bug.unsubscribe(subscriber, subscriber)
50+ unsubscribe_activity = dict(
51+ whatchanged=u'removed subscriber Mom',
52+ person=subscriber)
53+ self.assertRecordedChange(
54+ expected_activity=unsubscribe_activity, bug=bug)
55+
56 def test_title_changed(self):
57 # Changing the title of a Bug adds items to the activity log and
58 # the Bug's notifications.
59@@ -1131,48 +1147,78 @@
60 expected_activity=expected_activity,
61 expected_notification=expected_notification)
62
63+ def _test_unassign_bugtask(self, bug_task, expected_recipients):
64+ # A helper method used by tests for unassigning public and private bug
65+ # tasks.
66+ # Unassigning a bug task assigned to someone adds entries to the
67+ # bug activity and notifications sets.
68+
69+ old_assignee = bug_task.assignee
70+ bug_task_before_modification = Snapshot(
71+ bug_task, providing=providedBy(bug_task))
72+
73+ bug_task.transitionToAssignee(None)
74+
75+ notify(ObjectModifiedEvent(
76+ bug_task, bug_task_before_modification,
77+ ['assignee'], user=self.user))
78+
79+ expected_activity = {
80+ 'person': self.user,
81+ 'whatchanged': '%s: assignee' % bug_task.bugtargetname,
82+ 'oldvalue': old_assignee.unique_displayname,
83+ 'newvalue': None,
84+ 'message': None,
85+ }
86+
87+ expected_notification = {
88+ 'text': (
89+ u'** Changed in: %s\n'
90+ u' Assignee: %s => (unassigned)' % (
91+ bug_task.bugtargetname,
92+ old_assignee.unique_displayname)),
93+ 'person': self.user,
94+ 'recipients': expected_recipients,
95+ }
96+
97+ self.assertRecordedChange(
98+ expected_activity=expected_activity,
99+ expected_notification=expected_notification,
100+ bug=bug_task.bug)
101+
102 def test_unassign_bugtask(self):
103- # Unassigning a bug task assigned to someone adds entries to the
104+ # Test that unassigning a public bug task adds entries to the
105 # bug activity and notifications sets.
106 old_assignee = self.factory.makePerson()
107 self.bug_task.transitionToAssignee(old_assignee)
108 self.saveOldChanges()
109-
110- bug_task_before_modification = Snapshot(
111- self.bug_task, providing=providedBy(self.bug_task))
112-
113- self.bug_task.transitionToAssignee(None)
114-
115- notify(ObjectModifiedEvent(
116- self.bug_task, bug_task_before_modification,
117- ['assignee'], user=self.user))
118-
119- expected_activity = {
120- 'person': self.user,
121- 'whatchanged': '%s: assignee' % self.bug_task.bugtargetname,
122- 'oldvalue': old_assignee.unique_displayname,
123- 'newvalue': None,
124- 'message': None,
125- }
126-
127 # The old assignee got notified about the change, in addition
128 # to the default recipients.
129 expected_recipients = [
130 self.user, self.product_metadata_subscriber, old_assignee]
131-
132- expected_notification = {
133- 'text': (
134- u'** Changed in: %s\n'
135- u' Assignee: %s => (unassigned)' % (
136- self.bug_task.bugtargetname,
137- old_assignee.unique_displayname)),
138- 'person': self.user,
139- 'recipients': expected_recipients,
140- }
141-
142- self.assertRecordedChange(
143- expected_activity=expected_activity,
144- expected_notification=expected_notification)
145+ self._test_unassign_bugtask(self.bug_task, expected_recipients)
146+
147+ def test_unassign_private_bugtask(self):
148+ # Test that unassigning a private bug task adds entries to the
149+ # bug activity and notifications sets. This test creates a private bug
150+ # that the user can only see because they are assigned to it. The user
151+ # then unassigns themselves.
152+
153+ # Create the private bug.
154+ bug = self.factory.makeBug(
155+ product=self.product, owner=self.user, private=True)
156+ bug_task = bug.bugtasks[0]
157+ # Create a test assignee.
158+ old_assignee = self.factory.makePerson()
159+ # As the bug owner, assign the test assignee..
160+ with person_logged_in(self.user):
161+ bug_task.transitionToAssignee(old_assignee)
162+ self.saveOldChanges(bug=bug)
163+
164+ # Only the bug owner will get notified about the change.
165+ expected_recipients = [self.user]
166+ with person_logged_in(old_assignee):
167+ self._test_unassign_bugtask(bug_task, expected_recipients)
168
169 def test_target_bugtask_to_milestone(self):
170 # When a bugtask is targetted to a milestone BugActivity and
171
172=== modified file 'lib/lp/bugs/tests/test_bugvisibility.py'
173--- lib/lp/bugs/tests/test_bugvisibility.py 2011-01-06 20:59:52 +0000
174+++ lib/lp/bugs/tests/test_bugvisibility.py 2011-03-23 03:23:40 +0000
175@@ -5,7 +5,6 @@
176
177 from canonical.testing.layers import LaunchpadFunctionalLayer
178 from lp.testing import (
179- celebrity_logged_in,
180 person_logged_in,
181 TestCaseWithFactory,
182 )