From c6bdf448dbe6136f26413cd96db7c33a36ade456 Mon Sep 17 00:00:00 2001
From: Ahmed Hemdan <ahemdan@gitlab.com>
Date: Wed, 19 Jul 2023 13:20:20 +0000
Subject: [PATCH] Raise a generic exception in
CiConfiguration::BaseCreateService
---
app/graphql/types/project_type.rb | 2 +-
.../ci_configuration/base_create_service.rb | 12 +--
.../sast/components/app.vue | 4 +-
.../sast/components/app_spec.js | 75 +++++++------------
.../security_configuration/sast/constants.js | 1 -
spec/graphql/types/project_type_spec.rb | 2 +-
.../sast_create_service_spec.rb | 9 ++-
.../create_service_shared_examples.rb | 19 +++--
8 files changed, 55 insertions(+), 69 deletions(-)
diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb
index 992663b4d987..9d2a57388c71 100644
--- a/app/graphql/types/project_type.rb
+++ b/app/graphql/types/project_type.rb
@@ -707,7 +707,7 @@ def sast_ci_configuration
if project.repository.empty?
raise Gitlab::Graphql::Errors::MutationError,
- Gitlab::Utils::ErrorMessage.to_user_facing(_(format('You must %s before using Security features.', add_file_docs_link.html_safe)).html_safe)
+ _(format('You must %s before using Security features.', add_file_docs_link.html_safe)).html_safe
end
::Security::CiConfiguration::SastParserService.new(object).configuration
diff --git a/app/services/security/ci_configuration/base_create_service.rb b/app/services/security/ci_configuration/base_create_service.rb
index b60a949fd4ea..9c3cd6b25fbc 100644
--- a/app/services/security/ci_configuration/base_create_service.rb
+++ b/app/services/security/ci_configuration/base_create_service.rb
@@ -15,12 +15,13 @@ def execute
if project.repository.empty? && !(@params && @params[:initialize_with_sast])
docs_link = ActionController::Base.helpers.link_to _('add at least one file to the repository'),
Rails.application.routes.url_helpers.help_page_url('user/project/repository/index.md',
- anchor: 'add-files-to-a-repository'),
+ anchor: 'add-files-to-a-repository'),
target: '_blank',
rel: 'noopener noreferrer'
- raise Gitlab::Graphql::Errors::MutationError,
- Gitlab::Utils::ErrorMessage.to_user_facing(
- _(format('You must %s before using Security features.', docs_link.html_safe)).html_safe)
+
+ return ServiceResponse.error(
+ message: _(format('You must %s before using Security features.', docs_link)).html_safe
+ )
end
project.repository.add_branch(current_user, branch_name, project.default_branch)
@@ -59,8 +60,7 @@ def existing_gitlab_ci_content
YAML.safe_load(@gitlab_ci_yml) if @gitlab_ci_yml
rescue Psych::BadAlias
raise Gitlab::Graphql::Errors::MutationError,
- Gitlab::Utils::ErrorMessage.to_user_facing(
- _(".gitlab-ci.yml with aliases/anchors is not supported. Please change the CI configuration manually."))
+ _(".gitlab-ci.yml with aliases/anchors is not supported. Please change the CI configuration manually.")
rescue Psych::Exception => e
Gitlab::AppLogger.error("Failed to process existing .gitlab-ci.yml: #{e.message}")
raise Gitlab::Graphql::Errors::MutationError,
diff --git a/ee/app/assets/javascripts/security_configuration/sast/components/app.vue b/ee/app/assets/javascripts/security_configuration/sast/components/app.vue
index 80c2e712035a..9c84d4e34099 100644
--- a/ee/app/assets/javascripts/security_configuration/sast/components/app.vue
+++ b/ee/app/assets/javascripts/security_configuration/sast/components/app.vue
@@ -1,6 +1,5 @@
<script>
import { GlAlert, GlLink, GlLoadingIcon, GlSprintf } from '@gitlab/ui';
-import { parseErrorMessage } from '~/lib/utils/error_message';
import { __, s__ } from '~/locale';
import DismissibleFeedbackAlert from '~/vue_shared/components/dismissible_feedback_alert.vue';
import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
@@ -80,8 +79,9 @@ export default {
},
methods: {
onError(error) {
+ const { gqlError, networkError } = error;
this.hasLoadingError = true;
- this.errorText = parseErrorMessage(error, this.$options.i18n.genericErrorText);
+ this.errorText = networkError ? this.$options.i18n.genericErrorText : gqlError.message;
},
},
feedbackIssue: 'https://gitlab.com/gitlab-org/gitlab/-/issues/225991',
diff --git a/ee/spec/frontend/security_configuration/sast/components/app_spec.js b/ee/spec/frontend/security_configuration/sast/components/app_spec.js
index 1e3eeda3474e..1b5f6b017e2b 100644
--- a/ee/spec/frontend/security_configuration/sast/components/app_spec.js
+++ b/ee/spec/frontend/security_configuration/sast/components/app_spec.js
@@ -1,16 +1,15 @@
import { GlLink, GlLoadingIcon, GlSprintf } from '@gitlab/ui';
-import { merge } from 'lodash';
import Vue from 'vue';
import VueApollo from 'vue-apollo';
+import createMockApollo from 'helpers/mock_apollo_helper';
import ConfigurationPageLayout from 'ee/security_configuration/components/configuration_page_layout.vue';
import SASTConfigurationApp, { i18n } from 'ee/security_configuration/sast/components/app.vue';
import ConfigurationForm from 'ee/security_configuration/sast/components/configuration_form.vue';
import sastCiConfigurationQuery from 'ee/security_configuration/sast/graphql/sast_ci_configuration.query.graphql';
-import createMockApollo from 'helpers/mock_apollo_helper';
import waitForPromises from 'helpers/wait_for_promises';
-import { shallowMountExtended } from 'helpers/vue_test_utils_helper';
+import { mountExtended } from 'helpers/vue_test_utils_helper';
import { sastCiConfigurationQueryResponse } from '../mock_data';
-import { specificErrorMessage, technicalErrorMessage } from '../constants';
+import { specificErrorMessage } from '../constants';
Vue.use(VueApollo);
@@ -22,33 +21,23 @@ describe('SAST Configuration App', () => {
const pendingHandler = () => new Promise(() => {});
const successHandler = () => sastCiConfigurationQueryResponse;
- // Prefixed with window.gon.uf_error_prefix as used in lib/gitlab/utils/error_message.rb to indicate a user facing error
- const failureHandlerSpecific = () => ({
- errors: [{ message: `${window.gon.uf_error_prefix} ${specificErrorMessage}` }],
- });
- const failureHandlerGeneric = () => ({
- errors: [{ message: technicalErrorMessage }],
- });
- const createMockApolloProvider = (handler) =>
- createMockApollo([[sastCiConfigurationQuery, handler]]);
-
- const createComponent = (options) => {
- wrapper = shallowMountExtended(
- SASTConfigurationApp,
- merge(
- {
- // Use a function reference here so it's lazily initialized, and can
- // be replaced with other handlers in certain tests without
- // initialising twice.
- apolloProvider: () => createMockApolloProvider(successHandler),
- provide: {
- sastDocumentationPath,
- projectPath,
- },
- },
- options,
- ),
- );
+ const failureHandler = jest.fn().mockRejectedValue(new Error('Error'));
+ const failureHandlerGraphQL = () => ({ errors: [{ message: specificErrorMessage }] });
+
+ const createMockApolloProvider = (handler) => {
+ return createMockApollo([[sastCiConfigurationQuery, handler]]);
+ };
+
+ const createComponent = ({ options, customMock } = {}) => {
+ wrapper = mountExtended(SASTConfigurationApp, {
+ apolloProvider: customMock || createMockApolloProvider(successHandler),
+ provide: {
+ sastDocumentationPath,
+ projectPath,
+ },
+ options,
+ });
+ return waitForPromises();
};
const findHeader = () => wrapper.find('header');
@@ -62,7 +51,7 @@ describe('SAST Configuration App', () => {
describe('feedback alert', () => {
beforeEach(() => {
createComponent({
- stubs: { GlSprintf, ConfigurationPageLayout },
+ options: { stubs: { GlSprintf, ConfigurationPageLayout } },
});
});
@@ -82,7 +71,7 @@ describe('SAST Configuration App', () => {
describe('header', () => {
beforeEach(() => {
createComponent({
- stubs: { GlSprintf, ConfigurationPageLayout },
+ options: { stubs: { GlSprintf, ConfigurationPageLayout } },
});
});
@@ -97,9 +86,7 @@ describe('SAST Configuration App', () => {
describe('when loading', () => {
beforeEach(() => {
- createComponent({
- apolloProvider: createMockApolloProvider(pendingHandler),
- });
+ createComponent({ customMock: createMockApolloProvider(pendingHandler) });
});
it('displays a loading spinner', () => {
@@ -115,11 +102,9 @@ describe('SAST Configuration App', () => {
});
});
- describe('when loading failed with Error Message including user facing keyword', () => {
+ describe('when loading failed with a GraphQl error', () => {
beforeEach(() => {
- createComponent({
- apolloProvider: createMockApolloProvider(failureHandlerSpecific),
- });
+ createComponent({ customMock: createMockApolloProvider(failureHandlerGraphQL) });
return waitForPromises();
});
@@ -135,18 +120,16 @@ describe('SAST Configuration App', () => {
expect(findErrorAlert().exists()).toBe(true);
});
- it('shows specific error message without keyword and with link when defined', () => {
+ it('shows a specific error message with link when defined', () => {
expect(findErrorAlert().exists()).toBe(true);
expect(findErrorAlert().text()).toContain('some specific error');
expect(findErrorAlert().html()).toContain('<a href="#" rel="noopener">error</a>');
});
});
- describe('when loading failed with Error Message without user facing keyword', () => {
+ describe('when loading failed with a network error', () => {
beforeEach(() => {
- createComponent({
- apolloProvider: createMockApolloProvider(failureHandlerGeneric),
- });
+ createComponent({ customMock: createMockApolloProvider(failureHandler) });
return waitForPromises();
});
@@ -158,7 +141,7 @@ describe('SAST Configuration App', () => {
describe('when loaded', () => {
beforeEach(() => {
- createComponent();
+ createComponent({ customMock: createMockApolloProvider(successHandler) });
});
it('does not display a loading spinner', () => {
diff --git a/ee/spec/frontend/security_configuration/sast/constants.js b/ee/spec/frontend/security_configuration/sast/constants.js
index f6432e2cc78d..9f1abc192c59 100644
--- a/ee/spec/frontend/security_configuration/sast/constants.js
+++ b/ee/spec/frontend/security_configuration/sast/constants.js
@@ -1,2 +1 @@
export const specificErrorMessage = 'some specific <a rel="noopener" href="#">error</a>';
-export const technicalErrorMessage = 'some non-userfacing technical error';
diff --git a/spec/graphql/types/project_type_spec.rb b/spec/graphql/types/project_type_spec.rb
index 262164a08215..bccd4501830e 100644
--- a/spec/graphql/types/project_type_spec.rb
+++ b/spec/graphql/types/project_type_spec.rb
@@ -291,7 +291,7 @@
let_it_be(:project) { create(:project_empty_repo) }
it 'raises an error' do
- expect(subject['errors'][0]['message']).to eq('UF You must <a target="_blank" rel="noopener noreferrer" ' \
+ expect(subject['errors'][0]['message']).to eq('You must <a target="_blank" rel="noopener noreferrer" ' \
'href="http://localhost/help/user/project/repository/index.md#' \
'add-files-to-a-repository">add at least one file to the ' \
'repository</a> before using Security features.')
diff --git a/spec/services/security/ci_configuration/sast_create_service_spec.rb b/spec/services/security/ci_configuration/sast_create_service_spec.rb
index e80fe1a42fa8..555902fd77c6 100644
--- a/spec/services/security/ci_configuration/sast_create_service_spec.rb
+++ b/spec/services/security/ci_configuration/sast_create_service_spec.rb
@@ -45,8 +45,13 @@
let(:params) { { initialize_with_sast: false } }
- it 'raises an error' do
- expect { result }.to raise_error(Gitlab::Graphql::Errors::MutationError)
+ it 'returns a ServiceResponse error' do
+ expect(result).to be_kind_of(ServiceResponse)
+ expect(result.status).to eq(:error)
+ expect(result.message).to eq('You must <a target="_blank" rel="noopener noreferrer" ' \
+ 'href="http://localhost/help/user/project/repository/index.md#' \
+ 'add-files-to-a-repository">add at least one file to the ' \
+ 'repository</a> before using Security features.')
end
end
diff --git a/spec/support/shared_examples/services/security/ci_configuration/create_service_shared_examples.rb b/spec/support/shared_examples/services/security/ci_configuration/create_service_shared_examples.rb
index 21dc3c2bf701..9a7ecd8827b2 100644
--- a/spec/support/shared_examples/services/security/ci_configuration/create_service_shared_examples.rb
+++ b/spec/support/shared_examples/services/security/ci_configuration/create_service_shared_examples.rb
@@ -114,8 +114,8 @@
it 'fails with error' do
expect(project).to receive(:ci_config_for).and_return(unsupported_yaml)
- expect { result }.to raise_error(Gitlab::Graphql::Errors::MutationError, Gitlab::Utils::ErrorMessage.to_user_facing(
- _(".gitlab-ci.yml with aliases/anchors is not supported. Please change the CI configuration manually.")))
+ expect { result }.to raise_error(Gitlab::Graphql::Errors::MutationError,
+ _(".gitlab-ci.yml with aliases/anchors is not supported. Please change the CI configuration manually."))
end
end
@@ -166,14 +166,13 @@
let(:params) { nil }
let_it_be(:project) { create(:project_empty_repo) }
- it 'returns an error' do
- expect { result }.to raise_error { |error|
- expect(error).to be_a(Gitlab::Graphql::Errors::MutationError)
- expect(error.message).to eq('UF You must <a target="_blank" rel="noopener noreferrer" ' \
- 'href="http://localhost/help/user/project/repository/index.md' \
- '#add-files-to-a-repository">add at least one file to the repository' \
- '</a> before using Security features.')
- }
+ it 'returns a ServiceResponse error' do
+ expect(result).to be_kind_of(ServiceResponse)
+ expect(result.status).to eq(:error)
+ expect(result.message).to eq('You must <a target="_blank" rel="noopener noreferrer" ' \
+ 'href="http://localhost/help/user/project/repository/index.md' \
+ '#add-files-to-a-repository">add at least one file to the repository' \
+ '</a> before using Security features.')
end
end
end
--
GitLab