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
=== 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-23 03:23:40 +0000
@@ -780,6 +780,9 @@
780 """See `IBug`."""780 """See `IBug`."""
781 # Drop cached subscription info.781 # Drop cached subscription info.
782 clear_property_cache(self)782 clear_property_cache(self)
783 # Ensure the unsubscriber is in the _known_viewer cache for the bug so
784 # that the permissions are such that the operation can succeed.
785 get_property_cache(self)._known_viewers = set([unsubscribed_by.id])
783 if person is None:786 if person is None:
784 person = unsubscribed_by787 person = unsubscribed_by
785788
786789
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-03-20 22:39:45 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-03-23 03:23:40 +0000
@@ -1015,8 +1015,11 @@
1015 # value.1015 # value.
1016 self.date_assigned = None1016 self.date_assigned = None
1017 # The bugtask is unassigned, so clear the _known_viewer cached1017 # The bugtask is unassigned, so clear the _known_viewer cached
1018 # property for the bug.1018 # property for the bug. Retain the current assignee as a viewer so
1019 get_property_cache(self.bug)._known_viewers = set()1019 # that they are able to unassign themselves and get confirmation
1020 # that that worked.
1021 get_property_cache(self.bug)._known_viewers = set(
1022 [self.assignee.id])
1020 if not self.assignee and assignee:1023 if not self.assignee and assignee:
1021 # The task is going from not having an assignee to having1024 # The task is going from not having an assignee to having
1022 # one, so record when this happened1025 # one, so record when this happened
10231026
=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py 2011-02-22 10:44:48 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py 2011-03-23 03:23:40 +0000
@@ -217,6 +217,22 @@
217 person=self.user)217 person=self.user)
218 self.assertRecordedChange(expected_activity=unsubscribe_activity)218 self.assertRecordedChange(expected_activity=unsubscribe_activity)
219219
220 def test_unsubscribe_private_bug(self):
221 # Test that a person can unsubscribe themselves from a private bug
222 # that they are not assigned to.
223 subscriber = self.factory.makePerson(displayname='Mom')
224 # Create the private bug.
225 bug = self.factory.makeBug(
226 product=self.product, owner=self.user, private=True)
227 bug.subscribe(subscriber, self.user)
228 self.saveOldChanges(bug=bug)
229 bug.unsubscribe(subscriber, subscriber)
230 unsubscribe_activity = dict(
231 whatchanged=u'removed subscriber Mom',
232 person=subscriber)
233 self.assertRecordedChange(
234 expected_activity=unsubscribe_activity, bug=bug)
235
220 def test_title_changed(self):236 def test_title_changed(self):
221 # Changing the title of a Bug adds items to the activity log and237 # Changing the title of a Bug adds items to the activity log and
222 # the Bug's notifications.238 # the Bug's notifications.
@@ -1131,48 +1147,78 @@
1131 expected_activity=expected_activity,1147 expected_activity=expected_activity,
1132 expected_notification=expected_notification)1148 expected_notification=expected_notification)
11331149
1150 def _test_unassign_bugtask(self, bug_task, expected_recipients):
1151 # A helper method used by tests for unassigning public and private bug
1152 # tasks.
1153 # Unassigning a bug task assigned to someone adds entries to the
1154 # bug activity and notifications sets.
1155
1156 old_assignee = bug_task.assignee
1157 bug_task_before_modification = Snapshot(
1158 bug_task, providing=providedBy(bug_task))
1159
1160 bug_task.transitionToAssignee(None)
1161
1162 notify(ObjectModifiedEvent(
1163 bug_task, bug_task_before_modification,
1164 ['assignee'], user=self.user))
1165
1166 expected_activity = {
1167 'person': self.user,
1168 'whatchanged': '%s: assignee' % bug_task.bugtargetname,
1169 'oldvalue': old_assignee.unique_displayname,
1170 'newvalue': None,
1171 'message': None,
1172 }
1173
1174 expected_notification = {
1175 'text': (
1176 u'** Changed in: %s\n'
1177 u' Assignee: %s => (unassigned)' % (
1178 bug_task.bugtargetname,
1179 old_assignee.unique_displayname)),
1180 'person': self.user,
1181 'recipients': expected_recipients,
1182 }
1183
1184 self.assertRecordedChange(
1185 expected_activity=expected_activity,
1186 expected_notification=expected_notification,
1187 bug=bug_task.bug)
1188
1134 def test_unassign_bugtask(self):1189 def test_unassign_bugtask(self):
1135 # Unassigning a bug task assigned to someone adds entries to the1190 # Test that unassigning a public bug task adds entries to the
1136 # bug activity and notifications sets.1191 # bug activity and notifications sets.
1137 old_assignee = self.factory.makePerson()1192 old_assignee = self.factory.makePerson()
1138 self.bug_task.transitionToAssignee(old_assignee)1193 self.bug_task.transitionToAssignee(old_assignee)
1139 self.saveOldChanges()1194 self.saveOldChanges()
1140
1141 bug_task_before_modification = Snapshot(
1142 self.bug_task, providing=providedBy(self.bug_task))
1143
1144 self.bug_task.transitionToAssignee(None)
1145
1146 notify(ObjectModifiedEvent(
1147 self.bug_task, bug_task_before_modification,
1148 ['assignee'], user=self.user))
1149
1150 expected_activity = {
1151 'person': self.user,
1152 'whatchanged': '%s: assignee' % self.bug_task.bugtargetname,
1153 'oldvalue': old_assignee.unique_displayname,
1154 'newvalue': None,
1155 'message': None,
1156 }
1157
1158 # The old assignee got notified about the change, in addition1195 # The old assignee got notified about the change, in addition
1159 # to the default recipients.1196 # to the default recipients.
1160 expected_recipients = [1197 expected_recipients = [
1161 self.user, self.product_metadata_subscriber, old_assignee]1198 self.user, self.product_metadata_subscriber, old_assignee]
11621199 self._test_unassign_bugtask(self.bug_task, expected_recipients)
1163 expected_notification = {1200
1164 'text': (1201 def test_unassign_private_bugtask(self):
1165 u'** Changed in: %s\n'1202 # Test that unassigning a private bug task adds entries to the
1166 u' Assignee: %s => (unassigned)' % (1203 # bug activity and notifications sets. This test creates a private bug
1167 self.bug_task.bugtargetname,1204 # that the user can only see because they are assigned to it. The user
1168 old_assignee.unique_displayname)),1205 # then unassigns themselves.
1169 'person': self.user,1206
1170 'recipients': expected_recipients,1207 # Create the private bug.
1171 }1208 bug = self.factory.makeBug(
11721209 product=self.product, owner=self.user, private=True)
1173 self.assertRecordedChange(1210 bug_task = bug.bugtasks[0]
1174 expected_activity=expected_activity,1211 # Create a test assignee.
1175 expected_notification=expected_notification)1212 old_assignee = self.factory.makePerson()
1213 # As the bug owner, assign the test assignee..
1214 with person_logged_in(self.user):
1215 bug_task.transitionToAssignee(old_assignee)
1216 self.saveOldChanges(bug=bug)
1217
1218 # Only the bug owner will get notified about the change.
1219 expected_recipients = [self.user]
1220 with person_logged_in(old_assignee):
1221 self._test_unassign_bugtask(bug_task, expected_recipients)
11761222
1177 def test_target_bugtask_to_milestone(self):1223 def test_target_bugtask_to_milestone(self):
1178 # When a bugtask is targetted to a milestone BugActivity and1224 # When a bugtask is targetted to a milestone BugActivity and
11791225
=== 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-23 03:23:40 +0000
@@ -5,7 +5,6 @@
55
6from canonical.testing.layers import LaunchpadFunctionalLayer6from canonical.testing.layers import LaunchpadFunctionalLayer
7from lp.testing import (7from lp.testing import (
8 celebrity_logged_in,
9 person_logged_in,8 person_logged_in,
10 TestCaseWithFactory,9 TestCaseWithFactory,
11 )10 )