[LUPEYALPHA-1109] Bug: Clear the employee-email slug on form failure so it is not bypassed #3253
Conversation
There was a problem hiding this comment.
obviously outside the scope of this PR but think this whole mechanism needs an overhaul
session[:slugs]should go- these arbitrary controller callbacks
- the way we navigate forwards/backwards in journeys
think it would really help prevent quirks like this and make life easier
| def employee_email_after_form_save_failure | ||
| session[:slugs].delete("employee-email") | ||
| render_template_for_current_slug | ||
| end |
There was a problem hiding this comment.
Are we not just missing a validation somewhere?
If the practitioner_email_address is blank, than the claim should not be valid, and the ClaimSubmissionForm should not be valid.
I can't see how to add this validation to just the EY Authenticated journey - so actually, this callback may be the best solution.
There was a problem hiding this comment.
Either way, I think this needs a feature spec in spec/features/early_years_payment/provider/authenticated
There was a problem hiding this comment.
@alkesh it's true that the claim is considered submittable at this point; @kenfodder and I discussed this and the project has been taking the approach of removing validations from the Claim/ClaimSubmissionForm in favour of callbacks like this, but I agree it doesn't feel quite right - @asmega is correct in that the whole mechanism has flaws. I suspect there is a bigger issue where this bug is likely present whenever the second to last page (the last page before a dead-end/results slug) is submitted as invalid as the mechanism will mark that slug as completed regardless of whether it actually saved.
we also discussed whether adding a spec would provide any value and decided against it, but I'm happy to add one to cover the session manipulation behaviour.
There was a problem hiding this comment.
👍🏽 happy with it as it is for now.
My worry about a lack of feature spec, is when we refactor, we re-introduce this bug.
There was a problem hiding this comment.
I've added a feature spec to cover this 🙂
vacabor
deleted the
LUPEYALPHA-1109-fix-bypassable-slug-for-employee-email
branch
This is an edge case but if the user:
Fixes the underlying cause of #3247.