Migrate model display names from locale/en.yml to models #16836

mzazrivec · 2018-01-17T17:02:45Z

This is a continuation of the work started in #16596 where we want to migrate display model names from locale/en.yml into particular models, where we'll use standard gettext. Main goal here is to make this one aspect of our application pluggable.

The strings were migrated automatically and adjusted where needed (incorrect plurals and such), some of the display names were changed:

EVM Audit Event -> Audit Event
Chargeback for Vms -> Chargeback for VMs
Host / Nodes -> Hosts / Nodes
EVM Group -> Group
EVM User -> User
VMDB Table -> Table

Next step would be to switch ui_lookup() and Dictionary.gettext() to use display_name() rather than I18n.t() and locale.en.yml.

gaprindashvili/no

mzazrivec · 2018-01-25T15:29:21Z

@bdunne @Fryguy review please?

mzazrivec · 2018-01-25T15:29:38Z

Or even @martinpovolny ?

bdunne · 2018-01-25T16:00:04Z

app/models/container_group.rb

@@ -95,4 +95,8 @@ def disconnect_inv
    self.deleted_on = Time.now.utc
    save
  end
+
+  def self.display_name(number = 1)
+    n_('Container Pod', 'Container Pods', number)


Should this just be "Pod" / "Pods"?

Also, note that this term may not apply to other variants in the future, which is why we made the model name generic.

bdunne · 2018-01-25T16:03:13Z

app/models/configuration_script_source.rb

@@ -4,4 +4,8 @@ class ConfigurationScriptSource < ApplicationRecord
  belongs_to  :manager, :class_name => "ExtManagementSystem"

  virtual_total :total_payloads, :configuration_script_payloads
+
+  def self.display_name(number = 1)
+    n_('Repository', 'Repositories', number)


Note that this term may not apply to other variants in the future, which is why we made the model name generic. It works for Ansible, but that's not the only thing that can use this table.

bdunne · 2018-01-25T16:03:50Z

app/models/container_group_performance.rb

@@ -2,4 +2,8 @@ class ContainerGroupPerformance < MetricRollup
  default_scope { where("resource_type = 'ContainerGroup' and resource_id IS NOT NULL") }

  belongs_to :container_group, :foreign_key => :resource_id, :class_name => ContainerGroup.name
+
+  def self.display_name(number = 1)
+    n_('Performance - Pod', 'Performance - Pods', number)


bdunne · 2018-01-25T16:08:38Z

app/models/container_node_performance.rb

@@ -2,4 +2,8 @@ class ContainerNodePerformance < MetricRollup
  default_scope { where("resource_type = 'ContainerNode' and resource_id IS NOT NULL") }

  belongs_to :container_node, :foreign_key => :resource_id, :class_name => ContainerNode.name
+
+  def self.display_name(number = 1)
+    n_('Performance - Container Node', 'Performance - Container Nodes', number)


Should these be "Container Node Performance" and "Container Node Performances"? I think they are performance records for a single node.

bdunne · 2018-01-25T16:14:13Z

app/models/ems_cluster.rb

@@ -336,4 +336,8 @@ def self.non_openstack_clusters_exists?
  def openstack_cluster?
    ext_management_system.class == ManageIQ::Providers::Openstack::InfraManager
  end
+
+  def self.display_name(number = 1)
+    n_('Cluster / Deployment Role', 'Cluster / Deployment Roles', number)


Should the plural be "Clusters / Deployment Roles"?

Shouldn't "Cluster" also be pluralized?

Yes, this has been corrected to 'Clusters / Deployment Roles'.

bdunne · 2018-01-25T16:14:51Z

app/models/ems_cluster_performance.rb

@@ -2,4 +2,8 @@ class EmsClusterPerformance < MetricRollup
  default_scope { where("resource_type = 'EmsCluster' and resource_id IS NOT NULL") }

  belongs_to :ems_cluster, :foreign_key => :resource_id
+
+  def self.display_name(number = 1)
+    n_('Performance - Cluster', 'Performance - Clusters', number)


Again, "Cluster Performances"?

bdunne · 2018-01-25T16:16:06Z

app/models/ext_management_system.rb

@@ -674,6 +674,10 @@ def tenant_identity
    User.super_admin.tap { |u| u.current_group = tenant.default_miq_group }
  end

+  def self.display_name(number = 1)
+    n_('Provider', 'Providers', number)


This conflicts with the Provider model. They're not the same thing

martinpovolny · 2018-05-23T16:03:45Z

I'm fine with these methods being present at the model layer, just as I was ok with it in #16596 . I'm fine with override methods where the chosen string is different than the model name, however, where I object is the overrides that do exactly the same thing as the method in ApplicationRecord, making them basically redundant. I gave an alternate suggestion above - #16836 (comment)

The gettext thing is about collecting static strings from the sources. We have strings in the sources. Also we already have these particular static strings in the repository right now. In the YAMLs. Just the placement of these few strings is non-standard and is not pluggable and it requires an extra rake task to collect.

Instead of fixing that by moving the strings in the sources (where the other strings already are) you ask to create a new special rake task to collect the strings from the models.

Yes, it will work. But no, it's not the default or standard way to do it. Does it have benefits? Well you will not have those methods in core. A method per model. Other benefits? I don't see any.

Negative points: an extra rake task + handling some strings differently than others.

Furthemore the part:

exactly the same thing as the method in ApplicationRecord, making them basically redundant.

is simply not true. The method would not be the same. It would contain different strings.

jrafanie · 2018-05-24T15:22:06Z

I'm letting myself out of this PR review. I'm 🤷‍♀️ with the changes but if others are ok with it once rebased and all questions answered, then I'm 👍

My only comment is, we have what appears to many of us as unnecessary duplication that is hard for people who know our gettext steps to explain. I will continue to look at this code and not understand why the duplication is needed instead of an intermediate step that populates the strings in a pre-gettext step. But alas, I really don't spend time here so it won't bother me that often.

mzazrivec · 2018-05-24T15:43:23Z

I'm currently working on a solution (as suggested in previous comments) that would address the duplication concerns.

mzazrivec · 2018-05-25T14:56:10Z

Based on the previous discussion, I made the following changes:

I removed explicit display_name() for models, which can have their display names created automatically
created rake task locale:model_display_names which creates display names automatically for all models, which do not have explicit display_name() in them
locale:model_display_names is being called as a part of main i18n task: locale:update

bdunne · 2018-05-25T16:21:09Z

lib/tasks/locale.rake

+  task "model_display_names" => :environment do
+    f = File.open(Rails.root.join("config/model_display_names.rb"), "w+")
+    Rails.application.eager_load!
+    ApplicationRecord.descendants.sort_by(&:display_name).collect do |model|


Just a warning... there are some non-ApplicationRecord models. But I do prefer this approach

Is it ok if we're looking for descendants of ApplicationRecord then?

I don't know. If you only need a display name for things found in the database, then yes.

Or maybe those non-ApplicationRecord classes are just required to implement the display_name method. I could be okay with that until we find a better solution.

mzazrivec · 2018-05-29T14:48:31Z

This is what would the new rake task currently produce: https://gist.github.com/mzazrivec/ada935b99a67ceb5014d1893a5b3ec78

miq-bot · 2018-05-30T21:52:15Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2018-06-04T15:11:47Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2018-06-05T12:46:37Z

Some comments on commits mzazrivec/manageiq@cfdd29b~...4d31b2a

lib/tasks/locale.rake

⚠️ - 226 - Detected puts. Remove all debugging statements.

miq-bot · 2018-06-05T12:48:02Z

Checked commits mzazrivec/manageiq@cfdd29b~...4d31b2a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
152 files checked, 1 offense detected

lib/tasks/locale.rake

❗ - Line 222, Col 19 - Rails/FilePath - Please use Rails.root.join('path', 'to') instead.

martinpovolny · 2018-06-06T15:14:59Z

@bdunne, @Fryguy : can we move forward with this?

bdunne · 2018-06-07T22:08:20Z

miq-bot added the wip label Jan 17, 2018

mzazrivec force-pushed the migrate_display_names_for_models branch 2 times, most recently from 5d54487 to 4839295 Compare January 17, 2018 17:21

mzazrivec changed the title ~~[WIP] Migrate model display names from locale/en.yml to models~~ Migrate model display names from locale/en.yml to models Jan 18, 2018

miq-bot removed the wip label Jan 18, 2018

chessbyte added internationalization gaprindashvili/no labels Jan 23, 2018

mzazrivec force-pushed the migrate_display_names_for_models branch from 4839295 to 66b808b Compare January 25, 2018 15:26

bdunne reviewed Jan 25, 2018

View reviewed changes

mzazrivec force-pushed the migrate_display_names_for_models branch 3 times, most recently from c2941d4 to 56fec1a Compare May 25, 2018 14:54

bdunne reviewed May 25, 2018

View reviewed changes

mzazrivec force-pushed the migrate_display_names_for_models branch from 56fec1a to 02212ec Compare May 29, 2018 09:22

miq-bot added the unmergeable label May 30, 2018

mzazrivec force-pushed the migrate_display_names_for_models branch from 02212ec to f1d1e0e Compare May 31, 2018 10:46

miq-bot removed the unmergeable label May 31, 2018

miq-bot added the unmergeable label Jun 4, 2018

mzazrivec added 3 commits June 5, 2018 14:40

Migrate model display names from locale/en.yml to models

cfdd29b

locale:model_display_names: automatic creation of model display names

09efe3f

Call model_display_names as a part of locale:update

4d31b2a

mzazrivec force-pushed the migrate_display_names_for_models branch from f1d1e0e to 4d31b2a Compare June 5, 2018 12:41

miq-bot removed the unmergeable label Jun 5, 2018

bdunne approved these changes Jun 7, 2018

View reviewed changes

bdunne merged commit 0bd16ab into ManageIQ:master Jun 7, 2018

bdunne self-assigned this Jun 7, 2018

bdunne added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 7, 2018

mzazrivec deleted the migrate_display_names_for_models branch June 8, 2018 06:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate model display names from locale/en.yml to models #16836

Migrate model display names from locale/en.yml to models #16836

mzazrivec commented Jan 17, 2018 •

edited

Loading

mzazrivec commented Jan 25, 2018

mzazrivec commented Jan 25, 2018

bdunne Jan 25, 2018

bdunne Jan 25, 2018

bdunne Jan 25, 2018

bdunne Jan 25, 2018

bdunne Jan 25, 2018

bdunne Jan 25, 2018

bdunne Mar 7, 2018

mzazrivec Mar 8, 2018

bdunne Jan 25, 2018

bdunne Jan 25, 2018

martinpovolny commented May 23, 2018 •

edited

Loading

jrafanie commented May 24, 2018

mzazrivec commented May 24, 2018

mzazrivec commented May 25, 2018

bdunne May 25, 2018

mzazrivec May 25, 2018

bdunne May 25, 2018

bdunne May 25, 2018

mzazrivec commented May 29, 2018

miq-bot commented May 30, 2018

miq-bot commented Jun 4, 2018

miq-bot commented Jun 5, 2018

miq-bot commented Jun 5, 2018

martinpovolny commented Jun 6, 2018

bdunne commented Jun 7, 2018

Migrate model display names from locale/en.yml to models #16836

Migrate model display names from locale/en.yml to models #16836

Conversation

mzazrivec commented Jan 17, 2018 • edited Loading

mzazrivec commented Jan 25, 2018

mzazrivec commented Jan 25, 2018

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

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

martinpovolny commented May 23, 2018 • edited Loading

jrafanie commented May 24, 2018

mzazrivec commented May 24, 2018

mzazrivec commented May 25, 2018

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

mzazrivec commented May 29, 2018

miq-bot commented May 30, 2018

miq-bot commented Jun 4, 2018

miq-bot commented Jun 5, 2018

miq-bot commented Jun 5, 2018

martinpovolny commented Jun 6, 2018

bdunne commented Jun 7, 2018

mzazrivec commented Jan 17, 2018 •

edited

Loading

martinpovolny commented May 23, 2018 •

edited

Loading