From c262ef82c9ffcec724f2c11eaf0db2e42feecc29 Mon Sep 17 00:00:00 2001 From: sarahcaseybot Date: Wed, 18 May 2022 17:15:45 -0400 Subject: [PATCH] Remove all uses of the billingIdentifier field (#1608) * Remove all uses of the billingIdentifier field * Add @ignore flag * Add tag --- .../export/sheet/SyncRegistrarsSheet.java | 1 - .../registry/model/registrar/Registrar.java | 35 ++++------ .../tools/CreateOrUpdateRegistrarCommand.java | 11 --- .../export/sheet/SyncRegistrarsSheetTest.java | 3 - .../model/registrar/RegistrarTest.java | 4 +- .../tools/CreateRegistrarCommandTest.java | 68 ------------------- .../registry/tools/MutatingCommandTest.java | 28 ++++---- .../tools/UpdateRegistrarCommandTest.java | 31 +-------- .../google/registry/model/schema.txt | 1 - 9 files changed, 29 insertions(+), 153 deletions(-) diff --git a/core/src/main/java/google/registry/export/sheet/SyncRegistrarsSheet.java b/core/src/main/java/google/registry/export/sheet/SyncRegistrarsSheet.java index f718bfa43..70cb1c3c8 100644 --- a/core/src/main/java/google/registry/export/sheet/SyncRegistrarsSheet.java +++ b/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))); diff --git a/core/src/main/java/google/registry/model/registrar/Registrar.java b/core/src/main/java/google/registry/model/registrar/Registrar.java index d06c9a233..2aabaf05d 100644 --- a/core/src/main/java/google/registry/model/registrar/Registrar.java +++ b/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 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 poNumber) { getInstance().poNumber = poNumber.orElse(null); return this; diff --git a/core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java b/core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java index f5625ca82..6726f615e 100644 --- a/core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java +++ b/core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java @@ -150,14 +150,6 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand { validateWith = OptionalLongParameter.class) Optional ianaId; - @Nullable - @Parameter( - names = "--billing_id", - description = "Registrar Billing ID (i.e. Oracle #)", - converter = OptionalLongParameter.class, - validateWith = OptionalLongParameter.class) - private Optional 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 newBillingAccountMap = new LinkedHashMap<>(); diff --git a/core/src/test/java/google/registry/export/sheet/SyncRegistrarsSheetTest.java b/core/src/test/java/google/registry/export/sheet/SyncRegistrarsSheetTest.java index 9934d15d4..b228684e7 100644 --- a/core/src/test/java/google/registry/export/sheet/SyncRegistrarsSheetTest.java +++ b/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", ""); diff --git a/core/src/test/java/google/registry/model/registrar/RegistrarTest.java b/core/src/test/java/google/registry/model/registrar/RegistrarTest.java index 599eb9b85..705d822e7 100644 --- a/core/src/test/java/google/registry/model/registrar/RegistrarTest.java +++ b/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(); } diff --git a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java b/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java index ba479e71e..e39ef0889 100644 --- a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java +++ b/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java @@ -545,28 +545,6 @@ class CreateRegistrarCommandTest extends CommandTestCase 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.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 "--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 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 "--password=some_password", "--registrar_type=TEST", "--iana_id=", - "--billing_id=", "--phone=", "--fax=", "--url=", @@ -852,7 +827,6 @@ class CreateRegistrarCommandTest extends CommandTestCase 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 "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( diff --git a/core/src/test/java/google/registry/tools/MutatingCommandTest.java b/core/src/test/java/google/registry/tools/MutatingCommandTest.java index 64ed8617a..285d49d55 100644 --- a/core/src/test/java/google/registry/tools/MutatingCommandTest.java +++ b/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); diff --git a/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java b/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java index f30bb0f5d..a6e749df6 100644 --- a/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java +++ b/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java @@ -347,13 +347,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase 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 .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 .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 .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 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 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 .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 .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 runCommand( "--registrar_type=TEST", // necessary for null IANA ID "--iana_id=", - "--billing_id=", "--phone=", "--fax=", "--url=", @@ -599,7 +585,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase 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 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( diff --git a/core/src/test/resources/google/registry/model/schema.txt b/core/src/test/resources/google/registry/model/schema.txt index d96a1d2d3..47543aa5a 100644 --- a/core/src/test/resources/google/registry/model/schema.txt +++ b/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;