Only send registrar update notification emails to primary contacts

The test changes are perhaps a little bit more involved than expected, because I
had to add a second RegistrarContact which had knock-on effects on other tests.
This does make the other tests better though, in that we're now testing registrars
with multiple contacts (we weren't much before).

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=243827803

java/google/registry/ui/server/registrar/RegistrarSettingsAction.java

@@ -406,20 +406,19 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
       }
     }
     // Check that required contacts don't go away, once they are set.
-    Multimap<RegistrarContact.Type, RegistrarContact> oldContactsByType = HashMultimap.create();
+    Multimap<Type, RegistrarContact> oldContactsByType = HashMultimap.create();
     for (RegistrarContact contact : existingContacts) {
-      for (RegistrarContact.Type t : contact.getTypes()) {
+      for (Type t : contact.getTypes()) {
         oldContactsByType.put(t, contact);
       }
     }
-    Multimap<RegistrarContact.Type, RegistrarContact> newContactsByType = HashMultimap.create();
+    Multimap<Type, RegistrarContact> newContactsByType = HashMultimap.create();
     for (RegistrarContact contact : updatedContacts) {
-      for (RegistrarContact.Type t : contact.getTypes()) {
+      for (Type t : contact.getTypes()) {
         newContactsByType.put(t, contact);
       }
     }
-    for (RegistrarContact.Type t
+    for (Type t : difference(oldContactsByType.keySet(), newContactsByType.keySet())) {
-        : difference(oldContactsByType.keySet(), newContactsByType.keySet())) {
       if (t.isRequired()) {
         throw new ContactRequirementException(t);
       }
@@ -446,10 +445,10 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
    * one before.
    */
   private static void ensurePhoneNumberNotRemovedForContactTypes(
-      Multimap<RegistrarContact.Type, RegistrarContact> oldContactsByType,
+      Multimap<Type, RegistrarContact> oldContactsByType,
-      Multimap<RegistrarContact.Type, RegistrarContact> newContactsByType,
+      Multimap<Type, RegistrarContact> newContactsByType,
-      RegistrarContact.Type... types) {
+      Type... types) {
-    for (RegistrarContact.Type type : types) {
+    for (Type type : types) {
       if (oldContactsByType.get(type).stream().anyMatch(HAS_PHONE)
           && newContactsByType.get(type).stream().noneMatch(HAS_PHONE)) {
         throw new ContactRequirementException(
@@ -511,6 +510,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
             authResult.userIdForLogging(),
             DiffUtils.prettyPrintDiffedMap(diffs, null)),
         existingContacts.stream()
+            .filter(c -> c.getTypes().contains(Type.ADMIN))
             .map(RegistrarContact::getEmailAddress)
             .collect(toImmutableList()));
   }
@@ -521,8 +521,8 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
       super(msg);
     }
 
-    ContactRequirementException(RegistrarContact.Type type) {
+    ContactRequirementException(Type type) {
-      super("Must have at least one " + type.getDisplayName() + " contact");
+      super(String.format("Must have at least one %s contact", type.getDisplayName()));
     }
   }
 }

javatests/google/registry/ui/server/registrar/ContactSettingsTest.java

@@ -21,11 +21,10 @@ import static google.registry.testing.DatastoreHelper.persistSimpleResource;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import google.registry.model.registrar.Registrar;
 import google.registry.model.registrar.RegistrarContact;
+import google.registry.model.registrar.RegistrarContact.Type;
 import google.registry.testing.AppEngineRule;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import org.junit.Test;
@@ -68,12 +67,13 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
   public void testPost_updateContacts_success() throws Exception {
     // Remove all the contacts but the first by updating with list of
     // just it.
-    Map<String, /* @Nullable */ Object> adminContact1 = new HashMap<>();
+    ImmutableMap<String, String> adminContact1 =
-    adminContact1.put("name", "contact1");
+        ImmutableMap.of(
-    adminContact1.put("emailAddress", "contact1@email.com");
+            "name", "contact1",
-    adminContact1.put("phoneNumber", "+1.2125650001");
+            "emailAddress", "contact1@email.com",
-    // Have to keep ADMIN or else expect FormException for at-least-one.
+            "phoneNumber", "+1.2125650001",
-    adminContact1.put("types", "ADMIN");
+            // Have to keep ADMIN or else expect FormException for at-least-one.
+            "types", "ADMIN,TECH");
 
     Registrar registrar = loadRegistrar(CLIENT_ID);
     Map<String, Object> regMap = registrar.toJsonMap();
@@ -82,60 +82,52 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
         action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", regMap));
     assertThat(response).containsEntry("status", "SUCCESS");
 
-    RegistrarContact newContact = new RegistrarContact.Builder()
+    RegistrarContact newContact =
-        .setParent(registrar)
+        new RegistrarContact.Builder()
-        .setName((String) adminContact1.get("name"))
+            .setParent(registrar)
-        .setEmailAddress((String) adminContact1.get("emailAddress"))
+            .setName(adminContact1.get("name"))
-        .setPhoneNumber((String) adminContact1.get("phoneNumber"))
+            .setEmailAddress(adminContact1.get("emailAddress"))
-        .setTypes(ImmutableList.of(RegistrarContact.Type.ADMIN))
+            .setPhoneNumber(adminContact1.get("phoneNumber"))
-        .build();
+            .setTypes(ImmutableList.of(Type.ADMIN, Type.TECH))
+            .build();
     assertThat(loadRegistrar(CLIENT_ID).getContacts()).containsExactly(newContact);
     assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
-    verifyContactsNotified();
+    verifyNotificationEmailsSent();
   }
 
   @Test
   public void testPost_updateContacts_requiredTypes_error() {
     Map<String, Object> reqJson = loadRegistrar(CLIENT_ID).toJsonMap();
-    reqJson.put("contacts",
+    reqJson.put("contacts", ImmutableList.of(techContact.toJsonMap()));
-        ImmutableList.of(AppEngineRule.makeRegistrarContact2()
+    Map<String, Object> response =
-            .asBuilder()
+        action.handleJsonRequest(
-            .setTypes(ImmutableList.of())
+            ImmutableMap.of(
-            .build().toJsonMap()));
+                "op", "update",
-    Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
+                "id", CLIENT_ID,
-        "op", "update",
+                "args", reqJson));
-        "id", CLIENT_ID,
-        "args", reqJson));
     assertThat(response).containsEntry("status", "ERROR");
-    assertThat(response).containsEntry("message", "Must have at least one "
+    assertThat(response).containsEntry("message", "Must have at least one primary contact");
-        + RegistrarContact.Type.ADMIN.getDisplayName() + " contact");
     assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: ContactRequirementException");
   }
 
   @Test
   public void testPost_updateContacts_requireTechPhone_error() {
-    // First make the contact a tech contact as well.
+    Map<String, Object> reqJson = loadRegistrar(CLIENT_ID).toJsonMap();
-    Registrar registrar = loadRegistrar(CLIENT_ID);
+    reqJson.put(
-    RegistrarContact rc = AppEngineRule.makeRegistrarContact2()
+        "contacts",
-        .asBuilder()
+        ImmutableList.of(
-        .setTypes(ImmutableSet.of(RegistrarContact.Type.ADMIN, RegistrarContact.Type.TECH))
+            AppEngineRule.makeRegistrarContact2().toJsonMap(),
-        .build();
+            techContact.asBuilder().setPhoneNumber(null).build().toJsonMap()));
-    // Lest we anger the timestamp inversion bug.
+    Map<String, Object> response =
-    // (we also update the registrar so we get the timestamp right)
+        action.handleJsonRequest(
-    registrar = persistResource(registrar);
+            ImmutableMap.of(
-    persistSimpleResource(rc);
+                "op", "update",
-
+                "id", CLIENT_ID,
-    // Now try to remove the phone number.
+                "args", reqJson));
-    rc = rc.asBuilder().setPhoneNumber(null).build();
-    Map<String, Object> reqJson = registrar.toJsonMap();
-    reqJson.put("contacts", ImmutableList.of(rc.toJsonMap()));
-    Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
-        "op", "update",
-        "id", CLIENT_ID,
-        "args", reqJson));
     assertThat(response).containsEntry("status", "ERROR");
-    assertThat(response).containsEntry("message", "Please provide a phone number for at least one "
+    assertThat(response)
-        + RegistrarContact.Type.TECH.getDisplayName() + " contact");
+        .containsEntry(
+            "message", "Please provide a phone number for at least one technical contact");
     assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: ContactRequirementException");
   }
 
@@ -156,7 +148,7 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
     // Now try to remove the contact.
     rc = rc.asBuilder().setVisibleInDomainWhoisAsAbuse(false).build();
     Map<String, Object> reqJson = registrar.toJsonMap();
-    reqJson.put("contacts", ImmutableList.of(rc.toJsonMap()));
+    reqJson.put("contacts", ImmutableList.of(rc.toJsonMap(), techContact.toJsonMap()));
     Map<String, Object> response =
         action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", reqJson));
     assertThat(response).containsEntry("status", "ERROR");
@@ -183,7 +175,7 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
     // Now try to set the phone number to null.
     rc = rc.asBuilder().setPhoneNumber(null).build();
     Map<String, Object> reqJson = registrar.toJsonMap();
-    reqJson.put("contacts", ImmutableList.of(rc.toJsonMap()));
+    reqJson.put("contacts", ImmutableList.of(rc.toJsonMap(), techContact.toJsonMap()));
     Map<String, Object> response =
         action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", reqJson));
     assertThat(response).containsEntry("status", "ERROR");

javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java

@@ -56,9 +56,11 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
   @Test
   public void testSuccess_updateRegistrarInfo_andSendsNotificationEmail() throws Exception {
     String expectedEmailBody = loadFile(getClass(), "update_registrar_email.txt");
+    // This update changes some values on the admin contact and makes it a tech contact as well,
+    // while deleting the existing tech contact (by omission).
     action.handleJsonRequest(readJsonFromFile("update_registrar.json", getLastUpdateTime()));
     verify(rsp, never()).setStatus(anyInt());
-    verifyContactsNotified();
+    verifyNotificationEmailsSent();
     ArgumentCaptor<EmailMessage> contentCaptor = ArgumentCaptor.forClass(EmailMessage.class);
     verify(emailService).sendEmail(contentCaptor.capture());
     assertThat(contentCaptor.getValue().body()).isEqualTo(expectedEmailBody);

javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java

@@ -22,16 +22,20 @@ import static google.registry.request.auth.AuthenticatedRegistrarAccessor.Role.O
 import static google.registry.security.JsonHttpTestUtils.createJsonPayload;
 import static google.registry.testing.DatastoreHelper.createTlds;
 import static google.registry.testing.DatastoreHelper.disallowRegistrarAccess;
+import static google.registry.testing.DatastoreHelper.loadRegistrar;
+import static google.registry.testing.DatastoreHelper.persistResource;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import com.google.appengine.api.users.User;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.truth.Truth;
 import google.registry.config.RegistryEnvironment;
 import google.registry.model.ofy.Ofy;
+import google.registry.model.registrar.RegistrarContact;
 import google.registry.request.JsonActionRunner;
 import google.registry.request.JsonResponse;
 import google.registry.request.ResponseImpl;
@@ -84,14 +88,26 @@ public class RegistrarSettingsActionTestCase {
   final StringWriter writer = new StringWriter();
   final FakeClock clock = new FakeClock(DateTime.parse("2014-01-01T00:00:00Z"));
 
+  RegistrarContact techContact;
+
   @Before
   public void setUp() throws Exception {
-    // Create a tld and give access to registrar "TheRegistrar" for most common test case.
+    // Registrar "TheRegistrar" has access to TLD "currenttld" but not to "newtld".
-    createTlds("currenttld");
+    createTlds("currenttld", "newtld");
-    // Create another tld but remove access for registrar "TheRegistrar". This is for the test case
-    // of updating allowed tld for registrar
-    createTlds("newtld");
     disallowRegistrarAccess(CLIENT_ID, "newtld");
+
+    // Add a technical contact to the registrar (in addition to the default admin contact created by
+    // AppEngineRule).
+    techContact =
+        persistResource(
+            new RegistrarContact.Builder()
+                .setParent(loadRegistrar(CLIENT_ID))
+                .setName("Jian-Yang")
+                .setEmailAddress("jyang@bachman.accelerator")
+                .setPhoneNumber("+1.1234567890")
+                .setTypes(ImmutableSet.of(RegistrarContact.Type.TECH))
+                .build());
+
     action.registrarAccessor = null;
     action.appEngineServiceUtils = appEngineServiceUtils;
     when(appEngineServiceUtils.getCurrentVersionHostname("backend")).thenReturn("backend.hostname");
@@ -154,7 +170,7 @@ public class RegistrarSettingsActionTestCase {
   }
 
   /** Verifies that the original contact of TheRegistrar is among those notified of a change. */
-  protected void verifyContactsNotified() throws Exception {
+  protected void verifyNotificationEmailsSent() throws Exception {
     ArgumentCaptor<EmailMessage> captor = ArgumentCaptor.forClass(EmailMessage.class);
     verify(emailService).sendEmail(captor.capture());
     Truth.assertThat(captor.getValue().recipients())

javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java

@@ -55,7 +55,7 @@ public class SecuritySettingsTest extends RegistrarSettingsActionTestCase {
     assertThat(response).containsEntry("results", ImmutableList.of(modified.toJsonMap()));
     assertThat(loadRegistrar(CLIENT_ID)).isEqualTo(modified);
     assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
-    verifyContactsNotified();
+    verifyNotificationEmailsSent();
   }
 
   @Test
@@ -85,7 +85,7 @@ public class SecuritySettingsTest extends RegistrarSettingsActionTestCase {
     assertThat(registrar.getFailoverClientCertificate()).isNull();
     assertThat(registrar.getFailoverClientCertificateHash()).isNull();
     assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
-    verifyContactsNotified();
+    verifyNotificationEmailsSent();
   }
 
   @Test
@@ -99,7 +99,7 @@ public class SecuritySettingsTest extends RegistrarSettingsActionTestCase {
     assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT2);
     assertThat(registrar.getFailoverClientCertificateHash()).isEqualTo(SAMPLE_CERT2_HASH);
     assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
-    verifyContactsNotified();
+    verifyNotificationEmailsSent();
   }
 
   @Test

javatests/google/registry/ui/server/registrar/WhoisSettingsTest.java

@@ -61,7 +61,7 @@ public class WhoisSettingsTest extends RegistrarSettingsActionTestCase {
     assertThat(response.get("results")).isEqualTo(ImmutableList.of(modified.toJsonMap()));
     assertThat(loadRegistrar(CLIENT_ID)).isEqualTo(modified);
     assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
-    verifyContactsNotified();
+    verifyNotificationEmailsSent();
   }
 
   @Test

javatests/google/registry/ui/server/registrar/testdata/update_registrar.json

@@ -12,7 +12,7 @@
       {
         "visibleInWhoisAsAdmin": true,
         "faxNumber": null,
-        "phoneNumber": null,
+        "phoneNumber": "+1.2345678901",
         "name": "Extra Terrestrial",
         "visibleInWhoisAsTech": false,
         "emailAddress": "etphonehome@example.com",

javatests/google/registry/ui/server/registrar/testdata/update_registrar_email.txt

@@ -11,8 +11,9 @@ emailAddress: the.registrar@example.com -> thase@the.registrar
 url: http://my.fake.url -> http://my.new.url
 contacts:
     ADDED:
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, phoneNumber=null, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false}
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false}
     REMOVED:
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=John Doe, emailAddress=johndoe@theregistrar.com, phoneNumber=+1.1234567890, faxNumber=null, types=[ADMIN], gaeUserId=31337, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false}
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=John Doe, emailAddress=johndoe@theregistrar.com, phoneNumber=+1.1234567890, faxNumber=null, types=[ADMIN], gaeUserId=31337, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false},
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Jian-Yang, emailAddress=jyang@bachman.accelerator, phoneNumber=+1.1234567890, faxNumber=null, types=[TECH], gaeUserId=null, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false}
     FINAL CONTENTS:
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, phoneNumber=null, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false}
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false}