-
Notifications
You must be signed in to change notification settings - Fork 949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Autofill: Increase ratio of complete credential saves #5386
Autofill: Increase ratio of complete credential saves #5386
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
internal data class AutofillStoreFormDataJsonRequest( | ||
val credentials: AutofillStoreFormDataCredentialsJsonRequest?, | ||
val trigger: FormSubmissionTriggerType?, | ||
) | ||
|
||
internal data class AutofillStoreFormDataCredentialsJsonRequest( | ||
val username: String?, | ||
val password: String?, | ||
val autogenerated: Boolean = false, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a small tidy up as these are types specifically for Moshi / JSON parsing, used internally in this class. but we don’t necessarily want the same types returned out of this class
for example, the trigger
is nullable when parsing from JSON, but when returning we also want that non-nullable and defaulting to UNKNOWN
if it couldn’t be parsed from the JSON
9578c3a
to
969ed0e
Compare
a36b674
to
16e1970
Compare
2e53317
to
a24930b
Compare
40c14e9
to
d47c3a5
Compare
740b26f
to
62a3e7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM nothing major, we can discuss offline.
...fill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/AutofillJavascriptInterface.kt
Outdated
Show resolved
Hide resolved
...fill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/AutofillJavascriptInterface.kt
Outdated
Show resolved
Hide resolved
import javax.inject.Inject | ||
import timber.log.Timber | ||
|
||
interface UsernameBackFiller { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is confusing here, but unsure what to recommend to change because I might be missing some details. So leaving my impressions:
- when reviewing the implementation part, I got confused in
if (!usernameFromJavascript.isNullOrBlank())
. I was not expecting thatisBackFillingUsernameSupported
requires an empty username. - The logic inside just checks that nullability of the username and then calls
getUsernameForBackFilling
. why is not the caller checking that instead? I imagine this as "I don't have a username, let me check if I have a username for this url in the store", and then call that directly. - If you prefer to have a class for that, then we should update the interface to make it more clear. A way how I imagine this, giving the responsability of completing partial
LoginCredentials
. You provideLoginCrendentials
, andurl
, and this class can complete the missing data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should update the interface to make it more clear.
Happy to, though not totally sure what your suggestion is… can you elaborate? and how strongly do you feel? I’d rather not make deeper changes now if it could be avoided, tbh.
giving the responsability of completing partial LoginCredentials. You provide LoginCrendentials, and url, and this class can complete the missing data.
in one scenario we have LoginCredentials
and in the other you have a loose username
and password
. We could build those loose creds into a LoginCredentials
just to pass it in for completion, but seems better to work at the username level since it suits both scenarios.
the other part is that we also have to know if the backfilling happened, so it’s not just the case of passing in a username and getting a username back (or passing in LoginCredentials
and getting LoginCredentials
back) but we also want to know if the backfilling occurred. That’s why it’s wrapped into a BackFillResult
which gives back everything the caller needs to make the updates (and know if the backfilling happened). The caller could infer it based on if the returned username is different, but seems better to me to have that wrapped up in the result type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left to suggestions: removing this class or changing the interface (with an approach). However, change is optional and not blocking. My point here is the class seems just to have 1 check and a method call, and that could be a private method imho.
What confused me is that usernameFromJavascript
holding a value will avoid executing the actual logic of trying to backfill, and return "not supported". Seems something the caller should validate, but subjective I assume.
...fill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/AutofillJavascriptInterface.kt
Outdated
Show resolved
Hide resolved
...fill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/AutofillJavascriptInterface.kt
Outdated
Show resolved
Hide resolved
...ckduckgo/autofill/impl/ui/credential/passwordgeneration/ResultHandlerUseGeneratedPassword.kt
Outdated
Show resolved
Hide resolved
...fill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/AutofillJavascriptInterface.kt
Outdated
Show resolved
Hide resolved
62a3e7d
to
d6bd08c
Compare
d6bd08c
to
b4a1c8a
Compare
Task/Issue URL: https://app.asana.com/0/72649045549333/1206048666874234/f
Description
Increases the ratio of complete credential saves by being able to capture a username-only form submission, and then re-attach it to a nearby password-only form submission. This is useful for scenarios like resetting passwords and multi-step logins.
Steps to test this PR
Logcat filter:
message~:"partial save" | message~:”backfill"
Simulating a multi-step login form
test
Login
button (this simulates getting the first part of a multi-step login form)Login
button (this is submitting only a password now, simulating the second part of a multi-step login form)View
in snackbar)username=test
and password matches what you provided (i.e., the username was backfilled because the partial form submission’s username was later applied to the final form submission which didn’t have the username)Updating password for the above (not backfilling)
test
Login
button; verify you are offered to update the passwordUpdating password (with backfilling)
test
Login
button (this simulates getting the first part of a multi-step login form)Login
button; verify you are offered to update the passwordBackfilling username [test] from partial save
in logsPassword reset flow (with backfilling, automatic password generation)
Password reset flow (with backfilling, manual password entry)
Update Password
. Verify the password is correct and there are no duplicate credentials.Password reset flow (no backfilling, automatic password generation)
Password reset flow (no backfilling, manual password entry)
Email Protection, autofilling personal duck address contributes as username backfill candidate
New Password
field (can leave other two password fields blank) and tap the buttonEmail Protection, autofilling private duck address contributes as username backfill candidate
New Password
field (can leave other two password fields blank) and tap the buttonDisable feature flag
partialFormSaves
test
Login
button (this simulates getting the first part of a multi-step login form)Login
button (this is submitting only a password now, simulating the second part of a multi-step login form)Ensuring existing business rules are maintained
Autofill personal duck address and autogenerated password
Sign Up
button, verify a single, complete credential is saved with correct username+passwordAutofill private duck address and autogenerated password
Sign Up
button, verify a single, complete credential is saved with correct username+passwordAutofill personal duck address and manual password
Sign Up
button, verify you are prompted to save and upon accepting, a single, complete credential is saved with correct username+passwordAutofill private duck address and manual password
Sign Up
button, verify you are prompted to save and upon accepting, a single, complete credential is saved with correct username+password