Merge lp:~deryck/launchpad/buglistings-preload-people-901122 into lp:launchpad

[Deryck Hodge]

Add getBugTaskPeople method to IBugTaskSet to help with pre-loading bugtask-related people objects.

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.

[Deryck Hodge (deryck)]

I update the branch to use IPersonSet.getPrecachedPersonsFromIDs instead of a hand crafted query, per Robert's suggestion on IRC. This produces the same query count as my original new query.

FWIW, I mentioned to Robert on IRC that I thought I had seen more queries by listing out ownerID via

[bugtask.bug.ownerID for bugtask in bugtasks]

Turns out I was seeing more queries, but not from that. When you click around a bugtask search results list, we update the URL with query parameters. When you come to the page via these query strings, that view issues a couple more queries than without the query string. But these increased counts are consistent, no matter the data size -- 2 more than without the query string. Given that, I didn't dig too much into that issue, especially since I wanted to finish this off and get it landed.

[Aaron Bentley (abentley)]

Looks good.

review: Approve

[Robert Collins (lifeless)]

I have a small question - it looks like this increases the queries for
milestone views *without* needing too (because the tests were passing
before). That smells like waste - we're doing work the user doesn't
need. Of course, I'm probably wrong, but interesting mind wants to
know :)

=== 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-20 12:58:20 +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-20 12:58:20 +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-20 12:58:20 +0000
@@ -1793,22 +1793,31 @@
             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
+    bugtask = removeSecurityProxy(bugtask)
     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 +2360,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 +2380,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-20 12:58:20 +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-20 12:58:20 +0000
@@ -43,8 +43,10 @@
     And,
     Join,
     Or,
+    Select,
     SQL,
     Sum,
+    Union,
     )
 from storm.store import (
     EmptyResultSet,
@@ -1398,6 +1400,17 @@
             tags_by_bugtask[bugtask_id].append(tag_name)
         return dict(tags_by_bugtask)
 
+    def getBugTaskPeople(self, bugtasks):
+        """See `IBugTaskSet`"""
+        # Avoid circular imports.
+        from lp.registry.interfaces.person import IPersonSet
+        people_ids = set(
+            [bugtask.assigneeID for bugtask in bugtasks] +
+            [bugtask.bug.ownerID for bugtask in bugtasks])
+        people = getUtility(IPersonSet).getPrecachedPersonsFromIDs(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-20 12:58:20 +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-20 12:58:20 +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