Added subscription update middleware

[naz]

Adds the ability to cancel member's subscriptions.

@allouis a structural question about this change.
(i) Have placed validation handling (if the subscription exists and belongs to the member) in the route handler itself. Was wondering if this kind of checks might belong on the stripe class itself?

(ii) Another topic is how the deletion is handled on our side. Right now the flow would be following:

1. Member visits the "account" page
2. Clicks on "cancel subscription" button
3. The request to cancel the subscription goes out to Stripe.
   3.1. At some point, webhook about the cancellation comes back and we remove the record in the DB.
4. Frontend receives confirmation and member refreshes the page and still sees the subscription.
5. Refreshes again and it's gone (webhook came through)

Should we be removing the record in the db in pt.3 as well 🤔 this would prevent the pt. 4 being a little confusing.

Sidenote. Will bring up a topic of the policy of cancellation in separate thread (if we should cancel immediately or for example use cancel_at_period_end instead.

[allouis]

This looks good to me, few tiny comments 🎉

(i) Have placed validation handling (if the subscription exists and belongs to the member) in the route handler itself. Was wondering if this kind of checks might belong on the stripe class itself?

That's a good point - the Stripe class already deals with the stripe database stuff, so it's not just stripe - I think a method that either verifies a subscription belongs to a member, or fetches a members subscription (or null) by id would work, or leave it as it is! - your call though

(ii) Another topic is how the deletion is handled on our side. Right now the flow would be following:
...
Should we be removing the record in the db in pt.3 as well thinking this would prevent the pt. 4 being a little confusing.

Yes 100% and we should probably do the same in the cancel all subscriptions method too!

[naz]

@allouis fyi-sidenote:

(ii) Another topic is how the deletion is handled on our side. Right now the flow would be following:
...
Should we be removing the record in the db in pt.3 as well thinking this would prevent the pt. 4 being a little confusing.

Yes 100% and we should probably do the same in the cancel all subscriptions method too!

While working on this bit had a debugger sitting on one of the webhooks. And had ended up having this state:

mysql> select id, status, customer_id from members_stripe_customers_subscriptions where customer_id = 'cus_GIqT1EGK6l6INb';
+--------------------------+--------+--------------------+
| id                       | status | customer_id        |
+--------------------------+--------+--------------------+
| 5de8b2399f966361b18059a1 | active | cus_GIqT1EGK6l6INb |
| 5de8b2399f966361b18059a2 | active | cus_GIqT1EGK6l6INb |
+--------------------------+--------+--------------------+
2 rows in set (0.00 sec)

It's a very unlikely state to get into, but think we would benefit having at least some basic db level protection (unique key constraint) for columns like customer_id on this and other tables. Can imagine the situation where a db query "hangs" for whatever reason and the following incoming hook would recreate a similar state.

[allouis]

It's a very unlikely state to get into, but think we would benefit having at least some basic db level protection (unique key constraint) for columns like customer_id

Can you rerun that query and include the subscription_id column too - I hope you'll find that there should be two different ones.

We can't have a unique constraint on customer_id in that table because a customer can have multiple subscriptions and we want to be able to model that in our DB.

There should be a unique constraint on the subscription_id column in that table though

[naz]

@allouis soz, bad news 😬

mysql> select id, status, subscription_id, customer_id, updated_at from members_stripe_customers_subscriptions;
+--------------------------+----------+--------------------+--------------------+---------------------+
| id                       | status   | subscription_id    | customer_id        | updated_at          |
+--------------------------+----------+--------------------+--------------------+---------------------+
| 5de8d6588f2002187a758993 | active   | sub_GIsyg8dyUlvASo | cus_GIswmsKIExk3IC | 2019-12-05 10:05:12 |
| 5de8d6588f2002187a758994 | active   | sub_GIsyg8dyUlvASo | cus_GIswmsKIExk3IC | 2019-12-05 10:05:12 |

There should be a unique constraint on the subscription_id column in that table though

Yes, subscription_id is the one that should have a constraint. Brain fart 💨 😄

[allouis]

💃 Yup deffo! There might have been a reason we couldn't have that 🤔

Are you able to replicate this?

[naz]

Are you able to replicate this?

Yes, this new state was made from scratch. To have it can:

1. Put a breakpoint at

   Members/packages/members-api/lib/stripe/index.js

   Line 222 in 343d876

   const payment = subscription.default_payment_method;

2. Got and subscribe for a plan
3. When the breakpoint hits count to 5 and let it go.
4. Check the db - voilà :)

[naz]

Think this should've been true - https://github.com/TryGhost/Ghost/blob/3.1.1/core/server/data/schema/schema.js#L350

But yeah, good question: was there a reason we couldn't have that?

[naz]

@allouis have done the cancellation initiated by the user in this way, so that it doesn't interfere with the webhook's logic. Think it can be refactored into something nicer once the subscription model is passed in directly. Wdyt?

[allouis]

Hmmm, I would've thought we'd want to update the cancelSubscription method to be like

const cancelledSubscription = await del(this._stripe, blah blah); // this returns full subscription obj
await this._updateSubscription(cancelledSubscription);
return cancelledSubscription;

And use that method everywhere instead of raw calls to del(..... 🤔

[allouis]

But yeah, good question: was there a reason we couldn't have that?

AHA - found it: https://github.com/TryGhost/Ghost/blob/3.1.1/core/server/data/schema/schema.js#L339

[allouis]

I think this might be caused by listening to too many related webhooks - so we should think about removing the invoice.succeeded and invoice.failed webhooks.

For now it seems like a super edgecase though - need to execute 5 webhooks at the exact same time - and should be fixed by using the relation plugin?

[naz]

AHA - found it: TryGhost/Ghost:core/server/data/schema/schema.js@3.1.1#L339

Damn... wish they were allowed to be a little shorter...

ref: https://stripe.com/docs/upgrades#what-changes-does-stripe-consider-to-be-backwards-compatible

Changing the length or format of object IDs or other opaque strings.
This includes adding or removing fixed prefixes (such as ch_ on charge IDs).
You can safely assume object IDs we generate will never exceed 255 characters, but you should be able to handle IDs of up to that length. If for example you’re using MySQL, you should store IDs in a VARCHAR(255) COLLATE utf8_bin column (the COLLATE configuration ensures case-sensitivity in lookups).

For now it seems like a super edgecase though - need to execute 5 webhooks at the exact same time - and should be fixed by using the relation plugin?

Yeah, this seems like a sensible approach. Even though, having the constraint would be so much nicer :)

[allouis]

Yup - that's the reason we got them so long atm 😞

Cool okay, you think we will do that in this cycle, or next?

[naz]

I think we can do this cycle if we have time. Don't want to pile new things before we finish with current work.

[naz]

@allouis tried to be malicious about what we pass along to stripe and sent different types of data through this parameter. To avoid unnecessary complexity with error handling and type checking went with coercion solution. Let me know if you'd rather go with proper type checking here and maybe throw the same way Stripe API does?