From 3a722ff53fe86ce6194f81ade810196f4f8e870c Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 5 Jun 2018 22:34:06 -0700 Subject: [PATCH 1/2] 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 From 332275b766283ff65d0637ada3e4080c3cd4f038 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Wed, 6 Jun 2018 01:56:50 -0700 Subject: [PATCH 2/2] Simplify error message handling in Projects::CreateService There's no need to add a redundant message to the errors if the model is invalid. This cleans up the message as well for the importer. --- app/controllers/import/base_controller.rb | 7 +------ app/services/projects/create_service.rb | 2 +- ...githubish_import_controller_shared_examples.rb | 15 --------------- 3 files changed, 2 insertions(+), 22 deletions(-) diff --git a/app/controllers/import/base_controller.rb b/app/controllers/import/base_controller.rb index 0f401f77fcd..5766c6924cd 100644 --- a/app/controllers/import/base_controller.rb +++ b/app/controllers/import/base_controller.rb @@ -27,11 +27,6 @@ class Import::BaseController < ApplicationController 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 + project.errors.full_messages.join(', ') end end diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index d16ecdb7b9b..d4a5b979f63 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -63,6 +63,7 @@ module Projects message = "Unable to save #{e.record.type}: #{e.record.errors.full_messages.join(", ")} " fail(error: message) rescue => e + @project.errors.add(:base, e.message) if @project fail(error: e.message) end @@ -141,7 +142,6 @@ module Projects Rails.logger.error(log_message) if @project - @project.errors.add(:base, message) @project.mark_import_as_failed(message) if @project.persisted? && @project.import? end diff --git a/spec/support/controllers/githubish_import_controller_shared_examples.rb b/spec/support/controllers/githubish_import_controller_shared_examples.rb index 1a53c5fa487..1c1b68c12a2 100644 --- a/spec/support/controllers/githubish_import_controller_shared_examples.rb +++ b/spec/support/controllers/githubish_import_controller_shared_examples.rb @@ -119,21 +119,6 @@ shared_examples 'a GitHub-ish import controller: POST create' do end 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: 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')