Validate unescaped `%` chars in PO files

5 files changed, 140 insertions, 4 deletions

diff --git a/lib/gitlab/i18n/po_linter.rb b/lib/gitlab/i18n/po_linter.rb
index c3b1fe582af..2e02787a4f4 100644
--- a/lib/gitlab/i18n/po_linter.rb
+++ b/lib/gitlab/i18n/po_linter.rb
@@ -63,10 +63,25 @@ module Gitlab
         validate_variables(errors, entry)
         validate_newlines(errors, entry)
         validate_number_of_plurals(errors, entry)
+        validate_unescaped_chars(errors, entry)
 
         errors
       end
 
+      def validate_unescaped_chars(errors, entry)
+        if entry.msgid_contains_unescaped_chars?
+          errors << 'contains unescaped `%`, escape it using `%%`'
+        end
+
+        if entry.plural_id_contains_unescaped_chars?
+          errors << 'plural id contains unescaped `%`, escape it using `%%`'
+        end
+
+        if entry.translations_contain_unescaped_chars?
+          errors << 'translation contains unescaped `%`, escape it using `%%`'
+        end
+      end
+
       def validate_number_of_plurals(errors, entry)
         return unless metadata_entry&.expected_plurals
         return unless entry.translated?
@@ -79,15 +94,15 @@ module Gitlab
 
       def validate_newlines(errors, entry)
         if entry.msgid_contains_newlines?
-          errors << "is defined over multiple lines, this breaks some tooling."
+          errors << 'is defined over multiple lines, this breaks some tooling.'
         end
 
         if entry.plural_id_contains_newlines?
-          errors << "plural is defined over multiple lines, this breaks some tooling."
+          errors << 'plural is defined over multiple lines, this breaks some tooling.'
         end
 
         if entry.translations_contain_newlines?
-          errors << "has translations defined over multiple lines, this breaks some tooling."
+          errors << 'has translations defined over multiple lines, this breaks some tooling.'
         end
       end
 
diff --git a/lib/gitlab/i18n/translation_entry.rb b/lib/gitlab/i18n/translation_entry.rb
index 8d4fec0decd..e6c95afca7e 100644
--- a/lib/gitlab/i18n/translation_entry.rb
+++ b/lib/gitlab/i18n/translation_entry.rb
@@ -1,6 +1,8 @@
 module Gitlab
   module I18n
     class TranslationEntry
+      PERCENT_REGEX = /(?:^|[^%])%(?!{\w*}|[a-z%])/.freeze
+
       attr_reader :nplurals, :entry_data
 
       def initialize(entry_data, nplurals)
@@ -64,6 +66,22 @@ module Gitlab
         all_translations.any? { |translation| translation.is_a?(Array) }
       end
 
+      def msgid_contains_unescaped_chars?
+        contains_unescaped_chars?(msgid)
+      end
+
+      def plural_id_contains_unescaped_chars?
+        contains_unescaped_chars?(plural_id)
+      end
+
+      def translations_contain_unescaped_chars?
+        all_translations.any? { |translation| contains_unescaped_chars?(translation) }
+      end
+
+      def contains_unescaped_chars?(string)
+        string =~ PERCENT_REGEX
+      end
+
       private
 
       def translation_keys
diff --git a/spec/fixtures/unescaped_chars.po b/spec/fixtures/unescaped_chars.po
new file mode 100644
index 00000000000..fbafe523fb3
--- /dev/null
+++ b/spec/fixtures/unescaped_chars.po
@@ -0,0 +1,21 @@
+# 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 "You are going to transfer %{project_name_with_namespace} to another owner. Are you ABSOLUTELY sure?"
+msgstr "將要把 %{project_name_with_namespace} 的所有權轉移給另一個人。真的「100%確定」要這麼做嗎？"
diff --git a/spec/lib/gitlab/i18n/po_linter_spec.rb b/spec/lib/gitlab/i18n/po_linter_spec.rb
index bd31f0c3871..cd5c2b99751 100644
--- a/spec/lib/gitlab/i18n/po_linter_spec.rb
+++ b/spec/lib/gitlab/i18n/po_linter_spec.rb
@@ -110,6 +110,17 @@ describe Gitlab::I18n::PoLinter do
         is_expected.not_to be_empty
       end
     end
+
+    context 'with unescaped chars' do
+      let(:po_path) { 'spec/fixtures/unescaped_chars.po' }
+
+      it 'contains an error' do
+        message_id = 'You are going to transfer %{project_name_with_namespace} to another owner. Are you ABSOLUTELY sure?'
+        expected_error = 'translation contains unescaped `%`, escape it using `%%`'
+
+        expect(errors[message_id]).to include(expected_error)
+      end
+    end
   end
 
   describe '#parse_po' do
@@ -157,13 +168,14 @@ describe Gitlab::I18n::PoLinter do
   end
 
   describe '#validate_entry' do
-    it 'validates the flags, variable usage, and newlines' do
+    it 'validates the flags, variable usage, newlines, and unescaped chars' do
       fake_entry = double
 
       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)
+      expect(linter).to receive(:validate_unescaped_chars).with([], fake_entry)
 
       linter.validate_entry(fake_entry)
     end
diff --git a/spec/lib/gitlab/i18n/translation_entry_spec.rb b/spec/lib/gitlab/i18n/translation_entry_spec.rb
index d2d3ec03c6d..f68bc8feff9 100644
--- a/spec/lib/gitlab/i18n/translation_entry_spec.rb
+++ b/spec/lib/gitlab/i18n/translation_entry_spec.rb
@@ -130,4 +130,74 @@ describe Gitlab::I18n::TranslationEntry do
       expect(entry.translations_contain_newlines?).to be_truthy
     end
   end
+
+  describe '#contains_unescaped_chars' do
+    let(:data) { { msgid: '' } }
+    let(:entry) { described_class.new(data, 2) }
+    it 'is true when the msgid is an array' do
+      string = '「100%確定」'
+
+      expect(entry.contains_unescaped_chars?(string)).to be_truthy
+    end
+
+    it 'is false when the `%` char is escaped' do
+      string = '「100%%確定」'
+
+      expect(entry.contains_unescaped_chars?(string)).to be_falsy
+    end
+
+    it 'is false when using an unnamed variable' do
+      string = '「100%d確定」'
+
+      expect(entry.contains_unescaped_chars?(string)).to be_falsy
+    end
+
+    it 'is false when using a named variable' do
+      string = '「100%{named}確定」'
+
+      expect(entry.contains_unescaped_chars?(string)).to be_falsy
+    end
+
+    it 'is true when an unnamed variable is not closed' do
+      string = '「100%{named確定」'
+
+      expect(entry.contains_unescaped_chars?(string)).to be_truthy
+    end
+
+    it 'is true when the string starts with a `%`' do
+      string = '%10'
+
+      expect(entry.contains_unescaped_chars?(string)).to be_truthy
+    end
+  end
+
+  describe '#msgid_contains_unescaped_chars' do
+    it 'is true when the msgid contains a `%`' do
+      data = { msgid: '「100%確定」' }
+      entry = described_class.new(data, 2)
+
+      expect(entry).to receive(:contains_unescaped_chars?).and_call_original
+      expect(entry.msgid_contains_unescaped_chars?).to be_truthy
+    end
+  end
+
+  describe '#plural_id_contains_unescaped_chars' do
+    it 'is true when the plural msgid contains a `%`' do
+      data = { msgid_plural: '「100%確定」' }
+      entry = described_class.new(data, 2)
+
+      expect(entry).to receive(:contains_unescaped_chars?).and_call_original
+      expect(entry.plural_id_contains_unescaped_chars?).to be_truthy
+    end
+  end
+
+  describe '#translations_contain_unescaped_chars' do
+    it 'is true when the translation contains a `%`' do
+      data = { msgstr: '「100%確定」' }
+      entry = described_class.new(data, 2)
+
+      expect(entry).to receive(:contains_unescaped_chars?).and_call_original
+      expect(entry.translations_contain_unescaped_chars?).to be_truthy
+    end
+  end
 end