Fixes #36575 - Avoid translating labels during startup #9771

ekohl · 2023-07-07T15:29:54Z

It's invalid to do translations during startup, such as in models or in controllers. You should always use N_() (see https://projects.theforeman.org/projects/foreman/wiki/Translating#Extracting-strings). This is because when it's running you actually don't have translations at all, so it's effectively a noop anyway.

When updating fast_gettext to 2.1+ (#9770) it turns into infinite recursion so it must be addressed if we want to move forward.

theforeman-bot · 2023-07-07T15:29:56Z

Issues: #36575

ekohl · 2023-07-07T16:30:21Z

@ofedoren I've added some TODOs with procs, but I doubt it's valid. Mind having a look?

ekohl · 2023-07-07T16:43:29Z

app/models/domain.rb

@@ -51,7 +51,7 @@ class Domain < ApplicationRecord

  set_crud_hooks :domain

-  apipie :class, desc: "A class representing #{model_name.human} object" do
+  apipie :class, desc: "A class representing #{humanize_class_name} object" do


I just realized this actually doesn't exist and we need something else to describe the object.

Well, since the class names mostly taken as is for DSL, unless they explicitly renamed, it's okay to leave this untranslated and closer to something that is used within methods/return values. Although, there is a need to review all DSL documentation and make sure it's actually consistent.

Also, this whole string is not being translated...

ofedoren · 2023-07-10T14:13:20Z

app/controllers/concerns/api/v2/taxonomies_controller.rb

@@ -33,7 +33,8 @@ module Api::V2::TaxonomiesController
      param :ignore_types, Array, N_("List of resources types that will be automatically associated"), :required => false
      resource_name = (param_name == :location) ? 'organization' : 'location'
      resource_ids = "#{resource_name}_ids".to_sym
-      param resource_ids, Array, N_("Associated %{resource} IDs") % { resource: _(resource_name) }, :required => false
+      # TODO: translate resource_name?


I'm dumb for writing _(resource_name) instead of simply translating taxonomy names. Although, due to some language specialties, this would seems odd in the end anyway.

In this case, I'd better be more explicit or WET:

# ... mapping = { location: ['organization', N_("Associated organization IDs")], organization: ['location', N_("Associated location IDs")] } resource_ids = "#{mapping[param_name]&.at(0)}_ids".to_sym # & is just to be sure it doesn't crash, but can be omitted I guess # ... param resource_ids, Array, mapping[param_name]&.at(1), :required => false # & is for the same reason, can be changed to [1]

Ok, I'll expand it then.

ofedoren · 2023-07-10T14:16:04Z

app/models/domain.rb

@@ -51,7 +51,7 @@ class Domain < ApplicationRecord

  set_crud_hooks :domain

-  apipie :class, desc: "A class representing #{model_name.human} object" do
+  apipie :class, desc: "A class representing #{humanize_class_name} object" do


Well, since the class names mostly taken as is for DSL, unless they explicitly renamed, it's okay to leave this untranslated and closer to something that is used within methods/return values. Although, there is a need to review all DSL documentation and make sure it's actually consistent.

Also, this whole string is not being translated...

ekohl · 2023-08-01T18:34:47Z

@ofedoren any idea if apipie descriptions can be callable? That way we can have translatable descriptions without having to rewrite so much.

ofedoren · 2023-08-30T12:00:47Z

@ekohl, so sorry for the late reply.

It's not that they can be callable, but AFAIU we can defer the actual translation: https://github.com/theforeman/foreman/blob/develop/config/initializers/apipie.rb#L68. translate method should be called not at the initialization time, but rather "on demand", when the docs are being rendered in one way or another. Same should work for ApipieDSL, but didn't test though...

ekohl · 2023-10-06T15:04:44Z

I split off some of the changes in #9854 for easier review.

ekohl · 2023-10-09T09:43:40Z

Rebased on develop now #9854 is merged. This reduces the scope to apipie docs.

ekohl · 2023-10-09T09:46:45Z

app/models/domain.rb

@@ -51,7 +51,7 @@ class Domain < ApplicationRecord

  set_crud_hooks :domain

-  apipie :class, desc: "A class representing #{model_name.human} object" do
+  apipie :class, desc: "A class representing #{model_name.singular.humanize(capitalize: false)} object" do


@ofedoren I wonder if it makes sense to enhance apipie to provide a desc_from_class or some other way to automatically derive this.

It seems like all the descriptions are quite the same, so yeah, it makes sense to add something like default_desc. Since I've noticed that apipie-dsl needs some love, I'm going to add this as well.

Since, ideally we want to support translation for all the strings (and apipie-dsl doesn't have dependency on gettext), we shouldn't hardcode such string in the lib, but somehow provide it to be evaluated in the scope of a certain class. My suggestion is to add a new entry called e.g. default_model_description to the config (https://github.com/theforeman/foreman/blob/develop/config/initializers/apipie.rb#L3), which will accept a proc, which will be evaluated in the class (to be able to call model_name) and save the string in the object. This could be translated later via callback we provide in the same config. It would look like:

ApipieDSL.configure do |config| # ... config.default_model_description = ->(model) { N_("A class representing %s object") % model.model_name.singular.humanize(capitalize: false) } # ... end

That will be applied to all documented models unless there is an explicit override, such as :desc option or short method is called within a block. WDYT?

You do provide a translate method as ApipieDSL.translate(str, locale):

https://github.com/Apipie/apipie-dsl/blob/f7118ed268449199272ce1245b3e30aece84c7f8/lib/apipie_dsl/application.rb#L214-L220

AFAIK it doesn't support interpolating the translated strings with variables, which makes it rather hard to use. I think that would still be needed.

In the context of Foreman: do we configure the translate method:

foreman/config/initializers/apipie.rb

Lines 22 to 28 in d8200ee

config.translate = lambda do |str, loc|

old_loc = FastGettext.locale

FastGettext.set_locale(loc)

trans = _(str) if str

FastGettext.set_locale(old_loc)

trans

end

And I think we should rewrite that to use FastGettext.with_locale:

config.translate = lambda do |str, loc| str ? FastGettext.with_locale(loc) { _(str) } : nil end

So I think your suggestion makes sense, but I'm not sure how to hook it up properly.

archanaserver · 2023-10-12T07:02:12Z

app/models/taxonomy.rb

        sections only: %w[all additional]
-        name_exl, title_exl = class_scope.model_name.human == 'Location' ? ['Europe', 'Europe/Prague'] : ['Red Hat', 'Red Hat/Engineering']
+        name_exl, title_exl = model_name.to_s == 'Location' ? ['Europe', 'Europe/Prague'] : ['Red Hat', 'Red Hat/Engineering']


@ekohl I believe we can consider using class_scope here to determine the context of the model and this will help us to resolve the apipie issue.

name_exl, title_exl = class_scope.model_name.to_s == 'Location' ? ['Europe', 'Europe/Prague'] : ['Red Hat', 'Red Hat/Engineering']

Oh, good catch.

archanaserver · 2023-10-12T12:07:52Z

@ekohl should I raise a new PR to resolve the rubocop issue that I can see here, on top of this PR?

and what does single commit workflow mean here?

ekohl · 2023-10-12T13:15:17Z

@ekohl should I raise a new PR to resolve the rubocop issue that I can see here, on top of this PR?

I'll address it.

and what does single commit workflow mean here?

Generally speaking a PR should have a single commit. Sometimes contributors add small commits to fix review comments, but that makes it hard to cherry pick into stable releases. That check signals reviewers they should pay attention to it. In this case I chose it and I think it makes sense because I'm fixing individual commits. This is then a discussion a reviewer can have with the author. The check is not blocking, but GH isn't great a providing alternatives here.

ofedoren · 2023-10-12T17:25:47Z

@ekohl, I've released apipie-dsl-2.6.0. Along with some dependencies upgrades it supports now default_class_description method for configuration, so you can try out my suggestion above. In that case, don't forget to simply remove all desc: options instead of updating them.

I'd also highly recommend to add to the config block (properly supported since 2.6.0 :/):

# ...
config.reload_dsl = false
# ...

Otherwise it's a pain to load the pages in development.

ekohl · 2023-10-13T09:54:51Z

@archanaserver is updating to apipie-dsl 2.6.0 something you could look at? It would probably supersede this PR

archanaserver · 2023-10-13T12:23:56Z

@archanaserver is updating to apipie-dsl 2.6.0 something you could look at? It would probably supersede this PR

Sure.

ekohl · 2023-10-23T10:02:10Z

Needs to be rebased after #9867 is merged.

ekohl · 2024-02-29T10:44:50Z

@ofedoren can you take a look at this? I still think these translations are executed on application startup, which can't work.

theforeman-bot added the Not yet reviewed label Jul 7, 2023

ekohl mentioned this pull request Jul 7, 2023

Fixes #36574 - Update fast_gettext to ~> 2.1 #9770

Merged

ekohl force-pushed the 36575-avoid-translations-during-startup branch 2 times, most recently from 814d2b1 to e902a84 Compare July 7, 2023 16:27

ekohl marked this pull request as draft July 7, 2023 16:27

theforeman-bot added the Waiting on contributor label Jul 7, 2023

ekohl mentioned this pull request Jul 7, 2023

Rails 7 & Ruby 3.1 #9328

Closed

ekohl commented Jul 7, 2023

View reviewed changes

ofedoren reviewed Jul 10, 2023

View reviewed changes

ekohl force-pushed the 36575-avoid-translations-during-startup branch from 1eb6c35 to 7c974b4 Compare July 11, 2023 11:58

theforeman-bot removed the Waiting on contributor label Jul 11, 2023

ekohl mentioned this pull request Oct 6, 2023

Refs #36575 - Avoid some translations during startup #9854

Merged

ekohl force-pushed the 36575-avoid-translations-during-startup branch from 7c974b4 to 9e7596c Compare October 9, 2023 09:42

ekohl commented Oct 9, 2023

View reviewed changes

archanaserver suggested changes Oct 12, 2023

View reviewed changes

pr-processor bot added Waiting on contributor and removed Not yet reviewed labels Oct 12, 2023

ekohl force-pushed the 36575-avoid-translations-during-startup branch from 9e7596c to 46931e4 Compare October 12, 2023 10:08

pr-processor bot added Needs re-review and removed Waiting on contributor labels Oct 12, 2023

ekohl mentioned this pull request Oct 19, 2023

Fixes #36575 - Use apipie-dsl default model descriptions #9867

Merged

Refs #36575 - [WIP] Avoid i18n during startup

dd01d0e

ekohl force-pushed the 36575-avoid-translations-during-startup branch from 46931e4 to dd01d0e Compare November 16, 2023 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #36575 - Avoid translating labels during startup #9771

Fixes #36575 - Avoid translating labels during startup #9771

ekohl commented Jul 7, 2023 •

edited

Loading

theforeman-bot commented Jul 7, 2023

ekohl commented Jul 7, 2023

ekohl Jul 7, 2023

ofedoren Jul 10, 2023

ofedoren Jul 10, 2023

ekohl Jul 11, 2023

ofedoren Jul 10, 2023

ekohl commented Aug 1, 2023

ofedoren commented Aug 30, 2023

ekohl commented Oct 6, 2023

ekohl commented Oct 9, 2023

ekohl Oct 9, 2023

ofedoren Oct 12, 2023

ekohl Oct 12, 2023

archanaserver Oct 12, 2023

ekohl Oct 12, 2023

archanaserver commented Oct 12, 2023 •

edited

Loading

ekohl commented Oct 12, 2023

ofedoren commented Oct 12, 2023

ekohl commented Oct 13, 2023

archanaserver commented Oct 13, 2023

ekohl commented Oct 23, 2023

ekohl commented Feb 29, 2024

	config.translate = lambda do \|str, loc\|
	old_loc = FastGettext.locale
	FastGettext.set_locale(loc)
	trans = _(str) if str
	FastGettext.set_locale(old_loc)
	trans
	end

Fixes #36575 - Avoid translating labels during startup #9771

Are you sure you want to change the base?

Fixes #36575 - Avoid translating labels during startup #9771

Conversation

ekohl commented Jul 7, 2023 • edited Loading

theforeman-bot commented Jul 7, 2023

ekohl commented Jul 7, 2023

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

Choose a reason for hiding this comment

ekohl commented Aug 1, 2023

ofedoren commented Aug 30, 2023

ekohl commented Oct 6, 2023

ekohl commented Oct 9, 2023

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

Choose a reason for hiding this comment

archanaserver commented Oct 12, 2023 • edited Loading

ekohl commented Oct 12, 2023

ofedoren commented Oct 12, 2023

ekohl commented Oct 13, 2023

archanaserver commented Oct 13, 2023

ekohl commented Oct 23, 2023

ekohl commented Feb 29, 2024

ekohl commented Jul 7, 2023 •

edited

Loading

archanaserver commented Oct 12, 2023 •

edited

Loading