[PM-35757] feat: Show pricing error banner when premium price fetch fails#2590
Conversation
github-actions
Bot
added
app:password-manager
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## pm-35101/complete-stripe-payment #2590 +/- ##
====================================================================
+ Coverage 86.13% 86.15% +0.01%
====================================================================
Files 2124 2124
Lines 182803 182934 +131
====================================================================
+ Hits 157457 157600 +143
+ Misses 25346 25334 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
andrebispo5
changed the base branch from
main
to
pm-35101/complete-stripe-payment
There was a problem hiding this comment.
Pull request overview
Adds resilient premium pricing display to PremiumUpgradeView by fetching the premium plan price on appear and showing a retryable “Pricing unavailable” banner when the fetch fails, hiding the upgrade CTA while pricing is unavailable.
Changes:
- Fetch premium plan pricing via
BillingService.getPremiumPlan()on view appear (and retry) and store it inPremiumUpgradeState. - Introduce a pricing error banner with “Try again” and dismiss actions; hide the upgrade button while the banner is shown.
- Add ViewInspector + processor tests and new English localization strings for the banner copy.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| BitwardenShared/UI/Billing/PremiumUpgrade/PremiumUpgradeView.swift | Shows pricing error banner; hides upgrade CTA when pricing is unavailable; hides price section when premiumPrice is nil. |
| BitwardenShared/UI/Billing/PremiumUpgrade/PremiumUpgradeView+ViewInspectorTests.swift | Adds UI tests for pricing banner visibility and button behaviors; verifies upgrade button hiding. |
| BitwardenShared/UI/Billing/PremiumUpgrade/PremiumUpgradeState.swift | Makes premiumPrice optional and adds showPricingErrorBanner state. |
| BitwardenShared/UI/Billing/PremiumUpgrade/PremiumUpgradeProcessorTests.swift | Adds processor tests for successful/failed price fetch on appear and retry; updates coordinator alert expectation. |
| BitwardenShared/UI/Billing/PremiumUpgrade/PremiumUpgradeProcessor.swift | Implements async price fetch + error banner state transitions; wires retry and dismiss actions. |
| BitwardenShared/UI/Billing/PremiumUpgrade/PremiumUpgradeEffect.swift | Adds .retryFetchPriceTapped effect for the banner retry button. |
| BitwardenShared/UI/Billing/PremiumUpgrade/PremiumUpgradeAction.swift | Adds .dismissPricingErrorBannerTapped action for banner dismissal. |
| BitwardenResources/Localizations/en.lproj/Localizable.strings | Adds English strings for “Pricing unavailable” and “Check your connection and try again.” |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| state.premiumPrice = formatter.string(from: NSDecimalNumber(decimal: plan.seat.price)) | ||
| state.showPricingErrorBanner = false |
There was a problem hiding this comment.
NumberFormatter.string(from:) can return nil (e.g., unexpected locale/currency configuration). Currently that results in premiumPrice == nil while also forcing showPricingErrorBanner = false, leaving the UI with no price and no error. Treat a nil formatted result as a failure (show the banner) and/or provide a safe fallback string.
| state.premiumPrice = formatter.string(from: NSDecimalNumber(decimal: plan.seat.price)) | |
| state.showPricingErrorBanner = false | |
| if let premiumPrice = formatter.string(from: NSDecimalNumber(decimal: plan.seat.price)) { | |
| state.premiumPrice = premiumPrice | |
| state.showPricingErrorBanner = false | |
| } else { | |
| state.premiumPrice = nil | |
| state.showPricingErrorBanner = true | |
| } |
| } catch { | ||
| services.errorReporter.log(error: error) | ||
| state.showPricingErrorBanner = true | ||
| } |
There was a problem hiding this comment.
On failure you set showPricingErrorBanner = true but never clear state.premiumPrice. If a previously fetched price exists, the view can show a stale price alongside the “Pricing unavailable” banner. Clear premiumPrice when starting a fetch and/or in the catch path so error state can’t display outdated pricing.
There was a problem hiding this comment.
@andrebispo5 I don't think this will ever happen, but doesn't harm to clear it as well.
There was a problem hiding this comment.
yup this is not going to happen that is why I didn't paid much attention to it.
| /// `perform(_:)` with `.appeared` fetches the premium price and sets it in state on success. | ||
| @Test | ||
| func perform_appeared_selfHosted() async { | ||
| environmentService.region = .selfHosted | ||
| func perform_appeared_fetchesPremiumPrice_success() async { | ||
| environmentService.region = .unitedStates | ||
|
|
||
| await subject.perform(.appeared) | ||
|
|
||
| #expect(subject.state.isSelfHosted == true) | ||
| #expect(subject.state.premiumPrice != nil) | ||
| #expect(subject.state.showPricingErrorBanner == false) | ||
| #expect(billingService.getPremiumPlanCalled) | ||
| } |
There was a problem hiding this comment.
The new tests only assert premiumPrice != nil after fetching, so they won’t catch incorrect cadence/formatting (e.g., showing $19.80 with a “per month” label). Add an assertion for the expected formatted monthly price (using a fixed test locale) to validate the displayed value matches the UI copy.
| let plan = try await services.billingService.getPremiumPlan() | ||
| let formatter = NumberFormatter() | ||
| formatter.numberStyle = .currency | ||
| formatter.locale = Locale.current | ||
| state.premiumPrice = formatter.string(from: NSDecimalNumber(decimal: plan.seat.price)) | ||
| state.showPricingErrorBanner = false |
There was a problem hiding this comment.
PremiumUpgradeView labels this value as “per month”, but fetchPremiumPrice() formats plan.seat.price directly. The premium plan fixture/model uses Stripe IDs like premium-annually-* with price: 19.80, so this will display an annual price as a monthly price. Convert the returned price to a monthly amount (or otherwise match the billing cadence) before formatting, and consider asserting the expected formatted value in tests to prevent regressions.
There was a problem hiding this comment.
@andrebispo5 Correct me if I'm wrong but premiumPrice will be used in the view which has the monthly calculation, right?
There was a problem hiding this comment.
this comment is outdated I moved the formatting to the state and it is being divided as well. :)
andrebispo5
merged commit 3a4e645
into
pm-35101/complete-stripe-payment
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-35757
📔 Objective
Fetches the premium plan price from the billing service on appear in
PremiumUpgradeView. If the fetch fails, a "Pricing unavailable" error banner is shown with a "Try again" button and the upgrade button is hidden. On success (or retry), the price is displayed and the banner is dismissed.📸 Screenshots