Fixes #36330 - Make translated strings from plugins available in the browser and modern frontend code #9691

adamruzicka · 2023-04-26T11:59:53Z

For context about the problem see https://gist.github.com/adamruzicka/f578c2e519f51230d475fd72848629d1

Concerns

webhippie/po_to_json had last change in June 2018, this change is still unreleased. Sadly, the changes proposed here rely on that unreleased change. Unless webhippie/po_to_json#8 is resolved soon-ish, we may need to either depend (and maybe package?) latest master or carry the project ourselves. Version 1.1.0 is out.

webhippie/gettext_i18n_rails_js doesn't really support doing the conversion for other rails engines (and domains) than just ::Rails. The changes proposed here work around that in a rather hacky fashion. If webhippie/gettext_i18n_rails_js#57 is accepted and released soon, we could have a more sensible way of doing that, along the lines of https://github.com/theforeman/foreman/compare/develop...adamruzicka:foreman:potojson-next?expand=1

After this change is merged, all plugins will need to run the new rake task to make translations pop up.

Followup changes

refresh translations in all plugins
update plugin template

theforeman-bot · 2023-04-26T11:59:57Z

Issues: #36330

ekohl

I've been thinking about a way to register translations during plugin registration. We have this bit:

https://github.com/theforeman/foreman_plugin_template/blob/5a2f59c636e4a732b8da2d238744ab65a800fd4b/lib/foreman_plugin_template/engine.rb#L62-L66

Would it make sense to have some DSL method to register translations with a certain domain instead of having to write an initializer in every plugin?

Perhaps I'm broadening the scope here, but I've always been confused that a Rails engine registers a plugin, but in my mind it would have made more sense to make a ForemanPlugin that is an engine, or at least creates an engine.

app/views/layouts/base.html.erb

ekohl · 2023-04-26T12:16:39Z

app/views/layouts/base.html.erb

@@ -32,6 +32,9 @@
      <% end %>
    </script>
    <%= javascript_include_tag "locale/#{FastGettext.locale}/app" %>
+    <% for plugin in ::Foreman::Plugin.all do %>
+      <%= javascript_include_tag "locale/#{FastGettext.locale}/#{plugin.engine.class.to_s.underscore.split('/', 2).first}" %>


Does this work on foreman-tasks? Perhaps because the domain is foreman_tasks, but just want to make sure.

foreman-tasks is probably the sole reason why I couldn't just use plugin.name or plugin.id. The domain indeed has underscores in it

Are you starting to understand why I'd love to rename it? Even though it's a pain once, it'll be easier in the long run.

adamruzicka · 2023-04-26T14:10:43Z

Would it make sense to have some DSL method to register translations with a certain domain instead of having to write an initializer in every plugin?

I took a stab at this, does it somehow resemble what you had in mind? I hope I covered everything

ekohl · 2023-04-26T15:55:09Z

I took a stab at this, does it somehow resemble what you had in mind? I hope I covered everything

I think it does, very nice. Now I have more ideas so would it make sense to submit it as a standalone PR and then merge it?

Like enhance the rake task to utilize the domain:

foreman/lib/tasks/gettext.rake

Lines 23 to 46 in 96c6d18

    
           desc 'Extract plugin strings - called via rake plugin:gettext[plugin_name]' 
        
           task 'plugin:gettext', :engine do |t, args| 
        
             @domain = args[:engine] 
        
             @engine = "#{@domain.camelize}::Engine".constantize 
        
             @engine_root = @engine.root 
        
             namespace :gettext do 
        
               def locale_path 
        
                 "#{@engine_root}/locale" 
        
               end 
        
               def files_to_translate 
        
                 @engine.root.glob(FILE_GLOB).map(&:to_s) 
        
               end 
        
               def text_domain 
        
                 @domain 
        
               end 
        
             end 
        
             Foreman::Gettext::Support.add_text_domain @domain, "#{@engine_root}/locale" 
        
             Rake::Task['gettext:find'].invoke 
        
           end

Then also update the plugin template to utilize the new DSL method.

bundler.d/assets.rb

app/helpers/reactjs_helper.rb

ekohl

Argh, I forgot to submit my review last week.

app/helpers/reactjs_helper.rb

lib/tasks/gettext.rake

ekohl

And I wonder if we can either disable the spellcheck on those words or if we can add them to the known words.

ekohl

Looks like you can configure spellcheck/spell-checker to ignore words in .eslintrc. Other than that 👍 from my side, though I'm not an expert on JS. Perhaps @MariaAga can take a look at that part.

lib/tasks/gettext.rake

ekohl

Please squash the commits so we can merge them.

app/helpers/reactjs_helper.rb

ekohl · 2023-05-15T14:01:45Z

app/helpers/reactjs_helper.rb

+    domains = ::Foreman::Plugin.all.filter_map do |plugin|
+      domain = plugin.gettext_domain
+      domain if (FastGettext.translation_repositories[domain]&.available_locales || []).include? locale
+    end
+    domains.map do |domain|
+      javascript_include_tag("locale/#{locale}/#{domain}")
+    end.join.html_safe


Untested, but isn't it also needed to check if domain is nil? Or do you count on FastGettext.translation_repositories[nil returning nil? I was thinking about something like this:

Suggested change

domains = ::Foreman::Plugin.all.filter_map do |plugin|

domain = plugin.gettext_domain

domain if (FastGettext.translation_repositories[domain]&.available_locales || []).include? locale

end

domains.map do |domain|

javascript_include_tag("locale/#{locale}/#{domain}")

end.join.html_safe

::Foreman::Plugin.all.filter_map do |plugin|

domain = plugin.gettext_domain

if domain && (FastGettext.translation_repositories[domain]&.available_locales || []).include? locale

javascript_include_tag("locale/#{locale}/#{domain}")

end

end.join.html_safe

Or do you count on FastGettext.translation_repositories[nil returning nil

Yes, I did that. The assumption seems to hold, what you suggest is hands down more obvious and probably safe in the long run. It needed a small tweak (parentheses) though

adamruzicka · 2023-05-16T07:32:37Z

I went ahead and squashed this, unsquashed variant is still available here https://github.com/adamruzicka/foreman/tree/unsquashed/potojson

ekohl

This will require a bit on the packaging side, but that should be ok.

ekohl · 2023-05-17T10:55:50Z

bundler.d/assets.rb

@@ -1,7 +1,8 @@
 group :assets do
  gem 'jquery-ui-rails', '~> 6.0'
  gem 'patternfly-sass', '~> 3.59.4'
-  gem 'gettext_i18n_rails_js', '~> 1.3.1'
+  gem 'gettext_i18n_rails_js', '~> 1.4'
+  gem 'po_to_json', '~> 1.1.0'


Any particular reason to pin to ~> 1.1.0 instead of ~> 1.1?

No, not really, changed

- Add plugin:po_to_json rake task - Load js locale files for all plugins - Make i18n.js do multi-domain lookup

MariaAga · 2023-05-17T11:09:39Z

webpack/assets/javascripts/react_app/common/I18n.js

+const multidomain = (strict, fallback) => (...args) => {
+  // eslint-disable-next-line no-unused-vars
+  for (const domain of Object.keys(strictJed.options.locale_data)) {
+    try {


Js looks fine, is the only way to do this conditional return is with try-catch?

I haven't found any other way

ekohl

I'm still wondering about storing the JSON files in git, but that's a similar problem as the MO files.

Outdated

theforeman-bot added Not yet reviewed Docs UI labels Apr 26, 2023

ekohl reviewed Apr 26, 2023

View reviewed changes

adamruzicka force-pushed the potojson branch from 47081bb to f1ac7af Compare April 26, 2023 12:22

adamruzicka mentioned this pull request Apr 26, 2023

Fixes #36332 - Use plugin dsl for gettext theforeman/foreman-tasks#713

Merged

1 task

adamruzicka mentioned this pull request Apr 26, 2023

Fixes #36331 - Expose translation registration in plugin DSL #9692

Merged

2 tasks

adamruzicka force-pushed the potojson branch from 2c50815 to 2202aad Compare April 26, 2023 16:02

ekohl reviewed May 8, 2023

View reviewed changes

bundler.d/assets.rb Outdated Show resolved Hide resolved

ShimShtein previously requested changes May 8, 2023

View reviewed changes

app/helpers/reactjs_helper.rb Outdated Show resolved Hide resolved

theforeman-bot added Waiting on contributor Needs re-review and removed Not yet reviewed Waiting on contributor labels May 8, 2023

adamruzicka marked this pull request as ready for review May 10, 2023 14:40

adamruzicka requested a review from a team as a code owner May 10, 2023 14:40

adamruzicka force-pushed the potojson branch from 5406fec to 6b6f339 Compare May 10, 2023 14:41

ekohl reviewed May 15, 2023

View reviewed changes

app/helpers/reactjs_helper.rb Outdated Show resolved Hide resolved

app/helpers/reactjs_helper.rb Outdated Show resolved Hide resolved

lib/tasks/gettext.rake Outdated Show resolved Hide resolved

ekohl reviewed May 15, 2023

View reviewed changes

ekohl requested changes May 15, 2023

View reviewed changes

lib/tasks/gettext.rake Outdated Show resolved Hide resolved

theforeman-bot added Waiting on contributor and removed Needs re-review labels May 15, 2023

adamruzicka force-pushed the potojson branch from 809ddce to cd7b41f Compare May 15, 2023 13:41

theforeman-bot added Needs re-review and removed Waiting on contributor labels May 15, 2023

ekohl previously approved these changes May 15, 2023

View reviewed changes

app/helpers/reactjs_helper.rb Outdated Show resolved Hide resolved

theforeman-bot removed the Needs re-review label May 15, 2023

adamruzicka dismissed ekohl’s stale review via b8e054b May 15, 2023 13:51

adamruzicka force-pushed the potojson branch 2 times, most recently from b8e054b to cc4e4f2 Compare May 15, 2023 13:59

ekohl reviewed May 15, 2023

View reviewed changes

adamruzicka force-pushed the potojson branch 2 times, most recently from 828ae5b to 11595c2 Compare May 16, 2023 07:32

This was referenced May 16, 2023

I18n theforeman/foreman-tasks#718

Merged

Update makefile for frontend translations theforeman/foreman_plugin_template#41

Merged

adamruzicka force-pushed the potojson branch from 11595c2 to 33c524d Compare May 16, 2023 07:56

ekohl previously approved these changes May 17, 2023

View reviewed changes

ekohl reviewed May 17, 2023

View reviewed changes

Fixes #36330 - Fix plugin translations in frontend

40c4e51

- Add plugin:po_to_json rake task - Load js locale files for all plugins - Make i18n.js do multi-domain lookup

adamruzicka dismissed ekohl’s stale review via 40c4e51 May 17, 2023 11:01

adamruzicka force-pushed the potojson branch from 33c524d to 40c4e51 Compare May 17, 2023 11:01

MariaAga reviewed May 17, 2023

View reviewed changes

ekohl approved these changes May 17, 2023

View reviewed changes

ekohl merged commit e848148 into theforeman:develop May 17, 2023

adamruzicka deleted the potojson branch May 17, 2023 12:23

ekohl mentioned this pull request Jun 30, 2023

Refs #36330 - Place assets into plugin dir #9711

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #36330 - Make translated strings from plugins available in the browser and modern frontend code #9691

Fixes #36330 - Make translated strings from plugins available in the browser and modern frontend code #9691

adamruzicka commented Apr 26, 2023 •

edited

Loading

theforeman-bot commented Apr 26, 2023

ekohl left a comment

ekohl Apr 26, 2023

adamruzicka Apr 26, 2023

ekohl Apr 26, 2023

adamruzicka commented Apr 26, 2023

ekohl commented Apr 26, 2023

ekohl left a comment

ekohl left a comment

ekohl left a comment

ekohl left a comment

ekohl May 15, 2023

adamruzicka May 15, 2023

adamruzicka commented May 16, 2023

ekohl left a comment

ekohl May 17, 2023

adamruzicka May 17, 2023 •

edited

Loading

MariaAga May 17, 2023

adamruzicka May 17, 2023

ekohl left a comment

Fixes #36330 - Make translated strings from plugins available in the browser and modern frontend code #9691

Fixes #36330 - Make translated strings from plugins available in the browser and modern frontend code #9691

Conversation

adamruzicka commented Apr 26, 2023 • edited Loading

Concerns

Followup changes

theforeman-bot commented Apr 26, 2023

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamruzicka commented Apr 26, 2023

ekohl commented Apr 26, 2023

ekohl left a comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamruzicka commented May 16, 2023

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamruzicka May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

adamruzicka commented Apr 26, 2023 •

edited

Loading

adamruzicka May 17, 2023 •

edited

Loading