From 3bbf356497806e9a77a9ff055747a3002d557516 Mon Sep 17 00:00:00 2001 From: mountford Date: Thu, 1 Jun 2017 14:50:45 -0700 Subject: [PATCH] Fix bug in registrar contact nomulus command The command was set up such that an update without any contact types specified would clear out the list, instead of leaving them unchanged, as it should. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157766429 --- .../tools/RegistrarContactCommand.java | 32 +++++-- .../tools/RegistrarContactCommandTest.java | 92 +++++++++++++++++++ 2 files changed, 115 insertions(+), 9 deletions(-) diff --git a/java/google/registry/tools/RegistrarContactCommand.java b/java/google/registry/tools/RegistrarContactCommand.java index a0eb2d972..7e3e78637 100644 --- a/java/google/registry/tools/RegistrarContactCommand.java +++ b/java/google/registry/tools/RegistrarContactCommand.java @@ -18,7 +18,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Strings.isNullOrEmpty; import static com.google.common.collect.Iterables.transform; -import static com.google.common.collect.Sets.newHashSet; import static google.registry.util.CollectionUtils.nullToEmpty; import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; import static java.nio.charset.StandardCharsets.UTF_8; @@ -144,7 +143,7 @@ final class RegistrarContactCommand extends MutatingCommand { ImmutableSet.of(Mode.CREATE, Mode.UPDATE, Mode.DELETE); @Nullable - private Set contactTypes; + private ImmutableSet contactTypes; @Override protected void init() throws Exception { @@ -153,10 +152,21 @@ final class RegistrarContactCommand extends MutatingCommand { String clientId = mainParameters.get(0); Registrar registrar = checkNotNull(Registrar.loadByClientId(clientId), "Registrar %s not found", clientId); - contactTypes = - newHashSet( - transform( - nullToEmpty(contactTypeNames), Enums.stringConverter(RegistrarContact.Type.class))); + // If the contact_type parameter is not specified, we should not make any changes. + if (contactTypeNames == null) { + contactTypes = null; + // It appears that when the user specifies "--contact_type=" with no types following, JCommander + // sets contactTypeNames to a one-element list containing the empty string. This is strange, but + // we need to handle this by setting the contact types to the empty set. Also do this if + // contactTypeNames is empty, which is what I would hope JCommander would return in some future, + // better world. + } else if (contactTypeNames.isEmpty() + || ((contactTypeNames.size() == 1) && contactTypeNames.get(0).isEmpty())) { + contactTypes = ImmutableSet.of(); + } else { + contactTypes = ImmutableSet.copyOf( + transform(contactTypeNames, Enums.stringConverter(RegistrarContact.Type.class))); + } ImmutableSet contacts = registrar.getContacts(); Map contactsMap = new LinkedHashMap<>(); for (RegistrarContact rc : contacts) { @@ -169,7 +179,9 @@ final class RegistrarContactCommand extends MutatingCommand { break; case CREATE: stageEntityChange(null, createContact(registrar)); - unsetOtherWhoisAbuseFlags(contacts, null); + if ((visibleInDomainWhoisAsAbuse != null) && visibleInDomainWhoisAsAbuse.booleanValue()) { + unsetOtherWhoisAbuseFlags(contacts, null /* emailAddressNotToChange */ ); + } break; case UPDATE: oldContact = @@ -184,7 +196,9 @@ final class RegistrarContactCommand extends MutatingCommand { "Cannot clear visible_in_domain_whois_as_abuse flag, as that would leave no domain" + " WHOIS abuse contacts; instead, set the flag on another contact"); stageEntityChange(oldContact, newContact); - unsetOtherWhoisAbuseFlags(contacts, oldContact.getEmailAddress()); + if ((visibleInDomainWhoisAsAbuse != null) && visibleInDomainWhoisAsAbuse.booleanValue()) { + unsetOtherWhoisAbuseFlags(contacts, oldContact.getEmailAddress()); + } break; case DELETE: oldContact = @@ -226,7 +240,7 @@ final class RegistrarContactCommand extends MutatingCommand { if (fax != null) { builder.setFaxNumber(fax.orNull()); } - builder.setTypes(contactTypes); + builder.setTypes(nullToEmpty(contactTypes)); if (Objects.equals(allowConsoleAccess, Boolean.TRUE)) { builder.setGaeUserId(checkArgumentNotNull( diff --git a/javatests/google/registry/tools/RegistrarContactCommandTest.java b/javatests/google/registry/tools/RegistrarContactCommandTest.java index 7a2f7d2c0..334ac7129 100644 --- a/javatests/google/registry/tools/RegistrarContactCommandTest.java +++ b/javatests/google/registry/tools/RegistrarContactCommandTest.java @@ -17,6 +17,7 @@ package google.registry.tools; import static com.google.common.truth.Truth.assertThat; import static google.registry.model.registrar.RegistrarContact.Type.ABUSE; import static google.registry.model.registrar.RegistrarContact.Type.ADMIN; +import static google.registry.model.registrar.RegistrarContact.Type.TECH; import static google.registry.model.registrar.RegistrarContact.Type.WHOIS; import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.DatastoreHelper.persistSimpleResource; @@ -208,6 +209,85 @@ public class RegistrarContactCommandTest extends CommandTestCase