Update conditions when domain update flow triggers dns publish task (#1811)

Addressing b/246375161

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

@@ -87,6 +87,7 @@ import google.registry.model.poll.PendingActionNotificationResponse.DomainPendin
 import google.registry.model.poll.PollMessage;
 import google.registry.model.reporting.IcannReportingTypes.ActivityReportField;
 import google.registry.model.tld.Registry;
+import java.util.Objects;
 import java.util.Optional;
 import javax.inject.Inject;
 import org.joda.time.DateTime;
@@ -181,7 +182,9 @@ public final class DomainUpdateFlow implements TransactionalFlow {
     DomainHistory domainHistory =
         historyBuilder.setType(DOMAIN_UPDATE).setDomain(newDomain).build();
     validateNewState(newDomain);
+    if (requiresDnsUpdate(existingDomain, newDomain)) {
       dnsQueue.addDomainRefreshTask(targetId);
+    }
     ImmutableSet.Builder<ImmutableObject> entitiesToSave = new ImmutableSet.Builder<>();
     entitiesToSave.add(newDomain, domainHistory);
     Optional<BillingEvent.OneTime> statusUpdateBillingEvent =
@@ -203,6 +206,16 @@ public final class DomainUpdateFlow implements TransactionalFlow {
     return responseBuilder.build();
   }
 
+  /** Determines if any of the changes to new domain should trigger DNS update. */
+  private boolean requiresDnsUpdate(Domain existingDomain, Domain newDomain) {
+    if (existingDomain.shouldPublishToDns() != newDomain.shouldPublishToDns()
+        || !Objects.equals(newDomain.getDsData(), existingDomain.getDsData())
+        || !Objects.equals(newDomain.getNsHosts(), existingDomain.getNsHosts())) {
+      return true;
+    }
+    return false;
+  }
+
   /** Fail if the object doesn't exist or was deleted. */
   private void verifyUpdateAllowed(Update command, Domain existingDomain, DateTime now)
       throws EppException {
@@ -267,12 +280,18 @@ public final class DomainUpdateFlow implements TransactionalFlow {
             .setLastEppUpdateRegistrarId(registrarId)
             .addStatusValues(add.getStatusValues())
             .removeStatusValues(remove.getStatusValues())
-            .addNameservers(add.getNameservers().stream().collect(toImmutableSet()))
-            .removeNameservers(remove.getNameservers().stream().collect(toImmutableSet()))
             .removeContacts(remove.getContacts())
             .addContacts(add.getContacts())
             .setRegistrant(firstNonNull(change.getRegistrant(), domain.getRegistrant()))
             .setAuthInfo(firstNonNull(change.getAuthInfo(), domain.getAuthInfo()));
+
+    if (!add.getNameservers().isEmpty()) {
+      domainBuilder.addNameservers(add.getNameservers().stream().collect(toImmutableSet()));
+    }
+    if (!remove.getNameservers().isEmpty()) {
+      domainBuilder.removeNameservers(remove.getNameservers().stream().collect(toImmutableSet()));
+    }
+
     Optional<DomainUpdateSuperuserExtension> superuserExt =
         eppInput.getSingleExtension(DomainUpdateSuperuserExtension.class);
     if (superuserExt.isPresent()) {

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

@@ -47,6 +47,7 @@ import static google.registry.testing.DomainSubject.assertAboutDomains;
 import static google.registry.testing.EppExceptionSubject.assertAboutEppExceptions;
 import static google.registry.testing.HistoryEntrySubject.assertAboutHistoryEntries;
 import static google.registry.testing.TaskQueueHelper.assertDnsTasksEnqueued;
+import static google.registry.testing.TaskQueueHelper.assertNoDnsTasksEnqueued;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 import static org.joda.money.CurrencyUnit.USD;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -496,13 +497,11 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow, Domain
             expectedDsData.stream()
                 .map(ds -> ds.cloneWithDomainRepoId(resource.getRepoId()))
                 .collect(toImmutableSet()));
-
+    if (dnsTaskEnqueued) {
-    // TODO: REENABLE AFTER PROPER FIX FOR DNS PUBLISHING TASKS IS FOUND
+      assertDnsTasksEnqueued("example.tld");
-    // if (dnsTaskEnqueued) {
+    } else {
-    //   assertDnsTasksEnqueued("example.tld");
+      assertNoDnsTasksEnqueued();
-    // } else {
+    }
-    //   assertNoDnsTasksEnqueued();
-    // }
   }
 
   @Test
@@ -1747,4 +1746,51 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow, Domain
     assertAboutDomains().that(reloadResourceByForeignKey()).hasNoAutorenewEndTime();
   }
 
+  @Test
+  void testAddDnsPublishStatus_enqueueDnsTask() throws Exception {
+    setEppInput(
+        "domain_update_status_change.xml",
+        ImmutableMap.of("STATUS_ADD", "clientHold", "STATUS_REM", "clientTransferProhibited"));
+    persistReferencedEntities();
+    persistResource(
+        persistDomain()
+            .asBuilder()
+            .setDomainName("example.tld")
+            .setStatusValues(ImmutableSet.of(StatusValue.CLIENT_TRANSFER_PROHIBITED))
+            .build());
+    runFlowAsSuperuser();
+    assertDnsTasksEnqueued("example.tld");
+  }
+
+  @Test
+  void testRemoveEveryDnsPublishStatus_enqueueDnsTask() throws Exception {
+    setEppInput(
+        "domain_update_status_change.xml",
+        ImmutableMap.of("STATUS_REM", "serverHold", "STATUS_ADD", "clientTransferProhibited"));
+    persistReferencedEntities();
+    persistResource(
+        persistDomain()
+            .asBuilder()
+            .setDomainName("example.tld")
+            .setStatusValues(ImmutableSet.of(StatusValue.SERVER_HOLD))
+            .build());
+    runFlowAsSuperuser();
+    assertDnsTasksEnqueued("example.tld");
+  }
+
+  @Test
+  void testChangeSomeOrNoChangeDnsPublishStatus_doNotEnqueueDnsTask() throws Exception {
+    setEppInput(
+        "domain_update_status_change.xml",
+        ImmutableMap.of("STATUS_ADD", "clientUpdateProhibited", "STATUS_REM", "pendingDelete"));
+    persistReferencedEntities();
+    persistResource(
+        persistDomain()
+            .asBuilder()
+            .setDomainName("example.tld")
+            .setStatusValues(ImmutableSet.of(StatusValue.PENDING_DELETE, StatusValue.SERVER_HOLD))
+            .build());
+    runFlowAsSuperuser();
+    assertNoDnsTasksEnqueued();
+  }
 }

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

@@ -0,0 +1,17 @@
+<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:add>
+          <domain:status s="%STATUS_ADD%"/>
+        </domain:add>
+        <domain:rem>
+          <domain:status s="%STATUS_REM%"/>
+        </domain:rem>
+      </domain:update>
+    </update>
+    <clTRID>ABC-12345</clTRID>
+  </command>
+</epp>