Reject ruby interpolation in externalized stringsbvl-correct-interpolation-i18n

When using ruby interpolation in externalized strings, they can't be
detected. Which means they will never be presented to be translated.

To mix variables into translations we need to use `sprintf`
instead.

Instead of:

    _("Hello #{subject}")

Use:

    _("Hello %{subject}) % { subject: 'world' }

9 files changed, 185 insertions, 7 deletions

diff --git a/.haml-lint.yml b/.haml-lint.yml
index 32c7de0fb78..fcdc47af60f 100644
--- a/.haml-lint.yml
+++ b/.haml-lint.yml
@@ -70,14 +70,15 @@ linters:
     enabled: false
 
   RuboCop:
-    enabled: false
+    enabled: true
     # These cops are incredibly noisy when it comes to HAML templates, so we
     # ignore them.
     ignored_cops:
-      - Lint/BlockAlignment
-      - Lint/EndAlignment
+      - Layout/BlockAlignment
+      - Layout/EndAlignment
       - Lint/Void
       - Metrics/LineLength
+      - Naming/FileName
       - Style/AlignParameters
       - Style/BlockNesting
       - Style/ElseAlignment
@@ -91,6 +92,52 @@ linters:
       - Style/TrailingWhitespace
       - Style/WhileUntilModifier
 
+      # These cops should eventually get enabled
+      - Cop/LineBreakAfterGuardClauses
+      - Cop/LineBreakAroundConditionalBlock
+      - Cop/ProjectPathHelper
+      - GitlabSecurity/PublicSend
+      - Layout/LeadingCommentSpace
+      - Layout/SpaceAfterColon
+      - Layout/SpaceAfterComma
+      - Layout/SpaceAroundOperators
+      - Layout/SpaceBeforeBlockBraces
+      - Layout/SpaceBeforeComma
+      - Layout/SpaceBeforeFirstArg
+      - Layout/SpaceInsideArrayLiteralBrackets
+      - Layout/SpaceInsideHashLiteralBraces
+      - Layout/SpaceInsideStringInterpolation
+      - Layout/TrailingBlankLines
+      - Lint/BooleanSymbol
+      - Lint/LiteralInInterpolation
+      - Lint/ParenthesesAsGroupedExpression
+      - Lint/RedundantWithIndex
+      - Lint/Syntax
+      - Lint/UselessAssignment
+      - Metrics/BlockNesting
+      - Naming/VariableName
+      - Performance/RedundantMatch
+      - Performance/StringReplacement
+      - Rails/Presence
+      - Rails/RequestReferer
+      - Style/AndOr
+      - Style/ColonMethodCall
+      - Style/ConditionalAssignment
+      - Style/HashSyntax
+      - Style/IdenticalConditionalBranches
+      - Style/NegatedIf
+      - Style/NestedTernaryOperator
+      - Style/Not
+      - Style/ParenthesesAroundCondition
+      - Style/RedundantParentheses
+      - Style/SelfAssignment
+      - Style/Semicolon
+      - Style/TernaryParentheses
+      - Style/TrailingCommaInHashLiteral
+      - Style/UnlessElse
+      - Style/WordArray
+      - Style/ZeroLengthPredicate
+
   RubyComments:
     enabled: true
 
diff --git a/app/views/ci/runner/_how_to_setup_runner.html.haml b/app/views/ci/runner/_how_to_setup_runner.html.haml
index 13f96b9747c..c26eb873718 100644
--- a/app/views/ci/runner/_how_to_setup_runner.html.haml
+++ b/app/views/ci/runner/_how_to_setup_runner.html.haml
@@ -1,6 +1,6 @@
 - link = link_to _("Install GitLab Runner"), 'https://docs.gitlab.com/runner/install/', target: '_blank'
 .append-bottom-10
-  %h4= _("Setup a #{type} Runner manually")
+  %h4= _("Setup a %{type} Runner manually") % { type: type }
 
 %ol
   %li
diff --git a/app/views/projects/_issuable_by_email.html.haml b/app/views/projects/_issuable_by_email.html.haml
index 22adf5b4008..d59191a6f87 100644
--- a/app/views/projects/_issuable_by_email.html.haml
+++ b/app/views/projects/_issuable_by_email.html.haml
@@ -19,9 +19,16 @@
           = text_field_tag :issuable_email, email, class: "monospace js-select-on-focus form-control", readonly: true
           .input-group-append
             = clipboard_button(target: '#issuable_email', class: 'btn btn-clipboard input-group-text btn-transparent d-none d-sm-block')
+
+            - if issuable_type == 'issue'
+              - enter_title_text =  _('Enter the issue title')
+              - enter_description_text = _('Enter the issue description')
+            - else
+              - enter_title_text = _('Enter the merge request title')
+              - enter_description_text = _('Enter the merge request description')
             = mail_to email, class: 'btn btn-clipboard btn-transparent',
-              subject: _("Enter the #{name} title"),
-              body: _("Enter the #{name} description"),
+              subject: enter_title_text,
+              body: enter_description_text,
               title: _('Send email'),
               data: { toggle: 'tooltip', placement: 'bottom' } do
               = sprite_icon('mail')
diff --git a/app/views/projects/mirrors/_instructions.html.haml b/app/views/projects/mirrors/_instructions.html.haml
index e051f9e6331..35a6885318a 100644
--- a/app/views/projects/mirrors/_instructions.html.haml
+++ b/app/views/projects/mirrors/_instructions.html.haml
@@ -4,7 +4,9 @@
       = _('The repository must be accessible over <code>http://</code>,
       <code>https://</code>, <code>ssh://</code> and <code>git://</code>.').html_safe
     %li= _('Include the username in the URL if required: <code>https://username@gitlab.company.com/group/project.git</code>.').html_safe
-    %li= _("The update action will time out after #{import_will_timeout_message(Gitlab.config.gitlab_shell.git_timeout)} minutes. For big repositories, use a clone/push combination.")
+    %li
+      - minutes = Gitlab.config.gitlab_shell.git_timeout / 60
+      = _("The update action will time out after %{number_of_minutes} minutes. For big repositories, use a clone/push combination.") % { number_of_minutes: minutes }
     %li= _('The Git LFS objects will <strong>not</strong> be synced.').html_safe
     %li
       = _('This user will be the author of all events in the activity feed that are the result of an update,
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index f348f66ae17..1d461f0ad8f 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -2391,6 +2391,18 @@ msgstr ""
 msgid "Enter in your Bitbucket Server URL and personal access token below"
 msgstr ""
 
+msgid "Enter the issue description"
+msgstr ""
+
+msgid "Enter the issue title"
+msgstr ""
+
+msgid "Enter the merge request description"
+msgstr ""
+
+msgid "Enter the merge request title"
+msgstr ""
+
 msgid "Environments"
 msgstr ""
 
@@ -5050,6 +5062,9 @@ msgstr ""
 msgid "Settings"
 msgstr ""
 
+msgid "Setup a %{type} Runner manually"
+msgstr ""
+
 msgid "Setup a specific Runner automatically"
 msgstr ""
 
@@ -5552,6 +5567,9 @@ msgstr ""
 msgid "The time taken by each data entry gathered by that stage."
 msgstr ""
 
+msgid "The update action will time out after %{number_of_minutes} minutes. For big repositories, use a clone/push combination."
+msgstr ""
+
 msgid "The user map is a JSON document mapping the Google Code users that participated on your projects to the way their email addresses and usernames will be imported into GitLab. You can change this by changing the value on the right hand side of <code>:</code>. Be sure to preserve the surrounding double quotes, other punctuation and the email address or username on the left hand side."
 msgstr ""
 
diff --git a/rubocop/cop/line_break_around_conditional_block.rb b/rubocop/cop/line_break_around_conditional_block.rb
index 011f2bcf8bf..59fe6e5d98c 100644
--- a/rubocop/cop/line_break_around_conditional_block.rb
+++ b/rubocop/cop/line_break_around_conditional_block.rb
@@ -48,6 +48,8 @@ module RuboCop
       MSG = 'Add a line break around conditional blocks'
 
       def on_if(node)
+        # This cop causes errors in haml files, so let's skip those
+        return if in_haml?(node)
         return if node.single_line?
         return unless node.if? || node.unless?
 
@@ -116,6 +118,10 @@ module RuboCop
       def end_line?(line)
         line =~ /^\s*(end|})/
       end
+
+      def in_haml?(node)
+        node.location.expression.source_buffer.name.end_with?('.haml.rb')
+      end
     end
   end
 end
diff --git a/rubocop/cop/ruby_interpolation_in_translation.rb b/rubocop/cop/ruby_interpolation_in_translation.rb
new file mode 100644
index 00000000000..b9411fcfd6c
--- /dev/null
+++ b/rubocop/cop/ruby_interpolation_in_translation.rb
@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    class RubyInterpolationInTranslation < RuboCop::Cop::Cop
+      MSG = "Don't use ruby interpolation \#{} inside translated strings, instead use \%{}"
+
+      TRANSLATION_METHODS = ':_ :s_ :N_ :n_'
+      RUBY_INTERPOLATION_REGEX = /.*\#\{.*\}/
+
+      def_node_matcher :translation_method?, <<~PATTERN
+        (send nil? {#{TRANSLATION_METHODS}} $dstr ...)
+      PATTERN
+
+      def_node_matcher :plural_translation_method?, <<~PATTERN
+        (send nil? :n_ str $dstr ...)
+      PATTERN
+
+      def on_send(node)
+        interpolation = translation_method?(node) || plural_translation_method?(node)
+        return unless interpolation
+
+        interpolation.descendants.each do |possible_violation|
+          add_offense(possible_violation, message: MSG) if possible_violation.type != :str
+        end
+      end
+    end
+  end
+end
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index 88c9bbf24f4..eaf421a7235 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -28,3 +28,4 @@ require_relative 'cop/rspec/env_assignment'
 require_relative 'cop/rspec/factories_in_migration_specs'
 require_relative 'cop/sidekiq_options_queue'
 require_relative 'cop/destroy_all'
+require_relative 'cop/ruby_interpolation_in_translation'
diff --git a/spec/rubocop/cop/ruby_interpolation_in_translation_spec.rb b/spec/rubocop/cop/ruby_interpolation_in_translation_spec.rb
new file mode 100644
index 00000000000..7bd50866577
--- /dev/null
+++ b/spec/rubocop/cop/ruby_interpolation_in_translation_spec.rb
@@ -0,0 +1,68 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../rubocop/cop/ruby_interpolation_in_translation'
+
+# Disabling interpolation check as we deliberately want to have #{} in strings.
+# rubocop:disable Lint/InterpolationCheck
+describe RuboCop::Cop::RubyInterpolationInTranslation do
+  subject(:cop) { described_class.new }
+
+  it 'does not add an offence for a regular messages' do
+    inspect_source('_("Hello world")')
+
+    expect(cop.offenses).to be_empty
+  end
+
+  it 'adds the correct offence when using interpolation in a string' do
+    inspect_source('_("Hello #{world}")')
+
+    offense = cop.offenses.first
+
+    expect(offense.location.source).to eq('#{world}')
+    expect(offense.message).to eq('Don\'t use ruby interpolation #{} inside translated strings, instead use %{}')
+  end
+
+  it 'detects when using a ruby interpolation in the first argument of a pluralized string' do
+    inspect_source('n_("Hello #{world}", "Hello world")')
+
+    expect(cop.offenses).not_to be_empty
+  end
+
+  it 'detects when using a ruby interpolation in the second argument of a pluralized string' do
+    inspect_source('n_("Hello world", "Hello #{world}")')
+
+    expect(cop.offenses).not_to be_empty
+  end
+
+  it 'detects when using interpolation in a namespaced translation' do
+    inspect_source('s_("Hello|#{world}")')
+
+    expect(cop.offenses).not_to be_empty
+  end
+
+  it 'does not add an offence for messages defined over multiple lines' do
+    source = <<~SRC
+      _("Hello "\
+        "world ")
+    SRC
+
+    inspect_source(source)
+    expect(cop.offenses).to be_empty
+  end
+
+  it 'adds an offence for violations in a message defined over multiple lines' do
+    source = <<~SRC
+      _("Hello "\
+        "\#{world} ")
+    SRC
+
+    inspect_source(source)
+    expect(cop.offenses).not_to be_empty
+  end
+end
+# rubocop:enable Lint/InterpolationCheck