Closed
Bug 1397090
Opened 7 years ago
Closed 7 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)
Toolkit
Form Manager
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 hidden (mozreview-request) |
Comment 2•7 years 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/#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•7 years 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•7 years 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•7 years 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•7 years 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!
Comment 10•7 years 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
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cbd6e5862db8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 12•7 years ago
|
||
Verified as fixed with 57.0a1 20170913100125 on Wwindows 10x64, Ubuntu 16.04 and MacOS 10.12.6
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•