launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13833
[Merge] lp:~stevenk/launchpad/remove-suggested-translations into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/remove-suggested-translations into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1013013 in Launchpad itself: "Timeout viewing my own translations"
https://bugs.launchpad.net/launchpad/+bug/1013013
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/remove-suggested-translations/+merge/131820
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.
--
https://code.launchpad.net/~stevenk/launchpad/remove-suggested-translations/+merge/131820
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/remove-suggested-translations into 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-29 06:37: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:37: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:37: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:37: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:37:22 +0000
@@ -10,7 +10,7 @@
timedelta,
)
-from pytz import timezone
+from pytz import UTC
import transaction
from zope.security.proxy import removeSecurityProxy
@@ -23,9 +23,6 @@
from lp.translations.model.translator import TranslatorSet
-UTC = timezone('UTC')
-
-
class ReviewTestMixin:
"""Base for testing which translations a reviewer can review."""
@@ -99,12 +96,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 +181,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())
Follow ups
