CC is saved even when "There was an error saving the payment card." message is displayed

VERIFIED FIXED in Firefox 64

Status

()

P1
normal
VERIFIED FIXED
6 months ago
5 months ago

People

(Reporter: hani.yacoub, Assigned: dpino)

Tracking

({regression})

64 Branch
Firefox 64
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 unaffected, firefox63 unaffected, firefox64 verified)

Details

(Whiteboard: [webpayments-reserve])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 months ago
[Affected versions]: 
Nightly 64.0a1 

[Affected platforms]:
Platforms: Windows 10 x64, Ubuntu 18.04, Mac OS 10.13

[Preconditions]:
1. Set the pref dom.payments.request.enabled to "true";
2. Make sure you are using a clean profile.

[Steps to reproduce]:
1. Go to "https://rsolomakhin.github.io/pr/single/" page and click on "Buy".
2. Fill "Add shipping address" from correctly.
3. Fill "Add Credit Card" form correctly.
4. Close the payment dialog.
5. Go to "about:preferences" -> Privacy & Security -> Forms & Autofill.
6. Click on "Saved Credit Cards.." and delete the CC introduced before.
7. Go back to "https://rsolomakhin.github.io/pr/single/" page and click on "Buy".
8. Fill "Add Credit Card" form correctly. 


[Expected result]:
CC should be saved without any errors.

[Actual result]:
CC is saved even when "There was an error saving the payment card." message is displayed.
(Reporter)

Updated

6 months ago
Flags: qe-verify+
QA Contact: hani.yacoub
Whiteboard: [webpayments] [triage]
Comment hidden (obsolete)
(Reporter)

Comment 2

6 months ago
I found new steps to reproduce this issue:
1. Make sure you are on a FTU flow.
2. Go to "https://rsolomakhin.github.io/pr/single/" page and click on "Buy".
3. Fill "Add shipping address" from correctly.
4. From "Add Credit Card" form click on "Cancel" or on the "X" button.
5. From "https://rsolomakhin.github.io/pr/single/" page and click on "Buy" again.
6. Fill "Add Credit Card" form correctly.
7. Observe the error message displayed.
So if I turned on logging I can see the error is caused by this error

https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/browser/components/payments/res/containers/basic-card-form.js#425,435

It is unclear to me where selectedStateKey is set, and what is supposed to be. Matt, is that something obvious to you? If not I will keep debugging.
Flags: needinfo?(MattN+bmo)
Priority: -- → P1
Whiteboard: [webpayments] [triage] → [webpayments-reserve]
Wait, I've just realized the comment 1 linked to the other thing I am working on, which is totally unrelated to payment.

Timea, would you be able to check the regression range again? I backed out the bugs (and their dependencies) locally and I can still reproduce this bug.

(Leaving unassigned for now)

@  489509:31af1457befc timdream tip Backed out changeset 50d1b7c5b1a1
o  489508:ba8caabbafce timdream Backed out changeset d8dca67e2440
o  489507:578b0fd3e5d0 timdream Backed out changeset 4392b5198fb7
o  489506:a979b65806ad timdream Backed out changeset a632efc6cac4
o  489505:96549b50926d timdream Backed out changeset 039c4b2029a4
o    489504:20773596530b apavel central Backed out changeset 39f0d4e66898 (bug 675428) for causing Bug 1496380 a=backout
Assignee: timdream → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(MattN+bmo) → needinfo?(timea.babos)
Keywords: regressionwindow-wanted
Priority: P1 → P3
Whiteboard: [webpayments-reserve] → [webpayments] [triage]

Comment 6

6 months ago
Hi Tim,

Thanks for checking it on your side! I checked it again with mozregression using the steps from Comment 2 (makes it easier to repro) and you were right. I found the issue that is actually Payments related, makes sense now. Sorry for the inconvenience created before :(


Last good revision: 39762ef5d56e07f8046bf9dfb1b32f194c6ea1bd
First bad revision: 4ff0fcd61c7a1f791414516a6e22efca94530f19

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=39762ef5d56e07f8046bf9dfb1b32f194c6ea1bd&tochange=4ff0fcd61c7a1f791414516a6e22efca94530f19

Bug 1490805 - Add the CVV security code field to the add card form and make it required in all places 

Matt, can you please take a look at this?
Flags: needinfo?(timea.babos) → needinfo?(MattN+bmo)
Keywords: regressionwindow-wanted
(Assignee)

Comment 7

5 months ago
I took a look at this bug.

As pointed out in the bug description, the error happens at:

https://hg.mozilla.org/integration/autoland/rev/4ff0fcd61c7a#l3.71

The first time the basic credit card form is shown, selectedStateKey is initialized to 'selectedPaymentCard'. The initialization happens at:

https://hg.mozilla.org/integration/autoland/rev/4ff0fcd61c7a#l2.12

The second time, since an address already exists, selectedStateKey is not initialized. In fact, its value is initialized to null at:

https://hg.mozilla.org/integration/autoland/rev/4ff0fcd61c7a#l5.12

And since its value is null an Error is thrown (https://hg.mozilla.org/integration/autoland/rev/4ff0fcd61c7a#l3.71).

If basicCardPage.selectedStateKey is initialized to 'selectedPaymentCard' in PaymentStateSubscriberMixin.js the error doesn't happen. That's what the attached patch does to avoid the bug. But I'm not sure that's the right fix.
Attachment #9017523 - Flags: review?(MattN+bmo)
Assignee: nobody → dpino
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [webpayments] [triage] → [webpayments-reserve]
Comment on attachment 9017523 [details] [diff] [review]
Bug-1497514-Initialize-BasicCardPage-selectedStateKe.patch

Review of attachment 9017523 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/payments/res/mixins/PaymentStateSubscriberMixin.js
@@ +16,5 @@
>    orderDetailsShowing: false,
>    "basic-card-page": {
>      guid: null,
>      // preserveFieldValues: true,
> +    selectedStateKey: "selectPaymentCard",

Can you find the place where the page.id is set to "basic-card-page" and set this there instead?
Attachment #9017523 - Flags: review?(MattN+bmo)
(Assignee)

Comment 9

5 months ago
Updated patch.
Attachment #9017523 - Attachment is obsolete: true
Attachment #9017822 - Flags: review?(MattN+bmo)
Comment on attachment 9017822 [details] [diff] [review]
Bug-1497514-Initialize-BasicCardPage-selectedStateKe.patch

Review of attachment 9017822 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! That's what I was looking for. Now can you add the following after the page.id check[1] in basic-card-form to prevent this from happening again by causing test failures:

```js
if (!basicCardPage.selectedStateKey) {
  throw new Error("A `selectedStateKey` is required");
}
```

Then run all tests and ensure they pass or modify then to add a selectedStateKey: 
> ./mach test browser/components/payments/

[1] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/browser/components/payments/res/containers/basic-card-form.js#138-142
Attachment #9017822 - Flags: review?(MattN+bmo) → feedback+
(Assignee)

Comment 11

5 months ago
I've added a new patch adding the check suggested to `basic-card-form.js`. After that, I run the tests and there was one set of tests that reported several errors. All the fails where in one single test: `test/mochitest/test_basic_card_form.html`.

I tried to fix the tests by adding a `selectedStateKey: "selectedPaymentCard"` to all `basic-card-page` state objects defined (or defining it if necessary). Now the tests don't fail due to the extra check added in `basic-card-form.js`, but report an error in function `checkCCForm` (https://searchfox.org/mozilla-central/source/browser/components/payments/test/mochitest/test_basic_card_form.html#47).

It seems that when the function tries to fetch a property from the document, that property doesn't exist. I tried to fix the issue, but no success yet.
Attachment #9018197 - Flags: feedback?(MattN+bmo)
Comment on attachment 9018197 [details] [diff] [review]
Bug-1497514-Check-basicCardPage-has-a-selectedStateK.patch

Review of attachment 9018197 [details] [diff] [review]:
-----------------------------------------------------------------

I changed my mind that it's fine to change the default value as long as we have the new `throw`.
Attachment #9018197 - Flags: feedback?(MattN+bmo) → review+

Comment 13

5 months ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3b53341da2
Initialize BasicCardPage selectedStateKey to 'selectPaymentCard'. r=MattN
It looks like the failure is only with QuantumRender… Diego, can you import https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3b53341da2 and debug the failure using Try (or locally if you can run Quantum Render)? There is probably an existing race in the test.
Flags: needinfo?(hani.yacoub) → needinfo?(dpino)
(Assignee)

Comment 16

5 months ago
I imported the patch and run the test. It seems some tests that were using "basic-card-page" needed to be initialized with "selectedStateKey: selectedPaymentCard". After that change, the test passed.
Attachment #9017822 - Attachment is obsolete: true
Attachment #9018197 - Attachment is obsolete: true
Flags: needinfo?(dpino)
Attachment #9018523 - Flags: review?(MattN+bmo)
Comment on attachment 9018523 [details] [diff] [review]
Bug-1497514-Initialize-BasicCardPage-selectedStateKe.patch

Review of attachment 9018523 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, yeah, of course. Thanks!
Attachment #9018523 - Flags: review?(MattN+bmo) → review+
Are you able to push to try server?
(Assignee)

Updated

5 months ago
Keywords: checkin-needed

Comment 20

5 months ago
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25594c3013c4
Initialize BasicCardPage selectedStateKey to 'selectPaymentCard'. r=MattN
Keywords: checkin-needed

Comment 21

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/25594c3013c4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox64: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Blocks: 1493525
status-firefox62: --- → unaffected
status-firefox63: --- → unaffected
status-firefox-esr60: --- → unaffected
Flags: in-testsuite+
(Reporter)

Comment 22

5 months ago
Verified as fixed on Firefox Nightly 64.0a1 (2018-10-21) on Windows 10 x 64, Windows 7 x32, Mac OS X 10.13 and on Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox64: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.