From 9786e6732f0725dba7426f20ddf31dc0de72c59b Mon Sep 17 00:00:00 2001 From: nickfelt Date: Mon, 14 Mar 2016 13:31:48 -0700 Subject: [PATCH] Make GracePeriod OneTime vs Recurring logic explicit This CL cleans up some old crufy logic in GracePeriod that overloaded methods to accept either OneTime or Recurring billing events refs, despite storing them in separate fields in the entity (since BillingEvent is not a polymorphic superclass, just a Java-only one, you can't store them as refs to BillingEvent). That overloading was ultimately only there as a convenience/hack from when we added Recurring events and didn't want to go back and change everything. It obfuscates what's really going on, requires extra casting/loss of type-safety, and relies on indirect signals (e.g. the grace period type being AUTO_RENEW) to guess what the right billing event type is. That latter aspect will likely no longer work in a monthly billing world, and was brittle in any case. A coming CL will rip the same logic out of Cancellation. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=117164286 --- .../flows/domain/DomainDeleteFlow.java | 7 +- .../flows/domain/DomainUpdateFlow.java | 2 +- .../registry/model/billing/BillingEvent.java | 4 +- .../registry/model/domain/DomainResource.java | 2 +- .../registry/model/domain/GracePeriod.java | 73 +++++++---- .../domain/registry/flows/FlowTestCase.java | 8 +- .../flows/domain/DomainDeleteFlowTest.java | 2 +- .../flows/domain/DomainInfoFlowTest.java | 9 +- .../model/domain/DomainResourceTest.java | 2 +- .../model/domain/GracePeriodTest.java | 115 ++++++++++++++++++ 10 files changed, 186 insertions(+), 38 deletions(-) create mode 100644 javatests/com/google/domain/registry/model/domain/GracePeriodTest.java diff --git a/java/com/google/domain/registry/flows/domain/DomainDeleteFlow.java b/java/com/google/domain/registry/flows/domain/DomainDeleteFlow.java index 2723d579c..2d1679aa5 100644 --- a/java/com/google/domain/registry/flows/domain/DomainDeleteFlow.java +++ b/java/com/google/domain/registry/flows/domain/DomainDeleteFlow.java @@ -110,11 +110,10 @@ public class DomainDeleteFlow extends ResourceSyncDeleteFlow creditsBuilder = new ImmutableList.Builder<>(); for (GracePeriod gracePeriod : existingResource.getGracePeriods()) { // No cancellation is written if the grace period was not for a billable event. - if (gracePeriod.getBillingEvent() != null) { + if (gracePeriod.hasBillingEvent()) { ofy().save().entity( BillingEvent.Cancellation.forGracePeriod(gracePeriod, historyEntry, targetId)); diff --git a/java/com/google/domain/registry/flows/domain/DomainUpdateFlow.java b/java/com/google/domain/registry/flows/domain/DomainUpdateFlow.java index 187b35e0d..27d963f17 100644 --- a/java/com/google/domain/registry/flows/domain/DomainUpdateFlow.java +++ b/java/com/google/domain/registry/flows/domain/DomainUpdateFlow.java @@ -104,7 +104,7 @@ public class DomainUpdateFlow extends BaseDomainUpdateFlow getBillingEvent() { - return Optional.>fromNullable(billingEventOneTime) - .or(Optional.>fromNullable(billingEventRecurring)) - .orNull(); + /** Returns true if this GracePeriod has an associated BillingEvent; i.e. if it's refundable. */ + public boolean hasBillingEvent() { + return billingEventOneTime != null || billingEventRecurring != null; } /** @@ -106,26 +100,53 @@ public class GracePeriod extends ImmutableObject { return billingEventRecurring; } - /** - * Constructs a GracePeriod with some interpretation of the parameters. In particular, selects - * the field to store the billing event ref in based on the specified grace period status type. + private static GracePeriod createInternal( + GracePeriodStatus type, + DateTime expirationTime, + String clientId, + @Nullable Ref billingEventOneTime, + @Nullable Ref billingEventRecurring) { + checkArgument((billingEventOneTime == null) || (billingEventRecurring == null), + "A grace period can have at most one billing event"); + checkArgument((billingEventRecurring != null) == (GracePeriodStatus.AUTO_RENEW.equals(type)), + "Recurring billing events must be present on (and only on) autorenew grace periods"); + GracePeriod instance = new GracePeriod(); + instance.type = checkArgumentNotNull(type); + instance.expirationTime = checkArgumentNotNull(expirationTime); + instance.clientId = checkArgumentNotNull(clientId); + instance.billingEventOneTime = billingEventOneTime; + instance.billingEventRecurring = billingEventRecurring; + return instance; + } + + /** Create a GracePeriod for an (optional) OneTime billing event. + * + *

Normal callers should always use {@link #forBillingEvent} instead, assuming they do not + * need to avoid loading the BillingEvent from datastore. This method should typically be + * called only from test code to explicitly construct GracePeriods. */ - @SuppressWarnings("unchecked") public static GracePeriod create( GracePeriodStatus type, DateTime expirationTime, String clientId, - @Nullable Ref billingEvent) { - GracePeriod instance = new GracePeriod(); - instance.type = checkNotNull(type); - instance.expirationTime = checkNotNull(expirationTime); - instance.clientId = checkNotNull(clientId); - if (GracePeriodStatus.AUTO_RENEW.equals(instance.type)) { - instance.billingEventRecurring = (Ref) billingEvent; - } else { - instance.billingEventOneTime = (Ref) billingEvent; - } - return instance; + @Nullable Ref billingEventOneTime) { + return createInternal(type, expirationTime, clientId, billingEventOneTime, null); + } + + /** Create a GracePeriod for a Recurring billing event. */ + public static GracePeriod createForRecurring( + GracePeriodStatus type, + DateTime expirationTime, + String clientId, + Ref billingEventRecurring) { + checkArgumentNotNull(billingEventRecurring); + return createInternal(type, expirationTime, clientId, null, billingEventRecurring); + } + + /** Create a GracePeriod with no billing event. */ + public static GracePeriod createWithoutBillingEvent( + GracePeriodStatus type, DateTime expirationTime, String clientId) { + return createInternal(type, expirationTime, clientId, null, null); } /** Constructs a GracePeriod of the given type from the provided one-time BillingEvent. */ diff --git a/javatests/com/google/domain/registry/flows/FlowTestCase.java b/javatests/com/google/domain/registry/flows/FlowTestCase.java index 0c5c686fb..dc3ecc3ce 100644 --- a/javatests/com/google/domain/registry/flows/FlowTestCase.java +++ b/javatests/com/google/domain/registry/flows/FlowTestCase.java @@ -227,7 +227,13 @@ public abstract class FlowTestCase { new Function() { @Override public BillingEvent apply(GracePeriod gracePeriod) { - return gracePeriod.getBillingEvent().get(); + assertThat(gracePeriod.hasBillingEvent()) + .named("Billing event is present for grace period: " + gracePeriod) + .isTrue(); + return firstNonNull( + gracePeriod.getOneTimeBillingEvent(), + gracePeriod.getRecurringBillingEvent()) + .get(); }}; assertThat(canonicalizeGracePeriods(Maps.toMap(actual, gracePeriodExpander))) .isEqualTo(canonicalizeGracePeriods(expected)); diff --git a/javatests/com/google/domain/registry/flows/domain/DomainDeleteFlowTest.java b/javatests/com/google/domain/registry/flows/domain/DomainDeleteFlowTest.java index 24de6ecdc..e15ee2292 100644 --- a/javatests/com/google/domain/registry/flows/domain/DomainDeleteFlowTest.java +++ b/javatests/com/google/domain/registry/flows/domain/DomainDeleteFlowTest.java @@ -143,7 +143,7 @@ public class DomainDeleteFlowTest extends ResourceFlowTestCase