Convert more flow tests to replay/compare (#1009)

* Convert more flow tests to replay/compare

Add the replay extension to another batch of flow tests.  In the course of
this:

- Refactor out domain deletion code into DatabaseHelper so that it can be used
  from multiple tests.
- Make null handling uniform for contact phone numbers.

* Convert postLoad method to onLoad.

* Remove "Test" import missed during rebase

* Deal with persistence of billing cancellations

Deal with the persistence of billing cancellations, which were added in the
master branch since before this PR was initially sent for review.

* Adding forgotten flyway file

* Removed debug variable

core/src/main/java/google/registry/model/contact/ContactBase.java

@@ -21,6 +21,7 @@ import static google.registry.model.EppResourceUtils.projectResourceOntoBuilderA
 import com.google.common.collect.ImmutableList;
 import com.googlecode.objectify.annotation.IgnoreSave;
 import com.googlecode.objectify.annotation.Index;
+import com.googlecode.objectify.annotation.OnLoad;
 import com.googlecode.objectify.condition.IfNull;
 import google.registry.model.EppResource;
 import google.registry.model.EppResource.ResourceWithTransferData;
@@ -189,6 +190,17 @@ public class ContactBase extends EppResource implements ResourceWithTransferData
             + " use ContactResource instead");
   }
 
+  @OnLoad
+  void onLoad() {
+    if (voice != null && voice.hasNullFields()) {
+      voice = null;
+    }
+
+    if (fax != null && fax.hasNullFields()) {
+      fax = null;
+    }
+  }
+
   public String getContactId() {
     return contactId;
   }
@@ -325,11 +337,17 @@ public class ContactBase extends EppResource implements ResourceWithTransferData
     }
 
     public B setVoiceNumber(ContactPhoneNumber voiceNumber) {
+      if (voiceNumber != null && voiceNumber.hasNullFields()) {
+        voiceNumber = null;
+      }
       getInstance().voice = voiceNumber;
       return thisCastToDerived();
     }
 
     public B setFaxNumber(ContactPhoneNumber faxNumber) {
+      if (faxNumber != null && faxNumber.hasNullFields()) {
+        faxNumber = null;
+      }
       getInstance().fax = faxNumber;
       return thisCastToDerived();
     }

core/src/main/java/google/registry/model/eppcommon/PhoneNumber.java

@@ -71,6 +71,11 @@ public class PhoneNumber extends ImmutableObject {
     return phoneNumber + (extension != null ? " x" + extension : "");
   }
 
+  /** Returns true if both fields of the phone number are null. */
+  public boolean hasNullFields() {
+    return phoneNumber == null && extension == null;
+  }
+
   /** A builder for constructing {@link PhoneNumber}. */
   public static class Builder<T extends PhoneNumber> extends Buildable.Builder<T> {
     @Override

core/src/main/java/google/registry/model/transfer/DomainTransferData.java

@@ -83,7 +83,11 @@ public class DomainTransferData extends TransferData<DomainTransferData.Builder>
 
   @Ignore
   @Column(name = "transfer_billing_cancellation_id")
-  VKey<BillingEvent.Cancellation> billingCancellationId;
+  public VKey<BillingEvent.Cancellation> billingCancellationId;
+
+  @Ignore
+  @Column(name = "transfer_billing_cancellation_history_id")
+  Long billingCancellationHistoryId;
 
   /**
    * The regular one-time billing event that will be charged for a server-approved transfer.
@@ -149,6 +153,17 @@ public class DomainTransferData extends TransferData<DomainTransferData.Builder>
     serverApproveAutorenewPollMessage =
         DomainBase.restoreOfyFrom(
             rootKey, serverApproveAutorenewPollMessage, serverApproveAutorenewPollMessageHistoryId);
+    billingCancellationId =
+        DomainBase.restoreOfyFrom(rootKey, billingCancellationId, billingCancellationHistoryId);
+
+    // Reconstruct server approve entities.  We currently have to call postLoad() a _second_ time
+    // if the billing cancellation id has been reconstituted, as it is part of that set.
+    // TODO(b/183010623): Normalize the approaches to VKey reconstitution for the TransferData
+    // hierarchy (the logic currently lives either in PostLoad or here, depending on the key).
+    if (billingCancellationId != null) {
+      serverApproveEntities = null;
+      postLoad();
+    }
   }
 
   /**
@@ -179,6 +194,12 @@ public class DomainTransferData extends TransferData<DomainTransferData.Builder>
     serverApproveAutorenewPollMessageHistoryId = DomainBase.getHistoryId(val);
   }
 
+  @SuppressWarnings("unused") // For Hibernate.
+  private void billingCancellationHistoryId(
+      @AlsoLoad("billingCancellationHistoryId") VKey<BillingEvent.Cancellation> val) {
+    billingCancellationHistoryId = DomainBase.getHistoryId(val);
+  }
+
   public Period getTransferPeriod() {
     return transferPeriod;
   }
@@ -269,6 +290,7 @@ public class DomainTransferData extends TransferData<DomainTransferData.Builder>
       DomainTransferData domainTransferData) {
     if (isNullOrEmpty(serverApproveEntities)) {
       domainTransferData.billingCancellationId = null;
+      domainTransferData.billingCancellationHistoryId = null;
     } else {
       domainTransferData.billingCancellationId =
           (VKey<BillingEvent.Cancellation>)
@@ -276,6 +298,10 @@ public class DomainTransferData extends TransferData<DomainTransferData.Builder>
                   .filter(k -> k.getKind().equals(BillingEvent.Cancellation.class))
                   .findFirst()
                   .orElse(null);
+      domainTransferData.billingCancellationHistoryId =
+          domainTransferData.billingCancellationId != null
+              ? DomainBase.getHistoryId(domainTransferData.billingCancellationId)
+              : null;
     }
   }
 

core/src/test/java/google/registry/flows/contact/ContactTransferQueryFlowTest.java

@@ -32,15 +32,22 @@ import google.registry.model.eppcommon.AuthInfo.PasswordAuth;
 import google.registry.model.reporting.HistoryEntry;
 import google.registry.model.transfer.TransferStatus;
 import google.registry.testing.DualDatabaseTest;
+import google.registry.testing.ReplayExtension;
 import google.registry.testing.TestOfyAndSql;
 import org.joda.time.DateTime;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Order;
+import org.junit.jupiter.api.extension.RegisterExtension;
 
 /** Unit tests for {@link ContactTransferQueryFlow}. */
 @DualDatabaseTest
 class ContactTransferQueryFlowTest
     extends ContactTransferFlowTestCase<ContactTransferQueryFlow, ContactResource> {
 
+  @Order(value = Order.DEFAULT - 2)
+  @RegisterExtension
+  final ReplayExtension replayExtension = ReplayExtension.createWithCompare(clock);
+
   @BeforeEach
   void setUp() {
     setEppInput("contact_transfer_query.xml");

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

@@ -24,8 +24,6 @@ import static google.registry.model.reporting.HistoryEntry.Type.DOMAIN_TRANSFER_
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static google.registry.testing.DatabaseHelper.assertBillingEventsForResource;
 import static google.registry.testing.DatabaseHelper.createTld;
-import static google.registry.testing.DatabaseHelper.deleteResource;
-import static google.registry.testing.DatabaseHelper.getBillingEvents;
 import static google.registry.testing.DatabaseHelper.getOnlyHistoryEntryOfType;
 import static google.registry.testing.DatabaseHelper.getOnlyPollMessage;
 import static google.registry.testing.DatabaseHelper.getPollMessages;
@@ -529,24 +527,7 @@ class DomainTransferApproveFlowTest
 
   @Test
   void testFailure_nonexistentDomain() throws Exception {
-    Iterable<BillingEvent> billingEvents = getBillingEvents();
-    Iterable<HistoryEntry> historyEntries = tm().loadAllOf(HistoryEntry.class);
-    Iterable<PollMessage> pollMessages = tm().loadAllOf(PollMessage.class);
-    tm().transact(
-            () -> {
-              deleteResource(domain);
-              for (BillingEvent event : billingEvents) {
-                deleteResource(event);
-              }
-              for (PollMessage pollMessage : pollMessages) {
-                deleteResource(pollMessage);
-              }
-              deleteResource(subordinateHost);
-              for (HistoryEntry hist : historyEntries) {
-                deleteResource(hist);
-              }
-            });
-
+    deleteTestDomain(domain);
     ResourceDoesNotExistException thrown =
         assertThrows(
             ResourceDoesNotExistException.class,

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

@@ -23,7 +23,6 @@ import static google.registry.model.reporting.HistoryEntry.Type.DOMAIN_TRANSFER_
 import static google.registry.testing.DatabaseHelper.assertBillingEvents;
 import static google.registry.testing.DatabaseHelper.assertPollMessages;
 import static google.registry.testing.DatabaseHelper.createPollMessageForImplicitTransfer;
-import static google.registry.testing.DatabaseHelper.deleteResource;
 import static google.registry.testing.DatabaseHelper.getOnlyHistoryEntryOfType;
 import static google.registry.testing.DatabaseHelper.getPollMessages;
 import static google.registry.testing.DatabaseHelper.loadRegistrar;
@@ -54,15 +53,22 @@ import google.registry.model.reporting.HistoryEntry;
 import google.registry.model.transfer.TransferData;
 import google.registry.model.transfer.TransferResponse.DomainTransferResponse;
 import google.registry.model.transfer.TransferStatus;
+import google.registry.testing.ReplayExtension;
 import org.joda.time.DateTime;
 import org.joda.time.Duration;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Order;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
 
 /** Unit tests for {@link DomainTransferCancelFlow}. */
 class DomainTransferCancelFlowTest
     extends DomainTransferFlowTestCase<DomainTransferCancelFlow, DomainBase> {
 
+  @Order(value = Order.DEFAULT - 2)
+  @RegisterExtension
+  final ReplayExtension replayExtension = ReplayExtension.createWithCompare(clock);
+
   @BeforeEach
   void setUp() {
     setEppInput("domain_transfer_cancel.xml");
@@ -332,7 +338,7 @@ class DomainTransferCancelFlowTest
 
   @Test
   void testFailure_nonexistentDomain() throws Exception {
-    deleteResource(domain);
+    deleteTestDomain(domain);
     ResourceDoesNotExistException thrown =
         assertThrows(
             ResourceDoesNotExistException.class, () -> doFailingTest("domain_transfer_cancel.xml"));

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

@@ -17,8 +17,11 @@ package google.registry.flows.domain;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.reporting.HistoryEntry.Type.DOMAIN_CREATE;
+import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static google.registry.testing.DatabaseHelper.createBillingEventForTransfer;
 import static google.registry.testing.DatabaseHelper.createTld;
+import static google.registry.testing.DatabaseHelper.deleteResource;
+import static google.registry.testing.DatabaseHelper.getBillingEvents;
 import static google.registry.testing.DatabaseHelper.getOnlyHistoryEntryOfType;
 import static google.registry.testing.DatabaseHelper.persistActiveContact;
 import static google.registry.testing.DatabaseHelper.persistDomainWithDependentResources;
@@ -39,6 +42,7 @@ import google.registry.model.contact.ContactResource;
 import google.registry.model.domain.DomainBase;
 import google.registry.model.eppcommon.StatusValue;
 import google.registry.model.host.HostResource;
+import google.registry.model.poll.PollMessage;
 import google.registry.model.registry.Registry;
 import google.registry.model.reporting.HistoryEntry;
 import google.registry.model.transfer.TransferData;
@@ -159,6 +163,29 @@ abstract class DomainTransferFlowTestCase<F extends Flow, R extends EppResource>
         .build();
   }
 
+  /**
+   * Delete "domain" and all history records, billing events, poll messages and subordinate hosts.
+   */
+  void deleteTestDomain(DomainBase domain) {
+    Iterable<BillingEvent> billingEvents = getBillingEvents();
+    Iterable<HistoryEntry> historyEntries = tm().loadAllOf(HistoryEntry.class);
+    Iterable<PollMessage> pollMessages = tm().loadAllOf(PollMessage.class);
+    tm().transact(
+            () -> {
+              deleteResource(domain);
+              for (BillingEvent event : billingEvents) {
+                deleteResource(event);
+              }
+              for (PollMessage pollMessage : pollMessages) {
+                deleteResource(pollMessage);
+              }
+              deleteResource(subordinateHost);
+              for (HistoryEntry hist : historyEntries) {
+                deleteResource(hist);
+              }
+            });
+  }
+
   void assertTransferFailed(
       DomainBase domain, TransferStatus status, TransferData oldTransferData) {
     assertAboutDomains().that(domain)

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

@@ -16,7 +16,6 @@ package google.registry.flows.domain;
 
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.testing.DatabaseHelper.assertBillingEvents;
-import static google.registry.testing.DatabaseHelper.deleteResource;
 import static google.registry.testing.DatabaseHelper.getPollMessages;
 import static google.registry.testing.DatabaseHelper.persistResource;
 import static google.registry.testing.DomainBaseSubject.assertAboutDomains;
@@ -34,13 +33,20 @@ import google.registry.model.domain.DomainBase;
 import google.registry.model.eppcommon.AuthInfo.PasswordAuth;
 import google.registry.model.reporting.HistoryEntry;
 import google.registry.model.transfer.TransferStatus;
+import google.registry.testing.ReplayExtension;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Order;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
 
 /** Unit tests for {@link DomainTransferQueryFlow}. */
 class DomainTransferQueryFlowTest
     extends DomainTransferFlowTestCase<DomainTransferQueryFlow, DomainBase> {
 
+  @Order(value = Order.DEFAULT - 2)
+  @RegisterExtension
+  final ReplayExtension replayExtension = ReplayExtension.createWithCompare(clock);
+
   @BeforeEach
   void beforeEach() {
     setEppInput("domain_transfer_query.xml");
@@ -225,7 +231,7 @@ class DomainTransferQueryFlowTest
 
   @Test
   void testFailure_nonexistentDomain() throws Exception {
-    deleteResource(domain);
+    deleteTestDomain(domain);
     ResourceDoesNotExistException thrown =
         assertThrows(
             ResourceDoesNotExistException.class, () -> doFailingTest("domain_transfer_query.xml"));

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

@@ -57,16 +57,23 @@ import google.registry.model.transfer.TransferData;
 import google.registry.model.transfer.TransferResponse;
 import google.registry.model.transfer.TransferStatus;
 import google.registry.testing.DualDatabaseTest;
+import google.registry.testing.ReplayExtension;
 import google.registry.testing.TestOfyAndSql;
 import org.joda.time.DateTime;
 import org.joda.time.Duration;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Order;
+import org.junit.jupiter.api.extension.RegisterExtension;
 
 /** Unit tests for {@link DomainTransferRejectFlow}. */
 @DualDatabaseTest
 class DomainTransferRejectFlowTest
     extends DomainTransferFlowTestCase<DomainTransferRejectFlow, DomainBase> {
 
+  @Order(value = Order.DEFAULT - 2)
+  @RegisterExtension
+  final ReplayExtension replayExtension = ReplayExtension.createWithCompare(clock);
+
   @BeforeEach
   void setUp() {
     setEppInput("domain_transfer_reject.xml");

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

@@ -103,6 +103,7 @@ import google.registry.model.transfer.TransferResponse;
 import google.registry.model.transfer.TransferStatus;
 import google.registry.persistence.VKey;
 import google.registry.testing.DualDatabaseTest;
+import google.registry.testing.ReplayExtension;
 import google.registry.testing.TaskQueueHelper.TaskMatcher;
 import google.registry.testing.TestOfyAndSql;
 import java.util.Map;
@@ -112,12 +113,18 @@ import org.joda.money.Money;
 import org.joda.time.DateTime;
 import org.joda.time.Duration;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Order;
+import org.junit.jupiter.api.extension.RegisterExtension;
 
 /** Unit tests for {@link DomainTransferRequestFlow}. */
 @DualDatabaseTest
 class DomainTransferRequestFlowTest
     extends DomainTransferFlowTestCase<DomainTransferRequestFlow, DomainBase> {
 
+  @Order(value = Order.DEFAULT - 2)
+  @RegisterExtension
+  final ReplayExtension replayExtension = ReplayExtension.createWithCompare(clock);
+
   private static final ImmutableMap<String, String> BASE_FEE_MAP =
       new ImmutableMap.Builder<String, String>()
           .put("DOMAIN", "example.tld")

core/src/test/java/google/registry/flows/host/HostCheckFlowTest.java

@@ -25,12 +25,19 @@ import google.registry.flows.ResourceCheckFlowTestCase;
 import google.registry.flows.exceptions.TooManyResourceChecksException;
 import google.registry.model.host.HostResource;
 import google.registry.testing.DualDatabaseTest;
+import google.registry.testing.ReplayExtension;
 import google.registry.testing.TestOfyAndSql;
+import org.junit.jupiter.api.Order;
+import org.junit.jupiter.api.extension.RegisterExtension;
 
 /** Unit tests for {@link HostCheckFlow}. */
 @DualDatabaseTest
 class HostCheckFlowTest extends ResourceCheckFlowTestCase<HostCheckFlow, HostResource> {
 
+  @Order(value = Order.DEFAULT - 2)
+  @RegisterExtension
+  final ReplayExtension replayExtension = ReplayExtension.createWithCompare(clock);
+
   HostCheckFlowTest() {
     setEppInput("host_check.xml");
   }

db/src/main/resources/sql/er_diagram/brief_er_diagram.html

@@ -261,11 +261,11 @@ td.section {
     </tr> 
     <tr> 
      <td class="property_name">generated on</td> 
-     <td class="property_value">2021-03-04 16:08:40.773463</td> 
+     <td class="property_value">2021-03-16 19:06:44.592583</td> 
     </tr> 
     <tr>
      <td class="property_name">last flyway file</td>
-     <td id="lastFlywayFile" class="property_value">V87__fix_super_domain_fk.sql</td>
+     <td id="lastFlywayFile" class="property_value">V88__transfer_billing_cancellation_history_id.sql</td>
     </tr>
    </tbody>
   </table> 
@@ -284,7 +284,7 @@ td.section {
      generated on
     </text> 
     <text text-anchor="start" x="4027.94" y="-10.8" font-family="Helvetica,sans-Serif" font-size="14.00">
-     2021-03-04 16:08:40.773463
+     2021-03-16 19:06:44.592583
     </text> 
     <polygon fill="none" stroke="#888888" points="3940.44,-4 3940.44,-44 4205.44,-44 4205.44,-4 3940.44,-4" /> <!-- allocationtoken_a08ccbef --> 
     <g id="node1" class="node"> 

db/src/main/resources/sql/flyway.txt

@@ -85,3 +85,4 @@ V84__add_vkey_columns_in_billing_cancellation.sql
 V85__add_required_columns_in_transfer_data.sql
 V86__third_poll_message.sql
 V87__fix_super_domain_fk.sql
+V88__transfer_billing_cancellation_history_id.sql

db/src/main/resources/sql/flyway/V88__transfer_billing_cancellation_history_id.sql

@@ -0,0 +1,18 @@
+-- Copyright 2021 The Nomulus Authors. All Rights Reserved.
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--     http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+ALTER TABLE "Domain"
+    ADD COLUMN  "transfer_billing_cancellation_history_id" int8;
+ALTER TABLE "DomainHistory"
+    ADD COLUMN  "transfer_billing_cancellation_history_id" int8;

db/src/main/resources/sql/schema/db-schema.sql.generated

@@ -280,6 +280,7 @@
         subordinate_hosts text[],
         tech_contact text,
         tld text,
+        transfer_billing_cancellation_history_id int8,
         transfer_billing_cancellation_id int8,
         transfer_billing_recurrence_id int8,
         transfer_billing_recurrence_history_id int8,
@@ -352,6 +353,7 @@
         subordinate_hosts text[],
         tech_contact text,
         tld text,
+        transfer_billing_cancellation_history_id int8,
         transfer_billing_cancellation_id int8,
         transfer_billing_recurrence_id int8,
         transfer_billing_recurrence_history_id int8,

db/src/main/resources/sql/schema/nomulus.golden.sql

@@ -386,7 +386,8 @@ CREATE TABLE public."Domain" (
     transfer_billing_event_history_id bigint,
     transfer_history_entry_id bigint,
     transfer_repo_id text,
-    transfer_poll_message_id_3 bigint
+    transfer_poll_message_id_3 bigint,
+    transfer_billing_cancellation_history_id bigint
 );
 
 
@@ -477,7 +478,8 @@ CREATE TABLE public."DomainHistory" (
     transfer_billing_event_history_id bigint,
     transfer_history_entry_id bigint,
     transfer_repo_id text,
-    transfer_poll_message_id_3 bigint
+    transfer_poll_message_id_3 bigint,
+    transfer_billing_cancellation_history_id bigint
 );