Proper icon validator

22 files changed, 123 insertions, 61 deletions

diff --git a/app/assets/javascripts/vue_shared/components/icon.vue b/app/assets/javascripts/vue_shared/components/icon.vue
index c42c4a1fbe7..e7ff76c8218 100644
--- a/app/assets/javascripts/vue_shared/components/icon.vue
+++ b/app/assets/javascripts/vue_shared/components/icon.vue
@@ -1,24 +1,40 @@
 <script>
-/* This is a re-usable vue component for rendering a svg sprite
-  icon
 
-  Sample configuration:
-
-  <icon
-    name="retry"
-    :size="32"
-    css-classes="top"
-  />
-
-*/
 // only allow classes in images.scss e.g. s12
 const validSizes = [8, 12, 16, 18, 24, 32, 48, 72];
+let iconValidator = () => true;
+
+/*
+ During development/tests we want to validate that we are just using icons that are actually defined
+*/
+if (process.env.NODE_ENV !== 'production') {
+  // eslint-disable-next-line global-require
+  const data = require('@gitlab-org/gitlab-svgs/dist/icons.json');
+  const { icons } = data;
+  iconValidator = value => {
+    if (icons.includes(value)) {
+      return true;
+    }
+    // eslint-disable-next-line no-console
+    console.warn(`Icon '${value}' is not a known icon of @gitlab/gitlab-svg`);
+    return false;
+  };
+}
 
+/** This is a re-usable vue component for rendering a svg sprite icon
+ *  @example
+ *  <icon
+ *    name="retry"
+ *    :size="32"
+ *    css-classes="top"
+ *  />
+ */
 export default {
   props: {
     name: {
       type: String,
       required: true,
+      validator: iconValidator,
     },
 
     size: {
@@ -83,6 +99,6 @@ export default {
     :x="x"
     :y="y"
   >
-    <use v-bind="{ 'xlink:href':spriteHref }" />
+    <use v-bind="{ 'xlink:href':spriteHref }"/>
   </svg>
 </template>
diff --git a/app/helpers/icons_helper.rb b/app/helpers/icons_helper.rb
index 2f304b040c7..41084ec686f 100644
--- a/app/helpers/icons_helper.rb
+++ b/app/helpers/icons_helper.rb
@@ -1,3 +1,5 @@
+require 'json'
+
 module IconsHelper
   extend self
   include FontAwesome::Rails::IconHelper
@@ -38,6 +40,13 @@ module IconsHelper
   end
 
   def sprite_icon(icon_name, size: nil, css_class: nil)
+    if Gitlab::Sentry.should_raise?
+      unless known_sprites.include?(icon_name)
+        exception = ArgumentError.new("#{icon_name} is not a known icon in @gitlab-org/gitlab-svg")
+        raise exception
+      end
+    end
+
     css_classes = size ? "s#{size}" : ""
     css_classes << " #{css_class}" unless css_class.blank?
     content_tag(:svg, content_tag(:use, "", { "xlink:href" => "#{sprite_icon_path}##{icon_name}" } ), class: css_classes.empty? ? nil : css_classes)
@@ -134,4 +143,10 @@ module IconsHelper
 
     icon_class
   end
+
+  private
+
+  def known_sprites
+    @known_sprites ||= JSON.parse(File.read(Rails.root.join('node_modules/@gitlab-org/gitlab-svgs/dist/icons.json')))['icons']
+  end
 end
diff --git a/package.json b/package.json
index 9dd7d80945f..e1801d4d435 100644
--- a/package.json
+++ b/package.json
@@ -18,7 +18,7 @@
     "webpack-prod": "NODE_ENV=production webpack --config config/webpack.config.js"
   },
   "dependencies": {
-    "@gitlab-org/gitlab-svgs": "^1.25.0",
+    "@gitlab-org/gitlab-svgs": "^1.26.0",
     "autosize": "^4.0.0",
     "axios": "^0.17.1",
     "babel-core": "^6.26.3",
diff --git a/spec/helpers/icons_helper_spec.rb b/spec/helpers/icons_helper_spec.rb
index 93d8e672f8c..82f588d1a08 100644
--- a/spec/helpers/icons_helper_spec.rb
+++ b/spec/helpers/icons_helper_spec.rb
@@ -55,6 +55,29 @@ describe IconsHelper do
       expect(sprite_icon(icon_name, size: 72, css_class: 'icon-danger').to_s)
         .to eq "<svg class=\"s72 icon-danger\"><use xlink:href=\"#{icons_path}##{icon_name}\"></use></svg>"
     end
+
+    describe 'non existing icon' do
+      non_existing = 'non_existing_icon_sprite'
+
+      it 'should raise in development mode' do
+        allow(Rails.env).to receive(:development?).and_return(true)
+
+        expect { sprite_icon(non_existing) }.to raise_error(ArgumentError, /is not a known icon/)
+      end
+
+      it 'should raise in test mode' do
+        allow(Rails.env).to receive(:test?).and_return(true)
+
+        expect { sprite_icon(non_existing) }.to raise_error(ArgumentError, /is not a known icon/)
+      end
+
+      it 'should not raise in production mode' do
+        allow(Rails.env).to receive(:test?).and_return(false)
+        allow(Rails.env).to receive(:development?).and_return(false)
+
+        expect { sprite_icon(non_existing) }.not_to raise_error
+      end
+    end
   end
 
   describe 'file_type_icon_class' do
diff --git a/spec/javascripts/ide/components/ide_status_bar_spec.js b/spec/javascripts/ide/components/ide_status_bar_spec.js
index 9895682f388..0e93c5193a1 100644
--- a/spec/javascripts/ide/components/ide_status_bar_spec.js
+++ b/spec/javascripts/ide/components/ide_status_bar_spec.js
@@ -70,7 +70,7 @@ describe('ideStatusBar', () => {
           status: {
             text: 'success',
             details_path: 'test',
-            icon: 'success',
+            icon: 'status_success',
           },
         },
       });
diff --git a/spec/javascripts/ide/components/jobs/detail/description_spec.js b/spec/javascripts/ide/components/jobs/detail/description_spec.js
index 9b715a41499..babae00d2f7 100644
--- a/spec/javascripts/ide/components/jobs/detail/description_spec.js
+++ b/spec/javascripts/ide/components/jobs/detail/description_spec.js
@@ -23,6 +23,6 @@ describe('IDE job description', () => {
   });
 
   it('renders CI icon', () => {
-    expect(vm.$el.querySelector('.ci-status-icon .ic-status_passed_borderless')).not.toBe(null);
+    expect(vm.$el.querySelector('.ci-status-icon .ic-status_success_borderless')).not.toBe(null);
   });
 });
diff --git a/spec/javascripts/ide/components/jobs/item_spec.js b/spec/javascripts/ide/components/jobs/item_spec.js
index 79e07f00e7b..2f97d39e98e 100644
--- a/spec/javascripts/ide/components/jobs/item_spec.js
+++ b/spec/javascripts/ide/components/jobs/item_spec.js
@@ -24,7 +24,7 @@ describe('IDE jobs item', () => {
   });
 
   it('renders CI icon', () => {
-    expect(vm.$el.querySelector('.ic-status_passed_borderless')).not.toBe(null);
+    expect(vm.$el.querySelector('.ic-status_success_borderless')).not.toBe(null);
   });
 
   it('does not render view logs button if not started', done => {
diff --git a/spec/javascripts/ide/mock_data.js b/spec/javascripts/ide/mock_data.js
index 4bbda4c8e80..7be450a0df7 100644
--- a/spec/javascripts/ide/mock_data.js
+++ b/spec/javascripts/ide/mock_data.js
@@ -74,7 +74,7 @@ export const jobs = [
     name: 'test',
     path: 'testing',
     status: {
-      icon: 'status_passed',
+      icon: 'status_success',
       text: 'passed',
     },
     stage: 'test',
@@ -86,7 +86,7 @@ export const jobs = [
     name: 'test 2',
     path: 'testing2',
     status: {
-      icon: 'status_passed',
+      icon: 'status_success',
       text: 'passed',
     },
     stage: 'test',
@@ -98,7 +98,7 @@ export const jobs = [
     name: 'test 3',
     path: 'testing3',
     status: {
-      icon: 'status_passed',
+      icon: 'status_success',
       text: 'passed',
     },
     stage: 'test',
@@ -146,7 +146,7 @@ export const fullPipelinesResponse = {
         },
         details: {
           status: {
-            icon: 'status_passed',
+            icon: 'status_success',
             text: 'passed',
           },
           stages: [...stages],
diff --git a/spec/javascripts/jobs/header_spec.js b/spec/javascripts/jobs/header_spec.js
index cef30a007db..e21e2c6d6e3 100644
--- a/spec/javascripts/jobs/header_spec.js
+++ b/spec/javascripts/jobs/header_spec.js
@@ -20,7 +20,7 @@ describe('Job details header', () => {
       job: {
         status: {
           group: 'failed',
-          icon: 'ci-status-failed',
+          icon: 'status_failed',
           label: 'failed',
           text: 'failed',
           details_path: 'path',
diff --git a/spec/javascripts/jobs/mock_data.js b/spec/javascripts/jobs/mock_data.js
index dd025255bd1..8fdd9b309b7 100644
--- a/spec/javascripts/jobs/mock_data.js
+++ b/spec/javascripts/jobs/mock_data.js
@@ -14,7 +14,7 @@ export default {
   finished_at: threeWeeksAgo.toISOString(),
   queued: 9.54,
   status: {
-    icon: 'icon_status_success',
+    icon: 'status_success',
     text: 'passed',
     label: 'passed',
     group: 'success',
@@ -72,7 +72,7 @@ export default {
     },
     details: {
       status: {
-        icon: 'icon_status_success',
+        icon: 'status_success',
         text: 'passed',
         label: 'passed',
         group: 'success',
diff --git a/spec/javascripts/pipelines/graph/job_component_spec.js b/spec/javascripts/pipelines/graph/job_component_spec.js
index 9c55a19ebc7..56476253ad0 100644
--- a/spec/javascripts/pipelines/graph/job_component_spec.js
+++ b/spec/javascripts/pipelines/graph/job_component_spec.js
@@ -10,7 +10,7 @@ describe('pipeline graph job component', () => {
     id: 4256,
     name: 'test',
     status: {
-      icon: 'icon_status_success',
+      icon: 'status_success',
       text: 'passed',
       label: 'passed',
       tooltip: 'passed',
@@ -65,7 +65,7 @@ describe('pipeline graph job component', () => {
           id: 4257,
           name: 'test',
           status: {
-            icon: 'icon_status_success',
+            icon: 'status_success',
             text: 'passed',
             label: 'passed',
             group: 'success',
@@ -111,7 +111,7 @@ describe('pipeline graph job component', () => {
           id: 4258,
           name: 'test',
           status: {
-            icon: 'icon_status_success',
+            icon: 'status_success',
           },
         },
       });
@@ -125,7 +125,7 @@ describe('pipeline graph job component', () => {
           id: 4259,
           name: 'test',
           status: {
-            icon: 'icon_status_success',
+            icon: 'status_success',
             label: 'success',
             tooltip: 'success',
           },
diff --git a/spec/javascripts/pipelines/graph/job_name_component_spec.js b/spec/javascripts/pipelines/graph/job_name_component_spec.js
index 8e2071ba0b3..c861d452dd0 100644
--- a/spec/javascripts/pipelines/graph/job_name_component_spec.js
+++ b/spec/javascripts/pipelines/graph/job_name_component_spec.js
@@ -10,7 +10,7 @@ describe('job name component', () => {
       propsData: {
         name: 'foo',
         status: {
-          icon: 'icon_status_success',
+          icon: 'status_success',
         },
       },
     }).$mount();
diff --git a/spec/javascripts/pipelines/graph/mock_data.js b/spec/javascripts/pipelines/graph/mock_data.js
index 9e25a4b3fed..b2161d54bce 100644
--- a/spec/javascripts/pipelines/graph/mock_data.js
+++ b/spec/javascripts/pipelines/graph/mock_data.js
@@ -13,7 +13,7 @@ export default {
   path: '/root/ci-mock/pipelines/123',
   details: {
     status: {
-      icon: 'icon_status_success',
+      icon: 'status_success',
       text: 'passed',
       label: 'passed',
       group: 'success',
@@ -33,7 +33,7 @@ export default {
             name: 'test',
             size: 1,
             status: {
-              icon: 'icon_status_success',
+              icon: 'status_success',
               text: 'passed',
               label: 'passed',
               group: 'success',
@@ -58,7 +58,7 @@ export default {
                 created_at: '2017-04-13T09:25:18.959Z',
                 updated_at: '2017-04-13T09:25:23.118Z',
                 status: {
-                  icon: 'icon_status_success',
+                  icon: 'status_success',
                   text: 'passed',
                   label: 'passed',
                   group: 'success',
@@ -78,7 +78,7 @@ export default {
           },
         ],
         status: {
-          icon: 'icon_status_success',
+          icon: 'status_success',
           text: 'passed',
           label: 'passed',
           group: 'success',
@@ -98,7 +98,7 @@ export default {
             name: 'deploy to production',
             size: 1,
             status: {
-              icon: 'icon_status_success',
+              icon: 'status_success',
               text: 'passed',
               label: 'passed',
               group: 'success',
@@ -123,7 +123,7 @@ export default {
                 created_at: '2017-04-19T14:29:46.463Z',
                 updated_at: '2017-04-19T14:30:27.498Z',
                 status: {
-                  icon: 'icon_status_success',
+                  icon: 'status_success',
                   text: 'passed',
                   label: 'passed',
                   group: 'success',
@@ -145,7 +145,7 @@ export default {
             name: 'deploy to staging',
             size: 1,
             status: {
-              icon: 'icon_status_success',
+              icon: 'status_success',
               text: 'passed',
               label: 'passed',
               group: 'success',
@@ -170,7 +170,7 @@ export default {
                 created_at: '2017-04-18T16:32:08.420Z',
                 updated_at: '2017-04-18T16:32:12.631Z',
                 status: {
-                  icon: 'icon_status_success',
+                  icon: 'status_success',
                   text: 'passed',
                   label: 'passed',
                   group: 'success',
@@ -190,7 +190,7 @@ export default {
           },
         ],
         status: {
-          icon: 'icon_status_success',
+          icon: 'status_success',
           text: 'passed',
           label: 'passed',
           group: 'success',
diff --git a/spec/javascripts/pipelines/graph/stage_column_component_spec.js b/spec/javascripts/pipelines/graph/stage_column_component_spec.js
index f744f1af5e6..9d1e71fd117 100644
--- a/spec/javascripts/pipelines/graph/stage_column_component_spec.js
+++ b/spec/javascripts/pipelines/graph/stage_column_component_spec.js
@@ -7,7 +7,7 @@ describe('stage column component', () => {
     id: 4250,
     name: 'test',
     status: {
-      icon: 'icon_status_success',
+      icon: 'status_success',
       text: 'passed',
       label: 'passed',
       group: 'success',
diff --git a/spec/javascripts/pipelines/header_component_spec.js b/spec/javascripts/pipelines/header_component_spec.js
index cecc7ceb53d..034d3b4957d 100644
--- a/spec/javascripts/pipelines/header_component_spec.js
+++ b/spec/javascripts/pipelines/header_component_spec.js
@@ -18,7 +18,7 @@ describe('Pipeline details header', () => {
         details: {
           status: {
             group: 'failed',
-            icon: 'ci-status-failed',
+            icon: 'status_failed',
             label: 'failed',
             text: 'failed',
             details_path: 'path',
diff --git a/spec/javascripts/pipelines/stage_spec.js b/spec/javascripts/pipelines/stage_spec.js
index 16f6db39d6a..3f6789759ae 100644
--- a/spec/javascripts/pipelines/stage_spec.js
+++ b/spec/javascripts/pipelines/stage_spec.js
@@ -20,7 +20,7 @@ describe('Pipelines stage component', () => {
       stage: {
         status: {
           group: 'success',
-          icon: 'icon_status_success',
+          icon: 'status_success',
           title: 'success',
         },
         dropdown_path: 'path.json',
@@ -84,7 +84,7 @@ describe('Pipelines stage component', () => {
       component.stage = {
         status: {
           group: 'running',
-          icon: 'running',
+          icon: 'status_running',
           title: 'running',
         },
         dropdown_path: 'bar.json',
diff --git a/spec/javascripts/vue_mr_widget/mock_data.js b/spec/javascripts/vue_mr_widget/mock_data.js
index c0b5a7d4455..7fd1a2350f7 100644
--- a/spec/javascripts/vue_mr_widget/mock_data.js
+++ b/spec/javascripts/vue_mr_widget/mock_data.js
@@ -76,7 +76,7 @@ export default {
     path: '/root/acets-app/pipelines/172',
     details: {
       status: {
-        icon: 'icon_status_success',
+        icon: 'status_success',
         favicon: 'favicon_status_success',
         text: 'passed',
         label: 'passed',
@@ -91,7 +91,7 @@ export default {
           name: 'build',
           title: 'build: failed',
           status: {
-            icon: 'icon_status_failed',
+            icon: 'status_failed',
             favicon: 'favicon_status_failed',
             text: 'failed',
             label: 'failed',
@@ -106,7 +106,7 @@ export default {
           name: 'review',
           title: 'review: skipped',
           status: {
-            icon: 'icon_status_skipped',
+            icon: 'status_skipped',
             favicon: 'favicon_status_skipped',
             text: 'skipped',
             label: 'skipped',
diff --git a/spec/javascripts/vue_shared/components/ci_icon_spec.js b/spec/javascripts/vue_shared/components/ci_icon_spec.js
index 423bc746a22..b59a7d7544f 100644
--- a/spec/javascripts/vue_shared/components/ci_icon_spec.js
+++ b/spec/javascripts/vue_shared/components/ci_icon_spec.js
@@ -13,7 +13,7 @@ describe('CI Icon component', () => {
   it('should render a span element with an svg', () => {
     vm = mountComponent(Component, {
       status: {
-        icon: 'icon_status_success',
+        icon: 'status_success',
       },
     });
 
@@ -24,7 +24,7 @@ describe('CI Icon component', () => {
   it('should render a success status', () => {
     vm = mountComponent(Component, {
       status: {
-        icon: 'icon_status_success',
+        icon: 'status_success',
         group: 'success',
       },
     });
@@ -35,7 +35,7 @@ describe('CI Icon component', () => {
   it('should render a failed status', () => {
     vm = mountComponent(Component, {
       status: {
-        icon: 'icon_status_failed',
+        icon: 'status_failed',
         group: 'failed',
       },
     });
@@ -46,7 +46,7 @@ describe('CI Icon component', () => {
   it('should render success with warnings status', () => {
     vm = mountComponent(Component, {
       status: {
-        icon: 'icon_status_warning',
+        icon: 'status_warning',
         group: 'warning',
       },
     });
@@ -57,7 +57,7 @@ describe('CI Icon component', () => {
   it('should render pending status', () => {
     vm = mountComponent(Component, {
       status: {
-        icon: 'icon_status_pending',
+        icon: 'status_pending',
         group: 'pending',
       },
     });
@@ -68,7 +68,7 @@ describe('CI Icon component', () => {
   it('should render running status', () => {
     vm = mountComponent(Component, {
       status: {
-        icon: 'icon_status_running',
+        icon: 'status_running',
         group: 'running',
       },
     });
@@ -79,7 +79,7 @@ describe('CI Icon component', () => {
   it('should render created status', () => {
     vm = mountComponent(Component, {
       status: {
-        icon: 'icon_status_created',
+        icon: 'status_created',
         group: 'created',
       },
     });
@@ -90,7 +90,7 @@ describe('CI Icon component', () => {
   it('should render skipped status', () => {
     vm = mountComponent(Component, {
       status: {
-        icon: 'icon_status_skipped',
+        icon: 'status_skipped',
         group: 'skipped',
       },
     });
@@ -101,7 +101,7 @@ describe('CI Icon component', () => {
   it('should render canceled status', () => {
     vm = mountComponent(Component, {
       status: {
-        icon: 'icon_status_canceled',
+        icon: 'status_canceled',
         group: 'canceled',
       },
     });
@@ -112,7 +112,7 @@ describe('CI Icon component', () => {
   it('should render status for manual action', () => {
     vm = mountComponent(Component, {
       status: {
-        icon: 'icon_status_manual',
+        icon: 'status_manual',
         group: 'manual',
       },
     });
diff --git a/spec/javascripts/vue_shared/components/header_ci_component_spec.js b/spec/javascripts/vue_shared/components/header_ci_component_spec.js
index 65499a2d730..f17818c17c7 100644
--- a/spec/javascripts/vue_shared/components/header_ci_component_spec.js
+++ b/spec/javascripts/vue_shared/components/header_ci_component_spec.js
@@ -12,7 +12,7 @@ describe('Header CI Component', () => {
     props = {
       status: {
         group: 'failed',
-        icon: 'ci-status-failed',
+        icon: 'status_failed',
         label: 'failed',
         text: 'failed',
         details_path: 'path',
diff --git a/spec/javascripts/vue_shared/components/icon_spec.js b/spec/javascripts/vue_shared/components/icon_spec.js
index cc030e29d61..882420e602e 100644
--- a/spec/javascripts/vue_shared/components/icon_spec.js
+++ b/spec/javascripts/vue_shared/components/icon_spec.js
@@ -10,7 +10,7 @@ describe('Sprite Icon Component', function () {
       const IconComponent = Vue.extend(Icon);
 
       icon = mountComponent(IconComponent, {
-        name: 'test',
+        name: 'commit',
         size: 32,
         cssClasses: 'extraclasses',
       });
@@ -30,7 +30,7 @@ describe('Sprite Icon Component', function () {
 
     it('should have <use> as a child element with the correct href', function () {
       expect(icon.$el.firstChild.tagName).toBe('use');
-      expect(icon.$el.firstChild.getAttribute('xlink:href')).toBe(`${gon.sprite_icons}#test`);
+      expect(icon.$el.firstChild.getAttribute('xlink:href')).toBe(`${gon.sprite_icons}#commit`);
     });
 
     it('should properly compute iconSizeClass', function () {
@@ -50,5 +50,13 @@ describe('Sprite Icon Component', function () {
       expect(containsSizeClass).toBe(true);
       expect(containsCustomClass).toBe(true);
     });
+
+    it('`name` validator should return false for non existing icons', () => {
+      expect(Icon.props.name.validator('non_existing_icon_sprite')).toBe(false);
+    });
+
+    it('`name` validator should return false for existing icons', () => {
+      expect(Icon.props.name.validator('commit')).toBe(true);
+    });
   });
 });
diff --git a/spec/javascripts/vue_shared/components/notes/system_note_spec.js b/spec/javascripts/vue_shared/components/notes/system_note_spec.js
index aa4c9c4c88c..2a6015fe35f 100644
--- a/spec/javascripts/vue_shared/components/notes/system_note_spec.js
+++ b/spec/javascripts/vue_shared/components/notes/system_note_spec.js
@@ -19,7 +19,7 @@ describe('system note component', () => {
           path: '/root',
         },
         note_html: '<p dir="auto">closed</p>',
-        system_note_icon_name: 'icon_status_closed',
+        system_note_icon_name: 'status_closed',
         created_at: '2017-08-02T10:51:58.559Z',
       },
     };
diff --git a/yarn.lock b/yarn.lock
index 9d0eae5f509..67f9aa98c74 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -78,9 +78,9 @@
     lodash "^4.2.0"
     to-fast-properties "^2.0.0"
 
-"@gitlab-org/gitlab-svgs@^1.25.0":
-  version "1.25.0"
-  resolved "https://registry.yarnpkg.com/@gitlab-org/gitlab-svgs/-/gitlab-svgs-1.25.0.tgz#1a82b1be43e1a46e6b0767ef46f26f5fd6bbd101"
+"@gitlab-org/gitlab-svgs@^1.26.0":
+  version "1.26.0"
+  resolved "https://registry.yarnpkg.com/@gitlab-org/gitlab-svgs/-/gitlab-svgs-1.26.0.tgz#d89c633e866d031a9e4787b05eacc7144c3a7791"
 
 "@sindresorhus/is@^0.7.0":
   version "0.7.0"