[AF] Tweak local card migration strike settings

The following changes have been made at the request of Chrome Privacy:
- Closing promo bubble now gives 3 strikes instead of 2
- Declining main bubble now gives 6 (max) strikes instead of 3
- Accepting main bubble now gives 6 (max) strikes instead of 0
- As a result of the above, there is now no reason to add strikes for
  having deselected cards on the main dialog
- Strikes no longer expire after 1 year

Change-Id: I57c80fb94f120b9132b7f129d8e20996821de35b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1931652Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Fabio Tirelo <ftirelo@chromium.org>
Reviewed-by: Siyu An <siyua@chromium.org>
Commit-Queue: Jared Saul <jsaul@google.com>
Cr-Commit-Position: refs/heads/master@{#721153}

chrome/browser/ui/autofill/payments/local_card_migration_dialog_controller_impl.cc

@@ -154,6 +154,16 @@ const std::string& LocalCardMigrationDialogControllerImpl::GetUserEmail()
 
 void LocalCardMigrationDialogControllerImpl::OnSaveButtonClicked(
     const std::vector<std::string>& selected_cards_guids) {
+  // Add maximum strikes for local card migration due to user closing the main
+  // dialog. Even though the user accepted, we should not prompt migration again
+  // if there are other eligible cards, since they would have been intentionally
+  // deselected in this round.
+  LocalCardMigrationStrikeDatabase local_card_migration_strike_database(
+      StrikeDatabaseFactory::GetForProfile(
+          Profile::FromBrowserContext(web_contents()->GetBrowserContext())));
+  local_card_migration_strike_database.AddStrikes(
+      LocalCardMigrationStrikeDatabase::kStrikesToAddWhenDialogClosed);
+
   AutofillMetrics::LogLocalCardMigrationDialogUserSelectionPercentageMetric(
       selected_cards_guids.size(), migratable_credit_cards_.size());
   AutofillMetrics::LogLocalCardMigrationDialogUserInteractionMetric(
@@ -165,7 +175,8 @@ void LocalCardMigrationDialogControllerImpl::OnSaveButtonClicked(
 }
 
 void LocalCardMigrationDialogControllerImpl::OnCancelButtonClicked() {
-  // Add strikes for local card migration due to user closing the main dialog.
+  // Add maximum strikes for local card migration due to user closing the main
+  // dialog.
   LocalCardMigrationStrikeDatabase local_card_migration_strike_database(
       StrikeDatabaseFactory::GetForProfile(
           Profile::FromBrowserContext(web_contents()->GetBrowserContext())));

chrome/browser/ui/views/autofill/payments/local_card_migration_browsertest.cc

@@ -897,25 +897,47 @@ IN_PROC_BROWSER_TEST_F(LocalCardMigrationBrowserTest,
   EXPECT_EQ(nullptr, personal_data_->GetCreditCardByNumber(kSecondCardNumber));
 }
 
-// Ensures that rejecting the main migration dialog adds 3 strikes.
+// Ensures that accepting the main migration dialog adds strikes.
 IN_PROC_BROWSER_TEST_F(LocalCardMigrationBrowserTest,
-                       ClosingDialogAddsLocalCardMigrationStrikes) {
+                       AcceptingDialogAddsLocalCardMigrationStrikes) {
   base::HistogramTester histogram_tester;
 
   SaveLocalCard(kFirstCardNumber);
   SaveLocalCard(kSecondCardNumber);
   UseCardAndWaitForMigrationOffer(kFirstCardNumber);
   ClickOnOkButton(GetLocalCardMigrationOfferBubbleViews());
-  // Click the [Cancel] button, should add and log 3 strikes.
+  // Click the [Save] button, should add and log strikes.
+  ClickOnSaveButtonAndWaitForMigrationResults();
+
+  // Metrics
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples(
+          "Autofill.StrikeDatabase.NthStrikeAdded.LocalCardMigration"),
+      ElementsAre(Bucket(
+          LocalCardMigrationStrikeDatabase::kStrikesToAddWhenDialogClosed, 1)));
+}
+
+// Ensures that rejecting the main migration dialog adds strikes.
+IN_PROC_BROWSER_TEST_F(LocalCardMigrationBrowserTest,
+                       RejectingDialogAddsLocalCardMigrationStrikes) {
+  base::HistogramTester histogram_tester;
+
+  SaveLocalCard(kFirstCardNumber);
+  SaveLocalCard(kSecondCardNumber);
+  UseCardAndWaitForMigrationOffer(kFirstCardNumber);
+  ClickOnOkButton(GetLocalCardMigrationOfferBubbleViews());
+  // Click the [Cancel] button, should add and log strikes.
   ClickOnCancelButton(GetLocalCardMigrationMainDialogView());
 
   // Metrics
-  EXPECT_THAT(histogram_tester.GetAllSamples(
-                  "Autofill.StrikeDatabase.NthStrikeAdded.LocalCardMigration"),
-              ElementsAre(Bucket(3, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples(
+          "Autofill.StrikeDatabase.NthStrikeAdded.LocalCardMigration"),
+      ElementsAre(Bucket(
+          LocalCardMigrationStrikeDatabase::kStrikesToAddWhenDialogClosed, 1)));
 }
 
-// Ensures that rejecting the migration bubble adds 2 strikes.
+// Ensures that rejecting the migration bubble adds strikes.
 IN_PROC_BROWSER_TEST_F(LocalCardMigrationBrowserTest,
                        ClosingBubbleAddsLocalCardMigrationStrikes) {
   base::HistogramTester histogram_tester;
@@ -929,13 +951,16 @@ IN_PROC_BROWSER_TEST_F(LocalCardMigrationBrowserTest,
   // No bubble should be showing.
   EXPECT_EQ(nullptr, GetLocalCardMigrationOfferBubbleViews());
   // Metrics
-  EXPECT_THAT(histogram_tester.GetAllSamples(
-                  "Autofill.StrikeDatabase.NthStrikeAdded.LocalCardMigration"),
-              ElementsAre(Bucket(2, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples(
+          "Autofill.StrikeDatabase.NthStrikeAdded.LocalCardMigration"),
+      ElementsAre(Bucket(
+          LocalCardMigrationStrikeDatabase::kStrikesToAddWhenBubbleClosed, 1)));
 }
 
-// Ensures that rejecting the migration bubble repeatedly adds 2 strikes every
-// time, even for the same tab.
+// Ensures that rejecting the migration bubble repeatedly adds strikes every
+// time, even for the same tab. Currently, it adds 3 strikes (out of 6), so this
+// test can reliably test it being added twice.
 IN_PROC_BROWSER_TEST_F(LocalCardMigrationBrowserTest,
                        ClosingBubbleAgainAddsLocalCardMigrationStrikes) {
   base::HistogramTester histogram_tester;
@@ -952,10 +977,18 @@ IN_PROC_BROWSER_TEST_F(LocalCardMigrationBrowserTest,
 
   // No bubble should be showing.
   EXPECT_EQ(nullptr, GetLocalCardMigrationOfferBubbleViews());
-  // Metrics: Added 2 strikes each time, for totals of 2 then 4.
-  EXPECT_THAT(histogram_tester.GetAllSamples(
-                  "Autofill.StrikeDatabase.NthStrikeAdded.LocalCardMigration"),
-              ElementsAre(Bucket(2, 1), Bucket(4, 1)));
+  // Metrics: Added 3 strikes each time, for totals of 3 then 6.
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples(
+          "Autofill.StrikeDatabase.NthStrikeAdded.LocalCardMigration"),
+      ElementsAre(
+          Bucket(
+              LocalCardMigrationStrikeDatabase::kStrikesToAddWhenBubbleClosed,
+              1),
+          Bucket(
+              LocalCardMigrationStrikeDatabase::kStrikesToAddWhenBubbleClosed *
+                  2,
+              1)));
 }
 
 // Ensures that reshowing and closing bubble after previously closing it does

components/autofill/core/browser/payments/local_card_migration_manager.cc

@@ -172,15 +172,6 @@ void LocalCardMigrationManager::OnUserAcceptedMainMigrationDialog(
       "Autofill.StrikeDatabase.StrikesPresentWhenLocalCardMigrationAccepted",
       GetLocalCardMigrationStrikeDatabase()->GetStrikes());
 
-  // If there are cards which aren't selected, add
-  // kStrikesToAddWhenCardsDeselectedAtMigration strikes to
-  // LocalCardMigrationStrikeDatabase.
-  if (selected_card_guids.size() < migratable_credit_cards_.size()) {
-    GetLocalCardMigrationStrikeDatabase()->AddStrikes(
-        LocalCardMigrationStrikeDatabase::
-            kStrikesToAddWhenCardsDeselectedAtMigration);
-  }
-
   // Update the |migratable_credit_cards_| with the |selected_card_guids|. This
   // will remove any card from |migratable_credit_cards_| of which the GUID is
   // not in |selected_card_guids|.

components/autofill/core/browser/payments/local_card_migration_manager_unittest.cc

@@ -886,33 +886,6 @@ TEST_F(LocalCardMigrationManagerTest,
       AutofillMetrics::SaveTypeMetric::SERVER, 1);
 }
 
-// When local card migration is attempted and some cards aren't selected,
-// 3 strikes should be added.
-TEST_F(LocalCardMigrationManagerTest,
-       MigrateCreditCard_StrikesAddedWhenSomeCardsNotSelected) {
-  LocalCardMigrationStrikeDatabase local_card_migration_strike_database =
-      LocalCardMigrationStrikeDatabase(strike_database_);
-  // LocalCardMigrationStrikeDatabase should initially have no strikes.
-  EXPECT_EQ(local_card_migration_strike_database.GetStrikes(), 0);
-
-  AddLocalCreditCard(personal_data_, "Flo Master", "4111111111111111", "11",
-                     test::NextYear().c_str(), "1", "guid1");
-  AddLocalCreditCard(personal_data_, "Flo Master", "5454545454545454", "11",
-                     test::NextYear().c_str(), "1", "guid2");
-  // Set the billing_customer_number to designate existence of a Payments
-  // account.
-  personal_data_.SetPaymentsCustomerData(
-      std::make_unique<PaymentsCustomerData>(/*customer_id=*/"123456"));
-  local_card_migration_manager_->GetMigratableCreditCards();
-
-  // Only select one of the two cards.
-  autofill_client_.set_migration_card_selections(
-      std::vector<std::string>{"guid1"});
-  local_card_migration_manager_->AttemptToOfferLocalCardMigration(true);
-
-  EXPECT_EQ(local_card_migration_strike_database.GetStrikes(), 3);
-}
-
 // When local card migration is accepted, UMA metrics for LocalCardMigration
 // strike count is logged.
 TEST_F(LocalCardMigrationManagerTest, MigrateCreditCard_StrikeCountUMALogged) {

components/autofill/core/browser/payments/local_card_migration_strike_database.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "components/autofill/core/browser/payments/local_card_migration_strike_database.h"
+#include <limits>
 
 #include "components/autofill/core/browser/proto/strike_data.pb.h"
 
@@ -10,10 +11,8 @@ namespace autofill {
 
 const int LocalCardMigrationStrikeDatabase::kStrikesToRemoveWhenLocalCardAdded =
     2;
-const int LocalCardMigrationStrikeDatabase::kStrikesToAddWhenBubbleClosed = 2;
-const int LocalCardMigrationStrikeDatabase::kStrikesToAddWhenDialogClosed = 3;
-const int LocalCardMigrationStrikeDatabase::
-    kStrikesToAddWhenCardsDeselectedAtMigration = 3;
+const int LocalCardMigrationStrikeDatabase::kStrikesToAddWhenBubbleClosed = 3;
+const int LocalCardMigrationStrikeDatabase::kStrikesToAddWhenDialogClosed = 6;
 
 LocalCardMigrationStrikeDatabase::LocalCardMigrationStrikeDatabase(
     StrikeDatabase* strike_database)
@@ -32,8 +31,13 @@ int LocalCardMigrationStrikeDatabase::GetMaxStrikesLimit() {
 }
 
 int64_t LocalCardMigrationStrikeDatabase::GetExpiryTimeMicros() {
-  // Expiry time is 1 year.
-  return (int64_t)1000000 * 60 * 60 * 24 * 365;
+  // Ideally, we should be able to annotate cards deselected at migration time
+  // as cards the user is not interested in uploading.  Until then, we have been
+  // asked to not expire local card migration strikes based on a time limit.
+  // This option does not yet exist, so as a workaround the expiry time is set
+  // to the maximum amount (roughly 292,000 years).
+  // TODO(jsaul): Create an option to disable expiry time completely.
+  return std::numeric_limits<int64_t>::max();
 }
 
 bool LocalCardMigrationStrikeDatabase::UniqueIdsRequired() {