Merge branch '46552-fixes-redundant-message-for-failure-reasons' into 'master'

Fixes redudant script failure message

Closes #46552 and #44271

See merge request gitlab-org/gitlab-ce!19138

5 files changed, 26 insertions, 13 deletions

diff --git a/app/presenters/commit_status_presenter.rb b/app/presenters/commit_status_presenter.rb
index c7f7aa836bd..9a7aaf4ef32 100644
--- a/app/presenters/commit_status_presenter.rb
+++ b/app/presenters/commit_status_presenter.rb
@@ -1,11 +1,10 @@
 class CommitStatusPresenter < Gitlab::View::Presenter::Delegated
   CALLOUT_FAILURE_MESSAGES = {
       unknown_failure: 'There is an unknown failure, please try again',
-      script_failure: 'There has been a script failure. Check the job log for more information',
       api_failure: 'There has been an API failure, please try again',
       stuck_or_timeout_failure: 'There has been a timeout failure or the job got stuck. Check your timeout limits or try again',
       runner_system_failure: 'There has been a runner system failure, please try again',
-      missing_dependency_failure: 'There has been a missing dependency failure, check the job log for more information'
+      missing_dependency_failure: 'There has been a missing dependency failure'
   }.freeze
 
   presents :build
diff --git a/app/serializers/job_entity.rb b/app/serializers/job_entity.rb
index 3076fed1674..960e7291ae6 100644
--- a/app/serializers/job_entity.rb
+++ b/app/serializers/job_entity.rb
@@ -26,7 +26,7 @@ class JobEntity < Grape::Entity
   expose :created_at
   expose :updated_at
   expose :detailed_status, as: :status, with: StatusEntity
-  expose :callout_message, if: -> (*) { failed? }
+  expose :callout_message, if: -> (*) { failed? && !build.script_failure? }
   expose :recoverable, if: -> (*) { failed? }
 
   private
diff --git a/changelogs/unreleased/46552-fixes-redundant-message-for-failure-reasons.yml b/changelogs/unreleased/46552-fixes-redundant-message-for-failure-reasons.yml
new file mode 100644
index 00000000000..43427aaa242
--- /dev/null
+++ b/changelogs/unreleased/46552-fixes-redundant-message-for-failure-reasons.yml
@@ -0,0 +1,5 @@
+---
+title: Removes redundant script failure message from Job page
+merge_request: 19138
+author:
+type: changed
diff --git a/spec/presenters/ci/build_presenter_spec.rb b/spec/presenters/ci/build_presenter_spec.rb
index efd175247b5..6dfaa3b72f7 100644
--- a/spec/presenters/ci/build_presenter_spec.rb
+++ b/spec/presenters/ci/build_presenter_spec.rb
@@ -219,11 +219,11 @@ describe Ci::BuildPresenter do
   end
 
   describe '#callout_failure_message' do
-    let(:build) { create(:ci_build, :failed, :script_failure) }
+    let(:build) { create(:ci_build, :failed, :api_failure) }
 
     it 'returns a verbose failure reason' do
       description = subject.callout_failure_message
-      expect(description).to eq('There has been a script failure. Check the job log for more information')
+      expect(description).to eq('There has been an API failure, please try again')
     end
   end
 
diff --git a/spec/serializers/job_entity_spec.rb b/spec/serializers/job_entity_spec.rb
index c90396ebb28..a5581a34517 100644
--- a/spec/serializers/job_entity_spec.rb
+++ b/spec/serializers/job_entity_spec.rb
@@ -131,7 +131,7 @@ describe JobEntity do
   end
 
   context 'when job failed' do
-    let(:job) { create(:ci_build, :script_failure) }
+    let(:job) { create(:ci_build, :api_failure) }
 
     it 'contains details' do
       expect(subject[:status]).to include :icon, :favicon, :text, :label, :tooltip
@@ -142,20 +142,20 @@ describe JobEntity do
     end
 
     it 'should indicate the failure reason on tooltip' do
-      expect(subject[:status][:tooltip]).to eq('failed <br> (script failure)')
+      expect(subject[:status][:tooltip]).to eq('failed <br> (API failure)')
     end
 
     it 'should include a callout message with a verbose output' do
-      expect(subject[:callout_message]).to eq('There has been a script failure. Check the job log for more information')
+      expect(subject[:callout_message]).to eq('There has been an API failure, please try again')
     end
 
     it 'should state that it is not recoverable' do
-      expect(subject[:recoverable]).to be_falsy
+      expect(subject[:recoverable]).to be_truthy
     end
   end
 
   context 'when job is allowed to fail' do
-    let(:job) { create(:ci_build, :allowed_to_fail, :script_failure) }
+    let(:job) { create(:ci_build, :allowed_to_fail, :api_failure) }
 
     it 'contains details' do
       expect(subject[:status]).to include :icon, :favicon, :text, :label, :tooltip
@@ -166,15 +166,24 @@ describe JobEntity do
     end
 
     it 'should indicate the failure reason on tooltip' do
-      expect(subject[:status][:tooltip]).to eq('failed <br> (script failure) (allowed to fail)')
+      expect(subject[:status][:tooltip]).to eq('failed <br> (API failure) (allowed to fail)')
     end
 
     it 'should include a callout message with a verbose output' do
-      expect(subject[:callout_message]).to eq('There has been a script failure. Check the job log for more information')
+      expect(subject[:callout_message]).to eq('There has been an API failure, please try again')
     end
 
     it 'should state that it is not recoverable' do
-      expect(subject[:recoverable]).to be_falsy
+      expect(subject[:recoverable]).to be_truthy
+    end
+  end
+
+  context 'when the job failed with a script failure' do
+    let(:job) { create(:ci_build, :failed, :script_failure) }
+
+    it 'should not include callout message or recoverable keys' do
+      expect(subject).not_to include('callout_message')
+      expect(subject).not_to include('recoverable')
     end
   end