From 38c2e480bfa180241e94e77c049b1f5256d83bcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Wed, 6 Jun 2018 16:45:42 -0400 Subject: shave off 30% of the query count --- app/uploaders/file_uploader.rb | 6 +-- app/uploaders/object_storage.rb | 11 ++++- .../object_storage/migrate_uploads_worker_spec.rb | 50 ++++++++++++++++++---- 3 files changed, 53 insertions(+), 14 deletions(-) diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 133fdf6684d..36bc0a4575a 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -65,10 +65,10 @@ class FileUploader < GitlabUploader SecureRandom.hex end - def upload_paths(filename) + def upload_paths(identifier) [ - File.join(secret, filename), - File.join(base_dir(Store::REMOTE), secret, filename) + File.join(secret, identifier), + File.join(base_dir(Store::REMOTE), secret, identifier) ] end diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 5aa1bc7227c..3f5d0d200f4 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -29,7 +29,7 @@ module ObjectStorage end def retrieve_from_store!(identifier) - paths = store_dirs.map { |store, path| File.join(path, identifier) } + paths = upload_paths(identifier) unless current_upload_satisfies?(paths, model) # the upload we already have isn't right, find the correct one @@ -261,7 +261,7 @@ module ObjectStorage end def delete_migrated_file(migrated_file) - migrated_file.delete if exists? + migrated_file.delete end def exists? @@ -279,6 +279,13 @@ module ObjectStorage } end + # Returns all the possible paths for an upload. + # the `upload.path` is a lookup parameter, and it may change + # depending on the `store` param. + def upload_paths(identifier) + store_dirs.map { |store, path| File.join(path, identifier) } + end + def cache!(new_file = sanitized_file) # We intercept ::UploadedFile which might be stored on remote storage # We use that for "accelerated" uploads, where we store result on remote storage diff --git a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb index aed62f97448..18da0c6d39a 100644 --- a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb +++ b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb @@ -1,5 +1,7 @@ require 'spec_helper' +MIGRATION_QUERIES = 7 + describe ObjectStorage::MigrateUploadsWorker, :sidekiq do shared_context 'sanity_check! fails' do before do @@ -11,6 +13,13 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do let(:uploads) { Upload.all } let(:to_store) { ObjectStorage::Store::REMOTE } + def perform(uploads) + binding.pry + described_class.new.perform(uploads.ids, model_class.to_s, mounted_as, to_store) + rescue ObjectStorage::MigrateUploadsWorker::Report::MigrationFailures + # swallow + end + shared_examples "uploads migration worker" do describe '.enqueue!' do def enqueue! @@ -69,12 +78,6 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do end describe '#perform' do - def perform - described_class.new.perform(uploads.ids, model_class.to_s, mounted_as, to_store) - rescue ObjectStorage::MigrateUploadsWorker::Report::MigrationFailures - # swallow - end - shared_examples 'outputs correctly' do |success: 0, failures: 0| total = success + failures @@ -82,7 +85,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do it 'outputs the reports' do expect(Rails.logger).to receive(:info).with(%r{Migrated #{success}/#{total} files}) - perform + perform(uploads) end end @@ -90,7 +93,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do it 'outputs upload failures' do expect(Rails.logger).to receive(:warn).with(/Error .* I am a teapot/) - perform + perform(uploads) end end end @@ -98,7 +101,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do it_behaves_like 'outputs correctly', success: 10 it 'migrates files' do - perform + perform(uploads) expect(Upload.where(store: ObjectStorage::Store::LOCAL).count).to eq(0) end @@ -123,6 +126,18 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do end it_behaves_like "uploads migration worker" + + describe "limits N+1 queries" do + let!(:projects) { create_list(:project, 10, :with_avatar) } + + it do + query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } + + more_projects = create_list(:project, 100, :with_avatar) + expected_queries_per_migration = MIGRATION_QUERIES * more_projects.count + expect { perform(Upload.all) }.not_to exceed_query_limit(query_count).with_threshold(expected_queries_per_migration) + end + end end context "for FileUploader" do @@ -140,5 +155,22 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do end it_behaves_like "uploads migration worker" + + describe "limits N+1 queries" do + let!(:projects) { create_list(:project, 10) } + + it do + query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } + + more_projects = create_list(:project, 100) + more_projects.map do |project| + uploader = FileUploader.new(project) + uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt')) + end + expected_queries_per_migration = MIGRATION_QUERIES * more_projects.count + + expect { perform(Upload.all) }.not_to exceed_query_limit(query_count).with_threshold(expected_queries_per_migration) + end + end end end -- cgit v1.2.1 From 44975f8a5ad9c40c615f47f683fb46c94aa0e130 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Thu, 7 Jun 2018 10:01:47 -0400 Subject: shave off another 20% query --- app/uploaders/object_storage.rb | 7 ++++--- app/uploaders/records_uploads.rb | 2 +- .../workers/object_storage/migrate_uploads_worker_spec.rb | 3 +-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 3f5d0d200f4..bc8f1a5861a 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -376,12 +376,13 @@ module ObjectStorage end def with_exclusive_lease - uuid = Gitlab::ExclusiveLease.new(exclusive_lease_key, timeout: 1.hour.to_i).try_obtain - raise 'exclusive lease already taken' unless uuid + lease_key = exclusive_lease_key + uuid = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.hour.to_i).try_obtain + raise "Exclusive lease #{lease_key} already taken." unless uuid yield uuid ensure - Gitlab::ExclusiveLease.cancel(exclusive_lease_key, uuid) + Gitlab::ExclusiveLease.cancel(lease_key, uuid) end # diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb index 89c74a78835..301f4681fcd 100644 --- a/app/uploaders/records_uploads.rb +++ b/app/uploaders/records_uploads.rb @@ -22,7 +22,7 @@ module RecordsUploads Upload.transaction do uploads.where(path: upload_path).delete_all - upload.destroy! if upload + upload.delete if upload self.upload = build_upload.tap(&:save!) end diff --git a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb index 18da0c6d39a..ba01cfe53c5 100644 --- a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb +++ b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -MIGRATION_QUERIES = 7 +MIGRATION_QUERIES = 5 describe ObjectStorage::MigrateUploadsWorker, :sidekiq do shared_context 'sanity_check! fails' do @@ -14,7 +14,6 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do let(:to_store) { ObjectStorage::Store::REMOTE } def perform(uploads) - binding.pry described_class.new.perform(uploads.ids, model_class.to_s, mounted_as, to_store) rescue ObjectStorage::MigrateUploadsWorker::Report::MigrationFailures # swallow -- cgit v1.2.1 From a667de18d3b1d798992e1441ce774c63f801c07e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Thu, 7 Jun 2018 10:15:22 -0400 Subject: added changelog --- .../47408-migrateuploadsworker-is-doing-n-1-queries-on-migration.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/47408-migrateuploadsworker-is-doing-n-1-queries-on-migration.yml diff --git a/changelogs/unreleased/47408-migrateuploadsworker-is-doing-n-1-queries-on-migration.yml b/changelogs/unreleased/47408-migrateuploadsworker-is-doing-n-1-queries-on-migration.yml new file mode 100644 index 00000000000..c0df82f35f1 --- /dev/null +++ b/changelogs/unreleased/47408-migrateuploadsworker-is-doing-n-1-queries-on-migration.yml @@ -0,0 +1,5 @@ +--- +title: Optimize the upload migration proces +merge_request: 15947 +author: +type: fixed -- cgit v1.2.1 From 50872bcc242a582c7e3af25df4d32e1c3e0a28f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Thu, 7 Jun 2018 11:06:04 -0400 Subject: fix the failing spec --- app/uploaders/object_storage.rb | 3 ++- .../shared_examples/uploaders/object_storage_shared_examples.rb | 4 ++-- spec/uploaders/object_storage_spec.rb | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index bc8f1a5861a..bebaa3b807b 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -9,6 +9,7 @@ module ObjectStorage RemoteStoreError = Class.new(StandardError) UnknownStoreError = Class.new(StandardError) ObjectStorageUnavailable = Class.new(StandardError) + ExclusiveLeaseTaken = Class.new(StandardError) TMP_UPLOAD_PATH = 'tmp/uploads'.freeze @@ -378,7 +379,7 @@ module ObjectStorage def with_exclusive_lease lease_key = exclusive_lease_key uuid = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.hour.to_i).try_obtain - raise "Exclusive lease #{lease_key} already taken." unless uuid + raise ExclusiveLeaseTaken, "Exclusive lease #{lease_key} already taken." unless uuid yield uuid ensure diff --git a/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb b/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb index 6352f1527cd..1ecddc14d58 100644 --- a/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb +++ b/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb @@ -85,13 +85,13 @@ shared_examples "migrates" do |to_store:, from_store: nil| it 'does not execute migrate!' do expect(subject).not_to receive(:unsafe_migrate!) - expect { migrate(to) }.to raise_error('exclusive lease already taken') + expect { migrate(to) }.to raise_error(ObjectStorage::ExclusiveLeaseTaken) end it 'does not execute use_file' do expect(subject).not_to receive(:unsafe_use_file) - expect { subject.use_file }.to raise_error('exclusive lease already taken') + expect { subject.use_file }.to raise_error(ObjectStorage::ExclusiveLeaseTaken) end after do diff --git a/spec/uploaders/object_storage_spec.rb b/spec/uploaders/object_storage_spec.rb index 4503288e410..03386bf764f 100644 --- a/spec/uploaders/object_storage_spec.rb +++ b/spec/uploaders/object_storage_spec.rb @@ -321,7 +321,7 @@ describe ObjectStorage do when_file_is_in_use do expect(uploader).not_to receive(:unsafe_migrate!) - expect { uploader.migrate!(described_class::Store::REMOTE) }.to raise_error('exclusive lease already taken') + expect { uploader.migrate!(described_class::Store::REMOTE) }.to raise_error(ObjectStorage::ExclusiveLeaseTaken) end end @@ -329,7 +329,7 @@ describe ObjectStorage do when_file_is_in_use do expect(uploader).not_to receive(:unsafe_use_file) - expect { uploader.use_file }.to raise_error('exclusive lease already taken') + expect { uploader.use_file }.to raise_error(ObjectStorage::ExclusiveLeaseTaken) end end end -- cgit v1.2.1 From e1589a5c584acae83d97d41494616be1f3981da7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Fri, 8 Jun 2018 10:51:59 -0400 Subject: apply feedback --- app/uploaders/object_storage.rb | 13 +++++++++++-- .../workers/object_storage/migrate_uploads_worker_spec.rb | 4 ++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index bebaa3b807b..43c2b419332 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -9,7 +9,16 @@ module ObjectStorage RemoteStoreError = Class.new(StandardError) UnknownStoreError = Class.new(StandardError) ObjectStorageUnavailable = Class.new(StandardError) - ExclusiveLeaseTaken = Class.new(StandardError) + + class ExclusiveLeaseTaken < StandardError + def initialize(lease_key) + @lease_key = lease_key + end + + def message + "Exclusive lease #{@lease_key} already taken." + end + end TMP_UPLOAD_PATH = 'tmp/uploads'.freeze @@ -379,7 +388,7 @@ module ObjectStorage def with_exclusive_lease lease_key = exclusive_lease_key uuid = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.hour.to_i).try_obtain - raise ExclusiveLeaseTaken, "Exclusive lease #{lease_key} already taken." unless uuid + raise ExclusiveLeaseTaken.new(lease_key) unless uuid yield uuid ensure diff --git a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb index ba01cfe53c5..31d323626c5 100644 --- a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb +++ b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb @@ -129,7 +129,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do describe "limits N+1 queries" do let!(:projects) { create_list(:project, 10, :with_avatar) } - it do + it "to N*#{MIGRATION_QUERIES}" do query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } more_projects = create_list(:project, 100, :with_avatar) @@ -158,7 +158,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do describe "limits N+1 queries" do let!(:projects) { create_list(:project, 10) } - it do + it "to N*#{MIGRATION_QUERIES}" do query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } more_projects = create_list(:project, 100) -- cgit v1.2.1 From 6139050e3d394ce09bcb4ed0a0d74e18a5957e84 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 11 Jun 2018 15:35:52 +0100 Subject: Add active class to active file in IDE commit panel Closes #46051 --- .../javascripts/ide/components/commit_sidebar/list.vue | 5 +++++ .../ide/components/commit_sidebar/list_item.vue | 17 ++++++++++++++++- .../javascripts/ide/components/repo_commit_section.vue | 5 ++++- app/assets/stylesheets/pages/repo.scss | 4 ++++ 4 files changed, 29 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/ide/components/commit_sidebar/list.vue b/app/assets/javascripts/ide/components/commit_sidebar/list.vue index 1325fc993b2..c107462e335 100644 --- a/app/assets/javascripts/ide/components/commit_sidebar/list.vue +++ b/app/assets/javascripts/ide/components/commit_sidebar/list.vue @@ -43,6 +43,10 @@ export default { required: false, default: false, }, + activeFileKey: { + type: String, + required: true, + }, }, data() { return { @@ -115,6 +119,7 @@ export default { :action-component="itemActionComponent" :key-prefix="title" :staged-list="stagedList" + :active-file-key="activeFileKey" /> diff --git a/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue b/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue index 03f3e4de83c..b69a9a833a5 100644 --- a/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue +++ b/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue @@ -30,6 +30,10 @@ export default { required: false, default: false, }, + activeFileKey: { + type: String, + required: true, + }, }, computed: { iconName() { @@ -39,6 +43,12 @@ export default { iconClass() { return `multi-file-${this.file.tempFile ? 'addition' : 'modified'} append-right-8`; }, + fullKey() { + return `${this.keyPrefix.toLowerCase()}-${this.file.key}`; + }, + isActive() { + return this.activeFileKey === this.fullKey; + }, }, methods: { ...mapActions([ @@ -70,7 +80,12 @@ export default { Date: Fri, 8 Jun 2018 22:06:08 +0200 Subject: Use upload ID instead of model ID in lease key For FileUploaders it's possible that a model has many uploads and if lease key is created only from model id, it causes that the model's uploads can not be migrated in parallel because the exclusive lease key would be same for all uploads of the model. --- app/uploaders/object_storage.rb | 13 +++++++++---- ...ion-lease-key-is-incorrect-for-non-mounted-uploaders.yml | 5 +++++ .../uploaders/object_storage_shared_examples.rb | 6 ++---- spec/uploaders/object_storage_spec.rb | 12 ++++++++++++ 4 files changed, 28 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/47513-upload-migration-lease-key-is-incorrect-for-non-mounted-uploaders.yml diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 23b3dcf84c0..d8ec5a81968 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -298,6 +298,15 @@ module ObjectStorage super end + def exclusive_lease_key + # For FileUploaders, model may have many uploaders. In that case + # we want to use exclusive key per upload, not per model to allow + # parallel migration + key_object = self.is_a?(RecordsUploads::Concern) && upload ? upload : model + + "object_storage_migrate:#{key_object.class}:#{key_object.id}" + end + private def schedule_background_upload? @@ -364,10 +373,6 @@ module ObjectStorage end end - def exclusive_lease_key - "object_storage_migrate:#{model.class}:#{model.id}" - end - def with_exclusive_lease uuid = Gitlab::ExclusiveLease.new(exclusive_lease_key, timeout: 1.hour.to_i).try_obtain raise 'exclusive lease already taken' unless uuid diff --git a/changelogs/unreleased/47513-upload-migration-lease-key-is-incorrect-for-non-mounted-uploaders.yml b/changelogs/unreleased/47513-upload-migration-lease-key-is-incorrect-for-non-mounted-uploaders.yml new file mode 100644 index 00000000000..010c4e9bce7 --- /dev/null +++ b/changelogs/unreleased/47513-upload-migration-lease-key-is-incorrect-for-non-mounted-uploaders.yml @@ -0,0 +1,5 @@ +--- +title: Use upload ID for creating lease key for file uploaders. +merge_request: +author: +type: fixed diff --git a/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb b/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb index 6352f1527cd..12f7d3ed92a 100644 --- a/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb +++ b/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb @@ -76,10 +76,8 @@ shared_examples "migrates" do |to_store:, from_store: nil| end context 'when migrate! is occupied by another process' do - let(:exclusive_lease_key) { "object_storage_migrate:#{subject.model.class}:#{subject.model.id}" } - before do - @uuid = Gitlab::ExclusiveLease.new(exclusive_lease_key, timeout: 1.hour.to_i).try_obtain + @uuid = Gitlab::ExclusiveLease.new(subject.exclusive_lease_key, timeout: 1.hour.to_i).try_obtain end it 'does not execute migrate!' do @@ -95,7 +93,7 @@ shared_examples "migrates" do |to_store:, from_store: nil| end after do - Gitlab::ExclusiveLease.cancel(exclusive_lease_key, @uuid) + Gitlab::ExclusiveLease.cancel(subject.exclusive_lease_key, @uuid) end end diff --git a/spec/uploaders/object_storage_spec.rb b/spec/uploaders/object_storage_spec.rb index 0bc5b6751b3..902fc0bc030 100644 --- a/spec/uploaders/object_storage_spec.rb +++ b/spec/uploaders/object_storage_spec.rb @@ -332,6 +332,18 @@ describe ObjectStorage do expect { uploader.use_file }.to raise_error('exclusive lease already taken') end end + + it 'can still migrate other files of the same model' do + uploader2 = uploader_class.new(object, :file) + uploader2.upload = create(:upload) + uploader.upload = create(:upload) + + when_file_is_in_use do + expect(uploader2).to receive(:unsafe_migrate!) + + uploader2.migrate!(described_class::Store::REMOTE) + end + end end describe '#fog_credentials' do -- cgit v1.2.1 From cdd7dfc448fcb6bbcdb62aaac65a3af797f0f69a Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Tue, 12 Jun 2018 09:33:26 +0200 Subject: Override exclusive_lease_key method in RecordsUpload --- app/uploaders/object_storage.rb | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index d8ec5a81968..f5b466a1aba 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -62,6 +62,15 @@ module ObjectStorage upload.id) end + def exclusive_lease_key + # For FileUploaders, model may have many uploaders. In that case + # we want to use exclusive key per upload, not per model to allow + # parallel migration + key_object = upload ? upload : model + + "object_storage_migrate:#{key_object.class}:#{key_object.id}" + end + private def current_upload_satisfies?(paths, model) @@ -299,12 +308,7 @@ module ObjectStorage end def exclusive_lease_key - # For FileUploaders, model may have many uploaders. In that case - # we want to use exclusive key per upload, not per model to allow - # parallel migration - key_object = self.is_a?(RecordsUploads::Concern) && upload ? upload : model - - "object_storage_migrate:#{key_object.class}:#{key_object.id}" + "object_storage_migrate:#{model.class}:#{model.id}" end private -- cgit v1.2.1 From 39ed07a816c9f732715b835bd363057a315f6ee7 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 12 Jun 2018 09:38:04 +0100 Subject: karma updates --- app/assets/stylesheets/pages/repo.scss | 8 ++++---- spec/javascripts/ide/components/commit_sidebar/list_item_spec.js | 1 + spec/javascripts/ide/components/commit_sidebar/list_spec.js | 1 + spec/javascripts/ide/components/repo_commit_section_spec.js | 1 + 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/app/assets/stylesheets/pages/repo.scss b/app/assets/stylesheets/pages/repo.scss index 7d277b13224..f0131579b9e 100644 --- a/app/assets/stylesheets/pages/repo.scss +++ b/app/assets/stylesheets/pages/repo.scss @@ -553,6 +553,10 @@ } .multi-file-commit-list-item { + &.is-active { + background-color: $white-normal; + } + .multi-file-discard-btn { display: none; margin-top: -2px; @@ -612,10 +616,6 @@ } } -.multi-file-commit-list-item.is-active { - background-color: $white-normal; -} - .multi-file-commit-list-path { padding: 0; background: none; diff --git a/spec/javascripts/ide/components/commit_sidebar/list_item_spec.js b/spec/javascripts/ide/components/commit_sidebar/list_item_spec.js index cc7e0a3f26d..a732028158e 100644 --- a/spec/javascripts/ide/components/commit_sidebar/list_item_spec.js +++ b/spec/javascripts/ide/components/commit_sidebar/list_item_spec.js @@ -19,6 +19,7 @@ describe('Multi-file editor commit sidebar list item', () => { vm = createComponentWithStore(Component, store, { file: f, actionComponent: 'stage-button', + activeFileKey: `staged-${f.key}`, }).$mount(); }); diff --git a/spec/javascripts/ide/components/commit_sidebar/list_spec.js b/spec/javascripts/ide/components/commit_sidebar/list_spec.js index 54625ef90f8..a37cbd5a596 100644 --- a/spec/javascripts/ide/components/commit_sidebar/list_spec.js +++ b/spec/javascripts/ide/components/commit_sidebar/list_spec.js @@ -17,6 +17,7 @@ describe('Multi-file editor commit sidebar list', () => { action: 'stageAllChanges', actionBtnText: 'stage all', itemActionComponent: 'stage-button', + activeFileKey: 'staged-testing', }); vm.$store.state.rightPanelCollapsed = false; diff --git a/spec/javascripts/ide/components/repo_commit_section_spec.js b/spec/javascripts/ide/components/repo_commit_section_spec.js index 5e3e00a180b..0934c239a8e 100644 --- a/spec/javascripts/ide/components/repo_commit_section_spec.js +++ b/spec/javascripts/ide/components/repo_commit_section_spec.js @@ -98,6 +98,7 @@ describe('RepoCommitSection', () => { store.state.noChangesStateSvgPath = 'nochangessvg'; store.state.committedStateSvgPath = 'svg'; + vm.$destroy(); vm = createComponentWithStore(Component, store).$mount(); expect(vm.$el.querySelector('.js-empty-state').textContent.trim()).toContain('No changes'); -- cgit v1.2.1 From e1ec70cf8e4ca7964eb12e6d1a911285c57a3892 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 12 Jun 2018 10:02:15 +0100 Subject: more karma fixes --- app/assets/javascripts/ide/components/commit_sidebar/list.vue | 3 ++- app/assets/javascripts/ide/components/commit_sidebar/list_item.vue | 3 ++- app/assets/javascripts/ide/components/repo_commit_section.vue | 7 +++++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/ide/components/commit_sidebar/list.vue b/app/assets/javascripts/ide/components/commit_sidebar/list.vue index c107462e335..5a8566d3121 100644 --- a/app/assets/javascripts/ide/components/commit_sidebar/list.vue +++ b/app/assets/javascripts/ide/components/commit_sidebar/list.vue @@ -45,7 +45,8 @@ export default { }, activeFileKey: { type: String, - required: true, + required: false, + default: null, }, }, data() { diff --git a/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue b/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue index b69a9a833a5..95f332de10d 100644 --- a/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue +++ b/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue @@ -32,7 +32,8 @@ export default { }, activeFileKey: { type: String, - required: true, + required: false, + default: null, }, }, computed: { diff --git a/app/assets/javascripts/ide/components/repo_commit_section.vue b/app/assets/javascripts/ide/components/repo_commit_section.vue index 8ed182ff378..b369531176b 100644 --- a/app/assets/javascripts/ide/components/repo_commit_section.vue +++ b/app/assets/javascripts/ide/components/repo_commit_section.vue @@ -32,6 +32,9 @@ export default { showStageUnstageArea() { return !!(this.someUncommitedChanges || this.lastCommitMsg || !this.unusedSeal); }, + activeFileKey() { + return this.activeFile ? this.activeFile.key : null; + }, }, watch: { hasChanges() { @@ -93,7 +96,7 @@ export default { action="stageAllChanges" :action-btn-text="__('Stage all')" item-action-component="stage-button" - :active-file-key="activeFile.key" + :active-file-key="activeFileKey" /> Date: Tue, 12 Jun 2018 11:46:00 +0100 Subject: added specs for is-active class added spec for openPendingTab in component --- .../ide/components/commit_sidebar/list_item_spec.js | 16 ++++++++++++++++ .../ide/components/repo_commit_section_spec.js | 9 +++++++++ 2 files changed, 25 insertions(+) diff --git a/spec/javascripts/ide/components/commit_sidebar/list_item_spec.js b/spec/javascripts/ide/components/commit_sidebar/list_item_spec.js index a732028158e..8f7cf24c22f 100644 --- a/spec/javascripts/ide/components/commit_sidebar/list_item_spec.js +++ b/spec/javascripts/ide/components/commit_sidebar/list_item_spec.js @@ -90,4 +90,20 @@ describe('Multi-file editor commit sidebar list item', () => { }); }); }); + + describe('is active', () => { + it('does not add active class when dont keys match', () => { + expect(vm.$el.classList).not.toContain('is-active'); + }); + + it('adds active class when keys match', done => { + vm.keyPrefix = 'staged'; + + vm.$nextTick(() => { + expect(vm.$el.classList).toContain('is-active'); + + done(); + }); + }); + }); }); diff --git a/spec/javascripts/ide/components/repo_commit_section_spec.js b/spec/javascripts/ide/components/repo_commit_section_spec.js index 0934c239a8e..de00a372ed4 100644 --- a/spec/javascripts/ide/components/repo_commit_section_spec.js +++ b/spec/javascripts/ide/components/repo_commit_section_spec.js @@ -56,6 +56,8 @@ describe('RepoCommitSection', () => { vm.$store.state.entries[f.path] = f; }); + spyOn(vm, 'openPendingTab').and.callThrough(); + return vm.$mount(); } @@ -177,5 +179,12 @@ describe('RepoCommitSection', () => { expect(store.state.openFiles.length).toBe(1); expect(store.state.openFiles[0].pending).toBe(true); }); + + it('calls openPendingTab', () => { + expect(vm.openPendingTab).toHaveBeenCalledWith({ + file: vm.lastOpenedFile, + keyPrefix: 'unstaged', + }); + }); }); }); -- cgit v1.2.1 From b2cb0c6cbdd7ef46f68e824174df35356636b307 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 12 Jun 2018 12:19:59 +0100 Subject: fixed eslint --- spec/javascripts/ide/components/repo_commit_section_spec.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/spec/javascripts/ide/components/repo_commit_section_spec.js b/spec/javascripts/ide/components/repo_commit_section_spec.js index de00a372ed4..531bcd6e540 100644 --- a/spec/javascripts/ide/components/repo_commit_section_spec.js +++ b/spec/javascripts/ide/components/repo_commit_section_spec.js @@ -56,9 +56,7 @@ describe('RepoCommitSection', () => { vm.$store.state.entries[f.path] = f; }); - spyOn(vm, 'openPendingTab').and.callThrough(); - - return vm.$mount(); + return vm; } beforeEach(done => { @@ -66,6 +64,10 @@ describe('RepoCommitSection', () => { vm = createComponent(); + spyOn(vm, 'openPendingTab').and.callThrough(); + + vm.$mount(); + spyOn(service, 'getTreeData').and.returnValue( Promise.resolve({ headers: { -- cgit v1.2.1 From 3d42bab71ad293c99d029dfb4f0c9aa0378643d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Tue, 12 Jun 2018 11:22:35 -0400 Subject: apply feedback --- app/uploaders/object_storage.rb | 3 +- .../object_storage/migrate_uploads_worker_spec.rb | 34 +++++++++------------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 43c2b419332..14983943f76 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -16,7 +16,8 @@ module ObjectStorage end def message - "Exclusive lease #{@lease_key} already taken." + *lease_key_group, _ = *@lease_key.split(":") + "Exclusive lease for #{lease_key_group.join(':')} is already taken." end end diff --git a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb index 31d323626c5..da490cb02af 100644 --- a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb +++ b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb @@ -1,7 +1,5 @@ require 'spec_helper' -MIGRATION_QUERIES = 5 - describe ObjectStorage::MigrateUploadsWorker, :sidekiq do shared_context 'sanity_check! fails' do before do @@ -127,13 +125,12 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do it_behaves_like "uploads migration worker" describe "limits N+1 queries" do - let!(:projects) { create_list(:project, 10, :with_avatar) } - - it "to N*#{MIGRATION_QUERIES}" do + it "to N*5" do query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } - more_projects = create_list(:project, 100, :with_avatar) - expected_queries_per_migration = MIGRATION_QUERIES * more_projects.count + more_projects = create_list(:project, 3, :with_avatar) + + expected_queries_per_migration = 5 * more_projects.count expect { perform(Upload.all) }.not_to exceed_query_limit(query_count).with_threshold(expected_queries_per_migration) end end @@ -144,30 +141,27 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do let(:secret) { SecureRandom.hex } let(:mounted_as) { nil } + def upload_file(project) + uploader = FileUploader.new(project) + uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt')) + end + before do stub_uploads_object_storage(FileUploader) - projects.map do |project| - uploader = FileUploader.new(project) - uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt')) - end + projects.map(&method(:upload_file)) end it_behaves_like "uploads migration worker" describe "limits N+1 queries" do - let!(:projects) { create_list(:project, 10) } - - it "to N*#{MIGRATION_QUERIES}" do + it "to N*5" do query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } - more_projects = create_list(:project, 100) - more_projects.map do |project| - uploader = FileUploader.new(project) - uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt')) - end - expected_queries_per_migration = MIGRATION_QUERIES * more_projects.count + more_projects = create_list(:project, 3) + more_projects.map(&method(:upload_file)) + expected_queries_per_migration = 5 * more_projects.count expect { perform(Upload.all) }.not_to exceed_query_limit(query_count).with_threshold(expected_queries_per_migration) end end -- cgit v1.2.1 From 7530e4ec86a686dede8eb8535fd231c1a75d3659 Mon Sep 17 00:00:00 2001 From: Mario de la Ossa Date: Tue, 12 Jun 2018 14:35:45 -0600 Subject: Fix Banzai reference for milestones belonging to parent groups --- lib/banzai/filter/milestone_reference_filter.rb | 2 +- spec/lib/banzai/filter/milestone_reference_filter_spec.rb | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/banzai/filter/milestone_reference_filter.rb b/lib/banzai/filter/milestone_reference_filter.rb index b144bd8cf54..858e790005c 100644 --- a/lib/banzai/filter/milestone_reference_filter.rb +++ b/lib/banzai/filter/milestone_reference_filter.rb @@ -65,7 +65,7 @@ module Banzai # We don't support IID lookups for group milestones, because IIDs can # clash between group and project milestones. if project.group && !params[:iid] - finder_params[:group_ids] = [project.group.id] + finder_params[:group_ids] = project.group.self_and_ancestors.select(:id) end MilestonesFinder.new(finder_params).find_by(params) diff --git a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb index f8fa9b2d13d..91d4a60ba95 100644 --- a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb @@ -3,7 +3,8 @@ require 'spec_helper' describe Banzai::Filter::MilestoneReferenceFilter do include FilterSpecHelper - let(:group) { create(:group, :public) } + let(:parent_group) { create(:group, :public) } + let(:group) { create(:group, :public, parent: parent_group) } let(:project) { create(:project, :public, group: group) } it 'requires project context' do @@ -340,6 +341,13 @@ describe Banzai::Filter::MilestoneReferenceFilter do expect(doc.css('a')).to be_empty end + + it 'supports parent group references', :nested_groups do + milestone.update!(group: parent_group) + + doc = reference_filter("See #{reference}") + expect(doc.css('a').first.text).to eq(milestone.name) + end end context 'group context' do -- cgit v1.2.1 From 379a6a709a55f952133354febddf97854c2d5d4d Mon Sep 17 00:00:00 2001 From: Mario de la Ossa Date: Tue, 12 Jun 2018 16:40:16 -0600 Subject: Sidebar Milestone - Fix wrong URL when selecting a parent group milestone --- app/assets/javascripts/milestone_select.js | 2 +- lib/api/entities.rb | 4 ++++ lib/gitlab/url_builder.rb | 2 ++ .../api/schemas/public_api/v4/milestones.json | 3 ++- spec/lib/gitlab/url_builder_spec.rb | 25 ++++++++++++++++++++++ 5 files changed, 34 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/milestone_select.js b/app/assets/javascripts/milestone_select.js index f8b3d3061f0..d269c45203a 100644 --- a/app/assets/javascripts/milestone_select.js +++ b/app/assets/javascripts/milestone_select.js @@ -56,7 +56,7 @@ export default class MilestoneSelect { if (issueUpdateURL) { milestoneLinkTemplate = _.template( - '<%- title %>', + '<%- title %>', ); milestoneLinkNoneTemplate = 'None'; } diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 22afcb9edf2..3c9c87ac1c4 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -412,6 +412,10 @@ module API expose :state, :created_at, :updated_at expose :due_date expose :start_date + + expose :web_url do |milestone, _options| + Gitlab::UrlBuilder.build(milestone) + end end class IssueBasic < ProjectEntity diff --git a/lib/gitlab/url_builder.rb b/lib/gitlab/url_builder.rb index 824e2d7251f..e64033b0dba 100644 --- a/lib/gitlab/url_builder.rb +++ b/lib/gitlab/url_builder.rb @@ -26,6 +26,8 @@ module Gitlab project_snippet_url(object.project, object) when Snippet snippet_url(object) + when Milestone + milestone_url(object) else raise NotImplementedError.new("No URL builder defined for #{object.class}") end diff --git a/spec/fixtures/api/schemas/public_api/v4/milestones.json b/spec/fixtures/api/schemas/public_api/v4/milestones.json index c3c42b6ee60..448e97d6c85 100644 --- a/spec/fixtures/api/schemas/public_api/v4/milestones.json +++ b/spec/fixtures/api/schemas/public_api/v4/milestones.json @@ -13,7 +13,8 @@ "created_at": { "type": "date" }, "updated_at": { "type": "date" }, "start_date": { "type": "date" }, - "due_date": { "type": "date" } + "due_date": { "type": "date" }, + "web_url": { "type": "string" } }, "required": [ "id", "iid", "title", "description", "state", diff --git a/spec/lib/gitlab/url_builder_spec.rb b/spec/lib/gitlab/url_builder_spec.rb index b81749cf428..9f495a5d50b 100644 --- a/spec/lib/gitlab/url_builder_spec.rb +++ b/spec/lib/gitlab/url_builder_spec.rb @@ -22,6 +22,31 @@ describe Gitlab::UrlBuilder do end end + context 'when passing a Milestone' do + let(:group) { create(:group) } + let(:project) { create(:project, :public, namespace: group) } + + context 'belonging to a project' do + it 'returns a proper URL' do + milestone = create(:milestone, project: project) + + url = described_class.build(milestone) + + expect(url).to eq "#{Settings.gitlab['url']}/#{milestone.project.full_path}/milestones/#{milestone.iid}" + end + end + + context 'belonging to a group' do + it 'returns a proper URL' do + milestone = create(:milestone, group: group) + + url = described_class.build(milestone) + + expect(url).to eq "#{Settings.gitlab['url']}/groups/#{milestone.group.full_path}/-/milestones/#{milestone.iid}" + end + end + end + context 'when passing a MergeRequest' do it 'returns a proper URL' do merge_request = build_stubbed(:merge_request, iid: 42) -- cgit v1.2.1 From 1499b02f73a6d8800a8132ea7b19a7e97fff1b4e Mon Sep 17 00:00:00 2001 From: blackst0ne Date: Wed, 13 Jun 2018 14:12:38 +1100 Subject: [Rails5] Pass class references instead of strings to middleware builder It fixes Rails 5.0 deprecation flooding like: ``` DEPRECATION WARNING: Passing strings or symbols to the middleware builder is deprecated, please change them to actual class references. For example: "::Gitlab::Middleware::ReadOnly" => Gitlab::Middleware::ReadOnly (called from at /builds/gitlab-org/gitlab-ce/config/environment.rb:11) DEPRECATION WARNING: Passing strings or symbols to the middleware builder is deprecated, please change them to actual class references. For example: "ActionDispatch::Static" => ActionDispatch::Static (called from at /builds/gitlab-org/gitlab-ce/config/environment.rb:11) DEPRECATION WARNING: Passing strings or symbols to the middleware builder is deprecated, please change them to actual class references. For example: "Gitlab::Testing::RequestBlockerMiddleware" => Gitlab::Testing::RequestBlockerMiddleware (called from at /builds/gitlab-org/gitlab-ce/config/environment.rb:11) DEPRECATION WARNING: Passing strings or symbols to the middleware builder is deprecated, please change them to actual class references. For example: "ActionDispatch::Static" => ActionDispatch::Static (called from at /builds/gitlab-org/gitlab-ce/config/environment.rb:11) DEPRECATION WARNING: Passing strings or symbols to the middleware builder is deprecated, please change them to actual class references. For example: "Gitlab::Testing::RequestInspectorMiddleware" => Gitlab::Testing::RequestInspectorMiddleware (called from at /builds/gitlab-org/gitlab-ce/config/environment.rb:11) ``` --- config/application.rb | 3 ++- config/environments/test.rb | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/config/application.rb b/config/application.rb index d379d611074..95f6d2c9af1 100644 --- a/config/application.rb +++ b/config/application.rb @@ -12,6 +12,7 @@ module Gitlab require_dependency Rails.root.join('lib/gitlab/redis/shared_state') require_dependency Rails.root.join('lib/gitlab/request_context') require_dependency Rails.root.join('lib/gitlab/current_settings') + require_dependency Rails.root.join('lib/gitlab/middleware/read_only') # Settings in config/environments/* take precedence over those specified here. # Application configuration should go into files in config/initializers @@ -175,7 +176,7 @@ module Gitlab ENV['GIT_TERMINAL_PROMPT'] = '0' # Gitlab Read-only middleware support - config.middleware.insert_after ActionDispatch::Flash, '::Gitlab::Middleware::ReadOnly' + config.middleware.insert_after ActionDispatch::Flash, ::Gitlab::Middleware::ReadOnly config.generators do |g| g.factory_bot false diff --git a/config/environments/test.rb b/config/environments/test.rb index 1849c984351..af1011a1ab1 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -1,7 +1,7 @@ Rails.application.configure do # Make sure the middleware is inserted first in middleware chain - config.middleware.insert_before('ActionDispatch::Static', 'Gitlab::Testing::RequestBlockerMiddleware') - config.middleware.insert_before('ActionDispatch::Static', 'Gitlab::Testing::RequestInspectorMiddleware') + config.middleware.insert_before(ActionDispatch::Static, Gitlab::Testing::RequestBlockerMiddleware) + config.middleware.insert_before(ActionDispatch::Static, Gitlab::Testing::RequestInspectorMiddleware) # Settings specified here will take precedence over those in config/application.rb -- cgit v1.2.1 From be16f54d9d167c6338367c8175aaf135c44dab94 Mon Sep 17 00:00:00 2001 From: Jasper Maes Date: Wed, 13 Jun 2018 07:48:05 +0200 Subject: Rails5 fix expected `issuable.reload.updated_at` to have changed --- app/models/timelog.rb | 5 +++++ changelogs/unreleased/rails5-fix-47366.yml | 5 +++++ 2 files changed, 10 insertions(+) create mode 100644 changelogs/unreleased/rails5-fix-47366.yml diff --git a/app/models/timelog.rb b/app/models/timelog.rb index f4c5c581a11..659146f43e4 100644 --- a/app/models/timelog.rb +++ b/app/models/timelog.rb @@ -19,4 +19,9 @@ class Timelog < ActiveRecord::Base errors.add(:base, 'Issue or Merge Request ID is required') end end + + # Rails5 defaults to :touch_later, overwrite for normal touch + def belongs_to_touch_method + :touch + end end diff --git a/changelogs/unreleased/rails5-fix-47366.yml b/changelogs/unreleased/rails5-fix-47366.yml new file mode 100644 index 00000000000..7ea03d2b95e --- /dev/null +++ b/changelogs/unreleased/rails5-fix-47366.yml @@ -0,0 +1,5 @@ +--- +title: Rails5 fix expected `issuable.reload.updated_at` to have changed +merge_request: 19733 +author: Jasper Maes +type: fixed -- cgit v1.2.1 From c24bca94fe40a97572179178fd5b5bace005d5a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 11 Jun 2018 18:27:08 +0200 Subject: Ensure we look into the correct setion only when expanding a settings' section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/views/admin/application_settings/show.html.haml | 2 +- app/views/projects/deploy_keys/_index.html.haml | 2 +- app/views/projects/edit.html.haml | 4 ++-- .../protected_branches/shared/_index.html.haml | 2 +- app/views/projects/settings/ci_cd/show.html.haml | 6 +++--- qa/qa/page/admin/settings/main.rb | 4 ++-- qa/qa/page/project/settings/advanced.rb | 6 +++--- qa/qa/page/project/settings/ci_cd.rb | 18 ++++++++++-------- qa/qa/page/project/settings/main.rb | 4 ++-- qa/qa/page/project/settings/merge_request.rb | 12 ++++++------ qa/qa/page/project/settings/repository.rb | 10 +++++++--- qa/qa/page/settings/common.rb | 20 +++++++++----------- 12 files changed, 47 insertions(+), 43 deletions(-) diff --git a/app/views/admin/application_settings/show.html.haml b/app/views/admin/application_settings/show.html.haml index cb8c22ff076..38607ffca1c 100644 --- a/app/views/admin/application_settings/show.html.haml +++ b/app/views/admin/application_settings/show.html.haml @@ -169,7 +169,7 @@ .settings-content = render 'logging' -%section.settings.as-repository-storage.no-animate#js-repository-storage-settings{ class: ('expanded' if expanded) } +%section.qa-repository-storage-settings.settings.as-repository-storage.no-animate#js-repository-storage-settings{ class: ('expanded' if expanded) } .settings-header %h4 = _('Repository storage') diff --git a/app/views/projects/deploy_keys/_index.html.haml b/app/views/projects/deploy_keys/_index.html.haml index 6af57d3ab26..fb1ea471dec 100644 --- a/app/views/projects/deploy_keys/_index.html.haml +++ b/app/views/projects/deploy_keys/_index.html.haml @@ -1,5 +1,5 @@ - expanded = Rails.env.test? -%section.settings.no-animate{ class: ('expanded' if expanded) } +%section.qa-deploy-keys-settings.settings.no-animate{ class: ('expanded' if expanded) } .settings-header %h4 Deploy Keys diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 77665a2ac23..9f175d2376f 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -82,7 +82,7 @@ = render_if_exists 'projects/issues_settings' - %section.settings.merge-requests-feature.no-animate{ class: [('expanded' if expanded), ('hidden' if @project.project_feature.send(:merge_requests_access_level) == 0)] } + %section.qa-merge-request-settings.settings.merge-requests-feature.no-animate{ class: [('expanded' if expanded), ('hidden' if @project.project_feature.send(:merge_requests_access_level) == 0)] } .settings-header %h4 Merge request @@ -101,7 +101,7 @@ = render 'export', project: @project - %section.settings.advanced-settings.no-animate{ class: ('expanded' if expanded) } + %section.qa-advanced-settings.settings.advanced-settings.no-animate{ class: ('expanded' if expanded) } .settings-header %h4 Advanced diff --git a/app/views/projects/protected_branches/shared/_index.html.haml b/app/views/projects/protected_branches/shared/_index.html.haml index 846f8858d14..4f1c6c92484 100644 --- a/app/views/projects/protected_branches/shared/_index.html.haml +++ b/app/views/projects/protected_branches/shared/_index.html.haml @@ -1,6 +1,6 @@ - expanded = Rails.env.test? -%section.settings.no-animate{ class: ('expanded' if expanded) } +%section.qa-protected-branches-settings.settings.no-animate{ class: ('expanded' if expanded) } .settings-header %h4 Protected Branches diff --git a/app/views/projects/settings/ci_cd/show.html.haml b/app/views/projects/settings/ci_cd/show.html.haml index 3047207bca7..56c175f5649 100644 --- a/app/views/projects/settings/ci_cd/show.html.haml +++ b/app/views/projects/settings/ci_cd/show.html.haml @@ -16,7 +16,7 @@ .settings-content = render 'form' -%section.settings#autodevops-settings.no-animate{ class: ('expanded' if expanded) } +%section.qa-autodevops-settings.settings#autodevops-settings.no-animate{ class: ('expanded' if expanded) } .settings-header %h4 = s_('CICD|Auto DevOps') @@ -28,7 +28,7 @@ .settings-content = render 'autodevops_form' -%section.settings.no-animate{ class: ('expanded' if expanded) } +%section.qa-runners-settings.settings.no-animate{ class: ('expanded' if expanded) } .settings-header %h4 Runners @@ -39,7 +39,7 @@ .settings-content = render 'projects/runners/index' -%section.settings.no-animate{ class: ('expanded' if expanded) } +%section.qa-variables-settings.settings.no-animate{ class: ('expanded' if expanded) } .settings-header %h4 = _('Variables') diff --git a/qa/qa/page/admin/settings/main.rb b/qa/qa/page/admin/settings/main.rb index e7c1220c967..db3387b4557 100644 --- a/qa/qa/page/admin/settings/main.rb +++ b/qa/qa/page/admin/settings/main.rb @@ -6,11 +6,11 @@ module QA include QA::Page::Settings::Common view 'app/views/admin/application_settings/show.html.haml' do - element :advanced_settings_section, 'Repository storage' + element :repository_storage_settings end def expand_repository_storage(&block) - expand_section('Repository storage') do + expand_section(:repository_storage_settings) do RepositoryStorage.perform(&block) end end diff --git a/qa/qa/page/project/settings/advanced.rb b/qa/qa/page/project/settings/advanced.rb index 5ef00504fdf..d7b2b66b587 100644 --- a/qa/qa/page/project/settings/advanced.rb +++ b/qa/qa/page/project/settings/advanced.rb @@ -4,9 +4,9 @@ module QA module Settings class Advanced < Page::Base view 'app/views/projects/edit.html.haml' do - element :project_path_field, 'f.text_field :path' - element :project_name_field, 'f.text_field :name' - element :rename_project_button, "f.submit 'Rename project'" + element :project_path_field, 'text_field :path' + element :project_name_field, 'text_field :name' + element :rename_project_button, "submit 'Rename project'" end def rename_to(path) diff --git a/qa/qa/page/project/settings/ci_cd.rb b/qa/qa/page/project/settings/ci_cd.rb index d5da9ea0099..1466bc2e0bf 100644 --- a/qa/qa/page/project/settings/ci_cd.rb +++ b/qa/qa/page/project/settings/ci_cd.rb @@ -6,31 +6,33 @@ module QA # rubocop:disable Naming/FileName include Common view 'app/views/projects/settings/ci_cd/show.html.haml' do - element :runners_settings, 'Runners' - element :secret_variables, 'Variables' - element :auto_devops_section, 'Auto DevOps' + element :autodevops_settings + element :runners_settings + element :variables_settings end view 'app/views/projects/settings/ci_cd/_autodevops_form.html.haml' do - element :enable_auto_devops_button, 'Enable Auto DevOps' - element :domain_input, 'Domain' + element :enable_auto_devops_field, 'radio_button :enabled' + element :domain_field, 'text_field :domain' + element :enable_auto_devops_button, "%strong= s_('CICD|Enable Auto DevOps')" + element :domain_input, "%strong= _('Domain')" element :save_changes_button, "submit 'Save changes'" end def expand_runners_settings(&block) - expand_section('Runners') do + expand_section(:runners_settings) do Settings::Runners.perform(&block) end end def expand_secret_variables(&block) - expand_section('Variables') do + expand_section(:variables_settings) do Settings::SecretVariables.perform(&block) end end def enable_auto_devops_with_domain(domain) - expand_section('Auto DevOps') do + expand_section(:autodevops_settings) do choose 'Enable Auto DevOps' fill_in 'Domain', with: domain click_on 'Save changes' diff --git a/qa/qa/page/project/settings/main.rb b/qa/qa/page/project/settings/main.rb index e3faa76b966..d8cf1d49dd2 100644 --- a/qa/qa/page/project/settings/main.rb +++ b/qa/qa/page/project/settings/main.rb @@ -6,11 +6,11 @@ module QA include Common view 'app/views/projects/edit.html.haml' do - element :advanced_settings_section, 'Advanced' + element :advanced_settings end def expand_advanced_settings(&block) - expand_section('Advanced settings') do + expand_section(:advanced_settings) do Advanced.perform(&block) end end diff --git a/qa/qa/page/project/settings/merge_request.rb b/qa/qa/page/project/settings/merge_request.rb index 06d4937a4c8..d044d3715a9 100644 --- a/qa/qa/page/project/settings/merge_request.rb +++ b/qa/qa/page/project/settings/merge_request.rb @@ -5,17 +5,17 @@ module QA class MergeRequest < QA::Page::Base include Common - view 'app/views/projects/_merge_request_merge_method_settings.html.haml' do - element :radio_button_merge_ff - end - view 'app/views/projects/edit.html.haml' do - element :merge_request_settings, 'Merge request' + element :merge_request_settings element :save_merge_request_changes end + view 'app/views/projects/_merge_request_merge_method_settings.html.haml' do + element :radio_button_merge_ff + end + def enable_ff_only - expand_section('Merge request') do + expand_section(:merge_request_settings) do click_element :radio_button_merge_ff click_element :save_merge_request_changes end diff --git a/qa/qa/page/project/settings/repository.rb b/qa/qa/page/project/settings/repository.rb index 30900e74e90..1ed5f455a85 100644 --- a/qa/qa/page/project/settings/repository.rb +++ b/qa/qa/page/project/settings/repository.rb @@ -6,17 +6,21 @@ module QA include Common view 'app/views/projects/deploy_keys/_index.html.haml' do - element :deploy_keys_section, 'Deploy Keys' + element :deploy_keys_settings + end + + view 'app/views/projects/protected_branches/shared/_index.html.haml' do + element :protected_branches_settings end def expand_deploy_keys(&block) - expand_section('Deploy Keys') do + expand_section(:deploy_keys_settings) do DeployKeys.perform(&block) end end def expand_protected_branches(&block) - expand_section('Protected Branches') do + expand_section(:protected_branches_settings) do ProtectedBranches.perform(&block) end end diff --git a/qa/qa/page/settings/common.rb b/qa/qa/page/settings/common.rb index a683a6829d5..f9f71aa4a72 100644 --- a/qa/qa/page/settings/common.rb +++ b/qa/qa/page/settings/common.rb @@ -4,19 +4,17 @@ module QA module Common # Click the Expand button present in the specified section # - # @param [String] name present in the container in the DOM - def expand_section(name) - page.within('#content-body') do - page.within('section', text: name) do - # Because it is possible to click the button before the JS toggle code is bound - wait(reload: false) do - click_button 'Expand' unless first('button', text: 'Collapse') + # @param [Symbol] and `element` name defined in a `view` block + def expand_section(element_name) + within_element(element_name) do + # Because it is possible to click the button before the JS toggle code is bound + wait(reload: false) do + click_button 'Expand' unless first('button', text: 'Collapse') - page.has_content?('Collapse') - end - - yield if block_given? + page.has_content?('Collapse') end + + yield if block_given? end end end -- cgit v1.2.1 From 2582b90ca7df8a0678c1b163fe6fdebd231c1958 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 12 Jun 2018 11:27:58 +0200 Subject: Loosen the 'newly created MR' matcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- qa/qa/specs/features/merge_request/create_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qa/qa/specs/features/merge_request/create_spec.rb b/qa/qa/specs/features/merge_request/create_spec.rb index 0931e649e24..befbc0b281a 100644 --- a/qa/qa/specs/features/merge_request/create_spec.rb +++ b/qa/qa/specs/features/merge_request/create_spec.rb @@ -11,7 +11,7 @@ module QA expect(page).to have_content('This is a merge request') expect(page).to have_content('Great feature') - expect(page).to have_content(/Opened [\w\s]+ a minute ago/) + expect(page).to have_content(/Opened [\w\s]+ ago/) end end end -- cgit v1.2.1 From 3961407248c55f0524c8acfa154ace4ed33e087a Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Wed, 13 Jun 2018 10:44:14 +0200 Subject: fixed condition check --- app/uploaders/object_storage.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index f5b466a1aba..41fd5d2ee78 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -66,7 +66,7 @@ module ObjectStorage # For FileUploaders, model may have many uploaders. In that case # we want to use exclusive key per upload, not per model to allow # parallel migration - key_object = upload ? upload : model + key_object = upload || model "object_storage_migrate:#{key_object.class}:#{key_object.id}" end -- cgit v1.2.1 From f478db1a488c306391bc04fb1454aacfa0a78670 Mon Sep 17 00:00:00 2001 From: Ross Laird Date: Wed, 13 Jun 2018 08:45:06 +0000 Subject: Typo fix --- app/views/shared/empty_states/_wikis.html.haml | 2 +- locale/gitlab.pot | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/shared/empty_states/_wikis.html.haml b/app/views/shared/empty_states/_wikis.html.haml index fabb1f39a34..f1a41074c28 100644 --- a/app/views/shared/empty_states/_wikis.html.haml +++ b/app/views/shared/empty_states/_wikis.html.haml @@ -8,7 +8,7 @@ %h4 = s_('WikiEmpty|The wiki lets you write documentation for your project') %p.text-left - = s_("WikiEmpty|A wiki is where you can store all the details about your project. This can include why you've created it, it's principles, how to use it, and so on.") + = s_("WikiEmpty|A wiki is where you can store all the details about your project. This can include why you've created it, its principles, how to use it, and so on.") = create_link - elsif can?(current_user, :read_issue, @project) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 5aa6e5c05e6..22adfe48869 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4734,7 +4734,7 @@ msgstr "" msgid "WikiEmptyIssueMessage|issue tracker" msgstr "" -msgid "WikiEmpty|A wiki is where you can store all the details about your project. This can include why you've created it, it's principles, how to use it, and so on." +msgid "WikiEmpty|A wiki is where you can store all the details about your project. This can include why you've created it, its principles, how to use it, and so on." msgstr "" msgid "WikiEmpty|Create your first page" -- cgit v1.2.1 From 9d9d952c1172e2b68bd4537a0ce05f96eae7b340 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 13 Jun 2018 10:10:24 +0100 Subject: moved strings into constants file --- app/assets/javascripts/ide/components/commit_sidebar/list.vue | 6 +++++- app/assets/javascripts/ide/components/commit_sidebar/list_item.vue | 4 ++-- app/assets/javascripts/ide/components/repo_commit_section.vue | 7 +++++-- app/assets/javascripts/ide/constants.js | 5 +++++ spec/javascripts/ide/components/commit_sidebar/list_spec.js | 1 + 5 files changed, 18 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/ide/components/commit_sidebar/list.vue b/app/assets/javascripts/ide/components/commit_sidebar/list.vue index 5a8566d3121..3d59410cbc2 100644 --- a/app/assets/javascripts/ide/components/commit_sidebar/list.vue +++ b/app/assets/javascripts/ide/components/commit_sidebar/list.vue @@ -48,6 +48,10 @@ export default { required: false, default: null, }, + keyPrefix: { + type: String, + required: true, + }, }, data() { return { @@ -118,7 +122,7 @@ export default { diff --git a/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue b/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue index 95f332de10d..6c30b2a721d 100644 --- a/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue +++ b/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue @@ -45,7 +45,7 @@ export default { return `multi-file-${this.file.tempFile ? 'addition' : 'modified'} append-right-8`; }, fullKey() { - return `${this.keyPrefix.toLowerCase()}-${this.file.key}`; + return `${this.keyPrefix}-${this.file.key}`; }, isActive() { return this.activeFileKey === this.fullKey; @@ -62,7 +62,7 @@ export default { openFileInEditor() { return this.openPendingTab({ file: this.file, - keyPrefix: this.keyPrefix.toLowerCase(), + keyPrefix: this.keyPrefix, }).then(changeViewer => { if (changeViewer) { this.updateViewer(viewerTypes.diff); diff --git a/app/assets/javascripts/ide/components/repo_commit_section.vue b/app/assets/javascripts/ide/components/repo_commit_section.vue index b369531176b..46e8ee26d3c 100644 --- a/app/assets/javascripts/ide/components/repo_commit_section.vue +++ b/app/assets/javascripts/ide/components/repo_commit_section.vue @@ -6,7 +6,7 @@ import DeprecatedModal from '~/vue_shared/components/deprecated_modal.vue'; import CommitFilesList from './commit_sidebar/list.vue'; import EmptyState from './commit_sidebar/empty_state.vue'; import * as consts from '../stores/modules/commit/constants'; -import { activityBarViews } from '../constants'; +import { activityBarViews, stageKeys } from '../constants'; export default { components: { @@ -47,7 +47,7 @@ export default { if (this.lastOpenedFile) { this.openPendingTab({ file: this.lastOpenedFile, - keyPrefix: this.lastOpenedFile.changed ? 'unstaged' : 'staged', + keyPrefix: this.lastOpenedFile.changed ? stageKeys.unstaged : stageKeys.staged, }) .then(changeViewer => { if (changeViewer) { @@ -66,6 +66,7 @@ export default { return this.updateCommitAction(consts.COMMIT_TO_NEW_BRANCH).then(() => this.commitChanges()); }, }, + stageKeys, }; @@ -92,6 +93,7 @@ export default { class="is-first" icon-name="unstaged" :title="__('Unstaged')" + :key-prefix="$options.stageKeys.unstaged" :file-list="changedFiles" action="stageAllChanges" :action-btn-text="__('Stage all')" @@ -101,6 +103,7 @@ export default { { actionBtnText: 'stage all', itemActionComponent: 'stage-button', activeFileKey: 'staged-testing', + keyPrefix: 'staged', }); vm.$store.state.rightPanelCollapsed = false; -- cgit v1.2.1 From 4835253172da1ac6d77ae6b82af85eae7f0b46c8 Mon Sep 17 00:00:00 2001 From: Victor Wu Date: Wed, 13 Jun 2018 09:30:33 +0000 Subject: Link to release post for group issue board docs --- doc/user/project/issue_board.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/user/project/issue_board.md b/doc/user/project/issue_board.md index 7eab825fa32..aa2fcd82787 100644 --- a/doc/user/project/issue_board.md +++ b/doc/user/project/issue_board.md @@ -237,13 +237,15 @@ Issue Board, that is create/delete lists and drag issues around. ## Group Issue Board ->Introduced in GitLab 10.6 +> Introduced in [GitLab 10.6](https://about.gitlab.com/2018/03/22/gitlab-10-6-released/#single-group-issue-board-in-core-and-free) Group issue board is analogous to project-level issue board and it is accessible at the group navigation level. A group-level issue board allows you to view all issues from all projects in that group or descendant subgroups. Similarly, you can only filter by group labels for these boards. When updating milestones and labels for an issue through the sidebar update mechanism, again only group-level objects are available. +One group issue board per group was made available in GitLab 10.6 Core after multiple group issue boards were originally introduced in [GitLab 10.0 Premium](https://about.gitlab.com/2017/09/22/gitlab-10-0-released/#group-issue-boards). + ## Features per tier Different issue board features are available in different [GitLab tiers](https://about.gitlab.com/pricing/), as shown in the following table: -- cgit v1.2.1 From 8a23bcc9bc0f7ab453ee09d41a9407d40d57ba4c Mon Sep 17 00:00:00 2001 From: Tim Zallmann Date: Wed, 13 Jun 2018 09:35:52 +0000 Subject: Image Diff Viewing + Download Diff Viewing --- .../javascripts/ide/components/repo_editor.vue | 31 +++- app/assets/javascripts/ide/constants.js | 7 + .../javascripts/ide/stores/mutations/file.js | 14 +- app/assets/javascripts/ide/stores/utils.js | 2 +- .../content_viewer/viewers/download_viewer.vue | 5 +- .../content_viewer/viewers/image_viewer.vue | 43 ++++- .../vue_shared/components/diff_viewer/constants.js | 12 ++ .../components/diff_viewer/diff_viewer.vue | 70 ++++++++ .../diff_viewer/viewers/download_diff_viewer.vue | 69 ++++++++ .../viewers/image_diff/onion_skin_viewer.vue | 160 ++++++++++++++++++ .../viewers/image_diff/swipe_viewer.vue | 158 ++++++++++++++++++ .../viewers/image_diff/two_up_viewer.vue | 41 +++++ .../diff_viewer/viewers/image_diff_viewer.vue | 109 ++++++++++++ .../vue_shared/components/lib/utils/dom_utils.js | 5 + app/assets/stylesheets/framework/files.scss | 48 ++++++ app/assets/stylesheets/pages/diff.scss | 132 +++++++++++++-- app/assets/stylesheets/pages/repo.scss | 1 - .../unreleased/tz-diff-blob-image-viewer.yml | 5 + spec/javascripts/fixtures/images/green_box.png | Bin 0 -> 1306 bytes spec/javascripts/fixtures/images/red_box.png | Bin 0 -> 1305 bytes spec/javascripts/ide/stores/mutations/file_spec.js | 47 ++++++ spec/javascripts/test_constants.js | 3 + .../content_viewer/content_viewer_spec.js | 10 +- .../components/diff_viewer/diff_viewer_spec.js | 70 ++++++++ .../diff_viewer/viewers/image_diff_viewer_spec.js | 185 +++++++++++++++++++++ .../components/lib/utils/dom_utils_spec.js | 13 ++ 26 files changed, 1204 insertions(+), 36 deletions(-) create mode 100644 app/assets/javascripts/vue_shared/components/diff_viewer/constants.js create mode 100644 app/assets/javascripts/vue_shared/components/diff_viewer/diff_viewer.vue create mode 100644 app/assets/javascripts/vue_shared/components/diff_viewer/viewers/download_diff_viewer.vue create mode 100644 app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/onion_skin_viewer.vue create mode 100644 app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/swipe_viewer.vue create mode 100644 app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/two_up_viewer.vue create mode 100644 app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer.vue create mode 100644 app/assets/javascripts/vue_shared/components/lib/utils/dom_utils.js create mode 100644 changelogs/unreleased/tz-diff-blob-image-viewer.yml create mode 100644 spec/javascripts/fixtures/images/green_box.png create mode 100644 spec/javascripts/fixtures/images/red_box.png create mode 100644 spec/javascripts/vue_shared/components/diff_viewer/diff_viewer_spec.js create mode 100644 spec/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer_spec.js create mode 100644 spec/javascripts/vue_shared/components/lib/utils/dom_utils_spec.js diff --git a/app/assets/javascripts/ide/components/repo_editor.vue b/app/assets/javascripts/ide/components/repo_editor.vue index d365745d78b..96f181ff79d 100644 --- a/app/assets/javascripts/ide/components/repo_editor.vue +++ b/app/assets/javascripts/ide/components/repo_editor.vue @@ -2,6 +2,7 @@ import { mapState, mapGetters, mapActions } from 'vuex'; import flash from '~/flash'; import ContentViewer from '~/vue_shared/components/content_viewer/content_viewer.vue'; +import DiffViewer from '~/vue_shared/components/diff_viewer/diff_viewer.vue'; import { activityBarViews, viewerTypes } from '../constants'; import Editor from '../lib/editor'; import ExternalLink from './external_link.vue'; @@ -9,6 +10,7 @@ import ExternalLink from './external_link.vue'; export default { components: { ContentViewer, + DiffViewer, ExternalLink, }, props: { @@ -29,9 +31,18 @@ export default { shouldHideEditor() { return this.file && this.file.binary && !this.file.content; }, + showContentViewer() { + return ( + (this.shouldHideEditor || this.file.viewMode === 'preview') && + (this.viewer !== viewerTypes.mr || !this.file.mrChange) + ); + }, + showDiffViewer() { + return this.shouldHideEditor && this.file.mrChange && this.viewer === viewerTypes.mr; + }, editTabCSS() { return { - active: this.file.viewMode === 'edit', + active: this.file.viewMode === 'editor', }; }, previewTabCSS() { @@ -53,7 +64,7 @@ export default { if (this.currentActivityView !== activityBarViews.edit) { this.setFileViewMode({ file: this.file, - viewMode: 'edit', + viewMode: 'editor', }); } } @@ -62,7 +73,7 @@ export default { if (this.currentActivityView !== activityBarViews.edit) { this.setFileViewMode({ file: this.file, - viewMode: 'edit', + viewMode: 'editor', }); } }, @@ -197,7 +208,7 @@ export default { + @click.prevent="setFileViewMode({ file, viewMode: 'editor' })"> @@ -222,7 +233,7 @@ export default { />
+
diff --git a/app/assets/javascripts/ide/constants.js b/app/assets/javascripts/ide/constants.js index 65886c02b92..64271ca56d1 100644 --- a/app/assets/javascripts/ide/constants.js +++ b/app/assets/javascripts/ide/constants.js @@ -21,6 +21,13 @@ export const viewerTypes = { diff: 'diff', }; +export const diffModes = { + replaced: 'replaced', + new: 'new', + deleted: 'deleted', + renamed: 'renamed', +}; + export const rightSidebarViews = { pipelines: 'pipelines-list', jobsDetail: 'jobs-detail', diff --git a/app/assets/javascripts/ide/stores/mutations/file.js b/app/assets/javascripts/ide/stores/mutations/file.js index 13f123b6630..5826f6cb828 100644 --- a/app/assets/javascripts/ide/stores/mutations/file.js +++ b/app/assets/javascripts/ide/stores/mutations/file.js @@ -1,5 +1,6 @@ /* eslint-disable no-param-reassign */ import * as types from '../mutation_types'; +import { diffModes } from '../../constants'; export default { [types.SET_FILE_ACTIVE](state, { path, active }) { @@ -85,8 +86,19 @@ export default { }); }, [types.SET_FILE_MERGE_REQUEST_CHANGE](state, { file, mrChange }) { + let diffMode = diffModes.replaced; + if (mrChange.new_file) { + diffMode = diffModes.new; + } else if (mrChange.deleted_file) { + diffMode = diffModes.deleted; + } else if (mrChange.renamed_file) { + diffMode = diffModes.renamed; + } Object.assign(state.entries[file.path], { - mrChange, + mrChange: { + ...mrChange, + diffMode, + }, }); }, [types.SET_FILE_VIEWMODE](state, { file, viewMode }) { diff --git a/app/assets/javascripts/ide/stores/utils.js b/app/assets/javascripts/ide/stores/utils.js index e0b9766fbee..a04a33cd12d 100644 --- a/app/assets/javascripts/ide/stores/utils.js +++ b/app/assets/javascripts/ide/stores/utils.js @@ -39,7 +39,7 @@ export const dataStructure = () => ({ editorColumn: 1, fileLanguage: '', eol: '', - viewMode: 'edit', + viewMode: 'editor', previewMode: null, size: 0, parentPath: null, diff --git a/app/assets/javascripts/vue_shared/components/content_viewer/viewers/download_viewer.vue b/app/assets/javascripts/vue_shared/components/content_viewer/viewers/download_viewer.vue index 7b5367ac19b..c274d3ab590 100644 --- a/app/assets/javascripts/vue_shared/components/content_viewer/viewers/download_viewer.vue +++ b/app/assets/javascripts/vue_shared/components/content_viewer/viewers/download_viewer.vue @@ -32,7 +32,10 @@ export default {

- {{ fileName }} ({{ fileSizeReadable }}) + {{ fileName }} +

+import _ from 'underscore'; import { numberToHumanSize } from '../../../../lib/utils/number_utils'; export default { @@ -12,6 +13,10 @@ export default { required: false, default: 0, }, + renderInfo: { + type: Boolean, + default: true, + }, }, data() { return { @@ -26,14 +31,34 @@ export default { return numberToHumanSize(this.fileSize); }, }, + beforeDestroy() { + window.removeEventListener('resize', this.resizeThrottled, false); + }, + mounted() { + // The onImgLoad may have happened before the control was actually mounted + this.onImgLoad(); + this.resizeThrottled = _.throttle(this.onImgLoad, 400); + window.addEventListener('resize', this.resizeThrottled, false); + }, methods: { onImgLoad() { const contentImg = this.$refs.contentImg; - this.isZoomable = - contentImg.naturalWidth > contentImg.width || contentImg.naturalHeight > contentImg.height; - this.width = contentImg.naturalWidth; - this.height = contentImg.naturalHeight; + if (contentImg) { + this.isZoomable = + contentImg.naturalWidth > contentImg.width || + contentImg.naturalHeight > contentImg.height; + + this.width = contentImg.naturalWidth; + this.height = contentImg.naturalHeight; + + this.$emit('imgLoaded', { + width: this.width, + height: this.height, + renderedWidth: contentImg.clientWidth, + renderedHeight: contentImg.clientHeight, + }); + } }, onImgClick() { if (this.isZoomable) this.isZoomed = !this.isZoomed; @@ -47,20 +72,22 @@ export default {
-

+

diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/constants.js b/app/assets/javascripts/vue_shared/components/diff_viewer/constants.js new file mode 100644 index 00000000000..6c1840361af --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/diff_viewer/constants.js @@ -0,0 +1,12 @@ +export const diffModes = { + replaced: 'replaced', + new: 'new', + deleted: 'deleted', + renamed: 'renamed', +}; + +export const imageViewMode = { + twoup: 'twoup', + swipe: 'swipe', + onion: 'onion', +}; diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/diff_viewer.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/diff_viewer.vue new file mode 100644 index 00000000000..4eca3fd4e97 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/diff_viewer/diff_viewer.vue @@ -0,0 +1,70 @@ + + + diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/download_diff_viewer.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/download_diff_viewer.vue new file mode 100644 index 00000000000..50389b6ae63 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/download_diff_viewer.vue @@ -0,0 +1,69 @@ + + + diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/onion_skin_viewer.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/onion_skin_viewer.vue new file mode 100644 index 00000000000..efcc39197b0 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/onion_skin_viewer.vue @@ -0,0 +1,160 @@ + + + diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/swipe_viewer.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/swipe_viewer.vue new file mode 100644 index 00000000000..fc513ebfce1 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/swipe_viewer.vue @@ -0,0 +1,158 @@ + + + diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/two_up_viewer.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/two_up_viewer.vue new file mode 100644 index 00000000000..9c19266ecdf --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/two_up_viewer.vue @@ -0,0 +1,41 @@ + + + diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer.vue new file mode 100644 index 00000000000..43b28f96a06 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer.vue @@ -0,0 +1,109 @@ + + + diff --git a/app/assets/javascripts/vue_shared/components/lib/utils/dom_utils.js b/app/assets/javascripts/vue_shared/components/lib/utils/dom_utils.js new file mode 100644 index 00000000000..02f28da8bb0 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/lib/utils/dom_utils.js @@ -0,0 +1,5 @@ +export function pixeliseValue(val) { + return val ? `${val}px` : ''; +} + +export default {}; diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss index f77ec4b6a2c..f060254777c 100644 --- a/app/assets/stylesheets/framework/files.scss +++ b/app/assets/stylesheets/framework/files.scss @@ -400,3 +400,51 @@ span.idiff { color: $common-gray-light; border: 1px solid $common-gray-light; } + +.preview-container { + height: 100%; + overflow: auto; + + .file-container { + background-color: $gray-darker; + display: flex; + height: 100%; + align-items: center; + justify-content: center; + + text-align: center; + + .file-content { + padding: $gl-padding; + max-width: 100%; + max-height: 100%; + + img { + max-width: 90%; + max-height: 70vh; + } + + .is-zoomable { + cursor: pointer; + cursor: zoom-in; + + &.is-zoomed { + cursor: pointer; + cursor: zoom-out; + max-width: none; + max-height: none; + margin-right: $gl-padding; + } + } + } + + .file-info { + font-size: $label-font-size; + color: $diff-image-info-color; + } + } + + .md-previewer { + padding: $gl-padding; + } +} diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index f06c9dcdf8c..fbc97ec0c95 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -189,8 +189,22 @@ img { border: 1px solid $white-light; - background-image: linear-gradient(45deg, $border-color 25%, transparent 25%, transparent 75%, $border-color 75%, $border-color 100%), - linear-gradient(45deg, $border-color 25%, transparent 25%, transparent 75%, $border-color 75%, $border-color 100%); + background-image: linear-gradient( + 45deg, + $border-color 25%, + transparent 25%, + transparent 75%, + $border-color 75%, + $border-color 100% + ), + linear-gradient( + 45deg, + $border-color 25%, + transparent 25%, + transparent 75%, + $border-color 75%, + $border-color 100% + ); background-size: 10px 10px; background-position: 0 0, 5px 5px; max-width: 100%; @@ -395,6 +409,69 @@ .line_content { white-space: pre-wrap; } + + .diff-file-container { + .frame.deleted { + border: 0; + background-color: inherit; + + .image_file img { + border: 1px solid $deleted; + } + } + + .frame.added { + border: 0; + background-color: inherit; + + .image_file img { + border: 1px solid $added; + } + } + + .swipe.view, + .onion-skin.view { + .swipe-wrap { + top: 0; + right: 0; + } + + .frame.deleted { + top: 0; + right: 0; + } + + .swipe-bar { + top: 0; + + .top-handle { + top: -14px; + left: -7px; + } + + .bottom-handle { + bottom: -14px; + left: -7px; + } + } + + .file-container { + display: inline-block; + + .file-content { + padding: 0; + + img { + max-width: none; + } + } + } + } + + .onion-skin.view .controls { + bottom: -25px; + } + } } .file-content .diff-file { @@ -536,7 +613,7 @@ margin-right: 0; border-color: $white-light; cursor: pointer; - transition: all .1s ease-out; + transition: all 0.1s ease-out; @for $i from 1 through 4 { &:nth-child(#{$i}) { @@ -563,7 +640,7 @@ height: 24px; border-radius: 50%; padding: 0; - transition: transform .1s ease-out; + transition: transform 0.1s ease-out; z-index: 100; .collapse-icon { @@ -708,11 +785,35 @@ width: 100%; height: 10px; background-color: $white-light; - background-image: linear-gradient(45deg, transparent, transparent 73%, $diff-jagged-border-gradient-color 75%, $white-light 80%), - linear-gradient(225deg, transparent, transparent 73%, $diff-jagged-border-gradient-color 75%, $white-light 80%), - linear-gradient(135deg, transparent, transparent 73%, $diff-jagged-border-gradient-color 75%, $white-light 80%), - linear-gradient(-45deg, transparent, transparent 73%, $diff-jagged-border-gradient-color 75%, $white-light 80%); - background-position: 5px 5px,0 5px,0 5px,5px 5px; + background-image: linear-gradient( + 45deg, + transparent, + transparent 73%, + $diff-jagged-border-gradient-color 75%, + $white-light 80% + ), + linear-gradient( + 225deg, + transparent, + transparent 73%, + $diff-jagged-border-gradient-color 75%, + $white-light 80% + ), + linear-gradient( + 135deg, + transparent, + transparent 73%, + $diff-jagged-border-gradient-color 75%, + $white-light 80% + ), + linear-gradient( + -45deg, + transparent, + transparent 73%, + $diff-jagged-border-gradient-color 75%, + $white-light 80% + ); + background-position: 5px 5px, 0 5px, 0 5px, 5px 5px; background-size: 10px 10px; background-repeat: repeat; } @@ -750,11 +851,16 @@ .frame.click-to-comment { position: relative; cursor: image-url('illustrations/image_comment_light_cursor.svg') - $image-comment-cursor-left-offset $image-comment-cursor-top-offset, auto; + $image-comment-cursor-left-offset $image-comment-cursor-top-offset, + auto; // Retina cursor - cursor: -webkit-image-set(image-url('illustrations/image_comment_light_cursor.svg') 1x, image-url('illustrations/image_comment_light_cursor@2x.svg') 2x) - $image-comment-cursor-left-offset $image-comment-cursor-top-offset, auto; + cursor: -webkit-image-set( + image-url('illustrations/image_comment_light_cursor.svg') 1x, + image-url('illustrations/image_comment_light_cursor@2x.svg') 2x + ) + $image-comment-cursor-left-offset $image-comment-cursor-top-offset, + auto; .comment-indicator { position: absolute; @@ -840,7 +946,7 @@ .diff-notes-collapse, .note, - .discussion-reply-holder, { + .discussion-reply-holder { display: none; } diff --git a/app/assets/stylesheets/pages/repo.scss b/app/assets/stylesheets/pages/repo.scss index 6e7fc50c63d..0fb95572cfd 100644 --- a/app/assets/stylesheets/pages/repo.scss +++ b/app/assets/stylesheets/pages/repo.scss @@ -335,7 +335,6 @@ img { max-width: 90%; - max-height: 90%; } .isZoomable { diff --git a/changelogs/unreleased/tz-diff-blob-image-viewer.yml b/changelogs/unreleased/tz-diff-blob-image-viewer.yml new file mode 100644 index 00000000000..81d87bc71f5 --- /dev/null +++ b/changelogs/unreleased/tz-diff-blob-image-viewer.yml @@ -0,0 +1,5 @@ +--- +title: Web IDE supports now Image + Download Diff Viewing +merge_request: 18768 +author: +type: added diff --git a/spec/javascripts/fixtures/images/green_box.png b/spec/javascripts/fixtures/images/green_box.png new file mode 100644 index 00000000000..cd1ff9f9ade Binary files /dev/null and b/spec/javascripts/fixtures/images/green_box.png differ diff --git a/spec/javascripts/fixtures/images/red_box.png b/spec/javascripts/fixtures/images/red_box.png new file mode 100644 index 00000000000..73b2927da0f Binary files /dev/null and b/spec/javascripts/fixtures/images/red_box.png differ diff --git a/spec/javascripts/ide/stores/mutations/file_spec.js b/spec/javascripts/ide/stores/mutations/file_spec.js index e83961fcedc..52f83be8e8c 100644 --- a/spec/javascripts/ide/stores/mutations/file_spec.js +++ b/spec/javascripts/ide/stores/mutations/file_spec.js @@ -152,6 +152,53 @@ describe('IDE store file mutations', () => { expect(localFile.mrChange.diff).toBe('ABC'); }); + + it('has diffMode replaced by default', () => { + mutations.SET_FILE_MERGE_REQUEST_CHANGE(localState, { + file: localFile, + mrChange: { + diff: 'ABC', + }, + }); + + expect(localFile.mrChange.diffMode).toBe('replaced'); + }); + + it('has diffMode new', () => { + mutations.SET_FILE_MERGE_REQUEST_CHANGE(localState, { + file: localFile, + mrChange: { + diff: 'ABC', + new_file: true, + }, + }); + + expect(localFile.mrChange.diffMode).toBe('new'); + }); + + it('has diffMode deleted', () => { + mutations.SET_FILE_MERGE_REQUEST_CHANGE(localState, { + file: localFile, + mrChange: { + diff: 'ABC', + deleted_file: true, + }, + }); + + expect(localFile.mrChange.diffMode).toBe('deleted'); + }); + + it('has diffMode renamed', () => { + mutations.SET_FILE_MERGE_REQUEST_CHANGE(localState, { + file: localFile, + mrChange: { + diff: 'ABC', + renamed_file: true, + }, + }); + + expect(localFile.mrChange.diffMode).toBe('renamed'); + }); }); describe('DISCARD_FILE_CHANGES', () => { diff --git a/spec/javascripts/test_constants.js b/spec/javascripts/test_constants.js index df59195e9f6..a820dd2d09c 100644 --- a/spec/javascripts/test_constants.js +++ b/spec/javascripts/test_constants.js @@ -2,3 +2,6 @@ export const FIXTURES_PATH = '/base/spec/javascripts/fixtures'; export const TEST_HOST = 'http://test.host'; export const DUMMY_IMAGE_URL = `${FIXTURES_PATH}/one_white_pixel.png`; + +export const GREEN_BOX_IMAGE_URL = `${FIXTURES_PATH}/images/green_box.png`; +export const RED_BOX_IMAGE_URL = `${FIXTURES_PATH}/images/red_box.png`; diff --git a/spec/javascripts/vue_shared/components/content_viewer/content_viewer_spec.js b/spec/javascripts/vue_shared/components/content_viewer/content_viewer_spec.js index 383f0cd29ea..e2c34508b0d 100644 --- a/spec/javascripts/vue_shared/components/content_viewer/content_viewer_spec.js +++ b/spec/javascripts/vue_shared/components/content_viewer/content_viewer_spec.js @@ -3,6 +3,7 @@ import MockAdapter from 'axios-mock-adapter'; import axios from '~/lib/utils/axios_utils'; import contentViewer from '~/vue_shared/components/content_viewer/content_viewer.vue'; import mountComponent from 'spec/helpers/vue_mount_component_helper'; +import { GREEN_BOX_IMAGE_URL } from 'spec/test_constants'; describe('ContentViewer', () => { let vm; @@ -41,12 +42,12 @@ describe('ContentViewer', () => { it('renders image preview', done => { createComponent({ - path: 'test.jpg', + path: GREEN_BOX_IMAGE_URL, fileSize: 1024, }); setTimeout(() => { - expect(vm.$el.querySelector('.image_file img').getAttribute('src')).toBe('test.jpg'); + expect(vm.$el.querySelector('.image_file img').getAttribute('src')).toBe(GREEN_BOX_IMAGE_URL); done(); }); @@ -59,9 +60,8 @@ describe('ContentViewer', () => { }); setTimeout(() => { - expect(vm.$el.querySelector('.file-info').textContent.trim()).toContain( - 'test.abc (1.00 KiB)', - ); + expect(vm.$el.querySelector('.file-info').textContent.trim()).toContain('test.abc'); + expect(vm.$el.querySelector('.file-info').textContent.trim()).toContain('(1.00 KiB)'); expect(vm.$el.querySelector('.btn.btn-default').textContent.trim()).toContain('Download'); done(); diff --git a/spec/javascripts/vue_shared/components/diff_viewer/diff_viewer_spec.js b/spec/javascripts/vue_shared/components/diff_viewer/diff_viewer_spec.js new file mode 100644 index 00000000000..71d9145bf22 --- /dev/null +++ b/spec/javascripts/vue_shared/components/diff_viewer/diff_viewer_spec.js @@ -0,0 +1,70 @@ +import Vue from 'vue'; +import diffViewer from '~/vue_shared/components/diff_viewer/diff_viewer.vue'; +import mountComponent from 'spec/helpers/vue_mount_component_helper'; +import { GREEN_BOX_IMAGE_URL, RED_BOX_IMAGE_URL } from 'spec/test_constants'; + +describe('DiffViewer', () => { + let vm; + + function createComponent(props) { + const DiffViewer = Vue.extend(diffViewer); + vm = mountComponent(DiffViewer, props); + } + + afterEach(() => { + vm.$destroy(); + }); + + it('renders image diff', done => { + window.gon = { + relative_url_root: '', + }; + + createComponent({ + diffMode: 'replaced', + newPath: GREEN_BOX_IMAGE_URL, + newSha: 'ABC', + oldPath: RED_BOX_IMAGE_URL, + oldSha: 'DEF', + projectPath: '', + }); + + setTimeout(() => { + expect(vm.$el.querySelector('.deleted .image_file img').getAttribute('src')).toBe( + `//raw/DEF/${RED_BOX_IMAGE_URL}`, + ); + + expect(vm.$el.querySelector('.added .image_file img').getAttribute('src')).toBe( + `//raw/ABC/${GREEN_BOX_IMAGE_URL}`, + ); + + done(); + }); + }); + + it('renders fallback download diff display', done => { + createComponent({ + diffMode: 'replaced', + newPath: 'test.abc', + newSha: 'ABC', + oldPath: 'testold.abc', + oldSha: 'DEF', + }); + + setTimeout(() => { + expect(vm.$el.querySelector('.deleted .file-info').textContent.trim()).toContain( + 'testold.abc', + ); + expect(vm.$el.querySelector('.deleted .btn.btn-default').textContent.trim()).toContain( + 'Download', + ); + + expect(vm.$el.querySelector('.added .file-info').textContent.trim()).toContain('test.abc'); + expect(vm.$el.querySelector('.added .btn.btn-default').textContent.trim()).toContain( + 'Download', + ); + + done(); + }); + }); +}); diff --git a/spec/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer_spec.js b/spec/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer_spec.js new file mode 100644 index 00000000000..b878286ae3f --- /dev/null +++ b/spec/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer_spec.js @@ -0,0 +1,185 @@ +import Vue from 'vue'; +import imageDiffViewer from '~/vue_shared/components/diff_viewer/viewers/image_diff_viewer.vue'; +import mountComponent from 'spec/helpers/vue_mount_component_helper'; +import { GREEN_BOX_IMAGE_URL, RED_BOX_IMAGE_URL } from 'spec/test_constants'; + +describe('ImageDiffViewer', () => { + let vm; + + function createComponent(props) { + const ImageDiffViewer = Vue.extend(imageDiffViewer); + vm = mountComponent(ImageDiffViewer, props); + } + + const triggerEvent = (eventName, el = vm.$el, clientX = 0) => { + const event = document.createEvent('MouseEvents'); + event.initMouseEvent( + eventName, + true, + true, + window, + 1, + clientX, + 0, + clientX, + 0, + false, + false, + false, + false, + 0, + null, + ); + + el.dispatchEvent(event); + }; + + const dragSlider = (sliderElement, dragPixel = 20) => { + triggerEvent('mousedown', sliderElement); + triggerEvent('mousemove', document.body, dragPixel); + triggerEvent('mouseup', document.body); + }; + + afterEach(() => { + vm.$destroy(); + }); + + it('renders image diff for replaced', done => { + createComponent({ + diffMode: 'replaced', + newPath: GREEN_BOX_IMAGE_URL, + oldPath: RED_BOX_IMAGE_URL, + }); + + setTimeout(() => { + expect(vm.$el.querySelector('.added .image_file img').getAttribute('src')).toBe( + GREEN_BOX_IMAGE_URL, + ); + expect(vm.$el.querySelector('.deleted .image_file img').getAttribute('src')).toBe( + RED_BOX_IMAGE_URL, + ); + + expect(vm.$el.querySelector('.view-modes-menu li.active').textContent.trim()).toBe('2-up'); + expect(vm.$el.querySelector('.view-modes-menu li:nth-child(2)').textContent.trim()).toBe( + 'Swipe', + ); + expect(vm.$el.querySelector('.view-modes-menu li:nth-child(3)').textContent.trim()).toBe( + 'Onion skin', + ); + + done(); + }); + }); + + it('renders image diff for new', done => { + createComponent({ + diffMode: 'new', + newPath: GREEN_BOX_IMAGE_URL, + oldPath: '', + }); + + setTimeout(() => { + expect(vm.$el.querySelector('.added .image_file img').getAttribute('src')).toBe( + GREEN_BOX_IMAGE_URL, + ); + + done(); + }); + }); + + it('renders image diff for deleted', done => { + createComponent({ + diffMode: 'deleted', + newPath: '', + oldPath: RED_BOX_IMAGE_URL, + }); + + setTimeout(() => { + expect(vm.$el.querySelector('.deleted .image_file img').getAttribute('src')).toBe( + RED_BOX_IMAGE_URL, + ); + + done(); + }); + }); + + describe('swipeMode', () => { + beforeEach(done => { + createComponent({ + diffMode: 'replaced', + newPath: GREEN_BOX_IMAGE_URL, + oldPath: RED_BOX_IMAGE_URL, + }); + + setTimeout(() => { + done(); + }); + }); + + it('switches to Swipe Mode', done => { + vm.$el.querySelector('.view-modes-menu li:nth-child(2)').click(); + + vm.$nextTick(() => { + expect(vm.$el.querySelector('.view-modes-menu li.active').textContent.trim()).toBe('Swipe'); + done(); + }); + }); + + it('drag handler is working', done => { + vm.$el.querySelector('.view-modes-menu li:nth-child(2)').click(); + + vm.$nextTick(() => { + expect(vm.$el.querySelector('.swipe-bar').style.left).toBe('1px'); + expect(vm.$el.querySelector('.top-handle')).not.toBeNull(); + + dragSlider(vm.$el.querySelector('.swipe-bar'), 40); + + vm.$nextTick(() => { + expect(vm.$el.querySelector('.swipe-bar').style.left).toBe('-20px'); + done(); + }); + }); + }); + }); + + describe('onionSkin', () => { + beforeEach(done => { + createComponent({ + diffMode: 'replaced', + newPath: GREEN_BOX_IMAGE_URL, + oldPath: RED_BOX_IMAGE_URL, + }); + + setTimeout(() => { + done(); + }); + }); + + it('switches to Onion Skin Mode', done => { + vm.$el.querySelector('.view-modes-menu li:nth-child(3)').click(); + + vm.$nextTick(() => { + expect(vm.$el.querySelector('.view-modes-menu li.active').textContent.trim()).toBe( + 'Onion skin', + ); + done(); + }); + }); + + it('has working drag handler', done => { + vm.$el.querySelector('.view-modes-menu li:nth-child(3)').click(); + + vm.$nextTick(() => { + expect(vm.$el.querySelector('.dragger').style.left).toBe('100px'); + + dragSlider(vm.$el.querySelector('.dragger')); + + vm.$nextTick(() => { + expect(vm.$el.querySelector('.dragger').style.left).toBe('20px'); + expect(vm.$el.querySelector('.added.frame').style.opacity).toBe('0.2'); + done(); + }); + }); + }); + }); +}); diff --git a/spec/javascripts/vue_shared/components/lib/utils/dom_utils_spec.js b/spec/javascripts/vue_shared/components/lib/utils/dom_utils_spec.js new file mode 100644 index 00000000000..2388660b0c2 --- /dev/null +++ b/spec/javascripts/vue_shared/components/lib/utils/dom_utils_spec.js @@ -0,0 +1,13 @@ +import * as domUtils from '~/vue_shared/components/lib/utils/dom_utils'; + +describe('domUtils', () => { + describe('pixeliseValue', () => { + it('should add px to a given Number', () => { + expect(domUtils.pixeliseValue(12)).toEqual('12px'); + }); + + it('should not add px to 0', () => { + expect(domUtils.pixeliseValue(0)).toEqual(''); + }); + }); +}); -- cgit v1.2.1 From 1721bbcb013eed3c98c81d615087dd79dddb65be Mon Sep 17 00:00:00 2001 From: Jan Date: Wed, 13 Jun 2018 10:23:57 +0000 Subject: Resolve "Quick actions are case sensitive" --- .../unreleased/47050-quick-actions-case-insensitive.yml | 5 +++++ lib/gitlab/quick_actions/extractor.rb | 8 ++++---- lib/gitlab/quick_actions/substitution_definition.rb | 2 +- spec/lib/gitlab/quick_actions/extractor_spec.rb | 16 ++++++++++++++++ 4 files changed, 26 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/47050-quick-actions-case-insensitive.yml diff --git a/changelogs/unreleased/47050-quick-actions-case-insensitive.yml b/changelogs/unreleased/47050-quick-actions-case-insensitive.yml new file mode 100644 index 00000000000..176aba627b9 --- /dev/null +++ b/changelogs/unreleased/47050-quick-actions-case-insensitive.yml @@ -0,0 +1,5 @@ +--- +title: Make quick commands case insensitive +merge_request: 19614 +author: Jan Beckmann +type: fixed diff --git a/lib/gitlab/quick_actions/extractor.rb b/lib/gitlab/quick_actions/extractor.rb index 075ff91700c..30c6806b68e 100644 --- a/lib/gitlab/quick_actions/extractor.rb +++ b/lib/gitlab/quick_actions/extractor.rb @@ -39,7 +39,7 @@ module Gitlab content.delete!("\r") content.gsub!(commands_regex) do if $~[:cmd] - commands << [$~[:cmd], $~[:arg]].reject(&:blank?) + commands << [$~[:cmd].downcase, $~[:arg]].reject(&:blank?) '' else $~[0] @@ -102,14 +102,14 @@ module Gitlab # /close ^\/ - (?#{Regexp.union(names)}) + (?#{Regexp.new(Regexp.union(names).source, Regexp::IGNORECASE)}) (?: [ ] (?[^\n]*) )? (?:\n|$) ) - }mx + }mix end def perform_substitutions(content, commands) @@ -120,7 +120,7 @@ module Gitlab end substitution_definitions.each do |substitution| - match_data = substitution.match(content) + match_data = substitution.match(content.downcase) if match_data command = [substitution.name.to_s] command << match_data[1] unless match_data[1].empty? diff --git a/lib/gitlab/quick_actions/substitution_definition.rb b/lib/gitlab/quick_actions/substitution_definition.rb index 032c49ed159..688056e5d73 100644 --- a/lib/gitlab/quick_actions/substitution_definition.rb +++ b/lib/gitlab/quick_actions/substitution_definition.rb @@ -15,7 +15,7 @@ module Gitlab return unless content all_names.each do |a_name| - content.gsub!(%r{/#{a_name} ?(.*)$}, execute_block(action_block, context, '\1')) + content.gsub!(%r{/#{a_name} ?(.*)$}i, execute_block(action_block, context, '\1')) end content end diff --git a/spec/lib/gitlab/quick_actions/extractor_spec.rb b/spec/lib/gitlab/quick_actions/extractor_spec.rb index f7c288f2393..0166f6c2ee0 100644 --- a/spec/lib/gitlab/quick_actions/extractor_spec.rb +++ b/spec/lib/gitlab/quick_actions/extractor_spec.rb @@ -182,6 +182,14 @@ describe Gitlab::QuickActions::Extractor do expect(msg).to eq "hello\nworld" end + it 'extracts command case insensitive' do + msg = %(hello\n/PoWer @user.name %9.10 ~"bar baz.2"\nworld) + msg, commands = extractor.extract_commands(msg) + + expect(commands).to eq [['power', '@user.name %9.10 ~"bar baz.2"']] + expect(msg).to eq "hello\nworld" + end + it 'does not extract noop commands' do msg = %(hello\nworld\n/reopen\n/noop_command) msg, commands = extractor.extract_commands(msg) @@ -206,6 +214,14 @@ describe Gitlab::QuickActions::Extractor do expect(msg).to eq "hello\nworld\nthis is great? SHRUG" end + it 'extracts and performs substitution commands case insensitive' do + msg = %(hello\nworld\n/reOpen\n/sHRuG this is great?) + msg, commands = extractor.extract_commands(msg) + + expect(commands).to eq [['reopen'], ['shrug', 'this is great?']] + expect(msg).to eq "hello\nworld\nthis is great? SHRUG" + end + it 'extracts and performs substitution commands with comments' do msg = %(hello\nworld\n/reopen\n/substitution wow this is a thing.) msg, commands = extractor.extract_commands(msg) -- cgit v1.2.1 From 4077d4f735ed40b7a62baa35464677813c3b6784 Mon Sep 17 00:00:00 2001 From: Jan Date: Wed, 13 Jun 2018 10:28:27 +0000 Subject: Resolve "Provide ability to retrieve `visibility` level via Snippets API" --- changelogs/unreleased/35158-snippets-api-visibility.yml | 5 +++++ doc/api/snippets.md | 6 +++++- lib/api/entities.rb | 2 +- spec/fixtures/api/schemas/public_api/v4/snippets.json | 1 + spec/requests/api/snippets_spec.rb | 3 +++ 5 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/35158-snippets-api-visibility.yml diff --git a/changelogs/unreleased/35158-snippets-api-visibility.yml b/changelogs/unreleased/35158-snippets-api-visibility.yml new file mode 100644 index 00000000000..f06015dda46 --- /dev/null +++ b/changelogs/unreleased/35158-snippets-api-visibility.yml @@ -0,0 +1,5 @@ +--- +title: Expose visibility via Snippets API +merge_request: 19620 +author: Jan Beckmann +type: added diff --git a/doc/api/snippets.md b/doc/api/snippets.md index 42b760c107d..7892866cd8e 100644 --- a/doc/api/snippets.md +++ b/doc/api/snippets.md @@ -49,6 +49,7 @@ Example response: "title": "test", "file_name": "add.rb", "description": "Ruby test snippet", + "visibility": "private", "author": { "id": 1, "username": "john_smith", @@ -99,6 +100,7 @@ Example response: "title": "This is a snippet", "file_name": "test.txt", "description": "Hello World snippet", + "visibility": "internal", "author": { "id": 1, "username": "john_smith", @@ -150,6 +152,7 @@ Example response: "title": "test", "file_name": "add.rb", "description": "description of snippet", + "visibility": "internal", "author": { "id": 1, "username": "john_smith", @@ -238,7 +241,8 @@ Example response: "raw_url": "http://localhost:3000/snippets/48/raw", "title": "Minus similique nesciunt vel fugiat qui ullam sunt.", "updated_at": "2016-11-25T16:53:34.479Z", - "web_url": "http://localhost:3000/snippets/48" + "web_url": "http://localhost:3000/snippets/48", + "visibility": "public" } ] ``` diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 3395d53b363..1cc8fcb8408 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -362,7 +362,7 @@ module API end class Snippet < Grape::Entity - expose :id, :title, :file_name, :description + expose :id, :title, :file_name, :description, :visibility expose :author, using: Entities::UserBasic expose :updated_at, :created_at expose :project_id diff --git a/spec/fixtures/api/schemas/public_api/v4/snippets.json b/spec/fixtures/api/schemas/public_api/v4/snippets.json index e37e9704649..d13d703e063 100644 --- a/spec/fixtures/api/schemas/public_api/v4/snippets.json +++ b/spec/fixtures/api/schemas/public_api/v4/snippets.json @@ -8,6 +8,7 @@ "title": { "type": "string" }, "file_name": { "type": ["string", "null"] }, "description": { "type": ["string", "null"] }, + "visibility": { "type": "string" }, "web_url": { "type": "string" }, "created_at": { "type": "date" }, "updated_at": { "type": "date" }, diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb index b3e253befc6..c5456977b60 100644 --- a/spec/requests/api/snippets_spec.rb +++ b/spec/requests/api/snippets_spec.rb @@ -20,6 +20,7 @@ describe API::Snippets do private_snippet.id) expect(json_response.last).to have_key('web_url') expect(json_response.last).to have_key('raw_url') + expect(json_response.last).to have_key('visibility') end it 'hides private snippets from regular user' do @@ -112,6 +113,7 @@ describe API::Snippets do expect(json_response['title']).to eq(snippet.title) expect(json_response['description']).to eq(snippet.description) expect(json_response['file_name']).to eq(snippet.file_name) + expect(json_response['visibility']).to eq(snippet.visibility) end it 'returns 404 for invalid snippet id' do @@ -142,6 +144,7 @@ describe API::Snippets do expect(json_response['title']).to eq(params[:title]) expect(json_response['description']).to eq(params[:description]) expect(json_response['file_name']).to eq(params[:file_name]) + expect(json_response['visibility']).to eq(params[:visibility]) end it 'returns 400 for missing parameters' do -- cgit v1.2.1 From 5e2b8ab8d7d80f91a6d1e1951041defcd431b841 Mon Sep 17 00:00:00 2001 From: Jan Date: Wed, 13 Jun 2018 10:42:40 +0000 Subject: Resolve "Bug: When creating an account with invalid characters the error is "Username already taken" not "Invalid characters used"" --- app/assets/javascripts/pages/sessions/new/username_validator.js | 9 ++++----- changelogs/unreleased/45575-invalid-characters-signup.yml | 5 +++++ 2 files changed, 9 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/45575-invalid-characters-signup.yml diff --git a/app/assets/javascripts/pages/sessions/new/username_validator.js b/app/assets/javascripts/pages/sessions/new/username_validator.js index 825de01b5a2..87213c94eda 100644 --- a/app/assets/javascripts/pages/sessions/new/username_validator.js +++ b/app/assets/javascripts/pages/sessions/new/username_validator.js @@ -62,13 +62,13 @@ export default class UsernameValidator { return this.setPendingState(); } - if (!this.state.available) { - return this.setUnavailableState(); - } - if (!this.state.valid) { return this.setInvalidState(); } + + if (!this.state.available) { + return this.setUnavailableState(); + } } interceptInvalid(event) { @@ -89,7 +89,6 @@ export default class UsernameValidator { setAvailabilityState(usernameTaken) { if (usernameTaken) { - this.state.valid = false; this.state.available = false; } else { this.state.available = true; diff --git a/changelogs/unreleased/45575-invalid-characters-signup.yml b/changelogs/unreleased/45575-invalid-characters-signup.yml new file mode 100644 index 00000000000..679bd13e59b --- /dev/null +++ b/changelogs/unreleased/45575-invalid-characters-signup.yml @@ -0,0 +1,5 @@ +--- +title: 'Fix username validation order on signup, resolves #45575' +merge_request: 19610 +author: Jan Beckmann +type: fixed -- cgit v1.2.1