Bug 990219 - Part 2: Add merge function in storage. draft
authorsteveck-chung <schung@mozilla.com>
Fri, 19 May 2017 18:33:38 +0800
changeset 581337 1d9bde9ccae01bfa7dbd61bc464e47e4276e4137
parent 581336 31ed3c320ec8d2b971fd07770cb40238896b4d68
child 581338 8c3ab2c9dea66ff03a3fa552efeb451110ed4d4e
child 581348 c94c60f1971bac52d641b51e5b510aa976a8c7b9
push id59831
push userbmo:schung@mozilla.com
push dateFri, 19 May 2017 16:44:08 +0000
bugs990219
milestone55.0a1
Bug 990219 - Part 2: Add merge function in storage. MozReview-Commit-ID: AWYsnzmVJAY
browser/extensions/formautofill/ProfileStorage.jsm
browser/extensions/formautofill/test/unit/test_addressRecords.js
browser/extensions/formautofill/test/unit/test_creditCardRecords.js
browser/extensions/formautofill/test/unit/test_transformFields.js
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -166,18 +166,22 @@ class AutofillRecords {
 
   /**
    * Adds a new record.
    *
    * @param {Object} record
    *        The new record for saving.
    */
   add(record) {
+    // Try to merge to existing record before adding new record
+    if (this._mergeToStorage(record)) {
+      return;
+    }
+
     this.log.debug("add:", record);
-
     let recordToSave = this._clone(record);
     this._normalizeRecord(recordToSave);
 
     let guid;
     while (!guid || this._findByGUID(guid)) {
       guid = gUUIDGenerator.generateUUID().toString()
                            .replace(/[{}-]/g, "").substring(0, 12);
     }
@@ -263,16 +267,67 @@ class AutofillRecords {
     this._store.data[this._collectionName] =
       this._store.data[this._collectionName].filter(record => record.guid != guid);
     this._store.saveSoon();
 
     Services.obs.notifyObservers(null, "formautofill-storage-changed", "remove");
   }
 
   /**
+   * Merge new record into the specified record if mergeable.
+   *
+   * @param  {string} guid
+   *         Indicates which record to merge.
+   * @param  {Object} record
+   *         The new record used to merge into the old one.
+   * @returns {boolean}
+   *          Return true if record is merged into target with specific guid or false if not.
+   */
+  mergeIfPossible(guid, record) {
+    this.log.debug("merge:", guid, record);
+
+    let recordFound = this._findByGUID(guid);
+    if (!recordFound) {
+      throw new Error("No matching record.");
+    }
+
+    let recordToMerge = this._clone(record);
+    this._normalizeRecord(recordToMerge);
+    let hasMatchingField = false;
+
+    for (let field of this.VALID_FIELDS) {
+      if (recordToMerge[field] !== undefined && recordFound[field] !== undefined) {
+        if (recordToMerge[field] != recordFound[field]) {
+          this.log.debug("Conflicts: field", field, "has different value.");
+          return false;
+        }
+        hasMatchingField = true;
+      }
+    }
+
+    // We merge the record only when at least one field has the same value.
+    if (!hasMatchingField) {
+      this.log.debug("Unable to merge because there's no field has same value");
+      return false;
+    }
+
+    for (let field in recordToMerge) {
+      if (this.VALID_FIELDS.includes(field)) {
+        recordFound[field] = recordToMerge[field];
+      }
+    }
+
+    recordFound.timeLastModified = Date.now();
+
+    this._store.saveSoon();
+    Services.obs.notifyObservers(null, "formautofill-storage-changed", "merge");
+    return true;
+  }
+
+  /**
    * Returns the record with the specified GUID.
    *
    * @param   {string} guid
    *          Indicates which record to retrieve.
    * @returns {Object}
    *          A clone of the record.
    */
   get(guid) {
@@ -357,16 +412,33 @@ class AutofillRecords {
       }
       if (typeof record[key] !== "string" &&
           typeof record[key] !== "number") {
         throw new Error(`"${key}" contains invalid data type.`);
       }
     }
   }
 
+  /**
+   * Merge the record if storage has a mergeable record.
+   * @param {Object} targetRecord
+   *        The record for merge.
+   * @returns {boolean}
+   *          Return true if the target record is mergeable or false if not.
+   */
+  _mergeToStorage(targetRecord) {
+    for (let record of this._store.data[this._collectionName]) {
+      if (this.mergeIfPossible(record.guid, targetRecord)) {
+        return true;
+      }
+    }
+    this.log.debug("Unable to merge into storage");
+    return false;
+  }
+
   // An interface to be inherited.
   _recordReadProcessor(record, config) {}
 
   // An interface to be inherited.
   _recordWriteProcessor(record) {}
 }
 
 class Addresses extends AutofillRecords {
--- a/browser/extensions/formautofill/test/unit/test_addressRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js
@@ -27,16 +27,23 @@ 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();
@@ -178,16 +185,23 @@ add_task(async function test_add() {
   do_check_neq(addresses[0].guid, undefined);
   do_check_neq(addresses[0].timeCreated, undefined);
   do_check_eq(addresses[0].timeLastModified, addresses[0].timeCreated);
   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\./);
+
+  // Saving a mergeable address will not add a new address.
+  let timeLastModified = addresses[0].timeLastModified;
+  profileStorage.addresses.add(TEST_ADDRESS_4);
+  addresses = profileStorage.addresses.getAll();
+  do_check_eq(addresses.length, 2);
+  do_check_neq(addresses[0].timeLastModified, timeLastModified);
 });
 
 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();
@@ -279,8 +293,46 @@ add_task(async function test_remove() {
   await profileStorage.initialize();
 
   addresses = profileStorage.addresses.getAll();
 
   do_check_eq(addresses.length, 1);
 
   Assert.throws(() => profileStorage.addresses.get(guid), /No matching record\./);
 });
+
+add_task(async function test_merge() {
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  await prepareTestRecords(path);
+
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  let addresses = profileStorage.addresses.getAll();
+
+  do_check_eq(addresses.length, 2);
+
+  // Merge a superset
+  let superset = profileStorage.addresses._clone(TEST_ADDRESS_2);
+  superset.tel = "123456";
+  do_check_eq(profileStorage.addresses.mergeIfPossible(addresses[1].guid, superset), true);
+  do_check_eq(profileStorage.addresses.getAll()[1].tel, "123456");
+
+  // Merge a subset
+  let subset = profileStorage.addresses._clone(TEST_ADDRESS_1);
+  delete subset.tel;
+  do_check_eq(profileStorage.addresses.mergeIfPossible(addresses[0].guid, subset), true);
+  do_check_eq(profileStorage.addresses.getAll()[0].tel, TEST_ADDRESS_1.tel);
+
+  // Merge an address with partial overlaps
+  let overlap = profileStorage.addresses._clone(TEST_ADDRESS_2);
+  delete overlap.country;
+  overlap["postal-code"] = "12345";
+  do_check_eq(profileStorage.addresses.mergeIfPossible(addresses[1].guid, overlap), true);
+  do_check_eq(profileStorage.addresses.getAll()[1].country, TEST_ADDRESS_2.country);
+  do_check_eq(profileStorage.addresses.getAll()[1]["postal-code"], "12345");
+
+  // 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);
+});
--- a/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
@@ -35,24 +35,24 @@ const TEST_CREDIT_CARD_WITH_2_DIGITS_YEA
 };
 
 const TEST_CREDIT_CARD_WITH_INVALID_FIELD = {
   "cc-name": "John Doe",
   invalidField: "INVALID",
 };
 
 const TEST_CREDIT_CARD_WITH_INVALID_EXPIRY_DATE = {
-  "cc-name": "John Doe",
+  "cc-name": "John Doe Sr.",
   "cc-number": "1111222233334444",
   "cc-exp-month": 13,
   "cc-exp-year": -3,
 };
 
 const TEST_CREDIT_CARD_WITH_SPACES_BETWEEN_DIGITS = {
-  "cc-name": "John Doe",
+  "cc-name": "John Doe Jr.",
   "cc-number": "1111 2222 3333 4444",
 };
 
 const TEST_CREDIT_CARD_WITH_INVALID_NUMBERS = {
   "cc-name": "John Doe",
   "cc-number": "abcdefg",
 };
 
--- a/browser/extensions/formautofill/test/unit/test_transformFields.js
+++ b/browser/extensions/formautofill/test/unit/test_transformFields.js
@@ -106,22 +106,22 @@ const ADDRESS_NORMALIZE_TESTCASES = [
     },
   },
   {
     description: "Has both \"name\" and split names",
     address: {
       "name": "John Doe",
       "given-name": "Timothy",
       "additional-name": "John",
-      "family-name": "Berners-Lee",
+      "family-name": "Berners",
     },
     expectedResult: {
       "given-name": "Timothy",
       "additional-name": "John",
-      "family-name": "Berners-Lee",
+      "family-name": "Berners",
     },
   },
   {
     description: "Has \"name\", and some of split names are omitted",
     address: {
       "name": "John Doe",
       "given-name": "Timothy",
     },