From 92f69fbdabde83d53dc6b7a05838a6d5fe4bba9e Mon Sep 17 00:00:00 2001 From: Illya Klymov Date: Wed, 14 Dec 2022 19:34:30 +0200 Subject: [PATCH 1/3] Add ability to cancel github project improt * cancelling project keeps all data intact Changelog: added --- .../javascripts/import_entities/constants.js | 2 +- .../components/import_projects_table.vue | 6 ++ .../components/provider_repo_table_row.vue | 38 +++++++++++- .../import_entities/import_projects/index.js | 3 + .../import_projects/store/actions.js | 37 ++++++++++++ .../import_projects/store/mutation_types.js | 2 + .../import_projects/store/mutations.js | 5 ++ .../import_entities/import_projects/utils.js | 5 +- app/views/import/_githubish_status.html.haml | 2 + app/views/import/github/status.html.haml | 1 + locale/gitlab.pot | 12 ++++ .../provider_repo_table_row_spec.js | 57 +++++++++++++++++- .../import_projects/store/actions_spec.js | 59 ++++++++++++++++++- .../import_projects/store/mutations_spec.js | 20 +++++++ 14 files changed, 241 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/import_entities/constants.js b/app/assets/javascripts/import_entities/constants.js index d0a23f88ee71af..48b7febca4be67 100644 --- a/app/assets/javascripts/import_entities/constants.js +++ b/app/assets/javascripts/import_entities/constants.js @@ -9,7 +9,7 @@ export const STATUSES = { STARTED: 'started', NONE: 'none', SCHEDULING: 'scheduling', - CANCELLED: 'cancelled', + CANCELED: 'canceled', TIMEOUT: 'timeout', }; diff --git a/app/assets/javascripts/import_entities/import_projects/components/import_projects_table.vue b/app/assets/javascripts/import_entities/import_projects/components/import_projects_table.vue index d82acfa3f05387..75bbdcbf979e0e 100644 --- a/app/assets/javascripts/import_entities/import_projects/components/import_projects_table.vue +++ b/app/assets/javascripts/import_entities/import_projects/components/import_projects_table.vue @@ -37,6 +37,11 @@ export default { required: false, default: false, }, + cancelable: { + type: Boolean, + required: false, + default: false, + }, optionalStages: { type: Array, required: false, @@ -197,6 +202,7 @@ export default { :repo="repo" :user-namespace="defaultTargetNamespace" :optional-stages="optionalStagesSelection" + :cancelable="cancelable" /> diff --git a/app/assets/javascripts/import_entities/import_projects/components/provider_repo_table_row.vue b/app/assets/javascripts/import_entities/import_projects/components/provider_repo_table_row.vue index 371373d9f2b8ba..3b8cf4894e0af2 100644 --- a/app/assets/javascripts/import_entities/import_projects/components/provider_repo_table_row.vue +++ b/app/assets/javascripts/import_entities/import_projects/components/provider_repo_table_row.vue @@ -8,13 +8,15 @@ import { GlDropdownItem, GlDropdownDivider, GlDropdownSectionHeader, + GlTooltip, } from '@gitlab/ui'; import { mapState, mapGetters, mapActions } from 'vuex'; import { __ } from '~/locale'; +import { helpPagePath } from '~/helpers/help_page_helper'; import ImportGroupDropdown from '../../components/group_dropdown.vue'; import ImportStatus from '../../components/import_status.vue'; import { STATUSES } from '../../constants'; -import { isProjectImportable, isIncompatible, getImportStatus } from '../utils'; +import { isProjectImportable, isImporting, isIncompatible, getImportStatus } from '../utils'; export default { name: 'ProviderRepoTableRow', @@ -29,6 +31,7 @@ export default { GlIcon, GlBadge, GlLink, + GlTooltip, }, props: { repo: { @@ -43,6 +46,11 @@ export default { type: Object, required: true, }, + cancelable: { + type: Boolean, + required: false, + default: false, + }, }, computed: { @@ -69,6 +77,10 @@ export default { return getImportStatus(this.repo); }, + isImporting() { + return isImporting(this.repo); + }, + stats() { return this.repo.importedProject?.stats; }, @@ -92,7 +104,7 @@ export default { }, methods: { - ...mapActions(['fetchImport', 'setImportTarget']), + ...mapActions(['fetchImport', 'cancelImport', 'setImportTarget']), updateImportTarget(changedValues) { this.setImportTarget({ repoId: this.repo.importSource.id, @@ -100,6 +112,8 @@ export default { }); }, }, + + helpUrl: helpPagePath('/user/project/import/github.md'), }; @@ -160,6 +174,26 @@ export default { + +
+

{{ s__('ImportProjects|Cancel import') }}

+ {{ + s__( + 'ImportProjects|Imported files will be kept. You can import this repository again later.', + ) + }} + {{ __('Learn more.') }} +
+ + ( }); }; +export const cancelImportFactory = (cancelImportPath) => ({ state, commit }, { repoId }) => { + const existingRepo = state.repositories.find((r) => r.importSource.id === repoId); + + if (!existingRepo?.importedProject) { + return Promise.resolve(); + } + + const { id } = existingRepo.importedProject; + + return axios + .post(cancelImportPath, { + project_id: id, + }) + .then(() => { + commit(types.CANCEL_IMPORT_SUCCESS, { + repoId, + }); + }) + .catch((e) => { + const serverErrorMessage = e?.response?.data?.errors; + const flashMessage = serverErrorMessage + ? sprintf( + s__('ImportProjects|Cancelling project import failed: %{reason}'), + { + reason: serverErrorMessage, + }, + false, + ) + : s__('ImportProjects|Cancelling project import failed'); + + createAlert({ + message: flashMessage, + }); + }); +}; + export const fetchJobsFactory = (jobsPath = isRequired()) => ({ state, commit, dispatch }) => { if (eTagPoll) { stopJobsPolling(); @@ -211,5 +247,6 @@ export default ({ endpoints = isRequired() }) => ({ importAll, fetchRepos: fetchReposFactory({ reposPath: endpoints.reposPath }), fetchImport: fetchImportFactory(endpoints.importPath), + cancelImport: cancelImportFactory(endpoints.cancelPath), fetchJobs: fetchJobsFactory(endpoints.jobsPath), }); diff --git a/app/assets/javascripts/import_entities/import_projects/store/mutation_types.js b/app/assets/javascripts/import_entities/import_projects/store/mutation_types.js index 360582de2db19a..74832a03ac1ad9 100644 --- a/app/assets/javascripts/import_entities/import_projects/store/mutation_types.js +++ b/app/assets/javascripts/import_entities/import_projects/store/mutation_types.js @@ -6,6 +6,8 @@ export const REQUEST_IMPORT = 'REQUEST_IMPORT'; export const RECEIVE_IMPORT_SUCCESS = 'RECEIVE_IMPORT_SUCCESS'; export const RECEIVE_IMPORT_ERROR = 'RECEIVE_IMPORT_ERROR'; +export const CANCEL_IMPORT_SUCCESS = 'CANCEL_IMPORT_SUCCESS'; + export const RECEIVE_JOBS_SUCCESS = 'RECEIVE_JOBS_SUCCESS'; export const SET_FILTER = 'SET_FILTER'; diff --git a/app/assets/javascripts/import_entities/import_projects/store/mutations.js b/app/assets/javascripts/import_entities/import_projects/store/mutations.js index a7cbace0725ae3..160f5ef17c8348 100644 --- a/app/assets/javascripts/import_entities/import_projects/store/mutations.js +++ b/app/assets/javascripts/import_entities/import_projects/store/mutations.js @@ -122,6 +122,11 @@ export default { }); }, + [types.CANCEL_IMPORT_SUCCESS](state, { repoId }) { + const existingRepo = state.repositories.find((r) => r.importSource.id === repoId); + existingRepo.importedProject.importStatus = STATUSES.CANCELED; + }, + [types.SET_IMPORT_TARGET](state, { repoId, importTarget }) { const existingRepo = state.repositories.find((r) => r.importSource.id === repoId); diff --git a/app/assets/javascripts/import_entities/import_projects/utils.js b/app/assets/javascripts/import_entities/import_projects/utils.js index 38bd529321afe0..c4c9e544c1e534 100644 --- a/app/assets/javascripts/import_entities/import_projects/utils.js +++ b/app/assets/javascripts/import_entities/import_projects/utils.js @@ -9,7 +9,10 @@ export function getImportStatus(project) { } export function isProjectImportable(project) { - return !isIncompatible(project) && getImportStatus(project) === STATUSES.NONE; + return ( + !isIncompatible(project) && + [STATUSES.NONE, STATUSES.CANCELED].includes(getImportStatus(project)) + ); } export function isImporting(repo) { diff --git a/app/views/import/_githubish_status.html.haml b/app/views/import/_githubish_status.html.haml index 46b15de5caff72..4d2186a1352891 100644 --- a/app/views/import/_githubish_status.html.haml +++ b/app/views/import/_githubish_status.html.haml @@ -4,6 +4,7 @@ - filterable = local_assigns.fetch(:filterable, true) - paginatable = local_assigns.fetch(:paginatable, false) - default_namespace_path = (local_assigns[:default_namespace] || current_user.namespace).full_path +- cancel_path = local_assigns.fetch(:cancel_path, nil) - provider_title = Gitlab::ImportSources.title(local_assigns.fetch(:provider)) - optional_stages = local_assigns.fetch(:optional_stages, []) @@ -17,6 +18,7 @@ jobs_path: url_for([:realtime_changes, :import, provider, { format: :json }]), default_target_namespace: default_namespace_path, import_path: url_for([:import, provider, { format: :json }]), + cancel_path: cancel_path, filterable: filterable.to_s, paginatable: paginatable.to_s, optional_stages: optional_stages.to_json }.merge(extra_data) } diff --git a/app/views/import/github/status.html.haml b/app/views/import/github/status.html.haml index 25afe9a7b1b8d4..4a9f8be35c3c04 100644 --- a/app/views/import/github/status.html.haml +++ b/app/views/import/github/status.html.haml @@ -10,4 +10,5 @@ = render 'import/githubish_status', provider: 'github', paginatable: paginatable, default_namespace: @namespace, + cancel_path: cancel_import_github_path, optional_stages: Gitlab::GithubImport::Settings.stages_array diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 63299750b886b7..fbbdac860d8f98 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -21041,12 +21041,24 @@ msgstr "" msgid "ImportProjects|Blocked import URL: %{message}" msgstr "" +msgid "ImportProjects|Cancel import" +msgstr "" + +msgid "ImportProjects|Cancelling project import failed" +msgstr "" + +msgid "ImportProjects|Cancelling project import failed: %{reason}" +msgstr "" + msgid "ImportProjects|Error importing repository %{project_safe_import_url} into %{project_full_path} - %{message}" msgstr "" msgid "ImportProjects|Import repositories" msgstr "" +msgid "ImportProjects|Imported files will be kept. You can import this repository again later." +msgstr "" + msgid "ImportProjects|Importing the project failed" msgstr "" diff --git a/spec/frontend/import_entities/import_projects/components/provider_repo_table_row_spec.js b/spec/frontend/import_entities/import_projects/components/provider_repo_table_row_spec.js index f759e0c029a790..d686036781f90b 100644 --- a/spec/frontend/import_entities/import_projects/components/provider_repo_table_row_spec.js +++ b/spec/frontend/import_entities/import_projects/components/provider_repo_table_row_spec.js @@ -10,6 +10,7 @@ import ProviderRepoTableRow from '~/import_entities/import_projects/components/p describe('ProviderRepoTableRow', () => { let wrapper; const fetchImport = jest.fn(); + const cancelImport = jest.fn(); const setImportTarget = jest.fn(); const fakeImportTarget = { targetNamespace: 'target', @@ -24,7 +25,7 @@ describe('ProviderRepoTableRow', () => { getters: { getImportTarget: () => () => fakeImportTarget, }, - actions: { fetchImport, setImportTarget }, + actions: { fetchImport, cancelImport, setImportTarget }, }); return store; @@ -36,6 +37,14 @@ describe('ProviderRepoTableRow', () => { return buttons.length ? buttons.at(0) : buttons; }; + const findCancelButton = () => { + const buttons = wrapper + .findAllComponents(GlButton) + .filter((node) => node.attributes('aria-label') === 'Cancel'); + + return buttons.length ? buttons.at(0) : buttons; + }; + function mountComponent(props) { Vue.use(Vuex); @@ -110,6 +119,52 @@ describe('ProviderRepoTableRow', () => { }); }); + describe('when rendering importing project', () => { + const repo = { + importSource: { + id: 'remote-1', + fullName: 'fullName', + providerLink: 'providerLink', + }, + importedProject: { + id: 1, + fullPath: 'fullPath', + importSource: 'importSource', + importStatus: STATUSES.STARTED, + }, + }; + + describe('when cancelable is true', () => { + beforeEach(() => { + mountComponent({ repo, cancelable: true }); + }); + + it('shows cancel button', () => { + expect(findCancelButton().isVisible()).toBe(true); + }); + + it('cancels import when clicking cancel button', async () => { + findCancelButton().vm.$emit('click'); + + await nextTick(); + + expect(cancelImport).toHaveBeenCalledWith(expect.anything(), { + repoId: repo.importSource.id, + }); + }); + }); + + describe('when cancelable is false', () => { + beforeEach(() => { + mountComponent({ repo, cancelable: false }); + }); + + it('hides cancel button', () => { + expect(findCancelButton().isVisible()).toBe(false); + }); + }); + }); + describe('when rendering imported project', () => { const FAKE_STATS = {}; diff --git a/spec/frontend/import_entities/import_projects/store/actions_spec.js b/spec/frontend/import_entities/import_projects/store/actions_spec.js index 8e2e4d7c1ac6b7..4b34c21daa38a7 100644 --- a/spec/frontend/import_entities/import_projects/store/actions_spec.js +++ b/spec/frontend/import_entities/import_projects/store/actions_spec.js @@ -13,6 +13,7 @@ import { RECEIVE_IMPORT_SUCCESS, RECEIVE_IMPORT_ERROR, RECEIVE_JOBS_SUCCESS, + CANCEL_IMPORT_SUCCESS, SET_PAGE, SET_FILTER, SET_PAGE_CURSORS, @@ -28,6 +29,7 @@ const endpoints = { reposPath: MOCK_ENDPOINT, importPath: MOCK_ENDPOINT, jobsPath: MOCK_ENDPOINT, + cancelPath: MOCK_ENDPOINT, }; const { @@ -36,6 +38,7 @@ const { importAll, fetchRepos, fetchImport, + cancelImport, fetchJobs, setFilter, } = actionsFactory({ @@ -55,14 +58,17 @@ describe('import_projects store actions', () => { ...state(), defaultTargetNamespace, repositories: [ - { importSource: { id: importRepoId, sanitizedName }, importStatus: STATUSES.NONE }, + { + importSource: { id: importRepoId, sanitizedName }, + importedProject: { importStatus: STATUSES.NONE }, + }, { importSource: { id: otherImportRepoId, sanitizedName: 's2' }, - importStatus: STATUSES.NONE, + importedProject: { importStatus: STATUSES.NONE }, }, { importSource: { id: 3, sanitizedName: 's3', incompatible: true }, - importStatus: STATUSES.NONE, + importedProject: { importStatus: STATUSES.NONE }, }, ], provider: 'provider', @@ -417,4 +423,51 @@ describe('import_projects store actions', () => { ); }); }); + + describe('cancelImport', () => { + let mock; + beforeEach(() => { + mock = new MockAdapter(axios); + }); + + afterEach(() => mock.restore()); + + it('commits CANCEL_IMPORT_SUCCESS on success', async () => { + mock.onPost(MOCK_ENDPOINT).reply(200); + + await testAction( + cancelImport, + { repoId: importRepoId }, + localState, + [ + { + type: CANCEL_IMPORT_SUCCESS, + payload: { repoId: 1 }, + }, + ], + [], + ); + }); + + it('shows generic error message on an unsuccessful request', async () => { + mock.onPost(MOCK_ENDPOINT).reply(500); + + await testAction(cancelImport, { repoId: importRepoId }, localState, [], []); + + expect(createAlert).toHaveBeenCalledWith({ + message: 'Cancelling project import failed', + }); + }); + + it('shows detailed error message on an unsuccessful request with errors fields in response', async () => { + const ERROR_MESSAGE = 'dummy'; + mock.onPost(MOCK_ENDPOINT).reply(500, { errors: ERROR_MESSAGE }); + + await testAction(cancelImport, { repoId: importRepoId }, localState, [], []); + + expect(createAlert).toHaveBeenCalledWith({ + message: `Cancelling project import failed: ${ERROR_MESSAGE}`, + }); + }); + }); }); diff --git a/spec/frontend/import_entities/import_projects/store/mutations_spec.js b/spec/frontend/import_entities/import_projects/store/mutations_spec.js index 16c6d74d1e8964..90fc9e5af9221b 100644 --- a/spec/frontend/import_entities/import_projects/store/mutations_spec.js +++ b/spec/frontend/import_entities/import_projects/store/mutations_spec.js @@ -318,4 +318,24 @@ describe('import_projects store mutations', () => { expect(state.pageInfo).toEqual({ ...NEW_CURSORS, page: 1 }); }); }); + + describe(`${types.CANCEL_IMPORT_SUCCESS}`, () => { + const payload = { repoId: 1 }; + + beforeEach(() => { + state = { + repositories: [ + { + importSource: { id: 1 }, + importedProject: { importStatus: STATUSES.NONE }, + }, + ], + }; + mutations[types.CANCEL_IMPORT_SUCCESS](state, payload); + }); + + it('updates project status', () => { + expect(state.repositories[0].importedProject.importStatus).toBe(STATUSES.CANCELED); + }); + }); }); -- GitLab From db72d306383b1311ed54042ec8f23ac5dad12145 Mon Sep 17 00:00:00 2001 From: Illya Klymov Date: Thu, 15 Dec 2022 15:55:25 +0200 Subject: [PATCH 2/3] Swap early return with throw for being explicit --- .../import_entities/import_projects/store/actions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/import_entities/import_projects/store/actions.js b/app/assets/javascripts/import_entities/import_projects/store/actions.js index b4617d4c26ca46..51ac8c273c7345 100644 --- a/app/assets/javascripts/import_entities/import_projects/store/actions.js +++ b/app/assets/javascripts/import_entities/import_projects/store/actions.js @@ -163,7 +163,7 @@ export const cancelImportFactory = (cancelImportPath) => ({ state, commit }, { r const existingRepo = state.repositories.find((r) => r.importSource.id === repoId); if (!existingRepo?.importedProject) { - return Promise.resolve(); + throw new Error(`Attempting to cancel project which is not started: ${repoId}`); } const { id } = existingRepo.importedProject; -- GitLab From 7553543807930e32d449dbe0cf980437d5f9d50e Mon Sep 17 00:00:00 2001 From: Illya Klymov Date: Thu, 15 Dec 2022 20:27:14 +0200 Subject: [PATCH 3/3] Correctly handled scheduling state for import * do not allow to cancel repo when we're waiting for response --- .../import_entities/components/import_status.vue | 2 +- .../import_projects/components/import_projects_table.vue | 2 -- .../import_projects/components/provider_repo_table_row.vue | 6 +++++- .../import_entities/import_projects/store/mutations.js | 4 +++- .../components/import_projects_table_spec.js | 1 - 5 files changed, 9 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/import_entities/components/import_status.vue b/app/assets/javascripts/import_entities/components/import_status.vue index 5455a034106e9d..bd69165f0ca5e6 100644 --- a/app/assets/javascripts/import_entities/components/import_status.vue +++ b/app/assets/javascripts/import_entities/components/import_status.vue @@ -49,7 +49,7 @@ const STATUS_MAP = { text: __('Timeout'), variant: 'danger', }, - [STATUSES.CANCELLED]: { + [STATUSES.CANCELED]: { icon: 'status-stopped', text: __('Cancelled'), variant: 'neutral', diff --git a/app/assets/javascripts/import_entities/import_projects/components/import_projects_table.vue b/app/assets/javascripts/import_entities/import_projects/components/import_projects_table.vue index 75bbdcbf979e0e..63a36f1a79ffc0 100644 --- a/app/assets/javascripts/import_entities/import_projects/components/import_projects_table.vue +++ b/app/assets/javascripts/import_entities/import_projects/components/import_projects_table.vue @@ -102,7 +102,6 @@ export default { }, mounted() { - this.fetchNamespaces(); this.fetchJobs(); if (!this.paginatable) { @@ -119,7 +118,6 @@ export default { ...mapActions([ 'fetchRepos', 'fetchJobs', - 'fetchNamespaces', 'stopJobsPolling', 'clearJobsEtagPoll', 'setFilter', diff --git a/app/assets/javascripts/import_entities/import_projects/components/provider_repo_table_row.vue b/app/assets/javascripts/import_entities/import_projects/components/provider_repo_table_row.vue index 3b8cf4894e0af2..b8faf349375510 100644 --- a/app/assets/javascripts/import_entities/import_projects/components/provider_repo_table_row.vue +++ b/app/assets/javascripts/import_entities/import_projects/components/provider_repo_table_row.vue @@ -81,6 +81,10 @@ export default { return isImporting(this.repo); }, + isCancelable() { + return this.cancelable && this.isImporting && this.importStatus !== STATUSES.SCHEDULING; + }, + stats() { return this.repo.importedProject?.stats; }, @@ -186,7 +190,7 @@ export default { p.importStatus !== STATUSES.CANCELED, + ), existingRepositories: state.repositories, factory: makeNewImportedProject, }); diff --git a/spec/frontend/import_entities/import_projects/components/import_projects_table_spec.js b/spec/frontend/import_entities/import_projects/components/import_projects_table_spec.js index 37cd0a609bdbc8..51f82dab381af9 100644 --- a/spec/frontend/import_entities/import_projects/components/import_projects_table_spec.js +++ b/spec/frontend/import_entities/import_projects/components/import_projects_table_spec.js @@ -59,7 +59,6 @@ describe('ImportProjectsTable', () => { actions: { fetchRepos: fetchReposFn, fetchJobs: jest.fn(), - fetchNamespaces: jest.fn(), importAll: importAllFn, stopJobsPolling: jest.fn(), clearJobsEtagPoll: jest.fn(), -- GitLab