Closed Bug 1395519 Opened 3 years ago Closed 3 years ago

[Form Autofill] Data Loss in Saved Addresses when submitting to update from a form

Categories

(Toolkit :: Form Manager, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: Gabi, Assigned: steveck)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(3 files)

[Affected versions]:
- beta 56.0b7
- latest Nightly 57.0a1

[Affected platforms]:
- Windows 10 x64, macOS 10.12.6, Ubuntu 14.04 x64

[Steps to reproduce]:
1. Modify extensions.formautofill.available to "on" in about:config (if necessary)
2. In about:preferences#privacy click on 'Saved Addresses'
3. Create a new profile
4. Access the attached .html
5. Double click on any of the fields to trigger autofill
6. Select the profile to fill in fields
7. Modify a field (e.g change city name)
8. Submit form and tap Update on the door hanger popup
9. Verify the updated profile in Saved Addresses

[Expected result]:
- All the data entered in Saved address profile is deleted and only the fields submitted by demo form is displayed

[Actual result]:
- Saved Addresses profile should be updated and the rest of the fields should be displayed in users profile
[Regression range]: This is not regression, issue is seen since the door -hanger popup was implemented

Updated STR: 
[Expected result]:
- Saved Addresses profile should be updated and the rest of the fields should be displayed in users profile

[Actual result]:
- All the data entered in Saved address profile is deleted and only the fields submitted by demo form is displayed
Whiteboard: [form autofill]
Whiteboard: [form autofill] → [form autofill:MVP]
Assignee: nobody → schung
Comment on attachment 8903433 [details]
Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission.

https://reviewboard.mozilla.org/r/175280/#review180280

The patch itself looks good but it causes an autofilled but manually-cleared field unable to be cleared in the storage accordingly via update doorhanger. We probably need to consult UX about this.

::: browser/extensions/formautofill/FormAutofillParent.jsm:381
(Diff revision 1)
>                break;
>              case "update":
>                if (!changedGUIDs.length) {
> +                // Refill the fields if they are not in the new record.
> +                for (let field in originalAddress) {
> +                  if (FormAutofillUtils.isAddressField(field) &&

I don't think we need to check `isAddressField`. Fields in `originalAddress` should be all valid address fields.
Comment on attachment 8903433 [details]
Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission.

https://reviewboard.mozilla.org/r/175280/#review180280

That's a good point. I have another idea is that we cache the "non_autofilled" fields instead. If the original filed exists and it's not in the "non_autofilled" arrary, we should reset the new record with the original field data. wdyt?
Comment on attachment 8903433 [details]
Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission.

https://reviewboard.mozilla.org/r/175280/#review180320

::: browser/extensions/formautofill/FormAutofillParent.jsm:363
(Diff revision 2)
>    _onAddressSubmit(address, target) {
>      if (address.guid) {
>        // Avoid updating the fields that users don't modify.
>        let originalAddress = this.profileStorage.addresses.get(address.guid);
> -      for (let field in address.record) {
> -        if (address.untouchedFields.includes(field) && originalAddress[field]) {
> +      for (let field in originalAddress) {
> +        if (!this.profileStorage.addresses.VALID_FIELDS.includes(field)) {

It's the same like I did in the previous patch(checking isAddressField) is because the original record will contain some interal fields like guid and it will cause the error while updating the record. I use `VALID_FIELDS` instead in this patch since `isAddressField` might not clear enough.

::: browser/extensions/formautofill/test/fixtures/autocomplete_simple_basic.html:1
(Diff revision 2)
> +<!DOCTYPE html>

I created another `autocomplete_simple_basic.html` instead of adding another form in `autocomplete_basic.html` because heuristic unit test will rely on the `autocomplete_basic.html` structure.
Comment on attachment 8903433 [details]
Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission.

https://reviewboard.mozilla.org/r/175280/#review180280

> I don't think we need to check `isAddressField`. Fields in `originalAddress` should be all valid address fields.

Explained in another comment
(In reply to Steve Chung [:steveck] from comment #5)
> That's a good point. I have another idea is that we cache the
> "non_autofilled" fields instead. If the original filed exists and it's not
> in the "non_autofilled" arrary, we should reset the new record with the
> original field data. wdyt?

Doesn't it cause fields which are in the original record but not in the incoming record to also appear in the new created one in "Create New" case?
(In reply to Luke Chang [:lchang] from comment #9)
> (In reply to Steve Chung [:steveck] from comment #5)
> > That's a good point. I have another idea is that we cache the
> > "non_autofilled" fields instead. If the original filed exists and it's not
> > in the "non_autofilled" arrary, we should reset the new record with the
> > original field data. wdyt?
> 
> Doesn't it cause fields which are in the original record but not in the
> incoming record to also appear in the new created one in "Create New" case?

Ok, I can't think of any other better solution than adding another array to cache all existing details...
Comment on attachment 8903433 [details]
Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission.

https://reviewboard.mozilla.org/r/175280/#review180384

::: browser/extensions/formautofill/FormAutofillHandler.jsm:525
(Diff revision 3)
>  
>        data[type] = {
>          guid: this[type].filledRecordGUID,
>          record: {},
>          untouchedFields: [],
> +        allFields: this.allFieldNames,

Not sure if it's ok to add `allFields` for both records because this will have both creditcard and address fields. Maybe move it to upper level or split it into cc/address fields?
Comment on attachment 8903433 [details]
Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission.

https://reviewboard.mozilla.org/r/175280/#review180936

As we discussed, we need to handle the case that only computed fields are modified. Please also add some test cases for that. Thanks.
Attachment #8903433 - Flags: review?(lchang)
Comment on attachment 8903433 [details]
Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission.

https://reviewboard.mozilla.org/r/175280/#review181178

Remember to add a unit test to ensure that changed computed fields DO overwrite its source field if the latter is not present in the content page.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:539
(Diff revision 5)
>              element instanceof Ci.nsIDOMHTMLSelectElement) {
>            // Don't save the record when the option value is empty *OR* there
>            // are multiple options being selected. The empty option is usually
>            // assumed to be default along with a meaningless text to users.
>            if (!value || element.selectedOptions.length != 1) {
> +            // We will keep the property and preserve more information for address updating

nit: delete "We will".

::: browser/extensions/formautofill/FormAutofillHandler.jsm:549
(Diff revision 5)
>            let text = element.selectedOptions[0].text.trim();
>            value = FormAutofillUtils.getAbbreviatedStateName([value, text]) || text;
>          }
>  
>          if (!value) {
> +          // We will keep the property and preserve more information for updating

nit: ditto.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:562
(Diff revision 5)
>            data[type].untouchedFields.push(detail.fieldName);
>          }
>        });
>      });
>  
> -    if (data.address &&
> +    let addrFieldNumber = !data.address ? 0 :

nit: `validAddressFieldAmount`

::: browser/extensions/formautofill/FormAutofillHandler.jsm:563
(Diff revision 5)
>          }
>        });
>      });
>  
> -    if (data.address &&
> -        Object.keys(data.address.record).length < FormAutofillUtils.AUTOFILL_FIELDS_THRESHOLD) {
> +    let addrFieldNumber = !data.address ? 0 :
> +                          this.address.fieldDetails.filter(

nit: `Object.values(data.address.record).filter(f => f).length`

::: browser/extensions/formautofill/FormAutofillHandler.jsm:566
(Diff revision 5)
>  
> -    if (data.address &&
> -        Object.keys(data.address.record).length < FormAutofillUtils.AUTOFILL_FIELDS_THRESHOLD) {
> +    let addrFieldNumber = !data.address ? 0 :
> +                          this.address.fieldDetails.filter(
> +                            detail => data.address.record[detail.fieldName]
> +                          ).length;
> +    if (data.address && addrFieldNumber < FormAutofillUtils.AUTOFILL_FIELDS_THRESHOLD) {

No need to check `data.address` since it'll be set to 0 in the previous line.

::: browser/extensions/formautofill/ProfileStorage.jsm:377
(Diff revision 5)
> +   * @param  {boolean} preserveOldProperties
> +   *         Preserve old record's properties if they are not existed in new record.

nit: `{boolean} [preserveOldProperties=false]` and `... they don't exist in ...`

::: browser/extensions/formautofill/ProfileStorage.jsm:380
(Diff revision 5)
>     * @param  {Object} record
>     *         The new record used to overwrite the old one.
> +   * @param  {boolean} preserveOldProperties
> +   *         Preserve old record's properties if they are not existed in new record.
>     */
> -  update(guid, record) {
> +  update(guid, record, preserveOldProperties) {

nit: Use `preserveOldProperties = false` to explicitly set a default value.

::: browser/extensions/formautofill/ProfileStorage.jsm:397
(Diff revision 5)
>      for (let field of this.VALID_FIELDS) {
>        let oldValue = recordFound[field];
>        let newValue = recordToUpdate[field];
>  
> -      if (newValue != null) {
> -        recordFound[field] = newValue;
> +      // Resume the old field value in the perserve case
> +      if (preserveOldProperties && newValue == undefined) {

use `===`.
Attachment #8903433 - Flags: review?(lchang) → review+
Comment on attachment 8903433 [details]
Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission.

https://reviewboard.mozilla.org/r/175280/#review181178

> No need to check `data.address` since it'll be set to 0 in the previous line.

The reason I still check the `data.address` here because I don't want showing the misleading debug message about the insufficient field number for no data.address case
Comment on attachment 8903433 [details]
Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission.

https://reviewboard.mozilla.org/r/175280/#review181178

> The reason I still check the `data.address` here because I don't want showing the misleading debug message about the insufficient field number for no data.address case

I'll remove `validAddressFieldAmount` to avoid this concern
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d281f0c4644e
[Form Autofill] Keep the original data when record updated via submission. r=lchang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d281f0c4644e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8903433 [details]
Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission.

Approval Request Comment
[Feature/Bug causing the regression]:Not a regression, this issue existed from the day feature landed.
[User impact if declined]: Data might be lost while users updated their profile during form submission.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet but soon
[Needs manual test from QE? If yes, steps to reproduce]: No
[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 original profile if the submitted profile is changed, and it should be no chance to lose data after this patch.
[String changes made/needed]: N/A
Attachment #8903433 - Flags: approval-mozilla-beta?
Hi Gabi,
Can you help check if this issue was fixed as expected in the latest nightly? Thanks.
Flags: needinfo?(gasofie)
Verified issue as fixed using the latest nightly 57.0a1 20170907100318 on Ubuntu 16.04, Windows 10x64 and MacOS 10.12.6
Flags: needinfo?(gasofie)
Comment on attachment 8903433 [details]
Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission.

We're shipping this feature in 56 and the fix is verified. OK to uplift for beta 10.
Attachment #8903433 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/12cab10e453e
Whiteboard: [form autofill:MVP] → [form autofill:MVP]
You need to log in before you can comment on or make changes to this bug.