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

Members with cancelled comp plans aren't listed with the free members filter #12160

Closed
matthanley opened this issue Aug 25, 2020 · 4 comments
Closed
Assignees
Labels
affects:api Affects the Ghost API bug [triage] something behaving unexpectedly

Comments

@matthanley
Copy link
Contributor

Issue Summary

Members who have had complimentary plans removed don't appear in the free members filter on the members list.

To Reproduce

  1. Create a new member in a members-enabled site with Stripe connected
  2. Observe member appears in list of free members (at /ghost/#/members?paid=false)
  3. Enable "Complimentary premium plan" for the member
  4. Observe member appears in list of paid members (at /ghost/#/members?paid=true)
  5. Disable "Complimentary premium plan" for the member
  6. Observe member no longer appears in either free or paid filtered member list, but does appear in all members list

What did you expect to happen instead?
Member should be reverted to a free member, and appear in the free members filtered list.

We should also verify the behaviour for cancelled and expired paid members (they should also appear as free members).

Technical details:

  • Ghost Version: 3.31.1 (on Pro)
  • Browser/OS: Chrome
@matthanley matthanley added affects:admin Anything relating to Ghost Admin bug [triage] something behaving unexpectedly members / mega labels Aug 25, 2020
@kevinansfield
Copy link
Contributor

kevinansfield commented Sep 2, 2020

The issue is that our "free member" query works on the assumption that members do not have a Stripe customer record if they are unpaid.

We can adjust the generated SQL query to something like this:

select
    `members`.*
from
    `members`
    left join `members_stripe_customers` on `members`.`id` = `members_stripe_customers`.`member_id`
    left join `members_stripe_customers_subscriptions` on `members_stripe_customers`.`customer_id` = `members_stripe_customers_subscriptions`.`customer_id`
where
    `members_stripe_customers`.`member_id` is null
    or `members_stripe_customers_subscriptions`.`customer_id` is null
    or `members_stripe_customers_subscriptions`.`status` not in ("active", "trialed");

However that query involves fetching all members/customers/subscriptions no matter what limit is applied meaning performance isn't great. For ~100k members with subscriptions and 3 test members with no customer, no subscription, or subscription that isn't active or trialed the query takes ~1sec locally even for paged result sets.

I'm not sure if there's a way to improve the query or if we'd need to look at a different way of fetching free members. We could possibly run multiple queries checking:

  1. no customer
  2. no subscription
  3. subscription with inactive status

We'd need to re-architect how the ?paid=false query param is handled in that case.

@kevinansfield
Copy link
Contributor

Alternative quick-fix would be to ensure that stripe customer data is cleared from the database when a member has a complimentary plan removed or otherwise has their subscription cancelled. That would still leave us with a potentially brittle handling of ?paid=false

@naz naz self-assigned this Sep 20, 2020
@naz
Copy link
Member

naz commented Sep 21, 2020

Was having a look at this issue and think the last solution proposed: "ensure that stripe customer data is cleared from the database" is not a good option because we rely on the members_stripe_customer record when for example "un-cancelling" the complimentary plan and still keeping it within same customer in Stripe. We don't rely on the same stripe_customers_subscriptions record which might be what was meant to be cleared in the first place 🤔, but this part is also not a good data to rely on because of the webhooks logic which would upcate/recreate a subscription record from Stripe's side.

Given following:

  1. We want to keep the amount of possible joins to the minimum (ideally none)
  2. Having to keep the records for both stripe_customers and stripe_subscriptions in place
  3. Having to calculate isPaid flag in multiple places through db querying (ref 1, ref 2)

Maybe the most performant and reliable method would be introducing an is_paid flag on the members table? The main downside I see here is having to keep subscription status associated with the member in sync with the flag (it's only ever set here), the upside is solving possible performance problems and reducing querying logic.

@naz naz removed their assignment Sep 21, 2020
@naz naz added affects:api Affects the Ghost API models / data and removed affects:admin Anything relating to Ghost Admin labels Sep 21, 2020
@matthanley matthanley added members and removed email labels Oct 2, 2020
@rorysaur
Copy link

rorysaur commented Jan 2, 2021

I'm seeing a discrepancy where a member who started a checkout but didn't go through with it (in other words, a Stripe customer was created for them but no Stripe subscription) is also not registering as free or paid. So ideally the fix would address that case as well, and not only the case of a cancelled subscription.

allouis added a commit to allouis/Ghost that referenced this issue Jan 27, 2021
refs TryGhost#12160

This flag will be used to determine if a member is free or paid, rather
than relying on joins with the customers and subscriptions tables.
tpatel pushed a commit that referenced this issue Jan 27, 2021
refs #12160

This flag will allow us easier filtering of members via the API

* Added status column to members table

This flag will be used to determine if a member is free or paid, rather
than relying on joins with the customers and subscriptions tables.

* Added migration to populate members.status

As we add the column with a default value of "free" we only need to care
about the paid members here. We also preemptively handle migrations for
SQLite where there are > 998 paid members.
daniellockyer pushed a commit that referenced this issue Feb 2, 2021
refs #12160

This flag will allow us easier filtering of members via the API

* Added status column to members table

This flag will be used to determine if a member is free or paid, rather
than relying on joins with the customers and subscriptions tables.

* Added migration to populate members.status

As we add the column with a default value of "free" we only need to care
about the paid members here. We also preemptively handle migrations for
SQLite where there are > 998 paid members.
dmitrymarokhonov pushed a commit to dmitrymarokhonov/Ghost that referenced this issue Feb 28, 2021
refs TryGhost#12160

This flag will allow us easier filtering of members via the API

* Added status column to members table

This flag will be used to determine if a member is free or paid, rather
than relying on joins with the customers and subscriptions tables.

* Added migration to populate members.status

As we add the column with a default value of "free" we only need to care
about the paid members here. We also preemptively handle migrations for
SQLite where there are > 998 paid members.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:api Affects the Ghost API bug [triage] something behaving unexpectedly
Projects
None yet
Development

No branches or pull requests

5 participants