Closed Bug 1397090 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Form Manager, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox57 --- verified

People

(Reporter: scottwu, Assigned: scottwu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M4])

Attachments

(1 file)

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 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 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)
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 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 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!
Checking it in, thanks Luke!
Keywords: checkin-needed
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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Verified as fixed with 57.0a1 20170913100125 on Wwindows 10x64, Ubuntu 16.04 and MacOS 10.12.6
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.