When updating credit card in settings use existing card instance.

The current code creates a new card instance and sets all the values
form the settings page. It then replaces the existing card instance in
the DB. This is dangerous because it's easy to forget some. It would
also reset all the metadata associated with this card.

More specifically in this example, the settings don't have a way to set
the billing address id. Instead of passing the value along in the
settings to set it back on save, using the existing card and updating it
is much more simple.

Bug: 800829
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ia6ce196c16295961bb6536a609ea866f953e1590
Reviewed-on: https://chromium-review.googlesource.com/860200
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Reviewed-by: Jared Saul <jsaul@google.com>
Cr-Commit-Position: refs/heads/master@{#531710}

chrome/browser/extensions/api/autofill_private/autofill_private_api.cc

@@ -186,8 +186,20 @@ ExtensionFunction::ResponseAction AutofillPrivateSaveAddressFunction::Run() {
 
   api::autofill_private::AddressEntry* address = &parameters->address;
 
+  // If a profile guid is specified, get a copy of the profile identified by it.
+  // Otherwise create a new one.
   std::string guid = address->guid ? *address->guid : "";
-  autofill::AutofillProfile profile(guid, kSettingsOrigin);
+  const bool use_existing_profile = !guid.empty();
+  const autofill::AutofillProfile* existing_profile = nullptr;
+  if (use_existing_profile) {
+    existing_profile = personal_data->GetProfileByGUID(guid);
+    if (!existing_profile)
+      return RespondNow(Error(kErrorDataUnavailable));
+  }
+  autofill::AutofillProfile profile =
+      existing_profile
+          ? *existing_profile
+          : autofill::AutofillProfile(base::GenerateGUID(), kSettingsOrigin);
 
   // Strings from JavaScript use UTF-8 encoding. This container is used as an
   // intermediate container for functions which require UTF-16 strings.
@@ -268,11 +280,10 @@ ExtensionFunction::ResponseAction AutofillPrivateSaveAddressFunction::Run() {
   if (address->language_code)
     profile.set_language_code(*address->language_code);
 
-  if (!base::IsValidGUID(profile.guid())) {
-    profile.set_guid(base::GenerateGUID());
-    personal_data->AddProfile(profile);
-  } else {
+  if (use_existing_profile) {
     personal_data->UpdateProfile(profile);
+  } else {
+    personal_data->AddProfile(profile);
   }
 
   return RespondNow(NoArguments());
@@ -373,8 +384,20 @@ ExtensionFunction::ResponseAction AutofillPrivateSaveCreditCardFunction::Run() {
 
   api::autofill_private::CreditCardEntry* card = &parameters->card;
 
+  // If a card guid is specified, get a copy of the card identified by it.
+  // Otherwise create a new one.
   std::string guid = card->guid ? *card->guid : "";
-  autofill::CreditCard credit_card(guid, kSettingsOrigin);
+  const bool use_existing_card = !guid.empty();
+  const autofill::CreditCard* existing_card = nullptr;
+  if (use_existing_card) {
+    existing_card = personal_data->GetCreditCardByGUID(guid);
+    if (!existing_card)
+      return RespondNow(Error(kErrorDataUnavailable));
+  }
+  autofill::CreditCard credit_card =
+      existing_card
+          ? *existing_card
+          : autofill::CreditCard(base::GenerateGUID(), kSettingsOrigin);
 
   if (card->name) {
     credit_card.SetRawInfo(autofill::CREDIT_CARD_NAME_FULL,
@@ -399,15 +422,10 @@ ExtensionFunction::ResponseAction AutofillPrivateSaveCreditCardFunction::Run() {
         base::UTF8ToUTF16(*card->expiration_year));
   }
 
-  if (card->billing_address_id) {
-    credit_card.set_billing_address_id(*card->billing_address_id);
-  }
-
-  if (!base::IsValidGUID(credit_card.guid())) {
-    credit_card.set_guid(base::GenerateGUID());
-    personal_data->AddCreditCard(credit_card);
-  } else {
+  if (use_existing_card) {
     personal_data->UpdateCreditCard(credit_card);
+  } else {
+    personal_data->AddCreditCard(credit_card);
   }
 
   return RespondNow(NoArguments());

chrome/browser/extensions/api/autofill_private/autofill_private_apitest.cc

@@ -44,11 +44,6 @@ class AutofillPrivateApiTest : public ExtensionApiTest {
 
 // TODO(hcarmona): Investigate converting these tests to unittests.
 
-// TODO(crbug.com/643097) Disabled for flakiness.
-IN_PROC_BROWSER_TEST_F(AutofillPrivateApiTest, DISABLED_SaveAddress) {
-  EXPECT_TRUE(RunAutofillSubtest("saveAddress")) << message_;
-}
-
 // TODO(crbug.com/643097) Disabled for flakiness.
 IN_PROC_BROWSER_TEST_F(AutofillPrivateApiTest, DISABLED_GetCountryList) {
   EXPECT_TRUE(RunAutofillSubtest("getCountryList")) << message_;
@@ -58,11 +53,6 @@ IN_PROC_BROWSER_TEST_F(AutofillPrivateApiTest, GetAddressComponents) {
   EXPECT_TRUE(RunAutofillSubtest("getAddressComponents")) << message_;
 }
 
-// TODO(crbug.com/643097) Disabled for flakiness.
-IN_PROC_BROWSER_TEST_F(AutofillPrivateApiTest, DISABLED_SaveCreditCard) {
-  EXPECT_TRUE(RunAutofillSubtest("saveCreditCard")) << message_;
-}
-
 // TODO(crbug.com/643097) Disabled for flakiness.
 IN_PROC_BROWSER_TEST_F(AutofillPrivateApiTest, DISABLED_RemoveEntry) {
   EXPECT_TRUE(RunAutofillSubtest("removeEntry")) << message_;
@@ -72,5 +62,13 @@ IN_PROC_BROWSER_TEST_F(AutofillPrivateApiTest, ValidatePhoneNumbers) {
   EXPECT_TRUE(RunAutofillSubtest("ValidatePhoneNumbers")) << message_;
 }
 
+IN_PROC_BROWSER_TEST_F(AutofillPrivateApiTest, AddAndUpdateAddress) {
+  EXPECT_TRUE(RunAutofillSubtest("addAndUpdateAddress")) << message_;
+}
+
+IN_PROC_BROWSER_TEST_F(AutofillPrivateApiTest, AddAndUpdateCreditCard) {
+  EXPECT_TRUE(RunAutofillSubtest("addAndUpdateCreditCard")) << message_;
+}
+
 }  // namespace extensions
 

chrome/browser/extensions/api/autofill_private/autofill_util.cc

@@ -145,8 +145,6 @@ autofill_private::CreditCardEntry CreditCardToCreditCardEntry(
       credit_card.GetRawInfo(autofill::CREDIT_CARD_EXP_MONTH))));
   card.expiration_year.reset(new std::string(base::UTF16ToUTF8(
       credit_card.GetRawInfo(autofill::CREDIT_CARD_EXP_4_DIGIT_YEAR))));
-  card.billing_address_id.reset(
-      new std::string(credit_card.billing_address_id()));
 
   // Create address metadata and add it to |address|.
   std::unique_ptr<autofill_private::AutofillMetadata> metadata(

chrome/common/extensions/api/autofill_private.idl

@@ -157,9 +157,6 @@ namespace autofillPrivate {
     // Year as a 4-character string (as in "2015").
     DOMString? expirationYear;
 
-    // The identifier of the billing address for the card.
-    DOMString? billingAddressId;
-
     AutofillMetadata? metadata;
   };
 

chrome/test/data/extensions/api_test/autofill_private/test.js

@@ -6,29 +6,23 @@
 // that callbacks are correctly invoked, expected parameters are correct,
 // and failures are detected.
 
-var availableTests = [
-  function saveAddress() {
-    var NAME = 'Name';
-
-    var numCalls = 0;
-    var handler = function(addressList) {
-      numCalls++;
-
-      if (numCalls == 1) {
-        chrome.test.assertEq(addressList.length, 0);
-      } else {
-        chrome.test.assertEq(addressList.length, 1);
-        var address = addressList[0];
-        chrome.test.assertEq(address.fullNames[0], NAME);
-        chrome.test.succeed();
-      }
-    }
-
-    chrome.autofillPrivate.onAddressListChanged.addListener(handler);
-    chrome.autofillPrivate.getAddressList(handler);
-    chrome.autofillPrivate.saveAddress({fullNames: [NAME]});
-  },
+// Constants for the tests.
+var NAME = 'Name';
+var COMPANY_NAME = 'Company name';
+var ADDRESS_LEVEL1 = 'Address level 1';
+var ADDRESS_LEVEL2 = 'Address level 2';
+var ADDRESS_LEVEL3 = 'Address level 3';
+var POSTAL_CODE = 'Postal code';
+var SORTING_CODE = 'Sorting code';
+var COUNTRY_CODE = 'US';
+var PHONE = '1 123-123-1234';
+var EMAIL = 'johndoe@gmail.com';
+var CARD_NAME = 'CardName';
+var NUMBER = '4111 1111 1111 1111';
+var EXP_MONTH = '02';
+var EXP_YEAR = '2999';
 
+var availableTests = [
   function getCountryList() {
     var handler = function(countries) {
       var numSeparators = 0;
@@ -83,30 +77,197 @@ var availableTests = [
     chrome.autofillPrivate.getAddressComponents(COUNTRY_CODE, handler);
   },
 
-  function saveCreditCard() {
-    var NAME = 'Name';
+  function addNewAddress() {
+    function filterAddressProperties(addresses) {
+      return addresses.map(address => {
+        var filteredAddress = {};
+        ['fullNames', 'addressLevel1', 'addressLevel2', 'addressLevel3',
+         'postalCode', 'sortingCode', 'phoneNumbers', 'emailAddresses']
+            .forEach(property => {
+              filteredAddress[property] = address[property];
+            });
+        return filteredAddress;
+      });
+    }
 
-    var numCalls = 0;
-    var handler = function(creditCardList) {
-      numCalls++;
+    chrome.autofillPrivate.getAddressList(
+        chrome.test.callbackPass(function(addressList) {
+          chrome.test.assertEq([], addressList);
+
+          // Setup the callback that verifies that the address was correctly
+          // added.
+          chrome.test.listenOnce(
+              chrome.autofillPrivate.onAddressListChanged,
+              chrome.test.callbackPass(function(addressList) {
+                chrome.test.assertEq(
+                    [{
+                      fullNames: [NAME],
+                      addressLevel1: ADDRESS_LEVEL1,
+                      addressLevel2: ADDRESS_LEVEL2,
+                      addressLevel3: ADDRESS_LEVEL3,
+                      postalCode: POSTAL_CODE,
+                      sortingCode: SORTING_CODE,
+                      phoneNumbers: [PHONE],
+                      emailAddresses: [EMAIL]
+                    }],
+                    filterAddressProperties(addressList));
+              }));
+
+          chrome.autofillPrivate.saveAddress({
+            fullNames: [NAME],
+            addressLevel1: ADDRESS_LEVEL1,
+            addressLevel2: ADDRESS_LEVEL2,
+            addressLevel3: ADDRESS_LEVEL3,
+            postalCode: POSTAL_CODE,
+            sortingCode: SORTING_CODE,
+            countryCode: COUNTRY_CODE,
+            phoneNumbers: [PHONE],
+            emailAddresses: [EMAIL]
+          });
+        }));
+  },
 
-      if (numCalls == 1) {
-        chrome.test.assertEq(creditCardList.length, 0);
-      } else {
-        chrome.test.assertEq(creditCardList.length, 1);
-        var creditCard = creditCardList[0];
-        chrome.test.assertEq(creditCard.name, NAME);
-        chrome.test.succeed();
+  function updateExistingAddress() {
+    // The information that will be updated. It should be different than the
+    // information in the addNewAddress function.
+    var UPDATED_NAME = 'UpdatedName';
+    var UPDATED_PHONE = '1 987-987-9876'
+
+    function filterAddressProperties(addresses) {
+      return addresses.map(address => {
+        var filteredAddress = {};
+        ['guid', 'fullNames', 'addressLevel1', 'addressLevel2', 'addressLevel3',
+         'postalCode', 'sortingCode', 'phoneNumbers', 'emailAddresses']
+            .forEach(property => {
+              filteredAddress[property] = address[property];
+            });
+        return filteredAddress;
+      });
     }
+
+    chrome.autofillPrivate.getAddressList(
+        chrome.test.callbackPass(function(addressList) {
+          // The address from the addNewAddress function should still be there.
+          chrome.test.assertEq(1, addressList.length);
+          var addressGuid = addressList[0].guid;
+
+          // Setup the callback that verifies that the address was correctly
+          // updated.
+          chrome.test.listenOnce(
+              chrome.autofillPrivate.onAddressListChanged,
+              chrome.test.callbackPass(function(addressList) {
+                chrome.test.assertEq(
+                    [{
+                      guid: addressGuid,
+                      fullNames: [UPDATED_NAME],
+                      addressLevel1: ADDRESS_LEVEL1,
+                      addressLevel2: ADDRESS_LEVEL2,
+                      addressLevel3: ADDRESS_LEVEL3,
+                      postalCode: POSTAL_CODE,
+                      sortingCode: SORTING_CODE,
+                      phoneNumbers: [UPDATED_PHONE],
+                      emailAddresses: [EMAIL]
+                    }],
+                    filterAddressProperties(addressList));
+              }));
+
+          // Update the address by saving an address with the same guid and
+          // using some different information.
+          chrome.autofillPrivate.saveAddress({
+            guid: addressGuid,
+            fullNames: [UPDATED_NAME],
+            phoneNumbers: [UPDATED_PHONE]
+          });
+        }));
+  },
+
+  function addNewCreditCard() {
+    function filterCardProperties(cards) {
+      return cards.map(cards => {
+        var filteredCards = {};
+        ['name', 'cardNumber', 'expirationMonth', 'expirationYear'].forEach(
+            property => {
+              filteredCards[property] = cards[property];
+            });
+        return filteredCards;
+      });
     }
 
-    chrome.autofillPrivate.onCreditCardListChanged.addListener(handler);
-    chrome.autofillPrivate.getCreditCardList(handler);
-    chrome.autofillPrivate.saveCreditCard({name: NAME});
+    chrome.autofillPrivate.getCreditCardList(
+        chrome.test.callbackPass(function(cardList) {
+          chrome.test.assertEq([], cardList);
+
+          // Setup the callback that verifies that the card was correctly added.
+          chrome.test.listenOnce(
+              chrome.autofillPrivate.onCreditCardListChanged,
+              chrome.test.callbackPass(function(cardList) {
+                chrome.test.assertEq(
+                    [{
+                      name: CARD_NAME,
+                      cardNumber: NUMBER,
+                      expirationMonth: EXP_MONTH,
+                      expirationYear: EXP_YEAR
+                    }],
+                    filterCardProperties(cardList));
+              }));
+
+          chrome.autofillPrivate.saveCreditCard({
+            name: CARD_NAME,
+            cardNumber: NUMBER,
+            expirationMonth: EXP_MONTH,
+            expirationYear: EXP_YEAR
+          });
+        }));
+  },
+
+  function updateExistingCreditCard() {
+    var UPDATED_CARD_NAME = 'UpdatedCardName';
+    var UPDATED_EXP_YEAR = '2888';
+
+    function filterCardProperties(cards) {
+      return cards.map(cards => {
+        var filteredCards = {};
+        ['guid', 'name', 'cardNumber', 'expirationMonth', 'expirationYear']
+            .forEach(property => {
+              filteredCards[property] = cards[property];
+            });
+        return filteredCards;
+      });
+    }
+
+    chrome.autofillPrivate.getCreditCardList(
+        chrome.test.callbackPass(function(cardList) {
+          // The card from the addNewCreditCard function should still be there.
+          chrome.test.assertEq(1, cardList.length);
+          var cardGuid = cardList[0].guid;
+
+          // Setup the callback that verifies that the address was correctly
+          // updated.
+          chrome.test.listenOnce(
+              chrome.autofillPrivate.onCreditCardListChanged,
+              chrome.test.callbackPass(function(cardList) {
+                chrome.test.assertEq(
+                    [{
+                      guid: cardGuid,
+                      name: UPDATED_CARD_NAME,
+                      cardNumber: NUMBER,
+                      expirationMonth: EXP_MONTH,
+                      expirationYear: UPDATED_EXP_YEAR
+                    }],
+                    filterCardProperties(cardList));
+              }));
+
+          // Update the card by saving a card with the same guid and using some
+          // different information.
+          chrome.autofillPrivate.saveCreditCard({
+            guid: cardGuid,
+            name: UPDATED_CARD_NAME,
+            expirationYear: UPDATED_EXP_YEAR
+          });
+        }));
   },
 
   function removeEntry() {
-    var NAME = 'Name';
     var guid;
 
     var numCalls = 0;
@@ -180,7 +341,14 @@ var availableTests = [
   },
 ];
 
-var testToRun = window.location.search.substring(1);
+/** @const */
+var TESTS_FOR_CONFIG = {
+  'addAndUpdateAddress': ['addNewAddress', 'updateExistingAddress'],
+  'addAndUpdateCreditCard': ['addNewCreditCard', 'updateExistingCreditCard']
+};
+
+var testConfig = window.location.search.substring(1);
+var testsToRun = TESTS_FOR_CONFIG[testConfig] || [testConfig];
 chrome.test.runTests(availableTests.filter(function(op) {
-  return op.name == testToRun;
+  return testsToRun.includes(op.name);
 }));

third_party/closure_compiler/externs/autofill_private.js

@@ -108,7 +108,6 @@ chrome.autofillPrivate.AddressComponents;
  *   cardNumber: (string|undefined),
  *   expirationMonth: (string|undefined),
  *   expirationYear: (string|undefined),
- *   billingAddressId: (string|undefined),
  *   metadata: (!chrome.autofillPrivate.AutofillMetadata|undefined)
  * }}
  * @see https://developer.chrome.com/extensions/autofillPrivate#type-CreditCardEntry