From ae982a2afbb199ed5b20e59d233b3a2a49d4e6a6 Mon Sep 17 00:00:00 2001 From: nuwang <2070605+nuwang@users.noreply.github.com> Date: Fri, 9 Feb 2024 12:03:24 +0530 Subject: [PATCH 01/10] Add initializers to filesource options --- lib/galaxy/files/sources/__init__.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/files/sources/__init__.py b/lib/galaxy/files/sources/__init__.py index d369fcfe4293..cc6c854a36c1 100644 --- a/lib/galaxy/files/sources/__init__.py +++ b/lib/galaxy/files/sources/__init__.py @@ -53,13 +53,18 @@ class FilesSourceProperties(TypedDict): class FilesSourceOptions: - """Options to control behaviour of filesource operations, such as realize_to and write_from""" + """Options to control behavior of file source operations, such as realize_to, write_from and list.""" + + # Indicates access to the FS operation with intent to write. + # Even if a file source is "writeable" some directories (or elements) may be restricted or read-only + # so those should be skipped while browsing with writeable=True. + writeable: Optional[bool] = False # Property overrides for values initially configured through the constructor. For example # the HTTPFilesSource passes in additional http_headers through these properties, which # are merged with constructor defined http_headers. The interpretation of these properties # are filesystem specific. - extra_props: Optional[FilesSourceProperties] + extra_props: Optional[FilesSourceProperties] = {} class SingleFileSource(metaclass=abc.ABCMeta): From bffb7a99c50aeaafe8f4314d844f23d602e8f010 Mon Sep 17 00:00:00 2001 From: nuwang <2070605+nuwang@users.noreply.github.com> Date: Fri, 9 Feb 2024 15:57:29 +0530 Subject: [PATCH 02/10] Use dataclass to initialize default values --- lib/galaxy/files/sources/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/files/sources/__init__.py b/lib/galaxy/files/sources/__init__.py index cc6c854a36c1..24cf91f654e7 100644 --- a/lib/galaxy/files/sources/__init__.py +++ b/lib/galaxy/files/sources/__init__.py @@ -1,4 +1,5 @@ import abc +from dataclasses import dataclass, field import os import time from typing import ( @@ -52,6 +53,7 @@ class FilesSourceProperties(TypedDict): browsable: NotRequired[bool] +@dataclass class FilesSourceOptions: """Options to control behavior of file source operations, such as realize_to, write_from and list.""" @@ -64,7 +66,7 @@ class FilesSourceOptions: # the HTTPFilesSource passes in additional http_headers through these properties, which # are merged with constructor defined http_headers. The interpretation of these properties # are filesystem specific. - extra_props: Optional[FilesSourceProperties] = {} + extra_props: Optional[FilesSourceProperties] = field(default_factory=lambda: {}) class SingleFileSource(metaclass=abc.ABCMeta): From c7a5e4ff59cd93b3f336a6676c77524db51f0357 Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Fri, 9 Feb 2024 16:41:45 +0530 Subject: [PATCH 03/10] Code review suggestion Co-authored-by: Marius van den Beek --- lib/galaxy/files/sources/__init__.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/files/sources/__init__.py b/lib/galaxy/files/sources/__init__.py index 24cf91f654e7..40c2081d01c5 100644 --- a/lib/galaxy/files/sources/__init__.py +++ b/lib/galaxy/files/sources/__init__.py @@ -1,7 +1,10 @@ import abc -from dataclasses import dataclass, field import os import time +from dataclasses import ( + dataclass, + field, +) from typing import ( Any, ClassVar, @@ -66,7 +69,7 @@ class FilesSourceOptions: # the HTTPFilesSource passes in additional http_headers through these properties, which # are merged with constructor defined http_headers. The interpretation of these properties # are filesystem specific. - extra_props: Optional[FilesSourceProperties] = field(default_factory=lambda: {}) + extra_props: Optional[FilesSourceProperties] = field(default_factory=lambda: FilesSourceProperties()) class SingleFileSource(metaclass=abc.ABCMeta): From f130a3edf8ab8d2d8b614401035e42ab3894f717 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Mon, 12 Feb 2024 11:17:31 +0100 Subject: [PATCH 04/10] Refactor test to reduce selector duplication --- .../SelectionOperations.test.js | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.test.js b/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.test.js index 147e38622ceb..733652a214be 100644 --- a/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.test.js +++ b/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.test.js @@ -24,6 +24,7 @@ const TASKS_CONFIG = { enable_celery_tasks: true, }; +const getMenuSelectorFor = (option) => `[data-description="${option} option"]`; const getPurgedContentSelection = () => new Map([["FAKE_ID", { purged: true }]]); const getNonPurgedContentSelection = () => new Map([["FAKE_ID", { purged: false }]]); @@ -76,48 +77,48 @@ describe("History Selection Operations", () => { }); it("should display 'hide' option only on visible items", async () => { - const option = '[data-description="hide option"]'; + const option = getMenuSelectorFor("hide"); expect(wrapper.find(option).exists()).toBe(true); await wrapper.setProps({ filterText: "visible:false" }); expect(wrapper.find(option).exists()).toBe(false); }); it("should display 'unhide' option on hidden items", async () => { - const option = '[data-description="unhide option"]'; + const option = getMenuSelectorFor("unhide"); expect(wrapper.find(option).exists()).toBe(false); await wrapper.setProps({ filterText: "visible:false" }); expect(wrapper.find(option).exists()).toBe(true); }); it("should display 'unhide' option when hidden and visible items are mixed", async () => { - const option = '[data-description="unhide option"]'; + const option = getMenuSelectorFor("unhide"); expect(wrapper.find(option).exists()).toBe(false); await wrapper.setProps({ filterText: "visible:any" }); expect(wrapper.find(option).exists()).toBe(true); }); it("should display 'delete' option on non-deleted items", async () => { - const option = '[data-description="delete option"]'; + const option = getMenuSelectorFor("delete"); expect(wrapper.find(option).exists()).toBe(true); await wrapper.setProps({ filterText: "deleted:true" }); expect(wrapper.find(option).exists()).toBe(false); }); it("should display 'delete' option when non-deleted and deleted items are mixed", async () => { - const option = '[data-description="delete option"]'; + const option = getMenuSelectorFor("delete"); await wrapper.setProps({ filterText: "deleted:any" }); expect(wrapper.find(option).exists()).toBe(true); }); it("should display 'permanently delete' option always", async () => { - const option = '[data-description="purge option"]'; + const option = getMenuSelectorFor("purge"); expect(wrapper.find(option).exists()).toBe(true); await wrapper.setProps({ filterText: "deleted:true" }); expect(wrapper.find(option).exists()).toBe(true); }); it("should display 'undelete' option on deleted and non-purged items", async () => { - const option = '[data-description="undelete option"]'; + const option = getMenuSelectorFor("undelete"); expect(wrapper.find(option).exists()).toBe(false); await wrapper.setProps({ filterText: "deleted:true", @@ -127,7 +128,7 @@ describe("History Selection Operations", () => { }); it("should display 'undelete' option when non-purged items (deleted or not) are mixed", async () => { - const option = '[data-description="undelete option"]'; + const option = getMenuSelectorFor("undelete"); await wrapper.setProps({ filterText: "deleted:any", contentSelection: getNonPurgedContentSelection(), @@ -136,7 +137,7 @@ describe("History Selection Operations", () => { }); it("should not display 'undelete' when is manual selection mode and all selected items are purged", async () => { - const option = '[data-description="undelete option"]'; + const option = getMenuSelectorFor("undelete"); await wrapper.setProps({ filterText: "deleted:true", contentSelection: getPurgedContentSelection(), @@ -146,7 +147,7 @@ describe("History Selection Operations", () => { }); it("should display 'undelete' option when is query selection mode and filtering by deleted", async () => { - const option = '[data-description="undelete option"]'; + const option = getMenuSelectorFor("undelete"); // In query selection mode we don't know if some items may not be purged, so we allow to undelete await wrapper.setProps({ filterText: "deleted:true", @@ -157,7 +158,7 @@ describe("History Selection Operations", () => { }); it("should display 'undelete' option when is query selection mode and filtering by any deleted state", async () => { - const option = '[data-description="undelete option"]'; + const option = getMenuSelectorFor("undelete"); // In query selection mode we don't know if some items may not be purged, so we allow to undelete await wrapper.setProps({ filterText: "deleted:any", From 79fee2a7d42ab44a184007299fb3888876099e7d Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Mon, 12 Feb 2024 17:06:20 +0100 Subject: [PATCH 05/10] Add more test conditions for bulk menu --- .../SelectionOperations.test.js | 99 +++++++++++++------ 1 file changed, 71 insertions(+), 28 deletions(-) diff --git a/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.test.js b/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.test.js index 733652a214be..9acf326bdf40 100644 --- a/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.test.js +++ b/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.test.js @@ -25,8 +25,13 @@ const TASKS_CONFIG = { }; const getMenuSelectorFor = (option) => `[data-description="${option} option"]`; -const getPurgedContentSelection = () => new Map([["FAKE_ID", { purged: true }]]); -const getNonPurgedContentSelection = () => new Map([["FAKE_ID", { purged: false }]]); + +const getPurgedSelection = () => new Map([["FAKE_ID", { purged: true }]]); +const getNonPurgedSelection = () => new Map([["FAKE_ID", { purged: false }]]); +const getVisibleSelection = () => new Map([["FAKE_ID", { visible: true }]]); +const getHiddenSelection = () => new Map([["FAKE_ID", { visible: false }]]); +const getDeletedSelection = () => new Map([["FAKE_ID", { deleted: true }]]); +const getActiveSelection = () => new Map([["FAKE_ID", { deleted: false }]]); async function mountSelectionOperationsWrapper(config) { const wrapper = shallowMount( @@ -76,32 +81,62 @@ describe("History Selection Operations", () => { expect(wrapper.find('[data-description="selected count"]').text()).toContain("10"); }); - it("should display 'hide' option only on visible items", async () => { + it("should display 'hide' option on visible items", async () => { + const option = getMenuSelectorFor("hide"); + expect(wrapper.find(option).exists()).toBe(true); + await wrapper.setProps({ filterText: "visible:true" }); + expect(wrapper.find(option).exists()).toBe(true); + }); + + it("should display 'hide' option when visible and hidden items are mixed", async () => { const option = getMenuSelectorFor("hide"); expect(wrapper.find(option).exists()).toBe(true); + await wrapper.setProps({ filterText: "visible:any" }); + expect(wrapper.find(option).exists()).toBe(true); + }); + + it("should not display 'hide' option when only hidden items are selected", async () => { + const option = getMenuSelectorFor("hide"); + expect(wrapper.find(option).exists()).toBe(true); + await wrapper.setProps({ filterText: "visible:any", contentSelection: getHiddenSelection() }); + expect(wrapper.find(option).exists()).toBe(false); await wrapper.setProps({ filterText: "visible:false" }); expect(wrapper.find(option).exists()).toBe(false); }); it("should display 'unhide' option on hidden items", async () => { const option = getMenuSelectorFor("unhide"); - expect(wrapper.find(option).exists()).toBe(false); await wrapper.setProps({ filterText: "visible:false" }); expect(wrapper.find(option).exists()).toBe(true); }); it("should display 'unhide' option when hidden and visible items are mixed", async () => { const option = getMenuSelectorFor("unhide"); - expect(wrapper.find(option).exists()).toBe(false); await wrapper.setProps({ filterText: "visible:any" }); expect(wrapper.find(option).exists()).toBe(true); }); + it("should not display 'unhide' option when only visible items are selected", async () => { + const option = getMenuSelectorFor("unhide"); + await wrapper.setProps({ + filterText: "visible:any", + contentSelection: getVisibleSelection(), + }); + expect(wrapper.find(option).exists()).toBe(false); + }); + it("should display 'delete' option on non-deleted items", async () => { const option = getMenuSelectorFor("delete"); expect(wrapper.find(option).exists()).toBe(true); - await wrapper.setProps({ filterText: "deleted:true" }); - expect(wrapper.find(option).exists()).toBe(false); + await wrapper.setProps({ filterText: "deleted:false" }); + expect(wrapper.find(option).exists()).toBe(true); + }); + + it("should display 'delete' option on non-deleted items", async () => { + const option = getMenuSelectorFor("delete"); + expect(wrapper.find(option).exists()).toBe(true); + await wrapper.setProps({ filterText: "deleted:false" }); + expect(wrapper.find(option).exists()).toBe(true); }); it("should display 'delete' option when non-deleted and deleted items are mixed", async () => { @@ -110,10 +145,17 @@ describe("History Selection Operations", () => { expect(wrapper.find(option).exists()).toBe(true); }); + it("should not display 'delete' option when only deleted items are selected", async () => { + const option = getMenuSelectorFor("delete"); + expect(wrapper.find(option).exists()).toBe(true); + await wrapper.setProps({ filterText: "deleted:any", contentSelection: getDeletedSelection() }); + expect(wrapper.find(option).exists()).toBe(false); + }); + it("should display 'permanently delete' option always", async () => { const option = getMenuSelectorFor("purge"); expect(wrapper.find(option).exists()).toBe(true); - await wrapper.setProps({ filterText: "deleted:true" }); + await wrapper.setProps({ filterText: "deleted:any visible:any" }); expect(wrapper.find(option).exists()).toBe(true); }); @@ -122,7 +164,7 @@ describe("History Selection Operations", () => { expect(wrapper.find(option).exists()).toBe(false); await wrapper.setProps({ filterText: "deleted:true", - contentSelection: getNonPurgedContentSelection(), + contentSelection: getNonPurgedSelection(), }); expect(wrapper.find(option).exists()).toBe(true); }); @@ -131,16 +173,24 @@ describe("History Selection Operations", () => { const option = getMenuSelectorFor("undelete"); await wrapper.setProps({ filterText: "deleted:any", - contentSelection: getNonPurgedContentSelection(), + contentSelection: getNonPurgedSelection(), }); expect(wrapper.find(option).exists()).toBe(true); }); - it("should not display 'undelete' when is manual selection mode and all selected items are purged", async () => { + it("should not display 'undelete' when only non-deleted items are selected", async () => { const option = getMenuSelectorFor("undelete"); await wrapper.setProps({ - filterText: "deleted:true", - contentSelection: getPurgedContentSelection(), + filterText: "deleted:any", + contentSelection: getActiveSelection(), + }); + expect(wrapper.find(option).exists()).toBe(false); + }); + + it("should not display 'undelete' when only purged items are selected", async () => { + const option = getMenuSelectorFor("undelete"); + await wrapper.setProps({ + contentSelection: getPurgedSelection(), isQuerySelection: false, }); expect(wrapper.find(option).exists()).toBe(false); @@ -151,7 +201,7 @@ describe("History Selection Operations", () => { // In query selection mode we don't know if some items may not be purged, so we allow to undelete await wrapper.setProps({ filterText: "deleted:true", - contentSelection: getPurgedContentSelection(), + contentSelection: getPurgedSelection(), isQuerySelection: true, }); expect(wrapper.find(option).exists()).toBe(true); @@ -160,33 +210,26 @@ describe("History Selection Operations", () => { it("should display 'undelete' option when is query selection mode and filtering by any deleted state", async () => { const option = getMenuSelectorFor("undelete"); // In query selection mode we don't know if some items may not be purged, so we allow to undelete - await wrapper.setProps({ - filterText: "deleted:any", - contentSelection: getPurgedContentSelection(), - isQuerySelection: true, - }); + await wrapper.setProps({ filterText: "deleted:any", isQuerySelection: true }); expect(wrapper.find(option).exists()).toBe(true); }); - it("should display collection building options only on visible and non-deleted items", async () => { + it("should display collection building options only on active (non-deleted) items", async () => { const buildListOption = '[data-description="build list"]'; const buildPairOption = '[data-description="build pair"]'; const buildListOfPairsOption = '[data-description="build list of pairs"]'; + await wrapper.setProps({ filterText: "visible:true deleted:false" }); expect(wrapper.find(buildListOption).exists()).toBe(true); expect(wrapper.find(buildPairOption).exists()).toBe(true); expect(wrapper.find(buildListOfPairsOption).exists()).toBe(true); - await wrapper.setProps({ filterText: "visible:false" }); - expect(wrapper.find(buildListOption).exists()).toBe(false); - expect(wrapper.find(buildPairOption).exists()).toBe(false); - expect(wrapper.find(buildListOfPairsOption).exists()).toBe(false); await wrapper.setProps({ filterText: "deleted:true" }); expect(wrapper.find(buildListOption).exists()).toBe(false); expect(wrapper.find(buildPairOption).exists()).toBe(false); expect(wrapper.find(buildListOfPairsOption).exists()).toBe(false); - await wrapper.setProps({ filterText: "visible:any" }); - expect(wrapper.find(buildListOption).exists()).toBe(false); - expect(wrapper.find(buildPairOption).exists()).toBe(false); - expect(wrapper.find(buildListOfPairsOption).exists()).toBe(false); + await wrapper.setProps({ filterText: "visible:any deleted:false" }); + expect(wrapper.find(buildListOption).exists()).toBe(true); + expect(wrapper.find(buildPairOption).exists()).toBe(true); + expect(wrapper.find(buildListOfPairsOption).exists()).toBe(true); await wrapper.setProps({ filterText: "deleted:any" }); expect(wrapper.find(buildListOption).exists()).toBe(false); expect(wrapper.find(buildPairOption).exists()).toBe(false); From 811f8cf8fd73ae613605a59ccf2cddd52e5d0a3a Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Mon, 12 Feb 2024 17:06:58 +0100 Subject: [PATCH 06/10] Fix bulk menu conditions --- .../HistoryOperations/SelectionOperations.vue | 71 ++++++++++++++++--- 1 file changed, 60 insertions(+), 11 deletions(-) diff --git a/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.vue b/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.vue index 27e478cea34b..0930569fb50f 100644 --- a/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.vue +++ b/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.vue @@ -13,10 +13,13 @@ With {{ numSelected }} selected... - + Unhide - + Hide Undelete Delete @@ -195,20 +198,37 @@ export default { }, computed: { /** @returns {Boolean} */ - showHidden() { - return !HistoryFilters.checkFilter(this.filterText, "visible", true); + canUnhideSelection() { + return ( + this.areAllSelectedHidden || + (HistoryFilters.checkFilter(this.filterText, "visible", "any") && !this.areAllSelectedVisible) + ); + }, + /** @returns {Boolean} */ + canHideSelection() { + return ( + this.areAllSelectedVisible || + (HistoryFilters.checkFilter(this.filterText, "visible", "any") && !this.areAllSelectedHidden) + ); }, /** @returns {Boolean} */ showDeleted() { return !HistoryFilters.checkFilter(this.filterText, "deleted", false); }, /** @returns {Boolean} */ - showStrictDeleted() { - return HistoryFilters.checkFilter(this.filterText, "deleted", true); + canDeleteSelection() { + return ( + this.areAllSelectedActive || + (HistoryFilters.checkFilter(this.filterText, "deleted", "any") && !this.areAllSelectedDeleted) + ); + }, + /** @returns {Boolean} */ + canUndeleteSelection() { + return this.showDeleted && (this.isQuerySelection || !this.areAllSelectedPurged); }, /** @returns {Boolean} */ showBuildOptions() { - return !this.isQuerySelection && !this.showHidden && !this.showDeleted; + return !this.isQuerySelection && this.areAllSelectedActive && !this.showDeleted; }, /** @returns {Boolean} */ showBuildOptionForAll() { @@ -229,9 +249,6 @@ export default { noTagsSelected() { return this.selectedTags.length === 0; }, - canUndeleteSelection() { - return this.showDeleted && (this.isQuerySelection || !this.areAllSelectedPurged); - }, areAllSelectedPurged() { for (const item of this.contentSelection.values()) { if (Object.prototype.hasOwnProperty.call(item, "purged") && !item["purged"]) { @@ -240,6 +257,38 @@ export default { } return true; }, + areAllSelectedVisible() { + for (const item of this.contentSelection.values()) { + if (Object.prototype.hasOwnProperty.call(item, "visible") && !item["visible"]) { + return false; + } + } + return true; + }, + areAllSelectedHidden() { + for (const item of this.contentSelection.values()) { + if (Object.prototype.hasOwnProperty.call(item, "visible") && item["visible"]) { + return false; + } + } + return true; + }, + areAllSelectedActive() { + for (const item of this.contentSelection.values()) { + if (Object.prototype.hasOwnProperty.call(item, "deleted") && item["deleted"]) { + return false; + } + } + return true; + }, + areAllSelectedDeleted() { + for (const item of this.contentSelection.values()) { + if (Object.prototype.hasOwnProperty.call(item, "deleted") && !item["deleted"]) { + return false; + } + } + return true; + }, }, watch: { hasSelection(newVal) { From b7b5c05afd097bff69d007b5970ef0026ad95118 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Mon, 12 Feb 2024 17:20:36 +0100 Subject: [PATCH 07/10] Increase readability --- .../HistoryOperations/SelectionOperations.vue | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.vue b/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.vue index 0930569fb50f..c3165c5a5d96 100644 --- a/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.vue +++ b/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.vue @@ -199,17 +199,11 @@ export default { computed: { /** @returns {Boolean} */ canUnhideSelection() { - return ( - this.areAllSelectedHidden || - (HistoryFilters.checkFilter(this.filterText, "visible", "any") && !this.areAllSelectedVisible) - ); + return this.areAllSelectedHidden || (this.isAnyVisibilityAllowed && !this.areAllSelectedVisible); }, /** @returns {Boolean} */ canHideSelection() { - return ( - this.areAllSelectedVisible || - (HistoryFilters.checkFilter(this.filterText, "visible", "any") && !this.areAllSelectedHidden) - ); + return this.areAllSelectedVisible || (this.isAnyVisibilityAllowed && !this.areAllSelectedHidden); }, /** @returns {Boolean} */ showDeleted() { @@ -217,10 +211,7 @@ export default { }, /** @returns {Boolean} */ canDeleteSelection() { - return ( - this.areAllSelectedActive || - (HistoryFilters.checkFilter(this.filterText, "deleted", "any") && !this.areAllSelectedDeleted) - ); + return this.areAllSelectedActive || (this.isAnyDeletedStateAllowed && !this.areAllSelectedDeleted); }, /** @returns {Boolean} */ canUndeleteSelection() { @@ -289,6 +280,12 @@ export default { } return true; }, + isAnyVisibilityAllowed() { + return HistoryFilters.checkFilter(this.filterText, "visible", "any"); + }, + isAnyDeletedStateAllowed() { + return HistoryFilters.checkFilter(this.filterText, "deleted", "any"); + }, }, watch: { hasSelection(newVal) { From 8c3d967ad3bce144f0335c847cb694b17d2a6940 Mon Sep 17 00:00:00 2001 From: Ahmed Awan Date: Thu, 8 Feb 2024 13:14:31 -0600 Subject: [PATCH 08/10] fix published history collections are empty on expand Only check ownership in `__get_history_collection_instance` if: `collection_instance.history.published` = False --- lib/galaxy/managers/collections.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/managers/collections.py b/lib/galaxy/managers/collections.py index e941c69d680c..88d12d62b096 100644 --- a/lib/galaxy/managers/collections.py +++ b/lib/galaxy/managers/collections.py @@ -807,7 +807,7 @@ def __get_history_collection_instance( raise RequestParameterInvalidException("History dataset collection association not found") # TODO: that sure looks like a bug, we can't check ownership using the history of the object we're checking ownership for ... history = getattr(trans, "history", collection_instance.history) - if check_ownership: + if check_ownership and collection_instance.history.published is False: self.history_manager.error_unless_owner(collection_instance.history, trans.user, current_history=history) if check_accessible: self.history_manager.error_unless_accessible( From 4eab971489c79f3bcb159c4cb5276d88bdd75759 Mon Sep 17 00:00:00 2001 From: Ahmed Awan Date: Fri, 9 Feb 2024 09:57:59 -0600 Subject: [PATCH 09/10] remove `check_ownership` from `dataset_collections.contents` ... since this api only needs to `check_accessible` as it is not modifying a collection --- lib/galaxy/managers/collections.py | 2 +- lib/galaxy/webapps/galaxy/services/dataset_collections.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/managers/collections.py b/lib/galaxy/managers/collections.py index 88d12d62b096..e941c69d680c 100644 --- a/lib/galaxy/managers/collections.py +++ b/lib/galaxy/managers/collections.py @@ -807,7 +807,7 @@ def __get_history_collection_instance( raise RequestParameterInvalidException("History dataset collection association not found") # TODO: that sure looks like a bug, we can't check ownership using the history of the object we're checking ownership for ... history = getattr(trans, "history", collection_instance.history) - if check_ownership and collection_instance.history.published is False: + if check_ownership: self.history_manager.error_unless_owner(collection_instance.history, trans.user, current_history=history) if check_accessible: self.history_manager.error_unless_accessible( diff --git a/lib/galaxy/webapps/galaxy/services/dataset_collections.py b/lib/galaxy/webapps/galaxy/services/dataset_collections.py index d13e2066b3f4..c5f7cf4181a4 100644 --- a/lib/galaxy/webapps/galaxy/services/dataset_collections.py +++ b/lib/galaxy/webapps/galaxy/services/dataset_collections.py @@ -250,7 +250,7 @@ def contents( "Parameter instance_type not being 'history' is not yet implemented." ) hdca: "HistoryDatasetCollectionAssociation" = self.collection_manager.get_dataset_collection_instance( - trans, "history", hdca_id, check_ownership=True + trans, "history", hdca_id ) # check to make sure the dsc is part of the validated hdca From d18003e6616c05e2bf7e98d8c0e97a715523c80d Mon Sep 17 00:00:00 2001 From: Ahmed Awan Date: Mon, 12 Feb 2024 10:30:13 -0600 Subject: [PATCH 10/10] add api test for hdcas in public histories being accessible --- lib/galaxy_test/api/test_dataset_collections.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/galaxy_test/api/test_dataset_collections.py b/lib/galaxy_test/api/test_dataset_collections.py index c7601e2242a2..f24f8f70786b 100644 --- a/lib/galaxy_test/api/test_dataset_collections.py +++ b/lib/galaxy_test/api/test_dataset_collections.py @@ -418,6 +418,18 @@ def test_collection_contents_security(self, history_id): contents_response = self._get(contents_url) self._assert_status_code_is(contents_response, 403) + @requires_new_user + def test_published_collection_contents_accessible(self, history_id): + # request contents on an hdca that is in a published history + hdca, contents_url = self._create_collection_contents_pair(history_id) + with self._different_user(): + contents_response = self._get(contents_url) + self._assert_status_code_is(contents_response, 403) + self.dataset_populator.make_public(history_id) + with self._different_user(): + contents_response = self._get(contents_url) + self._assert_status_code_is(contents_response, 200) + def test_collection_contents_invalid_collection(self, history_id): # request an invalid collection from a valid hdca, should get 404 hdca, contents_url = self._create_collection_contents_pair(history_id)