Closed Bug 1411509 Opened 7 years ago Closed 7 years ago

[Form Autofill] Create a parser that processes the fmt attribute and returns which address fields are visible

Categories

(Toolkit :: Form Manager, enhancement, P1)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: scottwu, Assigned: scottwu)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [form autofill:V2])

Attachments

(1 file)

Bug 1411509 - Add the country address format parser.
59 bytes, text/x-review-board-request
lchang
: review+
steveck
: review+
Details 
A parser for the `fmt` attribute in libaddressinput should tell us which address fields are used for a given country.
Attachment #8923701 - Flags: review?(schung)
Comment on attachment 8923701 [details]
Bug 1411509 - Add the country address format parser.

https://reviewboard.mozilla.org/r/194838/#review200886

Overall looks fine. I just not sure about the additional tailing new line.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:246
(Diff revision 1)
>    /**
> +   * Parse a country address format string and outputs an array of fields
> +   * @param  {string} fmt Country address format string
> +   * @return {array<object>} List of fields
> +   */
> +  parseAddressFormat(fmt) {

nit: Early return or throw error if fmt is undefined(and having a test for it).

::: browser/extensions/formautofill/FormAutofillUtils.jsm:247
(Diff revision 1)
> +   * Parse a country address format string and outputs an array of fields
> +   * @param  {string} fmt Country address format string
> +   * @return {array<object>} List of fields
> +   */
> +  parseAddressFormat(fmt) {
> +    const fieldsLookup = {

Based on the description in https://github.com/googlei18n/libaddressinput/wiki/AddressValidationMetadata (maybe we should add a comment with this page for more detail about the format), Do you think we should add the full list at the begining? Like sorting code and dependent locality.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:256
(Diff revision 1)
> +      S: "addressLevel1",
> +      C: "addressLevel2",
> +      Z: "postalCode",
> +      n: "newLine",
> +    };
> +    // Add a new line symbol at the end for easier formatting

I'm not sure how it could make the formatting easier, and it'll add the new line attribute to the last object. Will it help you to design the dialog layout?
Attachment #8923701 - Flags: review?(schung)
Comment on attachment 8923701 [details]
Bug 1411509 - Add the country address format parser.

https://reviewboard.mozilla.org/r/194838/#review201338

::: browser/extensions/formautofill/FormAutofillUtils.jsm:246
(Diff revision 1)
>    /**
> +   * Parse a country address format string and outputs an array of fields
> +   * @param  {string} fmt Country address format string
> +   * @return {array<object>} List of fields
> +   */
> +  parseAddressFormat(fmt) {

Got it.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:247
(Diff revision 1)
> +   * Parse a country address format string and outputs an array of fields
> +   * @param  {string} fmt Country address format string
> +   * @return {array<object>} List of fields
> +   */
> +  parseAddressFormat(fmt) {
> +    const fieldsLookup = {

I'll add the comment in. We could add sorting code and dependent locality except I don't know what autocomplete attribute they belong to yet.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:256
(Diff revision 1)
> +      S: "addressLevel1",
> +      C: "addressLevel2",
> +      Z: "postalCode",
> +      n: "newLine",
> +    };
> +    // Add a new line symbol at the end for easier formatting

It helps with arranging the layout, but on second thought, I could just do that when it's needed. I'll remove this part.
Comment on attachment 8923701 [details]
Bug 1411509 - Add the country address format parser.

https://reviewboard.mozilla.org/r/194838/#review201724

::: browser/extensions/formautofill/FormAutofillUtils.jsm:264
(Diff revision 2)
> +      n: "newLine",
> +    };
> +
> +    return fmt.split("%").reduce((parsed, part) => {
> +      // Take the first letter of each segment and try to identify it
> +      let fieldId = fieldsLookup[part[0]];

I found that every split array will have empty array as the first index. Although checking the array with undefined index won't bust the function, maybe we could still early return `if (!part)`?
Attachment #8923701 - Flags: review?(schung) → review+
Comment on attachment 8923701 [details]
Bug 1411509 - Add the country address format parser.

https://reviewboard.mozilla.org/r/194838/#review201730

Looks fine, only one small nit from my side.
Comment on attachment 8923701 [details]
Bug 1411509 - Add the country address format parser.

https://reviewboard.mozilla.org/r/194838/#review201724

Thanks for the review!

> I found that every split array will have empty array as the first index. Although checking the array with undefined index won't bust the function, maybe we could still early return `if (!part)`?

Ok, I'll add the early return check.
Comment on attachment 8923701 [details]
Bug 1411509 - Add the country address format parser.

https://reviewboard.mozilla.org/r/194838/#review201812

::: browser/extensions/formautofill/FormAutofillUtils.jsm:242
(Diff revision 3)
> +   * Parse a country address format string and outputs an array of fields
> +   * @param   {string} fmt Country address format string
> +   * @returns {array<object>} List of fields

It would be clearer to have an example of the input and output so we can expect how it works without checking tests.

BTW, if we are not going to maintain the format of "separators" (spaces, commas, etc) for now, maybe one more comment would be better.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:256
(Diff revision 3)
> +      S: "addressLevel1",
> +      C: "addressLevel2",
> +      Z: "postalCode",

Not sure how they will be used but I'm thinking that making the IDs align with the storage (i.e. address-level1, postal-code) might be useful in the future. What do you think?

::: browser/extensions/formautofill/FormAutofillUtils.jsm:262
(Diff revision 3)
> +    return fmt.split("%").reduce((parsed, part) => {
> +      if (!part) {
> +        return parsed;
> +      }

If we want to skip the first empty token, how about `s.match(/%[^%]+/g)`?
Attachment #8923701 - Flags: review?(lchang) → review+
Comment on attachment 8923701 [details]
Bug 1411509 - Add the country address format parser.

https://reviewboard.mozilla.org/r/194838/#review201812

> It would be clearer to have an example of the input and output so we can expect how it works without checking tests.
> 
> BTW, if we are not going to maintain the format of "separators" (spaces, commas, etc) for now, maybe one more comment would be better.

Ok. I've added more comments.

> Not sure how they will be used but I'm thinking that making the IDs align with the storage (i.e. address-level1, postal-code) might be useful in the future. What do you think?

Aligning them with the IDs will help when I need to filter what fields should be saved (or which fields shouldn't be save even if they have value). I'll make this change. Thanks for the suggestion.

> If we want to skip the first empty token, how about `s.match(/%[^%]+/g)`?

Maybe `s.match(/%[^%.]/g)` would do the same trick, since I only need the first character?
Thanks Steve and Luke for reviewing. Checking this in.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7b905cce83cd
Add the country address format parser. r=lchang,steveck
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7b905cce83cd
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: