Merge lp:~stevenk/launchpad/set-recipe-text-bad-data into lp:launchpad
Status: | Merged | ||||||||
---|---|---|---|---|---|---|---|---|---|
Approved by: | Robert Collins | ||||||||
Approved revision: | no longer in the source branch. | ||||||||
Merged at revision: | 12385 | ||||||||
Proposed branch: | lp:~stevenk/launchpad/set-recipe-text-bad-data | ||||||||
Merge into: | lp:launchpad | ||||||||
Diff against target: |
141 lines (+51/-45) 2 files modified
lib/lp/code/browser/sourcepackagerecipe.py (+38/-45) lib/lp/code/browser/tests/test_sourcepackagerecipe.py (+13/-0) |
||||||||
To merge this branch: | bzr merge lp:~stevenk/launchpad/set-recipe-text-bad-data | ||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Robert Collins (community) | Approve | ||
Review via email: mp+48853@code.launchpad.net |
Commit message
[r=lifeless][bug=592513,683321] Don't OOPS if an invalid branch is used while editing a recipe.
Description of the change
Fix bug 683321, 'Editing recipe oopses if invalid branch used'. The real error was that the request_action for creating recipes dealt with four exceptions, but request_action for editing recipes only dealt with three. This means if the branch passed in was invalid, the exception wouldn't be caught and would leak up the stack, causing the OOPS.
I have fixed this by refactoring the exception handler to a parent class of both AddView and EditView, RecipeTextValid
I added a test that checks the error is raised correctly when a recipe is edited and is submitted with a branch that doesn't exist.
Broadly good; I have some tweaks that may help you.
Firstly, a docstring on the method might be nice, its not clear why you only accept *args, for instance.
Secondly to handle the fact that an error occured, you might like to raise a specific exception instead of returning None.
thusly: Exception) :
class ErrorHandled(
"""A field error occured and was handled."""
try: error_handler( ...)
self.
except ErrorHandled:
return
that, to me, would make it clearer than checking for 'is None'. What do you think?