Bug 1413491 - [Form Autofill] Provide 2 different option for address merge for form manual/autofill submission. r=lchang draft
authorsteveck-chung <schung@mozilla.com>
Thu, 02 Nov 2017 09:54:47 +0800
changeset 693383 e17815a70ef5f8c6b2a23e3c54f57f763c0380de
parent 689820 ee21e5f7f1c1726e0ed2697eb45df54cdceedd36
child 739007 9b94a7365427698ef2ddd35a4639458760de7bf4
push id87773
push userbmo:schung@mozilla.com
push dateMon, 06 Nov 2017 02:35:06 +0000
reviewerslchang
bugs1413491
milestone58.0a1
Bug 1413491 - [Form Autofill] Provide 2 different option for address merge for form manual/autofill submission. r=lchang MozReview-Commit-ID: FqbpOKzWGcb
browser/extensions/formautofill/FormAutofillParent.jsm
browser/extensions/formautofill/ProfileStorage.jsm
browser/extensions/formautofill/test/mochitest/test_on_address_submission.html
browser/extensions/formautofill/test/unit/test_addressRecords.js
--- a/browser/extensions/formautofill/FormAutofillParent.jsm
+++ b/browser/extensions/formautofill/FormAutofillParent.jsm
@@ -367,21 +367,21 @@ FormAutofillParent.prototype = {
       // 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]) {
           address.record[field] = originalAddress[field];
         }
       }
 
-      if (!this.profileStorage.addresses.mergeIfPossible(address.guid, address.record)) {
+      if (!this.profileStorage.addresses.mergeIfPossible(address.guid, address.record, true)) {
         this._recordFormFillingTime("address", "autofill-update", timeStartedFillingMS);
 
         FormAutofillDoorhanger.show(target, "update").then((state) => {
-          let changedGUIDs = this.profileStorage.addresses.mergeToStorage(address.record);
+          let changedGUIDs = this.profileStorage.addresses.mergeToStorage(address.record, true);
           switch (state) {
             case "create":
               if (!changedGUIDs.length) {
                 changedGUIDs.push(this.profileStorage.addresses.add(address.record));
               }
               break;
             case "update":
               if (!changedGUIDs.length) {
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -1149,16 +1149,37 @@ 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 multiple mergeable records.
+   * @param {Object} targetRecord
+   *        The record for merge.
+   * @param {boolean} [strict = false]
+   *        In strict merge mode, we'll treat the subset record with empty field
+   *        as unable to be merged, but mergeable if in non-strict mode.
+   * @returns {Array.<string>}
+   *          Return an array of the merged GUID string.
+   */
+  mergeToStorage(targetRecord, strict = false) {
+    let mergedGUIDs = [];
+    for (let record of this.data) {
+      if (!record.deleted && this.mergeIfPossible(record.guid, targetRecord, strict)) {
+        mergedGUIDs.push(record.guid);
+      }
+    }
+    this.log.debug("Existing records matching and merging count is", mergedGUIDs.length);
+    return mergedGUIDs;
+  }
+
   // A test-only helper.
   _nukeAllRecords() {
     this._store.data[this._collectionName] = [];
     // test-only, so there's no good reason to request a save!
   }
 
   _stripComputedFields(record) {
     this.VALID_COMPUTED_FIELDS.forEach(field => delete record[field]);
@@ -1169,20 +1190,17 @@ class AutofillRecords {
 
   // An interface to be inherited.
   _computeFields(record) {}
 
   // An interface to be inherited.
   _normalizeFields(record) {}
 
   // An interface to be inherited.
-  mergeIfPossible(guid, record) {}
-
-  // An interface to be inherited.
-  mergeToStorage(targetRecord) {}
+  mergeIfPossible(guid, record, strict) {}
 }
 
 class Addresses extends AutofillRecords {
   constructor(store) {
     super(store, "addresses", VALID_ADDRESS_FIELDS, VALID_ADDRESS_COMPUTED_FIELDS, ADDRESS_SCHEMA_VERSION);
   }
 
   _recordReadProcessor(address) {
@@ -1363,28 +1381,31 @@ class Addresses extends AutofillRecords 
 
   /**
    * 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.
+   * @param  {boolean} strict
+   *         In strict merge mode, we'll treat the subset record with empty field
+   *         as unable to be merged, but mergeable if in non-strict mode.
    * @returns {boolean}
    *          Return true if address is merged into target with specific guid or false if not.
    */
-  mergeIfPossible(guid, address) {
+  mergeIfPossible(guid, address, strict) {
     this.log.debug("mergeIfPossible:", guid, address);
 
     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);
     this._normalizeRecord(addressToMerge);
     let hasMatchingField = false;
 
     for (let field of this.VALID_FIELDS) {
       let existingField = addressFound[field];
       let incomingField = addressToMerge[field];
       if (incomingField !== undefined && existingField !== undefined) {
         if (incomingField != existingField) {
@@ -1409,45 +1430,36 @@ class Addresses extends AutofillRecords 
     }
 
     // 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) {
+    // Early return if the data is the same or subset.
+    let noNeedToUpdate = this.VALID_FIELDS.every((field) => {
+      // When addressFound doesn't contain a field, it's unnecessary to update
+      // if the same field in addressToMerge is omitted or an empty string.
+      if (addressFound[field] === undefined) {
+        return !addressToMerge[field];
+      }
+
+      // When addressFound contains a field, it's unnecessary to update if
+      // the same field in addressToMerge is omitted or a duplicate.
+      return (addressToMerge[field] === undefined) ||
+             (addressFound[field] === addressToMerge[field]);
+    });
+    if (noNeedToUpdate) {
       return true;
     }
 
     this.update(guid, addressToMerge, true);
     return true;
   }
-
-  /**
-   * Merge the address if storage has multiple mergeable records.
-   * @param {Object} targetAddress
-   *        The address for merge.
-   * @returns {Array.<string>}
-   *          Return an array of the merged GUID string.
-   */
-  mergeToStorage(targetAddress) {
-    let mergedGUIDs = [];
-    for (let address of this.data) {
-      if (!address.deleted && this.mergeIfPossible(address.guid, targetAddress)) {
-        mergedGUIDs.push(address.guid);
-      }
-    }
-    this.log.debug("Existing records matching and merging count is", mergedGUIDs.length);
-    return mergedGUIDs;
-  }
 }
 
 class CreditCards extends AutofillRecords {
   constructor(store) {
     super(store, "creditCards", VALID_CREDIT_CARD_FIELDS, VALID_CREDIT_CARD_COMPUTED_FIELDS, CREDIT_CARD_SCHEMA_VERSION);
   }
 
   _getMaskedCCNumber(ccNumber) {
--- a/browser/extensions/formautofill/test/mochitest/test_on_address_submission.html
+++ b/browser/extensions/formautofill/test/mochitest/test_on_address_submission.html
@@ -114,16 +114,34 @@ add_task(async function check_storage_af
   clickOnElement("input[type=submit]");
 
   let expectedAddresses = TEST_ADDRESSES.slice(0);
   await onStorageChanged("update");
   let matching = await checkAddresses(expectedAddresses);
   ok(matching, "Updated address merged as expected");
 });
 
+// Submit a subset address manually.
+add_task(async function submit_subset_manually() {
+  document.querySelector("form").reset();
+  for (let key in TEST_ADDRESSES[0]) {
+    await setInput("#" + key, TEST_ADDRESSES[0][key]);
+  }
+
+  // Set organization field to empty
+  await setInput("#organization", "");
+  clickOnElement("input[type=submit]");
+
+  let expectedAddresses = TEST_ADDRESSES.slice(0);
+
+  await sleep(1000);
+  let matching = await checkAddresses(expectedAddresses);
+  ok(matching, "The storage is still the same after submitting a subset");
+});
+
 </script>
 
 <div>
 
   <form onsubmit="return false">
     <p>This is a basic form for submitting test.</p>
     <p><label>organization: <input id="organization" name="organization" autocomplete="organization" type="text"></label></p>
     <p><label>streetAddress: <input id="street-address" name="street-address" autocomplete="street-address" type="text"></label></p>
--- a/browser/extensions/formautofill/test/unit/test_addressRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js
@@ -66,17 +66,17 @@ const MERGE_TESTCASES = [
     expectedAddress: {
       "given-name": "Timothy",
       "street-address": "331 E. Evelyn Avenue",
       "tel": "+16509030800",
       country: "US",
     },
   },
   {
-    description: "Merge a subset",
+    description: "Loose merge a subset",
     addressInStorage: {
       "given-name": "Timothy",
       "street-address": "331 E. Evelyn Avenue",
       "tel": "+16509030800",
       country: "US",
     },
     addressToMerge: {
       "given-name": "Timothy",
@@ -84,16 +84,39 @@ const MERGE_TESTCASES = [
       "tel": "+16509030800",
     },
     expectedAddress: {
       "given-name": "Timothy",
       "street-address": "331 E. Evelyn Avenue",
       "tel": "+16509030800",
       country: "US",
     },
+    noNeedToUpdate: true,
+  },
+  {
+    description: "Strict merge a subset without empty string",
+    addressInStorage: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "+16509030800",
+      country: "US",
+    },
+    addressToMerge: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "+16509030800",
+    },
+    expectedAddress: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "+16509030800",
+      country: "US",
+    },
+    strict: true,
+    noNeedToUpdate: true,
   },
   {
     description: "Merge an address with partial overlaps",
     addressInStorage: {
       "given-name": "Timothy",
       "street-address": "331 E. Evelyn Avenue",
       "tel": "+16509030800",
     },
@@ -427,26 +450,37 @@ MERGE_TESTCASES.forEach((testcase) => {
       (subject, data) =>
         data == "update" && subject.QueryInterface(Ci.nsISupportsString).data == guid
     );
 
     // Force to create sync metadata.
     profileStorage.addresses.pullSyncChanges();
     do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 1);
 
-    Assert.ok(profileStorage.addresses.mergeIfPossible(guid, testcase.addressToMerge));
-    await onMerged;
+    Assert.ok(profileStorage.addresses.mergeIfPossible(guid,
+                                                       testcase.addressToMerge,
+                                                       testcase.strict));
+    if (!testcase.noNeedToUpdate) {
+      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);
+    if (testcase.noNeedToUpdate) {
+      Assert.equal(addresses[0].timeLastModified, timeLastModified);
 
-    // Record merging should bump the change counter.
-    do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 2);
+      // No need to bump the change counter if the data is unchanged.
+      do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 1);
+    } else {
+      Assert.notEqual(addresses[0].timeLastModified, timeLastModified);
+
+      // Record merging should bump the change counter.
+      do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 2);
+    }
   });
 });
 
 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 guid = addresses[0].guid;
   let timeLastModified = addresses[0].timeLastModified;
@@ -475,23 +509,38 @@ add_task(async function test_merge_unabl
   do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 1);
 
   // Unable to merge because of conflict
   do_check_eq(profileStorage.addresses.mergeIfPossible(guid, TEST_ADDRESS_3), false);
 
   // Unable to merge because no overlap
   do_check_eq(profileStorage.addresses.mergeIfPossible(guid, TEST_ADDRESS_4), false);
 
+  // Unable to strict merge because subset with empty string
+  let subset = Object.assign({}, TEST_ADDRESS_1);
+  subset.organization = "";
+  do_check_eq(profileStorage.addresses.mergeIfPossible(guid, subset, true), false);
+
   // Shouldn't bump the change counter
   do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 1);
 });
 
 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).length, 2);
   do_check_eq(profileStorage.addresses.getAll()[1].email, anotherAddress.email);
   do_check_eq(profileStorage.addresses.getAll()[2].email, anotherAddress.email);
 });
+
+add_task(async function test_mergeToStorage_strict() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
+  // Try to merge a subset with empty string
+  let anotherAddress = profileStorage.addresses._clone(TEST_ADDRESS_1);
+  anotherAddress.email = "";
+  do_check_eq(profileStorage.addresses.mergeToStorage(anotherAddress, true).length, 0);
+  do_check_eq(profileStorage.addresses.getAll()[0].email, TEST_ADDRESS_1.email);
+});