NewFrontDoor / pwad.org.au

Public Worship and Aids to Devotion Website
https://pwad.org.au
2 stars 1 forks source link

Annual Subscription #58

Open BarryThePenguin opened 4 years ago

BarryThePenguin commented 4 years ago

I've been looking into what's needed to support subscriptions..

So far I've got the webhook listening for invoice.payment_succeeded events, which return an Invoice

We can then use the customer email to update the subscription status in Sanity, by finding the user with that email and patching some of the subscription details.

So far I'm think we add the following fields

With that information we could

Anything else we should add?

Are you able to add those few fields to Sanity? You can see examples of them on my User in Sanity

readeral commented 4 years ago

I'll find some time to think about this today. Initial thoughts:

Otherwise, I've published those changes to Sanity for you to use. I'll think about the rest later.

BarryThePenguin commented 4 years ago

Hmm.. subscriptionStatus is actually the invoiceStatus ๐Ÿ˜… The status of the invoice can be one of draft, open, paid, uncollectible, or void. But maybe we actually want the subscriptionStatus?

I've been making most of these decisions based on the stripe documentation, if that helps

Happy to drop the startDate, but we need to use the endDate as that's the important piece of information. Assuming it's a calendar year can lead to issues ๐Ÿ˜…

readeral commented 4 years ago

So I'm wondering, is there a reason we aren't using https://stripe.com/docs/api/subscriptions/object#subscription_object-status ?

readeral commented 4 years ago

Ok reading the doc above, that's absolutely what we should be using, and we should be using https://stripe.com/docs/billing/webhooks#state-changes to update the data in Sanity

readeral commented 4 years ago

So the newly amended fields now are:

      name: 'subscriptionStatus',
      title: 'Subscription Status',
      type: 'string',
      options: {
        list: [
          'incomplete',
          'incomplete_expired',
          'trialing',
          'active',
          'past_due',
          'canceled',
          'unpaid'
        ]
      }
    },
    {
      name: 'stripeCustomerId',
      title: 'Customer ID',
      type: 'string'
    },
    {
      name: 'periodEndDate',
      title: 'Subscription period end date',
      type: 'datetime'
    }
readeral commented 4 years ago

and we ought use the following fields: status customer current_period_end

For customer service reasons we might benefit from ended_at and latest_invoice to be able to output a link to it in the account page

BarryThePenguin commented 4 years ago

Reading a bit more about the topic, to determine if the current user "has access", I think we should only capture the customer, current invoice status, and period end date.

The role of a subscription is to charge a customer on a recurring basis.

An invoice is a statement of how much a customer owes. The easiest way to determine if someone has successfully paid for an invoice is to use the invoice.payment_succeeded event.

https://blog.checklyhq.com/our-stripe-billing-implementation-the-one-webhook-to-rule-them-all/

The issue with relying on the customer.subscription.updated event to capture the status change is webhook events can come in any order. Stripe can, and probably does intentionally during testing, send an event with "status": "active" first, and then sends an event with "status": "incomplete".

This is fine if you're capturing webhook events correctly, and processing them in an asynchronous manner. Ideally you'd put the event in a temporary cache, respond to Stripe immediately, then attempt to process the event in a seperate process at some point in the near future.

In that seperate process, you'd query the latest state from Stripe, so there's no real issue with events coming out of order.

We don't have the capacity, and probably don't need need, to process events using that approach..

I'd encourage having a read through https://stripe.com/docs/webhooks/best-practices#event-handling

readeral commented 4 years ago

I still think we should use the subscription status, as it helps us inform the customer of a number of things, such as when their renewal is coming up, or if they've successfully cancelled but has access until the renewal date. We should be retaining all that in sanity and providing it in the account page.

Invoices might give a definitive "they've paid" state, but we ought to be providing more information - and we can get everything we need from the subscription endpoint without needing to worry about the invoice endpoint.

The event object has a 'created' key, from which we can determine the time it was created. Any events that arrive after an event with a later time stamp can simply be discarded after responding.

BarryThePenguin commented 4 years ago

The event object has a 'created' key, from which we can determine the time it was created. Any events that arrive after an event with a later time stamp can simply be discarded after responding.

That means we have to now store the created key in sanity too, for comparison.

...we can get everything we need from the subscription endpoint without needing to worry about the invoice endpoint.

We can't fetch information from Stripe while processing the request from a webhook event, it introduces too much risk.

Acknowledge events immediately If your webhook script performs complex logic, or makes network calls, itโ€™s possible that the script would time out before Stripe sees its complete execution. Ideally, your webhook handler code (acknowledging receipt of an event by returning a 2xx status code) is separate of any other logic you do for that event.

While processing an event from Stripe, we're already sending a fetch to Sanity for the user id, before we can even perform the patch to update the stripe information.

We don't have the infrastructure to handle this correctly.

as it helps us inform the customer of a number of things, such as when their renewal is coming up, or if they've successfully cancelled but has access until the renewal date

If you want to show subscription information to the user, either we should consider Stripes own self service portal, or you can fetch it directly from Stripe when they view their account in PWAD

I'd like to keep the amount of data we need to replicate in Sanity to a minimum

readeral commented 4 years ago

Happy for you to implement it in the way you see fit - but we do need to give customers the renewal date, and a means to cancel.

On Sat, 22 Feb 2020 at 19:10, Jonathan Haines notifications@github.com wrote:

The event object has a 'created' key, from which we can determine the time it was created. Any events that arrive after an event with a later time stamp can simply be discarded after responding.

That means we have to now store the created key in sanity too, for comparison.

...we can get everything we need from the subscription endpoint without needing to worry about the invoice endpoint.

We can't fetch information from Stripe while processing the request from a webhook event, it introduces too much risk.

Acknowledge events immediately If your webhook script performs complex logic, or makes network calls, itโ€™s possible that the script would time out before Stripe sees its complete execution. Ideally, your webhook handler code (acknowledging receipt of an event by returning a 2xx status code) is separate of any other logic you do for that event.

While processing an event from StripeWe're already sending a fetch to Sanity for the user id, before we can even perform the patch to update the stripe information.

We don't have the infrastructure to handle this correctly.

as it helps us inform the customer of a number of things, such as when their renewal is coming up, or if they've successfully cancelled but has access until the renewal date

If you want to show subscription information to the user, either we should consider Stripes own self service portal https://stripe.com/docs/billing/subscriptions/self-serve-portal, or you can fetch it directly from Stripe when they view their account in PWAD

I'd like to keep the amount of data we need to replicate in Sanity to a minimum

โ€” You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/NewFrontDoor/pwad.org.au/issues/58?email_source=notifications&email_token=ADZK3CEBSJDB5M4NYITFE3LREDMWZA5CNFSM4KXCYD6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMU2FQY#issuecomment-589931203, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZK3CBBF6AZC6Q5LISCJFLREDMWZANCNFSM4KXCYD6A .

--

Alan Reader| e. areader0@gmail.com areader0@gmail.com | m. 0400 215 751 | fb. - readeral http://www.facebook.com/readeral | i. - instagram.com/readeral http://instagram.com/readeral