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.

Sign up for GitHub

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

Jump to bottom

fine-grained errors reporting for PaymentAddress #712

Merged
merged 2 commits into from
Jun 6, 2018
Merged

fine-grained errors reporting for PaymentAddress #712

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3350316
fine-grained error reporting for PaymentAddress
marcoscaceres May 16, 2018
a9aee3f
review feedback
marcoscaceres Jun 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
237 changes: 221 additions & 16 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,39 @@ <h3>
}
</pre>
</section>
<section>
<h3>
Fine-grained error reporting
</h3>
<p>
A developer can use the <a data-link-for=
"PaymentDetailsUpdate">shippingAddressErrors</a> member of the
<a>PaymentDetailsUpdate</a> dictionary to indicate that there are
validation errors with specific attributes of a
<a>PaymentAddress</a>. The <a data-link-for=
"PaymentDetailsUpdate">shippingAddressErrors</a> member is a
<a>AddressErrorFields</a> dictionary, whose members specifically
demarcate the fields of a <a>physical address</a> that are erroneous
while also providing helpful error messages to be displayed to the
end user.
</p>
<pre class="example">
request.onshippingaddresschange = ev =&gt; {
ev.updateWith(validateAddress(request.shippingAddress));
};
function validateAddress(shippingAddress) {
const error = "Can't ship to this address.";
const shippingAddressErrors = {
cityError: "FarmVille is not a real place.",
postalCodeError: "Unknown postal code for your country.",
};
// 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.

}
</pre>
</section>
<section data-link-for="PaymentResponse">
<h3>
POSTing payment response back to a server
Expand Down Expand Up @@ -1713,6 +1746,7 @@ <h2>
dictionary PaymentDetailsUpdate : PaymentDetailsBase {
DOMString error;
PaymentItem total;
AddressErrorFields shippingAddressErrors;
};
</pre>
<p>
Expand Down Expand Up @@ -1756,6 +1790,14 @@ <h2>
"PaymentCurrencyAmount.value">value</a> is a negative number.
</p>
</dd>
<dt>
<dfn>shippingAddressErrors</dfn> member
</dt>
<dd>
Represents validation errors with the shipping address that is
associated with the <a data-cite="!DOM#event-target">event
target</a>.
</dd>
</dl>
</section>
</section>
Expand Down Expand Up @@ -2040,7 +2082,7 @@ <h2>
Physical addresses
</h2>
<p>
A <dfn>physical address</dfn> is composed of the following concepts.
A <dfn>physical address</dfn> is composed of the following parts.
</p>
<dl data-sort="">
<dt>
Expand Down Expand Up @@ -2704,6 +2746,153 @@ <h2>
</dd>
</dl>
</section>
<section data-dfn-for="AddressErrorFields" data-link-for=
"AddressErrorFields">
<h2>
<dfn>AddressErrorFields</dfn> dictionary
</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 •
edited
Loading

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.

DOMString cityError;
DOMString countryError;
DOMString dependentLocalityError;
DOMString languageCodeError;
DOMString organizationError;
DOMString phoneError;
DOMString postalCodeError;
DOMString recipientError;
DOMString regionError;
DOMString regionCodeError;
DOMString sortingCodeError;
};
</pre>
<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 •
edited
Loading

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

a particular part of an address is suffering from a validation error.
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 •
edited
Loading

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.

Developers need to be aware that users might not have the ability to
fix certain parts of an address. As such, they need to be mindful to
not to ask the user to fix things they might not have control over
(e.g., <a>languageCodeError</a>).
</p>
<dl>
<dt>
<dfn>addressLineError</dfn> member
</dt>
<dd>
Denotes that the <a>address line</a> has a validation error. In the
user agent's UI, this member corresponds to the input field that
provided the <a>PaymentAddress</a>'s <a data-link-for=
"PaymentAddress">addressLine</a> attribute's value.
</dd>
<dt>
<dfn>cityError</dfn> member
</dt>
<dd>
Denotes that the <a>city</a> has a validation error. In the user
agent's UI, this member corresponds to the input field that
provided the <a>PaymentAddress</a>'s <a data-link-for=
"PaymentAddress">city</a> attribute's value.
</dd>
<dt>
<dfn>countryError</dfn> member
</dt>
<dd>
Denotes that the <a>country</a> has a validation error. In the user
agent's UI, this member corresponds to the input field that
provided the <a>PaymentAddress</a>'s <a data-link-for=
"PaymentAddress">country</a> attribute's value.
</dd>
<dt>
<dfn>dependentLocalityError</dfn> member
</dt>
<dd>
Denotes that the <a>dependent locality</a> has a validation error.
In the user agent's UI, this member corresponds to the input field
that provided the <a>PaymentAddress</a>'s <a data-link-for=
"PaymentAddress">dependentLocality</a> attribute's value.
</dd>
<dt>
<dfn>languageCodeError</dfn> member
</dt>
<dd>
Denotes that the <a>language code</a> has a validation error. In
the user agent's UI, this member corresponds to the input field
that provided the <a>PaymentAddress</a>'s <a data-link-for=
"PaymentAddress">languageCode</a> attribute's value.
</dd>
<dt>
<dfn>organizationError</dfn> member
</dt>
<dd>
Denotes that the <a>organization</a> has a validation error. In the
user agent's UI, this member corresponds to the input field that
provided the <a>PaymentAddress</a>'s <a data-link-for=
"PaymentAddress">organization</a> attribute's value.
</dd>
<dt>
<dfn>phoneError</dfn> member
</dt>
<dd>
Denotes that the <a>phone number</a> has a validation error. In the
user agent's UI, this member corresponds to the input field that
provided the <a>PaymentAddress</a>'s <a data-link-for=
"PaymentAddress">phone</a> attribute's value.
</dd>
<dt>
<dfn>postalCodeError</dfn> member
</dt>
<dd>
Denotes that the <a>postal code</a> has a validation error. In the
user agent's UI, this member corresponds to the input field that
provided the <a>PaymentAddress</a>'s <a data-link-for=
"PaymentAddress">postalCode</a> attribute's value.
</dd>
<dt>
<dfn>recipientError</dfn> member
</dt>
<dd>
Denotes that the <a>recipient</a> has a validation error. In the
user agent's UI, this member corresponds to the input field that
provided the <a>PaymentAddress</a>'s <a data-link-for=
"PaymentAddress">addressLine</a> attribute's value.
</dd>
<dt>
<dfn>regionError</dfn> member
</dt>
<dd>
Denotes that the <a>region</a> has a validation error. In the user
agent's UI, this member corresponds to the input field that
provided the <a>PaymentAddress</a>'s <a data-link-for=
"PaymentAddress">region</a> attribute's value.
</dd>
<dt>
<dfn>regionCodeError</dfn> member
</dt>
<dd>
Denotes that the region code representation of the <a>region</a>
has a validation error. In the user agent's UI, this member
corresponds to the input field that provided the
<a>PaymentAddress</a>'s <a data-link-for=
"PaymentAddress">regionCode</a> attribute's value.
</dd>
<dt>
<dfn>sortingCodeError</dfn> member
</dt>
<dd>
The <a>sorting code</a> has a validation error. In the user agent's
UI, this member corresponds to the input field that provided the
<a>PaymentAddress</a>'s <a data-link-for=
"PaymentAddress">sortingCode</a> attribute's value.
</dd>
</dl>
</section>
<section>
<h2>
Creating a <code>PaymentAddress</code> from user-provided input
Expand Down Expand Up @@ -3914,21 +4103,37 @@ <h2>
</li>
</ol>
</li>
<li>If <var>request</var>.<a>[[\options]]</a>.<a data-lt=
"PaymentOptions.requestShipping">requestShipping</a> is true,
and
<var>request</var>.<a>[[\details]]</a>.<a>shippingOptions</a>
is empty, then the developer has signified that there are no
valid shipping options for the currently-chosen shipping
address (given by <var>request</var>'s <a data-lt=
"PaymentRequest.shippingAddress">shippingAddress</a>). In
this case, the user agent SHOULD display an error indicating
this, and MAY indicate that the currently-chosen shipping
address is invalid in some way. The user agent SHOULD use the
<a data-lt="PaymentDetailsUpdate.error">error</a> member of
<var>details</var>, if it is present, to give more
information about why there are no valid shipping options for
that address.
<li>
<p>
If <var>request</var>.<a>[[\options]]</a>.<a data-lt=
"PaymentOptions.requestShipping">requestShipping</a> is
true, and
<var>request</var>.<a>[[\details]]</a>.<a>shippingOptions</a>
is empty, then the developer has signified that there are
no valid shipping options for the currently-chosen
shipping address (given by <var>request</var>'s
<a data-lt=
"PaymentRequest.shippingAddress">shippingAddress</a>).
</p>
<p>
In this case, the user agent SHOULD display an error
indicating this, and MAY indicate that the
currently-chosen shipping address is invalid in some way.
The user agent SHOULD use the <a data-lt=
"PaymentDetailsUpdate.error">error</a> member of
<var>details</var>, if it is present, to give more
information about why there are no valid shipping options
for that address.
</p>
<p>
Further, if the <a data-lt=
"PaymentDetailsUpdate.shippingAddressErrors">shippingAddressErrors</a>
member is present, the user agent SHOULD display an error
specifically for each erroneous field of the shipping
address. This is done by matching each present member of
the <a>AddressErrorFields</a> to a corresponding input
field in the shown user interface.
</p>
</li>
</ol>
</li>
Expand Down