Extract list_projects into a separate controller

Extracts `ErrorTrackingController#list_projects` to
`ErrorTracking::ProjectsController`

The main goal of the change is moving towards RESTful controllers,
and have a separate controller per resource.
As well as unload the `ErrorTrackingController` by extracting more
code out of it.

It also follows an established practice to use the HTTP GET method
for reading data.

* Extract Sentry project resource into a separate controller
* Change HTTP method from POST to GET
* Update front-end part of using a new route

app/assets/javascripts/error_tracking_settings/store/actions.js

@@ -25,8 +25,8 @@ export const receiveProjectsError = ({ commit }) => {
 export const fetchProjects = ({ dispatch, state }) => {
   dispatch('requestProjects');
   return axios
-    .post(state.listProjectsEndpoint, {
-      error_tracking_setting: {
+    .get(state.listProjectsEndpoint, {
+      params: {
         api_host: state.apiHost,
         token: state.token,
       },

app/controllers/projects/error_tracking/projects_controller.rb

+# frozen_string_literal: true
+
+module Projects
+  module ErrorTracking
+    class ProjectsController < Projects::ApplicationController
+      respond_to :json
+
+      before_action :authorize_read_sentry_issue!
+
+      def index
+        service = ::ErrorTracking::ListProjectsService.new(
+          project,
+          current_user,
+          list_projects_params
+        )
+        result = service.execute
+
+        if result[:status] == :success
+          render json: { projects: serialize_projects(result[:projects]) }
+        else
+          render(
+            status: result[:http_status] || :bad_request,
+            json: { message: result[:message] }
+          )
+        end
+      end
+
+      private
+
+      def list_projects_params
+        { api_host: params[:api_host], token: params[:token] }
+      end
+
+      def serialize_projects(projects)
+        ::ErrorTracking::ProjectSerializer
+          .new(project: project, user: current_user)
+          .represent(projects)
+      end
+    end
+  end
+end

app/controllers/projects/error_tracking_controller.rb

@@ -33,14 +33,6 @@ class Projects::ErrorTrackingController < Projects::ApplicationController
     end
   end
 
-  def list_projects
-    respond_to do |format|
-      format.json do
-        render_project_list_json
-      end
-    end
-  end
-
   private
 
   def render_index_json
@@ -84,28 +76,6 @@ class Projects::ErrorTrackingController < Projects::ApplicationController
     }
   end
 
-  def render_project_list_json
-    service = ErrorTracking::ListProjectsService.new(
-      project,
-      current_user,
-      list_projects_params
-    )
-    result = service.execute
-
-    if result[:status] == :success
-      render json: {
-        projects: serialize_projects(result[:projects])
-      }
-    else
-      return render(
-        status: result[:http_status] || :bad_request,
-        json: {
-          message: result[:message]
-        }
-      )
-    end
-  end
-
   def handle_errors(result)
     unless result[:status] == :success
       render json: { message: result[:message] },
@@ -117,10 +87,6 @@ class Projects::ErrorTrackingController < Projects::ApplicationController
     params.permit(:search_term, :sort, :cursor)
   end
 
-  def list_projects_params
-    params.require(:error_tracking_setting).permit([:api_host, :token])
-  end
-
   def issue_details_params
     params.permit(:issue_id)
   end
@@ -150,10 +116,4 @@ class Projects::ErrorTrackingController < Projects::ApplicationController
       .new(project: project, user: current_user)
       .represent(event)
   end
-
-  def serialize_projects(projects)
-    ErrorTracking::ProjectSerializer
-      .new(project: project, user: current_user)
-      .represent(projects)
-  end
 end

app/services/projects/operations/update_service.rb

@@ -30,7 +30,7 @@ module Projects
         settings = params[:error_tracking_setting_attributes]
         return {} if settings.blank?
 
-        api_url = ErrorTracking::ProjectErrorTrackingSetting.build_api_url_from(
+        api_url = ::ErrorTracking::ProjectErrorTrackingSetting.build_api_url_from(
           api_host: settings[:api_host],
           project_slug: settings.dig(:project, :slug),
           organization_slug: settings.dig(:project, :organization_slug)

app/views/projects/settings/operations/_error_tracking.html.haml

@@ -12,7 +12,7 @@
       = _('To link Sentry to GitLab, enter your Sentry URL and Auth Token.')
       = link_to _('More information'), help_page_path('user/project/operations/error_tracking'), target: '_blank', rel: 'noopener noreferrer'
   .settings-content
-    .js-error-tracking-form{ data: { list_projects_endpoint: list_projects_project_error_tracking_index_path(@project, format: :json),
+    .js-error-tracking-form{ data: { list_projects_endpoint: namespace_project_error_tracking_projects_path(@project.namespace, @project, format: :json),
         operations_settings_endpoint: project_settings_operations_path(@project),
         project: error_tracking_setting_project_json,
         api_host: setting.api_host,

config/routes/project.rb

@@ -248,6 +248,10 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           end
         end
 
+        namespace :error_tracking do
+          resources :projects, only: :index
+        end
+
         resources :error_tracking, only: [:index], controller: :error_tracking do
           collection do
             get ':issue_id/details',
@@ -256,7 +260,6 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
             get ':issue_id/stack_trace',
               to: 'error_tracking#stack_trace',
               as: 'stack_trace'
-            post :list_projects
           end
         end
 

spec/controllers/projects/error_tracking/projects_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::ErrorTracking::ProjectsController do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+
+  before do
+    sign_in(user)
+    project.add_maintainer(user)
+  end
+
+  describe 'POST #index' do
+    context 'with insufficient permissions' do
+      before do
+        project.add_guest(user)
+      end
+
+      it 'returns 404' do
+        get :index, params: list_projects_params
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'with an anonymous user' do
+      before do
+        sign_out(user)
+      end
+
+      it 'redirects to sign-in page' do
+        get :index, params: list_projects_params
+
+        expect(response).to have_gitlab_http_status(:redirect)
+      end
+    end
+
+    context 'with authorized user' do
+      let(:list_projects_service) { spy(:list_projects_service) }
+      let(:sentry_project) { build(:error_tracking_project) }
+
+      let(:query_params) do
+        list_projects_params.slice(:api_host, :token)
+      end
+
+      before do
+        allow(ErrorTracking::ListProjectsService)
+          .to receive(:new).with(project, user, query_params)
+          .and_return(list_projects_service)
+      end
+
+      context 'service result is successful' do
+        before do
+          expect(list_projects_service).to receive(:execute)
+            .and_return(status: :success, projects: [sentry_project])
+        end
+
+        it 'returns a list of projects' do
+          get :index, params: list_projects_params
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(response).to match_response_schema('error_tracking/list_projects')
+          expect(json_response['projects']).to eq([sentry_project].as_json)
+        end
+      end
+
+      context 'service result is erroneous' do
+        let(:error_message) { 'error message' }
+
+        context 'without http_status' do
+          before do
+            expect(list_projects_service).to receive(:execute)
+              .and_return(status: :error, message: error_message)
+          end
+
+          it 'returns 400 with message' do
+            get :index, params: list_projects_params
+
+            expect(response).to have_gitlab_http_status(:bad_request)
+            expect(json_response['message']).to eq(error_message)
+          end
+        end
+
+        context 'with explicit http_status' do
+          let(:http_status) { :no_content }
+
+          before do
+            expect(list_projects_service).to receive(:execute).and_return(
+              status: :error,
+              message: error_message,
+              http_status: http_status
+            )
+          end
+
+          it 'returns http_status with message' do
+            get :index, params: list_projects_params
+
+            expect(response).to have_gitlab_http_status(http_status)
+            expect(json_response['message']).to eq(error_message)
+          end
+        end
+      end
+    end
+
+    private
+
+    def list_projects_params(opts = {})
+      project_params(
+        format: :json,
+        api_host: 'gitlab.com',
+        token: 'token'
+      )
+    end
+  end
+
+  private
+
+  def project_params(opts = {})
+    opts.reverse_merge(namespace_id: project.namespace, project_id: project)
+  end
+end

spec/controllers/projects/error_tracking_controller_spec.rb

@@ -179,113 +179,6 @@ describe Projects::ErrorTrackingController do
     end
   end
 
-  describe 'POST #list_projects' do
-    context 'with insufficient permissions' do
-      before do
-        project.add_guest(user)
-      end
-
-      it 'returns 404' do
-        post :list_projects, params: list_projects_params
-
-        expect(response).to have_gitlab_http_status(:not_found)
-      end
-    end
-
-    context 'with an anonymous user' do
-      before do
-        sign_out(user)
-      end
-
-      it 'redirects to sign-in page' do
-        post :list_projects, params: list_projects_params
-
-        expect(response).to have_gitlab_http_status(:redirect)
-      end
-    end
-
-    context 'with authorized user' do
-      let(:list_projects_service) { spy(:list_projects_service) }
-      let(:sentry_project) { build(:error_tracking_project) }
-
-      let(:permitted_params) do
-        ActionController::Parameters.new(
-          list_projects_params[:error_tracking_setting]
-        ).permit!
-      end
-
-      before do
-        allow(ErrorTracking::ListProjectsService)
-          .to receive(:new).with(project, user, permitted_params)
-          .and_return(list_projects_service)
-      end
-
-      context 'service result is successful' do
-        before do
-          expect(list_projects_service).to receive(:execute)
-            .and_return(status: :success, projects: [sentry_project])
-        end
-
-        it 'returns a list of projects' do
-          post :list_projects, params: list_projects_params
-
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(response).to match_response_schema('error_tracking/list_projects')
-          expect(json_response['projects']).to eq([sentry_project].as_json)
-        end
-      end
-
-      context 'service result is erroneous' do
-        let(:error_message) { 'error message' }
-
-        context 'without http_status' do
-          before do
-            expect(list_projects_service).to receive(:execute)
-              .and_return(status: :error, message: error_message)
-          end
-
-          it 'returns 400 with message' do
-            get :list_projects, params: list_projects_params
-
-            expect(response).to have_gitlab_http_status(:bad_request)
-            expect(json_response['message']).to eq(error_message)
-          end
-        end
-
-        context 'with explicit http_status' do
-          let(:http_status) { :no_content }
-
-          before do
-            expect(list_projects_service).to receive(:execute).and_return(
-              status: :error,
-              message: error_message,
-              http_status: http_status
-            )
-          end
-
-          it 'returns http_status with message' do
-            get :list_projects, params: list_projects_params
-
-            expect(response).to have_gitlab_http_status(http_status)
-            expect(json_response['message']).to eq(error_message)
-          end
-        end
-      end
-    end
-
-    private
-
-    def list_projects_params(opts = {})
-      project_params(
-        format: :json,
-        error_tracking_setting: {
-          api_host: 'gitlab.com',
-          token: 'token'
-        }
-      )
-    end
-  end
-
   describe 'GET #issue_details' do
     let_it_be(:issue_id) { 1234 }
 

spec/frontend/error_tracking_settings/store/actions_spec.js

@@ -28,7 +28,7 @@ describe('error tracking settings actions', () => {
     });
 
     it('should request and transform the project list', done => {
-      mock.onPost(TEST_HOST).reply(() => [200, { projects: projectList }]);
+      mock.onGet(TEST_HOST).reply(() => [200, { projects: projectList }]);
       testAction(
         actions.fetchProjects,
         null,
@@ -42,14 +42,14 @@ describe('error tracking settings actions', () => {
           },
         ],
         () => {
-          expect(mock.history.post.length).toBe(1);
+          expect(mock.history.get.length).toBe(1);
           done();
         },
       );
     });
 
     it('should handle a server error', done => {
-      mock.onPost(`${TEST_HOST}.json`).reply(() => [400]);
+      mock.onGet(`${TEST_HOST}.json`).reply(() => [400]);
       testAction(
         actions.fetchProjects,
         null,
@@ -62,7 +62,7 @@ describe('error tracking settings actions', () => {
           },
         ],
         () => {
-          expect(mock.history.post.length).toBe(1);
+          expect(mock.history.get.length).toBe(1);
           done();
         },
       );