Closed Bug 1371113 Opened 7 years ago Closed 7 years ago

[Form Autofill] Prompt a door hanger to ask users whether to save a credit card profile when users submit a form

Categories

(Toolkit :: Form Autofill, enhancement, P3)

Product:
Toolkit
Component:
Form Autofill
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: lchang, Assigned: steveck)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M4][ETA:8/8])

Attachments

(6 files, 1 obsolete file)

Bug 1371113 - Add credit card doorhanger.
59 bytes, text/x-review-board-request
lchang
: review+
Details
Bug 1371113 - Part 2:Show doorhanger and save/disable credit card.
59 bytes, text/x-review-board-request
lchang
: review+
Details
Bug 1371113 - Part 3: Add mochitest for credit card doorhanger.
59 bytes, text/x-review-board-request
lchang
: review+
Details
Bug 1371113 - Part 1: Add credit card doorhanger.
59 bytes, text/x-review-board-request
lchang
: review+
Details
Bug 1371113 - Part 2: Show doorhanger and save/disable credit card.
59 bytes, text/x-review-board-request
lchang
: review+
Details
Bug 1371113 - Part 3: Add mochitest for credit card doorhanger.
59 bytes, text/x-review-board-request
lchang
: review+
Details 
There will be a door hanger prompted to ask users whether or not to save the credit card profile when every time they submit a form with a new credit card information.

There will also be a dropdown with an option "never save credit card details" in the door hanger to allow users to disable the credit card autofill feature entirely.
We need to make sure the credit card door hanger comes after the address' one when there are both address fields and credit card fields in the same page and the address saving door hanger hasn't shown before.
Assignee: nobody → schung
Whiteboard: [form autofill:M4] → [form autofill:M4][ETA:7/28]
Whiteboard: [form autofill:M4][ETA:7/28] → [form autofill:M4][ETA:8/8]
Comment on attachment 8892785 [details]
Bug 1371113 - Part 1: Add credit card enabled pref.

https://reviewboard.mozilla.org/r/163778/#review169674

Please file another bug to address this part as it needs to be uplifted but the other one doesn't.

I think you could also deal with the telemetry entry in `TelemetryEnvironment.jsm` in the same bug.
Attachment #8892785 - Flags: review?(lchang)
Comment on attachment 8892786 [details]
Bug 1371113 - Add credit card doorhanger.

https://reviewboard.mozilla.org/r/163780/#review169678

Are you going to add tests in another commit?
Attachment #8892786 - Flags: review?(lchang) → review+
Attachment #8892785 - Attachment is obsolete: true
(In reply to Luke Chang [:lchang] from comment #6)
> Comment on attachment 8892786 [details]
> Bug 1371113 - Add credit card doorhanger.
> 
> https://reviewboard.mozilla.org/r/163780/#review169678
> 
> Are you going to add tests in another commit?

Yes the test part would be added within the patch that actually displays the doorhanger in parent(blocked by bug 1337314). Maybe we can land the existing patch first and create another follow up for showing doorhanger + mochitest.
Since the crypto part only affects the last step (saving into storage), I would recommend you to implement the communication part in this bug and let door hanger itself can be tested first. Thus, we can also unblock other works such as metrics.
Hi Juwei, since we'll only have one save button for credit card record saving, is there a clear definition about when to "update" or "save a new one"? For example, we can always "update" if user apply the autofill if data is not exactly match and "create" new" if user entered manually, it should be easiest solution for us. But do we need to consider some cases like:
- User just want to save another new credit card if user apply the autofill but change the number.
- If user entered a card number manually but the last 4 digits matched the existing records(we can't know the complete card number  unless we decrypt first)
Flags: needinfo?(jhuang)
For MVP I would like to keep it as simple as possible, and since we don't show any hint or doorhanger for update/merge cases, I would suggest that if users change anything (no matter they delete the number and input the same number again), we just ask if users want to create a new one.
Flags: needinfo?(jhuang)
Comment on attachment 8896926 [details]
Bug 1371113 - Part 2:Show doorhanger and save/disable credit card.

https://reviewboard.mozilla.org/r/168198/#review173454
Attachment #8896926 - Flags: review?(lchang) → review+
Comment on attachment 8896926 [details]
Bug 1371113 - Part 2:Show doorhanger and save/disable credit card.

https://reviewboard.mozilla.org/r/168198/#review177212

::: browser/extensions/formautofill/FormAutofillParent.jsm:373
(Diff revision 2)
> +    if (state == "disable") {
> +      Services.prefs.setBoolPref("extensions.formautofill.creditCards.enabled", false);
> +      return;
> +    }
> +
> +    await this.profileStorage.creditCards.normalizeCCNumberFields(creditCard.record);

I noticed that it might fail if the format of "cc-number" isn't valid. We should either put "try...catch" here or validate "cc-number" in "createRecords" [1]. I personally prefer the latter so the doorhanger won't show up if it's invalid. What do you think?

BTW, Scott is going to introduce a "isCCNumber" function in FormAutofillUtils in bug 1370764. We can leverage it.


[1] https://dxr.mozilla.org/mozilla-central/rev/1867d7931c0a70ab90edf4aa84876525773a7139/browser/extensions/formautofill/FormAutofillHandler.jsm#520
Comment on attachment 8900223 [details]
Bug 1371113 - Part 3: Add mochitest for credit card doorhanger.

https://reviewboard.mozilla.org/r/171592/#review177884

::: browser/extensions/formautofill/test/browser/browser_creditCard_doorhanger.js:13
(Diff revision 3)
> +                                                       "popupshown");
> +      await ContentTask.spawn(browser, null, async function() {
> +        let form = content.document.getElementById("form");
> +        let name = form.querySelector("#cc-name");
> +        name.focus();
> +        await new Promise(resolve => setTimeout(resolve, 1000));

Why do we need to sleep before "setUserInput"?

::: browser/extensions/formautofill/test/browser/browser_creditCard_doorhanger.js:68
(Diff revision 3)
> +  is(creditCards.length, 1, "Still 1 address in storage");
> +  is(creditCards[0]["cc-name"], "User 1", "Verify the name field");
> +});
> +
> +add_task(async function test_submit_creditCard_never_save() {
> +  // TODO: Verify the menu item action once we clarify the problem.

Please file a bug and mention the bug number here.

::: browser/extensions/formautofill/test/browser/head.js:18
(Diff revision 3)
>  const ENABLED_PREF = "extensions.formautofill.addresses.enabled";
> +const CREDITCARD_PREF = "extensions.formautofill.creditCards.enabled";

They are going to be changed in bug 1370766. You'll need to rebase on it.
Attachment #8900223 - Flags: review?(lchang) → review+
Blocks: 1393756
Keywords: checkin-needed
Still has unresolved issues in MozReview.
Flags: needinfo?(schung)
Keywords: checkin-needed
Confirmed it was fixed. Thanks.
Flags: needinfo?(schung)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 4083953a1ec9 -d e58415175f5d: rebasing 415888:4083953a1ec9 "Bug 1371113 - Add credit card doorhanger. r=lchang"
merging browser/extensions/formautofill/locales/en-US/formautofill.properties
rebasing 415889:73a3de36b1e3 "Bug 1371113 - Part 2:Show doorhanger and save/disable credit card. r=lchang"
rebasing 415890:421e1ab0042f "Bug 1371113 - Part 3: Add mochitest for credit card doorhanger. r=lchang" (tip)
merging browser/extensions/formautofill/test/browser/browser.ini
merging browser/extensions/formautofill/test/browser/browser_first_time_use_doorhanger.js
merging browser/extensions/formautofill/test/browser/head.js
warning: conflicts while merging browser/extensions/formautofill/test/browser/head.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Conflicts resolved.
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08e729b36517
Add credit card doorhanger. r=lchang
https://hg.mozilla.org/integration/autoland/rev/397bc9636e83
Part 2:Show doorhanger and save/disable credit card. r=lchang
https://hg.mozilla.org/integration/autoland/rev/6819414d32eb
Part 3: Add mochitest for credit card doorhanger. r=lchang
Keywords: checkin-needed
Backed out for failing browser_misused_characters_in_strings.js: cancelCreditCardLabel has a misused apostrophe:

https://hg.mozilla.org/integration/autoland/rev/97b5e21ce80f4b430de03a466f452dc74cdbb626
https://hg.mozilla.org/integration/autoland/rev/05b970ff2affbeb7d35ec07b12a56bec2ebd0cc5
https://hg.mozilla.org/integration/autoland/rev/a4f59593c33d7d0a83167fa42cd3c482852215d9

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6819414d32eb9e2a293f5c937d3db745237ea26a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=126335978&repo=autoland
> TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_misused_characters_in_strings.js | jar:file:///Z:/task_1503894150/build/application/firefox/browser/omni.ja!/chrome/en-US/locale/en-US/formautofill.properties with key=cancelCreditCardLabel has a misused apostrophe. Strings with apostrophes should use foo’s instead of foo's. -
Flags: needinfo?(schung)
Comment on attachment 8901728 [details]
Bug 1371113 - Part 1: Add credit card doorhanger.

https://reviewboard.mozilla.org/r/173170/#review178492
Attachment #8901728 - Flags: review?(lchang) → review+
Comment on attachment 8901729 [details]
Bug 1371113 - Part 2: Show doorhanger and save/disable credit card.

https://reviewboard.mozilla.org/r/173172/#review178494
Attachment #8901729 - Flags: review?(lchang) → review+
Comment on attachment 8901730 [details]
Bug 1371113 - Part 3: Add mochitest for credit card doorhanger.

https://reviewboard.mozilla.org/r/173174/#review178496
Attachment #8901730 - Flags: review?(lchang) → review+
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d4c737cf76d
Part 1: Add credit card doorhanger. r=lchang
https://hg.mozilla.org/integration/autoland/rev/a6973ffa879c
Part 2: Show doorhanger and save/disable credit card. r=lchang
https://hg.mozilla.org/integration/autoland/rev/be72dae43ad2
Part 3: Add mochitest for credit card doorhanger. r=lchang
I can help on this. Thanks.
Flags: needinfo?(schung)
Backed out for frequently failing browser-chrome's browser/base/content/test/performance/browser_startup.js:

https://hg.mozilla.org/integration/autoland/rev/954af5fafc8abe18deca43a2aa0f31ede6a6a289
https://hg.mozilla.org/integration/autoland/rev/9b07254f54e2e1b6ea2e79493f1594b213c758a4
https://hg.mozilla.org/integration/autoland/rev/0e62978a8c3c0cb07d8c0a0f7d62ca2897529c09

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=be72dae43ad278f0e05c2e60826ec0b4f2a1a86e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=126385187&repo=autoland

[task 2017-08-28T09:55:23.736476Z] 09:55:23     INFO - TEST-PASS | browser/base/content/test/performance/browser_startup.js | PlacesCategoriesStarter.js is not allowed before handling user events - 
[task 2017-08-28T09:55:23.752589Z] 09:55:23     INFO - TEST-PASS | browser/base/content/test/performance/browser_startup.js | nsPlacesExpiration.js is not allowed before handling user events - 
[task 2017-08-28T09:55:23.760941Z] 09:55:23     INFO - TEST-INFO | started process screentopng
[task 2017-08-28T09:55:26.025271Z] 09:55:26     INFO - TEST-INFO | screentopng: exit 0
[task 2017-08-28T09:55:26.025717Z] 09:55:26     INFO - Buffered messages logged at 09:55:19
[task 2017-08-28T09:55:26.027471Z] 09:55:26     INFO - Entering test bound 
[task 2017-08-28T09:55:26.027744Z] 09:55:26     INFO - Buffered messages finished
[task 2017-08-28T09:55:26.029388Z] 09:55:26     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup.js | resource:///modules/RecentWindow.jsm is not allowed before handling user events - 
[task 2017-08-28T09:55:26.029601Z] 09:55:26     INFO - Stack trace:
[task 2017-08-28T09:55:26.031378Z] 09:55:26     INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/browser_startup.js:null:207
Flags: needinfo?(lchang)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/9a4e16c753a9
Part 1: Add credit card doorhanger. r=lchang
https://hg.mozilla.org/integration/autoland/rev/2b055a4c047e
Part 2: Show doorhanger and save/disable credit card. r=lchang
https://hg.mozilla.org/integration/autoland/rev/2801212a5855
Part 3: Add mochitest for credit card doorhanger. r=lchang
Relanded the patches. The failing test got put into a different test bucket by automation which broke the analytics.
Flags: needinfo?(lchang)
I'm unsure exactly where that leaves the state of this bug.
Flags: needinfo?(schung)
Hi Wes, may I know the problem of this patch right now? It seems you backed out and reland the patch again.
Flags: needinfo?(schung) → needinfo?(wkocher)
Comment 47 was the merge of backout commits from comment 43.

Comment 49 is the merge of the re-landed commits from comment 44.

As far as I can tell, that means that these patches are officially part of mozilla-central.
Flags: needinfo?(wkocher)
Depends on: 1618203
Component: Form Manager → Form Autofill
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: