Autofill dialog: don't save duplicate shipping addresses to Wallet.

When autofill_dialog in wallet mode, and user select "same-as-billing", avoid to save a duplicated shipping address when user already has an exact matched one in his/her wallet. This applies to both rAC and Autocheckout.

BUG=225442

Review URL: https://chromiumcodereview.appspot.com/14765013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@202266 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 195 insertions, 35 deletions

diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
index f4679ab..57ab5f0 100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
@@ -310,6 +310,18 @@ bool HasCompleteAndVerifiedData(const AutofillDataModel& data_model,
   return true;
 }
 
+// Loops through |addresses_| comparing to |address| ignoring ID. If a match
+// is not found, NULL is returned.
+const wallet::Address* FindDuplicateAddress(
+    const std::vector<wallet::Address*>& addresses,
+    const wallet::Address& address) {
+  for (size_t i = 0; i < addresses.size(); ++i) {
+    if (addresses[i]->EqualsIgnoreID(address))
+      return addresses[i];
+  }
+  return NULL;
+}
+
 }  // namespace
 
 AutofillDialogController::~AutofillDialogController() {}
@@ -2386,17 +2398,12 @@ void AutofillDialogControllerImpl::SubmitWithWallet() {
   base::StringToInt(shipping->GetItemKeyForCheckedItem(), &address_index);
 
   if (!IsManuallyEditingSection(SECTION_SHIPPING) &&
-      shipping->GetItemKeyForCheckedItem() != kSameAsBillingKey) {
+      !ShouldUseBillingForShipping()) {
     active_address_id_ =
         wallet_items_->addresses()[address_index]->object_id();
     DCHECK(!active_address_id_.empty());
   }
 
-  // If there's neither an address nor instrument to save, |GetFullWallet()|
-  // is called when the risk fingerprint is loaded.
-  if (!active_instrument_id_.empty() && !active_address_id_.empty())
-    return;
-
   scoped_ptr<wallet::Instrument> inputted_instrument =
       CreateTransientInstrument();
   scoped_ptr<wallet::WalletClient::UpdateInstrumentRequest> update_request =
@@ -2408,10 +2415,20 @@ void AutofillDialogControllerImpl::SubmitWithWallet() {
   scoped_ptr<wallet::Address> inputted_address;
   if (active_address_id_.empty()) {
     if (ShouldUseBillingForShipping()) {
-      inputted_address.reset(new wallet::Address(inputted_instrument ?
+      const wallet::Address& address = inputted_instrument ?
           inputted_instrument->address() :
-          wallet_items_->instruments()[instrument_index]->address()));
-      DCHECK(inputted_address->object_id().empty());
+          wallet_items_->instruments()[instrument_index]->address();
+      // Try to find an exact matched shipping address and use it for shipping,
+      // otherwise save it as a new shipping address. http://crbug.com/225442
+      const wallet::Address* duplicated_address =
+          FindDuplicateAddress(wallet_items_->addresses(), address);
+      if (duplicated_address) {
+        active_address_id_ = duplicated_address->object_id();
+        DCHECK(!active_address_id_.empty());
+      } else {
+        inputted_address.reset(new wallet::Address(address));
+        DCHECK(inputted_address->object_id().empty());
+      }
     } else {
       inputted_address = CreateTransientAddress();
       if (section_editing_state_[SECTION_SHIPPING]) {
@@ -2422,6 +2439,11 @@ void AutofillDialogControllerImpl::SubmitWithWallet() {
     }
   }
 
+  // If there's neither an address nor instrument to save, |GetFullWallet()|
+  // is called when the risk fingerprint is loaded.
+  if (!active_instrument_id_.empty() && !active_address_id_.empty())
+    return;
+
   // If instrument and address aren't based off of any existing data, save both.
   if (inputted_instrument && inputted_address && !update_request &&
       inputted_address->object_id().empty()) {
diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc b/chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc
index 6ab24d8..2465020 100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc
@@ -361,6 +361,10 @@ class AutofillDialogControllerTest : public testing::Test {
         TestAccountChooserModel::kActiveWalletItemId);
   }
 
+  void UseBillingForShipping() {
+    controller()->MenuModelForSection(SECTION_SHIPPING)->ActivatedAt(0);
+  }
+
   TestAutofillDialogController* controller() { return controller_; }
 
   TestingProfile* profile() { return &profile_; }
@@ -705,11 +709,9 @@ TEST_F(AutofillDialogControllerTest, UseBillingAsShipping) {
   controller()->GetTestingManager()->AddTestingProfile(&full_profile);
   controller()->GetTestingManager()->AddTestingProfile(&full_profile2);
   controller()->GetTestingManager()->AddTestingCreditCard(&credit_card);
-  ui::MenuModel* shipping_model =
-      controller()->MenuModelForSection(SECTION_SHIPPING);
 
   // Test after setting use billing for shipping.
-  shipping_model->ActivatedAt(0);
+  UseBillingForShipping();
 
   controller()->OnAccept();
   ASSERT_EQ(4U, form_structure()->field_count());
@@ -819,6 +821,12 @@ TEST_F(AutofillDialogControllerTest, SaveAddress) {
   scoped_ptr<wallet::WalletItems> wallet_items = wallet::GetTestWalletItems();
   wallet_items->AddInstrument(wallet::GetTestMaskedInstrument());
   controller()->OnDidGetWalletItems(wallet_items.Pass());
+  // If there is no shipping address in wallet, it will default to
+  // "same-as-billing" instead of "add-new-item". "same-as-billing" is covered
+  // by the following tests. The last item in the menu is "add-new-item".
+  ui::MenuModel* shipping_model =
+      controller()->MenuModelForSection(SECTION_SHIPPING);
+  shipping_model->ActivatedAt(shipping_model->GetItemCount() - 1);
   controller()->OnAccept();
 }
 
@@ -909,6 +917,7 @@ TEST_F(AutofillDialogControllerTest, SaveInstrumentUpdateAddress) {
 
   scoped_ptr<wallet::WalletItems> wallet_items = wallet::GetTestWalletItems();
   wallet_items->AddAddress(wallet::GetTestShippingAddress());
+
   controller()->OnDidGetWalletItems(wallet_items.Pass());
 
   controller()->EditClickedForSection(SECTION_SHIPPING);
@@ -919,6 +928,63 @@ MATCHER(UsesLocalBillingAddress, "uses the local billing address") {
   return arg.address_line_1() == ASCIIToUTF16(kEditedBillingAddress);
 }
 
+// Tests that when using billing address for shipping, and there is no exact
+// matched shipping address, then a shipping address should be added.
+TEST_F(AutofillDialogControllerTest, BillingForShipping) {
+  EXPECT_CALL(*controller()->GetTestingWalletClient(),
+              SaveAddress(_, _)).Times(1);
+
+  scoped_ptr<wallet::WalletItems> wallet_items = wallet::GetTestWalletItems();
+  wallet_items->AddInstrument(wallet::GetTestMaskedInstrument());
+  wallet_items->AddAddress(wallet::GetTestShippingAddress());
+
+  controller()->OnDidGetWalletItems(wallet_items.Pass());
+  // Select "Same as billing" in the address menu.
+  UseBillingForShipping();
+
+  controller()->OnAccept();
+}
+
+// Tests that when using billing address for shipping, and there is an exact
+// matched shipping address, then a shipping address should not be added.
+TEST_F(AutofillDialogControllerTest, BillingForShippingHasMatch) {
+  EXPECT_CALL(*controller()->GetTestingWalletClient(),
+              SaveAddress(_, _)).Times(0);
+
+  scoped_ptr<wallet::WalletItems> wallet_items = wallet::GetTestWalletItems();
+  scoped_ptr<wallet::WalletItems::MaskedInstrument> instrument =
+      wallet::GetTestMaskedInstrument();
+  // Copy billing address as shipping address, and assign an id to it.
+  scoped_ptr<wallet::Address> shipping_address(
+      new wallet::Address(instrument->address()));
+  shipping_address->set_object_id("shipping_address_id");
+  wallet_items->AddAddress(shipping_address.Pass());
+  wallet_items->AddInstrument(instrument.Pass());
+  wallet_items->AddAddress(wallet::GetTestShippingAddress());
+
+  controller()->OnDidGetWalletItems(wallet_items.Pass());
+  // Select "Same as billing" in the address menu.
+  UseBillingForShipping();
+
+  controller()->OnAccept();
+}
+
+// Tests that adding new instrument and also using billing address for shipping,
+// then a shipping address should not be added.
+TEST_F(AutofillDialogControllerTest, BillingForShippingNewInstrument) {
+  EXPECT_CALL(*controller()->GetTestingWalletClient(),
+              SaveInstrumentAndAddress(_, _, _, _)).Times(1);
+
+  scoped_ptr<wallet::WalletItems> wallet_items = wallet::GetTestWalletItems();
+  wallet_items->AddAddress(wallet::GetTestShippingAddress());
+
+  controller()->OnDidGetWalletItems(wallet_items.Pass());
+  // Select "Same as billing" in the address menu.
+  UseBillingForShipping();
+
+  controller()->OnAccept();
+}
+
 // Test that the local view contents is used when saving a new instrument and
 // the user has selected "Same as billing".
 TEST_F(AutofillDialogControllerTest, SaveInstrumentSameAsBilling) {
diff --git a/components/autofill/browser/wallet/wallet_address.cc b/components/autofill/browser/wallet/wallet_address.cc
index 6035144..92aeffb 100644
--- a/components/autofill/browser/wallet/wallet_address.cc
+++ b/components/autofill/browser/wallet/wallet_address.cc
@@ -291,7 +291,7 @@ string16 Address::GetInfo(AutofillFieldType type,
   }
 }
 
-bool Address::operator==(const Address& other) const {
+bool Address::EqualsIgnoreID(const Address& other) const {
   return country_name_code_ == other.country_name_code_ &&
          recipient_name_ == other.recipient_name_ &&
          address_line_1_ == other.address_line_1_ &&
@@ -300,10 +300,13 @@ bool Address::operator==(const Address& other) const {
          administrative_area_name_ == other.administrative_area_name_ &&
          postal_code_number_ == other.postal_code_number_ &&
          phone_number_ == other.phone_number_ &&
-         object_id_ == other.object_id_ &&
          is_complete_address_ == other.is_complete_address_;
 }
 
+bool Address::operator==(const Address& other) const {
+  return object_id_ == other.object_id_ && EqualsIgnoreID(other);
+}
+
 bool Address::operator!=(const Address& other) const {
   return !(*this == other);
 }
diff --git a/components/autofill/browser/wallet/wallet_address.h b/components/autofill/browser/wallet/wallet_address.h
index 953af5a..a13f2f2 100644
--- a/components/autofill/browser/wallet/wallet_address.h
+++ b/components/autofill/browser/wallet/wallet_address.h
@@ -143,6 +143,10 @@ class Address {
     is_complete_address_ = is_complete_address;
   }
 
+  // Tests if this address exact matches |other|. |object_id| is ignored.
+  bool EqualsIgnoreID(const Address& other) const;
+
+  // Tests if this address exact matches |other| including |object_id|.
   bool operator==(const Address& other) const;
   bool operator!=(const Address& other) const;
 
diff --git a/components/autofill/browser/wallet/wallet_address_unittest.cc b/components/autofill/browser/wallet/wallet_address_unittest.cc
index 476d3ab..27a501f 100644
--- a/components/autofill/browser/wallet/wallet_address_unittest.cc
+++ b/components/autofill/browser/wallet/wallet_address_unittest.cc
@@ -168,6 +168,71 @@ class WalletAddressTest : public testing::Test {
   scoped_ptr<const DictionaryValue> dict_;
 };
 
+TEST_F(WalletAddressTest, AddressEqualsIgnoreID) {
+  Address address1("country_name_code",
+                   ASCIIToUTF16("recipient_name"),
+                   ASCIIToUTF16("address_line_1"),
+                   ASCIIToUTF16("address_line_2"),
+                   ASCIIToUTF16("locality_name"),
+                   ASCIIToUTF16("administrative_area_name"),
+                   ASCIIToUTF16("postal_code_number"),
+                   ASCIIToUTF16("phone_number"),
+                   "id1");
+  // Same as address1, only id is different.
+  Address address2("country_name_code",
+                   ASCIIToUTF16("recipient_name"),
+                   ASCIIToUTF16("address_line_1"),
+                   ASCIIToUTF16("address_line_2"),
+                   ASCIIToUTF16("locality_name"),
+                   ASCIIToUTF16("administrative_area_name"),
+                   ASCIIToUTF16("postal_code_number"),
+                   ASCIIToUTF16("phone_number"),
+                   "id2");
+  // Has same id as address1, but name is different.
+  Address address3("country_name_code",
+                   ASCIIToUTF16("a_different_name"),
+                   ASCIIToUTF16("address_line_1"),
+                   ASCIIToUTF16("address_line_2"),
+                   ASCIIToUTF16("locality_name"),
+                   ASCIIToUTF16("administrative_area_name"),
+                   ASCIIToUTF16("postal_code_number"),
+                   ASCIIToUTF16("phone_number"),
+                   "id1");
+  // Same as address1, but no id.
+  Address address4("country_name_code",
+                   ASCIIToUTF16("recipient_name"),
+                   ASCIIToUTF16("address_line_1"),
+                   ASCIIToUTF16("address_line_2"),
+                   ASCIIToUTF16("locality_name"),
+                   ASCIIToUTF16("administrative_area_name"),
+                   ASCIIToUTF16("postal_code_number"),
+                   ASCIIToUTF16("phone_number"),
+                   std::string());
+
+  // Compare the address has id field to itself.
+  EXPECT_EQ(address1, address1);
+  EXPECT_TRUE(address1.EqualsIgnoreID(address1));
+
+  // Compare the address has no id field to itself
+  EXPECT_EQ(address4, address4);
+  EXPECT_TRUE(address4.EqualsIgnoreID(address4));
+
+  // Compare two addresses with different id.
+  EXPECT_NE(address1, address2);
+  EXPECT_TRUE(address1.EqualsIgnoreID(address2));
+  EXPECT_TRUE(address2.EqualsIgnoreID(address1));
+
+  // Compare two different addresses.
+  EXPECT_NE(address1, address3);
+  EXPECT_FALSE(address1.EqualsIgnoreID(address3));
+  EXPECT_FALSE(address3.EqualsIgnoreID(address1));
+
+  // Compare two same addresses, one has id, the other doesn't.
+  EXPECT_NE(address1, address4);
+  EXPECT_TRUE(address1.EqualsIgnoreID(address4));
+  EXPECT_TRUE(address4.EqualsIgnoreID(address1));
+}
+
 TEST_F(WalletAddressTest, CreateAddressMissingObjectId) {
   SetUpDictionary(kAddressMissingObjectId);
   Address address("country_name_code",
@@ -179,30 +244,30 @@ TEST_F(WalletAddressTest, CreateAddressMissingObjectId) {
                   ASCIIToUTF16("postal_code_number"),
                   ASCIIToUTF16("phone_number"),
                   std::string());
-  ASSERT_EQ(address, *Address::CreateAddress(*dict_));
+  EXPECT_EQ(address, *Address::CreateAddress(*dict_));
 }
 
 TEST_F(WalletAddressTest, CreateAddressWithIDMissingObjectId) {
   SetUpDictionary(kAddressMissingObjectId);
-  ASSERT_EQ(NULL, Address::CreateAddressWithID(*dict_).get());
+  EXPECT_EQ(NULL, Address::CreateAddressWithID(*dict_).get());
 }
 
 TEST_F(WalletAddressTest, CreateAddressMissingCountryNameCode) {
   SetUpDictionary(kAddressMissingCountryNameCode);
-  ASSERT_EQ(NULL, Address::CreateAddress(*dict_).get());
-  ASSERT_EQ(NULL, Address::CreateAddressWithID(*dict_).get());
+  EXPECT_EQ(NULL, Address::CreateAddress(*dict_).get());
+  EXPECT_EQ(NULL, Address::CreateAddressWithID(*dict_).get());
 }
 
 TEST_F(WalletAddressTest, CreateAddressMissingRecipientName) {
   SetUpDictionary(kAddressMissingRecipientName);
-  ASSERT_EQ(NULL, Address::CreateAddress(*dict_).get());
-  ASSERT_EQ(NULL, Address::CreateAddressWithID(*dict_).get());
+  EXPECT_EQ(NULL, Address::CreateAddress(*dict_).get());
+  EXPECT_EQ(NULL, Address::CreateAddressWithID(*dict_).get());
 }
 
 TEST_F(WalletAddressTest, CreateAddressMissingPostalCodeNumber) {
   SetUpDictionary(kAddressMissingPostalCodeNumber);
-  ASSERT_EQ(NULL, Address::CreateAddress(*dict_).get());
-  ASSERT_EQ(NULL, Address::CreateAddressWithID(*dict_).get());
+  EXPECT_EQ(NULL, Address::CreateAddress(*dict_).get());
+  EXPECT_EQ(NULL, Address::CreateAddressWithID(*dict_).get());
 }
 
 TEST_F(WalletAddressTest, CreateAddressWithID) {
@@ -217,23 +282,23 @@ TEST_F(WalletAddressTest, CreateAddressWithID) {
                   ASCIIToUTF16("phone_number"),
                   "id");
   address.set_is_complete_address(false);
-  ASSERT_EQ(address, *Address::CreateAddress(*dict_));
-  ASSERT_EQ(address, *Address::CreateAddressWithID(*dict_));
+  EXPECT_EQ(address, *Address::CreateAddress(*dict_));
+  EXPECT_EQ(address, *Address::CreateAddressWithID(*dict_));
 }
 
 TEST_F(WalletAddressTest, CreateDisplayAddressMissingCountryNameCode) {
   SetUpDictionary(kClientAddressMissingCountryCode);
-  ASSERT_EQ(NULL, Address::CreateDisplayAddress(*dict_).get());
+  EXPECT_EQ(NULL, Address::CreateDisplayAddress(*dict_).get());
 }
 
 TEST_F(WalletAddressTest, CreateDisplayAddressMissingName) {
   SetUpDictionary(kClientAddressMissingName);
-  ASSERT_EQ(NULL, Address::CreateDisplayAddress(*dict_).get());
+  EXPECT_EQ(NULL, Address::CreateDisplayAddress(*dict_).get());
 }
 
 TEST_F(WalletAddressTest, CreateDisplayAddressMissingPostalCode) {
   SetUpDictionary(kClientAddressMissingPostalCode);
-  ASSERT_EQ(NULL, Address::CreateDisplayAddress(*dict_).get());
+  EXPECT_EQ(NULL, Address::CreateDisplayAddress(*dict_).get());
 }
 
 TEST_F(WalletAddressTest, CreateDisplayAddress) {
@@ -247,7 +312,7 @@ TEST_F(WalletAddressTest, CreateDisplayAddress) {
                   ASCIIToUTF16("postal_code"),
                   ASCIIToUTF16("phone_number"),
                   std::string());
-  ASSERT_EQ(address, *Address::CreateDisplayAddress(*dict_));
+  EXPECT_EQ(address, *Address::CreateDisplayAddress(*dict_));
 }
 
 TEST_F(WalletAddressTest, ToDictionaryWithoutID) {
diff --git a/components/autofill/browser/wallet/wallet_items.cc b/components/autofill/browser/wallet/wallet_items.cc
index ff81bcc..062496f 100644
--- a/components/autofill/browser/wallet/wallet_items.cc
+++ b/components/autofill/browser/wallet/wallet_items.cc
@@ -222,13 +222,6 @@ bool WalletItems::MaskedInstrument::operator!=(
   return !(*this == other);
 }
 
-bool WalletItems::HasRequiredAction(RequiredAction action) const {
-  DCHECK(ActionAppliesToWalletItems(action));
-  return std::find(required_actions_.begin(),
-                   required_actions_.end(),
-                   action) != required_actions_.end();
-}
-
 const WalletItems::MaskedInstrument* WalletItems::GetInstrumentById(
     const std::string& object_id) const {
   if (object_id.empty())
@@ -242,6 +235,13 @@ const WalletItems::MaskedInstrument* WalletItems::GetInstrumentById(
   return NULL;
 }
 
+bool WalletItems::HasRequiredAction(RequiredAction action) const {
+  DCHECK(ActionAppliesToWalletItems(action));
+  return std::find(required_actions_.begin(),
+                   required_actions_.end(),
+                   action) != required_actions_.end();
+}
+
 base::string16 WalletItems::MaskedInstrument::DisplayName() const {
 #if defined(OS_ANDROID)
   // TODO(aruslan): improve this stub implementation.