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

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Steve Chung [:steveck]]

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.

Comment 1

[Steve Chung [:steveck]]

Attachment: Bug 1413491 - [Form Autofill] Provide 2 different option for address merge for form manual/autofill submission.

Review commit: https://reviewboard.mozilla.org/r/195732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195732/

Comment 2

[Luke Chang [:lchang] (inactive)]

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."

Comment 3

[Steve Chung [:steveck]]

Comment on attachment 8924446 [details]
Bug 1413491 - [Form Autofill] Provide 2 different option for address merge for form manual/autofill submission.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195732/diff/1-2/

Comment 4

[Steve Chung [:steveck]]

Thanks for the review!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/172216504d78
[Form Autofill] Provide 2 different option for address merge for form manual/autofill submission. r=lchang

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failure at browser/extensions/formautofill/test/unit/test_addressRecords.js:532: Missing semicolon:

https://hg.mozilla.org/integration/autoland/rev/0214367ea60e3c11059b405a44ccedae59fc3aa1

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=172216504d787aa6ad0448e929dcfde8c1e8047f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=141980561&repo=autoland

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8924446 [details]
Bug 1413491 - [Form Autofill] Provide 2 different option for address merge for form manual/autofill submission.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195732/diff/2-3/

Comment 8

[Steve Chung [:steveck]]

Error fixed, thanks.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf2259850d28
[Form Autofill] Provide 2 different option for address merge for form manual/autofill submission. r=lchang

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/bf2259850d28

Comment 11

[Steve Chung [:steveck]]

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

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.