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

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://staging/~abentley/launchpad/bad-branch-name
Merge into: lp://staging/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://staging/~abentley/launchpad/bad-branch-name
Reviewer Review Type Date Requested Status
Māris Fogels (community) Approve
Review via email: mp+27549@code.staging.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
The diff is not available at this time. You can reload the page or download it.