Launchpad itself

Merge lp:~rockstar/launchpad/bug-589454 into lp:launchpad

  1. bug-589454
  2. Merge into devel
Proposed by Paul Hummer
Status: Merged
Merged at revision: 10972
Proposed branch: lp:~rockstar/launchpad/bug-589454
Merge into: lp:launchpad
Diff against target: 56 lines (+29/-6)
2 files modified
lib/lp/code/browser/sourcepackagerecipe.py (+8/-6)
lib/lp/code/browser/tests/test_sourcepackagerecipe.py (+21/-0)
To merge this branch: bzr merge lp:~rockstar/launchpad/bug-589454
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email: mp+27012@code.launchpad.net

Description of the change

This branch fixes bug #589454. The problem is that LaunchpadFormView._validate gathers errors from widget validation and then continues on to gather errors in LaunchpadFormView.validate. The problem is that if the validate method relies on the widgets being valid, you'll get an oops from the validate method.

Curtis suggested I do something like:

if len(data.errors):
    return

I didn't like that solution, since it's not allowing for other validation that can and should also occur. The way I wrote it, if name isn't set, it shows validation errors, and if name is a duplicate, then it shows that error. This all happens without effecting other validation code.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

We discussed a few style improvements on IRC:
 * Keeping the ISourcePackageRecipeSource utility in a variable to avoid breaking the "if" line in an awkward way.
 * Double-quoting rather than single-quoting stirngs that may contain apostrophes.
 * A single data.get('name') instead of separate existence check and retrieval.

Unfortunately there seems to be no easy way to ask the test browser for the error message associated with the "name" field, so we'll continue to find them by class and then use list indexing.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
2--- lib/lp/code/browser/sourcepackagerecipe.py 2010-05-28 20:54:57 +0000
3+++ lib/lp/code/browser/sourcepackagerecipe.py 2010-06-08 05:56:28 +0000
4@@ -318,12 +318,14 @@
5 'recipe_text',
6 'The recipe text is not a valid bzr-builder recipe.')
7
8- if getUtility(ISourcePackageRecipeSource).exists(
9- self.user, data['name']):
10- self.setFieldError(
11- 'name',
12- 'There is already a recipe owned by %s with this name.' %
13- self.user.displayname)
14+ name = data.get('name', None)
15+ if name:
16+ SourcePackageRecipeSource = getUtility(ISourcePackageRecipeSource)
17+ if SourcePackageRecipeSource.exists(self.user, name):
18+ self.setFieldError(
19+ 'name',
20+ 'There is already a recipe owned by %s with this name.' %
21+ self.user.displayname)
22
23
24 class SourcePackageRecipeAddView(RecipeTextValidatorMixin, LaunchpadFormView):
25
26=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
27--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-06-03 21:13:05 +0000
28+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-06-08 05:56:28 +0000
29@@ -119,6 +119,27 @@
30 self.assertTextMatchesExpressionIgnoreWhitespace(
31 pattern, main_text)
32
33+ def test_create_new_recipe_empty_name(self):
34+ # Leave off the name and make sure that the widgets validate before
35+ # the content validates.
36+ product = self.factory.makeProduct(
37+ name='ratatouille', displayname='Ratatouille')
38+ branch = self.factory.makeBranch(
39+ owner=self.chef, product=product, name='veggies')
40+ self.factory.makeSourcePackage()
41+
42+ # A new recipe can be created from the branch page.
43+ browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
44+ browser.getLink('Create packaging recipe').click()
45+
46+ browser.getControl('Description').value = 'Make some food!'
47+ browser.getControl('Secret Squirrel').click()
48+ browser.getControl('Create Recipe').click()
49+
50+ self.assertEqual(
51+ extract_text(find_tags_by_class(browser.contents, 'message')[1]),
52+ 'Required input is missing.')
53+
54 def test_create_recipe_bad_text(self):
55 # If a user tries to create source package recipe with bad text, they
56 # should get an error.