Merge branch...

Merge branch '300684-provide-better-error-description-when-migration-fails-to-start-due-to-backend-validations' into 'master'

Provide better error description when migration fails to start due to backend validations

See merge request gitlab-org/gitlab!54730

app/assets/javascripts/import_entities/import_groups/graphql/client_factory.js

@@ -122,10 +122,8 @@ export function createResolvers({ endpoints, sourceUrl, GroupsManager = SourceGr
           });
           groupManager.startImport({ group, importId: response.data.id });
         } catch (e) {
-          createFlash({
-            message: s__('BulkImport|Importing the group failed'),
-          });
-
+          const message = e?.response?.data?.error ?? s__('BulkImport|Importing the group failed');
+          createFlash({ message });
           groupManager.setImportStatus(group, STATUSES.NONE);
           throw e;
         }

app/controllers/import/bulk_imports_controller.rb

@@ -37,8 +37,13 @@ class Import::BulkImportsController < ApplicationController
   end
 
   def create
-    result = BulkImportService.new(current_user, create_params, credentials).execute
-    render json: result.to_json(only: [:id])
+    response = BulkImportService.new(current_user, create_params, credentials).execute
+
+    if response.success?
+      render json: response.payload.to_json(only: [:id])
+    else
+      render json: { error: response.message }, status: response.http_status
+    end
   end
 
   def realtime_changes

app/services/bulk_import_service.rb

@@ -39,7 +39,12 @@ class BulkImportService
 
     BulkImportWorker.perform_async(bulk_import.id)
 
-    bulk_import
+    ServiceResponse.success(payload: bulk_import)
+  rescue ActiveRecord::RecordInvalid => e
+    ServiceResponse.error(
+      message: e.message,
+      http_status: :unprocessable_entity
+    )
   end
 
   private

spec/controllers/import/bulk_imports_controller_spec.rb

@@ -184,9 +184,15 @@ RSpec.describe Import::BulkImportsController do
       end
 
       describe 'POST create' do
-        let(:instance_url) { "http://fake-intance" }
+        let(:instance_url) { "http://fake-instance" }
         let(:bulk_import) { create(:bulk_import) }
         let(:pat) { "fake-pat" }
+        let(:bulk_import_params) do
+          [{ "source_type" => "group_entity",
+            "source_full_path" => "full_path",
+            "destination_name" => "destination_name",
+            "destination_namespace" => "root" }]
+        end
 
         before do
           session[:bulk_import_gitlab_access_token] = pat
@@ -194,15 +200,9 @@ RSpec.describe Import::BulkImportsController do
         end
 
         it 'executes BulkImportService' do
-          bulk_import_params = [{ "source_type" => "group_entity",
-            "source_full_path" => "full_path",
-            "destination_name" =>
-            "destination_name",
-            "destination_namespace" => "root" }]
-
           expect_next_instance_of(
             BulkImportService, user, bulk_import_params, { url: instance_url, access_token: pat }) do |service|
-            allow(service).to receive(:execute).and_return(bulk_import)
+            allow(service).to receive(:execute).and_return(ServiceResponse.success(payload: bulk_import))
           end
 
           post :create, params: { bulk_import: bulk_import_params }
@@ -210,6 +210,19 @@ RSpec.describe Import::BulkImportsController do
           expect(response).to have_gitlab_http_status(:ok)
           expect(response.body).to eq({ id: bulk_import.id }.to_json)
         end
+
+        it 'returns error when validation fails' do
+          error_response = ServiceResponse.error(message: 'Record invalid', http_status: :unprocessable_entity)
+          expect_next_instance_of(
+            BulkImportService, user, bulk_import_params, { url: instance_url, access_token: pat }) do |service|
+            allow(service).to receive(:execute).and_return(error_response)
+          end
+
+          post :create, params: { bulk_import: bulk_import_params }
+
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+          expect(response.body).to eq({ error: 'Record invalid' }.to_json)
+        end
       end
     end
 

spec/frontend/import_entities/import_groups/graphql/client_factory_spec.js

@@ -2,6 +2,7 @@ import { InMemoryCache } from 'apollo-cache-inmemory';
 import MockAdapter from 'axios-mock-adapter';
 import { createMockClient } from 'mock-apollo-client';
 import waitForPromises from 'helpers/wait_for_promises';
+import createFlash from '~/flash';
 import { STATUSES } from '~/import_entities/constants';
 import {
   clientTypenames,
@@ -18,6 +19,7 @@ import axios from '~/lib/utils/axios_utils';
 import httpStatus from '~/lib/utils/http_status';
 import { statusEndpointFixture, availableNamespacesFixture } from './fixtures';
 
+jest.mock('~/flash');
 jest.mock('~/import_entities/import_groups/graphql/services/status_poller', () => ({
   StatusPoller: jest.fn().mockImplementation(function mock() {
     this.startPolling = jest.fn();
@@ -287,6 +289,40 @@ describe('Bulk import resolvers', () => {
 
         expect(results[0].status).toBe(STATUSES.NONE);
       });
+
+      it('shows default error message when server error is not provided', async () => {
+        axiosMockAdapter
+          .onPost(FAKE_ENDPOINTS.createBulkImport)
+          .reply(httpStatus.INTERNAL_SERVER_ERROR);
+
+        client
+          .mutate({
+            mutation: importGroupMutation,
+            variables: { sourceGroupId: GROUP_ID },
+          })
+          .catch(() => {});
+        await waitForPromises();
+
+        expect(createFlash).toHaveBeenCalledWith({ message: 'Importing the group failed' });
+      });
+
+      it('shows provided error message when error is included in backend response', async () => {
+        const CUSTOM_MESSAGE = 'custom message';
+
+        axiosMockAdapter
+          .onPost(FAKE_ENDPOINTS.createBulkImport)
+          .reply(httpStatus.INTERNAL_SERVER_ERROR, { error: CUSTOM_MESSAGE });
+
+        client
+          .mutate({
+            mutation: importGroupMutation,
+            variables: { sourceGroupId: GROUP_ID },
+          })
+          .catch(() => {});
+        await waitForPromises();
+
+        expect(createFlash).toHaveBeenCalledWith({ message: CUSTOM_MESSAGE });
+      });
     });
   });
 });

spec/services/bulk_import_service_spec.rb

@@ -48,5 +48,22 @@ RSpec.describe BulkImportService do
 
       subject.execute
     end
+
+    it 'returns success ServiceResponse' do
+      result = subject.execute
+
+      expect(result).to be_a(ServiceResponse)
+      expect(result).to be_success
+    end
+
+    it 'returns ServiceResponse with error if validation fails' do
+      params[0][:source_full_path] = nil
+
+      result = subject.execute
+
+      expect(result).to be_a(ServiceResponse)
+      expect(result).to be_error
+      expect(result.message).to eq("Validation failed: Source full path can't be blank")
+    end
   end
 end