[PM-39767] fix: Show Upgraded to Premium celebration screen after Sync Now succeeds - #2956
Draft
KatherineInCode wants to merge 3 commits into
Draft
[PM-39767] fix: Show Upgraded to Premium celebration screen after Sync Now succeeds#2956KatherineInCode wants to merge 3 commits into
KatherineInCode wants to merge 3 commits into
Conversation
…c Now succeeds Tapping "Sync Now" on the Upgrade Pending alert and having the retry succeed showed no celebration screen — only the existing Upgraded to Premium action card, indistinguishable from a delayed background sync resolving later. The celebration screen (PremiumUpgradeCompleteView) already existed and was already presented correctly (a full-screen card-style modal, matching Figma) for the synchronous confirm-while-upgrade-screen-is-open path; it just had no way to be reached from the Sync Now retry, since that path dismisses the upgrade screen (and its nested BillingCoordinator) before the retry even runs. Adds a new BillingRoute.premiumUpgradeCompleteStandalone, reachable from all five screens that can initiate an upgrade via a new PremiumUpgradeRoute.premiumUpgradeComplete requirement, mirroring the existing premiumUpgrade wiring in each. After a Sync Now retry resolves, DefaultPremiumUpgradeHelper checks whether the upgrade actually succeeded (not still pending, not failed) before navigating — deliberately not reusing the durable premiumUpgradePendingStatePublisher signal, since that also fires for delayed syncs that must never show this screen.
…undary Settings is the only origin that pushes PremiumUpgradeView rather than presenting it as a fresh modal root, and SettingsCoordinator's .dismiss only dismisses presented view controllers, never pops. The standalone celebration was being presented over a still-visible, now-stale PremiumUpgradeView that has no way to notice premium was granted (its .task-driven self-dismiss check only runs once). Pop it before presenting the celebration so closing returns to the real Settings screen. Also added the missing (isPending: false, lastAttemptFailed: true) case to the Sync Now success test — the existing cases were satisfied by the isPending clause alone, leaving the lastAttemptFailed clause unverified.
github-actions
Bot
added
app:password-manager
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## pm-39767/sync-unsuccessful #2956 +/- ##
==============================================================
+ Coverage 81.12% 81.78% +0.66%
==============================================================
Files 1038 1049 +11
Lines 67459 68114 +655
==============================================================
+ Hits 54724 55710 +986
+ Misses 12735 12404 -331 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… Sync Now An end-to-end walkthrough of the whole stack surfaced an inconsistency: the original "Sync Now" button (on the upgrade pending alert) checked whether the retry actually resolved the upgrade and showed the celebration screen on success, but "Try Again" on the "Sync unsuccessful" alert (PR3) only called premiumStatusChanged() and did nothing further — a retry that succeeded there produced only a silent CTA update, with no celebration, even though it's the same kind of explicit, user-initiated retry. Extracted the shared "retry then show celebration if resolved" logic into PremiumUpgradeRetry so both entry points behave identically.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-39767
📔 Objective
Fourth and final of four stacked PRs addressing QA follow-up findings on PM-39767 (base:
pm-39767/sync-unsuccessful, PR #2955).Tapping "Sync Now" on the "Upgrade Pending" alert and having the retry succeed showed no celebration screen — only the existing "Upgraded to Premium" action card, indistinguishable from a delayed background sync resolving later. The celebration screen (
PremiumUpgradeCompleteView) already existed and was already presented correctly (a full-screen card-style modal, matching Figma) for the synchronous confirm-while-upgrade-screen-is-open path; it just had no way to be reached from the Sync Now retry, since that path dismisses the upgrade screen (and its nestedBillingCoordinator) before the retry even runs.Adds a new
BillingRoute.premiumUpgradeCompleteStandalone, reachable from all five screens that can initiate an upgrade via a newPremiumUpgradeRoute.premiumUpgradeCompleterequirement, mirroring the existingpremiumUpgradewiring already in each. After a "Sync Now" retry resolves,DefaultPremiumUpgradeHelperchecks whether the upgrade actually succeeded (not still pending, not failed) before navigating — deliberately not reusing the durablepremiumUpgradePendingStatePublisher()signal, since that also fires for delayed syncs that must never show this screen.A local code review caught one real bug: Settings is the only origin that pushes
PremiumUpgradeViewrather than presenting it as a fresh modal root, andSettingsCoordinator's.dismissonly dismisses presented view controllers, never pops. The new celebration was being presented over a still-visible, now-stale upgrade screen with no way to notice premium had been granted. Fixed by popping it before presenting the celebration, scoped to this new path. The review also flagged a test gap (a guard clause left unverified by the existing test cases) — fixed by completing the boundary matrix.📸 Screenshots
Not applicable — reuses the existing celebration screen and presentation style; no new UI.
A manual end-to-end pass through the whole PM-39767 stack (not just this PR in isolation) surfaced one more inconsistency: the "Sync Now" button on the original pending alert checked for success and showed the celebration screen, but "Try Again" on PR3's "Sync unsuccessful" alert only retried and silently updated the CTA — no celebration, even though it's the same kind of explicit, user-initiated retry. Fixed by extracting the shared "retry, then show celebration if resolved" logic into
PremiumUpgradeRetryso both entry points behave identically.