comicrelief / giftaid-react

React frontend for Giftaid application
https://giftaid.comicrelief.com/
2 stars 1 forks source link

Add pre and post whitespace to TransactionID regex #420

Closed AndyEPhipps closed 7 months ago

AndyEPhipps commented 7 months ago

https://comicrelief.atlassian.net/browse/ENG-3193

TransactionID field in /update now allows for a trailing and/or leading whitespace in order to handle any copy-paste errors, and strips this out of the submission value itself, rather than messing with the input field value directly (which would bring in its own problems).

NB: this is passed as both transactionID and donationID in the payload; I think the latter is some legacy thing maybe, or possibly used by a third party? Either way, I'm doing the same for both.

Allowing the new spaces: Screenshot 2024-03-27 at 10 27 06

Successful submission, showing the trimmed values: Screenshot 2024-03-27 at 10 29 03

KrupaPammi commented 7 months ago

Hi @AndyEPhipps, I tested this. The submission works when only one space is added after the transaction ID is pasted. Still, when I add two spaces after the transaction ID, I see the error message This transaction ID doesn't seem to be valid, please check your donation confirmation email or letter. Can we update the error message to what Rosie suggested in the ticket 'Please ensure there is no space included at the end of your transaction ID' when more spaces are added.

Screenshot 2024-03-27 at 12 03 04

AndyEPhipps commented 7 months ago

Hi @AndyEPhipps, I tested this. The submission works when only one space is added after the transaction ID is pasted. Still, when I add two spaces after the transaction ID, I see the error message This transaction ID doesn't seem to be valid, please check your donation confirmation email or letter. Can we update the error message to what Rosie suggested in the ticket 'Please ensure there is no space included at the end of your transaction ID' when more spaces are added.

Screenshot 2024-03-27 at 12 03 04

Hi Krupa, yeah I only added 0 or 1 spaces to the regex, I guess it doesn't need to be limited to just 1.

Re: error message, I figured this was probably better UX to just take care of it behind the scenes (rather than having to build an edgecase into messaging); this was what @corinja suggested in planning but I'm easy either way.

corinja commented 7 months ago

Hi Krupa, yeah I only added 0 or 1 spaces to the regex, I guess it doesn't need to be limited to just 1.

Re: error message, I figured this was probably better UX to just take care of it behind the scenes (rather than having to build an edgecase into messaging); this was what @corinja suggested in planning but I'm easy either way.

Yeah, definitely better to strip all leading and trailing whitespace from whatever the input is. Only if that can't work for some reason should we resort to asking the user to do it.

AndyEPhipps commented 7 months ago

I've updated the regex to cover 0+ spaces at the end and/or beginning of the transactionID value now.

I appreciate your attention to detail, @KrupaPammi, but I guess it's worth reflecting that solutions suggested by non-technical stakeholders aren't aaaaalways best? 😇

AndyEPhipps commented 7 months ago

(I should've added the conversation to the ticket anyways, so that's my bad)

KrupaPammi commented 7 months ago

Thanks for the fix @AndyEPhipps, sorry for being over-detailed; we don't want another request from the stakeholders for one space or 2 spaces issue :)

I've updated the regex to cover 0+ spaces at the end and beginning of the transactionID value now.

I appreciate your attention to detail, @KrupaPammi, but I guess it's worth reflecting that solutions suggested by non-technical stakeholders aren't aaaaalways best? 😇

AndyEPhipps commented 7 months ago

No no, all good, @KrupaPammi! You asked me to do what the ticket called for, and I just didn't feed the 'requirements gathering' conversations into it, so you weren't to know. I also totally should've allowed for any number of spaces from the beginning, so that's a win 🥇