From 9b91f882ac0d95978a248c3d73e79c5bb77aaa67 Mon Sep 17 00:00:00 2001 From: sarahcaseybot Date: Wed, 23 Nov 2022 15:48:59 -0500 Subject: [PATCH] Remove names from packages on automatic transfers (#1827) * Remove names from packages on automatic transfers * Add more tests * Remove unneccesary local variable * Eliminate unnecessary api call * Reformat if blocks * Don't overwrite existingRecurring --- .../domain/DomainTransferRequestFlow.java | 20 ++- .../flows/domain/DomainTransferUtils.java | 9 +- .../registry/model/domain/DomainBase.java | 3 +- .../domain/DomainTransferRequestFlowTest.java | 149 ++++++++++++++++++ .../registry/model/domain/DomainTest.java | 80 +++++++++- 5 files changed, 251 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/google/registry/flows/domain/DomainTransferRequestFlow.java b/core/src/main/java/google/registry/flows/domain/DomainTransferRequestFlow.java index b9d0630c4..be8b19b87 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainTransferRequestFlow.java +++ b/core/src/main/java/google/registry/flows/domain/DomainTransferRequestFlow.java @@ -194,11 +194,21 @@ public final class DomainTransferRequestFlow implements TransactionalFlow { } // If the period is zero, then there is no fee for the transfer. Recurring existingRecurring = tm().loadByKey(existingDomain.getAutorenewBillingEvent()); - Optional feesAndCredits = - period.getValue() == 0 - ? Optional.empty() - : Optional.of( - pricingLogic.getTransferPrice(registry, targetId, now, existingRecurring)); + Optional feesAndCredits; + if (period.getValue() == 0) { + feesAndCredits = Optional.empty(); + } else if (!existingDomain.getCurrentPackageToken().isPresent()) { + feesAndCredits = + Optional.of(pricingLogic.getTransferPrice(registry, targetId, now, existingRecurring)); + } else { + // If existing domain is in a package, calculate the transfer price with default renewal price + // behavior + feesAndCredits = + period.getValue() == 0 + ? Optional.empty() + : Optional.of(pricingLogic.getTransferPrice(registry, targetId, now, null)); + } + if (feesAndCredits.isPresent()) { validateFeeChallenge(feeTransfer, feesAndCredits.get()); } diff --git a/core/src/main/java/google/registry/flows/domain/DomainTransferUtils.java b/core/src/main/java/google/registry/flows/domain/DomainTransferUtils.java index fa1b8dd53..c1c69b50c 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainTransferUtils.java +++ b/core/src/main/java/google/registry/flows/domain/DomainTransferUtils.java @@ -24,6 +24,7 @@ import google.registry.model.billing.BillingEvent; import google.registry.model.billing.BillingEvent.Flag; import google.registry.model.billing.BillingEvent.Reason; import google.registry.model.billing.BillingEvent.Recurring; +import google.registry.model.billing.BillingEvent.RenewalPriceBehavior; import google.registry.model.domain.Domain; import google.registry.model.domain.GracePeriod; import google.registry.model.domain.Period; @@ -144,7 +145,13 @@ public final class DomainTransferUtils { return builder .add( createGainingClientAutorenewEvent( - existingRecurring, + existingDomain.getCurrentPackageToken().isPresent() + ? existingRecurring + .asBuilder() + .setRenewalPriceBehavior(RenewalPriceBehavior.DEFAULT) + .setRenewalPrice(null) + .build() + : existingRecurring, serverApproveNewExpirationTime, domainHistoryId, targetId, diff --git a/core/src/main/java/google/registry/model/domain/DomainBase.java b/core/src/main/java/google/registry/model/domain/DomainBase.java index 8aa995bd1..ef2ba1a41 100644 --- a/core/src/main/java/google/registry/model/domain/DomainBase.java +++ b/core/src/main/java/google/registry/model/domain/DomainBase.java @@ -481,7 +481,8 @@ public class DomainBase extends EppResource // Set the speculatively-written new autorenew events as the domain's autorenew // events. .setAutorenewBillingEvent(transferData.getServerApproveAutorenewEvent()) - .setAutorenewPollMessage(transferData.getServerApproveAutorenewPollMessage()); + .setAutorenewPollMessage(transferData.getServerApproveAutorenewPollMessage()) + .setCurrentPackageToken(null); if (transferData.getTransferPeriod().getValue() == 1) { // Set the grace period using a key to the pre-scheduled transfer billing event. Not using // GracePeriod.forBillingEvent() here in order to avoid the actual Datastore fetch. diff --git a/core/src/test/java/google/registry/flows/domain/DomainTransferRequestFlowTest.java b/core/src/test/java/google/registry/flows/domain/DomainTransferRequestFlowTest.java index a6ff7362f..524d486fe 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainTransferRequestFlowTest.java +++ b/core/src/test/java/google/registry/flows/domain/DomainTransferRequestFlowTest.java @@ -21,6 +21,7 @@ import static com.google.common.truth.Truth8.assertThat; import static google.registry.batch.AsyncTaskEnqueuer.PARAM_REQUESTED_TIME; import static google.registry.batch.AsyncTaskEnqueuer.PARAM_RESOURCE_KEY; import static google.registry.batch.AsyncTaskEnqueuer.QUEUE_ASYNC_ACTIONS; +import static google.registry.model.domain.token.AllocationToken.TokenType.PACKAGE; import static google.registry.model.domain.token.AllocationToken.TokenType.SINGLE_USE; import static google.registry.model.domain.token.AllocationToken.TokenType.UNLIMITED_USE; import static google.registry.model.reporting.DomainTransactionRecord.TransactionReportField.TRANSFER_SUCCESSFUL; @@ -1321,6 +1322,154 @@ class DomainTransferRequestFlowTest .build()); } + @Test + void testSuccess_specifiedRenewalPrice_notCarriedOverForPackageName() throws Exception { + setupDomain("example", "tld"); + persistResource(Registry.get("tld").asBuilder().build()); + domain = loadByEntity(domain); + persistResource( + loadByKey(domain.getAutorenewBillingEvent()) + .asBuilder() + .setRenewalPriceBehavior(RenewalPriceBehavior.SPECIFIED) + .setRenewalPrice(Money.of(USD, new BigDecimal("18.79"))) + .build()); + AllocationToken allocationToken = + persistResource( + new AllocationToken.Builder() + .setToken("abc123") + .setTokenType(PACKAGE) + .setRenewalPriceBehavior(RenewalPriceBehavior.SPECIFIED) + .setAllowedRegistrarIds(ImmutableSet.of("TheRegistrar")) + .build()); + domain = + persistResource( + domain.asBuilder().setCurrentPackageToken(allocationToken.createVKey()).build()); + DateTime now = clock.nowUtc(); + + setEppInput("domain_transfer_request.xml"); + eppLoader.replaceAll("JD1234-REP", contact.getRepoId()); + runFlowAssertResponse(loadFile("domain_transfer_request_response.xml")); + domain = loadByEntity(domain); + + DomainHistory requestHistory = + getOnlyHistoryEntryOfType(domain, DOMAIN_TRANSFER_REQUEST, DomainHistory.class); + // Check that the server approve billing recurrence (which will reify after 5 days if the + // transfer is not explicitly acked) maintains the non-premium behavior. + assertBillingEventsForResource( + domain, + new BillingEvent.OneTime.Builder() + .setBillingTime(now.plusDays(10)) // 5 day pending transfer + 5 day billing grace period + .setEventTime(now.plusDays(5)) + .setRegistrarId("NewRegistrar") + .setCost(Money.of(USD, new BigDecimal("11.00"))) + .setDomainHistory(requestHistory) + .setReason(Reason.TRANSFER) + .setPeriodYears(1) + .setTargetId("example.tld") + .build(), + getGainingClientAutorenewEvent() + .asBuilder() + .setRenewalPriceBehavior(RenewalPriceBehavior.DEFAULT) + .setRenewalPrice(null) + .setDomainHistory(requestHistory) + .build(), + getLosingClientAutorenewEvent() + .asBuilder() + .setRecurrenceEndTime(now.plusDays(5)) + .setRenewalPriceBehavior(RenewalPriceBehavior.SPECIFIED) + .setRenewalPrice(Money.of(USD, new BigDecimal("18.79"))) + .build()); + } + + @Test + void testSuccess_defaultRenewalPrice_carriedOverForPackageName() throws Exception { + setupDomain("example", "tld"); + persistResource(Registry.get("tld").asBuilder().build()); + domain = loadByEntity(domain); + persistResource( + loadByKey(domain.getAutorenewBillingEvent()) + .asBuilder() + .setRenewalPriceBehavior(RenewalPriceBehavior.DEFAULT) + .build()); + AllocationToken allocationToken = + persistResource( + new AllocationToken.Builder() + .setToken("abc123") + .setTokenType(PACKAGE) + .setRenewalPriceBehavior(RenewalPriceBehavior.SPECIFIED) + .setAllowedRegistrarIds(ImmutableSet.of("TheRegistrar")) + .build()); + domain = + persistResource( + domain.asBuilder().setCurrentPackageToken(allocationToken.createVKey()).build()); + DateTime now = clock.nowUtc(); + + setEppInput("domain_transfer_request.xml"); + eppLoader.replaceAll("JD1234-REP", contact.getRepoId()); + runFlowAssertResponse(loadFile("domain_transfer_request_response.xml")); + domain = loadByEntity(domain); + + DomainHistory requestHistory = + getOnlyHistoryEntryOfType(domain, DOMAIN_TRANSFER_REQUEST, DomainHistory.class); + // Check that the server approve billing recurrence (which will reify after 5 days if the + // transfer is not explicitly acked) maintains the non-premium behavior. + assertBillingEventsForResource( + domain, + new BillingEvent.OneTime.Builder() + .setBillingTime(now.plusDays(10)) // 5 day pending transfer + 5 day billing grace period + .setEventTime(now.plusDays(5)) + .setRegistrarId("NewRegistrar") + .setCost(Money.of(USD, new BigDecimal("11.00"))) + .setDomainHistory(requestHistory) + .setReason(Reason.TRANSFER) + .setPeriodYears(1) + .setTargetId("example.tld") + .build(), + getGainingClientAutorenewEvent() + .asBuilder() + .setRenewalPriceBehavior(RenewalPriceBehavior.DEFAULT) + .setRenewalPrice(null) + .setDomainHistory(requestHistory) + .build(), + getLosingClientAutorenewEvent() + .asBuilder() + .setRecurrenceEndTime(now.plusDays(5)) + .setRenewalPriceBehavior(RenewalPriceBehavior.DEFAULT) + .build()); + } + + @Test + void testSuccess_packageName_zeroPeriod() throws Exception { + setupDomain("example", "tld"); + persistResource(Registry.get("tld").asBuilder().build()); + domain = loadByEntity(domain); + persistResource( + loadByKey(domain.getAutorenewBillingEvent()) + .asBuilder() + .setRenewalPriceBehavior(RenewalPriceBehavior.DEFAULT) + .build()); + AllocationToken allocationToken = + persistResource( + new AllocationToken.Builder() + .setToken("abc123") + .setTokenType(PACKAGE) + .setRenewalPriceBehavior(RenewalPriceBehavior.SPECIFIED) + .setAllowedRegistrarIds(ImmutableSet.of("TheRegistrar")) + .build()); + domain = + persistResource( + domain.asBuilder().setCurrentPackageToken(allocationToken.createVKey()).build()); + + doSuccessfulSuperuserExtensionTest( + "domain_transfer_request_superuser_extension.xml", + "domain_transfer_request_response_su_ext_zero_period_zero_transfer_length.xml", + domain.getRegistrationExpirationTime().plusYears(0), + ImmutableMap.of("PERIOD", "0", "AUTOMATIC_TRANSFER_LENGTH", "0"), + Optional.empty(), + Period.create(0, Unit.YEARS), + Duration.ZERO); + } + private void runWrongCurrencyTest(Map substitutions) { Map fullSubstitutions = Maps.newHashMap(); fullSubstitutions.putAll(substitutions); diff --git a/core/src/test/java/google/registry/model/domain/DomainTest.java b/core/src/test/java/google/registry/model/domain/DomainTest.java index be14b34f6..fe83bea25 100644 --- a/core/src/test/java/google/registry/model/domain/DomainTest.java +++ b/core/src/test/java/google/registry/model/domain/DomainTest.java @@ -19,6 +19,7 @@ import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; import static google.registry.model.EppResourceUtils.loadByForeignKey; +import static google.registry.model.billing.BillingEvent.RenewalPriceBehavior.SPECIFIED; import static google.registry.model.domain.token.AllocationToken.TokenType.PACKAGE; import static google.registry.model.domain.token.AllocationToken.TokenType.SINGLE_USE; import static google.registry.testing.DatabaseHelper.cloneAndSetAutoTimestamps; @@ -48,7 +49,6 @@ import google.registry.model.ImmutableObjectSubject; import google.registry.model.billing.BillingEvent; import google.registry.model.billing.BillingEvent.Flag; import google.registry.model.billing.BillingEvent.Reason; -import google.registry.model.billing.BillingEvent.RenewalPriceBehavior; import google.registry.model.contact.Contact; import google.registry.model.domain.DesignatedContact.Type; import google.registry.model.domain.launch.LaunchNotice; @@ -821,6 +821,45 @@ public class DomainTest { .isEqualTo(newExpiration); } + @Test + void testClone_removesPackageFromTransferredDomain() { + // 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); + + DomainTransferData transferData = + new DomainTransferData.Builder() + .setPendingTransferExpirationTime(transferExpirationTime) + .setTransferStatus(TransferStatus.PENDING) + .setGainingRegistrarId("TheRegistrar") + .build(); + Period extensionPeriod = transferData.getTransferPeriod(); + DateTime newExpiration = previousExpiration.plusYears(extensionPeriod.getValue()); + AllocationToken allocationToken = + persistResource( + new AllocationToken.Builder() + .setToken("abc123") + .setTokenType(PACKAGE) + .setRenewalPriceBehavior(SPECIFIED) + .setAllowedRegistrarIds(ImmutableSet.of("TheRegistrar")) + .build()); + domain = + persistResource( + domain + .asBuilder() + .setRegistrationExpirationTime(previousExpiration) + .setTransferData(transferData) + .setCurrentPackageToken(allocationToken.createVKey()) + .build()); + + assertThat(domain.getCurrentPackageToken()).isPresent(); + Domain clonedDomain = domain.cloneProjectedAtTime(now); + assertThat(clonedDomain.getRegistrationExpirationTime()).isEqualTo(newExpiration); + assertThat(clonedDomain.getCurrentPackageToken()).isEmpty(); + } + @Test void testClone_doesNotExtendExpirationForPendingTransfer() { // Pending transfers shouldn't affect the expiration time @@ -846,6 +885,41 @@ public class DomainTest { .isEqualTo(previousExpiration); } + @Test + void testClone_doesNotRemovePackageForPendingTransfer() { + // Pending transfers shouldn't affect the expiration time + DateTime now = DateTime.now(UTC); + DateTime transferExpirationTime = now.plusDays(1); + DateTime previousExpiration = now.plusWeeks(2); + + DomainTransferData transferData = + new DomainTransferData.Builder() + .setPendingTransferExpirationTime(transferExpirationTime) + .setTransferStatus(TransferStatus.PENDING) + .setGainingRegistrarId("TheRegistrar") + .build(); + AllocationToken allocationToken = + persistResource( + new AllocationToken.Builder() + .setToken("abc123") + .setTokenType(PACKAGE) + .setRenewalPriceBehavior(SPECIFIED) + .setAllowedRegistrarIds(ImmutableSet.of("TheRegistrar")) + .build()); + domain = + persistResource( + domain + .asBuilder() + .setRegistrationExpirationTime(previousExpiration) + .setTransferData(transferData) + .setCurrentPackageToken(allocationToken.createVKey()) + .build()); + + Domain clonedDomain = domain.cloneProjectedAtTime(now); + assertThat(clonedDomain.getRegistrationExpirationTime()).isEqualTo(previousExpiration); + assertThat(clonedDomain.getCurrentPackageToken().get()).isEqualTo(allocationToken.createVKey()); + } + @Test void testClone_transferDuringAutorenew() { // When the domain is an autorenew grace period, we should not extend the registration @@ -1001,7 +1075,7 @@ public class DomainTest { new AllocationToken.Builder() .setToken("abc123") .setTokenType(PACKAGE) - .setRenewalPriceBehavior(RenewalPriceBehavior.SPECIFIED) + .setRenewalPriceBehavior(SPECIFIED) .setAllowedRegistrarIds(ImmutableSet.of("TheRegistrar")) .build(); IllegalArgumentException thrown = @@ -1018,7 +1092,7 @@ public class DomainTest { new AllocationToken.Builder() .setToken("abc123") .setTokenType(PACKAGE) - .setRenewalPriceBehavior(RenewalPriceBehavior.SPECIFIED) + .setRenewalPriceBehavior(SPECIFIED) .setAllowedRegistrarIds(ImmutableSet.of("TheRegistrar")) .build()); domain = domain.asBuilder().setCurrentPackageToken(allocationToken.createVKey()).build();