Merge branch 'cop-for-preventing-translation-assignment-to-constants' into 'master'

Rubocop rule to prevent assigning translations to constants

See merge request gitlab-org/gitlab!29031

.rubocop.yml

@@ -27,6 +27,12 @@ AllCops:
     - 'plugins/**/*'
   CacheRootDirectory: tmp
 
+Cop/StaticTranslationDefinition:
+  Enabled: true
+  Exclude:
+    - 'spec/**/*'
+    - 'ee/spec/**/*'
+
 # This cop checks whether some constant value isn't a
 # mutable literal (e.g. array or hash).
 Style/MutableConstant:

app/helpers/preferences_helper.rb

@@ -9,8 +9,22 @@ module PreferencesHelper
     ]
   end
 
+  # Returns an Array usable by a select field for more user-friendly option text
+  def dashboard_choices
+    dashboards = User.dashboards.keys
+
+    validate_dashboard_choices!(dashboards)
+    dashboards -= excluded_dashboard_choices
+
+    dashboards.map do |key|
+      # Use `fetch` so `KeyError` gets raised when a key is missing
+      [localized_dashboard_choices.fetch(key), key]
+    end
+  end
+
   # Maps `dashboard` values to more user-friendly option text
-  DASHBOARD_CHOICES = {
+  def localized_dashboard_choices
+    {
       projects: _("Your Projects (default)"),
       stars:    _("Starred Projects"),
       project_activity: _("Your Projects' Activity"),
@@ -21,18 +35,6 @@ module PreferencesHelper
       merge_requests: _("Assigned Merge Requests"),
       operations: _("Operations Dashboard")
     }.with_indifferent_access.freeze
-
-  # Returns an Array usable by a select field for more user-friendly option text
-  def dashboard_choices
-    dashboards = User.dashboards.keys
-
-    validate_dashboard_choices!(dashboards)
-    dashboards -= excluded_dashboard_choices
-
-    dashboards.map do |key|
-      # Use `fetch` so `KeyError` gets raised when a key is missing
-      [DASHBOARD_CHOICES.fetch(key), key]
-    end
   end
 
   def project_view_choices
@@ -75,9 +77,9 @@ module PreferencesHelper
 
   # Ensure that anyone adding new options updates `DASHBOARD_CHOICES` too
   def validate_dashboard_choices!(user_dashboards)
-    if user_dashboards.size != DASHBOARD_CHOICES.size
+    if user_dashboards.size != localized_dashboard_choices.size
       raise "`User` defines #{user_dashboards.size} dashboard choices," \
-        " but `DASHBOARD_CHOICES` defined #{DASHBOARD_CHOICES.size}."
+        " but `localized_dashboard_choices` defined #{localized_dashboard_choices.size}."
     end
   end
 

app/models/concerns/notification_branch_selection.rb

@@ -6,12 +6,14 @@
 module NotificationBranchSelection
   extend ActiveSupport::Concern
 
-  BRANCH_CHOICES = [
-    [_('All branches'), 'all'],
-    [_('Default branch'), 'default'],
-    [_('Protected branches'), 'protected'],
-    [_('Default branch and protected branches'), 'default_and_protected']
+  def branch_choices
+    [
+      [_('All branches'), 'all'].freeze,
+      [_('Default branch'), 'default'].freeze,
+      [_('Protected branches'), 'protected'].freeze,
+      [_('Default branch and protected branches'), 'default_and_protected'].freeze
     ].freeze
+  end
 
   def notify_for_branch?(data)
     ref = if data[:ref]

app/models/project_services/chat_notification_service.rb

@@ -59,11 +59,11 @@ class ChatNotificationService < Service
 
   def default_fields
     [
-      { type: 'text', name: 'webhook', placeholder: "e.g. #{webhook_placeholder}", required: true },
-      { type: 'text', name: 'username', placeholder: 'e.g. GitLab' },
-      { type: 'checkbox', name: 'notify_only_broken_pipelines' },
-      { type: 'select', name: 'branches_to_be_notified', choices: BRANCH_CHOICES }
-    ]
+      { type: 'text', name: 'webhook', placeholder: "e.g. #{webhook_placeholder}", required: true }.freeze,
+      { type: 'text', name: 'username', placeholder: 'e.g. GitLab' }.freeze,
+      { type: 'checkbox', name: 'notify_only_broken_pipelines' }.freeze,
+      { type: 'select', name: 'branches_to_be_notified', choices: branch_choices }.freeze
+    ].freeze
   end
 
   def execute(data)

app/models/project_services/discord_service.rb

@@ -44,7 +44,7 @@ class DiscordService < ChatNotificationService
     [
       { type: "text", name: "webhook", placeholder: "e.g. https://discordapp.com/api/webhooks/…" },
       { type: "checkbox", name: "notify_only_broken_pipelines" },
-      { type: 'select', name: 'branches_to_be_notified', choices: BRANCH_CHOICES }
+      { type: 'select', name: 'branches_to_be_notified', choices: branch_choices }
     ]
   end
 

app/models/project_services/emails_on_push_service.rb

@@ -66,7 +66,7 @@ class EmailsOnPushService < Service
         help: s_("EmailsOnPushService|Send notifications from the committer's email address if the domain is part of the domain GitLab is running on (e.g. %{domains}).") % { domains: domains } },
       { type: 'checkbox', name: 'disable_diffs', title: s_("EmailsOnPushService|Disable code diffs"),
         help: s_("EmailsOnPushService|Don't include possibly sensitive code diffs in notification body.") },
-      { type: 'select', name: 'branches_to_be_notified', choices: BRANCH_CHOICES },
+      { type: 'select', name: 'branches_to_be_notified', choices: branch_choices },
       { type: 'textarea', name: 'recipients', placeholder: s_('EmailsOnPushService|Emails separated by whitespace') }
     ]
   end

app/models/project_services/hangouts_chat_service.rb

@@ -44,7 +44,7 @@ class HangoutsChatService < ChatNotificationService
     [
       { type: 'text', name: 'webhook', placeholder: "e.g. #{webhook_placeholder}" },
       { type: 'checkbox', name: 'notify_only_broken_pipelines' },
-      { type: 'select', name: 'branches_to_be_notified', choices: BRANCH_CHOICES }
+      { type: 'select', name: 'branches_to_be_notified', choices: branch_choices }
     ]
   end
 

app/models/project_services/microsoft_teams_service.rb

@@ -42,7 +42,7 @@ class MicrosoftTeamsService < ChatNotificationService
     [
       { type: 'text', name: 'webhook', placeholder: "e.g. #{webhook_placeholder}" },
       { type: 'checkbox', name: 'notify_only_broken_pipelines' },
-      { type: 'select', name: 'branches_to_be_notified', choices: BRANCH_CHOICES }
+      { type: 'select', name: 'branches_to_be_notified', choices: branch_choices }
     ]
   end
 

app/models/project_services/pipelines_email_service.rb

@@ -72,7 +72,7 @@ class PipelinesEmailService < Service
         name: 'notify_only_broken_pipelines' },
       { type: 'select',
         name: 'branches_to_be_notified',
-        choices: BRANCH_CHOICES }
+        choices: branch_choices }
     ]
   end
 

app/models/project_services/unify_circuit_service.rb

@@ -38,7 +38,7 @@ class UnifyCircuitService < ChatNotificationService
     [
       { type: 'text', name: 'webhook', placeholder: "e.g. https://circuit.com/rest/v2/webhooks/incoming/…", required: true },
       { type: 'checkbox', name: 'notify_only_broken_pipelines' },
-      { type: 'select', name: 'branches_to_be_notified', choices: BRANCH_CHOICES }
+      { type: 'select', name: 'branches_to_be_notified', choices: branch_choices }
     ]
   end
 

app/presenters/ci/pipeline_presenter.rb

@@ -36,16 +36,18 @@ module Ci
       end
     end
 
-    NAMES = {
+    def localized_names
+      {
         merge_train: s_('Pipeline|Merge train pipeline'),
         merged_result: s_('Pipeline|Merged result pipeline'),
         detached: s_('Pipeline|Detached merge request pipeline')
       }.freeze
+    end
 
     def name
       # Currently, `merge_request_event_type` is the only source to name pipelines
       # but this could be extended with the other types in the future.
-      NAMES.fetch(pipeline.merge_request_event_type, s_('Pipeline|Pipeline'))
+      localized_names.fetch(pipeline.merge_request_event_type, s_('Pipeline|Pipeline'))
     end
 
     def ref_text

app/services/groups/transfer_service.rb

@@ -2,15 +2,6 @@
 
 module Groups
   class TransferService < Groups::BaseService
-    ERROR_MESSAGES = {
-      database_not_supported: s_('TransferGroup|Database is not supported.'),
-      namespace_with_same_path: s_('TransferGroup|The parent group already has a subgroup with the same path.'),
-      group_is_already_root: s_('TransferGroup|Group is already a root group.'),
-      same_parent_as_current: s_('TransferGroup|Group is already associated to the parent group.'),
-      invalid_policies: s_("TransferGroup|You don't have enough permissions."),
-      group_contains_images: s_('TransferGroup|Cannot update the path because there are projects under this group that contain Docker images in their Container Registry. Please remove the images from your projects first and try again.')
-    }.freeze
-
     TransferError = Class.new(StandardError)
 
     attr_reader :error, :new_parent_group
@@ -124,7 +115,18 @@ module Groups
     end
 
     def raise_transfer_error(message)
-      raise TransferError, ERROR_MESSAGES[message]
+      raise TransferError, localized_error_messages[message]
+    end
+
+    def localized_error_messages
+      {
+        database_not_supported: s_('TransferGroup|Database is not supported.'),
+        namespace_with_same_path: s_('TransferGroup|The parent group already has a subgroup with the same path.'),
+        group_is_already_root: s_('TransferGroup|Group is already a root group.'),
+        same_parent_as_current: s_('TransferGroup|Group is already associated to the parent group.'),
+        invalid_policies: s_("TransferGroup|You don't have enough permissions."),
+        group_contains_images: s_('TransferGroup|Cannot update the path because there are projects under this group that contain Docker images in their Container Registry. Please remove the images from your projects first and try again.')
+      }.freeze
     end
   end
 end

doc/development/i18n/externalization.md

@@ -306,6 +306,65 @@ This makes use of [`Intl.DateTimeFormat`](https://developer.mozilla.org/en-US/do
 
 ## Best practices
 
+### Keep translations dynamic
+
+There are cases when it makes sense to keep translations together within an array or a hash.
+
+Examples:
+
+- Mappings for a dropdown list
+- Error messages
+
+To store these kinds of data, using a constant seems like the best choice, however this won't work for translations.
+
+Bad, avoid it:
+
+```ruby
+class MyPresenter
+  MY_LIST = {
+    key_1: _('item 1'),
+    key_2: _('item 2'),
+    key_3: _('item 3')
+  }
+end
+```
+
+The translation method (`_`) will be called when the class is loaded for the first time and translates the text to the default locale. Regardless of what's the user's locale, these values will not be translated again.
+
+Similar thing happens when using class methods with memoization.
+
+Bad, avoid it:
+
+```ruby
+class MyModel
+  def self.list
+    @list ||= {
+      key_1: _('item 1'),
+      key_2: _('item 2'),
+      key_3: _('item 3')
+    }
+  end
+end
+```
+
+This method will memoize the translations using the locale of the user, who first "called" this method.
+
+To avoid these problems, keep the translations dynamic.
+
+Good:
+
+```ruby
+class MyPresenter
+  def self.my_list
+    {
+      key_1: _('item 1'),
+      key_2: _('item 2'),
+      key_3: _('item 3')
+    }.freeze
+  end
+end
+```
+
 ### Splitting sentences
 
 Please never split a sentence as that would assume the sentence grammar and

ee/app/presenters/projects/security/configuration_presenter.rb

@@ -7,15 +7,6 @@ module Projects
 
       presents :project
 
-      SCAN_DESCRIPTIONS = {
-        container_scanning: _('Check your Docker images for known vulnerabilities.'),
-        dast: _('Analyze a review version of your web application.'),
-        dependency_scanning: _('Analyze your dependencies for known vulnerabilities.'),
-        license_management: _('Search your project dependencies for their licenses and apply policies.'),
-        license_scanning: _('Search your project dependencies for their licenses and apply policies.'),
-        sast: _('Analyze your source code for known vulnerabilities.')
-      }.freeze
-
       SCAN_DOCS = {
         container_scanning: 'user/application_security/container_scanning/index',
         dast: 'user/application_security/dast/index',
@@ -25,7 +16,19 @@ module Projects
         sast: 'user/application_security/sast/index'
       }.freeze
 
-      SCAN_NAMES = {
+      def self.localized_scan_descriptions
+        {
+          container_scanning: _('Check your Docker images for known vulnerabilities.'),
+          dast: _('Analyze a review version of your web application.'),
+          dependency_scanning: _('Analyze your dependencies for known vulnerabilities.'),
+          license_management: _('Search your project dependencies for their licenses and apply policies.'),
+          license_scanning: _('Search your project dependencies for their licenses and apply policies.'),
+          sast: _('Analyze your source code for known vulnerabilities.')
+        }.freeze
+      end
+
+      def self.localized_scan_names
+        {
           container_scanning: _('Container Scanning'),
           dast: _('Dynamic Application Security Testing (DAST)'),
           dependency_scanning: _('Dependency Scanning'),
@@ -33,6 +36,7 @@ module Projects
           license_scanning: _('License Compliance'),
           sast: _('Static Application Security Testing (SAST)')
         }.freeze
+      end
 
       def to_h
         {
@@ -95,7 +99,7 @@ module Projects
       # in 13.0 support for `license_management` report type is scheduled to be dropped.
       # With this change we won't need this method anymore.
       def license_compliance_substitute(scans)
-        license_management = scans.find { |scan_type| scan_type[:name] == SCAN_NAMES[:license_management] }
+        license_management = scans.find { |scan_type| scan_type[:name] == localized_scan_names[:license_management] }
         license_compliance_config = license_management.fetch(:configured, false)
 
         scans.delete(license_management)
@@ -112,9 +116,9 @@ module Projects
       def scan(type, configured: false)
         {
           configured: configured,
-          description: SCAN_DESCRIPTIONS[type],
+          description: self.class.localized_scan_descriptions[type],
           link: help_page_path(SCAN_DOCS[type]),
-          name: SCAN_NAMES[type]
+          name: localized_scan_names[type]
         }
       end
 
@@ -125,6 +129,10 @@ module Projects
       def scan_types
         ::Security::SecurityJobsFinder.allowed_job_types + ::Security::LicenseComplianceJobsFinder.allowed_job_types
       end
+
+      def localized_scan_names
+        @localized_scan_names ||= self.class.localized_scan_names
+      end
     end
   end
 end

ee/app/services/ee/groups/transfer_service.rb

@@ -5,9 +5,11 @@ module EE
     module TransferService
       extend ::Gitlab::Utils::Override
 
-      EE_ERROR_MESSAGES = {
+      def localized_ee_error_messages
+        {
           group_contains_npm_packages: s_('TransferGroup|Group contains projects with NPM packages.')
         }.freeze
+      end
 
       def update_group_attributes
         ::Epic.nullify_lost_group_parents(group.self_and_descendants, lost_groups)
@@ -36,7 +38,7 @@ module EE
       end
 
       def raise_ee_transfer_error(message)
-        raise ::Groups::TransferService::TransferError, EE_ERROR_MESSAGES[message]
+        raise ::Groups::TransferService::TransferError, localized_ee_error_messages[message]
       end
 
       def different_root_ancestor?

ee/lib/ee/gitlab/project_template.rb

@@ -5,18 +5,20 @@ module EE
     module ProjectTemplate
       extend ActiveSupport::Concern
 
-      ENTERPRISE_TEMPLATES_TABLE = [
-        ::Gitlab::ProjectTemplate.new('hipaa_audit_protocol', 'HIPAA Audit Protocol', _('A project containing issues for each audit inquiry in the HIPAA Audit Protocol published by the U.S. Department of Health & Human Services'), 'https://gitlab.com/gitlab-org/project-templates/hipaa-audit-protocol')
-      ].freeze
-
       class_methods do
         extend ::Gitlab::Utils::Override
 
+        def localized_ee_templates_table
+          [
+            ::Gitlab::ProjectTemplate.new('hipaa_audit_protocol', 'HIPAA Audit Protocol', _('A project containing issues for each audit inquiry in the HIPAA Audit Protocol published by the U.S. Department of Health & Human Services'), 'https://gitlab.com/gitlab-org/project-templates/hipaa-audit-protocol')
+          ].freeze
+        end
+
         override :all
         def all
           return super unless License.feature_available?(:enterprise_templates)
 
-          super + ENTERPRISE_TEMPLATES_TABLE
+          super + localized_ee_templates_table
         end
       end
     end

ee/spec/features/projects/new_project_spec.rb

@@ -451,7 +451,7 @@ describe 'New project' do
   end
 
   context 'Built-in project templates' do
-    let(:enterprise_templates) { EE::Gitlab::ProjectTemplate::ENTERPRISE_TEMPLATES_TABLE }
+    let(:enterprise_templates) { Gitlab::ProjectTemplate.localized_ee_templates_table }
 
     context 'when `enterprise_templates` is licensed' do
       before do

ee/spec/presenters/projects/security/configuration_presenter_spec.rb

@@ -147,9 +147,9 @@ describe Projects::Security::ConfigurationPresenter do
   def security_scan(type, configured:)
     {
       "configured" => configured,
-      "description" => described_class::SCAN_DESCRIPTIONS[type],
+      "description" => described_class.localized_scan_descriptions[type],
       "link" => help_page_path(described_class::SCAN_DOCS[type]),
-      "name" => described_class::SCAN_NAMES[type]
+      "name" => described_class.localized_scan_names[type]
     }
   end
 end

lib/gitlab/project_template.rb

@@ -36,7 +36,8 @@ module Gitlab
       name == other.name && title == other.title
     end
 
-    TEMPLATES_TABLE = [
+    def self.localized_templates_table
+      [
         ProjectTemplate.new('rails', 'Ruby on Rails', _('Includes an MVC structure, Gemfile, Rakefile, along with many others, to help you get started.'), 'https://gitlab.com/gitlab-org/project-templates/rails', 'illustrations/logos/rails.svg'),
         ProjectTemplate.new('spring', 'Spring', _('Includes an MVC structure, mvnw and pom.xml to help you get started.'), 'https://gitlab.com/gitlab-org/project-templates/spring', 'illustrations/logos/spring.svg'),
         ProjectTemplate.new('express', 'NodeJS Express', _('Includes an MVC structure to help you get started.'), 'https://gitlab.com/gitlab-org/project-templates/express', 'illustrations/logos/express.svg'),
@@ -59,10 +60,11 @@ module Gitlab
         ProjectTemplate.new('serverless_framework', 'Serverless Framework/JS', _('A basic page and serverless function that uses AWS Lambda, AWS API Gateway, and GitLab Pages'), 'https://gitlab.com/gitlab-org/project-templates/serverless-framework', 'illustrations/logos/serverless_framework.svg'),
         ProjectTemplate.new('cluster_management', 'GitLab Cluster Management', _('An example project for managing Kubernetes clusters integrated with GitLab.'), 'https://gitlab.com/gitlab-org/project-templates/cluster-management')
       ].freeze
+    end
 
     class << self
       def all
-        TEMPLATES_TABLE
+        localized_templates_table
       end
 
       def find(name)

rubocop/cop/static_translation_definition.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    class StaticTranslationDefinition < RuboCop::Cop::Cop
+      MSG = "The text you're translating will be already in the translated form when it's assigned to the constant. When a users changes the locale, these texts won't be translated again. Consider moving the translation logic to a method.".freeze
+
+      TRANSLATION_METHODS = %i[_ s_ n_].freeze
+
+      def_node_matcher :translation_method?, <<~PATTERN
+      (send _ _ str*)
+      PATTERN
+
+      def_node_matcher :lambda_node?, <<~PATTERN
+      (send _ :lambda)
+      PATTERN
+
+      def on_send(node)
+        return unless translation_method?(node)
+
+        method_name = node.children[1]
+        return unless TRANSLATION_METHODS.include?(method_name)
+
+        node.each_ancestor do |ancestor|
+          receiver, _ = *ancestor
+          break if lambda_node?(receiver) # translations defined in lambda nodes should be allowed
+
+          if constant_assignment?(ancestor)
+            add_offense(node, location: :expression)
+
+            break
+          end
+        end
+      end
+
+      private
+
+      def constant_assignment?(node)
+        node.type == :casgn
+      end
+    end
+  end
+end

spec/rubocop/cop/static_translation_definition_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../rubocop/cop/static_translation_definition'
+
+describe RuboCop::Cop::StaticTranslationDefinition do
+  include CopHelper
+
+  using RSpec::Parameterized::TableSyntax
+
+  subject(:cop) { described_class.new }
+
+  shared_examples 'offense' do |code, highlight, line|
+    it 'registers an offense' do
+      inspect_source(code)
+
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.offenses.map(&:line)).to eq([line])
+      expect(cop.highlights).to eq([highlight])
+    end
+  end
+
+  shared_examples 'no offense' do |code|
+    it 'does not register an offense' do
+      inspect_source(code)
+
+      expect(cop.offenses).to be_empty
+    end
+  end
+
+  describe 'offenses' do
+    where(:code, :highlight, :line) do
+      [
+        ['A = _("a")', '_("a")', 1],
+        ['B = s_("b")', 's_("b")', 1],
+        ['C = n_("c")', 'n_("c")', 1],
+        [
+          <<~CODE,
+            module MyModule
+              A = {
+                b: {
+                  c: _("a")
+                }
+              }
+            end
+          CODE
+          '_("a")',
+          4
+        ],
+        [
+          <<~CODE,
+            class MyClass
+              B = [
+                [
+                  s_("a")
+                ]
+              ]
+            end
+          CODE
+          's_("a")',
+          4
+        ]
+      ]
+    end
+
+    with_them do
+      include_examples 'offense', params[:code], params[:highlight], params[:line]
+    end
+  end
+
+  describe 'ignore' do
+    where(:code) do
+      [
+        'CONSTANT_1 = __("a")',
+        'CONSTANT_2 = s__("a")',
+        'CONSTANT_3 = n__("a")',
+        <<~CODE,
+          def method
+            s_('a')
+          end
+        CODE
+        <<~CODE,
+          class MyClass
+            VALID = -> {
+              s_('hi')
+            }
+          end
+        CODE
+        <<~CODE
+          class MyClass
+            def hello
+              {
+                a: _('hi')
+              }
+            end
+          end
+        CODE
+      ]
+    end
+
+    with_them do
+      include_examples 'no offense', params[:code]
+    end
+  end
+end