Closed Bug 1418884 Opened 2 years ago Closed 2 years ago

[Form Autofill] Make getAbbreviatedStateName supports more languages

Categories

(Toolkit :: Form Autofill, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: steveck, Assigned: steveck)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:V2])

Attachments

(1 file)

Since Canada has 2 official languages(en/fr), we should be able to get the abbreviated province name no matter the given full province name is in English or in French. We can leverage the metadata in libaddressinput and implement multilanguage province/state name mapping.
Comment on attachment 8932322 [details]
Bug 1418884 - [Form Autofill] Make getAbbreviatedSubregionName/findOption supports more locales.

https://reviewboard.mozilla.org/r/203348/#review209282

::: browser/extensions/formautofill/FormAutofillUtils.jsm:104
(Diff revision 1)
> +        return null;
> +      }
> +      let locales;
> +      if (countryData.languages && countryData.languages.split("~").length > 1) {
> +        let list = countryData.languages.split("~").filter(key => key !== countryData.lang);
> +        locales = list.map(key => this._addressData[`${defaultLocale.id}--${key}`]);

Probably out of scope of this bug, but I don't think this would work for countries that have language specific level1 IDs (ex. HK, IN).

::: browser/extensions/formautofill/FormAutofillUtils.jsm:301
(Diff revision 1)
>      return sandbox;
>    },
>  
>    // Get country address data and fallback to US if not found.
>    // See AddressDataLoader.getData for more details of addressData structure.
> -  getCountryAddressData(country = FormAutofillUtils.DEFAULT_COUNTRY_CODE, level1 = null) {
> +  getCountryAddressData({country = FormAutofillUtils.DEFAULT_COUNTRY_CODE,

nit: Use JS doc format

::: browser/extensions/formautofill/FormAutofillUtils.jsm:306
(Diff revision 1)
> -  getCountryAddressData(country = FormAutofillUtils.DEFAULT_COUNTRY_CODE, level1 = null) {
> +  getCountryAddressData({country = FormAutofillUtils.DEFAULT_COUNTRY_CODE,
> +                         level1 = null,
> +                         allLocales = false} = {}) {
>      let metadata = AddressDataLoader.getData(country, level1);
>      if (!metadata) {
>        metadata = level1 ? null : AddressDataLoader.getData("US");

Why couldn't it fallback to `US` if there's `level1` data?

::: browser/extensions/formautofill/FormAutofillUtils.jsm:438
(Diff revision 1)
>    getAbbreviatedStateName(stateValues, country) {
>      let values = Array.isArray(stateValues) ? stateValues : [stateValues];
>  
>      let collators = this.getCollators(country);
> -    let {sub_keys: subKeys, sub_names: subNames} = this.getCountryAddressData(country);
> +    for (let metadata of this.getCountryAddressData({country, allLocales: true})) {
> +      let {sub_keys: subKeys, sub_names: subNames} = metadata;

Again, probably out of scope, but `TW`, `JP`, and more countries use `sub_lnames` instead of `sub_names`.
Attachment #8932322 - Flags: review?(scwwu)
Comment on attachment 8932322 [details]
Bug 1418884 - [Form Autofill] Make getAbbreviatedSubregionName/findOption supports more locales.

https://reviewboard.mozilla.org/r/203348/#review209282

> Probably out of scope of this bug, but I don't think this would work for countries that have language specific level1 IDs (ex. HK, IN).

Yes it'll need another process to convert the level 1 name to corresponding locale for correct locale id. I think only HK will need this because it's the only regoin that has level 2 list and multi locale. I can file a bug for this.

> Why couldn't it fallback to `US` if there's `level1` data?

It should be implemented in bug 1417818 and this patch will be rebased on top of that

> Again, probably out of scope, but `TW`, `JP`, and more countries use `sub_lnames` instead of `sub_names`.

You're right and I think we can fix this in this patch since it's not complicated. Metadata with `sub_names` will not contain `sub_lnames` property and vise versa. So we can get the sub name either from `sub_names` or `sub_lnames`.
Comment on attachment 8932322 [details]
Bug 1418884 - [Form Autofill] Make getAbbreviatedSubregionName/findOption supports more locales.

https://reviewboard.mozilla.org/r/203348/#review210094

::: browser/extensions/formautofill/FormAutofillUtils.jsm:318
(Diff revision 4)
>     *        default region if parameter is not set.
>     * @param {string} [level1=null]
>     *        Retrun address level 1/level 2 metadata if parameter is set.
> -   * @returns {object}
> -   *          Return the metadata of specific region.
> -   */
> +   * @param {string} [allLocales=false]
> +   *        Return all locales metadata or default locale metadata.
> +   * @returns {object|Array<object>}

nit: `@returns {object|Array<object>|null}`

Could there be an explaination as to why we are returning `null` if `level1` is present and couldn't be found?
Attachment #8932322 - Flags: review?(scwwu) → review+
Comment on attachment 8932322 [details]
Bug 1418884 - [Form Autofill] Make getAbbreviatedSubregionName/findOption supports more locales.

https://reviewboard.mozilla.org/r/203348/#review210208

::: browser/extensions/formautofill/FormAutofillUtils.jsm:97
(Diff revision 4)
> -   * @returns {object}
> +   * @returns {object|null}
> +   *          Get default locale and other locales metadata as array if exists.
> +   *          Returns null if default metadata does not exist.
>     */
>    getData(country, level1 = null) {
> +    let getMetadataWithLocales = (ct, lv1) => {

I prefer to use another function (e.g. `AddressDataLoader.getDataWithLocales()`) to deal with locales because the locale data are not required everytime. Besides, we can make `getData` simpler by treating it as a getter of the raw metadata.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:106
(Diff revision 4)
> +        return null;
> +      }
> +      let locales;
> +      //TODO: Should be able to support multi-locale level 1/ level 2 metadata query
> +      //      in Bug 1421886
> +      if (countryData.languages && countryData.languages.split("~").length > 1) {

I feel we will use `languages.split("~")` frequently, How about parsing and caching it directly in `getData`? I mean

```js
countryData.languages = countryData.languages ? countryData.languages.split("~") : [];
```

::: browser/extensions/formautofill/FormAutofillUtils.jsm:340
(Diff revision 4)
>      // Fallback to US if we couldn't get data from default region.
> -    return metadata || AddressDataLoader.getData("US");
> +    if (!metadata) {
> +      metadata = AddressDataLoader.getData("US");
> +    }
> +    return allLocales ? [metadata.defaultLocale, ...metadata.locales || []] :
> +                        metadata.defaultLocale;

Returning different types (array v.s. object) makes this function inconsistant and easier to make mistakes during implementation. You can either always return an array or create another function such as `getCountryAddressDataWithLocales`.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:537
(Diff revision 4)
> +          ["sub_keys", "sub_names", "sub_lnames"].forEach(key => {
> +            if (dataset[key] && !Array.isArray(dataset[key])) {
> +              dataset[key] = dataset[key].split("~");
> +            }
> +          });

There are two functions (`findAddressSelectOption` and `getAbbreviatedSubregionName`) that need to parse (and cache) these subregion data. I think we sould do it in the same place. For example, add a parameter (e.g. `{parseSubregionData: true}`) to `getCountryAddressData()`.
Attachment #8932322 - Flags: review?(lchang)
Comment on attachment 8932322 [details]
Bug 1418884 - [Form Autofill] Make getAbbreviatedSubregionName/findOption supports more locales.

https://reviewboard.mozilla.org/r/203348/#review210208

> I feel we will use `languages.split("~")` frequently, How about parsing and caching it directly in `getData`? I mean
> 
> ```js
> countryData.languages = countryData.languages ? countryData.languages.split("~") : [];
> ```

I integrate all the properties that will need to be parsed into `normalize` and it will be called right after fetching the data and cached.

> Returning different types (array v.s. object) makes this function inconsistant and easier to make mistakes during implementation. You can either always return an array or create another function such as `getCountryAddressDataWithLocales`.

Will separate into `getCountryAddressDataWithLocales`

> There are two functions (`findAddressSelectOption` and `getAbbreviatedSubregionName`) that need to parse (and cache) these subregion data. I think we sould do it in the same place. For example, add a parameter (e.g. `{parseSubregionData: true}`) to `getCountryAddressData()`.

Like I said it will be moved to `normalize`
Comment on attachment 8932322 [details]
Bug 1418884 - [Form Autofill] Make getAbbreviatedSubregionName/findOption supports more locales.

https://reviewboard.mozilla.org/r/203348/#review210876

Looks good. Clear r? because I'd like to take a look again.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:81
(Diff revision 5)
>        }
>      }
>      return sandbox;
>    },
> +
> +  _normalize(data) {

nit: maybe `_parse` would be more appropriate.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:114
(Diff revision 5)
>     *              }
>     * @param   {string} country
>     * @param   {string} level1
> -   * @returns {object}
> +   * @returns {object} Default locale metadata
>     */
>    getData(country, level1 = null) {

As we discussed, let's rename it to `_loadData`.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:134
(Diff revision 5)
> +   * We'll cache addressData in the loader once the data loaded from scripts.
> +   * It'll become the example below after loading addressReferences with extension:

This comment needs an update.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:140
(Diff revision 5)
> +   * It'll become the example below after loading addressReferences with extension:
> +   * @param   {string} country
> +   * @param   {string} level1
> +   * @returns {object} Default locale metadata
> +   */
> +  getDataWithLocales(country, level1) {

As we discussed, let this function replace `getData`.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:347
(Diff revision 5)
> +  getCountryAddressRawData({country = FormAutofillUtils.DEFAULT_REGION,
> +                            level1 = null} = {}) {

nit: I think the parameters can be changed back to the former format now.

::: browser/extensions/formautofill/test/mochitest/test_multi_locale_CA_address_form.html:96
(Diff revision 5)
> +  doKey("return");
> +  return Promise.all(promises);
> +}
> +
> +async function setupAddressStorage() {
> +  for (let storage of MOCK_STORAGE) {

nit: `for (let address of MOCK_STORAGE) {`

::: browser/extensions/formautofill/test/mochitest/test_multi_locale_CA_address_form.html:129
(Diff revision 5)
> +  document.querySelector("#form-en").reset();
> +  document.querySelector("#form-fr").reset();
> +});
> +
> +// Autofill the address with address level 1 full name.
> +add_task(async function check_fields_after_form_autofill() {

Would be better that the function name reflects the test case. So does the previous one.
Attachment #8932322 - Flags: review?(lchang)
Comment on attachment 8932322 [details]
Bug 1418884 - [Form Autofill] Make getAbbreviatedSubregionName/findOption supports more locales.

https://reviewboard.mozilla.org/r/203348/#review211090

Thanks.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:80
(Diff revision 7)
> +  // Convert certain properties' string value into array. We should make sure
> +  // the cached data is parsed.

nit: use jsdoc format.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:100
(Diff revision 7)
> +  },
>    /**

nit: For consistency, adding one additional blank line between `}` and `/**` would be better. So does the other methods around.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:135
(Diff revision 7)
>      }
> -    return this._addressData[`data/${country}/${level1}`];
> +    return this._parse(this._addressData[`data/${country}/${level1}`]);
> +  },
> +
> +  /**
> +   * Return the region metadata with default locale and other locales(if exists).

nit: One space between `locales` and `(if exists)`.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:154
(Diff revision 7)
> +    //      in Bug 1421886
> +    if (countryData.languages) {
> +      let list = countryData.languages.filter(key => key !== countryData.lang);
> +      locales = list.map(key => this._parse(this._addressData[`${defaultLocale.id}--${key}`]));
> +    }
> +    return {defaultLocale, locales};

How about returning an empty array when `locales` doesn't exist? Thus, we don't need to check its existence before using it outside (e.g. `getCountryAddressDataWithLocales`).

::: browser/extensions/formautofill/FormAutofillUtils.jsm:347
(Diff revision 7)
> -   * @returns {object}
> -   *          Return the metadata of specific region.
> +   * @returns {object|null}
> +   *          Return metadata of specific region with default locale and other supported
> +   *          locales. We need to return a deafult country metadata for layout format
> +   *          and collator, but for sub-region metadata we'll just return null if not found.
>     */
> -  getCountryAddressData(country = FormAutofillUtils.DEFAULT_REGION, level1 = null) {
> +  getCountryAddressRawData(country = FormAutofillUtils.DEFAULT_REGION, level1 = null) {

As discussed today, we need to revisit this function and review whether or not fallback is still needed. This shouldn't block this bug so let's file another one for tracking. Thanks.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:366
(Diff revision 7)
> +      metadata = AddressDataLoader.getData("US");
> +    }
> +    return metadata;
> +  },
> +
> +  getCountryAddressData(country, level1) {

Add a jsdoc comment.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:371
(Diff revision 7)
> +  getCountryAddressData(country, level1) {
> +    let metadata = this.getCountryAddressRawData(country, level1);
> +    return metadata && metadata.defaultLocale;
> +  },
> +
> +  getCountryAddressDataWithLocales(country, level1) {

Add a jsdoc comment.

::: browser/extensions/formautofill/test/unit/test_addressDataLoader.js:51
(Diff revision 7)
>    sinon.assert.notCalled(AddressDataLoader._loadScripts);
>    Assert.deepEqual(metadata, newMetadata, "metadata should be US if country is not specified");
>    AddressDataLoader._loadScripts.reset();
>  
>    // Load level 1 data that does not exist
> -  let undefinedMetadata = FormAutofillUtils.getCountryAddressData("US", "CA");
> +  let undefinedMetadata = FormAutofillUtils.getCountryAddressData("US","CA");

one space after the comma.

::: browser/extensions/formautofill/test/unit/test_addressDataLoader.js:60
(Diff revision 7)
>    Assert.ok(AddressDataLoader._dataLoaded.level1.has("US"),
>                 "level 1 state array should be set even there's no valid metadata");
>    AddressDataLoader._loadScripts.reset();
>  
>    // Load level 1 data again
> -  undefinedMetadata = FormAutofillUtils.getCountryAddressData("US", "AS");
> +  undefinedMetadata = FormAutofillUtils.getCountryAddressData("US","AS");

one space after the comma.
Attachment #8932322 - Flags: review?(lchang) → review+
Thanks for the review. I triggered a try run but I think it should pass since there's no further issue in the local test.
Keywords: checkin-needed
Pushed by toros@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ae2074a9f48
[Form Autofill] Make getAbbreviatedSubregionName/findOption supports more locales. r=lchang,scottwu
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6ae2074a9f48
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.