[Payments] Only send elided addresses in GetUploadDetails, not UploadCard

The feature was overriding values in |upload_request_.profiles|, which turns
out was being reused for a subsequent request.

We now use a temporary structure to send country-only addresses to Payments for
GetUploadDetails call.

This change also disables by default the SendOnlyCountryInGetUploadDetails and
PaymentsCustomerData features.

Bug: 878416
Test: components_unittests
Change-Id: If78098e8b494cf4bba2e5049c368d87b2913a84d
Reviewed-on: https://chromium-review.googlesource.com/1233815
Commit-Queue: Mathieu Perreault <mathp@chromium.org>
Reviewed-by: Jared Saul <jsaul@google.com>
Cr-Commit-Position: refs/heads/master@{#592504}

components/autofill/core/browser/credit_card_save_manager.cc

@@ -171,16 +171,19 @@ void CreditCardSaveManager::AttemptToOfferCardUploadSave(
   }
 
   // If the relevant feature is enabled, only send the country of the
-  // recently-used addresses.
-  if (base::FeatureList::IsEnabled(
-          features::kAutofillSendOnlyCountryInGetUploadDetails)) {
-    for (size_t i = 0; i < upload_request_.profiles.size(); i++) {
+  // recently-used addresses. We make a copy here to avoid modifying
+  // |upload_request_.profiles|, which should always have full addresses even
+  // after this function goes out of scope.
+  bool send_only_country_in_addresses = base::FeatureList::IsEnabled(
+      features::kAutofillSendOnlyCountryInGetUploadDetails);
+  std::vector<AutofillProfile> country_only_profiles;
+  if (send_only_country_in_addresses) {
+    for (const AutofillProfile& address : upload_request_.profiles) {
       AutofillProfile country_only;
       country_only.SetInfo(ADDRESS_HOME_COUNTRY,
-                           upload_request_.profiles[i].GetInfo(
-                               ADDRESS_HOME_COUNTRY, app_locale_),
+                           address.GetInfo(ADDRESS_HOME_COUNTRY, app_locale_),
                            app_locale_);
-      upload_request_.profiles[i] = std::move(country_only);
+      country_only_profiles.emplace_back(std::move(country_only));
     }
   }
 
@@ -188,8 +191,10 @@ void CreditCardSaveManager::AttemptToOfferCardUploadSave(
   if (observer_for_testing_)
     observer_for_testing_->OnDecideToRequestUploadSave();
   payments_client_->GetUploadDetails(
-      upload_request_.profiles, upload_request_.detected_values,
-      upload_request_.active_experiments, app_locale_,
+      send_only_country_in_addresses ? country_only_profiles
+                                     : upload_request_.profiles,
+      upload_request_.detected_values, upload_request_.active_experiments,
+      app_locale_,
       base::BindOnce(&CreditCardSaveManager::OnDidGetUploadDetails,
                      weak_ptr_factory_.GetWeakPtr()),
       payments::kUploadCardBillableServiceNumber);

components/autofill/core/browser/credit_card_save_manager.h

@@ -112,6 +112,7 @@ class CreditCardSaveManager {
                                const std::string& server_id);
 
  private:
+  friend class CreditCardSaveManagerTest;
   friend class CreditCardSaveManagerTestObserverBridge;
   friend class SaveCardBubbleViewsBrowserTestBase;
 

components/autofill/core/browser/credit_card_save_manager_unittest.cc

@@ -156,6 +156,10 @@ class CreditCardSaveManagerTest : public testing::Test {
         form, false, SubmissionSource::FORM_SUBMISSION, base::TimeTicks::Now());
   }
 
+  void UserHasAcceptedUpload(const base::string16& cardholder_name) {
+    credit_card_save_manager_->OnUserDidAcceptUpload(cardholder_name);
+  }
+
   // Populates |form| with data corresponding to a simple credit card form.
   // Note that this actually appends fields to the form data, which can be
   // useful for building up more complex test forms.
@@ -498,6 +502,13 @@ TEST_F(CreditCardSaveManagerTest, UploadCreditCard_FullAddresses) {
   // modified the profile.
   histogram_tester.ExpectTotalCount(
       "Autofill.DaysSincePreviousUseAtSubmission.Profile", 0);
+
+  // Simulate that the user has accepted the upload from the prompt.
+  UserHasAcceptedUpload(/*cardholder_name=*/base::ASCIIToUTF16(""));
+  // We should find that full addresses are included in the UploadCard request.
+  EXPECT_THAT(
+      payments_client_->addresses_in_upload_card(),
+      testing::UnorderedElementsAreArray({*personal_data_.GetProfiles()[0]}));
 }
 
 TEST_F(CreditCardSaveManagerTest, UploadCreditCard_OnlyCountryInAddresses) {
@@ -564,6 +575,14 @@ TEST_F(CreditCardSaveManagerTest, UploadCreditCard_OnlyCountryInAddresses) {
   // modified the profile.
   histogram_tester.ExpectTotalCount(
       "Autofill.DaysSincePreviousUseAtSubmission.Profile", 0);
+
+  // Simulate that the user has accepted the upload from the prompt.
+  UserHasAcceptedUpload(/*cardholder_name=*/base::ASCIIToUTF16(""));
+  // We should find that full addresses are included in the UploadCard request,
+  // even though only countries were included in GetUploadDetails.
+  EXPECT_THAT(
+      payments_client_->addresses_in_upload_card(),
+      testing::UnorderedElementsAreArray({*personal_data_.GetProfiles()[0]}));
 }
 
 // Tests that a credit card inferred from a form with a credit card first and

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

@@ -46,6 +46,7 @@ void TestPaymentsClient::UploadCard(
     const payments::PaymentsClient::UploadRequestDetails& request_details,
     base::OnceCallback<void(AutofillClient::PaymentsRpcResult,
                             const std::string&)> callback) {
+  upload_card_addresses_ = request_details.profiles;
   active_experiments_ = request_details.active_experiments;
   std::move(callback).Run(AutofillClient::SUCCESS, server_id_);
 }

components/autofill/core/browser/payments/test_payments_client.h

@@ -57,7 +57,9 @@ class TestPaymentsClient : public payments::PaymentsClient {
   const std::vector<AutofillProfile>& addresses_in_upload_details() const {
     return upload_details_addresses_;
   }
-
+  const std::vector<AutofillProfile>& addresses_in_upload_card() const {
+    return upload_card_addresses_;
+  }
   const std::vector<const char*>& active_experiments_in_request() const {
     return active_experiments_;
   }
@@ -65,6 +67,7 @@ class TestPaymentsClient : public payments::PaymentsClient {
  private:
   std::string server_id_;
   std::vector<AutofillProfile> upload_details_addresses_;
+  std::vector<AutofillProfile> upload_card_addresses_;
   int detected_values_;
   std::string pan_first_six_;
   std::vector<const char*> active_experiments_;

components/autofill/core/common/autofill_features.cc

@@ -181,7 +181,7 @@ const base::Feature kAutofillSendExperimentIdsInPaymentsRPCs{
 // are sent.
 const base::Feature kAutofillSendOnlyCountryInGetUploadDetails{
     "AutofillSendOnlyCountryInGetUploadDetails",
-    base::FEATURE_ENABLED_BY_DEFAULT};
+    base::FEATURE_DISABLED_BY_DEFAULT};
 
 // Enables or Disables (mostly for hermetic testing) autofill server
 // communication. The URL of the autofill server can further be controlled via
@@ -262,7 +262,7 @@ const base::Feature kAutofillUpstreamUseGooglePayBrandingOnMobile{
 // Controls whether the PaymentsCustomerData is used to make requests to
 // Google Payments.
 const base::Feature kAutofillUsePaymentsCustomerData{
-    "AutofillUsePaymentsCustomerData", base::FEATURE_ENABLED_BY_DEFAULT};
+    "AutofillUsePaymentsCustomerData", base::FEATURE_DISABLED_BY_DEFAULT};
 
 const base::Feature kAutofillVoteUsingInvalidProfileData{
     "AutofillVoteUsingInvalidProfileData", base::FEATURE_ENABLED_BY_DEFAULT};