Don't expect a renewal fee on restores when one isn't due (#637)

* Don't expect a renewal fee on restores when one isn't due

This is a fix on top of #632 so that domain restore commands don't require
acking an illusory renewal fee for 1 year when that isn't actually happening
(i.e. if the domain isn't yet past its original expiration).

Unfortunately, there's still a problem remaining wherein the restore fee on a
domain check will always include the additional year even if it's not
necessary. We don't have a good solution to that. Also note that in versions of
the fee extension more recent than 0.6, the fee extension cannot be passed on a
domain info command at all, so the domain check command is the only way you have
to determine what the restore fee should be. So we definitely do want to get
that right so that the apparent restore fee on a check is the same as the actual
restore fee when running the restore command. We're not quite there yet though
and it's hard to say how we will get there, since we don't load domains during a
domain check command for performance reasons yet we would need to do so in order
to know the expiration date and thus whether the additional year of renewal
should be charged.

A problem for a future PR.

core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java

@@ -247,6 +247,7 @@ public final class DomainCheckFlow implements Flow {
             feeCheckItem,
             builder,
             domainNames.get(domainName),
+            Optional.empty(),
             feeCheck.getCurrency(),
             now,
             pricingLogic,

core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java

@@ -551,7 +551,8 @@ public class DomainFlowUtils {
   static void handleFeeRequest(
       FeeQueryCommandExtensionItem feeRequest,
       FeeQueryResponseExtensionItem.Builder<?, ?> builder,
-      InternetDomainName domain,
+      InternetDomainName domainName,
+      Optional<DomainBase> domain,
       @Nullable CurrencyUnit topLevelCurrency,
       DateTime currentDate,
       DomainPricingLogic pricingLogic,
@@ -564,8 +565,8 @@ public class DomainFlowUtils {
       now = feeRequest.getEffectiveDate().get();
       builder.setEffectiveDateIfSupported(now);
     }
-    String domainNameString = domain.toString();
+    String domainNameString = domainName.toString();
-    Registry registry = Registry.get(domain.parent().toString());
+    Registry registry = Registry.get(domainName.parent().toString());
     int years = verifyUnitIsYears(feeRequest.getPeriod()).getValue();
     boolean isSunrise = (registry.getTldState(now) == START_DATE_SUNRISE);
 
@@ -589,7 +590,7 @@ public class DomainFlowUtils {
     switch (feeRequest.getCommandName()) {
       case CREATE:
         // Don't return a create price for reserved names.
-        if (isReserved(domain, isSunrise) && !isAvailable) {
+        if (isReserved(domainName, isSunrise) && !isAvailable) {
           builder.setClass("reserved"); // Override whatever class we've set above.
           builder.setAvailIfSupported(false);
           builder.setReasonIfSupported("reserved");
@@ -606,11 +607,25 @@ public class DomainFlowUtils {
         fees = pricingLogic.getRenewPrice(registry, domainNameString, now, years).getFees();
         break;
       case RESTORE:
+        // The minimum allowable period per the EPP spec is 1, so, strangely, 1 year still has to be
+        // passed in as the period for a restore even if the domain would *not* be renewed as part
+        // of a restore. This is fixed in RFC 8748 (which is a more recent version of the fee
+        // extension than we currently support), which does not allow the period to be passed at all
+        // on a restore fee check.
         if (years != 1) {
           throw new RestoresAreAlwaysForOneYearException();
         }
         builder.setAvailIfSupported(true);
-        fees = pricingLogic.getRestorePrice(registry, domainNameString, now).getFees();
+        // The domain object is present only on domain info commands, not on domain check commands,
+        // because check commands can query up to 50 domains and it isn't performant to load them
+        // all. So, only on info commands can we actually determine if we should include the renewal
+        // fee because the domain needs to have been loaded in order to know its expiration time. We
+        // default to including the renewal fee on domain checks because typically most domains are
+        // deleted during the autorenew grace period and thus if restored will require a renewal,
+        // but this is just a best guess.
+        boolean isExpired =
+            !domain.isPresent() || domain.get().getRegistrationExpirationTime().isBefore(now);
+        fees = pricingLogic.getRestorePrice(registry, domainNameString, now, isExpired).getFees();
         break;
       case TRANSFER:
         if (years != 1) {

core/src/main/java/google/registry/flows/domain/DomainInfoFlow.java

@@ -158,6 +158,7 @@ public final class DomainInfoFlow implements Flow {
           feeInfo.get(),
           builder,
           InternetDomainName.from(targetId),
+          Optional.of(domain),
           null,
           now,
           pricingLogic,

core/src/main/java/google/registry/flows/domain/DomainPricingLogic.java

@@ -124,23 +124,23 @@ public final class DomainPricingLogic {
   }
 
   /** Returns a new restore price for the pricer. */
-  public FeesAndCredits getRestorePrice(Registry registry, String domainName, DateTime dateTime)
+  public FeesAndCredits getRestorePrice(
+      Registry registry, String domainName, DateTime dateTime, boolean isExpired)
       throws EppException {
     DomainPrices domainPrices = getPricesForDomainName(domainName, dateTime);
-    FeesAndCredits feesAndCredits =
+    FeesAndCredits.Builder feesAndCredits =
         new FeesAndCredits.Builder()
             .setCurrency(registry.getCurrency())
             .addFeeOrCredit(
-                Fee.create(
+                Fee.create(registry.getStandardRestoreCost().getAmount(), FeeType.RESTORE, false));
-                    domainPrices.getRenewCost().getAmount(),
+    if (isExpired) {
-                    FeeType.RENEW,
+      feesAndCredits.addFeeOrCredit(
-                    domainPrices.isPremium()))
+          Fee.create(
-            .addFeeOrCredit(
+              domainPrices.getRenewCost().getAmount(), FeeType.RENEW, domainPrices.isPremium()));
-                Fee.create(registry.getStandardRestoreCost().getAmount(), FeeType.RESTORE, false))
+    }
-            .build();
     return customLogic.customizeRestorePrice(
         RestorePriceParameters.newBuilder()
-            .setFeesAndCredits(feesAndCredits)
+            .setFeesAndCredits(feesAndCredits.build())
             .setRegistry(registry)
             .setDomainName(InternetDomainName.from(domainName))
             .setAsOfDate(dateTime)

core/src/main/java/google/registry/flows/domain/DomainRestoreRequestFlow.java

@@ -136,20 +136,22 @@ public final class DomainRestoreRequestFlow implements TransactionalFlow  {
     Update command = (Update) resourceCommand;
     DateTime now = tm().getTransactionTime();
     DomainBase existingDomain = loadAndVerifyExistence(DomainBase.class, targetId, now);
+    boolean isExpired = existingDomain.getRegistrationExpirationTime().isBefore(now);
     FeesAndCredits feesAndCredits =
-        pricingLogic.getRestorePrice(Registry.get(existingDomain.getTld()), targetId, now);
+        pricingLogic.getRestorePrice(
+            Registry.get(existingDomain.getTld()), targetId, now, isExpired);
     Optional<FeeUpdateCommandExtension> feeUpdate =
         eppInput.getSingleExtension(FeeUpdateCommandExtension.class);
     verifyRestoreAllowed(command, existingDomain, feeUpdate, feesAndCredits, now);
     HistoryEntry historyEntry = buildHistoryEntry(existingDomain, now);
     ImmutableSet.Builder<ImmutableObject> entitiesToSave = new ImmutableSet.Builder<>();
 
+    DateTime newExpirationTime =
+        existingDomain.getRegistrationExpirationTime().plusYears(isExpired ? 1 : 0);
     // Restore the expiration time on the deleted domain, except if that's already passed, then add
     // a year and bill for it immediately, with no grace period.
-    DateTime newExpirationTime = existingDomain.getRegistrationExpirationTime();
+    if (isExpired) {
-    if (newExpirationTime.isBefore(now)) {
       entitiesToSave.add(createRenewBillingEvent(historyEntry, feesAndCredits.getRenewCost(), now));
-      newExpirationTime = newExpirationTime.plusYears(1);
     }
     // Always bill for the restore itself.
     entitiesToSave.add(
@@ -176,7 +178,7 @@ public final class DomainRestoreRequestFlow implements TransactionalFlow  {
     ofy().delete().key(existingDomain.getDeletePollMessage());
     dnsQueue.addDomainRefreshTask(existingDomain.getDomainName());
     return responseBuilder
-        .setExtensions(createResponseExtensions(feesAndCredits, feeUpdate))
+        .setExtensions(createResponseExtensions(feesAndCredits, feeUpdate, isExpired))
         .build();
   }
 
@@ -263,23 +265,29 @@ public final class DomainRestoreRequestFlow implements TransactionalFlow  {
   }
 
   private static ImmutableList<FeeTransformResponseExtension> createResponseExtensions(
-      FeesAndCredits feesAndCredits, Optional<FeeUpdateCommandExtension> feeUpdate) {
+      FeesAndCredits feesAndCredits,
+      Optional<FeeUpdateCommandExtension> feeUpdate,
+      boolean isExpired) {
+    ImmutableList.Builder<Fee> fees = new ImmutableList.Builder<>();
+    fees.add(
+        Fee.create(
+            feesAndCredits.getRestoreCost().getAmount(),
+            FeeType.RESTORE,
+            feesAndCredits.hasPremiumFeesOfType(FeeType.RESTORE)));
+    if (isExpired) {
+      fees.add(
+          Fee.create(
+              feesAndCredits.getRenewCost().getAmount(),
+              FeeType.RENEW,
+              feesAndCredits.hasPremiumFeesOfType(FeeType.RENEW)));
+    }
     return feeUpdate.isPresent()
         ? ImmutableList.of(
             feeUpdate
                 .get()
                 .createResponseBuilder()
                 .setCurrency(feesAndCredits.getCurrency())
-                .setFees(
+                .setFees(fees.build())
-                    ImmutableList.of(
-                        Fee.create(
-                            feesAndCredits.getRestoreCost().getAmount(),
-                            FeeType.RESTORE,
-                            feesAndCredits.hasPremiumFeesOfType(FeeType.RESTORE)),
-                        Fee.create(
-                            feesAndCredits.getRenewCost().getAmount(),
-                            FeeType.RENEW,
-                            feesAndCredits.hasPremiumFeesOfType(FeeType.RENEW))))
                 .build())
         : ImmutableList.of();
   }

core/src/test/java/google/registry/flows/domain/DomainInfoFlowTest.java

@@ -659,6 +659,39 @@ public class DomainInfoFlowTest extends ResourceFlowTestCase<DomainInfoFlow, Dom
     doSuccessfulTest("domain_info_fee_restore_response.xml", false);
   }
 
+  @Test
+  public void testFeeExtension_restoreCommand_pendingDelete_noRenewal() throws Exception {
+    setEppInput(
+        "domain_info_fee.xml",
+        updateSubstitutions(SUBSTITUTION_BASE, "COMMAND", "restore", "PERIOD", "1"));
+    persistTestEntities(false);
+    persistResource(
+        domain
+            .asBuilder()
+            .setDeletionTime(clock.nowUtc().plusDays(25))
+            .setStatusValues(ImmutableSet.of(StatusValue.PENDING_DELETE))
+            .build());
+    doSuccessfulTest("domain_info_fee_restore_response_no_renewal.xml", false);
+  }
+
+  @Test
+  public void testFeeExtension_restoreCommand_pendingDelete_withRenewal() throws Exception {
+    createTld("example");
+    setEppInput(
+        "domain_info_fee.xml",
+        updateSubstitutions(
+            SUBSTITUTION_BASE, "NAME", "rich.example", "COMMAND", "restore", "PERIOD", "1"));
+    persistTestEntities("rich.example", false);
+    persistResource(
+        domain
+            .asBuilder()
+            .setDeletionTime(clock.nowUtc().plusDays(25))
+            .setRegistrationExpirationTime(clock.nowUtc().minusDays(1))
+            .setStatusValues(ImmutableSet.of(StatusValue.PENDING_DELETE))
+            .build());
+    doSuccessfulTest("domain_info_fee_restore_response_with_renewal.xml", false);
+  }
+
   /** Test create command on a premium label. */
   @Test
   public void testFeeExtension_createCommandPremium() throws Exception {

core/src/test/java/google/registry/flows/domain/DomainRestoreRequestFlowTest.java

@@ -94,7 +94,8 @@ public class DomainRestoreRequestFlowTest
   }
 
   void persistPendingDeleteDomain() throws Exception {
-    persistPendingDeleteDomain(clock.nowUtc().plusYears(5).plusDays(45));
+    // The domain is now past what had been its expiration date at the time of deletion.
+    persistPendingDeleteDomain(clock.nowUtc().minusDays(5));
   }
 
   void persistPendingDeleteDomain(DateTime expirationTime) throws Exception {
@@ -285,6 +286,14 @@ public class DomainRestoreRequestFlowTest
     runFlowAssertResponse(loadFile("domain_update_restore_request_response_fee.xml", FEE_06_MAP));
   }
 
+  @Test
+  public void testSuccess_fee_v06_noRenewal() throws Exception {
+    setEppInput("domain_update_restore_request_fee_no_renewal.xml", FEE_06_MAP);
+    persistPendingDeleteDomain(clock.nowUtc().plusMonths(6));
+    runFlowAssertResponse(
+        loadFile("domain_update_restore_request_response_fee_no_renewal.xml", FEE_06_MAP));
+  }
+
   @Test
   public void testSuccess_fee_v11() throws Exception {
     setEppInput("domain_update_restore_request_fee.xml", FEE_11_MAP);
@@ -410,6 +419,15 @@ public class DomainRestoreRequestFlowTest
     runFlowAssertResponse(loadFile("domain_update_restore_request_response_premium.xml"));
   }
 
+  @Test
+  public void testSuccess_premiumNotBlocked_andNoRenewal() throws Exception {
+    createTld("example");
+    setEppInput("domain_update_restore_request_premium_no_renewal.xml");
+    persistPendingDeleteDomain(clock.nowUtc().plusYears(2));
+    runFlowAssertResponse(
+        loadFile("domain_update_restore_request_response_fee_no_renewal.xml", FEE_12_MAP));
+  }
+
   @Test
   public void testSuccess_superuserOverridesReservedList() throws Exception {
     persistResource(

core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_premium_response.xml

@@ -35,7 +35,6 @@
         <fee:currency>USD</fee:currency>
         <fee:command>restore</fee:command>
         <fee:period unit="y">1</fee:period>
-        <fee:fee description="renew">100.00</fee:fee>
         <fee:fee description="restore">17.00</fee:fee>
         <fee:class>premium</fee:class>
       </fee:infData>

core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response.xml

@@ -35,7 +35,6 @@
         <fee:currency>USD</fee:currency>
         <fee:command>restore</fee:command>
         <fee:period unit="y">1</fee:period>
-        <fee:fee description="renew">11.00</fee:fee>
         <fee:fee description="restore">17.00</fee:fee>
       </fee:infData>
     </extension>

core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response_no_renewal.xml

@@ -0,0 +1,49 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <response>
+    <result code="1000">
+      <msg>Command completed successfully</msg>
+    </result>
+    <resData>
+      <domain:infData
+       xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>example.tld</domain:name>
+        <domain:roid>%ROID%</domain:roid>
+        <domain:status s="pendingDelete"/>
+        <domain:registrant>jd1234</domain:registrant>
+        <domain:contact type="admin">sh8013</domain:contact>
+        <domain:contact type="tech">sh8013</domain:contact>
+        <domain:ns>
+          <domain:hostObj>ns1.example.tld</domain:hostObj>
+          <domain:hostObj>ns1.example.net</domain:hostObj>
+        </domain:ns>
+        <domain:host>ns1.example.tld</domain:host>
+        <domain:host>ns2.example.tld</domain:host>
+        <domain:clID>NewRegistrar</domain:clID>
+        <domain:crID>TheRegistrar</domain:crID>
+        <domain:crDate>1999-04-03T22:00:00.0Z</domain:crDate>
+        <domain:upID>NewRegistrar</domain:upID>
+        <domain:upDate>1999-12-03T09:00:00.0Z</domain:upDate>
+        <domain:exDate>2005-04-03T22:00:00.0Z</domain:exDate>
+        <domain:trDate>2000-04-08T09:00:00.0Z</domain:trDate>
+        <domain:authInfo>
+          <domain:pw>2fooBAR</domain:pw>
+        </domain:authInfo>
+      </domain:infData>
+    </resData>
+    <extension>
+      <rgp:infData xmlns:rgp="urn:ietf:params:xml:ns:rgp-1.0">
+        <rgp:rgpStatus s="pendingDelete"/>
+      </rgp:infData>
+      <fee:infData xmlns:fee="urn:ietf:params:xml:ns:fee-0.6">
+        <fee:currency>USD</fee:currency>
+        <fee:command>restore</fee:command>
+        <fee:period unit="y">1</fee:period>
+        <fee:fee description="restore">17.00</fee:fee>
+      </fee:infData>
+    </extension>
+    <trID>
+      <clTRID>ABC-12345</clTRID>
+      <svTRID>server-trid</svTRID>
+    </trID>
+  </response>
+</epp>

core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response_with_renewal.xml

@@ -0,0 +1,51 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <response>
+    <result code="1000">
+      <msg>Command completed successfully</msg>
+    </result>
+    <resData>
+      <domain:infData
+       xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>rich.example</domain:name>
+        <domain:roid>%ROID%</domain:roid>
+        <domain:status s="pendingDelete"/>
+        <domain:registrant>jd1234</domain:registrant>
+        <domain:contact type="admin">sh8013</domain:contact>
+        <domain:contact type="tech">sh8013</domain:contact>
+        <domain:ns>
+          <domain:hostObj>ns1.example.tld</domain:hostObj>
+          <domain:hostObj>ns1.example.net</domain:hostObj>
+        </domain:ns>
+        <domain:host>ns1.example.tld</domain:host>
+        <domain:host>ns2.example.tld</domain:host>
+        <domain:clID>NewRegistrar</domain:clID>
+        <domain:crID>TheRegistrar</domain:crID>
+        <domain:crDate>1999-04-03T22:00:00.0Z</domain:crDate>
+        <domain:upID>NewRegistrar</domain:upID>
+        <domain:upDate>1999-12-03T09:00:00.0Z</domain:upDate>
+        <domain:exDate>2005-03-02T22:00:00.000Z</domain:exDate>
+        <domain:trDate>2000-04-08T09:00:00.0Z</domain:trDate>
+        <domain:authInfo>
+          <domain:pw>2fooBAR</domain:pw>
+        </domain:authInfo>
+      </domain:infData>
+    </resData>
+    <extension>
+      <rgp:infData xmlns:rgp="urn:ietf:params:xml:ns:rgp-1.0">
+        <rgp:rgpStatus s="pendingDelete"/>
+      </rgp:infData>
+      <fee:infData xmlns:fee="urn:ietf:params:xml:ns:fee-0.6">
+        <fee:currency>USD</fee:currency>
+        <fee:command>restore</fee:command>
+        <fee:period unit="y">1</fee:period>
+        <fee:fee description="restore">17.00</fee:fee>
+        <fee:fee description="renew">100.00</fee:fee>
+        <fee:class>premium</fee:class>
+      </fee:infData>
+    </extension>
+    <trID>
+      <clTRID>ABC-12345</clTRID>
+      <svTRID>server-trid</svTRID>
+    </trID>
+  </response>
+</epp>

core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_fee_no_renewal.xml

@@ -0,0 +1,21 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <update>
+      <domain:update
+       xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>example.tld</domain:name>
+        <domain:chg/>
+      </domain:update>
+    </update>
+    <extension>
+      <rgp:update xmlns:rgp="urn:ietf:params:xml:ns:rgp-1.0">
+        <rgp:restore op="request"/>
+      </rgp:update>
+      <fee:update xmlns:fee="urn:ietf:params:xml:ns:fee-%FEE_VERSION%">
+        <fee:currency>%CURRENCY%</fee:currency>
+        <fee:fee description="restore">17.00</fee:fee>
+      </fee:update>
+    </extension>
+    <clTRID>ABC-12345</clTRID>
+  </command>
+</epp>

core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_premium_no_renewal.xml

@@ -0,0 +1,23 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <update>
+      <domain:update
+          xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>rich.example</domain:name>
+        <domain:chg/>
+      </domain:update>
+    </update>
+    <extension>
+      <rgp:update xmlns:rgp="urn:ietf:params:xml:ns:rgp-1.0">
+        <rgp:restore op="request"/>
+      </rgp:update>
+      <fee:update xmlns:fee="urn:ietf:params:xml:ns:fee-0.12">
+        <fee:currency>USD</fee:currency>
+        <fee:fee description="restore" refundable="true" grace-period="P0D" applied="immediate">
+          17.00
+        </fee:fee>
+      </fee:update>
+    </extension>
+    <clTRID>ABC-12345</clTRID>
+  </command>
+</epp>

core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_response_fee_no_renewal.xml

@@ -0,0 +1,17 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <response>
+    <result code="1000">
+      <msg>Command completed successfully</msg>
+    </result>
+    <extension>
+      <%FEE_NS%:updData xmlns:%FEE_NS%="urn:ietf:params:xml:ns:fee-%FEE_VERSION%">
+        <%FEE_NS%:currency>USD</%FEE_NS%:currency>
+        <%FEE_NS%:fee description="restore">17.00</%FEE_NS%:fee>
+      </%FEE_NS%:updData>
+    </extension>
+    <trID>
+      <clTRID>ABC-12345</clTRID>
+      <svTRID>server-trid</svTRID>
+    </trID>
+  </response>
+</epp>