feat: PUT /user/payment API saves payment info in stripe.com #1818

gobengo · 2022-08-31T05:03:37Z

Motivation

api: create api that can save payment settings #1748

Parts

ability to create stripe customers for users and look them up
PUT /user/payment actually saves payment method id
GET /user/payment includes stripe.card card info for frontend
add db migration that will work

… save logic

This reverts commit 7957f38.

…-stripe

github-actions · 2022-09-01T21:44:33Z

`package-lock.json` changes

Click to toggle table visibility

Name	Status	Previous	Current
`stripe`		-	10.7.0

github-actions · 2022-09-01T21:47:50Z

Website preview 🔗✨

build log

packages/api/src/utils/stripe.js

vasco-santos

Good job @gobengo

Did a first pass through this and left feedback. Let me know what you think

vasco-santos · 2022-09-07T10:41:16Z

packages/api/scripts/cli.js

+            })
+          }
+        },
+        NodeModulesPolyfillPlugin()


Is this for stripe? would be nice to have a comment if so

or is needed for assert?

Added a comment explaining how 'stripe' deps on some node things (though I think we dont actually import those parts, e.g. we use the Fetch http adapater instead of the Node one)

(afaict it wasn't related to the use of assert)

vasco-santos · 2022-09-07T10:43:01Z

packages/api/src/env.js

@@ -35,6 +39,7 @@ import { defaultBypassMagicLinkVariableName } from './magic.link.js'
 * @property {string} [LOGTAIL_TOKEN]
 * @property {string} MAINTENANCE_MODE
 * @property {string} [DANGEROUSLY_BYPASS_MAGIC_AUTH]
+ * @property {string} [STRIPE_SECRET_KEY]


I guess this is injected as a Worker secret? Can we add to https://github.com/web3-storage/web3.storage/tree/1661905101-api-tsc-and-stripe/packages/api#4-configure-your-worker secrets part?

vasco-santos · 2022-09-07T10:46:06Z

packages/api/src/env.js

+ */
+function createStripeBillingContext (env) {
+  const stripeSecretKey = env.STRIPE_SECRET_KEY
+  assert.ok(stripeSecretKey, 'env STRIPE_SECRET_KEY is required')


Can we not add assert dependency just for this?

sure. so i can have this in mind in the future: what's the downside of the fairly-well-standardized assert dependency in this cf worker context? FWIW assert.ok is nice because it's also a typed as a type guard. microsoft/TypeScript#32695

Assert will throw AssertionError and in the error handler we will need to define what user error we want to send (and whether it should have 500 code or 4xx code). We could get all this with assert, but I think it would make things more complex with current code

vasco-santos · 2022-09-07T10:49:49Z

packages/api/src/env.js

-  // this can be used to mock realistic values of a stripe.com paymentMethod id
-  // after fulls tripe integration, this may not be needed on the env
-  env.mockStripePaymentMethodId = 'pm_1LZnQ1IfErzTm2rETa7IGoVm'
+  Object.assign(env, (env.ENV === 'test') ? createTestBillingEnv() : createStripeBillingContext(env))


I would suggest not adding so much stripe specific logic into env file which is quite simple at the moment. So, thinking about a createStripeBillingContext(env) factory function in utils/stripe.js which would do this validation and have functions below. We also end up simplifying a lot the import statement here and what needs to be exported from stripe file.
What do you think @gobengo ?

Also, wondering if we could simplify a bit this to not be so env specific. If we make STRIPE_SECRET_KEY optional and use it as a way of identifying if we want a real billing context or a mock?

if (!env.STRIPE_SECRET_KEY) { return { billing: createMockBillingService(), customers: createTestEnvCustomerService() } }

Probably to throw an error only as:

if (!env.STRIPE_SECRET_KEY && !(env.ENV === 'test' || env.ENV === 'dev') { throw new Error(`Stripe secret key is required for env ${env.ENV}`) }

thoughts?

@vasco-santos great suggestion. I implemented as
475be01
3366de2

vasco-santos · 2022-09-07T12:20:10Z

packages/api/src/user.js

@@ -78,7 +79,7 @@ async function loginOrRegister (request, env) {

  const parsed =
    data.type === 'github'
-      ? parseGitHub(data.data, metadata)
+      ? await parseGitHub(data.data, metadata)


Not promise?

vasco-santos · 2022-09-07T12:44:03Z

packages/api/src/utils/stripe.js

+    /**
+     * @type {BillingService}
+     */
+    const instance = this // eslint-disable-line


do we need this?

It's the only way to have tsc check compatability with that BillingInterface when using jsdoc.

microsoft/TypeScript#30156 (comment)

I made it nicer, one line, no eslint-disable 67deec4 (also checked to make sure this new syntax gives us the same warning if the interface breaks)

vasco-santos · 2022-09-07T12:44:04Z

packages/api/src/utils/stripe.js

+      throw new Error('unable to get payment method')
+    }
+    if (response.deleted) {
+      throw new Error('unexpected response.deleted')


Are these internal Errors or user input error?

If these can happen because user input, they should be custom errors like the ones you see in src/errors.js, otherwise they will be interpreted as 500 and throw on call alerts

(this is also the case for other Errors in this PR that I am not flagging)

@vasco-santos this is not related to user error. These would be legitimately unexpected errors that might indicate something else was wrong in the stripe.com-hosted customer db. It shouldn't need to keep anyone up at night, but it would be nice to have a modest alarm (e.g. a sentry report) if these errors ever do come up, and throwing explicitly will give the debugger (probably me) a stack trace when that happens. Technically this case can happen (the type system told me), but it shouldn't, and if it does it is probably something that should be escalated to someone like us

I removed these two untyped Errrors. The first wasn't really needed. The second I added a specific CustomerNotFound type for, and added it to the function signature so tsc helps callers be ready for it or other errors. cd82b24

that sounds good. Thanks for details :)

vasco-santos · 2022-09-07T12:50:31Z

packages/db/index.js

+  async getUserCustomer (userId) {
+    const { data, error } = await this._client
+      .from('user_customer')
+      .select(['customer_id'].join(','))


No need for the join

according to the docs (and tsc types, which aren't being checked in db package yet), it must be a string. https://supabase.github.io/postgrest-js/classes/PostgrestQueryBuilder.html#select

(not disagreeing that the join works now at runtime, but I'd prefer to keep it here so it doesn't have to change again if/when we enable type checking #1860 )

packages/db/index.js

vasco-santos · 2022-09-07T12:51:25Z

packages/db/postgres/migrations/023-add-user-customer-table.sql

+CREATE TABLE IF NOT EXISTS user_customer
+(
+  id               BIGSERIAL PRIMARY KEY,
+  user_id          BIGINT NOT NULL UNIQUE REFERENCES public.user (id),


We do queries for user_id, probably we should add an index for it? What you think?

good idea! 3df2067

…ed generic Error throw)

Co-authored-by: Vasco Santos <[email protected]>

…s more time

…with firefox

gobengo · 2022-09-07T19:52:09Z

@vasco-santos Thanks for your review. I believe I addressed all of your comments. (I left an 🎉 emoji on comments I acted on). wdyt?

gobengo · 2022-09-08T00:01:44Z

I integrated some frontend work by @adamalton on top of this that actually results in a functional demo of the backend changes in this PR. It's really those changes too that we want to be deploying to staging and beyond. Let's consider this PR superceded by #1852 . @vasco-santos it's basically identical to what you reviewed already + changes based on your review.

gobengo added 15 commits August 29, 2022 16:31

Add typechecking via tsc to packages/api and start WIP billing/stripe…

74268b0

… save logic

fix type check

0e145be

Merge branch 'main' into 1661808772-payment-save

ece741d

rm new stripe stuff for now. just focus on tsc

7957f38

fix bug in pagination from bad merge

eeafc28

remove unneeded .paths in packages/api/tsconfig.json

5297699

build: packages/api build script runs typecheck

c60b474

Revert "rm new stripe stuff for now. just focus on tsc"

2e92778

This reverts commit 7957f38.

Merge branch 'main' into 1661905101-api-tsc-and-stripe

283ac7c

wip

1fe9d66

packages/api typecheck checks test dir

f7b8c71

Merge branch '1661968467-api-tsc-testdir' into 1661905101-api-tsc-and…

e9ca4ec

…-stripe

fix typecheck on new billing tests

45fa828

fix test

0efb76b

fix tests

f82ddfa

gobengo changed the title ~~1661905101 api tsc and stripe~~ feat: PUT /user/payment API saves payment info in stripe.com Aug 31, 2022

gobengo added 3 commits August 31, 2022 17:50

PUT /user/payment API calls saveUserPayments

6c76376

add packages/api/test/stripe.spec.js

721f62a

wip stripe

4e4e31e

gobengo commented Sep 1, 2022

View reviewed changes

packages/api/src/utils/stripe.js Outdated Show resolved Hide resolved

gobengo added 8 commits September 1, 2022 17:35

add user_customer table and use in StripeCustomersService

83a2286

finish StripeCustomersService#getOrCreateUser

8cb9d8b

cf workers tests use a createTestBillingContext

3c0c9d3

narrow type on createStripeBillingContext

5dab64a

StripeCustomersService has a stripe instance with paymentMethods.attach

88966a8

StripeBillingService calls stripe.paymentMethods.attach

0861c71

tests pass

3ae9149

working stripe userPaymentGet

dc845ab

gobengo added 2 commits September 6, 2022 11:35

add STRIPE_SECRET_KEY (commented) to .env.tpl

742fc12

rm env.mockStripePaymentMethodId

5d8d846

gobengo requested review from vasco-santos and yusefnapora September 6, 2022 18:42

gobengo added 4 commits September 6, 2022 12:14

relocate some things

a33f59a

rename db stripe.spec.js -> billing.spec.js

1cf554e

increase timeout of test w3name backward compatibility proxy

69a12ee

db mocha hooks log more to debug failed ci

a1dc497

gobengo mentioned this pull request Sep 6, 2022

feat: issues/1779 - I can save my payment settings #1852

Merged

vasco-santos reviewed Sep 7, 2022

View reviewed changes

gobengo added 9 commits September 7, 2022 10:03

add comment explaining why we build api with NodeModulesPolyfillPlugin

1926f9b

dont use assert in packages/api/src/env.js

83431fc

lint

1a5677e

move BillingEnv-creating code from env.js to stripe.js and billing.js

475be01

more change to how env sets up BillingContext on env

3366de2

Add wranger put STRIPE_SECRET_KEY to packages/api/README.md

9da3c8b

remove unnecessary 'await'

1a3ab1a

cleaner type assertion in StripeBillingService

67deec4

BillingService may return CustomerNotFound error (instead of an untyp…

cd82b24

…ed generic Error throw)

gobengo mentioned this pull request Sep 7, 2022

packages/db should have typecheck as part of ci #1860

Open

gobengo and others added 5 commits September 7, 2022 11:55

remove console.error in db getUserCustomer

c282c23

Co-authored-by: Vasco Santos <[email protected]>

add index user_customer_user_id_idx

3df2067

double packages/db test beforeAll timeout because github actions need…

4476908

…s more time

fix tests because test env wasnt setting up mockBillingContext correctly

cb0124d

ci: website workflow doesnt install playwright chromium when testing …

6e17da2

…with firefox

gobengo requested a review from vasco-santos September 7, 2022 19:52

gobengo mentioned this pull request Sep 7, 2022

chore: incorporate post-review changes into issues/1779 branch #1861

Merged

gobengo closed this Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: PUT /user/payment API saves payment info in stripe.com #1818

feat: PUT /user/payment API saves payment info in stripe.com #1818

gobengo commented Aug 31, 2022 •

edited

Loading

github-actions bot commented Sep 1, 2022

github-actions bot commented Sep 1, 2022 •

edited

Loading

vasco-santos left a comment

vasco-santos Sep 7, 2022

vasco-santos Sep 7, 2022

gobengo Sep 7, 2022

gobengo Sep 7, 2022

vasco-santos Sep 7, 2022

gobengo Sep 7, 2022

vasco-santos Sep 7, 2022

gobengo Sep 7, 2022 •

edited

Loading

gobengo Sep 7, 2022

vasco-santos Sep 8, 2022

vasco-santos Sep 7, 2022

vasco-santos Sep 7, 2022

gobengo Sep 7, 2022

vasco-santos Sep 7, 2022

vasco-santos Sep 7, 2022

gobengo Sep 7, 2022

gobengo Sep 7, 2022

gobengo Sep 7, 2022 •

edited

Loading

vasco-santos Sep 7, 2022

vasco-santos Sep 7, 2022

vasco-santos Sep 7, 2022

gobengo Sep 7, 2022 •

edited

Loading

gobengo Sep 7, 2022

vasco-santos Sep 8, 2022

vasco-santos Sep 7, 2022

gobengo Sep 7, 2022

vasco-santos Sep 7, 2022

gobengo Sep 7, 2022

gobengo commented Sep 7, 2022

gobengo commented Sep 8, 2022

feat: PUT /user/payment API saves payment info in stripe.com #1818

feat: PUT /user/payment API saves payment info in stripe.com #1818

Conversation

gobengo commented Aug 31, 2022 • edited Loading

github-actions bot commented Sep 1, 2022

package-lock.json changes

github-actions bot commented Sep 1, 2022 • edited Loading

Website preview 🔗✨

vasco-santos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gobengo Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gobengo Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gobengo Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gobengo commented Sep 7, 2022

gobengo commented Sep 8, 2022

gobengo commented Aug 31, 2022 •

edited

Loading

`package-lock.json` changes

github-actions bot commented Sep 1, 2022 •

edited

Loading

gobengo Sep 7, 2022 •

edited

Loading

gobengo Sep 7, 2022 •

edited

Loading

gobengo Sep 7, 2022 •

edited

Loading