-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: make MMA aware of new Stripe Membership USA account #1371
Conversation
fd3046c
to
1d4e2b8
Compare
Hey, I'm happy to approve, I just wanted to check, does the response from MDAPI specifically the |
Good question! The delivery address comes from the Salesforce contact record, so perhaps @graham228221 or @JoeMitchellGuardian can advise on this? |
This appears to be using the "Mailing Country" standard field on the Contact in Salesforce. This dataset is controlled by Salesforce and uses the full country name, rather than any abbreviations. In the example provided by @rBangay, it would show "United States" as shown below |
Actually some "legacy" subscribers might have abbreviations as they were never corrected. In Salesforce these are the contacts with active subscriptions:
I think for this piece of work the Billing Address (billToContact?) is more relevant:
|
|
||
test('Uses United States Stripe key for United States delivery address', () => { | ||
const stripePublicKey = getStripeKey( | ||
guardianWeeklySubscriptionUnitedStates.deliveryAddress?.country, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why create the whole guardianWeeklySubscriptionUnitedStates
object only to pick out just the delivery country?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks John - I have refactored here: 28d8365
client/fixtures/subscription.ts
Outdated
year: 2033, | ||
}, | ||
type: 'Visa', | ||
stripePublicKeyForUpdate: 'pk_test_123', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't traced the code fully, but it seems like the public key is coming from MDAPI in some? cases and it would be based off the billing country. https://github.com/guardian/members-data-api/blob/7094e7fd2927c97101fa11c7b29050aa55afd31b/membership-attribute-service/app/services/AccountDetailsFromZuora.scala#L52
If it's all tested with GW and other products and works ok then it's probably ok, I think GW is the most tricky with the billing vs delivery country plus currencies. (we lock the delivery country and currency together IIRC so that people definitely pay the right price)
Thanks @graham228221 and @JoeMitchellGuardian for commenting. Does this mean that we should check for all strings "United States", "UNITED STATES", "US" and "USA" when we pick the correct Stripe account? |
@rBangay @graham228221 @JoeMitchellGuardian FYI I will close this PR since ,following a call with Rupert and John, we are planning to update MDAPI so that manage-frontend won't need to work out the Stripe key for itself and always get it from MDAPI. See related conversation: https://chat.google.com/room/AAAA6HaLTYQ/HD_8XQ2HBK4/FS2h9zgg6_s?cls=10 |
What does this change?
Trello
This adds the new Membership USA Stripe account gateway to MMA.
Other changes not in code
The following S3 files have been updated with the new Stripe Membership USA account keys (all environments):
Why are you doing this?
As part of KD2 for optimising “interchange” (aka “over the Atlantic”) network charges with Stripe (SR Platform - Q2 FY 2024/25 OKR Submission).
How to test
I have deployed this branch to CODE with the current MDAPI main and all worked as before.
Please make some more tests as I am not the most familiar with MMA.
Notes