Skip to content

[PM-37920] fix: Show Storage cost row when additional storage is present#6997

Merged
SaintPatrck merged 4 commits into
mainfrom
PM-37920/show-storage-cost-row-when-present
May 29, 2026
Merged

[PM-37920] fix: Show Storage cost row when additional storage is present#6997
SaintPatrck merged 4 commits into
mainfrom
PM-37920/show-storage-cost-row-when-present

Conversation

@SaintPatrck
Copy link
Copy Markdown
Contributor

🎟️ Tracking

PM-37920

📔 Objective

The Plan screen's cost breakdown hid the "Storage cost" row whenever the
storage cost was null or zero, wrongly suppressing a legitimate
present-but-free $0.00 storage line — additional storage that exists but
currently costs nothing.

This gates the row on the presence of additional storage (storageCost
non-null) rather than its value, so a $0.00 line still renders while a truly
absent line stays hidden. A dedicated toPresentMoneyText() helper handles
the present-when-non-null case; the Discount line keeps its existing
hide-at-zero behavior.

Covered by unit tests: null storage hides the row, zero storage renders
$0.00, and zero discount still hides — no device screenshots needed.

The cost breakdown suppressed the Storage cost row whenever the storage cost
was null or zero, hiding a legitimate present-but-free $0.00 storage line.
Gate the row on the presence of additional storage rather than its value, so
a $0.00 line still renders while a truly absent line stays hidden.
@SaintPatrck SaintPatrck added the ai-review-vnext Request a Claude code review using the vNext workflow label May 29, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug labels May 29, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 29, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR fixes a Plan screen bug where the Storage cost row was suppressed for present-but-free additional storage by re-gating the row on the presence of storageCost rather than its value. The money-formatting helpers were extracted from PlanViewModel into a dedicated BigDecimalExtensions.kt and now take an explicit NumberFormat, improving testability. New unit tests cover each helper, and the existing PlanViewModelTest was updated to assert the new $0.00 storage row and the Unicode minus discount text.

Code Review Details

No actionable findings. The bug fix is correctly scoped to the presence-vs-value distinction documented in SubscriptionInfo.storageCost (null = no additional storage), the Discount line preserves its hide-when-non-positive behavior, the helper extraction is self-contained with no orphan references, and the Unicode minus sign change for the Discount line was already discussed and accepted in an inline thread.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.97%. Comparing base (b57fb9c) to head (def9ea9).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6997      +/-   ##
==========================================
- Coverage   86.26%   85.97%   -0.29%     
==========================================
  Files        1008      918      -90     
  Lines       66332    65535     -797     
  Branches     9308     9282      -26     
==========================================
- Hits        57221    56344     -877     
- Misses       5902     6003     +101     
+ Partials     3209     3188      -21     
Flag Coverage Δ
app-data 17.02% <0.00%> (-0.50%) ⬇️
app-ui-auth-tools 18.86% <0.00%> (-0.47%) ⬇️
app-ui-platform 16.61% <100.00%> (-0.42%) ⬇️
app-ui-vault 28.36% <0.00%> (+0.05%) ⬆️
authenticator 6.21% <0.00%> (+<0.01%) ⬆️
lib-core-network-bridge 4.09% <0.00%> (+<0.01%) ⬆️
lib-data-ui 1.14% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SaintPatrck SaintPatrck marked this pull request as ready for review May 29, 2026 16:28
@SaintPatrck SaintPatrck requested review from a team and david-livefront as code owners May 29, 2026 16:28
fun BigDecimal?.toDiscountMoneyText(currencyFormatter: NumberFormat): String? =
this
?.takeIf { it.signum() > 0 }
?.let { "-${currencyFormatter.format(it)}" }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is technically unchanged from the previous code but should this minus be \u2212?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Yes, since we're dealing with dollhairs

@SaintPatrck SaintPatrck enabled auto-merge May 29, 2026 16:59
@SaintPatrck
Copy link
Copy Markdown
Contributor Author

Thanks @david-livefront

@SaintPatrck SaintPatrck added this pull request to the merge queue May 29, 2026
Merged via the queue into main with commit aca9949 May 29, 2026
25 checks passed
@SaintPatrck SaintPatrck deleted the PM-37920/show-storage-cost-row-when-present branch May 29, 2026 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review-vnext Request a Claude code review using the vNext workflow app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants