launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06695
[Merge] lp:~deryck/launchpad/buglistings-preload-people-901122 into lp:launchpad
Deryck Hodge has proposed merging lp:~deryck/launchpad/buglistings-preload-people-901122 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~deryck/launchpad/buglistings-preload-people-901122/+merge/97041
This branch updates IBugTaskSet to provide a new method for getting people related to a set of bugtasks. This is useful in the new buglistings work so that we can show assignee and bug reporter without doing a lot more queries.
The bulk of the work is to add this helper method that gets all related people in one query. Then the call sites that deal with bugtask people are updated to make use of this. A few tests also had to be updated.
I did not do a pre-imp call, since this work mirrors the earlier pre-loading work I did. The same pattern is followed here as when I loaded tags, which followed the pattern used for loading bug badge properties.
I did do a bit more of a refactor to the test helper method make_bug_task_listing_item, since we now needed to be conscious of both tags and people. Rather than passing in those attributes at call sites, you can now pass in the bugtask and the method uses the provided methods on IBugTaskSet to get at the related data. This cleans up the tests better and also ensures the methods on BugTaskSet are indirectly tested.
I also chose to update the milestone browser code to follow the bugtask browser code, which means milestone pages now issue two more queries than before. However, this brings the two spots together and we could now add in data about assignee, bug reporter, or tags in milestone pages, without timing out the pages.
We are lint free here, too.
--
https://code.launchpad.net/~deryck/launchpad/buglistings-preload-people-901122/+merge/97041
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/buglistings-preload-people-901122 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2012-02-29 19:44:28 +0000
+++ lib/lp/bugs/browser/bugtask.py 2012-03-14 17:33:28 +0000
@@ -2161,14 +2161,15 @@
delegates(IBugTask, 'bugtask')
def __init__(self, bugtask, has_bug_branch,
- has_specification, has_patch, tags, request=None,
- target_context=None):
+ has_specification, has_patch, tags,
+ people, request=None, target_context=None):
self.bugtask = bugtask
self.review_action_widget = None
self.has_bug_branch = has_bug_branch
self.has_specification = has_specification
self.has_patch = has_patch
self.tags = tags
+ self.people = people
self.request = request
self.target_context = target_context
@@ -2207,8 +2208,9 @@
else:
milestone_name = None
assignee = None
- if self.assignee is not None:
- assignee = self.assignee.displayname
+ if self.assigneeID is not None:
+ assignee = self.people[self.assigneeID].displayname
+ reporter = self.people[self.bug.ownerID]
base_tag_url = "%s/?field.tag=" % canonical_url(
self.bugtask.target,
@@ -2227,7 +2229,7 @@
'importance_class': 'importance' + self.importance.name,
'last_updated': last_updated,
'milestone_name': milestone_name,
- 'reporter': self.bug.owner.displayname,
+ 'reporter': reporter.displayname,
'status': self.status.title,
'status_class': 'status' + self.status.name,
'tags': [{'url': base_tag_url + urllib.quote(tag), 'tag': tag}
@@ -2281,6 +2283,11 @@
"""Return a dict matching bugtask to it's tags."""
return getUtility(IBugTaskSet).getBugTaskTags(self.currentBatch())
+ @cachedproperty
+ def bugtask_people(self):
+ """Return mapping of people related to this bugtask set."""
+ return getUtility(IBugTaskSet).getBugTaskPeople(self.currentBatch())
+
def getCookieName(self):
"""Return the cookie name used in bug listings js code."""
cookie_name_template = '%s-buglist-fields'
@@ -2331,6 +2338,7 @@
badge_property['has_specification'],
badge_property['has_patch'],
tags,
+ self.bugtask_people,
request=self.request,
target_context=target_context)
=== modified file 'lib/lp/bugs/browser/cvereport.py'
--- lib/lp/bugs/browser/cvereport.py 2012-02-29 17:03:40 +0000
+++ lib/lp/bugs/browser/cvereport.py 2012-03-14 17:33:28 +0000
@@ -78,8 +78,9 @@
self.resolved_cve_bugtasks = []
return
- badge_properties = getUtility(IBugTaskSet).getBugTaskBadgeProperties(
- bugtasks)
+ bugtask_set = getUtility(IBugTaskSet)
+ badge_properties = bugtask_set.getBugTaskBadgeProperties(bugtasks)
+ people = bugtask_set.getBugTaskPeople(bugtasks)
open_bugtaskcves = {}
resolved_bugtaskcves = {}
@@ -92,7 +93,8 @@
has_bug_branch=badges['has_branch'],
has_specification=badges['has_specification'],
has_patch=badges['has_patch'],
- tags=())
+ tags=(),
+ people=people)
if bugtask.status in RESOLVED_BUGTASK_STATUSES:
current_bugtaskcves = resolved_bugtaskcves
else:
=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py 2012-03-08 11:51:36 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py 2012-03-14 17:33:28 +0000
@@ -1793,22 +1793,30 @@
batched_view.activity_and_comments)
-def make_bug_task_listing_item(factory, tags=()):
- owner = factory.makePerson()
- bug = factory.makeBug(
- owner=owner, private=True, security_related=True)
- with person_logged_in(owner):
- bugtask = bug.default_bugtask
+def make_bug_task_listing_item(factory, bugtask=None):
+ if bugtask is None:
+ owner = factory.makePerson()
+ bug = factory.makeBug(
+ owner=owner, private=True, security_related=True)
+ with person_logged_in(owner):
+ bugtask = bug.default_bugtask
+ else:
+ owner = bugtask.bug.owner
bug_task_set = getUtility(IBugTaskSet)
bug_badge_properties = bug_task_set.getBugTaskBadgeProperties(
[bugtask])
badge_property = bug_badge_properties[bugtask]
+ tags = bug_task_set.getBugTaskTags([bugtask])
+ if tags != {}:
+ tags = tags[bugtask.id]
+ people = bug_task_set.getBugTaskPeople([bugtask])
return owner, BugTaskListingItem(
bugtask,
badge_property['has_branch'],
badge_property['has_specification'],
badge_property['has_patch'],
tags,
+ people,
target_context=bugtask.target)
@@ -2351,11 +2359,14 @@
def test_model_assignee(self):
"""Model contains expected fields with expected values."""
- owner, item = make_bug_task_listing_item(self.factory)
+ assignee = self.factory.makePerson(displayname='Example Person')
+ bug = self.factory.makeBug()
+ with person_logged_in(bug.owner):
+ removeSecurityProxy(bug).default_bugtask.transitionToAssignee(
+ assignee)
+ owner, item = make_bug_task_listing_item(
+ self.factory, bugtask=bug.default_bugtask)
with person_logged_in(owner):
- self.assertIs(None, item.model['assignee'])
- assignee = self.factory.makePerson(displayname='Example Person')
- item.bugtask.transitionToAssignee(assignee)
self.assertEqual('Example Person', item.model['assignee'])
def test_model_age(self):
@@ -2368,8 +2379,11 @@
def test_model_tags(self):
"""Model contains bug tags."""
+ bug = self.factory.makeBug()
tags = ['tag1', 'tag2']
- owner, item = make_bug_task_listing_item(self.factory, tags=tags)
+ removeSecurityProxy(bug).tags = tags
+ owner, item = make_bug_task_listing_item(
+ self.factory, bug.default_bugtask)
with person_logged_in(owner):
self.assertEqual(2, len(item.model['tags']))
self.assertTrue('tag' in item.model['tags'][0].keys())
=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py 2012-03-01 02:12:35 +0000
+++ lib/lp/bugs/interfaces/bugtask.py 2012-03-14 17:33:28 +0000
@@ -1481,6 +1481,12 @@
Return a dict mapping from bugtask to tag.
"""
+ def getBugTaskPeople(bugtasks):
+ """Return a set of people related to bugtasks.
+
+ Return a dict mapping from Person.id to Person.
+ """
+
def getBugTaskBadgeProperties(bugtasks):
"""Return whether the bugtasks should have badges.
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2012-02-28 18:54:08 +0000
+++ lib/lp/bugs/model/bugtask.py 2012-03-14 17:33:28 +0000
@@ -43,8 +43,10 @@
And,
Join,
Or,
+ Select,
SQL,
Sum,
+ Union,
)
from storm.store import (
EmptyResultSet,
@@ -1398,6 +1400,30 @@
tags_by_bugtask[bugtask_id].append(tag_name)
return dict(tags_by_bugtask)
+ def getBugTaskPeople(self, bugtasks):
+ """See `IBugTaskSet`"""
+ # Import locally to avoid circular imports.
+ from lp.registry.model.person import Person
+ from lp.bugs.model.bug import Bug
+ from lp.bugs.model.bugtask import BugTask
+ bugtask_ids = set(bugtask.id for bugtask in bugtasks)
+ people_ids = Union(
+ Select(
+ Person.id,
+ And(Bug.owner == Person.id,
+ BugTask.bug == Bug.id,
+ BugTask.id.is_in(bugtask_ids))),
+ Select(
+ Person.id,
+ And(BugTask.assignee == Person.id,
+ BugTask.id.is_in(bugtask_ids))),
+ distinct=True)
+ people = IStore(Person).find(
+ Person,
+ Person.id.is_in(people_ids))
+ return dict(
+ (person.id, person) for person in people)
+
def getBugTaskBadgeProperties(self, bugtasks):
"""See `IBugTaskSet`."""
# Import locally to avoid circular imports.
=== modified file 'lib/lp/registry/browser/milestone.py'
--- lib/lp/registry/browser/milestone.py 2012-02-29 17:03:40 +0000
+++ lib/lp/registry/browser/milestone.py 2012-03-14 17:33:28 +0000
@@ -251,16 +251,27 @@
return getUtility(IBugTaskSet).getBugTaskBadgeProperties(
self._bugtasks)
+ @cachedproperty
+ def _bug_task_tags(self):
+ return getUtility(IBugTaskSet).getBugTaskTags(self._bugtasks)
+
+ @cachedproperty
+ def _bug_task_people(self):
+ """The people associated with a set of bug tasks."""
+ return getUtility(IBugTaskSet).getBugTaskPeople(self._bugtasks)
+
def _getListingItem(self, bugtask):
"""Return a decorated bugtask for the bug listing."""
badge_property = self._bug_badge_properties[bugtask]
- tags = ()
+ tags = self._bug_task_tags.get(bugtask.id, ())
+ people = self._bug_task_people
return BugTaskListingItem(
bugtask,
badge_property['has_branch'],
badge_property['has_specification'],
badge_property['has_patch'],
- tags)
+ tags,
+ people)
@cachedproperty
def bugtasks(self):
=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py 2012-01-18 12:04:16 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py 2012-03-14 17:33:28 +0000
@@ -288,9 +288,11 @@
# 4. Load links to specifications.
# 5. Load links to branches.
# 6. Loads milestones
+ # 7. Loads tags
+ # 8. All related people
bugtask_count = 10
self.assert_bugtasks_query_count(
- self.milestone, bugtask_count, query_limit=7)
+ self.milestone, bugtask_count, query_limit=9)
def test_milestone_eager_loading(self):
# Verify that the number of queries does not increase with more
@@ -403,16 +405,18 @@
logout()
def test_bugtasks_queries(self):
- # The view.bugtasks attribute will make five queries:
+ # The view.bugtasks attribute will make several queries:
# 1. For each project in the group load all the dev focus series ids.
# 2. Load bugtasks and bugs.
# 3. Load assignees (Person, Account, and EmailAddress).
# 4. Load links to specifications.
# 5. Load links to branches.
# 6. Loads milestones.
+ # 7. Loads tags.
+ # 8. All related people.
bugtask_count = 10
self.assert_bugtasks_query_count(
- self.milestone, bugtask_count, query_limit=7)
+ self.milestone, bugtask_count, query_limit=9)
def test_milestone_eager_loading(self):
# Verify that the number of queries does not increase with more
Follow ups
