Closed Bug 1417818 Opened 2 years ago Closed 2 years ago

[Form Autofill] Add CA/DE address metadata in addressReferences

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)

No description provided.
We'll need to copy the address metadata from libaddressinput[1] into our addressReferences.js to support CA/DE

[1] https://chromium-i18n.appspot.com/ssl-address
Priority: P1 → P3
Status: NEW → ASSIGNED
Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review207692

::: browser/app/profile/firefox.js:1714
(Diff revision 1)
>  pref("extensions.formautofill.creditCards.used", 0);
>  pref("extensions.formautofill.firstTimeUse", true);
>  pref("extensions.formautofill.heuristics.enabled", true);
>  pref("extensions.formautofill.loglevel", "Warn");
>  // Comma separated list of countries Form Autofill supports
> -pref("extensions.formautofill.supportedCountries", "US");
> +pref("extensions.formautofill.supportedCountries", "US,CA,DE");

Although we could not handle the fr province name right now, we could still support the province name in en for CA after this patch(I tried it on amazon.ca and it works). So I think we could still claim that we support CA address now, wdty?

::: browser/extensions/formautofill/addressmetadata/addressReferencesExt.js:16
(Diff revision 1)
>  //  contains the information we need but absent in "libaddressinput" such as alternative names.
>  
>  // TODO: We only support the alternative name of US in MVP. We are going to support more countries in
>  //       bug 1370193.
>  var addressDataExt = {
> +  "data/DE": {languages: "de"},

I add this mainly for Collator, but it seems work if we don't provide the locale for Intl.Collator API(locale is optional). We might need more testing for de locale to see if it works fine for all the de locales(For example, de-at => “Austrian German" and so on).
BTW I think maybe we could discuss about the DEFAULT_COUNTRY_CODE in the FormAutofillUtils. It was applied in getAbbreviatedStateName and findAddressSelectOption if country is undefined, but integrate this logic in getCountryAddressData might be an option instead of returning US metadata if not found. Furthermore, maybe we could even return US as DEFAULT_COUNTRY_CODE if "browser.search.countryCode" is not in the supported list.
Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review207692

> Although we could not handle the fr province name right now, we could still support the province name in en for CA after this patch(I tried it on amazon.ca and it works). So I think we could still claim that we support CA address now, wdty?

I think we can have `CA,US` now and add `DE` to the list in Bug 1383687, when fields could be rearranged and hidden.

> I add this mainly for Collator, but it seems work if we don't provide the locale for Intl.Collator API(locale is optional). We might need more testing for de locale to see if it works fine for all the de locales(For example, de-at => “Austrian German" and so on).

Or can we use country code directly? Maybe I over-complicated things when implementing `getCollators`. I think we can leave things as it is until we actually need it.
Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review207692

> I think we can have `CA,US` now and add `DE` to the list in Bug 1383687, when fields could be rearranged and hidden.

Ok and I added TODO comment about this.

> Or can we use country code directly? Maybe I over-complicated things when implementing `getCollators`. I think we can leave things as it is until we actually need it.

Agree. I'll remove the lang de for now and we can add it back once we really need it.
Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review207998

::: browser/extensions/formautofill/test/unit/test_transformFields.js:389
(Diff revision 2)
>      },
>    },
>    {
>      description: "Has unsupported \"country\"",
>      address: {
> -      "country": "CA",
> +      "country": "TV",

In the future `TV` will become supported. Could we use a fictional country? or just `AA` like above?
Attachment #8931219 - Flags: review?(scwwu) → review+
Duplicate of this bug: 1413494
Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review207998

> In the future `TV` will become supported. Could we use a fictional country? or just `AA` like above?

This test intentionally focuses on countries that are not supported yet because we need to make sure that the unsupported country code (but is actually stored in the storage) won't be exposed.
Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review208380

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

As discussed, let's implement 2-level fallback mechanism.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:411
(Diff revision 2)
>    },
>  
>    /**
>     * Try to find the abbreviation of the given state name
>     * @param   {string[]} stateValues A list of inferable state values.
>     * @param   {string} country A country name to be identified.

Use `[country]` to indicate that it's optional.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:414
(Diff revision 2)
>     * Try to find the abbreviation of the given state name
>     * @param   {string[]} stateValues A list of inferable state values.
>     * @param   {string} country A country name to be identified.
>     * @returns {string} The matching state abbreviation.
>     */
> -  getAbbreviatedStateName(stateValues, country = this.DEFAULT_COUNTRY_CODE) {
> +  getAbbreviatedStateName(stateValues, country) {

Will it be used for countries other than US? If so, the function name and the parameter name may need a change. If not, the parameter, `country`, seems unnecessary.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:464
(Diff revision 2)
> -    let country = address.country || this.DEFAULT_COUNTRY_CODE;
> +    let country = address.country;
>      let dataset = this.getCountryAddressData(country);
>      let collators = this.getCollators(country);

nit: Since `country` is identical to `address.country` now, I prefer to use the latter directly.

```js
let dataset = this.getCountryAddressData(address.country);
let collators = this.getCollators(address.country);
```

::: browser/extensions/formautofill/FormAutofillUtils.jsm:663
(Diff revision 2)
>        element.removeAttribute("data-localization");
>      }
>    },
>  };
>  
>  XPCOMUtils.defineLazyGetter(this.FormAutofillUtils, "DEFAULT_COUNTRY_CODE", () => {

nit: `DEFAULT_REGION`
Attachment #8931219 - Flags: review?(lchang)
Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review209698

Thanks.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:283
(Diff revision 4)
>    // Get country address data and fallback to US if not found.
>    // See AddressDataLoader.getData for more details of addressData structure.

nit: Use jsdoc format.
Attachment #8931219 - Flags: review?(lchang) → review+
Thanks for the review!
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6b54a120c2b7
Add CA/DE metadata from libaddressinput. r=lchang,scottwu
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6b54a120c2b7
Status: ASSIGNED → 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.