-
Notifications
You must be signed in to change notification settings - Fork 11
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
[Form] Trigger partialSave on username/email only form submit #702
Changes from 14 commits
28dc087
dbd1b66
65bead1
664a527
e1030b4
d8c7d83
7165166
40ccc96
60100bb
01dd794
225ca95
15e9ed9
35e9191
1098e1b
c6b596f
8f1e476
8c41aa1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -806,7 +806,17 @@ class InterfacePrototype { | |
password: this.passwordGenerator.password, | ||
username: this.emailProtection.lastGenerated, | ||
}); | ||
this.storeFormData(formData, 'formSubmission'); | ||
|
||
// If credentials has only username field, and no password field, then trigger is a partialSave | ||
const isUsernameOnly = | ||
Boolean(formData.credentials?.username) && !formData.credentials?.password && form.inputs.credentials.size === 1; | ||
// Is an email or phone number present in the form, but no other credentials | ||
const isEmailOrPhoneOnly = | ||
Boolean(formData.identities?.emailAddress) !== Boolean(formData.identities?.phone) && | ||
form.inputs.credentials.size === 0 && | ||
form.inputs.identities.size === 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See below, we may be able to get rid of these identities checks. |
||
const trigger = isUsernameOnly || isEmailOrPhoneOnly ? 'partialSave' : 'formSubmission'; | ||
this.storeFormData(formData, trigger); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -895,11 +895,24 @@ class Form { | |
|
||
// After autofill we check if form values match the data provided… | ||
const formValues = this.getValuesReadyForStorage(); | ||
const hasNoCredentialsData = !formValues.credentials?.username && !formValues.credentials?.password; | ||
const hasOnlyEmail = | ||
formValues.identities && Object.keys(formValues.identities ?? {}).length === 1 && formValues.identities?.emailAddress; | ||
|
||
const hasOnlyOneCredentialOrEmail = | ||
Boolean(formValues.credentials?.username) !== Boolean(formValues.credentials?.password) || | ||
(hasOnlyEmail && hasNoCredentialsData); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The complexity of this stuff blows my mind 🙃. Can you please take another look and make it simpler? Like, maybe you can first check whether the form has either an email or username, and then check that there is nothing else? Or even better return early if there is more than 1 item, then check if it's a username or email. Also maybe move it to a utility function? Not sure how complex it is after you simplify it. Feel free to ping me on MM with a preview. |
||
const areAllFormValuesKnown = Object.keys(formValues[dataType] || {}).every( | ||
(subtype) => formValues[dataType][subtype] === data[subtype], | ||
); | ||
if (areAllFormValuesKnown) { | ||
// …if we know all the values do not prompt to store data | ||
|
||
// If we only have a single credential field - then we want to prompt a partial save with username, | ||
// So that in multi step forms (like reset-password), we can identify which username was picked, or complete a password save. | ||
if (hasOnlyOneCredentialOrEmail) { | ||
this.shouldPromptToStoreData = true; | ||
this.shouldAutoSubmit = this.device.globalConfig.isMobileApp; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want to autosubmit here, do we? Imagine people just generating a private duck address on a newsletter form. We don't want to automatically submit that. Am I missing anything? |
||
} else if (areAllFormValuesKnown) { | ||
// …if it's a normal form with more than one field and if we know all the values do not prompt to store data | ||
this.shouldPromptToStoreData = false; | ||
// reset this to its initial value | ||
this.shouldAutoSubmit = this.device.globalConfig.isMobileApp; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,8 +84,8 @@ describe('Test the form class reading values correctly', () => { | |
<input type="password" value="" autocomplete="new-password" /> | ||
<button type="submit">Sign up</button> | ||
</form>`, | ||
expHasValues: false, | ||
expValues: { credentials: undefined }, | ||
expHasValues: true, | ||
expValues: { credentials: { username: 'testUsername' } }, | ||
}, | ||
{ | ||
testCase: 'form where the password is <=3 characters long', | ||
|
@@ -95,8 +95,8 @@ describe('Test the form class reading values correctly', () => { | |
<input type="password" value="abc" autocomplete="new-password" /> | ||
<button type="submit">Sign up</button> | ||
</form>`, | ||
expHasValues: false, | ||
expValues: { credentials: undefined }, | ||
expHasValues: true, | ||
expValues: { credentials: { username: 'testUsername' } }, | ||
}, | ||
{ | ||
testCase: 'form with hidden email field', | ||
|
@@ -276,7 +276,11 @@ describe('Test the form class reading values correctly', () => { | |
</form>`, | ||
expHasValues: true, | ||
expValues: { | ||
identities: undefined, | ||
identities: { | ||
emailAddress: '[email protected]', | ||
firstName: 'Peppa', | ||
lastName: 'Pig', | ||
}, | ||
creditCards: { | ||
cardName: 'Peppa Pig', | ||
cardSecurityCode: '123', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,14 +163,13 @@ const getMMAndYYYYFromString = (expiration) => { | |
* @param {InternalDataStorageObject} credentials | ||
* @return {boolean} | ||
*/ | ||
const shouldStoreCredentials = ({ credentials }) => Boolean(credentials.password); | ||
|
||
/** | ||
* @param {InternalDataStorageObject} credentials | ||
* @return {boolean} | ||
*/ | ||
const shouldStoreIdentities = ({ identities }) => | ||
Boolean((identities.firstName || identities.fullName) && identities.addressStreet && identities.addressCity); | ||
const shouldStoreIdentities = ({ identities }) => { | ||
return ( | ||
Boolean((identities.firstName || identities.fullName) && identities.addressStreet && identities.addressCity) || | ||
Boolean(identities.emailAddress) || | ||
Boolean(identities.phone) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this start prompting to save identities that just have a lone phone or lone email? Do we have some backend checks in the native apps to prevent that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, we have a guard against it in the interface prototype. Got it. But that makes this whole thing confusing because we're saying that we should store identities when in fact we shouldn't. What I think we should do instead is: when we detect one of these email-only or phone-only forms, move that email or phone to |
||
); | ||
}; | ||
|
||
/** | ||
* @param {InternalDataStorageObject} credentials | ||
|
@@ -207,9 +206,8 @@ const prepareFormValuesForStorage = (formValues) => { | |
creditCards.cardName = identities?.fullName || formatFullName(identities); | ||
} | ||
|
||
/** Fixes for credentials **/ | ||
// Don't store if there isn't enough data | ||
if (shouldStoreCredentials(formValues)) { | ||
/** Fixes for credentials */ | ||
if (credentials.username || credentials.password) { | ||
// If we don't have a username to match a password, let's see if the email is available | ||
if (credentials.password && !credentials.username && identities.emailAddress) { | ||
credentials.username = identities.emailAddress; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,15 +61,15 @@ describe('Can strip phone formatting characters', () => { | |
|
||
describe('prepareFormValuesForStorage()', () => { | ||
describe('handling credentials', () => { | ||
it('rejects for username only', () => { | ||
it('accepts for username only', () => { | ||
const values = prepareFormValuesForStorage({ | ||
credentials: { username: '[email protected]' }, | ||
// @ts-ignore | ||
creditCards: {}, | ||
// @ts-ignore | ||
identities: {}, | ||
}); | ||
expect(values.credentials).toBeUndefined(); | ||
expect(values.credentials?.username).toBe('[email protected]'); | ||
}); | ||
it('accepts password only', () => { | ||
const values = prepareFormValuesForStorage({ | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Shouldn't you just check for username and size here? Also, don't need to force to Boolean.