feat(plg): Add new Checkout flow that uses Stripe's `createToken` API #63213

vdavid · 2024-06-11T20:16:18Z

Closes https://github.com/sourcegraph/sourcegraph/issues/63076

I won't elaborate on the problem because I already did in the issue. Please read its description for more info.

To code reviewers: Best to review this commit by commit. The PR is not huge as a whole, but you'll save a lot on the complexity if you go commit by commit. I did my best to commit this in atomic pieces.

What I do here is:

Extracted the components from PaymentDetails for reuse. Note: I accidentally extracted two components that I didn't end up needing (heads up @taras-yemets), but the new structure felt tidier, so I kept it this way.
I now use the createToken Stripe API, based on the related design doc and decisions.
For the team creation, I use the new endpoint added by @chrsmith in https://github.com/sourcegraph/self-serve-cody/pull/864
I made callCodyProApi handle text responses because the endpoint gives an empty response, which resulted in an error when we tried to parse it as JSON.
A few loosely related warning and spelling fixes

Caveats:

With this change (and in the previous state since migrating the frontend to Dotcom), we're losing the coupon code functionality. It won't be difficult to fast-follow the current solution with it. This issue tracks the gap: https://github.com/sourcegraph/sourcegraph/issues/63211
We also lose Google Pay, Apple Pay, and all non-card payment methods. This gap is tracked by https://github.com/sourcegraph/self-serve-cody/issues/632.
I don't think 3D Secure will work with this solution.

Test plan

I've tested the checkout flow in the dark and light themes, to make sure it works and looks right. 1-min Loom | light theme screenshot
I've tested the Manage subscription page because I touched the PaymentDetails component there. It still works fine.

vdavid · 2024-06-11T20:17:39Z

client/web/src/cody/management/api/client.ts

+    export function previewCreateTeam(requestBody: types.PreviewCreateTeamRequest): Call<types.PreviewResult> {
+        return { method: 'POST', urlSuffix: '/team/preview', requestBody }
+    }


I don't use this yet (nor the other preview* function above), but it felt wrong to remove them once I added them incl. the related types. 🤷

vdavid · 2024-06-11T20:18:38Z

client/web/src/cody/management/api/react-query/callCodyProApi.ts

        // Throw errors for unsuccessful HTTP calls so that `callCodyProApi` callers don't need to check whether the response is OK.
        // Motivation taken from here: https://tanstack.com/query/latest/docs/framework/react/guides/query-functions#usage-with-fetch-and-other-clients-that-do-not-throw-by-default
-        throw new Error(`Request to Cody Pro API failed with status ${response.status}`)
+        throw new Error(`Request to Cody Pro API failed with status ${response.status}: ${response.statusText}`)


It might be helpful to see the status text as well? I can revert if not.

I faced a similar issue on the PR I'm working on.
As an alternative approach we can consider having an error class extending Error.
E.g.,

export class CodyProApiError extends Error { constructor(message: string, public status: number) { super(message) } } // then in callCodyProApi function or a custom hook around react-query useQuery/useMutation throw new CodyProApiError(await response.text(), response.status) // somewhere in component if (error instanceof CodyProApiError && error.status === 404) { // handle error }

This way we can use error message and status separately and add more fields/methods to the CodyProApiError class if we need in the future.

Addressed in https://github.com/sourcegraph/sourcegraph/pull/63213/commits/3eda37a7448a94684dfdf223a1dffba98d8d91b2, although we don't currently process this error anywhere.

vdavid · 2024-06-11T20:20:25Z

client/web/src/cody/management/api/react-query/callCodyProApi.ts

+    try {
+        return JSON.parse(responseText) as Data
+    } catch {
+        return responseText as Data
+    }


Note: the caller needs to know whether they want plain text or JSON. So far, I had no great idea how to make this better and frankly, we have 0 callers that use a text output, so I only needed this to avoid getting an error here upon an empty response.

As an alternative, we can make the callCodyProApi function return Response and then process it in a caller, e.g., parse as JSON, plain text, or ignore the return value in case of an empty response body.

Addressed in https://github.com/sourcegraph/sourcegraph/pull/63213/commits/792dcd59fbdbf6224810f65bfce3d57a538ab409

vdavid · 2024-06-11T20:21:18Z

client/web/src/cody/management/api/react-query/subscriptions.ts

+    return useMutation({
+        mutationFn: async requestBody => callCodyProApi(Client.createTeam(requestBody)),
+        onSuccess: () => queryClient.invalidateQueries({ queryKey: queryKeys.all }),
+    })


@taras-yemets, I appreciate your 👀 here because you've spent more time with the use-query docs than I have.

vdavid · 2024-06-11T20:21:49Z

client/web/src/cody/management/api/teamSubscriptions.ts

+    seats: number
+    billingInterval: BillingInterval
+    couponCode?: string
+}


Would be super great if these were generated, btw.

vdavid · 2024-06-11T20:24:06Z

client/web/src/cody/management/subscription/StripeCardDetails.tsx

+import { useTheme, Theme } from '@sourcegraph/shared/src/theme'
+import { Label, Text, Grid } from '@sourcegraph/wildcard'
+
+const useCardElementOptions = (): ((type: 'number' | 'expiry' | 'cvc') => StripeCardElementOptions) => {


@taras-yemets, I made this a function because we were getting warnings from Stripe in the JS console because we were passing disableLink (and hidePostalCode!) to the wrong components.

vdavid · 2024-06-11T20:25:03Z

client/web/src/cody/management/subscription/manage/CodySubscriptionManagePage.tsx

                gradientTransform="translate(23.4233 -44.4873) rotate(77.074) scale(58.2161)"
            >
-                <stop stopColor="#00DBFF" style={{ stopColor: '#00DBFF', stopOpacity: 1 }} />
+                <stop stopColor="#00DBFF" style={{ stopColor: '#00dbff', stopOpacity: 1 }} />


Super unimportant, but my IDE did it and it felt neater after.

vdavid · 2024-06-11T20:25:26Z

client/web/src/cody/management/subscription/manage/NonEditableBillingAddress.tsx

+
+import type { Subscription } from '../../api/teamSubscriptions'
+
+export const NonEditableBillingAddress: React.FC<{ subscription: Subscription }> = ({


I will use this when I add the "Add seats" feature tomorrow-ish!

vdavid · 2024-06-11T20:27:06Z

client/web/src/cody/management/subscription/new/CodyProCheckoutForm.tsx

-        void updateSeatCount()
-    }, [firstLineItemId, debouncedSeatCount, updateLineItemQuantity])
+    // N * $9. We expect this to be more complex later with annual plans, etc.
+    const total = seatCount * 9


@chrsmith suggested using a function for this, so I did that first, but it felt overengineered for a simple multiplication, so I inlined the calculation and left this note instead. We can extract it any time.

vdavid · 2024-06-11T20:28:37Z

client/web/src/cody/management/subscription/new/NewCodyProSubscriptionPage.tsx

 import { defaultCodyProApiClientContext, CodyProApiClientContext } from '../../api/components/CodyProApiClient'
+import { useBillingAddressStripeElementsOptions } from '../manage/BillingAddress'

-import { CodyProCheckoutFormContainer } from './CodyProCheckoutFormContainer'


Both CodyProCheckoutFormContainer and PayButton lost so much logic that I just put everything in CodyProCheckoutForm for now.

vdavid · 2024-06-11T20:29:14Z

client/web/src/cody/management/subscription/new/NewCodyProSubscriptionPage.tsx

 // We do it here, meaning that "stripe.js" will get loaded lazily, when the user
 // routes to this page.
 const publishableKey = window.context.frontendCodyProConfig?.stripePublishableKey
-const stripe = await stripeJs.loadStripe(publishableKey || '', { betas: ['custom_checkout_beta_2'] })


Not needing the beta anymore. We probably want to remove this from the back end as well—I'll create a PR soon after this, or we'll do it later.

taras-yemets · 2024-06-12T07:26:25Z

client/web/src/cody/management/subscription/manage/PaymentDetails.tsx

+            <div className={styles.gridItem}>
+                <PaymentMethod paymentMethod={subscription.paymentMethod} />
+            </div>


nit: we can wrap PaymentMethod with Elements here (same as we do for BillingAddress

Ah, I see, we might not need Elements when payment method is missing

That's true, but I think you were right in seeing that the diff between the two branches was weird. WDYT about https://github.com/sourcegraph/sourcegraph/pull/63213/commits/88110af228bd88d5abfafe6c4fd73ab6be6028fa? It needed passing an extra prop, but the <Elements> use is more specific to where we actually need it.

taras-yemets · 2024-06-12T07:30:19Z

client/web/src/cody/management/subscription/new/CodyProCheckoutForm.tsx

-            }
-            setUpdatingSeatCount(false)
-        }
+    const [seatCount, setSeatCount] = React.useState(initialSeatCount)


Can initialSeatCount prop change? Do we need to sync state with props in this case?

It's controlled by a query param, and it cannot change.

taras-yemets · 2024-06-12T07:31:38Z

client/web/src/cody/management/subscription/new/CodyProCheckoutForm.tsx

    const navigate = useNavigate()

-    const { total, lineItems, updateLineItemQuantity, email, updateEmail, status } = useCustomCheckout()
+    const creatingTeamMutation = initialSeatCount > 1


nit: "creating" sounds like pending process to me.

Suggested change

const creatingTeamMutation = initialSeatCount > 1

const shouldCreateTeamMutation = initialSeatCount > 1

Thanks, was bad naming on my part. I ended up going with just isTeam: https://github.com/sourcegraph/sourcegraph/pull/63213/commits/a09ff2f7da2083af0c5f59af4a9b9e6f61dba32e

taras-yemets · 2024-06-12T07:34:13Z

client/web/src/cody/management/subscription/new/CodyProCheckoutForm.tsx

+
+            // This is where we send the token to the backend to create a subscription.
+            try {
+                await createTeamMutation.mutateAsync({


As we don't do anything with the returned promise, it's recommended to use mutate instead of mutateAsync

Ah, true. I meant to use the result, that's why I did it, but then I didn't use it. https://github.com/sourcegraph/sourcegraph/pull/63213/commits/cccb7510d25cd7c5f7a4819fe34731f9a60b61c5 fixes this.

Oh, no, wait, I did it this way to make sure we wait for the result. I wonder if .mutate executes synchronously. I want to avoid navigating away too early. I'll test this.

I actually reverted this and added a comment about this: https://github.com/sourcegraph/sourcegraph/pull/63213/commits/9eb57d13a4a3fd5d30f9b948c6c67484bf960847

We can navigate if the mutation was successful using the onSuccess callback.

createTeamMutation.mutate(data, {onSuccess: () => navigate('/cody/manage?welcome=1')})

and derive the error and loading state from createTeamMutation which will result in less local state: we won't need extra setErrorMessage for example:

// somewhere in component body const errorMsg = stripeErrorMessageFromState || (createTeamMutation.isError ? `We couldn't create the team. This is what happened: ${createTeamMutation.error.message}` : "")

But I agree that mutateAsync also works in this case.

taras-yemets · 2024-06-12T07:37:16Z

client/web/src/cody/management/subscription/new/CodyProCheckoutForm.tsx

+                })
+            } catch (error) {
+                setErrorMessage(`We couldn't create the team. This is what happened: ${error}`)
+                setSubmitting(false)


We have two loading states: waiting for stripe.createToken and createTeamMutation.mutate promises to resolve. I suggest we split them and use like

const isLoading = isStripeLoading || createTeamMutation.isPending

for clarity.

I just went with mutateAsync (https://github.com/sourcegraph/sourcegraph/pull/63213/commits/9eb57d13a4a3fd5d30f9b948c6c67484bf960847) and kept the single loading var.

taras-yemets · 2024-06-12T07:39:19Z

client/web/src/cody/management/subscription/new/CodyProCheckoutForm.tsx

+        } catch (error) {
+            setErrorMessage(`We couldn't create the Stripe token. This is what happened: ${error}`)
+            setSubmitting(false)


As createTeamMutation.mutate has it's own catch block, can we move this one closer to stripe.createToken call?

Did it in https://github.com/sourcegraph/sourcegraph/pull/63213/commits/846db9919fd380e52942ca142d8680f1786f258c, and also extracted the Stripe checkout functionality for clarity.

taras-yemets · 2024-06-12T07:42:51Z

client/web/src/cody/management/subscription/new/CodyProCheckoutForm.tsx

+                                {submitting ? (
+                                    <LoadingSpinner className={styles.lineHeightLoadingSpinner} />
+                                ) : (
+                                    'Subscribe'
+                                )}


LoadingIconButton may serve likely solves a similar problem.
https://github.com/sourcegraph/sourcegraph/blob/dd42f20620c629c175e24a75ced4100f2344c601/client/web/src/cody/management/subscription/manage/LoadingIconButton.tsx#L10

I've just tried it, but that component always displays an icon on the button, which we don't want here. It doesn't allow a null icon either. I'm leaving this as is for now.

taras-yemets

LGTM.
Left a few non-blocking comments inline.

vdavid added 6 commits June 11, 2024 17:56

Make callCodyProApi handle text responses

897ee09

Typo fixes

034d38a

Extract components from PaymentDetails

316dccd

Warning fixes

d4f4064

Use createToken API

cc47ad8

Use new endpoint to create team

734fcef

cla-bot bot added the cla-signed label Jun 11, 2024

vdavid commented Jun 11, 2024

View reviewed changes

CI and bug fix

99380b3

vdavid marked this pull request as ready for review June 11, 2024 20:55

taras-yemets reviewed Jun 12, 2024

View reviewed changes

vdavid added 2 commits June 12, 2024 09:35

Return Response from callCodyProApi

792dcd5

Return custom error

3eda37a

taras-yemets reviewed Jun 12, 2024

View reviewed changes

Making Elements more specific. (Plus linter fixes)

88110af

taras-yemets reviewed Jun 12, 2024

View reviewed changes

Rename variable

a09ff2f

taras-yemets approved these changes Jun 12, 2024

View reviewed changes

vdavid added 4 commits June 12, 2024 09:44

Use mutate

cccb751

Use mutateAsync

9eb57d1

Extract function

846db99

Oops, fix

2b2d8d9

vdavid enabled auto-merge (squash) June 12, 2024 09:48

vdavid mentioned this pull request Jun 12, 2024

Subscriptions are created in a "canceled" state by default #63225

Closed

vdavid merged commit 4774879 into main Jun 12, 2024

vdavid deleted the dv/use-stripe-create-token branch June 12, 2024 09:57


		import type { Subscription } from '../../api/teamSubscriptions'

		export const NonEditableBillingAddress: React.FC<{ subscription: Subscription }> = ({

	const creatingTeamMutation = initialSeatCount > 1
	const shouldCreateTeamMutation = initialSeatCount > 1

feat(plg): Add new Checkout flow that uses Stripe's createToken API #63213

feat(plg): Add new Checkout flow that uses Stripe's createToken API #63213

Uh oh!

Conversation

vdavid commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

vdavid Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdavid Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taras-yemets left a comment

Choose a reason for hiding this comment

feat(plg): Add new Checkout flow that uses Stripe's `createToken` API #63213

feat(plg): Add new Checkout flow that uses Stripe's `createToken` API #63213

vdavid commented Jun 11, 2024 •

edited

Loading

vdavid Jun 11, 2024 •

edited

Loading

vdavid Jun 11, 2024 •

edited

Loading