Merge lp:~adiroiban/launchpad/bug-61081 into lp:launchpad

[Adi Roiban]

Check that on potemplate updates, the priority value is in a certain range.

= Bug 61081 =

Entering a big priority value in POTemplate +edit pages generates and OOPS

== Proposed fix ==

Validate priority value to be in a certain range (0..100000 suggested by Danilo).

== Pre-implementation notes ==

15:16 danilos : adiroiban, fyi, on staging: http://paste.ubuntu.com/419817/
15:19 adiroiban : danilos: shoud I use those limits? Maybe -100 ... 5000000 ?
15:20 danilos : adiroiban, there is only one potemplate (pyroom) that has this priority, all the others are lower than 10k
15:20 danilos : adiroiban, as for negative ones, that sounds wrong and we should probably just reset them
15:21 danilos : adiroiban, 6 templates have negative priorities
15:23 adiroiban : danilos: what values do you suggest for the priority range?
15:23 danilos : adiroiban, I guess 0-100000 (i.e. 10x10000 to leave some room there)
15:24 adiroiban : danilos: ok. we can reset the values... but I suppose that user will be warned when the want to update
                   again the priority and will have to fix it
15:26 adiroiban : danilos: is there anything on this bug that is worth mentioning or it is ready to code or I should just
                   change the range, write tests and create MP ?
15:26 danilos : adiroiban, yeah, we can notify them as well
15:26 danilos : adiroiban, no, it should be ready to code; I'll add a comment to the bug explaining my "findings"

== Implementation details ==

I ried writing a LaunchpadZopelessLayer test but I was not able to get a proper view. I have no idea why POTemplateEditView is giving this error.

potemplate = factory.makePOTemplate()
view = POTemplateEditView(potemplate, LaunchpadTestRequest())
view.initialize()
ComponentLookupError: ((<zope.schema._field.Choice object at 0xd9ae5ec>, <canonical.launchpad.webapp.servers.LaunchpadTestRequest instance URL=http://127.0.0.1>), <InterfaceClass zope.app.form.interfaces.IInputWidget>, u'')

Also, any hints regarding the 'make lint' import errors is much appreciated.

== Tests ==

lp-tt potemplate-edit

== Demo and Q/A ==
Login as admin and go to:
1. http://launchpad.dev/products/evolution/trunk/+pots/evolution-2.2/+edit
2. Fill in 'Priority': 999999999999 or -21
3. Click 'Change'
4. You should see an error message telling you that the priority value should be in a specific range.

Same steps for
http://launchpad.dev/products/evolution/trunk/+pots/evolution-2.2/+admin

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/translations/browser/potemplate.py
  lib/lp/translations/stories/standalone/xx-potemplate-edit.txt

== Pylint notices ==

lib/lp/translations/browser/potemplate.py
    37: [F0401] Unable to import 'canonical.lazr.utils' (No module named canonical.lazr.utils)
    39: [F0401] Unable to import 'canonical.launchpad' (No module named canonical.launchpad)
    40: [F0401] Unable to import 'canonical.launchpad.webapp.breadcrumb' (No module named canonical.launchpad.webapp.breadcrumb)
    41: [F0401] Unable to import 'canonical.launchpad.webapp.interfaces' (No module named canonical.launchpad.webapp.interfaces)
    42: [F0401] Unable to import 'lp.translations.browser.poexportrequest' (No module named lp.translations.browser.poexportrequest)
    43: [F0401] Unable to import 'lp.registry.browser.productseries' (No module named lp.registry.browser.productseries)
    44: [F0401] Unable to import 'lp.translations.browser.translations' (No module named lp.translations.browser.translations)
    45: [F0401] Unable to import 'lp.registry.browser.sourcepackage' (No module named lp.registry.browser.sourcepackage)
    46: [F0401] Unable to import 'lp.registry.interfaces.productseries' (No module named lp.registry.interfaces.productseries)
    47: [F0401] Unable to import 'lp.registry.interfaces.sourcepackage' (No module named lp.registry.interfaces.sourcepackage)
    48: [F0401] Unable to import 'lp.translations.interfaces.pofile' (No module named lp.translations.interfaces.pofile)
    49: [F0401] Unable to import 'lp.translations.interfaces.potemplate' (No module named lp.translations.interfaces.potemplate)
    53: [F0401] Unable to import 'lp.translations.interfaces.translationimporter' (No module named lp.translations.interfaces.translationimporter)
    55: [F0401] Unable to import 'lp.translations.interfaces.translationimportqueue' (No module named lp.translations.interfaces.translationimportqueue)
    57: [F0401] Unable to import 'canonical.launchpad.webapp' (No module named canonical.launchpad.webapp)
    61: [F0401] Unable to import 'canonical.launchpad.webapp.authorization' (No module named canonical.launchpad.webapp.authorization)
    62: [F0401] Unable to import 'canonical.launchpad.webapp.interfaces' (No module named canonical.launchpad.webapp.interfaces)
    63: [F0401] Unable to import 'canonical.launchpad.webapp.launchpadform' (No module named canonical.launchpad.webapp.launchpadform)
    64: [F0401] Unable to import 'canonical.launchpad.webapp.menu' (No module named canonical.launchpad.webapp.menu)
    245: [F0401, POTemplateView.pofiles] Unable to import 'lp.translations.browser.pofile' (No module named lp.translations.browser.pofile)
    594: [E1002, POTemplateAdminView.validate] Use super on an old style class

[Michael Nelson (michael.nelson)]

> == Implementation details ==
>
> I ried writing a LaunchpadZopelessLayer test but I was not able to get a proper view. I have no idea why POTemplateEditView is giving this error.
>
> potemplate = factory.makePOTemplate()
> view = POTemplateEditView(potemplate, LaunchpadTestRequest())
> view.initialize()
> ComponentLookupError: ((<zope.schema._field.Choice object at 0xd9ae5ec>, <canonical.launchpad.webapp.servers.LaunchpadTestRequest instance URL=http://127.0.0.1>), <InterfaceClass zope.app.form.interfaces.IInputWidget>, u'')
>
> Also, any hints regarding the 'make lint' import errors is much appreciated.

Grepping for F0401 in the code shows that it's disabled for many files - I assume it's safe to do so here too. Not sure about the component lookup error though - reproduced it in a harness, but didn't spend too much time investigating since you've written the test anyway.

>
> == Tests ==
>
> lp-tt potemplate-edit
>
> == Demo and Q/A ==
> Login as admin and go to:
> 1. http://launchpad.dev/products/evolution/trunk/+pots/evolution-2.2/+edit

ah, missed the translations.launchpad... got it now :)

> 2. Fill in 'Priority': 999999999999 or -21
> 3. Click 'Change'
> 4. You should see an error message telling you that the priority value should be in a specific range.
>
> Same steps for
> http://launchpad.dev/products/evolution/trunk/+pots/evolution-2.2/+admin

Great, my only suggestion would be a minor modification to the error message, see below.
> self.addError(
> "Priority value must be between %s and %s" % (

Just to turn it into a sentence, add 'The' and '.', eg "The priority value must be between %s and %s."

Thanks!

review: Approve (code)

[Michael Nelson (michael.nelson)]

Oh, and from a UI-point-of-view, I think you need to add an error for the field itself, rather than for the Launchpad form, so that the error is displayed in context, rather than at the top of the screen.

review: Needs Fixing (ui)

[Adi Roiban (adiroiban)]

Hi and many thanks for the review!

I have added a field error and change the message into a sentence.

Here is the latest diff:

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py 2010-04-21 19:02:19 +0000
+++ lib/lp/translations/browser/potemplate.py 2010-04-22 16:58:50 +0000
@@ -1,5 +1,6 @@
 # Copyright 2009 Canonical Ltd. This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
+# pylint: disable-msg=F0401

 """Browser code for PO templates."""

@@ -547,8 +548,9 @@

         if (priority < self.PRIORITY_MIN_VALUE or
             priority > self.PRIORITY_MAX_VALUE):
- self.addError(
- "Priority value must be between %s and %s" % (
+ self.setFieldError(
+ 'priority',
+ 'The priority value must be between %s and %s.' % (
                 self.PRIORITY_MIN_VALUE, self.PRIORITY_MAX_VALUE))
             return

=== modified file 'lib/lp/translations/stories/standalone/xx-potemplate-edit.txt'
--- lib/lp/translations/stories/standalone/xx-potemplate-edit.txt 2010-04-21 19:02:19 +0000
+++ lib/lp/translations/stories/standalone/xx-potemplate-edit.txt 2010-04-22 17:03:35 +0000
@@ -145,8 +145,8 @@
 Priority range
 --------------

-Priority value must be between 0 and 100000. When entering a priority that is
-not in this range the form validation will inform users about what values
+The priority value must be between 0 and 100000. When entering a priority that
+is not in this range the form validation will inform users about what values
 are accepted for the priority field.

   >>> admin_browser.open(
@@ -157,9 +157,9 @@
   >>> print browser.url
   http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/+edit

- >>> print_errors(admin_browser.contents)
+ >>> print_feedback_messages(admin_browser.contents)
   There is 1 error.
- Priority value must be between ...
+ The priority value must be between ...

   >>> admin_browser.open(
   ... 'http://translations.launchpad.dev/evolution/trunk/+pots/'
@@ -169,9 +169,9 @@
   >>> print browser.url
   http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/+edit

- >>> print_errors(admin_browser.contents)
+ >>> print_feedback_messages(admin_browser.contents)
   There is 1 error.
- Priority value must be between ...
+ The priority value must be between ...

 Change and cancel actions
 -------------------------

[Michael Nelson (michael.nelson)]

Great, thanks Adi.

review: Approve (code ui)

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py	2010-03-15 22:52:49 +0000
+++ lib/lp/translations/browser/potemplate.py	2010-04-22 17:11:01 +0000
@@ -1,5 +1,6 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
+# pylint: disable-msg=F0401
 
 """Browser code for PO templates."""
 
@@ -519,6 +520,8 @@
         'path', 'owner', 'iscurrent']
     label = 'Edit translation template details'
     page_title = 'Edit details'
+    PRIORITY_MIN_VALUE = 0
+    PRIORITY_MAX_VALUE = 100000
 
     @action(_('Change'), name='change')
     def change_action(self, action, data):
@@ -538,6 +541,19 @@
             naked_context = removeSecurityProxy(context)
             naked_context.date_last_updated = datetime.datetime.now(pytz.UTC)
 
+    def validate(self, data):
+        priority = data.get('priority')
+        if priority is None:
+            return
+
+        if (priority < self.PRIORITY_MIN_VALUE or
+            priority > self.PRIORITY_MAX_VALUE):
+            self.setFieldError(
+                'priority',
+                'The priority value must be between %s and %s.' % (
+                self.PRIORITY_MIN_VALUE, self.PRIORITY_MAX_VALUE))
+            return
+
     @property
     def _return_attribute_name(self):
         """See 'ReturnToReferrerMixin'."""
@@ -578,6 +594,7 @@
             return
 
     def validate(self, data):
+        super(POTemplateAdminView, self).validate(data)
         distroseries = data.get('distroseries')
         sourcepackagename = data.get('sourcepackagename')
         productseries = data.get('productseries')

=== modified file 'lib/lp/translations/stories/standalone/xx-potemplate-edit.txt'
--- lib/lp/translations/stories/standalone/xx-potemplate-edit.txt	2010-02-16 12:15:12 +0000
+++ lib/lp/translations/stories/standalone/xx-potemplate-edit.txt	2010-04-22 17:11:01 +0000
@@ -97,7 +97,6 @@
 after updating the template.
 
   >>> from zope.component import getUtility
-  >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
   >>> from lp.translations.model.potemplate import POTemplateSubset
   >>> from lp.registry.interfaces.product import IProductSet
   >>> login('[e-mail]')
@@ -143,6 +142,37 @@
   True
 
 
+Priority range
+--------------
+
+The priority value must be between 0 and 100000. When entering a priority that
+is not in this range the form validation will inform users about what values
+are accepted for the priority field.
+
+  >>> admin_browser.open(
+  ...     'http://translations.launchpad.dev/evolution/trunk/+pots/'
+  ...     'evolution-2.2/+edit')
+  >>> admin_browser.getControl(name='field.priority').value = '-1'
+  >>> admin_browser.getControl('Change').click()
+  >>> print browser.url
+  http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/+edit
+
+  >>> print_feedback_messages(admin_browser.contents)
+  There is 1 error.
+  The priority value must be between ...
+
+  >>> admin_browser.open(
+  ...     'http://translations.launchpad.dev/evolution/trunk/+pots/'
+  ...     'evolution-2.2/+edit')
+  >>> admin_browser.getControl(name='field.priority').value = '100001'
+  >>> admin_browser.getControl('Change').click()
+  >>> print browser.url
+  http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/+edit
+
+  >>> print_feedback_messages(admin_browser.contents)
+  There is 1 error.
+  The priority value must be between ...
+
 Change and cancel actions
 -------------------------