Take `nplurals` into account when validating translations.

9 files changed, 120 insertions, 112 deletions

diff --git a/lib/gitlab/i18n/metadata_entry.rb b/lib/gitlab/i18n/metadata_entry.rb
index 3f9cbae62c8..35d57459a3d 100644
--- a/lib/gitlab/i18n/metadata_entry.rb
+++ b/lib/gitlab/i18n/metadata_entry.rb
@@ -1,13 +1,16 @@
 module Gitlab
   module I18n
-    class MetadataEntry < PoEntry
+    class MetadataEntry
+      attr_reader :entry_data
+
+      def initialize(entry_data)
+        @entry_data = entry_data
+      end
+
       def expected_plurals
         return nil unless plural_information
 
-        nplurals = plural_information['nplurals'].to_i
-        if nplurals > 0
-          nplurals
-        end
+        plural_information['nplurals'].to_i
       end
 
       private
diff --git a/lib/gitlab/i18n/po_entry.rb b/lib/gitlab/i18n/po_entry.rb
deleted file mode 100644
index 014043cfdd6..00000000000
--- a/lib/gitlab/i18n/po_entry.rb
+++ /dev/null
@@ -1,19 +0,0 @@
-module Gitlab
-  module I18n
-    class PoEntry
-      def self.build(entry_data)
-        if entry_data[:msgid].empty?
-          MetadataEntry.new(entry_data)
-        else
-          TranslationEntry.new(entry_data)
-        end
-      end
-
-      attr_reader :entry_data
-
-      def initialize(entry_data)
-        @entry_data = entry_data
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/i18n/po_linter.rb b/lib/gitlab/i18n/po_linter.rb
index f5ffc6669e4..c3b1fe582af 100644
--- a/lib/gitlab/i18n/po_linter.rb
+++ b/lib/gitlab/i18n/po_linter.rb
@@ -25,12 +25,19 @@ module Gitlab
       end
 
       def parse_po
-        entries = SimplePoParser.parse(po_path).map { |data| Gitlab::I18n::PoEntry.build(data) }
+        entries = SimplePoParser.parse(po_path)
 
         # The first entry is the metadata entry if there is one.
         # This is an entry when empty `msgid`
-        @metadata_entry = entries.shift if entries.first.is_a?(Gitlab::I18n::MetadataEntry)
-        @translation_entries = entries
+        if entries.first[:msgid].empty?
+          @metadata_entry = Gitlab::I18n::MetadataEntry.new(entries.shift)
+        else
+          return 'Missing metadata entry.'
+        end
+
+        @translation_entries = entries.map do |entry_data|
+          Gitlab::I18n::TranslationEntry.new(entry_data, metadata_entry.expected_plurals)
+        end
 
         nil
       rescue SimplePoParser::ParserError => e
@@ -64,7 +71,7 @@ module Gitlab
         return unless metadata_entry&.expected_plurals
         return unless entry.translated?
 
-        if entry.plural? && entry.all_translations.size != metadata_entry.expected_plurals
+        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)}"
         end
@@ -85,11 +92,11 @@ module Gitlab
       end
 
       def validate_variables(errors, entry)
-        if entry.has_singular?
+        if entry.has_singular_translation?
           validate_variables_in_message(errors, entry.msgid, entry.singular_translation)
         end
 
-        if entry.plural?
+        if entry.has_plural?
           entry.plural_translations.each do |translation|
             validate_variables_in_message(errors, entry.plural_id, translation)
           end
@@ -161,8 +168,8 @@ module Gitlab
         translation = join_message(translation)
 
         # We don't need to validate when the message is empty.
-        # Translations could fallback to the default, or we could be validating a
-        # language that does not have plurals.
+        # In this case we fall back to the default, which has all the the
+        # required variables.
         return if translation.empty?
 
         found_variables = translation.scan(VARIABLE_REGEX)
diff --git a/lib/gitlab/i18n/translation_entry.rb b/lib/gitlab/i18n/translation_entry.rb
index 4fe8f569f9c..75d5aa0cfe1 100644
--- a/lib/gitlab/i18n/translation_entry.rb
+++ b/lib/gitlab/i18n/translation_entry.rb
@@ -1,6 +1,13 @@
 module Gitlab
   module I18n
-    class TranslationEntry < PoEntry
+    class TranslationEntry
+      attr_reader :nplurals, :entry_data
+
+      def initialize(entry_data, nplurals)
+        @entry_data = entry_data
+        @nplurals = nplurals
+      end
+
       def msgid
         entry_data[:msgid]
       end
@@ -9,16 +16,17 @@ module Gitlab
         entry_data[:msgid_plural]
       end
 
-      def plural?
+      def has_plural?
         plural_id.present?
       end
 
       def singular_translation
-        plural? ? entry_data['msgstr[0]'] : entry_data[:msgstr]
+        all_translations.first if has_singular_translation?
       end
 
       def all_translations
-        @all_translations ||= entry_data.fetch_values(*translation_keys).reject(&:empty?)
+        @all_translations ||= entry_data.fetch_values(*translation_keys)
+                                .reject(&:empty?)
       end
 
       def translated?
@@ -26,25 +34,22 @@ module Gitlab
       end
 
       def plural_translations
-        return [] unless plural?
+        return [] unless has_plural?
         return [] unless translated?
 
-        # The singular translation is used if there's only translation. This is
-        # the case for languages without plurals.
-        return all_translations if all_translations.size == 1
-
-        entry_data.fetch_values(*plural_translation_keys)
+        @plural_translations ||= if has_singular_translation?
+                                   all_translations.drop(1)
+                                 else
+                                   all_translations
+                                 end
       end
 
       def flag
         entry_data[:flag]
       end
 
-      # When a translation is a plural, but only has 1 translation, we could be
-      # talking about a language in which plural and singular is the same thing.
-      # In which case we always translate as a plural.
-      def has_singular?
-        !plural? || all_translations.size > 1
+      def has_singular_translation?
+        nplurals > 1 || !has_plural?
       end
 
       def msgid_contains_newlines?
@@ -61,15 +66,8 @@ module Gitlab
 
       private
 
-      def plural_translation_keys
-        @plural_translation_keys ||= translation_keys.select do |key|
-          plural_index = key.scan(/\d+/).first.to_i
-          plural_index > 0
-        end
-      end
-
       def translation_keys
-        @translation_keys ||= if plural?
+        @translation_keys ||= if has_plural?
                                 entry_data.keys.select { |key| key =~ /msgstr\[\d+\]/ }
                               else
                                 [:msgstr]
diff --git a/spec/fixtures/missing_metadata.po b/spec/fixtures/missing_metadata.po
new file mode 100644
index 00000000000..b1999c933f1
--- /dev/null
+++ b/spec/fixtures/missing_metadata.po
@@ -0,0 +1,4 @@
+msgid "1 commit"
+msgid_plural "%d commits"
+msgstr[0] "1 cambio"
+msgstr[1] "%d cambios"
diff --git a/spec/lib/gitlab/i18n/metadata_entry_spec.rb b/spec/lib/gitlab/i18n/metadata_entry_spec.rb
index 5bc16e1806e..ab71d6454a9 100644
--- a/spec/lib/gitlab/i18n/metadata_entry_spec.rb
+++ b/spec/lib/gitlab/i18n/metadata_entry_spec.rb
@@ -24,5 +24,28 @@ describe Gitlab::I18n::MetadataEntry do
 
       expect(entry.expected_plurals).to eq(2)
     end
+
+    it 'returns 0 for the POT-metadata' do
+      data = {
+        msgid: "",
+        msgstr: [
+          "",
+          "Project-Id-Version: gitlab 1.0.0\\n",
+          "Report-Msgid-Bugs-To: \\n",
+          "PO-Revision-Date: 2017-07-13 12:10-0500\\n",
+          "Language-Team: Spanish\\n",
+          "Language: es\\n",
+          "MIME-Version: 1.0\\n",
+          "Content-Type: text/plain; charset=UTF-8\\n",
+          "Content-Transfer-Encoding: 8bit\\n",
+          "Plural-Forms: nplurals=INTEGER; plural=EXPRESSION;\n",
+          "Last-Translator: Bob Van Landuyt <bob@gitlab.com>\\n",
+          "X-Generator: Poedit 2.0.2\\n"
+        ]
+      }
+      entry = described_class.new(data)
+
+      expect(entry.expected_plurals).to eq(0)
+    end
   end
 end
diff --git a/spec/lib/gitlab/i18n/po_entry_spec.rb b/spec/lib/gitlab/i18n/po_entry_spec.rb
deleted file mode 100644
index 4be1b3d4383..00000000000
--- a/spec/lib/gitlab/i18n/po_entry_spec.rb
+++ /dev/null
@@ -1,18 +0,0 @@
-require 'spec_helper'
-
-describe Gitlab::I18n::PoEntry do
-  describe '.build' do
-    it 'builds a metadata entry when the msgid is empty' do
-      entry = described_class.build(msgid: '')
-
-      expect(entry).to be_kind_of(Gitlab::I18n::MetadataEntry)
-    end
-
-    it 'builds a translation entry when the msgid is empty' do
-      entry = described_class.build(msgid: 'Hello world')
-
-      expect(entry).to be_kind_of(Gitlab::I18n::TranslationEntry)
-    end
-
-  end
-end
diff --git a/spec/lib/gitlab/i18n/po_linter_spec.rb b/spec/lib/gitlab/i18n/po_linter_spec.rb
index 0fa4e05c7b1..97a8c105264 100644
--- a/spec/lib/gitlab/i18n/po_linter_spec.rb
+++ b/spec/lib/gitlab/i18n/po_linter_spec.rb
@@ -69,6 +69,14 @@ describe Gitlab::I18n::PoLinter do
       end
     end
 
+    context 'with missing metadata' do
+      let(:po_path) { 'spec/fixtures/missing_metadata.po' }
+
+      it 'returns the an error' do
+        is_expected.to include('PO-syntax errors' => a_kind_of(Array))
+      end
+    end
+
     context 'with a valid po' do
       it 'parses the file' do
         expect(linter).to receive(:parse_po).and_call_original
@@ -90,7 +98,7 @@ describe Gitlab::I18n::PoLinter do
     context 'with missing plurals' do
       let(:po_path) { 'spec/fixtures/missing_plurals.po' }
 
-      it 'has no errors' do
+      it 'has errors' do
         is_expected.not_to be_empty
       end
     end
@@ -136,8 +144,7 @@ describe Gitlab::I18n::PoLinter do
   describe '#validate_entries' do
     it 'keeps track of errors for entries' do
       fake_invalid_entry = Gitlab::I18n::TranslationEntry.new(
-        msgid: "Hello %{world}",
-        msgstr: "Bonjour %{monde}"
+        { msgid: "Hello %{world}", msgstr: "Bonjour %{monde}" }, 2
       )
       allow(linter).to receive(:translation_entries) { [fake_invalid_entry] }
 
@@ -169,9 +176,8 @@ describe Gitlab::I18n::PoLinter do
       allow(linter).to receive(:metadata_entry).and_return(fake_metadata)
 
       fake_entry = Gitlab::I18n::TranslationEntry.new(
-        msgid: 'the singular',
-        msgid_plural: 'the plural',
-        'msgstr[0]' => 'the singular'
+        { msgid: 'the singular', msgid_plural: 'the plural', 'msgstr[0]' => 'the singular' },
+        2
       )
       errors = []
 
@@ -183,27 +189,31 @@ describe Gitlab::I18n::PoLinter do
 
   describe '#validate_variables' do
     it 'validates both signular and plural in a pluralized string when the entry has a singular' do
-      pluralized_entry = Gitlab::I18n::TranslationEntry.new({
-        msgid: 'Hello %{world}',
-        msgid_plural: 'Hello all %{world}',
-        'msgstr[0]' => 'Bonjour %{world}',
-        'msgstr[1]' => 'Bonjour tous %{world}'
-      })
+      pluralized_entry = Gitlab::I18n::TranslationEntry.new(
+        { msgid: 'Hello %{world}',
+          msgid_plural: 'Hello all %{world}',
+          'msgstr[0]' => 'Bonjour %{world}',
+          'msgstr[1]' => 'Bonjour tous %{world}' },
+        2
+      )
 
       expect(linter).to receive(:validate_variables_in_message)
                           .with([], 'Hello %{world}', 'Bonjour %{world}')
+                          .and_call_original
       expect(linter).to receive(:validate_variables_in_message)
                           .with([], 'Hello all %{world}', 'Bonjour tous %{world}')
+                          .and_call_original
 
       linter.validate_variables([], pluralized_entry)
     end
 
     it 'only validates plural when there is no separate singular' do
-      pluralized_entry = Gitlab::I18n::TranslationEntry.new({
-        msgid: 'Hello %{world}',
-        msgid_plural: 'Hello all %{world}',
-        'msgstr[0]' => 'Bonjour %{world}'
-      })
+      pluralized_entry = Gitlab::I18n::TranslationEntry.new(
+        { msgid: 'Hello %{world}',
+          msgid_plural: 'Hello all %{world}',
+          'msgstr[0]' => 'Bonjour %{world}' },
+        1
+      )
 
       expect(linter).to receive(:validate_variables_in_message)
                           .with([], 'Hello all %{world}', 'Bonjour %{world}')
@@ -213,8 +223,8 @@ describe Gitlab::I18n::PoLinter do
 
     it 'validates the message variables' do
       entry = Gitlab::I18n::TranslationEntry.new(
-        msgid: 'Hello',
-        msgstr: 'Bonjour'
+        { msgid: 'Hello', msgstr: 'Bonjour' },
+        2
       )
 
       expect(linter).to receive(:validate_variables_in_message)
diff --git a/spec/lib/gitlab/i18n/translation_entry_spec.rb b/spec/lib/gitlab/i18n/translation_entry_spec.rb
index e48ba28be0e..d2d3ec03c6d 100644
--- a/spec/lib/gitlab/i18n/translation_entry_spec.rb
+++ b/spec/lib/gitlab/i18n/translation_entry_spec.rb
@@ -4,7 +4,7 @@ describe Gitlab::I18n::TranslationEntry do
   describe '#singular_translation' do
     it 'returns the normal `msgstr` for translations without plural' do
       data = { msgid: 'Hello world', msgstr: 'Bonjour monde' }
-      entry = described_class.new(data)
+      entry = described_class.new(data, 2)
 
       expect(entry.singular_translation).to eq('Bonjour monde')
     end
@@ -16,7 +16,7 @@ describe Gitlab::I18n::TranslationEntry do
         'msgstr[0]' => 'Bonjour monde',
         'msgstr[1]' => 'Bonjour mondes'
       }
-      entry = described_class.new(data)
+      entry = described_class.new(data, 2)
 
       expect(entry.singular_translation).to eq('Bonjour monde')
     end
@@ -25,7 +25,7 @@ describe Gitlab::I18n::TranslationEntry do
   describe '#all_translations' do
     it 'returns all translations for singular translations' do
       data = { msgid: 'Hello world', msgstr: 'Bonjour monde' }
-      entry = described_class.new(data)
+      entry = described_class.new(data, 2)
 
       expect(entry.all_translations).to eq(['Bonjour monde'])
     end
@@ -37,7 +37,7 @@ describe Gitlab::I18n::TranslationEntry do
         'msgstr[0]' => 'Bonjour monde',
         'msgstr[1]' => 'Bonjour mondes'
       }
-      entry = described_class.new(data)
+      entry = described_class.new(data, 2)
 
       expect(entry.all_translations).to eq(['Bonjour monde', 'Bonjour mondes'])
     end
@@ -50,7 +50,7 @@ describe Gitlab::I18n::TranslationEntry do
         msgid_plural: 'Hello worlds',
         'msgstr[0]' => 'Bonjour monde'
       }
-      entry = described_class.new(data)
+      entry = described_class.new(data, 1)
 
       expect(entry.plural_translations).to eq(['Bonjour monde'])
     end
@@ -63,21 +63,21 @@ describe Gitlab::I18n::TranslationEntry do
         'msgstr[1]' => 'Bonjour mondes',
         'msgstr[2]' => 'Bonjour tous les mondes'
       }
-      entry = described_class.new(data)
+      entry = described_class.new(data, 3)
 
       expect(entry.plural_translations).to eq(['Bonjour mondes', 'Bonjour tous les mondes'])
     end
   end
 
-  describe '#has_singular?' do
+  describe '#has_singular_translation?' do
     it 'has a singular when the translation is not pluralized' do
       data = {
         msgid: 'hello world',
         msgstr: 'hello'
       }
-      entry = described_class.new(data)
+      entry = described_class.new(data, 2)
 
-      expect(entry).to have_singular
+      expect(entry).to have_singular_translation
     end
 
     it 'has a singular when plural and singular are separately defined' do
@@ -87,9 +87,9 @@ describe Gitlab::I18n::TranslationEntry do
         "msgstr[0]" => 'hello world',
         "msgstr[1]" => 'hello worlds'
       }
-      entry = described_class.new(data)
+      entry = described_class.new(data, 2)
 
-      expect(entry).to have_singular
+      expect(entry).to have_singular_translation
     end
 
     it 'does not have a separate singular if the plural string only has one translation' do
@@ -98,34 +98,34 @@ describe Gitlab::I18n::TranslationEntry do
         msgid_plural: 'hello worlds',
         "msgstr[0]" => 'hello worlds'
       }
-      entry = described_class.new(data)
+      entry = described_class.new(data, 1)
 
-      expect(entry).not_to have_singular
+      expect(entry).not_to have_singular_translation
     end
   end
 
-  describe '#msgid_contains_newlines'do
+  describe '#msgid_contains_newlines' do
     it 'is true when the msgid is an array' do
       data = { msgid: %w(hello world) }
-      entry = described_class.new(data)
+      entry = described_class.new(data, 2)
 
       expect(entry.msgid_contains_newlines?).to be_truthy
     end
   end
 
-  describe '#plural_id_contains_newlines'do
+  describe '#plural_id_contains_newlines' do
     it 'is true when the msgid is an array' do
-      data = { plural_id: %w(hello world) }
-      entry = described_class.new(data)
+      data = { msgid_plural: %w(hello world) }
+      entry = described_class.new(data, 2)
 
       expect(entry.plural_id_contains_newlines?).to be_truthy
     end
   end
 
-  describe '#translations_contain_newlines'do
+  describe '#translations_contain_newlines' do
     it 'is true when the msgid is an array' do
       data = { msgstr: %w(hello world) }
-      entry = described_class.new(data)
+      entry = described_class.new(data, 2)
 
       expect(entry.translations_contain_newlines?).to be_truthy
     end