Merge lp:~stevenk/launchpad/remove-suggested-translation-files into lp:launchpad

[Steve Kowalik]

Stop displaying random results in the list of suggestions on Person:+translations.

Destroy ITranslationsPerson.suggestTranslatableFiles() and all of its callsites. This should hopefully stop the timeouts on Person:+translations since we no longer want to do an anti-join involving a few million rows. This changes the list that is populated to no longer be made up of roughly '3 * most active projects, 3 * least active projects, 4 * random suggestions', but now '5 * most active projects, 5 * least active projects'.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/translations/browser/person.py'
--- lib/lp/translations/browser/person.py	2012-10-30 15:54:00 +0000
+++ lib/lp/translations/browser/person.py	2013-01-14 03:38:22 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Person-related translations view classes."""
@@ -333,17 +333,6 @@
 
         return TranslateLinksAggregator().aggregate(pofiles)
 
-    def _suggestTargetsForTranslation(self, max_fetch=None):
-        """Suggest translations this person could be helping complete."""
-        person = ITranslationsPerson(self.context)
-        pofiles = person.suggestTranslatableFiles(
-            no_older_than=self.history_horizon)
-
-        if max_fetch is not None:
-            pofiles = pofiles[:max_fetch]
-
-        return TranslateLinksAggregator().aggregate(pofiles)
-
     @cachedproperty
     def all_projects_and_packages_to_review(self):
         """Top projects and packages for this person to review."""
@@ -408,10 +397,10 @@
         """Suggest translations for this person to help complete."""
         # Maximum number of translations to list that need the most work
         # done.
-        max_urgent_targets = 3
+        max_urgent_targets = 5
         # Maximum number of translations to list that are almost
         # complete.
-        max_almost_complete_targets = 3
+        max_almost_complete_targets = 5
         # Length of overall list to display.
         list_length = 10
 
@@ -426,11 +415,6 @@
             overall, almost_complete, max_almost_complete_targets,
             list_length)
 
-        fetch = 5 * (list_length - len(overall))
-        suggestions = self._suggestTargetsForTranslation(fetch)
-        overall = self._addToTargetsList(
-            overall, suggestions, list_length, list_length)
-
         return overall
 
     to_complete_template = ViewPageTemplateFile(
 
=== modified file 'lib/lp/translations/browser/tests/test_persontranslationview.py'
--- lib/lp/translations/browser/tests/test_persontranslationview.py	2012-10-30 15:54:00 +0000
+++ lib/lp/translations/browser/tests/test_persontranslationview.py	2013-01-14 03:38:22 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -308,38 +308,9 @@
             nonurgent_pofile.potemplate.productseries.product,
             descriptions[0]['target'])
 
-    def test_suggestTargetsForTranslation(self):
-        # suggestTargetsForTranslation finds targets that the person
-        # could help translate.
-        previous_contrib = self._makePOFiles(1, previously_worked_on=True)
-        pofile = self._makePOFiles(1, previously_worked_on=False)[0]
-        self._addUntranslatedMessages(pofile, 1)
-
-        descriptions = self.view._suggestTargetsForTranslation()
-
-        self.assertEqual(1, len(descriptions))
-        self.assertEqual(
-            pofile.potemplate.productseries.product,
-            descriptions[0]['target'])
-
-    def test_suggestTargetsForTranslation_limits_query(self):
-        # The max_fetch argument limits how many POFiles
-        # suggestTargetsForTranslation fetches.
-        previous_contrib = self._makePOFiles(1, previously_worked_on=True)
-        pofiles = self._makePOFiles(3, previously_worked_on=False)
-        for pofile in pofiles:
-            self._addUntranslatedMessages(pofile, 1)
-
-        descriptions = self.view._suggestTargetsForTranslation(max_fetch=2)
-
-        self.assertEqual(2, len(descriptions))
-        self.assertNotEqual(
-            descriptions[0]['target'], descriptions[1]['target'])
-
     def test_top_projects_and_packages_to_translate(self):
         # top_projects_and_packages_to_translate lists targets that the
-        # user has worked on and could help translate, followed by
-        # randomly suggested ones that also need translation.
+        # user has worked on and could help translate.
         worked_on = self._makePOFiles(1, previously_worked_on=True)[0]
         self._addUntranslatedMessages(worked_on, 1)
         not_worked_on = self._makePOFiles(1, previously_worked_on=False)[0]
@@ -347,16 +318,13 @@
 
         descriptions = self.view.top_projects_and_packages_to_translate
 
-        self.assertEqual(2, len(descriptions))
+        self.assertEqual(1, len(descriptions))
         self.assertEqual(
             worked_on.potemplate.productseries.product,
             descriptions[0]['target'])
-        self.assertEqual(
-            not_worked_on.potemplate.productseries.product,
-            descriptions[1]['target'])
 
     def test_top_p_n_p_to_translate_caps_existing_involvement(self):
-        # top_projects_and_packages_to_translate shows no more than 6
+        # top_projects_and_packages_to_translate shows up to ten
         # targets that the user has already worked on.
         pofiles = self._makePOFiles(7, previously_worked_on=True)
         for pofile in pofiles:
@@ -364,12 +332,12 @@
 
         descriptions = self.view.top_projects_and_packages_to_translate
 
-        self.assertEqual(6, len(descriptions))
+        self.assertEqual(7, len(descriptions))
 
     def test_top_p_n_p_to_translate_lists_most_and_least_translated(self):
-        # Of the maximum of 6 translations that the user has already
-        # worked on, the first 3 will be the ones with the most
-        # untranslated strings; the last 3 will have the fewest.
+        # Of the maximum of ten translations that the user has already
+        # worked on, the first 5 will be the ones with the most
+        # untranslated strings; the last 5 will have the fewest.
         # We create a lot more POFiles because internally the property
         # will fetch many POFiles.
         pofiles = self._makePOFiles(50, previously_worked_on=True)
@@ -380,15 +348,15 @@
 
         descriptions = self.view.top_projects_and_packages_to_translate
 
-        self.assertEqual(6, len(descriptions))
+        self.assertEqual(10, len(descriptions))
         targets = [item['target'] for item in descriptions]
 
-        # We happen to know that no more than 15 POFiles are fetched for
-        # each of the two categories, so the top 3 targets must be taken
-        # from the last 15 pofiles and the next 3 must be taken from the
-        # first 15 pofiles.
-        self.assertTrue(set(targets[:3]).issubset(products[15:]))
-        self.assertTrue(set(targets[3:]).issubset(products[:15]))
+        # We happen to know that no more than 25 POFiles are fetched for
+        # each of the two categories, so the top 5 targets must be taken
+        # from the last 25 pofiles and the next 5 must be taken from the
+        # first 25 pofiles.
+        self.assertTrue(set(targets[:5]).issubset(products[25:]))
+        self.assertTrue(set(targets[5:]).issubset(products[:25]))
 
         # No target is mentioned more than once in the listing.
         self.assertEqual(len(targets), len(set(targets)))
 
=== modified file 'lib/lp/translations/interfaces/translationsperson.py'
--- lib/lp/translations/interfaces/translationsperson.py	2012-10-29 06:13:44 +0000
+++ lib/lp/translations/interfaces/translationsperson.py	2013-01-14 03:38:22 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -72,12 +72,3 @@
         :return: A query result of `POFile`s ordered by number of
             untranslated messages.
         """
-
-    def suggestTranslatableFiles(no_older_than=None):
-        """Suggest `POFile`s this person could help translate.
-
-        Similar to `getTranslatableFiles`, this method picks an
-        arbitrary series of `POFile`s that the user is not a reviewer
-        for and has not worked on recently, but which are in a language
-        the user works in.
-        """
 
=== modified file 'lib/lp/translations/model/translationsperson.py'
--- lib/lp/translations/model/translationsperson.py	2012-10-30 15:54:00 +0000
+++ lib/lp/translations/model/translationsperson.py	2013-01-14 03:38:22 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -161,15 +161,9 @@
                     '(SELECT * FROM translatable_distroseries)'))).config(
             distinct=True).order_by(POFile.date_changed)
 
-    def _queryTranslatableFiles(self, worked_on, no_older_than=None,
-                                languages=None):
+    def _queryTranslatableFiles(self, no_older_than=None, languages=None):
         """Get `POFile`s this person could help translate.
 
-        :param worked_on: If True, get `POFile`s that the person has
-            been working on recently (where "recently" is defined as
-            `no_older_than`).  If False, get ones that the person has
-            not been working on recently (those that the person has
-            never worked on, or last worked on before `no_older_than`).
         :param no_older_than: Oldest involvement to consider.  If the
             person last worked on a `POFile` before this date, that
             counts as not having worked on it.
@@ -179,18 +173,25 @@
         if self.person.is_team:
             return []
 
-        tables = self._composePOFileReviewerJoins(
-            expect_reviewer_status=False)
-
-        translator_join, translator_condition = (
-            self._composePOFileTranslatorJoin(worked_on, no_older_than))
+        tables = self._composePOFileReviewerJoins(expect_reviewer_status=False)
+
+        join_condition = And(
+            POFileTranslator.personID == self.person.id,
+            POFileTranslator.pofileID == POFile.id,
+            POFile.language != getUtility(ILaunchpadCelebrities).english)
+
+        if no_older_than is not None:
+            join_condition = And(
+                join_condition,
+                POFileTranslator.date_last_touched >= no_older_than)
+
+        translator_join = Join(POFileTranslator, join_condition)
         tables.append(translator_join)
 
         translated_count = (
             POFile.currentcount + POFile.updatescount + POFile.rosettacount)
 
-        conditions = And(
-            translated_count < POTemplate.messagecount, translator_condition)
+        conditions = translated_count < POTemplate.messagecount
 
         # The person must not be a reviewer for this translation (unless
         # it's in the sense that any user gets review permissions
@@ -219,12 +220,11 @@
         if languages is not None:
             conditions = And(conditions, POFile.languageID.is_in(languages))
 
-        source = Store.of(self.person).using(*tables)
-        return source.find(POFile, conditions)
+        return Store.of(self.person).using(*tables).find(POFile, conditions)
 
     def getTranslatableFiles(self, no_older_than=None, urgent_first=True):
         """See `ITranslationsPerson`."""
-        results = self._queryTranslatableFiles(True, no_older_than)
+        results = self._queryTranslatableFiles(no_older_than)
 
         translated_count = (
             POFile.currentcount + POFile.updatescount + POFile.rosettacount)
@@ -234,16 +234,6 @@
 
         return results.order_by(ordering)
 
-    def suggestTranslatableFiles(self, no_older_than=None):
-        """See `ITranslationsPerson`."""
-        # XXX JeroenVermeulen 2009-08-28: Ideally this would also check
-        # for a free licence.  That's hard to do in SQL though.
-        languages = set([
-            language.id for language in self.translatable_languages])
-        results = self._queryTranslatableFiles(
-            False, no_older_than, languages=languages)
-        return results.order_by(['random()'])
-
     def _composePOFileReviewerCTEs(self, no_older_than):
         """Compose Storm CTEs for common `POFile` queries.
 
@@ -405,42 +395,3 @@
             TranslatorJoin,
             TranslationTeamJoin,
             ]
-
-    def _composePOFileTranslatorJoin(self, expected_presence,
-                                     no_older_than=None):
-        """Compose join condition for `POFileTranslator`.
-
-        Checks for a `POFileTranslator` record matching a `POFile` and
-        `Person` in a join.
-
-        :param expected_presence: whether the `POFileTranslator` record
-            is to be present, or absent.  The join will enforce presence
-            through a regular inner join, or absence by an outer join
-            with a condition that the record not be present.
-        :param no_older_than: optional cutoff date.  `POFileTranslator`
-            records older than this date are not considered.
-        :return: a tuple of the join, and a condition to be checked by
-            the query.  Combine it with the query's other conditions
-            using `And`.
-        """
-        join_condition = And(
-            POFileTranslator.personID == self.person.id,
-            POFileTranslator.pofileID == POFile.id,
-            POFile.language != getUtility(ILaunchpadCelebrities).english)
-
-        if no_older_than is not None:
-            join_condition = And(
-                join_condition,
-                POFileTranslator.date_last_touched >= no_older_than)
-
-        if expected_presence:
-            # A regular inner join enforces this.  No need for an extra
-            # condition; the join does it more efficiently.
-            return Join(POFileTranslator, join_condition), True
-        else:
-            # Check for absence.  Usually the best way to check for this
-            # is an outer join plus a condition that the outer join
-            # match no record.
-            return (
-                LeftJoin(POFileTranslator, join_condition),
-                POFileTranslator.id == None)
 
=== modified file 'lib/lp/translations/tests/test_potmsgset.py'
--- lib/lp/translations/tests/test_potmsgset.py	2013-01-07 02:40:55 +0000
+++ lib/lp/translations/tests/test_potmsgset.py	2013-01-14 03:38:22 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -408,7 +408,7 @@
         self.assertContentEqual(
             self.potmsgset.getExternallyUsedTranslationMessages(language),
             [other_translation, current_translation])
-        self.assertEquals(
+        self.assertContentEqual(
             self.potmsgset.getExternallySuggestedOrUsedTranslationMessages(
                 used_languages=[language])[language].used,
             [other_translation, current_translation])