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

Translate CodeHarbor to German #1143

Merged
merged 8 commits into from
Mar 21, 2024
Merged

Translate CodeHarbor to German #1143

merged 8 commits into from
Mar 21, 2024

Conversation

MrSerth
Copy link
Member

@MrSerth MrSerth commented Dec 13, 2023

This PR adds the current status of the translation of CodeHarbor to German.

As specified in the first commit, the translations provided here were not checked manually yet and therefore contain some inconsistencies, such as the salutation ("Du" vs "Sie") or the gender form. Before merging, we should unify those (probably "Sie" and gendered nouns with : [e.g., "Empfänger:in"]). Further, I omitted the "external" translations, since we can probably borrow them from some other location without the need to review them.

The other changes beside the locale files are probably enough to enable an additional language for CodeHarbor and allow users to switch. These parts of the code originate from CodeOcean and are only adjusted where necessary.

@MrSerth MrSerth linked an issue Dec 13, 2023 that may be closed by this pull request
Copy link

codecov bot commented Dec 13, 2023

Codecov Report

Attention: Patch coverage is 93.02326% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 87.55%. Comparing base (52d29e5) to head (d94b312).

❗ Current head d94b312 differs from pull request most recent head 6a58a21. Consider uploading reports for the commit 6a58a21 to get more accurate results

Files Patch % Lines
app/mailers/access_request_mailer.rb 50.00% 2 Missing ⚠️
app/helpers/authenticated_url_helper.rb 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1143      +/-   ##
==========================================
+ Coverage   86.55%   87.55%   +1.00%     
==========================================
  Files          98       99       +1     
  Lines        2291     2315      +24     
==========================================
+ Hits         1983     2027      +44     
+ Misses        308      288      -20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Mathis-Z Mathis-Z force-pushed the translate-german branch 4 times, most recently from 3f82047 to 992290b Compare January 31, 2024 21:16
@Mathis-Z
Copy link
Contributor

Mathis-Z commented Feb 17, 2024

I encountered a problem with the flash messages. Apparently there is an encoding problem for German characters "äöü". The flash messages are transferred using HTTP headers which are supposed to be ASCII encoded. I am not sure how we should solve this. One possibility would be to encode flash messages with base64 and decode them on the frontend. However that is not exactly ideal because we could not use the normal flash messages anymore but replace them with a custom wrapper doing the base64 encoding.
Do you have another idea?

Edit: I just thought we could base64 encode the flash message in an after_action and realized there already is an after_action handling the flash messages... So I will just use the base64 solution.

@Mathis-Z
Copy link
Contributor

Mathis-Z commented Feb 17, 2024

I found out that the base64 part is not even required. We can use "ERB::Util.url_encode(flash_message)" on the Rails side and "decodeURI(request.getResponseHeader("X-Message"))" in the Coffeescript. This even works for Unicode characters :)

@MrSerth
Copy link
Member Author

MrSerth commented Feb 17, 2024

Thanks for describing the flash message encoding issue. I think one of your solutions is fine (e.g., the URL encoding). The problem here is that the mechanism chosen with the X-Message is already a custom and non-Rails solution only required for some of the AJAX actions.

@Mathis-Z
Copy link
Contributor

Mathis-Z commented Feb 18, 2024

How do we want to translate the mails users receive when somebody requests access to join their group? The problem I see is that we do not know the preferred locale of the receiver at the time we send the mail. Maybe we should store the last used locale for users? We could also use this to automatically set the last used locale when a user signs in.

Edit: I think this could be stored in the User model. What do you think?

@MrSerth
Copy link
Member Author

MrSerth commented Feb 19, 2024

How do we want to translate the mails users receive when somebody requests access to join their group?

Generally speaking, I see two possibilities here: Either, sending the messages in both languages ("German version below") or storing the desired locale in the user profile (this is the mechanism we chose for openHPI). If you want to go the extra mile, storing it in the profile and updating it on each locale change would hence be the "better" option.

@Mathis-Z
Copy link
Contributor

I would go with the second option. I can probably take some code from CodeOcean so this should not be too difficult.

app/models/message.rb Outdated Show resolved Hide resolved
@Mathis-Z

This comment was marked as resolved.

@MrSerth

This comment was marked as resolved.

MrSerth

This comment was marked as resolved.

@Mathis-Z Mathis-Z marked this pull request as ready for review February 28, 2024 09:52
config/locales/de/views/tasks.yml Outdated Show resolved Hide resolved
config/locales/en/views/tasks.yml Outdated Show resolved Hide resolved
app/controllers/application_controller.rb Outdated Show resolved Hide resolved
app/controllers/application_controller.rb Outdated Show resolved Hide resolved
app/controllers/groups_controller.rb Outdated Show resolved Hide resolved
app/controllers/rails_admin_controller.rb Outdated Show resolved Hide resolved
app/javascript/application.js Outdated Show resolved Hide resolved
app/mailers/access_request_mailer.rb Outdated Show resolved Hide resolved
config/locales/de/views/tasks.yml Show resolved Hide resolved
config/locales/de/views/users.yml Outdated Show resolved Hide resolved
@Mathis-Z
Copy link
Contributor

Mathis-Z commented Mar 1, 2024

About the commit squashing: I would squash all of my own commits and leave yours as they are if you are okay with that.

@MrSerth
Copy link
Member Author

MrSerth commented Mar 1, 2024

About the commit squashing: I would squash all of my own commits and leave yours as they are if you are okay with that.

Well, generally, a similar approach is okay. However, I would suggest not squashing too much. What do you think if we mainly squash those that contain changes to the translations and keep those separate, that include (further, relevant) code changes? Specifically, I thought of:

  • "Fixing bug in task validation error display"
  • "Encoding X-Message header urlsafe to support non-ascii characters in flash messages"
  • "Storing preferred user locale in database" + "Removing not-NULL constraint and default value for user preferred_locale"
  • either separate or together with the previous one: "Translating access request mails using recipients preferred locale" + "Translating hardcoded subject in access request mails"

Of course, this is just a suggestion and you do not need to comply with them. My suggestion is mainly based on the idea that each commit should contain something like a self-contained, small, shippable increment. For that, we can also include my first commit "Provide initial German translation" in the squashing and merge it with the rephrasing performed later. The idea to keep some other separate is that I'd argue each of them contains a dedicated "feature" or a "bug fix", or that some "explanation" in the commit might be helpful for further debugging.

@Mathis-Z
Copy link
Contributor

Mathis-Z commented Mar 6, 2024

I was wondering whether we want to pass the locale parameter across requests like the rails guide suggests:

def default_url_options
  { locale: I18n.locale }
end

We could replace the session locale with that. One advantage of this is that the user keeps their selected locale when logging out. Currently you fall back to either your browser locale or the default locale if you log out because you loose your session.

@Mathis-Z
Copy link
Contributor

Mathis-Z commented Mar 8, 2024

I now squashed the commits in a way that I think makes sense. What do you think?

app/controllers/groups_controller.rb Outdated Show resolved Hide resolved
app/models/message.rb Show resolved Hide resolved
config/locales/en/models.yml Outdated Show resolved Hide resolved
config/webpack/webpack.config.js Show resolved Hide resolved
spec/models/user_spec.rb Outdated Show resolved Hide resolved
app/javascript/application.js Outdated Show resolved Hide resolved
MrSerth

This comment was marked as resolved.

@Mathis-Z Mathis-Z force-pushed the translate-german branch 2 times, most recently from 0556fdf to cfed5bb Compare March 12, 2024 17:54
Copy link
Member Author

@MrSerth MrSerth left a comment

Choose a reason for hiding this comment

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

Thanks for your final touches on this PR. The only part we need to check is the localisation of select2, otherwise I think that's fine 👍

db/migrate/20240312165451_rename_group_param_type.rb Outdated Show resolved Hide resolved
@Mathis-Z
Copy link
Contributor

Mathis-Z commented Mar 15, 2024

Just after pushing the commit I noticed there was another bug in it. I will update it soon.

@Mathis-Z
Copy link
Contributor

Actually I cannot figure it out. Maybe you see the problem? I pushed my current state. The problem is that only the label select2 gets initialized and the others are not.

app/assets/javascripts/tasks_index.coffee Outdated Show resolved Hide resolved
app/javascript/application.js Outdated Show resolved Hide resolved
app/javascript/application.js Outdated Show resolved Hide resolved
MrSerth and others added 6 commits March 21, 2024 23:59
All locales were just translated using OpenAI, and are not validated yet. Further, we had to split the translation into several requests, which probably accounts for the different greeting. Those should be unified later
----------------------------------------------------------------------
- using formal address for all German translations
- adding translations for devise and simple_form; moving some translations from devise to user
- translating select2
- translating group/collection messages
- removing user locales that are already in devise
Copy link
Member Author

@MrSerth MrSerth left a comment

Choose a reason for hiding this comment

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

Great, thank you very much for your endurance and perfection to finalise this PR! 💯 I've just rebased this branch on the current master, localised two additional strings I discovered and tackled the remaining comment. Now, I am happy to approve and merge it right away 👍

@MrSerth MrSerth merged commit 90cf84d into master Mar 21, 2024
8 checks passed
@MrSerth MrSerth deleted the translate-german branch March 21, 2024 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Localize CodeHarbor in German
2 participants