Closed Bug 1441683 Opened 2 years ago Closed 2 years ago

response.complete() does not make the completeStatus available to the payment UI service

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jaws, Assigned: jaws)

Details

(Whiteboard: [webpayments])

Attachments

(1 file)

From the looks of reading the DOM code, it looks like `response.complete("success")` does not forward the "success" value (the `completeStatus`), or anything related to it to the payment UI service.

The `complete` method is documented in the spec at https://www.w3.org/TR/payment-request/#complete-method

nsIPaymentCompleteActionRequest and nsIPaymentCompleteActionResponse have been implemented, and calling response.complete from the merchant side eventually calls LaunchUIAction, calling completePayment. However, paymentUIService.completePayment only receives the requestId, without the completeStatus.

Most of the logic for the complete method is found at https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/dom/payments/PaymentRequestManager.cpp#514-534

From my reading of the code, it looks like https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/dom/payments/PaymentRequestService.cpp#314 will need to get the completeStatus off of the request object and pass it to LaunchUIAction.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review230574


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/payments/test/browser/browser_complete_response.js:42
(Diff revision 2)
> +      info("clicking pay");
> +      spawnPaymentDialogTask(frame, PTU.DialogContentTasks.completePayment);
> +
> +      // Add a handler to complete the payment above.
> +      info("acknowledging the completion from the merchant page");
> +      let result = await ContentTask.spawn(browser, {result: status}, PTU.ContentTasks.addCompletionHandler);

Error: Line 42 exceeds the maximum line length of 100. [eslint: max-len]

::: toolkit/components/payments/test/browser/browser_complete_response.js:42
(Diff revision 2)
> +      info("clicking pay");
> +      spawnPaymentDialogTask(frame, PTU.DialogContentTasks.completePayment);
> +
> +      // Add a handler to complete the payment above.
> +      info("acknowledging the completion from the merchant page");
> +      let result = await ContentTask.spawn(browser, {result: status}, PTU.ContentTasks.addCompletionHandler);

Error: 'result' is assigned a value but never used. allowed unused vars must match /^exported_symbols$/. [eslint: no-unused-vars]

::: toolkit/components/payments/test/browser/browser_complete_response.js:49
(Diff revision 2)
> +      info("completion handler resolved");
> +      let payButtonDetails =
> +        await spawnPaymentDialogTask(frame, PTU.DialogContentTasks.getPayButtonDetails);
> +      const attrName = "data-" + status + "-label";
> +      const expectedButtonText = payButtonDetails.attributes[attrName];
> +      is(payButtonDetails.textContent, expectedButtonText, "Pay button should show be in '" + status + "' state");

Error: Line 49 exceeds the maximum line length of 100. [eslint: max-len]
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review230578


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/payments/test/browser/browser_complete_response.js:42
(Diff revision 1)
> +      info("clicking pay");
> +      spawnPaymentDialogTask(frame, PTU.DialogContentTasks.completePayment);
> +
> +      // Add a handler to complete the payment above.
> +      info("acknowledging the completion from the merchant page");
> +      let result = await ContentTask.spawn(browser, {result: status}, PTU.ContentTasks.addCompletionHandler);

Error: Line 42 exceeds the maximum line length of 100. [eslint: max-len]

::: toolkit/components/payments/test/browser/browser_complete_response.js:42
(Diff revision 1)
> +      info("clicking pay");
> +      spawnPaymentDialogTask(frame, PTU.DialogContentTasks.completePayment);
> +
> +      // Add a handler to complete the payment above.
> +      info("acknowledging the completion from the merchant page");
> +      let result = await ContentTask.spawn(browser, {result: status}, PTU.ContentTasks.addCompletionHandler);

Error: 'result' is assigned a value but never used. allowed unused vars must match /^exported_symbols$/. [eslint: no-unused-vars]

::: toolkit/components/payments/test/browser/browser_complete_response.js:49
(Diff revision 1)
> +      info("completion handler resolved");
> +      let payButtonDetails =
> +        await spawnPaymentDialogTask(frame, PTU.DialogContentTasks.getPayButtonDetails);
> +      const attrName = "data-" + status + "-label";
> +      const expectedButtonText = payButtonDetails.attributes[attrName];
> +      is(payButtonDetails.textContent, expectedButtonText, "Pay button should show be in '" + status + "' state");

Error: Line 49 exceeds the maximum line length of 100. [eslint: max-len]
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review230586

::: commit-message-0cbf5:3
(Diff revision 2)
> +Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service. r?baku,mattn
> +
> +todo: add some tests in the DOM code that don't rely on the UI layer

Agreed, I was just going to ask for that

::: dom/interfaces/payments/nsIPaymentRequest.idl:83
(Diff revision 2)
> +  readonly attribute AString completeStatus;
>    readonly attribute nsIArray paymentMethods;
>    readonly attribute nsIPaymentDetails paymentDetails;
>    readonly attribute nsIPaymentOptions paymentOptions;
>  
> +  void setCompleteStatus(in AString aCompleteStatus);

Without looking too closely at the patch… why would we have a readonly attribute but then effectively have a setter? Why does setCompleteStatus need to be exposed in the IDL?
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review231184

::: dom/payments/PaymentRequestData.cpp:606
(Diff revision 4)
>                                 nsIArray* aPaymentMethods,
>                                 nsIPaymentDetails* aPaymentDetails,
>                                 nsIPaymentOptions* aPaymentOptions)
>    : mTabId(aTabId)
>    , mRequestId(aRequestId)
> +  , mCompleteStatus(EmptyString())

This is not needed.

::: toolkit/components/payments/res/paymentRequest.xhtml:21
(Diff revision 4)
>    <!ENTITY successPaymentButton.label    "Done">
>    <!ENTITY failPaymentButton.label       "Fail">
>    <!ENTITY unknownPaymentButton.label    "Unknown">
>    <!ENTITY orderDetailsLabel          "Order Details">
>    <!ENTITY orderTotalLabel            "Total">
> +  <!ENTITY closeButton.label           "Close me">

Indentation
Attachment #8955411 - Flags: review?(amarchesini) → review+
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review230586

> Without looking too closely at the patch… why would we have a readonly attribute but then effectively have a setter? Why does setCompleteStatus need to be exposed in the IDL?

The attribute should be readonly from JS but needs a C++ setter. I've updated the patch to have [noscript] on the setter since it should only be called from C++.
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review230586

> The attribute should be readonly from JS but needs a C++ setter. I've updated the patch to have [noscript] on the setter since it should only be called from C++.

OK, I think this is not the usual style… the method should be defined in the .h and not in the .idl at all AFAIK. Since you pointed out that `updatePaymentDetails` is there and it also doesn't need to be on the IDL I guess it's fine to pile onto this for now.
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review231396

::: toolkit/components/payments/content/paymentDialogWrapper.js:256
(Diff revision 4)
>    updateRequest() {
>      // There is no need to update this.request since the object is live
>      // and will automatically get updated if event.updateWith is used.
>      let requestSerialized = this._serializeRequest(this.request);
>  
> -    this.mm.sendAsyncMessage("paymentChromeToContent", {
> +    this.sendMessageToContent("updateState", {
> -      messageType: "updateState",
> -      data: {
> -        request: requestSerialized,
> +      request: requestSerialized,
> +    });
> -      },
> +  },
> +

We should re-use `updateRequest` and have the UI code change to use `request.completeStatus`
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review231396

> We should re-use `updateRequest` and have the UI code change to use `request.completeStatus`

Are you suggesting that we also change how state.completionState works? Because completionState uses values not found in the completeStatus enum, and changing to use request.completeStatus would cause us to check state.completionState also (which would now be very confusingly similar names yet different states in the state machine).
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review231396

> Are you suggesting that we also change how state.completionState works? Because completionState uses values not found in the completeStatus enum, and changing to use request.completeStatus would cause us to check state.completionState also (which would now be very confusingly similar names yet different states in the state machine).

Maybe we should just add a comment in the state mixin with the default state to explain the difference of the two completion state things.
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review231428

::: toolkit/components/payments/res/paymentRequest.js:72
(Diff revision 4)
> +      case "completePayment": {
> +        document.querySelector("payment-dialog").requestStore.setState({
> +          changesPrevented: true,
> +          completionState: detail.completeStatus,
> +        });
> +        break;
> +      }
>        case "responseSent": {
>          document.querySelector("payment-dialog").requestStore.setState({
>            changesPrevented: true,
>            completionState: "processing",
>          });
>          break;
>        }

You could dedupe these but I don't know if that helps in the long run.
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review231504

::: toolkit/components/payments/paymentUIService.js:80
(Diff revision 4)
> -    let closed = this.closeDialog(requestId);
> +    let responseCode = dialog ?
> -    let responseCode = closed ?
>          Ci.nsIPaymentActionResponse.COMPLETE_SUCCEEDED :
>          Ci.nsIPaymentActionResponse.COMPLETE_FAILED;
>      let completeResponse = Cc["@mozilla.org/dom/payments/payment-complete-action-response;1"]

Unless I'm missing something `dialog` can never be falsy at this point because of the early return you addded on line 75.

::: toolkit/components/payments/res/paymentRequest.xhtml:98
(Diff revision 4)
>                    data-initial-label="&approvePaymentButton.label;"
>                    data-processing-label="&processingPaymentButton.label;"
>                    data-fail-label="&failPaymentButton.label;"
>                    data-unknown-label="&unknownPaymentButton.label;"
>                    data-success-label="&successPaymentButton.label;"></button>
> +          <button id="closeButton" hidden="hidden">&closeButton.label;</button>

Is this button something that UX/UI said to do? I don't see it in UX/UI specs.
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review231504

> Unless I'm missing something `dialog` can never be falsy at this point because of the early return you addded on line 75.

I've rearranged this to send the response in either case, but still return early if dialog is not found before attempting to clear the timeout and calling dialog.paymentDialogWrapper.completePayment().

> Is this button something that UX/UI said to do? I don't see it in UX/UI specs.

UX did not ask for this, it's not in the specs. However, this part of the spec is stil unspecified. We discussed in Vancouver the dialog should not auto-close when response.complete() is shown, otherwise these various screens done/fail/unknown (excluding timeout->unknown autoclosing) screens would never be seen.

This button is added temporarily until this part of the spec is fleshed out. I'll add a comment explaining it.
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review231504

> UX did not ask for this, it's not in the specs. However, this part of the spec is stil unspecified. We discussed in Vancouver the dialog should not auto-close when response.complete() is shown, otherwise these various screens done/fail/unknown (excluding timeout->unknown autoclosing) screens would never be seen.
> 
> This button is added temporarily until this part of the spec is fleshed out. I'll add a comment explaining it.

I would rather any of the following options:
a) We figure out what UX wants by asking them and heading in that direction
b) Move the close button to the debug panel to avoid churn and complexity on the shipping code
c) auto-close the dialog until we have (a)
d) Have the pay/done/fail button trigger a close when in the final states (that would only be one `if` with a `close` call IIUC) so less to clean up later.

We don't need to worry about confusing UX of options (b) through (d) for now while we're the only ones testing.
Comment on attachment 8955411 [details]
Bug 1441683 - response.complete() should make the completeStatus available to the payment UI service.

https://reviewboard.mozilla.org/r/224590/#review231874

I think this is fine but I don't like the idea of adding code in many files for a temporary close button so please address that.
Attachment #8955411 - Flags: review?(MattN+bmo)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1af9fcd0041c
response.complete() should make the completeStatus available to the payment UI service. r=baku
https://hg.mozilla.org/mozilla-central/rev/1af9fcd0041c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Whiteboard: [webpayments]
You need to log in before you can comment on or make changes to this bug.