Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENG-3255: 'Update' form updates - remove transactionID #427

Merged
merged 24 commits into from
Jun 24, 2024

Conversation

AndyEPhipps
Copy link
Contributor

@AndyEPhipps AndyEPhipps commented Jun 17, 2024

Updates purely on the /update form; as per https://comicrelief.atlassian.net/browse/ENG-3255

  • Remove transaction ID field, validation and any other config that relates to it

  • Update and bring in improved PCLU that clarifies the UK-centricity of the postcode validation

  • make Email Address mandatory

  • Add a new optional Mobile Number field, but with copy to stress that if the donor made their donation via SMS we need it - NB they cannot donate via a non-UK mobile number

@AndyEPhipps AndyEPhipps changed the title ENG-2775 mk 2 ENG-3255: 'Update' updates - remove transactionID, add mobile field, make email mandatory Jun 17, 2024
@AndyEPhipps AndyEPhipps self-assigned this Jun 17, 2024
@AndyEPhipps AndyEPhipps marked this pull request as draft June 17, 2024 13:56
@AndyEPhipps AndyEPhipps marked this pull request as ready for review June 18, 2024 10:39
@AndyEPhipps
Copy link
Contributor Author

AndyEPhipps commented Jun 18, 2024

@KrupaPammi, additional bit of a context from the main ticket:

Andy Phipps:
While we’re removing the “manual” field from the user flow, do we want to do away with EVERYTHING transactionID related?

We currently have functionality to pass a transactionID to the application as a URL param (presumably dynamically set in some email template somewhere) - are we sacking this all off?

Corin Ashwell:
yep, get rid of it completely.

Andy Phipps:
Sorry, one other thing; there’s also functionality in the /update form using the URL param as a trigger to then ask the user how they made the donation (SMS, Online, Call centre). Not sure why it’s only in this specific context but 🤷

Seems like a step backwards to remove this data source completely; probs best to leave it in for all update journeys aye?

Corin Ashwell:
In the old days this form was principally so that we could send letters to call centre donors after NOTV and if they needed to amend their Gift Aid status they could use this page. I am guessing the type of donation was so that we could use that to match to their donation data in SSV. maybe.

If you can leave it in without any trouble maybe that’s a good idea, but all the backend will do now is collect the user details and pass those into ERP/CRM/data lake/wherever

So basically, we show the donationType checkboxes for all /Update journeys now, and I've updated the tests to reflect that.

@AndyEPhipps AndyEPhipps changed the title ENG-3255: 'Update' updates - remove transactionID, add mobile field, make email mandatory ENG-3255: 'Update' form updates - remove transactionID Jun 18, 2024
@AndyEPhipps
Copy link
Contributor Author

AndyEPhipps commented Jun 18, 2024

Agreed that for the Gift Aid Update form we will:

  • remove Transaction ID field completely;

  • make Email Address mandatory;

  • make Mobile Number an optional field but with copy to stress that if the donor made their donation via SMS we need it - NB they cannot donate via a non-UK mobile number

  • improve copy on postal address lookup so that overseas addresses are easy to understand

Doh, sorry - realised I missed a few other bits from the original PR - will re-assign once I'm done.

@AndyEPhipps AndyEPhipps marked this pull request as draft June 18, 2024 10:47
@AndyEPhipps AndyEPhipps marked this pull request as ready for review June 19, 2024 16:31
@AndyEPhipps
Copy link
Contributor Author

AndyEPhipps commented Jun 19, 2024

Sorry for the big ugly diff to review, gang.

It's worth noting that the donationType radiobutton up top:

Screenshot 2024-06-19 at 17 38 07

... used to be only shown when the transactionID was passed as a URL param (from a personalised email). Rather than 100% removing this data source along with the transactionID itself, Corin and I elected that we'd best keep that radiobutton in for all /update contexts, useful data and all.

Testable here: https://deploy-preview-427--giftaid-react.netlify.app/update

@AndyEPhipps
Copy link
Contributor Author

@KrupaPammi if you're able to prioritise this this morning, I'd appreciate it - we're having to sync the B/E and F/E deploys, and have 3pm pencilled-in to do so.

@KrupaPammi
Copy link
Contributor

@AndyEPhipps sure, will do it now.

@@ -6,22 +6,22 @@ const { v4: uuidv4 } = require('uuid');
const Chance = require('chance');
const chance = new Chance();

test.describe.only('Giftaid Update form validation @sanity @nightly-sanity', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops 😬

@KrupaPammi
Copy link
Contributor

Hi @AndyEPhipps, I just finished testing this, works as expected. I've updated a couple of tests.

@AndyEPhipps
Copy link
Contributor Author

Thanks very much @KrupaPammi; appreciate the speediness!

@AndyEPhipps AndyEPhipps merged commit 9ffa4c0 into master Jun 24, 2024
7 checks passed
@AndyEPhipps AndyEPhipps deleted the ENG-2775_remove_transaction_ID_take_2 branch June 24, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants