From 6af9299a3c12d6a9b757a90e8f6d93d704cbcd3e Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Tue, 16 Nov 2021 16:12:08 -0500 Subject: [PATCH] Grandfather in old data for one-time billing event requirement (#1423) * Grandfather in old data for one-time billing event requirement We have data from 2018 and earlier where we didn't consistently set periodYears for OneTime BillingEvents with certain reasons. This grandfathers in that old data so that we can successfully move it over to Cloud SQL for now, then we can later run a query that will backfill it, after which we can then tighten up the requirement again. Note that the requirement is still being enforced for all billing events from 2019 onwards. This also improves the handling of validation, by adding a private field to the Reason enum rather than creating a throwaway inline ImmmutableSet in the Builder. --- .../registry/model/billing/BillingEvent.java | 51 +++++++++------- .../model/billing/BillingEventTest.java | 58 +++++++++++++++++++ 2 files changed, 89 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/google/registry/model/billing/BillingEvent.java b/core/src/main/java/google/registry/model/billing/BillingEvent.java index f702fd9ab..af436282d 100644 --- a/core/src/main/java/google/registry/model/billing/BillingEvent.java +++ b/core/src/main/java/google/registry/model/billing/BillingEvent.java @@ -27,7 +27,6 @@ import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; import com.googlecode.objectify.Key; import com.googlecode.objectify.annotation.Entity; import com.googlecode.objectify.annotation.Id; @@ -77,14 +76,29 @@ public abstract class BillingEvent extends ImmutableObject /** The reason for the bill, which maps 1:1 to skus in go/registry-billing-skus. */ public enum Reason { - CREATE, + CREATE(true), @Deprecated // TODO(b/31676071): remove this legacy value once old data is cleaned up. - ERROR, - FEE_EARLY_ACCESS, - RENEW, - RESTORE, - SERVER_STATUS, - TRANSFER + ERROR(false), + FEE_EARLY_ACCESS(true), + RENEW(true), + RESTORE(true), + SERVER_STATUS(false), + TRANSFER(true); + + private final boolean requiresPeriod; + + Reason(boolean requiresPeriod) { + this.requiresPeriod = requiresPeriod; + } + + /** + * Returns whether billing events with this reason have a period years associated with them. + * + *

Note that this is an "if an only if" condition. + */ + public boolean hasPeriodYears() { + return requiresPeriod; + } } /** Set of flags that can be applied to billing events. */ @@ -268,7 +282,7 @@ public abstract class BillingEvent extends ImmutableObject public T build() { T instance = getInstance(); checkNotNull(instance.reason, "Reason must be set"); - checkNotNull(instance.clientId, "Client ID must be set"); + checkNotNull(instance.clientId, "Registrar ID must be set"); checkNotNull(instance.eventTime, "Event time must be set"); checkNotNull(instance.targetId, "Target ID must be set"); checkNotNull(instance.parent, "Parent must be set"); @@ -462,17 +476,14 @@ public abstract class BillingEvent extends ImmutableObject checkNotNull(instance.billingTime); checkNotNull(instance.cost); checkState(!instance.cost.isNegative(), "Costs should be non-negative."); - ImmutableSet reasonsWithPeriods = - Sets.immutableEnumSet( - Reason.CREATE, - Reason.FEE_EARLY_ACCESS, - Reason.RENEW, - Reason.RESTORE, - Reason.TRANSFER); - checkState( - reasonsWithPeriods.contains(instance.reason) == (instance.periodYears != null), - "Period years must be set if and only if reason is " - + "CREATE, FEE_EARLY_ACCESS, RENEW, RESTORE or TRANSFER."); + // TODO(mcilwain): Enforce this check on all billing events (not just more recent ones) + // post-migration after we add the missing period years values in SQL. + if (instance.eventTime.isAfter(DateTime.parse("2019-01-01T00:00:00Z"))) { + checkState( + instance.reason.hasPeriodYears() == (instance.periodYears != null), + "Period years must be set if and only if reason is " + + "CREATE, FEE_EARLY_ACCESS, RENEW, RESTORE or TRANSFER."); + } checkState( instance.getFlags().contains(Flag.SYNTHETIC) == (instance.syntheticCreationTime != null), diff --git a/core/src/test/java/google/registry/model/billing/BillingEventTest.java b/core/src/test/java/google/registry/model/billing/BillingEventTest.java index d7b284e35..5acdd1c6a 100644 --- a/core/src/test/java/google/registry/model/billing/BillingEventTest.java +++ b/core/src/test/java/google/registry/model/billing/BillingEventTest.java @@ -416,4 +416,62 @@ public class BillingEventTest extends EntityTestCase { assertThat(recurring.getParentKey()).isEqualTo(Key.create(domainHistory)); new BillingEvent.OneTime.Builder().setParent(Key.create(domainHistory)); } + + @TestOfyAndSql + void testReasonRequiringPeriodYears_missingPeriodYears_throwsException() { + IllegalStateException thrown = + assertThrows( + IllegalStateException.class, + () -> + new BillingEvent.OneTime.Builder() + .setBillingTime(DateTime.parse("2020-02-05T15:33:11Z")) + .setEventTime(DateTime.parse("2020-01-05T15:33:11Z")) + .setCost(Money.of(USD, 10)) + .setReason(Reason.RENEW) + .setCost(Money.of(USD, 10)) + .setRegistrarId("TheRegistrar") + .setTargetId("example.tld") + .setParent(domainHistory) + .build()); + assertThat(thrown) + .hasMessageThat() + .contains("Period years must be set if and only if reason is"); + } + + @TestOfyAndSql + void testReasonNotRequiringPeriodYears_havingPeriodYears_throwsException() { + IllegalStateException thrown = + assertThrows( + IllegalStateException.class, + () -> + new BillingEvent.OneTime.Builder() + .setBillingTime(DateTime.parse("2020-02-05T15:33:11Z")) + .setEventTime(DateTime.parse("2020-01-05T15:33:11Z")) + .setCost(Money.of(USD, 10)) + .setPeriodYears(2) + .setReason(Reason.SERVER_STATUS) + .setCost(Money.of(USD, 10)) + .setRegistrarId("TheRegistrar") + .setTargetId("example.tld") + .setParent(domainHistory) + .build()); + assertThat(thrown) + .hasMessageThat() + .contains("Period years must be set if and only if reason is"); + } + + @TestOfyAndSql + void testReasonRequiringPeriodYears_missingPeriodYears_isAllowedOnOldData() { + // This won't throw even though periodYears is missing on a RESTORE because the event time + // is before 2019. + new BillingEvent.OneTime.Builder() + .setBillingTime(DateTime.parse("2018-02-05T15:33:11Z")) + .setEventTime(DateTime.parse("2018-01-05T15:33:11Z")) + .setReason(Reason.RESTORE) + .setCost(Money.of(USD, 10)) + .setRegistrarId("TheRegistrar") + .setTargetId("example.tld") + .setParent(domainHistory) + .build(); + } }