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 reporting with canMakePayment() #847

Open
ianbjacobs opened this issue Mar 8, 2019 · 2 comments
Open

Fine-grained error reporting with canMakePayment() #847

ianbjacobs opened this issue Mar 8, 2019 · 2 comments

Comments

@ianbjacobs
Copy link
Collaborator

I am creating this issue based on discussion on issue #843.

I am hearing several use cases for fine-grained error reporting when canMakePayment() returns false:

  • An error message (e.g., "okToUseAPI") that indicates that the user does not want to share detailed information but would like to use the API under show() conditions would reveal some information about the user's browser configuration.

  • Errors in SSL. For example, self-signed SSL causes Chrome to always return "false" to canMakePayment() and reject show() with "AbortError: Request cancelled."

  • Reasons why the payment sheet closed, e.g., "AbortError: User closed the payment handler", "AbortError: The payment handler cancelled the transaction", or "AbortError: User closed the card CVV entry form."

I propose that we take up these use cases after version 1.

@rsolomakhin
Copy link
Collaborator

  • An error message (e.g., "okToUseAPI") that indicates that the user does not want to share detailed information but would like to use the API under show() conditions would reveal some information about the user's browser configuration.

After thinking about this more, let's not add this state for canMakePayment, because it seems to be equivalent to canMakePayment returning true in the current version of the spec: true should be returned when a payment handler is available regardless of presence of any instruments in the payment handler. If there're no payment handlers available, then show() would be rejected with NotSupportedError, which means it's not "ok to use API."

Personally, I would be OK to not add this feature to hasEnrolledInstrument either and allow browsers to always return the same value as canMakePayment regardless of user wallet contents. However, if the group consensus is to be more granular here, then we can add this state to hasEnrolledInstrument as "NotAllowedError: User has turned off sharing this information."

  • Errors in SSL. For example, self-signed SSL causes Chrome to always return "false" to canMakePayment() and reject show() with "AbortError: Request cancelled."

After version 1 is finished, let's discuss a getCannotMakePaymentReason() method .

  • Reasons why the payment sheet closed, e.g., "AbortError: User closed the payment handler", "AbortError: The payment handler cancelled the transaction", or "AbortError: User closed the card CVV entry form."

Let's recommend to be specific with error messages without requiring all browsers to use the same error message text. The error codes (e.g., AbortError) are normative, but the error messages (e.g., "Request cancelled") are informative.

I propose that we take up these use cases after version 1.

Agreed.

@aestes
Copy link
Collaborator

aestes commented Mar 14, 2019

  • An error message (e.g., "okToUseAPI") that indicates that the user does not want to share detailed information but would like to use the API under show() conditions would reveal some information about the user's browser configuration.

After thinking about this more, let's not add this state for canMakePayment, because it seems to be equivalent to canMakePayment returning true in the current version of the spec: true should be returned when a payment handler is available regardless of presence of any instruments in the payment handler. If there're no payment handlers available, then show() would be rejected with NotSupportedError, which means it's not "ok to use API."

👍

Personally, I would be OK to not add this feature to hasEnrolledInstrument either and allow browsers to always return the same value as canMakePayment regardless of user wallet contents. However, if the group consensus is to be more granular here, then we can add this state to hasEnrolledInstrument as "NotAllowedError: User has turned off sharing this information."

👍

It's not clear to me what merchants would do with the extra granularity. hasEnrolledInstrument() returning false should not be a reason to not call show() so long as canMakePayment() returns true. Handlers can choose to offer instrument enrollment as part of their payment flow or just reject the call to show(), which as you point out offers the same information.

  • Errors in SSL. For example, self-signed SSL causes Chrome to always return "false" to canMakePayment() and reject show() with "AbortError: Request cancelled."

After version 1 is finished, let's discuss a getCannotMakePaymentReason() method .

FWIW, WebKit throws an InvalidAccessError in the PaymentRequest constructor if a non-root certificate in the chain is signed with SHA1. I don't think we reject on self-signed certificates, but maybe I'm misreading the code (and maybe we should in a way that doesn't break our testing).

We could start here (after v1) by agreeing on what exception to throw, when to throw it, and under what circumstances (and then maybe add a new method if we still think we need it).

  • Reasons why the payment sheet closed, e.g., "AbortError: User closed the payment handler", "AbortError: The payment handler cancelled the transaction", or "AbortError: User closed the card CVV entry form."

Let's recommend to be specific with error messages without requiring all browsers to use the same error message text. The error codes (e.g., AbortError) are normative, but the error messages (e.g., "Request cancelled") are informative.

👍

If we really need to be programmatically granular, I guess we can use different error codes.

I propose that we take up these use cases after version 1.

Agreed.

👍

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

No branches or pull requests

3 participants