[PM-41291] feat: Add model and data layer for Identity Autofill - #7232
[PM-41291] feat: Add model and data layer for Identity Autofill#7232aj-rosado wants to merge 4 commits into
Conversation
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the Identity Autofill model/data layer: new Code Review DetailsNo findings at or above the reporting threshold. Notes considered and intentionally not raised as findings:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7232 +/- ##
==========================================
- Coverage 86.19% 84.31% -1.89%
==========================================
Files 936 1132 +196
Lines 66989 69160 +2171
Branches 9860 10047 +187
==========================================
+ Hits 57742 58311 +569
- Misses 5738 7267 +1529
- Partials 3509 3582 +73
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
aj-rosado
added
t:feature
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| internal val IdentityView.identityAddress: String? | ||
| get() = listOfNotNull( | ||
| address1, | ||
| address2, | ||
| address3, | ||
| listOf(city ?: "-", state ?: "-", postalCode ?: "-") | ||
| .nullIfAllEqual("-") | ||
| ?.joinToString(", "), | ||
| country, | ||
| ) | ||
| .joinToString("\n") | ||
| .orNullIfBlank() |
There was a problem hiding this comment.
identityAddress is a display formatter; reusing it as AutofillCipher.Identity.fullAddress will fill - placeholders and newlines into form fields.
Details and fix
identityAddress was written for the vault item detail screen, where - placeholders and \n separators render acceptably. AutofillCipher.Identity.fullAddress is documented as "address parts joined for filling a combined full-address field", so the later fill phase will push this exact string into a PostalAddressFull input.
The new test in CipherViewExtensionsTest.kt locks in the problematic output:
assertEquals("address3\n-, state, -", identityView.identityAddress)For a cipher with only state populated, the user's address field would be filled with a literal -, state, -. Newlines in a single-line EditText are also dropped or rendered as spaces depending on the app.
Suggested fix: keep identityAddress for display and add a separate autofill-oriented formatter that omits missing parts entirely (no - placeholders) and joins with ", " — e.g. listOfNotNull(address1, address2, address3, city, state, postalCode, country).joinToString(", ").orNullIfBlank().
Same root cause applies to identityName: it prefixes the title, so a PersonNameFull field would be filled with Mr John Doe even though PersonNamePrefix exists as its own view for the title.
Since fill is a no-op in this phase, this is not user-visible yet — but the shape is being locked in here and asserted by tests, so it's cheaper to correct now than in the fill phase.
There was a problem hiding this comment.
@claude I agree with you. Reverted this method to the CipherViewExtensions it originally belonged and added a new one for the autofill
| // Identity hint detection/dispatch is wired up in a later phase; treating these as Unused for | ||
| // now is a no-op since nothing yet produces an IDENTITY_* hint. | ||
| else -> { | ||
| AutofillView.Unused( | ||
| data = autofillViewData, | ||
| ) | ||
| } |
There was a problem hiding this comment.
♻️ DEBT: The else -> branch removes when exhaustiveness over AutofillHint, so a missed hint in the next phase compiles silently.
Details and fix
Everywhere else in this PR the new sealed-class/enum members are enumerated explicitly, which is what makes the "later phase will have to touch this" guarantee work. Here the else does the opposite: after the 17 IDENTITY_* entries are added to AutofillHint, the compiler can no longer tell the next author which ones still need a dispatch case, and any hint they forget quietly becomes AutofillView.Unused — a silent no-fill rather than a build failure.
Suggested fix: list the identity hints in a single grouped branch instead of else:
AutofillHint.IDENTITY_PERSON_NAME_FULL,
AutofillHint.IDENTITY_PERSON_NAME_PREFIX,
// ... remaining IDENTITY_* entries
-> {
// Identity hint dispatch is wired up in a later phase.
AutofillView.Unused(data = autofillViewData)
}This keeps the same inert behavior while preserving the compile-time check.
982905e to
6f0c34c
Compare
| saveCallback.onSuccess(intentSender) | ||
| } else { | ||
| saveCallback.onSuccess() | ||
| } |
There was a problem hiding this comment.
Can we simplify this:
autofillRequest
.toAutofillSaveItem()
?.let { autofillSaveItem ->
createAutofillSavedItemIntentSender(
autofillAppInfo = autofillAppInfo,
autofillSaveItem = autofillSaveItem,
)
}
?.let { saveCallback.onSuccess(it) }
?: saveCallback.onSuccess()| } | ||
| ?.let { nonNullCipherListView -> | ||
| nonNullCipherListView.id?.let { cipherId -> | ||
| decryptCipherOrNull(cipherId = cipherId)?.let { cipherView -> |
There was a problem hiding this comment.
We shouldn't do this in this PR but we might want to consider optimizing this flow in the future with new DB functions.
We can make a make specific DB queries to fetch the relevant ciphers only, filtering by type, active, and reprompt. That should make for a fairly meaningful performance boost.
What you have here conforms to the existing pattern and seems perfectly fine for now though.
| is AutofillPartition.Identity -> { | ||
| // Capturing identity data from a filled form is out of scope. This is never actually | ||
| // reached because AutofillPartition.Identity.canPerformSaveRequest is always false, so | ||
| // SaveInfo (and therefore a save callback) is never built for it. |
| val licenseNumber: String, | ||
| ) : AutofillCipher() { | ||
| override val iconRes: Int | ||
| @DrawableRes get() = BitwardenDrawable.ic_id_card |
There was a problem hiding this comment.
Is this the correct icon?
There was a problem hiding this comment.
yes 😅 The one for cards is ic_payment_card this is the used on each Identity scenario
| postalCode = identityView?.postalCode.orEmpty(), | ||
| country = identityView?.country.orEmpty(), | ||
| company = identityView?.company.orEmpty(), | ||
| email = identityView?.email.orEmpty(), |
There was a problem hiding this comment.
I see we have the orEmpty usages in the other spots too, does this cause autofill to clear the value if the user has already typed into the field?
There was a problem hiding this comment.
No, later on the flow we are ignoring empty fields
0cc466b to
532e403
Compare
| is AutofillView.Identity.AddressCountry -> { | ||
| this.copy(data = this.data.copy(website = site)) | ||
| } | ||
| is AutofillView.Identity.AddressLocality -> { |
There was a problem hiding this comment.
Wanna run the formatter on this file, you should have some newlines in here
47cfacc to
4de9092
Compare
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41291
📔 Objective
Phase A/B of Identity Autofill (PM-38138): adds the model layer and data-layer plumbing that later phases build on.
AutofillView.Identity,AutofillPartition.Identity,AutofillCipher.Identity, and the correspondingAutofillHintentries.AutofillCipherProvider.getIdentityAutofillCiphers()(+ implementation) to fetch identity ciphers, mirroring the existinggetCardAutofillCiphers().whenforced by the new sealed-class members is completed now, either with permanent trivial logic or an explicit inert stub (e.g.AutofillRequest.Unfillable) commented to say which later phase replaces it — no behavior change yet.AutofillCipherProviderimplementation (CipherViewExtensions.kt'stoAutofillCipherProvider(), used by the manual-selection/accessibility completion flow) is updated in parallel so the two don't drift.This PR is intentionally behavior-neutral — nothing yet classifies a field as Identity, so none of this is reachable in production. Detection and fill land in later stacked phases (heuristic detection, fill-assist mapping, and the identity partition build-out).
📸 Screenshots
N/A — model/data layer only, no UI changes.