From 51b04b5f2273284c674a8813a4c5da13825b431e Mon Sep 17 00:00:00 2001 From: Nick Kipling Date: Fri, 12 Jul 2019 10:27:23 +0100 Subject: [PATCH 01/12] Implement multi select deletion for container registry Added checkboxes to each image row Added delete selected images button Changed row delete button to appear on row hover Changed confirmation modal message Changed delete logic to support multi Added tests for multi select Updated pot file Updated rspec test for new functionality --- .../components/collapsible_container.vue | 2 +- .../registry/components/table_registry.vue | 156 +++++++++++++++--- .../stylesheets/pages/container_registry.scss | 23 +++ locale/gitlab.pot | 11 +- spec/features/container_registry_spec.rb | 10 +- .../components/table_registry_spec.js | 117 +++++++++++-- spec/javascripts/registry/mock_data.js | 11 ++ 7 files changed, 283 insertions(+), 47 deletions(-) diff --git a/app/assets/javascripts/registry/components/collapsible_container.vue b/app/assets/javascripts/registry/components/collapsible_container.vue index e157036871b..bfb2305c48c 100644 --- a/app/assets/javascripts/registry/components/collapsible_container.vue +++ b/app/assets/javascripts/registry/components/collapsible_container.vue @@ -84,7 +84,7 @@ export default { v-gl-modal="modalId" :title="s__('ContainerRegistry|Remove repository')" :aria-label="s__('ContainerRegistry|Remove repository')" - class="js-remove-repo" + class="js-remove-repo btn-inverted" variant="danger" > diff --git a/app/assets/javascripts/registry/components/table_registry.vue b/app/assets/javascripts/registry/components/table_registry.vue index a498a553908..a241db13e5a 100644 --- a/app/assets/javascripts/registry/components/table_registry.vue +++ b/app/assets/javascripts/registry/components/table_registry.vue @@ -1,7 +1,13 @@ @@ -74,15 +153,43 @@ export default { + - + - + + - diff --git a/spec/javascripts/registry/components/table_registry_spec.js b/spec/javascripts/registry/components/table_registry_spec.js index 556a66c5666..a96a505a3a4 100644 --- a/spec/javascripts/registry/components/table_registry_spec.js +++ b/spec/javascripts/registry/components/table_registry_spec.js @@ -27,7 +27,8 @@ describe('table registry', () => { }); }; - const toggleSelectAll = () => vm.selectAll(); + const selectAllCheckboxes = () => vm.selectAll(); + const deselectAllCheckboxes = () => vm.deselectAll(); beforeEach(() => { createComponent(); @@ -58,7 +59,7 @@ describe('table registry', () => { describe('multi select', () => { it('should support multiselect and selecting a row should enable delete button', done => { findSelectAllCheckbox().click(); - toggleSelectAll(); + selectAllCheckboxes(); expect(findSelectAllCheckbox().checked).toBe(true); @@ -69,8 +70,7 @@ describe('table registry', () => { }); it('selecting all checkbox should select all rows and enable delete button', done => { - findSelectAllCheckbox().click(); - toggleSelectAll(); + selectAllCheckboxes(); Vue.nextTick(() => { const checkedValues = findAllRowCheckboxes().filter(x => x.checked); @@ -81,9 +81,8 @@ describe('table registry', () => { }); it('deselecting select all checkbox should deselect all rows and disable delete button', done => { - findSelectAllCheckbox().click(); - toggleSelectAll(); // Select them all on - toggleSelectAll(); // Select them all off + selectAllCheckboxes(); + deselectAllCheckboxes(); Vue.nextTick(() => { const checkedValues = findAllRowCheckboxes().filter(x => x.checked); @@ -94,8 +93,7 @@ describe('table registry', () => { }); it('should delete multiple items when multiple items are selected', done => { - findSelectAllCheckbox().click(); - toggleSelectAll(); + selectAllCheckboxes(); Vue.nextTick(() => { expect(vm.itemsToBeDeleted).toEqual([0, 1]); @@ -177,8 +175,7 @@ describe('table registry', () => { }); it('should show the plural title and image count when deleting more than one image', done => { - findSelectAllCheckbox().click(); - toggleSelectAll(); + selectAllCheckboxes(); Vue.nextTick(() => { expect(vm.modalTitle).toBe('Remove images'); From c2d1fbe507cc1732927ca7c656078cf47754ceeb Mon Sep 17 00:00:00 2001 From: Giorgenes Gelatti Date: Tue, 23 Jul 2019 19:57:28 +1000 Subject: [PATCH 08/12] Validates tag names and tags#bulk_destroy --- app/controllers/projects/registry/tags_controller.rb | 9 +++++++++ lib/container_registry/tag.rb | 7 +++++++ 2 files changed, 16 insertions(+) diff --git a/app/controllers/projects/registry/tags_controller.rb b/app/controllers/projects/registry/tags_controller.rb index 22c87dfe1c0..633a7865cfe 100644 --- a/app/controllers/projects/registry/tags_controller.rb +++ b/app/controllers/projects/registry/tags_controller.rb @@ -29,7 +29,16 @@ module Projects end def bulk_destroy + unless params[:ids].present? + head :bad_request + return + end + @tags = (params[:ids] || []).map { |tag_name| image.tag(tag_name) } + unless @tags.all? { |tag| tag.valid_name? } + head :bad_request + return + end success_count = 0 @tags.each do |tag| diff --git a/lib/container_registry/tag.rb b/lib/container_registry/tag.rb index ef41dc560c9..ebea84fa1ca 100644 --- a/lib/container_registry/tag.rb +++ b/lib/container_registry/tag.rb @@ -6,6 +6,9 @@ module ContainerRegistry attr_reader :repository, :name + # https://github.com/docker/distribution/commit/3150937b9f2b1b5b096b2634d0e7c44d4a0f89fb + TAG_NAME_REGEX = /^[\w][\w.-]{0,127}$/.freeze + delegate :registry, :client, to: :repository delegate :revision, :short_revision, to: :config_blob, allow_nil: true @@ -13,6 +16,10 @@ module ContainerRegistry @repository, @name = repository, name end + def valid_name? + !name.match(TAG_NAME_REGEX).nil? + end + def valid? manifest.present? end From a37d672ff5c4c102a5b507ad744919748cbdcb34 Mon Sep 17 00:00:00 2001 From: Nick Kipling Date: Wed, 24 Jul 2019 19:38:39 +0100 Subject: [PATCH 09/12] Applying feedback changes Updated table registry to remove singleItemToBeDeleted Renamed usages of idx to index Tidied and simplified css styling Added clarification comment to test regex Updated pot file --- .../registry/components/table_registry.vue | 66 ++++++++++--------- .../stylesheets/pages/container_registry.scss | 24 +++---- locale/gitlab.pot | 7 +- .../components/table_registry_spec.js | 1 + 4 files changed, 49 insertions(+), 49 deletions(-) diff --git a/app/assets/javascripts/registry/components/table_registry.vue b/app/assets/javascripts/registry/components/table_registry.vue index ff9f8871a8c..90d6a4ce27f 100644 --- a/app/assets/javascripts/registry/components/table_registry.vue +++ b/app/assets/javascripts/registry/components/table_registry.vue @@ -38,7 +38,6 @@ export default { }, data() { return { - singleItemToBeDeleted: null, itemsToBeDeleted: [], modalId: `confirm-image-deletion-modal-${this.repo.id}`, selectAllChecked: false, @@ -52,19 +51,20 @@ export default { return this.repo.pagination.total > this.repo.pagination.perPage; }, modalTitle() { - if (this.singleItemSelected) { - return s__('ContainerRegistry|Remove image'); - } - return s__('ContainerRegistry|Remove images'); + return n__( + 'ContainerRegistry|Remove image', + 'ContainerRegistry|Remove images', + this.singleItemSelected ? 1 : this.itemsToBeDeleted.length, + ); }, modalDescription() { const selectedCount = this.itemsToBeDeleted.length; if (this.singleItemSelected) { - const { tag } = - this.singleItemToBeDeleted !== null - ? this.repo.list[this.singleItemToBeDeleted] - : this.repo.list[this.itemsToBeDeleted[0]]; + // Attempt to pull 'single' property if it's an object in this.itemsToBeDeleted + // Otherwise, simply use the int value of the selected row + const { single: itemIndex = this.itemsToBeDeleted[0] } = this.itemsToBeDeleted[0]; + const { tag } = this.repo.list[itemIndex]; return sprintf( s__(`ContainerRegistry|You are about to delete the image %{title}. This will @@ -80,7 +80,10 @@ export default { ); }, singleItemSelected() { - return this.singleItemToBeDeleted !== null || this.itemsToBeDeleted.length === 1; + return this.findSingleRowToDelete || this.itemsToBeDeleted.length === 1; + }, + findSingleRowToDelete() { + return this.itemsToBeDeleted.find(x => x.single !== undefined); }, }, methods: { @@ -91,22 +94,25 @@ export default { formatSize(size) { return numberToHumanSize(size); }, - setSingleItemToBeDeleted(idx) { - this.singleItemToBeDeleted = idx; + addSingleItemToBeDeleted(index) { + this.itemsToBeDeleted.push({ single: index }); }, - resetSingleItemToBeDeleted() { - this.singleItemToBeDeleted = null; + removeSingleItemToBeDeleted() { + const singleIndex = this.itemsToBeDeleted.findIndex(x => x.single !== undefined); + + if (singleIndex > -1) { + this.itemsToBeDeleted.splice(singleIndex, 1); + } }, handleDeleteRegistry() { let { itemsToBeDeleted } = this; - this.itemsToBeDeleted = []; - if (this.singleItemToBeDeleted !== null) { - const { singleItemToBeDeleted } = this; - this.singleItemToBeDeleted = null; - itemsToBeDeleted = [singleItemToBeDeleted]; + if (this.findSingleRowToDelete) { + itemsToBeDeleted = [this.findSingleRowToDelete.single]; } + this.itemsToBeDeleted = []; + if (this.bulkDeletePath) { this.deleteItems({ path: this.bulkDeletePath, @@ -134,21 +140,21 @@ export default { } }, selectAll() { - this.itemsToBeDeleted = this.repo.list.map((x, idx) => idx); + this.itemsToBeDeleted = this.repo.list.map((x, index) => index); this.selectAllChecked = true; }, deselectAll() { this.itemsToBeDeleted = []; this.selectAllChecked = false; }, - updateItemsToBeDeleted(idx) { - const delIdx = this.itemsToBeDeleted.findIndex(x => x === idx); + updateItemsToBeDeleted(index) { + const delIndex = this.itemsToBeDeleted.findIndex(x => x === index); - if (delIdx > -1) { - this.itemsToBeDeleted.splice(delIdx, 1); + if (delIndex > -1) { + this.itemsToBeDeleted.splice(delIndex, 1); this.selectAllChecked = false; } else { - this.itemsToBeDeleted.push(idx); + this.itemsToBeDeleted.push(index); if (this.itemsToBeDeleted.length === this.repo.list.length) { this.selectAllChecked = true; @@ -191,13 +197,13 @@ export default { - + - +
+ + {{ s__('ContainerRegistry|Tag') }} {{ s__('ContainerRegistry|Tag ID') }} {{ s__('ContainerRegistry|Size') }} {{ s__('ContainerRegistry|Last Updated') }} + +
+ + {{ item.tag }} + @@ -135,19 +241,15 @@ export default { :page-info="repo.pagination" /> - - - -

+ + + +

diff --git a/app/assets/stylesheets/pages/container_registry.scss b/app/assets/stylesheets/pages/container_registry.scss index a21fa29f34a..ed9de6f7e30 100644 --- a/app/assets/stylesheets/pages/container_registry.scss +++ b/app/assets/stylesheets/pages/container_registry.scss @@ -31,4 +31,27 @@ .table.tags { margin-bottom: 0; + + th { + height: 55px; + } + + tr { + &:hover { + td { + &.action-buttons { + opacity: 1; + } + } + } + + td.check { + padding-right: $gl-padding; + width: 5%; + } + + td.action-buttons { + opacity: 0; + } + } } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 591dc2a7e39..557a8f2681e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3132,12 +3132,18 @@ msgstr "" msgid "ContainerRegistry|Remove image" msgstr "" -msgid "ContainerRegistry|Remove image and tags" +msgid "ContainerRegistry|Remove image(s) and tags" +msgstr "" + +msgid "ContainerRegistry|Remove images" msgstr "" msgid "ContainerRegistry|Remove repository" msgstr "" +msgid "ContainerRegistry|Remove selected images" +msgstr "" + msgid "ContainerRegistry|Size" msgstr "" @@ -3159,6 +3165,9 @@ msgstr "" msgid "ContainerRegistry|With the Docker Container Registry integrated into GitLab, every project can have its own space to store its Docker images. %{docLinkStart}More Information%{docLinkEnd}" msgstr "" +msgid "ContainerRegistry|You are about to delete %{count} images. This will delete the images and all tags pointing to them." +msgstr "" + msgid "ContainerRegistry|You are about to delete the image %{title}. This will delete the image and all tags pointing to this image." msgstr "" diff --git a/spec/features/container_registry_spec.rb b/spec/features/container_registry_spec.rb index 89dece97a35..aefdc4d6d4f 100644 --- a/spec/features/container_registry_spec.rb +++ b/spec/features/container_registry_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe "Container Registry", :js do +describe 'Container Registry', :js do let(:user) { create(:user) } let(:project) { create(:project) } @@ -40,8 +40,7 @@ describe "Container Registry", :js do it 'user removes entire container repository' do visit_container_registry - expect_any_instance_of(ContainerRepository) - .to receive(:delete_tags!).and_return(true) + expect_any_instance_of(ContainerRepository).to receive(:delete_tags!).and_return(true) click_on(class: 'js-remove-repo') expect(find('.modal .modal-title')).to have_content 'Remove repository' @@ -54,10 +53,9 @@ describe "Container Registry", :js do find('.js-toggle-repo').click wait_for_requests - expect_any_instance_of(ContainerRegistry::Tag) - .to receive(:delete).and_return(true) + expect_any_instance_of(ContainerRegistry::Tag).to receive(:delete).and_return(true) - click_on(class: 'js-delete-registry') + click_on(class: 'js-delete-registry-row', visible: false) expect(find('.modal .modal-title')).to have_content 'Remove image' find('.modal .modal-footer .btn-danger').click end diff --git a/spec/javascripts/registry/components/table_registry_spec.js b/spec/javascripts/registry/components/table_registry_spec.js index 31ac970378e..9ee326325e0 100644 --- a/spec/javascripts/registry/components/table_registry_spec.js +++ b/spec/javascripts/registry/components/table_registry_spec.js @@ -3,15 +3,19 @@ import tableRegistry from '~/registry/components/table_registry.vue'; import store from '~/registry/stores'; import { repoPropsData } from '../mock_data'; -const [firstImage] = repoPropsData.list; +const [firstImage, secondImage] = repoPropsData.list; describe('table registry', () => { let vm; let Component; const findDeleteBtn = () => vm.$el.querySelector('.js-delete-registry'); + const findDeleteBtnRow = () => vm.$el.querySelector('.js-delete-registry-row'); + const findSelectAllCheckbox = () => vm.$el.querySelector('.js-select-all-checkbox > input'); + const findAllRowCheckboxes = () => + Array.from(vm.$el.querySelectorAll('.js-select-checkbox input')); - beforeEach(() => { + const createComponent = () => { Component = Vue.extend(tableRegistry); vm = new Component({ store, @@ -19,6 +23,10 @@ describe('table registry', () => { repo: repoPropsData, }, }).$mount(); + }; + + beforeEach(() => { + createComponent(); }); afterEach(() => { @@ -41,23 +49,108 @@ describe('table registry', () => { expect(textRendered).toContain(repoPropsData.list[0].size); }); - describe('delete registry', () => { - it('should be possible to delete a registry', () => { - expect(findDeleteBtn()).toBeDefined(); + describe('multi select', () => { + beforeEach(() => { + vm.itemsToBeDeleted = []; }); - it('should call deleteItem and reset itemToBeDeleted when confirming deletion', done => { - findDeleteBtn().click(); - spyOn(vm, 'deleteItem').and.returnValue(Promise.resolve()); + it('should support multiselect and selecting a row should enable delete button', done => { + findSelectAllCheckbox().click(); + + vm.selectAll(); + + expect(findSelectAllCheckbox().checked).toBe(true); Vue.nextTick(() => { - document.querySelector(`#${vm.modalId} .btn-danger`).click(); - - expect(vm.deleteItem).toHaveBeenCalledWith(firstImage); - expect(vm.itemToBeDeleted).toBeNull(); + expect(findDeleteBtn().disabled).toBe(false); done(); }); }); + + it('selecting all checkbox should select all rows and enable delete button', done => { + findSelectAllCheckbox().click(); + vm.selectAll(); + + Vue.nextTick(() => { + const checkedValues = findAllRowCheckboxes().filter(x => x.checked); + + expect(checkedValues.length).toBe(repoPropsData.list.length); + done(); + }); + }); + + it('deselecting select all checkbox should deselect all rows and disable delete button', done => { + findSelectAllCheckbox().click(); + vm.selectAll(); // Select them all on + vm.selectAll(); // Select them all off + + Vue.nextTick(() => { + const checkedValues = findAllRowCheckboxes().filter(x => x.checked); + + expect(checkedValues.length).toBe(0); + done(); + }); + }); + + it('should delete multiple items when multiple items are selected', done => { + findSelectAllCheckbox().click(); + vm.selectAll(); + + Vue.nextTick(() => { + expect(vm.itemsToBeDeleted).toEqual([0, 1]); + expect(findDeleteBtn().disabled).toBe(false); + + findDeleteBtn().click(); + spyOn(vm, 'deleteItem').and.returnValue(Promise.resolve()); + + Vue.nextTick(() => { + const modal = document.querySelector(`#${vm.modalId}`); + document.querySelector(`#${vm.modalId} .btn-danger`).click(); + + expect(modal).toExist(); + + Vue.nextTick(() => { + expect(vm.itemsToBeDeleted).toEqual([]); + expect(vm.deleteItem).toHaveBeenCalledWith(firstImage); + expect(vm.deleteItem).toHaveBeenCalledWith(secondImage); + done(); + }); + }); + }); + }); + }); + + describe('delete registry', () => { + beforeEach(() => { + vm.itemsToBeDeleted = [0]; + }); + + it('should be possible to delete a registry', done => { + Vue.nextTick(() => { + expect(vm.itemsToBeDeleted).toEqual([0]); + expect(findDeleteBtn()).toBeDefined(); + expect(findDeleteBtn().disabled).toBe(false); + expect(findDeleteBtnRow()).toBeDefined(); + done(); + }); + }); + + it('should call deleteItem and reset itemsToBeDeleted when confirming deletion', done => { + Vue.nextTick(() => { + expect(vm.itemsToBeDeleted).toEqual([0]); + expect(findDeleteBtn().disabled).toBe(false); + findDeleteBtn().click(); + spyOn(vm, 'deleteItem').and.returnValue(Promise.resolve()); + + Vue.nextTick(() => { + document.querySelector(`#${vm.modalId} .btn-danger`).click(); + + expect(vm.itemsToBeDeleted).toEqual([]); + expect(vm.deleteItem).toHaveBeenCalledWith(firstImage); + done(); + }); + }); + }); }); describe('pagination', () => { diff --git a/spec/javascripts/registry/mock_data.js b/spec/javascripts/registry/mock_data.js index 22db203e77f..130ab298e89 100644 --- a/spec/javascripts/registry/mock_data.js +++ b/spec/javascripts/registry/mock_data.js @@ -108,6 +108,17 @@ export const repoPropsData = { destroyPath: 'path', canDelete: true, }, + { + tag: 'test-image', + revision: 'b969de599faea2b3d9b6605a8b0897261c571acaa36db1bdc7349b5775b4e0b4', + shortRevision: 'b969de599', + size: 19, + layers: 10, + location: 'location-2', + createdAt: 1505828744434, + destroyPath: 'path-2', + canDelete: true, + }, ], location: 'location', name: 'foo', From 15bda06cab075f2365830a2808836516f6c25590 Mon Sep 17 00:00:00 2001 From: Nick Kipling Date: Wed, 17 Jul 2019 11:24:14 +0100 Subject: [PATCH 02/12] Added changelog entry --- .../24705-multi-selection-for-delete-on-registry-page.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/24705-multi-selection-for-delete-on-registry-page.yml diff --git a/changelogs/unreleased/24705-multi-selection-for-delete-on-registry-page.yml b/changelogs/unreleased/24705-multi-selection-for-delete-on-registry-page.yml new file mode 100644 index 00000000000..5254bd36b9c --- /dev/null +++ b/changelogs/unreleased/24705-multi-selection-for-delete-on-registry-page.yml @@ -0,0 +1,5 @@ +--- +title: Added multi-select deletion of container registry images +merge_request: 30837 +author: +type: other From 0426d15c080255a97297a2d45fbc4e8c5d119124 Mon Sep 17 00:00:00 2001 From: Giorgenes Gelatti Date: Thu, 18 Jul 2019 15:24:40 +1000 Subject: [PATCH 03/12] Support bulk registry tag delete --- .../projects/registry/tags_controller.rb | 15 +++++++++ config/routes/project.rb | 6 +++- .../projects/registry/tags_controller_spec.rb | 33 +++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/app/controllers/projects/registry/tags_controller.rb b/app/controllers/projects/registry/tags_controller.rb index bf1d8d8b5fc..22c87dfe1c0 100644 --- a/app/controllers/projects/registry/tags_controller.rb +++ b/app/controllers/projects/registry/tags_controller.rb @@ -28,6 +28,21 @@ module Projects end end + def bulk_destroy + @tags = (params[:ids] || []).map { |tag_name| image.tag(tag_name) } + + success_count = 0 + @tags.each do |tag| + if tag.delete + success_count += 1 + end + end + + respond_to do |format| + format.json { head(success_count == @tags.size ? :no_content : :bad_request) } + end + end + private def tags diff --git a/config/routes/project.rb b/config/routes/project.rb index 1f632765317..4bb0bce2965 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -474,7 +474,11 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do # in JSON format, or a request for tag named `latest.json`. scope format: false do resources :tags, only: [:index, :destroy], - constraints: { id: Gitlab::Regex.container_registry_tag_regex } + constraints: { id: Gitlab::Regex.container_registry_tag_regex } do + collection do + delete :bulk_destroy + end + end end end end diff --git a/spec/controllers/projects/registry/tags_controller_spec.rb b/spec/controllers/projects/registry/tags_controller_spec.rb index ff35139ae2e..c6e063d8229 100644 --- a/spec/controllers/projects/registry/tags_controller_spec.rb +++ b/spec/controllers/projects/registry/tags_controller_spec.rb @@ -113,4 +113,37 @@ describe Projects::Registry::TagsController do format: :json end end + + describe 'POST bulk_destroy' do + context 'when user has access to registry' do + before do + project.add_developer(user) + end + + context 'when there is matching tag present' do + before do + stub_container_registry_tags(repository: repository.path, tags: %w[rc1 test.]) + end + + it 'makes it possible to delete tags in bulk' do + allow_any_instance_of(ContainerRegistry::Tag).to receive(:delete) { |*args| ContainerRegistry::Tag.delete(*args) } + expect(ContainerRegistry::Tag).to receive(:delete).exactly(2).times + + bulk_destroy_tags(['rc1', 'test.']) + end + end + end + + private + + def bulk_destroy_tags(names) + post :bulk_destroy, params: { + namespace_id: project.namespace, + project_id: project, + repository_id: repository, + ids: names + }, + format: :json + end + end end From 71f2d4bb8944fff33f511a9687468c87dc329cc5 Mon Sep 17 00:00:00 2001 From: Nick Kipling Date: Thu, 18 Jul 2019 10:10:41 +0100 Subject: [PATCH 04/12] Updating FE to use new bulk_destroy endpoint --- .../registry/components/table_registry.vue | 27 ++++++++++--------- .../javascripts/registry/stores/actions.js | 1 + 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/registry/components/table_registry.vue b/app/assets/javascripts/registry/components/table_registry.vue index a241db13e5a..cf1ee0e06a6 100644 --- a/app/assets/javascripts/registry/components/table_registry.vue +++ b/app/assets/javascripts/registry/components/table_registry.vue @@ -45,6 +45,9 @@ export default { }; }, computed: { + bulkDeletePath() { + return this.repo.tagsPath ? this.repo.tagsPath.replace('?format=json', '/bulk_destroy') : ''; + }, shouldRenderPagination() { return this.repo.pagination.total > this.repo.pagination.perPage; }, @@ -78,7 +81,7 @@ export default { }, }, methods: { - ...mapActions(['fetchList', 'deleteItem']), + ...mapActions(['fetchList', 'deleteItems']), layers(item) { return item.layers ? n__('%d layer', '%d layers', item.layers) : ''; }, @@ -101,18 +104,16 @@ export default { itemsToBeDeleted = [singleItemToBeDeleted]; } - const deleteActions = itemsToBeDeleted.map( - x => - new Promise((resolve, reject) => { - this.deleteItem(this.repo.list[x]) - .then(resolve) - .catch(reject); - }), - ); - - Promise.all(deleteActions) - .then(() => this.fetchList({ repo: this.repo })) - .catch(() => this.showError(errorMessagesTypes.DELETE_REGISTRY)); + if (this.bulkDeletePath) { + this.deleteItems({ + path: this.bulkDeletePath, + items: itemsToBeDeleted.map(x => this.repo.list[x].tag), + }) + .then(() => this.fetchList({ repo: this.repo })) + .catch(() => this.showError(errorMessagesTypes.DELETE_REGISTRY)); + } else { + this.showError(errorMessagesTypes.DELETE_REGISTRY); + } }, onPageChange(pageNumber) { this.fetchList({ repo: this.repo, page: pageNumber }).catch(() => diff --git a/app/assets/javascripts/registry/stores/actions.js b/app/assets/javascripts/registry/stores/actions.js index 0f5e9cc73a0..4c20c003c5a 100644 --- a/app/assets/javascripts/registry/stores/actions.js +++ b/app/assets/javascripts/registry/stores/actions.js @@ -36,6 +36,7 @@ export const fetchList = ({ commit }, { repo, page }) => { }; export const deleteItem = (_, item) => axios.delete(item.destroyPath); +export const deleteItems = (_, { path, items }) => axios.delete(path, { params: { ids: items } }); export const setMainEndpoint = ({ commit }, data) => commit(types.SET_MAIN_ENDPOINT, data); export const toggleLoading = ({ commit }) => commit(types.TOGGLE_MAIN_LOADING); From b5d50025fc8bc232de1a82ccb2d13906a96cd829 Mon Sep 17 00:00:00 2001 From: Nick Kipling Date: Thu, 18 Jul 2019 10:22:53 +0100 Subject: [PATCH 05/12] Updating table_registry tests --- .../registry/components/table_registry_spec.js | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/spec/javascripts/registry/components/table_registry_spec.js b/spec/javascripts/registry/components/table_registry_spec.js index 9ee326325e0..9dca29d4702 100644 --- a/spec/javascripts/registry/components/table_registry_spec.js +++ b/spec/javascripts/registry/components/table_registry_spec.js @@ -8,6 +8,7 @@ const [firstImage, secondImage] = repoPropsData.list; describe('table registry', () => { let vm; let Component; + const bulkDeletePath = 'path'; const findDeleteBtn = () => vm.$el.querySelector('.js-delete-registry'); const findDeleteBtnRow = () => vm.$el.querySelector('.js-delete-registry-row'); @@ -101,7 +102,7 @@ describe('table registry', () => { expect(findDeleteBtn().disabled).toBe(false); findDeleteBtn().click(); - spyOn(vm, 'deleteItem').and.returnValue(Promise.resolve()); + spyOn(vm, 'deleteItems').and.returnValue(Promise.resolve()); Vue.nextTick(() => { const modal = document.querySelector(`#${vm.modalId}`); @@ -111,8 +112,10 @@ describe('table registry', () => { Vue.nextTick(() => { expect(vm.itemsToBeDeleted).toEqual([]); - expect(vm.deleteItem).toHaveBeenCalledWith(firstImage); - expect(vm.deleteItem).toHaveBeenCalledWith(secondImage); + expect(vm.deleteItems).toHaveBeenCalledWith({ + path: bulkDeletePath, + items: [firstImage.tag, secondImage.tag], + }); done(); }); }); @@ -135,18 +138,21 @@ describe('table registry', () => { }); }); - it('should call deleteItem and reset itemsToBeDeleted when confirming deletion', done => { + it('should call deleteItems and reset itemsToBeDeleted when confirming deletion', done => { Vue.nextTick(() => { expect(vm.itemsToBeDeleted).toEqual([0]); expect(findDeleteBtn().disabled).toBe(false); findDeleteBtn().click(); - spyOn(vm, 'deleteItem').and.returnValue(Promise.resolve()); + spyOn(vm, 'deleteItems').and.returnValue(Promise.resolve()); Vue.nextTick(() => { document.querySelector(`#${vm.modalId} .btn-danger`).click(); expect(vm.itemsToBeDeleted).toEqual([]); - expect(vm.deleteItem).toHaveBeenCalledWith(firstImage); + expect(vm.deleteItems).toHaveBeenCalledWith({ + path: bulkDeletePath, + items: [firstImage.tag], + }); done(); }); }); From 237f434ce83488f252b5f593c67ecaf76ceb9e86 Mon Sep 17 00:00:00 2001 From: Nick Kipling Date: Thu, 18 Jul 2019 17:56:09 +0100 Subject: [PATCH 06/12] Updating with suggestions as per review --- .../registry/components/table_registry.vue | 7 +- .../components/table_registry_spec.js | 83 ++++++++++++------- 2 files changed, 58 insertions(+), 32 deletions(-) diff --git a/app/assets/javascripts/registry/components/table_registry.vue b/app/assets/javascripts/registry/components/table_registry.vue index cf1ee0e06a6..807b81c66e9 100644 --- a/app/assets/javascripts/registry/components/table_registry.vue +++ b/app/assets/javascripts/registry/components/table_registry.vue @@ -52,7 +52,7 @@ export default { return this.repo.pagination.total > this.repo.pagination.perPage; }, modalTitle() { - if (this.singleItemToBeDeleted !== null || this.itemsToBeDeleted.length === 1) { + if (this.singleItemSelected) { return s__('ContainerRegistry|Remove image'); } return s__('ContainerRegistry|Remove images'); @@ -60,7 +60,7 @@ export default { modalDescription() { const selectedCount = this.itemsToBeDeleted.length; - if (this.singleItemToBeDeleted !== null || selectedCount === 1) { + if (this.singleItemSelected) { const { tag } = this.singleItemToBeDeleted !== null ? this.repo.list[this.singleItemToBeDeleted] @@ -79,6 +79,9 @@ export default { { count: selectedCount }, ); }, + singleItemSelected() { + return this.singleItemToBeDeleted !== null || this.itemsToBeDeleted.length === 1; + }, }, methods: { ...mapActions(['fetchList', 'deleteItems']), diff --git a/spec/javascripts/registry/components/table_registry_spec.js b/spec/javascripts/registry/components/table_registry_spec.js index 9dca29d4702..556a66c5666 100644 --- a/spec/javascripts/registry/components/table_registry_spec.js +++ b/spec/javascripts/registry/components/table_registry_spec.js @@ -1,13 +1,14 @@ import Vue from 'vue'; import tableRegistry from '~/registry/components/table_registry.vue'; import store from '~/registry/stores'; +import { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; import { repoPropsData } from '../mock_data'; const [firstImage, secondImage] = repoPropsData.list; describe('table registry', () => { let vm; - let Component; + const Component = Vue.extend(tableRegistry); const bulkDeletePath = 'path'; const findDeleteBtn = () => vm.$el.querySelector('.js-delete-registry'); @@ -15,17 +16,19 @@ describe('table registry', () => { const findSelectAllCheckbox = () => vm.$el.querySelector('.js-select-all-checkbox > input'); const findAllRowCheckboxes = () => Array.from(vm.$el.querySelectorAll('.js-select-checkbox input')); + const confirmationModal = (child = '') => document.querySelector(`#${vm.modalId} ${child}`); const createComponent = () => { - Component = Vue.extend(tableRegistry); - vm = new Component({ + vm = mountComponentWithStore(Component, { store, - propsData: { + props: { repo: repoPropsData, }, - }).$mount(); + }); }; + const toggleSelectAll = () => vm.selectAll(); + beforeEach(() => { createComponent(); }); @@ -34,31 +37,28 @@ describe('table registry', () => { vm.$destroy(); }); - it('should render a table with the registry list', () => { - expect(vm.$el.querySelectorAll('table tbody tr').length).toEqual(repoPropsData.list.length); - }); + describe('rendering', () => { + it('should render a table with the registry list', () => { + expect(vm.$el.querySelectorAll('table tbody tr').length).toEqual(repoPropsData.list.length); + }); - it('should render registry tag', () => { - const textRendered = vm.$el - .querySelector('.table tbody tr') - .textContent.trim() - .replace(/\s\s+/g, ' '); + it('should render registry tag', () => { + const textRendered = vm.$el + .querySelector('.table tbody tr') + .textContent.trim() + .replace(/\s\s+/g, ' '); - expect(textRendered).toContain(repoPropsData.list[0].tag); - expect(textRendered).toContain(repoPropsData.list[0].shortRevision); - expect(textRendered).toContain(repoPropsData.list[0].layers); - expect(textRendered).toContain(repoPropsData.list[0].size); + expect(textRendered).toContain(repoPropsData.list[0].tag); + expect(textRendered).toContain(repoPropsData.list[0].shortRevision); + expect(textRendered).toContain(repoPropsData.list[0].layers); + expect(textRendered).toContain(repoPropsData.list[0].size); + }); }); describe('multi select', () => { - beforeEach(() => { - vm.itemsToBeDeleted = []; - }); - it('should support multiselect and selecting a row should enable delete button', done => { findSelectAllCheckbox().click(); - - vm.selectAll(); + toggleSelectAll(); expect(findSelectAllCheckbox().checked).toBe(true); @@ -70,7 +70,7 @@ describe('table registry', () => { it('selecting all checkbox should select all rows and enable delete button', done => { findSelectAllCheckbox().click(); - vm.selectAll(); + toggleSelectAll(); Vue.nextTick(() => { const checkedValues = findAllRowCheckboxes().filter(x => x.checked); @@ -82,8 +82,8 @@ describe('table registry', () => { it('deselecting select all checkbox should deselect all rows and disable delete button', done => { findSelectAllCheckbox().click(); - vm.selectAll(); // Select them all on - vm.selectAll(); // Select them all off + toggleSelectAll(); // Select them all on + toggleSelectAll(); // Select them all off Vue.nextTick(() => { const checkedValues = findAllRowCheckboxes().filter(x => x.checked); @@ -95,7 +95,7 @@ describe('table registry', () => { it('should delete multiple items when multiple items are selected', done => { findSelectAllCheckbox().click(); - vm.selectAll(); + toggleSelectAll(); Vue.nextTick(() => { expect(vm.itemsToBeDeleted).toEqual([0, 1]); @@ -105,8 +105,8 @@ describe('table registry', () => { spyOn(vm, 'deleteItems').and.returnValue(Promise.resolve()); Vue.nextTick(() => { - const modal = document.querySelector(`#${vm.modalId}`); - document.querySelector(`#${vm.modalId} .btn-danger`).click(); + const modal = confirmationModal(); + confirmationModal('.btn-danger').click(); expect(modal).toExist(); @@ -146,7 +146,7 @@ describe('table registry', () => { spyOn(vm, 'deleteItems').and.returnValue(Promise.resolve()); Vue.nextTick(() => { - document.querySelector(`#${vm.modalId} .btn-danger`).click(); + confirmationModal('.btn-danger').click(); expect(vm.itemsToBeDeleted).toEqual([]); expect(vm.deleteItems).toHaveBeenCalledWith({ @@ -164,4 +164,27 @@ describe('table registry', () => { expect(vm.$el.querySelector('.gl-pagination')).toBeDefined(); }); }); + + describe('modal content', () => { + it('should show the singular title and image name when deleting a single image', done => { + findDeleteBtnRow().click(); + + Vue.nextTick(() => { + expect(vm.modalTitle).toBe('Remove image'); + expect(vm.modalDescription).toContain(firstImage.tag); + done(); + }); + }); + + it('should show the plural title and image count when deleting more than one image', done => { + findSelectAllCheckbox().click(); + toggleSelectAll(); + + Vue.nextTick(() => { + expect(vm.modalTitle).toBe('Remove images'); + expect(vm.modalDescription).toContain('2 images'); + done(); + }); + }); + }); }); From 786133d31434d1dbb185b2c0ff5eee663f5841d5 Mon Sep 17 00:00:00 2001 From: Nick Kipling Date: Fri, 19 Jul 2019 16:12:17 +0100 Subject: [PATCH 07/12] Updated select all to be more explicit --- .../registry/components/table_registry.vue | 20 ++++++++++++------- .../components/table_registry_spec.js | 19 ++++++++---------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/app/assets/javascripts/registry/components/table_registry.vue b/app/assets/javascripts/registry/components/table_registry.vue index 807b81c66e9..ff9f8871a8c 100644 --- a/app/assets/javascripts/registry/components/table_registry.vue +++ b/app/assets/javascripts/registry/components/table_registry.vue @@ -126,15 +126,21 @@ export default { showError(message) { createFlash(errorMessages[message]); }, - selectAll() { - if (!this.selectAllChecked) { - this.itemsToBeDeleted = this.repo.list.map((x, idx) => idx); - this.selectAllChecked = true; + onSelectAllChange() { + if (this.selectAllChecked) { + this.deselectAll(); } else { - this.itemsToBeDeleted = []; - this.selectAllChecked = false; + this.selectAll(); } }, + selectAll() { + this.itemsToBeDeleted = this.repo.list.map((x, idx) => idx); + this.selectAllChecked = true; + }, + deselectAll() { + this.itemsToBeDeleted = []; + this.selectAllChecked = false; + }, updateItemsToBeDeleted(idx) { const delIdx = this.itemsToBeDeleted.findIndex(x => x === idx); @@ -162,7 +168,7 @@ export default { v-if="repo.canDelete" class="js-select-all-checkbox" :checked="selectAllChecked" - @change="selectAll" + @change="onSelectAllChange" /> 		{{ s__('ContainerRegistry|Tag') }}
@@ -236,7 +242,7 @@ export default { :aria-label="s__('ContainerRegistry|Remove image')" variant="danger" class="js-delete-registry-row float-right btn-inverted btn-border-color btn-icon" - @click="setSingleItemToBeDeleted(idx)" + @click="addSingleItemToBeDeleted(index)" > @@ -255,7 +261,7 @@ export default { :modal-id="modalId" ok-variant="danger" @ok="handleDeleteRegistry" - @cancel="resetSingleItemToBeDeleted" + @cancel="removeSingleItemToBeDeleted" > diff --git a/app/assets/stylesheets/pages/container_registry.scss b/app/assets/stylesheets/pages/container_registry.scss index ed9de6f7e30..6e6a3a1a394 100644 --- a/app/assets/stylesheets/pages/container_registry.scss +++ b/app/assets/stylesheets/pages/container_registry.scss @@ -32,26 +32,20 @@ .table.tags { margin-bottom: 0; - th { - height: 55px; - } - - tr { - &:hover { - td { - &.action-buttons { - opacity: 1; - } - } - } - - td.check { + .image-row { + .check { padding-right: $gl-padding; width: 5%; } - td.action-buttons { + .action-buttons { opacity: 0; } + + &:hover { + .action-buttons { + opacity: 1; + } + } } } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 557a8f2681e..156a527163e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3130,14 +3130,13 @@ msgid "ContainerRegistry|Quick Start" msgstr "" msgid "ContainerRegistry|Remove image" -msgstr "" +msgid_plural "ContainerRegistry|Remove images" +msgstr[0] "" +msgstr[1] "" msgid "ContainerRegistry|Remove image(s) and tags" msgstr "" -msgid "ContainerRegistry|Remove images" -msgstr "" - msgid "ContainerRegistry|Remove repository" msgstr "" diff --git a/spec/javascripts/registry/components/table_registry_spec.js b/spec/javascripts/registry/components/table_registry_spec.js index a96a505a3a4..2ca7aa30c0e 100644 --- a/spec/javascripts/registry/components/table_registry_spec.js +++ b/spec/javascripts/registry/components/table_registry_spec.js @@ -47,6 +47,7 @@ describe('table registry', () => { const textRendered = vm.$el .querySelector('.table tbody tr') .textContent.trim() + // replace additional whitespace characters (e.g. new lines) with a single empty space .replace(/\s\s+/g, ' '); expect(textRendered).toContain(repoPropsData.list[0].tag); From 918e7d43dfe614475ee2dd2b6f4c765726db6ef4 Mon Sep 17 00:00:00 2001 From: Nick Kipling Date: Fri, 26 Jul 2019 19:42:49 +0100 Subject: [PATCH 10/12] Reworked how deletion works with multi vs single Single deletion no longer requires a prop Modal description is now generated on demand Added dedicated functions for deleting Updated tests to match new function naming Updated css class name to be more specific --- .../registry/components/table_registry.vue | 109 +++++++++--------- .../javascripts/registry/stores/actions.js | 3 +- .../stylesheets/pages/container_registry.scss | 2 +- .../components/table_registry_spec.js | 8 +- 4 files changed, 64 insertions(+), 58 deletions(-) diff --git a/app/assets/javascripts/registry/components/table_registry.vue b/app/assets/javascripts/registry/components/table_registry.vue index 90d6a4ce27f..df665b36c4b 100644 --- a/app/assets/javascripts/registry/components/table_registry.vue +++ b/app/assets/javascripts/registry/components/table_registry.vue @@ -41,6 +41,7 @@ export default { itemsToBeDeleted: [], modalId: `confirm-image-deletion-modal-${this.repo.id}`, selectAllChecked: false, + modalDescription: '', }; }, computed: { @@ -54,67 +55,68 @@ export default { return n__( 'ContainerRegistry|Remove image', 'ContainerRegistry|Remove images', - this.singleItemSelected ? 1 : this.itemsToBeDeleted.length, + this.itemsToBeDeleted.length === 0 ? 1 : this.itemsToBeDeleted.length, ); }, - modalDescription() { - const selectedCount = this.itemsToBeDeleted.length; - - if (this.singleItemSelected) { - // Attempt to pull 'single' property if it's an object in this.itemsToBeDeleted - // Otherwise, simply use the int value of the selected row - const { single: itemIndex = this.itemsToBeDeleted[0] } = this.itemsToBeDeleted[0]; - const { tag } = this.repo.list[itemIndex]; - - return sprintf( - s__(`ContainerRegistry|You are about to delete the image %{title}. This will - delete the image and all tags pointing to this image.`), - { title: `${this.repo.name}:${tag}` }, - ); - } - - return sprintf( - s__(`ContainerRegistry|You are about to delete %{count} images. This will - delete the images and all tags pointing to them.`), - { count: selectedCount }, - ); - }, - singleItemSelected() { - return this.findSingleRowToDelete || this.itemsToBeDeleted.length === 1; - }, - findSingleRowToDelete() { - return this.itemsToBeDeleted.find(x => x.single !== undefined); - }, }, methods: { - ...mapActions(['fetchList', 'deleteItems']), + ...mapActions(['fetchList', 'deleteItem', 'multiDeleteItems']), + setModalDescription(itemsToDeleteLength, itemIndex) { + if (itemsToDeleteLength) { + this.modalDescription = sprintf( + s__(`ContainerRegistry|You are about to delete %{count} images. This will + delete the images and all tags pointing to them.`), + { count: itemsToDeleteLength }, + ); + } else { + const { tag } = this.repo.list[itemIndex]; + + this.modalDescription = sprintf( + s__(`ContainerRegistry|You are about to delete the image %{title}. This will + delete the image and all tags pointing to this image.`), + { title: `${this.repo.name}:${tag}` }, + ); + } + }, layers(item) { return item.layers ? n__('%d layer', '%d layers', item.layers) : ''; }, formatSize(size) { return numberToHumanSize(size); }, - addSingleItemToBeDeleted(index) { - this.itemsToBeDeleted.push({ single: index }); + removeModalEvents() { + this.$refs.deleteModal.$refs.modal.$off('ok'); + this.$refs.deleteModal.$refs.modal.$off('hide'); }, - removeSingleItemToBeDeleted() { - const singleIndex = this.itemsToBeDeleted.findIndex(x => x.single !== undefined); + deleteSingleItem(index) { + this.setModalDescription(0, index); - if (singleIndex > -1) { - this.itemsToBeDeleted.splice(singleIndex, 1); - } + this.$refs.deleteModal.$refs.modal.$once('ok', () => { + this.removeModalEvents(); + this.handleSingleDelete(this.repo.list[index]); + }); + + this.$refs.deleteModal.$refs.modal.$once('hide', this.removeModalEvents); }, - handleDeleteRegistry() { - let { itemsToBeDeleted } = this; - - if (this.findSingleRowToDelete) { - itemsToBeDeleted = [this.findSingleRowToDelete.single]; - } + deleteMultipleItems() { + this.$refs.deleteModal.$refs.modal.$once('ok', () => { + this.removeModalEvents(); + this.handleMultipleDelete(); + }); + this.$refs.deleteModal.$refs.modal.$once('hide', this.removeModalEvents); + }, + handleSingleDelete(itemToDelete) { + this.deleteItem(itemToDelete) + .then(() => this.fetchList({ repo: this.repo })) + .catch(() => this.showError(errorMessagesTypes.DELETE_REGISTRY)); + }, + handleMultipleDelete() { + const { itemsToBeDeleted } = this; this.itemsToBeDeleted = []; if (this.bulkDeletePath) { - this.deleteItems({ + this.multiDeleteItems({ path: this.bulkDeletePath, items: itemsToBeDeleted.map(x => this.repo.list[x].tag), }) @@ -142,6 +144,7 @@ export default { selectAll() { this.itemsToBeDeleted = this.repo.list.map((x, index) => index); this.selectAllChecked = true; + this.setModalDescription(this.itemsToBeDeleted.length); }, deselectAll() { this.itemsToBeDeleted = []; @@ -160,6 +163,12 @@ export default { this.selectAllChecked = true; } } + + if (this.itemsToBeDeleted.length === 1) { + this.setModalDescription(0, this.itemsToBeDeleted[0]); + } else if (this.itemsToBeDeleted.length > 1) { + this.setModalDescription(this.itemsToBeDeleted.length); + } }, }, }; @@ -191,13 +200,14 @@ export default { variant="danger" :title="s__('ContainerRegistry|Remove selected images')" :aria-label="s__('ContainerRegistry|Remove selected images')" + @click="deleteMultipleItems()" >
@@ -257,12 +267,7 @@ export default { :page-info="repo.pagination" /> - +

diff --git a/app/assets/javascripts/registry/stores/actions.js b/app/assets/javascripts/registry/stores/actions.js index 4c20c003c5a..a2e0130e79e 100644 --- a/app/assets/javascripts/registry/stores/actions.js +++ b/app/assets/javascripts/registry/stores/actions.js @@ -36,7 +36,8 @@ export const fetchList = ({ commit }, { repo, page }) => { }; export const deleteItem = (_, item) => axios.delete(item.destroyPath); -export const deleteItems = (_, { path, items }) => axios.delete(path, { params: { ids: items } }); +export const multiDeleteItems = (_, { path, items }) => + axios.delete(path, { params: { ids: items } }); export const setMainEndpoint = ({ commit }, data) => commit(types.SET_MAIN_ENDPOINT, data); export const toggleLoading = ({ commit }) => commit(types.TOGGLE_MAIN_LOADING); diff --git a/app/assets/stylesheets/pages/container_registry.scss b/app/assets/stylesheets/pages/container_registry.scss index 6e6a3a1a394..0f4bdb219a3 100644 --- a/app/assets/stylesheets/pages/container_registry.scss +++ b/app/assets/stylesheets/pages/container_registry.scss @@ -32,7 +32,7 @@ .table.tags { margin-bottom: 0; - .image-row { + .registry-image-row { .check { padding-right: $gl-padding; width: 5%; diff --git a/spec/javascripts/registry/components/table_registry_spec.js b/spec/javascripts/registry/components/table_registry_spec.js index 2ca7aa30c0e..b6a37abe4f7 100644 --- a/spec/javascripts/registry/components/table_registry_spec.js +++ b/spec/javascripts/registry/components/table_registry_spec.js @@ -101,7 +101,7 @@ describe('table registry', () => { expect(findDeleteBtn().disabled).toBe(false); findDeleteBtn().click(); - spyOn(vm, 'deleteItems').and.returnValue(Promise.resolve()); + spyOn(vm, 'multiDeleteItems').and.returnValue(Promise.resolve()); Vue.nextTick(() => { const modal = confirmationModal(); @@ -111,7 +111,7 @@ describe('table registry', () => { Vue.nextTick(() => { expect(vm.itemsToBeDeleted).toEqual([]); - expect(vm.deleteItems).toHaveBeenCalledWith({ + expect(vm.multiDeleteItems).toHaveBeenCalledWith({ path: bulkDeletePath, items: [firstImage.tag, secondImage.tag], }); @@ -142,13 +142,13 @@ describe('table registry', () => { expect(vm.itemsToBeDeleted).toEqual([0]); expect(findDeleteBtn().disabled).toBe(false); findDeleteBtn().click(); - spyOn(vm, 'deleteItems').and.returnValue(Promise.resolve()); + spyOn(vm, 'multiDeleteItems').and.returnValue(Promise.resolve()); Vue.nextTick(() => { confirmationModal('.btn-danger').click(); expect(vm.itemsToBeDeleted).toEqual([]); - expect(vm.deleteItems).toHaveBeenCalledWith({ + expect(vm.multiDeleteItems).toHaveBeenCalledWith({ path: bulkDeletePath, items: [firstImage.tag], }); From 128a04ef0ec10e4524e138a61143d0d1ba1f54ac Mon Sep 17 00:00:00 2001 From: Nick Kipling Date: Tue, 30 Jul 2019 09:00:30 +0100 Subject: [PATCH 11/12] Adjustments to event removal and modal description --- .../registry/components/table_registry.vue | 29 +++++++++---------- .../components/table_registry_spec.js | 1 + 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/app/assets/javascripts/registry/components/table_registry.vue b/app/assets/javascripts/registry/components/table_registry.vue index df665b36c4b..e9067bc2b56 100644 --- a/app/assets/javascripts/registry/components/table_registry.vue +++ b/app/assets/javascripts/registry/components/table_registry.vue @@ -59,14 +59,17 @@ export default { ); }, }, + mounted() { + this.$refs.deleteModal.$refs.modal.$on('hide', this.removeModalEvents); + }, methods: { ...mapActions(['fetchList', 'deleteItem', 'multiDeleteItems']), - setModalDescription(itemsToDeleteLength, itemIndex) { - if (itemsToDeleteLength) { + setModalDescription(itemIndex = -1) { + if (itemIndex === -1) { this.modalDescription = sprintf( s__(`ContainerRegistry|You are about to delete %{count} images. This will delete the images and all tags pointing to them.`), - { count: itemsToDeleteLength }, + { count: this.itemsToBeDeleted.length }, ); } else { const { tag } = this.repo.list[itemIndex]; @@ -86,25 +89,26 @@ export default { }, removeModalEvents() { this.$refs.deleteModal.$refs.modal.$off('ok'); - this.$refs.deleteModal.$refs.modal.$off('hide'); }, deleteSingleItem(index) { - this.setModalDescription(0, index); + this.setModalDescription(index); this.$refs.deleteModal.$refs.modal.$once('ok', () => { this.removeModalEvents(); this.handleSingleDelete(this.repo.list[index]); }); - - this.$refs.deleteModal.$refs.modal.$once('hide', this.removeModalEvents); }, deleteMultipleItems() { + if (this.itemsToBeDeleted.length === 1) { + this.setModalDescription(this.itemsToBeDeleted[0]); + } else if (this.itemsToBeDeleted.length > 1) { + this.setModalDescription(); + } + this.$refs.deleteModal.$refs.modal.$once('ok', () => { this.removeModalEvents(); this.handleMultipleDelete(); }); - - this.$refs.deleteModal.$refs.modal.$once('hide', this.removeModalEvents); }, handleSingleDelete(itemToDelete) { this.deleteItem(itemToDelete) @@ -144,7 +148,6 @@ export default { selectAll() { this.itemsToBeDeleted = this.repo.list.map((x, index) => index); this.selectAllChecked = true; - this.setModalDescription(this.itemsToBeDeleted.length); }, deselectAll() { this.itemsToBeDeleted = []; @@ -163,12 +166,6 @@ export default { this.selectAllChecked = true; } } - - if (this.itemsToBeDeleted.length === 1) { - this.setModalDescription(0, this.itemsToBeDeleted[0]); - } else if (this.itemsToBeDeleted.length > 1) { - this.setModalDescription(this.itemsToBeDeleted.length); - } }, }, }; diff --git a/spec/javascripts/registry/components/table_registry_spec.js b/spec/javascripts/registry/components/table_registry_spec.js index b6a37abe4f7..9c7439206ef 100644 --- a/spec/javascripts/registry/components/table_registry_spec.js +++ b/spec/javascripts/registry/components/table_registry_spec.js @@ -177,6 +177,7 @@ describe('table registry', () => { it('should show the plural title and image count when deleting more than one image', done => { selectAllCheckboxes(); + vm.setModalDescription(); Vue.nextTick(() => { expect(vm.modalTitle).toBe('Remove images'); From f3de7855f90ed6785f546ed4831e3cc9d34c63ad Mon Sep 17 00:00:00 2001 From: Giorgenes Gelatti Date: Thu, 15 Aug 2019 16:22:13 +1000 Subject: [PATCH 12/12] Limit registry tag bulk delete to 15 items --- app/controllers/projects/registry/tags_controller.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/app/controllers/projects/registry/tags_controller.rb b/app/controllers/projects/registry/tags_controller.rb index 633a7865cfe..54e2faa2dd7 100644 --- a/app/controllers/projects/registry/tags_controller.rb +++ b/app/controllers/projects/registry/tags_controller.rb @@ -5,6 +5,8 @@ module Projects class TagsController < ::Projects::Registry::ApplicationController before_action :authorize_destroy_container_image!, only: [:destroy] + LIMIT = 15 + def index respond_to do |format| format.json do @@ -34,7 +36,13 @@ module Projects return end - @tags = (params[:ids] || []).map { |tag_name| image.tag(tag_name) } + tag_names = params[:ids] || [] + if tag_names.size > LIMIT + head :bad_request + return + end + + @tags = tag_names.map { |tag_name| image.tag(tag_name) } unless @tags.all? { |tag| tag.valid_name? } head :bad_request return @@ -55,7 +63,7 @@ module Projects private def tags - Kaminari::PaginatableArray.new(image.tags, limit: 15) + Kaminari::PaginatableArray.new(image.tags, limit: LIMIT) end def image