Closed Bug 1447777 Opened 6 years ago Closed 6 years ago

Handle the "timeout" completeStatus that is set when PaymentResponse.complete is not called

Categories

(Firefox :: WebPayments UI, enhancement, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- verified

People

(Reporter: jaws, Assigned: sfoster)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webpayments])

Attachments

(4 files, 3 obsolete files)

completeTimeout.html
3.95 KB, text/html
Details
Bug 1447777 - Move state.completionState to state.request.completeStatus.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1447777 - Add completion fail and timeout error pages.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Error Screen
12.67 KB, image/png
Details 
Bug 1447773 will implement functionality to timeout the PaymentResponse after some amount of time when .complete() is not called.

This should get handled the same as if .complete() is called with no arguments (this would be "unknown"), but we will have a unique completeStatus of "timeout" so the front-end can potentially react differently or at least store telemetry results to know how often this API times out.
Whiteboard: [webpayments]
Priority: -- → P2
Product: Toolkit → Firefox
Version: unspecified → Trunk
Depends on: 1447773
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Priority: P2 → P1
I've pushed a WIP of this patch, see commit message for details. The patch covers both the 'fail' and 'timeout' case as they are handled very similarly. 

Once we have retry, we may end up with quite different handling of these error cases. So I'll probably remove the 'cancel' buttons and make it clear this is for non-retry scenarios only. 

I need a consult on how to hook up the 'done' (OK) buttons; the obvious content.close() from the frame script didn't work, so either I'm missing something trivial or this needs a little thought.
Attached file completeTimeout.html — 
I've been using this test page for testing 'timeout' and 'fail' outcomes.
Comment on attachment 8988960 [details]
Bug 1447777 - Add completion fail and timeout error pages.

https://reviewboard.mozilla.org/r/254072/#review262060

Overall this looks on the right track

::: browser/components/payments/content/paymentDialogFrameScript.js:98
(Diff revision 2)
> +    if (messageType == "closeCompleted") {
> +      this.log.debug("closeCompleted received");
> +      // Doesnt work...
> +      content.close();
> +      // paymentUIService.js does have a helper to close the dialog, but I'm not able to use it
> +      // as it is not a part of the nsIPaymentUIService interface I guess
> +      // The request is complete()'d at this point, so calling paymentSrv.abortPayment()
> +      // isn't the right thing either
> +      return;
> +    }
>      this.log.debug("sendToChrome:", messageType, detail);
>      this.sendMessageToChrome(messageType, detail);

Clearing the flag since we talked about this the other day and we can do the window.close in the wrapper instead using the `sendMessageToChrome`
Attachment #8988960 - Flags: review?(MattN+bmo)
Comment on attachment 8988960 [details]
Bug 1447777 - Add completion fail and timeout error pages.

https://reviewboard.mozilla.org/r/254072/#review263812

::: browser/components/payments/res/debugging.js:419
(Diff revision 4)
>      requestStore.setState({
> +      changesPrevented: true,
>        completionState: "processing",
>      });

There is a separate "Prevent changes" button that does this and the idea that was that you could test them separately. I've been meaning to change the completionState debugging buttons to be radio buttons and the changesPrevented buttons to be a single checkbox which I think would be easier to digest when looking at the panel.
Comment on attachment 8988960 [details]
Bug 1447777 - Add completion fail and timeout error pages.

https://reviewboard.mozilla.org/r/254072/#review263816

::: browser/components/payments/paymentUIService.js:71
(Diff revision 4)
>      this.log.debug("completePayment:", requestId);
> -    let closed = this.closeDialog(requestId);
> +    let {completeStatus = ""} = paymentSrv.getPaymentRequestById(requestId);
> +    let closed;
> +    this.log.debug("completeStatus:", completeStatus);

I would combine the two log.debug calls into one after the `completeStatus` is retrieved.

Nit: I would move the `closed` definition right above the `switch` to make it closer to the usage.

::: browser/components/payments/paymentUIService.js:72
(Diff revision 4)
>      paymentSrv.respondPayment(abortResponse);
>    },
>  
>    completePayment(requestId) {
>      this.log.debug("completePayment:", requestId);
> -    let closed = this.closeDialog(requestId);
> +    let {completeStatus = ""} = paymentSrv.getPaymentRequestById(requestId);

I don't think we should provide a default value as it will hide bugs if it becomes `undefined`. The DOM code should provide the default IMO.

::: browser/components/payments/paymentUIService.js:83
(Diff revision 4)
> +    }
> +    // Should be one of "timeout", "success", "fail", ""

Is this comment supposed to go above the `switch`?

::: browser/components/payments/res/containers/completion-error-page.js:17
(Diff revision 4)
> +export default class CompletionErrorPage extends PaymentStateSubscriberMixin(HTMLElement) {
> +  constructor() {

Any particular reason not to subclass PaymentRequestPage?

::: browser/components/payments/res/containers/error-page.css:6
(Diff revision 4)
> + padding-inline-start: 38%;
> +}

Nit: indentation

::: browser/components/payments/res/containers/payment-dialog.js:326
(Diff revision 4)
>      this._disabledOverlay.hidden = !changesPrevented;
>    }
> +
> +  toggleHeader(hide) {
> +    if (typeof hide == "undefined") {
> +      hide  = !this._header.hidden;

Nit: extra space

::: browser/components/payments/res/paymentRequest.js:187
(Diff revision 4)
> +  closeCompleted() {
> +    this.sendMessageToChrome("closeCompleted");
> +  },

I think it would be more clear to call the method and the message name `closeDialog` as I found `closeCompleted` confusing about what it's going to do.
Comment on attachment 8988960 [details]
Bug 1447777 - Add completion fail and timeout error pages.

https://reviewboard.mozilla.org/r/254072/#review263812

> There is a separate "Prevent changes" button that does this and the idea that was that you could test them separately. I've been meaning to change the completionState debugging buttons to be radio buttons and the changesPrevented buttons to be a single checkbox which I think would be easier to digest when looking at the panel.

Yeah I can do something with this. But at this point "processing" is the only state that needs changesPrevented, and it doesnt really make sense to set completionState: "processessing" without changesPrevented. I'll try keeping this one a seperate button and the radio for the other states. 

Similarly, there should be no time when we have completionState="timeout" or "fail" and we don't also set the page.id to the corresponding page. And it would make no sense to have e.g. completionState="fail" on page.id="payment-summary"
Comment on attachment 8988960 [details]
Bug 1447777 - Add completion fail and timeout error pages.

https://reviewboard.mozilla.org/r/254072/#review264910

Sorry for the delay… I had some concerns that I needed to set time aside to think through more and got busy.

::: browser/components/payments/content/paymentDialogWrapper.js:366
(Diff revision 5)
>    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.sendMessageToContent("updateState", {
>        request: requestSerialized,
>      });
>    },
>  
> +  updateCompleteStatus(completeStatus) {
> +    this.sendMessageToContent("paymentComplete", {
> +      completeStatus,
> +    });
> +  },

I don't like that we're not updating the whole request object which already has a `completeStatus` property. When we have errors from retry we'd likely need to update the whole request like `updateRequest` does

::: browser/components/payments/paymentUIService.js:72
(Diff revision 5)
>      paymentSrv.respondPayment(abortResponse);
>    },
>  
>    completePayment(requestId) {
> -    this.log.debug("completePayment:", requestId);
> -    let closed = this.closeDialog(requestId);
> +    // completeStatus should be one of "timeout", "success", "fail", ""
> +    let {completeStatus} = paymentSrv.getPaymentRequestById(requestId);

Side note: Can you document `completeStatus` in the default object inside the PaymentStateSubscriberMixin… I think we also need to clearly document the interaction between `state.request.completeStatus` and `state.completionState` there. I think `completionState` simply pre-dates `completeStatus` so we may be able to consolidate on `completeStatus` and remove `completionState`?

Even for "processing" which doesn't come from the DOM… if we set that ourselves overwriting (request.completeStatus which is not the cleanest), then that's probably fine since it will get properly overwritten when the next DOM response happens.

::: browser/components/payments/paymentUIService.js:77
(Diff revision 5)
> +      case "fail":
> +        break;
> +      case "timeout":

Nit: no need to break between these two

::: browser/components/payments/res/containers/completion-error-page.js:36
(Diff revision 5)
> +    if (this.id && page && page.id !== this.id) {
> +      log.debug(`CompletionErrorPage: no need to further render inactive page: ${page.id}`);
> +      return;
> +    }

Side note (not necessarily for this bug or commit): I wonder if we could have this done in `PaymentRequestPage` instead e.g. by having page subclasses implement `renderPage(state)` instead or something.

::: browser/components/payments/res/containers/completion-error-page.js:44
(Diff revision 5)
> +    for (let child of this.suggestionsList.childNodes) {
> +      this.suggestionsList.removeChild(child);
> +    }

this.suggestionsList.textContent = "";

::: browser/components/payments/res/containers/completion-error-page.js:53
(Diff revision 5)
> +      let textNode = document.createTextNode(suggestionText);
> +      let listNode = document.createElement("li");
> +      listNode.appendChild(textNode);

`listNode.textContent = suggestionText;` is more natural than `createTextNode` IMO

::: browser/components/payments/res/containers/payment-dialog.js:324
(Diff revision 5)
> +  toggleHeader(hide) {
> +    if (typeof hide == "undefined") {
> +      hide = !this._header.hidden;
> +    }
> +    this._header.hidden = hide;
> +  }

What is this helper solving?

::: browser/components/payments/res/paymentRequest.js:97
(Diff revision 5)
> +        let {completeStatus} = detail;
> +        let state = {
> +          changesPrevented: false,
> +          completionState: completeStatus,
> +        };
> +        if (completeStatus == "fail" || completeStatus == "timeout") {
> +          state.page = {
> +            id: `completion-${completeStatus}-error`,
> +          };
> +        }
> +        document.querySelector("payment-dialog").requestStore.setState(state);
> +        break;
> +      }

If we just used `updateState` like I mentioned elsewhere, then this could be done in `setStateFromParent` (or maybe a helper called from there) probably though I think changesPrevented shouldn't always be set to false in that case but that should be easy to handle.

::: browser/components/payments/res/paymentRequest.xhtml:175
(Diff revision 5)
> +      <completion-error-page id="completion-timeout-error" class="page error-page illustrated"
> +                  data-page-title="&timeoutErrorPage.title;"
> +                  data-done-button-label="&timeoutErrorPage.doneButton.label;"
> +                  hidden="hidden"></completion-error-page>
> +      <completion-error-page id="completion-fail-error" class="page error-page illustrated"

Nit: the `page` class gets added automatically by the constructor and the error-page class should probably be done the same way and neither listed here.
Attachment #8988960 - Flags: review?(MattN+bmo)
Comment on attachment 8992023 [details]
Bug 1447777 - Add tests for the complete() scenarios.

https://reviewboard.mozilla.org/r/256920/#review264926

I still have to review the `test_*` files… the rest I reviewed fully.

::: browser/components/payments/test/browser/head.js:494
(Diff revision 3)
> +async function clickPrimaryButton(frame) {
> +  await spawnPaymentDialogTask(frame, async () => {
> +    let {
> +      PaymentTestUtils,
> +    } = ChromeUtils.import("resource://testing-common/PaymentTestUtils.jsm", {});
> +
> +    let {page}  = await PaymentTestUtils.DialogContentUtils.getCurrentState(content);
> +    let button = content.document.querySelector(`#${page.id} button.primary`);
> +    button.click();
> +  });
> +}

I think this is better suited for PTU.DialogContentTasks until we decide to remove that since we're no longer using them from outside m-bc.

::: browser/components/payments/test/mochitest/test_payment_dialog.html:36
(Diff revision 3)
> -let el1;
> +function isVisible(elem) {
> +  let result = elem.getBoundingClientRect().height > 0;
> +  return result;
> +}

I know you copied this helper but it was a mistake to have it since we already have `isHidden` (which you can negate) if you add the EventUtils <script>: https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/testing/mochitest/tests/SimpleTest/EventUtils.js#202
Comment on attachment 8988960 [details]
Bug 1447777 - Add completion fail and timeout error pages.

https://reviewboard.mozilla.org/r/254072/#review264910

> Side note: Can you document `completeStatus` in the default object inside the PaymentStateSubscriberMixin… I think we also need to clearly document the interaction between `state.request.completeStatus` and `state.completionState` there. I think `completionState` simply pre-dates `completeStatus` so we may be able to consolidate on `completeStatus` and remove `completionState`?
> 
> Even for "processing" which doesn't come from the DOM… if we set that ourselves overwriting (request.completeStatus which is not the cleanest), then that's probably fine since it will get properly overwritten when the next DOM response happens.

Yeah I think I agree. There isn't a useful distinction between request.completeStatus and state.completionState, and introducing a "processing" transition state which is simply overwritten and handled as a state update should work well. request.completeStatus isnt part of the PaymentRequest interface as defined by the spec so I'm not too worried about our book-keeping introducing confusion here.
Comment on attachment 8988960 [details]
Bug 1447777 - Add completion fail and timeout error pages.

https://reviewboard.mozilla.org/r/254072/#review263816

> Any particular reason not to subclass PaymentRequestPage?

I dont think it existed when I started this patch. I've ported it over.

> I think it would be more clear to call the method and the message name `closeDialog` as I found `closeCompleted` confusing about what it's going to do.

I went with closeDialog for the mesage name and paymentRequest method, but onCloseDialogMessage to avoid confusion around when this method is called - when the message is received, rather than when the dialog is closed.
Attachment #8992023 - Attachment is obsolete: true
Attachment #8992023 - Flags: review?(MattN+bmo)
I broke out the completionState to request.completeStatus change into its own patch to make it a bit easier to review. I'll probably fold together the 2nd and 3rd patches before landing as there are test failures in step 2 which are addressed in step 3.
Attachment #8993756 - Attachment is obsolete: true
Attachment #8993756 - Flags: review?(MattN+bmo)
Comment on attachment 8993593 [details]
Bug 1447777 - Move state.completionState to state.request.completeStatus.

https://reviewboard.mozilla.org/r/258292/#review265492

Mutating state directly means that we're not really testing the setState path

::: browser/components/payments/res/debugging.js:410
(Diff revision 1)
> -    requestStore.setState({
> -      completionState: "initial",
> +    let {request} = requestStore.getState();
> +    request.completeStatus = "initial";

You should really be using a clone for all of these to avoid directly mutatating `request`:
```js
let request = Object.assign({}, requestStore.getState().request, {
  completeStatus: "initial",
});
requestStore.setState({ request });
```

::: browser/components/payments/test/mochitest/test_payer_address_picker.html:137
(Diff revision 1)
>      changesPrevented: false,
> -    completionState: "initial",
> +    request: Object.assign(request, { completeStatus: "initial" }),
>      orderDetailsShowing: false,

This is also directly mutating `request` so add `{}` as the first argument to Object.assign here to clone `request` first.

::: browser/components/payments/test/mochitest/test_payment_dialog.html:74
(Diff revision 1)
>      changesPrevented: false,
> -    completionState: "initial",
> +    request: Object.assign(request, {completeStatus: "initial"}),
>      orderDetailsShowing: false,

Same issue here and elsewhere.
Attachment #8993593 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8988960 [details]
Bug 1447777 - Add completion fail and timeout error pages.

https://reviewboard.mozilla.org/r/254072/#review265532

Thanks!

::: browser/components/payments/res/containers/completion-error-page.js:14
(Diff revision 8)
> +/* import-globals-from ../unprivileged-fallbacks.js */
> +
> +/**
> + * <completion-error-page></completion-error-page>
> + *
> + * XXX: Bug xxxxx - This page isn't fully localized when used via this custom element

I guess the bug number can now be 1473772? I've been adding issues to the user story there if they don't have their own bug.

::: browser/components/payments/res/containers/error-page.css:9
(Diff revision 8)
> +  background-repeat: no-repeat;
> +  background-size: 38%;
> +  padding-inline-start: 38%;
> +}
> +
> +[dir="rtl"] .error-page.illustrated > .page-body {

I'm not too familiar with uring @dir in html but wouldn't it be more performant to use the :dir pseudo-selector instead: https://developer.mozilla.org/en-US/docs/Web/CSS/:dir?

```css
.error-page.illustrated > .page-body:dir(rtl) {
  …
}
```

::: browser/components/payments/res/containers/error-page.css:19
(Diff revision 8)
> +.error-page#completion-timeout-error .page-body,
> +.error-page#completion-fail-error .page-body {

I guess you can use a child selector here like you do  on line 1?

::: browser/components/payments/res/containers/payment-dialog.js:131
(Diff revision 8)
> +      case "unknown":
> +        state.page = {
> +          id: `completion-${completeStatus}-error`,
> +        };
> +        state.changesPrevented = false;
> +        break;
> +    }

Nit: Putting braces around the `case`s is preferred to support block scoped variables in a future change without changing blame.

::: browser/components/payments/res/debugging.js:412
(Diff revision 8)
>      let {request} = requestStore.getState();
> -    request.completeStatus = "initial";
> -    requestStore.setState({ request });
> +    requestStore.setStateFromParent({
> +      request: Object.assign(request, { completeStatus }),

Please clone here (already mentioned in the last commit but repeating just in case)
Attachment #8988960 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8993762 [details]
Bug 1447777 - Add completion fail and timeout error pages.

https://reviewboard.mozilla.org/r/258462/#review265538

::: browser/components/payments/test/PaymentTestUtils.jsm:169
(Diff revision 1)
> +     * Don't await on this task since the button can close the dialog before
> +     * ContentTask can resolve the promise.

I think consumers *should* await on this task *unless* the dialog will close to avoid races.

::: browser/components/payments/test/PaymentTestUtils.jsm:174
(Diff revision 1)
> +    clickPrimaryButton: () => {
> +      let {requestStore} = Cu.waiveXrays(content.document.querySelector("payment-dialog"));

This is a great helper btw.

::: browser/components/payments/test/PaymentTestUtils.jsm:176
(Diff revision 1)
> +     *
> +     * @returns {undefined}
> +     */
> +    clickPrimaryButton: () => {
> +      let {requestStore} = Cu.waiveXrays(content.document.querySelector("payment-dialog"));
> +      let {page}  = requestStore.getState();

Nit: extra space… we really should have an eslint rule for that…

::: browser/components/payments/test/PaymentTestUtils.jsm:223
(Diff revision 1)
>  
> -    getCurrentState: async (content) => {
> +    getCurrentState: (content) => {
>        let {requestStore} = Cu.waiveXrays(content.document.querySelector("payment-dialog"));

What's this change for? It may slightly change the timing of things (not sure if for the better or worse) but I'd like to know more.

::: browser/components/payments/test/browser/browser_payment_completion.js:68
(Diff revision 1)
> +    let {completeException} = await ContentTask.spawn(browser,
> +                                                      { result: "fail" },
> +                                                      PTU.ContentTasks.addCompletionHandler);
> +    ok(!completeException, "Expect no exception to be thrown when calling complete()");
> +
> +    ok(!win.closed, "dialog shouldnt be closed yet");

Nit: shouldn't (missing apostrophe) and same issue below

::: browser/components/payments/test/browser/browser_payment_completion.js:92
(Diff revision 1)
> +    info("clicking pay");
> +    await spawnPaymentDialogTask(frame, PTU.DialogContentTasks.completePayment);
> +
> +    info("acknowledging the completion from the merchant page after a delay");
> +    let {completeException} = await ContentTask.spawn(browser,
> +                                                      { result: "fail", delayMs: 1000 },
> +                                                      PTU.ContentTasks.addCompletionHandler);
> +    ok(completeException,
> +       "Expect an exception to be thrown when calling complete() too late");

It would be good to test that the dialog isn't closed before the timeout (and use a larger pref value for that obviously) but maybe we already have a test for that?
Attachment #8993762 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8993762 [details]
Bug 1447777 - Add completion fail and timeout error pages.

https://reviewboard.mozilla.org/r/258462/#review265538

> What's this change for? It may slightly change the timing of things (not sure if for the better or worse) but I'd like to know more.

I changed it in an earlier version of this patch and didnt revert it. The thought at the time was that getCurrentState isn't actually async, but regardless I didn't intend to commit the change.
Blocks: 1477450
Comment on attachment 8993762 [details]
Bug 1447777 - Add completion fail and timeout error pages.

https://reviewboard.mozilla.org/r/258462/#review265538

> It would be good to test that the dialog isn't closed before the timeout (and use a larger pref value for that obviously) but maybe we already have a test for that?

I've filed bug https://bugzilla.mozilla.org/show_bug.cgi?id=1477450 for this. I hope that summary captures your concern, please add/edit if necessary.
Attachment #8988960 - Attachment is obsolete: true
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 2e2e30120b3f052c1a61250f6531d32c4b825a60 -d 08339a56f3ae: rebasing 473803:2e2e30120b3f "Bug 1447777 - Move state.completionState to state.request.completeStatus. r=MattN"
merging browser/components/payments/res/containers/payment-dialog.js
merging browser/components/payments/res/paymentRequest.css
merging browser/components/payments/test/mochitest/test_payment_dialog.html
rebasing 473804:ec77d2aa342f "Bug 1447777 - Add completion fail and timeout error pages. r=MattN" (tip)
merging browser/components/payments/content/paymentDialogWrapper.js
merging browser/components/payments/res/containers/payment-dialog.js
merging browser/components/payments/res/paymentRequest.xhtml
merging browser/components/payments/test/mochitest/test_payment_dialog.html
warning: conflicts while merging browser/components/payments/res/paymentRequest.xhtml! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4ecdfcd8f9f
Move state.completionState to state.request.completeStatus. r=MattN
https://hg.mozilla.org/integration/autoland/rev/9daa53881b7a
Add completion fail and timeout error pages. r=MattN
https://hg.mozilla.org/mozilla-central/rev/b4ecdfcd8f9f
https://hg.mozilla.org/mozilla-central/rev/9daa53881b7a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Flags: qe-verify+
QA Contact: hani.yacoub
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

Verified fixed on the latest Nightly version 63.0a1 (2018-07-30) on Windows 10, Mac 10.13, Ubuntu 16.04.
Both the timeout and the fail cases have error screens with their specific message.

Note that I only checked if the screens appear, is there a Bug submitted that handles the design of the error screens (suggestions, image - as it can be seen in the design doc)? 

Check the attached screenshot for reference.
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
Flags: qe-verify+ → needinfo?(sfoster)
(In reply to Timea Babos from comment #40)
> 
> Note that I only checked if the screens appear, is there a Bug submitted
> that handles the design of the error screens (suggestions, image - as it can
> be seen in the design doc)? 

Yeah, that would be bug 1470207
Blocks: 1470207
Flags: needinfo?(sfoster)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: