Launchpad itself

Merge lp:~abentley/launchpad/bad-branch-name into lp:launchpad

  1. bad-branch-name
  2. Merge into devel
Proposed by Aaron Bentley
Status: Merged
Approved by: Māris Fogels
Approved revision: no longer in the source branch.
Merged at revision: 11015
Proposed branch: lp:~abentley/launchpad/bad-branch-name
Merge into: lp:launchpad
Diff against target: 141 lines (+55/-19)
3 files modified
lib/lp/code/browser/sourcepackagerecipe.py (+5/-0)
lib/lp/code/browser/tests/test_sourcepackagerecipe.py (+48/-19)
lib/lp/code/model/sourcepackagerecipedata.py (+2/-0)
To merge this branch: bzr merge lp:~abentley/launchpad/bad-branch-name
Reviewer Review Type Date Requested Status
Māris Fogels (community) Approve
Review via email: mp+27549@code.launchpad.net

Commit message

Treat bad branch location in recipe as field error

Description of the change

= Summary =
Fix bug #592792: bad branch names in recipes produce oopses

== Proposed fix ==
Catch NoSuchBranch when creating the SourcePackageRecipe and set a suitable
field error.

== Pre-implementation notes ==
None

== Implementation details ==
Extracted createRecipe so it could be reused.

Implemented bad branch handling for SourcePackageRecipeDataInstruction.

== Tests ==
bin/test -t test_create_recipe_bad_base_branch -t test_create_recipe_bad_instruction_branch

== Demo and Q/A ==
Attempt to create a recipe for a base branch that doesn't exist. This should
produce a validation error.

Attempt to create a recipe that merges a branchh that doesn't exist. This
should produce a validation error.

= 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/code/model/sourcepackagerecipedata.py
  lib/lp/code/browser/sourcepackagerecipe.py
  lib/lp/code/browser/tests/test_sourcepackagerecipe.py

== Pyflakes notices ==

lib/lp/code/browser/tests/test_sourcepackagerecipe.py
    283: local variable 'recipe_fings' is assigned to but never used

To post a comment you must log in.
Revision history for this message
Māris Fogels (mars) wrote :

Hi Aaron,

This change looks good. Some questions:

  * Would the text "No such branch: foo" be clearer for our users than "Unknown branch: foo"? I personally find the meaning of "No such X" clearer than "Unknown X".

  * I see two convoluted calls to "extract_text(find_tags_by_class(browser.contents, 'message')[1])" in this diff. Should it be extracted into a nice helper function like get_page_message()?

Otherwise, this looks good to me. r=mars

Maris

review: Approve
Revision history for this message
Aaron Bentley (abentley) wrote :

Updated per our discussion.

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-06-12 13:34:11 +0000
3+++ lib/lp/code/browser/sourcepackagerecipe.py 2010-06-15 15:10:56 +0000
4@@ -40,6 +40,7 @@
5 from canonical.widgets.itemswidgets import LabeledMultiCheckBoxWidget
6 from lp.buildmaster.interfaces.buildbase import BuildStatus
7 from lp.code.errors import ForbiddenInstruction
8+from lp.code.interfaces.branch import NoSuchBranch
9 from lp.code.interfaces.sourcepackagerecipe import (
10 ISourcePackageRecipe, ISourcePackageRecipeSource, MINIMAL_RECIPE_TEXT)
11 from lp.code.interfaces.sourcepackagerecipebuild import (
12@@ -364,6 +365,10 @@
13 'recipe_text',
14 'The bzr-builder instruction "run" is not permitted here.')
15 return
16+ except NoSuchBranch, e:
17+ self.setFieldError(
18+ 'recipe_text', '%s is not a branch on Launchpad.' % e.name)
19+ return
20
21 self.next_url = canonical_url(source_package_recipe)
22
23
24=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
25--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-06-12 13:52:31 +0000
26+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-06-15 15:10:56 +0000
27@@ -65,6 +65,12 @@
28 return extract_text(find_main_content(browser.contents))
29
30
31+def get_message_text(browser, index):
32+ """Return the text of a message, specified by index."""
33+ tags = find_tags_by_class(browser.contents, 'message')[index]
34+ return extract_text(tags)
35+
36+
37 class TestSourcePackageRecipeAddView(TestCaseForRecipe):
38
39 layer = DatabaseFunctionalLayer
40@@ -142,7 +148,7 @@
41 browser.getControl('Create Recipe').click()
42
43 self.assertEqual(
44- extract_text(find_tags_by_class(browser.contents, 'message')[1]),
45+ get_message_text(browser, 1),
46 'The bzr-builder instruction "run" is not permitted here.')
47
48 def test_create_new_recipe_empty_name(self):
49@@ -162,30 +168,53 @@
50 browser.getControl('Create Recipe').click()
51
52 self.assertEqual(
53- extract_text(find_tags_by_class(browser.contents, 'message')[1]),
54- 'Required input is missing.')
55+ get_message_text(browser, 1), 'Required input is missing.')
56+
57+ def createRecipe(self, recipe_text, branch=None):
58+ if branch is None:
59+ product = self.factory.makeProduct(
60+ name='ratatouille', displayname='Ratatouille')
61+ branch = self.factory.makeBranch(
62+ owner=self.chef, product=product, name='veggies')
63+
64+ # A new recipe can be created from the branch page.
65+ browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
66+ browser.getLink('Create packaging recipe').click()
67+
68+ browser.getControl(name='field.name').value = 'daily'
69+ browser.getControl('Description').value = 'Make some food!'
70+ browser.getControl('Recipe text').value = recipe_text
71+ browser.getControl('Create Recipe').click()
72+ return browser
73
74 def test_create_recipe_bad_text(self):
75 # If a user tries to create source package recipe with bad text, they
76 # should get an error.
77- product = self.factory.makeProduct(
78- name='ratatouille', displayname='Ratatouille')
79- branch = self.factory.makeBranch(
80- owner=self.chef, product=product, name='veggies')
81-
82- # A new recipe can be created from the branch page.
83- browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
84- browser.getLink('Create packaging recipe').click()
85-
86- browser.getControl(name='field.name').value = 'daily'
87- browser.getControl('Description').value = 'Make some food!'
88- browser.getControl('Recipe text').value = 'Foo bar baz'
89- browser.getControl('Create Recipe').click()
90-
91+ browser = self.createRecipe('Foo bar baz')
92 self.assertEqual(
93- extract_text(find_tags_by_class(browser.contents, 'message')[1]),
94+ get_message_text(browser, 1),
95 'The recipe text is not a valid bzr-builder recipe.')
96
97+ def test_create_recipe_bad_base_branch(self):
98+ # If a user tries to create source package recipe with a bad base
99+ # branch location, they should get an error.
100+ browser = self.createRecipe(MINIMAL_RECIPE_TEXT % 'foo')
101+ self.assertEqual(
102+ get_message_text(browser, 1), 'foo is not a branch on Launchpad.')
103+
104+ def test_create_recipe_bad_instruction_branch(self):
105+ # If a user tries to create source package recipe with a bad
106+ # instruction branch location, they should get an error.
107+ product = self.factory.makeProduct(
108+ name='ratatouille', displayname='Ratatouille')
109+ branch = self.factory.makeBranch(
110+ owner=self.chef, product=product, name='veggies')
111+ recipe = MINIMAL_RECIPE_TEXT % branch.bzr_identity
112+ recipe += 'nest packaging foo debian'
113+ browser = self.createRecipe(recipe, branch)
114+ self.assertEqual(
115+ get_message_text(browser, 1), 'foo is not a branch on Launchpad.')
116+
117 def test_create_dupe_recipe(self):
118 # You shouldn't be able to create a duplicate recipe owned by the same
119 # person with the same name.
120@@ -206,7 +235,7 @@
121 browser.getControl('Create Recipe').click()
122
123 self.assertEqual(
124- extract_text(find_tags_by_class(browser.contents, 'message')[1]),
125+ get_message_text(browser, 1),
126 'There is already a recipe owned by Master Chef with this name.')
127
128
129
130=== modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
131--- lib/lp/code/model/sourcepackagerecipedata.py 2010-06-11 04:28:38 +0000
132+++ lib/lp/code/model/sourcepackagerecipedata.py 2010-06-15 15:10:56 +0000
133@@ -211,6 +211,8 @@
134 line_number += 1
135 comment = None
136 db_branch = branch_map[instruction.recipe_branch.url]
137+ if db_branch is None:
138+ raise NoSuchBranch(instruction.recipe_branch.url)
139 insn = _SourcePackageRecipeDataInstruction(
140 instruction.recipe_branch.name, type, comment,
141 line_number, db_branch, instruction.recipe_branch.revspec,