Skip to content

[stripe] Create customers with attributionId #12752

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

Merged
merged 1 commit into from
Sep 8, 2022
Merged

Conversation

easyCZ
Copy link
Member

@easyCZ easyCZ commented Sep 8, 2022

Description

Create customers (user/team) with an AttributionID instead of user/team ID. We cannot yet update the find queries to use this field as we have customers in Stripe without the attributionID property.

Once this has propagated enough, we can also kill the user/team specific methods and use the Attribution ID only.

Related Issue(s)

Fixes #

How to test

Release Notes

NONE

Documentation

Werft options:

  • /werft with-preview

@easyCZ easyCZ requested a review from a team September 8, 2022 07:01
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Sep 8, 2022
@andrew-farries andrew-farries self-assigned this Sep 8, 2022
@easyCZ
Copy link
Member Author

easyCZ commented Sep 8, 2022

/werft run

👍 started the job as gitpod-build-mp-stripe-attribution-id.1
(with .werft/ from main)

Comment on lines +64 to 66
// userId is deprecated, use attributionId where possible
userId: user.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think the only place where this userId metadata is used is a few lines above in findCustomerByUserId.

If you also update findCustomerByUserId to use the attributionId, then you can completely remove the now useless userId metadata. 👍

Suggested change
// userId is deprecated, use attributionId where possible
userId: user.id,

Comment on lines +90 to 92
// teamId is deprecated, use attributionId where possible
teamId: team.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Same as above -- the only place where the teamId metadata is used is in findCustomerByTeamId above. If you update this method, you can remove the now useless teamId metadata.

Suggested change
// teamId is deprecated, use attributionId where possible
teamId: team.id,

Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot yet update the find queries to use this field as we have customers in Stripe without the attributionID property.

We cannot remove it yet, lookup queries would fail on existing customers.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason it stays on the create is consistency, we want to phase out teamId, but that doesn't mean we can stop setting it, due to existing queries.

@easyCZ
Copy link
Member Author

easyCZ commented Sep 8, 2022

/hold

@jankeromnes
Copy link
Contributor

jankeromnes commented Sep 8, 2022

We cannot yet update the find queries to use this field as we have customers in Stripe without the attributionID property.

Actually yes, we can. The updated find queries may no longer find existing Stripe customers, and thus conclude that your team/user is not a customer yet. This is totally safe and allows you to create a new customer.

Also, in production, there is currently one single customer. I'll even go ahead and add the attributionId metadata to it, so that it can still be found by the new find queries.

EDIT: Done ✅

Screenshot 2022-09-08 at 09 19 12

@easyCZ
Copy link
Member Author

easyCZ commented Sep 8, 2022

There's also the Usage component which performs these queries. Let's do it properly:

  1. Set the new property additively
  2. Ensure all staging + prod customers have this updated
  3. Update the queries to use attribution ID
  4. Remove team/User ID

@easyCZ easyCZ force-pushed the mp/stripe-attribution-id branch from 270dcb5 to 3196d74 Compare September 8, 2022 07:20
@jankeromnes
Copy link
Contributor

jankeromnes commented Sep 8, 2022

There's also the Usage component which performs these queries.

True, there are also 4 occurrences to fix in https://github.com/gitpod-io/gitpod/blob/main/components/usage/pkg/stripe/stripe.go

Fixing them shouldn't take too long. 😊

Let's do it properly:

  1. Set the new property additively
  2. Ensure all staging + prod customers have this updated
  3. Update the queries to use attribution ID
  4. Remove team/User ID

I don't see any benefit in doing this very slowly over multiple PRs/deployments.

Why take several days to do something we could do in just 5 minutes in one PR?

Reminder: There is just one customer in Stripe that matters, and its metadata is already up-to-date. We don't need a very slow & very safe migration path for one customer which is ourselves.

@andrew-farries
Copy link
Contributor

Sounds like #12753 is relevant here.

This is where we start creating new Stripe customers for individual users. It currently sets userId: 1234-.... in the metadata for the new user.

@easyCZ
Copy link
Member Author

easyCZ commented Sep 8, 2022

There is just one customer in Stripe that matters

We want staging to work also.

The reason for doing it properly is to practice the mechanics. Sooner or later we will need to do this when UBP is released to real customers and we won't have as much room for error on this.

@easyCZ
Copy link
Member Author

easyCZ commented Sep 8, 2022

/unhold

@roboquat roboquat merged commit fb2840a into main Sep 8, 2022
@roboquat roboquat deleted the mp/stripe-attribution-id branch September 8, 2022 07:28
@jankeromnes
Copy link
Contributor

jankeromnes commented Sep 8, 2022

We want staging to work also.

Customers in staging don't matter. If queries can't find existing customers, it's just like the customers never existed, and teams can upgrade again with a fake credit card.

The reason for doing it properly is to practice the mechanics. Sooner or later we will need to do this when UBP is released to real customers and we won't have as much room for error on this.

Agreed, once UBP is released and deployed and massively used, we will need to migrate much more slowly and safely.

However, today it's not useful to slow ourselves down. In fact, the more days of work we save ourselves, the sooner we can release UBP.

@easyCZ
Copy link
Member Author

easyCZ commented Sep 8, 2022

I'll do it iteratively. If not for any other benefit than my own to practice this when it is needed for real. Thank you for the suggestion of an alternative approach!

@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/XS team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants