Validate the number of plurals in an entry

6 files changed, 177 insertions, 37 deletions

diff --git a/lib/gitlab/i18n/po_entry.rb b/lib/gitlab/i18n/po_entry.rb
index aabb477bbea..f2a4bfbd1cd 100644
--- a/lib/gitlab/i18n/po_entry.rb
+++ b/lib/gitlab/i18n/po_entry.rb
@@ -28,11 +28,16 @@ module Gitlab
       end
 
       def all_translations
-        @all_translations ||= entry_data.fetch_values(*translation_keys)
+        @all_translations ||= entry_data.fetch_values(*translation_keys).reject(&:empty?)
+      end
+
+      def translated?
+        all_translations.any?
       end
 
       def plural_translations
         return [] unless plural?
+        return [] unless translated?
 
         # The singular translation is used if there's only translation. This is
         # the case for languages without plurals.
@@ -45,8 +50,34 @@ module Gitlab
         entry_data[:flag]
       end
 
+      def expected_plurals
+        return nil unless metadata?
+        return nil unless plural_information
+
+        nplurals = plural_information['nplurals'].to_i
+        if nplurals > 0
+          nplurals
+        end
+      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
+      end
+
       private
 
+      def plural_information
+        return nil unless metadata?
+        return @plural_information if defined?(@plural_information)
+
+        if plural_line = entry_data[:msgstr].detect { |metadata_line| metadata_line.starts_with?('Plural-Forms: ') }
+          @plural_information = Hash[plural_line.scan(/(\w+)=([^;\n]+)/)]
+        end
+      end
+
       def plural_translation_keys
         @plural_translation_keys ||= translation_keys.select do |key|
           plural_index = key.scan(/\d+/).first.to_i
diff --git a/lib/gitlab/i18n/po_linter.rb b/lib/gitlab/i18n/po_linter.rb
index 2f6965a19aa..e7c92be1383 100644
--- a/lib/gitlab/i18n/po_linter.rb
+++ b/lib/gitlab/i18n/po_linter.rb
@@ -3,7 +3,7 @@ require 'simple_po_parser'
 module Gitlab
   module I18n
     class PoLinter
-      attr_reader :po_path, :entries, :locale
+      attr_reader :po_path, :entries, :metadata, :locale
 
       VARIABLE_REGEX = /%{\w*}|%[a-z]/.freeze
 
@@ -26,6 +26,7 @@ module Gitlab
 
       def parse_po
         @entries = SimplePoParser.parse(po_path).map { |data| Gitlab::I18n::PoEntry.new(data) }
+        @metadata = @entries.detect { |entry| entry.metadata? }
         nil
       rescue SimplePoParser::ParserError => e
         @entries = []
@@ -51,24 +52,34 @@ module Gitlab
         validate_flags(errors, entry)
         validate_variables(errors, entry)
         validate_newlines(errors, entry)
+        validate_number_of_plurals(errors, entry)
 
         errors
       end
 
-      def validate_newlines(errors, entry)
-        message_id = join_message(entry.msgid)
+      def validate_number_of_plurals(errors, entry)
+        return unless metadata&.expected_plurals
+        return unless entry.translated?
+
+        if entry.plural? && entry.all_translations.size != metadata.expected_plurals
+          errors << "should have #{metadata.expected_plurals} #{'translations'.pluralize(metadata.expected_plurals)}"
+        end
+      end
 
+      def validate_newlines(errors, entry)
         if entry.msgid.is_a?(Array)
-          errors << "<#{message_id}> is defined over multiple lines, this breaks some tooling."
+          errors << "is defined over multiple lines, this breaks some tooling."
         end
 
         if entry.all_translations.any? { |translation| translation.is_a?(Array) }
-          errors << "<#{message_id}> has translations defined over multiple lines, this breaks some tooling."
+          errors << "has translations defined over multiple lines, this breaks some tooling."
         end
       end
 
       def validate_variables(errors, entry)
-        validate_variables_in_message(errors, entry.msgid, entry.singular_translation)
+        if entry.has_singular?
+          validate_variables_in_message(errors, entry.msgid, entry.singular_translation)
+        end
 
         if entry.plural?
           entry.plural_translations.each do |translation|
diff --git a/spec/fixtures/missing_plurals.po b/spec/fixtures/missing_plurals.po
new file mode 100644
index 00000000000..09ca0c82718
--- /dev/null
+++ b/spec/fixtures/missing_plurals.po
@@ -0,0 +1,22 @@
+# Spanish translations for gitlab package.
+# Copyright (C) 2017 THE PACKAGE'S COPYRIGHT HOLDER
+# This file is distributed under the same license as the gitlab package.
+# FIRST AUTHOR <EMAIL@ADDRESS>, 2017.
+#
+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=2; plural=n != 1;\n"
+"Last-Translator: Bob Van Landuyt <bob@gitlab.com>\n"
+"X-Generator: Poedit 2.0.2\n"
+
+msgid "%d commit"
+msgid_plural "%d commits"
+msgstr[0] "%d cambio"
diff --git a/spec/fixtures/no_plurals.po b/spec/fixtures/no_plurals.po
deleted file mode 100644
index 1bfc4ecbb04..00000000000
--- a/spec/fixtures/no_plurals.po
+++ /dev/null
@@ -1,24 +0,0 @@
-# Arthur Charron <arthur.charron@hotmail.fr>, 2017. #zanata
-# Huang Tao <htve@outlook.com>, 2017. #zanata
-# Kohei Ota <inductor@kela.jp>, 2017. #zanata
-# Taisuke Inoue <taisuke.inoue.jp@gmail.com>, 2017. #zanata
-# Takuya Noguchi <takninnovationresearch@gmail.com>, 2017. #zanata
-# YANO  Tethurou <tetuyano+zana@gmail.com>, 2017. #zanata
-msgid ""
-msgstr ""
-"Project-Id-Version: gitlab 1.0.0\n"
-"Report-Msgid-Bugs-To: \n"
-"MIME-Version: 1.0\n"
-"Content-Type: text/plain; charset=UTF-8\n"
-"Content-Transfer-Encoding: 8bit\n"
-"PO-Revision-Date: 2017-08-06 11:23-0400\n"
-"Last-Translator: Taisuke Inoue <taisuke.inoue.jp@gmail.com>\n"
-"Language-Team: Japanese \"Language-Team: Russian (https://translate.zanata.org/"
-"project/view/GitLab)\n"
-"Language: ja\n"
-"X-Generator: Zanata 3.9.6\n"
-"Plural-Forms: nplurals=1; plural=0\n"
-
-msgid "%d commit"
-msgid_plural "%d commits"
-msgstr[0] "%d個のコミット"
diff --git a/spec/lib/gitlab/i18n/po_entry_spec.rb b/spec/lib/gitlab/i18n/po_entry_spec.rb
index 0317caedbff..e671f3c17a1 100644
--- a/spec/lib/gitlab/i18n/po_entry_spec.rb
+++ b/spec/lib/gitlab/i18n/po_entry_spec.rb
@@ -68,4 +68,71 @@ describe Gitlab::I18n::PoEntry do
       expect(entry.plural_translations).to eq(['Bonjour mondes', 'Bonjour tous les mondes'])
     end
   end
+
+  describe '#expected_plurals' do
+    it 'returns nil when the entry is an actual translation' do
+      data = { msgid: 'Hello world', msgstr: 'Bonjour monde' }
+      entry = described_class.new(data)
+
+      expect(entry.expected_plurals).to be_nil
+    end
+
+    it 'returns the number of plurals' 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=2; plural=n != 1;\\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(2)
+    end
+  end
+
+  describe '#has_singular?' do
+    it 'has a singular when the translation is not pluralized' do
+      data = {
+        msgid: 'hello world',
+        msgstr: 'hello'
+      }
+      entry = described_class.new(data)
+
+      expect(entry).to have_singular
+    end
+
+    it 'has a singular when plural and singular are separately defined' do
+      data = {
+        msgid: 'hello world',
+        msgid_plural: 'hello worlds',
+        "msgstr[0]" => 'hello world',
+        "msgstr[1]" => 'hello worlds'
+      }
+      entry = described_class.new(data)
+
+      expect(entry).to have_singular
+    end
+
+    it 'does not have a separate singular if the plural string only has one translation' do
+      data = {
+        msgid: 'hello world',
+        msgid_plural: 'hello worlds',
+        "msgstr[0]" => 'hello worlds'
+      }
+      entry = described_class.new(data)
+
+      expect(entry).not_to have_singular
+    end
+  end
 end
diff --git a/spec/lib/gitlab/i18n/po_linter_spec.rb b/spec/lib/gitlab/i18n/po_linter_spec.rb
index 2e155511076..307ea8b2640 100644
--- a/spec/lib/gitlab/i18n/po_linter_spec.rb
+++ b/spec/lib/gitlab/i18n/po_linter_spec.rb
@@ -28,21 +28,21 @@ describe Gitlab::I18n::PoLinter do
 
       it 'has an error for a normal string' do
         message_id = "You are going to remove %{group_name}.\\nRemoved groups CANNOT be restored!\\nAre you ABSOLUTELY sure?"
-        expected_message = "<#{message_id}> is defined over multiple lines, this breaks some tooling."
+        expected_message = "is defined over multiple lines, this breaks some tooling."
 
         expect(errors[message_id]).to include(expected_message)
       end
 
       it 'has an error when a translation is defined over multiple lines' do
         message_id = "You are going to remove %{group_name}.\\nRemoved groups CANNOT be restored!\\nAre you ABSOLUTELY sure?"
-        expected_message = "<#{message_id}> has translations defined over multiple lines, this breaks some tooling."
+        expected_message = "has translations defined over multiple lines, this breaks some tooling."
 
         expect(errors[message_id]).to include(expected_message)
       end
 
       it 'raises an error when a plural translation is defined over multiple lines' do
         message_id = 'With plural'
-        expected_message = "<#{message_id}> has translations defined over multiple lines, this breaks some tooling."
+        expected_message = "has translations defined over multiple lines, this breaks some tooling."
 
         expect(errors[message_id]).to include(expected_message)
       end
@@ -81,10 +81,10 @@ describe Gitlab::I18n::PoLinter do
     end
 
     context 'with missing plurals' do
-      let(:po_path) { 'spec/fixtures/no_plurals.po' }
+      let(:po_path) { 'spec/fixtures/missing_plurals.po' }
 
       it 'has no errors' do
-        is_expected.to be_empty
+        is_expected.not_to be_empty
       end
     end
 
@@ -151,13 +151,33 @@ describe Gitlab::I18n::PoLinter do
       expect(linter).to receive(:validate_flags).with([], fake_entry)
       expect(linter).to receive(:validate_variables).with([], fake_entry)
       expect(linter).to receive(:validate_newlines).with([], fake_entry)
+      expect(linter).to receive(:validate_number_of_plurals).with([], fake_entry)
 
       linter.validate_entry(fake_entry)
     end
   end
 
+  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(linter).to receive(:metadata).and_return(fake_metadata)
+
+      fake_entry = Gitlab::I18n::PoEntry.new(
+        msgid: 'the singular',
+        msgid_plural: 'the plural',
+        'msgstr[0]' => 'the singular'
+      )
+      errors = []
+
+      linter.validate_number_of_plurals(errors, fake_entry)
+
+      expect(errors).to include('should have 2 translations')
+    end
+  end
+
   describe '#validate_variables' do
-    it 'validates both signular and plural in a pluralized string' do
+    it 'validates both signular and plural in a pluralized string when the entry has a singular' do
       pluralized_entry = Gitlab::I18n::PoEntry.new({
         msgid: 'Hello %{world}',
         msgid_plural: 'Hello all %{world}',
@@ -173,6 +193,19 @@ describe Gitlab::I18n::PoLinter do
       linter.validate_variables([], pluralized_entry)
     end
 
+    it 'only validates plural when there is no separate singular' do
+      pluralized_entry = Gitlab::I18n::PoEntry.new({
+        msgid: 'Hello %{world}',
+        msgid_plural: 'Hello all %{world}',
+        'msgstr[0]' => 'Bonjour %{world}'
+      })
+
+      expect(linter).to receive(:validate_variables_in_message)
+                          .with([], 'Hello all %{world}', 'Bonjour %{world}')
+
+      linter.validate_variables([], pluralized_entry)
+    end
+
     it 'validates the message variables' do
       entry = Gitlab::I18n::PoEntry.new({ msgid: 'Hello', msgstr: 'Bonjour' })