Launchpad itself

Merge lp:~bac/launchpad/bug-5927 into lp:launchpad

  1. bug-5927
  2. Merge into devel
Proposed by Brad Crittenden
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
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+45403@code.launchpad.net

Commit message

[r=allenap][ui=none][bug=5927] Allow bugtask assignees to see private bugs.

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/bugs/tests/test_bugvisibility.py
  lib/lp/bugs/model/bug.py
  lib/lp/registry/tests/test_product.py

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

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 ?

review: Approve
Revision history for this message
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.

Revision history for this message
Gavin Panella (allenap) wrote :
Download full text (4.3 KiB)

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/bugs/model/bug.py:

- FROM Bug, BugSubscription, TeamParticipation
- WHERE Bug.id = BugSubscription.bug AND
- TeamParticipation.person = %(personid)s AND
- BugSubscription.person = TeamParticipation.team))
+ FROM Bug, BugSubscription, BugTask, TeamParticipation
+ WHERE (Bug.id = BugSubscription.bug AND
+ TeamParticipation.person = %(personid)s AND
+ BugSubscription.person = TeamParticipation.team)
+ OR (
+ BugTask.bug = Bug.id AND
+ TeamParticipation.person = %(personid)s AND
+ TeamParticipation.team = BugTask.assignee)
+ ))

The new join to BugTask combined with the first of the ORed clauses:

    Bug.id = BugSubscription.bug AND
    TeamParticipation.person = %(personid)s AND
    BugSubscription.person = TeamParticipation.team

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
           TeamParticipation.person = %(personid)s AND
           BugSubscription.person = TeamParticipation.team)
    UNION
    SELECT Bug.id
      FROM Bug, BugTask, TeamParticipation
     WHERE BugTask.bug = Bug.id AND
           TeamParticipation.person = %(personid)s AND
           TeamParticipation.team = BugTask.assignee

In addition, the join to Bug could be removed from both queries:

    SELECT BugSubscription.bug
      FROM BugSubscription, TeamParticipation
     WHERE TeamParticipation.person = %(personid)s AND
           BugSubscription.person = TeamParticipation.team
    UNION
    SELECT BugTask.bug
      FROM BugTask, TeamParticipation
     WHERE TeamParticipation.person = %(personid)s AND
           TeamParticipation.team = BugTask.assignee

[7]

In lib/lp/bugs/model/bugtask.py:

- FROM BugSubscription, TeamParticipation
- WHERE TeamParticipation.person = %(personid)s AND
+ FROM BugSubscription, TeamParticipation, BugTask
+ WHERE (TeamParticipation.person = %(personid)s AND
                    BugSubscription.person = TeamParticipation.team AND
- BugSubscription.bug = Bug.id))
+ BugSubscription.bug = Bug.id) OR
+ (BugTask.bug = Bug.id AND
+ TeamParticipation.person = %(personid)s AND
+ TeamParticipation.team = BugTask.assignee)
+ ))

Similar problem to [6].

[8]

+ def cache_assignees(rows):
+ # 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...

Read more...

review: Needs Fixing
Revision history for this message
Brad Crittenden (bac) wrote :

Thanks for the suggestions Gavin. I've incorporated them all. Have another look?

Revision history for this message
Gavin Panella (allenap) wrote :

Cool :) +1

A couple more really really minor things.

[12]

+ def _get_bug_tasks(self):
+ 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.assignee.name for bugtask in bugtasks]

Strictly this is a list of assignee names.

This appears twice.

review: Approve
Revision history for this message
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.

Revision history for this message
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

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2010-11-11 11:55:53 +0000
+++ lib/lp/bugs/configure.zcml 2011-01-14 11:09:14 +0000
@@ -26,6 +26,11 @@
26 <lp:help-folder26 <lp:help-folder
27 folder="help" type="lp.bugs.publisher.BugsLayer" />27 folder="help" type="lp.bugs.publisher.BugsLayer" />
2828
29 <class class="lp.bugs.model.bugtask.BugTaskResultSet">
30 <allow interface="storm.zope.interfaces.IResultSet" />
31 <allow attributes="__getslice__" />
32 </class>
33
29 <class34 <class
30 class="lp.bugs.model.bugactivity.BugActivity">35 class="lp.bugs.model.bugactivity.BugActivity">
31 <allow36 <allow
3237
=== modified file 'lib/lp/bugs/doc/bug.txt'
--- lib/lp/bugs/doc/bug.txt 2010-10-18 22:24:59 +0000
+++ lib/lp/bugs/doc/bug.txt 2011-01-14 11:09:14 +0000
@@ -228,7 +228,8 @@
228subscribed to the bug can see it. Launchpad admins can always view and228subscribed to the bug can see it. Launchpad admins can always view and
229modify private bugs.229modify private bugs.
230230
231=== Marking Bugs Private ===231Marking Bugs Private
232....................
232233
233For the purposes of demonstration, we'll make the firefox crashing bug234For the purposes of demonstration, we'll make the firefox crashing bug
234private. A bug cannot be made private by an anonymous user.235private. A bug cannot be made private by an anonymous user.
@@ -298,7 +299,8 @@
298 >>> firefox_crashes.setPrivate(True, current_user())299 >>> firefox_crashes.setPrivate(True, current_user())
299 False300 False
300301
301=== How Privacy Affects Access to a Bug ===302How Privacy Affects Access to a Bug
303...................................
302304
303Once a bug is made private, it can only be accessed by the users that305Once a bug is made private, it can only be accessed by the users that
304are directly subscribed to the bug and Launchpad admins.306are directly subscribed to the bug and Launchpad admins.
@@ -323,6 +325,7 @@
323 ... user=current_user()))325 ... user=current_user()))
324 ... return sorted(all_bugs - found_bugs)326 ... return sorted(all_bugs - found_bugs)
325327
328 >>> login("test@canonical.com")
326 >>> hidden_bugs()329 >>> hidden_bugs()
327 [14]330 [14]
328331
@@ -344,6 +347,16 @@
344 >>> hidden_bugs()347 >>> hidden_bugs()
345 []348 []
346349
350If a bug is private, bugtask assignees can see the bug. Previously
351sample person could not see bug 14. But after making him the
352assignee it is visible.
353
354 >>> bug14 = bugset.get(14)
355 >>> bug14.default_bugtask.transitionToAssignee(sample_person)
356 >>> login("test@canonical.com")
357 >>> hidden_bugs()
358 []
359
347As one would expect, the permissions are team aware. So, let's retrieve a bug360As one would expect, the permissions are team aware. So, let's retrieve a bug
348and set it private (as Foo Bar again who, of course, is an admin.)361and set it private (as Foo Bar again who, of course, is an admin.)
349362
@@ -409,20 +422,9 @@
409 >>> hidden_bugs()422 >>> hidden_bugs()
410 [2, 6, 14]423 [2, 6, 14]
411424
412IBug.userCanView() offers a quick means by which to verify that a425
413given user can see a given bug.426Filing Public vs. Private Bugs
414427..............................
415 >>> no_priv = personset.getByEmail('no-priv@canonical.com')
416 >>> bug_6 = getUtility(IBugSet).get(6)
417 >>> bug_6.userCanView(no_priv)
418 False
419
420 >>> foobar = personset.getByEmail('foo.bar@canonical.com')
421 >>> bug_6.userCanView(foobar)
422 True
423
424
425=== Filing Public vs. Private Bugs ===
426428
427Let's log back in as Foo Bar to continue our examples:429Let's log back in as Foo Bar to continue our examples:
428430
@@ -430,6 +432,7 @@
430432
431When a public bug is filed:433When a public bug is filed:
432434
435 >>> foobar = personset.getByEmail('foo.bar@canonical.com')
433 >>> params = CreateBugParams(436 >>> params = CreateBugParams(
434 ... title="test firefox bug", comment="blah blah blah", owner=foobar)437 ... title="test firefox bug", comment="blah blah blah", owner=foobar)
435 >>> params.setBugTarget(product=firefox)438 >>> params.setBugTarget(product=firefox)
@@ -1122,7 +1125,8 @@
11221125
1123 >>> chunks = bug_two.getMessageChunks()1126 >>> chunks = bug_two.getMessageChunks()
1124 >>> for chunk in sorted(chunks, key=lambda x:x.id):1127 >>> for chunk in sorted(chunks, key=lambda x:x.id):
1125 ... chunk.id, chunk.message.id, chunk.message.owner.id, chunk.content[:30]1128 ... (chunk.id, chunk.message.id, chunk.message.owner.id,
1129 ... chunk.content[:30])
1126 (4, 1, 16, u'Problem exists between chair a')1130 (4, 1, 16, u'Problem exists between chair a')
1127 (7, 5, 12, u'This would be a real killer fe')1131 (7, 5, 12, u'This would be a real killer fe')
1128 (8, 6, 12, u'Oddly enough the bug system se')1132 (8, 6, 12, u'Oddly enough the bug system se')
@@ -1131,7 +1135,7 @@
1131information, too:1135information, too:
11321136
1133XXX: bug=https://bugs.launchpad.net/storm/+bug/619017 means that this sometimes1137XXX: bug=https://bugs.launchpad.net/storm/+bug/619017 means that this sometimes
1134does 3 queries, depending on the precise state of the storm cache. To avoid 1138does 3 queries, depending on the precise state of the storm cache. To avoid
1135spurious failures it has been changed to tolerate this additional query.1139spurious failures it has been changed to tolerate this additional query.
11361140
1137 >>> len(CursorWrapper.last_executed_sql) - queries <= 31141 >>> len(CursorWrapper.last_executed_sql) - queries <= 3
@@ -1276,7 +1280,8 @@
1276If there is another dup of the master bug, filed by someone else, the1280If there is another dup of the master bug, filed by someone else, the
1277master bug's affected count with dups increases.1281master bug's affected count with dups increases.
12781282
1279 >>> dupe_affected_other_user = factory.makePerson(name='napoleon-bonaparte')1283 >>> dupe_affected_other_user = factory.makePerson(
1284 ... name='napoleon-bonaparte')
1280 >>> dupe_three = factory.makeBug(owner=dupe_affected_other_user)1285 >>> dupe_three = factory.makeBug(owner=dupe_affected_other_user)
1281 >>> dupe_three.markAsDuplicate(test_bug)1286 >>> dupe_three.markAsDuplicate(test_bug)
1282 >>> test_bug.users_affected_count_with_dupes1287 >>> test_bug.users_affected_count_with_dupes
@@ -1425,6 +1430,7 @@
1425 >>> new_bug_2.setPrivate(True, foobar)1430 >>> new_bug_2.setPrivate(True, foobar)
1426 True1431 True
14271432
1433 >>> no_priv = personset.getByEmail('no-priv@canonical.com')
1428 >>> matching_bugs = getUtility(IBugSet).getDistinctBugsForBugTasks(1434 >>> matching_bugs = getUtility(IBugSet).getDistinctBugsForBugTasks(
1429 ... bug_tasks, user=no_priv)1435 ... bug_tasks, user=no_priv)
14301436
14311437
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2010-12-24 11:02:19 +0000
+++ lib/lp/bugs/model/bug.py 2011-01-14 11:09:14 +0000
@@ -1,4 +1,4 @@
1# Copyright 2009 Canonical Ltd. This software is licensed under the1# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4# pylint: disable-msg=E0611,W02124# pylint: disable-msg=E0611,W0212
@@ -1773,17 +1773,24 @@
1773 def _known_viewers(self):1773 def _known_viewers(self):
1774 """A set of known persons able to view this bug.1774 """A set of known persons able to view this bug.
17751775
1776 Seed it by including the list of all owners of bugtasks for the bug.1776 Seed it by including the list of all owners of pillars for bugtasks
1777 for the bug.
1777 """1778 """
1778 bugtask_owners = [bt.pillar.owner.id for bt in self.bugtasks]1779 pillar_owners = [bt.pillar.owner.id for bt in self.bugtasks]
1779 return set(bugtask_owners)1780 return set(pillar_owners)
17801781
1781 def userCanView(self, user):1782 def userCanView(self, user):
1782 """See `IBug`.1783 """See `IBug`.
17831784
1784 Note that Editing is also controlled by this check,1785 Note that Editing is also controlled by this check,
1785 because we permit editing of any bug one can see.1786 because we permit editing of any bug one can see.
1787
1788 If bug privacy rights are changed here, corresponding changes need
1789 to be made to the queries which screen for privacy. See
1790 Bug.searchAsUser and BugTask.get_bug_privacy_filter_with_decorator.
1786 """1791 """
1792 assert user is not None, "User may not be None"
1793
1787 if user.id in self._known_viewers:1794 if user.id in self._known_viewers:
1788 return True1795 return True
1789 if not self.private:1796 if not self.private:
@@ -1793,7 +1800,15 @@
1793 # Admins can view all bugs.1800 # Admins can view all bugs.
1794 return True1801 return True
1795 else:1802 else:
1796 # This is a private bug. Only explicit subscribers may view it.1803 # At this point we know the bug is private and the user is
1804 # unprivileged.
1805
1806 # Assignees to bugtasks can see the private bug.
1807 for bugtask in self.bugtasks:
1808 if user.inTeam(bugtask.assignee):
1809 self._known_viewers.add(user.id)
1810 return True
1811 # Explicit subscribers may also view it.
1797 for subscription in self.subscriptions:1812 for subscription in self.subscriptions:
1798 if user.inTeam(subscription.person):1813 if user.inTeam(subscription.person):
1799 self._known_viewers.add(user.id)1814 self._known_viewers.add(user.id)
@@ -2185,13 +2200,23 @@
2185 # allowed to see.2200 # allowed to see.
2186 where_clauses.append("""2201 where_clauses.append("""
2187 (Bug.private = FALSE OR2202 (Bug.private = FALSE OR
2188 Bug.id in (2203 Bug.id in (
2189 SELECT Bug.id2204 -- Users who have a subscription to this bug.
2190 FROM Bug, BugSubscription, TeamParticipation2205 SELECT BugSubscription.bug
2191 WHERE Bug.id = BugSubscription.bug AND2206 FROM BugSubscription, TeamParticipation
2192 TeamParticipation.person = %(personid)s AND2207 WHERE
2193 BugSubscription.person = TeamParticipation.team))2208 TeamParticipation.person = %(personid)s AND
2194 """ % sqlvalues(personid=user.id))2209 BugSubscription.person = TeamParticipation.team
2210 UNION
2211 -- Users who are the assignee for one of the bug's
2212 -- bugtasks.
2213 SELECT BugTask.bug
2214 FROM BugTask, TeamParticipation
2215 WHERE
2216 TeamParticipation.person = %(personid)s AND
2217 TeamParticipation.team = BugTask.assignee
2218 )
2219 )""" % sqlvalues(personid=user.id))
2195 else:2220 else:
2196 # Anonymous user; filter to include only public bugs in2221 # Anonymous user; filter to include only public bugs in
2197 # the search results.2222 # the search results.
21982223
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-01-14 00:35:43 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-01-14 11:09:14 +0000
@@ -1,4 +1,4 @@
1# Copyright 2009-2010 Canonical Ltd. This software is licensed under the1# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4# pylint: disable-msg=E0611,W02124# pylint: disable-msg=E0611,W0212
@@ -9,6 +9,7 @@
99
10__all__ = [10__all__ = [
11 'BugTaskDelta',11 'BugTaskDelta',
12 'BugTaskResultSet',
12 'BugTaskToBugAdapter',13 'BugTaskToBugAdapter',
13 'BugTaskMixin',14 'BugTaskMixin',
14 'BugTask',15 'BugTask',
@@ -424,6 +425,21 @@
424 self.bug.id, self.bugtargetdisplayname, self.bug.title)425 self.bug.id, self.bugtargetdisplayname, self.bug.title)
425426
426427
428class BugTaskResultSet(DecoratedResultSet):
429 """Decorated results with cached assignees."""
430
431 def __iter__(self, *args, **kwargs):
432 """Iter with caching of assignees.
433
434 Assumes none of the decorators will need to access the assignees or
435 there is not benefit.
436 """
437 bugtasks = list(
438 super(BugTaskResultSet, self).__iter__(*args, **kwargs))
439 BugTaskSet._cache_assignees(bugtasks)
440 return iter(bugtasks)
441
442
427def BugTaskToBugAdapter(bugtask):443def BugTaskToBugAdapter(bugtask):
428 """Adapt an IBugTask to an IBug."""444 """Adapt an IBugTask to an IBug."""
429 return bugtask.bug445 return bugtask.bug
@@ -1058,12 +1074,20 @@
1058 # The assignee is being cleared, so clear the date_assigned1074 # The assignee is being cleared, so clear the date_assigned
1059 # value.1075 # value.
1060 self.date_assigned = None1076 self.date_assigned = None
1077 # The bugtask is unassigned, so clear the _known_viewer cached
1078 # property for the bug.
1079 get_property_cache(self.bug)._known_viewers = set()
1061 if not self.assignee and assignee:1080 if not self.assignee and assignee:
1062 # The task is going from not having an assignee to having1081 # The task is going from not having an assignee to having
1063 # one, so record when this happened1082 # one, so record when this happened
1064 self.date_assigned = now1083 self.date_assigned = now
10651084
1066 self.assignee = assignee1085 self.assignee = assignee
1086 # Invalidate the old visibility cache for this bug and replace it with
1087 # the new assignee.
1088 if self.assignee is not None:
1089 get_property_cache(self.bug)._known_viewers = set(
1090 [self.assignee.id])
10671091
1068 def transitionToTarget(self, target):1092 def transitionToTarget(self, target):
1069 """See `IBugTask`.1093 """See `IBugTask`.
@@ -1347,8 +1371,15 @@
1347 SELECT BugSubscription.bug1371 SELECT BugSubscription.bug
1348 FROM BugSubscription, TeamParticipation1372 FROM BugSubscription, TeamParticipation
1349 WHERE TeamParticipation.person = %(personid)s AND1373 WHERE TeamParticipation.person = %(personid)s AND
1350 BugSubscription.person = TeamParticipation.team AND1374 TeamParticipation.team = BugSubscription.person AND
1351 BugSubscription.bug = Bug.id))1375 BugSubscription.bug = Bug.id
1376 UNION
1377 SELECT BugTask.bug
1378 FROM BugTask, TeamParticipation
1379 WHERE TeamParticipation.person = %(personid)s AND
1380 TeamParticipation.team = BugTask.assignee AND
1381 BugTask.bug = Bug.id
1382 ))
1352 """ % sqlvalues(personid=user.id),1383 """ % sqlvalues(personid=user.id),
1353 _make_cache_user_can_view_bug(user))1384 _make_cache_user_can_view_bug(user))
13541385
@@ -2305,7 +2336,7 @@
2305 origin.append(table)2336 origin.append(table)
2306 return origin2337 return origin
23072338
2308 def _search(self, resultrow, prejoins, params, *args, **kw):2339 def _search(self, resultrow, prejoins, params, *args):
2309 """Return a Storm result set for the given search parameters.2340 """Return a Storm result set for the given search parameters.
23102341
2311 :param resultrow: The type of data returned by the query.2342 :param resultrow: The type of data returned by the query.
@@ -2314,6 +2345,7 @@
2314 :param params: A BugTaskSearchParams instance.2345 :param params: A BugTaskSearchParams instance.
2315 :param args: optional additional BugTaskSearchParams instances,2346 :param args: optional additional BugTaskSearchParams instances,
2316 """2347 """
2348
2317 store = IStore(BugTask)2349 store = IStore(BugTask)
2318 [query, clauseTables, orderby, bugtask_decorator, join_tables,2350 [query, clauseTables, orderby, bugtask_decorator, join_tables,
2319 has_duplicate_results] = self.buildQuery(params)2351 has_duplicate_results] = self.buildQuery(params)
@@ -2372,7 +2404,8 @@
23722404
2373 result = store.using(*origin).find(resultrow)2405 result = store.using(*origin).find(resultrow)
2374 result.order_by(orderby)2406 result.order_by(orderby)
2375 return DecoratedResultSet(result, result_decorator=decorator)2407 return BugTaskResultSet(
2408 result, result_decorator=decorator)
23762409
2377 def search(self, params, *args, **kwargs):2410 def search(self, params, *args, **kwargs):
2378 """See `IBugTaskSet`.2411 """See `IBugTaskSet`.
@@ -2412,6 +2445,15 @@
2412 """See `IBugTaskSet`."""2445 """See `IBugTaskSet`."""
2413 return self._search(BugTask.bugID, [], params).result_set2446 return self._search(BugTask.bugID, [], params).result_set
24142447
2448 @staticmethod
2449 def _cache_assignees(rows):
2450 assignee_ids = set(
2451 bug_task.assigneeID for bug_task in rows)
2452 assignees = getUtility(IPersonSet).getPrecachedPersonsFromIDs(
2453 assignee_ids, need_validity=True)
2454 # Execute query to load storm cache.
2455 list(assignees)
2456
2415 def getPrecachedNonConjoinedBugTasks(self, user, milestone):2457 def getPrecachedNonConjoinedBugTasks(self, user, milestone):
2416 """See `IBugTaskSet`."""2458 """See `IBugTaskSet`."""
2417 params = BugTaskSearchParams(2459 params = BugTaskSearchParams(
@@ -2420,16 +2462,8 @@
2420 omit_dupes=True, exclude_conjoined_tasks=True)2462 omit_dupes=True, exclude_conjoined_tasks=True)
2421 non_conjoined_slaves = self.search(params)2463 non_conjoined_slaves = self.search(params)
24222464
2423 def cache_people(rows):
2424 assignee_ids = set(
2425 bug_task.assigneeID for bug_task in rows)
2426 assignees = getUtility(IPersonSet).getPrecachedPersonsFromIDs(
2427 assignee_ids, need_validity=True)
2428 # Execute query to load storm cache.
2429 list(assignees)
2430
2431 return DecoratedResultSet(2465 return DecoratedResultSet(
2432 non_conjoined_slaves, pre_iter_hook=cache_people)2466 non_conjoined_slaves, pre_iter_hook=BugTaskSet._cache_assignees)
24332467
2434 def createTask(self, bug, owner, product=None, productseries=None,2468 def createTask(self, bug, owner, product=None, productseries=None,
2435 distribution=None, distroseries=None,2469 distribution=None, distroseries=None,
@@ -2581,7 +2615,7 @@
2581 bug_privacy_filter = "AND " + bug_privacy_filter2615 bug_privacy_filter = "AND " + bug_privacy_filter
2582 unconfirmed_bug_condition = self._getUnconfirmedBugCondition()2616 unconfirmed_bug_condition = self._getUnconfirmedBugCondition()
2583 (target_join, target_clause) = self._getTargetJoinAndClause(target)2617 (target_join, target_clause) = self._getTargetJoinAndClause(target)
2584 expirable_bugtasks = BugTask.select("""2618 query = """
2585 BugTask.bug = Bug.id2619 BugTask.bug = Bug.id
2586 AND BugTask.id IN (2620 AND BugTask.id IN (
2587 SELECT BugTask.id2621 SELECT BugTask.id
@@ -2600,13 +2634,13 @@
2600 AND Bug.date_last_updated < CURRENT_TIMESTAMP2634 AND Bug.date_last_updated < CURRENT_TIMESTAMP
2601 AT TIME ZONE 'UTC' - interval '%s days'2635 AT TIME ZONE 'UTC' - interval '%s days'
2602 AND BugWatch.id IS NULL2636 AND BugWatch.id IS NULL
2603 )""" % sqlvalues(BugTaskStatus.INCOMPLETE, min_days_old) +2637 )""" % sqlvalues(BugTaskStatus.INCOMPLETE, min_days_old)
2604 unconfirmed_bug_condition,2638 expirable_bugtasks = BugTask.select(
2639 query + unconfirmed_bug_condition,
2605 clauseTables=['Bug'],2640 clauseTables=['Bug'],
2606 orderBy='Bug.date_last_updated')2641 orderBy='Bug.date_last_updated')
2607 if limit is not None:2642 if limit is not None:
2608 expirable_bugtasks = expirable_bugtasks.limit(limit)2643 expirable_bugtasks = expirable_bugtasks.limit(limit)
2609
2610 return expirable_bugtasks2644 return expirable_bugtasks
26112645
2612 def _getUnconfirmedBugCondition(self):2646 def _getUnconfirmedBugCondition(self):
26132647
=== modified file 'lib/lp/bugs/stories/bugtask-searches/xx-searching-by-tags.txt'
--- lib/lp/bugs/stories/bugtask-searches/xx-searching-by-tags.txt 2010-10-18 22:24:59 +0000
+++ lib/lp/bugs/stories/bugtask-searches/xx-searching-by-tags.txt 2011-01-14 11:09:14 +0000
@@ -46,7 +46,7 @@
46 Summary Importance Status Heat46 Summary Importance Status Heat
47 16 test bug a Undecided New47 16 test bug a Undecided New
48 17 test bug b Undecided New48 17 test bug b Undecided New
49 49
50Same works for user related bugs:50Same works for user related bugs:
5151
52 >>> anon_browser.open('http://launchpad.dev/~name16/+bugs?advanced=1')52 >>> anon_browser.open('http://launchpad.dev/~name16/+bugs?advanced=1')
5353
=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py 2010-12-24 15:32:32 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py 2011-01-14 11:09:14 +0000
@@ -1,4 +1,4 @@
1# Copyright 2010 Canonical Ltd. This software is licensed under the1# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4__metaclass__ = type4__metaclass__ = type
@@ -9,7 +9,11 @@
9 )9 )
10from new import classobj10from new import classobj
11import sys11import sys
12from testtools.matchers import Equals12from testtools.matchers import (
13 Equals,
14 LessThan,
15 Not,
16 )
13import unittest17import unittest
1418
15import pytz19import pytz
@@ -32,7 +36,7 @@
32 BugTaskStatus,36 BugTaskStatus,
33 IBugTaskSet,37 IBugTaskSet,
34 )38 )
35from lp.bugs.model.bugtask import BugTask39from lp.bugs.model.bugtask import BugTask, BugTaskResultSet
36from lp.registry.interfaces.distribution import IDistribution40from lp.registry.interfaces.distribution import IDistribution
37from lp.registry.interfaces.distributionsourcepackage import (41from lp.registry.interfaces.distributionsourcepackage import (
38 IDistributionSourcePackage,42 IDistributionSourcePackage,
@@ -87,17 +91,25 @@
87 # If the user is subscribed to the bug, it is included in the91 # If the user is subscribed to the bug, it is included in the
88 # search result.92 # search result.
89 user = self.factory.makePerson()93 user = self.factory.makePerson()
90 with person_logged_in(self.owner):94 admin = getUtility(IPersonSet).getByEmail('foo.bar@canonical.com')
95 with person_logged_in(admin):
91 bug = self.bugtasks[-1].bug96 bug = self.bugtasks[-1].bug
92 bug.subscribe(user, self.owner)97 bug.subscribe(user, self.owner)
93 params = self.getBugTaskSearchParams(user=user)98 params = self.getBugTaskSearchParams(user=user)
94 self.assertSearchFinds(params, self.bugtasks)99 self.assertSearchFinds(params, self.bugtasks)
95100
96 # Private bugs are included in search results for admins.101 # Private bugs are included in search results for admins.
97 admin = getUtility(IPersonSet).getByEmail('foo.bar@canonical.com')
98 params = self.getBugTaskSearchParams(user=admin)102 params = self.getBugTaskSearchParams(user=admin)
99 self.assertSearchFinds(params, self.bugtasks)103 self.assertSearchFinds(params, self.bugtasks)
100104
105 # Private bugs are included in search results for the assignee.
106 user = self.factory.makePerson()
107 bugtask = self.bugtasks[-1]
108 with person_logged_in(admin):
109 bugtask.transitionToAssignee(user)
110 params = self.getBugTaskSearchParams(user=user)
111 self.assertSearchFinds(params, self.bugtasks)
112
101 def test_search_by_bug_reporter(self):113 def test_search_by_bug_reporter(self):
102 # Search results can be limited to bugs filed by a given person.114 # Search results can be limited to bugs filed by a given person.
103 bugtask = self.bugtasks[0]115 bugtask = self.bugtasks[0]
@@ -976,6 +988,65 @@
976 return [bugtask.bug.id for bugtask in expected_bugtasks]988 return [bugtask.bug.id for bugtask in expected_bugtasks]
977989
978990
991class TestCachingAssignees(TestCaseWithFactory):
992 """Searching bug tasks should pre-cache the bugtask assignees."""
993
994 layer = LaunchpadFunctionalLayer
995
996 def setUp(self):
997 super(TestCachingAssignees, self).setUp()
998 self.owner = self.factory.makePerson(name="bug-owner")
999 with person_logged_in(self.owner):
1000 # Create some bugs with assigned bugtasks.
1001 self.bug = self.factory.makeBug(
1002 owner=self.owner)
1003 self.bug.default_bugtask.transitionToAssignee(
1004 self.factory.makePerson())
1005
1006 for i in xrange(9):
1007 bugtask = self.factory.makeBugTask(
1008 bug=self.bug)
1009 bugtask.transitionToAssignee(
1010 self.factory.makePerson())
1011 self.bug.setPrivate(True, self.owner)
1012
1013 def _get_bug_tasks(self):
1014 """Get the bugtasks for a bug.
1015
1016 This method is used rather than Bug.bugtasks since the later does
1017 prejoining which would spoil the test.
1018 """
1019 store = Store.of(self.bug)
1020 return store.find(
1021 BugTask, BugTask.bug == self.bug)
1022
1023 def test_no_precaching(self):
1024 bugtasks = self._get_bug_tasks()
1025 Store.of(self.bug).invalidate()
1026 with person_logged_in(self.owner):
1027 with StormStatementRecorder() as recorder:
1028 # Access the assignees to trigger a query.
1029 names = [bugtask.assignee.name for bugtask in bugtasks]
1030 # With no caching, the number of queries is roughly twice the
1031 # number of bugtasks.
1032 query_count_floor = len(names) * 2
1033 self.assertThat(
1034 recorder, HasQueryCount(Not(LessThan(query_count_floor))))
1035
1036 def test_precaching(self):
1037 bugtasks = self._get_bug_tasks()
1038 Store.of(self.bug).invalidate()
1039 with person_logged_in(self.owner):
1040 with StormStatementRecorder() as recorder:
1041 bugtasks = BugTaskResultSet(bugtasks)
1042 # Access the assignees to trigger a query if not properly
1043 # cached.
1044 names = [bugtask.assignee.name for bugtask in bugtasks]
1045 # With caching the number of queries is two, one for the
1046 # bugtask and one for all of the assignees at once.
1047 self.assertThat(recorder, HasQueryCount(Equals(2)))
1048
1049
979def test_suite():1050def test_suite():
980 module = sys.modules[__name__]1051 module = sys.modules[__name__]
981 for bug_target_search_type_class in (1052 for bug_target_search_type_class in (
9821053
=== added file 'lib/lp/bugs/tests/test_bugvisibility.py'
--- lib/lp/bugs/tests/test_bugvisibility.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/tests/test_bugvisibility.py 2011-01-14 11:09:14 +0000
@@ -0,0 +1,81 @@
1# Copyright 2011 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Tests for visibility of a bug."""
5
6from canonical.testing.layers import LaunchpadFunctionalLayer
7from lp.testing import (
8 celebrity_logged_in,
9 person_logged_in,
10 TestCaseWithFactory,
11 )
12
13
14class TestPublicBugVisibility(TestCaseWithFactory):
15 """Test visibility for a public bug."""
16
17 layer = LaunchpadFunctionalLayer
18
19 def setUp(self):
20 super(TestPublicBugVisibility, self).setUp()
21 owner = self.factory.makePerson(name="bugowner")
22 self.bug = self.factory.makeBug(owner=owner)
23
24 def test_publicBugAnonUser(self):
25 # userCanView does not get called for anonymous users.
26 self.assertRaises(AssertionError, self.bug.userCanView, None)
27
28 def test_publicBugRegularUser(self):
29 # A regular (non-privileged) user can view a public bug.
30 user = self.factory.makePerson()
31 self.assertTrue(self.bug.userCanView(user))
32
33
34class TestPrivateBugVisibility(TestCaseWithFactory):
35 """Test visibility for a private bug."""
36
37 layer = LaunchpadFunctionalLayer
38
39 def setUp(self):
40 super(TestPrivateBugVisibility, self).setUp()
41 self.owner = self.factory.makePerson(name="bugowner")
42 self.product_owner = self.factory.makePerson(name="productowner")
43 self.product = self.factory.makeProduct(
44 name="regular-product", owner=self.product_owner)
45 self.bug_team = self.factory.makeTeam(
46 name="bugteam", owner=self.product.owner)
47 self.bug_team_member = self.factory.makePerson(name="bugteammember")
48 with person_logged_in(self.product.owner):
49 self.bug_team.addMember(self.bug_team_member, self.product.owner)
50 self.product.setBugSupervisor(
51 bug_supervisor=self.bug_team,
52 user=self.product.owner)
53 self.bug = self.factory.makeBug(
54 owner=self.owner, private=True, product=self.product)
55
56 def test_privateBugRegularUser(self):
57 # A regular (non-privileged) user can not view a private bug.
58 user = self.factory.makePerson()
59 self.assertFalse(self.bug.userCanView(user))
60
61 def test_privateBugOwner(self):
62 # The bug submitter may view a private bug.
63 self.assertTrue(self.bug.userCanView(self.owner))
64
65 def test_privateBugSupervisor(self):
66 # A member of the bug supervisor team can not see a private bug.
67 self.assertFalse(self.bug.userCanView(self.bug_team_member))
68
69 def test_privateBugSubscriber(self):
70 # A person subscribed to a private bug can see it.
71 user = self.factory.makePerson()
72 with person_logged_in(self.owner):
73 self.bug.subscribe(user, self.owner)
74 self.assertTrue(self.bug.userCanView(user))
75
76 def test_privateBugAssignee(self):
77 # The bug assignee can see the private bug.
78 bug_assignee = self.factory.makePerson(name="bugassignee")
79 with person_logged_in(self.product.owner):
80 self.bug.default_bugtask.transitionToAssignee(bug_assignee)
81 self.assertTrue(self.bug.userCanView(bug_assignee))
082
=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py 2011-01-12 21:42:48 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py 2011-01-14 11:09:14 +0000
@@ -6,7 +6,10 @@
6import transaction6import transaction
7from storm.expr import LeftJoin7from storm.expr import LeftJoin
8from storm.store import Store8from storm.store import Store
9from testtools.matchers import Equals9from testtools.matchers import (
10 Equals,
11 LessThan,
12 )
10from zope.component import getUtility13from zope.component import getUtility
1114
12from canonical.config import config15from canonical.config import config
@@ -858,7 +861,7 @@
858 prejoins=[(Person, LeftJoin(Person, BugTask.owner==Person.id))]861 prejoins=[(Person, LeftJoin(Person, BugTask.owner==Person.id))]
859 bugtasks = view.searchUnbatched(prejoins=prejoins)862 bugtasks = view.searchUnbatched(prejoins=prejoins)
860 [bugtask.owner for bugtask in bugtasks]863 [bugtask.owner for bugtask in bugtasks]
861 self.assertThat(recorder, HasQueryCount(Equals(1)))864 self.assertThat(recorder, HasQueryCount(LessThan(3)))
862865
863 def test_getMilestoneWidgetValues(self):866 def test_getMilestoneWidgetValues(self):
864 view = create_initialized_view(self.person, self.view_name)867 view = create_initialized_view(self.person, self.view_name)
@@ -876,7 +879,7 @@
876 Store.of(milestones[0]).invalidate()879 Store.of(milestones[0]).invalidate()
877 with StormStatementRecorder() as recorder:880 with StormStatementRecorder() as recorder:
878 self.assertEqual(expected, view.getMilestoneWidgetValues())881 self.assertEqual(expected, view.getMilestoneWidgetValues())
879 self.assertThat(recorder, HasQueryCount(Equals(1)))882 self.assertThat(recorder, HasQueryCount(LessThan(3)))
880883
881884
882class TestPersonRelatedBugTaskSearchListingView(885class TestPersonRelatedBugTaskSearchListingView(
883886
=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py 2010-12-09 07:14:34 +0000
+++ lib/lp/registry/tests/test_product.py 2011-01-14 11:09:14 +0000
@@ -1,4 +1,4 @@
1# Copyright 2009-2010 Canonical Ltd. This software is licensed under the1# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4__metaclass__ = type4__metaclass__ = type
@@ -10,7 +10,6 @@
10from lazr.lifecycle.snapshot import Snapshot10from lazr.lifecycle.snapshot import Snapshot
11import pytz11import pytz
12import transaction12import transaction
13from zope.component import getUtility
1413
15from canonical.launchpad.ftests import syncUpdate14from canonical.launchpad.ftests import syncUpdate
16from canonical.launchpad.testing.pages import (15from canonical.launchpad.testing.pages import (
@@ -22,7 +21,6 @@
22 DatabaseFunctionalLayer,21 DatabaseFunctionalLayer,
23 LaunchpadFunctionalLayer,22 LaunchpadFunctionalLayer,
24 )23 )
25from lp.registry.interfaces.person import IPersonSet
26from lp.registry.interfaces.product import (24from lp.registry.interfaces.product import (
27 IProduct,25 IProduct,
28 License,26 License,
@@ -345,9 +343,8 @@
345 def testPersonCanSetSelfAsSupervisor(self):343 def testPersonCanSetSelfAsSupervisor(self):
346 # A person can set themselves as bug supervisor for a product.344 # A person can set themselves as bug supervisor for a product.
347 # This is a regression test for bug 438985.345 # This is a regression test for bug 438985.
348 user = getUtility(IPersonSet).getByName(self.person.name)
349 self.product.setBugSupervisor(346 self.product.setBugSupervisor(
350 bug_supervisor=self.person, user=user)347 bug_supervisor=self.person, user=self.person)
351348
352 self.assertEqual(349 self.assertEqual(
353 self.product.bug_supervisor, self.person,350 self.product.bug_supervisor, self.person,