Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept i18n errors #684

Merged
merged 9 commits into from
Oct 25, 2017
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ protected final void saveFormError(final Form<?> form, final String message) {
}

protected final void saveUnexpectedFormError(final Form<?> form, final Throwable throwable, final Logger logger) {
form.reject("Something went wrong, please try again"); // TODO i18n
form.reject("messages:defaultError");
logger.error("The CTP request raised an unexpected exception", throwable);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ private boolean isFalsy(@Nullable final String value) {
return value != null && value.equals("false");
}

ErrorsBean extractErrors(@Nullable final Form<?> form) {
ErrorsBean extractErrors(@Nullable final Form<?> form) {
final ErrorsBean errorsBean = new ErrorsBean();
final List<ErrorBean> errorList = new ArrayList<>();
if (form != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
import com.google.inject.ImplementedBy;
import play.data.validation.ValidationError;

import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.stream.IntStream;

/**
* Allows to format errors according to the available locales and error information.
Expand All @@ -18,16 +21,22 @@ public interface ErrorFormatter {
* @param message error message
* @return the error message localized and formatted
*/
String format(final List<Locale> locales, final String message);
String format(final List<Locale> locales, final String message, final Map<String, Object> hashArgs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer the parameter name args here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I named them hashArgs here and in I18nResolver because they are hash arguments. What don't you like in particular from the name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the type of hashArgs is just Map, I was wondering how useful it is to mention hash in the parameter name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!


/**
* Formats the Play error message somehow, with the translation to the first available locale.
* As hash arguments, it defines the field key as "field" and all other arguments as its index, e.g. "0", "1", "2".
* @param locales current given locales
* @param error Play's validation error
* @return the error message localized and formatted
*/
default String format(final List<Locale> locales, final ValidationError error) {
final String message = format(locales, error.message());
return !error.key().isEmpty() ? message + ": " + error.key() : message;
final Map<String, Object> hashArgs = new HashMap<>();
if (error.key() != null && !error.key().isEmpty()) {
hashArgs.put("field", error.key());
}
IntStream.range(0, error.arguments().size())
.forEach(index -> hashArgs.put(String.valueOf(index), error.arguments().get(index)));
return format(locales, error.message(), hashArgs);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,22 @@
import javax.inject.Inject;
import java.util.List;
import java.util.Locale;
import java.util.Map;

class ErrorFormatterImpl implements ErrorFormatter {

private final I18nResolver i18nResolver;
private final I18nIdentifierFactory i18nIdentifierFactory;

@Inject
private I18nResolver i18nResolver;
@Inject
private I18nIdentifierFactory i18nIdentifierFactory;
ErrorFormatterImpl(final I18nResolver i18nResolver, final I18nIdentifierFactory i18nIdentifierFactory) {
this.i18nResolver = i18nResolver;
this.i18nIdentifierFactory = i18nIdentifierFactory;
}

@Override
public String format(final List<Locale> locales, final String messageKey) {
public String format(final List<Locale> locales, final String messageKey, final Map<String, Object> hashArgs) {
final I18nIdentifier i18nIdentifier = i18nIdentifierFactory.create(messageKey);
return i18nResolver.getOrKey(locales, i18nIdentifier);
return i18nResolver.getOrKey(locales, i18nIdentifier, hashArgs);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,19 @@

import com.commercetools.sunrise.common.forms.ErrorBean;
import com.commercetools.sunrise.common.forms.ErrorsBean;
import com.commercetools.sunrise.common.utils.ErrorFormatter;
import com.google.common.collect.Maps;
import org.junit.Test;
import org.mockito.Mockito;
import play.data.Form;
import play.data.validation.ValidationError;

import java.util.*;
import java.util.function.Predicate;

import static java.util.Collections.singletonList;
import static org.assertj.core.api.Assertions.assertThat;

public class PlayJavaFormResolverTest {

PlayJavaFormResolver formResolver = new PlayJavaFormResolver(singletonList(Locale.ENGLISH), (locales, message) ->
PlayJavaFormResolver formResolver = new PlayJavaFormResolver(singletonList(Locale.ENGLISH), (locales, message, args) ->
message);

@Test
Expand All @@ -35,7 +32,7 @@ public void extractErrors() throws Exception {

private void checkError(ErrorBean error, String field, String key, String message) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't check it in the IDE, but to me it looks the key parameter is unused?

assertThat(error.getField()).isEqualTo(field);
assertThat(error.getMessage()).isEqualTo(message + ": " + key);
assertThat(error.getMessage()).isEqualTo(message);

}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package com.commercetools.sunrise.common.utils;

import com.commercetools.sunrise.common.template.i18n.I18nIdentifier;
import com.commercetools.sunrise.common.template.i18n.I18nIdentifierFactory;
import com.commercetools.sunrise.common.template.i18n.I18nResolver;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.Spy;
import org.mockito.junit.MockitoJUnitRunner;

import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Optional;

import static java.util.Collections.*;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.*;
import static org.mockito.Mockito.when;

@RunWith(MockitoJUnitRunner.class)
public class ErrorFormatterImplTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the new tests 👍


private static final List<Locale> LOCALES = singletonList(Locale.ENGLISH);
private static final String MESSAGE_KEY = "error.required";
private static final I18nIdentifier I18N_IDENTIFIER = I18nIdentifier.of("main", MESSAGE_KEY);
private static final Map<String, Object> ARGS_WITH_FIELD = singletonMap("field", "username");

@Mock
private I18nResolver i18nResolver;
@Spy
private I18nIdentifierFactory i18nIdentifierFactory;

@InjectMocks
private ErrorFormatterImpl errorFormatter;

@Before
public void setUp() throws Exception {
when(i18nResolver.getOrKey(any(), any(), any())).thenCallRealMethod();
}

@Test
public void translatesMessageKey() throws Exception {
mockI18nResolverWithError();
final String errorMessage = errorFormatter.format(LOCALES, MESSAGE_KEY, emptyMap());
assertThat(errorMessage).isEqualTo("Required field");
}

@Test
public void returnsMessageKeyWhenNoMatch() throws Exception {
mockI18nResolverWithoutError();
final String errorMessage = errorFormatter.format(LOCALES, MESSAGE_KEY, emptyMap());
assertThat(errorMessage).isEqualTo(MESSAGE_KEY);
}

@Test
public void returnsMessageKeyWithFieldIfProvided() throws Exception {
mockI18nResolverWithError();
final String errorMessage = errorFormatter.format(LOCALES, MESSAGE_KEY, ARGS_WITH_FIELD);
assertThat(errorMessage).isEqualTo("Required field: username");
}

private void mockI18nResolverWithoutError() {
when(i18nResolver.get(any(), eq(I18N_IDENTIFIER), anyMap())).thenReturn(Optional.empty());
}

private void mockI18nResolverWithError() {
when(i18nResolver.get(any(), eq(I18N_IDENTIFIER), anyMap())).thenReturn(Optional.of("Required field"));
when(i18nResolver.get(any(), eq(I18N_IDENTIFIER), eq(singletonMap("field", "username")))).thenReturn(Optional.of("Required field: username"));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package com.commercetools.sunrise.common.utils;

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;
import play.data.validation.ValidationError;

import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.function.Consumer;

import static java.util.Arrays.asList;
import static java.util.Collections.*;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

@RunWith(MockitoJUnitRunner.class)
public class ErrorFormatterTest {

private static final List<Locale> LOCALES = singletonList(Locale.ENGLISH);
private static final String MESSAGE_KEY = "error.required";
private static final String SOME_ERROR_MESSAGE = "errorMessage";

@Mock
private ErrorFormatter errorFormatter;

@Before
public void setUp() throws Exception {
when(errorFormatter.format(any(), any(), any())).thenReturn(SOME_ERROR_MESSAGE);
when(errorFormatter.format(any(), any())).thenCallRealMethod();
}

@Test
public void transformsToMessage() throws Exception {
final ValidationError validationError = new ValidationError("", MESSAGE_KEY);
test(validationError, errorFormatter ->
verify(errorFormatter).format(LOCALES, MESSAGE_KEY, emptyMap()));
}

@Test
public void transformsToMessageWithField() throws Exception {
final ValidationError validationError = new ValidationError("username", MESSAGE_KEY);
test(validationError, errorFormatter ->
verify(errorFormatter).format(LOCALES, MESSAGE_KEY, singletonMap("field", "username")));
}

@Test
public void transformsToMessageWithArgs() throws Exception {
final ValidationError validationError = new ValidationError("", MESSAGE_KEY, asList("arg1", "arg2", "arg3"));
test(validationError, errorFormatter -> {
final Map<String, Object> hashArgs = new HashMap<>();
hashArgs.put("0", "arg1");
hashArgs.put("1", "arg2");
hashArgs.put("2", "arg3");
verify(errorFormatter).format(LOCALES, MESSAGE_KEY, hashArgs);
});
}

@Test
public void transformsToMessageWithFieldAndArgs() throws Exception {
final ValidationError validationError = new ValidationError("username", MESSAGE_KEY, asList("arg1", "arg2", "arg3"));
test(validationError, errorFormatter -> {
final Map<String, Object> hashArgs = new HashMap<>();
hashArgs.put("field", "username");
hashArgs.put("0", "arg1");
hashArgs.put("1", "arg2");
hashArgs.put("2", "arg3");
verify(errorFormatter).format(LOCALES, MESSAGE_KEY, hashArgs);
});
}

private void test(final ValidationError validationError, final Consumer<ErrorFormatter> test) {
final String errorMessage = errorFormatter.format(LOCALES, validationError);
assertThat(errorMessage).isEqualTo(SOME_ERROR_MESSAGE);
test.accept(errorFormatter);
}
}
2 changes: 1 addition & 1 deletion conf/application.conf
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ application.currencies = ${?CURRENCIES}
application.i18n.languages = ${?LANGUAGES}

# List of i18n bundles to load.
application.i18n.bundles = ["sunrise", "main", "banner", "catalog", "checkout", "my-account-login", "my-account"]
application.i18n.bundles = ["sunrise", "main", "banner", "catalog", "checkout", "my-account-login", "my-account", "messages"]

# If you want to change the way i18n messages are resolved, you can change the list of resolver loaders you want to use.
# Keep in mind that the list order determines the order in which the resolvers are going to be invoked for each message.
Expand Down
24 changes: 24 additions & 0 deletions conf/i18n/de/messages.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
defaultError: Something went wrong, please try again
checkout:
payment:
invalidPayment: Invalid payment
addresses:
invalidShippingCountry: Invalid shipping country
invalidBillingCountry: Invalid billing country
myAccount:
logIn:
invalidCredentials: Invalid credentials
signUp:
duplicatedEmail: A user with this email already exists
notMatchingPasswords: Not matching passwords
agreeToTerms: You must agree to terms
recoverPassword:
emailSent: A message with further instructions has been sent to your email address
emailNotFound: Email not found
emailNotDelivered: Email could not be delivered
resetPassword:
invalidToken: Reset token is not valid
addressBook:
invalidCountry: Invalid country
confirmation:
agreeToTerms: You must agree to terms
24 changes: 24 additions & 0 deletions conf/i18n/en/messages.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
defaultError: Something went wrong, please try again
checkout:
payment:
invalidPayment: Invalid payment
addresses:
invalidShippingCountry: Invalid shipping country
invalidBillingCountry: Invalid billing country
myAccount:
logIn:
invalidCredentials: Invalid credentials
signUp:
duplicatedEmail: A user with this email already exists
notMatchingPasswords: Not matching passwords
agreeToTerms: You must agree to terms
recoverPassword:
emailSent: A message with further instructions has been sent to your email address
emailNotFound: Email not found
emailNotDelivered: Email could not be delivered
resetPassword:
invalidToken: Reset token is not valid
addressBook:
invalidCountry: Invalid country
confirmation:
agreeToTerms: You must agree to terms
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public void showsErrorOnEmailDeliveryException() throws Exception {
.bodyForm(singletonMap("email", email)));

assertThat(result.status()).isEqualTo(INTERNAL_SERVER_ERROR);
assertThat(contentAsString(result)).contains("Email delivery error");
assertThat(contentAsString(result)).contains("Email could not be delivered");

return customerSignInResult.getCustomer();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public DefaultAddressBookAddressFormData() {
public String validate() {
final CountryCode country = CountryCode.getByCode(this.country);
if (country == null || country.equals(CountryCode.UNDEFINED)) {
return "Invalid country"; // TODO use i18n version
return "messages:myAccount.addressBook.invalidCountry";
}
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public CompletionStage<? extends CustomerSignInResult> doAction(final LogInFormD
@Override
public CompletionStage<Result> handleClientErrorFailedAction(final Form<? extends LogInFormData> form, final Void context, final ClientErrorException clientErrorException) {
if (isInvalidCredentialsError(clientErrorException)) {
saveFormError(form, "Invalid credentials"); // TODO i18n
saveFormError(form, "messages:myAccount.logIn.invalidCredentials");
} else {
saveUnexpectedFormError(form, clientErrorException, logger);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,18 +100,18 @@ public CompletionStage<Result> handleClientErrorFailedAction(final Form<? extend
}

protected CompletionStage<Result> handleNotFoundEmail(final Form<? extends RecoverPasswordFormData> form) {
saveFormError(form, "Email not found");
saveFormError(form, "messages:myAccount.recoverPassword.emailNotFound");
return asyncBadRequest(renderPage(form, null, null));
}

protected CompletionStage<Result> handleEmailDeliveryException(final Form<? extends RecoverPasswordFormData> form, final EmailDeliveryException emailDeliveryException) {
saveFormError(form, "Email delivery error");
saveFormError(form, "messages:myAccount.recoverPassword.emailNotDelivered");
return asyncInternalServerError(renderPage(form, null, null));
}

@Override
public CompletionStage<Result> handleSuccessfulAction(final RecoverPasswordFormData formData, final Void context, final CustomerToken customerToken) {
flash("success", "A message with further instructions has been sent to your email address");
flash("success", "messages:myAccount.recoverPassword.emailSent");
return redirectToSameForm();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public CompletionStage<Result> handleClientErrorFailedAction(final Form<? extend
}

protected CompletionStage<Result> handleNotFoundToken(final Form<? extends ResetPasswordFormData> form, final String resetToken) {
saveFormError(form, "Reset token is not valid");
saveFormError(form, "messages:myAccount.resetPassword.invalidToken");
return asyncBadRequest(renderPage(form, resetToken, null));
}

Expand Down
Loading