Merge branch '56871-list-issues-error' into 'master'

Catch exceptions in list_issues reactive cache calculation

Closes #56871

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

app/assets/javascripts/error_tracking/store/actions.js

@@ -2,7 +2,7 @@ import Service from '../services';
 import * as types from './mutation_types';
 import createFlash from '~/flash';
 import Poll from '~/lib/utils/poll';
-import { __ } from '~/locale';
+import { __, sprintf } from '~/locale';
 
 let eTagPoll;
 
@@ -19,9 +19,17 @@ export function startPolling({ commit }, endpoint) {
       commit(types.SET_EXTERNAL_URL, data.external_url);
       commit(types.SET_LOADING, false);
     },
-    errorCallback: () => {
+    errorCallback: response => {
+      let errorMessage = '';
+      if (response && response.data && response.data.message) {
+        errorMessage = response.data.message;
+      }
       commit(types.SET_LOADING, false);
-      createFlash(__('Failed to load errors from Sentry'));
+      createFlash(
+        sprintf(__(`Failed to load errors from Sentry. Error message: %{errorMessage}`), {
+          errorMessage,
+        }),
+      );
     },
   });
 

app/models/error_tracking/project_error_tracking_setting.rb

@@ -58,7 +58,7 @@ module ErrorTracking
 
     def list_sentry_issues(opts = {})
       with_reactive_cache('list_issues', opts.stringify_keys) do |result|
-        { issues: result }
+        result
       end
     end
 
@@ -69,8 +69,10 @@ module ErrorTracking
     def calculate_reactive_cache(request, opts)
       case request
       when 'list_issues'
-        sentry_client.list_issues(**opts.symbolize_keys)
+        { issues: sentry_client.list_issues(**opts.symbolize_keys) }
       end
+    rescue Sentry::Client::Error => e
+      { error: e.message }
     end
 
     # http://HOST/api/0/projects/ORG/PROJECT

app/services/error_tracking/list_issues_service.rb

@@ -6,15 +6,19 @@ module ErrorTracking
     DEFAULT_LIMIT = 20
 
     def execute
-      return error('not enabled') unless enabled?
-      return error('access denied') unless can_read?
+      return error('Error Tracking is not enabled') unless enabled?
+      return error('Access denied', :unauthorized) unless can_read?
 
       result = project_error_tracking_setting
         .list_sentry_issues(issue_status: issue_status, limit: limit)
 
       # our results are not yet ready
       unless result
-        return error('not ready', :no_content)
+        return error('Not ready. Try again later', :no_content)
+      end
+
+      if result[:error].present?
+        return error(result[:error], :bad_request)
       end
 
       success(issues: result[:issues])

changelogs/unreleased/56871-list-issues-error.yml

+---
+title: Display error message when API call to list Sentry issues fails
+merge_request: 24936
+author:
+type: fixed

lib/sentry/client.rb

@@ -54,7 +54,7 @@ module Sentry
 
     def handle_response(response)
       unless response.code == 200
-        raise Client::Error, "Sentry response error: #{response.code}"
+        raise Client::Error, "Sentry response status code: #{response.code}"
       end
 
       response.as_json

locale/gitlab.pot

@@ -3295,7 +3295,7 @@ msgstr ""
 msgid "Failed to load emoji list."
 msgstr ""
 
-msgid "Failed to load errors from Sentry"
+msgid "Failed to load errors from Sentry. Error message: %{errorMessage}"
 msgstr ""
 
 msgid "Failed to remove issue from board, please try again."

spec/lib/sentry/client_spec.rb

@@ -36,7 +36,7 @@ describe Sentry::Client do
     end
 
     it 'does not follow redirects' do
-      expect { subject }.to raise_exception(Sentry::Client::Error, 'Sentry response error: 302')
+      expect { subject }.to raise_exception(Sentry::Client::Error, 'Sentry response status code: 302')
       expect(redirect_req_stub).to have_been_requested
       expect(redirected_req_stub).not_to have_been_requested
     end

spec/models/error_tracking/project_error_tracking_setting_spec.rb

@@ -145,6 +145,24 @@ describe ErrorTracking::ProjectErrorTrackingSetting do
         expect(result).to be_nil
       end
     end
+
+    context 'when sentry client raises exception' do
+      let(:sentry_client) { spy(:sentry_client) }
+
+      before do
+        synchronous_reactive_cache(subject)
+
+        allow(subject).to receive(:sentry_client).and_return(sentry_client)
+        allow(sentry_client).to receive(:list_issues).with(opts)
+          .and_raise(Sentry::Client::Error, 'error message')
+      end
+
+      it 'returns error' do
+        expect(result).to eq(error: 'error message')
+        expect(subject).to have_received(:sentry_client)
+        expect(sentry_client).to have_received(:list_issues)
+      end
+    end
   end
 
   describe '#list_sentry_projects' do

spec/services/error_tracking/list_issues_service_spec.rb

@@ -45,7 +45,23 @@ describe ErrorTracking::ListIssuesService do
 
         it 'result is not ready' do
           expect(result).to eq(
-            status: :error, http_status: :no_content, message: 'not ready')
+            status: :error, http_status: :no_content, message: 'Not ready. Try again later')
+        end
+      end
+
+      context 'when list_sentry_issues returns error' do
+        before do
+          allow(error_tracking_setting)
+            .to receive(:list_sentry_issues)
+            .and_return(error: 'Sentry response status code: 401')
+        end
+
+        it 'returns the error' do
+          expect(result).to eq(
+            status: :error,
+            http_status: :bad_request,
+            message: 'Sentry response status code: 401'
+          )
         end
       end
     end
@@ -58,7 +74,11 @@ describe ErrorTracking::ListIssuesService do
       it 'returns error' do
         result = subject.execute
 
-        expect(result).to include(status: :error, message: 'access denied')
+        expect(result).to include(
+          status: :error,
+          message: 'Access denied',
+          http_status: :unauthorized
+        )
       end
     end
 
@@ -70,7 +90,7 @@ describe ErrorTracking::ListIssuesService do
       it 'raises error' do
         result = subject.execute
 
-        expect(result).to include(status: :error, message: 'not enabled')
+        expect(result).to include(status: :error, message: 'Error Tracking is not enabled')
       end
     end
   end

spec/services/error_tracking/list_projects_service_spec.rb

@@ -53,11 +53,11 @@ describe ErrorTracking::ListProjectsService do
       context 'sentry client raises exception' do
         before do
           expect(error_tracking_setting).to receive(:list_sentry_projects)
-            .and_raise(Sentry::Client::Error, 'Sentry response error: 500')
+            .and_raise(Sentry::Client::Error, 'Sentry response status code: 500')
         end
 
         it 'returns error response' do
-          expect(result[:message]).to eq('Sentry response error: 500')
+          expect(result[:message]).to eq('Sentry response status code: 500')
           expect(result[:http_status]).to eq(:bad_request)
         end
       end