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

Conversation

tkuzynow
Copy link
Contributor

@tkuzynow tkuzynow commented Jan 4, 2024

Fixes #

Proposed Changes

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

Copy link
Member

@patric-dosch-vi patric-dosch-vi left a comment

Choose a reason for hiding this comment

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

hey @tkuzynow, I think I started to have a look and again forgot to submit the review, stupido haha

For now it's just a typo but I think we should think about Marc's comment.

@@ -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 :)

private String fetchDefaultTranslations(String translationComponentName, String languageCode, Dialect dialect) {
String translationFilename = getTranslationFilename(
translationComponentName + "." + languageCode
+ TranlationMangementServiceApiClient.getDialectSuffix(dialect));
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
Member

@patric-dosch-vi patric-dosch-vi left a comment

Choose a reason for hiding this comment

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

hey @tkuzynow, I had a look and I think (and as it blocks) we can merge this.
Patric

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@patric-dosch-vi patric-dosch-vi left a comment

Choose a reason for hiding this comment

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

@tkuzynow for sure approved again

@tkuzynow tkuzynow merged commit 0870404 into develop Jan 17, 2024
2 of 4 checks passed
@tkuzynow tkuzynow deleted the OB-7109-add-support-for-informal-language-fix branch January 17, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants