Repository navigation
Show the subscription expiring soon banner before the trial banners - #102307
Conversation
The pre-trial check also passes when the free trial NVPs are absent, so an annual subscriber without them saw "Start a free trial" instead of the expiry warning on the Subscription page. Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
🤖 Android test steps verification results: This PR's Tests section is still a TODO, so these steps come from the PR description and the parent PR's precondition (subscription state set with
|
|
🤖 Web test steps verification results: This PR's Tests section is still a TODO, so these steps come from the PR description. On web, I couldn't set an expiring annual subscription because the web test tooling can't run
|
|
Bug: The subscription page is showing the expiry banner, but the Account menu still uses the trial dates. this happened after run this in browser Onyx.merge('nvp_private_subscription', {
type: 'yearly2018',
autoRenew: false,
endDate: '2026-10-22',
});
|
… shows Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
Fixed in 874b607. Now, when the expiring-soon warning applies, the Account menu no longer shows the "30 days left!" trial badge. Cause: The badge comes from Fix: One thing I didn't change: the Home free trial widget ( |
|
The failing checks are unrelated to this PR, so I didn't push a fix. Failures:
Next step: Merge |
…piringSoonBannerPrecedence Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
I merged The failures came from a merge race on The merge had no conflicts. I couldn't run the tests locally, so the new CI run will confirm the fix. |
|
@MelvinBot Please update TestsPrecondition:
Offline testsSame as Tests step QA StepsSame as Tests step PR Author ChecklistMark all author checklist checked |
|
Updated the PR description:
I also added one line to the explanation for the |
Screen.Recording.2026-09-25.at.10.34.59.PM.mov |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-09-25.at.10.34.59.PM.mov |
ahmedGaber93
left a comment
ahmedGaber93
left a comment
There was a problem hiding this comment.
LGTM!
CC @Valforte since you review the orignal PR
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #101724 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@ahmedGaber93 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| it('still shows the pre-trial banner when the subscription is not about to expire', async () => { | ||
| // Given an annual subscriber whose subscription still auto-renews, and no free trial NVPs | ||
| await Onyx.merge(ONYXKEYS.NVP_PRIVATE_SUBSCRIPTION, { | ||
| type: CONST.SUBSCRIPTION.TYPE.ANNUAL, | ||
| autoRenew: true, | ||
| endDate: format(addDays(new Date(), 10), CONST.DATE.FNS_FORMAT_STRING), | ||
| }); | ||
| await waitForBatchedUpdatesWithAct(); | ||
|
|
||
| // When the Subscription page card section renders | ||
| renderCardSection(); | ||
| await waitForBatchedUpdatesWithAct(); | ||
|
|
||
| // Then the existing pre-trial banner is unchanged, because the expiring soon check does not apply | ||
| expect(screen.getByText('PreTrialBillingBanner')).toBeOnTheScreen(); | ||
| expect(screen.queryByText('SubscriptionExpiringSoonBanner')).not.toBeOnTheScreen(); | ||
| }); |
There was a problem hiding this comment.
Hmm isn't it kind of weird to show a free trial banner if the user is already on a subscription?
There was a problem hiding this comment.
@amyevans You are right. I was trying to quickly understand how pre-trial works, but I couldn't find when it should be displayed. The subscription page isn't available until the user creates a workspace, and when a user creates a workspace, the initial state is a free trial. This looks like old code or something controlled by backend NVP values.
It was added here: #43670
Since this has been the current behavior on main for a long time, I decided to keep it as is and not widen the PR scope.
There was a problem hiding this comment.
@MelvinBot Can you find the actual test steps (not fake Onyx data) to display this banner? Also, is it intentional on the current main for the pre-trial banner to be displayed when the user is already on a subscription?
|
1. Real test steps: You can't fully reach this state without faking the end date. The app never sends or sets
Caution: once you're on Annual, you may not be able to switch back to Pay-per-use. 2. Pre-trial banner for a subscriber: It's not intentional. It's an unhandled edge case. This mostly affects older annual owners who never got trial dates. I couldn't confirm how many exist. A new owner gets trial dates as soon as the server finishes creating the workspace. That weakens my earlier reason for not adding a Supporting details
|
@MelvinBot Isn't this the correct behavior? If the trial dates are missing, shouldn't we consider the user to be in pre-trial? What's wrong with that? |
|
For a new workspace owner, yes. Missing trial dates means pre-trial, and this PR doesn't change that. The problem is that missing trial dates can also mean something else: an owner who is already paying and never had a trial. For example, an owner on an Annual subscription is paying by definition. Showing Start a free trial to them is wrong. The same file already handles this case differently. In practice, this only matters if Annual owners without trial dates exist. I couldn't confirm that they do. If you're fine treating them as out of scope, no change is needed. This PR's reorder already covers the expiring-soon case. |
|
@amyevans I think the best choice now is to remove that test case and leave Please let me know if you’d like to handle it here. |
|
@MelvinBot remove this test case #102307 (comment) and leave |
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
Done in e7aca72. I removed the This PR doesn't change |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Yeah, this whole "pre-trial" thing I don't think is a case anymore is it? I believe a few years ago we didn't start the trial until the first action was taken, but we stopped doing that or something. |
|
@trjExpensify assigning you since you've been involved in the issue. |
Yeah I am not entirely sure of the history, but I dug into it a bit with AI and we could not find any accounts that would fall into this in practice.
Agreed with your assessment, thanks for removing it! |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Cool, works for me too. 👍
|
🚧 amyevans has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/amyevans in version: 9.4.95-0 🚀
|
|
No help site changes are required for this PR, so I didn't create a docs PR. Why
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.95-4 🚀
|
Explanation of Change
Follow-up to Alert subscription owners when their annual subscription is about to expire with auto-renew off.
On Account > Subscription, the card section shows only the first banner that matches. The expiring-soon banner was checked last, after four trial banners. The pre-trial check (
shouldShowPreTrialBillingBanner) also returnstruewhen the free trial NVPs are absent. So an annual subscriber without those NVPs saw Start a free trial instead of Your subscription ends …, while Home and the green dot showed the warning correctly.This PR checks
shouldShowSubscriptionExpiringSoonUIfirst. It now matches the order the green dot already uses. The billing status override below the chain is unchanged.getFreeTrialTextnow also returns nothing when the expiring-soon warning applies. So the Account menu, LHN, and report header no longer show a free trial badge next to the expiring-soon banner.I didn't add a
firstDayFreeTrialguard toshouldShowPreTrialBillingBanner. The help site says the trial starts once you start using your workspace. So a new owner in the pre-trial window may have no trial NVPs yet. The guard could hide that banner and the matching Start a free trial badge fromgetFreeTrialText. Reordering can't affect new owners, because the expiring-soon check requires an annual subscription with auto-renew off and an end date inside one month.AI Tests
tests/ui/SubscriptionCardSectionBannerTest.tsx: passes. The expiring-soon case fails without the fix.tests/unit/SubscriptionUtilsTest.ts(including a newgetFreeTrialTextexpiring-soon case),tests/unit/CardsSectionUtilsTest.ts: pass.npm run typecheck,npm run lint,npm run fmt(no diff): pass.react-compiler-compliance-check checkonCardSection.tsx,FreeTrial.tsx, anduseInitialSettingsPageMenuData.ts: pass.check-changedcouldn't run locally because the base ref resolved to an empty value.Fixed Issues
$ #101724
PROPOSAL: #101741 (comment)
Tests
Precondition:
Onyx.merge('nvp_private_firstDayFreeTrial', null)andOnyx.merge('nvp_private_lastDayFreeTrial', null)Onyx.merge('nvp_private_subscription', {type: 'yearly2018', autoRenew: false, endDate: '2026-10-22'})Offline tests
Same as Tests step
QA Steps
Same as Tests step
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari