ActiveRecord extension for pluggable human model names

[mzazrivec]

Currently, we store human readable / understandable model names in locale/en.yml under model: subtree. On top of this, we have Dictionary.gettext() and ui_lookup() to return a nice looking model name for given model / class.

Problem with this implementation is:

• it's not standard gettext, so we have to extract the strings from yaml in a special way and construct string plurals during the extraction
• current implementation doesn't support multiple plural forms, it simply assumes one plural form only
• current implementation is not pluggable: all models (even the provider ones) have to maintain their pretty looking model names in the core repo.

What this PR proposes is an extension to ActiveRecord: class & instance method human_model_name(), which would, for given class or class instance, return human looking model name. The extension itself would obviously be just a fallback, every model would have to override this method on its own:

irb(main):001:0> MiqReport.human_model_name
=> "Miq Report"
irb(main):002:0> MiqReport.human_model_name 2
=> "Miq Reports"
irb(main):003:0> MiqReport.first.human_model_name 2
=> "Miq Reports"

and with the following change in app/models/miq_report.rb:

--- a/app/models/miq_report.rb
+++ b/app/models/miq_report.rb
+  def self.human_model_name(number = 1)
+    n_('Report', 'Reports', number)
+  end

we'd see:

irb(main):001:0> MiqReport.human_model_name
=> "Report"
irb(main):002:0> MiqReport.human_model_name 2
=> "Reports"

PR migrating current content of locale/en.yml into models would come separately.

[martinpovolny]

I like this proposal ❤️

• It moves us forward with the remainign i18n tasks by replacing one of the remaining weirdnesses with standard gettext
• It improves the i18n possibilities (multiple plurals for models)
• It solves the pluggability aspect for models

[himdel]

LGTM as well 👍

The only thing.. will this work together well with rails reloads in devel mode?

Shouldn't this be a part of app/models/application_record.rb instead of changing ActiveRecord::Base? (There's no miq models inheriting from ActiveRecord::Base directly, all inherit from ApplicationRecord.)

[mzazrivec]

@chessbyte @Fryguy What do you guys think of this? What do you think about the ActiveRecord::Base vs ApplicationRecord thing?

[bdunne]

I like where this is headed, but I have a comments:

• I'd prefer to put this in ApplicationRecord since nothing should inherit directly from ActiveRecord::Base
• There are some models that don't inherit from ApplicationRecord (they're not backed by the database). As a first pass, I probably wouldn't worry about those.
• I don't like the instance method. The class name isn't accessible directly off of the instance, instead you would do instance.class.name and I think this should behave similarly (only implement it on the class).
• Is there any way to get rid of the parameter?
• Should it be called display_name?

[martinpovolny]

Is there any way to get rid of the parameter?

What do you mean by that? How do you provide the count of items w/o the parameter?

[bdunne]

@martinpovolny It feels strange to me to have the pluralization logic in the class since i think of that as a presentation detail. Are there some examples of the callers? Maybe that would help clear up where the pluralization logic belongs.

[bdunne]

I just noticed it now, but I also don't like the translation being here.

[mzazrivec]

The thing with the plural here is that we simply have to have n_("Singular", "Plural", no) somewhere for this to be correctly included into the catalog as a string, that needs to be translated as singular and all applicable plural forms. Otherwise we'd be where we're right now.

Other than this I don't have problem with the changes requested here.

[mzazrivec]

Also, regarding the argument that this is just a presentation detail -- ordinarily I would not do this change in core. I would simply include these model names into the decorator classes that we use in ui-classic. But since ui_lookup() and Dictionary.gettext() is being used all over the place (core, ui, provider classes), I canont really do that. Not unless we want to have some strange coupling between UI and the core repos.

[Fryguy]

> I'd prefer to put this in ApplicationRecord since nothing should inherit directly from ActiveRecord::Base

More specifically, create this as a module extension in lib/extensions, prefixed with ar_, then include the module in ApplicationRecord.

EDIT: I misread this as an extension to ActiveRecord, so ignore this.

[Fryguy]

But since ui_lookup() and Dictionary.gettext() is being used all over the place (core, ui, provider classes), I canont really do that.

Can we discuss this then? There a bunch of places where we translate exceptions, which makes no sense. If we get diagnostic logs in a language we don't speak, how are we supposed to diagnose the problem? I think the reason it was done is because the UI takes Exception objects and presents them directly in the UI, but if that's the reason, is there a better way this can be done?

[martinpovolny]

@Fryguy:

Can we discuss this then? There a bunch of places where we translate exceptions, which makes no sense. If we get diagnostic logs in a language we don't speak, how are we supposed to diagnose the problem? I think the reason it was done is because the UI takes Exception objects and presents them directly in the UI, but if that's the reason, is there a better way this can be done?

Can we rather stick to the topic of the PR?

We are not trying to change how exceptions are being translated. Also we are not working on removing the translation process from the backend in this PR.

devil - [~/Projects/manageiq] (master)$ grep -r '_(' app/ | wc -l
801

This ^^^ is not about exceptions. There are many translations in the core and in the providers.

There's a number of goals explained in the description or this PR. Also we are trying to limit the number of different ways how translations are made.

This PR is a part of the i18n effort that is in progress for some time and we should finish it.

@bdunne :

I just noticed it now, but I also don't like the translation being here.

The strings for translations are in the core repo. And they are in the core repo in different places in different forms. What this PR is trying to do is to actually limit the number of different forms that these strings have.

If the next step would be to remove the strings from this repo then this next step will actually be simpler because of the work done in this PR and follow up PRs that will lead to removal of the locale/en.yml file and the ui_lookup* methods.

As @mzazrivec wrote, the methods that we need for translations of model names could live in some decorators. That can be done in the UI repo. But not right now.

Right now, the core repo and the providers use the translations. @mzazrivec has been removing some translations even from the API repo last week.

[bdunne]

@martinpovolny I hope my review didn't come across the wrong way because I really do like the idea of the models having a display name and implementing that in ApplicationRecord (with overrides) makes perfect sense.
I have no knowledge of any i18n effort. A few of us are concerned with how parts of the core repo are tied to i18n, so any time more translations are added I question it hoping to find a better way to solve the problem. I understand that this may be a single step in a longer journey, but I also don't know where this journey is taking us 😄. Can you shed some light on it (or a link to a document)?

[Fryguy]

Can we rather stick to the topic of the PR?

@martinpovolny I am...just because I want to discuss something, doesn't mean I won't let this PR move forward. (FWIW, I think this PR is good, and I'd approve if it aside from the direct modifications to ActiveRecord::Base). However, after discussing we may feel that this PR is not enough, the wrong direction...could be anything. Also, I'm curious about what the backend translations are for to begin with...I was under the impression that most were translated exceptions, so even the fact that you've told me they aren't is more info than I had before for making a decision. 😄

It feels strange to me to have the pluralization logic in the class since i think of that as a presentation detail.

@bdunne It's not a presentation detail, it's fundamental to translating something. You need to know what kind of plural you want, and how many, as some languages have different plural forms depending on the number of objects. You could maybe argue that translations, in general, are a presentation detail, but you can't single out the pluralization part of it.

[Fryguy]

Also, I don't see why this can't be gaprindashvili/yes. It doesn't really change anything, and if you end up backporting something, then the translations would just work. Or does the

PR migrating current content of locale/en.yml into models would come separately.

cause it to be gaprindashvili/no ?

[mzazrivec]

My primary motivation for this PR was to get rid of locale/en.yml (or at least big chunk of it) and make this one tiny aspect of our application pluggable.

As things are, the model names have to be translatable, so I have to use gettext here. This really is not changing compared to what we have now: we do translate model names now as well, we just do it in a non-standard way. The fact that I get to use n_() here and be able to support multiple plurals is a nice bonus.

I certainly understand the point that this is may seem like a presentation detail and normally I would do this in ui-classic in model decorators. But that's not possible at this moment, since there are some ui_lookup() and Dictionary.gettext() invocations present in the core. I'd like to get rid of them (see #16267 or #16682 for example), but we probably won't be able to get rid of the concept entirely, especially because of reporting and chargebacks.

The part about logging localized (or partly localized) content: yes, that should not be happening and I'll be happy to look at the problem and fix it.

Why gaprindashvili/no -- the fact that this would change how we translate models (n_() instead of plain _()) and the fact that translations for gaprindashvili have already landed would mean that we'd have to re-do the translations again. I'd like to avoid that as much as possible.

Regarding the general directions and big picture: we're heading towards a concept of UI built on top of rest API, right? The current concept is that the API would not be sending translated strings. This doesn't mean there won't be any gettext in the core though: the strings still will have to be at least marked for translation (with N_() for example). But otherwise, I'd like for core to contain as few gettext strings as possible. What we currently have in the core gettext catalog is info / error strings coming from models (about 8% of the its content), then model attributes (about 50% of its content) and finally strings extracted from yamls: product features, charts, reports and locale/en.yml (about 42% of its content).

[martinpovolny]

Why gaprindashvili/no -- the fact that this would change how we translate models (n_() instead of plain _()) and the fact that translations for gaprindashvili have already landed would mean that we'd have to re-do the translations again. I'd like to avoid that as much as possible.

That would likely involve adding extra work to the translator teams.

[mzazrivec]

I made the changes as Brandon suggested:

• moved the code into ApplicationRecord
• renamed the routine to display_name()
• got rid of the instance method

[miq-bot]

Checked commit mzazrivec@c59e45a with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

[bdunne]

I'm good with this for now 👍 Thnaks @mzazrivec