fine-grained errors reporting for PaymentAddress

[marcoscaceres]

closes #647

The following tasks have been completed:

• Confirmed there are no ReSpec errors/warnings.
• Added Web platform tests
• added MDN Docs (link)

Implementation commitment:

• Safari - already implemented.
• Chrome (link to issue)
• Firefox
• Edge (public signal)

Impact on Payment Handler spec?

---

Preview | Diff

[marcoscaceres]

First draft ready for review. This part only deals with errors related to shipping addresses.

Will start working on #705, which will then give us the ability to correct both address-releated errors and Payer Errors (e.g., bad email) in the PaymentResponse.

[marcoscaceres]

Kept this (languageCodeError) for completeness... but wonder if we should just ditch languageCodeError. 🤔

[marcoscaceres]

Clarified what "this" was above.

[marcoscaceres]

Decided to postfix all these with "Error" in the name, as I felt it made it a little bit more clear when in userland code.

[msujaws]

The extra "Error" suffix feels redundant me, as it should be implied since these are members of the shippingAddressErrors property.

[marcoscaceres]

depends... the problem I ran into was in destructuring and using object short hands. Also, consider the shippingAddressErrors can be the result of another function. Thus:

function validateAddress(address){
   // whoops, now "postalCode" has been squatted. 
   const  { postalCode } = address;
   const postalCodeError =  !isValidPostCode(postCode) ? "Invalid zip code" : undefined;
   return { postalCodeError }
}

function collectErrors(response) {
   // same problem here!
   const { shippingAddress } = response;
   const  shippingAddressErrors = validateAddress(shippingAddress);
   return { shippingAddressErrors }
}

And so on...

[msujaws]

Fair enough :)

[domenic]

I tend to agree this is redundant. I think you have chosen a code style with a combination of object literal shorthand, destructuring, and redundant variable declarations that makes it extra confusing. But I don't think that's a good reason to verbose-ify the API, personally. I would instead suggest writing your code in a more conventional way, such as

function validateAddress(address){
   return {
    postalCode: !isValidPostalCode(address.postalCode) ? "Invalid zip code" : undefined
  };
}

function collectErrors(response) {
   return {
    shippingAddress: validateAddress(response.shippingAddress)
  };
}

(basically, don't declare variables you're only going to use once.

[marcoscaceres]

We have a partial implementation of this in Firefox now (currently based on the original proposal - but will get updated to match the new one soon).

[marcoscaceres]

@domenic, when you have a few mins, would appreciate a final review. We've landed this in Firefox.

[domenic]

Mostly LGTM, although I agree with others who are concerned about the redundancy. Will leave it to you to make the call though.

[domenic]

I tend to agree this is redundant. I think you have chosen a code style with a combination of object literal shorthand, destructuring, and redundant variable declarations that makes it extra confusing. But I don't think that's a good reason to verbose-ify the API, personally. I would instead suggest writing your code in a more conventional way, such as

function validateAddress(address){
   return {
    postalCode: !isValidPostalCode(address.postalCode) ? "Invalid zip code" : undefined
  };
}

function collectErrors(response) {
   return {
    shippingAddress: validateAddress(response.shippingAddress)
  };
}

(basically, don't declare variables you're only going to use once.

[domenic]

I'd say "firstly, its presence denotes..."

[domenic]

Empty shippingOptions should not be necessary, since not-present gets converted to empty in the processing model, I am pretty sure.

[marcoscaceres]

In the "update a PaymentRequest's details algorithm", we do an incremental update to request, so:

If the shippingOptions member of details is present, and request.[[options]].requestShipping is true, then:

1. Set request.[[details]].shippingOptions to shippingOptions.

Otherwise, the original shipping options are retained - so the developer needs to be explicitly thrash them.

[marcoscaceres]

@ianbjacobs, could you please republish the document on TR?

[ianbjacobs]

Hi @marcoscaceres, I have send a publication request for 7 June. Thanks for all the editing!

[marcoscaceres]

Added tests web-platform-tests/wpt#12396