Hani Yacoub, Desktop QA Reporter
	Description • 6 years 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.

	Matthew N. [:MattN]
	Comment 1 • 6 years ago

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?

	Hani Yacoub, Desktop QA Reporter
	Comment 2 • 6 years 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.

	Matthew N. [:MattN]
	Comment 3 • 6 years ago

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

	Matthew N. [:MattN]
	Comment 5 • 6 years ago
mozreview-review

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

	Matthew N. [:MattN]
	Comment 6 • 6 years ago

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?

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 7 • 6 years 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

	Matthew N. [:MattN]
	Comment 10 • 6 years ago
mozreview-review

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.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 12 • 6 years 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

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 13 • 6 years 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

	Pulsebot
	Comment 14 • 6 years 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

	Dorel Luca [:dluca]
	Comment 15 • 6 years ago
bugherder

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

	Marcos Caceres [:marcosc]
	Comment 16 • 6 years ago

(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.

	Timea Cernea [:tbabos][inactive]
	Comment 17 • 6 years 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

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 18 • 6 years ago

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!

	Timea Cernea [:tbabos][inactive]
	Comment 19 • 6 years 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.