1429189 - Show shipping address errors on the summary screen

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

If an error with the shipping address happens and it's isn't a result of a shippingaddresschange event (from the user editing it) then we need to indicate the error on the summary view (rather than preventing the user from leaving the edit screen).

Example:
1) Have only a partial/invalid shipping address in autofill storage
2) Open a Payment Request
3) Address from #1 is chosen by default
4) Don't change the shipping address but complete the request

I don't believe shippingaddresschange should be fired in the above steps but upon payment time the site will get the shipping address and may reject it.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1429189 - Show shipping address errors on the summary screen.

Review commit: https://reviewboard.mozilla.org/r/222562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222562/

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8953281 [details]
Bug 1429189 - Show shipping address errors on the summary screen.

https://reviewboard.mozilla.org/r/222562/#review228452


Code analysis found 2 defects in this patch:
 - 2 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/PaymentTestUtils.jsm:42
(Diff revision 1)
>      },
>  
> +    updateWith: async ({eventName, details}) => {
> +      if (details.error &&
> +          (!details.shippingOptions || details.shippingOptions.length)) {
> +        ok(false, "Need to clear the shipping options to show error text");

Error: 'ok' is not defined. [eslint: no-undef]

::: toolkit/components/payments/test/PaymentTestUtils.jsm:45
(Diff revision 1)
> +      if (details.error &&
> +          (!details.shippingOptions || details.shippingOptions.length)) {
> +        ok(false, "Need to clear the shipping options to show error text");
> +      }
> +      if (!details.total) {
> +        ok(false, "`total: { label, amount: { value, currency } }` is required for updateWith");

Error: 'ok' is not defined. [eslint: no-undef]

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8953281 [details]
Bug 1429189 - Show shipping address errors on the summary screen.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222562/diff/1-2/

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8953281 [details]
Bug 1429189 - Show shipping address errors on the summary screen.

https://reviewboard.mozilla.org/r/222562/#review228506

::: toolkit/components/payments/content/paymentDialogWrapper.js:253
(Diff revision 2)
>        data,
>        messageType,
>      });
>    },
>  
> +  onDetailsUpdated() {

Nit: Since this isn't a listener and isn't just about the PaymentRequest `details`, I think `updateRequest` may be a clearer name.

::: toolkit/components/payments/content/paymentDialogWrapper.js:253
(Diff revision 2)
> +  onDetailsUpdated() {
> +    let requestSerialized = this._serializeRequest(this.request);

I am surprised this works without doing
```js
this.request = paymentSrv.getPaymentRequestById(requestId);
```

Please double-check this. If the request is live we should probably have a comment about that and you mentioned we could make it a `const`

::: toolkit/components/payments/paymentUIService.js:85
(Diff revision 2)
> +    if (!dialog) {
> +      this.log.debug("updatePayment: no dialog found");
> +      return;
> +    }

Not sure if we should bother trying to mask this potential error as it may hide bugs. At a minimum this should be a log.warn or probably better as log.error so it logs by default as it will help with bug reports.

::: toolkit/components/payments/paymentUIService.js:100
(Diff revision 2)
> +    if (win) {
> +      this.log.debug(`closing: ${win.name}`);
> +      win.close();
> +      return true;
> +    }
> +    return false;

Nit: ```js
if (!win) {
  return false;
}
…
```

::: toolkit/components/payments/res/paymentRequest.xhtml:74
(Diff revision 2)
> +          <div><label id="error-text"></label></div>
>          </section>

I think this is better at the top of the page or in the footer

::: toolkit/components/payments/res/paymentRequest.xhtml:74
(Diff revision 2)
>            <address-picker class="shippingRequested" selected-state-key="selectedShippingAddress"></address-picker>
>            <div class="shippingRequested"><label>&shippingOptionsLabel;</label></div>
>            <shipping-option-picker class="shippingRequested"></shipping-option-picker>
>            <div><label>&paymentMethodsLabel;</label></div>
>            <payment-method-picker selected-state-key="selectedPaymentCard"></payment-method-picker>
> +          <div><label id="error-text"></label></div>

Why use a label if it's not labelling a control? And why not set the text content of the div directly without a wrapper?

::: toolkit/components/payments/test/browser/browser_shippingaddresschange_error.js:55
(Diff revision 2)
> +    await ContentTask.spawn(browser, {
> +      eventName: "shippingoptionchange",
> +    }, PTU.ContentTasks.awaitPaymentRequestEventPromise);
> +
> +    let errors = await spawnPaymentDialogTask(frame, PTU.DialogContentTasks.getErrorDetails);
> +    is(errors.text, EXPECTED_ERROR_TEXT, "Error text should be present on dialog");

Can you check that it's visible?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b281723b73dd
Show shipping address errors on the summary screen. r=mattn

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b281723b73dd