[PM-36969] feat: Surface subscription substate to premium gates#6931
[PM-36969] feat: Surface subscription substate to premium gates#6931SaintPatrck wants to merge 1 commit into
Conversation
The server keeps `Profile.Premium=true` during the grace window after a subscription enters a recovery or terminal state, so callers that gate purely on `isPremium` either route users away from the Plan-screen badge that explains their situation (PM-36969, PM-36970, PM-37181) or suppress the upgrade CTA users need to recover their account (PM-37093, PM-37177, PM-37180). Expose the Stripe substate via a new `PremiumStateManager.subscriptionStatusStateFlow` and use it to compute "effectively premium" for banner eligibility and Plan-screen routing. Renames the misleading `OVERDUE_PAYMENT` enum value to `UPDATE_PAYMENT` so the badge label matches Figma. The subscription endpoint 404s for free users (no `GatewaySubscriptionId`); a new `SubscriptionResult.NotFound` maps that case to "free, show upgrade CTA" instead of an error dialog.
SaintPatrck
added
the
ai-review-vnext
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR introduces a new Code Review Details
Additional minor observations (not posted inline):
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6931 +/- ##
==========================================
+ Coverage 86.29% 86.34% +0.04%
==========================================
Files 856 867 +11
Lines 62034 62580 +546
Branches 9017 9067 +50
==========================================
+ Hits 53532 54032 +500
- Misses 5404 5433 +29
- Partials 3098 3115 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @OptIn(ExperimentalCoroutinesApi::class) | ||
| override val subscriptionStatusStateFlow: StateFlow<SubscriptionStatusState> = | ||
| authRepository | ||
| .userStateFlow | ||
| .map { it?.activeAccount?.userId } | ||
| .distinctUntilChanged() | ||
| .flatMapLatest { userId -> | ||
| if (userId == null) { | ||
| flowOf(SubscriptionStatusState.NoSubscription) | ||
| } else { | ||
| fetchSubscriptionStatusFlow() | ||
| } | ||
| } | ||
| .stateIn( | ||
| scope = unconfinedScope, | ||
| started = SharingStarted.Eagerly, | ||
| initialValue = SubscriptionStatusState.Loading, | ||
| ) |
There was a problem hiding this comment.
subscriptionStatusStateFlow fetches subscription for every signed-in user, including free users, even though the interface KDoc says fetches happen "lazily when the active account is premium."
Details and fix
The interface contract in PremiumStateManager.kt says:
"Fetches lazily when the active account is premium; emits
SubscriptionStatusState.NoSubscriptionfor non-premium accounts and for 404 responses..."
But the implementation only short-circuits on userId == null — any signed-in user (free or premium) triggers fetchSubscriptionStatusFlow(), which hits GET /accounts/subscription. For free users this is an unconditional 404 round-trip on every app launch and every push (since subscriptionRefreshTriggerFlow always re-emits regardless of premium state).
Suggested gate inside flatMapLatest:
.flatMapLatest { userId ->
val activeAccount = authRepository.userStateFlow.value?.activeAccount
when {
userId == null -> flowOf(SubscriptionStatusState.NoSubscription)
activeAccount?.isPremium != true -> flowOf(SubscriptionStatusState.NoSubscription)
else -> fetchSubscriptionStatusFlow()
}
}You'd still want the push handler to nudge the flow re-evaluate when a data.isPremium == true push arrives for the active user (e.g., by re-keying on a combined (userId, isPremium) flow, or by triggering subscriptionRefreshTriggerFlow only after the user flips premium).
Either align the implementation to the documented "lazy on premium only" behavior, or update the KDoc to reflect that fetches happen for any signed-in user.
| SubscriptionResult.NotFound -> { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| viewState = PlanState.ViewState.Free( | ||
| rate = PLACEHOLDER_TEXT, | ||
| checkoutUrl = null, | ||
| isAwaitingPremiumStatus = false, | ||
| ), | ||
| dialogState = null, | ||
| ) | ||
| } | ||
| viewModelScope.launch { | ||
| sendAction( | ||
| PlanAction.Internal.PricingResultReceive( | ||
| result = billingRepository.getPremiumPlanPricing(), | ||
| ), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
❓ QUESTION: When Account.isPremium=true is granted via organization membership (not personal Stripe), getSubscription() returns 404 → NotFound. This handler then switches the user from the initial Premium view to the Free upgrade view, prompting them to "Upgrade to Premium" even though they already have premium via their org. Is the assumption that PM-37465 (gating the Plan entry on personal premium) lands before this PR, preventing org-only premium users from ever reaching this state? If those PRs land out of order or the Plan screen is reached via a path that doesn't honor the personal-premium gate (e.g., modal from a feature gate), org-premium users will see the upgrade pitch.
| mockkStatic("com.bitwarden.network.model.BitwardenErrorKt") | ||
| every { throwable.toBitwardenError() } returns bitwardenHttpError | ||
| coEvery { | ||
| billingService.getSubscription() | ||
| } returns throwable.asFailure() | ||
|
|
||
| val result = repository.getSubscription() | ||
|
|
||
| assertEquals(SubscriptionResult.NotFound, result) | ||
| unmockkStatic("com.bitwarden.network.model.BitwardenErrorKt") |
There was a problem hiding this comment.
🎨 SUGGESTED: Move mockkStatic/unmockkStatic to @BeforeEach/@AfterEach to prevent leaked static mocks if an assertion throws.
Details and fix
If an assertion in the body throws before reaching the inline unmockkStatic(...) line, the static mock for BitwardenErrorKt stays installed and leaks into the next test in the suite, which can produce confusing failures. The codebase already follows the @AfterEach pattern for unmockkStatic (e.g., TotpIntentUtilsTest.kt).
@BeforeEach
fun setup() {
mockkStatic("com.bitwarden.network.model.BitwardenErrorKt")
}
@AfterEach
fun tearDown() {
unmockkStatic("com.bitwarden.network.model.BitwardenErrorKt")
}Then drop the inline mockkStatic/unmockkStatic calls from the two new tests.
1 participant
🎟️ Tracking
📔 Objective
The server keeps
Profile.Premium=trueduring the grace window after a subscription enters a recovery or terminal Stripe state. Surfaces that gate purely onisPremiumeither suppress the Plan-screen badge that would explain the user's situation, or hide the upgrade CTA users need to recover their account.Adds
PremiumStateManager.subscriptionStatusStateFlow(Loading/NoSubscription/Available/Error) so callers can derive "effectively premium" from both the account flag and the Stripe substate. The upgrade banner now flips back on when the active subscription is in a trouble state (past_due,update_payment,canceled,paused) even while the server still reports premium. The Plan screen routes free-with-trouble-substate users to the Premium view so they see the right badge and Manage/Resubscribe affordances.Renames
OVERDUE_PAYMENT → UPDATE_PAYMENTso the badge label matches the Figma frame. A newSubscriptionResult.NotFoundmaps the 404 returned for users without aGatewaySubscriptionIdto "free, show upgrade CTA" instead of an error dialog.📸 Screenshots