3 ms·

The bug was in multiple subscriptions not just users. And I can't think of one non-contrived reason to do it. Even when testing the visibility/access of subscri

The bug was in multiple subscriptions not just users. And I can't think of one non-contrived reason to do it. Even when testing the visibility/access of subscriptions between users you need 2 users, but only one subscription.

hn_throwaway_99 2y ago
What? This doesn't make any sense: 1. First, if you look at the code they posted, they had the same bug on line 45 where they create new Stripe customers. 2. The issue is not multiple subscriptions per user (again, if you look at the code, you'll see each Subscription has one foreign key user_id column). The problem is if you had multiple subscriptions (each from different users) created from the same backend instance then they'd get the same PK.
- viraptor 2y ago
  Not every user needs a stripe customer. I'm creating the stripe entries only on subscription in my app. Your second point is true, but I don't see what it changes. Most automated unit/integration testing would just wipe the database between tests and needing two subscribed users in a single test is not that likely.
  - hn_throwaway_99 2y ago
    > needing two subscribed users in a single test is not that likely. Apparently not.
jbattle 2y ago
create a subscription for a test user. delete it. Make sure you can create another subscription for the same user. create subscriptions with and without overlapping effective windows Those seem like very basic tests that would have highlighted the underlying issue
- Ozzie_osman 2y ago
  Or add some debug logging? 5 days into a revenue-block bug, if I can't repro manually or via tests, I would have logged the hell out of this code. No code path or metric would be spared.
- zeroonetwothree 2y ago
  Hindsight is 20/20. It’s always easy to think of tests that would have caught the issue. Can you think of tests that will catch the next issue though?
  - jbattle 2y ago
    I'd argue that a suite of tests that exercise all reasonably likely scenarios is table stakes. And would have caught this particular bug. I'm not talking about 100% branch coverage, but 100% coverage of all happy paths and all unhappy paths that a user might reasonably bump into. OK maybe not 100% of the scenarios the entire system expresses, but pretty darn close for the business critical flows (signups, orders, checkouts, whatever).
  - hn_throwaway_99 2y ago
    Sure, hindsight is 20/20, but a bunch of these comments are replying to the assertion "And I can't think of one non-contrived reason to do it" (have a single test case with multiple subscriptions). That's the assertion I think is totally weird - I can think of tons of non-contrived reasons to have multiple subscriptions in a single test case. I wouldn't pillory someone if they left out a test case like this, but neither would I assert that a test case like this is for some reason unthinkable or some outlandish edge case.
    - viraptor 2y ago
      Go on then - so far the examples I've seen don't make sense in the context of stripe.
- viraptor 2y ago
  Have a look at stripe API. You don't delete subscriptions. You change them to free plan instead / cancel and then resume later. This for would not result in deletion of the entry. You also can update to change the billing date, so no overlapping subscriptions are needed. Neither test would result in the described bug.

hn_throwaway_99 2y ago

What? This doesn't make any sense: 1. First, if you look at the code they posted, they had the same bug on line 45 where they create new Stripe customers. 2. The issue is not multiple subscriptions per user (again, if you look at the code, you'll see each Subscription has one foreign key user_id column). The problem is if you had multiple subscriptions (each from different users) created from the same backend instance then they'd get the same PK.

viraptor 2y ago
Not every user needs a stripe customer. I'm creating the stripe entries only on subscription in my app. Your second point is true, but I don't see what it changes. Most automated unit/integration testing would just wipe the database between tests and needing two subscribed users in a single test is not that likely.
- hn_throwaway_99 2y ago
  > needing two subscribed users in a single test is not that likely. Apparently not.

jbattle 2y ago

create a subscription for a test user. delete it. Make sure you can create another subscription for the same user. create subscriptions with and without overlapping effective windows Those seem like very basic tests that would have highlighted the underlying issue

Ozzie_osman 2y ago
Or add some debug logging? 5 days into a revenue-block bug, if I can't repro manually or via tests, I would have logged the hell out of this code. No code path or metric would be spared.
zeroonetwothree 2y ago
Hindsight is 20/20. It’s always easy to think of tests that would have caught the issue. Can you think of tests that will catch the next issue though?
- jbattle 2y ago
  I'd argue that a suite of tests that exercise all reasonably likely scenarios is table stakes. And would have caught this particular bug. I'm not talking about 100% branch coverage, but 100% coverage of all happy paths and all unhappy paths that a user might reasonably bump into. OK maybe not 100% of the scenarios the entire system expresses, but pretty darn close for the business critical flows (signups, orders, checkouts, whatever).
- hn_throwaway_99 2y ago
  Sure, hindsight is 20/20, but a bunch of these comments are replying to the assertion "And I can't think of one non-contrived reason to do it" (have a single test case with multiple subscriptions). That's the assertion I think is totally weird - I can think of tons of non-contrived reasons to have multiple subscriptions in a single test case. I wouldn't pillory someone if they left out a test case like this, but neither would I assert that a test case like this is for some reason unthinkable or some outlandish edge case.
  - viraptor 2y ago
    Go on then - so far the examples I've seen don't make sense in the context of stripe.
viraptor 2y ago
Have a look at stripe API. You don't delete subscriptions. You change them to free plan instead / cancel and then resume later. This for would not result in deletion of the entry. You also can update to change the billing date, so no overlapping subscriptions are needed. Neither test would result in the described bug.