Merge branch 'sarnold-move-alerts-metrics-api-to-ce' into 'master'

Move Alerts metric image API to CE

See merge request gitlab-org/gitlab!84918

app/controllers/uploads_controller.rb

@@ -14,6 +14,7 @@ class UploadsController < ApplicationController
     "appearance"       => Appearance,
     "personal_snippet" => PersonalSnippet,
     "projects/topic"   => Projects::Topic,
+    'alert_management_metric_image' => ::AlertManagement::MetricImage,
     nil => PersonalSnippet
   }.freeze
 
@@ -56,6 +57,8 @@ class UploadsController < ApplicationController
       true
     when Projects::Topic
       true
+    when ::AlertManagement::MetricImage
+      can?(current_user, :read_alert_management_metric_image, model.alert)
     else
       can?(current_user, "read_#{model.class.underscore}".to_sym, model)
     end

app/models/alert_management/alert.rb

@@ -27,6 +27,7 @@ module AlertManagement
     has_many :notes, as: :noteable, inverse_of: :noteable, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
     has_many :ordered_notes, -> { fresh }, as: :noteable, class_name: 'Note'
     has_many :user_mentions, class_name: 'AlertManagement::AlertUserMention', foreign_key: :alert_management_alert_id
+    has_many :metric_images, class_name: '::AlertManagement::MetricImage'
 
     has_internal_id :iid, scope: :project
 
@@ -142,6 +143,10 @@ module AlertManagement
       reference.to_i > 0 && reference.to_i <= Gitlab::Database::MAX_INT_VALUE
     end
 
+    def metric_images_available?
+      ::AlertManagement::MetricImage.available_for?(project)
+    end
+
     def prometheus?
       monitoring_tool == Gitlab::AlertManagement::Payload::MONITORING_TOOLS[:prometheus]
     end

ee/app/models/alert_management/metric_image.rb → app/models/alert_management/metric_image.rb

@@ -8,7 +8,7 @@ module AlertManagement
     belongs_to :alert, class_name: 'AlertManagement::Alert', foreign_key: 'alert_id', inverse_of: :metric_images
 
     def self.available_for?(project)
-      project&.feature_available?(:alert_metric_upload)
+      true
     end
 
     private

app/policies/alert_management/alert_policy.rb

@@ -3,7 +3,15 @@
 module AlertManagement
   class AlertPolicy < ::BasePolicy
     delegate { @subject.project }
+
+    rule { can?(:read_alert_management_alert) }.policy do
+      enable :read_alert_management_metric_image
     end
-end
 
-AlertManagement::AlertPolicy.prepend_mod
+    rule { can?(:update_alert_management_alert) }.policy do
+      enable :upload_alert_management_metric_image
+      enable :update_alert_management_metric_image
+      enable :destroy_alert_management_metric_image
+    end
+  end
+end

ee/app/services/alert_management/metric_images/upload_service.rb → app/services/alert_management/metric_images/upload_service.rb

@@ -15,7 +15,12 @@ module AlertManagement
       end
 
       def execute
-        return ServiceResponse.error(message: _("You are not authorized to upload metric images"), http_status: :forbidden) unless can_upload_metrics?
+        unless can_upload_metrics?
+          return ServiceResponse.error(
+            message: _("You are not authorized to upload metric images"),
+            http_status: :forbidden
+          )
+        end
 
         metric = AlertManagement::MetricImage.new(
           alert: alert,

config/routes/uploads.rb

@@ -38,6 +38,12 @@ scope path: :uploads do
   post ':model/authorize',
     to: 'uploads#authorize',
     constraints: { model: /personal_snippet|user/ }
+
+  # Alert Metric Images
+  get "-/system/:model/:mounted_as/:id/:filename",
+      to:           "uploads#show",
+      constraints:  { model: /alert_management_metric_image/, mounted_as: /file/, filename: %r{[^/]+} },
+      as: 'alert_metric_image_upload'
 end
 
 # Redirect old note attachments path to new uploads path.

ee/app/controllers/ee/uploads_controller.rb

@@ -6,8 +6,7 @@ module EE
     extend ::Gitlab::Utils::Override
 
     EE_MODEL_CLASSES = {
-      'issuable_metric_image' => IssuableMetricImage,
-      'alert_management_metric_image' => ::AlertManagement::MetricImage
+      'issuable_metric_image' => IssuableMetricImage
     }.freeze
 
     class_methods do
@@ -24,8 +23,6 @@ module EE
       case model
       when IssuableMetricImage
         can?(current_user, :read_issuable_metric_image, model)
-      when ::AlertManagement::MetricImage
-        can?(current_user, :read_alert_management_metric_image, model.alert)
       else
         super
       end

ee/app/models/ee/alert_management/alert.rb

@@ -9,7 +9,6 @@ module EE
         include AfterCommitQueue
 
         has_many :pending_escalations, class_name: 'IncidentManagement::PendingEscalations::Alert', foreign_key: :alert_id, inverse_of: :alert
-        has_many :metric_images, class_name: '::AlertManagement::MetricImage'
 
         after_create do |alert|
           run_after_commit { alert.trigger_auto_rollback }
@@ -21,10 +20,6 @@ module EE
 
         ::Deployments::AutoRollbackWorker.perform_async(environment.id)
       end
-
-      def metric_images_available?
-        ::AlertManagement::MetricImage.available_for?(project)
-      end
     end
   end
 end

ee/app/models/gitlab_subscriptions/features.rb

@@ -72,7 +72,6 @@ module GitlabSubscriptions
     PREMIUM_FEATURES = %i[
       adjourned_deletion_for_projects_and_groups
       admin_audit_log
-      alert_metric_upload
       auditor_user
       blocking_merge_requests
       board_assignee_lists

ee/app/policies/ee/alert_management/alert_policy.rb

-# frozen_string_literal: true
-
-module EE
-  module AlertManagement
-    module AlertPolicy
-      extend ActiveSupport::Concern
-
-      prepended do
-        rule { can?(:read_alert_management_alert) }.policy do
-          enable :read_alert_management_metric_image
-        end
-
-        rule { can?(:update_alert_management_alert) }.policy do
-          enable :upload_alert_management_metric_image
-          enable :update_alert_management_metric_image
-          enable :destroy_alert_management_metric_image
-        end
-      end
-    end
-  end
-end

ee/config/routes/uploads.rb

@@ -6,10 +6,4 @@ scope path: :uploads do
       to:           "uploads#show",
       constraints:  { model: /issuable_metric_image/, mounted_as: /file/, filename: %r{[^/]+} },
       as: 'issuable_metric_image_upload'
-
-  # Alert Metric Images
-  get "-/system/:model/:mounted_as/:id/:filename",
-      to:           "uploads#show",
-      constraints:  { model: /alert_management_metric_image/, mounted_as: /file/, filename: %r{[^/]+} },
-      as: 'alert_metric_image_upload'
 end

ee/lib/ee/api/api.rb

@@ -10,7 +10,6 @@ module EE
 
         mount ::EE::API::GroupBoards
 
-        mount ::API::AlertManagementAlerts
         mount ::API::AuditEvents
         mount ::API::ProjectApprovalRules
         mount ::API::StatusChecks

ee/spec/controllers/ee/uploads_controller_spec.rb

@@ -22,21 +22,5 @@ RSpec.describe UploadsController do
         expect(response).to have_gitlab_http_status(:ok)
       end
     end
-
-    context 'when viewing alert metric images' do
-      let(:alert) { create(:alert_management_alert, project: project) }
-      let(:metric_image) { create(:alert_metric_image, alert: alert) }
-
-      before do
-        project.add_developer(user)
-        sign_in(user)
-      end
-
-      it "responds with status 200" do
-        get :show, params: { model: "alert_management_metric_image", mounted_as: 'file', id: metric_image.id, filename: metric_image.filename }
-
-        expect(response).to have_gitlab_http_status(:ok)
-      end
-    end
   end
 end

ee/spec/policies/alert_management/alert_policy_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe AlertManagement::AlertPolicy do
-  describe '#rules' do
-    let_it_be(:user) { create(:user) }
-    let_it_be(:project) { create(:project) }
-    let_it_be(:alert) { create(:alert_management_alert, project: project, issue: incident) }
-    let_it_be(:incident) { nil }
-
-    let(:policy) { described_class.new(user, alert) }
-
-    describe 'rules' do
-      shared_examples 'does not allow metric image reads' do
-        it { expect(policy).to be_disallowed(:read_alert_management_metric_image) }
-      end
-
-      shared_examples 'does not allow metric image updates' do
-        specify do
-          expect(policy).to be_disallowed(:upload_alert_management_metric_image)
-          expect(policy).to be_disallowed(:destroy_alert_management_metric_image)
-        end
-      end
-
-      shared_examples 'allows metric image reads' do
-        it { expect(policy).to be_allowed(:read_alert_management_metric_image) }
-      end
-
-      shared_examples 'allows metric image updates' do
-        specify do
-          expect(policy).to be_allowed(:upload_alert_management_metric_image)
-          expect(policy).to be_allowed(:destroy_alert_management_metric_image)
-        end
-      end
-
-      context 'when user is not a member' do
-        include_examples 'does not allow metric image reads'
-        include_examples 'does not allow metric image updates'
-      end
-
-      context 'when user is a guest' do
-        before do
-          project.add_guest(user)
-        end
-
-        include_examples 'does not allow metric image reads'
-        include_examples 'does not allow metric image updates'
-      end
-
-      context 'when user is a developer' do
-        before do
-          project.add_developer(user)
-        end
-
-        include_examples 'allows metric image reads'
-        include_examples 'allows metric image updates'
-      end
-    end
-  end
-end

ee/lib/api/alert_management_alerts.rb → lib/api/alert_management_alerts.rb

@@ -50,7 +50,10 @@ module API
           ).execute
 
           if upload.success?
-            present upload.payload[:metric], with: Entities::MetricImage, current_user: current_user, project: user_project
+            present upload.payload[:metric],
+              with: Entities::MetricImage,
+              current_user: current_user,
+              project: user_project
           else
             render_api_error!(upload.message, upload.http_status)
           end

lib/api/api.rb

@@ -163,6 +163,7 @@ module API
       mount ::API::Admin::InstanceClusters
       mount ::API::Admin::PlanLimits
       mount ::API::Admin::Sidekiq
+      mount ::API::AlertManagementAlerts
       mount ::API::Appearance
       mount ::API::Applications
       mount ::API::Avatar

spec/controllers/uploads_controller_spec.rb

@@ -701,6 +701,24 @@ RSpec.describe UploadsController do
         end
       end
     end
+
+    context 'when viewing alert metric images' do
+      let!(:user) { create(:user) }
+      let!(:project) { create(:project) }
+      let(:alert) { create(:alert_management_alert, project: project) }
+      let(:metric_image) { create(:alert_metric_image, alert: alert) }
+
+      before do
+        project.add_developer(user)
+        sign_in(user)
+      end
+
+      it "responds with status 200" do
+        get :show, params: { model: "alert_management_metric_image", mounted_as: 'file', id: metric_image.id, filename: metric_image.filename }
+
+        expect(response).to have_gitlab_http_status(:ok)
+      end
+    end
   end
 
   def post_authorize(verified: true)

ee/spec/models/alert_management/metric_image_spec.rb → spec/models/alert_management/metric_image_spec.rb

@@ -19,22 +19,8 @@ RSpec.describe AlertManagement::MetricImage do
   describe '.available_for?' do
     subject { described_class.available_for?(issue.project) }
 
-    before do
-      stub_licensed_features(alert_metric_upload: true)
-    end
-
     let_it_be_with_refind(:issue) { create(:issue) }
 
-    context 'license enabled' do
     it { is_expected.to eq(true) }
   end
-
-    context 'license disabled' do
-      before do
-        stub_licensed_features(alert_metric_upload: false)
-      end
-
-      it { is_expected.to eq(false) }
-    end
-  end
 end

spec/policies/alert_management/alert_policy_spec.rb

@@ -3,9 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe AlertManagement::AlertPolicy, :models do
-  let(:alert) { create(:alert_management_alert) }
-  let(:project) { alert.project }
-  let(:user) { create(:user) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:alert) { create(:alert_management_alert, project: project, issue: incident) }
+  let_it_be(:incident) { nil }
 
   subject(:policy) { described_class.new(user, alert) }
 
@@ -21,5 +22,50 @@ RSpec.describe AlertManagement::AlertPolicy, :models do
       it { is_expected.to be_allowed :read_alert_management_alert }
       it { is_expected.to be_allowed :update_alert_management_alert }
     end
+
+    shared_examples 'does not allow metric image reads' do
+      it { expect(policy).to be_disallowed(:read_alert_management_metric_image) }
+    end
+
+    shared_examples 'does not allow metric image updates' do
+      specify do
+        expect(policy).to be_disallowed(:upload_alert_management_metric_image)
+        expect(policy).to be_disallowed(:destroy_alert_management_metric_image)
+      end
+    end
+
+    shared_examples 'allows metric image reads' do
+      it { expect(policy).to be_allowed(:read_alert_management_metric_image) }
+    end
+
+    shared_examples 'allows metric image updates' do
+      specify do
+        expect(policy).to be_allowed(:upload_alert_management_metric_image)
+        expect(policy).to be_allowed(:destroy_alert_management_metric_image)
+      end
+    end
+
+    context 'when user is not a member' do
+      include_examples 'does not allow metric image reads'
+      include_examples 'does not allow metric image updates'
+    end
+
+    context 'when user is a guest' do
+      before do
+        project.add_guest(user)
+      end
+
+      include_examples 'does not allow metric image reads'
+      include_examples 'does not allow metric image updates'
+    end
+
+    context 'when user is a developer' do
+      before do
+        project.add_developer(user)
+      end
+
+      include_examples 'allows metric image reads'
+      include_examples 'allows metric image updates'
+    end
   end
 end

ee/spec/requests/api/alert_management_alerts_spec.rb → spec/requests/api/alert_management_alerts_spec.rb

@@ -18,7 +18,10 @@ RSpec.describe API::AlertManagementAlerts do
       project.add_developer(user)
     end
 
-    subject { post api("/projects/#{project.id}/alert_management_alerts/#{alert.iid}/metric_images/authorize", user), headers: workhorse_headers }
+    subject do
+      post api("/projects/#{project.id}/alert_management_alerts/#{alert.iid}/metric_images/authorize", user),
+        headers: workhorse_headers
+    end
 
     it 'authorizes uploading with workhorse header' do
       subject
@@ -104,9 +107,10 @@ RSpec.describe API::AlertManagementAlerts do
         expect(json_response['filename']).to eq(file_name)
         expect(json_response['url']).to eq(url)
         expect(json_response['url_text']).to eq(url_text)
-        expect(json_response['file_path']).to match(%r{/uploads/-/system/alert_management_metric_image/file/\d+/#{file_name}})
         expect(json_response['created_at']).not_to be_nil
         expect(json_response['id']).not_to be_nil
+        file_path_regex = %r{/uploads/-/system/alert_management_metric_image/file/\d+/#{file_name}}
+        expect(json_response['file_path']).to match(file_path_regex)
       end
     end
 
@@ -133,7 +137,6 @@ RSpec.describe API::AlertManagementAlerts do
           allow(uploader).to receive(:file_storage?).and_return(true)
         end
 
-        stub_licensed_features(alert_metric_upload: true)
         project.send("add_#{user_role}", user)
       end
 
@@ -142,7 +145,6 @@ RSpec.describe API::AlertManagementAlerts do
 
     context 'file size too large' do
       before do
-        stub_licensed_features(alert_metric_upload: true)
         allow_next_instance_of(UploadedFile) do |upload_file|
           allow(upload_file).to receive(:size).and_return(AlertManagement::MetricImage::MAX_FILE_SIZE + 1)
         end
@@ -161,7 +163,7 @@ RSpec.describe API::AlertManagementAlerts do
         project.add_developer(user)
 
         allow_next_instance_of(::AlertManagement::MetricImages::UploadService) do |service|
-          error = double(success?: false, message: 'some error', http_status: :bad_request)
+          error = instance_double(ServiceResponse, success?: false, message: 'some error', http_status: :bad_request)
           allow(service).to receive(:execute).and_return(error)
         end
       end
@@ -177,7 +179,6 @@ RSpec.describe API::AlertManagementAlerts do
     context 'object storage enabled' do
       before do
         # Object storage
-        stub_licensed_features(alert_metric_upload: true)
         stub_uploads_object_storage(MetricImageUploader)
 
         allow_next_instance_of(MetricImageUploader) do |uploader|
@@ -240,7 +241,6 @@ RSpec.describe API::AlertManagementAlerts do
 
     with_them do
       before do
-        stub_licensed_features(alert_metric_upload: true)
         project.send("add_#{user_role}", user) unless user_role == :not_member
         project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) unless public_project
       end
@@ -255,7 +255,10 @@ RSpec.describe API::AlertManagementAlerts do
     let!(:image) { create(:alert_metric_image, alert: alert) }
     let(:params) { { url: 'http://test.example.com', url_text: 'Example website 123' } }
 
-    subject { put api("/projects/#{project.id}/alert_management_alerts/#{alert.iid}/metric_images/#{image.id}", user), params: params }
+    subject do
+      put api("/projects/#{project.id}/alert_management_alerts/#{alert.iid}/metric_images/#{image.id}", user),
+        params: params
+    end
 
     shared_examples 'can_update_metric_image' do
       it 'can update the metric images' do
@@ -286,7 +289,6 @@ RSpec.describe API::AlertManagementAlerts do
 
     with_them do
       before do
-        stub_licensed_features(alert_metric_upload: true)
         project.send("add_#{user_role}", user) unless user_role == :not_member
         project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) unless public_project
       end
@@ -299,13 +301,10 @@ RSpec.describe API::AlertManagementAlerts do
         project.add_developer(user)
       end
 
-      context 'feature is enabled' do
-        before do
-          stub_licensed_features(alert_metric_upload: true)
-        end
-
       context 'and metric image not found' do
-          subject { put api("/projects/#{project.id}/alert_management_alerts/#{alert.iid}/metric_images/#{non_existing_record_id}", user) }
+        subject do
+          put api("/projects/#{project.id}/alert_management_alerts/#{alert.iid}/metric_images/#{non_existing_record_id}", user) # rubocop: disable Layout/LineLength
+        end
 
         it 'returns an error' do
           subject
@@ -326,20 +325,6 @@ RSpec.describe API::AlertManagementAlerts do
         end
       end
     end
-
-      context 'feature not enabled' do
-        before do
-          stub_licensed_features(alert_metric_upload: false)
-        end
-
-        it 'returns an error' do
-          subject
-
-          expect(response).to have_gitlab_http_status(:forbidden)
-          expect(json_response['message']).to eq('Feature not available')
-        end
-      end
-    end
   end
 
   describe 'DELETE /projects/:id/alert_management_alerts/:alert_iid/metric_images/:metric_image_id' do
@@ -347,7 +332,9 @@ RSpec.describe API::AlertManagementAlerts do
 
     let!(:image) { create(:alert_metric_image, alert: alert) }
 
-    subject { delete api("/projects/#{project.id}/alert_management_alerts/#{alert.iid}/metric_images/#{image.id}", user) }
+    subject do
+      delete api("/projects/#{project.id}/alert_management_alerts/#{alert.iid}/metric_images/#{image.id}", user)
+    end
 
     shared_examples 'can delete metric image successfully' do
       it 'can delete the metric images' do
@@ -377,7 +364,6 @@ RSpec.describe API::AlertManagementAlerts do
 
     with_them do
       before do
-        stub_licensed_features(alert_metric_upload: true)
         project.send("add_#{user_role}", user) unless user_role == :not_member
         project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) unless public_project
       end
@@ -387,12 +373,13 @@ RSpec.describe API::AlertManagementAlerts do
 
     context 'when user has access' do
       before do
-        stub_licensed_features(alert_metric_upload: true)
         project.add_developer(user)
       end
 
       context 'when metric image not found' do
-        subject { delete api("/projects/#{project.id}/alert_management_alerts/#{alert.iid}/metric_images/#{non_existing_record_id}", user) }
+        subject do
+          delete api("/projects/#{project.id}/alert_management_alerts/#{alert.iid}/metric_images/#{non_existing_record_id}", user) # rubocop: disable Layout/LineLength
+        end
 
         it 'returns an error' do
           subject
@@ -419,19 +406,6 @@ RSpec.describe API::AlertManagementAlerts do
           expect(json_response['message']).to eq('Metric image could not be deleted')
         end
       end
-
-      context 'when feature not enabled' do
-        before do
-          stub_licensed_features(alert_metric_upload: false)
-        end
-
-        it 'returns an error' do
-          subject
-
-          expect(response).to have_gitlab_http_status(:forbidden)
-          expect(json_response['message']).to eq('Feature not available')
-        end
-      end
     end
   end
 end

spec/routing/uploads_routing_spec.rb

@@ -21,6 +21,17 @@ RSpec.describe 'Uploads', 'routing' do
     )
   end
 
+  it 'allows fetching alert metric metric images' do
+    expect(get('/uploads/-/system/alert_management_metric_image/file/1/test.jpg')).to route_to(
+      controller: 'uploads',
+      action: 'show',
+      model: 'alert_management_metric_image',
+      id: '1',
+      filename: 'test.jpg',
+      mounted_as: 'file'
+    )
+  end
+
   it 'does not allow creating uploads for other models' do
     unroutable_models = UploadsController::MODEL_CLASSES.keys.compact - %w(personal_snippet user)
 

ee/spec/services/alert_management/metric_images/upload_service_spec.rb → spec/services/alert_management/metric_images/upload_service_spec.rb

@@ -44,13 +44,6 @@ RSpec.describe AlertManagement::MetricImages::UploadService do
         project.add_developer(current_user)
       end
 
-      it_behaves_like 'no metric saved, an error given', 'You are not authorized to upload metric images'
-
-      context 'with license' do
-        before do
-          stub_licensed_features(alert_metric_upload: true)
-        end
-
       it_behaves_like 'uploads the metric'
 
       context 'no url given' do
@@ -71,7 +64,7 @@ RSpec.describe AlertManagement::MetricImages::UploadService do
           }
         end
 
-          it_behaves_like 'no metric saved, an error given', /File does not have a supported extension. Only png, jpg, jpeg, gif, bmp, tiff, ico, and webp are supported/
+        it_behaves_like 'no metric saved, an error given', /File does not have a supported extension. Only png, jpg, jpeg, gif, bmp, tiff, ico, and webp are supported/ # rubocop: disable Layout/LineLength
       end
 
       context 'user is guest' do
@@ -83,5 +76,4 @@ RSpec.describe AlertManagement::MetricImages::UploadService do
       end
     end
   end
-  end
 end