Correctly translate all forms in testsbvl-i18n-validation

7 files changed, 125 insertions, 44 deletions

diff --git a/lib/gitlab/i18n/metadata_entry.rb b/lib/gitlab/i18n/metadata_entry.rb
index 35d57459a3d..36fc1bcdcb7 100644
--- a/lib/gitlab/i18n/metadata_entry.rb
+++ b/lib/gitlab/i18n/metadata_entry.rb
@@ -3,16 +3,25 @@ module Gitlab
     class MetadataEntry
       attr_reader :entry_data
 
+      # Avoid testing too many plurals if `nplurals` was incorrectly set.
+      # Based on info on https://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html
+      # which mentions special cases for numbers ending in 2 digits
+      MAX_FORMS_TO_TEST = 101
+
       def initialize(entry_data)
         @entry_data = entry_data
       end
 
-      def expected_plurals
+      def expected_forms
         return nil unless plural_information
 
         plural_information['nplurals'].to_i
       end
 
+      def forms_to_test
+        @forms_to_test ||= [expected_forms, MAX_FORMS_TO_TEST].compact.min
+      end
+
       private
 
       def plural_information
diff --git a/lib/gitlab/i18n/po_linter.rb b/lib/gitlab/i18n/po_linter.rb
index ba08ef59776..d8e7269a2c2 100644
--- a/lib/gitlab/i18n/po_linter.rb
+++ b/lib/gitlab/i18n/po_linter.rb
@@ -36,7 +36,7 @@ module Gitlab
         end
 
         @translation_entries = entries.map do |entry_data|
-          Gitlab::I18n::TranslationEntry.new(entry_data, metadata_entry.expected_plurals)
+          Gitlab::I18n::TranslationEntry.new(entry_data, metadata_entry.expected_forms)
         end
 
         nil
@@ -84,25 +84,25 @@ module Gitlab
       end
 
       def validate_number_of_plurals(errors, entry)
-        return unless metadata_entry&.expected_plurals
+        return unless metadata_entry&.expected_forms
         return unless entry.translated?
 
-        if entry.has_plural? && entry.all_translations.size != metadata_entry.expected_plurals
-          errors << "should have #{metadata_entry.expected_plurals} "\
-                    "#{'translations'.pluralize(metadata_entry.expected_plurals)}"
+        if entry.has_plural? && entry.all_translations.size != metadata_entry.expected_forms
+          errors << "should have #{metadata_entry.expected_forms} "\
+                    "#{'translations'.pluralize(metadata_entry.expected_forms)}"
         end
       end
 
       def validate_newlines(errors, entry)
-        if entry.msgid_contains_newlines?
+        if entry.msgid_has_multiple_lines?
           errors << 'is defined over multiple lines, this breaks some tooling.'
         end
 
-        if entry.plural_id_contains_newlines?
+        if entry.plural_id_has_multiple_lines?
           errors << 'plural is defined over multiple lines, this breaks some tooling.'
         end
 
-        if entry.translations_contain_newlines?
+        if entry.translations_have_multiple_lines?
           errors << 'has translations defined over multiple lines, this breaks some tooling.'
         end
       end
@@ -179,16 +179,41 @@ module Gitlab
       end
 
       def numbers_covering_all_plurals
-        @numbers_covering_all_plurals ||= Array.new(metadata_entry.expected_plurals) do |index|
-          number_for_pluralization(index)
+        @numbers_covering_all_plurals ||= calculate_numbers_covering_all_plurals
+      end
+
+      def calculate_numbers_covering_all_plurals
+        required_numbers = []
+        discovered_indexes = []
+        counter = 0
+
+        while discovered_indexes.size < metadata_entry.forms_to_test && counter < Gitlab::I18n::MetadataEntry::MAX_FORMS_TO_TEST
+          index_for_count = index_for_pluralization(counter)
+
+          unless discovered_indexes.include?(index_for_count)
+            discovered_indexes << index_for_count
+            required_numbers << counter
+          end
+
+          counter += 1
         end
+
+        required_numbers
       end
 
-      def number_for_pluralization(counter)
-        pluralization_result = FastGettext.pluralisation_rule.call(counter)
+      def index_for_pluralization(counter)
+        # This calls the C function that defines the pluralization rule, it can
+        # return a boolean (`false` represents 0, `true` represents 1) or an integer
+        # that specifies the plural form to be used for the given number
+        pluralization_result = Gitlab::I18n.with_locale(locale) do
+          FastGettext.pluralisation_rule.call(counter)
+        end
 
-        if pluralization_result.is_a?(TrueClass) || pluralization_result.is_a?(FalseClass)
-          counter
+        case pluralization_result
+        when false
+          0
+        when true
+          1
         else
           pluralization_result
         end
@@ -211,11 +236,13 @@ module Gitlab
       end
 
       def validate_unnamed_variables(errors, variables)
-        if variables.any? { |name| unnamed_variable?(name) } && variables.any? { |name| !unnamed_variable?(name) }
+        unnamed_variables, named_variables = variables.partition { |name| unnamed_variable?(name) }
+
+        if unnamed_variables.any? && named_variables.any?
           errors << 'is combining named variables with unnamed variables'
         end
 
-        if variables.select { |variable_name| unnamed_variable?(variable_name) }.size > 1
+        if unnamed_variables.size > 1
           errors << 'is combining multiple unnamed variables'
         end
       end
diff --git a/lib/gitlab/i18n/translation_entry.rb b/lib/gitlab/i18n/translation_entry.rb
index 13c544aed27..54adb98f42d 100644
--- a/lib/gitlab/i18n/translation_entry.rb
+++ b/lib/gitlab/i18n/translation_entry.rb
@@ -53,15 +53,15 @@ module Gitlab
         nplurals > 1 || !has_plural?
       end
 
-      def msgid_contains_newlines?
+      def msgid_has_multiple_lines?
         entry_data[:msgid].is_a?(Array)
       end
 
-      def plural_id_contains_newlines?
+      def plural_id_has_multiple_lines?
         entry_data[:msgid_plural].is_a?(Array)
       end
 
-      def translations_contain_newlines?
+      def translations_have_multiple_lines?
         translation_entries.any? { |translation| translation.is_a?(Array) }
       end
 
diff --git a/lib/tasks/gettext.rake b/lib/tasks/gettext.rake
index efe2997d78e..d52c419d66b 100644
--- a/lib/tasks/gettext.rake
+++ b/lib/tasks/gettext.rake
@@ -51,7 +51,7 @@ namespace :gettext do
   end
 
   task :updated_check do
-    # Removeing all pre-translated files speeds up `gettext:find` as the
+    # Removing all pre-translated files speeds up `gettext:find` as the
     # files don't need to be merged.
     `rm locale/*/gitlab.po`
 
@@ -62,15 +62,15 @@ namespace :gettext do
     changed_files = `git diff --name-only`.lines.map(&:strip)
 
     # reset the locale folder for potential next tasks
-    `git checkout locale`
+    `git checkout -- locale`
 
     if changed_files.include?('locale/gitlab.pot')
       raise <<~MSG
         Newly translated strings found, please add them to `gitlab.pot` by running:
 
-          bundle exec rake gettext:find; git checkout locale/*/gitlab.po;
+          bundle exec rake gettext:find; git checkout -- locale/*/gitlab.po;
 
-        Then check in the resulting `locale/gitlab.pot`
+        Then commit and push the resulting changes to `locale/gitlab.pot`.
 
       MSG
     end
diff --git a/spec/lib/gitlab/i18n/metadata_entry_spec.rb b/spec/lib/gitlab/i18n/metadata_entry_spec.rb
index ab71d6454a9..a399517cc04 100644
--- a/spec/lib/gitlab/i18n/metadata_entry_spec.rb
+++ b/spec/lib/gitlab/i18n/metadata_entry_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe Gitlab::I18n::MetadataEntry do
-  describe '#expected_plurals' do
+  describe '#expected_forms' do
     it 'returns the number of plurals' do
       data = {
         msgid: "",
@@ -22,7 +22,7 @@ describe Gitlab::I18n::MetadataEntry do
       }
       entry = described_class.new(data)
 
-      expect(entry.expected_plurals).to eq(2)
+      expect(entry.expected_forms).to eq(2)
     end
 
     it 'returns 0 for the POT-metadata' do
@@ -45,7 +45,7 @@ describe Gitlab::I18n::MetadataEntry do
       }
       entry = described_class.new(data)
 
-      expect(entry.expected_plurals).to eq(0)
+      expect(entry.expected_forms).to eq(0)
     end
   end
 end
diff --git a/spec/lib/gitlab/i18n/po_linter_spec.rb b/spec/lib/gitlab/i18n/po_linter_spec.rb
index 045916e6dc2..3dbc23d2aaf 100644
--- a/spec/lib/gitlab/i18n/po_linter_spec.rb
+++ b/spec/lib/gitlab/i18n/po_linter_spec.rb
@@ -1,20 +1,22 @@
 require 'spec_helper'
 require 'simple_po_parser'
 
+# Disabling this cop to allow for multi-language examples in comments
+# rubocop:disable Style/AsciiComments
 describe Gitlab::I18n::PoLinter do
   let(:linter) { described_class.new(po_path) }
   let(:po_path) { 'spec/fixtures/valid.po' }
 
-  def fake_translation(id:, translation:, plural_id: nil, plurals: [])
-    data = { msgid: id, msgid_plural: plural_id }
+  def fake_translation(msgid:, translation:, plural_id: nil, plurals: [])
+    data = { msgid: msgid, msgid_plural: plural_id }
 
     if plural_id
       [translation, *plurals].each_with_index do |plural, index|
-        allow(FastGettext::Translation).to receive(:n_).with(plural_id, index).and_return(plural)
+        allow(FastGettext::Translation).to receive(:n_).with(msgid, plural_id, index).and_return(plural)
         data.merge!("msgstr[#{index}]" => plural)
       end
     else
-      allow(FastGettext::Translation).to receive(:_).with(id).and_return(translation)
+      allow(FastGettext::Translation).to receive(:_).with(msgid).and_return(translation)
       data[:msgstr] = translation
     end
 
@@ -174,7 +176,7 @@ describe Gitlab::I18n::PoLinter do
 
   describe '#validate_entries' do
     it 'keeps track of errors for entries' do
-      fake_invalid_entry = fake_translation(id: "Hello %{world}",
+      fake_invalid_entry = fake_translation(msgid: "Hello %{world}",
                                             translation: "Bonjour %{monde}")
       allow(linter).to receive(:translation_entries) { [fake_invalid_entry] }
 
@@ -204,7 +206,7 @@ describe Gitlab::I18n::PoLinter do
   describe '#validate_number_of_plurals' do
     it 'validates when there are an incorrect number of translations' do
       fake_metadata = double
-      allow(fake_metadata).to receive(:expected_plurals).and_return(2)
+      allow(fake_metadata).to receive(:expected_forms).and_return(2)
       allow(linter).to receive(:metadata_entry).and_return(fake_metadata)
 
       fake_entry = Gitlab::I18n::TranslationEntry.new(
@@ -226,7 +228,7 @@ describe Gitlab::I18n::PoLinter do
 
     it 'validates both singular and plural in a pluralized string when the entry has a singular' do
       pluralized_entry = fake_translation(
-        id: 'Hello %{world}',
+        msgid: 'Hello %{world}',
         translation: 'Bonjour %{world}',
         plural_id: 'Hello all %{world}',
         plurals: ['Bonjour tous %{world}']
@@ -244,7 +246,7 @@ describe Gitlab::I18n::PoLinter do
 
     it 'only validates plural when there is no separate singular' do
       pluralized_entry = fake_translation(
-        id: 'Hello %{world}',
+        msgid: 'Hello %{world}',
         translation: 'Bonjour %{world}',
         plural_id: 'Hello all %{world}'
       )
@@ -256,7 +258,7 @@ describe Gitlab::I18n::PoLinter do
     end
 
     it 'validates the message variables' do
-      entry = fake_translation(id: 'Hello', translation: 'Bonjour')
+      entry = fake_translation(msgid: 'Hello', translation: 'Bonjour')
 
       expect(linter).to receive(:validate_variables_in_message)
                           .with([], 'Hello', 'Bonjour')
@@ -266,7 +268,7 @@ describe Gitlab::I18n::PoLinter do
 
     it 'validates variable usage in message ids' do
       entry = fake_translation(
-        id: 'Hello %{world}',
+        msgid: 'Hello %{world}',
         translation: 'Bonjour %{world}',
         plural_id: 'Hello all %{world}',
         plurals: ['Bonjour tous %{world}']
@@ -309,7 +311,7 @@ describe Gitlab::I18n::PoLinter do
   end
 
   describe '#validate_translation' do
-    let(:entry) { fake_translation(id: 'Hello %{world}', translation: 'Bonjour %{world}') }
+    let(:entry) { fake_translation(msgid: 'Hello %{world}', translation: 'Bonjour %{world}') }
 
     it 'succeeds with valid variables' do
       errors = []
@@ -330,7 +332,7 @@ describe Gitlab::I18n::PoLinter do
     end
 
     it 'adds an error message when translating fails when translating with context' do
-      entry = fake_translation(id: 'Tests|Hello', translation: 'broken')
+      entry = fake_translation(msgid: 'Tests|Hello', translation: 'broken')
       errors = []
 
       expect(FastGettext::Translation).to receive(:s_) { raise 'broken' }
@@ -341,7 +343,7 @@ describe Gitlab::I18n::PoLinter do
     end
 
     it "adds an error when trying to translate with incorrect variables when using unnamed variables" do
-      entry = fake_translation(id: 'Hello %s', translation: 'Hello %d')
+      entry = fake_translation(msgid: 'Hello %s', translation: 'Hello %d')
       errors = []
 
       linter.validate_translation(errors, entry)
@@ -350,13 +352,55 @@ describe Gitlab::I18n::PoLinter do
     end
 
     it "adds an error when trying to translate with named variables when unnamed variables are expected" do
-      entry = fake_translation(id: 'Hello %s', translation: 'Hello %{thing}')
+      entry = fake_translation(msgid: 'Hello %s', translation: 'Hello %{thing}')
       errors = []
 
       linter.validate_translation(errors, entry)
 
       expect(errors.first).to start_with("Failure translating to en")
     end
+
+    it 'tests translation for all given forms' do
+      # Fake a language that has 3 forms to translate
+      fake_metadata = double
+      allow(fake_metadata).to receive(:forms_to_test).and_return(3)
+      allow(linter).to receive(:metadata_entry).and_return(fake_metadata)
+      entry = fake_translation(
+        msgid: '%d exception',
+        translation: '%d uitzondering',
+        plural_id: '%d exceptions',
+        plurals: ['%d uitzonderingen', '%d uitzonderingetjes']
+      )
+
+      # Make each count use a different index
+      allow(linter).to receive(:index_for_pluralization).with(0).and_return(0)
+      allow(linter).to receive(:index_for_pluralization).with(1).and_return(1)
+      allow(linter).to receive(:index_for_pluralization).with(2).and_return(2)
+
+      expect(FastGettext::Translation).to receive(:n_).with('%d exception', '%d exceptions', 0).and_call_original
+      expect(FastGettext::Translation).to receive(:n_).with('%d exception', '%d exceptions', 1).and_call_original
+      expect(FastGettext::Translation).to receive(:n_).with('%d exception', '%d exceptions', 2).and_call_original
+
+      linter.validate_translation([], entry)
+    end
+  end
+
+  describe '#numbers_covering_all_plurals' do
+    it 'can correctly find all required numbers to translate to Polish' do
+      # Polish used as an example with 3 different forms:
+      # 0, all plurals except the ones ending in 2,3,4: Kotów
+      # 1: Kot
+      # 2-3-4: Koty
+      # So translating with [0, 1, 2] will give us all different posibilities
+      fake_metadata = double
+      allow(fake_metadata).to receive(:forms_to_test).and_return(4)
+      allow(linter).to receive(:metadata_entry).and_return(fake_metadata)
+      allow(linter).to receive(:locale).and_return('pl_PL')
+
+      numbers = linter.numbers_covering_all_plurals
+
+      expect(numbers).to contain_exactly(0, 1, 2)
+    end
   end
 
   describe '#fill_in_variables' do
@@ -380,3 +424,4 @@ describe Gitlab::I18n::PoLinter do
     end
   end
 end
+# rubocop:enable Style/AsciiComments
diff --git a/spec/lib/gitlab/i18n/translation_entry_spec.rb b/spec/lib/gitlab/i18n/translation_entry_spec.rb
index f68bc8feff9..b301e6ea443 100644
--- a/spec/lib/gitlab/i18n/translation_entry_spec.rb
+++ b/spec/lib/gitlab/i18n/translation_entry_spec.rb
@@ -109,7 +109,7 @@ describe Gitlab::I18n::TranslationEntry do
       data = { msgid: %w(hello world) }
       entry = described_class.new(data, 2)
 
-      expect(entry.msgid_contains_newlines?).to be_truthy
+      expect(entry.msgid_has_multiple_lines?).to be_truthy
     end
   end
 
@@ -118,7 +118,7 @@ describe Gitlab::I18n::TranslationEntry do
       data = { msgid_plural: %w(hello world) }
       entry = described_class.new(data, 2)
 
-      expect(entry.plural_id_contains_newlines?).to be_truthy
+      expect(entry.plural_id_has_multiple_lines?).to be_truthy
     end
   end
 
@@ -127,7 +127,7 @@ describe Gitlab::I18n::TranslationEntry do
       data = { msgstr: %w(hello world) }
       entry = described_class.new(data, 2)
 
-      expect(entry.translations_contain_newlines?).to be_truthy
+      expect(entry.translations_have_multiple_lines?).to be_truthy
     end
   end