Don't extend expiration times for deleted domains (#160)

* Don't extend expiration times for deleted domains

* Flip order and add a comment

* oops forgot a period

* Use END_OF_TIME

* Add tests for expiration times of domains with pending transfers

* Add test for transfer during autorenew and clean up other tests

* Clarify tests

* Add domain expiration check in EppLifecycleDomainTest

* Add a comment and format test files

core/src/main/java/google/registry/model/domain/DomainBase.java

@@ -28,6 +28,7 @@ import static google.registry.util.CollectionUtils.forceEmptyToNull;
 import static google.registry.util.CollectionUtils.nullToEmpty;
 import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy;
 import static google.registry.util.CollectionUtils.union;
+import static google.registry.util.DateTimeUtils.END_OF_TIME;
 import static google.registry.util.DateTimeUtils.earliestOf;
 import static google.registry.util.DateTimeUtils.isBeforeOrAt;
 import static google.registry.util.DateTimeUtils.leapSafeAddYears;
@@ -376,10 +377,10 @@ public class DomainBase extends EppResource
 
     Optional<DateTime> newLastEppUpdateTime = Optional.empty();
 
-    // There is no transfer. Do any necessary autorenews.
+    // There is no transfer. Do any necessary autorenews for active domains.
 
     Builder builder = asBuilder();
-    if (isBeforeOrAt(registrationExpirationTime, now)) {
+    if (isBeforeOrAt(registrationExpirationTime, now) && END_OF_TIME.equals(getDeletionTime())) {
       // Autorenew by the number of years between the old expiration time and now.
       DateTime lastAutorenewTime = leapSafeAddYears(
           registrationExpirationTime,

core/src/test/java/google/registry/flows/EppLifecycleDomainTest.java

@@ -33,6 +33,7 @@ import static org.joda.money.CurrencyUnit.USD;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.Ordering;
+import com.google.common.truth.Truth;
 import com.google.re2j.Matcher;
 import com.google.re2j.Pattern;
 import google.registry.model.billing.BillingEvent;
@@ -197,6 +198,11 @@ public class EppLifecycleDomainTest extends EppTestCase {
         makeRecurringCreateBillingEvent(domain, createTime.plusYears(2), deleteTime));
 
     assertThatLogoutSucceeds();
+
+    // Make sure that in the future, the domain expiration is unchanged after deletion
+    DomainBase clonedDomain = domain.cloneProjectedAtTime(deleteTime.plusYears(5));
+    Truth.assertThat(clonedDomain.getRegistrationExpirationTime())
+        .isEqualTo(createTime.plusYears(2));
   }
 
   @Test

core/src/test/java/google/registry/model/domain/DomainBaseTest.java

@@ -28,6 +28,7 @@ import static google.registry.testing.DomainBaseSubject.assertAboutDomains;
 import static google.registry.testing.JUnitBackports.assertThrows;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 import static org.joda.money.CurrencyUnit.USD;
+import static org.joda.time.DateTimeZone.UTC;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
@@ -61,34 +62,40 @@ import org.junit.Test;
 /** Unit tests for {@link DomainBase}. */
 public class DomainBaseTest extends EntityTestCase {
 
-  DomainBase domain;
+  private DomainBase domain;
+  private Key<BillingEvent.OneTime> oneTimeBillKey;
+  private Key<BillingEvent.Recurring> recurringBillKey;
 
   @Before
   public void setUp() {
     createTld("com");
     Key<DomainBase> domainKey = Key.create(null, DomainBase.class, "4-COM");
-    Key<HostResource> hostKey = Key.create(persistResource(
+    Key<HostResource> hostKey =
+        Key.create(
+            persistResource(
                 new HostResource.Builder()
                     .setFullyQualifiedHostName("ns1.example.com")
                     .setSuperordinateDomain(domainKey)
                     .setRepoId("1-COM")
                     .build()));
-    Key<ContactResource> contact1Key = Key.create(persistResource(
+    Key<ContactResource> contact1Key =
+        Key.create(
+            persistResource(
                 new ContactResource.Builder()
                     .setContactId("contact_id1")
                     .setRepoId("2-COM")
                     .build()));
-    Key<ContactResource> contact2Key = Key.create(persistResource(
+    Key<ContactResource> contact2Key =
+        Key.create(
+            persistResource(
                 new ContactResource.Builder()
                     .setContactId("contact_id2")
                     .setRepoId("3-COM")
                     .build()));
     Key<HistoryEntry> historyEntryKey =
         Key.create(persistResource(new HistoryEntry.Builder().setParent(domainKey).build()));
-    Key<BillingEvent.OneTime> oneTimeBillKey =
+    oneTimeBillKey = Key.create(historyEntryKey, BillingEvent.OneTime.class, 1);
-        Key.create(historyEntryKey, BillingEvent.OneTime.class, 1);
+    recurringBillKey = Key.create(historyEntryKey, BillingEvent.Recurring.class, 2);
-    Key<BillingEvent.Recurring> recurringBillKey =
-        Key.create(historyEntryKey, BillingEvent.Recurring.class, 2);
     Key<PollMessage.Autorenew> autorenewPollKey =
         Key.create(historyEntryKey, PollMessage.Autorenew.class, 3);
     Key<PollMessage.OneTime> onetimePollKey =
@@ -168,17 +175,23 @@ public class DomainBaseTest extends EntityTestCase {
 
   @Test
   public void testEmptyStringsBecomeNull() {
-    assertThat(newDomainBase("example.com").asBuilder()
+    assertThat(
+            newDomainBase("example.com")
+                .asBuilder()
                 .setPersistedCurrentSponsorClientId(null)
                 .build()
                 .getCurrentSponsorClientId())
         .isNull();
-    assertThat(newDomainBase("example.com").asBuilder()
+    assertThat(
+            newDomainBase("example.com")
+                .asBuilder()
                 .setPersistedCurrentSponsorClientId("")
                 .build()
                 .getCurrentSponsorClientId())
         .isNull();
-    assertThat(newDomainBase("example.com").asBuilder()
+    assertThat(
+            newDomainBase("example.com")
+                .asBuilder()
                 .setPersistedCurrentSponsorClientId(" ")
                 .build()
                 .getCurrentSponsorClientId())
@@ -201,29 +214,49 @@ public class DomainBaseTest extends EntityTestCase {
                 .build()
                 .nsHosts)
         .isNull();
-    assertThat(newDomainBase("example.com").asBuilder()
+    assertThat(
+            newDomainBase("example.com")
+                .asBuilder()
                 .setNameservers(ImmutableSet.of(Key.create(newHostResource("foo.example.tld"))))
-            .build().nsHosts)
+                .build()
+                .nsHosts)
         .isNotNull();
     // This behavior should also hold true for ImmutableObjects nested in collections.
-    assertThat(newDomainBase("example.com").asBuilder()
+    assertThat(
+            newDomainBase("example.com")
+                .asBuilder()
                 .setDsData(ImmutableSet.of(DelegationSignerData.create(1, 1, 1, (byte[]) null)))
-        .build().getDsData().asList().get(0).getDigest())
+                .build()
+                .getDsData()
+                .asList()
+                .get(0)
+                .getDigest())
         .isNull();
-    assertThat(newDomainBase("example.com").asBuilder()
+    assertThat(
-        .setDsData(ImmutableSet.of(DelegationSignerData.create(1, 1, 1, new byte[]{})))
+            newDomainBase("example.com")
-        .build().getDsData().asList().get(0).getDigest())
+                .asBuilder()
+                .setDsData(ImmutableSet.of(DelegationSignerData.create(1, 1, 1, new byte[] {})))
+                .build()
+                .getDsData()
+                .asList()
+                .get(0)
+                .getDigest())
         .isNull();
-    assertThat(newDomainBase("example.com").asBuilder()
+    assertThat(
-        .setDsData(ImmutableSet.of(DelegationSignerData.create(1, 1, 1, new byte[]{1})))
+            newDomainBase("example.com")
-        .build().getDsData().asList().get(0).getDigest())
+                .asBuilder()
+                .setDsData(ImmutableSet.of(DelegationSignerData.create(1, 1, 1, new byte[] {1})))
+                .build()
+                .getDsData()
+                .asList()
+                .get(0)
+                .getDigest())
         .isNotNull();
   }
 
   @Test
   public void testEmptyTransferDataBecomesNull() {
-    DomainBase withNull =
+    DomainBase withNull = newDomainBase("example.com").asBuilder().setTransferData(null).build();
-        newDomainBase("example.com").asBuilder().setTransferData(null).build();
     DomainBase withEmpty = withNull.asBuilder().setTransferData(TransferData.EMPTY).build();
     assertThat(withNull).isEqualTo(withEmpty);
     assertThat(withEmpty.transferData).isNull();
@@ -241,7 +274,9 @@ public class DomainBaseTest extends EntityTestCase {
     StatusValue[] statuses1 = {StatusValue.CLIENT_HOLD};
     // If there are other status values, OK should be suppressed. (Domains can't be LINKED.)
     assertAboutDomains()
-        .that(newDomainBase("example.com").asBuilder()
+        .that(
+            newDomainBase("example.com")
+                .asBuilder()
                 .setNameservers(nameservers)
                 .setStatusValues(ImmutableSet.of(StatusValue.CLIENT_HOLD))
                 .build())
@@ -249,7 +284,9 @@ public class DomainBaseTest extends EntityTestCase {
     StatusValue[] statuses2 = {StatusValue.CLIENT_HOLD};
     // When OK is suppressed, it should be removed even if it was originally there.
     assertAboutDomains()
-        .that(newDomainBase("example.com").asBuilder()
+        .that(
+            newDomainBase("example.com")
+                .asBuilder()
                 .setNameservers(nameservers)
                 .setStatusValues(ImmutableSet.of(StatusValue.OK, StatusValue.CLIENT_HOLD))
                 .build())
@@ -262,14 +299,18 @@ public class DomainBaseTest extends EntityTestCase {
     StatusValue[] statuses4 = {StatusValue.CLIENT_HOLD, StatusValue.INACTIVE};
     // If there are no nameservers but there are status values, INACTIVE should still be added.
     assertAboutDomains()
-        .that(newDomainBase("example.com").asBuilder()
+        .that(
+            newDomainBase("example.com")
+                .asBuilder()
                 .setStatusValues(ImmutableSet.of(StatusValue.CLIENT_HOLD))
                 .build())
         .hasExactlyStatusValues(statuses4);
     StatusValue[] statuses5 = {StatusValue.CLIENT_HOLD};
     // If there are nameservers, INACTIVE should be removed even if it was originally there.
     assertAboutDomains()
-        .that(newDomainBase("example.com").asBuilder()
+        .that(
+            newDomainBase("example.com")
+                .asBuilder()
                 .setNameservers(nameservers)
                 .setStatusValues(ImmutableSet.of(StatusValue.INACTIVE, StatusValue.CLIENT_HOLD))
                 .build())
@@ -290,14 +331,18 @@ public class DomainBaseTest extends EntityTestCase {
 
   private void doExpiredTransferTest(DateTime oldExpirationTime) {
     HistoryEntry historyEntry = new HistoryEntry.Builder().setParent(domain).build();
-    BillingEvent.OneTime transferBillingEvent = persistResource(
+    BillingEvent.OneTime transferBillingEvent =
+        persistResource(
             new BillingEvent.OneTime.Builder()
                 .setReason(Reason.TRANSFER)
                 .setClientId("winner")
                 .setTargetId("example.com")
                 .setEventTime(clock.nowUtc())
                 .setBillingTime(
-                clock.nowUtc().plusDays(1).plus(Registry.get("com").getTransferGracePeriodLength()))
+                    clock
+                        .nowUtc()
+                        .plusDays(1)
+                        .plus(Registry.get("com").getTransferGracePeriodLength()))
                 .setCost(Money.of(USD, 11))
                 .setPeriodYears(1)
                 .setParent(historyEntry)
@@ -329,13 +374,15 @@ public class DomainBaseTest extends EntityTestCase {
         domain.getTransferData().getServerApproveAutorenewEvent();
     assertTransferred(afterTransfer, newExpirationTime, serverApproveAutorenewEvent);
     assertThat(afterTransfer.getGracePeriods())
-        .containsExactly(GracePeriod.create(
+        .containsExactly(
+            GracePeriod.create(
                 GracePeriodStatus.TRANSFER,
                 clock.nowUtc().plusDays(1).plus(Registry.get("com").getTransferGracePeriodLength()),
                 "winner",
                 Key.create(transferBillingEvent)));
     // If we project after the grace period expires all should be the same except the grace period.
-    DomainBase afterGracePeriod = domain.cloneProjectedAtTime(
+    DomainBase afterGracePeriod =
+        domain.cloneProjectedAtTime(
             clock.nowUtc().plusDays(2).plus(Registry.get("com").getTransferGracePeriodLength()));
     assertTransferred(afterGracePeriod, newExpirationTime, serverApproveAutorenewEvent);
     assertThat(afterGracePeriod.getGracePeriods()).isEmpty();
@@ -439,7 +486,8 @@ public class DomainBaseTest extends EntityTestCase {
 
   @Test
   public void testStackedGracePeriods() {
-    ImmutableList<GracePeriod> gracePeriods = ImmutableList.of(
+    ImmutableList<GracePeriod> gracePeriods =
+        ImmutableList.of(
             GracePeriod.create(GracePeriodStatus.ADD, clock.nowUtc().plusDays(3), "foo", null),
             GracePeriod.create(GracePeriodStatus.ADD, clock.nowUtc().plusDays(2), "bar", null),
             GracePeriod.create(GracePeriodStatus.ADD, clock.nowUtc().plusDays(1), "baz", null));
@@ -452,10 +500,12 @@ public class DomainBaseTest extends EntityTestCase {
 
   @Test
   public void testGracePeriodsByType() {
-    ImmutableSet<GracePeriod> addGracePeriods = ImmutableSet.of(
+    ImmutableSet<GracePeriod> addGracePeriods =
+        ImmutableSet.of(
             GracePeriod.create(GracePeriodStatus.ADD, clock.nowUtc().plusDays(3), "foo", null),
             GracePeriod.create(GracePeriodStatus.ADD, clock.nowUtc().plusDays(1), "baz", null));
-    ImmutableSet<GracePeriod> renewGracePeriods = ImmutableSet.of(
+    ImmutableSet<GracePeriod> renewGracePeriods =
+        ImmutableSet.of(
             GracePeriod.create(GracePeriodStatus.RENEW, clock.nowUtc().plusDays(3), "foo", null),
             GracePeriod.create(GracePeriodStatus.RENEW, clock.nowUtc().plusDays(1), "baz", null));
     domain =
@@ -472,8 +522,7 @@ public class DomainBaseTest extends EntityTestCase {
 
   @Test
   public void testRenewalsHappenAtExpiration() {
-    DomainBase renewed =
+    DomainBase renewed = domain.cloneProjectedAtTime(domain.getRegistrationExpirationTime());
-        domain.cloneProjectedAtTime(domain.getRegistrationExpirationTime());
     assertThat(renewed.getRegistrationExpirationTime())
         .isEqualTo(domain.getRegistrationExpirationTime().plusYears(1));
     assertThat(renewed.getLastEppUpdateTime()).isEqualTo(domain.getRegistrationExpirationTime());
@@ -490,7 +539,9 @@ public class DomainBaseTest extends EntityTestCase {
 
   @Test
   public void testRenewalsDontHappenOnFebruary29() {
-    domain = domain.asBuilder()
+    domain =
+        domain
+            .asBuilder()
             .setRegistrationExpirationTime(DateTime.parse("2004-02-29T22:00:00.0Z"))
             .build();
     DomainBase renewed =
@@ -517,16 +568,17 @@ public class DomainBaseTest extends EntityTestCase {
                     .put(oldExpirationTime.plusYears(2).plusMillis(1), Money.of(USD, 5))
                     .build())
             .build());
-    DomainBase renewedThreeTimes =
+    DomainBase renewedThreeTimes = domain.cloneProjectedAtTime(oldExpirationTime.plusYears(2));
-        domain.cloneProjectedAtTime(oldExpirationTime.plusYears(2));
     assertThat(renewedThreeTimes.getRegistrationExpirationTime())
         .isEqualTo(oldExpirationTime.plusYears(3));
     assertThat(renewedThreeTimes.getLastEppUpdateTime()).isEqualTo(oldExpirationTime.plusYears(2));
     assertThat(renewedThreeTimes.getGracePeriods())
-        .containsExactly(GracePeriod.createForRecurring(
+        .containsExactly(
+            GracePeriod.createForRecurring(
                 GracePeriodStatus.AUTO_RENEW,
-            oldExpirationTime.plusYears(2).plus(
+                oldExpirationTime
-                Registry.get("com").getAutoRenewGracePeriodLength()),
+                    .plusYears(2)
+                    .plus(Registry.get("com").getAutoRenewGracePeriodLength()),
                 renewedThreeTimes.getCurrentSponsorClientId(),
                 renewedThreeTimes.autorenewBillingEvent));
   }
@@ -557,4 +609,154 @@ public class DomainBaseTest extends EntityTestCase {
         .hasMessageThat()
         .contains("Domain name must be in puny-coded, lower-case form");
   }
+
+  @Test
+  public void testClone_doNotExtendExpirationOnDeletedDomain() {
+    DateTime now = DateTime.now(UTC);
+    domain =
+        persistResource(
+            domain
+                .asBuilder()
+                .setRegistrationExpirationTime(now.minusDays(1))
+                .setDeletionTime(now.minusDays(10))
+                .setStatusValues(ImmutableSet.of(StatusValue.PENDING_DELETE, StatusValue.INACTIVE))
+                .build());
+    assertThat(domain.cloneProjectedAtTime(now).getRegistrationExpirationTime())
+        .isEqualTo(now.minusDays(1));
+  }
+
+  @Test
+  public void testClone_doNotExtendExpirationOnFutureDeletedDomain() {
+    // if a domain is in pending deletion (StatusValue.PENDING_DELETE), don't extend expiration
+    DateTime now = DateTime.now(UTC);
+    domain =
+        persistResource(
+            domain
+                .asBuilder()
+                .setRegistrationExpirationTime(now.plusDays(1))
+                .setDeletionTime(now.plusDays(20))
+                .setStatusValues(ImmutableSet.of(StatusValue.PENDING_DELETE, StatusValue.INACTIVE))
+                .build());
+    assertThat(domain.cloneProjectedAtTime(now).getRegistrationExpirationTime())
+        .isEqualTo(now.plusDays(1));
+  }
+
+  @Test
+  public void testClone_extendsExpirationForExpiredTransferredDomain() {
+    // If the transfer implicitly succeeded, the expiration time should be extended
+    DateTime now = DateTime.now(UTC);
+    DateTime transferExpirationTime = now.minusDays(1);
+    DateTime previousExpiration = now.minusDays(2);
+
+    TransferData transferData =
+        new TransferData.Builder()
+            .setPendingTransferExpirationTime(transferExpirationTime)
+            .setTransferStatus(TransferStatus.PENDING)
+            .setGainingClientId("TheRegistrar")
+            .build();
+    Period extensionPeriod = transferData.getTransferPeriod();
+    DateTime newExpiration = previousExpiration.plusYears(extensionPeriod.getValue());
+    domain =
+        persistResource(
+            domain
+                .asBuilder()
+                .setRegistrationExpirationTime(previousExpiration)
+                .setTransferData(transferData)
+                .build());
+
+    assertThat(domain.cloneProjectedAtTime(now).getRegistrationExpirationTime())
+        .isEqualTo(newExpiration);
+  }
+
+  @Test
+  public void testClone_extendsExpirationForNonExpiredTransferredDomain() {
+    // If the transfer implicitly succeeded, the expiration time should be extended even if it
+    // hadn't already expired
+    DateTime now = DateTime.now(UTC);
+    DateTime transferExpirationTime = now.minusDays(1);
+    DateTime previousExpiration = now.plusWeeks(2);
+
+    TransferData transferData =
+        new TransferData.Builder()
+            .setPendingTransferExpirationTime(transferExpirationTime)
+            .setTransferStatus(TransferStatus.PENDING)
+            .setGainingClientId("TheRegistrar")
+            .build();
+    Period extensionPeriod = transferData.getTransferPeriod();
+    DateTime newExpiration = previousExpiration.plusYears(extensionPeriod.getValue());
+    domain =
+        persistResource(
+            domain
+                .asBuilder()
+                .setRegistrationExpirationTime(previousExpiration)
+                .setTransferData(transferData)
+                .build());
+
+    assertThat(domain.cloneProjectedAtTime(now).getRegistrationExpirationTime())
+        .isEqualTo(newExpiration);
+  }
+
+  @Test
+  public void testClone_doesNotExtendExpirationForPendingTransfer() {
+    // Pending transfers shouldn't affect the expiration time
+    DateTime now = DateTime.now(UTC);
+    DateTime transferExpirationTime = now.plusDays(1);
+    DateTime previousExpiration = now.plusWeeks(2);
+
+    TransferData transferData =
+        new TransferData.Builder()
+            .setPendingTransferExpirationTime(transferExpirationTime)
+            .setTransferStatus(TransferStatus.PENDING)
+            .setGainingClientId("TheRegistrar")
+            .build();
+    domain =
+        persistResource(
+            domain
+                .asBuilder()
+                .setRegistrationExpirationTime(previousExpiration)
+                .setTransferData(transferData)
+                .build());
+
+    assertThat(domain.cloneProjectedAtTime(now).getRegistrationExpirationTime())
+        .isEqualTo(previousExpiration);
+  }
+
+  @Test
+  public void testClone_transferDuringAutorenew() {
+    // When the domain is an an autorenew grace period, we should not extend the registration
+    // expiration by a further year--it should just be whatever the autorenew was
+    DateTime now = DateTime.now(UTC);
+    DateTime transferExpirationTime = now.minusDays(1);
+    DateTime previousExpiration = now.minusDays(2);
+
+    TransferData transferData =
+        new TransferData.Builder()
+            .setPendingTransferExpirationTime(transferExpirationTime)
+            .setTransferStatus(TransferStatus.PENDING)
+            .setGainingClientId("TheRegistrar")
+            .setServerApproveAutorenewEvent(recurringBillKey)
+            .setServerApproveBillingEvent(oneTimeBillKey)
+            .build();
+    domain =
+        persistResource(
+            domain
+                .asBuilder()
+                .setRegistrationExpirationTime(previousExpiration)
+                .setGracePeriods(
+                    ImmutableSet.of(
+                        GracePeriod.createForRecurring(
+                            GracePeriodStatus.AUTO_RENEW,
+                            now.plusDays(1),
+                            "NewRegistrar",
+                            recurringBillKey)))
+                .setTransferData(transferData)
+                .setAutorenewBillingEvent(recurringBillKey)
+                .build());
+    DomainBase clone = domain.cloneProjectedAtTime(now);
+    assertThat(clone.getRegistrationExpirationTime())
+        .isEqualTo(domain.getRegistrationExpirationTime().plusYears(1));
+    // Transferring removes the AUTORENEW grace period and adds a TRANSFER grace period
+    assertThat(getOnlyElement(clone.getGracePeriods()).getType())
+        .isEqualTo(GracePeriodStatus.TRANSFER);
+  }
 }