From 3a722ff53fe86ce6194f81ade810196f4f8e870c Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 5 Jun 2018 22:34:06 -0700 Subject: [PATCH] Show a more helpful error for import status Importing a project from GitHub for a project namespace that already exists would show an unhelpful error, "An error occurred while importing project." We now add the base message from Projects::CreateService when this fails. Closes #47365 --- app/assets/javascripts/importer_status.js | 10 +++++++- app/controllers/import/base_controller.rb | 9 +++++++ .../import/bitbucket_controller.rb | 2 +- app/controllers/import/fogbugz_controller.rb | 2 +- app/controllers/import/github_controller.rb | 2 +- app/controllers/import/gitlab_controller.rb | 2 +- .../import/google_code_controller.rb | 2 +- .../sh-improve-import-status-error.yml | 5 ++++ spec/javascripts/importer_status_spec.js | 18 ++++++++++++++ ...ubish_import_controller_shared_examples.rb | 24 +++++++++++++++++-- 10 files changed, 68 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/sh-improve-import-status-error.yml diff --git a/app/assets/javascripts/importer_status.js b/app/assets/javascripts/importer_status.js index 52455885248..f9ff0722c01 100644 --- a/app/assets/javascripts/importer_status.js +++ b/app/assets/javascripts/importer_status.js @@ -67,7 +67,15 @@ class ImporterStatus { false, )); }) - .catch(() => flash(__('An error occurred while importing project'))); + .catch((error) => { + let details = error; + + if (error.response && error.response.data && error.response.data.errors) { + details = error.response.data.errors; + } + + flash(__(`An error occurred while importing project: ${details}`)); + }); } autoUpdate() { diff --git a/app/controllers/import/base_controller.rb b/app/controllers/import/base_controller.rb index 663269a0f92..0f401f77fcd 100644 --- a/app/controllers/import/base_controller.rb +++ b/app/controllers/import/base_controller.rb @@ -25,4 +25,13 @@ class Import::BaseController < ApplicationController current_user.namespace end + + def project_save_error(project) + # Projects::CreateService will set base message if unable to save + if project.errors[:base].present? + project.errors[:base].last + else + project.errors.full_messages.join(', ') + end + end end diff --git a/app/controllers/import/bitbucket_controller.rb b/app/controllers/import/bitbucket_controller.rb index 77af5fb9c4f..fa31933e778 100644 --- a/app/controllers/import/bitbucket_controller.rb +++ b/app/controllers/import/bitbucket_controller.rb @@ -55,7 +55,7 @@ class Import::BitbucketController < Import::BaseController if project.persisted? render json: ProjectSerializer.new.represent(project) else - render json: { errors: project.errors.full_messages }, status: :unprocessable_entity + render json: { errors: project_save_error(project) }, status: :unprocessable_entity end else render json: { errors: 'This namespace has already been taken! Please choose another one.' }, status: :unprocessable_entity diff --git a/app/controllers/import/fogbugz_controller.rb b/app/controllers/import/fogbugz_controller.rb index 25ec13b8075..2d665e05ac3 100644 --- a/app/controllers/import/fogbugz_controller.rb +++ b/app/controllers/import/fogbugz_controller.rb @@ -66,7 +66,7 @@ class Import::FogbugzController < Import::BaseController if project.persisted? render json: ProjectSerializer.new.represent(project) else - render json: { errors: project.errors.full_messages }, status: :unprocessable_entity + render json: { errors: project_save_error(project) }, status: :unprocessable_entity end end diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb index f67ec4c248b..c9870332c0f 100644 --- a/app/controllers/import/github_controller.rb +++ b/app/controllers/import/github_controller.rb @@ -48,7 +48,7 @@ class Import::GithubController < Import::BaseController if project.persisted? render json: ProjectSerializer.new.represent(project) else - render json: { errors: project.errors.full_messages }, status: :unprocessable_entity + render json: { errors: project_save_error(project) }, status: :unprocessable_entity end else render json: { errors: 'This namespace has already been taken! Please choose another one.' }, status: :unprocessable_entity diff --git a/app/controllers/import/gitlab_controller.rb b/app/controllers/import/gitlab_controller.rb index 39e2e9e094b..fccbdbca0f6 100644 --- a/app/controllers/import/gitlab_controller.rb +++ b/app/controllers/import/gitlab_controller.rb @@ -32,7 +32,7 @@ class Import::GitlabController < Import::BaseController if project.persisted? render json: ProjectSerializer.new.represent(project) else - render json: { errors: project.errors.full_messages }, status: :unprocessable_entity + render json: { errors: project_save_error(project) }, status: :unprocessable_entity end else render json: { errors: 'This namespace has already been taken! Please choose another one.' }, status: :unprocessable_entity diff --git a/app/controllers/import/google_code_controller.rb b/app/controllers/import/google_code_controller.rb index 9b26a00f7c7..3bce27e810a 100644 --- a/app/controllers/import/google_code_controller.rb +++ b/app/controllers/import/google_code_controller.rb @@ -92,7 +92,7 @@ class Import::GoogleCodeController < Import::BaseController if project.persisted? render json: ProjectSerializer.new.represent(project) else - render json: { errors: project.errors.full_messages }, status: :unprocessable_entity + render json: { errors: project_save_error(project) }, status: :unprocessable_entity end end diff --git a/changelogs/unreleased/sh-improve-import-status-error.yml b/changelogs/unreleased/sh-improve-import-status-error.yml new file mode 100644 index 00000000000..6523280f9e6 --- /dev/null +++ b/changelogs/unreleased/sh-improve-import-status-error.yml @@ -0,0 +1,5 @@ +--- +title: Show a more helpful error for import status +merge_request: +author: +type: other diff --git a/spec/javascripts/importer_status_spec.js b/spec/javascripts/importer_status_spec.js index 87b46ccf7c3..63cdb3d5114 100644 --- a/spec/javascripts/importer_status_spec.js +++ b/spec/javascripts/importer_status_spec.js @@ -50,6 +50,24 @@ describe('Importer Status', () => { }) .catch(done.fail); }); + + it('shows error message after failed POST request', (done) => { + appendSetFixtures('
'); + + mock.onPost(importUrl).reply(422, { + errors: 'You forgot your lunch', + }); + + instance.addToImport({ + currentTarget: document.querySelector('.js-add-to-import'), + }) + .then(() => { + const flashMessage = document.querySelector('.flash-text'); + expect(flashMessage.textContent.trim()).toEqual('An error occurred while importing project: You forgot your lunch'); + done(); + }) + .catch(done.fail); + }); }); describe('autoUpdate', () => { diff --git a/spec/support/controllers/githubish_import_controller_shared_examples.rb b/spec/support/controllers/githubish_import_controller_shared_examples.rb index 368439aa5b0..1a53c5fa487 100644 --- a/spec/support/controllers/githubish_import_controller_shared_examples.rb +++ b/spec/support/controllers/githubish_import_controller_shared_examples.rb @@ -118,14 +118,34 @@ shared_examples 'a GitHub-ish import controller: POST create' do expect(response).to have_gitlab_http_status(200) end - it 'returns 422 response when the project could not be imported' do + it 'returns 422 response with the base error when the project could not be imported' do + project = build(:project) + error_message = 'This is an error' + project.errors.add(:base, error_message) + allow(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, provider_repo.name, user.namespace, user, access_params, type: provider) - .and_return(double(execute: build(:project))) + .and_return(double(execute: project)) post :create, format: :json expect(response).to have_gitlab_http_status(422) + expect(json_response['errors']).to eq(error_message) + end + + it 'returns 422 response with a combined error when the project could not be imported' do + project = build(:project) + project.errors.add(:name, 'is invalid') + project.errors.add(:path, 'is old') + + allow(Gitlab::LegacyGithubImport::ProjectCreator) + .to receive(:new).with(provider_repo, provider_repo.name, user.namespace, user, access_params, type: provider) + .and_return(double(execute: project)) + + post :create, format: :json + + expect(response).to have_gitlab_http_status(422) + expect(json_response['errors']).to eq('Name is invalid, Path is old') end context "when the repository owner is the provider user" do