Implement PaymentResponse.retry() method

RESOLVED FIXED in Firefox 64

Status

()

P1
normal
RESOLVED FIXED
10 months ago
3 days ago

People

(Reporter: MattN, Assigned: edenchuang)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla64
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [webpayments] [user-testing], URL)

Attachments

(4 attachments, 6 obsolete attachments)

27.42 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
10.67 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
63.47 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
21.21 KB, patch
edenchuang
: review+
marcosc
: feedback+
Details | Diff | Splinter Review
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.

Updated

8 months ago
Priority: P1 → P2
Whiteboard: [webpayments]

Updated

6 months ago
Priority: P2 → P3
Whiteboard: [webpayments] → [webpayments-reserve]
Summary: Add Payment Request Retry support → Implement PaymentResponse.retry() method
Whiteboard: [webpayments-reserve] → [webpayments-reserve] [user-testing]

Updated

4 months ago
Priority: P3 → P2
Whiteboard: [webpayments-reserve] [user-testing] → [webpayments] [user-testing]
Flags: qe-verify-

Updated

4 months ago
Assignee: nobody → echuang
Status: NEW → ASSIGNED
Priority: P2 → P1
Eden, do you mind if I take this one? I have it in my quarterly goals to implement this?
(Assignee)

Comment 2

3 months ago
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().
Flags: needinfo?(mcaceres)
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.
Flags: needinfo?(mcaceres)
(Assignee)

Comment 4

3 months ago
(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. :)
(Assignee)

Comment 5

3 months ago
Created attachment 9006222 [details] [diff] [review]
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.
Attachment #9006222 - Flags: review?(amarchesini)
(Assignee)

Comment 6

3 months ago
Created attachment 9006224 [details] [diff] [review]
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.
Attachment #9006224 - Flags: review?(amarchesini)
(Assignee)

Comment 7

3 months ago
Created attachment 9006225 [details] [diff] [review]
Mochitest for PaymentResponse.retry()
Attachment #9006225 - Flags: review?(amarchesini)
(Assignee)

Comment 8

3 months ago
Marcos I wrote some testcases for PaymentResponse.retry(). Could you take a look if there is any problem?
Flags: needinfo?(mcaceres)
Keywords: dev-doc-needed
(Assignee)

Comment 9

3 months ago
Created attachment 9006259 [details] [diff] [review]
Part 3 Support error fields in PaymentDetailsUpdate
Attachment #9006259 - Flags: review?(amarchesini)
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1473081
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.
Attachment #9006222 - Flags: review?(amarchesini) → review+
Attachment #9006224 - Flags: review?(amarchesini) → review+
Attachment #9006225 - Flags: review?(amarchesini) → review+
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.
Attachment #9006259 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 13

2 months ago
Created attachment 9006735 [details] [diff] [review]
Part1 Removing unnecessary XPCOM nsIPaymentActionRequest and nsIPaymentActionCallback in the codebase.

Update the patch with comments.
Attachment #9006222 - Attachment is obsolete: true
Attachment #9006735 - Flags: review+
(Assignee)

Comment 14

2 months ago
Created attachment 9006736 [details] [diff] [review]
Part2 Implement PaymentResponse.retry().

rebase the patch.
Attachment #9006224 - Attachment is obsolete: true
Attachment #9006736 - Flags: review+
(Assignee)

Comment 15

2 months ago
Created attachment 9006738 [details] [diff] [review]
Part3 Support error fields in PaymentDetailsUpdate

Update the patch according to comments.
Attachment #9006259 - Attachment is obsolete: true
Attachment #9006738 - Flags: review+
(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?
Flags: needinfo?(mcaceres)
(Assignee)

Updated

2 months ago
Attachment #9006225 - Flags: feedback?(mcaceres)
(Assignee)

Comment 17

2 months ago
Created attachment 9007031 [details] [diff] [review]
Part1 Removing unnecessary XPCOM nsIPaymentActionRequest and nsIPaymentActionCallback in the codebase.

Fix the compile errors in some platforms.
Attachment #9006735 - Attachment is obsolete: true
Attachment #9007031 - Flags: review+
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.
Attachment #9006225 - Flags: feedback?(mcaceres) → feedback-
(Assignee)

Comment 19

2 months ago
Created attachment 9007164 [details] [diff] [review]
Mochitest for PaymentResponse.retry()

Update the patch according to the feedback. Marcos, could you double check the patch again. Thanks.
Attachment #9006225 - Attachment is obsolete: true
Attachment #9007164 - Flags: review+
Attachment #9007164 - Flags: feedback?(mcaceres)
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,
    "bpacheco@test.org",
    `${testName} : Expected 'bpacheco@test.org' on payerEmail, but got '${payerEmail}' after retry PaymentRequest`
  );
  is(
    payerPhone,
    "+123456789",
    `${testName} : Expected '+123456789' 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.
Attachment #9007164 - Flags: feedback?(mcaceres) → feedback-
(Assignee)

Comment 21

2 months ago
Created attachment 9007791 [details] [diff] [review]
Mochitest for PaymentResponse.retry()

Update the patch according to the comment 20.
Attachment #9007164 - Attachment is obsolete: true
Attachment #9007791 - Flags: review+
Attachment #9007791 - Flags: feedback?(mcaceres)
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!
Attachment #9007791 - Flags: feedback?(mcaceres) → feedback+
(Assignee)

Updated

2 months ago
Keywords: checkin-needed

Comment 23

2 months ago
Pushed by csabou@mozilla.com:
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
Keywords: checkin-needed

Comment 24

2 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1490754
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.