Merge branch 'refactor-jira-contr-validations' into 'master'

Refactor validations in JiraController

See merge request gitlab-org/gitlab!31604

app/controllers/projects/import/jira_controller.rb

@@ -4,27 +4,30 @@ module Projects
   module Import
     class JiraController < Projects::ApplicationController
       before_action :authenticate_user!
-      before_action :check_issues_available!
       before_action :authorize_read_project!
-      before_action :authorize_admin_project!, only: [:import]
+      before_action :validate_jira_import_settings!
 
       def show
       end
 
-      def import
-        jira_project_key = jira_import_params[:jira_project_key]
+      private
 
-        if jira_project_key.present?
-          response = ::JiraImport::StartImportService.new(current_user, @project, jira_project_key).execute
-          flash[:notice] = response.message if response.message.present?
-        else
-          flash[:alert] = 'No Jira project key has been provided.'
-        end
+      def validate_jira_import_settings!
+        Gitlab::JiraImport.validate_project_settings!(@project, user: current_user, configuration_check: false)
+
+        true
+      rescue Projects::ImportService::Error => e
+        flash[:notice] = e.message
+        redirect_to project_issues_path(@project)
 
-        redirect_to project_import_jira_path(@project)
+        false
       end
 
-      private
+      def jira_service
+        strong_memoize(:jira_service) do
+          @project.jira_service
+        end
+      end
 
       def jira_import_params
         params.permit(:jira_project_key)

app/models/project_services/jira_service.rb

@@ -203,6 +203,10 @@ class JiraService < IssueTrackerService
     add_comment(data, jira_issue)
   end
 
+  def valid_connection?
+    test(nil)[:success]
+  end
+
   def test(_)
     result = test_settings
     success = result.present?

app/views/projects/import/jira/show.html.haml

 .js-jira-import-root{ data: { project_path: @project.full_path,
   issues_path: project_issues_path(@project),
   jira_integration_path: edit_project_service_path(@project, :jira),
-  is_jira_configured: @project.jira_service.present?.to_s,
+  is_jira_configured: @project.jira_service&.valid_connection?.to_s,
   in_progress_illustration: image_path('illustrations/export-import.svg'),
   setup_illustration: image_path('illustrations/manual_action.svg') } }

config/routes/project.rb

@@ -324,9 +324,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         draw :wiki
 
         namespace :import do
-          resource :jira, only: [:show], controller: :jira do
-            post :import
-          end
+          resource :jira, only: [:show], controller: :jira
         end
       end
       # End of the /-/ scope.

lib/gitlab/jira_import.rb

@@ -10,16 +10,18 @@ module Gitlab
     ITEMS_MAPPER_CACHE_KEY = 'jira-import/items-mapper/%{project_id}/%{collection_type}/%{jira_isssue_id}'
     ALREADY_IMPORTED_ITEMS_CACHE_KEY = 'jira-importer/already-imported/%{project}/%{collection_type}'
 
-    def self.validate_project_settings!(project, user: nil)
+    def self.validate_project_settings!(project, user: nil, configuration_check: true)
       if user
         raise Projects::ImportService::Error, _('Cannot import because issues are not available in this project.') unless project.feature_available?(:issues, user)
         raise Projects::ImportService::Error, _('You do not have permissions to run the import.') unless user.can?(:admin_project, project)
       end
 
+      return unless configuration_check
+
       jira_service = project.jira_service
 
       raise Projects::ImportService::Error, _('Jira integration not configured.') unless jira_service&.active?
-      raise Projects::ImportService::Error, _('Unable to connect to the Jira instance. Please check your Jira integration configuration.') unless jira_service.test(nil)[:success]
+      raise Projects::ImportService::Error, _('Unable to connect to the Jira instance. Please check your Jira integration configuration.') unless jira_service&.valid_connection?
     end
 
     def self.jira_issue_cache_key(project_id, jira_issue_id)

spec/controllers/projects/import/jira_controller_spec.rb

@@ -9,142 +9,94 @@ RSpec.describe Projects::Import::JiraController do
   let_it_be(:project) { create(:project) }
   let_it_be(:jira_project_key) { 'Test' }
 
-  context 'with anonymous user' do
-    context 'get show' do
-      it 'redirects to issues page' do
-        get :show, params: { namespace_id: project.namespace, project_id: project }
+  def ensure_correct_config
+    sign_in(user)
+    project.add_maintainer(user)
+    stub_feature_flags(jira_issue_import: true)
+    stub_jira_service_test
+  end
 
-        expect(response).to redirect_to(new_user_session_path)
-      end
+  shared_examples 'redirect with error' do |error|
+    it 'redirects to project issues path' do
+      subject
+
+      expect(response).to redirect_to(project_issues_path(project))
     end
 
-    context 'post import' do
-      it 'redirects to issues page' do
-        post :import, params: { namespace_id: project.namespace, project_id: project, jira_project_key: jira_project_key }
+    it 'renders a correct error' do
+      subject
 
-        expect(response).to redirect_to(new_user_session_path)
-      end
+      expect(flash[:notice]).to eq(error)
     end
   end
 
-  context 'with logged in user' do
-    before do
-      sign_in(user)
-      project.add_maintainer(user)
-      stub_jira_service_test
-    end
-
-    context 'when Jira service is enabled for the project' do
-      let_it_be(:jira_service) { create(:jira_service, project: project) }
-
-      context 'when user is developer' do
-        let_it_be(:dev) { create(:user) }
+  shared_examples 'template with no message' do
+    it 'does not set any message' do
+      subject
 
-        before do
-          sign_in(dev)
-          project.add_developer(dev)
-        end
-
-        context 'get show' do
-          before do
-            get :show, params: { namespace_id: project.namespace.to_param, project_id: project }
-          end
+      expect(flash).to be_empty
+    end
 
-          it 'does not query Jira service' do
-            expect(project).not_to receive(:jira_service)
-          end
+    it 'renders show template' do
+      subject
 
-          it 'renders show template' do
-            expect(response).to render_template(:show)
-          end
-        end
+      expect(response).to render_template(template)
+    end
+  end
 
-        context 'post import' do
-          it 'returns 404' do
-            post :import, params: { namespace_id: project.namespace, project_id: project, jira_project_key: jira_project_key }
+  shared_examples 'users without permissions' do
+    context 'with anonymous user' do
+      it 'redirects to new user page' do
+        subject
 
-            expect(response).to have_gitlab_http_status(:not_found)
-          end
-        end
+        expect(response).to redirect_to(new_user_session_path)
       end
+    end
 
-      context 'when issues disabled' do
-        let_it_be(:disabled_issues_project) { create(:project, :public, :issues_disabled) }
-
-        context 'get show' do
-          it 'returns 404' do
-            get :show, params: { namespace_id: project.namespace.to_param, project_id: disabled_issues_project }
-
-            expect(response).to have_gitlab_http_status(:not_found)
-          end
-        end
-
-        context 'post import' do
-          it 'returns 404' do
-            post :import, params: { namespace_id: disabled_issues_project.namespace, project_id: disabled_issues_project, jira_project_key: jira_project_key }
+    context 'when loged user is a developer' do
+      before do
+        create(:jira_service, project: project)
+        stub_jira_service_test
 
-            expect(response).to have_gitlab_http_status(:not_found)
-          end
-        end
+        sign_in(user)
+        project.add_developer(user)
       end
 
-      context 'when running Jira import first time' do
-        context 'post import' do
-          context 'when Jira project key is empty' do
-            it 'redirects back to show with an error' do
-              post :import, params: { namespace_id: project.namespace, project_id: project, jira_project_key: '' }
-
-              expect(response).to redirect_to(project_import_jira_path(project))
-              expect(flash[:alert]).to eq('No Jira project key has been provided.')
-            end
-          end
+      it_behaves_like 'redirect with error', 'You do not have permissions to run the import.'
+    end
+  end
 
-          context 'when everything is ok' do
-            it 'creates import state' do
-              expect(project.latest_jira_import).to be_nil
+  describe 'GET #show' do
+    let(:template) { 'show' }
 
-              post :import, params: { namespace_id: project.namespace, project_id: project, jira_project_key: jira_project_key }
+    subject { get :show, params: { namespace_id: project.namespace, project_id: project } }
 
-              project.reload
+    it_behaves_like 'users without permissions'
 
-              jira_import = project.latest_jira_import
-              expect(project.import_type).to eq 'jira'
-              expect(jira_import.status).to eq 'scheduled'
-              expect(jira_import.jira_project_key).to eq jira_project_key
-              expect(response).to redirect_to(project_import_jira_path(project))
-            end
-          end
-        end
+    context 'jira service configuration' do
+      before do
+        sign_in(user)
+        project.add_maintainer(user)
+        stub_feature_flags(jira_issue_import: true)
       end
 
-      context 'when import state is scheduled' do
-        let_it_be(:jira_import_state) { create(:jira_import_state, :scheduled, project: project) }
-
-        context 'post import' do
-          it 'uses the existing import data' do
-            post :import, params: { namespace_id: project.namespace, project_id: project, jira_project_key: 'New Project' }
-
-            expect(flash[:notice]).to eq('Jira import is already running.')
-            expect(response).to redirect_to(project_import_jira_path(project))
-          end
+      context 'when Jira service is not enabled for the project' do
+        it 'does not query Jira service' do
+          expect(project).not_to receive(:jira_service)
         end
-      end
 
-      context 'when Jira import ran before' do
-        let_it_be(:jira_import_state) { create(:jira_import_state, :finished, project: project, jira_project_key: jira_project_key) }
+        it_behaves_like 'template with no message'
+      end
 
-        context 'post import' do
-          it 'uses the existing import data' do
-            post :import, params: { namespace_id: project.namespace, project_id: project, jira_project_key: 'New Project' }
+      context 'when Jira service is not configured correctly for the project' do
+        let_it_be(:jira_service) { create(:jira_service, project: project) }
 
-            project.reload
-            expect(project.latest_jira_import.status).to eq 'scheduled'
-            expect(project.jira_imports.size).to eq 2
-            expect(project.jira_imports.first.jira_project_key).to eq jira_project_key
-            expect(project.jira_imports.last.jira_project_key).to eq 'New Project'
-            expect(response).to redirect_to(project_import_jira_path(project))
-          end
+        before do
+          WebMock.stub_request(:get, 'https://jira.example.com/rest/api/2/serverInfo')
+            .to_raise(JIRA::HTTPError.new(double(message: 'Some failure.')))
         end
+
+        it_behaves_like 'template with no message'
       end
     end
   end

spec/lib/gitlab/jira_import_spec.rb

@@ -9,6 +9,9 @@ describe Gitlab::JiraImport do
     include JiraServiceHelper
 
     let_it_be(:project, reload: true) { create(:project) }
+    let(:additional_params) { {} }
+
+    subject { described_class.validate_project_settings!(project, additional_params) }
 
     shared_examples 'raise Jira import error' do |message|
       it 'returns error' do
@@ -17,6 +20,16 @@ describe Gitlab::JiraImport do
     end
 
     shared_examples 'jira configuration base checks' do
+      context 'with configuration_check set to false' do
+        before do
+          additional_params[:configuration_check] = false
+        end
+
+        it 'does not raise Jira integration error' do
+          expect { subject }.not_to raise_error
+        end
+      end
+
       context 'when Jira service was not setup' do
         it_behaves_like 'raise Jira import error', 'Jira integration not configured.'
       end
@@ -40,8 +53,6 @@ describe Gitlab::JiraImport do
     end
 
     context 'without user param' do
-      subject { described_class.validate_project_settings!(project) }
-
       it_behaves_like 'jira configuration base checks'
 
       context 'when jira connection is valid' do
@@ -56,7 +67,7 @@ describe Gitlab::JiraImport do
     context 'with user param provided' do
       let_it_be(:user) { create(:user) }
 
-      subject { described_class.validate_project_settings!(project, user: user) }
+      let(:additional_params) { { user: user } }
 
       context 'when user has permission to run import' do
         before do