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

Status: VERIFIED FIXED

(Firefox :: WebPayments UI, defect, P1)

Description

[Hani Yacoub, Desktop QA]

Attachment: ice_video_20181009-163335.webm

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

Comment 1

[Timea Cernea [:tbabos][inactive]]

[Regression - Range]: 

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

Last good revision: 4ff0fcd61c7a1f791414516a6e22efca94530f19
First bad revision: d8dca67e244037cefcee992c48b1ef1f0fc7293c


Also retested the issue manually using the two build for Bug 1493525:

First release with: nightly win64 / be037b5b6327 / 64.0a1 / 20180927220034  [Issue occurs]
Last release without: nightly win64 / ba2b3ed1eb96 / 64.0a1 / 20180927100044 [Issue does not occur] 

Tim, can you please take a look at this?

Comment 2

[Hani Yacoub, Desktop QA]

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.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Thanks for finding the regression.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 6

[Timea Cernea [:tbabos][inactive]]

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?

Comment 7

[Diego Pino]

Attachment: Bug-1497514-Initialize-BasicCardPage-selectedStateKe.patch

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.

Comment 8

[Matthew N. [:MattN]]

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?

Comment 9

[Diego Pino]

Attachment: Bug-1497514-Initialize-BasicCardPage-selectedStateKe.patch

Updated patch.

Comment 10

[Matthew N. [:MattN]]

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

Comment 11

[Diego Pino]

Attachment: Bug-1497514-Check-basicCardPage-has-a-selectedStateK.patch

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.

Comment 12

[Matthew N. [:MattN]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3b53341da2
Initialize BasicCardPage selectedStateKey to 'selectPaymentCard'. r=MattN

Comment 14

[Noemi Erli[:noemi_erli]]

Backed out changeset 8b3b53341da2 (Bug 1497514) for failures in browser/components/payments/test/mochitest/test_basic_card_form.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=206496270&revision=9c7087c217509cc8ad7186295e12410264f3b40a

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=206496270&repo=mozilla-inbound&lineNumber=2172

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7e9756a2efa098d8e0d0e3a4d4447da0d0f1fd

Comment 15

[Matthew N. [: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.

Comment 16

[Diego Pino]

Attachment: Bug-1497514-Initialize-BasicCardPage-selectedStateKe.patch

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.

Comment 17

[Matthew N. [:MattN]]

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!

Comment 18

[Matthew N. [:MattN]]

Are you able to push to try server?

Comment 19

[Diego Pino]

Yes, I can (it slipped my mind). Treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=abda8b03a1114b35db11ec4036ac9106d049c040

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25594c3013c4
Initialize BasicCardPage selectedStateKey to 'selectPaymentCard'. r=MattN

Comment 21

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/25594c3013c4

Comment 22

[Hani Yacoub, Desktop QA]

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.