Closed Bug 1412788 Opened 6 years ago Closed 6 years ago

[Form Autofill] The sync metadata should be updated while merging address records

Categories

(Toolkit :: Form Manager, defect, P3)

Product:
Toolkit
Component:
Form Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(2 files)

Bug 1412788 - [Form Autofill] The sync metadata should be updated while merging address records.
59 bytes, text/x-review-board-request
steveck
: review+
ritu
: approval-mozilla-beta+
Details
Bug1412788_for_beta.patch
9.08 KB, patch
Details | Diff | Splinter Review 
If an address record is updated by merging mechanism, the sync metadata should be updated as well.
Blocks: 1402963
Comment on attachment 8923329 [details]
Bug 1412788 - [Form Autofill] The sync metadata should be updated while merging address records.

https://reviewboard.mozilla.org/r/194532/#review199858

::: browser/extensions/formautofill/ProfileStorage.jsm:453
(Diff revision 2)
>  
>      this._store.saveSoon();
> -    Services.obs.notifyObservers(null, "formautofill-storage-changed", "update");
> +
> +    let str = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
> +    str.data = guid;
> +    Services.obs.notifyObservers(str, "formautofill-storage-changed", "update");

nit: Since we'll expose guid for update, the guid could be verified in test_addressRecords.js update onChange as well. Anyway it's really trivial and unrelated to this bug so we can update it later once we want to expose guid for rest of the APIs.

::: browser/extensions/formautofill/test/unit/test_addressRecords.js:422
(Diff revision 2)
>      let addresses = profileStorage.addresses.getAll();
>      // Merge address and verify the guid in notifyObservers subject
>      let onMerged = TestUtils.topicObserved(
>        "formautofill-storage-changed",
>        (subject, data) =>
> -        data == "merge" && subject.QueryInterface(Ci.nsISupportsString).data == addresses[0].guid
> +        data == "update" && subject.QueryInterface(Ci.nsISupportsString).data == addresses[0].guid

nit: Maybe we can move `let guid = addresses[0].guid` above `onMerge` and  s/addresses[0].guid/guid like other tests?

::: browser/extensions/formautofill/test/unit/test_addressRecords.js:455
(Diff revision 2)
> +  // Force to create sync metadata.
> +  profileStorage.addresses.pullSyncChanges();
> +  do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 1);
> +
>    // Merge same address will still return true but it won't update timeLastModified.
> -  Assert.equal(profileStorage.addresses.mergeIfPossible(addresses[0].guid, TEST_ADDRESS_1), true);
> +  Assert.ok(profileStorage.addresses.mergeIfPossible(addresses[0].guid, TEST_ADDRESS_1));

nit: s/addresses[0].guid/guid ?
Attachment #8923329 - Flags: review?(schung) → review+
Good point. Thanks.
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5110cc80ef6
[Form Autofill] The sync metadata should be updated while merging address records. r=steveck
Comment on attachment 8923329 [details]
Bug 1412788 - [Form Autofill] The sync metadata should be updated while merging address records.

Approval Request Comment
[Feature/Bug causing the regression]: Feature.
[User impact if declined]: An address record updated by merging mechanism won't be sync'ed across devices.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Verified locally.
[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?]: No.
[Why is the change risky/not risky?]: Just changed to use another existing internal API.
[String changes made/needed]: N/A
Attachment #8923329 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/b5110cc80ef6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8923329 [details]
Bug 1412788 - [Form Autofill] The sync metadata should be updated while merging address records.

Form autofill related, Beta57+
Attachment #8923329 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs a rebased patch for Beta.
Flags: needinfo?(lchang)
Patch rebased. Thanks.
Flags: needinfo?(lchang)
(In reply to Luke Chang [:lchang] from comment #7)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Verified locally.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Luke's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.