[PM-32809] feat: add Bank Account foundation types and icon#2573
[PM-32809] feat: add Bank Account foundation types and icon#2573SaintPatrck wants to merge 3 commits intomainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2573 +/- ##
==========================================
- Coverage 87.20% 86.13% -1.08%
==========================================
Files 1894 2121 +227
Lines 167519 182358 +14839
==========================================
+ Hits 146087 157067 +10980
- Misses 21432 25291 +3859 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
62b9cab to
b1f457d
Compare
KatherineInCode
left a comment
There was a problem hiding this comment.
I realize this is still in draft, but there are a number of things I noticed, mostly relating to verbosity of comments
| "UserIDUnavailable" = "User ID unavailable"; | ||
| "ToManageYourPremiumSubscriptionDescriptionLong" = "To manage your Premium subscription, please log in to your web vault on a computer."; | ||
|
|
||
| /* PM-32009: New Item Types — Bank Account (Part 1/3 type-level keys) |
There was a problem hiding this comment.
🤔 This comment is really weird, and wouldn't be helpful for translators. I suggest removing it.
| /* PM-32009: New Item Types — Bank Account (Part 1/3 type-level keys) | ||
| Field labels, account-type display names, and processor alert strings are added | ||
| in PM-32809 Part 2/3 and Part 3/3 as the UI surfaces that consume them land. */ | ||
| "TypeBankAccount" = "Bank account"; |
There was a problem hiding this comment.
🎨 Our convention for this would be just a key BankAccount, because we try to match the English text as closely as possible
| /// | ||
| /// - Note: All members are `static` and side-effect free. The bridge does not hold state. | ||
| /// | ||
| enum NewItemTypesSdkBridge { |
There was a problem hiding this comment.
🤔 I don't love the idea of having an object hanging around called "new item types". If the plan is to eliminate this eventually, we probably should put an explicit TODO in here linking to the ticket to do just that.
There was a problem hiding this comment.
Agreed. This is an artifact of not having the SDK types available. I'm still trying to figure out a better way to split the work to avoid this entirely.
| /// | ||
| /// - Returns: The SDK enum case, or `nil` while the SDK is blocked on PM-32009. | ||
| /// | ||
| static func sdkCipherTypeForBankAccount() -> BitwardenSdk.CipherType? { |
There was a problem hiding this comment.
⛏️ These should be alphabetized within their section
|
|
||
| @testable import BitwardenShared | ||
|
|
||
| class BankAccountTypeTests: BitwardenTestCase { |
There was a problem hiding this comment.
🎨 This is a new test file, so should use Swift Testing
| /// The specific fields for the type of item being created or updated. | ||
| @ViewBuilder private var itemTypeSection: some View { | ||
| switch store.state.type { | ||
| // TODO: PM-32809 Part 3/3 wires `AddEditBankAccountItemView` here. Part 1 |
There was a problem hiding this comment.
💭 In my personal opinion, this should just be a TODO: PM-32809 implement view or something similarly concise
| key: key, | ||
| name: addEditState.name, | ||
| notes: addEditState.notes.nilIfEmpty, | ||
| // `BitwardenSdk.CipherType(_:)` is failable as of PM-32009 Part 1/3. |
There was a problem hiding this comment.
🤔 This feels over-explainy to me, and just a TODO—especially if the ticket number is a typo and it's just this same ticket, but a later PR—is sufficient
| @ViewBuilder private var itemInformationSection: some View { | ||
| // check for type | ||
| switch store.state.type { | ||
| // TODO: PM-32809 Part 3/3 wires `ViewBankAccountItemView` here. Part 1 |
There was a problem hiding this comment.
🤔 See above comments about concision
|
|
||
| init(cloneItem cipherView: CipherView, hasPremium: Bool) { | ||
| // Unknown SDK cipher types fall back to `.secureNote` on clone | ||
| // (PM-32009 Part 1/3 made `CipherType(type:)` failable; PM-32813 will |
There was a problem hiding this comment.
🤔 Per concision comments, I think just the first line of this is sufficient. Not everything has to explain all the future tickets that will touch this. This applies to pretty much all the comments being added in this file.
| iconBaseURL: nil, | ||
| showWebIcons: false, | ||
| type: .init(type: cipherView.type), | ||
| type: CipherType(type: cipherView.type), |
b1f457d to
a7552c1
Compare
a7552c1 to
6cfe443
Compare
…M-32809 Part 1/3) First of three vertical slices for PM-32809. Establishes the type- level foundation for the Bank Account cipher type without touching the vault-list plumbing (Part 2/3) or the add/edit/view UI surfaces (Part 3/3). Included: - `FeatureFlag.newItemTypes` (`pm-32009-new-item-types`) added to the flag registry. Gates the epic's three new cipher types at the flag layer; default OFF. - `CipherType.bankAccount = 6` added alongside `allCases`, `localizedName`, `allowedFieldTypes` (text / hidden / boolean — `.linked` is excluded per US-1.1), and the new `iconPlaceholder` helper that centralizes icon routing for PM-34128's eventual asset swap. - `CipherType.init(type:)` and `CipherType.init(_:)` become failable (`init?`) with `@unknown default: return nil`, so future SDK bumps no longer force a closed-switch compile break on unrelated PRs. Cascade `?? .secureNote` fallbacks land at the five existing call sites (`CipherItemState` clone / existing / apply, `CipherView+ Update.updatedView`, `CipherRequestModel`, `BitwardenSdk+Vault` response-model inits, `ExportVaultService` restricted-type filter). - `canCreateCasesBase` / `canCreateCasesWithNewItemTypes` constants plus the flag-aware `canCreateCases(isNewItemTypesEnabled:)` helper. The legacy static `canCreateCases` is gone; Part 2/3 wires the helper through `VaultRepository.getItemTypesUserCanCreate()`. - `BankAccountType` raw-value enum (Checking / Savings / CD / LineOfCredit / InvestmentBrokerage / MoneyMarket / Other). Ships without `Menuable` conformance in Part 1 — that conformance (and the seven display-name localization keys it depends on) lands in Part 3/3 when the menu field consumes it. - `CipherBankAccountModel` API model (Codable, Equatable, Sendable) with an explicit memberwise init defaulting every parameter to `nil`. Carries the serialization-tripwire docstring so future-you knows the model must never reach the wire directly — only through SDK encryption once the SDK exposes `BankAccount` / `BankAccountView`. - `CipherDetailsResponseModel` / `CipherMiniResponseModel` each gain `var bankAccount: CipherBankAccountModel? = nil` with the swiftformat/swiftlint pins that keep the `= nil` alive, so the synthesized memberwise init remains source-compatible with all pre-PM-32009 test call sites. - `NewItemTypesSdkBridge` (stateless enum shim) centralizes SDK- availability gating. `isBankAccountAvailable` is the single switch to flip when the SDK dependency is bumped; `sdkCipherTypeForBank Account()` and `appCipherTypeForBankAccount(_:)` route the conversions. PRs 2 and 3 (Driver's License, Passport) will extend this shim. - `BitwardenSdk.CipherType(_:)` becomes failable with a Debug-build `assertionFailure` tripwire on the `.bankAccount → nil` path so accidentally enabling the flag before SDK readiness is caught in development. - `ViewItemState.navigationTitle` gets a `.bankAccount` arm using `Localizations.viewBankAccount`; `CipherItemState.navigationTitle` uses `Localizations.newItem` / `Localizations.edit` as Part 1 placeholders — Part 3/3 replaces those with `newBankAccount` / `editBankAccount`. - `LinkedIdType.getLinkedIdType(for:)` gains an empty-array arm for `.bankAccount` so the exhaustive switch keeps compiling. Tests: `BankAccountTypeTests` (raw values, ordering, JSON round-trip), `CipherBankAccountModelTests` (memberwise init, Codable round-trip), expanded `CipherTypeTests` (`canCreateCasesBase`, `canCreateCasesWithNewItemTypes`, `canCreateCases(isNewItemTypesEnabled:)`, `.bankAccount` on `allowedFieldTypes` / `localizedName` / `allCases`). Localization: ONLY `TypeBankAccount` and `ViewBankAccount` added. Field labels, account-type display names, and processor alert strings are deferred to the Part 2/3 and Part 3/3 slices that introduce the UI consuming them. SDK dependency: `BitwardenSdk` currently stops at `CipherType.sshKey = 5` with no `BankAccount` / `BankAccountView`. All SDK-facing paths are gated through `NewItemTypesSdkBridge` and TODO-marked with `PM-32009 Blocked on SDK`. The flag must remain OFF until the SDK is bumped — coordinate with PM-34060 (Nick Krantz's cross-platform SDK work). Ticket: PM-32809 (Part 1/3)
Folds in the two non-blocking nits from review Round 2: - `CipherItemState.swift`: Add the parallel `// TODO: PM-32809 Part 3/3` comment to the `.add → .bankAccount` branch so both the new/edit placeholder strings get picked up atomically when Part 3/3 swaps in `Localizations.newBankAccount` / `Localizations.editBankAccount`. - `BankAccountType.swift`: Drop the unused `import BitwardenResources`. The `Menuable` conformance (which needs the localization keys) is deferred to Part 3/3; the raw enum in Part 1 doesn't need the import. Also tightens three cascade breaks that appeared after the previous build pass — all stem from `CipherType.bankAccount` becoming a case on an exhaustive switch / a renamed static: - `View.swift`, `VaultListState.swift`, `VaultGroupState.swift`: the three default-parameter sites that previously pointed at the legacy `CipherType.canCreateCases` static (now removed in favor of the flag-aware helper) now seed from `CipherType.canCreateCasesBase`. The processors continue to override these defaults via `VaultRepository.getItemTypesUserCanCreate()` — that wiring lands in Part 2/3. - `AddEditItemView.swift` (`itemTypeSection`) and `ViewItemDetailsView.swift` (`itemInformationSection`): add an `EmptyView()` placeholder for `.bankAccount` so the exhaustive switch compiles. Creation is flag-gated upstream, so these branches are unreachable until Part 3/3 wires the real `AddEditBankAccountItemView` / `ViewBankAccountItemView` — TODOs pinned at both sites. Ticket: PM-32809 (Part 1/3)
Relies solely on the `newItemTypes` feature flag to gate bank account creation end-to-end. Adds the final PM-34128 bank account icon asset.
6cfe443 to
1d619c0
Compare
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the Bank Account foundation slice (PM-32809 Part 1/3): Code Review DetailsNo new findings. The earlier review thread concerns (Int → String raw type for |
🎟️ Tracking
📔 Objective
Lay down the Bank Account foundation that doesn't require SDK changes:
BankAccountTypeenum,CipherBankAccountModel, response-model properties,newItemTypesfeature flag, and icon asset. TheCipherType.bankAccountenum case is intentionally deferred to a follow-up PR paired with the sdk-swift bump that landsBitwardenSdk.BankAccount/BankAccountView— matching the convention used for every prior new item type.