[Form Autofill] Manually submit an address record that is subset to other records in storage should be merge-able

RESOLVED FIXED in Firefox 58

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: steveck, Assigned: steveck)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 attachment)

When user submits an address record without autofill and the record is a subset to other record in storage, we'll create another record to storage without any doorhanger now. For example:

User submit: new address: [A, B, C]
In storage: address1: [A, B, C, D]
Current result in storage: address1: [A, B, C, D], address2: [A, B, C]

Ideally we should not create any record since it's subset to address 1 for manual case. This behavior should only apply to autofill case. If user autofill address1 and remove D, we'll prompt a update doorhanger and ask if user wants to create a new record or update the existing one.

We'll propose a solution that provide config for merge function for loose merge or strict merge, and we can perform loose merge for manual fill and strict merge for autofill.
Whiteboard: [form autofill:MVP]
Comment on attachment 8924446 [details]
Bug 1413491 - [Form Autofill] Provide 2 different option for address merge for form manual/autofill submission.

https://reviewboard.mozilla.org/r/195732/#review200912

Looks good with only a few nits.

::: browser/extensions/formautofill/ProfileStorage.jsm:1403
(Diff revision 1)
>      let addressFound = this._findByGUID(guid);
>      if (!addressFound) {
>        throw new Error("No matching address.");
>      }
>  
> -    let addressToMerge = this._clone(address);
> +    let addressToMerge = strict ? this._clone(address): this._cloneAndCleanUp(address);

nit: Add a space before `:`

::: browser/extensions/formautofill/ProfileStorage.jsm:1440
(Diff revision 1)
> -      addressFound[field] === addressToMerge[field]
> -    );
> +      // If found address doesn't contain the field, address to merge could be either
> +      // empty string or undefined.

nit: "When addressFound doesn't contain a field, it's unnecessary to update if the same field in addressToMerge is omitted or an empty string"

::: browser/extensions/formautofill/ProfileStorage.jsm:1446
(Diff revision 1)
> -    );
> -    if (exactlyMatch) {
> -      return true;
> +      // empty string or undefined.
> +      if (addressFound[field] === undefined) {
> +        return !addressToMerge[field];
> -    }
> +      }
>  
> -    this.update(guid, addressToMerge, true);
> +      // If addressToMerge doesn't contain the field, return as subset and mergeable.

nit: "When addressFound contains a field, it's unnecessary to update if the same field in addressToMerge is omitted or a duplicate."
Attachment #8924446 - Flags: review?(lchang) → review+
Thanks for the review!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/172216504d78
[Form Autofill] Provide 2 different option for address merge for form manual/autofill submission. r=lchang
Keywords: checkin-needed
Error fixed, thanks.
Flags: needinfo?(schung)
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf2259850d28
[Form Autofill] Provide 2 different option for address merge for form manual/autofill submission. r=lchang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bf2259850d28
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8924446 [details]
Bug 1413491 - [Form Autofill] Provide 2 different option for address merge for form manual/autofill submission.

Approval Request Comment
[Feature/Bug causing the regression]: Original behavior in merge patch bug 1364825
[User impact if declined]: User could save duplicate addresses if form is filled without autofill. 
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]:
1. Save an address via preferences.
2. Open the website, fill the form manually with same address again and submit.
3. See if the there's still one address in preferences.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Low. 
[Why is the change risky/not risky?]: It only affects the result for manual submitted form and it's covered by automated tests(xpcshell test/mochitest)
[String changes made/needed]: N/A
Attachment #8924446 - Flags: approval-mozilla-release?
Comment on attachment 8924446 [details]
Bug 1413491 - [Form Autofill] Provide 2 different option for address merge for form manual/autofill submission.

While this is a valid bug, I do not believe this is a must fix for 57. After talking to MattN, perhaps doing a SAO update that fixes such bugs during 57 release is a better (low risk) option.
Attachment #8924446 - Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.