launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04385
[Merge] lp:~henninge/launchpad/bug-504062-external-suggestions into lp:launchpad
Henning Eggers has proposed merging lp:~henninge/launchpad/bug-504062-external-suggestions into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #504062 in Launchpad itself: "External suggestion "Used in" links to disabled template"
https://bugs.launchpad.net/launchpad/+bug/504062
Bug #816366 in Launchpad itself: "Misleading code in POTMsgSet"
https://bugs.launchpad.net/launchpad/+bug/816366
For more details, see:
https://code.launchpad.net/~henninge/launchpad/bug-504062-external-suggestions/+merge/69296
= Summary =
The description for bug 504062 is probably a bit old. External
suggestions used to make this page timeout and that's why we now have a
"suggestivepotemplates" cache table. It is rebuilt daily by the garbo.py
script and does *not* include inactive templates. So fixing this bug is
not about linking to a non-existent template but about cache
invalidation.
Also it is not clear how common this error is as deactivating templates
is not somehting that is done very often. OTOH it can be done by non-
admins nowadays, so it needs some sort of solution.
== Proposed fix ==
The easiest and most straight forward fix is to remove a template from
the cache when it is deactivated. There are three places where a
template can be deactivated: the edit form, the admin form and the
webservice. The last one is the least common, I expect, since the
Translation is not much used because it is rather incomplete. I moved
that concern into bug 816361 to be followed-up on later.
== Pre-implementation notes ==
I talked about this to jtv and we agreed on this solution.
== Implementation details ==
I removed a lot of lint, sorry about the noise.
lib/lp/translations/browser/potemplate.py
* The method _removeFromSuggestivePOTemplateCache was introduced to
make it possible to patch it during testing, as you will see further
down.
lib/lp/translations/model/potmsgset.py
* All changes in this file are purely cosmetical.
* I fixed bug 816366 as a drive-by which was discovered during the
pre-imp discussion.
== Tests ==
lib/lp/translations/tests/test_potemplate.py
* I hope the edge detection test is not too complicated to follow. I
like as I always like ways to reduce the number of asserts in a
test.
* Here is where the patching happens.
I think most translation page tests need to be re-run to make sure none
of them depends on external suggestions that might be gone now. Might be
best to run run all translations tests, they don't take *that* long.
bin/test -vvcm lp.translations
== Demo and Q/A ==
Go to any translation page (+translate) where you see external
suggestions. Go to the template where the external suggestion is coming
from and disable it. Now return to the original page and reload it. The
external suggestion is gone.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/translations/tests/test_potemplate.py
lib/lp/translations/interfaces/potemplate.py
lib/lp/translations/scripts/tests/test_cache_suggestive_templates.py
lib/lp/translations/model/potmsgset.py
lib/lp/translations/model/potemplate.py
lib/lp/translations/browser/potemplate.py
--
https://code.launchpad.net/~henninge/launchpad/bug-504062-external-suggestions/+merge/69296
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/bug-504062-external-suggestions into lp:launchpad.
=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py 2011-06-07 10:28:11 +0000
+++ lib/lp/translations/browser/potemplate.py 2011-07-26 15:04:52 +0000
@@ -479,7 +479,7 @@
'be imported, %s will be reviewed manually by an '
'administrator in the coming few days. You can track '
'your upload\'s status in the '
- '<a href="%s/+imports">Translation Import Queue</a>' %(
+ '<a href="%s/+imports">Translation Import Queue</a>' % (
num, plural_s, plural_s, itthey,
canonical_url(self.context.translationtarget))))
if len(conflicts) > 0:
@@ -551,6 +551,8 @@
@action(_('Change'), name='change')
def change_action(self, action, data):
context = self.context
+ iscurrent = data.get('iscurrent', context.iscurrent)
+ context.setActive(iscurrent)
old_description = context.description
old_translation_domain = context.translation_domain
self.updateContextFromData(data)
=== modified file 'lib/lp/translations/interfaces/potemplate.py'
--- lib/lp/translations/interfaces/potemplate.py 2011-06-08 09:00:23 +0000
+++ lib/lp/translations/interfaces/potemplate.py 2011-07-26 15:04:52 +0000
@@ -331,6 +331,13 @@
method to do that.
"""
+ def setActive(active):
+ """Toggle the iscurrent flag.
+
+ Takes care of updating the suggestive potempalte cache when the
+ template is disabled.
+ """
+
def getHeader():
"""Return an `ITranslationHeaderData` representing its header."""
@@ -691,6 +698,12 @@
:return: Number of rows deleted.
"""
+ def removeFromSuggestivePOTemplatesCache(potemplate):
+ """Remove the given potemplate from the suggestive-templates cache.
+
+ :return: True if the template was in the cache.
+ """
+
def populateSuggestivePOTemplatesCache():
"""Populate suggestive-templates cache.
=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py 2011-05-27 21:12:25 +0000
+++ lib/lp/translations/model/potemplate.py 2011-07-26 15:04:52 +0000
@@ -244,6 +244,17 @@
"""See `IPOTemplate`."""
self._cached_pofiles_by_language = None
+ def _removeFromSuggestivePOTemplatesCache(self):
+ """One level of indirection to make testing easier."""
+ getUtility(
+ IPOTemplateSet).removeFromSuggestivePOTemplatesCache(self)
+
+ def setActive(self, active):
+ """See `IPOTemplate`."""
+ if not active and active != self.iscurrent:
+ self._removeFromSuggestivePOTemplatesCache()
+ self.iscurrent = active
+
@property
def uses_english_msgids(self):
"""See `IPOTemplate`."""
@@ -1375,6 +1386,13 @@
return IMasterStore(POTemplate).execute(
"DELETE FROM SuggestivePOTemplate").rowcount
+ def removeFromSuggestivePOTemplatesCache(self, potemplate):
+ """See `IPOTemplateSet`."""
+ rowcount = IMasterStore(POTemplate).execute(
+ "DELETE FROM SuggestivePOTemplate "
+ "WHERE potemplate = %s" % sqlvalues(potemplate)).rowcount
+ return rowcount == 1
+
def populateSuggestivePOTemplatesCache(self):
"""See `IPOTemplateSet`."""
# XXX j.c.sackett 2010-08-30 bug=627631 Once data migration has
=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py 2011-05-27 21:12:25 +0000
+++ lib/lp/translations/model/potmsgset.py 2011-07-26 15:04:52 +0000
@@ -375,7 +375,8 @@
:param suggested_languages: Languages that suggestions should be found
for.
- :param used_languages: Languages that used messages should be found for.
+ :param used_languages: Languages that used messages should be found
+ for.
"""
if not config.rosetta.global_suggestions_enabled:
return []
@@ -399,7 +400,7 @@
used_languages = used_languages - both_languages
lang_used = []
if both_languages:
- lang_used.append('TranslationMessage.language IN %s' %
+ lang_used.append('TranslationMessage.language IN %s' %
quote(both_languages))
if used_languages:
lang_used.append('(TranslationMessage.language IN %s AND %s)' % (
@@ -409,7 +410,7 @@
'(TranslationMessage.language IN %s AND NOT %s)' % (
quote(suggested_languages), in_use_clause))
- pots = SQL('''pots AS (
+ msgsets = SQL('''msgsets AS (
SELECT POTMsgSet.id
FROM POTMsgSet
JOIN TranslationTemplateItem ON
@@ -437,12 +438,13 @@
ids_query = '''
SELECT DISTINCT ON (%(msgstrs)s)
TranslationMessage.id
- FROM TranslationMessage join pots on pots.id=translationmessage.potmsgset
+ FROM TranslationMessage
+ JOIN msgsets ON msgsets.id = TranslationMessage.potmsgset
WHERE %(where)s
ORDER BY %(msgstrs)s, date_created DESC
''' % ids_query_params
- result = IStore(TranslationMessage).with_(pots).find(
+ result = IStore(TranslationMessage).with_(msgsets).find(
TranslationMessage,
TranslationMessage.id.is_in(SQL(ids_query)))
@@ -454,10 +456,11 @@
def getExternallySuggestedTranslationMessages(self, language):
"""See `IPOTMsgSet`."""
- return self._getExternalTranslationMessages(suggested_languages=[language])
+ return self._getExternalTranslationMessages(
+ suggested_languages=[language])
def getExternallySuggestedOrUsedTranslationMessages(self,
- suggested_languages=(), used_languages=()):
+ suggested_languages=(), used_languages=()):
"""See `IPOTMsgSet`."""
# This method exists because suggestions + used == all external
# messages : its better not to do the work twice. We could use a
@@ -465,7 +468,7 @@
# 2000, doing a single pass in python should be insignificantly
# slower.
result_type = namedtuple('SuggestedOrUsed', 'suggested used')
- result = defaultdict(lambda:result_type([], []))
+ result = defaultdict(lambda: result_type([], []))
for message in self._getExternalTranslationMessages(
suggested_languages=suggested_languages,
used_languages=used_languages):
=== modified file 'lib/lp/translations/scripts/tests/test_cache_suggestive_templates.py'
--- lib/lp/translations/scripts/tests/test_cache_suggestive_templates.py 2010-08-20 20:31:18 +0000
+++ lib/lp/translations/scripts/tests/test_cache_suggestive_templates.py 2011-07-26 15:04:52 +0000
@@ -5,7 +5,6 @@
__metaclass__ = type
-import transaction
from zope.component import getUtility
from canonical.launchpad.webapp.interfaces import (
@@ -54,6 +53,32 @@
self.assertEqual([], self._readCache())
+ def test_removeFromSuggestivePOTemplatesCache(self):
+ # It is possible to remove a template from the cache.
+ pot = self.factory.makePOTemplate()
+ self._refreshCache()
+ cache_with_template = self._readCache()
+
+ was_in_cache = self.utility.removeFromSuggestivePOTemplatesCache(pot)
+ cache_without_template = self._readCache()
+
+ self.assertTrue(was_in_cache)
+ self.assertNotEqual(cache_with_template, cache_without_template)
+ self.assertContentEqual(
+ cache_with_template, cache_without_template + [pot.id])
+
+ def test_removeFromSuggestivePOTemplatesCache_not_in_cache(self):
+ # Removing a not-cached template from the cache does nothing.
+ self._refreshCache()
+ cache_before = self._readCache()
+
+ pot = self.factory.makePOTemplate()
+
+ was_in_cache = self.utility.removeFromSuggestivePOTemplatesCache(pot)
+
+ self.assertFalse(was_in_cache)
+ self.assertEqual(cache_before, self._readCache())
+
def test_populateSuggestivePOTemplatesCache(self):
# The populate method fills an empty cache.
self.utility.wipeSuggestivePOTemplatesCache()
@@ -114,7 +139,20 @@
cache_before = self._readCache()
pot = self.factory.makePOTemplate()
- pot.iscurrent = False
+ pot.setActive(False)
self._refreshCache()
self.assertEqual(cache_before, self._readCache())
+
+ def test_disabled_template_is_removed(self):
+ # A disabled template is removed from the cache immediately.
+ pot = self.factory.makePOTemplate()
+ self._refreshCache()
+ cache_with_template = self._readCache()
+
+ pot.setActive(False)
+ cache_without_template = self._readCache()
+
+ self.assertNotEqual(cache_with_template, cache_without_template)
+ self.assertContentEqual(
+ cache_with_template, cache_without_template + [pot.id])
=== modified file 'lib/lp/translations/tests/test_potemplate.py'
--- lib/lp/translations/tests/test_potemplate.py 2011-03-21 09:56:49 +0000
+++ lib/lp/translations/tests/test_potemplate.py 2011-07-26 15:04:52 +0000
@@ -19,6 +19,7 @@
person_logged_in,
TestCaseWithFactory,
)
+from lp.testing.fakemethod import FakeMethod
from lp.translations.interfaces.potemplate import IPOTemplateSet
from lp.translations.interfaces.side import (
TranslationSide,
@@ -36,7 +37,7 @@
def setUp(self):
TestCaseWithFactory.setUp(self)
self.potemplate = removeSecurityProxy(self.factory.makePOTemplate(
- translation_domain = "testdomain"))
+ translation_domain="testdomain"))
def assertIsDummy(self, pofile):
"""Assert that `pofile` is actually a `DummyPOFile`."""
@@ -91,7 +92,7 @@
# Test that getDummyPOFile fails when trying to get a DummyPOFile
# where a POFile already exists for that language.
language = self.factory.makeLanguage('sr@test')
- pofile = self.potemplate.newPOFile(language.code)
+ self.potemplate.newPOFile(language.code)
self.assertRaises(
AssertionError, self.potemplate.getDummyPOFile, language)
@@ -100,7 +101,7 @@
# where a POFile already exists for that language when
# check_for_existing=False is passed in.
language = self.factory.makeLanguage('sr@test')
- pofile = self.potemplate.newPOFile(language.code)
+ self.potemplate.newPOFile(language.code)
# This is just "assertNotRaises".
dummy = self.potemplate.getDummyPOFile(language,
check_for_existing=False)
@@ -161,6 +162,31 @@
sourcepackagename=package.sourcepackagename)
self.assertEqual(package, template.translationtarget)
+ def _toggleIsCurrent(self, states):
+ """Toggle iscurrent according to states and report call count.
+
+ :param states: An array of Boolean values to set iscurrent to.
+ :returns: An array of integers representing the call count for
+ removeFromSuggestivePOTemplatesCache after each toggle.
+ """
+ patched_method = FakeMethod(result=True)
+ self.potemplate._removeFromSuggestivePOTemplatesCache = patched_method
+ call_counts = []
+ for state in states:
+ self.potemplate.setActive(state)
+ call_counts.append(patched_method.call_count)
+ return call_counts
+
+ def test_setActive_detects_negative_edge(self):
+ # SetActive will only trigger suggestive cache removal if the flag
+ # changes from true to false.
+ # Start with a current template.
+ self.assertTrue(self.potemplate.iscurrent)
+ # The toggle sequence, contains two negative edges.
+ self.assertEqual(
+ [0, 1, 1, 1, 2],
+ self._toggleIsCurrent([True, False, False, True, False]))
+
class EquivalenceClassTestMixin:
"""Helper for POTemplate equivalence class tests."""
@@ -242,7 +268,7 @@
productseries=self.trunk, name='foo')
stable_template = self.factory.makePOTemplate(
productseries=self.stable, name='foo')
- other_stable_template = self.factory.makePOTemplate(
+ self.factory.makePOTemplate(
productseries=self.stable, name='foo-other')
templates = set(list(self.subset.getSharingPOTemplates('foo')))
@@ -350,7 +376,7 @@
hoary_template = self.factory.makePOTemplate(
distroseries=self.warty, sourcepackagename=self.package,
name=template_name)
- other_hoary_template = self.factory.makePOTemplate(
+ self.factory.makePOTemplate(
distroseries=self.warty, sourcepackagename=self.package,
name=not_matching_name)
subset = getUtility(IPOTemplateSet).getSharingSubset(
@@ -518,7 +544,7 @@
# Manually creating a productseries to get one that is not the
# translation focus.
other_productseries = self.factory.makeProductSeries()
- other_template = self.factory.makePOTemplate(
+ self.factory.makePOTemplate(
productseries=other_productseries)
product = other_productseries.product
productseries = self.factory.makeProductSeries(
Follow ups
