diff --git a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java index 0c28d18f7..cd28a0d86 100644 --- a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java +++ b/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 oldContactsByType = HashMultimap.create(); + Multimap oldContactsByType = HashMultimap.create(); for (RegistrarContact contact : existingContacts) { - for (RegistrarContact.Type t : contact.getTypes()) { + for (Type t : contact.getTypes()) { oldContactsByType.put(t, contact); } } - Multimap newContactsByType = HashMultimap.create(); + Multimap 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 - : difference(oldContactsByType.keySet(), newContactsByType.keySet())) { + for (Type t : 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 oldContactsByType, - Multimap newContactsByType, - RegistrarContact.Type... types) { - for (RegistrarContact.Type type : types) { + Multimap oldContactsByType, + Multimap newContactsByType, + 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) { - super("Must have at least one " + type.getDisplayName() + " contact"); + ContactRequirementException(Type type) { + super(String.format("Must have at least one %s contact", type.getDisplayName())); } } } diff --git a/javatests/google/registry/ui/server/registrar/ContactSettingsTest.java b/javatests/google/registry/ui/server/registrar/ContactSettingsTest.java index 3cb98b90b..7e75e1dc5 100644 --- a/javatests/google/registry/ui/server/registrar/ContactSettingsTest.java +++ b/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 adminContact1 = new HashMap<>(); - adminContact1.put("name", "contact1"); - adminContact1.put("emailAddress", "contact1@email.com"); - adminContact1.put("phoneNumber", "+1.2125650001"); - // Have to keep ADMIN or else expect FormException for at-least-one. - adminContact1.put("types", "ADMIN"); + ImmutableMap adminContact1 = + ImmutableMap.of( + "name", "contact1", + "emailAddress", "contact1@email.com", + "phoneNumber", "+1.2125650001", + // Have to keep ADMIN or else expect FormException for at-least-one. + "types", "ADMIN,TECH"); Registrar registrar = loadRegistrar(CLIENT_ID); Map 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() - .setParent(registrar) - .setName((String) adminContact1.get("name")) - .setEmailAddress((String) adminContact1.get("emailAddress")) - .setPhoneNumber((String) adminContact1.get("phoneNumber")) - .setTypes(ImmutableList.of(RegistrarContact.Type.ADMIN)) - .build(); + RegistrarContact newContact = + new RegistrarContact.Builder() + .setParent(registrar) + .setName(adminContact1.get("name")) + .setEmailAddress(adminContact1.get("emailAddress")) + .setPhoneNumber(adminContact1.get("phoneNumber")) + .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 reqJson = loadRegistrar(CLIENT_ID).toJsonMap(); - reqJson.put("contacts", - ImmutableList.of(AppEngineRule.makeRegistrarContact2() - .asBuilder() - .setTypes(ImmutableList.of()) - .build().toJsonMap())); - Map response = action.handleJsonRequest(ImmutableMap.of( - "op", "update", - "id", CLIENT_ID, - "args", reqJson)); + reqJson.put("contacts", ImmutableList.of(techContact.toJsonMap())); + Map response = + action.handleJsonRequest( + ImmutableMap.of( + "op", "update", + "id", CLIENT_ID, + "args", reqJson)); assertThat(response).containsEntry("status", "ERROR"); - assertThat(response).containsEntry("message", "Must have at least one " - + RegistrarContact.Type.ADMIN.getDisplayName() + " contact"); + assertThat(response).containsEntry("message", "Must have at least one primary contact"); assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: ContactRequirementException"); } @Test public void testPost_updateContacts_requireTechPhone_error() { - // First make the contact a tech contact as well. - Registrar registrar = loadRegistrar(CLIENT_ID); - RegistrarContact rc = AppEngineRule.makeRegistrarContact2() - .asBuilder() - .setTypes(ImmutableSet.of(RegistrarContact.Type.ADMIN, RegistrarContact.Type.TECH)) - .build(); - // Lest we anger the timestamp inversion bug. - // (we also update the registrar so we get the timestamp right) - registrar = persistResource(registrar); - persistSimpleResource(rc); - - // Now try to remove the phone number. - rc = rc.asBuilder().setPhoneNumber(null).build(); - Map reqJson = registrar.toJsonMap(); - reqJson.put("contacts", ImmutableList.of(rc.toJsonMap())); - Map response = action.handleJsonRequest(ImmutableMap.of( - "op", "update", - "id", CLIENT_ID, - "args", reqJson)); + Map reqJson = loadRegistrar(CLIENT_ID).toJsonMap(); + reqJson.put( + "contacts", + ImmutableList.of( + AppEngineRule.makeRegistrarContact2().toJsonMap(), + techContact.asBuilder().setPhoneNumber(null).build().toJsonMap())); + Map 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 " - + RegistrarContact.Type.TECH.getDisplayName() + " contact"); + assertThat(response) + .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 reqJson = registrar.toJsonMap(); - reqJson.put("contacts", ImmutableList.of(rc.toJsonMap())); + reqJson.put("contacts", ImmutableList.of(rc.toJsonMap(), techContact.toJsonMap())); Map 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 reqJson = registrar.toJsonMap(); - reqJson.put("contacts", ImmutableList.of(rc.toJsonMap())); + reqJson.put("contacts", ImmutableList.of(rc.toJsonMap(), techContact.toJsonMap())); Map response = action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", reqJson)); assertThat(response).containsEntry("status", "ERROR"); diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java index cfbe6c122..7476943e8 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java +++ b/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 contentCaptor = ArgumentCaptor.forClass(EmailMessage.class); verify(emailService).sendEmail(contentCaptor.capture()); assertThat(contentCaptor.getValue().body()).isEqualTo(expectedEmailBody); diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java index ed1d75fbc..267e79c80 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java +++ b/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. - createTlds("currenttld"); - // Create another tld but remove access for registrar "TheRegistrar". This is for the test case - // of updating allowed tld for registrar - createTlds("newtld"); + // Registrar "TheRegistrar" has access to TLD "currenttld" but not to "newtld". + createTlds("currenttld", "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 captor = ArgumentCaptor.forClass(EmailMessage.class); verify(emailService).sendEmail(captor.capture()); Truth.assertThat(captor.getValue().recipients()) diff --git a/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java b/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java index 89e55bcd8..ffe78b467 100644 --- a/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java +++ b/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 diff --git a/javatests/google/registry/ui/server/registrar/WhoisSettingsTest.java b/javatests/google/registry/ui/server/registrar/WhoisSettingsTest.java index 9b7c6ec63..1814853ef 100644 --- a/javatests/google/registry/ui/server/registrar/WhoisSettingsTest.java +++ b/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 diff --git a/javatests/google/registry/ui/server/registrar/testdata/update_registrar.json b/javatests/google/registry/ui/server/registrar/testdata/update_registrar.json index a4508e3c4..c72f2dbcc 100644 --- a/javatests/google/registry/ui/server/registrar/testdata/update_registrar.json +++ b/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", diff --git a/javatests/google/registry/ui/server/registrar/testdata/update_registrar_email.txt b/javatests/google/registry/ui/server/registrar/testdata/update_registrar_email.txt index ad28d8b6e..630877de9 100644 --- a/javatests/google/registry/ui/server/registrar/testdata/update_registrar_email.txt +++ b/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}