Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fine-grained errors reporting for PaymentAddress #712

Merged
merged 2 commits into from
Jun 6, 2018

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented May 16, 2018

closes #647

The following tasks have been completed:

Implementation commitment:

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

Impact on Payment Handler spec?


Preview | Diff

@marcoscaceres marcoscaceres changed the title WIP: fine grained error recovery fine grained error reporting May 17, 2018
@marcoscaceres marcoscaceres changed the title fine grained error reporting Part 1: fine grained error reporting for shipping addresses May 17, 2018
@marcoscaceres marcoscaceres changed the title Part 1: fine grained error reporting for shipping addresses Part 1: fine grained error for addresses May 17, 2018
@marcoscaceres
Copy link
Member Author

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.

index.html Outdated
Secondly, the string value allows the developer to describe the
validation error (and possibly how the end user can fix the error).
</p>
<p class="note">
Copy link
Member Author

@marcoscaceres marcoscaceres May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified what "this" was above.

</h2>
<pre class="idl">
dictionary AddressErrorFields {
DOMString addressLineError;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@msujaws msujaws May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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 marcoscaceres changed the title Part 1: fine grained error for addresses fine-grained error for addresses May 21, 2018
@marcoscaceres marcoscaceres changed the title fine-grained error for addresses fine-grained errors reporting for PaymentAddress May 21, 2018
@marcoscaceres
Copy link
Member Author

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

Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</h2>
<pre class="idl">
dictionary AddressErrorFields {
DOMString addressLineError;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

index.html Outdated
<p>
The members of the <a>AddressErrorFields</a> dictionary represent
validation errors with specific parts of a <a>physical address</a>.
Each dictionary member has a dual function: firstly, it denotes that
Copy link
Collaborator

@domenic domenic Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Empty shippingOptions implies that we can't ship
// to this address.
const shippingOptions = [];
return { error, shippingAddressErrors, shippingOptions };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 marcoscaceres merged commit 10dc96e into gh-pages Jun 6, 2018
@marcoscaceres marcoscaceres deleted the error_recovery branch June 6, 2018 01:15
@marcoscaceres
Copy link
Member Author

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

@ianbjacobs
Copy link
Collaborator

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

@marcoscaceres
Copy link
Member Author

Added tests web-platform-tests/wpt#12396

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fine grained error recovery for fields
4 participants