[Payments] Add billing_address_id and has_converted to autofill_table

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

Index: components/autofill/core/browser/webdata/autofill_table.cc
diff --git a/components/autofill/core/browser/webdata/autofill_table.cc b/components/autofill/core/browser/webdata/autofill_table.cc
index 3c1d9b3cb86371c66cce68c029a4771b60515239..2fca9fe6660600ef62f94cb3497f9eba39cea298 100644
--- a/components/autofill/core/browser/webdata/autofill_table.cc
+++ b/components/autofill/core/browser/webdata/autofill_table.cc
@@ -471,6 +471,9 @@ bool AutofillTable::MigrateToVersion(int version,
     case 70:
       *update_compatible_version = false;
       return MigrateToVersion70AddSyncMetadata();
+    case 71:
+      *update_compatible_version = false;
+      return MigrateToVersion71AddServerCardMetadataBillingAddress();
   }
   return true;
 }
@@ -1210,17 +1213,18 @@ bool AutofillTable::GetServerCreditCards(
 
   sql::Statement s(db_->GetUniqueStatement(
       "SELECT "
-      "card_number_encrypted, "  // 0
-      "last_four,"     // 1
-      "masked.id,"     // 2
-      "metadata.use_count,"     // 3
-      "metadata.use_date,"      // 4
-      "type,"          // 5
-      "status,"        // 6
-      "name_on_card,"  // 7
-      "exp_month,"     // 8
-      "exp_year,"      // 9
-      "billing_address_id "     // 10
+      "card_number_encrypted, "       // 0
+      "last_four,"                    // 1
+      "masked.id,"                    // 2
+      "metadata.use_count,"           // 3
+      "metadata.use_date,"            // 4
+      "type,"                         // 5
+      "status,"                       // 6
+      "name_on_card,"                 // 7
+      "exp_month,"                    // 8
+      "exp_year,"                     // 9
+      "metadata.billing_address_id,"  // 10
+      "masked.billing_address_id "    // 11

[please use gerrit instead, 2017/01/11 19:23:03]

Why store billing address identifier in two different places?

[sebsg, 2017/01/12 15:36:31]

Right, I didn't realize I could just migrate the data. Fixed!

       "FROM masked_credit_cards masked "
       "LEFT OUTER JOIN unmasked_credit_cards USING (id) "
       "LEFT OUTER JOIN server_card_metadata metadata USING (id)"));
@@ -1261,7 +1265,13 @@ bool AutofillTable::GetServerCreditCards(
     card->SetRawInfo(CREDIT_CARD_NAME_FULL, s.ColumnString16(index++));
     card->SetRawInfo(CREDIT_CARD_EXP_MONTH, s.ColumnString16(index++));
     card->SetRawInfo(CREDIT_CARD_EXP_4_DIGIT_YEAR, s.ColumnString16(index++));
+
+    // The billing address id from the metadata has priority, but if it's empty,
+    // use the one from the masked_credit_cards table.
     card->set_billing_address_id(s.ColumnString(index++));
+    if (card->billing_address_id().empty())
+      card->set_billing_address_id(s.ColumnString(index++));
+
     credit_cards->push_back(std::move(card));
   }
 
@@ -1308,7 +1318,7 @@ void AutofillTable::SetServerCreditCards(
     masked_insert.Reset(true);
 
     // Save the use count and use date of the card.
-    UpdateServerCardUsageStats(card);
+    UpdateServerCardMetadata(card);
   }
 
   // Delete all items in the unmasked table that aren't in the new set.
@@ -1349,7 +1359,7 @@ bool AutofillTable::UnmaskServerCreditCard(const CreditCard& masked,
   unmasked.set_record_type(CreditCard::FULL_SERVER_CARD);
   unmasked.SetNumber(full_number);
   unmasked.RecordAndLogUse();
-  UpdateServerCardUsageStats(unmasked);
+  UpdateServerCardMetadata(unmasked);
 
   return db_->GetLastChangeCount() > 0;
 }
@@ -1362,8 +1372,7 @@ bool AutofillTable::MaskServerCreditCard(const std::string& id) {
   return db_->GetLastChangeCount() > 0;
 }
 
-bool AutofillTable::UpdateServerCardUsageStats(
-    const CreditCard& credit_card) {
+bool AutofillTable::UpdateServerCardMetadata(const CreditCard& credit_card) {
   DCHECK_NE(CreditCard::LOCAL_CARD, credit_card.record_type());
   sql::Transaction transaction(db_);
   if (!transaction.Begin())
@@ -1374,12 +1383,14 @@ bool AutofillTable::UpdateServerCardUsageStats(
   remove.BindString(0, credit_card.server_id());
   remove.Run();
 
-  sql::Statement s(db_->GetUniqueStatement(
-      "INSERT INTO server_card_metadata(use_count, use_date, id)"
-      "VALUES (?,?,?)"));
+  sql::Statement s(
+      db_->GetUniqueStatement("INSERT INTO server_card_metadata(use_count, "
+                              "use_date, billing_address_id, id)"

[please use gerrit instead, 2017/01/11 19:23:03]

You appear to have moved the billing_address_id column from the
masked_credit_cards table to the server_card_metadata table. What advantage does
this provide?

[sebsg, 2017/01/12 15:36:31]

I think it makes more sense to have it be in the server_card_metadata table,
because this table is used as read-write, compared to maked_credit_cards which
is supposed to be read only (only refreshed by the messages from sync).

+                              "VALUES (?,?,?,?)"));
   s.BindInt64(0, credit_card.use_count());
   s.BindInt64(1, credit_card.use_date().ToInternalValue());
-  s.BindString(2, credit_card.server_id());
+  s.BindString(2, credit_card.billing_address_id());
+  s.BindString(3, credit_card.server_id());
   s.Run();
 
   transaction.Commit();
@@ -1387,7 +1398,8 @@ bool AutofillTable::UpdateServerCardUsageStats(
   return db_->GetLastChangeCount() > 0;
 }
 
-bool AutofillTable::UpdateServerAddressUsageStats(
+// TODO(crbug.com/680182): Record the address conversion status.

[please use gerrit instead, 2017/01/11 19:23:03]

This comment is not clear. Please explain what you intend to happen.

[sebsg, 2017/01/12 15:36:31]

Tried to make the comment more clear. It's just to explain that this value
(has_converted) will not always be set to false. It will get assigned and read
from in a future CL, the one where the wallet addresses get converted to local
Autofill profiles.

+bool AutofillTable::UpdateServerAddressMetadata(
     const AutofillProfile& profile) {
   DCHECK_EQ(AutofillProfile::SERVER_PROFILE, profile.record_type());
 
@@ -1400,12 +1412,14 @@ bool AutofillTable::UpdateServerAddressUsageStats(
   remove.BindString(0, profile.server_id());
   remove.Run();
 
-  sql::Statement s(db_->GetUniqueStatement(
-      "INSERT INTO server_address_metadata(use_count, use_date, id)"
-      "VALUES (?,?,?)"));
+  sql::Statement s(
+      db_->GetUniqueStatement("INSERT INTO server_address_metadata(use_count, "
+                              "use_date, has_converted, id)"
+                              "VALUES (?,?,?,?)"));
   s.BindInt64(0, profile.use_count());
   s.BindInt64(1, profile.use_date().ToInternalValue());
-  s.BindString(2, profile.server_id());
+  s.BindBool(2, false);
+  s.BindString(3, profile.server_id());
   s.Run();
 
   transaction.Commit();
@@ -1413,21 +1427,6 @@ bool AutofillTable::UpdateServerAddressUsageStats(
   return db_->GetLastChangeCount() > 0;
 }
 
-bool AutofillTable::UpdateServerCardBillingAddress(
-    const CreditCard& credit_card) {
-  DCHECK_NE(CreditCard::LOCAL_CARD, credit_card.record_type());
-
-  sql::Statement update(db_->GetUniqueStatement(
-      "UPDATE masked_credit_cards SET billing_address_id = ? "
-      "WHERE id = ?"));
-  update.BindString(0, credit_card.billing_address_id());
-  update.BindString(1, credit_card.server_id());
-  if (!update.Run())
-    return false;
-
-  return db_->GetLastChangeCount() > 0;
-}
-
 bool AutofillTable::ClearAllServerData() {
   sql::Transaction transaction(db_);
   if (!transaction.Begin())
@@ -1957,7 +1956,8 @@ bool AutofillTable::InitServerCardMetadataTable() {
     if (!db_->Execute("CREATE TABLE server_card_metadata ("
                       "id VARCHAR NOT NULL,"
                       "use_count INTEGER NOT NULL DEFAULT 0, "
-                      "use_date INTEGER NOT NULL DEFAULT 0)")) {
+                      "use_date INTEGER NOT NULL DEFAULT 0, "
+                      "billing_address_id VARCHAR)")) {
       NOTREACHED();
       return false;
     }
@@ -1995,7 +1995,8 @@ bool AutofillTable::InitServerAddressMetadataTable() {
     if (!db_->Execute("CREATE TABLE server_address_metadata ("
                       "id VARCHAR NOT NULL,"
                       "use_count INTEGER NOT NULL DEFAULT 0, "
-                      "use_date INTEGER NOT NULL DEFAULT 0)")) {
+                      "use_date INTEGER NOT NULL DEFAULT 0, "
+                      "has_converted BOOL NOT NULL DEFAULT FALSE)")) {
       NOTREACHED();
       return false;
     }
@@ -2473,4 +2474,24 @@ bool AutofillTable::MigrateToVersion70AddSyncMetadata() {
       "BLOB)");
 }
 
+bool AutofillTable::MigrateToVersion71AddServerCardMetadataBillingAddress() {
+  sql::Transaction transaction(db_);
+  if (!transaction.Begin())
+    return false;
+
+  if (!db_->DoesColumnExist("server_card_metadata", "billing_address_id") &&
+      !db_->Execute("ALTER TABLE server_card_metadata ADD COLUMN "
+                    "billing_address_id VARCHAR")) {
+    return false;
+  }
+
+  if (!db_->DoesColumnExist("server_address_metadata", "has_converted") &&
+      !db_->Execute("ALTER TABLE server_address_metadata ADD COLUMN "
+                    "has_converted BOOL NOT NULL DEFAULT FALSE")) {
+    return false;
+  }
+
+  return transaction.Commit();
+}
+
 }  // namespace autofill