-
-
Notifications
You must be signed in to change notification settings - Fork 11.3k
🐛 Allowed complimentary members to upgrade to a paid plan #25890
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
Conversation
WalkthroughThe PR removes the exported helper Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 21163089875 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
1 similar comment
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 21163089875 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I create a comped member locally, the subscriptions array is an empty array in the GET /members/api/member/ call and therefore allowCompMemberUpgrade is true.
So this does seem to work already without the changes in the normal use case. Reading through the conversation on the Linear issue, it seems this fixes an edge case where member has a paid sub -> cancelled it -> got a comped sub - which is indeed pretty much an edge case.
I don't think we need to cover this specific edge case via automated test, but it would be great to have a test for the more general case: "a comped member can upgrade to a paid plan" - what do you think?
We should be able reuse the existing upgrade tests for free members and apply them to comped members: apps/portal/test/upgrade-flow.test.js
| <h3>{planLabel}</h3> | ||
| <PlanLabel price={price} isComplimentary={isComplimentary} subscription={subscription} /> | ||
| </div> | ||
| <PlanUpdateButton isComplimentary={isComplimentary} isPaid={isPaid} isCancelled={isCancelled} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like isCancelled prop was never used 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah 😬
sagzy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
Previously only complimentary members with expiring access could upgrade. Now all complimentary members can access the plan selection page and upgrade.
Since all complimentary members can now upgrade, this function was redundant with isComplimentaryMember. Simplified all call sites to use isComplimentaryMember directly and removed dead code that could never execute.
Covers the general case where a comped member can upgrade to a paid plan, including the edge case where they previously had a cancelled paid subscription.
73a03fd to
c8135ef
Compare
Leftover import from helper function removal was causing lint failure
ref https://linear.app/ghost/issue/BER-2985
Allowing all complimentary members to upgrade to a paid plan. In turn removed
allowCompMemberUpgrade(because that would now always return true for all comped members) and simplified the places where it was used.Note
Makes complimentary members eligible to upgrade like free members and removes now-redundant gating logic.
allowCompMemberUpgradefromutils/helpersand all call sitesaccount-plan-pageandpaid-account-actions(always showsChangebutton unless only free plans)checkoutPlaninstead of subscription update pathsapp.jspage resolution (no longer forcesaccountPlantoaccountHomefor complimentary members)complimentaryWithCancelledSubscriptionfixtureWritten by Cursor Bugbot for commit 8090bc9. This will update automatically on new commits. Configure here.