diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-05-20 17:34:42 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-05-20 17:34:42 +0300 |
commit | 9f46488805e86b1bc341ea1620b866016c2ce5ed (patch) | |
tree | f9748c7e287041e37d6da49e0a29c9511dc34768 /spec/services/design_management | |
parent | dfc92d081ea0332d69c8aca2f0e745cb48ae5e6d (diff) |
Add latest changes from gitlab-org/gitlab@13-0-stable-ee
Diffstat (limited to 'spec/services/design_management')
4 files changed, 671 insertions, 0 deletions
diff --git a/spec/services/design_management/delete_designs_service_spec.rb b/spec/services/design_management/delete_designs_service_spec.rb new file mode 100644 index 00000000000..2c0c1570cb4 --- /dev/null +++ b/spec/services/design_management/delete_designs_service_spec.rb @@ -0,0 +1,195 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DesignManagement::DeleteDesignsService do + include DesignManagementTestHelpers + + let_it_be(:project) { create(:project) } + let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:user) { create(:user) } + let(:designs) { create_designs } + + subject(:service) { described_class.new(project, user, issue: issue, designs: designs) } + + # Defined as a method so that the reponse is not cached. We also construct + # a new service executor each time to avoid the intermediate cached values + # it constructs during its execution. + def run_service(delenda = nil) + service = described_class.new(project, user, issue: issue, designs: delenda || designs) + service.execute + end + + let(:response) { run_service } + + shared_examples 'a service error' do + it 'returns an error', :aggregate_failures do + expect(response).to include(status: :error) + end + end + + shared_examples 'a top-level error' do + let(:expected_error) { StandardError } + it 'raises an en expected error', :aggregate_failures do + expect { run_service }.to raise_error(expected_error) + end + end + + shared_examples 'a success' do + it 'returns successfully', :aggregate_failures do + expect(response).to include(status: :success) + end + + it 'saves the user as the author' do + version = response[:version] + + expect(version.author).to eq(user) + end + end + + before do + enable_design_management(enabled) + project.add_developer(user) + end + + describe "#execute" do + context "when the feature is not available" do + let(:enabled) { false } + + it_behaves_like "a service error" + end + + context "when the feature is available" do + let(:enabled) { true } + + it 'is able to delete designs' do + expect(service.send(:can_delete_designs?)).to be true + end + + context 'no designs were passed' do + let(:designs) { [] } + + it_behaves_like "a top-level error" + + it 'does not log any events' do + counter = ::Gitlab::UsageDataCounters::DesignsCounter + expect { run_service rescue nil }.not_to change { counter.totals } + end + end + + context 'one design is passed' do + before do + create_designs(2) + end + + let!(:designs) { create_designs(1) } + + it 'removes that design' do + expect { run_service }.to change { issue.designs.current.count }.from(3).to(2) + end + + it 'logs a deletion event' do + counter = ::Gitlab::UsageDataCounters::DesignsCounter + expect { run_service }.to change { counter.read(:delete) }.by(1) + end + + it 'informs the new-version-worker' do + expect(::DesignManagement::NewVersionWorker).to receive(:perform_async).with(Integer) + + run_service + end + + it 'creates a new version' do + expect { run_service }.to change { DesignManagement::Version.where(issue: issue).count }.by(1) + end + + it 'returns the new version' do + version = response[:version] + + expect(version).to eq(DesignManagement::Version.for_issue(issue).ordered.first) + end + + it_behaves_like "a success" + + it 'removes the design from the current design list' do + run_service + + expect(issue.designs.current).not_to include(designs.first) + end + + it 'marks the design as deleted' do + expect { run_service } + .to change { designs.first.deleted? }.from(false).to(true) + end + end + + context 'more than one design is passed' do + before do + create_designs(1) + end + + let!(:designs) { create_designs(2) } + + it 'removes those designs' do + expect { run_service } + .to change { issue.designs.current.count }.from(3).to(1) + end + + it 'logs the correct number of deletion events' do + counter = ::Gitlab::UsageDataCounters::DesignsCounter + expect { run_service }.to change { counter.read(:delete) }.by(2) + end + + it_behaves_like "a success" + + context 'after executing the service' do + let(:deleted_designs) { designs.map(&:reset) } + + let!(:version) { run_service[:version] } + + it 'removes the removed designs from the current design list' do + expect(issue.designs.current).not_to include(*deleted_designs) + end + + it 'does not make the designs impossible to find' do + expect(issue.designs).to include(*deleted_designs) + end + + it 'associates the new version with all the designs' do + current_versions = deleted_designs.map { |d| d.most_recent_action.version } + expect(current_versions).to all(eq version) + end + + it 'marks all deleted designs as deleted' do + expect(deleted_designs).to all(be_deleted) + end + + it 'marks all deleted designs with the same deletion version' do + expect(deleted_designs.map { |d| d.most_recent_action.version_id }.uniq) + .to have_attributes(size: 1) + end + end + end + + describe 'scalability' do + before do + run_service(create_designs(1)) # ensure project, issue, etc are created + end + + it 'makes the same number of DB requests for one design as for several' do + one = create_designs(1) + many = create_designs(5) + + baseline = ActiveRecord::QueryRecorder.new { run_service(one) } + + expect { run_service(many) }.not_to exceed_query_limit(baseline) + end + end + end + end + + private + + def create_designs(how_many = 2) + create_list(:design, how_many, :with_lfs_file, issue: issue) + end +end diff --git a/spec/services/design_management/design_user_notes_count_service_spec.rb b/spec/services/design_management/design_user_notes_count_service_spec.rb new file mode 100644 index 00000000000..62211a4dd0f --- /dev/null +++ b/spec/services/design_management/design_user_notes_count_service_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DesignManagement::DesignUserNotesCountService, :use_clean_rails_memory_store_caching do + let_it_be(:design) { create(:design, :with_file) } + + subject { described_class.new(design) } + + it_behaves_like 'a counter caching service' + + describe '#count' do + it 'returns the count of notes' do + create_list(:diff_note_on_design, 3, noteable: design) + + expect(subject.count).to eq(3) + end + end + + describe '#cache_key' do + it 'contains the `VERSION` and `design.id`' do + expect(subject.cache_key).to eq(['designs', 'notes_count', DesignManagement::DesignUserNotesCountService::VERSION, design.id]) + end + end + + describe 'cache invalidation' do + it 'changes when a new note is created' do + new_note_attrs = attributes_for(:diff_note_on_design, noteable: design) + + expect do + Notes::CreateService.new(design.project, create(:user), new_note_attrs).execute + end.to change { subject.count }.by(1) + end + + it 'changes when a note is destroyed' do + note = create(:diff_note_on_design, noteable: design) + + expect do + Notes::DestroyService.new(note.project, note.author).execute(note) + end.to change { subject.count }.by(-1) + end + end +end diff --git a/spec/services/design_management/generate_image_versions_service_spec.rb b/spec/services/design_management/generate_image_versions_service_spec.rb new file mode 100644 index 00000000000..cd021c8d7d3 --- /dev/null +++ b/spec/services/design_management/generate_image_versions_service_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DesignManagement::GenerateImageVersionsService do + let_it_be(:project) { create(:project) } + let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:version) { create(:design, :with_lfs_file, issue: issue).versions.first } + let_it_be(:action) { version.actions.first } + + describe '#execute' do + it 'generates the image' do + expect { described_class.new(version).execute } + .to change { action.reload.image_v432x230.file } + .from(nil).to(CarrierWave::SanitizedFile) + end + + it 'skips generating image versions if the mime type is not whitelisted' do + stub_const('DesignManagement::DesignV432x230Uploader::MIME_TYPE_WHITELIST', []) + + described_class.new(version).execute + + expect(action.reload.image_v432x230.file).to eq(nil) + end + + it 'skips generating image versions if the design file size is too large' do + stub_const("#{described_class.name}::MAX_DESIGN_SIZE", 1.byte) + + described_class.new(version).execute + + expect(action.reload.image_v432x230.file).to eq(nil) + end + + it 'returns the status' do + result = described_class.new(version).execute + + expect(result[:status]).to eq(:success) + end + + it 'returns the version' do + result = described_class.new(version).execute + + expect(result[:version]).to eq(version) + end + + it 'logs if the raw image cannot be found' do + version.designs.first.update(filename: 'foo.png') + + expect(Gitlab::AppLogger).to receive(:error).with("No design file found for Action: #{action.id}") + + described_class.new(version).execute + end + + context 'when an error is encountered when generating the image versions' do + before do + expect_next_instance_of(DesignManagement::DesignV432x230Uploader) do |uploader| + expect(uploader).to receive(:cache!).and_raise(CarrierWave::DownloadError, 'foo') + end + end + + it 'logs the error' do + expect(Gitlab::AppLogger).to receive(:error).with('foo') + + described_class.new(version).execute + end + + it 'tracks the error' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with( + instance_of(CarrierWave::DownloadError), + project_id: project.id, version_id: version.id, design_id: version.designs.first.id + ) + + described_class.new(version).execute + end + end + end +end diff --git a/spec/services/design_management/save_designs_service_spec.rb b/spec/services/design_management/save_designs_service_spec.rb new file mode 100644 index 00000000000..013d5473860 --- /dev/null +++ b/spec/services/design_management/save_designs_service_spec.rb @@ -0,0 +1,356 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DesignManagement::SaveDesignsService do + include DesignManagementTestHelpers + include ConcurrentHelpers + + let_it_be(:developer) { create(:user) } + let(:project) { issue.project } + let(:issue) { create(:issue) } + let(:user) { developer } + let(:files) { [rails_sample] } + let(:design_repository) { ::Gitlab::GlRepository::DESIGN.repository_resolver.call(project) } + let(:rails_sample_name) { 'rails_sample.jpg' } + let(:rails_sample) { sample_image(rails_sample_name) } + let(:dk_png) { sample_image('dk.png') } + + def sample_image(filename) + fixture_file_upload("spec/fixtures/#{filename}") + end + + before do + project.add_developer(developer) + end + + def run_service(files_to_upload = nil) + design_files = files_to_upload || files + design_files.each(&:rewind) + + service = described_class.new(project, user, + issue: issue, + files: design_files) + service.execute + end + + # Randomly alter the content of files. + # This allows the files to be updated by the service, as unmodified + # files are rejected. + def touch_files(files_to_touch = nil) + design_files = files_to_touch || files + + design_files.each do |f| + f.tempfile.write(SecureRandom.random_bytes) + end + end + + let(:response) { run_service } + + shared_examples 'a service error' do + it 'returns an error', :aggregate_failures do + expect(response).to match(a_hash_including(status: :error)) + end + end + + shared_examples 'an execution error' do + it 'returns an error', :aggregate_failures do + expect { service.execute }.to raise_error(some_error) + end + end + + describe '#execute' do + context 'when the feature is not available' do + before do + enable_design_management(false) + end + + it_behaves_like 'a service error' + end + + context 'when the feature is available' do + before do + enable_design_management(true) + end + + describe 'repository existence' do + def repository_exists + # Expire the memoized value as the service creates it's own instance + design_repository.expire_exists_cache + design_repository.exists? + end + + it 'creates a design repository when it did not exist' do + expect { run_service }.to change { repository_exists }.from(false).to(true) + end + end + + it 'updates the creation count' do + counter = Gitlab::UsageDataCounters::DesignsCounter + expect { run_service }.to change { counter.read(:create) }.by(1) + end + + it 'creates a commit in the repository' do + run_service + + expect(design_repository.commit).to have_attributes( + author: user, + message: include(rails_sample_name) + ) + end + + it 'can run the same command in parallel' do + blocks = Array.new(10).map do + unique_files = %w(rails_sample.jpg dk.png) + .map { |name| RenameableUpload.unique_file(name) } + + -> { run_service(unique_files) } + end + + expect { run_parallel(blocks) }.to change(DesignManagement::Version, :count).by(10) + end + + it 'causes diff_refs not to be nil' do + expect(response).to include( + designs: all(have_attributes(diff_refs: be_present)) + ) + end + + it 'creates a design & a version for the filename if it did not exist' do + expect(issue.designs.size).to eq(0) + + updated_designs = response[:designs] + + expect(updated_designs.size).to eq(1) + expect(updated_designs.first.versions.size).to eq(1) + end + + it 'saves the user as the author' do + updated_designs = response[:designs] + + expect(updated_designs.first.versions.first.author).to eq(user) + end + + describe 'saving the file to LFS' do + before do + expect_next_instance_of(Lfs::FileTransformer) do |transformer| + expect(transformer).to receive(:lfs_file?).and_return(true) + end + end + + it 'saves the design to LFS' do + expect { run_service }.to change { LfsObject.count }.by(1) + end + + it 'saves the repository_type of the LfsObjectsProject as design' do + expect do + run_service + end.to change { project.lfs_objects_projects.count }.from(0).to(1) + + expect(project.lfs_objects_projects.first.repository_type).to eq('design') + end + end + + context 'when a design is being updated' do + before do + run_service + touch_files + end + + it 'creates a new version for the existing design and updates the file' do + expect(issue.designs.size).to eq(1) + expect(DesignManagement::Version.for_designs(issue.designs).size).to eq(1) + + updated_designs = response[:designs] + + expect(updated_designs.size).to eq(1) + expect(updated_designs.first.versions.size).to eq(2) + end + + it 'increments the update counter' do + counter = Gitlab::UsageDataCounters::DesignsCounter + expect { run_service }.to change { counter.read(:update) }.by 1 + end + + context 'when uploading a new design' do + it 'does not link the new version to the existing design' do + existing_design = issue.designs.first + + updated_designs = run_service([dk_png])[:designs] + + expect(existing_design.versions.reload.size).to eq(1) + expect(updated_designs.size).to eq(1) + expect(updated_designs.first.versions.size).to eq(1) + end + end + end + + context 'when a design has not changed since its previous version' do + before do + run_service + end + + it 'does not create a new version' do + expect { run_service }.not_to change { issue.design_versions.count } + end + + it 'returns the design in `skipped_designs` instead of `designs`' do + response = run_service + + expect(response[:designs]).to be_empty + expect(response[:skipped_designs].size).to eq(1) + end + end + + context 'when doing a mixture of updates and creations' do + let(:files) { [rails_sample, dk_png] } + + before do + # Create just the first one, which we will later update. + run_service([files.first]) + touch_files([files.first]) + end + + it 'counts one creation and one update' do + counter = Gitlab::UsageDataCounters::DesignsCounter + expect { run_service } + .to change { counter.read(:create) }.by(1) + .and change { counter.read(:update) }.by(1) + end + + it 'creates a single commit' do + commit_count = -> do + design_repository.expire_all_method_caches + design_repository.commit_count + end + + expect { run_service }.to change { commit_count.call }.by(1) + end + + it 'enqueues just one new version worker' do + expect(::DesignManagement::NewVersionWorker) + .to receive(:perform_async).once.with(Integer) + + run_service + end + end + + context 'when uploading multiple files' do + let(:files) { [rails_sample, dk_png] } + + it 'returns information about both designs in the response' do + expect(response).to include(designs: have_attributes(size: 2), status: :success) + end + + it 'creates 2 designs with a single version' do + expect { run_service }.to change { issue.designs.count }.from(0).to(2) + + expect(DesignManagement::Version.for_designs(issue.designs).size).to eq(1) + end + + it 'increments the creation count by 2' do + counter = Gitlab::UsageDataCounters::DesignsCounter + expect { run_service }.to change { counter.read(:create) }.by 2 + end + + it 'enqueues a new version worker' do + expect(::DesignManagement::NewVersionWorker) + .to receive(:perform_async).once.with(Integer) + + run_service + end + + it 'creates a single commit' do + commit_count = -> do + design_repository.expire_all_method_caches + design_repository.commit_count + end + + expect { run_service }.to change { commit_count.call }.by(1) + end + + it 'only does 5 gitaly calls', :request_store, :sidekiq_might_not_need_inline do + allow(::DesignManagement::NewVersionWorker).to receive(:perform_async).with(Integer) + service = described_class.new(project, user, issue: issue, files: files) + # Some unrelated calls that are usually cached or happen only once + service.__send__(:repository).create_if_not_exists + service.__send__(:repository).has_visible_content? + + request_count = -> { Gitlab::GitalyClient.get_request_count } + + # An exists?, a check for existing blobs, default branch, an after_commit + # callback on LfsObjectsProject + expect { service.execute }.to change(&request_count).by(4) + end + + context 'when uploading too many files' do + let(:files) { Array.new(DesignManagement::SaveDesignsService::MAX_FILES + 1) { dk_png } } + + it 'returns the correct error' do + expect(response[:message]).to match(/only \d+ files are allowed simultaneously/i) + end + end + end + + context 'when the user is not allowed to upload designs' do + let(:user) { create(:user) } + + it_behaves_like 'a service error' + end + + describe 'failure modes' do + let(:service) { described_class.new(project, user, issue: issue, files: files) } + let(:response) { service.execute } + + before do + expect(service).to receive(:run_actions).and_raise(some_error) + end + + context 'when creating the commit fails' do + let(:some_error) { Gitlab::Git::BaseError } + + it_behaves_like 'an execution error' + end + + context 'when creating the versions fails' do + let(:some_error) { ActiveRecord::RecordInvalid } + + it_behaves_like 'a service error' + end + end + + context "when a design already existed in the repo but we didn't know about it in the database" do + let(:filename) { rails_sample_name } + + before do + path = File.join(build(:design, issue: issue, filename: filename).full_path) + design_repository.create_if_not_exists + design_repository.create_file(user, path, 'something fake', + branch_name: 'master', + message: 'Somehow created without being tracked in db') + end + + it 'creates the design and a new version for it' do + first_updated_design = response[:designs].first + + expect(first_updated_design.filename).to eq(filename) + expect(first_updated_design.versions.size).to eq(1) + end + end + + describe 'scalability', skip: 'See: https://gitlab.com/gitlab-org/gitlab/-/issues/213169' do + before do + run_service([sample_image('banana_sample.gif')]) # ensure project, issue, etc are created + end + + it 'runs the same queries for all requests, regardless of number of files' do + one = [dk_png] + two = [rails_sample, dk_png] + + baseline = ActiveRecord::QueryRecorder.new { run_service(one) } + + expect { run_service(two) }.not_to exceed_query_limit(baseline) + end + end + end + end +end |