launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

Deryck Hodge has proposed merging lp:~deryck/launchpad/fewer-milestone-queries-bugtask-index into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


Inspired by Robert's work related to fixing bug 636158 (timeouts on
bugtask+index), I've been trying to find spots to further reduce the
query count.  This is a small branch that fixes up how we get
milestone data for the bug page and reduces the overall query count
for bugtask+index by 2 queries.

It's small, I know, but hey this can land on its own from other work
I'm doing. :-)

To do this, I fixed up the method that looks up the milestones for
the widget and passed in the milestones themselves rather than a
base vocabulary object.  I ran a liberal test expression to make
sure this has no fallout:

./bin/test -cvvt "milestone|vocab"

I also lowered the query count in the test Robert added in
TestBugTaskView by 2 queries.  To confirm, run:

./bin/test -cvvt TestBugTaskView

The test really only has the one query count change.  The rest is me
fixing up lint.
-- 
https://code.launchpad.net/~deryck/launchpad/fewer-milestone-queries-bugtask-index/+merge/36458
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/fewer-milestone-queries-bugtask-index into lp:launchpad/devel.

=== modified file 'lib/canonical/widgets/lazrjs.py'
--- lib/canonical/widgets/lazrjs.py	2010-02-23 14:07:54 +0000
+++ lib/canonical/widgets/lazrjs.py	2010-09-23 15:21:10 +0000
@@ -359,24 +359,28 @@
         disabled_items = []
     items = []
     for item in vocab:
+        # Introspect to make sure we're dealing with the object itself.
+        # SimpleTerm objects have the object itself at item.value.
+        if hasattr(item, 'value'):
+            item = item.value
         if name_fn is not None:
-            name = name_fn(item.value)
+            name = name_fn(item)
         else:
-            name = item.value.title
+            name = item.title
         if value_fn is not None:
-            value = value_fn(item.value)
+            value = value_fn(item)
         else:
-            value = item.value.title
+            value = item.title
         new_item = {
             'name': name,
             'value': value,
             'style': '', 'help': '', 'disabled': False}
         for disabled_item in disabled_items:
-            if disabled_item == item.value:
+            if disabled_item == item:
                 new_item['disabled'] = True
                 break
         if css_class_prefix is not None:
-            new_item['css_class'] = css_class_prefix + item.value.name
+            new_item['css_class'] = css_class_prefix + item.name
         items.append(new_item)
 
     if as_json:

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2010-09-21 10:03:47 +0000
+++ lib/lp/bugs/browser/bugtask.py	2010-09-23 15:21:10 +0000
@@ -3608,12 +3608,17 @@
 
         return items
 
+    @cachedproperty
+    def _visible_milestones(self):
+        """The visible milestones for this context."""
+        return MilestoneVocabulary(self.context).visible_milestones
+
     @property
     def milestone_widget_items(self):
         """The available milestone items as JSON."""
         if self.user is not None:
             items = vocabulary_to_choice_edit_items(
-                MilestoneVocabulary(self.context),
+                self._visible_milestones,
                 value_fn=lambda item: canonical_url(
                     item, request=IWebServiceClientRequest(self.request)))
             items.append({
@@ -3628,8 +3633,7 @@
     @cachedproperty
     def target_has_milestones(self):
         """Are there any milestones we can target?"""
-        return MilestoneVocabulary.getMilestoneTarget(
-            self.context).has_milestones
+        return len(self._visible_milestones) > 0
 
     def bugtask_canonical_url(self):
         """Return the canonical url for the bugtask."""

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2010-09-22 20:56:52 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2010-09-23 15:21:10 +0000
@@ -6,16 +6,12 @@
 __metaclass__ = type
 
 
-from contextlib import nested
 from doctest import DocTestSuite
 import unittest
 
 from storm.store import Store
-from testtools.matchers import (
-    Equals,
-    LessThan,
-    MatchesAny,
-    )
+from testtools.matchers import LessThan
+
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.ftests import (
@@ -33,7 +29,6 @@
 from canonical.testing import LaunchpadFunctionalLayer
 from lp.bugs.browser import bugtask
 from lp.bugs.browser.bugtask import (
-    BugTaskView,
     BugTaskEditView,
     BugTasksAndNominationsView,
     )
@@ -62,23 +57,23 @@
 
     def test_rendered_query_counts_constant_with_team_memberships(self):
         login(ADMIN_EMAIL)
-        bugtask = self.factory.makeBugTask()
+        task = self.factory.makeBugTask()
         person_no_teams = self.factory.makePerson(password='test')
         person_with_teams = self.factory.makePerson(password='test')
         for _ in range(10):
             self.factory.makeTeam(members=[person_with_teams])
         # count with no teams
-        url = canonical_url(bugtask)
+        url = canonical_url(task)
         recorder = QueryCollector()
         recorder.register()
         self.addCleanup(recorder.unregister)
-        self.invalidate_caches(bugtask)
+        self.invalidate_caches(task)
         self.getUserBrowser(url, person_no_teams)
         # This may seem large: it is; there is easily another 30% fat in there.
-        self.assertThat(recorder, HasQueryCount(LessThan(64)))
+        self.assertThat(recorder, HasQueryCount(LessThan(62)))
         count_with_no_teams = recorder.count
         # count with many teams
-        self.invalidate_caches(bugtask)
+        self.invalidate_caches(task)
         self.getUserBrowser(url, person_with_teams)
         # Allow an increase of one because storm bug 619017 causes additional
         # queries, revalidating things unnecessarily. An increase which is