Bug 1413491 - [Form Autofill] Provide 2 different option for address merge for form manual/autofill submission. r=lchang
MozReview-Commit-ID: FqbpOKzWGcb
--- 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);
+});