Bug 1428414 - Removed cleared record attributes on card and address options. r=jaws
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Thu, 22 Mar 2018 23:01:32 -0700
changeset 411561 6045035ad2edad1ed5a8c336883db01aa73bd463
parent 411560 b045a7779f4ddc2a2dcd93cbfe187d4b880a1f57
child 411562 70eb77d5bd24e93c06d4547b028cdf9d12c5cce7
push id101686
push useraciure@mozilla.com
push dateTue, 03 Apr 2018 21:59:31 +0000
treeherdermozilla-inbound@8d846598d35d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs1428414
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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;