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

Fixing branded checkout donorDetails #1077

Merged
merged 8 commits into from
Jan 16, 2024

Conversation

dr-bizz
Copy link
Contributor

@dr-bizz dr-bizz commented Dec 4, 2023

Description

Helpscout: https://secure.helpscout.net/conversation/2439584263/1061424?folderId=7769710

On Unto a client noticed that when we're passing donorDetails to the branded checkout, the values are not being set.
(You set the donorDetails, by adding a window.donorDetails object with the values you want the branded-checkout to add to the form. I've put an example of how to do this below.)

I tested this on my local. The branded checkout grabbed the window.donorDetails object, but was not using its values. Upon further investigating, I found the <contact-info> component was only using the data from the window.donorDetails object if it was the initial load of the form.

The function loadDonorDetails worked out if it was the initial load by seeing if 2 properties were missing from a request to the cortex API, donorDetails.links.rel and donorDetails.links.donormatchesform.
If rel and donormatchesform were missing, it would use the window.donorDetails data.

My best guess is that over the last 4 years (When this code was first added), there was a change to the API, to always return rel and donormatchesform, resulting in the window.donorDetails data no longer be used.

Example:

window.donorDetails = {
        name: {
            title: '',
            givenName: 'First Name',
            middleInitial: '',
            familyName: 'Last Name',
            suffix: ''
        },
        email: '[email protected]'
}
<branded-checkout
      ...
      donor-details="donorDetails"
    ></branded-checkout>

More info here: https://github.com/CruGlobal/give-web#branded-checkout-config

Changes

As we don't want to undo the work on PR-745 #745, I had to ensure overrideDonorDetails only overwrites the this.donorDetails on the form's first load and after that only overwrites when donorDetails properties are empty.

  • On initial load, use assign to merge the this.donorDetails returned from the API and overrideDonorDetails. Then set session data afterInitialLoad to prevent any edited fields from being overwritten if the component gets rendered again.
  • If session data afterInitialLoad is set and overrideDonorDetails is defined, merge this.donorDetails returned from the API and overrideDonorDetails, favouring this.donorDetails to prevent unwanted overwrites

@dr-bizz dr-bizz requested review from canac and wrandall22 December 4, 2023 17:49
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

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

@wrandall22 might want to look at this also, but it looks good to me. Great writeup about the cause of the error!

@dr-bizz
Copy link
Contributor Author

dr-bizz commented Dec 4, 2023

Yes, I was going to wait until Bill approves it as Give currently has a code-freeze, and I want to ensure no further issues are caused.

@dr-bizz dr-bizz requested a review from wrandall22 December 11, 2023 21:34
@wrandall22
Copy link
Contributor

The only other thing I'm wondering is about users who might go through branded checkout twice in the same browser session. I think before (prior to the EP upgrade), the donormatches link would not exist due to a logout call at the beginning of the process of branded checkout, but in this code, the session storage is not cleared. Maybe we can clear it in the same place that the logout call is made for branded checkout?

@dr-bizz dr-bizz added the On Staging Will be merged to the staging branch by Github Actions label Dec 12, 2023
@dr-bizz
Copy link
Contributor Author

dr-bizz commented Dec 12, 2023

No on the initial signout, we remove the initialLoadComplete session storage

@dr-bizz dr-bizz requested a review from wrandall22 December 12, 2023 18:04
@wrandall22
Copy link
Contributor

At this point I think the code looks good, but waiting to hear from product owner about whether we should wait to deploy until after peak giving.

@dr-bizz
Copy link
Contributor Author

dr-bizz commented Dec 12, 2023

That sounds good.

@wrandall22
Copy link
Contributor

Did you get a chance to test this in staging?

@dr-bizz
Copy link
Contributor Author

dr-bizz commented Dec 12, 2023

Yes, I have a branded checkout on my local, pulling in the staging branded checkout JS. I tested to make sure the names were coming through and that session storage initialLoadComplete gets removed when signout happens on load and added again after the initial load.

@dr-bizz
Copy link
Contributor Author

dr-bizz commented Jan 16, 2024

Checked again in staging before pushing to Prod

@dr-bizz
Copy link
Contributor Author

dr-bizz commented Jan 16, 2024

Updated staging manually

@dr-bizz dr-bizz merged commit 829d0c4 into master Jan 16, 2024
5 of 6 checks passed
@dr-bizz dr-bizz deleted the fix-branded-checkout-defined-donor-details branch January 16, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On Staging Will be merged to the staging branch by Github Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants