Bug 1428414 - Removed cleared record attributes on card and address options. r=jaws draft
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Thu, 22 Mar 2018 23:01:32 -0700
changeset 776795 58d4cad0fb2d2670a90ba0935056d3ffbd4cf96f
parent 776695 9a1133aef407576ee1427b45e40161c9bfba897f
child 776796 8d7ffa9290b5baec4afa38f416111b342c4c6352
push id104995
push usermozilla@noorenberghe.ca
push dateTue, 03 Apr 2018 18:27:34 +0000
reviewersjaws
bugs1428414
milestone61.0a1
Bug 1428414 - Removed cleared record attributes on card and address options. r=jaws If a property gets cleared in autofill storage, that property is not defined on the object when retrieved which means it won't get updated. Previously I thought that known fields were always returned and had a default value. MozReview-Commit-ID: KnBrpFdOk8P
toolkit/components/payments/res/components/address-option.js
toolkit/components/payments/res/components/basic-card-option.js
toolkit/components/payments/res/containers/address-picker.js
toolkit/components/payments/res/containers/payment-method-picker.js
toolkit/components/payments/test/mochitest/test_address_picker.html
toolkit/components/payments/test/mochitest/test_payer_address_picker.html
toolkit/components/payments/test/mochitest/test_payment_method_picker.html
--- a/toolkit/components/payments/res/components/address-option.js
+++ b/toolkit/components/payments/res/components/address-option.js
@@ -16,28 +16,32 @@
  * </rich-select>
  *
  * Attribute names follow FormAutofillStorage.jsm.
  */
 
 /* global ObservedPropertiesMixin, RichOption */
 
 class AddressOption extends ObservedPropertiesMixin(RichOption) {
-  static get observedAttributes() {
-    return RichOption.observedAttributes.concat([
+  static get recordAttributes() {
+    return [
       "address-level1",
       "address-level2",
       "country",
       "email",
       "guid",
       "name",
       "postal-code",
       "street-address",
       "tel",
-    ]);
+    ];
+  }
+
+  static get observedAttributes() {
+    return RichOption.observedAttributes.concat(AddressOption.recordAttributes);
   }
 
   constructor() {
     super();
 
     for (let name of ["name", "street-address", "email", "tel"]) {
       this[`_${name}`] = document.createElement("span");
       this[`_${name}`].classList.add(name);
--- a/toolkit/components/payments/res/components/basic-card-option.js
+++ b/toolkit/components/payments/res/components/basic-card-option.js
@@ -6,24 +6,28 @@
  * <rich-select>
  *  <basic-card-option></basic-card-option>
  * </rich-select>
  */
 
 /* global ObservedPropertiesMixin, RichOption */
 
 class BasicCardOption extends ObservedPropertiesMixin(RichOption) {
-  static get observedAttributes() {
-    return RichOption.observedAttributes.concat([
+  static get recordAttributes() {
+    return [
       "cc-exp",
       "cc-name",
       "cc-number",
       "guid",
       "type", // XXX Bug 1429181.
-    ]);
+    ];
+  }
+
+  static get observedAttributes() {
+    return RichOption.observedAttributes.concat(BasicCardOption.recordAttributes);
   }
 
   constructor() {
     super();
 
     for (let name of ["cc-name", "cc-number", "cc-exp", "type"]) {
       this[`_${name}`] = document.createElement("span");
       this[`_${name}`].classList.add(name);
--- a/toolkit/components/payments/res/containers/address-picker.js
+++ b/toolkit/components/payments/res/containers/address-picker.js
@@ -1,13 +1,14 @@
 /* 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/. */
 
-/* global PaymentStateSubscriberMixin */
+/* import-globals-from ../mixins/PaymentStateSubscriberMixin.js */
+/* import-globals-from ../components/address-option.js */
 
 "use strict";
 
 /**
  * <address-picker></address-picker>
  * Container around <rich-select> (eventually providing add/edit links) with
  * <address-option> listening to savedAddresses.
  */
@@ -33,40 +34,38 @@ class AddressPicker extends PaymentState
       this.render(this.requestStore.getState());
     }
   }
 
   /**
    * De-dupe and filter addresses for the given set of fields that will be visible
    *
    * @param {object} addresses
-   * @param {array?} fieldNames - optional list of field names that be used when de-duping entries
+   * @param {array?} fieldNames - optional list of field names that be used when
+   *                              de-duping and excluding entries
    * @returns {object} filtered copy of given addresses
    */
   filterAddresses(addresses, fieldNames = [
     "address-level1",
     "address-level2",
     "country",
     "name",
     "postal-code",
     "street-address",
   ]) {
     let uniques = new Set();
     let result = {};
     for (let [guid, address] of Object.entries(addresses)) {
       let addressCopy = {};
-      let isMatch;
-      // exclude addresses that are missing any of the requested fields
+      let isMatch = false;
+      // exclude addresses that are missing all of the requested fields
       for (let name of fieldNames) {
         if (address[name]) {
           isMatch = true;
           addressCopy[name] = address[name];
-        } else {
-          isMatch = false;
-          break;
         }
       }
       if (isMatch) {
         let key = JSON.stringify(addressCopy);
         // exclude duplicated addresses
         if (!uniques.has(key)) {
           uniques.add(key);
           result[guid] = address;
@@ -90,19 +89,25 @@ class AddressPicker extends PaymentState
 
     for (let [guid, address] of Object.entries(filteredAddresses)) {
       let optionEl = this.dropdown.getOptionByValue(guid);
       if (!optionEl) {
         optionEl = document.createElement("address-option");
         optionEl.value = guid;
       }
 
-      for (let [key, val] of Object.entries(address)) {
-        optionEl.setAttribute(key, val);
+      for (let key of AddressOption.recordAttributes) {
+        let val = address[key];
+        if (val) {
+          optionEl.setAttribute(key, val);
+        } else {
+          optionEl.removeAttribute(key);
+        }
       }
+
       desiredOptions.push(optionEl);
     }
 
     let el = null;
     while ((el = this.dropdown.popupBox.querySelector(":scope > address-option"))) {
       el.remove();
     }
     for (let option of desiredOptions) {
--- a/toolkit/components/payments/res/containers/payment-method-picker.js
+++ b/toolkit/components/payments/res/containers/payment-method-picker.js
@@ -1,13 +1,14 @@
 /* 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/. */
 
-/* global PaymentStateSubscriberMixin */
+/* import-globals-from ../mixins/PaymentStateSubscriberMixin.js */
+/* import-globals-from ../components/basic-card-option.js */
 
 "use strict";
 
 /**
  * <payment-method-picker></payment-method-picker>
  * Container around add/edit links and <rich-select> with
  * <basic-card-option> listening to savedBasicCards.
  */
@@ -46,18 +47,23 @@ class PaymentMethodPicker extends Paymen
     let {savedBasicCards} = state;
     let desiredOptions = [];
     for (let [guid, basicCard] of Object.entries(savedBasicCards)) {
       let optionEl = this.dropdown.getOptionByValue(guid);
       if (!optionEl) {
         optionEl = document.createElement("basic-card-option");
         optionEl.value = guid;
       }
-      for (let [key, val] of Object.entries(basicCard)) {
-        optionEl.setAttribute(key, val);
+      for (let key of BasicCardOption.recordAttributes) {
+        let val = basicCard[key];
+        if (val) {
+          optionEl.setAttribute(key, val);
+        } else {
+          optionEl.removeAttribute(key);
+        }
       }
       desiredOptions.push(optionEl);
     }
     let el = null;
     while ((el = this.dropdown.popupBox.querySelector(":scope > basic-card-option"))) {
       el.remove();
     }
     for (let option of desiredOptions) {
--- a/toolkit/components/payments/test/mochitest/test_address_picker.html
+++ b/toolkit/components/payments/test/mochitest/test_address_picker.html
@@ -80,17 +80,17 @@ add_task(async function test_initialSet(
 });
 
 add_task(async function test_update() {
   picker1.requestStore.setState({
     savedAddresses: {
       "48bnds6854t": {
         // Same GUID, different values to trigger an update
         "address-level1": "MI-edit",
-        "address-level2": "Some City-edit",
+        // address-level2 was cleared which means it's not returned
         "country": "CA",
         "guid": "48bnds6854t",
         "name": "Mr. Foo-edit",
         "postal-code": "90210-1234",
         "street-address": "new-edit",
         "tel": "+1 650 555-5555",
       },
       "68gjdh354j": {
@@ -104,17 +104,17 @@ add_task(async function test_update() {
         "tel": "+1 650 555-5555",
       },
     },
   });
   await asyncElementRendered();
   let options = picker1.dropdown.popupBox.children;
   is(options.length, 2, "Check dropdown still has both addresses");
   ok(options[0].textContent.includes("MI-edit"), "Check updated first address-level1");
-  ok(options[0].textContent.includes("Some City-edit"), "Check updated first address-level2");
+  ok(!options[0].textContent.includes("Some City"), "Check removed first address-level2");
   ok(options[0].textContent.includes("new-edit"), "Check updated first address");
 
   ok(options[1].textContent.includes("P.O. Box 123"), "Check second address is the same");
 });
 
 add_task(async function test_change_selected_address() {
   let options = picker1.dropdown.popupBox.children;
   let selectedOption = picker1.dropdown.selectedOption;
--- a/toolkit/components/payments/test/mochitest/test_payer_address_picker.html
+++ b/toolkit/components/payments/test/mochitest/test_payer_address_picker.html
@@ -258,17 +258,17 @@ add_task(async function test_filtered_op
     selectedPayerAddress: "a9e830667189",
   });
 
   await asyncElementRendered();
 
   let visibleOptions = getVisiblePickerOptions(elPicker);
   let visibleOption = visibleOptions[0];
 
-  is(elPicker.dropdown.popupBox.children.length, 3, "Check dropdown has 3 addresses");
+  is(elPicker.dropdown.popupBox.children.length, 4, "Check dropdown has 3 addresses");
   is(visibleOptions.length, 1, "One option should be visible");
   is(visibleOption.getAttribute("guid"), "a9e830667189", "expected option is visible");
 
   for (let fieldName of ["name", "email"]) {
     let elem = visibleOption.querySelector(`.${fieldName}`);
     ok(elem, `field ${fieldName} exists`);
     ok(isVisible(elem), `field ${fieldName} is visible`);
   }
@@ -287,22 +287,31 @@ add_task(async function test_filtered_op
   is(elPicker.dropdown.popupBox.children.length, 4, "Check dropdown has 4 addresses");
   is(visibleOptions.length, 1, "One option should be visible");
 });
 
 add_task(async function test_no_matches() {
   await setup();
   let requestStore = elPicker.requestStore;
   setPaymentOptions(requestStore, {
-    requestPayerEmail: true,
     requestPayerPhone: true,
   });
 
   requestStore.setState({
-    savedAddresses: DUPED_ADDRESSES,
+    savedAddresses: {
+      "2b4dce0fbc1f": {
+        "email": "rita@foo.com",
+        "guid": "2b4dce0fbc1f",
+        "name": "Rita Foo",
+      },
+      "46b2635a5b26": {
+        "guid": "46b2635a5b26",
+        "name": "Rita Foo",
+      },
+    },
     selectedPayerAddress: "a9e830667189",
   });
 
   await asyncElementRendered();
 
   let visibleOptions = getVisiblePickerOptions(elPicker);
   is(elPicker.dropdown.popupBox.children.length, 0, "Check dropdown is empty");
   is(visibleOptions.length, 0, "No options should be visible");
--- a/toolkit/components/payments/test/mochitest/test_payment_method_picker.html
+++ b/toolkit/components/payments/test/mochitest/test_payment_method_picker.html
@@ -78,34 +78,34 @@ add_task(async function test_initialSet(
 add_task(async function test_update() {
   picker1.requestStore.setState({
     savedBasicCards: {
       "48bnds6854t": {
         // Same GUID, different values to trigger an update
         "cc-exp": "2017-09",
         "cc-exp-month": 9,
         "cc-exp-year": 2017,
-        "cc-name": "John Edit Doe",
+        // cc-name was cleared which means it's not returned
         "cc-number": "************9876",
         "guid": "48bnds6854t",
       },
       "68gjdh354j": {
         "cc-exp": "2017-08",
         "cc-exp-month": 8,
         "cc-exp-year": 2017,
         "cc-name": "J Smith",
         "cc-number": "***********1234",
         "guid": "68gjdh354j",
       },
     },
   });
   await asyncElementRendered();
   let options = picker1.dropdown.popupBox.children;
   is(options.length, 2, "Check dropdown still has both cards");
-  ok(options[0].textContent.includes("John Edit Doe"), "Check updated first cc-name");
+  ok(!options[0].textContent.includes("John Doe"), "Check cleared first cc-name");
   ok(options[0].textContent.includes("9876"), "Check updated first cc-number");
   ok(options[0].textContent.includes("09"), "Check updated first exp-month");
 
   ok(options[1].textContent.includes("J Smith"), "Check second card is the same");
 });
 
 add_task(async function test_change_selected_card() {
   let options = picker1.dropdown.popupBox.children;