Merge lp:~stevenk/launchpad/remove-suggested-translations into lp:launchpad

[Steve Kowalik]

Remove ITranslationsPerson.suggestReviewableTranslationFiles() and the parts of Person:+translations that call it.

Remove ITranslationsPerson.suggestReviewableTranslationFiles() and the parts of Person:+translations that call it.

The slightly longer story is that in r16208, the query that is at the heart of ITranslationsPerson.getReviewableTranslationFiles() was rewritten to make use of 4 CTEs rather than 10 joins. This dropped it from taking over 1.5 seconds to about 11ms. The new query was written with one assumption at its core -- we have *lots* of pofiles, but translators themselves don't tend to touch many of them, so if we limit there first, we get a small result, and therefore a fast query.

The query in suggestReviewableTranslationFiles() on the other hand, can not abide that assumption at all. The very first thing is does is a anti-join because it wants a list of pofiles that the translator has never touched. This query is incredibly expensive, and I personally don't think it's worth the value of showing maybe 1 suggestion on Person:+translations if the query is going to take over 2 seconds to run -- or worse, cause a timeout.

[Jeroen T. Vermeulen (jtv)]

Discussed this on IRC and IRL with Steve and Huw.

The functionality is there to get some random scatter on where translators go. We still want that. Some translations are naturally easier to get to than others, which leads to duplication of effort. For example, if many translators end up on the same translation page on the same day, they could produce a hundred suggestions for the same string — and review may become the bottleneck. There's much more use to spreading the effort out randomly, even if it guides people to lower-priority work. This is also why we can't do a lot of caching here.

From a UI perspective, filling out the list of translations-you-should-work-on with random suggestions is confusing. It's very hard to avoid making nonsensical suggestions, but also necessary because the suggestions are unsolicited. And coincidentally, that difficulty also drives up the cost of the query.

So what we suggest is to stop filling out the list with random suggestions, but add a “take me to a random project where I can help” link. It doesn't need to contain a precomputed link (it could link to a view that produces a temporary redirect) and the “slot-machine” presentation makes it a lot more acceptable to suggest projects that the user has already worked on, for example.

This may simplify the search. For example, the random suggestion could now:

 - be a bit more limited in which permissions models a project must have, to simplify the privileges check;

 - ignore the risk of suggesting a project the user is currently active in;

 - skip the complex multi-level aggregation logic for grouping POFiles.

Each as appropriate, of course. One caveat is that the list of suggested work items may now be empty, but the link would still have to be there.

Jeroen

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== 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-29 06:41:22 +0000
@@ -313,21 +313,6 @@
 
         return ReviewLinksAggregator().aggregate(pofiles)
 
-    def _suggestTargetsForReview(self, max_fetch):
-        """Find random translation 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.
-        """
-        person = ITranslationsPerson(self.context)
-        pofiles = person.suggestReviewableTranslationFiles(
-            no_older_than=self.history_horizon)[:max_fetch]
-
-        return ReviewLinksAggregator().aggregate(pofiles)
-
     def _getTargetsForTranslation(self, max_fetch=None):
         """Get translation targets for this person to translate.
 
@@ -405,18 +390,9 @@
         # Start out with the translations that the person has recently
         # worked on.
         recent = self._review_targets
-        overall = self._addToTargetsList(
+        return self._addToTargetsList(
             [], recent, max_known_targets, list_length)
 
-        # Fill out the list with other, randomly suggested translations
-        # that the person could also be reviewing.
-        fetch = 5 * (list_length - len(overall))
-        suggestions = self._suggestTargetsForReview(fetch)
-        overall = self._addToTargetsList(
-            overall, suggestions, list_length, list_length)
-
-        return overall
-
     @cachedproperty
     def num_projects_and_packages_to_review(self):
         """How many translations do we suggest for reviewing?"""

=== 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-29 06:41:22 +0000
@@ -167,20 +167,13 @@
 
     def test_top_projects_and_packages_to_review(self):
         # top_projects_and_packages_to_review tries to name at least one
-        # translation target that the person has worked on, and at least
-        # one random suggestion that the person hasn't worked on.
+        # translation target that the person has worked on.
         self._makeReviewer()
         pofile_worked_on = self._makePOFiles(1, previously_worked_on=True)[0]
-        pofile_not_worked_on = self._makePOFiles(
-            1, previously_worked_on=False)[0]
-
         targets = self.view.top_projects_and_packages_to_review
 
         pofile_suffix = '/+translate?show=new_suggestions'
-        expected_links = [
-            canonical_url(pofile_worked_on) + pofile_suffix,
-            canonical_url(pofile_not_worked_on) + pofile_suffix,
-            ]
+        expected_links = [canonical_url(pofile_worked_on) + pofile_suffix]
         self.assertEqual(
             set(expected_links), set(item['link'] for item in targets))
 
@@ -221,29 +214,16 @@
         self.assertEqual(9, len(targets))
         self.assertEqual(9, len(set(item['link'] for item in targets)))
 
-    def test_top_p_n_p_to_review_caps_suggestions(self):
-        # top_projects_and_packages will suggest at most 10 POFiles that
-        # the person has not worked on.
-        self._makeReviewer()
-        self._makePOFiles(11, previously_worked_on=False)
-
-        targets = self.view.top_projects_and_packages_to_review
-
-        self.assertEqual(10, len(targets))
-        self.assertEqual(10, len(set(item['link'] for item in targets)))
-
     def test_top_p_n_p_to_review_caps_total(self):
-        # top_projects_and_packages will show at most 10 POFiles
-        # overall.  The last one will be a suggestion.
+        # top_projects_and_packages will show at most 9 POFiles
+        # overall.
         self._makeReviewer()
         pofiles_worked_on = self._makePOFiles(11, previously_worked_on=True)
-        pofiles_not_worked_on = self._makePOFiles(
-            11, previously_worked_on=False)
 
         targets = self.view.top_projects_and_packages_to_review
 
-        self.assertEqual(10, len(targets))
-        self.assertEqual(10, len(set(item['link'] for item in targets)))
+        self.assertEqual(9, len(targets))
+        self.assertEqual(9, len(set(item['link'] for item in targets)))
 
     def test_person_is_translator_false(self):
         # By default, a user is not a translator.

=== modified file 'lib/lp/translations/interfaces/translationsperson.py'
--- lib/lp/translations/interfaces/translationsperson.py	2011-12-24 16:54:44 +0000
+++ lib/lp/translations/interfaces/translationsperson.py	2012-10-29 06:41:22 +0000
@@ -1,8 +1,6 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=E0213
-
 __metaclass__ = type
 __all__ = [
     'ITranslationsPerson',
@@ -61,16 +59,6 @@
             unreviewed `TranslationMessage` (oldest first).
         """
 
-    def suggestReviewableTranslationFiles(maximum):
-        """Suggest `POFile`s this person could review.
-
-        Unlike `getReviewableTranslationFiles`, this method looks for
-        arbitrary translations that the person has not worked on in the
-        recent past.
-
-        :param maximum: Maximum number of `POFile`s to return.
-        """
-
     def getTranslatableFiles(no_older_than=None, urgent_first=True):
         """List `POFile`s this person should be able to help translate.
 

=== modified file 'lib/lp/translations/model/translationsperson.py'
--- lib/lp/translations/model/translationsperson.py	2012-10-25 06:08:35 +0000
+++ lib/lp/translations/model/translationsperson.py	2012-10-29 06:41:22 +0000
@@ -160,22 +160,6 @@
                     '(SELECT * FROM translatable_distroseries)'))).config(
             distinct=True).order_by(POFile.date_changed)
 
-    def suggestReviewableTranslationFiles(self, no_older_than=None):
-        """See `ITranslationsPerson`."""
-        tables = self._composePOFileReviewerJoins()
-
-        # Pick files that this person has no recent POFileTranslator entry
-        # for.
-        translator_join, translator_condition = (
-            self._composePOFileTranslatorJoin(False, no_older_than))
-        tables.append(translator_join)
-
-        conditions = And(POFile.unreviewed_count > 0, translator_condition)
-
-        source = Store.of(self.person).using(*tables)
-        query = source.find(POFile, conditions)
-        return query.config(distinct=True).order_by(POFile.id)
-
     def _queryTranslatableFiles(self, worked_on, no_older_than=None,
                                 languages=None):
         """Get `POFile`s this person could help translate.

=== modified file 'lib/lp/translations/tests/test_translations_to_review.py'
--- lib/lp/translations/tests/test_translations_to_review.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/tests/test_translations_to_review.py	2012-10-29 06:41:22 +0000
@@ -10,7 +10,7 @@
     timedelta,
     )
 
-from pytz import timezone
+from pytz import UTC
 import transaction
 from zope.security.proxy import removeSecurityProxy
 
@@ -19,13 +19,9 @@
 from lp.testing import TestCaseWithFactory
 from lp.testing.layers import DatabaseFunctionalLayer
 from lp.translations.interfaces.translationsperson import ITranslationsPerson
-from lp.translations.model.pofiletranslator import POFileTranslatorSet
 from lp.translations.model.translator import TranslatorSet
 
 
-UTC = timezone('UTC')
-
-
 class ReviewTestMixin:
     """Base for testing which translations a reviewer can review."""
 
@@ -99,12 +95,6 @@
         return list(person.getReviewableTranslationFiles(
             *args, **kwargs))
 
-    def _suggestReviewables(self, *args, **kwargs):
-        """Shorthand for `self.person.suggestReviewableTranslationFiles`."""
-        person = ITranslationsPerson(self.person)
-        return list(person.suggestReviewableTranslationFiles(
-            *args, **kwargs))
-
 
 class ReviewableTranslationFilesTest:
     """Test getReviewableTranslationFiles for a given setup.
@@ -190,69 +180,3 @@
     def setUp(self):
         super(TestReviewableDistroTranslationFiles, self).setUp()
         ReviewTestMixin.setUpMixin(self, for_product=False)
-
-
-class TestSuggestReviewableTranslationFiles(TestCaseWithFactory,
-                                            ReviewTestMixin):
-    """Test `Person.suggestReviewableTranslationFiles`."""
-    layer = DatabaseFunctionalLayer
-
-    def setUp(self):
-        super(TestSuggestReviewableTranslationFiles, self).setUp()
-        ReviewTestMixin.setUpMixin(self)
-
-    def _makeOtherPOFile(self, language_code='nl', same_group=True,
-                         with_unreviewed=True):
-        """Set up a `POFile` for an unrelated `POTemplate`."""
-        other_pofile = self.factory.makePOFile(language_code=language_code)
-        other_pofile = removeSecurityProxy(other_pofile)
-
-        product = other_pofile.potemplate.productseries.product
-        product.translations_usage = ServiceUsage.LAUNCHPAD
-
-        if with_unreviewed:
-            other_pofile.unreviewed_count = 1
-
-        if same_group:
-            product.translationgroup = self.translationgroup
-
-        return other_pofile
-
-    def test_suggestReviewableTranslationFiles_suggests_files(self):
-        # suggestReviewableTranslationFiles suggests translations to
-        # review.
-        other_pofile = self._makeOtherPOFile()
-        self.assertEqual([other_pofile], self._suggestReviewables())
-
-    def test_suggestReviewableTranslationFiles_is_complementary(self):
-        # suggestReviewableTranslationFiles does not suggest files that
-        # the person is already working on.
-        self.assertFalse(self.pofile in self._suggestReviewables())
-
-    def test_suggestReviewableTranslationFiles_ignores_old_involvement(self):
-        # After a person's involvement with a translation grows old
-        # enough, it becomes eligible for suggestion again.
-        poftset = POFileTranslatorSet()
-        involvement = poftset.getForPersonPOFile(self.person, self.pofile)
-        removeSecurityProxy(involvement).date_last_touched -= timedelta(366)
-        suggestions = self._suggestReviewables(
-            no_older_than=involvement.date_last_touched + timedelta(1))
-
-        self.assertEqual([self.pofile], suggestions)
-
-    def test_suggestReviewableTranslationFiles_no_translation_group(self):
-        # Only translations that fall under the same translation group
-        # are suggested.
-        other_pofile = self._makeOtherPOFile(same_group=False)
-        self.assertFalse(other_pofile in self._suggestReviewables())
-
-    def test_suggestReviewableTranslationFiles_ignores_other_languages(self):
-        # suggestReviewableTranslationFiles does not suggest files in
-        # languages that the person is not active in.
-        other_pofile = self._makeOtherPOFile(language_code='ban')
-        self.assertFalse(other_pofile in self._suggestReviewables())
-
-    def test_suggestReviewableTranslationFiles_checks_unreviewed(self):
-        # Translations without unreviewed suggestions are ignored.
-        other_pofile = self._makeOtherPOFile(with_unreviewed=False)
-        self.assertFalse(other_pofile in self._suggestReviewables())