From 6a8b25360c93a804274424d1fdfe8d3a0fd7071e Mon Sep 17 00:00:00 2001 From: nickfelt Date: Wed, 15 Mar 2017 15:11:30 -0700 Subject: [PATCH] Use StatusValue XML names in EPP error messages This changes ResourceStatusProhibitsOperationException so that we print out the list of StatusValues using their XML names rather than the literal enum name, i.e. we use "pendingDelete" rather than "PENDING_DELETE". This seems more correct given that EPP clients will be used to seeing the status values in the XML representation, and it also matches the existing ResourceHasClientUpdateProhibitedException that hardcodes "clientUpdateProhibited": http://[]/third_party/java_src/gtld/java/google/registry/flows/exceptions/ResourceHasClientUpdateProhibitedException.java?l=22&rcl=146111211 Also reorganized related test methods and added some missing tests, including for ContactTransferRequestFlow which previously had none. I also renamed the "clientProhibitedStatusValue" tests to instead say "statusValueNotClientSettable" to be clearer about what's being tested, and that it's not related to the "clientXXProhibited" statuses. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=150248562 --- docs/flows.md | 2 + .../contact/ContactTransferRequestFlow.java | 1 + ...urceStatusProhibitsOperationException.java | 14 +++- .../flows/contact/ContactDeleteFlowTest.java | 20 ++--- .../ContactTransferRequestFlowTest.java | 26 ++++++ .../flows/contact/ContactUpdateFlowTest.java | 44 ++++++---- .../DomainApplicationUpdateFlowTest.java | 2 +- .../flows/domain/DomainDeleteFlowTest.java | 6 +- .../flows/domain/DomainRenewFlowTest.java | 26 +++--- .../domain/DomainTransferRequestFlowTest.java | 20 ++++- .../flows/domain/DomainUpdateFlowTest.java | 54 ++++++------ .../flows/host/HostDeleteFlowTest.java | 20 ++--- .../flows/host/HostUpdateFlowTest.java | 82 +++++++++++-------- 13 files changed, 197 insertions(+), 120 deletions(-) diff --git a/docs/flows.md b/docs/flows.md index fde936085..daafc0ab3 100644 --- a/docs/flows.md +++ b/docs/flows.md @@ -50,6 +50,8 @@ or rejected, and the gaining registrar can also cancel the transfer request. * The resource is already pending transfer. * 2303 * Resource with this id does not exist. +* 2304 + * Resource status prohibits this operation. ## ContactTransferRejectFlow diff --git a/java/google/registry/flows/contact/ContactTransferRequestFlow.java b/java/google/registry/flows/contact/ContactTransferRequestFlow.java index 32716ddc5..6c1fc3308 100644 --- a/java/google/registry/flows/contact/ContactTransferRequestFlow.java +++ b/java/google/registry/flows/contact/ContactTransferRequestFlow.java @@ -64,6 +64,7 @@ import org.joda.time.Duration; * @error {@link google.registry.flows.exceptions.AlreadyPendingTransferException} * @error {@link google.registry.flows.exceptions.MissingTransferRequestAuthInfoException} * @error {@link google.registry.flows.exceptions.ObjectAlreadySponsoredException} + * @error {@link google.registry.flows.exceptions.ResourceStatusProhibitsOperationException} */ public final class ContactTransferRequestFlow implements TransactionalFlow { diff --git a/java/google/registry/flows/exceptions/ResourceStatusProhibitsOperationException.java b/java/google/registry/flows/exceptions/ResourceStatusProhibitsOperationException.java index 6570753d8..2765a2a0b 100644 --- a/java/google/registry/flows/exceptions/ResourceStatusProhibitsOperationException.java +++ b/java/google/registry/flows/exceptions/ResourceStatusProhibitsOperationException.java @@ -15,6 +15,7 @@ package google.registry.flows.exceptions; import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableSet; import google.registry.flows.EppException.StatusProhibitsOperationException; import google.registry.model.eppcommon.StatusValue; import java.util.Set; @@ -22,7 +23,16 @@ import java.util.Set; /** Resource status prohibits this operation. */ public class ResourceStatusProhibitsOperationException extends StatusProhibitsOperationException { - public ResourceStatusProhibitsOperationException(Set status) { - super("Operation disallowed by status: " + Joiner.on(", ").join(status)); + public ResourceStatusProhibitsOperationException(Set statuses) { + super("Operation disallowed by status: " + formatStatusValues(statuses)); + } + + /** Returns a human-readable string listing the XML names of the given status values. */ + private static String formatStatusValues(Set statuses) { + ImmutableSet.Builder statusXmlNames = new ImmutableSet.Builder<>(); + for (StatusValue status : statuses) { + statusXmlNames.add(status.getXmlName()); + } + return Joiner.on(", ").join(statusXmlNames.build()); } } diff --git a/javatests/google/registry/flows/contact/ContactDeleteFlowTest.java b/javatests/google/registry/flows/contact/ContactDeleteFlowTest.java index 9d3b97e34..7e0db27fa 100644 --- a/javatests/google/registry/flows/contact/ContactDeleteFlowTest.java +++ b/javatests/google/registry/flows/contact/ContactDeleteFlowTest.java @@ -44,16 +44,6 @@ public class ContactDeleteFlowTest setEppInput("contact_delete.xml"); } - private void doFailingStatusTest(StatusValue statusValue, Class exception) - throws Exception { - persistResource( - newContactResource(getUniqueIdFromCommand()).asBuilder() - .setStatusValues(ImmutableSet.of(statusValue)) - .build()); - thrown.expect(exception); - runFlow(); - } - @Test public void testDryRun() throws Exception { persistActiveContact(getUniqueIdFromCommand()); @@ -92,6 +82,16 @@ public class ContactDeleteFlowTest runFlow(); } + private void doFailingStatusTest(StatusValue statusValue, Class exception) + throws Exception { + persistResource( + newContactResource(getUniqueIdFromCommand()).asBuilder() + .setStatusValues(ImmutableSet.of(statusValue)) + .build()); + thrown.expect(exception, statusValue.getXmlName()); + runFlow(); + } + @Test public void testFailure_existedButWasClientDeleteProhibited() throws Exception { doFailingStatusTest( diff --git a/javatests/google/registry/flows/contact/ContactTransferRequestFlowTest.java b/javatests/google/registry/flows/contact/ContactTransferRequestFlowTest.java index 00e8863ab..306d4491f 100644 --- a/javatests/google/registry/flows/contact/ContactTransferRequestFlowTest.java +++ b/javatests/google/registry/flows/contact/ContactTransferRequestFlowTest.java @@ -28,9 +28,11 @@ import google.registry.flows.ResourceFlowUtils.ResourceDoesNotExistException; import google.registry.flows.exceptions.AlreadyPendingTransferException; import google.registry.flows.exceptions.MissingTransferRequestAuthInfoException; import google.registry.flows.exceptions.ObjectAlreadySponsoredException; +import google.registry.flows.exceptions.ResourceStatusProhibitsOperationException; import google.registry.model.contact.ContactAuthInfo; import google.registry.model.contact.ContactResource; import google.registry.model.eppcommon.AuthInfo.PasswordAuth; +import google.registry.model.eppcommon.StatusValue; import google.registry.model.reporting.HistoryEntry; import google.registry.model.transfer.TransferStatus; import org.joda.time.DateTime; @@ -187,4 +189,28 @@ public class ContactTransferRequestFlowTest String.format("(%s)", getUniqueIdFromCommand())); doFailingTest("contact_transfer_request.xml"); } + + @Test + public void testFailure_clientTransferProhibited() throws Exception { + contact = persistResource( + contact.asBuilder().addStatusValue(StatusValue.CLIENT_TRANSFER_PROHIBITED).build()); + thrown.expect(ResourceStatusProhibitsOperationException.class, "clientTransferProhibited"); + doFailingTest("contact_transfer_request.xml"); + } + + @Test + public void testFailure_serverTransferProhibited() throws Exception { + contact = persistResource( + contact.asBuilder().addStatusValue(StatusValue.SERVER_TRANSFER_PROHIBITED).build()); + thrown.expect(ResourceStatusProhibitsOperationException.class, "serverTransferProhibited"); + doFailingTest("contact_transfer_request.xml"); + } + + @Test + public void testFailure_pendingDelete() throws Exception { + contact = persistResource( + contact.asBuilder().addStatusValue(StatusValue.PENDING_DELETE).build()); + thrown.expect(ResourceStatusProhibitsOperationException.class, "pendingDelete"); + doFailingTest("contact_transfer_request.xml"); + } } diff --git a/javatests/google/registry/flows/contact/ContactUpdateFlowTest.java b/javatests/google/registry/flows/contact/ContactUpdateFlowTest.java index 911bb344d..4bb157dec 100644 --- a/javatests/google/registry/flows/contact/ContactUpdateFlowTest.java +++ b/javatests/google/registry/flows/contact/ContactUpdateFlowTest.java @@ -71,18 +71,6 @@ public class ContactUpdateFlowTest doSuccessfulTest(); } - @Test - public void testSuccess_removeClientUpdateProhibited() throws Exception { - setEppInput("contact_update_remove_client_update_prohibited.xml"); - persistResource( - newContactResource(getUniqueIdFromCommand()).asBuilder() - .setStatusValues(ImmutableSet.of(StatusValue.CLIENT_UPDATE_PROHIBITED)) - .build()); - doSuccessfulTest(); - assertAboutContacts().that(reloadResourceByForeignKey()) - .doesNotHaveStatusValue(StatusValue.CLIENT_UPDATE_PROHIBITED); - } - @Test public void testSuccess_updatingInternationalizedPostalInfoDeletesLocalized() throws Exception { ContactResource contact = @@ -281,7 +269,7 @@ public class ContactUpdateFlowTest } @Test - public void testFailure_clientProhibitedStatusValue() throws Exception { + public void testFailure_statusValueNotClientSettable() throws Exception { setEppInput("contact_update_prohibited_status.xml"); persistActiveContact(getUniqueIdFromCommand()); thrown.expect(StatusNotClientSettableException.class); @@ -289,7 +277,7 @@ public class ContactUpdateFlowTest } @Test - public void testSuccess_superuserClientProhibitedStatusValue() throws Exception { + public void testSuccess_superuserStatusValueNotClientSettable() throws Exception { setEppInput("contact_update_prohibited_status.xml"); persistActiveContact(getUniqueIdFromCommand()); clock.advanceOneMilli(); @@ -317,7 +305,19 @@ public class ContactUpdateFlowTest } @Test - public void testSuccess_superuserClientUpdateProhibited() throws Exception { + public void testSuccess_clientUpdateProhibited_removed() throws Exception { + setEppInput("contact_update_remove_client_update_prohibited.xml"); + persistResource( + newContactResource(getUniqueIdFromCommand()).asBuilder() + .setStatusValues(ImmutableSet.of(StatusValue.CLIENT_UPDATE_PROHIBITED)) + .build()); + doSuccessfulTest(); + assertAboutContacts().that(reloadResourceByForeignKey()) + .doesNotHaveStatusValue(StatusValue.CLIENT_UPDATE_PROHIBITED); + } + + @Test + public void testSuccess_superuserClientUpdateProhibited_notRemoved() throws Exception { setEppInput("contact_update_prohibited_status.xml"); persistResource( newContactResource(getUniqueIdFromCommand()).asBuilder() @@ -334,7 +334,7 @@ public class ContactUpdateFlowTest } @Test - public void testFailure_clientUpdateProhibited() throws Exception { + public void testFailure_clientUpdateProhibited_notRemoved() throws Exception { persistResource( newContactResource(getUniqueIdFromCommand()).asBuilder() .setStatusValues(ImmutableSet.of(StatusValue.CLIENT_UPDATE_PROHIBITED)) @@ -349,7 +349,17 @@ public class ContactUpdateFlowTest newContactResource(getUniqueIdFromCommand()).asBuilder() .setStatusValues(ImmutableSet.of(StatusValue.SERVER_UPDATE_PROHIBITED)) .build()); - thrown.expect(ResourceStatusProhibitsOperationException.class); + thrown.expect(ResourceStatusProhibitsOperationException.class, "serverUpdateProhibited"); + runFlow(); + } + + @Test + public void testFailure_pendingDeleteProhibited() throws Exception { + persistResource( + newContactResource(getUniqueIdFromCommand()).asBuilder() + .setStatusValues(ImmutableSet.of(StatusValue.PENDING_DELETE)) + .build()); + thrown.expect(ResourceStatusProhibitsOperationException.class, "pendingDelete"); runFlow(); } diff --git a/javatests/google/registry/flows/domain/DomainApplicationUpdateFlowTest.java b/javatests/google/registry/flows/domain/DomainApplicationUpdateFlowTest.java index 090bdbd7b..df3524965 100644 --- a/javatests/google/registry/flows/domain/DomainApplicationUpdateFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainApplicationUpdateFlowTest.java @@ -444,7 +444,7 @@ public class DomainApplicationUpdateFlowTest persistReferencedEntities(); persistResource(newApplicationBuilder().setStatusValues( ImmutableSet.of(StatusValue.SERVER_UPDATE_PROHIBITED)).build()); - thrown.expect(ResourceStatusProhibitsOperationException.class); + thrown.expect(ResourceStatusProhibitsOperationException.class, "serverUpdateProhibited"); runFlow(); } diff --git a/javatests/google/registry/flows/domain/DomainDeleteFlowTest.java b/javatests/google/registry/flows/domain/DomainDeleteFlowTest.java index 1dad39588..aa3a0f4c3 100644 --- a/javatests/google/registry/flows/domain/DomainDeleteFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainDeleteFlowTest.java @@ -690,7 +690,7 @@ public class DomainDeleteFlowTest extends ResourceFlowTestCase exception) - throws Exception { - persistResource( - newHostResource("ns1.example.tld").asBuilder() - .setStatusValues(ImmutableSet.of(statusValue)) - .build()); - thrown.expect(exception); - runFlow(); - } - @Test public void testDryRun() throws Exception { persistActiveHost("ns1.example.tld"); @@ -99,6 +89,16 @@ public class HostDeleteFlowTest extends ResourceFlowTestCase exception) + throws Exception { + persistResource( + newHostResource("ns1.example.tld").asBuilder() + .setStatusValues(ImmutableSet.of(statusValue)) + .build()); + thrown.expect(exception, statusValue.getXmlName()); + runFlow(); + } + @Test public void testFailure_existedButWasClientDeleteProhibited() throws Exception { doFailingStatusTest( diff --git a/javatests/google/registry/flows/host/HostUpdateFlowTest.java b/javatests/google/registry/flows/host/HostUpdateFlowTest.java index ed5d6373b..8f60c260b 100644 --- a/javatests/google/registry/flows/host/HostUpdateFlowTest.java +++ b/javatests/google/registry/flows/host/HostUpdateFlowTest.java @@ -152,20 +152,6 @@ public class HostUpdateFlowTest extends ResourceFlowTestCase