[Form Autofill] Manage credit cards dialog should have the ability to mask card numbers again after they are shown

VERIFIED FIXED in Firefox 57

Status

()

Toolkit
Form Manager
VERIFIED FIXED
2 months ago
2 months ago

People

(Reporter: scottwu, Assigned: scottwu)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [form autofill:M4])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 months ago
When "Show Credit Cards" button is clicked, the card numbers are unmasked, but we should give users the ability to hide them, similar to login manager.
Comment hidden (mozreview-request)
Comment on attachment 8905767 [details]
Bug 1397090 - Add the ability to mask credit card numbers after they have been unmasked.

https://reviewboard.mozilla.org/r/177568/#review182636

This looks fine from a quick skim but I didn't test it. I'm fine with Luke reviewing this.
Attachment #8905767 - Flags: review?(MattN+bmo)

Comment 3

2 months ago
mozreview-review
Comment on attachment 8905767 [details]
Bug 1397090 - Add the ability to mask credit card numbers after they have been unmasked.

https://reviewboard.mozilla.org/r/177568/#review182658

Looks good except a few more tests to ensure the new behaviors. Thanks.

::: browser/extensions/formautofill/content/manageDialog.js:392
(Diff revision 1)
> +    // For testing only: Notify when credit cards labels have been updated
> +    this._elements.records.dispatchEvent(new CustomEvent("LabelsUpdated"));
> +  }
> +
> +  async renderRecordElements(records) {
> +    // Revert back to encrypted form when re-rendering happens

Please also cover this behavior in tests.

::: browser/extensions/formautofill/content/manageDialog.js:402
(Diff revision 1)
> +  updateButtonsStates(selectedCount) {
> +    this.updateShowHideButtonState(this._isDecrypted);
> +    super.updateButtonsStates(selectedCount);
> +  }
> +
> +  updateShowHideButtonState(isDecrypted) {

nit: Just curious if we can use `this._isDecrypted` directly?

::: browser/extensions/formautofill/test/browser/browser_manageCreditCardsDialog.js:119
(Diff revision 1)
>  
>    is(selRecords[0].text, "9999888877776666", "Decrypted credit card 3");
>    is(selRecords[1].text, "1111222233334444, Timothy Berners-Lee", "Decrypted credit card 2");
>    is(selRecords[2].text, "1234567812345678, John Doe", "Decrypted credit card 1");
>  
> +  is(btnShowHideCreditCards.textContent, "Hide Credit Cards", "Label should be 'Hide Credit Cards'");

We should also test the functionality of "Hide Credit Cards".
Attachment #8905767 - Flags: review?(lchang)
(Assignee)

Comment 4

2 months ago
mozreview-review-reply
Comment on attachment 8905767 [details]
Bug 1397090 - Add the ability to mask credit card numbers after they have been unmasked.

https://reviewboard.mozilla.org/r/177568/#review182658

> Please also cover this behavior in tests.

Will do.

> nit: Just curious if we can use `this._isDecrypted` directly?

Yes I think it's appropriate since I'm passing in `this._isDecrypted` anyway. Thanks.

> We should also test the functionality of "Hide Credit Cards".

We should! Thanks for the reminder.
Comment hidden (mozreview-request)

Comment 6

2 months ago
mozreview-review
Comment on attachment 8905767 [details]
Bug 1397090 - Add the ability to mask credit card numbers after they have been unmasked.

https://reviewboard.mozilla.org/r/177568/#review183028

Thanks.

::: browser/extensions/formautofill/test/browser/browser_manageCreditCardsDialog.js:125
(Diff revision 2)
> +  // Hide credit card numbers
> +  EventUtils.synthesizeMouseAtCenter(btnShowHideCreditCards, {}, win);
> +  await BrowserTestUtils.waitForEvent(selRecords, "LabelsUpdated");
> +  is(selRecords[0].text, "**** 6666", "Masked credit card 3");
> +  is(selRecords[1].text, "**** 4444, Timothy Berners-Lee", "Masked credit card 2");
> +  is(selRecords[2].text, "**** 5678, John Doe", "Masked credit card 1");

nit: test the button's label again here.
Attachment #8905767 - Flags: review?(lchang) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 months ago
mozreview-review-reply
Comment on attachment 8905767 [details]
Bug 1397090 - Add the ability to mask credit card numbers after they have been unmasked.

https://reviewboard.mozilla.org/r/177568/#review183028

> nit: test the button's label again here.

Got it. Thanks!
(Assignee)

Comment 9

2 months ago
Checking it in, thanks Luke!
Keywords: checkin-needed

Comment 10

2 months ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cbd6e5862db8
Add the ability to mask credit card numbers after they have been unmasked. r=lchang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cbd6e5862db8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 12

2 months ago
Verified as fixed with 57.0a1 20170913100125 on Wwindows 10x64, Ubuntu 16.04 and MacOS 10.12.6

Updated

2 months ago
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.