Credit card record is not correctly passed into GeckoView
Categories
(Toolkit :: Form Autofill, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr91 | --- | unaffected |
firefox98 | --- | unaffected |
firefox99 | + | fixed |
firefox100 | --- | fixed |
People
(Reporter: gl, Assigned: dimi)
References
Details
(Keywords: regression, Whiteboard: [fenix:q2])
Attachments
(3 files)
(deleted),
image/png
|
Details | |
(deleted),
text/x-phabricator-request
|
dmeehan
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-phabricator-request
|
dmeehan
:
approval-mozilla-beta+
|
Details |
While working on supporting a save credit prompt, I noticed that only the credit card number (not the guid, expiry dates) manages to be passed into GeckoView.
My primary investigation happened on desktop:
- Navigate to https://checkout.stripe.dev/preview and fill out a credit card information and perform a form submit.
- In the debugger, observe what the
creditCard
object looks like before callingFormAutofillPrompter.promptToSaveCreditCard
. See screenshot. - Notice that the parameters for the
creditCard
does that line up when translated in https://searchfox.org/mozilla-central/source/toolkit/components/formautofill/android/FormAutofillPrompter.jsm#46 tonewCreditCard
andCreditCard.fromGecko(newCreditCard.record)
.
It seemed interesting to me that that the creditCard.record
parameters no longer matches the parameter that is expected when it's being translated to GeckoView prior to calling GeckoViewAutocomplete.onCreditCardSave
.
In https://searchfox.org/mozilla-central/source/toolkit/components/formautofill/android/FormAutofillPrompter.jsm#53-58, we are expecting to have record.expMonth
and record.expYear
, but I am not quite sure if we even have those parameters anymore. From the screenshot, you can see we have cc-exp
, cc-name
, and cc-number
in creditCard.record
.
Similarly, in https://searchfox.org/mozilla-central/source/mobile/android/modules/geckoview/GeckoViewAutocomplete.jsm#256 called from https://searchfox.org/mozilla-central/source/toolkit/components/formautofill/android/FormAutofillPrompter.jsm#64 is calling. We are also expecting a different set of parameters inside of the creditCard.record
object that is passed to fromGecko
.
Expected outcome is that we should be able to get the credit card name, guid, expiry date in the credit card object passed to GeckoView. It would be highly desirable to ensure we validate that none of the parameters are null by calling isValid() prior to calling GeckoViewAutocomplete.onCreditCardSave(selectedCreditCard)
. In Fenix, we are expecting to save a credit card with a number and expiry date since we do not expect these values to be nullable.
Comment 1•3 years ago
|
||
The severity field is not set for this bug.
:mak, could you have a look please?
For more information, please visit auto_nag documentation.
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 2•3 years ago
|
||
Fenix's credit card autofill feature is blocked waiting for this bug fix.
Assignee | ||
Comment 3•3 years ago
|
||
I'll look into this issue this week.
Assignee | ||
Comment 4•3 years ago
|
||
This was added in Bug 1747096 but was accidentally was removed in Bug 1745248.
Assignee | ||
Comment 5•3 years ago
|
||
There are two bugs in promptToSaveCreditCard
newCreditCard
doesn't containrecord
memeber variablecreditCard.record
should usecc-name
,cc-number
, etc to access
data instead ofrecord.name
,record.number
Depends on D141766
Assignee | ||
Comment 6•3 years ago
|
||
Even with these patches, expiration month and year are still not correctly passed while submitting the form in https://checkout.stripe.dev/preview, but this seems to be a generic bug. I filed bug 1760834 to track this issue.
Updated•3 years ago
|
Comment 8•3 years ago
|
||
[Tracking Requested - why for this release]:
This regression was caused by a backout of the browser.search.region pref from bug 1747096. Without this fix, credit card autofill works in Firefox Android 98, but not 99 or 100.
Is this fix safe to uplift to Beta 99?
Comment 9•3 years ago
|
||
Please submit a beta uplift request when this is ready.
This week is the final week of beta for 99
Comment 10•3 years ago
|
||
Comment 11•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/19d2ac03e307
https://hg.mozilla.org/mozilla-central/rev/86636c7226c3
Assignee | ||
Comment 12•3 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #8)
Is this fix safe to uplift to Beta 99?
Yes, I think this is safe to uplift.
Is it possible someone in your team help test fenix with the latest build? (but not with the reported website https://checkout.stripe.dev/preview because there is still an issue for this site). If everything works fine, I'll request uplift, thanks!
Comment 13•3 years ago
|
||
Please note, tomorrow is the final beta for 99. Please submit an uplift request as soon as this ready. Thanks.
Comment 14•3 years ago
|
||
(In reply to Dimi Lee [:dimi][:dlee] from comment #12)
Is it possible someone in your team help test fenix with the latest build? (but not with the reported website https://checkout.stripe.dev/preview because there is still an issue for this site). If everything works fine, I'll request uplift, thanks!
I filed a PI request asking Delia (the QA tester who tested CC Autofill in Android v98 and found the v99 regression) to retest CC Autofill in Nightly v100:
https://mozilla-hub.atlassian.net/browse/QA-1441
If it’s too late to uplift the fix to Beta v99, we can just let it ride the trains in v100. In that case, CC recording will work in v98, stop working in v99, and start working again four weeks later in v100. Not the end of the world.
Assignee | ||
Comment 15•3 years ago
|
||
Hi Chris, thank you for filing the PI request.
I just test this with latest build with geckoview example app on emulator (verify debug message in geckoview side because I believe we don't support prompt with the example app). It looks good.
Since this is simple change and part of the change has already been verified before (before been backout), I'll just request uplift now.
Assignee | ||
Comment 16•3 years ago
|
||
Comment on attachment 9268908 [details]
Bug 1747898 - P2. Fix issues in promptToSaveCreditCard
r=sgalich,tgiles
Beta/Release Uplift Approval Request
- User impact if declined: Credit Card Capture doesn't work on fenix
- Is this code covered by automated tests?: Unknown. Not sure if mobile team has testcase covers this up.
- Has the fix been verified in Nightly?: Yes, with geckoview example app.
- Needs manual test from QE?: Yes
- If yes, steps to reproduce: Save:
- navigate to a web page that requires credit card information
- fill in the credit card information and then submit the form
- Check the save credit card prompt shows up and contain the correct information
Update: - navigate to a web page that requires credit card information
- autofill the credit card information previously saved
- Change one of the fields (name or expiration date) and then submit the form
- Check the update credit card prompt shows up and contain the correct information
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): There are two part of this patch. The part 1 of this patch has already been verified before. We only restore the change back. The change itself is also only reading a default value when the pref doesn't exist.
The part 2 of this patch doesn't change the logic, it only fixes issue by accessing the correct member variable. - String changes made/needed:
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 17•3 years ago
|
||
Hi Chris, I'm sorry I didn't look into this issue earlier, I thought this was a site-specific issue (I thought if this was a generic problem, it would be discovered by our testcases). So in order to make the desktop change not break the support of cc on mobile, I would like to know whether we have testcases that cover "save" and "update" credit card doorhanger on the mobile. If yes, why it didn't catch this change? What can our team do to prevent this from happening again? Thanks!
Comment 18•3 years ago
|
||
:dimi looks like Part 1, Attachment #9268907 [details], should also have approval-mozilla-beta set to "?"
Please confirm
Comment 19•3 years ago
|
||
Hello,
I attempted testing the CC Autofill with Firefox Nightly 100.0a1 (Build 2015870571 GV: 100.0a1-20220323094932) from 7 hours ago, but the feature is not available.
Should we expect the feature to be available in the tomorrow's Nightly build?
If the feature is making it to the latest Beta v99 from this week, I will proceed and test it when the build will be provided.
Assignee | ||
Comment 20•3 years ago
|
||
Comment on attachment 9268907 [details]
Bug 1747898 - P1. Use default region when "browser.search.region" pref is not present r=sgalich,tgiles
Beta/Release Uplift Approval Request
- User impact if declined:
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: Yes
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky):
- String changes made/needed:
Comment 21•3 years ago
|
||
Comment on attachment 9268907 [details]
Bug 1747898 - P1. Use default region when "browser.search.region" pref is not present r=sgalich,tgiles
Approved for 99.0b8. Thanks.
Comment 22•3 years ago
|
||
Comment on attachment 9268908 [details]
Bug 1747898 - P2. Fix issues in promptToSaveCreditCard
r=sgalich,tgiles
Approved for 99.0b8. Thanks.
Comment 23•3 years ago
|
||
bugherder uplift |
Comment 24•3 years ago
|
||
(In reply to Dimi Lee [:dimi][:dlee] from comment #17)
in order to make the desktop change not break the support of cc on mobile, I would like to know whether we have testcases that cover "save" and "update" credit card doorhanger on the mobile. If yes, why it didn't catch this change? What can our team do to prevent this from happening again?
Tim or Gabriel, do you know if the credit card autofill tests run on Android? Did this regression only happen for non-US countries (UK, FR, and DE)? Do we run credit card autofill tests for different countries?
Updated•3 years ago
|
Comment 25•3 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #24)
Tim or Gabriel, do you know if the credit card autofill tests run on Android?
I don't know if Android has their own credit card tests or not. :gl will be able to answer that better than I can :)
Did this regression only happen for non-US countries (UK, FR, and DE)?
This regression was independent of region.
Do we run credit card autofill tests for different countries?
On Desktop, we test combinations of supported and unsupported locales to ensure the feature works as expected. Can't speak for Android though.
Comment 26•3 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #24)
(In reply to Dimi Lee [:dimi][:dlee] from comment #17)
in order to make the desktop change not break the support of cc on mobile, I would like to know whether we have testcases that cover "save" and "update" credit card doorhanger on the mobile. If yes, why it didn't catch this change? What can our team do to prevent this from happening again?
Tim or Gabriel, do you know if the credit card autofill tests run on Android? Did this regression only happen for non-US countries (UK, FR, and DE)? Do we run credit card autofill tests for different countries?
Hello,
For the Fenix Android browser we have a Subsection of test cases regarding the Credit Cards functionality as part of the Full Functional Tests Suite: https://testrail.stage.mozaws.net/index.php?/suites/view/3192&group_by=cases:section_id&group_order=asc&group_id=227739 .
This section is tested upon every Geckoview release (Beta 1), however we did not catch the issue with the Autofill before since we used the https://checkout.stripe.dev/preview testing page that already has a build in prompt that can be used to copy the credit card fields, which mislead us.
Based on the latest implementations, we will update/add more test cases for the feature, and continue to test it after every new version release.
Reporter | ||
Comment 27•3 years ago
|
||
Seems like Delia has answered about tests. I have tested the latest Beta and can see the Select Credit Card prompt working. I will pass the news over to QA to do another smoke test.
Updated•3 years ago
|
Description
•