Remove all uses of the billingIdentifier field (#1608)

* Remove all uses of the billingIdentifier field

* Add @ignore flag

* Add tag

core/src/main/java/google/registry/export/sheet/SyncRegistrarsSheet.java

@@ -120,7 +120,6 @@ class SyncRegistrarsSheet {
                   builder.put("registrarName", convert(registrar.getRegistrarName()));
                   builder.put("state", convert(registrar.getState()));
                   builder.put("ianaIdentifier", convert(registrar.getIanaIdentifier()));
-                  builder.put("billingIdentifier", convert(registrar.getBillingIdentifier()));
                   builder.put("billingAccountMap", convert(registrar.getBillingAccountMap()));
                   builder.put("primaryContacts", convertContacts(contacts, byType(ADMIN)));
                   builder.put("techContacts", convertContacts(contacts, byType(TECH)));

core/src/main/java/google/registry/model/registrar/Registrar.java

@@ -62,6 +62,7 @@ import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Embed;
 import com.googlecode.objectify.annotation.Entity;
 import com.googlecode.objectify.annotation.Id;
+import com.googlecode.objectify.annotation.Ignore;
 import com.googlecode.objectify.annotation.IgnoreSave;
 import com.googlecode.objectify.annotation.Index;
 import com.googlecode.objectify.annotation.Mapify;
@@ -75,6 +76,7 @@ import google.registry.model.JsonMapBuilder;
 import google.registry.model.Jsonifiable;
 import google.registry.model.UnsafeSerializable;
 import google.registry.model.UpdateAutoTimestamp;
+import google.registry.model.annotations.DeleteAfterMigration;
 import google.registry.model.annotations.InCrossTld;
 import google.registry.model.annotations.ReportedOn;
 import google.registry.model.common.EntityGroupRoot;
@@ -123,24 +125,24 @@ public class Registrar extends ImmutableObject
 
   /** Represents the type of a registrar entity. */
   public enum Type {
-    /** A real-world, third-party registrar. Should have non-null IANA and billing IDs. */
+    /** A real-world, third-party registrar. Should have non-null IANA and billing account IDs. */
     REAL(Objects::nonNull),
 
     /**
      * A registrar account used by a real third-party registrar undergoing operational testing and
-     * evaluation. Should only be created in sandbox, and should have null IANA/billing IDs.
+     * evaluation. Should only be created in sandbox, and should have null IANA/billing account IDs.
      */
     OTE(Objects::isNull),
 
     /**
-     * A registrar used for predelegation testing. Should have a null billing ID. The IANA ID should
-     * be either 9995 or 9996, which are reserved for predelegation testing.
+     * A registrar used for predelegation testing. Should have a null billing account ID. The IANA
+     * ID should be either 9995 or 9996, which are reserved for predelegation testing.
      */
     PDT(n -> ImmutableSet.of(9995L, 9996L).contains(n)),
 
     /**
      * A registrar used for external monitoring by ICANN. Should have IANA ID 9997 and a null
-     * billing ID.
+     * billing account ID.
      */
     EXTERNAL_MONITORING(isEqual(9997L)),
 
@@ -148,13 +150,13 @@ public class Registrar extends ImmutableObject
      * A registrar used for when the registry acts as a registrar. Must have either IANA ID 9998
      * (for billable transactions) or 9999 (for non-billable transactions).
      */
-    // TODO(b/13786188): determine what billing ID for this should be, if any.
+    // TODO(b/13786188): determine what billing account ID for this should be, if any.
     INTERNAL(n -> ImmutableSet.of(9998L, 9999L).contains(n)),
 
-    /** A registrar used for internal monitoring. Should have null IANA/billing IDs. */
+    /** A registrar used for internal monitoring. Should have null IANA/billing account IDs. */
     MONITORING(Objects::isNull),
 
-    /** A registrar used for internal testing. Should have null IANA/billing IDs. */
+    /** A registrar used for internal testing. Should have null IANA/billing account IDs. */
     TEST(Objects::isNull);
 
     /**
@@ -388,7 +390,8 @@ public class Registrar extends ImmutableObject
   @Index @Nullable Long ianaIdentifier;
 
   /** Identifier of registrar used in external billing system (e.g. Oracle). */
-  @Nullable Long billingIdentifier;
+  // TODO(sarahbot@): Drop this column from the table in a flyway script in a follow up PR.
+  @DeleteAfterMigration @Nullable @Deprecated @Ignore Long billingIdentifier;
 
   /** Purchase Order number used for invoices in external billing system, if applicable. */
   @Nullable String poNumber;
@@ -496,11 +499,6 @@ public class Registrar extends ImmutableObject
     return ianaIdentifier;
   }
 
-  @Nullable
-  public Long getBillingIdentifier() {
-    return billingIdentifier;
-  }
-
   public Optional<String> getPoNumber() {
     return Optional.ofNullable(poNumber);
   }
@@ -688,7 +686,6 @@ public class Registrar extends ImmutableObject
     return new JsonMapBuilder()
         .put("clientIdentifier", clientIdentifier)
         .put("ianaIdentifier", ianaIdentifier)
-        .put("billingIdentifier", billingIdentifier)
         .putString("creationTime", creationTime.getTimestamp())
         .putString("lastUpdateTime", lastUpdateTime.getTimestamp())
         .putString("lastCertificateUpdateTime", lastCertificateUpdateTime)
@@ -785,14 +782,6 @@ public class Registrar extends ImmutableObject
       return this;
     }
 
-    public Builder setBillingIdentifier(@Nullable Long billingIdentifier) {
-      checkArgument(
-          billingIdentifier == null || billingIdentifier > 0,
-          "Billing ID must be a positive number");
-      getInstance().billingIdentifier = billingIdentifier;
-      return this;
-    }
-
     public Builder setPoNumber(Optional<String> poNumber) {
       getInstance().poNumber = poNumber.orElse(null);
       return this;

core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java

@@ -150,14 +150,6 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand {
       validateWith = OptionalLongParameter.class)
   Optional<Long> ianaId;
 
-  @Nullable
-  @Parameter(
-      names = "--billing_id",
-      description = "Registrar Billing ID (i.e. Oracle #)",
-      converter = OptionalLongParameter.class,
-      validateWith = OptionalLongParameter.class)
-  private Optional<Long> billingId;
-
   @Nullable
   @Parameter(
       names = "--po_number",
@@ -363,9 +355,6 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand {
       if (ianaId != null) {
         builder.setIanaIdentifier(ianaId.orElse(null));
       }
-      if (billingId != null) {
-        builder.setBillingIdentifier(billingId.orElse(null));
-      }
       Optional.ofNullable(poNumber).ifPresent(builder::setPoNumber);
       if (billingAccountMap != null) {
         LinkedHashMap<CurrencyUnit, String> newBillingAccountMap = new LinkedHashMap<>();

core/src/test/java/google/registry/export/sheet/SyncRegistrarsSheetTest.java

@@ -204,7 +204,6 @@ public class SyncRegistrarsSheetTest {
     assertThat(row).containsEntry("registrarName", "AAA Registrar Inc.");
     assertThat(row).containsEntry("state", "SUSPENDED");
     assertThat(row).containsEntry("ianaIdentifier", "8");
-    assertThat(row).containsEntry("billingIdentifier", "");
     assertThat(row)
         .containsEntry(
             "primaryContacts",
@@ -300,7 +299,6 @@ public class SyncRegistrarsSheetTest {
     assertThat(row).containsEntry("registrarName", "Another Registrar LLC");
     assertThat(row).containsEntry("state", "ACTIVE");
     assertThat(row).containsEntry("ianaIdentifier", "1");
-    assertThat(row).containsEntry("billingIdentifier", "");
     assertThat(row).containsEntry("primaryContacts", "");
     assertThat(row).containsEntry("techContacts", "");
     assertThat(row).containsEntry("marketingContacts", "");
@@ -350,7 +348,6 @@ public class SyncRegistrarsSheetTest {
     assertThat(row).containsEntry("registrarName", "Some Registrar");
     assertThat(row).containsEntry("state", "ACTIVE");
     assertThat(row).containsEntry("ianaIdentifier", "8");
-    assertThat(row).containsEntry("billingIdentifier", "");
     assertThat(row).containsEntry("primaryContacts", "");
     assertThat(row).containsEntry("techContacts", "");
     assertThat(row).containsEntry("marketingContacts", "");

core/src/test/java/google/registry/model/registrar/RegistrarTest.java

@@ -121,7 +121,6 @@ class RegistrarTest extends EntityTestCase {
                 .setIcannReferralEmail("foo@example.com")
                 .setDriveFolderId("drive folder id")
                 .setIanaIdentifier(8L)
-                .setBillingIdentifier(5325L)
                 .setBillingAccountMap(
                     ImmutableMap.of(CurrencyUnit.USD, "abc123", CurrencyUnit.JPY, "789xyz"))
                 .setPhonePasscode("01234")
@@ -261,12 +260,11 @@ class RegistrarTest extends EntityTestCase {
   }
 
   @TestOfyAndSql
-  void testSuccess_clearingIanaAndBillingIds() {
+  void testSuccess_clearingIanaId() {
     registrar
         .asBuilder()
         .setType(Type.TEST)
         .setIanaIdentifier(null)
-        .setBillingIdentifier(null)
         .build();
   }
 

core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java

@@ -545,28 +545,6 @@ class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarCommand>
     assertThat(registrar.get().getIanaIdentifier()).isEqualTo(12345);
   }
 
-  @TestOfyAndSql
-  void testSuccess_billingId() throws Exception {
-    runCommandForced(
-        "--name=blobio",
-        "--password=some_password",
-        "--registrar_type=REAL",
-        "--iana_id=8",
-        "--billing_id=12345",
-        "--passcode=01234",
-        "--icann_referral_email=foo@bar.test",
-        "--street=\"123 Fake St\"",
-        "--city Fakington",
-        "--state MA",
-        "--zip 00351",
-        "--cc US",
-        "clientz");
-
-    Optional<Registrar> registrar = Registrar.loadByRegistrarId("clientz");
-    assertThat(registrar).isPresent();
-    assertThat(registrar.get().getBillingIdentifier()).isEqualTo(12345);
-  }
-
   @TestOfyAndSql
   void testSuccess_poNumber() throws Exception {
     runCommandForced(
@@ -805,7 +783,6 @@ class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarCommand>
         "--registrar_type=TEST",
         "--icann_referral_email=foo@bar.test",
         "--iana_id=null",
-        "--billing_id=null",
         "--phone=null",
         "--fax=null",
         "--url=null",
@@ -821,7 +798,6 @@ class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarCommand>
     assertThat(registrarOptional).isPresent();
     Registrar registrar = registrarOptional.get();
     assertThat(registrar.getIanaIdentifier()).isNull();
-    assertThat(registrar.getBillingIdentifier()).isNull();
     assertThat(registrar.getPhoneNumber()).isNull();
     assertThat(registrar.getFaxNumber()).isNull();
     assertThat(registrar.getUrl()).isNull();
@@ -835,7 +811,6 @@ class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarCommand>
         "--password=some_password",
         "--registrar_type=TEST",
         "--iana_id=",
-        "--billing_id=",
         "--phone=",
         "--fax=",
         "--url=",
@@ -852,7 +827,6 @@ class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarCommand>
     assertThat(registrarOptional).isPresent();
     Registrar registrar = registrarOptional.get();
     assertThat(registrar.getIanaIdentifier()).isNull();
-    assertThat(registrar.getBillingIdentifier()).isNull();
     assertThat(registrar.getPhoneNumber()).isNull();
     assertThat(registrar.getFaxNumber()).isNull();
     assertThat(registrar.getUrl()).isNull();
@@ -1514,48 +1488,6 @@ class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarCommand>
                 "clientz"));
   }
 
-  @TestOfyAndSql
-  void testFailure_negativeBillingId() {
-    assertThrows(
-        IllegalArgumentException.class,
-        () ->
-            runCommandForced(
-                "--name=blobio",
-                "--password=some_password",
-                "--registrar_type=REAL",
-                "--iana_id=8",
-                "--billing_id=-1",
-                "--passcode=01234",
-                "--icann_referral_email=foo@bar.test",
-                "--street=\"123 Fake St\"",
-                "--city Fakington",
-                "--state MA",
-                "--zip 00351",
-                "--cc US",
-                "clientz"));
-  }
-
-  @TestOfyAndSql
-  void testFailure_nonIntegerBillingId() {
-    assertThrows(
-        ParameterException.class,
-        () ->
-            runCommandForced(
-                "--name=blobio",
-                "--password=some_password",
-                "--registrar_type=REAL",
-                "--iana_id=8",
-                "--billing_id=ABC12345",
-                "--passcode=01234",
-                "--icann_referral_email=foo@bar.test",
-                "--street=\"123 Fake St\"",
-                "--city Fakington",
-                "--state MA",
-                "--zip 00351",
-                "--cc US",
-                "clientz"));
-  }
-
   @TestOfyAndSql
   void testFailure_missingPhonePasscode() {
     assertThrows(

core/src/test/java/google/registry/tools/MutatingCommandTest.java

@@ -33,6 +33,7 @@ import google.registry.model.registrar.Registrar;
 import google.registry.persistence.VKey;
 import google.registry.testing.AppEngineExtension;
 import java.util.Arrays;
+import java.util.Optional;
 import org.joda.time.DateTime;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -58,7 +59,7 @@ public class MutatingCommandTest {
   void beforeEach() {
     registrar1 = persistNewRegistrar("Registrar1", "Registrar1", Registrar.Type.REAL, 1L);
     registrar2 = persistNewRegistrar("Registrar2", "Registrar2", Registrar.Type.REAL, 2L);
-    newRegistrar1 = registrar1.asBuilder().setBillingIdentifier(42L).build();
+    newRegistrar1 = registrar1.asBuilder().setPoNumber(Optional.of("23")).build();
     newRegistrar2 = registrar2.asBuilder().setBlockPremiumNames(true).build();
 
     createTld("tld");
@@ -94,18 +95,19 @@ public class MutatingCommandTest {
     };
     command.init();
     String changes = command.prompt();
-    assertThat(changes).isEqualTo(
-        "Update HostResource@2-ROID\n"
-            + "lastEppUpdateTime: null -> 2014-09-09T09:09:09.000Z\n"
-            + "\n"
-            + "Update HostResource@3-ROID\n"
-            + "currentSponsorClientId: TheRegistrar -> Registrar2\n"
-            + "\n"
-            + "Update Registrar@Registrar1\n"
-            + "billingIdentifier: null -> 42\n"
-            + "\n"
-            + "Update Registrar@Registrar2\n"
-            + "blockPremiumNames: false -> true\n");
+    assertThat(changes)
+        .isEqualTo(
+            "Update HostResource@2-ROID\n"
+                + "lastEppUpdateTime: null -> 2014-09-09T09:09:09.000Z\n"
+                + "\n"
+                + "Update HostResource@3-ROID\n"
+                + "currentSponsorClientId: TheRegistrar -> Registrar2\n"
+                + "\n"
+                + "Update Registrar@Registrar1\n"
+                + "poNumber: null -> 23\n"
+                + "\n"
+                + "Update Registrar@Registrar2\n"
+                + "blockPremiumNames: false -> true\n");
     String results = command.execute();
     assertThat(results).isEqualTo("Updated 4 entities.\n");
     assertThat(loadByEntity(host1)).isEqualTo(newHost1);

core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java

@@ -347,13 +347,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
     assertThat(loadRegistrar("NewRegistrar").getIanaIdentifier()).isEqualTo(12345);
   }
 
-  @Test
-  void testSuccess_billingId() throws Exception {
-    assertThat(loadRegistrar("NewRegistrar").getBillingIdentifier()).isNull();
-    runCommand("--billing_id=12345", "--force", "NewRegistrar");
-    assertThat(loadRegistrar("NewRegistrar").getBillingIdentifier()).isEqualTo(12345);
-  }
-
   @Test
   void testSuccess_poNumber() throws Exception {
     assertThat(loadRegistrar("NewRegistrar").getPoNumber()).isEmpty();
@@ -504,7 +497,7 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
             .setContactsRequireSyncing(true)
             .build());
     // Make some unrelated change where we don't specify the flags for the booleans.
-    runCommandForced("--billing_id=12345", "NewRegistrar");
+    runCommandForced("NewRegistrar");
     // Make sure that the boolean fields didn't get reset back to false.
     Registrar reloadedRegistrar = loadRegistrar("NewRegistrar");
     assertThat(reloadedRegistrar.getBlockPremiumNames()).isTrue();
@@ -529,7 +522,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
                 .asBuilder()
                 .setType(Type.PDT) // for non-null IANA ID
                 .setIanaIdentifier(9995L)
-                .setBillingIdentifier(1L)
                 .setPhoneNumber("+1.2125555555")
                 .setFaxNumber("+1.2125555556")
                 .setUrl("http://www.example.tld")
@@ -537,7 +529,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
                 .build());
 
     assertThat(registrar.getIanaIdentifier()).isNotNull();
-    assertThat(registrar.getBillingIdentifier()).isNotNull();
     assertThat(registrar.getPhoneNumber()).isNotNull();
     assertThat(registrar.getFaxNumber()).isNotNull();
     assertThat(registrar.getUrl()).isNotNull();
@@ -546,7 +537,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
     runCommand(
         "--registrar_type=TEST", // necessary for null IANA ID
         "--iana_id=null",
-        "--billing_id=null",
         "--phone=null",
         "--fax=null",
         "--url=null",
@@ -556,7 +546,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
 
     registrar = loadRegistrar("NewRegistrar");
     assertThat(registrar.getIanaIdentifier()).isNull();
-    assertThat(registrar.getBillingIdentifier()).isNull();
     assertThat(registrar.getPhoneNumber()).isNull();
     assertThat(registrar.getFaxNumber()).isNull();
     assertThat(registrar.getUrl()).isNull();
@@ -572,7 +561,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
                 .asBuilder()
                 .setType(Type.PDT) // for non-null IANA ID
                 .setIanaIdentifier(9995L)
-                .setBillingIdentifier(1L)
                 .setPhoneNumber("+1.2125555555")
                 .setFaxNumber("+1.2125555556")
                 .setUrl("http://www.example.tld")
@@ -580,7 +568,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
                 .build());
 
     assertThat(registrar.getIanaIdentifier()).isNotNull();
-    assertThat(registrar.getBillingIdentifier()).isNotNull();
     assertThat(registrar.getPhoneNumber()).isNotNull();
     assertThat(registrar.getFaxNumber()).isNotNull();
     assertThat(registrar.getUrl()).isNotNull();
@@ -589,7 +576,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
     runCommand(
         "--registrar_type=TEST", // necessary for null IANA ID
         "--iana_id=",
-        "--billing_id=",
         "--phone=",
         "--fax=",
         "--url=",
@@ -599,7 +585,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
 
     registrar = loadRegistrar("NewRegistrar");
     assertThat(registrar.getIanaIdentifier()).isNull();
-    assertThat(registrar.getBillingIdentifier()).isNull();
     assertThat(registrar.getPhoneNumber()).isNull();
     assertThat(registrar.getFaxNumber()).isNull();
     assertThat(registrar.getUrl()).isNull();
@@ -663,20 +648,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
         ParameterException.class, () -> runCommand("--iana_id=ABC123", "--force", "NewRegistrar"));
   }
 
-  @Test
-  void testFailure_negativeBillingId() {
-    assertThrows(
-        IllegalArgumentException.class,
-        () -> runCommand("--billing_id=-1", "--force", "NewRegistrar"));
-  }
-
-  @Test
-  void testFailure_nonIntegerBillingId() {
-    assertThrows(
-        ParameterException.class,
-        () -> runCommand("--billing_id=ABC123", "--force", "NewRegistrar"));
-  }
-
   @Test
   void testFailure_passcodeTooShort() {
     assertThrows(

core/src/test/resources/google/registry/model/schema.txt

@@ -568,7 +568,6 @@ class google.registry.model.registrar.Registrar {
   google.registry.model.registrar.Registrar$Type type;
   google.registry.model.registrar.RegistrarAddress internationalizedAddress;
   google.registry.model.registrar.RegistrarAddress localizedAddress;
-  java.lang.Long billingIdentifier;
   java.lang.Long ianaIdentifier;
   java.lang.String clientCertificate;
   java.lang.String clientCertificateHash;