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 error recovery for fields #647

Closed
marcoscaceres opened this issue Nov 7, 2017 · 20 comments · Fixed by #712
Closed

Fine grained error recovery for fields #647

marcoscaceres opened this issue Nov 7, 2017 · 20 comments · Fixed by #712

Comments

@marcoscaceres
Copy link
Member

marcoscaceres commented Nov 7, 2017

Similarly to Apple Pay, we need need to define a means to signal which fields in the payment sheet suffer from a validation error.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Dec 7, 2017

@aestes, @michelle-stripe, @jenan-stripe, @rsolomakhin and I discussed fine grained error recovery for fields during the F2F, including the ability to "retry" the payment.

Rough proposal is to add AddressField:

 enum AddressField {
  "addressLine",
  "city",
  "country",
  "dependentLocality",
  "languageCode",
  "organization",
  "phone",
  "postalCode",
  "recipient",
  "region",
  "sortingCode"
};

Define PaymentError interface, somewhat matching ApplePayError:

enum PaymentErrorCode {
   "invalidShippingAddress",
   "invalidBillingAddress",
   "unknown",
   "badAddress" // "unserviceable" so hard to spell
};

[Constructor(PaymentErrorCode code, optional AddressField? field, optional DOMString message = "")]
interface PaymentError {
   readonly attribute PaymentErrorCode type;
   readonly attribute AddressField? AddressField; 
   readonly attribute DOMString message;
};

And add the following member to PaymentDetailsUpdate:

   sequence<PaymentError> errorFields; 

Thus, "onshippingaddresschange":

const errorField = [
   new PaymentError("invalidShippingAddress", "country", "We don't ship to your country."),
   new PaymentError("invalidShippingAddress", "postCode", "This postcode isn't valid."),
];
 ev.updateWith({
      ...details,
      error: "Couple of issues with the shipping address.", 
      errorFields,
});

Add lastly, we add response.retry(...errors), which returns a promise that resolves with undefined... waits for user to try hit "Pay" again:

let errors = [
   new PaymentError("invalidShippingAddress", "addressLine", "I can't ship to PO boxes."),
   new PaymentError("invalidBillingAddress", null, "Srsly? that's an abandoned house."),
];
await response.retry(...errors);
errors = checkResponse(response);
if(errors.length){
   // try again... this would be recursive... 
   await response.retry(...errors);
} else {
  const status = processPayment(response);
  await response.complete(status);  
}

@mnoorenberghe
Copy link
Member

mnoorenberghe commented Dec 7, 2017

It might be a bit of a UX problem for UAs to have to render multiple errors that aren't AddressField-specific. e.g. with error set to a string and/or multiple PaymentError with a null AddressField. The UA won't know how to combine them appropriately.

@marcoscaceres
Copy link
Member Author

Yeah, we need to say last one wins in the case of duplicates.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Dec 7, 2017

Here is an implementation showing how it could work with .retry(...errors) .

async function doPaymentRequest() {
  const request = new PaymentRequest(methods, details, options);
  const response = await request.show();
  try {
    await retryUntilGood(response);
    const status = await processPayment(response);
    await response.complete(status);
  } catch (err) { 
    // AbortError? user got bored retrying. 
  }
}

async function retryUntilGood(response) {
  const errors = await validateResponse(response);
  if (errors.length) {
    await response.retry(...errors); // Can throw AbortError
    await retryUntilGood(response);
  }
}

@marcoscaceres
Copy link
Member Author

Fixed typo above.

@mnoorenberghe
Copy link
Member

An alternative approach to allowing the user to retry a payment without introducing a new method would be to use an event like Apple Pay's onpaymentauthorized but implementing the PaymentRequestAuthorizedEvent interface:

interface PaymentRequestAuthorizedEvent : PaymentRequestUpdateEvent {
    readonly attribute PaymentResponse response;
};

errorFields would still be added to PaymentDetailsUpdate to be used by onpaymentauthorized's updateWith method to specify errors (field-specific and otherwise) with the same PaymentError interface as previous proposal.

An author responds to onpaymentauthorized with either an updateWith to allow the user to retry with some errors or by calling response.complete() to close the request UI.

request.onpaymentauthorized = function(event) {
  let errorFields = checkResponse(event.response);
  if (errors.length) {
    event.updateWith(Promise.resolve({
      details,
      errorFields,
    }));
  } else {
    const status = processPayment(response);
    event.response.complete(status);  
  }
}

This approach feels more natural to me than getting a new Promise from each .retry(…) and provides a single way to provide PaymentErrors, rather than updateWith + retry.
Just my two cents.

@marcoscaceres
Copy link
Member Author

Third alternative - from @rsolomakhin's notes from the F2F meeting.

PaymentResponse.complete() gets a new "retry" enum value for correcting payment errors. Merchant would specify an enum value like “billing addressing zip code is not correct” or “card declined,” for example. Then the user can attempt to fix the error and retry the payment. The payment handler should be able to specify whether a retry is possible, so push-payments can work correctly.

const response = await pr.show();
async function validateResponse(response){
  const badThings = await getInvalidThings(response);
  if (!badThings){
    return; // yay, it’s all good!
  }
  // Oh noes, let’s get the user to fix bad things
  await response.complete("retry", badThings);
  return validateResponse(response);
}
await validateResponse(response);

@rsolomakhin
Copy link
Collaborator

The method response.retry(errors) will be easy to feature-detect for early adopters. I would prefer this one, if that's OK.

@marcoscaceres
Copy link
Member Author

Regrettably, @mnoorenberghe's proposal would have been great if we were not already returning response from .show() (i.e., if we would have designed the API like that initially).

However, introducing the event leaves const response = await request.show(); in a strange place, specially when you try to wire everything up. We effectively end up with two sources for the response, and it means developers would need to wrap the request.onpaymentauthorized in a promise that guards against premature completion (as below, I am personally not a big fan):

const request = new PaymentRequest();
const canContinue = new Promise(resolve => {
  request.onpaymentauthorized = event => {
    const errorFields = checkResponse(event.response);
    if (errorFields.length) {
      event.updateWith({
        details,
        errorFields,
      });
      return;
    }
    resolve();
  };
});
// This is somewhat messy
// because now I'm holding promises instead of awaiting them :(
const showPromise = request.show();
await canContinue;
const response = await showPromise;
const status = processPayment(response);
response.complete(status);

Definitely, could make the above leaner by doing what @mnoorenberghe does in his example above (do the .compete() logic in the handler itself)... but, as I said, I think we missed that opportunity with the current design that returns response from await request.show().

Other cons would be having to introduce a new EventHandler interface type, as well as an attribute. While .retry() only adds a single method and could work well with the existing state machine.

Now, API design aside, a larger problem when retrying will be figuring out the restrictions on what can actually be fixed... If the shippingAddress is not serviceable, then what does picking a new one do (specially because it could change the total, and now the request is in a "close" state - so effectively dead)? Does the request again wake up and get put into an "interactive" state?

Need to think more about the above, or see how ApplePay is handling it.

@rsolomakhin
Copy link
Collaborator

@marcoscaceres, IMHO, shouldn't .retry() be for billing issues only? Shipping address validation happens before user authorizes the payment, if the merchant needs to validate it, I thought.

@marcoscaceres
Copy link
Member Author

Well, depends on #648 and if addressLine should be redacted. But ideally you are correct: then user can only change or update the payment instrument (and associated billing address) at this stage. This doesn’t quite match what ApplePay allows, but that might be workable nonetheless.

@marcoscaceres
Copy link
Member Author

Well, depends on #648 and if addressLine should be redacted.

To be clear, redacted when the “shippingaddresschange” event fires.

@marcoscaceres marcoscaceres changed the title fine grained error recovery for fields retry and fine grained error recovery for fields Dec 18, 2017
@marcoscaceres
Copy link
Member Author

Consider supporting email, phoneNumber, etc. for retry.

@marcoscaceres marcoscaceres self-assigned this Apr 19, 2018
@marcoscaceres
Copy link
Member Author

Seeking input:

  • @domenic, feedback on IDL and general design would be appreciated.
  • @aestes, are there cases that are not covered (i.e., that you couldn't then map to ApplePayError)?
  • @mnoorenberghe, @edenchuang, implementable?

Shipping address errors

Looking back over the discussion, I think having a PaymentError interface might be confusing and a bit heavyweight: Confusing, in that it's not an Error in the JS/DOM sense (has no stack, for example). Heavyweight, in that it simply models a triple, but provides no other utilities.

As an alternative, I think we can just use dictionaries to achieve the same result:

dictionary AddressErrors {
  DOMString addressLine;
  DOMString city;
  DOMString country;
  DOMString dependentLocality;
  DOMString languageCode;
  DOMString organization;
  DOMString phone;
  DOMString postalCode;
  DOMString recipient;
  DOMString region;
  DOMString regionCode;
  DOMString sortingCode;
};

partial dictionary PaymentDetailsUpdate {
  AddressErrors shippingAddressErrors;
}

Note: AddressErrors could be reused for billing address errors, once the Payment Request API actually supports billing addresses properly (#27 and #550).

Note: Errors for redacted fields could be ignored by the UA (e.g., complaining about shipping address' phone during the "payment request" phase would be ignored, but would be honored when retrying the payment response).

Usage example

pr.onshippingaddresschange = ev => {
  const shippingAddressErrors = {
    "city": "FarmVille is not a real place.",
    "postalCode": "Doesn't match a known zip code for your country.",
  };
  event.updateWith({ shippingAddressErrors });
}

Payer-related errors

The next class of errors deals specifically with payer related errors (name, email, phone):

dictionary PayerErrors {
  DOMString email;
  DOMString name;
  DOMString phone;
}
partial dictionary PaymentDetailsUpdate {
  PayerErrors payerErrors;
}

Example

const payerErrors = {
  email: "The domain name is invalid",
  phone: "Invalid format",
};
// We don't have a means to use this object yet... that will be `retry()`.

Thoughts, before I spec it up?

@marcoscaceres marcoscaceres changed the title retry and fine grained error recovery for fields Fine grained error recovery for fields May 2, 2018
@stpeter
Copy link

stpeter commented May 2, 2018

@marcoscaceres I like this approach - clean and lightweight. Thanks for the concrete proposal!

@domenic
Copy link
Collaborator

domenic commented May 2, 2018

This design seems reasonable from a spec/IDL perspective. Pretty nice and simple; just an extension of the existing "error" option in PaymentDetailsUpdate.

@mnoorenberghe
Copy link
Member

LGTM! Thanks for getting back to this so quickly!

I only have two nits:

  • I'm not sure any UAs plan to expose the underlying languageCode so I'm not sure how useful an error message for it would be but I think it's fine to be consistent and support errors on all fields in the shipping address.
  • Tangent: I'd rather we fixed the misnomer of addressLine => addressLines everywhere rather than continuing to propagate it e.g. support both in the code already shipped in UAs treating the old name as an alias to the new name.

@marcoscaceres
Copy link
Member Author

I'm not sure any UAs plan to expose the underlying languageCode so I'm not sure how useful an error message for it would be but I think it's fine to be consistent and support errors on all fields in the shipping address.

Agree.

Tangent: I'd rather we fixed the misnomer of addressLine => addressLines everywhere rather than continuing to propagate it

I'm thinking it might be too late... consider what happened already when we changed supportedMethods to a string. I think we are still dealing with the fallout.

support both in the code already shipped in UAs treating the old name as an alias to the new name.

I personally think this will cause confusion, as people might think they can set one or the other, but then one will trash the other, etc. Naming things is hard 😢

That reminds me, nevertheless, that maybe addressLine needs special treatment, because it's a sequence of lines.

So, maybe we actually need:

  (DOMString or sequence<DOMString?>) addressLine;

We could say that:

  • When it's a DOMString - the whole address is bad.
  • When it's a sequence, the UA can match the errors to particular lines.

So:

// The whole address is bad!
const addressErrors.addressLine = "Look, buddy, I can't ship to a PO Box!";

// Address is partially bad!
const addressErrors.addressLine = [
  null, 
  "the second line is bad!", 
  null, 
  "the fourth line is bad!"
];

Or is that overkill?

@mnoorenberghe
Copy link
Member

Or is that overkill?

Sounds like overkill to me

@marcoscaceres
Copy link
Member Author

Ok, let's see how we go with DOMString addressLine;.

I'll wait a few days for others to chime in, but can start drafting things up behind the scenes.

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

Successfully merging a pull request may close this issue.

5 participants