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

Ob 7109 add support for informal language fix #106

Merged
merged 7 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
@@ -1,5 +1,6 @@
package de.caritas.cob.mailservice.api;

import de.caritas.cob.mailservice.api.model.Dialect;
import de.caritas.cob.mailservice.api.service.TranslationService;
import java.util.Locale;
import lombok.NonNull;
Expand All @@ -13,16 +14,17 @@
@Service
@RequiredArgsConstructor
@Slf4j
public class RestApiMessageSource implements MessageSource {

public class TranslationMessageSource implements MessageSource {

public static final String VA_POSIX = "va-posix";
public final @NonNull TranslationService translationService;

@Override
public String getMessage(String code, Object[] args, String defaultMessage, Locale locale) {
log.info("getMessage called with code: {}, args: {}, defaultMessage: {}, locale: {}", code,
args, defaultMessage, locale);
return translationService.fetchTranslations(locale.getLanguage()).get(code);
Dialect dialect = determineDialect(locale);
return translationService.fetchTranslations(locale.getLanguage(), dialect).get(code);
}

@Override
Expand All @@ -40,4 +42,12 @@ public String getMessage(MessageSourceResolvable resolvable, Locale locale)
}
return getMessage(resolvable.getCodes()[0], null, locale);
}

private Dialect determineDialect(Locale locale) {
Dialect dialect = Dialect.FORMAL;
if ("DE".equals(locale.getCountry()) && VA_POSIX.equals(locale.getExtension('u'))) {
dialect = Dialect.INFORMAL;
}
return dialect;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public class TranslationController {

@GetMapping(value = "/translations")
public ResponseEntity<Map<String, String>> getTranslations() {
var result = translationService.fetchTranslations("de");
var result = translationService.fetchTranslations("de", Dialect.FORMAL);
return new ResponseEntity<>(result, org.springframework.http.HttpStatus.OK);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package de.caritas.cob.mailservice.api.helper;

import de.caritas.cob.mailservice.api.model.Dialect;
import de.caritas.cob.mailservice.api.model.LanguageCode;
import java.util.Locale;
import java.util.Map;
Expand All @@ -13,6 +14,8 @@
@Component
public class ThymeleafHelper {

private static final String INFORMAL_GERMAL_LANGUAGE_TAG = "de-DE-u-va-posix";
Copy link
Member

Choose a reason for hiding this comment

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

GERMAN :)


@Autowired
private TemplateEngine tempTemplateEngine;

Expand All @@ -23,17 +26,23 @@ void init() {
templateEngine = tempTemplateEngine;
}

public static Optional<String> getProcessedHtml(Map<String, Object> data, LanguageCode languageCode, String templateName) {
public static Optional<String> getProcessedHtml(Map<String, Object> data, LanguageCode languageCode, String templateName, Dialect dialect) {

Context context = new Context();
Locale locale = Locale.forLanguageTag(languageCode.getValue());
Locale locale = Locale.forLanguageTag(getLanguageTag(languageCode, dialect));
context.setLocale(locale);

if (data != null) {
data.forEach(context::setVariable);
return Optional.of(templateEngine.process(templateName, context));
}
return Optional.empty();

}

private static String getLanguageTag(LanguageCode languageCode, Dialect dialect) {
if (languageCode == LanguageCode.DE && dialect == Dialect.INFORMAL) {
return INFORMAL_GERMAL_LANGUAGE_TAG;
}
return languageCode.getValue();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ public Optional<String> render(TemplateDescription desc, MailDTO mail, Map<Strin

var templateFilename = desc.getTemplateFilenameOrFallback(mail.getLanguage());

return translationsArePresentAndNotEmpty(mail) ? getProcessedHtml(data, mail.getLanguage(), templateFilename) :
getProcessedHtml(data, LanguageCode.DE, templateFilename);
return translationsArePresentAndNotEmpty(mail) ? getProcessedHtml(data, mail.getLanguage(), templateFilename, mail.getDialect()) :
getProcessedHtml(data, LanguageCode.DE, templateFilename, mail.getDialect());
}

private boolean translationsArePresentAndNotEmpty(MailDTO mailDTO) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.springframework.http.HttpStatus;
import org.springframework.stereotype.Service;
import org.springframework.web.client.HttpClientErrorException;
import org.springframework.web.client.ResourceAccessException;

@Service
@Slf4j
Expand All @@ -35,17 +36,13 @@ public class TranslationService {
@Value("${weblate.component}")
private String component;

private final @NonNull TranlationMangementServiceApiClient tranlationMangementServiceApiClient;
private final @NonNull TranlationMangementServiceApiClient tranlationMangementServiceApiClient;

public TranslationService(TranlationMangementServiceApiClient tranlationMangementServiceApiClient) {
public TranslationService(
TranlationMangementServiceApiClient tranlationMangementServiceApiClient) {
this.tranlationMangementServiceApiClient = tranlationMangementServiceApiClient;
}

@Cacheable(value = "translations")
public Map<String, String> fetchTranslations(String languageCode) {
return this.fetchTranslations(languageCode, Dialect.FORMAL);
}

@Cacheable(value = "translations")
public Map<String, String> fetchTranslations(String languageCode, Dialect dialect) {
try {
Expand All @@ -63,7 +60,8 @@ public void evictCache() {
log.info("Evicting translations cache");
}

private Map<String, String> fetchTranslationAsMap(String languageCode, Dialect dialect) throws JsonProcessingException {
private Map<String, String> fetchTranslationAsMap(String languageCode, Dialect dialect)
throws JsonProcessingException {
String translations = fetchTranslationsAsString(languageCode, dialect);
ObjectMapper mapper = new ObjectMapper();
return mapper.readValue(translations, Map.class);
Expand All @@ -76,30 +74,41 @@ public Optional<Map<String, String>> tryFetchTranslations(String languageCode, D
var result = fetchTranslationAsMap(languageCode, dialect);
return result.isEmpty() ? Optional.empty() : Optional.of(result);
} catch (JsonProcessingException e) {
log.warn("Error while processing json file with translations. Returning empty translations", e);
log.warn("Error while processing json file with translations. Returning empty translations",
e);
return Optional.empty();
}
}

private String fetchTranslationsAsString(String languageCode, Dialect dialect) {
try {
return tranlationMangementServiceApiClient.tryFetchTranslationsFromTranslationManagementService(project, component,
return tranlationMangementServiceApiClient.tryFetchTranslationsFromTranslationManagementService(
project, component,
languageCode, dialect);
} catch (HttpClientErrorException e) {
if (HttpStatus.NOT_FOUND.equals(e.getStatusCode())) {
log.warn("Translations for component {}, language {} not found in weblate, returning default translations", component,
log.warn(
"Translations for component {}, language {} not found in weblate, returning default translations",
component,
languageCode);
return fetchDefaultTranslations(component, languageCode);
return fetchDefaultTranslations(component, languageCode, dialect);
} else {
log.error("Error while fetching translations from translation management service", e);
throw e;
}
} catch (ResourceAccessException ex) {
log.error("ResourceAccessException error while fetching translations from translation management service. Will fallback to resolve default translations.");
log.debug("Exception details: ", ex);
return fetchDefaultTranslations(component, languageCode, dialect);
}
}

private String fetchDefaultTranslations(String translationComponentName, String languageCode) {
private String fetchDefaultTranslations(String translationComponentName, String languageCode, Dialect dialect) {
String translationFilename = getTranslationFilename(
translationComponentName + "." + languageCode
+ TranlationMangementServiceApiClient.getDialectSuffix(dialect));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this fallback to mailservice.de.json? Or how does it work when its using dialect but we have no translation catalog for dialect stored in project?
In frontend there is a logic for fallback because not everything needs to be translated in dialect so we only dialect stuff in dialect catalog and all other translations are taken from the de catalog. Maybe we need a merge of them here too?

Copy link
Contributor

Choose a reason for hiding this comment

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

For the weblate api call it should be fine but we need to check if the configuration is correct. It should automatically fill up non translated strings in dialect with fallback to de.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @web-mi , do you want to have fallback logic to DE in case there is informal language setting in consultant profile but no informal file found? Ok, we can build this.
But I am against merging of json files. IMO if informal dialect can be set we should have all keys translated to dialect -> this should be required configuration.
Otherwise some keys will have formal, another informal translations which will lead to a mix-up between Sie/Ihr and Du/Dir forms in the applications.
So in worst case in one email user will get mixed up form. Either informal lang is fully supported and therefore all keys are translated, or not and in this case we don't need to have separate informal json file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes fallback logic if file does not exists locally. What is happening now when there is no informal file?

In Frontend we have 2700 lines inside de.json and only some of them have informal Translation. Thats why we have the fallback to de.
For example if we have a email subject "New message from advice seeker" its not required to translate it to informal because there is no formal language in this sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @web-mi , added the fallback logic in case informal translation file does not exist

Copy link
Member

Choose a reason for hiding this comment

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

Oh dear, Marc has touched on an nice topic here. In the context of the mail service, I would be fine to have all translations duplicated and adapted where needed. That's how it would currently work, because it's a new component in Weblate.
However, with the many keys in the frontend, Marc's merge is of course important. Well, as I understand it, the frontend handles this itself, but of course it would be nice if all translation variants worked identically.
Do we need to have a chat about this?

Copy link
Contributor Author

@tkuzynow tkuzynow Jan 17, 2024

Choose a reason for hiding this comment

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

For frontend it makes sense to build a merge logic, but I don't see added value for the mailservice. Frontend translations are huge and there are would be many repetitions with duplicated keys.
For mailservice we have small amount of labels, files are small and this will not change, therefore treating dialect as separate lang and having all keys translated there is not a problem. We avoid the need to have merge logic that has to be tested and maintained in future. Building merge logic would IMO unnecessarily increase the complexity of the project.
I am always for consistency, but at the same time we need to avoid complexity which is already too big. Since we have microservice architecture, so we have also some freedom to decide on a service level. So we don't need to have force apply FE solutions if it does not fit to the given microservice.

Copy link
Member

Choose a reason for hiding this comment

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

I was also speaking more from the perspective of the user operating Weblate... because they now need to understand how it works and where

var inputStream = TranslationService.class.getResourceAsStream(
getTranslationFilename(translationComponentName + "." + languageCode));
translationFilename);
if (inputStream == null) {
return "{}";
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package de.caritas.cob.mailservice.config;

import de.caritas.cob.mailservice.api.RestApiMessageSource;
import de.caritas.cob.mailservice.api.TranslationMessageSource;
import de.caritas.cob.mailservice.api.service.TranslationService;
import java.nio.charset.StandardCharsets;
import org.springframework.beans.factory.annotation.Value;
Expand Down Expand Up @@ -56,7 +56,7 @@ private ITemplateResolver htmlClassLoaderTemplateResolver() {

@Bean
public MessageSource messageSource(TranslationService translationService) {
return new RestApiMessageSource(translationService);
return new TranslationMessageSource(translationService);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public String tryFetchTranslationsFromTranslationManagementService(String projec
return response.getBody();
}

private String getDialectSuffix(Dialect dialect) {
public static String getDialectSuffix(Dialect dialect) {
if (dialect == null) {
return StringUtils.EMPTY;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ <h1 style='color: #FFFFFF; font-size: 26px; font-weight: bold; line-height: 26px
<div style="color: #3F373F; margin: 16px; font-size: 16px; line-height: 21px; font-family: 'Open Sans', 'OpenSans', 'Arial', 'sans-serif';">
<b>Liebe(r) <span data-th-text="${name}"></span>,</b><br/>
<br/>
Sie haben eine neue Nachricht in Ihren Beratungen.
<span data-th-text="#{mail.label.you.have.a.new.message.in.your.counselings}"/>
</div>
</td>
</tr>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package de.caritas.cob.mailservice.api;

import static org.mockito.Mockito.verify;

import de.caritas.cob.mailservice.api.model.Dialect;
import de.caritas.cob.mailservice.api.service.TranslationService;
import java.util.Locale;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
class TranslationMessageSourceTest {

private static final String INFORMAL_GERMAL_LANGUAGE_TAG = "de-DE-u-va-posix";

@InjectMocks
private TranslationMessageSource translationMessageSource;

@Mock
private TranslationService translationService;

@Test
void getMessage_Should_CallFetchTranslations_With_FormalDialect_For_DefaultLocale() {
// given
Locale locale = Locale.getDefault();

// when
translationMessageSource.getMessage("translation_key", new Object[]{}, "Message", locale);

// then
verify(translationService, Mockito.times(1)).fetchTranslations(locale.getLanguage(), Dialect.FORMAL);
}

@Test
void getMessage_Should_CallFetchTranslations_With_InformalDialect_For_ForLocaleForInformalGerman() {
// given
Locale locale = Locale.forLanguageTag(INFORMAL_GERMAL_LANGUAGE_TAG);

// when
translationMessageSource.getMessage("translation_key", new Object[]{}, "Message", locale);

// then
verify(translationService, Mockito.times(1)).fetchTranslations(locale.getLanguage(), Dialect.INFORMAL);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ private void givenAnEmailList(LanguageCode languageCode) {
var email = new MailDTO();
email.setEmail(RandomStringUtils.randomAlphanumeric(32));
email.setTemplate("reassign-confirmation-notification");
email.setDialect(Dialect.INFORMAL);
email.setDialect(Dialect.FORMAL);

var nameRecipient = new TemplateDataDTO()
.key("name_recipient")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package de.caritas.cob.mailservice.api.service;



import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

import com.google.common.collect.Lists;
import de.caritas.cob.mailservice.api.exception.TemplateServiceException;
import de.caritas.cob.mailservice.api.mailtemplate.SubjectDescription;
import de.caritas.cob.mailservice.api.mailtemplate.TemplateDescription;
import de.caritas.cob.mailservice.api.model.Dialect;
import de.caritas.cob.mailservice.api.model.LanguageCode;
import de.caritas.cob.mailservice.api.model.MailDTO;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.test.context.ActiveProfiles;

@SpringBootTest
@AutoConfigureMockMvc
@ActiveProfiles("testing")
class TemplateServiceIntegrationTest {

@Autowired
private TemplateService templateService;

@Autowired
private TranslationService translationService;

@Test
void getProcessedHtmlTemplate_Should_RenderTemplateData_With_InformalDialect()
throws TemplateServiceException {

Optional<String> renderedHtml = templateService.render(
new TemplateDescription(Map.of(LanguageCode.DE, "message-notification-consultant.html"),
new SubjectDescription("translationKey"),
Lists.newArrayList(),
null),
new MailDTO()
.template("message-notification-consultant")
.language(LanguageCode.DE)
.dialect(Dialect.INFORMAL),
new HashMap<>());
assertThat(renderedHtml).isPresent();
assertThat(renderedHtml.get()).contains("Du hast eine neue Nachricht in Deinen Beratungen");
}

@Test
void getProcessedHtmlTemplate_Should_RenderTemplateData_With_FormalDialect()
throws TemplateServiceException {

Optional<String> renderedHtml = templateService.render(
new TemplateDescription(Map.of(LanguageCode.DE, "message-notification-consultant.html"),
new SubjectDescription("translationKey"),
Lists.newArrayList(),
null),
new MailDTO()
.template("message-notification-consultant")
.language(LanguageCode.DE)
.dialect(Dialect.FORMAL),
new HashMap<>());
assertThat(renderedHtml).isPresent();
assertThat(renderedHtml.get()).contains("Sie haben eine neue Nachricht in Ihren Beratungen");
}

}
Loading