From 3809ff59a534dd3b31ad85f9572a4c5fc322a50d Mon Sep 17 00:00:00 2001
From: larryruili <larryruili@google.com>
Date: Thu, 31 Aug 2017 15:56:11 -0700
Subject: [PATCH] Filter cancellation records for only cancellable records

Previously, I would cancel all the records associated with HistoryEntry that's
available for cancellation. This could cause unexpected behavior if we
cancelled a historyEntry which itself had cancelled records (in effect we would
negate the negation unintentionally). This is easily remedied by only
cancelling records which want to be cancelled.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=167204383
---
 java/google/registry/flows/domain/DomainFlowUtils.java   | 7 +++++--
 .../registry/flows/domain/DomainDeleteFlowTest.java      | 7 +++++--
 .../flows/domain/DomainTransferApproveFlowTest.java      | 8 ++++++--
 .../flows/domain/DomainTransferCancelFlowTest.java       | 9 +++++++--
 .../flows/domain/DomainTransferRejectFlowTest.java       | 8 ++++++--
 5 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/java/google/registry/flows/domain/DomainFlowUtils.java b/java/google/registry/flows/domain/DomainFlowUtils.java
index aa3ce74c9..31581ef96 100644
--- a/java/google/registry/flows/domain/DomainFlowUtils.java
+++ b/java/google/registry/flows/domain/DomainFlowUtils.java
@@ -960,8 +960,11 @@ public class DomainFlowUtils {
     ImmutableSet.Builder<DomainTransactionRecord> recordsBuilder = new ImmutableSet.Builder<>();
     if (entryToCancel.isPresent()) {
       for (DomainTransactionRecord record : entryToCancel.get().getDomainTransactionRecords()) {
-        int cancelledAmount = -1 * record.getReportAmount();
-        recordsBuilder.add(record.asBuilder().setReportAmount(cancelledAmount).build());
+        // Only cancel fields which are cancelable
+        if (cancelableFields.contains(record.getReportField())) {
+          int cancelledAmount = -1 * record.getReportAmount();
+          recordsBuilder.add(record.asBuilder().setReportAmount(cancelledAmount).build());
+        }
       }
     }
     return recordsBuilder.build();
diff --git a/javatests/google/registry/flows/domain/DomainDeleteFlowTest.java b/javatests/google/registry/flows/domain/DomainDeleteFlowTest.java
index 1981c7d0e..5359ac2bd 100644
--- a/javatests/google/registry/flows/domain/DomainDeleteFlowTest.java
+++ b/javatests/google/registry/flows/domain/DomainDeleteFlowTest.java
@@ -870,17 +870,20 @@ public class DomainDeleteFlowTest extends ResourceFlowTestCase<DomainDeleteFlow,
     clock.advanceOneMilli();
     DomainTransactionRecord renewRecord =
         DomainTransactionRecord.create("tld", TIME_BEFORE_FLOW.plusDays(1), NET_RENEWS_3_YR, 1);
+    // We don't want to cancel non-add or renew records
+    DomainTransactionRecord notCancellableRecord =
+        DomainTransactionRecord.create("tld", TIME_BEFORE_FLOW.plusDays(1), RESTORED_DOMAINS, 5);
     earlierHistoryEntry =
         persistResource(
             earlierHistoryEntry
                 .asBuilder()
                 .setType(DOMAIN_CREATE)
                 .setModificationTime(TIME_BEFORE_FLOW.minusDays(2))
-                .setDomainTransactionRecords(ImmutableSet.of(renewRecord))
+                .setDomainTransactionRecords(ImmutableSet.of(renewRecord, notCancellableRecord))
                 .build());
     runFlow();
     HistoryEntry persistedEntry = getOnlyHistoryEntryOfType(domain, DOMAIN_DELETE);
-    // Transaction record should be the non-grace period delete, and the renew cancellation record
+    // We should only see the non-grace period delete record and the renew cancellation record
     assertThat(persistedEntry.getDomainTransactionRecords())
         .containsExactly(
             DomainTransactionRecord.create(
diff --git a/javatests/google/registry/flows/domain/DomainTransferApproveFlowTest.java b/javatests/google/registry/flows/domain/DomainTransferApproveFlowTest.java
index 586f74162..dc999a481 100644
--- a/javatests/google/registry/flows/domain/DomainTransferApproveFlowTest.java
+++ b/javatests/google/registry/flows/domain/DomainTransferApproveFlowTest.java
@@ -17,6 +17,7 @@ package google.registry.flows.domain;
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.ofy.ObjectifyService.ofy;
+import static google.registry.model.reporting.DomainTransactionRecord.TransactionReportField.NET_ADDS_4_YR;
 import static google.registry.model.reporting.DomainTransactionRecord.TransactionReportField.TRANSFER_SUCCESSFUL;
 import static google.registry.model.reporting.HistoryEntry.Type.DOMAIN_CREATE;
 import static google.registry.model.reporting.HistoryEntry.Type.DOMAIN_TRANSFER_APPROVE;
@@ -480,17 +481,20 @@ public class DomainTransferApproveFlowTest
     DomainTransactionRecord previousSuccessRecord =
         DomainTransactionRecord.create(
             "tld", clock.nowUtc().plusDays(1), TRANSFER_SUCCESSFUL, 1);
+    // We only want to cancel TRANSFER_SUCCESSFUL records
+    DomainTransactionRecord notCancellableRecord =
+        DomainTransactionRecord.create("tld", clock.nowUtc().plusDays(1), NET_ADDS_4_YR, 5);
     persistResource(
         new HistoryEntry.Builder()
             .setType(DOMAIN_TRANSFER_REQUEST)
             .setParent(domain)
             .setModificationTime(clock.nowUtc().minusDays(4))
             .setDomainTransactionRecords(
-                ImmutableSet.of(previousSuccessRecord))
+                ImmutableSet.of(previousSuccessRecord, notCancellableRecord))
             .build());
     runFlow();
     HistoryEntry persistedEntry = getOnlyHistoryEntryOfType(domain, DOMAIN_TRANSFER_APPROVE);
-    // We should produce cancellation records for the original reporting date (now + 1 day) and
+    // We should only produce cancellation records for the original reporting date (now + 1 day) and
     // success records for the new reporting date (now + transferGracePeriod=3 days)
     assertThat(persistedEntry.getDomainTransactionRecords())
         .containsExactly(
diff --git a/javatests/google/registry/flows/domain/DomainTransferCancelFlowTest.java b/javatests/google/registry/flows/domain/DomainTransferCancelFlowTest.java
index 32e94b986..b20bd049c 100644
--- a/javatests/google/registry/flows/domain/DomainTransferCancelFlowTest.java
+++ b/javatests/google/registry/flows/domain/DomainTransferCancelFlowTest.java
@@ -15,6 +15,7 @@
 package google.registry.flows.domain;
 
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.model.reporting.DomainTransactionRecord.TransactionReportField.RESTORED_DOMAINS;
 import static google.registry.model.reporting.DomainTransactionRecord.TransactionReportField.TRANSFER_SUCCESSFUL;
 import static google.registry.model.reporting.HistoryEntry.Type.DOMAIN_CREATE;
 import static google.registry.model.reporting.HistoryEntry.Type.DOMAIN_TRANSFER_CANCEL;
@@ -351,16 +352,20 @@ public class DomainTransferCancelFlowTest
             .build());
     DomainTransactionRecord previousSuccessRecord =
         DomainTransactionRecord.create("tld", clock.nowUtc().plusDays(1), TRANSFER_SUCCESSFUL, 1);
+    // We only want to cancel TRANSFER_SUCCESSFUL records
+    DomainTransactionRecord notCancellableRecord =
+        DomainTransactionRecord.create("tld", clock.nowUtc().plusDays(1), RESTORED_DOMAINS, 5);
     persistResource(
         new HistoryEntry.Builder()
             .setType(DOMAIN_TRANSFER_REQUEST)
             .setParent(domain)
             .setModificationTime(clock.nowUtc().minusDays(4))
-            .setDomainTransactionRecords(ImmutableSet.of(previousSuccessRecord))
+            .setDomainTransactionRecords(
+                ImmutableSet.of(previousSuccessRecord, notCancellableRecord))
             .build());
     runFlow();
     HistoryEntry persistedEntry = getOnlyHistoryEntryOfType(domain, DOMAIN_TRANSFER_CANCEL);
-    // We should produce a cancellation record for the original transfer success
+    // We should only produce a cancellation record for the original transfer success
     assertThat(persistedEntry.getDomainTransactionRecords())
         .containsExactly(previousSuccessRecord.asBuilder().setReportAmount(-1).build());
   }
diff --git a/javatests/google/registry/flows/domain/DomainTransferRejectFlowTest.java b/javatests/google/registry/flows/domain/DomainTransferRejectFlowTest.java
index a0eea3794..b08b6b570 100644
--- a/javatests/google/registry/flows/domain/DomainTransferRejectFlowTest.java
+++ b/javatests/google/registry/flows/domain/DomainTransferRejectFlowTest.java
@@ -15,6 +15,7 @@
 package google.registry.flows.domain;
 
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.model.reporting.DomainTransactionRecord.TransactionReportField.NET_RENEWS_3_YR;
 import static google.registry.model.reporting.DomainTransactionRecord.TransactionReportField.TRANSFER_NACKED;
 import static google.registry.model.reporting.DomainTransactionRecord.TransactionReportField.TRANSFER_SUCCESSFUL;
 import static google.registry.model.reporting.HistoryEntry.Type.DOMAIN_CREATE;
@@ -317,17 +318,20 @@ public class DomainTransferRejectFlowTest
     DomainTransactionRecord previousSuccessRecord =
         DomainTransactionRecord.create(
             "tld", clock.nowUtc().plusDays(1), TRANSFER_SUCCESSFUL, 1);
+    // We only want to cancel TRANSFER_SUCCESSFUL records
+    DomainTransactionRecord notCancellableRecord =
+        DomainTransactionRecord.create("tld", clock.nowUtc().plusDays(1), NET_RENEWS_3_YR, 5);
     persistResource(
         new HistoryEntry.Builder()
             .setType(DOMAIN_TRANSFER_REQUEST)
             .setParent(domain)
             .setModificationTime(clock.nowUtc().minusDays(4))
             .setDomainTransactionRecords(
-                ImmutableSet.of(previousSuccessRecord))
+                ImmutableSet.of(previousSuccessRecord, notCancellableRecord))
             .build());
     runFlow();
     HistoryEntry persistedEntry = getOnlyHistoryEntryOfType(domain, DOMAIN_TRANSFER_REJECT);
-    // We should produce cancellation records for the original success records and nack records
+    // We should only produce cancellation records for the original success records and nack records
     assertThat(persistedEntry.getDomainTransactionRecords())
         .containsExactly(
             previousSuccessRecord.asBuilder().setReportAmount(-1).build(),