Bug 1413491 - [Form Autofill] Provide 2 different option for address merge for form manual/autofill submission. r=lchang
☠☠ backed out by 0214367ea60e ☠ ☠
authorsteveck-chung <schung@mozilla.com>
Thu, 02 Nov 2017 09:54:47 +0800
changeset 443330 172216504d787aa6ad0448e929dcfde8c1e8047f
parent 443329 568b366e806cfaddd34f72a334b2fcea8232a982
child 443331 f8eae610782afc8212877b0c3301ca0c31e6dbf7
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslchang
bugs1413491
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
@@ -1148,16 +1148,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]);
@@ -1168,20 +1189,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) {
@@ -1366,28 +1384,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) {
@@ -1412,45 +1433,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",
     },
@@ -439,26 +462,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;
@@ -487,23 +521,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);
+});