Merge branch '42700-filter-error-list-by-error-status-ignored-resolved-unresolved' into 'master'

Backend: Filter error list by error status

See merge request gitlab-org/gitlab!23186

app/controllers/projects/error_tracking_controller.rb

@@ -30,7 +30,7 @@ class Projects::ErrorTrackingController < Projects::ErrorTracking::BaseControlle
     service = ErrorTracking::IssueUpdateService.new(project, current_user, issue_update_params)
     result = service.execute
 
-    return if handle_errors(result)
+    return if render_errors(result)
 
     render json: {
       result: result
@@ -47,7 +47,7 @@ class Projects::ErrorTrackingController < Projects::ErrorTracking::BaseControlle
     )
     result = service.execute
 
-    return if handle_errors(result)
+    return if render_errors(result)
 
     render json: {
       errors: serialize_errors(result[:issues]),
@@ -60,14 +60,14 @@ class Projects::ErrorTrackingController < Projects::ErrorTracking::BaseControlle
     service = ErrorTracking::IssueDetailsService.new(project, current_user, issue_details_params)
     result = service.execute
 
-    return if handle_errors(result)
+    return if render_errors(result)
 
     render json: {
       error: serialize_detailed_error(result[:issue])
     }
   end
 
-  def handle_errors(result)
+  def render_errors(result)
     unless result[:status] == :success
       render json: { message: result[:message] },
              status: result[:http_status] || :bad_request
@@ -75,7 +75,7 @@ class Projects::ErrorTrackingController < Projects::ErrorTracking::BaseControlle
   end
 
   def list_issues_params
-    params.permit(:search_term, :sort, :cursor)
+    params.permit(:search_term, :sort, :cursor, :issue_status)
   end
 
   def issue_update_params

app/models/error_tracking/project_error_tracking_setting.rb

@@ -89,8 +89,10 @@ module ErrorTracking
     end
 
     def list_sentry_projects
+      handle_exceptions do
         { projects: sentry_client.projects }
       end
+    end
 
     def issue_details(opts = {})
       with_reactive_cache('issue_details', opts.stringify_keys) do |result|

app/services/error_tracking/base_service.rb

@@ -3,21 +3,9 @@
 module ErrorTracking
   class BaseService < ::BaseService
     def execute
-      unauthorized = check_permissions
       return unauthorized if unauthorized
 
-      begin
-        response = perform
-      rescue Sentry::Client::Error => e
-        return error(e.message, :bad_request)
-      rescue Sentry::Client::MissingKeysError => e
-        return error(e.message, :internal_server_error)
-      end
-
-      errors = parse_errors(response)
-      return errors if errors
-
-      success(parse_response(response))
+      perform
     end
 
     private
@@ -27,12 +15,21 @@ module ErrorTracking
           "#{self.class} does not implement #{__method__}"
     end
 
+    def compose_response(response, &block)
+      errors = parse_errors(response)
+      return errors if errors
+
+      yield if block_given?
+
+      success(parse_response(response))
+    end
+
     def parse_response(response)
       raise NotImplementedError,
           "#{self.class} does not implement #{__method__}"
     end
 
-    def check_permissions
+    def unauthorized
       return error('Error Tracking is not enabled') unless enabled?
       return error('Access denied', :unauthorized) unless can_read?
     end

app/services/error_tracking/issue_details_service.rb

@@ -5,7 +5,9 @@ module ErrorTracking
     private
 
     def perform
-      project_error_tracking_setting.issue_details(issue_id: params[:issue_id])
+      response = project_error_tracking_setting.issue_details(issue_id: params[:issue_id])
+
+      compose_response(response)
     end
 
     def parse_response(response)

app/services/error_tracking/issue_latest_event_service.rb

@@ -5,7 +5,9 @@ module ErrorTracking
     private
 
     def perform
-      project_error_tracking_setting.issue_latest_event(issue_id: params[:issue_id])
+      response = project_error_tracking_setting.issue_latest_event(issue_id: params[:issue_id])
+
+      compose_response(response)
     end
 
     def parse_response(response)

app/services/error_tracking/issue_update_service.rb

@@ -7,21 +7,15 @@ module ErrorTracking
     private
 
     def perform
-      response = fetch
+      response = project_error_tracking_setting.update_issue(
+        issue_id: params[:issue_id],
+        params: update_params
+      )
 
-      unless parse_errors(response).present?
+      compose_response(response) do
         response[:closed_issue_iid] = update_related_issue&.iid
         project_error_tracking_setting.expire_issues_cache
       end
-
-      response
-    end
-
-    def fetch
-      project_error_tracking_setting.update_issue(
-        issue_id: params[:issue_id],
-        params: update_params
-      )
     end
 
     def update_related_issue
@@ -74,7 +68,7 @@ module ErrorTracking
       }
     end
 
-    def check_permissions
+    def unauthorized
       return error('Error Tracking is not enabled') unless enabled?
       return error('Access denied', :unauthorized) unless can_update?
     end

app/services/error_tracking/list_issues_service.rb

@@ -6,6 +6,13 @@ module ErrorTracking
     DEFAULT_LIMIT = 20
     DEFAULT_SORT = 'last_seen'
 
+    # Sentry client supports 'muted' and 'assigned' but GitLab does not
+    ISSUE_STATUS_VALUES = %w[
+      resolved
+      unresolved
+      ignored
+    ].freeze
+
     def external_url
       project_error_tracking_setting&.sentry_external_url
     end
@@ -13,19 +20,31 @@ module ErrorTracking
     private
 
     def perform
-      project_error_tracking_setting.list_sentry_issues(
+      return invalid_status_error unless valid_status?
+
+      response = project_error_tracking_setting.list_sentry_issues(
         issue_status: issue_status,
         limit: limit,
         search_term: params[:search_term].presence,
         sort: sort,
         cursor: params[:cursor].presence
       )
+
+      compose_response(response)
     end
 
     def parse_response(response)
       response.slice(:issues, :pagination)
     end
 
+    def invalid_status_error
+      error('Bad Request: Invalid issue_status', http_status_for(:bad_Request))
+    end
+
+    def valid_status?
+      ISSUE_STATUS_VALUES.include?(issue_status)
+    end
+
     def issue_status
       params[:issue_status] || DEFAULT_ISSUE_STATUS
     end

app/services/error_tracking/list_projects_service.rb

@@ -2,18 +2,16 @@
 
 module ErrorTracking
   class ListProjectsService < ErrorTracking::BaseService
-    def execute
+    private
+
+    def perform
       unless project_error_tracking_setting.valid?
         return error(project_error_tracking_setting.errors.full_messages.join(', '), :bad_request)
       end
 
-      super
-    end
+      response = project_error_tracking_setting.list_sentry_projects
 
-    private
-
-    def perform
-      project_error_tracking_setting.list_sentry_projects
+      compose_response(response)
     end
 
     def parse_response(response)

spec/services/error_tracking/base_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ErrorTracking::BaseService do
+  describe '#compose_response' do
+    let(:project) { double('project') }
+    let(:user) { double('user') }
+    let(:service) { described_class.new(project, user) }
+
+    it 'returns bad_request error when response has an error key' do
+      data = { error: 'Unexpected Error' }
+
+      result = service.send(:compose_response, data)
+
+      expect(result[:status]).to be(:error)
+      expect(result[:message]).to be('Unexpected Error')
+      expect(result[:http_status]).to be(:bad_request)
+    end
+
+    it 'returns server error when response has missing key error_type' do
+      data = { error: 'Unexpected Error', error_type: ErrorTracking::ProjectErrorTrackingSetting::SENTRY_API_ERROR_TYPE_MISSING_KEYS }
+
+      result = service.send(:compose_response, data)
+
+      expect(result[:status]).to be(:error)
+      expect(result[:message]).to be('Unexpected Error')
+      expect(result[:http_status]).to be(:internal_server_error)
+    end
+
+    it 'returns no content when response is nil' do
+      data = nil
+
+      result = service.send(:compose_response, data)
+
+      expect(result[:status]).to be(:error)
+      expect(result[:message]).to be('Not ready. Try again later')
+      expect(result[:http_status]).to be(:no_content)
+    end
+
+    context 'when result has no errors key' do
+      let(:data) { { thing: :cat } }
+
+      it 'raises NotImplementedError' do
+        expect { service.send(:compose_response, data) }
+          .to raise_error(NotImplementedError)
+      end
+
+      context 'when parse_response is implemented' do
+        before do
+          expect(service).to receive(:parse_response) do |response|
+            { animal: response[:thing] }
+          end
+        end
+
+        it 'returns successful response' do
+          result = service.send(:compose_response, data)
+
+          expect(result[:animal]).to eq(:cat)
+          expect(result[:status]).to eq(:success)
+        end
+
+        it 'returns successful response with changes from passed block' do
+          result = service.send(:compose_response, data) do
+            data[:thing] = :fish
+          end
+
+          expect(result[:animal]).to eq(:fish)
+          expect(result[:status]).to eq(:success)
+        end
+      end
+    end
+  end
+end

spec/services/error_tracking/list_issues_service_spec.rb

@@ -20,19 +20,29 @@ describe ErrorTracking::ListIssuesService do
 
   describe '#execute' do
     context 'with authorized user' do
-      context 'when list_sentry_issues returns issues' do
-        let(:issues) { [:list, :of, :issues] }
+      let(:issues) { [] }
 
-        before do
-          expect(error_tracking_setting)
-            .to receive(:list_sentry_issues)
-            .with(list_sentry_issues_args)
-            .and_return(issues: issues, pagination: {})
+      described_class::ISSUE_STATUS_VALUES.each do |status|
+        it "returns the issues with #{status} issue_status" do
+          params[:issue_status] = status
+          list_sentry_issues_args[:issue_status] = status
+          expect_list_sentry_issues_with(list_sentry_issues_args)
+
+          expect(result).to eq(status: :success, pagination: {}, issues: issues)
+        end
       end
 
-        it 'returns the issues' do
+      it 'returns the issues with no issue_status' do
+        expect_list_sentry_issues_with(list_sentry_issues_args)
+
         expect(result).to eq(status: :success, pagination: {}, issues: issues)
       end
+
+      it 'returns bad request for an issue_status not on the whitelist' do
+        params[:issue_status] = 'assigned'
+
+        expect(error_tracking_setting).not_to receive(:list_sentry_issues)
+        expect(result).to eq(message: "Bad Request: Invalid issue_status", status: :error, http_status: :bad_request)
       end
 
       include_examples 'error tracking service data not ready', :list_sentry_issues
@@ -52,3 +62,10 @@ describe ErrorTracking::ListIssuesService do
     end
   end
 end
+
+def expect_list_sentry_issues_with(list_sentry_issues_args)
+  expect(error_tracking_setting)
+    .to receive(:list_sentry_issues)
+    .with(list_sentry_issues_args)
+    .and_return(issues: [], pagination: {})
+end

spec/services/error_tracking/list_projects_service_spec.rb

@@ -63,32 +63,6 @@ describe ErrorTracking::ListProjectsService do
         end
       end
 
-      context 'sentry client raises exception' do
-        context 'Sentry::Client::Error' do
-          before do
-            expect(error_tracking_setting).to receive(:list_sentry_projects)
-              .and_raise(Sentry::Client::Error, 'Sentry response status code: 500')
-          end
-
-          it 'returns error response' do
-            expect(result[:message]).to eq('Sentry response status code: 500')
-            expect(result[:http_status]).to eq(:bad_request)
-          end
-        end
-
-        context 'Sentry::Client::MissingKeysError' do
-          before do
-            expect(error_tracking_setting).to receive(:list_sentry_projects)
-              .and_raise(Sentry::Client::MissingKeysError, 'Sentry API response is missing keys. key not found: "id"')
-          end
-
-          it 'returns error response' do
-            expect(result[:message]).to eq('Sentry API response is missing keys. key not found: "id"')
-            expect(result[:http_status]).to eq(:internal_server_error)
-          end
-        end
-      end
-
       context 'with invalid url' do
         let(:params) do
           ActionController::Parameters.new(