launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-590014 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #590014 in Launchpad itself: "Person:+translations timeouts"
  https://bugs.launchpad.net/launchpad/+bug/590014

For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-590014/+merge/70614

This branch reduces the number of queries needed to render the
translations page for a person by refactoring the code such that already
run queries are retained by cachedproperty decorators.

Tests for the view can be run with this command:

    bin/test -c -m lp.translations.browser.tests -t test_persontranslationview

Lint: the lint report is clean

QA: Compare translations.launchpad.net/~ and
translations.qastaging.launchpad.net/~ and make sure they look vaguely
similar (not that some lists are randomized so variances there are to be
expected).

-- 
https://code.launchpad.net/~benji/launchpad/bug-590014/+merge/70614
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-590014 into lp:launchpad.

=== modified file 'lib/lp/translations/browser/configure.zcml'
--- lib/lp/translations/browser/configure.zcml	2011-05-27 21:03:22 +0000
+++ lib/lp/translations/browser/configure.zcml	2011-08-05 18:45:01 +0000
@@ -772,20 +772,6 @@
             permission="zope.Public"
             template="../templates/person-translations-to-review.pt"
             layer="lp.translations.publisher.TranslationsLayer"/>
-        <browser:page
-            for="lp.registry.interfaces.person.IPerson"
-            name="+translations-to-review-table"
-            class="lp.translations.browser.person.PersonTranslationView"
-            permission="zope.Public"
-            template="../templates/person-translations-to-review-table.pt"
-            layer="lp.translations.publisher.TranslationsLayer"/>
-        <browser:page
-            for="lp.registry.interfaces.person.IPerson"
-            name="+translations-to-complete-table"
-            class="lp.translations.browser.person.PersonTranslationView"
-            permission="zope.Public"
-            template="../templates/person-translations-to-complete-table.pt"
-            layer="lp.translations.publisher.TranslationsLayer"/>
 
 
 <!-- Product views -->

=== modified file 'lib/lp/translations/browser/person.py'
--- lib/lp/translations/browser/person.py	2011-05-19 13:26:10 +0000
+++ lib/lp/translations/browser/person.py	2011-08-05 18:45:01 +0000
@@ -19,6 +19,7 @@
 import urllib
 
 import pytz
+from z3c.ptcompat import ViewPageTemplateFile
 from zope.app.form.browser import TextWidget
 from zope.component import getUtility
 from zope.interface import (
@@ -134,12 +135,24 @@
             "while listing activity for %s." % (
                 person.name, pofiletranslator.person.name))
 
-        self.date = pofiletranslator.date_last_touched
-
-        pofile = pofiletranslator.pofile
-
-        self.title = pofile.potemplate.translationtarget.title
-        self.url = compose_pofile_filter_url(pofile, person)
+        self._person = person
+        self._pofiletranslator = pofiletranslator
+
+    @cachedproperty
+    def date(self):
+        return self._pofiletranslator.date_last_touched
+
+    @cachedproperty
+    def _pofile(self):
+        return self._pofiletranslator.pofile
+
+    @cachedproperty
+    def title(self):
+        return self._pofile.potemplate.translationtarget.title
+
+    @cachedproperty
+    def url(self):
+        return compose_pofile_filter_url(self._pofile, self._person)
 
 
 def person_is_reviewer(person):
@@ -193,7 +206,10 @@
     def __init__(self, *args, **kwargs):
         super(PersonTranslationView, self).__init__(*args, **kwargs)
         now = datetime.now(pytz.timezone('UTC'))
-        self.history_horizon = now - timedelta(90, 0, 0)
+        # Down-to-the-second detail isn't important so the hope is that this
+        # will result in faster queries (cache effects).
+        today = now.replace(minute=0, second=0, microsecond=0)
+        self.history_horizon = today - timedelta(90, 0, 0)
 
     @property
     def page_title(self):
@@ -282,22 +298,17 @@
         return not (
             translationmessage.potmsgset.hide_translations_from_anonymous)
 
-    def _getTargetsForReview(self, max_fetch=None):
+    @cachedproperty
+    def _review_targets(self):
         """Query and aggregate the top targets for review.
 
-        :param max_fetch: Maximum number of `POFile`s to fetch while
-            looking for these.
-        :return: a list of at most `max_fetch` translation targets.
-            Multiple `POFile`s may be aggregated together into a single
-            target.
+        :return: a list of translation targets.  Multiple `POFile`s may be
+            aggregated together into a single target.
         """
         person = ITranslationsPerson(self.context)
         pofiles = person.getReviewableTranslationFiles(
             no_older_than=self.history_horizon)
 
-        if max_fetch is not None:
-            pofiles = pofiles[:max_fetch]
-
         return ReviewLinksAggregator().aggregate(pofiles)
 
     def _suggestTargetsForReview(self, max_fetch):
@@ -344,7 +355,7 @@
     @cachedproperty
     def all_projects_and_packages_to_review(self):
         """Top projects and packages for this person to review."""
-        return self._getTargetsForReview()
+        return self._review_targets
 
     def _addToTargetsList(self, existing_targets, new_targets, max_items,
                           max_overall):
@@ -390,10 +401,8 @@
         list_length = 10
 
         # Start out with the translations that the person has recently
-        # worked on.  Aggregation may reduce the number we get, so ask
-        # the database for a few extra.
-        fetch = 5 * max_known_targets
-        recent = self._getTargetsForReview(fetch)
+        # worked on.
+        recent = self._review_targets
         overall = self._addToTargetsList(
             [], recent, max_known_targets, list_length)
 
@@ -442,6 +451,21 @@
         return overall
 
 
+    to_complete_template = ViewPageTemplateFile(
+        '../templates/person-translations-to-complete-table.pt')
+
+    def translations_to_complete_table(self):
+        return self.to_complete_template(dict(view=self))
+
+
+    to_review_template = ViewPageTemplateFile(
+        '../templates/person-translations-to-review-table.pt')
+
+    def translations_to_review_table(self):
+        return self.to_review_template(dict(view=self))
+
+
+
 class PersonTranslationReviewView(PersonTranslationView):
     """View for translation-related Person pages."""
 

=== modified file 'lib/lp/translations/templates/person-translations.pt'
--- lib/lp/translations/templates/person-translations.pt	2011-07-01 15:24:41 +0000
+++ lib/lp/translations/templates/person-translations.pt	2011-08-05 18:45:01 +0000
@@ -55,7 +55,7 @@
           By reviewing the following translations, you can help ensure other translators' work is published to software users. (<a href="/+help/reviewing.html" target="help">More about reviewing</a>)
         </p>
         <metal:translations-to-review
-          tal:replace="structure context/@@+translations-to-review-table" />
+          tal:replace="structure view/translations_to_review_table" />
         <div class="see-all"
              tal:condition="python: view.num_projects_and_packages_to_review > 1">
           <a href="+translations-to-review" id="translations-to-review-link">
@@ -74,7 +74,7 @@
       <div id="translations-to-complete-section" class="portlet">
         <h2>Translations you can help complete</h2>
         <metal:translations-to-complete
-          tal:replace="structure context/@@+translations-to-complete-table" />
+          tal:replace="structure view/translations_to_complete_table" />
       </div>
     </tal:translator>
   </tal:me>