fix: Postcode validation within MarketingPrefs: Replace simple regex with more robust methods from the postcode library#756
Conversation
…with more robust methods from the postcode library
| is: val => (!(mpValidationOptions.mp_permissionPost.disableOption) | ||
| && mpValidationOptions.mp_permissionPost[val]), | ||
| then: schema => schema.required('Please enter your postcode').matches(/^[a-zA-Z]{1,2}\d[a-zA-Z\d]?\s*\d[a-zA-Z]{2}$/, 'Please enter a valid postcode') | ||
| then: schema => schema.required('Please enter your postcode').test('postcode-valid', 'Please enter a valid postcode', validatePostcode) |
There was a problem hiding this comment.
Thanks for this, makes a lot of sense to update both lookup components!
With Yup this logic will need to be done in two steps; the first is a transform which should apply fixPostcode to string values (and return anything else untouched), the second is the validity test which should be simply postcode.isValid. If we have only the test, Yup will accept the "fixed" value but not return it to us, and we'll get the original un-fixed value passed into the submission payload.
Something else to double-check, that we are actually using the return value from mpValidationSchema.validate(...).
There was a problem hiding this comment.
That's a bloody good spot!
There was a problem hiding this comment.
It appears transformed values are being used correctly.
|
Where did this requirement come from? We don't even use the Postal option in MPs any more (due to changes in policy re: legitimate interest, as per this ticket), so bit confused what context this has been flagged in? |
Ruddy good point - pointless adding this to the MP bit. It's really just the Lookup component that needs it. I'll come back to this soon |
|
Going to close this. It's still of use, this unmerged work, but will be better suited in our grand new validation library. |
|
Mere hours after closing this, Matt Hutson has chased this up and would like it added! Hah. |
Is there a Slack/email chain for this anywhere? |
There is not, not that I've been involved in at least, it's all been verbal in our planning meetings. @matthutson please could you link to a convo, or forward an email request, if you have such a thing with whoever it was you said you were talking to about it. |
|
OK in that case it is definitely the general postcode lookup I need to change rather than the MP specifically. |
|
Reclosing this as it's not needed within MP; it's instead being implemented in Donate here and FSU here |
4 participants
PR description
What is it doing?
The basic regex currently in use is not sufficient. This replaces it with more robust autofix and validation methods from the postcode library.
The same approach is being taken in CRcom here https://github.com/comicrelief/comicrelief-contentful/pull/1775
Why is this required?
We have had contact into Supporter Care, about users mistakenly putting 1 instead of I, or 0 instead of O, and not understanding why they can't submit.
link to Jira ticket:
ENG-4308
Quick Checklist:
My PR title follows the Conventional Commit spec.
I have filled out the PR description as per the template above.
I have added tests to cover new or changed behaviour.
I have updated any relevant documentation.
Important! - lastly, make sure to squash merge...