1435161 - Implement PaymentResponse.retry() method

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement, P1)

Description

[Matthew N. [:MattN]]

If there is an error with the user's payment method when the user clicks Pay, the PaymentRequest spec currently requires that the dialog is closed and any decisions/state (shipping address, shipping option, payment method, etc.) that the user had in the dialog would be lost. This leads to a poor user experience so the spec will be changing to allow retrying.

The spec change hasn't been made yet, discussion is in https://github.com/w3c/payment-request/issues/647, but I'm filing this to track the work to implement as it will likely be required for our MVP.

Comment 1

[Marcos Caceres [:marcosc]]

Eden, do you mind if I take this one? I have it in my quarterly goals to implement this?

Comment 2

[Eden Chuang[:edenchuang]]

Marcos, sure, I am totally okay with that.
BTW, I have already some local patches for function implementation. I will try to finish and attach them on the bug.
Maybe you want to start with tests? Because I do still not writing any tests for retry().

Comment 3

[Marcos Caceres [:marcosc]]

That all sounds great to me! Put up whatever you have. Happy to write tests. Did you add retry to the UI Testing service? If not, that's also fine - happy to help with that.

Comment 4

[Eden Chuang[:edenchuang]]

(In reply to Marcos Caceres [:marcosc] from comment #3)
> That all sounds great to me! Put up whatever you have. Happy to write tests.
> Did you add retry to the UI Testing service? If not, that's also fine -
> happy to help with that.

Sure, you can help writing tests and adding retry to the UI Testing service. That would be great. :)

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: Part1 Removing unnecessary XPCOM nsIPaymentActionRequest and nsIPaymentActionCallback in the codebase.

nsIPaymentActionRequest and nsIPaymentActionCallback are not exposed to front-end, so the interfaces are useless and should be implemented directly to improve the performance.

Comment 6

[Eden Chuang[:edenchuang]]

Attachment: Part2 Implement PaymentResponse.retry().

1. Add PaymentValidationErrors and PayerErrorFields in PaymentRequest.webidl and PaymentResponse.retry() in PaymentResponse.webidl

2. Implement PaymentRequest.retryPayment() and PaymentRequestManager.retryPayment() in content process.

3. Add IPCPaymentRetryActionRequest in PPaymentRequest.ipdl to transfer the error fields to chrome process.
    
4. Implement PaymentResponse.retry() by reusing the code of show() and updateWith() of PaymentRequestService in chrome process.

Comment 7

[Eden Chuang[:edenchuang]]

Attachment: Mochitest for PaymentResponse.retry()

Comment 8

[Eden Chuang[:edenchuang]]

Marcos I wrote some testcases for PaymentResponse.retry(). Could you take a look if there is any problem?

Comment 9

[Eden Chuang[:edenchuang]]

Attachment: Part 3 Support error fields in PaymentDetailsUpdate

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 9006222 [details] [diff] [review]
Part1 Removing unnecessary XPCOM nsIPaymentActionRequest and nsIPaymentActionCallback in the codebase.

Review of attachment 9006222 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/PaymentRequestData.h
@@ +190,5 @@
>                   nsIPaymentDetails* aPaymentDetails,
>                   nsIPaymentOptions* aPaymentOptions,
>                   const nsAString& aShippingOption);
>  
> +  void SetIPC(PaymentRequestParent* aIPC) {

void
SetIPC(PaymenetRequestParent* aPIC)
{
  mIPC = aIPC;
}

same for the other methods.

@@ +194,5 @@
> +  void SetIPC(PaymentRequestParent* aIPC) {
> +    mIPC = aIPC;
> +  }
> +
> +  PaymentRequestParent* GetIPC() {

GetIPC() const

@@ +216,5 @@
>    nsCOMPtr<nsIArray> mPaymentMethods;
>    nsCOMPtr<nsIPaymentDetails> mPaymentDetails;
>    nsCOMPtr<nsIPaymentOptions> mPaymentOptions;
>    nsString mShippingOption;
> +  PaymentRequestParent* mIPC;

Write a comment about why this is a raw pointer.

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 9006259 [details] [diff] [review]
Part 3 Support error fields in PaymentDetailsUpdate

Review of attachment 9006259 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/PaymentRequestData.h
@@ +132,5 @@
>    static nsresult Create(const IPCPaymentDetails& aIPCDetails,
>                           nsIPaymentDetails** aDetails);
>    nsresult Update(nsIPaymentDetails* aDetails, const bool aRequestShipping);
>    nsString GetShippingAddressErrors() const;
> +  nsString GetPayer() const;

you could do:
nsString& GetPayer() const;

::: dom/payments/PaymentRequestManager.cpp
@@ +215,5 @@
>    if (!aDetails.mShippingAddressErrors.ToJSON(shippingAddressErrors)) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  nsString payerErrors(EmptyString());

By default is empty. Just do nsAutoString payerErrors;

@@ +220,5 @@
> +  if (!aDetails.mPayerErrors.ToJSON(payerErrors)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsString paymentMethodErrors(EmptyString());

same here. Fix other similar calls.

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: Part1 Removing unnecessary XPCOM nsIPaymentActionRequest and nsIPaymentActionCallback in the codebase.

Update the patch with comments.

Comment 14

[Eden Chuang[:edenchuang]]

Attachment: Part2 Implement PaymentResponse.retry().

rebase the patch.

Comment 15

[Eden Chuang[:edenchuang]]

Attachment: Part3 Support error fields in PaymentDetailsUpdate

Update the patch according to comments.

Comment 16

[Marcos Caceres [:marcosc]]

(In reply to Eden Chuang[:edenchuang] from comment #8)
> Marcos I wrote some testcases for PaymentResponse.retry(). Could you take a
> look if there is any problem?

Happy to, but I wasn't able to find them in any of the attachments?

Comment 17

[Eden Chuang[:edenchuang]]

Attachment: Part1 Removing unnecessary XPCOM nsIPaymentActionRequest and nsIPaymentActionCallback in the codebase.

Fix the compile errors in some platforms.

Comment 18

[Marcos Caceres [:marcosc]]

Comment on attachment 9006225 [details] [diff] [review]
Mochitest for PaymentResponse.retry()

Review of attachment 9006225 [details] [diff] [review]:
-----------------------------------------------------------------

Couple of potential issues in the html file, but all generally good. Just a reminder about languageCode going away, which will cause this to fail after rebase. 

We should consider changing "billingAddress.init()" to take a property bag instead in the future. If we need to remove things in the future, it will avoid some issues. Does Mozilla's IDL support dictionaries?

::: dom/payments/test/RetryPaymentChromeScript.js
@@ +26,5 @@
> +                     "San Bruno",        // city
> +                     "",                 // dependent locality
> +                     "94066",            // postal code
> +                     "123456",           // sorting code
> +                     "en",               // language code

Language code has been removed, so this will cause fails once you rebase.

@@ +153,5 @@
> +                 errors.password + "'");
> +  }
> +}
> +
> +var rejectRetry = false;

Nit: Globals are eeeky... Why not just just make this a property of the DummyService? 
```
const DummyUIService = {
  rejectRetry: false,
  ...
};
```

Then on 179 you can do:

```
  if (this.rejectRetry) {
```

And: 

```
addMessageListener("reject-retry", function () {
  DummyService.rejectRetry = true;
  sendAsyncMessage("reject-retry-complete");	
});
```

@@ +156,5 @@
> +
> +var rejectRetry = false;
> +
> +const DummyUIService = {
> +  showPayment: (requestId) => {

nit, change:
```
 showPayment: (requestId) => {
```

To:

```
 showPayment(requestId){
```

Otherwise, `this` will be wrong if these tests are updated in the future.

This applies to the other methods in this object.

::: dom/payments/test/test_retryPayment.html
@@ +13,5 @@
> +
> +  "use strict";
> +  SimpleTest.waitForExplicitFinish();
> +
> +  var gUrl = SimpleTest.getTestFileURL('RetryPaymentChromeScript.js');

nit: const

@@ +14,5 @@
> +  "use strict";
> +  SimpleTest.waitForExplicitFinish();
> +
> +  var gUrl = SimpleTest.getTestFileURL('RetryPaymentChromeScript.js');
> +  var gScript = SpecialPowers.loadChromeScript(gUrl);

nit: const

@@ +43,5 @@
> +      addressLine: "addressLine error",
> +      city: "city error",
> +      country: "country error",
> +      dependentLocality: "dependentLocality error",
> +      languageCode: "languageCode error",

language code is going away :)

@@ +96,5 @@
> +       payResponse.payerPhone + "' after retry PaymentRequest");
> +  }
> +
> +  function testRetryAfterComplete() {
> +    return new Promise(async (resolve, reject) => {

Here you don't need the promise, just make testRetryAfterComplete() async. Then just replace `resolve()` for `return`;

@@ +115,5 @@
> +        }
> +        try {
> +          await payResponse.complete("success");
> +        } catch (err) {
> +          ok(false, testName + ": Unexpected error(" + err.name + ") when complete the PaymentResposne.");

nit: typo PaymentResposne -> PaymentResponse

@@ +116,5 @@
> +        try {
> +          await payResponse.complete("success");
> +        } catch (err) {
> +          ok(false, testName + ": Unexpected error(" + err.name + ") when complete the PaymentResposne.");
> +          resolve();

This test seems wrong also, as line 114 could have already called `resolve()`, so calling it again has no effect. This will be fixed or more obvious with async/await.

@@ +121,5 @@
> +        }
> +        try {
> +          await payResponse.retry(validationErrors);
> +          ok(false, testName + ": Unexpected success when retry the PaymentResponse.");
> +          resolve();

As above.

@@ +134,5 @@
> +    });
> +  }
> +
> +  function testRetryAfterRetry() {
> +    return new Promise(async (resolve, reject) => {

As above. convert `testRetryAfterRetry()` to `async testRetryAfterRetry()` and drop the promise.

@@ +175,5 @@
> +    });
> +  }
> +
> +  function testRetryWithEmptyErrors() {
> +    return new Promise(async (resolve, reject) => {

Same again.

@@ +203,5 @@
> +    });
> +  }
> +
> +  function testRetry() {
> +    return new Promise(async (resolve, reject) => {

and here :)

@@ +275,5 @@
> +      gScript.sendAsyncMessage("teardown");
> +    });
> +  }
> +
> +  function runTests() {

nit, async/await this too. It makes it easier to track the flow of tests.

Comment 19

[Eden Chuang[:edenchuang]]

Attachment: Mochitest for PaymentResponse.retry()

Update the patch according to the feedback. Marcos, could you double check the patch again. Thanks.

Comment 20

[Marcos Caceres [:marcosc]]

Comment on attachment 9007164 [details] [diff] [review]
Mochitest for PaymentResponse.retry()

Review of attachment 9007164 [details] [diff] [review]:
-----------------------------------------------------------------

couple of small things - typos and nits. There is one small bug in ChromeScript, however.

::: dom/payments/test/RetryPaymentChromeScript.js
@@ +80,5 @@
> +}
> +
> +function checkAddressErrors(errors) {
> +  if (!errors) {
> +    emitTestFail("Expect non-null shippingAddressErrors, but got null.");

We need an "return" after `emitTestFail` here, otherwise, the next checks will throw an exception for trying to access a property on a null/undefined object. Same applies to on line 126.

@@ +82,5 @@
> +function checkAddressErrors(errors) {
> +  if (!errors) {
> +    emitTestFail("Expect non-null shippingAddressErrors, but got null.");
> +  }
> +  if (errors.addressLine != "addressLine error") {

could maybe just be:

```
for (const [key, msg] of Object.entries(errors)) {
  const expected = `${key} error`;
  if (msg !== expected) {
    emitTestFail(
      "Expected '${expected}' on shippingAddressErrors.${key}, but got '${msg}'."
    );
  }
}
```

Then we don't need to worry about updating this if we add new errors. 

Same with the `checkPayerErrors()`

@@ +120,5 @@
> +                  errors.sortingCode + "'");
> +  }
> +}
> +
> +function checkPayerErrors (errors) {

Nit: whitespace between `checkPayerErrors (errors)`. 

Highly recommend formatting JS code using: https://github.com/prettier/prettier

@@ +122,5 @@
> +}
> +
> +function checkPayerErrors (errors) {
> +  if (!errors) {
> +    emitTestFail("Expect non-null payerErrors, but got null.");

`return;` after this line.

@@ +138,5 @@
> +                 errors.phone + "'");
> +  }
> +}
> +
> +function checkPaymentMethodErrors(errors) {

Maybe we should start testing basic card errors here instead?

::: dom/payments/test/test_retryPayment.html
@@ +15,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +
> +  const gUrl = SimpleTest.getTestFileURL('RetryPaymentChromeScript.js');
> +  const gScript = SpecialPowers.loadChromeScript(gUrl);
> +  var testName = "";

nit: use `let` for variables that are reassigned... generally, there is no valid reason to use a `var`. Maybe we can avoid using this global?

@@ +70,5 @@
> +    requestShipping: true,
> +    shippingType: "shipping"
> +  };
> +
> +  function checkShowResponse(payResponse) {

You should just pass "testName" as an argument here, instead of using the global. Its less fragile.

@@ +72,5 @@
> +  };
> +
> +  function checkShowResponse(payResponse) {
> +    is(payResponse.payerName, "Bill A. Pacheco", testName +
> +       ": Expected 'Bill A. Pacheco' on PaymentResponse.payerName, but got'" +

nit: typo space after got. Also, copy/pasta errors in the names of properties. 

might be a bit cleaner to do/maintainable to do:

```
function checkShowResponse(payResponse) {
  const { payerName, payerEmail, payerPhone } = payResponse.toJSON();
  is(
    payerName,
    "Bill A. Pacheco",
    `${testName}: Expected 'Bill A. Pacheco' on payerName, but got '${payerName}' after show PaymentRequest`
  );
  is(
    payerEmail,
    "",
    `${testName}: Expected '' on payerEmail, but got '${payerEmail}' after show PaymentRequest`
  );
  is(
    payerPhone,
    "",
    `${testName}: Expected '' on payerPhone, but got '${payerPhone}' after show PaymentRequest`
  );
}

```

@@ +82,5 @@
> +       ": Expected '' on PaymentResponse.payerName, but got'" +
> +       payResponse.payerPhone + "' after show PaymentRequest");
> +  }
> +
> +  function checkRetryResponse(payResponse) {

I think you should pass the "testName" as an argument here.

@@ +83,5 @@
> +       payResponse.payerPhone + "' after show PaymentRequest");
> +  }
> +
> +  function checkRetryResponse(payResponse) {
> +    is(payResponse.payerName, "Bill A. Pacheco", testName +

As above, fixed typos and copy/pasta:

```
function checkRetryResponse(payResponse) {
  const { payerName, payerEmail, payerPhone } = payResponse.toJSON();
  is(
    payerName,
    "Bill A. Pacheco",
    `${testName}: Expected 'Bill A. Pacheco' on payerName, but got '${payerName}' after retry PaymentRequest`
  );
  is(
    payerEmail,
    "[e-mail]",
    `${testName} : Expected '[e-mail]' on payerEmail, but got '${payerEmail}' after retry PaymentRequest`
  );
  is(
    payerPhone,
    "[phone]",
    `${testName} : Expected '[phone]' on payerPhone, but got '${payerPhone}' after retry PaymentRequest`
  );
}
```

@@ +98,5 @@
> +  async function testRetryAfterComplete() {
> +    testName = "testRetryAfterComplete";
> +    const payRequest = new PaymentRequest(defaultMethods, defaultDetails, options);
> +    ok(payRequest, testName + ": failed to create PaymentRequest.");
> +    if (payRequest) {

maybe just:

```
if (!payrequest) return; 
```

Avoid the unnecessary nesting. Same with others.

@@ +106,5 @@
> +        payResponse = await payRequest.show();
> +        await checkShowResponse(payResponse);
> +        handler.destruct();
> +      } catch (err) {
> +        ok(false, testName + ": Unexpected error(" + err.name +

nit: use template string to make this more legible - same with others.

@@ +132,5 @@
> +    }
> +  }
> +
> +  async function testRetryAfterRetry() {
> +    testName = "testRetryAfterRetry";

nit const.

Comment 21

[Eden Chuang[:edenchuang]]

Attachment: Mochitest for PaymentResponse.retry()

Update the patch according to the comment 20.

Comment 22

[Marcos Caceres [:marcosc]]

Comment on attachment 9007791 [details] [diff] [review]
Mochitest for PaymentResponse.retry()

Review of attachment 9007791 [details] [diff] [review]:
-----------------------------------------------------------------

Tests are looking pretty solid with the async error feedback! This is really nice work, Eden!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3bb55a328e0
mochitest for PaymentResponse.retry(). r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/ead0d6dcd6f0
Part 1 Removing unnecessary XPCOM components for PaymentRequest API. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/e65bec7e01c8
Part 2 supporting PaymentResponse.retry(). r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3e7d6d6e0d5
Part 3 support error fields in PaymentDetailsUpdate. r=baku

Comment 24

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c3bb55a328e0
https://hg.mozilla.org/mozilla-central/rev/ead0d6dcd6f0
https://hg.mozilla.org/mozilla-central/rev/e65bec7e01c8
https://hg.mozilla.org/mozilla-central/rev/d3e7d6d6e0d5

Comment 25

[Eric Shepherd [:sheppy]]

Content is up to date, including assorted additional content added to the main PaymentResponse.retry() page as well as to show(), and the BCD data has been created and will go up in a PR that will roll up a few of these changes.