From 84a7ed1184a6af8ddecc20756ac28257a1d3715f Mon Sep 17 00:00:00 2001 From: Sam Foster Date: Mon, 9 Apr 2018 16:15:15 -0700 Subject: [PATCH] Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen. r=MattN * Add a new labelled-checkbox component, and use it for the persist checkbox in basic card add/edit form * Pass an isPrivate flag from the parent to UI in the state * Re-work save logic for the basic card form to set correct defaults when payment is initiated from a private window * Add a tempBasicCards object on the state, and a paymentRequest.getBasicCards(state) helper to get the union of both saved and temporary cards * Set a newly added temporary card as the selectedPaymentCard * Tests for basic-card-form.js in private windows, and correctly persisting or not new card info basic on the state of the 'Save to Firefox' checkbox * Add paymentRequest.js to mochitests, pending landing of bug 1427939 MozReview-Commit-ID: 9oQ1gbHPojf --HG-- extra : rebase_source : 947b74d62d9257d1ee27dbaffe47e9f907543518 --- .../payments/content/paymentDialogWrapper.js | 6 + .../res/components/labelled-checkbox.js | 47 ++++++++ .../res/containers/basic-card-form.js | 67 ++++++++--- .../payments/res/containers/payment-dialog.js | 7 +- .../res/containers/payment-method-picker.js | 5 +- .../res/mixins/PaymentStateSubscriberMixin.js | 1 + .../components/payments/res/paymentRequest.js | 7 ++ .../payments/res/paymentRequest.xhtml | 2 + .../test/browser/browser_card_edit.js | 104 ++++++++++++++++++ .../components/payments/test/browser/head.js | 23 ++++ .../payments/test/mochitest/mochitest.ini | 1 + .../mochitest/test_labelled_checkbox.html | 74 +++++++++++++ .../mochitest/test_payment_method_picker.html | 1 + 13 files changed, 324 insertions(+), 21 deletions(-) create mode 100644 toolkit/components/payments/res/components/labelled-checkbox.js create mode 100644 toolkit/components/payments/test/mochitest/test_labelled_checkbox.html diff --git a/toolkit/components/payments/content/paymentDialogWrapper.js b/toolkit/components/payments/content/paymentDialogWrapper.js index 0a9cb61772b1..245064cd9d5c 100644 --- a/toolkit/components/payments/content/paymentDialogWrapper.js +++ b/toolkit/components/payments/content/paymentDialogWrapper.js @@ -17,6 +17,8 @@ ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); ChromeUtils.defineModuleGetter(this, "MasterPassword", "resource://formautofill/MasterPassword.jsm"); +ChromeUtils.defineModuleGetter(this, "PrivateBrowsingUtils", + "resource://gre/modules/PrivateBrowsingUtils.jsm"); XPCOMUtils.defineLazyGetter(this, "formAutofillStorage", () => { let formAutofillStorage; @@ -383,10 +385,14 @@ var paymentDialogWrapper = { initializeFrame() { let requestSerialized = this._serializeRequest(this.request); + let chromeWindow = Services.wm.getMostRecentWindow("navigator:browser"); + let isPrivate = PrivateBrowsingUtils.isWindowPrivate(chromeWindow); + this.sendMessageToContent("showPaymentRequest", { request: requestSerialized, savedAddresses: this.fetchSavedAddresses(), savedBasicCards: this.fetchSavedPaymentCards(), + isPrivate, }); Services.obs.addObserver(this, "formautofill-storage-changed", true); diff --git a/toolkit/components/payments/res/components/labelled-checkbox.js b/toolkit/components/payments/res/components/labelled-checkbox.js new file mode 100644 index 000000000000..3fd7b297a04c --- /dev/null +++ b/toolkit/components/payments/res/components/labelled-checkbox.js @@ -0,0 +1,47 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +import ObservedPropertiesMixin from "../mixins/ObservedPropertiesMixin.js"; + +/** + * + */ + +export default class LabelledCheckbox extends ObservedPropertiesMixin(HTMLElement) { + static get observedAttributes() { + return [ + "label", + "value", + ]; + } + constructor() { + super(); + + this._label = document.createElement("label"); + this._labelSpan = document.createElement("span"); + this._checkbox = document.createElement("input"); + this._checkbox.type = "checkbox"; + } + + connectedCallback() { + this.appendChild(this._label); + this._label.appendChild(this._checkbox); + this._label.appendChild(this._labelSpan); + this.render(); + } + + render() { + this._labelSpan.textContent = this.label; + } + + get checked() { + return this._checkbox.checked; + } + + set checked(value) { + return this._checkbox.checked = value; + } +} + +customElements.define("labelled-checkbox", LabelledCheckbox); diff --git a/toolkit/components/payments/res/containers/basic-card-form.js b/toolkit/components/payments/res/containers/basic-card-form.js index 4aa56224202b..f29e71501432 100644 --- a/toolkit/components/payments/res/containers/basic-card-form.js +++ b/toolkit/components/payments/res/containers/basic-card-form.js @@ -3,8 +3,10 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ /* import-globals-from ../../../../../browser/extensions/formautofill/content/autofillEditForms.js*/ +import LabelledCheckbox from "../components/labelled-checkbox.js"; import PaymentStateSubscriberMixin from "../mixins/PaymentStateSubscriberMixin.js"; import paymentRequest from "../paymentRequest.js"; + /* import-globals-from ../unprivileged-fallbacks.js */ /** @@ -26,6 +28,8 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme this.saveButton = document.createElement("button"); this.saveButton.addEventListener("click", this); + this.persistCheckbox = new LabelledCheckbox(); + // The markup is shared with form autofill preferences. let url = "formautofill/editCreditCard.xhtml"; this.promiseReady = this._fetchMarkup(url).then(doc => { @@ -60,6 +64,7 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme getAddressLabel: PaymentDialogUtils.getAddressLabel, }); + this.appendChild(this.persistCheckbox); this.appendChild(this.genericErrorText); this.appendChild(this.backButton); this.appendChild(this.saveButton); @@ -72,14 +77,15 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme render(state) { this.backButton.textContent = this.dataset.backButtonLabel; this.saveButton.textContent = this.dataset.saveButtonLabel; + this.persistCheckbox.label = this.dataset.persistCheckboxLabel; let record = {}; let { page, savedAddresses, - savedBasicCards, selectedShippingAddress, } = state; + let basicCards = paymentRequest.getBasicCards(state); this.genericErrorText.textContent = page.error; @@ -88,12 +94,19 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme // If a card is selected we want to edit it. if (editing) { - record = savedBasicCards[page.guid]; + record = basicCards[page.guid]; if (!record) { throw new Error("Trying to edit a non-existing card: " + page.guid); } - } else if (selectedShippingAddress) { - record.billingAddressGUID = selectedShippingAddress; + // When editing an existing record, prevent changes to persistence + this.persistCheckbox.hidden = true; + } else { + if (selectedShippingAddress) { + record.billingAddressGUID = selectedShippingAddress; + } + // Adding a new record: default persistence to checked when in a not-private session + this.persistCheckbox.hidden = false; + this.persistCheckbox.checked = !state.isPrivate; } this.formHandler.loadRecord(record, savedAddresses); @@ -132,7 +145,10 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme let record = this.formHandler.buildFormObject(); let { page, + tempBasicCards, } = this.requestStore.getState(); + let editing = !!page.guid; + let tempRecord = editing && tempBasicCards[page.guid]; for (let editableFieldName of ["cc-name", "cc-exp-month", "cc-exp-year"]) { record[editableFieldName] = record[editableFieldName] || ""; @@ -140,25 +156,44 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme // Only save the card number if we're saving a new record, otherwise we'd // overwrite the unmasked card number with the masked one. - if (!page.guid) { + if (!editing) { record["cc-number"] = record["cc-number"] || ""; } - paymentRequest.updateAutofillRecord("creditCards", record, page.guid, { - errorStateChange: { - page: { - id: "basic-card-page", - error: this.dataset.errorGenericSave, + if (!tempRecord && this.persistCheckbox.checked) { + log.debug(`BasicCardForm: persisting creditCard record: ${page.guid || "(new)"}`); + paymentRequest.updateAutofillRecord("creditCards", record, page.guid, { + errorStateChange: { + page: { + id: "basic-card-page", + error: this.dataset.errorGenericSave, + }, }, - }, - preserveOldProperties: true, - selectedStateKey: "selectedPaymentCard", - successStateChange: { + preserveOldProperties: true, + selectedStateKey: "selectedPaymentCard", + successStateChange: { + page: { + id: "payment-summary", + }, + }, + }); + } else { + // This record will never get inserted into the store + // so we generate a faux-guid for a new record + record.guid = page.guid || "temp-" + Math.abs(Math.random() * 0xffffffff|0); + + log.debug(`BasicCardForm: saving temporary record: ${record.guid}`); + this.requestStore.setState({ page: { id: "payment-summary", }, - }, - }); + selectedPaymentCard: record.guid, + tempBasicCards: Object.assign({}, tempBasicCards, { + // Mix-in any previous values - equivalent to the store's preserveOldProperties: true, + [record.guid]: Object.assign({}, tempRecord, record), + }), + }); + } } } diff --git a/toolkit/components/payments/res/containers/payment-dialog.js b/toolkit/components/payments/res/containers/payment-dialog.js index 868ceb1a268e..bc36ea61fb11 100644 --- a/toolkit/components/payments/res/containers/payment-dialog.js +++ b/toolkit/components/payments/res/containers/payment-dialog.js @@ -124,7 +124,6 @@ export default class PaymentDialog extends PaymentStateSubscriberMixin(HTMLEleme let { request: {paymentOptions: {requestShipping: requestShipping}}, savedAddresses, - savedBasicCards, selectedPayerAddress, selectedPaymentCard, selectedShippingAddress, @@ -160,9 +159,11 @@ export default class PaymentDialog extends PaymentStateSubscriberMixin(HTMLEleme // Ensure `selectedPaymentCard` never refers to a deleted payment card and refers // to a payment card if one exists. - if (!savedBasicCards[selectedPaymentCard]) { + let basicCards = paymentRequest.getBasicCards(state); + if (!basicCards[selectedPaymentCard]) { + // Determining the initial selection is tracked in bug 1455789 this.requestStore.setState({ - selectedPaymentCard: Object.keys(savedBasicCards)[0] || null, + selectedPaymentCard: Object.keys(basicCards)[0] || null, selectedPaymentCardSecurityCode: null, }); } diff --git a/toolkit/components/payments/res/containers/payment-method-picker.js b/toolkit/components/payments/res/containers/payment-method-picker.js index 3d242ccea5df..37f123366919 100644 --- a/toolkit/components/payments/res/containers/payment-method-picker.js +++ b/toolkit/components/payments/res/containers/payment-method-picker.js @@ -5,6 +5,7 @@ import BasicCardOption from "../components/basic-card-option.js"; import PaymentStateSubscriberMixin from "../mixins/PaymentStateSubscriberMixin.js"; import RichSelect from "../components/rich-select.js"; +import paymentRequest from "../paymentRequest.js"; /** * @@ -43,9 +44,9 @@ export default class PaymentMethodPicker extends PaymentStateSubscriberMixin(HTM } render(state) { - let {savedBasicCards} = state; + let basicCards = paymentRequest.getBasicCards(state); let desiredOptions = []; - for (let [guid, basicCard] of Object.entries(savedBasicCards)) { + for (let [guid, basicCard] of Object.entries(basicCards)) { let optionEl = this.dropdown.getOptionByValue(guid); if (!optionEl) { optionEl = new BasicCardOption(); diff --git a/toolkit/components/payments/res/mixins/PaymentStateSubscriberMixin.js b/toolkit/components/payments/res/mixins/PaymentStateSubscriberMixin.js index 64b080cad125..76f177a4207a 100644 --- a/toolkit/components/payments/res/mixins/PaymentStateSubscriberMixin.js +++ b/toolkit/components/payments/res/mixins/PaymentStateSubscriberMixin.js @@ -47,6 +47,7 @@ export let requestStore = new PaymentsStore({ selectedShippingOption: null, savedAddresses: {}, savedBasicCards: {}, + tempBasicCards: {}, }); diff --git a/toolkit/components/payments/res/paymentRequest.js b/toolkit/components/payments/res/paymentRequest.js index e242d42bd5bd..e78ec97f9b16 100644 --- a/toolkit/components/payments/res/paymentRequest.js +++ b/toolkit/components/payments/res/paymentRequest.js @@ -113,10 +113,12 @@ var paymentRequest = { await this.domReadyPromise; log.debug("onShowPaymentRequest: domReadyPromise resolved"); + log.debug("onShowPaymentRequest, isPrivate?", detail.isPrivate); document.querySelector("payment-dialog").setStateFromParent({ request: detail.request, savedAddresses: detail.savedAddresses, savedBasicCards: detail.savedBasicCards, + isPrivate: detail.isPrivate, }); }, @@ -203,6 +205,11 @@ var paymentRequest = { // remove listeners that may be used multiple times here window.removeEventListener("paymentChromeToContent", this); }, + + getBasicCards(state) { + let cards = Object.assign({}, state.savedBasicCards, state.tempBasicCards); + return cards; + }, }; paymentRequest.init(); diff --git a/toolkit/components/payments/res/paymentRequest.xhtml b/toolkit/components/payments/res/paymentRequest.xhtml index c9ecf7f4ecc7..9a787cc11538 100644 --- a/toolkit/components/payments/res/paymentRequest.xhtml +++ b/toolkit/components/payments/res/paymentRequest.xhtml @@ -27,6 +27,7 @@ + ]> @@ -111,6 +112,7 @@ data-error-generic-save="&basicCardPage.error.genericSave;" data-back-button-label="&basicCardPage.backButton.label;" data-save-button-label="&basicCardPage.saveButton.label;" + data-persist-checkbox-label="&basicCardPage.persistCheckbox.label;" hidden="hidden"> diff --git a/toolkit/components/payments/test/browser/browser_card_edit.js b/toolkit/components/payments/test/browser/browser_card_edit.js index 7d220f6948e7..6c33bb6fd7f2 100644 --- a/toolkit/components/payments/test/browser/browser_card_edit.js +++ b/toolkit/components/payments/test/browser/browser_card_edit.js @@ -20,6 +20,11 @@ add_task(async function test_add_link() { }, "Check add page state"); + ok(!state.isPrivate, + "isPrivate flag is not set when paymentrequest is shown from a non-private session"); + let persistInput = content.document.querySelector("basic-card-form labelled-checkbox"); + ok(Cu.waiveXrays(persistInput).checked, "persist checkbox should be checked by default"); + let year = (new Date()).getFullYear(); let card = { "cc-number": "4111111111111111", @@ -115,3 +120,102 @@ add_task(async function test_edit_link() { }, args); }); +add_task(async function test_private_persist_defaults() { + const args = { + methodData: [PTU.MethodData.basicCard], + details: PTU.Details.total60USD, + }; + await spawnInDialogForMerchantTask(PTU.ContentTasks.createRequest, async function check() { + let { + PaymentTestUtils: PTU, + } = ChromeUtils.import("resource://testing-common/PaymentTestUtils.jsm", {}); + + let addLink = content.document.querySelector("payment-method-picker a"); + is(addLink.textContent, "Add", "Add link text"); + + addLink.click(); + + let state = await PTU.DialogContentUtils.waitForState(content, (state) => { + return state.page.id == "basic-card-page" && !state.page.guid; + }, + "Check add page state"); + + ok(!state.isPrivate, + "isPrivate flag is not set when paymentrequest is shown from a non-private session"); + let persistInput = content.document.querySelector("basic-card-form labelled-checkbox"); + ok(Cu.waiveXrays(persistInput).checked, + "checkbox is checked by default from a non-private session"); + }, args); + + await spawnInDialogForPrivateMerchantTask(PTU.ContentTasks.createRequest, async function check() { + let { + PaymentTestUtils: PTU, + } = ChromeUtils.import("resource://testing-common/PaymentTestUtils.jsm", {}); + + let addLink = content.document.querySelector("payment-method-picker a"); + is(addLink.textContent, "Add", "Add link text"); + + addLink.click(); + + let state = await PTU.DialogContentUtils.waitForState(content, (state) => { + return state.page.id == "basic-card-page" && !state.page.guid; + }, + "Check add page state"); + + ok(state.isPrivate, + "isPrivate flag is set when paymentrequest is shown from a private session"); + let persistInput = content.document.querySelector("labelled-checkbox"); + ok(!Cu.waiveXrays(persistInput).checked, + "checkbox is not checked by default from a private session"); + }, args); +}); + +add_task(async function test_private_card_adding() { + const args = { + methodData: [PTU.MethodData.basicCard], + details: PTU.Details.total60USD, + }; + await spawnInDialogForPrivateMerchantTask(PTU.ContentTasks.createRequest, async function check() { + let { + PaymentTestUtils: PTU, + } = ChromeUtils.import("resource://testing-common/PaymentTestUtils.jsm", {}); + + let addLink = content.document.querySelector("payment-method-picker a"); + is(addLink.textContent, "Add", "Add link text"); + + addLink.click(); + + let state = await PTU.DialogContentUtils.waitForState(content, (state) => { + return state.page.id == "basic-card-page" && !state.page.guid; + }, + "Check add page state"); + + let savedCardCount = Object.keys(state.savedBasicCards).length; + let tempCardCount = Object.keys(state.tempBasicCards).length; + + let year = (new Date()).getFullYear(); + let card = { + "cc-number": "4111111111111111", + "cc-name": "J. Smith", + "cc-exp-month": 11, + "cc-exp-year": year, + }; + + info("filling fields"); + for (let [key, val] of Object.entries(card)) { + let field = content.document.getElementById(key); + field.value = val; + ok(!field.disabled, `Field #${key} shouldn't be disabled`); + } + + content.document.querySelector("basic-card-form button:last-of-type").click(); + + state = await PTU.DialogContentUtils.waitForState(content, (state) => { + return Object.keys(state.tempBasicCards).length > tempCardCount; + }, + "Check card was added to temp collection"); + + is(savedCardCount, Object.keys(state.savedBasicCards).length, "No card was saved in state"); + is(Object.keys(state.tempBasicCards).length, 1, "Card was added temporarily"); + }, args); +}); diff --git a/toolkit/components/payments/test/browser/head.js b/toolkit/components/payments/test/browser/head.js index 3deb63543f58..b78dbeee32d3 100644 --- a/toolkit/components/payments/test/browser/head.js +++ b/toolkit/components/payments/test/browser/head.js @@ -239,6 +239,29 @@ async function spawnInDialogForMerchantTask(merchantTaskFn, dialogTaskFn, taskAr }); } +async function spawnInDialogForPrivateMerchantTask(merchantTaskFn, dialogTaskFn, taskArgs, { + origin = "https://example.com", +} = { + origin: "https://example.com", +}) { + let privateWin = await BrowserTestUtils.openNewBrowserWindow({private: true}); + + await withMerchantTab({ + url: origin + BLANK_PAGE_PATH, + browser: privateWin.gBrowser, + }, async merchBrowser => { + await ContentTask.spawn(merchBrowser, taskArgs, merchantTaskFn); + + const requests = getPaymentRequests(); + is(requests.length, 1, "Should have one payment request"); + let request = requests[0]; + ok(!!request.requestId, "Got a payment request with an ID"); + + await spawnTaskInNewDialog(request.requestId, dialogTaskFn, taskArgs); + }); + await BrowserTestUtils.closeWindow(privateWin); +} + async function setupFormAutofillStorage() { await formAutofillStorage.initialize(); } diff --git a/toolkit/components/payments/test/mochitest/mochitest.ini b/toolkit/components/payments/test/mochitest/mochitest.ini index 22c855c9bffe..51854e154afb 100644 --- a/toolkit/components/payments/test/mochitest/mochitest.ini +++ b/toolkit/components/payments/test/mochitest/mochitest.ini @@ -12,6 +12,7 @@ skip-if = !e10s [test_address_picker.html] [test_basic_card_form.html] [test_currency_amount.html] +[test_labelled_checkbox.html] [test_order_details.html] [test_payer_address_picker.html] [test_payment_dialog.html] diff --git a/toolkit/components/payments/test/mochitest/test_labelled_checkbox.html b/toolkit/components/payments/test/mochitest/test_labelled_checkbox.html new file mode 100644 index 000000000000..ca4149c229ec --- /dev/null +++ b/toolkit/components/payments/test/mochitest/test_labelled_checkbox.html @@ -0,0 +1,74 @@ + + + + + + Test the labelled-checkbox component + + + + + + + + +

+ + +

+ +
+
+ + + + diff --git a/toolkit/components/payments/test/mochitest/test_payment_method_picker.html b/toolkit/components/payments/test/mochitest/test_payment_method_picker.html index d5571ce2b2e4..57b1b25a7b93 100644 --- a/toolkit/components/payments/test/mochitest/test_payment_method_picker.html +++ b/toolkit/components/payments/test/mochitest/test_payment_method_picker.html @@ -11,6 +11,7 @@ Test the payment-method-picker component +