[PM-38145] feat: Add View Bank Account screen#2769
Conversation
SaintPatrck
added
ai-review
SaintPatrck
force-pushed
the
vault/pm-38145-ios-bank-account-view
branch
from
92741cb to
f6ed75b
Compare
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the read-only View Bank Account screen feature (PM-38145): the new Code Review DetailsNo blocking findings. The four existing review threads (state-file placement of the computed property, Notes verified during review:
|
|
Warning @SaintPatrck Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details. |
Add the read-only detail view for Bank Account vault items. Each non-empty field is shown, with copy buttons on the identifying numbers (account number, routing number, SWIFT code, IBAN, PIN) and reveal toggles masking the account number, PIN, and IBAN by default. The whole section is hidden when the item has no bank account details. Wires the new view into the existing ViewItem dispatch, action, processor, and state plumbing, mirroring the established card and identity view patterns. Reuses BankAccountItemState (from the add/edit surface) so no new localization keys are required.
SaintPatrck
force-pushed
the
vault/pm-38145-ios-bank-account-view
branch
from
f6ed75b to
0d59623
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2769 +/- ##
==========================================
+ Coverage 87.93% 87.94% +0.01%
==========================================
Files 1728 1741 +13
Lines 168727 170500 +1773
==========================================
+ Hits 148365 149951 +1586
- Misses 20362 20549 +187 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fedemkr
left a comment
fedemkr
left a comment
There was a problem hiding this comment.
Looks good, have some minor suggestions but they shouldn't block the PR.
I think with Matt suggested changes this would be good to approve.
|
|
||
| /// The account type field, displaying the selected type's localized name, hidden when no type is selected. | ||
| @ViewBuilder private var accountTypeItem: some View { | ||
| if case let .custom(type) = store.state.accountType { |
There was a problem hiding this comment.
⛏️ There's a computed property customValue to get the value directly:
| if case let .custom(type) = store.state.accountType { | |
| if let type = store.state.accountType.customValue { |
| accessibilityIdentifier: "ShowBankAccountNumberButton", | ||
| isPasswordVisible: isVisible, | ||
| ) { | ||
| store.send(.bankAccountItemAction(.toggleAccountNumberVisibilityChanged(!isVisible))) |
There was a problem hiding this comment.
🎨 If possible, I would like us to move to the approach used in ViewSSHKeyItemView where we don't pass the final result of the state and just fire the action of toggling the visibility leaving the actual logic to the processor. This would align better with the architecture of the Unidirectional data flow.
Just in case, this is not blocking the PR and we can just tackle this when we refactor the other places where this approach is currently being used.
Same applies to IBAN and PIN item views.
There was a problem hiding this comment.
Logged https://bitwarden.atlassian.net/browse/PM-39242 to track this refactor.
- Move isBankAccountDetailsSectionEmpty into BankAccountItemState, relocating its tests to BankAccountItemStateTests - Group bank account cases under a MARK in CopyableField - Use accountType.customValue in place of the case-let unwrap
3 participants
🎟️ Tracking
📔 Objective
Adds the read-only View Bank Account detail screen for the Password Manager.
📸 Screenshots