launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/broken-links-person-translations into lp:launchpad.

Commit message:
Filters products not using translations out of the recent history in Person:+translations to prevent 404 links.

Requested reviews:
  Curtis Hovey (sinzui)
Related bugs:
  Bug #164530 in Launchpad itself: "User translations showing broken links"
  https://bugs.launchpad.net/launchpad/+bug/164530

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/broken-links-person-translations/+merge/131199

Summary
=======
Products which have translations but are no longer setup to use translations
create 404 links in Person+translations in the recent activity display.

The initial assumption was that the translation_history query on
TranslationPerson was bad, and should do the filtering. However, this query is
used in places where the absolute history is necessary (e.g. showing if a
person has *ever* done a translation) and the number of joins necessary to
filter at the level poses a performance risk with large histories.

The view for Person+translations has a method, `recent_history` which already
filters the translation_history to weed out inactive projects. This method
should also filter out projects no longer using translations.

Preimp
======
Spoke with Curtis Hovey.

Implementation
==============
`recent_activity`'s filtering method, `is_active`, now checks for
`product.translations_usage == ServiceUsage.LAUNCHPAD` as well as
`product.active`.

The test for recent_activity now includes a pofile, which should be excluded,
that is linked to a product where translations_usage is
ServiceUsage.NOT_APPLICABLE.

Tests
=====
bin/test -vvct test_recent_translation_activity

QA
==
Ensure that the page linked in the bug no longer has 404ing links.

LoC
===
I have ~400 LoC in credit.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/browser/tests/test_persontranslationview.py
  lib/lp/translations/model/translationsperson.py
  lib/lp/translations/browser/person.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/broken-links-person-translations/+merge/131199
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/translations/browser/person.py'
--- lib/lp/translations/browser/person.py	2012-10-10 18:10:52 +0000
+++ lib/lp/translations/browser/person.py	2012-10-24 14:36:25 +0000
@@ -33,6 +33,7 @@
     custom_widget,
     LaunchpadFormView,
     )
+from lp.app.enums import ServiceUsage
 from lp.app.widgets.itemswidgets import LaunchpadRadioWidget
 from lp.registry.interfaces.sourcepackage import ISourcePackage
 from lp.services.propertycache import cachedproperty
@@ -231,7 +232,10 @@
             if potemplate is None:
                 return True
             product = potemplate.product
-            return product is None or product.active
+            product_is_active = (
+                product.active and
+                product.translations_usage == ServiceUsage.LAUNCHPAD)
+            return product is None or product_is_active
 
         active_entries = (entry for entry in all_entries if is_active(entry))
         return [ActivityDescriptor(self.context, entry)

=== modified file 'lib/lp/translations/browser/tests/test_persontranslationview.py'
--- lib/lp/translations/browser/tests/test_persontranslationview.py	2012-10-10 19:10:07 +0000
+++ lib/lp/translations/browser/tests/test_persontranslationview.py	2012-10-24 14:36:25 +0000
@@ -7,6 +7,7 @@
 
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.enums import ServiceUsage
 from lp.services.webapp import canonical_url
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
@@ -196,6 +197,12 @@
         # and make one which has not been worked on (will be excluded)
         self._makePOFiles(1, previously_worked_on=False)
 
+        # and make one which has a product no longer using translations (will
+        # be excluded)
+        [pofile] = self._makePOFiles(1, previously_worked_on=True)
+        naked_product = removeSecurityProxy(pofile.potemplate.product)
+        naked_product.translations_usage = ServiceUsage.NOT_APPLICABLE
+
         pofiles_worked_on = self._makePOFiles(11, previously_worked_on=True)
 
         # the expected results

=== modified file 'lib/lp/translations/model/translationsperson.py'
--- lib/lp/translations/model/translationsperson.py	2012-10-19 00:37:58 +0000
+++ lib/lp/translations/model/translationsperson.py	2012-10-24 14:36:25 +0000
@@ -71,7 +71,8 @@
 
     def getTranslationHistory(self, no_older_than=None):
         """See `ITranslationsPerson`."""
-        conditions = (POFileTranslator.person == self.person)
+        conditions = And(
+            POFileTranslator.person == self.person)
         if no_older_than is not None:
             conditions = And(
                 conditions,
@@ -294,10 +295,6 @@
 
         ProductSeriesJoin = LeftJoin(
             ProductSeries, ProductSeries.id == POTemplate.productseriesID)
-        # XXX j.c.sackett 2010-11-19 bug=677532 It's less than ideal that
-        # this query is using _translations_usage, but there's no cleaner
-        # way to deal with it. Once the bug above is resolved, this should
-        # should be fixed to use translations_usage.
         ProductJoin = LeftJoin(Product, And(
             Product.id == ProductSeries.productID,
             Product.translations_usage == ServiceUsage.LAUNCHPAD,