[Autofill local card migration] Refactor the local_card_migration_manager

To simplify the logic of triggering from settings page, we need to refactor the current local_card_migration_manager:

1. Move the isServerCard to the personal_data_manager. To determine a card is whether a server card or has server dup only depends on the personal_data_manager. So later we can use this function directly without create local_card_migration_manager when we create new attribute for each credit card entry for the settings page. Add unittests in the personal_data_manager.

2. Extract the prepare migratable credit cards to a helper function. Later, when we implement checking migration requirements in the settings page, we do not need to check the migration requirement twice after we bridge back. So we can directly call PrepareMigratableCreditCard() and AttemptToOfferLocalCardMigration() for the settings page.

Bug: 852904
Change-Id: I12b90054e96a9cafb6905c39608c7a5c1cbc2f8c
Reviewed-on: https://chromium-review.googlesource.com/1167796Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Reviewed-by: Jared Saul <jsaul@google.com>
Commit-Queue: Sujie Zhu <sujiezhu@google.com>
Cr-Commit-Position: refs/heads/master@{#582729}

components/autofill/core/browser/local_card_migration_manager.cc

@@ -53,20 +53,8 @@ bool LocalCardMigrationManager::ShouldOfferLocalCardMigration(
   if (!IsCreditCardMigrationEnabled())
     return false;
 
-  std::vector<CreditCard*> local_credit_cards =
-      personal_data_manager_->GetLocalCreditCards();
-
-  // Empty previous state.
-  migratable_credit_cards_.clear();
-
-  // Initialize the local credit card list and queue for showing and uploading.
-  for (CreditCard* credit_card : local_credit_cards) {
-    // If the card is valid (has a valid card number, expiration date, and is
-    // not expired) and is not a server card, add it to the list of migratable
-    // cards.
-    if (credit_card->IsValid() && !IsServerCard(credit_card))
-      migratable_credit_cards_.push_back(MigratableCreditCard(*credit_card));
-  }
+  // Fetch all migratable credit cards and store in |migratable_credit_cards_|.
+  GetMigratableCreditCards();
 
   // If the form was submitted with a local card, only offer migration instead
   // of Upstream if there are other local cards to migrate as well. If the form
@@ -174,15 +162,23 @@ int LocalCardMigrationManager::GetDetectedValues() const {
   return detected_values;
 }
 
-bool LocalCardMigrationManager::IsServerCard(CreditCard* local_card) const {
-  std::vector<CreditCard*> server_credit_cards =
-      personal_data_manager_->GetServerCreditCards();
-  // Check whether the current card is already uploaded.
-  for (const CreditCard* server_card : server_credit_cards) {
-    if (local_card->HasSameNumberAs(*server_card))
-      return true;
+void LocalCardMigrationManager::GetMigratableCreditCards() {
+  std::vector<CreditCard*> local_credit_cards =
+      personal_data_manager_->GetLocalCreditCards();
+
+  // Empty previous state.
+  migratable_credit_cards_.clear();
+
+  // Initialize the local credit card list and queue for showing and uploading.
+  for (CreditCard* credit_card : local_credit_cards) {
+    // If the card is valid (has a valid card number, expiration date, and is
+    // not expired) and is not a server card, add it to the list of migratable
+    // cards.
+    if (credit_card->IsValid() &&
+        !personal_data_manager_->IsServerCard(credit_card)) {
+      migratable_credit_cards_.push_back(MigratableCreditCard(*credit_card));
+    }
   }
-  return false;
 }
 
 }  // namespace autofill

components/autofill/core/browser/local_card_migration_manager.h

@@ -84,8 +84,8 @@ class LocalCardMigrationManager {
       const base::string16& context_token,
       std::unique_ptr<base::DictionaryValue> legal_message);
 
-  // Check whether a local card is already a server card.
-  bool IsServerCard(CreditCard* local_card) const;
+  // Fetch all migratable credit cards and store in |migratable_credit_cards_|.
+  void GetMigratableCreditCards();
 
   AutofillClient* const client_;
 

components/autofill/core/browser/personal_data_manager.cc

@@ -1993,6 +1993,20 @@ bool PersonalDataManager::IsKnownCard(const CreditCard& credit_card) {
   return false;
 }
 
+bool PersonalDataManager::IsServerCard(const CreditCard* credit_card) const {
+  // Check whether the current card itself is a server card.
+  if (credit_card->record_type() != autofill::CreditCard::LOCAL_CARD)
+    return true;
+
+  std::vector<CreditCard*> server_credit_cards = GetServerCreditCards();
+  // Check whether the current card is already uploaded.
+  for (const CreditCard* server_card : server_credit_cards) {
+    if (credit_card->HasSameNumberAs(*server_card))
+      return true;
+  }
+  return false;
+}
+
 std::vector<Suggestion> PersonalDataManager::GetSuggestionsForCards(
     const AutofillType& type,
     const base::string16& field_contents,

components/autofill/core/browser/personal_data_manager.h

@@ -336,6 +336,9 @@ class PersonalDataManager : public KeyedService,
   // |credit_card| is equal to any masked server card known by the browser.
   bool IsKnownCard(const CreditCard& credit_card);
 
+  // Check whether a card is a server card or has a duplicated server card.
+  bool IsServerCard(const CreditCard* credit_card) const;
+
   // Sets the value that can skip the checks to see if we are syncing in a test.
   void SetSyncingForTest(bool is_syncing_for_test) {
     is_syncing_for_test_ = is_syncing_for_test;

components/autofill/core/browser/personal_data_manager_unittest.cc

@@ -2190,6 +2190,102 @@ TEST_F(PersonalDataManagerTest, IsKnownCard_LastFourDoesNotMatch) {
   ASSERT_FALSE(personal_data_->IsKnownCard(cardToCompare));
 }
 
+TEST_F(PersonalDataManagerTest, IsServerCard_DuplicateOfFullServerCard) {
+  // Add a full server card.
+  std::vector<CreditCard> server_cards;
+  server_cards.push_back(CreditCard(CreditCard::FULL_SERVER_CARD, "b459"));
+  test::SetCreditCardInfo(&server_cards.back(), "Emmet Dalton",
+                          "4234567890122110" /* Visa */, "12", "2999", "1");
+
+  SetServerCards(server_cards);
+
+  // Add a dupe local card of a full server card.
+  CreditCard local_card("287151C8-6AB1-487C-9095-28E80BE5DA15",
+                        test::kEmptyOrigin);
+  test::SetCreditCardInfo(&local_card, "Emmet Dalton",
+                          "4234 5678 9012 2110" /* Visa */, "12", "2999", "1");
+  personal_data_->AddCreditCard(local_card);
+
+  // Make sure everything is set up correctly.
+  personal_data_->Refresh();
+  WaitForOnPersonalDataChanged();
+  EXPECT_EQ(2U, personal_data_->GetCreditCards().size());
+
+  CreditCard cardToCompare;
+  cardToCompare.SetNumber(base::ASCIIToUTF16("4234 5678 9012 2110") /* Visa */);
+  ASSERT_TRUE(personal_data_->IsServerCard(&cardToCompare));
+  ASSERT_TRUE(personal_data_->IsServerCard(&local_card));
+}
+
+TEST_F(PersonalDataManagerTest, IsServerCard_DuplicateOfMaskedServerCard) {
+  // Add a masked server card.
+  std::vector<CreditCard> server_cards;
+  server_cards.push_back(CreditCard(CreditCard::MASKED_SERVER_CARD, "b459"));
+  test::SetCreditCardInfo(&server_cards.back(), "Emmet Dalton",
+                          "2110" /* last 4 digits */, "12", "2999", "1");
+  server_cards.back().SetNetworkForMaskedCard(kVisaCard);
+
+  SetServerCards(server_cards);
+
+  // Add a dupe local card of a full server card.
+  CreditCard local_card("287151C8-6AB1-487C-9095-28E80BE5DA15",
+                        test::kEmptyOrigin);
+  test::SetCreditCardInfo(&local_card, "Emmet Dalton",
+                          "4234 5678 9012 2110" /* Visa */, "12", "2999", "1");
+  personal_data_->AddCreditCard(local_card);
+
+  // Make sure everything is set up correctly.
+  personal_data_->Refresh();
+  WaitForOnPersonalDataChanged();
+  EXPECT_EQ(2U, personal_data_->GetCreditCards().size());
+
+  CreditCard cardToCompare;
+  cardToCompare.SetNumber(base::ASCIIToUTF16("4234 5678 9012 2110") /* Visa */);
+  ASSERT_TRUE(personal_data_->IsServerCard(&cardToCompare));
+  ASSERT_TRUE(personal_data_->IsServerCard(&local_card));
+}
+
+TEST_F(PersonalDataManagerTest, IsServerCard_AlreadyServerCard) {
+  std::vector<CreditCard> server_cards;
+  // Create a full server card.
+  CreditCard full_server_card(CreditCard::FULL_SERVER_CARD, "c789");
+  test::SetCreditCardInfo(&full_server_card, "Homer Simpson",
+                          "4234567890123456" /* Visa */, "01", "2999", "1");
+  server_cards.push_back(full_server_card);
+  // Create a masked server card.
+  CreditCard masked_card(CreditCard::MASKED_SERVER_CARD, "a123");
+  test::SetCreditCardInfo(&masked_card, "Homer Simpson", "2110" /* Visa */,
+                          "01", "2999", "1");
+  masked_card.SetNetworkForMaskedCard(kVisaCard);
+  server_cards.push_back(masked_card);
+
+  SetServerCards(server_cards);
+
+  // Make sure everything is set up correctly.
+  personal_data_->Refresh();
+  WaitForOnPersonalDataChanged();
+  EXPECT_EQ(2U, personal_data_->GetCreditCards().size());
+
+  ASSERT_TRUE(personal_data_->IsServerCard(&full_server_card));
+  ASSERT_TRUE(personal_data_->IsServerCard(&masked_card));
+}
+
+TEST_F(PersonalDataManagerTest, IsServerCard_UniqueLocalCard) {
+  // Add a unique local card.
+  CreditCard local_card("1141084B-72D7-4B73-90CF-3D6AC154673B",
+                        test::kEmptyOrigin);
+  test::SetCreditCardInfo(&local_card, "Homer Simpson",
+                          "4234567890123456" /* Visa */, "01", "2999", "1");
+  personal_data_->AddCreditCard(local_card);
+
+  // Make sure everything is set up correctly.
+  personal_data_->Refresh();
+  WaitForOnPersonalDataChanged();
+  EXPECT_EQ(1U, personal_data_->GetCreditCards().size());
+
+  ASSERT_FALSE(personal_data_->IsServerCard(&local_card));
+}
+
 // Test that a masked server card is not suggested if more that six numbers have
 // been typed in the field.
 TEST_F(PersonalDataManagerTest,