Make Cancellation OneTime vs Recurring logic explicit

This CL does a very similar cleanup to Cancellation as the diffbase
does to GracePeriod - see []  It simplifes logic where we
used to overload methods to accept eitherw 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
Billing event reason 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 anyways (as came up in the
context of removing the AUTO_RENEW reason - see []
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=117165174

java/com/google/domain/registry/flows/domain/DomainTransferRequestFlow.java

@@ -201,7 +201,7 @@ public class DomainTransferRequestFlow
           .setClientId(existingResource.getCurrentSponsorClientId())
           .setEventTime(automaticTransferTime)
           .setBillingTime(expirationTime.plus(registry.getAutoRenewGracePeriodLength()))
-          .setEventRef(existingResource.getAutorenewBillingEvent())
+          .setRecurringEventRef(existingResource.getAutorenewBillingEvent())
           .setParent(historyEntry)
           .build();
       ofy().save().entity(autorenewCancellation);

java/com/google/domain/registry/model/billing/BillingEvent.java

@@ -376,24 +376,30 @@ public abstract class BillingEvent extends ImmutableObject
             GracePeriodStatus.TRANSFER, Reason.TRANSFER);
 
     /**
-     * Creates a cancellation billing event for the provided grace period parented on the provided
+     * Creates a cancellation billing event (parented on the provided history entry, and with the
-     * history entry (and with the same event time) that will cancel out the
+     * history entry's event time) that will cancel out the provided grace period's billing event,
-     * grace-period-originating billing event on the supplied targetId.
+     * using the supplied targetId and deriving other metadata (clientId, billing time, and the
+     * cancellation reason) from the grace period.
      */
     public static BillingEvent.Cancellation forGracePeriod(
         GracePeriod gracePeriod, HistoryEntry historyEntry, String targetId) {
-      return new BillingEvent.Cancellation.Builder()
+      checkArgument(gracePeriod.hasBillingEvent(),
+          "Cannot create cancellation for grace period without billing event");
+      BillingEvent.Cancellation.Builder builder = new BillingEvent.Cancellation.Builder()
           .setReason(checkNotNull(GRACE_PERIOD_TO_REASON.get(gracePeriod.getType())))
           .setTargetId(targetId)
           .setClientId(gracePeriod.getClientId())
           .setEventTime(historyEntry.getModificationTime())
           // The charge being cancelled will take place at the grace period's expiration time.
           .setBillingTime(gracePeriod.getExpirationTime())
-          .setEventRef(firstNonNull(
+          .setParent(historyEntry);
-              gracePeriod.getOneTimeBillingEvent(),
+      // Set the grace period's billing event using the appropriate Cancellation builder method.
-              gracePeriod.getRecurringBillingEvent()))
+      if (gracePeriod.getOneTimeBillingEvent() != null) {
-          .setParent(historyEntry)
+        builder.setOneTimeEventRef(gracePeriod.getOneTimeBillingEvent());
-          .build();
+      } else if (gracePeriod.getRecurringBillingEvent() != null) {
+        builder.setRecurringEventRef(gracePeriod.getRecurringBillingEvent());
+      }
+      return builder.build();
     }
 
     @Override
@@ -404,8 +410,6 @@ public abstract class BillingEvent extends ImmutableObject
     /** A builder for {@link Cancellation} since it is immutable. */
     public static class Builder extends BillingEvent.Builder<Cancellation, Builder> {
 
-      private Ref<? extends BillingEvent> refTemp;
-
       public Builder() {}
 
       private Builder(Cancellation instance) {
@@ -417,28 +421,23 @@ public abstract class BillingEvent extends ImmutableObject
         return this;
       }
 
-      public Builder setEventRef(Ref<? extends BillingEvent> eventRef) {
+      public Builder setOneTimeEventRef(Ref<BillingEvent.OneTime> eventRef) {
-        refTemp = eventRef;
+        getInstance().refOneTime = eventRef;
+        return this;
+      }
+
+      public Builder setRecurringEventRef(Ref<BillingEvent.Recurring> eventRef) {
+        getInstance().refRecurring = eventRef;
         return this;
       }
 
       @Override
-      @SuppressWarnings("unchecked")
       public Cancellation build() {
         Cancellation instance = getInstance();
         checkNotNull(instance.billingTime);
         checkNotNull(instance.reason);
-        // If refTemp is set, use it to populate the correct ref.
+        checkState((instance.refOneTime == null) != (instance.refRecurring == null),
-        if (refTemp != null) {
+            "Cancellations must have exactly one billing event ref set");
-          if (Reason.AUTO_RENEW.equals(instance.reason)) {
-            instance.refRecurring = (Ref<BillingEvent.Recurring>) refTemp;
-          } else {
-            instance.refOneTime = (Ref<BillingEvent.OneTime>) refTemp;
-          }
-        }
-        // Ensure that even if refTemp was not set, the builder has exactly one of the two refs
-        // set to a non-null value (using != as an XOR for booleans).
-        checkState((instance.refOneTime == null) != (instance.refRecurring == null));
         return super.build();
       }
     }

javatests/com/google/domain/registry/flows/domain/DomainDeleteFlowTest.java

@@ -169,7 +169,7 @@ public class DomainDeleteFlowTest extends ResourceFlowTestCase<DomainDeleteFlow,
             .setClientId("TheRegistrar")
             .setEventTime(eventTime)
             .setBillingTime(TIME_BEFORE_FLOW.plusDays(1))
-            .setEventRef(Ref.create(graceBillingEvent))
+            .setOneTimeEventRef(Ref.create(graceBillingEvent))
             .setParent(historyEntryDomainDelete)
             .build());
   }

javatests/com/google/domain/registry/flows/domain/DomainTransferApproveFlowTest.java

@@ -373,7 +373,7 @@ public class DomainTransferApproveFlowTest
             .setEventTime(clock.nowUtc())  // The cancellation happens at the moment of transfer.
             .setBillingTime(
                 oldExpirationTime.plus(Registry.get("tld").getAutoRenewGracePeriodLength()))
-            .setEventRef(domain.getAutorenewBillingEvent()));
+            .setRecurringEventRef(domain.getAutorenewBillingEvent()));
   }
 
   @Test

javatests/com/google/domain/registry/flows/domain/DomainTransferRequestFlowTest.java

@@ -407,7 +407,7 @@ public class DomainTransferRequestFlowTest
             .setBillingTime(oldResource.getRegistrationExpirationTime().plus(
                 Registry.get("tld").getAutoRenewGracePeriodLength()))
             // The cancellation should refer to the old autorenew billing event.
-            .setEventRef(oldResource.getAutorenewBillingEvent()));
+            .setRecurringEventRef(oldResource.getAutorenewBillingEvent()));
   }
 
   @Test

javatests/com/google/domain/registry/flows/domain/DomainUpdateFlowTest.java

@@ -198,7 +198,7 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
             .setClientId("TheRegistrar")
             .setEventTime(clock.nowUtc())
             .setBillingTime(sunrushAddBillingEvent.getBillingTime())
-            .setEventRef(Ref.create(sunrushAddBillingEvent))
+            .setOneTimeEventRef(Ref.create(sunrushAddBillingEvent))
             .setParent(historyEntryDomainUpdate)
             .build(),
         regularAddBillingEvent);

javatests/com/google/domain/registry/model/billing/BillingEventTest.java

@@ -19,67 +19,103 @@ import static com.google.domain.registry.model.ofy.ObjectifyService.ofy;
 import static com.google.domain.registry.testing.DatastoreHelper.createTld;
 import static com.google.domain.registry.testing.DatastoreHelper.persistActiveDomain;
 import static com.google.domain.registry.testing.DatastoreHelper.persistResource;
-import static com.google.domain.registry.util.DateTimeUtils.START_OF_TIME;
+import static com.google.domain.registry.util.DateTimeUtils.END_OF_TIME;
 import static org.joda.money.CurrencyUnit.USD;
+import static org.joda.time.DateTimeZone.UTC;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.domain.registry.model.EntityTestCase;
 import com.google.domain.registry.model.billing.BillingEvent.Reason;
 import com.google.domain.registry.model.domain.DomainResource;
+import com.google.domain.registry.model.domain.GracePeriod;
+import com.google.domain.registry.model.domain.rgp.GracePeriodStatus;
 import com.google.domain.registry.model.reporting.HistoryEntry;
+import com.google.domain.registry.testing.ExceptionRule;
 
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.Ref;
 
 import org.joda.money.Money;
+import org.joda.time.DateTime;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
 
 /** Unit tests for {@link BillingEvent}. */
 public class BillingEventTest extends EntityTestCase {
 
+  @Rule
+  public final ExceptionRule thrown = new ExceptionRule();
+
+  private final DateTime now = DateTime.now(UTC);
+
   HistoryEntry historyEntry;
+  HistoryEntry historyEntry2;
   DomainResource domain;
-  BillingEvent oneTime;
+  BillingEvent.OneTime oneTime;
-  BillingEvent recurring;
+  BillingEvent.Recurring recurring;
-  BillingEvent cancellation;
+  BillingEvent.Cancellation cancellationOneTime;
-  BillingEvent modification;
+  BillingEvent.Cancellation cancellationRecurring;
+  BillingEvent.Modification modification;
 
   @Before
   public void setUp() throws Exception {
     createTld("tld");
-    domain = persistActiveDomain("ratt.tld");
+    domain = persistActiveDomain("foo.tld");
-    historyEntry = persistResource(new HistoryEntry.Builder().setParent(domain).build());
+    historyEntry = persistResource(
+        new HistoryEntry.Builder()
+            .setParent(domain)
+            .setModificationTime(now)
+            .build());
+    historyEntry2 = persistResource(
+        new HistoryEntry.Builder()
+        .setParent(domain)
+        .setModificationTime(now.plusDays(1))
+        .build());
+
     oneTime = persistResource(commonInit(
         new BillingEvent.OneTime.Builder()
+            .setParent(historyEntry)
             .setReason(Reason.CREATE)
+            .setFlags(ImmutableSet.of(BillingEvent.Flag.ANCHOR_TENANT))
             .setPeriodYears(2)
             .setCost(Money.of(USD, 1))
-            .setBillingTime(START_OF_TIME)));
+            .setEventTime(now)
+            .setBillingTime(now.plusDays(5))));
     recurring = persistResource(commonInit(
         new BillingEvent.Recurring.Builder()
+            .setParent(historyEntry)
             .setReason(Reason.AUTO_RENEW)
-            .setRecurrenceEndTime(START_OF_TIME)));
+            .setEventTime(now.plusYears(1))
-    cancellation = persistResource(commonInit(
+            .setRecurrenceEndTime(END_OF_TIME)));
+    cancellationOneTime = persistResource(commonInit(
         new BillingEvent.Cancellation.Builder()
+            .setParent(historyEntry2)
             .setReason(Reason.CREATE)
-            .setBillingTime(START_OF_TIME)
+            .setEventTime(now.plusDays(1))
-            .setEventRef(Ref.create(oneTime))));
+            .setBillingTime(now.plusDays(5))
+            .setOneTimeEventRef(Ref.create(oneTime))));
+    cancellationRecurring = persistResource(commonInit(
+        new BillingEvent.Cancellation.Builder()
+            .setParent(historyEntry2)
+            .setReason(Reason.AUTO_RENEW)
+            .setEventTime(now.plusDays(1))
+            .setBillingTime(now.plusYears(1).plusDays(45))
+            .setRecurringEventRef(Ref.create(recurring))));
     modification = persistResource(commonInit(
         new BillingEvent.Modification.Builder()
+            .setParent(historyEntry2)
             .setReason(Reason.CREATE)
             .setCost(Money.of(USD, 1))
             .setDescription("Something happened")
-            .setEventRef(Ref.create((BillingEvent.OneTime) oneTime))));
+            .setEventTime(now.plusDays(1))
+            .setEventRef(Ref.create(oneTime))));
   }
 
-  private BillingEvent commonInit(BillingEvent.Builder<?, ?> builder) {
+  private <E extends BillingEvent, B extends BillingEvent.Builder<E, B>> E commonInit(B builder) {
     return builder
         .setClientId("a registrar")
-        .setFlags(ImmutableSet.of(BillingEvent.Flag.ANCHOR_TENANT))
+        .setTargetId("foo.tld")
-        .setEventTime(START_OF_TIME)
-        .setTargetId("foo")
-        .setParent(historyEntry)
         .build();
   }
 
@@ -87,7 +123,8 @@ public class BillingEventTest extends EntityTestCase {
   public void testPersistence() throws Exception {
     assertThat(ofy().load().entity(oneTime).now()).isEqualTo(oneTime);
     assertThat(ofy().load().entity(recurring).now()).isEqualTo(recurring);
-    assertThat(ofy().load().entity(cancellation).now()).isEqualTo(cancellation);
+    assertThat(ofy().load().entity(cancellationOneTime).now()).isEqualTo(cancellationOneTime);
+    assertThat(ofy().load().entity(cancellationRecurring).now()).isEqualTo(cancellationRecurring);
     assertThat(ofy().load().entity(modification).now()).isEqualTo(modification);
   }
 
@@ -100,16 +137,16 @@ public class BillingEventTest extends EntityTestCase {
     assertThat(ofy().load().type(BillingEvent.Recurring.class).ancestor(domain).list())
         .containsExactly(recurring);
     assertThat(ofy().load().type(BillingEvent.Cancellation.class).ancestor(domain).list())
-        .containsExactly(cancellation);
+        .containsExactly(cancellationOneTime, cancellationRecurring);
     assertThat(ofy().load().type(BillingEvent.Modification.class).ancestor(domain).list())
         .containsExactly(modification);
     assertThat(ofy().load().type(BillingEvent.OneTime.class).ancestor(historyEntry).list())
         .containsExactly(oneTime);
     assertThat(ofy().load().type(BillingEvent.Recurring.class).ancestor(historyEntry).list())
         .containsExactly(recurring);
-    assertThat(ofy().load().type(BillingEvent.Cancellation.class).ancestor(historyEntry).list())
+    assertThat(ofy().load().type(BillingEvent.Cancellation.class).ancestor(historyEntry2).list())
-        .containsExactly(cancellation);
+        .containsExactly(cancellationOneTime, cancellationRecurring);
-    assertThat(ofy().load().type(BillingEvent.Modification.class).ancestor(historyEntry).list())
+    assertThat(ofy().load().type(BillingEvent.Modification.class).ancestor(historyEntry2).list())
         .containsExactly(modification);
   }
 
@@ -118,10 +155,63 @@ public class BillingEventTest extends EntityTestCase {
     verifyIndexing(oneTime, "clientId", "eventTime", "billingTime");
     verifyIndexing(
         recurring, "clientId", "eventTime", "recurrenceEndTime", "recurrenceTimeOfYear.timeString");
-    verifyIndexing(cancellation, "clientId", "eventTime", "billingTime");
+    verifyIndexing(cancellationOneTime, "clientId", "eventTime", "billingTime");
     verifyIndexing(modification, "clientId", "eventTime");
   }
 
+  @Test
+  public void testSuccess_cancellation_forGracePeriod_withOneTime() {
+    BillingEvent.Cancellation newCancellation = BillingEvent.Cancellation.forGracePeriod(
+        GracePeriod.forBillingEvent(GracePeriodStatus.ADD, oneTime),
+        historyEntry2,
+        "foo.tld");
+    // Set ID to be the same to ignore for the purposes of comparison.
+    newCancellation = newCancellation.asBuilder().setId(cancellationOneTime.getId()).build();
+    assertThat(newCancellation).isEqualTo(cancellationOneTime);
+  }
+
+  @Test
+  public void testSuccess_cancellation_forGracePeriod_withRecurring() {
+    BillingEvent.Cancellation newCancellation = BillingEvent.Cancellation.forGracePeriod(
+        GracePeriod.createForRecurring(
+            GracePeriodStatus.AUTO_RENEW,
+            now.plusYears(1).plusDays(45),
+            "a registrar",
+            Ref.create(recurring)),
+        historyEntry2,
+        "foo.tld");
+    // Set ID to be the same to ignore for the purposes of comparison.
+    newCancellation = newCancellation.asBuilder().setId(cancellationRecurring.getId()).build();
+    assertThat(newCancellation).isEqualTo(cancellationRecurring);
+  }
+
+  @Test
+  public void testFailure_cancellation_forGracePeriodWithoutBillingEvent() {
+    thrown.expect(IllegalArgumentException.class, "grace period without billing event");
+    BillingEvent.Cancellation.forGracePeriod(
+        GracePeriod.createWithoutBillingEvent(
+            GracePeriodStatus.REDEMPTION,
+            now.plusDays(1),
+            "a registrar"),
+        historyEntry,
+        "foo.tld");
+  }
+
+  @Test
+  public void testFailure_cancellationWithNoBillingEvent() {
+    thrown.expect(IllegalStateException.class, "exactly one billing event");
+    cancellationOneTime.asBuilder().setOneTimeEventRef(null).setRecurringEventRef(null).build();
+  }
+
+  @Test
+  public void testFailure_cancellationWithBothBillingEvents() {
+    thrown.expect(IllegalStateException.class, "exactly one billing event");
+    cancellationOneTime.asBuilder()
+        .setOneTimeEventRef(Ref.create(oneTime))
+        .setRecurringEventRef(Ref.create(recurring))
+        .build();
+  }
+
   @Test
   public void testDeadCodeThatDeletedScrapCommandsReference() throws Exception {
     assertThat(recurring.getParentKey()).isEqualTo(Key.create(historyEntry));