launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

Deryck Hodge has proposed merging lp:~deryck/launchpad/preload-tags-for-buglistings-901122 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #901122 in Launchpad itself: "New bug listings need to preload more attributes"
  https://bugs.launchpad.net/launchpad/+bug/901122

For more details, see:
https://code.launchpad.net/~deryck/launchpad/preload-tags-for-buglistings-901122/+merge/95023

This is the first branch of refactorings to load new data that was added in new buglistings without adding more queries.  This first bit here is to get tags loaded as a set for a given buglisting batch, rather than accessing them in a loop for each item in the buglist.

I followed the pattern used for bug_badge_properties, which calls out to a method on IBugTaskSet to get bug badge data. In my case, I added tags_for_batch which calls out to IBugTaskSet.getBugTaskTags.  getBugTaskTags is where the query happens, and then _getListingItem uses tags_for_batch, which is cached, to get the tags for the listing item.

There were a few test updates required, since the required arguments changed for BugTaskListingItem.  I also updated doc/bugtask.txt to test the new BugTaskSet method.  Otherwise, it's a pretty straight forward change.
-- 
https://code.launchpad.net/~deryck/launchpad/preload-tags-for-buglistings-901122/+merge/95023
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/preload-tags-for-buglistings-901122 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2012-02-23 22:27:32 +0000
+++ lib/lp/bugs/browser/bugtask.py	2012-02-28 19:17:18 +0000
@@ -2161,13 +2161,14 @@
     delegates(IBugTask, 'bugtask')
 
     def __init__(self, bugtask, has_bug_branch,
-                 has_specification, has_patch, request=None,
+                 has_specification, has_patch, tags, 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.request = request
         self.target_context = target_context
 
@@ -2230,7 +2231,7 @@
             'status': self.status.title,
             'status_class': 'status' + self.status.name,
             'tags': [{'url': base_tag_url + urllib.quote(tag), 'tag': tag}
-                for tag in self.bug.tags],
+                for tag in self.tags],
             'title': self.bug.title,
             }
 
@@ -2275,6 +2276,11 @@
         return getUtility(IBugTaskSet).getBugTaskBadgeProperties(
             self.currentBatch())
 
+    @cachedproperty
+    def tags_for_batch(self):
+        """Return a dict matching bugtask to it's tags."""
+        return getUtility(IBugTaskSet).getBugTaskTags(self.currentBatch())
+
     def getCookieName(self):
         """Return the cookie name used in bug listings js code."""
         cookie_name_template = '%s-buglist-fields'
@@ -2310,6 +2316,7 @@
     def _getListingItem(self, bugtask):
         """Return a decorated bugtask for the bug listing."""
         badge_property = self.bug_badge_properties[bugtask]
+        tags = self.tags_for_batch.get(bugtask.id, '')
         if (IMaloneApplication.providedBy(self.target_context) or
             IPerson.providedBy(self.target_context)):
             # XXX Tom Berger bug=529846
@@ -2323,6 +2330,7 @@
             badge_property['has_branch'],
             badge_property['has_specification'],
             badge_property['has_patch'],
+            tags,
             request=self.request,
             target_context=target_context)
 

=== modified file 'lib/lp/bugs/browser/cvereport.py'
--- lib/lp/bugs/browser/cvereport.py	2012-01-24 15:15:18 +0000
+++ lib/lp/bugs/browser/cvereport.py	2012-02-28 19:17:18 +0000
@@ -91,7 +91,8 @@
                 bugtask,
                 has_bug_branch=badges['has_branch'],
                 has_specification=badges['has_specification'],
-                has_patch=badges['has_patch'])
+                has_patch=badges['has_patch'],
+                tags='')
             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-02-24 04:00:24 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-02-28 19:17:18 +0000
@@ -1793,7 +1793,7 @@
             batched_view.activity_and_comments)
 
 
-def make_bug_task_listing_item(factory):
+def make_bug_task_listing_item(factory, tags=''):
     owner = factory.makePerson()
     bug = factory.makeBug(
         owner=owner, private=True, security_related=True)
@@ -1808,6 +1808,7 @@
         badge_property['has_branch'],
         badge_property['has_specification'],
         badge_property['has_patch'],
+        tags,
         target_context=bugtask.target)
 
 
@@ -2339,9 +2340,9 @@
 
     def test_model_tags(self):
         """Model contains bug tags."""
-        owner, item = make_bug_task_listing_item(self.factory)
+        tags = ['tag1', 'tag2']
+        owner, item = make_bug_task_listing_item(self.factory, tags=tags)
         with person_logged_in(owner):
-            item.bug.tags = ['tag1', 'tag2']
             self.assertEqual(2, len(item.model['tags']))
             self.assertTrue('tag' in item.model['tags'][0].keys())
             self.assertTrue('url' in item.model['tags'][0].keys())

=== modified file 'lib/lp/bugs/doc/bugtask.txt'
--- lib/lp/bugs/doc/bugtask.txt	2012-02-08 10:33:31 +0000
+++ lib/lp/bugs/doc/bugtask.txt	2012-02-28 19:17:18 +0000
@@ -1153,6 +1153,21 @@
      has_specification: False
 
 
+Bugtask Tags
+------------
+
+List of bugtasks often need to display related tags.  Since tags are
+related to bugtasks via bugs, BugTaskSet has a method getBugTaskTags
+that can calculate the tags in one query.
+
+    >>> some_bugtask.bug.tags = [u'foo', u'bar']
+    >>> another_bugtask.bug.tags = [u'baz', u'bop']
+    >>> tags_by_task = getUtility(IBugTaskSet).getBugTaskTags([
+    ...     some_bugtask, another_bugtask])
+    >>> print tags_by_task
+    {3: [u'foo', u'bar'], 6: [u'bop', u'baz']}
+
+
 Similar bugs
 ------------
 

=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py	2012-02-14 23:01:04 +0000
+++ lib/lp/bugs/interfaces/bugtask.py	2012-02-28 19:17:18 +0000
@@ -1490,6 +1490,12 @@
         :return: A dictionary mapping the bugs to their bugtasks.
         """
 
+    def getBugTaskTags(bugtasks):
+        """Return a set of bugtasks bug tags
+
+        Return a dict mapping from bugtask to tag.
+        """
+
     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-15 08:13:51 +0000
+++ lib/lp/bugs/model/bugtask.py	2012-02-28 19:17:18 +0000
@@ -21,6 +21,7 @@
     ]
 
 
+from collections import defaultdict
 import datetime
 from itertools import chain
 from operator import attrgetter
@@ -1380,6 +1381,23 @@
             bugs_and_tasks[bug].append(task)
         return bugs_and_tasks
 
+    def getBugTaskTags(self, bugtasks):
+        """See `IBugTaskSet`"""
+        # Import locally to avoid circular imports.
+        from lp.bugs.model.bug import Bug, BugTag
+        bugtask_ids = set(bugtask.id for bugtask in bugtasks)
+        bug_ids = set(bugtask.bugID for bugtask in bugtasks)
+        tags = IStore(BugTag).find(
+            (BugTag.tag, BugTask.id),
+            BugTask.bug == Bug.id,
+            BugTag.bug == Bug.id,
+            BugTag.bugID.is_in(bug_ids),
+            BugTask.id.is_in(bugtask_ids))
+        tags_by_bugtask = defaultdict(list)
+        for tag_name, bugtask_id in tags:
+            tags_by_bugtask[bugtask_id].append(tag_name)
+        return dict(tags_by_bugtask)
+
     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-21 22:46:28 +0000
+++ lib/lp/registry/browser/milestone.py	2012-02-28 19:17:18 +0000
@@ -254,11 +254,13 @@
     def _getListingItem(self, bugtask):
         """Return a decorated bugtask for the bug listing."""
         badge_property = self._bug_badge_properties[bugtask]
+        tags = ''
         return BugTaskListingItem(
             bugtask,
             badge_property['has_branch'],
             badge_property['has_specification'],
-            badge_property['has_patch'])
+            badge_property['has_patch'],
+            tags)
 
     @cachedproperty
     def bugtasks(self):