Reland: [AF] Integration tests for changing additional info for wallet metadata

This CL adds more integration tests; it also fixes a bug that
stopped local changes of billing address and local changes of
has_converted to get propagated to sync.

Compared to the original CL (patch set 1 here), patch set 2 in this CL
fixes (a) the fixture to make sure PDM converts addresses (a bug
introduced after landing the original CL) and (b) the Checker to wait
until PDM converts remote addresses to local ones (the reason for the
flakiness).

Bug: 894001
Change-Id: Idf4e87ae36f7ed6348250a5d69bcd66b1f16ec54
Reviewed-on: https://chromium-review.googlesource.com/c/1350614
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610842}

chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc

@@ -423,7 +423,8 @@ IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest,
   InitWithDefaultFeatures();
 
   GetFakeServer()->SetWalletData(
-      {CreateSyncWalletCard(/*name=*/"card-1", /*last_four=*/"0001"),
+      {CreateSyncWalletCard(/*name=*/"card-1", /*last_four=*/"0001",
+                            kDefaultBillingAddressID),
        CreateSyncWalletAddress(/*name=*/"address-1", /*company=*/"Company-1"),
        CreateDefaultSyncPaymentsCustomerData()});
   ASSERT_TRUE(SetupSync());
@@ -442,7 +443,8 @@ IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest,
 
   // Put some completely new data in the sync server.
   GetFakeServer()->SetWalletData(
-      {CreateSyncWalletCard(/*name=*/"new-card", /*last_four=*/"0002"),
+      {CreateSyncWalletCard(/*name=*/"new-card", /*last_four=*/"0002",
+                            kDefaultBillingAddressID),
        CreateSyncWalletAddress(/*name=*/"new-address", /*company=*/"Company-2"),
        CreateSyncPaymentsCustomerData(/*customer_id=*/"newid")});
 
@@ -471,7 +473,8 @@ IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest,
 IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest, EmptyUpdatesAreIgnored) {
   InitWithDefaultFeatures();
   GetFakeServer()->SetWalletData(
-      {CreateSyncWalletCard(/*name=*/"card-1", /*last_four=*/"0001"),
+      {CreateSyncWalletCard(/*name=*/"card-1", /*last_four=*/"0001",
+                            kDefaultBillingAddressID),
        CreateSyncWalletAddress(/*name=*/"address-1", /*company=*/"Company-1"),
        CreateDefaultSyncPaymentsCustomerData()});
   ASSERT_TRUE(SetupSync());

chrome/browser/sync/test/integration/wallet_helper.cc

@@ -112,7 +112,7 @@ void LogLists(const std::vector<Item*>& list_a,
   }
 }
 
-bool WalletDataAndMetadataMatchImpl(
+bool WalletDataAndMetadataMatchAndAddressesHaveConverted(
     int profile_a,
     const std::vector<CreditCard*>& server_cards_a,
     const std::vector<AutofillProfile*>& server_profiles_a,
@@ -127,6 +127,12 @@ bool WalletDataAndMetadataMatchImpl(
     LogLists(server_profiles_a, server_profiles_b);
     return false;
   }
+  // Check that all server profiles have converted to local ones.
+  for (AutofillProfile* profile : server_profiles_a) {
+    if (!profile->has_converted()) {
+      return false;
+    }
+  }
   return true;
 }
 
@@ -238,11 +244,14 @@ void UpdateServerAddressMetadata(int profile,
 }
 
 sync_pb::SyncEntity CreateDefaultSyncWalletCard() {
-  return CreateSyncWalletCard(kDefaultCardID, kDefaultCardLastFour);
+  return CreateSyncWalletCard(kDefaultCardID, kDefaultCardLastFour,
+                              kDefaultBillingAddressID);
 }
 
-sync_pb::SyncEntity CreateSyncWalletCard(const std::string& name,
-                                         const std::string& last_four) {
+sync_pb::SyncEntity CreateSyncWalletCard(
+    const std::string& name,
+    const std::string& last_four,
+    const std::string& billing_address_id) {
   sync_pb::SyncEntity entity;
   entity.set_name(name);
   entity.set_id_string(name);
@@ -263,7 +272,9 @@ sync_pb::SyncEntity CreateSyncWalletCard(const std::string& name,
   credit_card->set_name_on_card(kDefaultCardName);
   credit_card->set_status(sync_pb::WalletMaskedCreditCard::VALID);
   credit_card->set_type(kDefaultCardType);
-  credit_card->set_billing_address_id(kDefaultBillingAddressID);
+  if (!billing_address_id.empty()) {
+    credit_card->set_billing_address_id(billing_address_id);
+  }
   return entity;
 }
 
@@ -397,6 +408,12 @@ std::vector<AutofillProfile*> GetServerProfiles(int profile) {
   return pdm->GetServerProfiles();
 }
 
+std::vector<AutofillProfile*> GetLocalProfiles(int profile) {
+  WaitForPDMToRefresh(profile);
+  PersonalDataManager* pdm = GetPersonalDataManager(profile);
+  return pdm->GetProfiles();
+}
+
 std::vector<CreditCard*> GetServerCreditCards(int profile) {
   WaitForPDMToRefresh(profile);
   PersonalDataManager* pdm = GetPersonalDataManager(profile);
@@ -430,7 +447,7 @@ bool AutofillWalletChecker::IsExitConditionSatisfied() {
       wallet_helper::GetPersonalDataManager(profile_a_);
   autofill::PersonalDataManager* pdm_b =
       wallet_helper::GetPersonalDataManager(profile_b_);
-  return WalletDataAndMetadataMatchImpl(
+  return WalletDataAndMetadataMatchAndAddressesHaveConverted(
       profile_a_, pdm_a->GetServerCreditCards(), pdm_a->GetServerProfiles(),
       profile_b_, pdm_b->GetServerCreditCards(), pdm_b->GetServerProfiles());
 }

chrome/browser/sync/test/integration/wallet_helper.h

@@ -65,7 +65,8 @@ void UpdateServerAddressMetadata(
 sync_pb::SyncEntity CreateDefaultSyncWalletCard();
 
 sync_pb::SyncEntity CreateSyncWalletCard(const std::string& name,
-                                         const std::string& last_four);
+                                         const std::string& last_four,
+                                         const std::string& billing_address_id);
 
 sync_pb::SyncEntity CreateSyncPaymentsCustomerData(
     const std::string& customer_id);
@@ -92,6 +93,7 @@ void ExpectDefaultProfileValues(const autofill::AutofillProfile& profile);
 
 // Load current data from the database of profile |profile|.
 std::vector<autofill::AutofillProfile*> GetServerProfiles(int profile);
+std::vector<autofill::AutofillProfile*> GetLocalProfiles(int profile);
 std::vector<autofill::CreditCard*> GetServerCreditCards(int profile);
 
 }  // namespace wallet_helper

components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc

@@ -155,6 +155,28 @@ void ApplyChangesToCache(const syncer::SyncChangeList& changes,
   }
 }
 
+template <class DataType>
+bool AreLocalUseStatsUpdated(const sync_pb::WalletMetadataSpecifics& remote,
+                             const DataType& local) {
+  return base::checked_cast<size_t>(remote.use_count()) < local.use_count() &&
+         base::Time::FromInternalValue(remote.use_date()) < local.use_date();
+}
+
+bool IsLocalBillingAddressUpdated(
+    const sync_pb::WalletMetadataSpecifics& remote,
+    const CreditCard& local) {
+  std::string remote_billing_address_id;
+  base::Base64Decode(remote.card_billing_address_id(),
+                     &remote_billing_address_id);
+  return local.billing_address_id() != remote_billing_address_id;
+}
+
+bool IsLocalHasConvertedStatusUpdated(
+    const sync_pb::WalletMetadataSpecifics& remote,
+    const AutofillProfile& local) {
+  return remote.address_has_converted() != local.has_converted();
+}
+
 // Merges the metadata of the remote and local versions of the data model.
 void MergeCommonMetadata(
     const sync_pb::WalletMetadataSpecifics& remote_metadata,
@@ -513,9 +535,20 @@ void AutofillWalletMetadataSyncableService::AutofillProfileChanged(
     // Implicitly, we filter out ADD (not in cache) and REMOVE (!data_model()).
     DCHECK(change.type() == AutofillProfileChange::UPDATE);
 
-    AutofillDataModelUpdated(
-        server_id, sync_pb::WalletMetadataSpecifics::ADDRESS,
-        it->GetSpecifics().wallet_metadata(), *change.data_model());
+    const sync_pb::WalletMetadataSpecifics& remote =
+        it->GetSpecifics().wallet_metadata();
+    const AutofillProfile& local = *change.data_model();
+
+    if (!AreLocalUseStatsUpdated(remote, local) &&
+        !IsLocalHasConvertedStatusUpdated(remote, local)) {
+      return;
+    }
+
+    SendChangesToSyncServer(syncer::SyncChangeList(
+        1, syncer::SyncChange(
+               FROM_HERE, syncer::SyncChange::ACTION_UPDATE,
+               BuildSyncData(sync_pb::WalletMetadataSpecifics::ADDRESS,
+                             server_id, local))));
   }
 }
 
@@ -536,9 +569,19 @@ void AutofillWalletMetadataSyncableService::CreditCardChanged(
     // Implicitly, we filter out ADD (not in cache) and REMOVE (!data_model()).
     DCHECK(change.type() == AutofillProfileChange::UPDATE);
 
-    AutofillDataModelUpdated(server_id, sync_pb::WalletMetadataSpecifics::CARD,
-                             it->GetSpecifics().wallet_metadata(),
-                             *change.data_model());
+    const sync_pb::WalletMetadataSpecifics& remote =
+        it->GetSpecifics().wallet_metadata();
+    const CreditCard& local = *change.data_model();
+    if (!AreLocalUseStatsUpdated(remote, local) &&
+        !IsLocalBillingAddressUpdated(remote, local)) {
+      return;
+    }
+
+    SendChangesToSyncServer(syncer::SyncChangeList(
+        1,
+        syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE,
+                           BuildSyncData(sync_pb::WalletMetadataSpecifics::CARD,
+                                         server_id, local))));
   }
 }
 
@@ -718,18 +761,4 @@ syncer::SyncMergeResult AutofillWalletMetadataSyncableService::MergeData(
   return result;
 }
 
-template <class DataType>
-void AutofillWalletMetadataSyncableService::AutofillDataModelUpdated(
-    const std::string& server_id,
-    const sync_pb::WalletMetadataSpecifics::Type& type,
-    const sync_pb::WalletMetadataSpecifics& remote,
-    const DataType& local) {
-  if (base::checked_cast<size_t>(remote.use_count()) < local.use_count() &&
-      base::Time::FromInternalValue(remote.use_date()) < local.use_date()) {
-    SendChangesToSyncServer(syncer::SyncChangeList(
-        1, syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE,
-                              BuildSyncData(remote.type(), server_id, local))));
-  }
-}
-
 }  // namespace autofill

components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h

@@ -130,15 +130,6 @@ class AutofillWalletMetadataSyncableService
   // is not present locally.
   syncer::SyncMergeResult MergeData(const syncer::SyncDataList& sync_data);
 
-  // Sends the autofill data model updates to the sync server if the local
-  // version is more recent. Used for both profiles and credit cards.
-  template <class DataType>
-  void AutofillDataModelUpdated(
-      const std::string& server_id,
-      const sync_pb::WalletMetadataSpecifics::Type& type,
-      const sync_pb::WalletMetadataSpecifics& remote,
-      const DataType& local);
-
   base::ThreadChecker thread_checker_;
   AutofillWebDataBackend* web_data_backend_;  // Weak ref.
   ScopedObserver<AutofillWebDataBackend, AutofillWalletMetadataSyncableService>

components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc

@@ -476,8 +476,8 @@ TEST_F(AutofillWalletMetadataSyncableServiceTest,
   MergeMetadata(&local_, &remote_);
 }
 
-// Verify that lower values of metadata are not sent to the sync server when
-// local metadata is updated.
+// Verify that lower or equal values of metadata are not sent to the sync server
+// when local metadata is updated.
 TEST_F(AutofillWalletMetadataSyncableServiceTest,
        DontSendLowerValueToServerOnSingleChange) {
   local_.UpdateAddressStats(BuildAddress(kAddr1, 1, 2, true));
@@ -485,8 +485,8 @@ TEST_F(AutofillWalletMetadataSyncableServiceTest,
   remote_.UpdateAddressStats(BuildAddress(kAddr1, 1, 2, true));
   remote_.UpdateCardStats(BuildCard(kCard1, 3, 4, kAddr1));
   MergeMetadata(&local_, &remote_);
-  AutofillProfile address = BuildAddress(kAddr1, 0, 0, false);
-  CreditCard card = BuildCard(kCard1, 0, 0, kAddr2);
+  AutofillProfile address = BuildAddress(kAddr1, 0, 0, true);
+  CreditCard card = BuildCard(kCard1, 3, 4, kAddr1);
 
   EXPECT_CALL(local_, SendChangesToSyncServer(_)).Times(0);
 
@@ -525,6 +525,35 @@ TEST_F(AutofillWalletMetadataSyncableServiceTest,
       CreditCardChange(CreditCardChange::UPDATE, card.guid(), &card));
 }
 
+// Verify that other changed metadata elements are sent to the sync server when
+// local metadata is updated.
+TEST_F(AutofillWalletMetadataSyncableServiceTest,
+       SendChangedMetadataToServerOnLocalSingleChange) {
+  local_.UpdateAddressStats(BuildAddress(kAddr1, 1, 2, false));
+  local_.UpdateCardStats(BuildCard(kCard1, 3, 4, kAddr1));
+  remote_.UpdateAddressStats(BuildAddress(kAddr1, 1, 2, false));
+  remote_.UpdateCardStats(BuildCard(kCard1, 3, 4, kAddr1));
+  MergeMetadata(&local_, &remote_);
+  AutofillProfile address = BuildAddress(kAddr1, 1, 2, true);
+  CreditCard card = BuildCard(kCard1, 3, 4, kAddr2);
+
+  EXPECT_CALL(
+      local_,
+      SendChangesToSyncServer(ElementsAre(SyncAddressChangeAndDataMatch(
+          syncer::SyncChange::ACTION_UPDATE, kAddr1SyncTag,
+          sync_pb::WalletMetadataSpecifics::ADDRESS, kAddr1Utf8, 1, 2, true))));
+  EXPECT_CALL(local_,
+              SendChangesToSyncServer(ElementsAre(SyncCardChangeAndDataMatch(
+                  syncer::SyncChange::ACTION_UPDATE, kCard1SyncTag,
+                  sync_pb::WalletMetadataSpecifics::CARD, kCard1Utf8, 3, 4,
+                  kAddr2Utf8))));
+
+  local_.AutofillProfileChanged(AutofillProfileChange(
+      AutofillProfileChange::UPDATE, address.guid(), &address));
+  local_.CreditCardChanged(
+      CreditCardChange(CreditCardChange::UPDATE, card.guid(), &card));
+}
+
 // Verify that one-off addition of metadata is not sent to the sync
 // server. Metadata add and delete trigger multiple changes notification
 // instead.