Merge branch '35228-move-endpoint-to-copy-common_metrics-yml-to-core' into 'master'

Move endpoint to copy common_metrics.yml to core

See merge request gitlab-org/gitlab!21548

ee/app/controllers/projects/performance_monitoring/dashboards_controller.rb → app/controllers/projects/performance_monitoring/dashboards_controller.rb

@@ -6,13 +6,13 @@ module Projects
       include BlobHelper
 
       before_action :check_repository_available!
+      before_action :validate_required_params!
       before_action :validate_dashboard_template!
       before_action :authorize_push!
 
       USER_DASHBOARDS_DIR = ::Metrics::Dashboard::ProjectDashboardService::DASHBOARD_ROOT
       DASHBOARD_TEMPLATES = {
-        ::Metrics::Dashboard::SystemDashboardService::DASHBOARD_PATH => true,
-        ::Metrics::Dashboard::ClusterDashboardService::DASHBOARD_PATH => true
+        ::Metrics::Dashboard::SystemDashboardService::DASHBOARD_PATH => ::Metrics::Dashboard::SystemDashboardService::DASHBOARD_PATH
       }.freeze
 
       def create
@@ -29,8 +29,8 @@ module Projects
 
       def respond_success
         respond_to do |format|
-          format.html { redirect_to ide_edit_path(project, branch, new_dashboard_path) }
-          format.json { render json: { redirect_to: ide_edit_path(project, branch, new_dashboard_path) }, status: :created }
+          format.html { redirect_to ide_edit_path(project, redirect_safe_branch_name, new_dashboard_path) }
+          format.json { render json: { redirect_to: ide_edit_path(project, redirect_safe_branch_name, new_dashboard_path) }, status: :created }
         end
       end
 
@@ -44,11 +44,15 @@ module Projects
       end
 
       def authorize_push!
-        access_denied!(%q(You can't commit to this project)) unless user_access(project).can_push_to_branch?(branch)
+        access_denied!(%q(You can't commit to this project)) unless user_access(project).can_push_to_branch?(params[:branch])
       end
 
-      def branch
-        params.require(:branch)
+      def validate_required_params!
+        params.require(%i(branch file_name dashboard))
+      end
+
+      def validate_dashboard_template!
+        access_denied! unless dashboard_template
       end
 
       def dashboard_attrs
@@ -57,26 +61,36 @@ module Projects
           file_path: new_dashboard_path,
           file_content: new_dashboard_content,
           encoding: 'text',
-          branch_name: branch,
-          start_branch: repository.branch_exists?(branch) ? branch : project.default_branch
+          branch_name: params[:branch],
+          start_branch: repository.branch_exists?(params[:branch]) ? params[:branch] : project.default_branch
         }
       end
 
       def commit_message
-        params[:commit_message] || "Create custom dashboard #{params.require(:file_name)}"
+        params[:commit_message] || "Create custom dashboard #{params[:file_name]}"
       end
 
       def new_dashboard_path
-        File.join(USER_DASHBOARDS_DIR, params.require(:file_name))
+        File.join(USER_DASHBOARDS_DIR, params[:file_name])
       end
 
       def new_dashboard_content
-        File.read params.require(:dashboard)
+        File.read(Rails.root.join(dashboard_template))
       end
 
-      def validate_dashboard_template!
-        access_denied! unless DASHBOARD_TEMPLATES[params.require(:dashboard)]
+      def dashboard_template
+        dashboard_templates[params[:dashboard]]
+      end
+
+      def dashboard_templates
+        DASHBOARD_TEMPLATES
+      end
+
+      def redirect_safe_branch_name
+        repository.find_branch(params[:branch]).name
       end
     end
   end
 end
+
+Projects::PerformanceMonitoring::DashboardsController.prepend_if_ee('EE::Projects::PerformanceMonitoring::DashboardsController')

config/routes/project.rb

@@ -259,6 +259,10 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           end
         end
 
+        namespace :performance_monitoring do
+          resources :dashboards, only: [:create]
+        end
+
         namespace :error_tracking do
           resources :projects, only: :index
         end

ee/app/controllers/ee/projects/performance_monitoring/dashboards_controller.rb

+# frozen_string_literal: true
+
+module EE
+  module Projects
+    module PerformanceMonitoring
+      module DashboardsController
+        extend ::Gitlab::Utils::Override
+
+        DASHBOARD_TEMPLATES = {
+          ::Metrics::Dashboard::SystemDashboardService::DASHBOARD_PATH => ::Metrics::Dashboard::SystemDashboardService::DASHBOARD_PATH,
+          ::Metrics::Dashboard::ClusterDashboardService::DASHBOARD_PATH => ::Metrics::Dashboard::ClusterDashboardService::DASHBOARD_PATH
+        }.freeze
+
+        private
+
+        override :dashboard_templates
+        def dashboard_templates
+          DASHBOARD_TEMPLATES
+        end
+      end
+    end
+  end
+end

ee/config/routes/project.rb

@@ -60,10 +60,6 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
 
         resources :subscriptions, only: [:create, :destroy]
 
-        namespace :performance_monitoring do
-          resources :dashboards, only: [:create]
-        end
-
         resources :licenses, only: [:index, :create, :update]
 
         resource :threat_monitoring, only: [:show], controller: :threat_monitoring

ee/spec/controllers/projects/performance_monitoring/dashboards_controller_spec.rb

@@ -6,8 +6,10 @@ describe Projects::PerformanceMonitoring::DashboardsController do
   let_it_be(:user) { create(:user) }
   let_it_be(:namespace) { create(:namespace) }
   let!(:project) { create(:project, :repository, name: 'dashboard-project', namespace: namespace) }
+  let(:repository) { project.repository }
+  let(:branch) { double(name: branch_name) }
   let(:commit_message) { 'test' }
-  let(:branch) { "#{Time.current.to_i}_dashboard_new_branch" }
+  let(:branch_name) { "#{Time.current.to_i}_dashboard_new_branch" }
   let(:dashboard) { 'config/prometheus/common_metrics.yml' }
   let(:file_name) { 'custom_dashboard.yml' }
   let(:params) do
@@ -17,7 +19,7 @@ describe Projects::PerformanceMonitoring::DashboardsController do
       dashboard: dashboard,
       file_name: file_name,
       commit_message: commit_message,
-      branch: branch,
+      branch: branch_name,
       format: :json
     }
   end
@@ -35,30 +37,44 @@ describe Projects::PerformanceMonitoring::DashboardsController do
           end
 
           context 'valid parameters' do
-            it 'delegates commit creation to service' do
-              dashboard_attrs = {
-                commit_message: commit_message,
-                branch_name: branch,
-                start_branch: 'master',
-                encoding: 'text',
-                file_path: '.gitlab/dashboards/custom_dashboard.yml',
-                file_content: File.read('config/prometheus/common_metrics.yml')
-              }
+            ['config/prometheus/common_metrics.yml', 'ee/config/prometheus/cluster_metrics.yml'].each do |dashboard_template|
+              context "dashboard template #{dashboard_template}" do
+                let(:dashboard) { dashboard_template }
+
+                it 'delegates commit creation to service' do
+                  allow(controller).to receive(:repository).and_return(repository)
+                  allow(repository).to receive(:find_branch).and_return(branch)
+                  dashboard_attrs = {
+                    commit_message: commit_message,
+                    branch_name: branch_name,
+                    start_branch: 'master',
+                    encoding: 'text',
+                    file_path: '.gitlab/dashboards/custom_dashboard.yml',
+                    file_content: File.read(dashboard)
+                  }
+
+                  service_instance = instance_double(::Files::CreateService)
+                  expect(::Files::CreateService).to receive(:new).with(project, user, dashboard_attrs).and_return(service_instance)
+                  expect(service_instance).to receive(:execute).and_return(status: :success)
 
-              service_instance = instance_double(::Files::CreateService)
-              expect(::Files::CreateService).to receive(:new).with(project, user, dashboard_attrs).and_return(service_instance)
-              expect(service_instance).to receive(:execute).and_return(status: :success)
+                  post :create, params: params
+                end
+              end
+            end
+
+            it 'extends dashboard template path to absolute url' do
+              allow(::Files::CreateService).to receive(:new).and_return(double(execute: { status: :success }))
+              allow(controller).to receive(:repository).and_return(repository)
+              allow(repository).to receive(:find_branch).and_return(branch)
+
+              expect(File).to receive(:read).with(Rails.root.join('config/prometheus/common_metrics.yml')).and_return('')
 
               post :create, params: params
             end
 
             context 'selected branch already exists' do
               it 'responds with :created status code', :aggregate_failures do
-                post :create, params: params
-
-                expect(response).to have_gitlab_http_status :created
-
-                params[:file_name] = 'some_new_name.yml'
+                repository.add_branch(user, branch_name, 'master')
 
                 post :create, params: params
 
@@ -69,11 +85,14 @@ describe Projects::PerformanceMonitoring::DashboardsController do
             context 'request format json' do
               it 'returns path to new file' do
                 allow(::Files::CreateService).to receive(:new).and_return(double(execute: { status: :success }))
+                allow(controller).to receive(:repository).and_return(repository)
+
+                expect(repository).to receive(:find_branch).with(branch_name).and_return(branch)
 
                 post :create, params: params
 
                 expect(response).to have_gitlab_http_status :created
-                expect(json_response).to eq('redirect_to' => "/-/ide/project/#{namespace.path}/#{project.name}/edit/#{branch}/-/.gitlab/dashboards/#{file_name}")
+                expect(json_response).to eq('redirect_to' => "/-/ide/project/#{namespace.path}/#{project.name}/edit/#{branch_name}/-/.gitlab/dashboards/#{file_name}")
               end
 
               context 'files create service failure' do
@@ -96,10 +115,13 @@ describe Projects::PerformanceMonitoring::DashboardsController do
 
               it 'redirects to ide with new file' do
                 allow(::Files::CreateService).to receive(:new).and_return(double(execute: { status: :success }))
+                allow(controller).to receive(:repository).and_return(repository)
+
+                expect(repository).to receive(:find_branch).with(branch_name).and_return(branch)
 
                 post :create, params: params
 
-                expect(response).to redirect_to "/-/ide/project/#{namespace.path}/#{project.name}/edit/#{branch}/-/.gitlab/dashboards/#{file_name}"
+                expect(response).to redirect_to "/-/ide/project/#{namespace.path}/#{project.name}/edit/#{branch_name}/-/.gitlab/dashboards/#{file_name}"
               end
 
               context 'files create service failure' do
@@ -131,9 +153,11 @@ describe Projects::PerformanceMonitoring::DashboardsController do
             end
 
             it 'use default commit message' do
+              allow(controller).to receive(:repository).and_return(repository)
+              allow(repository).to receive(:find_branch).and_return(branch)
               dashboard_attrs = {
                 commit_message: 'Create custom dashboard custom_dashboard.yml',
-                branch_name: branch,
+                branch_name: branch_name,
                 start_branch: 'master',
                 encoding: 'text',
                 file_path: ".gitlab/dashboards/custom_dashboard.yml",
@@ -149,7 +173,7 @@ describe Projects::PerformanceMonitoring::DashboardsController do
           end
 
           context 'missing branch' do
-            let(:branch) { nil }
+            let(:branch_name) { nil }
 
             it 'raises ActionController::ParameterMissing' do
               expect { post :create, params: params }.to raise_error ActionController::ParameterMissing

spec/controllers/projects/performance_monitoring/dashboards_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::PerformanceMonitoring::DashboardsController do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:namespace) { create(:namespace) }
+  let!(:project) { create(:project, :repository, name: 'dashboard-project', namespace: namespace) }
+  let(:repository) { project.repository }
+  let(:branch) { double(name: branch_name) }
+  let(:commit_message) { 'test' }
+  let(:branch_name) { "#{Time.current.to_i}_dashboard_new_branch" }
+  let(:dashboard) { 'config/prometheus/common_metrics.yml' }
+  let(:file_name) { 'custom_dashboard.yml' }
+  let(:params) do
+    {
+      namespace_id: namespace,
+      project_id: project,
+      dashboard: dashboard,
+      file_name: file_name,
+      commit_message: commit_message,
+      branch: branch_name,
+      format: :json
+    }
+  end
+
+  describe 'POST #create' do
+    context 'authenticated user' do
+      before do
+        sign_in(user)
+      end
+
+      context 'project with repository feature' do
+        context 'with rights to push to the repository' do
+          before do
+            project.add_maintainer(user)
+          end
+
+          context 'valid parameters' do
+            it 'delegates commit creation to service' do
+              allow(controller).to receive(:repository).and_return(repository)
+              allow(repository).to receive(:find_branch).and_return(branch)
+              dashboard_attrs = {
+                commit_message: commit_message,
+                branch_name: branch_name,
+                start_branch: 'master',
+                encoding: 'text',
+                file_path: '.gitlab/dashboards/custom_dashboard.yml',
+                file_content: File.read('config/prometheus/common_metrics.yml')
+              }
+
+              service_instance = instance_double(::Files::CreateService)
+              expect(::Files::CreateService).to receive(:new).with(project, user, dashboard_attrs).and_return(service_instance)
+              expect(service_instance).to receive(:execute).and_return(status: :success)
+
+              post :create, params: params
+            end
+
+            it 'extends dashboard template path to absolute url' do
+              allow(::Files::CreateService).to receive(:new).and_return(double(execute: { status: :success }))
+              allow(controller).to receive(:repository).and_return(repository)
+              allow(repository).to receive(:find_branch).and_return(branch)
+
+              expect(File).to receive(:read).with(Rails.root.join('config/prometheus/common_metrics.yml')).and_return('')
+
+              post :create, params: params
+            end
+
+            context 'selected branch already exists' do
+              it 'responds with :created status code', :aggregate_failures do
+                repository.add_branch(user, branch_name, 'master')
+
+                post :create, params: params
+
+                expect(response).to have_gitlab_http_status :created
+              end
+            end
+
+            context 'request format json' do
+              it 'returns path to new file' do
+                allow(::Files::CreateService).to receive(:new).and_return(double(execute: { status: :success }))
+                allow(controller).to receive(:repository).and_return(repository)
+
+                expect(repository).to receive(:find_branch).with(branch_name).and_return(branch)
+
+                post :create, params: params
+
+                expect(response).to have_gitlab_http_status :created
+                expect(json_response).to eq('redirect_to' => "/-/ide/project/#{namespace.path}/#{project.name}/edit/#{branch_name}/-/.gitlab/dashboards/#{file_name}")
+              end
+
+              context 'files create service failure' do
+                it 'returns json with failure message' do
+                  allow(::Files::CreateService).to receive(:new).and_return(double(execute: { status: false, message: 'something went wrong' }))
+
+                  post :create, params: params
+
+                  expect(response).to have_gitlab_http_status :bad_request
+                  expect(response).to set_flash[:alert].to eq('something went wrong')
+                  expect(json_response).to eq('error' => 'something went wrong')
+                end
+              end
+            end
+
+            context 'request format html' do
+              before do
+                params.delete(:format)
+              end
+
+              it 'redirects to ide with new file' do
+                allow(::Files::CreateService).to receive(:new).and_return(double(execute: { status: :success }))
+                allow(controller).to receive(:repository).and_return(repository)
+
+                expect(repository).to receive(:find_branch).with(branch_name).and_return(branch)
+
+                post :create, params: params
+
+                expect(response).to redirect_to "/-/ide/project/#{namespace.path}/#{project.name}/edit/#{branch_name}/-/.gitlab/dashboards/#{file_name}"
+              end
+
+              context 'files create service failure' do
+                it 'redirects back and sets alert' do
+                  allow(::Files::CreateService).to receive(:new).and_return(double(execute: { status: false, message: 'something went wrong' }))
+                  allow(controller).to receive(:repository).and_return(repository)
+                  allow(repository).to receive(:find_branch).and_return(branch)
+
+                  post :create, params: params
+
+                  expect(response).to set_flash[:alert].to eq('something went wrong')
+                  expect(response).to redirect_to namespace_project_environments_path
+                end
+              end
+            end
+          end
+
+          context 'invalid dashboard template' do
+            let(:dashboard) { 'config/database.yml' }
+
+            it 'responds 404 not found' do
+              post :create, params: params
+
+              expect(response).to have_gitlab_http_status :not_found
+            end
+          end
+
+          context 'missing commit message' do
+            before do
+              params.delete(:commit_message)
+            end
+
+            it 'use default commit message' do
+              allow(controller).to receive(:repository).and_return(repository)
+              allow(repository).to receive(:find_branch).and_return(branch)
+              dashboard_attrs = {
+                commit_message: 'Create custom dashboard custom_dashboard.yml',
+                branch_name: branch_name,
+                start_branch: 'master',
+                encoding: 'text',
+                file_path: ".gitlab/dashboards/custom_dashboard.yml",
+                file_content: File.read('config/prometheus/common_metrics.yml')
+              }
+
+              service_instance = instance_double(::Files::CreateService)
+              expect(::Files::CreateService).to receive(:new).with(project, user, dashboard_attrs).and_return(service_instance)
+              expect(service_instance).to receive(:execute).and_return(status: :success)
+
+              post :create, params: params
+            end
+          end
+
+          context 'missing branch' do
+            let(:branch_name) { nil }
+
+            it 'raises ActionController::ParameterMissing' do
+              expect { post :create, params: params }.to raise_error ActionController::ParameterMissing
+            end
+          end
+        end
+
+        context 'without rights to push to repository' do
+          before do
+            project.add_guest(user)
+          end
+
+          it 'responds with :forbidden status code' do
+            post :create, params: params
+
+            expect(response).to have_gitlab_http_status :forbidden
+          end
+        end
+      end
+
+      context 'project without repository feature' do
+        let!(:project) { create(:project, name: 'dashboard-project', namespace: namespace) }
+
+        it 'responds with :not_found status code' do
+          post :create, params: params
+
+          expect(response).to have_gitlab_http_status :not_found
+        end
+      end
+    end
+  end
+end