launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

I have updated code to show an error on the form if an invalid target is chosen. There are some implementation details which need to be considered. 

The form submit action has been updated to catch the BranchTargetError raised if an invalid target is used. After calling setFieldError(), the submit action returns.

Normally, the launchpad form infrastructure expects any field/form errors to be set during validation, before processing the submit action. However, this would require an extra method on the model "canSetTarget" and didn't seem worth the extra code. So what I did was to slightly alter the LaunchpadFormView class to re-check for errors after calling submit and abort if there are any. Note that this was recently added anyway for ajax requests so it is simply a matter of doing the same thing for posts as well. Without these changes, the form would just goto the next_url without setting the target and without showing any error.

The upshot is that form submit handlers can set form errors as they see fit - sometimes errors arise during processing not during validation. Such errors can then be displayed to the user without an oops page, if the view sees fit to catch the errors raised and set the form errors accordingly. So I think the change has wider benefits.

I added a browser test also for the new functionality, coded in the same style as the existing tests in the module.
-- 
https://code.launchpad.net/~wallyworld/launchpad/branch-setTarget-validation/+merge/124691
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.