Merge branch 'refactor-pipeline-warnings-ux' into 'master'

Group and limit pipeline warnings

See merge request gitlab-org/gitlab!39634

app/helpers/ci/pipelines_helper.rb

@@ -2,16 +2,35 @@
 
 module Ci
   module PipelinesHelper
+    include Gitlab::Ci::Warnings
+
     def pipeline_warnings(pipeline)
       return unless pipeline.warning_messages.any?
 
-      content_tag(:div, class: 'alert alert-warning') do
-        content_tag(:h4, 'Warning:') <<
-          content_tag(:div) do
-            pipeline.warning_messages.each do |warning|
-              concat(markdown(warning.content))
-            end
-          end
+      total_warnings = pipeline.warning_messages.length
+      message = warning_header(total_warnings)
+
+      content_tag(:div, class: 'bs-callout bs-callout-warning') do
+        content_tag(:details) do
+          concat content_tag(:summary, message, class: 'gl-mb-2')
+          warning_markdown(pipeline) { |markdown| concat markdown }
+        end
+      end
+    end
+
+    def warning_header(count)
+      message = _("%{total_warnings} warning(s) found:") % { total_warnings: count }
+
+      return message unless count > MAX_LIMIT
+
+      _("%{message} showing first %{warnings_displayed}") % { message: message, warnings_displayed: MAX_LIMIT }
+    end
+
+    private
+
+    def warning_markdown(pipeline)
+      pipeline.warning_messages(limit: MAX_LIMIT).each do |warning|
+        yield markdown(warning.content)
       end
     end
   end

app/models/ci/pipeline.rb

@@ -677,8 +677,10 @@ module Ci
       messages.select(&:error?)
     end
 
-    def warning_messages
-      messages.select(&:warning?)
+    def warning_messages(limit: nil)
+      messages.select(&:warning?).tap do |warnings|
+        break warnings.take(limit) if limit
+      end
     end
 
     # Manually set the notes for a Ci::Pipeline

app/views/projects/ci/lints/_lint_warnings.html.haml

 - if warnings
-  - warnings.each do |warning|
+  - total_warnings = warnings.length
+  - message = warning_header(total_warnings)
+
+  - if warnings.any?
     .bs-callout.bs-callout-warning
-      %p
-        %b= _("Warning:")
-        = markdown(warning)
+      %details
+        %summary.gl-mb-2= message
+        - warnings.each do |warning|
+          = markdown(warning)

app/views/projects/pipelines/_pipeline_warnings.html.haml

+- total_warnings = warnings.length
+- message = warning_header(total_warnings)
+
 - if warnings.any?
-  - warnings.map(&:content).each do |warning|
-    .bs-callout.bs-callout-warning
-      %p
-        %b= _("Warning:")
+  .bs-callout.bs-callout-warning
+    %details
+      %summary.gl-mb-2= message
+      - warnings.map(&:content).each do |warning|
         = markdown(warning)

app/views/projects/pipelines/show.html.haml

@@ -19,7 +19,7 @@
       - lint_link_start = '<a href="%{url}">'.html_safe % { url: lint_link_url }
       = s_('You can also test your %{gitlab_ci_yml} in %{lint_link_start}CI Lint%{lint_link_end}').html_safe % { gitlab_ci_yml: '.gitlab-ci.yml', lint_link_start: lint_link_start, lint_link_end: '</a>'.html_safe }
 
-  = render "projects/pipelines/pipeline_warnings", warnings: @pipeline.warning_messages
+  = render "projects/pipelines/pipeline_warnings", warnings: @pipeline.warning_messages(limit: Gitlab::Ci::Warnings::MAX_LIMIT)
   = render "projects/pipelines/with_tabs", pipeline: @pipeline, pipeline_has_errors: pipeline_has_errors
 
 .js-pipeline-details-vue{ data: { endpoint: project_pipeline_path(@project, @pipeline, format: :json) } }

changelogs/unreleased/refactor-pipeline-warnings-ux.yml

+---
+title: Group pipeline warnings and make them collapsible
+merge_request: 39634
+author:
+type: changed

lib/gitlab/ci/lint.rb

@@ -40,7 +40,7 @@ module Gitlab
         Result.new(
           jobs: dry_run_convert_to_jobs(pipeline.stages),
           errors: pipeline.error_messages.map(&:content),
-          warnings: pipeline.warning_messages.map(&:content)
+          warnings: pipeline.warning_messages(limit: ::Gitlab::Ci::Warnings::MAX_LIMIT).map(&:content)
         )
       end
 
@@ -55,7 +55,7 @@ module Gitlab
         Result.new(
           jobs: static_validation_convert_to_jobs(result),
           errors: result.errors,
-          warnings: result.warnings
+          warnings: result.warnings.take(::Gitlab::Ci::Warnings::MAX_LIMIT) # rubocop: disable CodeReuse/ActiveRecord
         )
       end
 

lib/gitlab/ci/warnings.rb

+# frozen_string_literal: true
+
+module Gitlab::Ci::Warnings
+  MAX_LIMIT = 25
+end

locale/gitlab.pot

@@ -545,6 +545,9 @@ msgstr ""
 msgid "%{mergeLength}/%{usersLength} can merge"
 msgstr ""
 
+msgid "%{message} showing first %{warnings_displayed}"
+msgstr ""
+
 msgid "%{milestone_name} (Past due)"
 msgstr ""
 
@@ -788,6 +791,9 @@ msgstr ""
 msgid "%{totalWeight} total weight"
 msgstr ""
 
+msgid "%{total_warnings} warning(s) found:"
+msgstr ""
+
 msgid "%{total} open issue weight"
 msgstr ""
 

spec/helpers/ci/pipelines_helper_spec.rb

@@ -22,8 +22,8 @@ RSpec.describe Ci::PipelinesHelper do
       let(:warning_messages) { [double(content: 'Warning 1'), double(content: 'Warning 2')] }
 
       it 'returns a warning callout box' do
-        expect(subject).to have_css 'div.alert-warning'
-        expect(subject).to include 'Warning:'
+        expect(subject).to have_css 'div.bs-callout-warning'
+        expect(subject).to include '2 warning(s) found:'
       end
 
       it 'lists the the warnings' do
@@ -32,4 +32,24 @@ RSpec.describe Ci::PipelinesHelper do
       end
     end
   end
+
+  describe 'warning_header' do
+    subject { helper.warning_header(count) }
+
+    context 'when warnings are more than max cap' do
+      let(:count) { 30 }
+
+      it 'returns 30 warning(s) found: showing first 25' do
+        expect(subject).to eq('30 warning(s) found: showing first 25')
+      end
+    end
+
+    context 'when warnings are less than max cap' do
+      let(:count) { 15 }
+
+      it 'returns 15 warning(s) found' do
+        expect(subject).to eq('15 warning(s) found:')
+      end
+    end
+  end
 end

spec/lib/gitlab/ci/lint_spec.rb

@@ -92,6 +92,31 @@ RSpec.describe Gitlab::Ci::Lint do
         end
       end
 
+      context 'when content has more warnings than max limit' do
+        # content will result in 2 warnings
+        let(:content) do
+          <<~YAML
+          rspec:
+            script: rspec
+            rules:
+              - when: always
+          rspec2:
+            script: rspec
+            rules:
+              - when: always
+          YAML
+        end
+
+        before do
+          stub_const('Gitlab::Ci::Warnings::MAX_LIMIT', 1)
+        end
+
+        it 'returns a result with warnings' do
+          expect(subject).to be_valid
+          expect(subject.warnings.size).to eq(1)
+        end
+      end
+
       context 'when content has errors and warnings' do
         let(:content) do
           <<~YAML

spec/views/projects/ci/lints/show.html.haml_spec.rb

@@ -87,7 +87,7 @@ RSpec.describe 'projects/ci/lints/show' do
       it 'shows warning messages' do
         render
 
-        expect(rendered).to have_content('Warning:')
+        expect(rendered).to have_content('2 warning(s) found:')
         expect(rendered).to have_content('Warning 1')
         expect(rendered).to have_content('Warning 2')
       end
@@ -116,7 +116,7 @@ RSpec.describe 'projects/ci/lints/show' do
     it 'shows warning messages' do
       render
 
-      expect(rendered).to have_content('Warning:')
+      expect(rendered).to have_content('2 warning(s) found:')
       expect(rendered).to have_content('Warning 1')
       expect(rendered).to have_content('Warning 2')
     end

spec/views/projects/pipelines/new.html.haml_spec.rb

@@ -26,7 +26,7 @@ RSpec.describe 'projects/pipelines/new' do
     it 'displays the warnings' do
       render
 
-      expect(rendered).to have_css('div.alert-warning')
+      expect(rendered).to have_css('div.bs-callout-warning')
       expect(rendered).to have_content('warning 1')
       expect(rendered).to have_content('warning 2')
     end