launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/language-add-admin into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #417021 in Launchpad itself: "+languages/+add form should have form validation to force users to enter plural expressions if plural forms number is given"
  https://bugs.launchpad.net/launchpad/+bug/417021

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/language-add-admin/+merge/61267

+languages/+add must validate both plural_forms and plural_expression.

    Launchpad bug: https://bugs.launchpad.net/bugs/417021
    Pre-implementation: no one

/+languages/+add and /+languages/ace/+admin, will oops if the number of
plural forms is given, but not plural form expressions.
    IntegrityError: new row for relation "language" violates check
    constraint "valid_language"

The form stated that the "Plural form expression" is optional. This is a
documentation error. It is required when providing the number of plural forms.

--------------------------------------------------------------------

RULES

    * The schema requires an invariant method to ensure that both pluralforms
      or pluralexpression are provided, or are None.


QA

    * Visit https://translations.staging.launchpad.net/+languages/ace/+admin
    * Enter 2 for the plural forms and submit
    * Verify the form explains that the plural expression must be
      provided too.


LINT

    lib/lp/services/worlddata/interfaces/language.py
    lib/lp/translations/browser/tests/test_language.py


TEST

    ./bin/test -vv -m lp.translations.browser.tests.test_language


IMPLEMENTATION

Added an invariant to ILanguage to ensure that both pluralforms and
pluralexpression are both provided, or both are None. Add a test to verify
the common case were we saw the oops.
    lib/lp/services/worlddata/interfaces/language.py
    lib/lp/translations/browser/tests/test_language.py
-- 
https://code.launchpad.net/~sinzui/launchpad/language-add-admin/+merge/61267
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/language-add-admin into lp:launchpad.

=== modified file 'lib/lp/services/worlddata/interfaces/language.py'
--- lib/lp/services/worlddata/interfaces/language.py	2011-03-07 06:40:01 +0000
+++ lib/lp/services/worlddata/interfaces/language.py	2011-05-17 15:08:41 +0000
@@ -31,6 +31,8 @@
     Attribute,
     Interface,
     )
+from zope.interface.exceptions import Invalid
+from zope.interface.interface import invariant
 from zope.schema import (
     Bool,
     Choice,
@@ -164,6 +166,14 @@
         required=True,
         readonly=True)
 
+    @invariant
+    def validatePluralData(form_language):
+        pair = (form_language.pluralforms, form_language.pluralexpression)
+        if None in pair and pair != (None, None):
+            raise Invalid(
+                'The number of plural forms and the plural form expression '
+                'must be set together, or not at all.')
+
 
 class ILanguageSet(Interface):
     """The collection of languages.

=== added file 'lib/lp/translations/browser/tests/test_language.py'
--- lib/lp/translations/browser/tests/test_language.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/browser/tests/test_language.py	2011-05-17 15:08:41 +0000
@@ -0,0 +1,45 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the language views."""
+
+__metaclass__ = type
+
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.testing import (
+    login_celebrity,
+    TestCaseWithFactory,
+    )
+from lp.testing.views import create_initialized_view
+
+
+class LanguageAdminViewTestCase(TestCaseWithFactory):
+    """Test language view."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_validatePluralData_invariant_error(self):
+        # Both the number of plural forms and the plural form expression
+        # fields must be provided together, or not at all.
+        language = self.factory.makeLanguage(
+            language_code='qq', name='Queque',
+            pluralforms=None, plural_expression=None)
+        form = {
+            'field.code': 'qq',
+            'field.englishname': 'Queque',
+            'field.nativename': '',
+            'field.pluralforms': '2',
+            'field.pluralexpression': '',
+            'field.visible': True,
+            'field.direction': 'LTR',
+            'field.actions.admin': 'Admin Language',
+            }
+        login_celebrity('admin')
+        view = create_initialized_view(
+             language, '+admin', rootsite='translations', form=form)
+        self.assertEqual(1, len(view.errors), view.errors)
+        self.assertEqual(
+            'The number of plural forms and the plural form expression '
+            'must be set together, or not at all.',
+            view.errors[0])