Merge lp:~sinzui/launchpad/6-plural-forms into lp:launchpad

[Curtis Hovey]

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

[Steve Kowalik (stevenk)]

review: Approve (code)

=== 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:59 +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:59 +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