Merge lp:~jtv/launchpad/bug-511097 into lp:launchpad

[Jeroen T. Vermeulen]

Make potemplate admin form check for uniqueness of changed name/domain.

[Jeroen T. Vermeulen (jtv)]

= Bug 511097 =

When a translation template's name or domain is changed, the new name must of course still be available within that package or productseries. There was no explicit check for this, and so the form would bomb out before it even got to a database constraint violation.

The failure was particularly horrific in the case of a name change, where the http(s) request gets redirected to an updated URL (the template's URL contains its name) which does not in fact exist because the name change failed. You wouldn't even get an oops in that case, just a "failed to contact server" message.

This branch builds on the fix for bug 135052 (not yet landed) which adds a validator method for the form in question.

To Q/A, go to the translations page for a source package or productseries with multiple translation templates. "Administer" (shiver) the template, changing its name and/or translation domain to that of one of the other templates. The form should come back with errors shown in red saying that the respective name and/or domain is already in use.

Test:
{{{
./bin/test -vv -t xx-potemplate-admin.txt
}}}

No lint.

Jeroen

[Brad Crittenden (bac)]

Hi Jeroen,

Nice branch and tests. One small suggestion.

Thanks!

--Brad

> === modified file 'lib/lp/translations/stories/standalone/xx-potemplate-admin.txt'
> --- lib/lp/translations/stories/standalone/xx-potemplate-admin.txt 2009-12-11 11:33:39 +0000
> +++ lib/lp/translations/stories/standalone/xx-potemplate-admin.txt 2010-01-26 12:50:30 +0000
> @@ -254,3 +254,67 @@
> >>> group_owner_browser.getControl('Change').click()

> +Input Check
> +-----------
> +
> +The template admin form requires either a productseries or a
> +distroseries.
> +
> + >>> login(ANONYMOUS)
> + >>> product = factory.makeProduct(name='aproduct')
> + >>> template = factory.makePOTemplate(
> + ... productseries=product.getSeries('trunk'))
> + >>> template_url = canonical_url(template)
> + >>> logout()
> +
> + >>> admin_browser.open(template_url + '/+admin')
> +
> + >>> admin_browser.getControl(name='field.distroseries').value = [None]
> + >>> admin_browser.getControl(name='field.productseries').value = ''
> + >>> admin_browser.getControl('Change').click()
> + >>> print_errors(admin_browser.contents)
> + There is 1 error.
> + Choose either a distribution release series or a project release series.
> +
> +It can't be both.
> +
> + >>> admin_browser.open(template_url + '/+admin')
> +
> + >>> admin_browser.getControl(name='field.distroseries').value = [
> + ... 'ubuntu/hoary']
> + >>> print admin_browser.getControl(name='field.productseries').value
> + aproduct/trunk

Rather than demonstrating that a value is already there it would be
easier to follow if you just set it, as above.

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

Brad,

Once again, your suggestion implemented with gratitude.

Jeroen

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py	2010-01-21 15:57:29 +0000
+++ lib/lp/translations/browser/potemplate.py	2010-02-01 19:29:25 +0000
@@ -556,9 +556,33 @@
     label = 'Administer translation template'
     page_title = "Administer"
 
+    def validateName(self, name, similar_templates):
+        """Form submission changes template name.  Validate it."""
+        if name == self.context.name:
+            # Not changed.
+            return
+
+        if similar_templates.getPOTemplateByName(name) is not None:
+            self.setFieldError('name', "Name is already in use.")
+            return
+
+    def validateDomain(self, domain, similar_templates):
+        if domain == self.context.translation_domain:
+            # Not changed.
+            return
+
+        other_template = similar_templates.getPOTemplateByTranslationDomain(
+            domain)
+        if other_template is not None:
+            self.setFieldError(
+                'translation_domain', "Domain is already in use.")
+            return
+
     def validate(self, data):
-        distroseries = data['distroseries']
-        productseries = data['productseries']
+        distroseries = data.get('distroseries')
+        sourcepackagename = data.get('sourcepackagename')
+        productseries = data.get('productseries')
+
         if distroseries is not None and productseries is not None:
             message = ("Choose a distribution release series or a project "
                 "release series, but not both.")
@@ -570,6 +594,19 @@
 
         if message is not None:
             self.addError(message)
+            return
+
+        # Validate name and domain; they should be unique within the
+        # template's translation target (productseries or source
+        # package).  Validate against the target selected by the form,
+        # not the template's existing target; the form may change the
+        # template's target as well.
+        similar_templates = getUtility(IPOTemplateSet).getSubset(
+            distroseries=distroseries, sourcepackagename=sourcepackagename,
+            productseries=productseries)
+
+        self.validateName(data.get('name'), similar_templates)
+        self.validateDomain(data.get('translation_domain'), similar_templates)
 
 
 class POTemplateExportView(BaseExportView):

=== modified file 'lib/lp/translations/stories/standalone/xx-potemplate-admin.txt'
--- lib/lp/translations/stories/standalone/xx-potemplate-admin.txt	2010-01-21 15:57:29 +0000
+++ lib/lp/translations/stories/standalone/xx-potemplate-admin.txt	2010-02-01 19:29:25 +0000
@@ -282,12 +282,39 @@
 
     >>> admin_browser.getControl(name='field.distroseries').value = [
     ...     'ubuntu/hoary']
-    >>> print admin_browser.getControl(name='field.productseries').value
-    aproduct/trunk
+    >>> admin_browser.getControl(name='field.productseries').value = (
+    ...     'aproduct/trunk')
     >>> admin_browser.getControl('Change').click()
     >>> print_errors(admin_browser.contents)
     There is 1 error.
     Choose a distribution release series or a project release series,
     but not both.
 
-
+Also, the form won't allow you to set a name that duplicates that of
+another template.
+
+    >>> login(ANONYMOUS)
+    >>> other_template = factory.makePOTemplate(
+    ...     productseries=template.productseries)
+    >>> other_name = other_template.name
+    >>> other_domain = other_template.translation_domain
+    >>> logout()
+
+    >>> admin_browser.open(template_url + '/+admin')
+    >>> admin_browser.getControl(name='field.name').value = other_name
+    >>> admin_browser.getControl('Change').click()
+    >>> print_errors(admin_browser.contents)
+    There is 1 error.
+    Template name: Name is already in use.
+    ...
+
+Similar for the translation domain.
+
+    >>> admin_browser.open(template_url + '/+admin')
+    >>> admin_browser.getControl(name='field.translation_domain').value = (
+    ...     other_domain)
+    >>> admin_browser.getControl('Change').click()
+    >>> print_errors(admin_browser.contents)
+    There is 1 error.
+    Translation domain: Domain is already in use.
+    ...