launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03657
[Merge] lp:~sinzui/launchpad/6-plural-forms into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/6-plural-forms into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/6-plural-forms/+merge/61322
Do not raise an AssertionError when the MAX_PLURAL_FORMS is honoured.
Launchpad bug: https://bugs.launchpad.net/bugs/708875
Pre-implementation: no one
OOPS-1853D2494 shows that a user submitted 6 translations, each field indexed
from 0 to 5, but an assert error was raised stating that more than 6
translations were submitted.
JTV wrote:
The check does something like…
for pluralform in xrange(6):
# …
if pluralform not in translations_submitted_in_form:
break
# …
else:
raise AssertionError("More than 6 plural forms were submitted!")
That goes wrong if you submit exactly 6 plural forms. The loop then never hits
a plural-form number that didn't have a translation submitted, and drops into
the "else" part.
The loop requires that 'break' be called during when an empty translation
is found while iterating over exactly 6 items. It is impossible to submit
6 translations! The assertion exists to ensure that the generated form
fields match the configured number of possible translations.
--------------------------------------------------------------------
RULES
* Since the assertion is predicated on the existence of a 7th field with
the index of 6, a guard can check for the existence of the field before
there is an attempt to store the submitted data.
QA
* Visit https://translations.qastaging.launchpad.net/wubi/trunk/+pots/wubi/ar/5/+translate
* Submit 6 translations/suggestions
* Verify the form does not oops.
LINT
lib/lp/translations/browser/translationmessage.py
lib/lp/translations/browser/tests/test_translationmessage_view.py
TEST
./bin/test -vv -t TestCurrentTranslationMessagePageView.
IMPLEMENTATION
Spent hours constructing a test to instrument the base view to reproduce the
the error. Moved the assertion before the loop to store the translations
since the work will be lost if there is an error. Lint reported:
1123: local variable 'translation' is assigned to but never used
Which reveals an if-statement and assignment have no purpose. I checked that
vars() and locals() were not used.
lib/lp/translations/browser/translationmessage.py
lib/lp/translations/browser/tests/test_translationmessage_view.py
--
https://code.launchpad.net/~sinzui/launchpad/6-plural-forms/+merge/61322
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/6-plural-forms into lp:launchpad.
=== modified file 'lib/lp/translations/browser/tests/test_translationmessage_view.py'
--- lib/lp/translations/browser/tests/test_translationmessage_view.py 2010-12-10 12:14:52 +0000
+++ lib/lp/translations/browser/tests/test_translationmessage_view.py 2011-05-17 22:09:25 +0000
@@ -304,9 +304,26 @@
layer = ZopelessDatabaseLayer
- def _makeView(self):
- message = self.factory.makeCurrentTranslationMessage()
- request = LaunchpadTestRequest()
+ def _makeView(self, with_form=False):
+ potemplate = self.factory.makePOTemplate()
+ pofile = self.factory.makePOFile(potemplate=potemplate)
+ potmsgset = self.factory.makePOTMsgSet(potemplate)
+ message = self.factory.makeCurrentTranslationMessage(
+ pofile=pofile, potmsgset=potmsgset)
+ message.browser_pofile = pofile
+ form = {}
+ if with_form:
+ msgset_id_field = 'msgset_%d' % potmsgset.id
+ form[msgset_id_field] = u'poit'
+ base_field_name = 'msgset_%d_%s_translation_' % (
+ message.potmsgset.id, pofile.language.code)
+ # Add the expected plural forms fields.
+ for plural_form in xrange(TranslationConstants.MAX_PLURAL_FORMS):
+ field_name = '%s%d_new' % (base_field_name, plural_form)
+ form[field_name] = u'snarf'
+ url = '/%s/%s/%s/+translate' % (
+ canonical_url(potemplate), pofile.language.code, 1)
+ request = LaunchpadTestRequest(SERVER_URL=url, form=form)
view = CurrentTranslationMessagePageView(message, request)
view.lock_timestamp = datetime.now(pytz.utc)
return view
@@ -353,6 +370,27 @@
view = self._makeView()
self.assertRaises(UnexpectedFormData, view._checkSubmitConditions)
+ def test_max_plural_forms_fields_accepted(self):
+ # The plural forms translation fields are accepted if there are less
+ # than or equal to TranslationConstants.MAX_PLURAL_FORM.
+ view = self._makeView(with_form=True)
+ view.initialize()
+ self.assertEqual(
+ None, view._extractFormPostedTranslations(view.context.potmsgset))
+
+ def test_max_plural_forms_fields_greater_error(self):
+ # An AssertionError is raised if the number of plural forms
+ # translation fields exceed TranslationConstants.MAX_PLURAL_FORMS.
+ view = self._makeView(with_form=True)
+ view.initialize()
+ # Add a field that is greater than the expected MAX_PLURAL_FORMS.
+ field_name = 'msgset_%d_%s_translation_%d_new' % (
+ view.context.potmsgset.id, view.pofile.language.code,
+ TranslationConstants.MAX_PLURAL_FORMS)
+ view.request.form[field_name] = u'snarf'
+ self.assertRaises(AssertionError,
+ view._extractFormPostedTranslations, view.context.potmsgset)
+
class TestHelpers(TestCaseWithFactory):
=== modified file 'lib/lp/translations/browser/translationmessage.py'
--- lib/lp/translations/browser/translationmessage.py 2011-03-30 11:19:30 +0000
+++ lib/lp/translations/browser/translationmessage.py 2011-05-17 22:09:25 +0000
@@ -131,7 +131,6 @@
"""
schema, netloc, path, parameters, query, fragment = (
urlparse(str(request.URL)))
-
# For safety, delete the start and batch variables, if they
# appear in the URL. The situation in which 'start' appears
# today is when the alternative language form is posted back and
@@ -336,7 +335,6 @@
UTC = pytz.timezone('UTC')
self.lock_timestamp = datetime.datetime.now(UTC)
-
# The batch navigator needs to be initialized early, before
# _submitTranslations is called; the reason for this is that
# _submitTranslations, in the case of no errors, redirects to
@@ -746,6 +744,16 @@
msgset_ID_LANGCODE_translation_ = 'msgset_%d_%s_translation_' % (
potmsgset_ID, language_code)
+ msgset_ID_LANGCODE_translation_GREATER_PLURALFORM_new = '%s%d_new' % (
+ msgset_ID_LANGCODE_translation_,
+ TranslationConstants.MAX_PLURAL_FORMS)
+ if msgset_ID_LANGCODE_translation_GREATER_PLURALFORM_new in form:
+ # The plural form translation generation rules created too many
+ # fields, or the form was hacked.
+ raise AssertionError(
+ 'More than %d plural forms were submitted!'
+ % TranslationConstants.MAX_PLURAL_FORMS)
+
# Extract the translations from the form, and store them in
# self.form_posted_translations. We try plural forms in turn,
# starting at 0.
@@ -822,9 +830,6 @@
if store:
self.form_posted_translations_has_store_flag[
potmsgset].append(pluralform)
- else:
- raise AssertionError('More than %d plural forms were submitted!'
- % TranslationConstants.MAX_PLURAL_FORMS)
def _observeTranslationUpdate(self, potmsgset):
"""Observe that a translation was updated for the potmsgset.
@@ -1109,12 +1114,6 @@
other_translation = self.getOtherTranslation(index)
shared_translation = self.getSharedTranslation(index)
submitted_translation = self.getSubmittedTranslation(index)
- if (submitted_translation is None and
- self.user_is_official_translator):
- # We don't have anything to show as the submitted translation
- # and the user is the official one. We prefill the 'New
- # translation' field with the current translation.
- translation = current_translation
is_multi_line = (count_lines(current_translation) > 1 or
count_lines(submitted_translation) > 1 or
count_lines(self.singular_text) > 1 or
