Bug 1364825 - Add merge function in autofill storage. r?MattN draft
authorsteveck-chung <schung@mozilla.com>
Fri, 19 May 2017 18:33:38 +0800
changeset 590778 5d804f7961ff5054b969a42c7454140610475809
parent 588783 8a3aa1701537ea6b8334f432cd030d260d492fa3
child 591429 c29b23d234d4aaa3f915d60d9564856ae4f84e99
child 591555 d03f30933a53f0f5c8b60489453129cc6f8bb2d0
push id62826
push userbmo:schung@mozilla.com
push dateThu, 08 Jun 2017 03:03:09 +0000
reviewersMattN
bugs1364825
milestone55.0a1
Bug 1364825 - Add merge function in autofill storage. r?MattN MozReview-Commit-ID: AWYsnzmVJAY
browser/extensions/formautofill/FormAutofillParent.jsm
browser/extensions/formautofill/ProfileStorage.jsm
browser/extensions/formautofill/test/unit/head.js
browser/extensions/formautofill/test/unit/test_addressRecords.js
--- a/browser/extensions/formautofill/FormAutofillParent.jsm
+++ b/browser/extensions/formautofill/FormAutofillParent.jsm
@@ -265,17 +265,19 @@ FormAutofillParent.prototype = {
                                         Services.ppmm.initialProcessData.autofillSavedFieldNames);
     this._updateStatus();
   },
 
   _onFormSubmit(data, target) {
     let {address} = data;
 
     if (address.guid) {
-      // TODO: Show update doorhanger(bug 1303513) and set probe(bug 990200)
-      // if (!profileStorage.addresses.mergeIfPossible(address.guid, address.record)) {
-      // }
+      if (!this.profileStorage.addresses.mergeIfPossible(address.guid, address.record)) {
+        // TODO: Show update doorhanger(bug 1303513) and set probe(bug 990200)
+        return;
+      }
+      this.profileStorage.addresses.notifyUsed(address.guid);
     } else {
       // TODO: Add first time use probe(bug 990199) and doorhanger(bug 1303510)
       // profileStorage.addresses.add(address.record);
     }
   },
 };
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -374,16 +374,22 @@ class AutofillRecords {
     }
   }
 
   // An interface to be inherited.
   _recordReadProcessor(record, config) {}
 
   // An interface to be inherited.
   _recordWriteProcessor(record) {}
+
+  // An interface to be inherited.
+  mergeIfPossible(guid, record) {}
+
+  // An interface to be inherited.
+  mergeToStorage(targetRecord) {}
 }
 
 class Addresses extends AutofillRecords {
   constructor(store) {
     super(store, "addresses", VALID_PROFILE_FIELDS, ADDRESS_SCHEMA_VERSION);
   }
 
   _recordReadProcessor(profile, {noComputedFields} = {}) {
@@ -449,16 +455,95 @@ class Addresses extends AutofillRecords 
       });
 
       // Concatenate "address-line*" if "street-address" is omitted.
       if (!profile["street-address"]) {
         profile["street-address"] = addressLines.join("\n");
       }
     }
   }
+
+  /**
+   * Merge new address into the specified address if mergeable.
+   *
+   * @param  {string} guid
+   *         Indicates which address to merge.
+   * @param  {Object} address
+   *         The new address used to merge into the old one.
+   * @returns {boolean}
+   *          Return true if address is merged into target with specific guid or false if not.
+   */
+  mergeIfPossible(guid, address) {
+    this.log.debug("mergeIfPossible:", guid, address);
+
+    let addressFound = this._findByGUID(guid);
+    if (!addressFound) {
+      throw new Error("No matching address.");
+    }
+
+    let addressToMerge = this._clone(address);
+    this._normalizeRecord(addressToMerge);
+    let hasMatchingField = false;
+
+    for (let field of this.VALID_FIELDS) {
+      if (addressToMerge[field] !== undefined && addressFound[field] !== undefined) {
+        if (addressToMerge[field] != addressFound[field]) {
+          this.log.debug("Conflicts: field", field, "has different value.");
+          return false;
+        }
+        hasMatchingField = true;
+      }
+    }
+
+    // We merge the address only when at least one field has the same value.
+    if (!hasMatchingField) {
+      this.log.debug("Unable to merge because no field has the same value");
+      return false;
+    }
+
+    // Early return if the data is the same.
+    let exactlyMatch = this.VALID_FIELDS.every((field) =>
+      addressFound[field] === addressToMerge[field]
+    );
+    if (exactlyMatch) {
+      return true;
+    }
+
+    for (let field in addressToMerge) {
+      if (this.VALID_FIELDS.includes(field)) {
+        addressFound[field] = addressToMerge[field];
+      }
+    }
+
+    addressFound.timeLastModified = Date.now();
+    this._store.saveSoon();
+    let str = Cc["@mozilla.org/supports-string;1"]
+                 .createInstance(Ci.nsISupportsString);
+    str.data = guid;
+    Services.obs.notifyObservers(str, "formautofill-storage-changed", "merge");
+    return true;
+  }
+
+  /**
+   * Merge the address if storage has multiple mergeable records.
+   * @param {Object} targetAddress
+   *        The address for merge.
+   * @returns {boolean}
+   *          Return true if the target address is mergeable or false if not.
+   */
+  mergeToStorage(targetAddress) {
+    let merged = false;
+    for (let address of this._store.data[this._collectionName]) {
+      if (this.mergeIfPossible(address.guid, targetAddress)) {
+        merged = true;
+      }
+    }
+    this.log.debug("Existing records matching and merging is", merged);
+    return merged;
+  }
 }
 
 class CreditCards extends AutofillRecords {
   constructor(store) {
     super(store, "creditCards", VALID_CREDIT_CARD_FIELDS, CREDIT_CARD_SCHEMA_VERSION);
   }
 
   _recordReadProcessor(creditCard, {noComputedFields} = {}) {
--- a/browser/extensions/formautofill/test/unit/head.js
+++ b/browser/extensions/formautofill/test/unit/head.js
@@ -1,13 +1,15 @@
 /**
  * Provides infrastructure for automated formautofill components tests.
  */
 
-/* exported getTempFile, loadFormAutofillContent, runHeuristicsTest, sinon */
+/* exported getTempFile, loadFormAutofillContent, runHeuristicsTest, sinon,
+ *          initProfileStorage
+ */
 
 "use strict";
 
 const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/NetUtil.jsm");
@@ -74,16 +76,36 @@ function getTempFile(leafName) {
     if (file.exists()) {
       file.remove(false);
     }
   });
 
   return file;
 }
 
+async function initProfileStorage(fileName, records) {
+  let {ProfileStorage} = Cu.import("resource://formautofill/ProfileStorage.jsm", {});
+  let path = getTempFile(fileName).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  if (!records || !Array.isArray(records)) {
+    return profileStorage;
+  }
+
+  let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
+                                          (subject, data) => data == "add");
+  for (let record of records) {
+    do_check_true(profileStorage.addresses.add(record));
+    await onChanged;
+  }
+  await profileStorage._saveImmediately();
+  return profileStorage;
+}
+
 function runHeuristicsTest(patterns, fixturePathPrefix) {
   Cu.import("resource://gre/modules/FormLikeFactory.jsm");
   Cu.import("resource://formautofill/FormAutofillHeuristics.jsm");
   Cu.import("resource://formautofill/FormAutofillUtils.jsm");
 
   patterns.forEach(testPattern => {
     add_task(function* () {
       do_print("Starting test fixture: " + testPattern.fixturePath);
@@ -110,17 +132,17 @@ function runHeuristicsTest(patterns, fix
           expectedField.elementWeakRef = field.elementWeakRef;
           Assert.deepEqual(field, expectedField);
         });
       });
     });
   });
 }
 
-add_task(function* head_initialize() {
+add_task(async function head_initialize() {
   Services.prefs.setBoolPref("extensions.formautofill.experimental", true);
   Services.prefs.setBoolPref("extensions.formautofill.heuristics.enabled", true);
   Services.prefs.setBoolPref("dom.forms.autocomplete.experimental", true);
 
   // Clean up after every test.
   do_register_cleanup(function head_cleanup() {
     Services.prefs.clearUserPref("extensions.formautofill.experimental");
     Services.prefs.clearUserPref("extensions.formautofill.heuristics.enabled");
--- a/browser/extensions/formautofill/test/unit/test_addressRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js
@@ -1,16 +1,14 @@
 /**
  * Tests ProfileStorage object with addresses records.
  */
 
 "use strict";
 
-const {ProfileStorage} = Cu.import("resource://formautofill/ProfileStorage.jsm", {});
-
 const TEST_STORE_FILE_NAME = "test-profile.json";
 
 const TEST_ADDRESS_1 = {
   "given-name": "Timothy",
   "additional-name": "John",
   "family-name": "Berners-Lee",
   organization: "World Wide Web Consortium",
   "street-address": "32 Vassar Street\nMIT Room 32-G524",
@@ -27,64 +25,115 @@ const TEST_ADDRESS_2 = {
   country: "US",
 };
 
 const TEST_ADDRESS_3 = {
   "street-address": "Other Address",
   "postal-code": "12345",
 };
 
+const TEST_ADDRESS_4 = {
+  "given-name": "Timothy",
+  "additional-name": "John",
+  "family-name": "Berners-Lee",
+  organization: "World Wide Web Consortium",
+};
+
 const TEST_ADDRESS_WITH_INVALID_FIELD = {
   "street-address": "Another Address",
   invalidField: "INVALID",
 };
 
-let prepareTestRecords = async function(path) {
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
-
-  let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
-                                          (subject, data) => data == "add");
-  do_check_true(profileStorage.addresses.add(TEST_ADDRESS_1));
-  await onChanged;
-  do_check_true(profileStorage.addresses.add(TEST_ADDRESS_2));
-  await profileStorage._saveImmediately();
-};
+const MERGE_TESTCASES = [
+  {
+    description: "Merge a superset",
+    addressInStorage: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+    },
+    addressToMerge: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+      country: "US",
+    },
+    expectedAddress: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+      country: "US",
+    },
+  },
+  {
+    description: "Merge a subset",
+    addressInStorage: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+      country: "US",
+    },
+    addressToMerge: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+    },
+    expectedAddress: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+      country: "US",
+    },
+  },
+  {
+    description: "Merge an address with partial overlaps",
+    addressInStorage: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+    },
+    addressToMerge: {
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+      country: "US",
+    },
+    expectedAddress: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+      country: "US",
+    },
+  },
+];
 
 let do_check_record_matches = (recordWithMeta, record) => {
   for (let key in record) {
     do_check_eq(recordWithMeta[key], record[key]);
   }
 };
 
 add_task(async function test_initialize() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME);
 
   do_check_eq(profileStorage._store.data.version, 1);
   do_check_eq(profileStorage._store.data.addresses.length, 0);
 
   let data = profileStorage._store.data;
   Assert.deepEqual(data.addresses, []);
 
   await profileStorage._saveImmediately();
 
-  profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME);
 
   Assert.deepEqual(profileStorage._store.data, data);
 });
 
 add_task(async function test_getAll() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
 
   do_check_eq(addresses.length, 2);
   do_check_record_matches(addresses[0], TEST_ADDRESS_1);
   do_check_record_matches(addresses[1], TEST_ADDRESS_2);
 
   // Check computed fields.
@@ -99,41 +148,35 @@ add_task(async function test_getAll() {
   do_check_eq(addresses[0]["address-line2"], undefined);
 
   // Modifying output shouldn't affect the storage.
   addresses[0].organization = "test";
   do_check_record_matches(profileStorage.addresses.getAll()[0], TEST_ADDRESS_1);
 });
 
 add_task(async function test_get() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
   let guid = addresses[0].guid;
 
   let address = profileStorage.addresses.get(guid);
   do_check_record_matches(address, TEST_ADDRESS_1);
 
   // Modifying output shouldn't affect the storage.
   address.organization = "test";
   do_check_record_matches(profileStorage.addresses.get(guid), TEST_ADDRESS_1);
 
   do_check_eq(profileStorage.addresses.get("INVALID_GUID"), null);
 });
 
 add_task(async function test_getByFilter() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let filter = {info: {fieldName: "street-address"}, searchString: "Some"};
   let addresses = profileStorage.addresses.getByFilter(filter);
   do_check_eq(addresses.length, 1);
   do_check_record_matches(addresses[0], TEST_ADDRESS_2);
 
   filter = {info: {fieldName: "country"}, searchString: "u"};
   addresses = profileStorage.addresses.getByFilter(filter);
@@ -156,21 +199,18 @@ add_task(async function test_getByFilter
 
   // Prevent broken while searching the property that does not exist.
   filter = {info: {fieldName: "tel"}, searchString: "1"};
   addresses = profileStorage.addresses.getByFilter(filter);
   do_check_eq(addresses.length, 0);
 });
 
 add_task(async function test_add() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
 
   do_check_eq(addresses.length, 2);
 
   do_check_record_matches(addresses[0], TEST_ADDRESS_1);
   do_check_record_matches(addresses[1], TEST_ADDRESS_2);
 
@@ -181,38 +221,32 @@ add_task(async function test_add() {
   do_check_eq(addresses[0].timeLastUsed, 0);
   do_check_eq(addresses[0].timesUsed, 0);
 
   Assert.throws(() => profileStorage.addresses.add(TEST_ADDRESS_WITH_INVALID_FIELD),
     /"invalidField" is not a valid field\./);
 });
 
 add_task(async function test_update() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
   let guid = addresses[1].guid;
   let timeLastModified = addresses[1].timeLastModified;
 
   let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
                                           (subject, data) => data == "update");
 
   do_check_neq(addresses[1].country, undefined);
 
   profileStorage.addresses.update(guid, TEST_ADDRESS_3);
   await onChanged;
   await profileStorage._saveImmediately();
 
-  profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
-
   let address = profileStorage.addresses.get(guid);
 
   do_check_eq(address.country, undefined);
   do_check_neq(address.timeLastModified, timeLastModified);
   do_check_record_matches(address, TEST_ADDRESS_3);
 
   Assert.throws(
     () => profileStorage.addresses.update("INVALID_GUID", TEST_ADDRESS_3),
@@ -221,66 +255,111 @@ add_task(async function test_update() {
 
   Assert.throws(
     () => profileStorage.addresses.update(guid, TEST_ADDRESS_WITH_INVALID_FIELD),
     /"invalidField" is not a valid field\./
   );
 });
 
 add_task(async function test_notifyUsed() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
   let guid = addresses[1].guid;
   let timeLastUsed = addresses[1].timeLastUsed;
   let timesUsed = addresses[1].timesUsed;
 
   let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
                                           (subject, data) => data == "notifyUsed");
 
   profileStorage.addresses.notifyUsed(guid);
   await onChanged;
   await profileStorage._saveImmediately();
 
-  profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
-
   let address = profileStorage.addresses.get(guid);
 
   do_check_eq(address.timesUsed, timesUsed + 1);
   do_check_neq(address.timeLastUsed, timeLastUsed);
 
   Assert.throws(() => profileStorage.addresses.notifyUsed("INVALID_GUID"),
     /No matching record\./);
 });
 
 add_task(async function test_remove() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
   let guid = addresses[1].guid;
 
   let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
                                           (subject, data) => data == "remove");
 
   do_check_eq(addresses.length, 2);
 
   profileStorage.addresses.remove(guid);
   await onChanged;
   await profileStorage._saveImmediately();
 
-  profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
-
   addresses = profileStorage.addresses.getAll();
 
   do_check_eq(addresses.length, 1);
 
   do_check_eq(profileStorage.addresses.get(guid), null);
 });
+
+MERGE_TESTCASES.forEach((testcase) => {
+  add_task(async function test_merge() {
+    do_print("Starting testcase: " + testcase.description);
+    let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                  [testcase.addressInStorage]);
+    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
+    );
+    let timeLastModified = addresses[0].timeLastModified;
+    Assert.equal(
+      profileStorage.addresses.mergeIfPossible(addresses[0].guid, testcase.addressToMerge),
+      true);
+    await onMerged;
+    addresses = profileStorage.addresses.getAll();
+    Assert.equal(addresses.length, 1);
+    Assert.notEqual(addresses[0].timeLastModified, timeLastModified);
+    do_check_record_matches(addresses[0], testcase.expectedAddress);
+  });
+});
+
+add_task(async function test_merge_same_address() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME, [TEST_ADDRESS_1]);
+  let addresses = profileStorage.addresses.getAll();
+  let timeLastModified = addresses[0].timeLastModified;
+  // 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.equal(addresses[0].timeLastModified, timeLastModified);
+});
+
+add_task(async function test_merge_unable_merge() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
+
+  let addresses = profileStorage.addresses.getAll();
+  // Unable to merge because of conflict
+  do_check_eq(profileStorage.addresses.mergeIfPossible(addresses[1].guid, TEST_ADDRESS_3), false);
+
+  // Unable to merge because no overlap
+  do_check_eq(profileStorage.addresses.mergeIfPossible(addresses[1].guid, TEST_ADDRESS_4), false);
+});
+
+add_task(async function test_mergeToStorage() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
+  // Merge an address to storage
+  let anotherAddress = profileStorage.addresses._clone(TEST_ADDRESS_2);
+  profileStorage.addresses.add(anotherAddress);
+  anotherAddress.email = "timbl@w3.org";
+  do_check_eq(profileStorage.addresses.mergeToStorage(anotherAddress), true);
+  do_check_eq(profileStorage.addresses.getAll()[1].email, anotherAddress.email);
+  do_check_eq(profileStorage.addresses.getAll()[2].email, anotherAddress.email);
+});