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

Status: VERIFIED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Scott Wu [:scottwu]]

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 1

[Scott Wu [:scottwu]]

Attachment: Bug 1397090 - Add the ability to mask credit card numbers after they have been unmasked.

Review commit: https://reviewboard.mozilla.org/r/177568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177568/

Comment 2

[Matthew N. [:MattN]]

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.

Comment 3

[Luke Chang [:lchang] (inactive)]

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

Comment 4

[Scott Wu [:scottwu]]

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 5

[Scott Wu [:scottwu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177568/diff/1-2/

Comment 6

[Luke Chang [:lchang] (inactive)]

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.

Comment 7

[Scott Wu [:scottwu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177568/diff/2-3/

Comment 8

[Scott Wu [:scottwu]]

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!

Comment 9

[Scott Wu [:scottwu]]

Checking it in, thanks Luke!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cbd6e5862db8
Add the ability to mask credit card numbers after they have been unmasked. r=lchang

Comment 11

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/cbd6e5862db8

Comment 12

[Gabi Cheta [:Gabi] Release Desktop QA]

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