Merge lp:~bac/launchpad/bug-5927 into lp:launchpad
- bug-5927
- Merge into devel
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Gavin Panella | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 12218 | ||||
Proposed branch: | lp:~bac/launchpad/bug-5927 | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
681 lines (+285/-63) 9 files modified
lib/lp/bugs/configure.zcml (+5/-0) lib/lp/bugs/doc/bug.txt (+25/-19) lib/lp/bugs/model/bug.py (+37/-12) lib/lp/bugs/model/bugtask.py (+52/-18) lib/lp/bugs/stories/bugtask-searches/xx-searching-by-tags.txt (+1/-1) lib/lp/bugs/tests/test_bugtask_search.py (+76/-5) lib/lp/bugs/tests/test_bugvisibility.py (+81/-0) lib/lp/registry/browser/tests/test_person_view.py (+6/-3) lib/lp/registry/tests/test_product.py (+2/-5) |
||||
To merge this branch: | bzr merge lp:~bac/launchpad/bug-5927 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gavin Panella (community) | Approve | ||
Review via email: mp+45403@code.launchpad.net |
Commit message
[r=allenap]
Description of the change
= Summary =
Bug assignees should be allowed to see private bugs.
== Proposed fix ==
Since assignees already get bugnotifications, all that is required is to
change userCanView to explicitly allow bugtask assignees to view the
bug. When the assignee is changed there is no residual subscription,
which would have been a problem if the assignee were given visibility
through the subscription model as is done for the bug supervisor.
== Pre-implementation notes ==
Talks with Curtis.
== Implementation details ==
As above.
== Tests ==
bin/test -vvm lp.bugs -t test_bugvisibility -t bugnotification
== Demo and Q/A ==
Mark a bug with no assignee private. Ensure user 'bac' cannot see the
bug. Assign bac to the bug. Ensure bac can now see the bug. Unassign
bac. Ensure bac can no longer see the bug. Repeat.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/
lib/lp/
lib/lp/
Robert Collins (lifeless) wrote : | # |
This needs eager loading in bug queries or we'll throw performance
out; we also need to change bug searches to consider assignee as
granting accessability to bugs.
So this is good, but incomplete and landing it is likely to cause a
performance regression on bugs with many bug tasks.
Gavin Panella (allenap) wrote : | # |
Hi Brad,
The search methods for bugs/bugtasks are horrible to work on, and
eager loading doesn't make it any prettier... you got the short straw
on this one :-/
I found a few issues, one fairly major, but none too hard to fix.
Gavin.
[6]
In lib/lp/
- FROM Bug, BugSubscription, TeamParticipation
- WHERE Bug.id = BugSubscription.bug AND
- TeamParticipati
- BugSubscription
+ FROM Bug, BugSubscription, BugTask, TeamParticipation
+ WHERE (Bug.id = BugSubscription.bug AND
+ TeamParticipati
+ BugSubscription
+ OR (
+ BugTask.bug = Bug.id AND
+ TeamParticipati
+ TeamParticipati
+ ))
The new join to BugTask combined with the first of the ORed clauses:
Bug.id = BugSubscription.bug AND
TeamPartici
BugSubscrip
results in a cartestian product against BugTask.
I think this could be expressed as a UNION:
SELECT Bug.id
FROM Bug, BugSubscription, TeamParticipation
WHERE Bug.id = BugSubscription.bug AND
UNION
SELECT Bug.id
FROM Bug, BugTask, TeamParticipation
WHERE BugTask.bug = Bug.id AND
In addition, the join to Bug could be removed from both queries:
SELECT BugSubscription.bug
FROM BugSubscription, TeamParticipation
WHERE TeamParticipati
UNION
SELECT BugTask.bug
FROM BugTask, TeamParticipation
WHERE TeamParticipati
[7]
In lib/lp/
- FROM BugSubscription, TeamParticipation
- WHERE TeamParticipati
+ FROM BugSubscription, TeamParticipation, BugTask
+ WHERE (TeamParticipat
- BugSubscription.bug = Bug.id))
+ BugSubscription.bug = Bug.id) OR
+ (BugTask.bug = Bug.id AND
+ TeamParticipati
+ TeamParticipati
+ ))
Similar problem to [6].
[8]
+ def cache_assignees
+ # The decorator must be called to turn the rows of the result into
+ # a list of BugTasks, which can be used for caching the assigne...
Brad Crittenden (bac) wrote : | # |
Thanks for the suggestions Gavin. I've incorporated them all. Have another look?
Gavin Panella (allenap) wrote : | # |
Cool :) +1
A couple more really really minor things.
[12]
+ def _get_bug_
+ store = Store.of(self.bug)
+ return store.find(
+ BugTask, BugTask.bug == self.bug)
Perhaps a docstring to make it obvious what this is for?
[13]
+ bugtasks = [bugtask.
Strictly this is a list of assignee names.
This appears twice.
Robert Collins (lifeless) wrote : | # |
Something you might be interested in - John Meinel did some profiling
of generator vs lists in constructors and short lived lists are
sometimes faster :)... perf is tricky.
Robert Collins (lifeless) wrote : | # |
Following up on recent search performance, I noticed a couple of odd things introduced in this branch. Firstly, the specialised BugTaskResultSet was unnecessary - the addition of assignee to the permission lookups on BugTask searching has an immediate effect of precaching assignee *access* to the bug; none of our bug views show the assignee at the moment, so precaching actual assignee objects is a pessimism. As such I'm rolling that part of the patch backout.
The other odd thing was that I saw the initial patch *had* a pre_iter_hook parameter, which is actually more general and more compact than creating a class : as we get more complex query interactions being able to to compose and reuse eager loading requirements will become more important: I don't think we want a proliferation of result set /types/ to achieve that: simple functions are going to be generally more powerful.
Preview Diff
1 | === modified file 'lib/lp/bugs/configure.zcml' |
2 | --- lib/lp/bugs/configure.zcml 2010-11-11 11:55:53 +0000 |
3 | +++ lib/lp/bugs/configure.zcml 2011-01-14 11:09:14 +0000 |
4 | @@ -26,6 +26,11 @@ |
5 | <lp:help-folder |
6 | folder="help" type="lp.bugs.publisher.BugsLayer" /> |
7 | |
8 | + <class class="lp.bugs.model.bugtask.BugTaskResultSet"> |
9 | + <allow interface="storm.zope.interfaces.IResultSet" /> |
10 | + <allow attributes="__getslice__" /> |
11 | + </class> |
12 | + |
13 | <class |
14 | class="lp.bugs.model.bugactivity.BugActivity"> |
15 | <allow |
16 | |
17 | === modified file 'lib/lp/bugs/doc/bug.txt' |
18 | --- lib/lp/bugs/doc/bug.txt 2010-10-18 22:24:59 +0000 |
19 | +++ lib/lp/bugs/doc/bug.txt 2011-01-14 11:09:14 +0000 |
20 | @@ -228,7 +228,8 @@ |
21 | subscribed to the bug can see it. Launchpad admins can always view and |
22 | modify private bugs. |
23 | |
24 | -=== Marking Bugs Private === |
25 | +Marking Bugs Private |
26 | +.................... |
27 | |
28 | For the purposes of demonstration, we'll make the firefox crashing bug |
29 | private. A bug cannot be made private by an anonymous user. |
30 | @@ -298,7 +299,8 @@ |
31 | >>> firefox_crashes.setPrivate(True, current_user()) |
32 | False |
33 | |
34 | -=== How Privacy Affects Access to a Bug === |
35 | +How Privacy Affects Access to a Bug |
36 | +................................... |
37 | |
38 | Once a bug is made private, it can only be accessed by the users that |
39 | are directly subscribed to the bug and Launchpad admins. |
40 | @@ -323,6 +325,7 @@ |
41 | ... user=current_user())) |
42 | ... return sorted(all_bugs - found_bugs) |
43 | |
44 | + >>> login("test@canonical.com") |
45 | >>> hidden_bugs() |
46 | [14] |
47 | |
48 | @@ -344,6 +347,16 @@ |
49 | >>> hidden_bugs() |
50 | [] |
51 | |
52 | +If a bug is private, bugtask assignees can see the bug. Previously |
53 | +sample person could not see bug 14. But after making him the |
54 | +assignee it is visible. |
55 | + |
56 | + >>> bug14 = bugset.get(14) |
57 | + >>> bug14.default_bugtask.transitionToAssignee(sample_person) |
58 | + >>> login("test@canonical.com") |
59 | + >>> hidden_bugs() |
60 | + [] |
61 | + |
62 | As one would expect, the permissions are team aware. So, let's retrieve a bug |
63 | and set it private (as Foo Bar again who, of course, is an admin.) |
64 | |
65 | @@ -409,20 +422,9 @@ |
66 | >>> hidden_bugs() |
67 | [2, 6, 14] |
68 | |
69 | -IBug.userCanView() offers a quick means by which to verify that a |
70 | -given user can see a given bug. |
71 | - |
72 | - >>> no_priv = personset.getByEmail('no-priv@canonical.com') |
73 | - >>> bug_6 = getUtility(IBugSet).get(6) |
74 | - >>> bug_6.userCanView(no_priv) |
75 | - False |
76 | - |
77 | - >>> foobar = personset.getByEmail('foo.bar@canonical.com') |
78 | - >>> bug_6.userCanView(foobar) |
79 | - True |
80 | - |
81 | - |
82 | -=== Filing Public vs. Private Bugs === |
83 | + |
84 | +Filing Public vs. Private Bugs |
85 | +.............................. |
86 | |
87 | Let's log back in as Foo Bar to continue our examples: |
88 | |
89 | @@ -430,6 +432,7 @@ |
90 | |
91 | When a public bug is filed: |
92 | |
93 | + >>> foobar = personset.getByEmail('foo.bar@canonical.com') |
94 | >>> params = CreateBugParams( |
95 | ... title="test firefox bug", comment="blah blah blah", owner=foobar) |
96 | >>> params.setBugTarget(product=firefox) |
97 | @@ -1122,7 +1125,8 @@ |
98 | |
99 | >>> chunks = bug_two.getMessageChunks() |
100 | >>> for chunk in sorted(chunks, key=lambda x:x.id): |
101 | - ... chunk.id, chunk.message.id, chunk.message.owner.id, chunk.content[:30] |
102 | + ... (chunk.id, chunk.message.id, chunk.message.owner.id, |
103 | + ... chunk.content[:30]) |
104 | (4, 1, 16, u'Problem exists between chair a') |
105 | (7, 5, 12, u'This would be a real killer fe') |
106 | (8, 6, 12, u'Oddly enough the bug system se') |
107 | @@ -1131,7 +1135,7 @@ |
108 | information, too: |
109 | |
110 | XXX: bug=https://bugs.launchpad.net/storm/+bug/619017 means that this sometimes |
111 | -does 3 queries, depending on the precise state of the storm cache. To avoid |
112 | +does 3 queries, depending on the precise state of the storm cache. To avoid |
113 | spurious failures it has been changed to tolerate this additional query. |
114 | |
115 | >>> len(CursorWrapper.last_executed_sql) - queries <= 3 |
116 | @@ -1276,7 +1280,8 @@ |
117 | If there is another dup of the master bug, filed by someone else, the |
118 | master bug's affected count with dups increases. |
119 | |
120 | - >>> dupe_affected_other_user = factory.makePerson(name='napoleon-bonaparte') |
121 | + >>> dupe_affected_other_user = factory.makePerson( |
122 | + ... name='napoleon-bonaparte') |
123 | >>> dupe_three = factory.makeBug(owner=dupe_affected_other_user) |
124 | >>> dupe_three.markAsDuplicate(test_bug) |
125 | >>> test_bug.users_affected_count_with_dupes |
126 | @@ -1425,6 +1430,7 @@ |
127 | >>> new_bug_2.setPrivate(True, foobar) |
128 | True |
129 | |
130 | + >>> no_priv = personset.getByEmail('no-priv@canonical.com') |
131 | >>> matching_bugs = getUtility(IBugSet).getDistinctBugsForBugTasks( |
132 | ... bug_tasks, user=no_priv) |
133 | |
134 | |
135 | === modified file 'lib/lp/bugs/model/bug.py' |
136 | --- lib/lp/bugs/model/bug.py 2010-12-24 11:02:19 +0000 |
137 | +++ lib/lp/bugs/model/bug.py 2011-01-14 11:09:14 +0000 |
138 | @@ -1,4 +1,4 @@ |
139 | -# Copyright 2009 Canonical Ltd. This software is licensed under the |
140 | +# Copyright 2009-2011 Canonical Ltd. This software is licensed under the |
141 | # GNU Affero General Public License version 3 (see the file LICENSE). |
142 | |
143 | # pylint: disable-msg=E0611,W0212 |
144 | @@ -1773,17 +1773,24 @@ |
145 | def _known_viewers(self): |
146 | """A set of known persons able to view this bug. |
147 | |
148 | - Seed it by including the list of all owners of bugtasks for the bug. |
149 | + Seed it by including the list of all owners of pillars for bugtasks |
150 | + for the bug. |
151 | """ |
152 | - bugtask_owners = [bt.pillar.owner.id for bt in self.bugtasks] |
153 | - return set(bugtask_owners) |
154 | + pillar_owners = [bt.pillar.owner.id for bt in self.bugtasks] |
155 | + return set(pillar_owners) |
156 | |
157 | def userCanView(self, user): |
158 | """See `IBug`. |
159 | |
160 | Note that Editing is also controlled by this check, |
161 | because we permit editing of any bug one can see. |
162 | + |
163 | + If bug privacy rights are changed here, corresponding changes need |
164 | + to be made to the queries which screen for privacy. See |
165 | + Bug.searchAsUser and BugTask.get_bug_privacy_filter_with_decorator. |
166 | """ |
167 | + assert user is not None, "User may not be None" |
168 | + |
169 | if user.id in self._known_viewers: |
170 | return True |
171 | if not self.private: |
172 | @@ -1793,7 +1800,15 @@ |
173 | # Admins can view all bugs. |
174 | return True |
175 | else: |
176 | - # This is a private bug. Only explicit subscribers may view it. |
177 | + # At this point we know the bug is private and the user is |
178 | + # unprivileged. |
179 | + |
180 | + # Assignees to bugtasks can see the private bug. |
181 | + for bugtask in self.bugtasks: |
182 | + if user.inTeam(bugtask.assignee): |
183 | + self._known_viewers.add(user.id) |
184 | + return True |
185 | + # Explicit subscribers may also view it. |
186 | for subscription in self.subscriptions: |
187 | if user.inTeam(subscription.person): |
188 | self._known_viewers.add(user.id) |
189 | @@ -2185,13 +2200,23 @@ |
190 | # allowed to see. |
191 | where_clauses.append(""" |
192 | (Bug.private = FALSE OR |
193 | - Bug.id in ( |
194 | - SELECT Bug.id |
195 | - FROM Bug, BugSubscription, TeamParticipation |
196 | - WHERE Bug.id = BugSubscription.bug AND |
197 | - TeamParticipation.person = %(personid)s AND |
198 | - BugSubscription.person = TeamParticipation.team)) |
199 | - """ % sqlvalues(personid=user.id)) |
200 | + Bug.id in ( |
201 | + -- Users who have a subscription to this bug. |
202 | + SELECT BugSubscription.bug |
203 | + FROM BugSubscription, TeamParticipation |
204 | + WHERE |
205 | + TeamParticipation.person = %(personid)s AND |
206 | + BugSubscription.person = TeamParticipation.team |
207 | + UNION |
208 | + -- Users who are the assignee for one of the bug's |
209 | + -- bugtasks. |
210 | + SELECT BugTask.bug |
211 | + FROM BugTask, TeamParticipation |
212 | + WHERE |
213 | + TeamParticipation.person = %(personid)s AND |
214 | + TeamParticipation.team = BugTask.assignee |
215 | + ) |
216 | + )""" % sqlvalues(personid=user.id)) |
217 | else: |
218 | # Anonymous user; filter to include only public bugs in |
219 | # the search results. |
220 | |
221 | === modified file 'lib/lp/bugs/model/bugtask.py' |
222 | --- lib/lp/bugs/model/bugtask.py 2011-01-14 00:35:43 +0000 |
223 | +++ lib/lp/bugs/model/bugtask.py 2011-01-14 11:09:14 +0000 |
224 | @@ -1,4 +1,4 @@ |
225 | -# Copyright 2009-2010 Canonical Ltd. This software is licensed under the |
226 | +# Copyright 2009-2011 Canonical Ltd. This software is licensed under the |
227 | # GNU Affero General Public License version 3 (see the file LICENSE). |
228 | |
229 | # pylint: disable-msg=E0611,W0212 |
230 | @@ -9,6 +9,7 @@ |
231 | |
232 | __all__ = [ |
233 | 'BugTaskDelta', |
234 | + 'BugTaskResultSet', |
235 | 'BugTaskToBugAdapter', |
236 | 'BugTaskMixin', |
237 | 'BugTask', |
238 | @@ -424,6 +425,21 @@ |
239 | self.bug.id, self.bugtargetdisplayname, self.bug.title) |
240 | |
241 | |
242 | +class BugTaskResultSet(DecoratedResultSet): |
243 | + """Decorated results with cached assignees.""" |
244 | + |
245 | + def __iter__(self, *args, **kwargs): |
246 | + """Iter with caching of assignees. |
247 | + |
248 | + Assumes none of the decorators will need to access the assignees or |
249 | + there is not benefit. |
250 | + """ |
251 | + bugtasks = list( |
252 | + super(BugTaskResultSet, self).__iter__(*args, **kwargs)) |
253 | + BugTaskSet._cache_assignees(bugtasks) |
254 | + return iter(bugtasks) |
255 | + |
256 | + |
257 | def BugTaskToBugAdapter(bugtask): |
258 | """Adapt an IBugTask to an IBug.""" |
259 | return bugtask.bug |
260 | @@ -1058,12 +1074,20 @@ |
261 | # The assignee is being cleared, so clear the date_assigned |
262 | # value. |
263 | self.date_assigned = None |
264 | + # The bugtask is unassigned, so clear the _known_viewer cached |
265 | + # property for the bug. |
266 | + get_property_cache(self.bug)._known_viewers = set() |
267 | if not self.assignee and assignee: |
268 | # The task is going from not having an assignee to having |
269 | # one, so record when this happened |
270 | self.date_assigned = now |
271 | |
272 | self.assignee = assignee |
273 | + # Invalidate the old visibility cache for this bug and replace it with |
274 | + # the new assignee. |
275 | + if self.assignee is not None: |
276 | + get_property_cache(self.bug)._known_viewers = set( |
277 | + [self.assignee.id]) |
278 | |
279 | def transitionToTarget(self, target): |
280 | """See `IBugTask`. |
281 | @@ -1347,8 +1371,15 @@ |
282 | SELECT BugSubscription.bug |
283 | FROM BugSubscription, TeamParticipation |
284 | WHERE TeamParticipation.person = %(personid)s AND |
285 | - BugSubscription.person = TeamParticipation.team AND |
286 | - BugSubscription.bug = Bug.id)) |
287 | + TeamParticipation.team = BugSubscription.person AND |
288 | + BugSubscription.bug = Bug.id |
289 | + UNION |
290 | + SELECT BugTask.bug |
291 | + FROM BugTask, TeamParticipation |
292 | + WHERE TeamParticipation.person = %(personid)s AND |
293 | + TeamParticipation.team = BugTask.assignee AND |
294 | + BugTask.bug = Bug.id |
295 | + )) |
296 | """ % sqlvalues(personid=user.id), |
297 | _make_cache_user_can_view_bug(user)) |
298 | |
299 | @@ -2305,7 +2336,7 @@ |
300 | origin.append(table) |
301 | return origin |
302 | |
303 | - def _search(self, resultrow, prejoins, params, *args, **kw): |
304 | + def _search(self, resultrow, prejoins, params, *args): |
305 | """Return a Storm result set for the given search parameters. |
306 | |
307 | :param resultrow: The type of data returned by the query. |
308 | @@ -2314,6 +2345,7 @@ |
309 | :param params: A BugTaskSearchParams instance. |
310 | :param args: optional additional BugTaskSearchParams instances, |
311 | """ |
312 | + |
313 | store = IStore(BugTask) |
314 | [query, clauseTables, orderby, bugtask_decorator, join_tables, |
315 | has_duplicate_results] = self.buildQuery(params) |
316 | @@ -2372,7 +2404,8 @@ |
317 | |
318 | result = store.using(*origin).find(resultrow) |
319 | result.order_by(orderby) |
320 | - return DecoratedResultSet(result, result_decorator=decorator) |
321 | + return BugTaskResultSet( |
322 | + result, result_decorator=decorator) |
323 | |
324 | def search(self, params, *args, **kwargs): |
325 | """See `IBugTaskSet`. |
326 | @@ -2412,6 +2445,15 @@ |
327 | """See `IBugTaskSet`.""" |
328 | return self._search(BugTask.bugID, [], params).result_set |
329 | |
330 | + @staticmethod |
331 | + def _cache_assignees(rows): |
332 | + assignee_ids = set( |
333 | + bug_task.assigneeID for bug_task in rows) |
334 | + assignees = getUtility(IPersonSet).getPrecachedPersonsFromIDs( |
335 | + assignee_ids, need_validity=True) |
336 | + # Execute query to load storm cache. |
337 | + list(assignees) |
338 | + |
339 | def getPrecachedNonConjoinedBugTasks(self, user, milestone): |
340 | """See `IBugTaskSet`.""" |
341 | params = BugTaskSearchParams( |
342 | @@ -2420,16 +2462,8 @@ |
343 | omit_dupes=True, exclude_conjoined_tasks=True) |
344 | non_conjoined_slaves = self.search(params) |
345 | |
346 | - def cache_people(rows): |
347 | - assignee_ids = set( |
348 | - bug_task.assigneeID for bug_task in rows) |
349 | - assignees = getUtility(IPersonSet).getPrecachedPersonsFromIDs( |
350 | - assignee_ids, need_validity=True) |
351 | - # Execute query to load storm cache. |
352 | - list(assignees) |
353 | - |
354 | return DecoratedResultSet( |
355 | - non_conjoined_slaves, pre_iter_hook=cache_people) |
356 | + non_conjoined_slaves, pre_iter_hook=BugTaskSet._cache_assignees) |
357 | |
358 | def createTask(self, bug, owner, product=None, productseries=None, |
359 | distribution=None, distroseries=None, |
360 | @@ -2581,7 +2615,7 @@ |
361 | bug_privacy_filter = "AND " + bug_privacy_filter |
362 | unconfirmed_bug_condition = self._getUnconfirmedBugCondition() |
363 | (target_join, target_clause) = self._getTargetJoinAndClause(target) |
364 | - expirable_bugtasks = BugTask.select(""" |
365 | + query = """ |
366 | BugTask.bug = Bug.id |
367 | AND BugTask.id IN ( |
368 | SELECT BugTask.id |
369 | @@ -2600,13 +2634,13 @@ |
370 | AND Bug.date_last_updated < CURRENT_TIMESTAMP |
371 | AT TIME ZONE 'UTC' - interval '%s days' |
372 | AND BugWatch.id IS NULL |
373 | - )""" % sqlvalues(BugTaskStatus.INCOMPLETE, min_days_old) + |
374 | - unconfirmed_bug_condition, |
375 | + )""" % sqlvalues(BugTaskStatus.INCOMPLETE, min_days_old) |
376 | + expirable_bugtasks = BugTask.select( |
377 | + query + unconfirmed_bug_condition, |
378 | clauseTables=['Bug'], |
379 | orderBy='Bug.date_last_updated') |
380 | if limit is not None: |
381 | expirable_bugtasks = expirable_bugtasks.limit(limit) |
382 | - |
383 | return expirable_bugtasks |
384 | |
385 | def _getUnconfirmedBugCondition(self): |
386 | |
387 | === modified file 'lib/lp/bugs/stories/bugtask-searches/xx-searching-by-tags.txt' |
388 | --- lib/lp/bugs/stories/bugtask-searches/xx-searching-by-tags.txt 2010-10-18 22:24:59 +0000 |
389 | +++ lib/lp/bugs/stories/bugtask-searches/xx-searching-by-tags.txt 2011-01-14 11:09:14 +0000 |
390 | @@ -46,7 +46,7 @@ |
391 | Summary Importance Status Heat |
392 | 16 test bug a Undecided New |
393 | 17 test bug b Undecided New |
394 | - |
395 | + |
396 | Same works for user related bugs: |
397 | |
398 | >>> anon_browser.open('http://launchpad.dev/~name16/+bugs?advanced=1') |
399 | |
400 | === modified file 'lib/lp/bugs/tests/test_bugtask_search.py' |
401 | --- lib/lp/bugs/tests/test_bugtask_search.py 2010-12-24 15:32:32 +0000 |
402 | +++ lib/lp/bugs/tests/test_bugtask_search.py 2011-01-14 11:09:14 +0000 |
403 | @@ -1,4 +1,4 @@ |
404 | -# Copyright 2010 Canonical Ltd. This software is licensed under the |
405 | +# Copyright 2010-2011 Canonical Ltd. This software is licensed under the |
406 | # GNU Affero General Public License version 3 (see the file LICENSE). |
407 | |
408 | __metaclass__ = type |
409 | @@ -9,7 +9,11 @@ |
410 | ) |
411 | from new import classobj |
412 | import sys |
413 | -from testtools.matchers import Equals |
414 | +from testtools.matchers import ( |
415 | + Equals, |
416 | + LessThan, |
417 | + Not, |
418 | + ) |
419 | import unittest |
420 | |
421 | import pytz |
422 | @@ -32,7 +36,7 @@ |
423 | BugTaskStatus, |
424 | IBugTaskSet, |
425 | ) |
426 | -from lp.bugs.model.bugtask import BugTask |
427 | +from lp.bugs.model.bugtask import BugTask, BugTaskResultSet |
428 | from lp.registry.interfaces.distribution import IDistribution |
429 | from lp.registry.interfaces.distributionsourcepackage import ( |
430 | IDistributionSourcePackage, |
431 | @@ -87,17 +91,25 @@ |
432 | # If the user is subscribed to the bug, it is included in the |
433 | # search result. |
434 | user = self.factory.makePerson() |
435 | - with person_logged_in(self.owner): |
436 | + admin = getUtility(IPersonSet).getByEmail('foo.bar@canonical.com') |
437 | + with person_logged_in(admin): |
438 | bug = self.bugtasks[-1].bug |
439 | bug.subscribe(user, self.owner) |
440 | params = self.getBugTaskSearchParams(user=user) |
441 | self.assertSearchFinds(params, self.bugtasks) |
442 | |
443 | # Private bugs are included in search results for admins. |
444 | - admin = getUtility(IPersonSet).getByEmail('foo.bar@canonical.com') |
445 | params = self.getBugTaskSearchParams(user=admin) |
446 | self.assertSearchFinds(params, self.bugtasks) |
447 | |
448 | + # Private bugs are included in search results for the assignee. |
449 | + user = self.factory.makePerson() |
450 | + bugtask = self.bugtasks[-1] |
451 | + with person_logged_in(admin): |
452 | + bugtask.transitionToAssignee(user) |
453 | + params = self.getBugTaskSearchParams(user=user) |
454 | + self.assertSearchFinds(params, self.bugtasks) |
455 | + |
456 | def test_search_by_bug_reporter(self): |
457 | # Search results can be limited to bugs filed by a given person. |
458 | bugtask = self.bugtasks[0] |
459 | @@ -976,6 +988,65 @@ |
460 | return [bugtask.bug.id for bugtask in expected_bugtasks] |
461 | |
462 | |
463 | +class TestCachingAssignees(TestCaseWithFactory): |
464 | + """Searching bug tasks should pre-cache the bugtask assignees.""" |
465 | + |
466 | + layer = LaunchpadFunctionalLayer |
467 | + |
468 | + def setUp(self): |
469 | + super(TestCachingAssignees, self).setUp() |
470 | + self.owner = self.factory.makePerson(name="bug-owner") |
471 | + with person_logged_in(self.owner): |
472 | + # Create some bugs with assigned bugtasks. |
473 | + self.bug = self.factory.makeBug( |
474 | + owner=self.owner) |
475 | + self.bug.default_bugtask.transitionToAssignee( |
476 | + self.factory.makePerson()) |
477 | + |
478 | + for i in xrange(9): |
479 | + bugtask = self.factory.makeBugTask( |
480 | + bug=self.bug) |
481 | + bugtask.transitionToAssignee( |
482 | + self.factory.makePerson()) |
483 | + self.bug.setPrivate(True, self.owner) |
484 | + |
485 | + def _get_bug_tasks(self): |
486 | + """Get the bugtasks for a bug. |
487 | + |
488 | + This method is used rather than Bug.bugtasks since the later does |
489 | + prejoining which would spoil the test. |
490 | + """ |
491 | + store = Store.of(self.bug) |
492 | + return store.find( |
493 | + BugTask, BugTask.bug == self.bug) |
494 | + |
495 | + def test_no_precaching(self): |
496 | + bugtasks = self._get_bug_tasks() |
497 | + Store.of(self.bug).invalidate() |
498 | + with person_logged_in(self.owner): |
499 | + with StormStatementRecorder() as recorder: |
500 | + # Access the assignees to trigger a query. |
501 | + names = [bugtask.assignee.name for bugtask in bugtasks] |
502 | + # With no caching, the number of queries is roughly twice the |
503 | + # number of bugtasks. |
504 | + query_count_floor = len(names) * 2 |
505 | + self.assertThat( |
506 | + recorder, HasQueryCount(Not(LessThan(query_count_floor)))) |
507 | + |
508 | + def test_precaching(self): |
509 | + bugtasks = self._get_bug_tasks() |
510 | + Store.of(self.bug).invalidate() |
511 | + with person_logged_in(self.owner): |
512 | + with StormStatementRecorder() as recorder: |
513 | + bugtasks = BugTaskResultSet(bugtasks) |
514 | + # Access the assignees to trigger a query if not properly |
515 | + # cached. |
516 | + names = [bugtask.assignee.name for bugtask in bugtasks] |
517 | + # With caching the number of queries is two, one for the |
518 | + # bugtask and one for all of the assignees at once. |
519 | + self.assertThat(recorder, HasQueryCount(Equals(2))) |
520 | + |
521 | + |
522 | def test_suite(): |
523 | module = sys.modules[__name__] |
524 | for bug_target_search_type_class in ( |
525 | |
526 | === added file 'lib/lp/bugs/tests/test_bugvisibility.py' |
527 | --- lib/lp/bugs/tests/test_bugvisibility.py 1970-01-01 00:00:00 +0000 |
528 | +++ lib/lp/bugs/tests/test_bugvisibility.py 2011-01-14 11:09:14 +0000 |
529 | @@ -0,0 +1,81 @@ |
530 | +# Copyright 2011 Canonical Ltd. This software is licensed under the |
531 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
532 | + |
533 | +"""Tests for visibility of a bug.""" |
534 | + |
535 | +from canonical.testing.layers import LaunchpadFunctionalLayer |
536 | +from lp.testing import ( |
537 | + celebrity_logged_in, |
538 | + person_logged_in, |
539 | + TestCaseWithFactory, |
540 | + ) |
541 | + |
542 | + |
543 | +class TestPublicBugVisibility(TestCaseWithFactory): |
544 | + """Test visibility for a public bug.""" |
545 | + |
546 | + layer = LaunchpadFunctionalLayer |
547 | + |
548 | + def setUp(self): |
549 | + super(TestPublicBugVisibility, self).setUp() |
550 | + owner = self.factory.makePerson(name="bugowner") |
551 | + self.bug = self.factory.makeBug(owner=owner) |
552 | + |
553 | + def test_publicBugAnonUser(self): |
554 | + # userCanView does not get called for anonymous users. |
555 | + self.assertRaises(AssertionError, self.bug.userCanView, None) |
556 | + |
557 | + def test_publicBugRegularUser(self): |
558 | + # A regular (non-privileged) user can view a public bug. |
559 | + user = self.factory.makePerson() |
560 | + self.assertTrue(self.bug.userCanView(user)) |
561 | + |
562 | + |
563 | +class TestPrivateBugVisibility(TestCaseWithFactory): |
564 | + """Test visibility for a private bug.""" |
565 | + |
566 | + layer = LaunchpadFunctionalLayer |
567 | + |
568 | + def setUp(self): |
569 | + super(TestPrivateBugVisibility, self).setUp() |
570 | + self.owner = self.factory.makePerson(name="bugowner") |
571 | + self.product_owner = self.factory.makePerson(name="productowner") |
572 | + self.product = self.factory.makeProduct( |
573 | + name="regular-product", owner=self.product_owner) |
574 | + self.bug_team = self.factory.makeTeam( |
575 | + name="bugteam", owner=self.product.owner) |
576 | + self.bug_team_member = self.factory.makePerson(name="bugteammember") |
577 | + with person_logged_in(self.product.owner): |
578 | + self.bug_team.addMember(self.bug_team_member, self.product.owner) |
579 | + self.product.setBugSupervisor( |
580 | + bug_supervisor=self.bug_team, |
581 | + user=self.product.owner) |
582 | + self.bug = self.factory.makeBug( |
583 | + owner=self.owner, private=True, product=self.product) |
584 | + |
585 | + def test_privateBugRegularUser(self): |
586 | + # A regular (non-privileged) user can not view a private bug. |
587 | + user = self.factory.makePerson() |
588 | + self.assertFalse(self.bug.userCanView(user)) |
589 | + |
590 | + def test_privateBugOwner(self): |
591 | + # The bug submitter may view a private bug. |
592 | + self.assertTrue(self.bug.userCanView(self.owner)) |
593 | + |
594 | + def test_privateBugSupervisor(self): |
595 | + # A member of the bug supervisor team can not see a private bug. |
596 | + self.assertFalse(self.bug.userCanView(self.bug_team_member)) |
597 | + |
598 | + def test_privateBugSubscriber(self): |
599 | + # A person subscribed to a private bug can see it. |
600 | + user = self.factory.makePerson() |
601 | + with person_logged_in(self.owner): |
602 | + self.bug.subscribe(user, self.owner) |
603 | + self.assertTrue(self.bug.userCanView(user)) |
604 | + |
605 | + def test_privateBugAssignee(self): |
606 | + # The bug assignee can see the private bug. |
607 | + bug_assignee = self.factory.makePerson(name="bugassignee") |
608 | + with person_logged_in(self.product.owner): |
609 | + self.bug.default_bugtask.transitionToAssignee(bug_assignee) |
610 | + self.assertTrue(self.bug.userCanView(bug_assignee)) |
611 | |
612 | === modified file 'lib/lp/registry/browser/tests/test_person_view.py' |
613 | --- lib/lp/registry/browser/tests/test_person_view.py 2011-01-12 21:42:48 +0000 |
614 | +++ lib/lp/registry/browser/tests/test_person_view.py 2011-01-14 11:09:14 +0000 |
615 | @@ -6,7 +6,10 @@ |
616 | import transaction |
617 | from storm.expr import LeftJoin |
618 | from storm.store import Store |
619 | -from testtools.matchers import Equals |
620 | +from testtools.matchers import ( |
621 | + Equals, |
622 | + LessThan, |
623 | + ) |
624 | from zope.component import getUtility |
625 | |
626 | from canonical.config import config |
627 | @@ -858,7 +861,7 @@ |
628 | prejoins=[(Person, LeftJoin(Person, BugTask.owner==Person.id))] |
629 | bugtasks = view.searchUnbatched(prejoins=prejoins) |
630 | [bugtask.owner for bugtask in bugtasks] |
631 | - self.assertThat(recorder, HasQueryCount(Equals(1))) |
632 | + self.assertThat(recorder, HasQueryCount(LessThan(3))) |
633 | |
634 | def test_getMilestoneWidgetValues(self): |
635 | view = create_initialized_view(self.person, self.view_name) |
636 | @@ -876,7 +879,7 @@ |
637 | Store.of(milestones[0]).invalidate() |
638 | with StormStatementRecorder() as recorder: |
639 | self.assertEqual(expected, view.getMilestoneWidgetValues()) |
640 | - self.assertThat(recorder, HasQueryCount(Equals(1))) |
641 | + self.assertThat(recorder, HasQueryCount(LessThan(3))) |
642 | |
643 | |
644 | class TestPersonRelatedBugTaskSearchListingView( |
645 | |
646 | === modified file 'lib/lp/registry/tests/test_product.py' |
647 | --- lib/lp/registry/tests/test_product.py 2010-12-09 07:14:34 +0000 |
648 | +++ lib/lp/registry/tests/test_product.py 2011-01-14 11:09:14 +0000 |
649 | @@ -1,4 +1,4 @@ |
650 | -# Copyright 2009-2010 Canonical Ltd. This software is licensed under the |
651 | +# Copyright 2009-2011 Canonical Ltd. This software is licensed under the |
652 | # GNU Affero General Public License version 3 (see the file LICENSE). |
653 | |
654 | __metaclass__ = type |
655 | @@ -10,7 +10,6 @@ |
656 | from lazr.lifecycle.snapshot import Snapshot |
657 | import pytz |
658 | import transaction |
659 | -from zope.component import getUtility |
660 | |
661 | from canonical.launchpad.ftests import syncUpdate |
662 | from canonical.launchpad.testing.pages import ( |
663 | @@ -22,7 +21,6 @@ |
664 | DatabaseFunctionalLayer, |
665 | LaunchpadFunctionalLayer, |
666 | ) |
667 | -from lp.registry.interfaces.person import IPersonSet |
668 | from lp.registry.interfaces.product import ( |
669 | IProduct, |
670 | License, |
671 | @@ -345,9 +343,8 @@ |
672 | def testPersonCanSetSelfAsSupervisor(self): |
673 | # A person can set themselves as bug supervisor for a product. |
674 | # This is a regression test for bug 438985. |
675 | - user = getUtility(IPersonSet).getByName(self.person.name) |
676 | self.product.setBugSupervisor( |
677 | - bug_supervisor=self.person, user=user) |
678 | + bug_supervisor=self.person, user=self.person) |
679 | |
680 | self.assertEqual( |
681 | self.product.bug_supervisor, self.person, |
Nice change. I wonder how we ever managed without this!
[1]
- # This is a private bug. Only explicit subscribers may view it. subscription. person) :
self._known_ viewers. add(user. id)
return True bugtask. assignee) : viewers. add(user. id)
+ # This is a private bug. Explicit subscribers may view it.
for subscription in self.subscriptions:
if user.inTeam(
+ # Assignees to bugtasks can also see the private bug.
+ for bugtask in self.bugtasks:
+ if user.inTeam(
+ self._known_
+ 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_privateBug Owner(self) : (self.bug. userCanView( self.owner) )
+ # A regular (non-privileged) user can not view a private bug.
+ self.assertTrue
I think the comment here is wrong.
[3]
+ def test_privateBug SupervisorPriva teBugsByDefault (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_privateBug Assignee( self): makePerson( name="bugassign ee") logged_ in(self. product. owner): default_ bugtask. transitionToAss ignee(bug_ assignee) (self.bug. userCanView( bug_assignee) )
+ # The bug assignee can see the private bug.
+ bug_assignee = self.factory.
+ with person_
+ self.bug.
+ self.assertTrue
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 bugs/stories/ bug-privacy bugs/stories/ upstream- bugprivacy ?
lib/lp/
lib/lp/