Another comment
', createdAt: '2023-11-02T02:45:46Z', - lastEditedAt: '2023-11-02T02:45:46Z', + lastEditedAt: null, url: 'http://127.0.0.1:3000/admin/abuse_reports/20#note_6', resolved: false, author: { @@ -389,3 +389,34 @@ export const createAbuseReportNoteResponse = { }, }, }; + +export const editAbuseReportNoteResponse = { + data: { + updateNote: { + errors: [], + note: { + id: 'gid://gitlab/Note/1', + body: 'Updated comment', + bodyHtml: 'Updated comment
', + createdAt: '2023-10-20T07:47:42Z', + lastEditedAt: '2023-10-20T07:47:42Z', + url: 'http://127.0.0.1:3000/admin/abuse_reports/1#note_1', + resolved: false, + author: { + id: 'gid://gitlab/User/1', + avatarUrl: + 'https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80\u0026d=identicon', + name: 'Administrator', + username: 'root', + webUrl: 'http://127.0.0.1:3000/root', + __typename: 'UserCore', + }, + lastEditedBy: 'root', + userPermissions: { + adminNote: true, + __typename: 'NotePermissions', + }, + }, + }, + }, +}; diff --git a/spec/frontend/sidebar/components/sidebar_dropdown_widget_spec.js b/spec/frontend/sidebar/components/sidebar_dropdown_widget_spec.js index c1c3c1fea91..f3709e67037 100644 --- a/spec/frontend/sidebar/components/sidebar_dropdown_widget_spec.js +++ b/spec/frontend/sidebar/components/sidebar_dropdown_widget_spec.js @@ -1,11 +1,11 @@ import { GlLink, GlLoadingIcon, GlSearchBoxByType } from '@gitlab/ui'; -import { shallowMount, mount } from '@vue/test-utils'; import Vue, { nextTick } from 'vue'; import VueApollo from 'vue-apollo'; import * as Sentry from '~/sentry/sentry_browser_wrapper'; import createMockApollo from 'helpers/mock_apollo_helper'; import { createMockDirective, getBinding } from 'helpers/vue_mock_directive'; -import { extendedWrapper } from 'helpers/vue_test_utils_helper'; +import { mountExtended, shallowMountExtended } from 'helpers/vue_test_utils_helper'; +import { stubComponent } from 'helpers/stub_component'; import waitForPromises from 'helpers/wait_for_promises'; import { createAlert } from '~/alert'; import { getIdFromGraphQLId } from '~/graphql_shared/utils'; @@ -97,59 +97,56 @@ describe('SidebarDropdownWidget', () => { ...requestHandlers, ]); - wrapper = extendedWrapper( - mount(SidebarDropdownWidget, { - provide: { canUpdate: true }, - apolloProvider: mockApollo, - propsData: { - workspacePath: mockIssue.projectPath, - attrWorkspacePath: mockIssue.projectPath, - iid: mockIssue.iid, - issuableType: TYPE_ISSUE, - issuableAttribute: IssuableAttributeType.Milestone, - }, - attachTo: document.body, - }), - ); + wrapper = mountExtended(SidebarDropdownWidget, { + provide: { canUpdate: true }, + apolloProvider: mockApollo, + propsData: { + workspacePath: mockIssue.projectPath, + attrWorkspacePath: mockIssue.projectPath, + iid: mockIssue.iid, + issuableType: TYPE_ISSUE, + issuableAttribute: IssuableAttributeType.Milestone, + }, + attachTo: document.body, + }); await waitForApollo(); }; const createComponent = ({ data = {}, mutationPromise = mutationSuccess, queries = {} } = {}) => { - wrapper = extendedWrapper( - shallowMount(SidebarDropdownWidget, { - provide: { canUpdate: true }, - data() { - return data; - }, - propsData: { - workspacePath: '', - attrWorkspacePath: '', - iid: '', - issuableType: TYPE_ISSUE, - issuableAttribute: IssuableAttributeType.Milestone, - }, - mocks: { - $apollo: { - mutate: mutationPromise(), - queries: { - issuable: { loading: false }, - attributesList: { loading: false }, - ...queries, - }, + wrapper = shallowMountExtended(SidebarDropdownWidget, { + provide: { canUpdate: true }, + data() { + return data; + }, + propsData: { + workspacePath: '', + attrWorkspacePath: '', + iid: '', + issuableType: TYPE_ISSUE, + issuableAttribute: IssuableAttributeType.Milestone, + }, + mocks: { + $apollo: { + mutate: mutationPromise(), + queries: { + issuable: { loading: false }, + attributesList: { loading: false }, + ...queries, }, }, - directives: { - GlTooltip: createMockDirective('gl-tooltip'), - }, - stubs: { - SidebarEditableItem, - GlSearchBoxByType, - }, - }), - ); - - wrapper.vm.$refs.dropdown.show = jest.fn(); + }, + directives: { + GlTooltip: createMockDirective('gl-tooltip'), + }, + stubs: { + SidebarEditableItem, + GlSearchBoxByType, + SidebarDropdown: stubComponent(SidebarDropdown, { + methods: { show: jest.fn() }, + }), + }, + }); }; describe('when not editing', () => { diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 05e3bffbeaf..d16a78be533 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -26,6 +26,7 @@ RSpec.describe ApplicationSetting, feature_category: :shared, type: :model do it { expect(setting.default_branch_protection_defaults).to eq({}) } it { expect(setting.max_decompressed_archive_size).to eq(25600) } it { expect(setting.decompress_archive_file_timeout).to eq(210) } + it { expect(setting.bulk_import_concurrent_pipeline_batch_limit).to eq(25) } end describe 'validations' do @@ -1351,6 +1352,13 @@ RSpec.describe ApplicationSetting, feature_category: :shared, type: :model do .with_message("must be a value between 0 and 1") end end + + describe 'bulk_import_concurrent_pipeline_batch_limit' do + it do + is_expected.to validate_numericality_of(:bulk_import_concurrent_pipeline_batch_limit) + .is_greater_than(0) + end + end end context 'restrict creating duplicates' do diff --git a/spec/models/bulk_imports/batch_tracker_spec.rb b/spec/models/bulk_imports/batch_tracker_spec.rb index 336943228c7..1c7cbc0cb8c 100644 --- a/spec/models/bulk_imports/batch_tracker_spec.rb +++ b/spec/models/bulk_imports/batch_tracker_spec.rb @@ -13,4 +13,19 @@ RSpec.describe BulkImports::BatchTracker, type: :model, feature_category: :impor it { is_expected.to validate_presence_of(:batch_number) } it { is_expected.to validate_uniqueness_of(:batch_number).scoped_to(:tracker_id) } end + + describe 'scopes' do + describe '.in_progress' do + it 'returns only batches that are in progress' do + created = create(:bulk_import_batch_tracker, :created) + started = create(:bulk_import_batch_tracker, :started) + create(:bulk_import_batch_tracker, :finished) + create(:bulk_import_batch_tracker, :timeout) + create(:bulk_import_batch_tracker, :failed) + create(:bulk_import_batch_tracker, :skipped) + + expect(described_class.in_progress).to contain_exactly(created, started) + end + end + end end diff --git a/spec/models/bulk_imports/export_status_spec.rb b/spec/models/bulk_imports/export_status_spec.rb index e7a037b54be..aa3bce78534 100644 --- a/spec/models/bulk_imports/export_status_spec.rb +++ b/spec/models/bulk_imports/export_status_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe BulkImports::ExportStatus, feature_category: :importers do +RSpec.describe BulkImports::ExportStatus, :clean_gitlab_redis_cache, feature_category: :importers do let_it_be(:relation) { 'labels' } let_it_be(:import) { create(:bulk_import) } let_it_be(:config) { create(:bulk_import_configuration, bulk_import: import) } @@ -282,4 +282,111 @@ RSpec.describe BulkImports::ExportStatus, feature_category: :importers do end end end + + describe 'caching' do + let(:cached_status) do + subject.send(:status) + subject.send(:status_from_cache) + end + + shared_examples 'does not result in a cached status' do + specify do + expect(cached_status).to be_nil + end + end + + shared_examples 'results in a cached status' do + specify do + expect(cached_status).to include('status' => status) + end + + context 'when something goes wrong during export status fetch' do + before do + allow_next_instance_of(BulkImports::Clients::HTTP) do |client| + allow(client).to receive(:get).and_raise( + BulkImports::NetworkError.new("Unsuccessful response", response: nil) + ) + end + end + + include_examples 'does not result in a cached status' + end + end + + context 'when export status is started' do + let(:status) { BulkImports::Export::STARTED } + + it_behaves_like 'does not result in a cached status' + end + + context 'when export status is failed' do + let(:status) { BulkImports::Export::FAILED } + + it_behaves_like 'results in a cached status' + end + + context 'when export status is finished' do + let(:status) { BulkImports::Export::FINISHED } + + it_behaves_like 'results in a cached status' + end + + context 'when export status is not present' do + let(:status) { nil } + + it_behaves_like 'does not result in a cached status' + end + + context 'when the cache is empty' do + let(:status) { BulkImports::Export::FAILED } + + it 'fetches the status from the remote' do + expect(subject).to receive(:status_from_remote).and_call_original + expect(subject.send(:status)).to include('status' => status) + end + end + + context 'when the cache is not empty' do + let(:status) { BulkImports::Export::FAILED } + + before do + Gitlab::Cache::Import::Caching.write( + described_class.new(tracker, 'labels').send(:cache_key), + { 'status' => 'mock status' }.to_json + ) + end + + it 'does not fetch the status from the remote' do + expect(subject).not_to receive(:status_from_remote) + expect(subject.send(:status)).to eq({ 'status' => 'mock status' }) + end + + context 'with a different entity' do + before do + tracker.entity = create(:bulk_import_entity, bulk_import: import, source_full_path: 'foo') + end + + it 'fetches the status from the remote' do + expect(subject).to receive(:status_from_remote).and_call_original + expect(subject.send(:status)).to include('status' => status) + end + end + + context 'with a different relation' do + let_it_be(:relation) { 'merge_requests' } + + let(:response_double) do + instance_double(HTTParty::Response, parsed_response: [ + { 'relation' => 'labels', 'status' => status }, + { 'relation' => 'merge_requests', 'status' => status } + ]) + end + + it 'fetches the status from the remote' do + expect(subject).to receive(:status_from_remote).and_call_original + expect(subject.send(:status)).to include('status' => status) + end + end + end + end end diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb index c304ae514a6..4e24689c17a 100644 --- a/spec/requests/api/settings_spec.rb +++ b/spec/requests/api/settings_spec.rb @@ -93,6 +93,7 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting, featu expect(json_response['default_branch_protection_defaults']).to be_kind_of(Hash) expect(json_response['max_login_attempts']).to be_nil expect(json_response['failed_login_attempts_unlock_period_in_minutes']).to be_nil + expect(json_response['bulk_import_concurrent_pipeline_batch_limit']).to eq(25) end end @@ -202,6 +203,7 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting, featu jira_connect_proxy_url: 'http://example.com', bulk_import_enabled: false, bulk_import_max_download_file_size: 1, + bulk_import_concurrent_pipeline_batch_limit: 2, allow_runner_registration_token: true, user_defaults_to_private_profile: true, default_syntax_highlighting_theme: 2, @@ -296,6 +298,7 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting, featu expect(json_response['max_import_remote_file_size']).to be(2) expect(json_response['bulk_import_max_download_file_size']).to be(1) expect(json_response['security_txt_content']).to be(nil) + expect(json_response['bulk_import_concurrent_pipeline_batch_limit']).to be(2) end end diff --git a/spec/workers/bulk_imports/pipeline_worker_spec.rb b/spec/workers/bulk_imports/pipeline_worker_spec.rb index 19688596a18..368c7537641 100644 --- a/spec/workers/bulk_imports/pipeline_worker_spec.rb +++ b/spec/workers/bulk_imports/pipeline_worker_spec.rb @@ -48,7 +48,7 @@ RSpec.describe BulkImports::PipelineWorker, feature_category: :importers do end end - include_examples 'an idempotent worker' do + it_behaves_like 'an idempotent worker' do let(:job_args) { [pipeline_tracker.id, pipeline_tracker.stage, entity.id] } it 'runs the pipeline and sets tracker to finished' do @@ -478,8 +478,8 @@ RSpec.describe BulkImports::PipelineWorker, feature_category: :importers do end end - context 'when export is batched' do - let(:batches_count) { 2 } + context 'when export is batched', :aggregate_failures do + let(:batches_count) { 3 } before do allow_next_instance_of(BulkImports::ExportStatus) do |status| @@ -489,10 +489,14 @@ RSpec.describe BulkImports::PipelineWorker, feature_category: :importers do allow(status).to receive(:empty?).and_return(false) allow(status).to receive(:failed?).and_return(false) end + allow(worker).to receive(:log_extra_metadata_on_done).and_call_original end it 'enqueues pipeline batches' do - expect(BulkImports::PipelineBatchWorker).to receive(:perform_async).twice + expect(BulkImports::PipelineBatchWorker).to receive(:perform_async).exactly(3).times + expect(worker).to receive(:log_extra_metadata_on_done).with(:batched, true) + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_batch_numbers_enqueued, [1, 2, 3]) + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_final_batch_was_enqueued, true) worker.perform(pipeline_tracker.id, pipeline_tracker.stage, entity.id) @@ -500,7 +504,24 @@ RSpec.describe BulkImports::PipelineWorker, feature_category: :importers do expect(pipeline_tracker.status_name).to eq(:started) expect(pipeline_tracker.batched).to eq(true) - expect(pipeline_tracker.batches.count).to eq(batches_count) + expect(pipeline_tracker.batches.pluck_batch_numbers).to contain_exactly(1, 2, 3) + expect(described_class.jobs).to be_empty + end + + it 'enqueues only missing pipelines batches' do + create(:bulk_import_batch_tracker, tracker: pipeline_tracker, batch_number: 2) + expect(BulkImports::PipelineBatchWorker).to receive(:perform_async).twice + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_batch_numbers_enqueued, [1, 3]) + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_final_batch_was_enqueued, true) + + worker.perform(pipeline_tracker.id, pipeline_tracker.stage, entity.id) + + pipeline_tracker.reload + + expect(pipeline_tracker.status_name).to eq(:started) + expect(pipeline_tracker.batched).to eq(true) + expect(pipeline_tracker.batches.pluck_batch_numbers).to contain_exactly(1, 2, 3) + expect(described_class.jobs).to be_empty end context 'when batches count is less than 1' do @@ -514,6 +535,127 @@ RSpec.describe BulkImports::PipelineWorker, feature_category: :importers do expect(pipeline_tracker.reload.status_name).to eq(:finished) end end + + context 'when pipeline batch enqueuing should be limited' do + using RSpec::Parameterized::TableSyntax + + before do + allow(::Gitlab::CurrentSettings).to receive(:bulk_import_concurrent_pipeline_batch_limit).and_return(2) + end + + it 'only enqueues limited batches and reenqueues itself' do + expect(BulkImports::PipelineBatchWorker).to receive(:perform_async).twice + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_batch_numbers_enqueued, [1, 2]) + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_final_batch_was_enqueued, false) + + worker.perform(pipeline_tracker.id, pipeline_tracker.stage, entity.id) + + pipeline_tracker.reload + + expect(pipeline_tracker.status_name).to eq(:started) + expect(pipeline_tracker.batched).to eq(true) + expect(pipeline_tracker.batches.pluck_batch_numbers).to contain_exactly(1, 2) + expect(described_class.jobs).to contain_exactly( + hash_including( + 'args' => [pipeline_tracker.id, pipeline_tracker.stage, entity.id], + 'scheduled_at' => be_within(1).of(10.seconds.from_now.to_i) + ) + ) + end + + context 'when there is a batch in progress' do + where(:status) { BulkImports::BatchTracker::IN_PROGRESS_STATES } + + with_them do + before do + create(:bulk_import_batch_tracker, status, batch_number: 1, tracker: pipeline_tracker) + end + + it 'counts the in progress batch against the limit' do + expect(BulkImports::PipelineBatchWorker).to receive(:perform_async).once + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_batch_numbers_enqueued, [2]) + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_final_batch_was_enqueued, false) + + worker.perform(pipeline_tracker.id, pipeline_tracker.stage, entity.id) + + pipeline_tracker.reload + + expect(pipeline_tracker.status_name).to eq(:started) + expect(pipeline_tracker.batched).to eq(true) + expect(pipeline_tracker.batches.pluck_batch_numbers).to contain_exactly(1, 2) + expect(described_class.jobs).to contain_exactly( + hash_including( + 'args' => [pipeline_tracker.id, pipeline_tracker.stage, entity.id], + 'scheduled_at' => be_within(1).of(10.seconds.from_now.to_i) + ) + ) + end + end + end + + context 'when there is a batch that has finished' do + where(:status) do + all_statuses = BulkImports::BatchTracker.state_machines[:status].states.map(&:name) + all_statuses - BulkImports::BatchTracker::IN_PROGRESS_STATES + end + + with_them do + before do + create(:bulk_import_batch_tracker, status, batch_number: 1, tracker: pipeline_tracker) + end + + it 'does not count the finished batch against the limit' do + expect(BulkImports::PipelineBatchWorker).to receive(:perform_async).twice + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_batch_numbers_enqueued, [2, 3]) + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_final_batch_was_enqueued, true) + + worker.perform(pipeline_tracker.id, pipeline_tracker.stage, entity.id) + + pipeline_tracker.reload + + expect(pipeline_tracker.batches.pluck_batch_numbers).to contain_exactly(1, 2, 3) + expect(described_class.jobs).to be_empty + end + end + end + + context 'when the feature flag is disabled' do + before do + stub_feature_flags(bulk_import_limit_concurrent_batches: false) + end + + it 'does not limit batches' do + expect(BulkImports::PipelineBatchWorker).to receive(:perform_async).exactly(3).times + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_batch_numbers_enqueued, [1, 2, 3]) + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_final_batch_was_enqueued, true) + + worker.perform(pipeline_tracker.id, pipeline_tracker.stage, entity.id) + + pipeline_tracker.reload + + expect(pipeline_tracker.status_name).to eq(:started) + expect(pipeline_tracker.batched).to eq(true) + expect(pipeline_tracker.batches.pluck_batch_numbers).to contain_exactly(1, 2, 3) + expect(described_class.jobs).to be_empty + end + + it 'still enqueues only missing pipelines batches' do + create(:bulk_import_batch_tracker, tracker: pipeline_tracker, batch_number: 2) + expect(BulkImports::PipelineBatchWorker).to receive(:perform_async).twice + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_batch_numbers_enqueued, [1, 3]) + expect(worker).to receive(:log_extra_metadata_on_done).with(:tracker_final_batch_was_enqueued, true) + + worker.perform(pipeline_tracker.id, pipeline_tracker.stage, entity.id) + + pipeline_tracker.reload + + expect(pipeline_tracker.status_name).to eq(:started) + expect(pipeline_tracker.batched).to eq(true) + expect(pipeline_tracker.batches.pluck_batch_numbers).to contain_exactly(1, 2, 3) + expect(described_class.jobs).to be_empty + end + end + end end end end