Merge branch 'unify-return-type-of-alerting-notify-services' into 'master'

Return ServiceResponse from Alert::NotifyService

See merge request gitlab-org/gitlab!28663

app/controllers/projects/alerting/notifications_controller.rb

@@ -14,7 +14,7 @@ module Projects
         token = extract_alert_manager_token(request)
         result = notify_service.execute(token)
 
-        head(response_status(result))
+        head result.http_status
       end
 
       private
@@ -33,12 +33,6 @@ module Projects
           .new(project, current_user, notification_payload)
       end
 
-      def response_status(result)
-        return :ok if result.success?
-
-        result.http_status
-      end
-
       def notification_payload
         params.permit![:notification]
       end

app/controllers/projects/prometheus/alerts_controller.rb

@@ -26,12 +26,9 @@ module Projects
 
       def notify
         token = extract_alert_manager_token(request)
+        result = notify_service.execute(token)
 
-        if notify_service.execute(token)
-          head :ok
-        else
-          head :unprocessable_entity
-        end
+        head result.http_status
       end
 
       def create

app/services/projects/alerting/notify_service.rb

@@ -46,15 +46,15 @@ module Projects
       end
 
       def bad_request
-        ServiceResponse.error(message: 'Bad Request', http_status: 400)
+        ServiceResponse.error(message: 'Bad Request', http_status: :bad_request)
       end
 
       def unauthorized
-        ServiceResponse.error(message: 'Unauthorized', http_status: 401)
+        ServiceResponse.error(message: 'Unauthorized', http_status: :unauthorized)
       end
 
       def forbidden
-        ServiceResponse.error(message: 'Forbidden', http_status: 403)
+        ServiceResponse.error(message: 'Forbidden', http_status: :forbidden)
       end
     end
   end

app/services/projects/prometheus/alerts/notify_service.rb

@@ -8,15 +8,15 @@ module Projects
         include IncidentManagement::Settings
 
         def execute(token)
-          return false unless valid_payload_size?
-          return false unless valid_version?
-          return false unless valid_alert_manager_token?(token)
+          return bad_request unless valid_payload_size?
+          return unprocessable_entity unless valid_version?
+          return unauthorized unless valid_alert_manager_token?(token)
 
           persist_events
           send_alert_email if send_email?
           process_incident_issues if process_issues?
 
-          true
+          ServiceResponse.success
         end
 
         private
@@ -118,6 +118,18 @@ module Projects
         def persist_events
           CreateEventsService.new(project, nil, params).execute
         end
+
+        def bad_request
+          ServiceResponse.error(message: 'Bad Request', http_status: :bad_request)
+        end
+
+        def unauthorized
+          ServiceResponse.error(message: 'Unauthorized', http_status: :unauthorized)
+        end
+
+        def unprocessable_entity
+          ServiceResponse.error(message: 'Unprocessable Entity', http_status: :unprocessable_entity)
+        end
       end
     end
   end

ee/spec/requests/rack_attack_spec.rb

@@ -87,7 +87,7 @@ describe 'Rack Attack EE throttles' do
   describe 'requests to prometheus alert notify endpoint with oauth token' do
     before do
       allow_next_instance_of(Projects::Prometheus::Alerts::NotifyService) do |instance|
-        allow(instance).to receive(:execute).and_return true
+        allow(instance).to receive(:execute).and_return(ServiceResponse.success)
       end
     end
 
@@ -99,7 +99,7 @@ describe 'Rack Attack EE throttles' do
   describe 'requests to generic alert notify endpoint with oauth token' do
     before do
       allow_next_instance_of(Projects::Alerting::NotifyService) do |instance|
-        allow(instance).to receive(:execute).and_return double(success?: true)
+        allow(instance).to receive(:execute).and_return(ServiceResponse.success)
       end
     end
 

ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb

@@ -30,7 +30,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
       expect(notification_service)
         .to receive_message_chain(:async, :prometheus_alerts_fired)
 
-      expect(subject).to eq(true)
+      expect(subject).to be_success
     end
   end
 
@@ -44,7 +44,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
         .exactly(amount).times
 
       Sidekiq::Testing.inline! do
-        expect(subject).to eq(true)
+        expect(subject).to be_success
       end
     end
   end
@@ -54,7 +54,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
       expect(IncidentManagement::ProcessPrometheusAlertWorker)
         .not_to receive(:perform_async)
 
-      expect(subject).to eq(true)
+      expect(subject).to be_success
     end
   end
 
@@ -69,7 +69,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
       expect(create_events_service)
         .to receive(:execute)
 
-      expect(subject).to eq(true)
+      expect(subject).to be_success
     end
   end
 
@@ -78,7 +78,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
     it_behaves_like 'persists events'
   end
 
-  shared_examples 'no notifications' do
+  shared_examples 'no notifications' do |http_status:|
     let(:notification_service) { spy }
     let(:create_events_service) { spy }
 
@@ -86,7 +86,8 @@ describe Projects::Prometheus::Alerts::NotifyService do
       expect(notification_service).not_to receive(:async)
       expect(create_events_service).not_to receive(:execute)
 
-      expect(subject).to eq(false)
+      expect(subject).to be_error
+      expect(subject.http_status).to eq(http_status)
     end
   end
 
@@ -130,7 +131,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
       end
 
       context 'with token' do
-        it_behaves_like 'no notifications'
+        it_behaves_like 'no notifications', http_status: :unauthorized
       end
     end
   end

spec/controllers/projects/alerting/notifications_controller_spec.rb

@@ -48,7 +48,7 @@ describe Projects::Alerting::NotificationsController do
     end
 
     context 'when notification service fails' do
-      let(:service_response) { ServiceResponse.error(message: 'Unauthorized', http_status: 401) }
+      let(:service_response) { ServiceResponse.error(message: 'Unauthorized', http_status: :unauthorized) }
 
       it 'responds with the service response' do
         make_request

spec/controllers/projects/prometheus/alerts_controller_spec.rb

@@ -158,7 +158,8 @@ describe Projects::Prometheus::AlertsController do
   end
 
   describe 'POST #notify' do
-    let(:notify_service) { spy }
+    let(:service_response) { ServiceResponse.success }
+    let(:notify_service) { instance_double(Projects::Prometheus::Alerts::NotifyService, execute: service_response) }
 
     before do
       sign_out(user)
@@ -170,7 +171,7 @@ describe Projects::Prometheus::AlertsController do
     end
 
     it 'returns ok if notification succeeds' do
-      expect(notify_service).to receive(:execute).and_return(true)
+      expect(notify_service).to receive(:execute).and_return(ServiceResponse.success)
 
       post :notify, params: project_params, session: { as: :json }
 
@@ -178,7 +179,9 @@ describe Projects::Prometheus::AlertsController do
     end
 
     it 'returns unprocessable entity if notification fails' do
-      expect(notify_service).to receive(:execute).and_return(false)
+      expect(notify_service).to receive(:execute).and_return(
+        ServiceResponse.error(message: 'Unprocessable Entity', http_status: :unprocessable_entity)
+      )
 
       post :notify, params: project_params, session: { as: :json }
 

spec/services/projects/alerting/notify_service_spec.rb

@@ -20,7 +20,7 @@ describe Projects::Alerting::NotifyService do
         .exactly(amount).times
 
       Sidekiq::Testing.inline! do
-        expect(subject.status).to eq(:success)
+        expect(subject).to be_success
       end
     end
   end
@@ -36,7 +36,7 @@ describe Projects::Alerting::NotifyService do
       expect(notification_service)
         .to receive_message_chain(:async, :prometheus_alerts_fired)
 
-      expect(subject.status).to eq(:success)
+      expect(subject).to be_success
     end
   end
 
@@ -45,7 +45,7 @@ describe Projects::Alerting::NotifyService do
       expect(IncidentManagement::ProcessAlertWorker)
         .not_to receive(:perform_async)
 
-      expect(subject.status).to eq(:success)
+      expect(subject).to be_success
     end
   end
 
@@ -54,7 +54,7 @@ describe Projects::Alerting::NotifyService do
       expect(IncidentManagement::ProcessAlertWorker)
         .not_to receive(:perform_async)
 
-      expect(subject.status).to eq(:error)
+      expect(subject).to be_error
       expect(subject.http_status).to eq(http_status)
     end
   end
@@ -102,7 +102,7 @@ describe Projects::Alerting::NotifyService do
                 .and_raise(Gitlab::Alerting::NotificationPayloadParser::BadPayloadError)
             end
 
-            it_behaves_like 'does not process incident issues due to error', http_status: 400
+            it_behaves_like 'does not process incident issues due to error', http_status: :bad_request
           end
         end
 
@@ -114,13 +114,13 @@ describe Projects::Alerting::NotifyService do
       end
 
       context 'with invalid token' do
-        it_behaves_like 'does not process incident issues due to error', http_status: 401
+        it_behaves_like 'does not process incident issues due to error', http_status: :unauthorized
       end
 
       context 'with deactivated Alerts Service' do
         let!(:alerts_service) { create(:alerts_service, :inactive, project: project) }
 
-        it_behaves_like 'does not process incident issues due to error', http_status: 403
+        it_behaves_like 'does not process incident issues due to error', http_status: :forbidden
       end
     end
   end

spec/services/projects/prometheus/alerts/notify_service_spec.rb

@@ -30,7 +30,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
       expect(notification_service)
         .to receive_message_chain(:async, :prometheus_alerts_fired)
 
-      expect(subject).to eq(true)
+      expect(subject).to be_success
     end
   end
 
@@ -44,7 +44,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
         .exactly(amount).times
 
       Sidekiq::Testing.inline! do
-        expect(subject).to eq(true)
+        expect(subject).to be_success
       end
     end
   end
@@ -54,7 +54,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
       expect(IncidentManagement::ProcessPrometheusAlertWorker)
         .not_to receive(:perform_async)
 
-      expect(subject).to eq(true)
+      expect(subject).to be_success
     end
   end
 
@@ -69,7 +69,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
       expect(create_events_service)
         .to receive(:execute)
 
-      expect(subject).to eq(true)
+      expect(subject).to be_success
     end
   end
 
@@ -78,7 +78,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
     it_behaves_like 'persists events'
   end
 
-  shared_examples 'no notifications' do
+  shared_examples 'no notifications' do |http_status:|
     let(:notification_service) { spy }
     let(:create_events_service) { spy }
 
@@ -86,7 +86,8 @@ describe Projects::Prometheus::Alerts::NotifyService do
       expect(notification_service).not_to receive(:async)
       expect(create_events_service).not_to receive(:execute)
 
-      expect(subject).to eq(false)
+      expect(subject).to be_error
+      expect(subject.http_status).to eq(http_status)
     end
   end
 
@@ -130,7 +131,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
         when :success
           it_behaves_like 'notifies alerts'
         when :failure
-          it_behaves_like 'no notifications'
+          it_behaves_like 'no notifications', http_status: :unauthorized
         else
           raise "invalid result: #{result.inspect}"
         end
@@ -140,7 +141,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
     context 'without project specific cluster' do
       let!(:cluster) { create(:cluster, enabled: true) }
 
-      it_behaves_like 'no notifications'
+      it_behaves_like 'no notifications', http_status: :unauthorized
     end
 
     context 'with manual prometheus installation' do
@@ -171,7 +172,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
         when :success
           it_behaves_like 'notifies alerts'
         when :failure
-          it_behaves_like 'no notifications'
+          it_behaves_like 'no notifications', http_status: :unauthorized
         else
           raise "invalid result: #{result.inspect}"
         end
@@ -193,7 +194,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
           expect_any_instance_of(NotificationService)
             .not_to receive(:async)
 
-          expect(subject).to eq(true)
+          expect(subject).to be_success
         end
       end
 
@@ -211,7 +212,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
         it 'does not send notification' do
           expect(NotificationService).not_to receive(:new)
 
-          expect(subject).to eq(true)
+          expect(subject).to be_success
         end
       end
     end
@@ -260,19 +261,19 @@ describe Projects::Prometheus::Alerts::NotifyService do
     context 'without version' do
       let(:payload) { {} }
 
-      it_behaves_like 'no notifications'
+      it_behaves_like 'no notifications', http_status: :unprocessable_entity
     end
 
     context 'when version is not "4"' do
       let(:payload) { { 'version' => '5' } }
 
-      it_behaves_like 'no notifications'
+      it_behaves_like 'no notifications', http_status: :unprocessable_entity
     end
 
     context 'with missing alerts' do
       let(:payload) { { 'version' => '4' } }
 
-      it_behaves_like 'no notifications'
+      it_behaves_like 'no notifications', http_status: :unauthorized
     end
 
     context 'when the payload is too big' do
@@ -283,7 +284,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
         allow(Gitlab::Utils::DeepSize).to receive(:new).and_return(deep_size_object)
       end
 
-      it_behaves_like 'no notifications'
+      it_behaves_like 'no notifications', http_status: :bad_request
 
       it 'does not process issues' do
         expect(IncidentManagement::ProcessPrometheusAlertWorker)