diff --git a/browser/extensions/formautofill/test/browser/address/browser_address_capture_form_removal.js b/browser/extensions/formautofill/test/browser/address/browser_address_capture_form_removal.js index 13cd3b4fc5aa..e0639a28dc72 100644 --- a/browser/extensions/formautofill/test/browser/address/browser_address_capture_form_removal.js +++ b/browser/extensions/formautofill/test/browser/address/browser_address_capture_form_removal.js @@ -17,7 +17,7 @@ async function expectSavedAddresses(expectedAddresses) { } const ADDRESS_FIELD_VALUES = { - "given-name": "Test User", + "given-name": "John", organization: "Sesame Street", "street-address": "123 Sesame Street", }; diff --git a/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_display.js b/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_display.js index 11a0b676f5cb..5a799bf1b723 100644 --- a/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_display.js +++ b/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_display.js @@ -30,7 +30,8 @@ add_task(async function test_save_doorhanger_shown_no_profile() { await focusUpdateSubmitForm(browser, { focusSelector: "#given-name", newValues: { - "#given-name": "Test User", + "#given-name": "John", + "#family-name": "Doe", "#organization": "Sesame Street", "#street-address": "123 Sesame Street", "#tel": "1-345-345-3456", @@ -85,7 +86,8 @@ add_task( await focusUpdateSubmitForm(browser, { focusSelector: "#given-name", newValues: { - "#given-name": "Cena", + "#given-name": "John", + "#family-name": "Doe", "#street-address": TEST_ADDRESS_1["street-address"], "#country": TEST_ADDRESS_1.country, "#email": TEST_ADDRESS_1.email, diff --git a/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_invalid_fields.js b/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_invalid_fields.js index 61357b01f9a3..71eb3bc9c348 100644 --- a/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_invalid_fields.js +++ b/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_invalid_fields.js @@ -50,6 +50,7 @@ add_task(async function test_do_not_save_invalid_fields() { focusSelector: "#given-name", newValues: { "#given-name": VALID_ADDRESS["given-name"], + "#family-name": VALID_ADDRESS["family-name"], "#street-address": VALID_ADDRESS["street-address"], "#address-level1": VALID_ADDRESS["address-level1"], "#address-level2": VALID_ADDRESS["address-level2"], diff --git a/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_state.js b/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_state.js index d53e35d24fef..1ac1b299bd83 100644 --- a/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_state.js +++ b/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_state.js @@ -24,7 +24,8 @@ add_setup(async function () { add_task(async function test_save_doorhanger_state_invalid() { const DEFAULT = { - "given-name": "Test User", + "given-name": "John", + "family-name": "Doe", organization: "Mozilla", "street-address": "123 Sesame Street", country: "US", @@ -53,6 +54,7 @@ add_task(async function test_save_doorhanger_state_invalid() { focusSelector: "#given-name", newValues: { "#given-name": DEFAULT["given-name"], + "#family-name": DEFAULT["family-name"], "#organization": DEFAULT.organization, "#street-address": DEFAULT["street-address"], "#address-level1": TEST.filled["address-level1"], @@ -71,7 +73,8 @@ add_task(async function test_save_doorhanger_state_invalid() { add_task(async function test_save_doorhanger_state_valid() { const DEFAULT = { - "given-name": "Test User", + "given-name": "John", + "family-name": "Doe", organization: "Mozilla", "street-address": "123 Sesame Street", country: "US", @@ -108,6 +111,7 @@ add_task(async function test_save_doorhanger_state_valid() { focusSelector: "#given-name", newValues: { "#given-name": DEFAULT["given-name"], + "#family-name": DEFAULT["family-name"], "#organization": DEFAULT.organization, "#street-address": DEFAULT["street-address"], "#address-level1": TEST.filled["address-level1"], diff --git a/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_tel.js b/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_tel.js index 32167ce66096..4d95452da483 100644 --- a/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_tel.js +++ b/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_tel.js @@ -25,7 +25,8 @@ add_setup(async function () { add_task(async function test_save_doorhanger_tel_invalid() { const EXPECTED = [ { - "given-name": "Test User", + "given-name": "John", + "family-name": "Doe", organization: "Mozilla", "street-address": "123 Sesame Street", tel: "", @@ -49,7 +50,8 @@ add_task(async function test_save_doorhanger_tel_invalid() { await focusUpdateSubmitForm(browser, { focusSelector: "#given-name", newValues: { - "#given-name": "Test User", + "#given-name": "John", + "#family-name": "Doe", "#organization": "Mozilla", "#street-address": "123 Sesame Street", "#tel": TEST, @@ -69,7 +71,8 @@ add_task(async function test_save_doorhanger_tel_invalid() { add_task(async function test_save_doorhanger_tel_concatenated() { const EXPECTED = [ { - "given-name": "Test User", + "given-name": "John", + "family-name": "Doe", organization: "Mozilla", tel: "+15202486621", }, @@ -77,6 +80,7 @@ add_task(async function test_save_doorhanger_tel_concatenated() { const MARKUP = `
+ @@ -97,7 +101,8 @@ add_task(async function test_save_doorhanger_tel_concatenated() { await focusUpdateSubmitForm(browser, { focusSelector: "#given-name", newValues: { - "#given-name": "Test User", + "#given-name": "John", + "#family-name": "Doe", "#organization": "Mozilla", "#tel-country-code": "+1", "#tel-national": "5202486621", diff --git a/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_ui.js b/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_ui.js index 5df9f146c21b..8a63cbe746bb 100644 --- a/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_ui.js +++ b/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_ui.js @@ -153,6 +153,7 @@ add_task(async function test_address_display_in_save_doorhanger() { { description: "Test submit a form without email and tel fields", form: { + "#given-name": "John", "#family-name": "Doe", "#organization": "Mozilla", "#street-address": "123 Sesame Street", @@ -163,6 +164,7 @@ add_task(async function test_address_display_in_save_doorhanger() { description: "Test submit a form with email field", form: { "#given-name": "John", + "#family-name": "Doe", "#organization": "Mozilla", "#street-address": "123 Sesame Street", "#email": "test@mozilla.org", diff --git a/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_unsupported_region.js b/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_unsupported_region.js index b63265fd9ceb..15e0dd9f81cb 100644 --- a/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_unsupported_region.js +++ b/browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_unsupported_region.js @@ -22,7 +22,8 @@ add_task(async function test_save_doorhanger_supported_region() { await focusUpdateSubmitForm(browser, { focusSelector: "#given-name", newValues: { - "#given-name": "John Doe", + "#given-name": "John", + "#family-name": "Doe", "#organization": "Mozilla", "#street-address": "123 Sesame Street", "#country": "US", @@ -44,7 +45,8 @@ add_task(async function test_save_doorhanger_unsupported_region_from_record() { await focusUpdateSubmitForm(browser, { focusSelector: "#given-name", newValues: { - "#given-name": "John Doe", + "#given-name": "John", + "#family-name": "Doe", "#organization": "Mozilla", "#street-address": "123 Sesame Street", "#country": "DE", @@ -70,7 +72,8 @@ add_task(async function test_save_doorhanger_unsupported_region_from_pref() { await focusUpdateSubmitForm(browser, { focusSelector: "#given-name", newValues: { - "#given-name": "John Doe", + "#given-name": "John", + "#family-name": "Doe", "#organization": "Mozilla", "#street-address": "123 Sesame Street", }, diff --git a/browser/extensions/formautofill/test/mochitest/test_autofill_and_ordinal_forms.html b/browser/extensions/formautofill/test/mochitest/test_autofill_and_ordinal_forms.html index 5c143c3f4ad0..54efc8335925 100644 --- a/browser/extensions/formautofill/test/mochitest/test_autofill_and_ordinal_forms.html +++ b/browser/extensions/formautofill/test/mochitest/test_autofill_and_ordinal_forms.html @@ -47,7 +47,7 @@ add_task(async function check_switch_autofill_form_popup() { checkMenuEntries( [ `{"primary":"+13453453456","secondary":"123 Sesame Street."}`, - `{"primary":"","secondary":"","categories":["name","organization","address","tel"],"focusedCategory":"tel"}`, + `{"primary":"","secondary":"","categories":["organization","address","tel","name"],"focusedCategory":"tel"}`, ], false ); @@ -75,7 +75,7 @@ add_task(async function check_switch_autofill_form_popup_back() { checkMenuEntries( [ `{"primary":"+13453453456","secondary":"123 Sesame Street."}`, - `{"primary":"","secondary":"","categories":["name","organization","address","tel"],"focusedCategory":"tel"}`, + `{"primary":"","secondary":"","categories":["organization","address","tel","name"],"focusedCategory":"tel"}`, ], false ); diff --git a/browser/extensions/formautofill/test/unit/head.js b/browser/extensions/formautofill/test/unit/head.js index 7b01a84da01a..b8c5df7ef7b3 100644 --- a/browser/extensions/formautofill/test/unit/head.js +++ b/browser/extensions/formautofill/test/unit/head.js @@ -308,7 +308,8 @@ function getSyncChangeCounter(records, guid) { /** * Performs a partial deep equality check to determine if an object contains - * the given fields. + * the given fields. To ensure the object doesn't contain a property, set the + * property of the `fields` object to `undefined` * * @param {object} object * The object to check. Unlike `ObjectUtils.deepEqual`, properties in @@ -320,9 +321,11 @@ function getSyncChangeCounter(records, guid) { */ function objectMatches(object, fields) { let actual = {}; - for (let key in fields) { + for (const key in fields) { if (!object.hasOwnProperty(key)) { - return false; + if (fields[key] != undefined) { + return false; + } } actual[key] = object[key]; } diff --git a/browser/extensions/formautofill/test/unit/test_addressRecords.js b/browser/extensions/formautofill/test/unit/test_addressRecords.js index a33581ee0221..a2d7ce23b5d5 100644 --- a/browser/extensions/formautofill/test/unit/test_addressRecords.js +++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js @@ -8,9 +8,7 @@ const TEST_STORE_FILE_NAME = "test-profile.json"; const COLLECTION_NAME = "addresses"; const TEST_ADDRESS_1 = { - "given-name": "Timothy", - "additional-name": "John", - "family-name": "Berners-Lee", + name: "Timothy John Berners-Lee", organization: "World Wide Web Consortium", "street-address": "32 Vassar Street\nMIT Room 32-G524", "address-level2": "Cambridge", @@ -28,8 +26,7 @@ const TEST_ADDRESS_2 = { }; const TEST_ADDRESS_3 = { - "given-name": "Timothy", - "family-name": "Berners-Lee", + name: "Timothy Berners-Lee", "street-address": "Other Address", "postal-code": "12345", }; @@ -40,7 +37,9 @@ const TEST_ADDRESS_WITH_EMPTY_FIELD = { }; const TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD = { - name: "", + "given-name": "", + "additional-name": "", + "family-name": "", "address-line1": "", "address-line2": "", "address-line3": "", @@ -106,13 +105,18 @@ add_task(async function test_getAll() { do_check_record_matches(addresses[1], TEST_ADDRESS_2); // Check computed fields. - Assert.equal(addresses[0].name, "Timothy John Berners-Lee"); + Assert.equal(addresses[0]["given-name"], "Timothy"); + Assert.equal(addresses[0]["additional-name"], "John"); + Assert.equal(addresses[0]["family-name"], "Berners-Lee"); Assert.equal(addresses[0]["address-line1"], "32 Vassar Street"); Assert.equal(addresses[0]["address-line2"], "MIT Room 32-G524"); // Test with rawData set. addresses = await profileStorage.addresses.getAll({ rawData: true }); - Assert.equal(addresses[0].name, undefined); + // For backward-compatibility, we keep *-name fields when `rawData` is true + Assert.equal(addresses[0]["given-name"], "Timothy"); + Assert.equal(addresses[0]["additional-name"], "John"); + Assert.equal(addresses[0]["family-name"], "Berners-Lee"); Assert.equal(addresses[0]["address-line1"], undefined); Assert.equal(addresses[0]["address-line2"], undefined); @@ -138,7 +142,10 @@ add_task(async function test_get() { // Test with rawData set. address = await profileStorage.addresses.get(guid, { rawData: true }); - Assert.equal(address.name, undefined); + // For backward-compatibility, we keep *-name fields when `rawData` is true + Assert.equal(address["given-name"], "Timothy"); + Assert.equal(address["additional-name"], "John"); + Assert.equal(address["family-name"], "Berners-Lee"); Assert.equal(address["address-line1"], undefined); Assert.equal(address["address-line2"], undefined); @@ -258,8 +265,7 @@ add_task(async function test_update() { address = await profileStorage.addresses.get(guid, { rawData: true }); - Assert.equal(address["given-name"], "Tim"); - Assert.equal(address["family-name"], "Berners"); + Assert.equal(address.name, "Tim Berners"); Assert.equal(address["street-address"], undefined); Assert.equal(address["postal-code"], "12345"); Assert.notEqual(address.timeLastModified, timeLastModified); diff --git a/browser/extensions/formautofill/test/unit/test_migrateRecords.js b/browser/extensions/formautofill/test/unit/test_migrateRecords.js index 24b13b43221e..33c2d5387630 100644 --- a/browser/extensions/formautofill/test/unit/test_migrateRecords.js +++ b/browser/extensions/formautofill/test/unit/test_migrateRecords.js @@ -27,15 +27,16 @@ const ADDRESS_TESTCASES = [ record: { guid: "test-guid", version: ADDRESS_SCHEMA_VERSION, - "given-name": "Timothy", - name: "John", // The cached name field doesn't align "given-name" but it + // The cached address-line1 field doesn't align "street-address" but it // won't be recomputed because the migration isn't invoked. + "address-line1": "Some Address", + "street-address": "32 Vassar Street", }, expectedResult: { guid: "test-guid", version: ADDRESS_SCHEMA_VERSION, - "given-name": "Timothy", - name: "John", + "address-line1": "Some Address", + "street-address": "32 Vassar Street", }, }, { @@ -44,14 +45,14 @@ const ADDRESS_TESTCASES = [ record: { guid: "test-guid", version: 99, - "given-name": "Timothy", - name: "John", + "address-line1": "Some Address", + "street-address": "32 Vassar Street", }, expectedResult: { guid: "test-guid", version: 99, - "given-name": "Timothy", - name: "John", + "address-line1": "Some Address", + "street-address": "32 Vassar Street", }, }, { @@ -60,14 +61,14 @@ const ADDRESS_TESTCASES = [ record: { guid: "test-guid", version: 0, - "given-name": "Timothy", - name: "John", + "address-line1": "Some Address", + "street-address": "32 Vassar Street", }, expectedResult: { guid: "test-guid", version: ADDRESS_SCHEMA_VERSION, - "given-name": "Timothy", - name: "Timothy", + "address-line1": "32 Vassar Street", + "street-address": "32 Vassar Street", }, }, { @@ -75,15 +76,15 @@ const ADDRESS_TESTCASES = [ "The record version is omitted. The migration should be invoked.", record: { guid: "test-guid", - "given-name": "Timothy", - name: "John", + "address-line1": "Some Address", + "street-address": "32 Vassar Street", "unknown-1": "an unknown field from another client", }, expectedResult: { guid: "test-guid", version: ADDRESS_SCHEMA_VERSION, - "given-name": "Timothy", - name: "Timothy", + "address-line1": "32 Vassar Street", + "street-address": "32 Vassar Street", "unknown-1": "an unknown field from another client", }, }, @@ -93,15 +94,15 @@ const ADDRESS_TESTCASES = [ record: { guid: "test-guid", version: "ABCDE", - "given-name": "Timothy", - name: "John", + "address-line1": "Some Address", + "street-address": "32 Vassar Street", "unknown-1": "an unknown field from another client", }, expectedResult: { guid: "test-guid", version: ADDRESS_SCHEMA_VERSION, - "given-name": "Timothy", - name: "Timothy", + "address-line1": "32 Vassar Street", + "street-address": "32 Vassar Street", "unknown-1": "an unknown field from another client", }, }, @@ -111,13 +112,13 @@ const ADDRESS_TESTCASES = [ record: { guid: "test-guid", version: ADDRESS_SCHEMA_VERSION, - "given-name": "Timothy", + "street-address": "32 Vassar Street", }, expectedResult: { guid: "test-guid", version: ADDRESS_SCHEMA_VERSION, - "given-name": "Timothy", - name: "Timothy", + "address-line1": "32 Vassar Street", + "street-address": "32 Vassar Street", }, }, { @@ -137,6 +138,98 @@ const ADDRESS_TESTCASES = [ name: undefined, }, }, + + // Bug 1836438 - Migrate "*-name" to "name" + { + description: + "Migrate address - `given-name`, `additional-name`, and `family-name` should be migrated to `name`", + record: { + guid: "test-guid", + version: ADDRESS_SCHEMA_VERSION, + "given-name": "Timothy", + "additional-name": "John", + "family-name": "Berners-Lee", + }, + expectedResult: { + guid: "test-guid", + version: ADDRESS_SCHEMA_VERSION, + name: "Timothy John Berners-Lee", + }, + }, + { + description: "Migrate address - `given-name` should be migrated to `name`", + record: { + guid: "test-guid", + version: ADDRESS_SCHEMA_VERSION, + "given-name": "Timothy", + }, + expectedResult: { + guid: "test-guid", + version: ADDRESS_SCHEMA_VERSION, + name: "Timothy", + }, + }, + { + description: + "Migrate address - `additional-name` should be migrated to `name`", + record: { + guid: "test-guid", + version: ADDRESS_SCHEMA_VERSION, + "additional-name": "John", + }, + expectedResult: { + guid: "test-guid", + version: ADDRESS_SCHEMA_VERSION, + name: "John", + }, + }, + { + description: "Migrate address - `family-name` should be migrated to `name`", + record: { + guid: "test-guid", + version: ADDRESS_SCHEMA_VERSION, + "family-name": "Berners-Lee", + }, + expectedResult: { + guid: "test-guid", + version: ADDRESS_SCHEMA_VERSION, + name: "Berners-Lee", + }, + }, + { + description: + "Migrate address - `name` should still be empty when there is no *-name fields in the record", + record: { + guid: "test-guid", + version: ADDRESS_SCHEMA_VERSION, + }, + expectedResult: { + guid: "test-guid", + version: ADDRESS_SCHEMA_VERSION, + }, + }, + { + description: + "Migrate address - do not run migration as long as the name field exists", + record: { + guid: "test-guid", + version: ADDRESS_SCHEMA_VERSION, + // The cached field doesn't align "name" but it + // won't be recomputed because the migration isn't invoked. + "given-name": "Timothy", + "additional-name": "John", + "family-name": "Berners-Lee", + name: "Jane", + }, + expectedResult: { + guid: "test-guid", + version: ADDRESS_SCHEMA_VERSION, + "given-name": "Timothy", + "additional-name": "John", + "family-name": "Berners-Lee", + name: "Jane", + }, + }, ]; const CREDIT_CARD_TESTCASES = [ diff --git a/browser/extensions/formautofill/test/unit/test_reconcile.js b/browser/extensions/formautofill/test/unit/test_reconcile.js index 9784bfb2639b..d9417eef8cf2 100644 --- a/browser/extensions/formautofill/test/unit/test_reconcile.js +++ b/browser/extensions/formautofill/test/unit/test_reconcile.js @@ -21,15 +21,15 @@ const ADDRESS_RECONCILE_TESTCASES = [ // So when we last wrote the record to the server, it had these values. guid: "2bbd2d8fbc6b", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, local: [ { // The current local record - by comparing against parent we can see that - // only the given-name has changed locally. - "given-name": "Skip", - "family-name": "Hammond", + // only the name has changed locally. + name: "Skip", + "street-address": "32 Vassar Street", }, ], remote: { @@ -38,13 +38,13 @@ const ADDRESS_RECONCILE_TESTCASES = [ // can safely ignore the incoming record and write our local changes. guid: "2bbd2d8fbc6b", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, reconciled: { guid: "2bbd2d8fbc6b", - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", }, }, { @@ -52,25 +52,25 @@ const ADDRESS_RECONCILE_TESTCASES = [ parent: { guid: "e3680e9f890d", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, local: [ { - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, ], remote: { guid: "e3680e9f890d", version: 1, - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", }, reconciled: { guid: "e3680e9f890d", - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", }, }, { @@ -78,26 +78,26 @@ const ADDRESS_RECONCILE_TESTCASES = [ parent: { guid: "0cba738b1be0", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, local: [ { - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", tel: "123456", }, ], remote: { guid: "0cba738b1be0", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, reconciled: { guid: "0cba738b1be0", - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", tel: "123456", }, }, @@ -106,26 +106,26 @@ const ADDRESS_RECONCILE_TESTCASES = [ parent: { guid: "be3ef97f8285", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, local: [ { - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, ], remote: { guid: "be3ef97f8285", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", tel: "123456", }, reconciled: { guid: "be3ef97f8285", - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", tel: "123456", }, }, @@ -134,27 +134,27 @@ const ADDRESS_RECONCILE_TESTCASES = [ parent: { guid: "9627322248ec", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", tel: "123456", }, local: [ { - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, ], remote: { guid: "9627322248ec", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", tel: "123456", }, reconciled: { guid: "9627322248ec", - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, }, { @@ -162,27 +162,27 @@ const ADDRESS_RECONCILE_TESTCASES = [ parent: { guid: "7d7509f3eeb2", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", tel: "123456", }, local: [ { - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", tel: "123456", }, ], remote: { guid: "7d7509f3eeb2", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, reconciled: { guid: "7d7509f3eeb2", - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, }, { @@ -191,17 +191,17 @@ const ADDRESS_RECONCILE_TESTCASES = [ // The last time we wrote this to the server, country was NZ. guid: "e087a06dfc57", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", country: "NZ", // We also had an unknown field we round-tripped foo: "bar", }, local: [ { - // The current local record - so locally we've changed given-name to Skip. - "given-name": "Skip", - "family-name": "Hammond", + // The current local record - so locally we've changed name to Skip. + name: "Skip", + "street-address": "32 Vassar Street", country: "NZ", }, ], @@ -209,16 +209,16 @@ const ADDRESS_RECONCILE_TESTCASES = [ // Remotely, we've changed the country to AU. guid: "e087a06dfc57", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", country: "AU", // This is a new unknown field that should send instead! "unknown-1": "an unknown field from another client", }, reconciled: { guid: "e087a06dfc57", - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", country: "AU", // This is a new unknown field that should send instead! "unknown-1": "an unknown field from another client", @@ -229,20 +229,20 @@ const ADDRESS_RECONCILE_TESTCASES = [ parent: { guid: "340a078c596f", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", tel: "123456", country: "US", }, local: [ { - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", country: "US", }, { - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", organization: "Mozilla", country: "US", }, @@ -250,15 +250,15 @@ const ADDRESS_RECONCILE_TESTCASES = [ remote: { guid: "340a078c596f", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", tel: "123456", country: "AU", }, reconciled: { guid: "340a078c596f", - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", organization: "Mozilla", country: "AU", }, @@ -270,29 +270,29 @@ const ADDRESS_RECONCILE_TESTCASES = [ parent: { guid: "0b3a72a1bea2", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", // unknown fields we previously roundtripped foo: "bar", }, local: [ { - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", }, ], remote: { guid: "0b3a72a1bea2", version: 1, - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", // New unknown field that should be the new round trip "unknown-1": "an unknown field from another client", }, reconciled: { guid: "0b3a72a1bea2", - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", }, }, { @@ -301,37 +301,37 @@ const ADDRESS_RECONCILE_TESTCASES = [ // This is what we last wrote to the sync server. guid: "62068784d089", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", "unknown-1": "an unknown field from another client", }, local: [ { - // The current version of the local record - the given-name has changed locally. - "given-name": "Skip", - "family-name": "Hammond", + // The current version of the local record - the name has changed locally. + name: "Skip", + "street-address": "32 Vassar Street", }, ], remote: { - // An incoming record has a different given-name than any of the above! + // An incoming record has a different name than any of the above! guid: "62068784d089", version: 1, - "given-name": "Kip", - "family-name": "Hammond", + name: "Kip", + "street-address": "32 Vassar Street", "unknown-1": "an unknown field from another client", }, forked: { // So we've forked the local record to a new GUID (and the next sync is // going to write this as a new record) - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", "unknown-1": "an unknown field from another client", }, reconciled: { // And we've updated the local version of the record to be the remote version. guid: "62068784d089", - "given-name": "Kip", - "family-name": "Hammond", + name: "Kip", + "street-address": "32 Vassar Street", "unknown-1": "an unknown field from another client", }, }, @@ -340,33 +340,33 @@ const ADDRESS_RECONCILE_TESTCASES = [ parent: { guid: "244dbb692e94", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", country: "NZ", }, local: [ { - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", country: "AU", }, ], remote: { guid: "244dbb692e94", version: 1, - "given-name": "Kip", - "family-name": "Hammond", + name: "Kip", + "street-address": "32 Vassar Street", country: "CA", }, forked: { - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", country: "AU", }, reconciled: { guid: "244dbb692e94", - "given-name": "Kip", - "family-name": "Hammond", + name: "Kip", + "street-address": "32 Vassar Street", country: "CA", }, }, @@ -375,31 +375,31 @@ const ADDRESS_RECONCILE_TESTCASES = [ parent: { guid: "6fc45e03d19a", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", country: "AU", }, local: [ { - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, ], remote: { guid: "6fc45e03d19a", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", country: "NZ", }, forked: { - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, reconciled: { guid: "6fc45e03d19a", - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", country: "NZ", }, }, @@ -408,32 +408,32 @@ const ADDRESS_RECONCILE_TESTCASES = [ parent: { guid: "fff9fa27fa18", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", country: "AU", }, local: [ { - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", country: "NZ", }, ], remote: { guid: "fff9fa27fa18", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, forked: { - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", country: "NZ", }, reconciled: { guid: "fff9fa27fa18", - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }, }, { @@ -445,8 +445,8 @@ const ADDRESS_RECONCILE_TESTCASES = [ parent: { guid: "5113f329c42f", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", timeCreated: 1234, timeLastModified: 5678, timeLastUsed: 5678, @@ -456,8 +456,8 @@ const ADDRESS_RECONCILE_TESTCASES = [ remote: { guid: "5113f329c42f", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", timeCreated: 1200, timeLastModified: 5700, timeLastUsed: 5700, @@ -465,8 +465,8 @@ const ADDRESS_RECONCILE_TESTCASES = [ }, reconciled: { guid: "5113f329c42f", - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", timeCreated: 1200, timeLastModified: 5700, timeLastUsed: 5678, @@ -481,8 +481,8 @@ const ADDRESS_RECONCILE_TESTCASES = [ parent: { guid: "791e5608b80a", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", timeCreated: 1234, timeLastModified: 5678, timeLastUsed: 5678, @@ -490,15 +490,15 @@ const ADDRESS_RECONCILE_TESTCASES = [ }, local: [ { - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", }, ], remote: { guid: "791e5608b80a", version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", timeCreated: 1300, timeLastModified: 5000, timeLastUsed: 5000, @@ -506,8 +506,8 @@ const ADDRESS_RECONCILE_TESTCASES = [ }, reconciled: { guid: "791e5608b80a", - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", timeCreated: 1234, timeLastUsed: 5678, timesUsed: 6, @@ -1022,8 +1022,8 @@ add_task(async function test_reconcile_unknown_version() { profileStorage.addresses.reconcile({ guid: "31d83d2725ec", version: 3, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", }), /Got unknown record version/ ); @@ -1037,24 +1037,24 @@ add_task(async function test_reconcile_idempotent() { { guid, version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", // an unknown field from a previous sync foo: "bar", }, { sourceSync: true } ); await profileStorage.addresses.update(guid, { - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", organization: "Mozilla", }); let remote = { guid, version: 1, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", + "street-address": "32 Vassar Street", tel: "123456", "unknown-1": "an unknown field from another client", }; @@ -1069,8 +1069,8 @@ add_task(async function test_reconcile_idempotent() { ok( objectMatches(updatedRecord, { guid: "de1ba7b094fe", - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", organization: "Mozilla", tel: "123456", "unknown-1": "an unknown field from another client", @@ -1089,8 +1089,8 @@ add_task(async function test_reconcile_idempotent() { ok( objectMatches(updatedRecord, { guid: "de1ba7b094fe", - "given-name": "Skip", - "family-name": "Hammond", + name: "Skip", + "street-address": "32 Vassar Street", organization: "Mozilla", tel: "123456", "unknown-1": "an unknown field from another client", diff --git a/browser/extensions/formautofill/test/unit/test_storage_syncfields.js b/browser/extensions/formautofill/test/unit/test_storage_syncfields.js index e304aa4df0e7..9325a877cf1b 100644 --- a/browser/extensions/formautofill/test/unit/test_storage_syncfields.js +++ b/browser/extensions/formautofill/test/unit/test_storage_syncfields.js @@ -8,9 +8,7 @@ const TEST_STORE_FILE_NAME = "test-profile.json"; const TEST_ADDRESS_1 = { - "given-name": "Timothy", - "additional-name": "John", - "family-name": "Berners-Lee", + name: "Timothy John Berners-Lee", organization: "World Wide Web Consortium", "street-address": "32 Vassar Street\nMIT Room 32-G524", "address-level2": "Cambridge", @@ -195,7 +193,7 @@ add_task(async function test_add_resurrects_tombstones() { equal(guid, guid3); let got = await profileStorage.addresses.get(guid); - equal(got["given-name"], TEST_ADDRESS_1["given-name"]); + equal(got.name, TEST_ADDRESS_1.name); }); add_task(async function test_remove_sourceSync_localChanges() { diff --git a/browser/extensions/formautofill/test/unit/test_sync.js b/browser/extensions/formautofill/test/unit/test_sync.js index bc9467d7c9a0..e2c04c78e5b1 100644 --- a/browser/extensions/formautofill/test/unit/test_sync.js +++ b/browser/extensions/formautofill/test/unit/test_sync.js @@ -25,9 +25,7 @@ initTestLogging("Trace"); const TEST_STORE_FILE_NAME = "test-profile.json"; const TEST_PROFILE_1 = { - "given-name": "Timothy", - "additional-name": "John", - "family-name": "Berners-Lee", + name: "Timothy John Berners-Lee", organization: "World Wide Web Consortium", "street-address": "32 Vassar Street\nMIT Room 32-G524", "address-level2": "Cambridge", @@ -272,7 +270,7 @@ add_task(async function test_incoming_existing() { // now server records that modify the existing items. let modifiedEntry1 = Object.assign({}, TEST_PROFILE_1, { version: 1, - "given-name": "NewName", + name: "NewName", }); let lastSync = await engine.getLastSync(); @@ -482,8 +480,7 @@ add_task(async function test_applyIncoming_incoming_restored() { let localRecord = await profileStorage.addresses.get(guid); ok( objectMatches(localRecord, { - "given-name": "Timothy", - "family-name": "Berners-Lee", + name: "Timothy John Berners-Lee", "street-address": "I moved!", }) ); @@ -537,8 +534,7 @@ add_task(async function test_applyIncoming_outgoing_restored() { ok(!serverPayload.deleted, "Should resurrect record on server"); ok( objectMatches(serverPayload.entry, { - "given-name": "Timothy", - "family-name": "Berners-Lee", + name: "Timothy John Berners-Lee", "street-address": "I moved!", // resurrection also beings back any unknown fields we had "unknown-1": "some unknown data from another client", @@ -750,8 +746,7 @@ add_task(async function test_dedupe_multiple_candidates() { // overwrite that tombstone when we see A. let localRecord = { - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", organization: "Mozilla", country: "AU", tel: "+12345678910", @@ -799,16 +794,14 @@ add_task(async function test_dedupe_multiple_candidates() { await expectLocalProfiles(profileStorage, [ { guid: aGuid, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", organization: "Mozilla", country: "AU", tel: "+12345678910", }, { guid: bGuid, - "given-name": "Mark", - "family-name": "Hammond", + name: "Mark Hammond", organization: "Mozilla", country: "AU", tel: "+12345678910", @@ -875,14 +868,12 @@ add_task(async function test_reconcile_both_modified_conflict() { await expectLocalProfiles(profileStorage, [ { guid, - "given-name": "Timothy", - "family-name": "Berners-Lee", + name: "Timothy John Berners-Lee", "street-address": "I moved, too!", }, { guid: forkedPayload.id, - "given-name": "Timothy", - "family-name": "Berners-Lee", + name: "Timothy John Berners-Lee", "street-address": "I moved!", }, ]); diff --git a/browser/extensions/formautofill/test/unit/test_sync_deprecate_address_x_name_fields.js b/browser/extensions/formautofill/test/unit/test_sync_deprecate_address_x_name_fields.js new file mode 100644 index 000000000000..f30a70870982 --- /dev/null +++ b/browser/extensions/formautofill/test/unit/test_sync_deprecate_address_x_name_fields.js @@ -0,0 +1,1100 @@ +/** + * Tests sync functionality. + */ + +/* import-globals-from ../../../../../services/sync/tests/unit/head_appinfo.js */ +/* import-globals-from ../../../../../services/common/tests/unit/head_helpers.js */ +/* import-globals-from ../../../../../services/sync/tests/unit/head_helpers.js */ +/* import-globals-from ../../../../../services/sync/tests/unit/head_http_server.js */ + +"use strict"; + +const { Service } = ChromeUtils.importESModule( + "resource://services-sync/service.sys.mjs" +); + +const { AddressesEngine } = ChromeUtils.importESModule( + "resource://autofill/FormAutofillSync.sys.mjs" +); + +Services.prefs.setCharPref("extensions.formautofill.loglevel", "Debug"); +initTestLogging("Trace"); + +const TEST_STORE_FILE_NAME = "test-profile.json"; + +function expectProfiles(profiles, expected) { + expected.sort((a, b) => a.guid.localeCompare(b.guid)); + profiles.sort((a, b) => a.guid.localeCompare(b.guid)); + try { + deepEqual( + profiles.map(p => p.guid), + expected.map(p => p.guid) + ); + for (let i = 0; i < expected.length; i++) { + let thisExpected = expected[i]; + let thisGot = profiles[i]; + // always check "deleted". + equal(thisExpected.deleted, thisGot.deleted); + ok(objectMatches(thisGot, thisExpected)); + } + } catch (ex) { + info("Comparing expected profiles:"); + info(JSON.stringify(expected, undefined, 2)); + info("against actual profiles:"); + info(JSON.stringify(profiles, undefined, 2)); + throw ex; + } +} + +async function expectServerProfiles(collection, expected) { + const profiles = collection + .payloads() + .map(payload => Object.assign({ guid: payload.id }, payload.entry)); + expectProfiles(profiles, expected); +} + +async function expectLocalProfiles(profileStorage, expected) { + const profiles = await profileStorage.addresses.getAll({ + rawData: true, + includeDeleted: true, + }); + expectProfiles(profiles, expected); +} + +async function setup() { + let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME); + // should always start with no profiles. + Assert.equal( + (await profileStorage.addresses.getAll({ includeDeleted: true })).length, + 0 + ); + + Services.prefs.setCharPref( + "services.sync.log.logger.engine.addresses", + "Trace" + ); + let engine = new AddressesEngine(Service); + await engine.initialize(); + // Avoid accidental automatic sync due to our own changes + Service.scheduler.syncThreshold = 10000000; + let syncID = await engine.resetLocalSyncID(); + let server = serverForUsers( + { foo: "password" }, + { + meta: { + global: { + engines: { addresses: { version: engine.version, syncID } }, + }, + }, + addresses: {}, + } + ); + + Service.engineManager._engines.addresses = engine; + engine.enabled = true; + engine._store._storage = profileStorage.addresses; + + generateNewKeys(Service.collectionKeys); + + await SyncTestingInfrastructure(server); + + let collection = server.user("foo").collection("addresses"); + + return { profileStorage, server, collection, engine }; +} + +async function cleanup(server) { + let promiseStartOver = promiseOneObserver("weave:service:start-over:finish"); + await Service.startOver(); + await promiseStartOver; + await promiseStopServer(server); +} + +function setupServerRecords(server, records) { + for (const record of records) { + server.insertWBO( + "foo", + "addresses", + new ServerWBO( + record.guid, + encryptPayload({ + id: record.guid, + entry: Object.assign({}, record), + }), + getDateForSync() + ) + ); + } +} + +function assertNumberRecordUploadedBySync(engine, expectedNumber) { + const uploadOutgoing = engine._uploadOutgoing; + engine._uploadOutgoing = async function () { + engine._uploadOutgoing = uploadOutgoing; + try { + await uploadOutgoing.call(this); + } finally { + Assert.equal(this._modified.ids().length, expectedNumber); + } + }; +} + +/** + * The following tests test uploading. Ensure `given-name`, `additional-name`, and `family-name` fields are included + * in the sync payload. + */ + +add_task(async function test_local_upload() { + const { collection, profileStorage, server, engine } = await setup(); + + try { + const localGuid = await profileStorage.addresses.add({ + name: "Mr John William Doe", + "street-address": "Some Address", + }); + + await engine.setLastSync(0); + await engine.sync(); + + await expectServerProfiles(collection, [ + { + guid: localGuid, + version: 1, + name: "Mr John William Doe", + "given-name": "John", + "additional-name": "William", + "family-name": "Doe", + "street-address": "Some Address", + }, + ]); + } finally { + await cleanup(server); + } +}); + +add_task(async function test_local_upload_no_addtional_name() { + const { collection, profileStorage, server, engine } = await setup(); + + try { + const localGuid = await profileStorage.addresses.add({ + name: "Timothy Berners-Lee", + "street-address": "Some Address", + }); + + await engine.setLastSync(0); + await engine.sync(); + + await expectServerProfiles(collection, [ + { + guid: localGuid, + version: 1, + name: "Timothy Berners-Lee", + "given-name": "Timothy", + "additional-name": undefined, + "family-name": "Berners-Lee", + "street-address": "Some Address", + }, + ]); + } finally { + await cleanup(server); + } +}); + +add_task(async function test_local_upload_no_name() { + const { collection, profileStorage, server, engine } = await setup(); + + try { + const localGuid = await profileStorage.addresses.add({ + "street-address": "Some Address", + }); + + await engine.setLastSync(0); + await engine.sync(); + + await expectServerProfiles(collection, [ + { + guid: localGuid, + version: 1, + name: undefined, + "given-name": undefined, + "additional-name": undefined, + "family-name": undefined, + "street-address": "Some Address", + }, + ]); + } finally { + await cleanup(server); + } +}); + +/** + * The following tasks test cases where no matching local record is found while applying an + * incoming record. We test: + * 1. An incoming record with `name` field and `*-name` fields, indicating this is a new record. + * 2. An incoming record with only `*-name` fields, indicating this is an old record. + * 3. An incoming record without any name fields, leaving it unknown whether this record comes + * from a new or an old device. + * + * The expected result for each task is: + * 1. The `name` field should NOT be rebuilt based on the deprecated `*-name` fields. + * 2. The `name` field should be rebuilt based on the deprecated `*-name` fields. + * 3. All the name related fields should remain empty. + */ + +// 1. Remote record is a new record +add_task(async function test_apply_incoming() { + const { profileStorage, server, engine } = await setup(); + + try { + setupServerRecords(server, [ + { + guid: "86d961c7717a", + version: 1, + name: "Mr. John William Doe", + "given-name": "John", + "additional-name": "William", + "family-name": "Doe", + organization: "Mozilla", + }, + ]); + + assertNumberRecordUploadedBySync(engine, 0); + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid: "86d961c7717a", + version: 1, + name: "Mr. John William Doe", // Prefix `Mr.` remains + organization: "Mozilla", + }, + ]); + } finally { + await cleanup(server); + } +}); + +// 2. Remote record is an old record +add_task(async function test_apply_incoming_legacy() { + const { profileStorage, server, engine } = await setup(); + + try { + setupServerRecords(server, [ + { + guid: "86d961c7717a", + version: 1, + "given-name": "John", + "additional-name": "William", + "family-name": "Doe", + organization: "Mozilla", + }, + ]); + + assertNumberRecordUploadedBySync(engine, 0); + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid: "86d961c7717a", + version: 1, + name: "John William Doe", // rebuild name field based on `*-name` fields + organization: "Mozilla", + }, + ]); + } finally { + await cleanup(server); + } +}); + +// 3. Remote record does not have `name` +add_task(async function test_apply_incoming_without_name() { + const { profileStorage, server, engine } = await setup(); + + try { + setupServerRecords(server, [ + { + guid: "86d961c7717a", + version: 1, + "street-address": "Some Address", + }, + ]); + + assertNumberRecordUploadedBySync(engine, 0); + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid: "86d961c7717a", + version: 1, + "street-address": "Some Address", + name: undefined, + "given-name": undefined, + "additional-name": undefined, + "family-name": undefined, + }, + ]); + } finally { + await cleanup(server); + } +}); + +/** + * The following tasks test cases where a matching local record is found by matching GUID while applying an + * incoming record. We test: + * 1. An incoming record with `name` field and `*-name` fields, indicating this is a new record. + * 2. An incoming record with only `*-name` fields, indicating this is an old record, `*-name` is not updated + * 3. An incoming record with only `*-name` fields, indicating this is an old record, `*-name` is updated + * 4. An incoming record with only `*-name` fields, indicating this is an old record. Local record does not have `name`. + * 5. An incoming record without any name fields, leaving it unknown whether this record comes + * from a new or an old device. + * + * The expected result for each task is: + * 1. The `name` field of the local record should be replaced by the remote record. + * 2. The `name` field of the local record should NOT be replaced by the remote record, `name` remains the same. + * 3. The `name` field of the local record should be replaced by the remote record. + * 4. The `name` field of the local record should be replaced by the remote record. + * 5. The `name` field of the local record should be replaced by the remote record. + */ + +// 1. Remote record is a new record +add_task(async function test_apply_incoming_repalce() { + const { profileStorage, server, engine } = await setup(); + + try { + // Setup the local record and sync the record to sync + const localGuid = await profileStorage.addresses.add({ + name: "Mr John William Doe", + "street-address": "Some Address", + email: "john.doe@mozilla.org", + }); + await engine.setLastSync(0); + await engine.sync(); + + // Remote record is updated by a new device + setupServerRecords(server, [ + { + guid: localGuid, + version: 1, + name: "Dr Timothy Berners Lee", + "given-name": "Timothy", + "additional-name": "Berners", + "family-name": "Lee", + "street-address": "32 Vassar Street", // updated + organization: "Mozilla", // added + email: undefined, // removed + }, + ]); + + assertNumberRecordUploadedBySync(engine, 0); + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid: localGuid, + version: 1, + name: "Dr Timothy Berners Lee", // Should be replaced! + "street-address": "32 Vassar Street", + organization: "Mozilla", + email: undefined, + }, + ]); + } finally { + await cleanup(server); + } +}); + +// 2. Remote record is an old record, `*-name` is not updated +add_task(async function test_apply_incoming_legacy_replace_name_is_updated() { + const { profileStorage, server, engine } = await setup(); + + try { + // Setup the local record and sync the record to sync + const localGuid = await profileStorage.addresses.add({ + name: "Mr John William Doe", + "street-address": "Some Address", + email: "john.doe@mozilla.org", + }); + await engine.setLastSync(0); + await engine.sync(); + + // Remote record is updated by an old device + setupServerRecords(server, [ + { + guid: localGuid, + version: 1, + "given-name": "John", + "additional-name": "William", + "family-name": "Doe", + "street-address": "32 Vassar Street", // updated + organization: "Mozilla", // added + email: undefined, // removed + }, + ]); + + assertNumberRecordUploadedBySync(engine, 0); + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid: localGuid, + version: 1, + name: "Mr John William Doe", // Shoult not be replaced! + "street-address": "32 Vassar Street", + organization: "Mozilla", + email: undefined, + }, + ]); + } finally { + await cleanup(server); + } +}); + +// 3. Remote record is an old record, `*-name` is updated +add_task( + async function test_apply_incoming_legacy_replace_name_is_not_updated() { + const { profileStorage, server, engine } = await setup(); + + try { + // Setup the local record and sync the record to sync + const localGuid = await profileStorage.addresses.add({ + name: "Mr John William Doe", + "street-address": "Some Address", + email: "john.doe@mozilla.org", + }); + await engine.setLastSync(0); + await engine.sync(); + + // Remote record is updated by an old device + setupServerRecords(server, [ + { + guid: localGuid, + version: 1, + "given-name": "Timothy", + "additional-name": "Berners", + "family-name": "Lee", + "street-address": "32 Vassar Street", // updated + organization: "Mozilla", // added + email: undefined, // removed + }, + ]); + + assertNumberRecordUploadedBySync(engine, 0); + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid: localGuid, + version: 1, + name: "Timothy Berners Lee", // Shoult be replaced! + "street-address": "32 Vassar Street", + organization: "Mozilla", + email: undefined, + }, + ]); + } finally { + await cleanup(server); + } + } +); + +// 4. Remote record is an old record. Local record does not have `name` +add_task( + async function test_apply_incoming_legacy_repalce_local_without_name() { + const { profileStorage, server, engine } = await setup(); + + try { + // Setup the local record and sync the record to sync + const localGuid = await profileStorage.addresses.add({ + "street-address": "Some Address", + email: "john.doe@mozilla.org", + }); + await engine.setLastSync(0); + await engine.sync(); + + // Remote record is updated by an old device + setupServerRecords(server, [ + { + guid: localGuid, + version: 1, + "given-name": "Timothy", + "additional-name": "Berners", + "family-name": "Lee", + "street-address": "32 Vassar Street", // updated + organization: "Mozilla", // added + email: undefined, // removed + }, + ]); + + assertNumberRecordUploadedBySync(engine, 0); + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid: localGuid, + version: 1, + name: "Timothy Berners Lee", // Shoult be replaced! + "street-address": "32 Vassar Street", + organization: "Mozilla", + email: undefined, + }, + ]); + } finally { + await cleanup(server); + } + } +); + +// 5. Remote record doesn't have `name` +add_task(async function test_apply_incoming_without_name_replace() { + const { profileStorage, server, engine } = await setup(); + + try { + // Setup the local record and sync the record to sync + const localGuid = await profileStorage.addresses.add({ + name: "Mr. John William Doe", + "street-address": "Some Address", + email: "john.doe@mozilla.org", + }); + await engine.setLastSync(0); + await engine.sync(); + + // Remote record is updated by a device + setupServerRecords(server, [ + { + guid: localGuid, + version: 1, + "street-address": "32 Vassar Street", // updated + organization: "Mozilla", // added + email: undefined, // removed + }, + ]); + + assertNumberRecordUploadedBySync(engine, 0); + await engine.setLastSync(0); + await engine.sync(); + + // `name` field should be removed + await expectLocalProfiles(profileStorage, [ + { + guid: localGuid, + version: 1, + name: undefined, // Should be replaced! + "street-address": "32 Vassar Street", + organization: "Mozilla", + email: undefined, + }, + ]); + } finally { + await cleanup(server); + } +}); + +/** + * The following tasks test cases where a matching local record is found by matching GUID while applying an + * incoming record. And both the local and remote records are modified. + * + * We test: + * 1. An incoming record with `name` field and `*-name` fields, indicating this is a new record. + * 2. An incoming record with only `*-name` fields, indicating this is an old record, `*-name` is not updated + * 3. An incoming record with only `*-name` fields, indicating this is an old record, `*-name` is updated + * 4. An incoming record with only `*-name` fields, indicating this is an old record. Local record does not have `name`. + * 5. An incoming record without any name fields, leaving it unknown whether this record comes + * from a new or an old device. + * + * The expected result for each task is: + * 1. The `name` field of the local record should be replaced by the remote record + * 2. The `name` field of the local record should NOT be replaced by the remote record, `name` remains the same. + * 3. The `name` field of the local record should be replaced by the remote record. + * 4. The `name` field of the local record should be replaced by the remote record. + * 5. The `name` field of the local record should be replaced by the remote record + */ + +// 1. Remote record is a new record +add_task(async function test_apply_incoming_merge() { + const { collection, profileStorage, server, engine } = await setup(); + const LOCAL_ENTRY = { + name: "Mr John William Doe", + "street-address": "Some Address", + email: "john.doe@mozilla.org", + }; + + try { + const guid = await profileStorage.addresses.add(LOCAL_ENTRY); + await engine.setLastSync(0); + await engine.sync(); + + // local modifies "street-address" + let localCopy = Object.assign({}, LOCAL_ENTRY); + localCopy["street-address"] = "I moved!"; + await profileStorage.addresses.update(guid, localCopy); + + setupServerRecords(server, [ + { + guid, + version: 1, + name: "Dr Timothy Berners Lee", // `name` is modified + "given-name": "Timothy", + "additional-name": "Berners", + "family-name": "Lee", + "street-address": "Some Address", + organization: "Mozilla", // `organization` is added + email: undefined, // `email` is removed + }, + ]); + + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid, + version: 1, + name: "Dr Timothy Berners Lee", // Name should be replaced! + "street-address": "I moved!", + organization: "Mozilla", + email: undefined, + }, + ]); + await expectServerProfiles(collection, [ + { + guid, + version: 1, + name: "Dr Timothy Berners Lee", // Name should be replaced! + "given-name": "Timothy", + "additional-name": "Berners", + "family-name": "Lee", + "street-address": "I moved!", + organization: "Mozilla", + email: undefined, + }, + ]); + } finally { + await cleanup(server); + } +}); + +// 2. Remote record is an old record, `*-name` is not updated +add_task(async function test_apply_incoming_legacy_merge_name_is_not_updated() { + const { collection, profileStorage, server, engine } = await setup(); + const LOCAL_ENTRY = { + name: "Mr John William Doe", + "street-address": "Some Address", + email: "john.doe@mozilla.org", + }; + + try { + const guid = await profileStorage.addresses.add(LOCAL_ENTRY); + await engine.setLastSync(0); + await engine.sync(); + + // local modifies "street-address" + let localCopy = Object.assign({}, LOCAL_ENTRY); + localCopy["street-address"] = "I moved!"; + await profileStorage.addresses.update(guid, localCopy); + + setupServerRecords(server, [ + { + guid, + version: 1, + "given-name": "John", + "additional-name": "William", + "family-name": "Doe", + "street-address": "Some Address", + organization: "Mozilla", // `organization` is added + email: undefined, // `email` is removed + }, + ]); + + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid, + version: 1, + name: "Mr John William Doe", // `name` should NOT be replaced! + "street-address": "I moved!", + organization: "Mozilla", + email: undefined, + }, + ]); + await expectServerProfiles(collection, [ + { + guid, + version: 1, + name: "Mr John William Doe", + "given-name": "John", + "additional-name": "William", + "family-name": "Doe", + "street-address": "I moved!", + organization: "Mozilla", + email: undefined, + }, + ]); + } finally { + await cleanup(server); + } +}); + +// 3. Remote record is an old record, `*-name` is updated +add_task(async function test_apply_incoming_legacy_merge_name_is_updated() { + const { collection, profileStorage, server, engine } = await setup(); + const LOCAL_ENTRY = { + name: "Mr John William Doe", + "street-address": "Some Address", + email: "john.doe@mozilla.org", + }; + + try { + const guid = await profileStorage.addresses.add(LOCAL_ENTRY); + await engine.setLastSync(0); + await engine.sync(); + + // local modifies "street-address" + let localCopy = Object.assign({}, LOCAL_ENTRY); + localCopy["street-address"] = "I moved!"; + await profileStorage.addresses.update(guid, localCopy); + + setupServerRecords(server, [ + { + guid, + version: 1, + "given-name": "Timothy", // `name` is modified + "additional-name": "Berners", + "family-name": "Lee", + "street-address": "Some Address", + organization: "Mozilla", // `organization` is added + email: undefined, // `email` is removed + }, + ]); + + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid, + version: 1, + name: "Timothy Berners Lee", // `name` should be replaced! + "street-address": "I moved!", + organization: "Mozilla", + email: undefined, + }, + ]); + await expectServerProfiles(collection, [ + { + guid, + version: 1, + name: "Timothy Berners Lee", + "given-name": "Timothy", + "additional-name": "Berners", + "family-name": "Lee", + "street-address": "I moved!", + organization: "Mozilla", + email: undefined, + }, + ]); + } finally { + await cleanup(server); + } +}); + +// 4. Remote record is an old record. Local record does not have `name` +add_task(async function test_apply_incoming_legacy_merge_local_without_name() { + const { collection, profileStorage, server, engine } = await setup(); + const LOCAL_ENTRY = { + "street-address": "Some Address", + email: "john.doe@mozilla.org", + }; + + try { + const guid = await profileStorage.addresses.add(LOCAL_ENTRY); + await engine.setLastSync(0); + await engine.sync(); + + // local modifies "street-address" + let localCopy = Object.assign({}, LOCAL_ENTRY); + localCopy["street-address"] = "I moved!"; + await profileStorage.addresses.update(guid, localCopy); + + setupServerRecords(server, [ + { + guid, + version: 1, + "given-name": "Timothy", // `name` is modified + "additional-name": "Berners", + "family-name": "Lee", + "street-address": "Some Address", + organization: "Mozilla", // `organization` is added + email: undefined, // `email` is removed + }, + ]); + + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid, + version: 1, + name: "Timothy Berners Lee", // `name` should be replaced! + "street-address": "I moved!", + organization: "Mozilla", + email: undefined, + }, + ]); + await expectServerProfiles(collection, [ + { + guid, + version: 1, + name: "Timothy Berners Lee", + "given-name": "Timothy", + "additional-name": "Berners", + "family-name": "Lee", + "street-address": "I moved!", + organization: "Mozilla", + email: undefined, + }, + ]); + } finally { + await cleanup(server); + } +}); + +// 5. Remote record does not have `name` +add_task(async function test_apply_incoming_without_name_merge() { + const { collection, profileStorage, server, engine } = await setup(); + const LOCAL_ENTRY = { + "street-address": "Some Address", + email: "john.doe@mozilla.org", + }; + + try { + const guid = await profileStorage.addresses.add(LOCAL_ENTRY); + await engine.setLastSync(0); + await engine.sync(); + + // local modifies "street-address" + let localCopy = Object.assign({}, LOCAL_ENTRY); + localCopy["street-address"] = "I moved!"; + await profileStorage.addresses.update(guid, localCopy); + + setupServerRecords(server, [ + { + guid, + version: 1, + "street-address": "Some Address", + organization: "Mozilla", + }, + ]); + + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid, + version: 1, + "street-address": "I moved!", + organization: "Mozilla", // `organization` is added + email: undefined, // `email` is removed + }, + ]); + await expectServerProfiles(collection, [ + { + guid, + version: 1, + "street-address": "I moved!", + organization: "Mozilla", + email: undefined, + }, + ]); + } finally { + await cleanup(server); + } +}); + +/** + * The following tasks test cases where a matching local record is found by running dedupe algorithm while applying an + * incoming record. We test: + * 1. An incoming record with `name` field and `*-name` fields, indicating this is a new record. + * 2. An incoming record with only `*-name` fields, indicating this is an old record. + * 3. An incoming record without any name fields, leaving it unknown whether this record comes + * from a new or an old device. + * + * The expected result is still one record in the local after merging the incoming record + */ + +// 1. Remote record is a new record +add_task(async function test_apply_incoming_dedupe() { + const { collection, profileStorage, server, engine } = await setup(); + const LOCAL_ENTRY = { + name: "Mr John William Doe", + "street-address": "Some Address", + country: "US", + }; + + try { + const localGuid = await profileStorage.addresses.add(LOCAL_ENTRY); + + const remoteGuid = Utils.makeGUID(); + notEqual(localGuid, remoteGuid); + + setupServerRecords(server, [ + { + guid: remoteGuid, + version: 1, + name: "Mr John William Doe", + "given-name": "John", + "additional-name": "William", + "family-name": "Doe", + "street-address": "Some Address", + country: "US", + }, + ]); + + // Local duplicated record has not been synced before, so will trigger + // sync upload after merging. + assertNumberRecordUploadedBySync(engine, 1); + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid: remoteGuid, + version: 1, + name: "Mr John William Doe", + "street-address": "Some Address", + country: "US", + }, + ]); + await expectServerProfiles(collection, [ + { + guid: remoteGuid, + version: 1, + name: "Mr John William Doe", + "given-name": "John", + "additional-name": "William", + "family-name": "Doe", + "street-address": "Some Address", + country: "US", + }, + ]); + } finally { + await cleanup(server); + } +}); + +// 2. Remote record is an old record +add_task(async function test_apply_incoming_legacy_dedupe() { + const { collection, profileStorage, server, engine } = await setup(); + const LOCAL_ENTRY = { + name: "John William Doe", + "street-address": "Some Address", + country: "US", + }; + + try { + const localGuid = await profileStorage.addresses.add(LOCAL_ENTRY); + + const remoteGuid = Utils.makeGUID(); + notEqual(localGuid, remoteGuid); + + setupServerRecords(server, [ + { + guid: remoteGuid, + version: 1, + "given-name": "John", + "additional-name": "William", + "family-name": "Doe", + "street-address": "Some Address", + country: "US", + }, + ]); + + // Local duplicated record has not been synced before, so will trigger + // sync upload after merging. + assertNumberRecordUploadedBySync(engine, 1); + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid: remoteGuid, + version: 1, + name: "John William Doe", + "street-address": "Some Address", + country: "US", + }, + ]); + await expectServerProfiles(collection, [ + { + guid: remoteGuid, + version: 1, + name: "John William Doe", + "given-name": "John", + "additional-name": "William", + "family-name": "Doe", + "street-address": "Some Address", + country: "US", + }, + ]); + } finally { + await cleanup(server); + } +}); + +// 3. Remote record does not have `name` +add_task(async function test_apply_incoming_without_name_dedupe() { + const { collection, profileStorage, server, engine } = await setup(); + const LOCAL_ENTRY = { + "street-address": "Some Address", + country: "US", + }; + + try { + const localGuid = await profileStorage.addresses.add(LOCAL_ENTRY); + + const remoteGuid = Utils.makeGUID(); + notEqual(localGuid, remoteGuid); + + setupServerRecords(server, [ + { + guid: remoteGuid, + version: 1, + "street-address": "Some Address", + country: "US", + }, + ]); + + assertNumberRecordUploadedBySync(engine, 1); + await engine.setLastSync(0); + await engine.sync(); + + await expectLocalProfiles(profileStorage, [ + { + guid: remoteGuid, + version: 1, + "street-address": "Some Address", + country: "US", + }, + ]); + await expectServerProfiles(collection, [ + { + guid: remoteGuid, + version: 1, + "street-address": "Some Address", + country: "US", + }, + ]); + } finally { + await cleanup(server); + } +}); diff --git a/browser/extensions/formautofill/test/unit/test_transformFields.js b/browser/extensions/formautofill/test/unit/test_transformFields.js index 816b94e77dd7..552407f07fa9 100644 --- a/browser/extensions/formautofill/test/unit/test_transformFields.js +++ b/browser/extensions/formautofill/test/unit/test_transformFields.js @@ -203,23 +203,18 @@ const ADDRESS_NORMALIZE_TESTCASES = [ name: "Timothy John Berners-Lee", }, expectedResult: { - "given-name": "Timothy", - "additional-name": "John", - "family-name": "Berners-Lee", + name: "Timothy John Berners-Lee", }, }, { description: 'Has both "name" and split names', address: { - name: "John Doe", - "given-name": "Timothy", - "additional-name": "John", - "family-name": "Berners-Lee", + name: "Timothy John Berners-Lee", + "given-name": "John", + "family-name": "Doe", }, expectedResult: { - "given-name": "Timothy", - "additional-name": "John", - "family-name": "Berners-Lee", + name: "Timothy John Berners-Lee", }, }, { @@ -229,9 +224,39 @@ const ADDRESS_NORMALIZE_TESTCASES = [ "given-name": "Timothy", }, expectedResult: { + name: "John Doe", + }, + }, + { + description: 'Does not have "name", and has split names', + address: { "given-name": "Timothy", + "additional-name": "John", + "family-name": "Berners-Lee", + }, + expectedResult: { + name: "Timothy John Berners-Lee", + }, + }, + { + description: 'Does not have "name", and has some split names', + address: { + "given-name": "John", "family-name": "Doe", }, + expectedResult: { + name: "John Doe", + }, + }, + { + description: 'Does not have "name" and split names', + address: { + organization: "Mozilla", + }, + expectedResult: { + name: undefined, + organization: "Mozilla", + }, }, // Address diff --git a/browser/extensions/formautofill/test/unit/xpcshell.toml b/browser/extensions/formautofill/test/unit/xpcshell.toml index d4a6563eb87a..16eb261e3eaa 100644 --- a/browser/extensions/formautofill/test/unit/xpcshell.toml +++ b/browser/extensions/formautofill/test/unit/xpcshell.toml @@ -40,13 +40,8 @@ head = "head_addressComponent.js" ["test_addressDataLoader.js"] ["test_addressRecords.js"] -skip-if = ["apple_silicon"] # bug 1729554 ["test_autofillFormFields.js"] -skip-if = [ - "tsan", # Times out, bug 1612707 - "apple_silicon", # bug 1729554 -] ["test_clearPopulatedForm.js"] @@ -55,10 +50,6 @@ skip-if = [ ["test_createRecords.js"] ["test_creditCardRecords.js"] -skip-if = [ - "tsan", # Times out, bug 1612707 - "apple_silicon", # bug 1729554 -] ["test_extractLabelStrings.js"] @@ -77,10 +68,6 @@ skip-if = [ ["test_getInfo.js"] ["test_getRecords.js"] -skip-if = [ - "tsan", # Times out, bug 1612707 - "apple_silicon", # bug 1729554 -] ["test_isAddressAutofillAvailable.js"] @@ -113,26 +100,14 @@ skip-if = ["tsan"] # Times out, bug 1612707 ["test_profileAutocompleteResult.js"] ["test_reconcile.js"] -skip-if = [ - "tsan", # Times out, bug 1612707 - "apple_silicon", # bug 1729554 -] ["test_savedFieldNames.js"] ["test_storage_remove.js"] -skip-if = [ - "tsan", # Times out, bug 1612707 - "apple_silicon", # bug 1729554 -] ["test_storage_syncfields.js"] ["test_storage_tombstones.js"] -skip-if = [ - "tsan", # Times out, bug 1612707 - "apple_silicon", # bug 1729554 -] ["test_sync.js"] head = "head.js ../../../../../services/sync/tests/unit/head_appinfo.js ../../../../../services/common/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_http_server.js" @@ -140,15 +115,10 @@ skip-if = ["tsan"] # Times out, bug 1612707 ["test_sync_deprecate_credit_card_v4.js"] head = "head.js ../../../../../services/sync/tests/unit/head_appinfo.js ../../../../../services/common/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_http_server.js" -skip-if = [ - "tsan", # Times out, bug 1612707 - "apple_silicon", # bug 1729554 -] + +["test_sync_deprecate_address_x_name_fields.js"] +head = "head.js ../../../../../services/sync/tests/unit/head_appinfo.js ../../../../../services/common/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_http_server.js" ["test_toOneLineAddress.js"] ["test_transformFields.js"] -skip-if = [ - "tsan", # Times out, bug 1612707 - "apple_silicon", # bug 1729554 -] diff --git a/toolkit/components/formautofill/FormAutofillStorageBase.sys.mjs b/toolkit/components/formautofill/FormAutofillStorageBase.sys.mjs index 8963852dbe6f..591bfc1578aa 100644 --- a/toolkit/components/formautofill/FormAutofillStorageBase.sys.mjs +++ b/toolkit/components/formautofill/FormAutofillStorageBase.sys.mjs @@ -20,6 +20,7 @@ * given-name, * additional-name, * family-name, + * name, * organization, // Company * street-address, // (Multiline) * address-level3, // Suburb/Sublocality @@ -32,7 +33,9 @@ * * // computed fields (These fields are computed based on the above fields * // and are not allowed to be modified directly.) - * name, + * given-name, + * additional-name, + * family-name, * address-line1, * address-line2, * address-line3, @@ -163,20 +166,7 @@ export const ADDRESS_SCHEMA_VERSION = 1; // Please talk to the sync team before changing this! export const CREDIT_CARD_SCHEMA_VERSION = 3; -const VALID_ADDRESS_FIELDS = [ - "given-name", - "additional-name", - "family-name", - "organization", - "street-address", - "address-level3", - "address-level2", - "address-level1", - "postal-code", - "country", - "tel", - "email", -]; +const NAME_COMPONENTS = ["given-name", "additional-name", "family-name"]; const STREET_ADDRESS_COMPONENTS = [ "address-line1", @@ -193,10 +183,25 @@ const TEL_COMPONENTS = [ "tel-local-suffix", ]; -const VALID_ADDRESS_COMPUTED_FIELDS = ["name", "country-name"].concat( - STREET_ADDRESS_COMPONENTS, - TEL_COMPONENTS -); +const VALID_ADDRESS_FIELDS = [ + "name", + "organization", + "street-address", + "address-level3", + "address-level2", + "address-level1", + "postal-code", + "country", + "tel", + "email", +]; + +const VALID_ADDRESS_COMPUTED_FIELDS = [ + "country-name", + ...NAME_COMPONENTS, + ...STREET_ADDRESS_COMPONENTS, + ...TEL_COMPONENTS, +]; const VALID_CREDIT_CARD_FIELDS = [ "billingAddressGUID", @@ -665,7 +670,13 @@ class AutofillRecords { // The record is cloned to avoid accidental modifications from outside. let clonedRecord = this._cloneAndCleanUp(recordFound); if (rawData) { - await this._stripComputedFields(clonedRecord); + // The *-name fields, previously listed in VALID_FIELDS, have been moved to + // COMPUTED_FIELDS. By default, the sync payload includes only those fields in VALID_FIELDS. + // Excluding *-name fields from the sync payload would prevent older devices from + // synchronizing with newer devices. To maintain backward compatibility, keep those deprecated + // ields in the payload, ensuring that older devices can still sync with newer devices. + const fieldsToKeep = NAME_COMPONENTS; + await this._stripComputedFields(clonedRecord, fieldsToKeep); } else { this._recordReadProcessor(clonedRecord); } @@ -692,7 +703,8 @@ class AutofillRecords { await Promise.all( clonedRecords.map(async record => { if (rawData) { - await this._stripComputedFields(record); + const fieldsToKeep = NAME_COMPONENTS; + await this._stripComputedFields(record, fieldsToKeep); } else { this._recordReadProcessor(record); } @@ -1367,7 +1379,7 @@ class AutofillRecords { record.version = 0; } - if (this._isMigrationNeeded(record.version)) { + if (this._isMigrationNeeded(record)) { hasChanges = true; record = await this._computeMigratedRecord(record); @@ -1441,8 +1453,8 @@ class AutofillRecords { ); } - _isMigrationNeeded(recordVersion) { - return recordVersion < this.version; + _isMigrationNeeded(record) { + return record.version < this.version; } /** @@ -1464,8 +1476,13 @@ class AutofillRecords { return record; } - async _stripComputedFields(record) { - this.VALID_COMPUTED_FIELDS.forEach(field => delete record[field]); + async _stripComputedFields(record, fieldsToKeep = []) { + for (const field of this.VALID_COMPUTED_FIELDS) { + if (fieldsToKeep.includes(field)) { + continue; + } + delete record[field]; + } } // An interface to be inherited. @@ -1496,6 +1513,9 @@ class AutofillRecords { * @throws */ _validateFields(record) {} + + // An interface to be inherited. + migrateRemoteRecord(remoteRecord) {} } export class AddressesBase extends AutofillRecords { @@ -1516,6 +1536,40 @@ export class AddressesBase extends AutofillRecords { } } + _isMigrationNeeded(record) { + if (super._isMigrationNeeded(record)) { + return true; + } + + if ( + !record.name && + (record["given-name"] || + record["additional-name"] || + record["family-name"]) + ) { + return true; + } + return false; + } + + async _computeMigratedRecord(address) { + // Bug 1836438 - `name` field was moved from computed fields to valid fields. + if ( + !address.name && + (address["given-name"] || + address["additional-name"] || + address["family-name"]) + ) { + address.name = lazy.FormAutofillNameUtils.joinNameParts({ + given: address["given-name"] ?? "", + middle: address["additional-name"] ?? "", + family: address["family-name"] ?? "", + }); + } + + return super._computeMigratedRecord(address); + } + async computeFields(address) { // NOTE: Remember to bump the schema version number if any of the existing // computing algorithm changes. (No need to bump when just adding new @@ -1530,14 +1584,12 @@ export class AddressesBase extends AutofillRecords { return hasNewComputedFields; } - // Compute name - if (!("name" in address)) { - let name = lazy.FormAutofillNameUtils.joinNameParts({ - given: address["given-name"], - middle: address["additional-name"], - family: address["family-name"], - }); - address.name = name; + // Compute split names + if (!("given-name" in address)) { + const nameParts = lazy.FormAutofillNameUtils.splitName(address.name); + address["given-name"] = nameParts.given; + address["additional-name"] = nameParts.middle; + address["family-name"] = nameParts.family; hasNewComputedFields = true; } @@ -1629,19 +1681,22 @@ export class AddressesBase extends AutofillRecords { } _normalizeNameFields(address) { - if (address.name) { - let nameParts = lazy.FormAutofillNameUtils.splitName(address.name); - if (!address["given-name"] && nameParts.given) { - address["given-name"] = nameParts.given; - } - if (!address["additional-name"] && nameParts.middle) { - address["additional-name"] = nameParts.middle; - } - if (!address["family-name"] && nameParts.family) { - address["family-name"] = nameParts.family; - } + if ( + !address.name && + (address["given-name"] || + address["additional-name"] || + address["family-name"]) + ) { + address.name = lazy.FormAutofillNameUtils.joinNameParts({ + given: address["given-name"] ?? "", + middle: address["additional-name"] ?? "", + family: address["family-name"] ?? "", + }); } - delete address.name; + + delete address["given-name"]; + delete address["additional-name"]; + delete address["family-name"]; } _normalizeAddressFields(address) { @@ -1711,6 +1766,57 @@ export class AddressesBase extends AutofillRecords { } TEL_COMPONENTS.forEach(c => delete address[c]); } + + /** + * Migrate the remote record to the expected format. + * + * @param {object} remoteRecord The remote record. + */ + migrateRemoteRecord(remoteRecord) { + // When a remote record lacks the `name` field but includes any `*-name` fields, we can + // assume that the record originates from an older device. This is because even if an older + // device pulls the `name` field from a newer record from the sync server, the `name` field, + // being a computed field for an older device, will always be stripped. + + // If the remote record comes from an older device, we compare the `*-name` fields in the + // remote record with those in the corresponding local record. If the values of the `*-name` + // fields differ, it indicates that the remote record has updated these fields. If the + // values are the same, we replace the name field of the remote record with the local + // name field to ensure the completeness of the name field when reconciling. + // + // Here is an example: + // Assume the local record is {"name": "Mr. John Doe"}. If an updated remote record + // has {"given-name": "John", "family-name": "Doe"}, we will NOT join the `*-name` fields + // and replace the local `name` field with "John Doe". This allows us to retain the complete + // name - "Mr. John Doe". + // However, if the updated remote record has {"given-name": "Jane", "family-name": "Poe"}, + // we will rebuild it and replace the local `name` field with "Jane Poe". + if ( + !("name" in remoteRecord) && + NAME_COMPONENTS.some(c => c in remoteRecord) + ) { + const localRecord = this._findByGUID(remoteRecord.guid); + if ( + localRecord && + NAME_COMPONENTS.every(c => remoteRecord[c] == localRecord[c]) + ) { + remoteRecord.name = localRecord.name; + } else { + remoteRecord.name = lazy.FormAutofillNameUtils.joinNameParts({ + given: remoteRecord["given-name"], + middle: remoteRecord["additional-name"], + family: remoteRecord["family-name"], + }); + } + } + + // To enable new devices to sync name field changes with older devices, we still + // include the computed *-name fields in the sync payload while uploading. + // This also means that the incoming remote record will also contain *-name fields. + // However, since the autofill storage does not expect remote records to contain + // computed fields while merging, we remove them from the remote record. + NAME_COMPONENTS.forEach(f => delete remoteRecord[f]); + } } export class CreditCardsBase extends AutofillRecords { @@ -1745,7 +1851,7 @@ export class CreditCardsBase extends AutofillRecords { // Compute split names if (!("cc-given-name" in creditCard)) { - let nameParts = lazy.FormAutofillNameUtils.splitName( + const nameParts = lazy.FormAutofillNameUtils.splitName( creditCard["cc-name"] ); creditCard["cc-given-name"] = nameParts.given; @@ -1777,10 +1883,10 @@ export class CreditCardsBase extends AutofillRecords { throw Components.Exception("", Cr.NS_ERROR_NOT_IMPLEMENTED); } - _isMigrationNeeded(recordVersion) { + _isMigrationNeeded(record) { return ( // version 4 is deprecated and is rolled back to version 3 - recordVersion == 4 || recordVersion < this.version + record.version == 4 || record.version < this.version ); } diff --git a/toolkit/components/formautofill/FormAutofillSync.sys.mjs b/toolkit/components/formautofill/FormAutofillSync.sys.mjs index d45c2c04caa6..ac7aefcd1ddb 100644 --- a/toolkit/components/formautofill/FormAutofillSync.sys.mjs +++ b/toolkit/components/formautofill/FormAutofillSync.sys.mjs @@ -114,6 +114,19 @@ FormAutofillStore.prototype = { return; } + // Records from the remote might come from an older device. To ensure that + // remote records from older devices can still sync with the local records, + // we migrate the remote records. This enables the merging of older records + // with newer records. + // + // Currently, this migration is only used for converting `*-name` fields to `name` fields. + // The migration process involves: + // 1. Generating a `name` field so we don't assume the `name` field is empty, thereby + // avoiding erasing its value. + // 2. Removing deprecated *-name fields from the remote record because the autofill storage + // does not expect to see those fields. + this.storage.migrateRemoteRecord(remoteRecord.entry); + if (await this.itemExists(remoteRecord.id)) { // We will never get a tombstone here, so we are updating a real record. await this._doUpdateRecord(remoteRecord); diff --git a/toolkit/components/formautofill/default/FormAutofillPrompter.sys.mjs b/toolkit/components/formautofill/default/FormAutofillPrompter.sys.mjs index 6cf17d1f0956..ecf787137e56 100644 --- a/toolkit/components/formautofill/default/FormAutofillPrompter.sys.mjs +++ b/toolkit/components/formautofill/default/FormAutofillPrompter.sys.mjs @@ -18,8 +18,6 @@ const lazy = {}; ChromeUtils.defineESModuleGetters(lazy, { CreditCard: "resource://gre/modules/CreditCard.sys.mjs", - FormAutofillNameUtils: - "resource://gre/modules/shared/FormAutofillNameUtils.sys.mjs", formAutofillStorage: "resource://autofill/FormAutofillStorage.sys.mjs", }); @@ -463,13 +461,6 @@ export class AddressSaveDoorhanger extends AutofillDoorhanger { #formatTextByAddressCategory(fieldName) { let data = []; switch (fieldName) { - case "name": - data = ["given-name", "additional-name", "family-name"].map(field => [ - field, - this.oldRecord[field], - this.newRecord[field], - ]); - break; case "street-address": data = [ [ @@ -488,6 +479,7 @@ export class AddressSaveDoorhanger extends AutofillDoorhanger { field => [field, this.oldRecord[field], this.newRecord[field]] ); break; + case "name": case "country": case "tel": case "email": @@ -684,17 +676,6 @@ export class AddressEditDoorhanger extends AutofillDoorhanger { ); } - #getFieldDisplayData(field) { - if (field == "name") { - return lazy.FormAutofillNameUtils.joinNameParts({ - given: this.newRecord["given-name"], - middle: this.newRecord["additional-name"], - family: this.newRecord["family-name"], - }); - } - return this.newRecord[field]; - } - #buildCountryMenupopup() { const menupopup = this.doc.createXULElement("menupopup"); @@ -806,7 +787,7 @@ export class AddressEditDoorhanger extends AutofillDoorhanger { input.setAttribute("id", inputId); - const value = this.#getFieldDisplayData(fieldName) ?? null; + const value = this.newRecord[fieldName] ?? ""; if (popup) { const menuitem = Array.from(popup.childNodes).find( item => diff --git a/toolkit/components/formautofill/shared/AddressComponent.sys.mjs b/toolkit/components/formautofill/shared/AddressComponent.sys.mjs index ecab4f05baae..a849e889b24c 100644 --- a/toolkit/components/formautofill/shared/AddressComponent.sys.mjs +++ b/toolkit/components/formautofill/shared/AddressComponent.sys.mjs @@ -645,17 +645,7 @@ class Name extends AddressField { } static fromRecord(record, region) { - let name; - if (record.name) { - name = record.name; - } else { - name = lazy.FormAutofillNameUtils.joinNameParts({ - given: record["given-name"], - middle: record["additional-name"], - family: record["family-name"], - }); - } - return new Name(name, region); + return new Name(record[Name.ac], region); } } @@ -1025,37 +1015,35 @@ export class AddressComponent { this.record = {}; // Get country code first so we can use it to parse other fields - const country = Country.fromRecord(record, FormAutofill.DEFAULT_REGION); + const country = new Country( + record[Country.ac], + FormAutofill.DEFAULT_REGION + ); const region = country.country_code || lazy.FormAutofillUtils.identifyCountryCode(FormAutofill.DEFAULT_REGION); - let fields = [ + // Build an mapping that the key is field name and the value is the AddressField object + [ country, - StreetAddress.fromRecord(record, region), - PostalCode.fromRecord(record, region), - State.fromRecord(record, region), - City.fromRecord(record, region), - Name.fromRecord(record, region), - Tel.fromRecord(record, region), - Organization.fromRecord(record, region), - Email.fromRecord(record, region), - ]; - - for (const field of fields) { - if (field.isEmpty() || (ignoreInvalid && !field.isValid())) { - continue; + new StreetAddress(record[StreetAddress.ac], region), + new PostalCode(record[PostalCode.ac], region), + new State(record[State.ac], region), + new City(record[City.ac], region), + new Name(record[Name.ac], region), + new Tel(record[Tel.ac], region), + new Organization(record[Organization.ac], region), + new Email(record[Email.ac], region), + ].forEach(addressField => { + if ( + !addressField.isEmpty() && + (!ignoreInvalid || addressField.isValid()) + ) { + const fieldName = addressField.constructor.ac; + this.#fields[fieldName] = addressField; + this.record[fieldName] = record[fieldName]; } - this.#fields[field.constructor.ac] = field; - - if (field.constructor.ac == "name") { - this.record["given-name"] = record["given-name"] ?? ""; - this.record["additional-name"] = record["additional-name"] ?? ""; - this.record["family-name"] = record["family-name"] ?? ""; - } else { - this.record[field.constructor.ac] = record[field.constructor.ac]; - } - } + }); } /**