Automatic merge of gitlab-org/gitlab-ce master

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

doc/administration/index.md

@@ -57,7 +57,7 @@ Learn how to install, configure, update, and maintain your GitLab instance.
 - [Third party offers](../user/admin_area/settings/third_party_offers.md)
 - [Compliance](compliance.md): A collection of features from across the application that you may configure to help ensure that your GitLab instance and DevOps workflow meet compliance standards.
 - [Diff limits](../user/admin_area/diff_limits.md): Configure the diff rendering size limits of branch comparison pages.
-- [Merge request diffs](merge_request_diffs.md): Configure the diffs shown on merge requests
+- [Merge request diffs storage](merge_request_diffs.md): Configure merge requests diffs external storage.
 - [Broadcast Messages](../user/admin_area/broadcast_messages.md): Send messages to GitLab users through the UI.
 - [Elasticsearch](../integration/elasticsearch.md): Enable Elasticsearch to empower GitLab's Advanced Global Search. Useful when you deal with a huge amount of data. **[STARTER ONLY]**
 - [External Classification Policy Authorization](../user/admin_area/settings/external_authorization.md) **[PREMIUM ONLY]**

doc/administration/merge_request_diffs.md

-# Merge request diffs administration
+# Merge request diffs storage **[CORE ONLY]**
 
-> **Notes:**
-> - External merge request diffs introduced in GitLab 11.8
+> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/52568) in GitLab 11.8.
 
 Merge request diffs are size-limited copies of diffs associated with merge
 requests. When viewing a merge request, diffs are sourced from these copies
@@ -16,9 +15,7 @@ large, in which case, switching to external storage is recommended.
 Merge request diffs can be stored on disk, or in object storage. In general, it
 is better to store the diffs in the database than on disk.
 
-To enable external storage of merge request diffs:
-
----
+To enable external storage of merge request diffs, follow the instructions below.
 
 **In Omnibus installations:**
 
@@ -30,16 +27,14 @@ To enable external storage of merge request diffs:
 
 1. _The external diffs will be stored in in
    `/var/opt/gitlab/gitlab-rails/shared/external-diffs`._ To change the path,
-   for example to `/mnt/storage/external-diffs`, edit `/etc/gitlab/gitlab.rb`
+   for example, to `/mnt/storage/external-diffs`, edit `/etc/gitlab/gitlab.rb`
    and add the following line:
 
     ```ruby
     gitlab_rails['external_diffs_storage_path'] = "/mnt/storage/external-diffs"
     ```
 
-1. Save the file and [reconfigure GitLab][] for the changes to take effect.
-
----
+1. Save the file and [reconfigure GitLab](restart_gitlab.md#omnibus-gitlab-reconfigure) for the changes to take effect.
 
 **In installations from source:**
 
@@ -52,7 +47,7 @@ To enable external storage of merge request diffs:
     ```
 
 1. _The external diffs will be stored in 
-   `/home/git/gitlab/shared/external-diffs`._ To change the path,  for example
+   `/home/git/gitlab/shared/external-diffs`._ To change the path, for example,
    to `/mnt/storage/external-diffs`, edit `/home/git/gitlab/config/gitlab.yml`
    and add or amend the following lines:
 
@@ -62,18 +57,18 @@ To enable external storage of merge request diffs:
       storage_path: /mnt/storage/external-diffs
     ```
 
-1. Save the file and [restart GitLab][] for the changes to take effect.
+1. Save the file and [restart GitLab](restart_gitlab.md#installations-from-source) for the changes to take effect.
 
 ### Using object storage
 
-Instead of storing the external diffs on disk, we recommended you use an object
+Instead of storing the external diffs on disk, we recommended the use of an object
 store like AWS S3 instead. This configuration relies on valid AWS credentials to
 be configured already.
 
 ### Object Storage Settings
 
 For source installations, these settings are nested under `external_diffs:` and
-then `object_store:`. On omnibus installs, they are prefixed by
+then `object_store:`. On Omnibus installations, they are prefixed by
 `external_diffs_object_store_`.
 
 | Setting | Description | Default |
@@ -118,7 +113,7 @@ The connection settings match those provided by [Fog](https://github.com/fog), a
     }
     ```
 
-    NOTE: if you are using AWS IAM profiles, be sure to omit the
+    Note that, if you are using AWS IAM profiles, be sure to omit the
     AWS access key and secret access key/value pairs. For example:
 
     ```ruby
@@ -129,9 +124,7 @@ The connection settings match those provided by [Fog](https://github.com/fog), a
     }
     ```
 
-1. Save the file and [reconfigure GitLab][] for the changes to take effect.
-
----
+1. Save the file and [reconfigure GitLab](restart_gitlab.md#omnibus-gitlab-reconfigure) for the changes to take effect.
 
 **In installations from source:**
 
@@ -151,4 +144,4 @@ The connection settings match those provided by [Fog](https://github.com/fog), a
           region: eu-central-1
     ```
 
-1. Save the file and [restart GitLab][] for the changes to take effect.
+1. Save the file and [restart GitLab](restart_gitlab.md#installations-from-source) for the changes to take effect.

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

@@ -3996,7 +3996,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