Closed Bug 1477699 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: WebPayments UI, defect, P1)

63 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox63 --- verified

People

(Reporter: hani.yacoub, Assigned: jaws)

References

(Depends on 1 open bug, )

Details

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

Attachments

(1 file)

[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.
Whiteboard: [webpayments] [triage]
Whiteboard: [webpayments] [triage] → [webpayments] [triage] [user-testing]
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
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)
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
Duplicate of this bug: 1477534
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 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
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
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
https://hg.mozilla.org/mozilla-central/rev/b1cbedbdcb77
Status: ASSIGNED → RESOLVED
Closed: 4 years 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)
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)
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)
Depends on: 1481243
You need to log in before you can comment on or make changes to this bug.