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

Stripe template TS migration #122

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Stripe template TS migration #122

wants to merge 14 commits into from

Conversation

DevAOC
Copy link
Collaborator

@DevAOC DevAOC commented Nov 11, 2024

No description provided.

@DevAOC DevAOC requested a review from rdraward November 11, 2024 18:14
Copy link
Collaborator

@rdraward rdraward left a comment

Choose a reason for hiding this comment

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

🎩 works well!

A couple of things that should be looked at, in addition to comments:

web/stripe-template/api/actions/getProducts.ts Outdated Show resolved Hide resolved
web/stripe-template/api/actions/getProducts.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,103 @@
import { stripe } from "../stripe";

type StripeProduct = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried using this before but it caused more issues

@DevAOC
Copy link
Collaborator Author

DevAOC commented Nov 26, 2024

🎩 works well!

A couple of things that should be looked at, in addition to comments:

I don't see the GET-hello route in my app

@DevAOC DevAOC requested a review from rdraward November 26, 2024 18:42
@rdraward
Copy link
Collaborator

  • the api/routes/GET-hello route is present

I don't see the GET-hello route in my app

Ignore! That was bad copy-paste on my part when testing!

Copy link
Collaborator

@rdraward rdraward left a comment

Choose a reason for hiding this comment

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

Two tophat issues:

  1. Still get an unauthenticated warning in dev harness when I visit the index route in the frontend. We have signed in components to check for this status and I think we may need to reconsider how the Provider is positioned
  2. We don't properly handle subscription cancellations. When I cancel in Stripe's dashboard, we delete the record and I still have access to my "app". We only check for the stripeCustomerId and not the status of the subscription

web/stripe-template/api/actions/getProducts.ts Outdated Show resolved Hide resolved
request: request,
endpointSecret: process.env.STRIPE_SUBSCRIPTION_WEBHOOK_SECRET,
endpointSecret: String(process.env.STRIPE_SUBSCRIPTION_WEBHOOK_SECRET),
});
} catch (err) {
return await reply.status(400).send();
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I cancel a stripe subscription we delete the subscription. Instead, we should keep the record and update the status field, then check to make sure the user has an active subscription

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've changed the Stripe subscription logic to immediately end the subscription so the delete webhook will be fired

@DevAOC DevAOC requested a review from rdraward December 5, 2024 16:11
@rdraward
Copy link
Collaborator

rdraward commented Dec 5, 2024

3 tophat issues on my first go-through while setting up from scratch:

  1. I still see the permission denied error when I first open the app
    CleanShot 2024-12-05 at 13 39 43@2x

  2. When I subscribe -> unsubscribe and log out, I get these permission errors after logging out
    CleanShot 2024-12-05 at 13 41 51@2x
    CleanShot 2024-12-05 at 13 41 44@2x

  3. I can still access the app even after canceling a subscription. No subscriptions in my model, but access to the Home page still says "Customer detected!". Maybe that is okay, but we should probably have an indication of whether or not a subscription is active because right now the page is the same if I have a subscription or don't

@DevAOC
Copy link
Collaborator Author

DevAOC commented Dec 5, 2024

I might have forgotten to look into the permissions error issue. Also not sure how you're getting that when you should only be calling getProducts from the /billing page.

I'll remove the ability to see the homepage when there's no sub

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants