The generic merchant error message isn't displayed when it should

VERIFIED FIXED in Firefox 63

Status

()

defect
P1
normal
VERIFIED FIXED
11 months ago
8 months ago

People

(Reporter: hani.yacoub, Assigned: jaws)

Tracking

(Depends on 1 bug)

63 Branch
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 verified)

Details

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

Attachments

(1 attachment)

Reporter

Description

11 months ago
[Affected versions]:
Nightly 63.0a1

[Affected platforms]:
Windows 10 x64, Mac OS X 10.12, Ubuntu 18.04 x64

[Prerequisites]:
- set the pref dom.payments.request.enabled to "true"
- make sure to have at least 2 Shipping Address saved on your profile on from US and one outside US

[Steps to reproduce]:
1. Go to “https://rsolomakhin.github.io/pr/pickup/” or "https://rsolomakhin.github.io/pr/delivery/" or "https://rsolomakhin.github.io/pr/de/" and click on “Buy”.
2. Select a pickup/delivery address from DE.

[Expected Result]:
"Cannot ship outside of US" message should be displayed when DE address is selected for US-only delivery or US-only pickup tests and payment shouldn't be processed.

[Actual Result]: 
"Cannot ship outside of US" message isn't displayed when DE address is selected for US-only delivery or US-only pickup tests and payment will be processed.
Reporter

Updated

11 months ago
Whiteboard: [webpayments] [triage]
Whiteboard: [webpayments] [triage] → [webpayments] [triage] [user-testing]

Updated

11 months ago
Priority: -- → P2
Whiteboard: [webpayments] [triage] [user-testing] → [webpayments] [user-testing]
We'll need to investigate if this is a testcase bug, DOM bug, or front-end.

For the former, Hani, can you confirm if Chrome has a different actual result?
Flags: needinfo?(hani.yacoub)
Summary: "Cannot ship outside of US" message isn't displayed when DE address is selected → The generic merchant error message isn't displayed when it should
Reporter

Comment 2

11 months ago
I can confirm that Chrome has a different actual result, they display "Can't pick up from this address. Select a different address." message after selecting an address outside US.
Flags: needinfo?(hani.yacoub)
I see the following error in the logs because shippingOptions is null but we're using it in a for…of loop. Not sure if that's the cause or not.
> TypeError: "shippingOptions is null"
>  render resource://payments/containers/shipping-option-picker.js:26:1
>  stateChangeCallback resource://payments/mixins/PaymentStateSubscriberMixin.js:104:7
>  setState resource://payments/PaymentsStore.js:70:9 PaymentsStore.js:72:9
Flags: qe-verify+
QA Contact: hani.yacoub
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8996415 [details]
Bug 1477699 - Show a generic error message when a merchant-supplied error message isn't present.

https://reviewboard.mozilla.org/r/260516/#review268138

::: commit-message-90e11:1
(Diff revision 1)
> +Bug 1477699 - Show a generic error message when a merchant-supplied error message isn't present. r?mattn

Could you also address comment 3 if it's not already fixed?

::: browser/components/payments/test/mochitest/test_payment_dialog.html:182
(Diff revision 1)
> +  info(el1._errorText.outerHTML);
> +  info(JSON.stringify(el1.requestStore.getState()));

Nit: I guess you want to remove these
Marcos, the issue seems to be that the merchant pages are returning no shipping options but also no error… should we really allow this from the spec side? I guess it means that the default error message will be properly localized for the browser UI. Jared's solution is to show a generic error message in this case. Thoughts?
Flags: needinfo?(mcaceres)
Assignee

Comment 7

11 months ago
mozreview-review-reply
Comment on attachment 8996415 [details]
Bug 1477699 - Show a generic error message when a merchant-supplied error message isn't present.

https://reviewboard.mozilla.org/r/260516/#review268138

> Could you also address comment 3 if it's not already fixed?

That was fixed by bug 1475521,
https://searchfox.org/mozilla-central/diff/55494981b15dea765a2b0a2c8cb79554b9bddaba/browser/components/payments/res/containers/shipping-option-picker.js#26
Comment hidden (mozreview-request)
Comment on attachment 8996415 [details]
Bug 1477699 - Show a generic error message when a merchant-supplied error message isn't present.

https://reviewboard.mozilla.org/r/260516/#review268296

Thanks

::: browser/components/payments/res/containers/payment-dialog.js:303
(Diff revision 2)
> +    let optionPickerLabel = this._shippingOptionPicker.dataset[shippingType + "OptionsLabel"];
> +    this._shippingOptionPicker.setAttribute("label", optionPickerLabel);

Could you update one of the existing tests to cover this please? There is hopefully one for the shipping Address Picker label already.

::: browser/components/payments/res/containers/payment-dialog.js:318
(Diff revision 2)
> +        (!request.paymentDetails.shippingOptions ||
> +         !request.paymentDetails.shippingOptions.length)) {

Could you file a DOM follow-up to have `request.paymentDetails.shippingOptions` default to an empty array (I think the spec implies they default to an empty sequence on the web-facing side anyways) so that we don't have to keep handling the `null` vs. `[]` case in the front-end.

::: browser/components/payments/res/paymentRequest.xhtml:22
(Diff revision 2)
> +  <!ENTITY shippingGenericError       "Can't ship to this address. Select a different address.">
> +  <!ENTITY deliveryGenericError       "Can't deliver to this address. Select a different address.">
> +  <!ENTITY pickupGenericError         "Can't pick up from this address. Select a different address.">

Can you get these added to the UX or UI specs so that Brian can review them and that QA then knows what to expect. I'm fine with them for now since they're the same as Chrome.
Attachment #8996415 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Assignee

Comment 12

11 months ago
mozreview-review-reply
Comment on attachment 8996415 [details]
Bug 1477699 - Show a generic error message when a merchant-supplied error message isn't present.

https://reviewboard.mozilla.org/r/260516/#review268296

> Could you update one of the existing tests to cover this please? There is hopefully one for the shipping Address Picker label already.

Thanks. I added tests to test_payment_dialog.html
Assignee

Comment 13

11 months ago
mozreview-review-reply
Comment on attachment 8996415 [details]
Bug 1477699 - Show a generic error message when a merchant-supplied error message isn't present.

https://reviewboard.mozilla.org/r/260516/#review268296

> Could you file a DOM follow-up to have `request.paymentDetails.shippingOptions` default to an empty array (I think the spec implies they default to an empty sequence on the web-facing side anyways) so that we don't have to keep handling the `null` vs. `[]` case in the front-end.

I filed bug 1480872.

> Can you get these added to the UX or UI specs so that Brian can review them and that QA then knows what to expect. I'm fine with them for now since they're the same as Chrome.

I commented on the Invision document and described these strings as well as asking for feedback on page 6.3,
https://mozilla.invisionapp.com/share/SDFY4PA4EQ7#/screens/304878928

Comment 14

11 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1cbedbdcb77
Show a generic error message when a merchant-supplied error message isn't present. r=MattN

Comment 15

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b1cbedbdcb77
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #6)
> Marcos, the issue seems to be that the merchant pages are returning no
> shipping options but also no error… should we really allow this from the
> spec side? 

Yes, unfortunately, this was baked into the spec early on.  

> I guess it means that the default error message will be properly
> localized for the browser UI. 

Exactly. This was the intent. 

>Jared's solution is to show a generic error message in this case. Thoughts?

Sounds good.
Flags: needinfo?(mcaceres)

Comment 17

11 months ago
Hi Jared,

I tried to verify the fix on the latest Nightly 63.0a1 (2018-08-06)by checking if there is a generic error message displayed (as described in the Invision doc) when a location outside the US is selected.

The delivery and pick-up strings appear as expected upon selecting a DE address. However, there are 2 issues that appeared with this fix:

[1] Both the "Cannot ship outside the US." and "Can't ship to this address. Select a different address" error strings are displayed, check: https://streamable.com/5mkzl. This occurs for US-only shipping test: https://rsolomakhin.github.io/pr/us/

[2] The Generic error strings are displayed for a second after a US address is selected, check: https://streamable.com/lv6d6
Flags: needinfo?(jaws)
Hi Timea, both [1] and [2] are related in that we don't know when to display the error message. We could wait to display it until after we get an updateWith response, but if there is no updateWith response from the website then we will never show these error messages.

We can investigate this further and see if we should always wait for an updateWith response. Can you please file a bug for this? These two issues are the same so we only need one bug filed. Thanks!
Flags: needinfo?(jaws) → needinfo?(timea.babos)

Comment 19

11 months ago
I filed Bug 1481243 as it was requested for both issues.

Since the Generic merchant error is displayed now as expected and respecting the error strings provided in the Invision doc, I will close the issue as verified - Fixed for all OS.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(timea.babos)
You need to log in before you can comment on or make changes to this bug.