From 71237cf5cfc0390e324171e3dd648454ae24f767 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 6 Feb 2024 14:29:37 +0100 Subject: [PATCH 1/6] Add unit test for Tools copyLink utility --- client/src/components/Tool/utilities.test.ts | 29 ++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 client/src/components/Tool/utilities.test.ts diff --git a/client/src/components/Tool/utilities.test.ts b/client/src/components/Tool/utilities.test.ts new file mode 100644 index 000000000000..2a22aaddfbe9 --- /dev/null +++ b/client/src/components/Tool/utilities.test.ts @@ -0,0 +1,29 @@ +import { copyLink } from "./utilities"; + +const writeText = jest.fn(); + +Object.assign(navigator, { + clipboard: { + writeText, + }, +}); + +describe("copyLink", () => { + beforeEach(() => { + (navigator.clipboard.writeText as jest.Mock).mockResolvedValue(undefined); + }); + + it("should copy the link to the clipboard", () => { + const toolId = "MyToolId"; + copyLink(toolId); + expect(writeText).toHaveBeenCalledTimes(1); + expect(writeText).toHaveBeenCalledWith(expect.stringContaining(toolId)); + }); + + it("should encode the tool id", () => { + const toolId = "My Tool Id"; + copyLink(toolId); + expect(writeText).toHaveBeenCalledTimes(1); + expect(writeText).toHaveBeenCalledWith(expect.stringContaining("My%20Tool%20Id")); + }); +}); From 22d507112c72839216490069484b5cee6a468ca6 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 6 Feb 2024 14:30:49 +0100 Subject: [PATCH 2/6] Fix tool link encoding Makes it easier to share when pasting it in oder contexts outside the web browser. --- client/src/components/Tool/utilities.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/client/src/components/Tool/utilities.js b/client/src/components/Tool/utilities.js index 1b0a6d5a0ad2..84f9d2bd3fd2 100644 --- a/client/src/components/Tool/utilities.js +++ b/client/src/components/Tool/utilities.js @@ -2,7 +2,9 @@ import { getAppRoot } from "onload/loadConfig"; import { copy } from "utils/clipboard"; export function copyLink(toolId, message) { - copy(`${window.location.origin + getAppRoot()}root?tool_id=${toolId}`, message); + const link = `${window.location.origin + getAppRoot()}root?tool_id=${toolId}`; + // Encode the link to handle special characters in tool id + copy(encodeURI(link), message); } export function copyId(toolId, message) { From d7877275610668b77fb0518d935781ad7733ddbe Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 6 Feb 2024 14:53:17 +0100 Subject: [PATCH 3/6] Add test case for tool IDs with `+` --- client/src/components/Tool/utilities.test.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/client/src/components/Tool/utilities.test.ts b/client/src/components/Tool/utilities.test.ts index 2a22aaddfbe9..8295d1e2bfe6 100644 --- a/client/src/components/Tool/utilities.test.ts +++ b/client/src/components/Tool/utilities.test.ts @@ -20,10 +20,17 @@ describe("copyLink", () => { expect(writeText).toHaveBeenCalledWith(expect.stringContaining(toolId)); }); - it("should encode the tool id", () => { + it("should encode the tool id with spaces", () => { const toolId = "My Tool Id"; copyLink(toolId); expect(writeText).toHaveBeenCalledTimes(1); expect(writeText).toHaveBeenCalledWith(expect.stringContaining("My%20Tool%20Id")); }); + + it("should not encode the character '+' in the tool id", () => { + const toolId = "My Tool Id+1"; + copyLink(toolId); + expect(writeText).toHaveBeenCalledTimes(1); + expect(writeText).toHaveBeenCalledWith(expect.stringContaining("My%20Tool%20Id+1")); + }); }); From 5f14cd25a6ecd5f71fdcfc620320f11863bf6fa2 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 7 Feb 2024 14:50:28 +0100 Subject: [PATCH 4/6] Add test coverage for "any" state in menu options --- .../SelectionOperations.test.js | 47 +++++++++++++++++-- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.test.js b/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.test.js index c66ecb6d197d..147e38622ceb 100644 --- a/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.test.js +++ b/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.test.js @@ -82,20 +82,33 @@ describe("History Selection Operations", () => { expect(wrapper.find(option).exists()).toBe(false); }); - it("should display 'unhide' option only on hidden items", async () => { + it("should display 'unhide' option on hidden items", async () => { const option = '[data-description="unhide option"]'; expect(wrapper.find(option).exists()).toBe(false); await wrapper.setProps({ filterText: "visible:false" }); expect(wrapper.find(option).exists()).toBe(true); }); - it("should display 'delete' option only on non-deleted items", async () => { + it("should display 'unhide' option when hidden and visible items are mixed", async () => { + const option = '[data-description="unhide option"]'; + 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"]'; 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"]'; + 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"]'; expect(wrapper.find(option).exists()).toBe(true); @@ -103,7 +116,7 @@ describe("History Selection Operations", () => { expect(wrapper.find(option).exists()).toBe(true); }); - it("should display 'undelete' option only on deleted and non-purged items", async () => { + it("should display 'undelete' option on deleted and non-purged items", async () => { const option = '[data-description="undelete option"]'; expect(wrapper.find(option).exists()).toBe(false); await wrapper.setProps({ @@ -113,6 +126,15 @@ describe("History Selection Operations", () => { expect(wrapper.find(option).exists()).toBe(true); }); + it("should display 'undelete' option when non-purged items (deleted or not) are mixed", async () => { + const option = '[data-description="undelete option"]'; + await wrapper.setProps({ + filterText: "deleted:any", + contentSelection: getNonPurgedContentSelection(), + }); + expect(wrapper.find(option).exists()).toBe(true); + }); + it("should not display 'undelete' when is manual selection mode and all selected items are purged", async () => { const option = '[data-description="undelete option"]'; await wrapper.setProps({ @@ -134,6 +156,17 @@ describe("History Selection Operations", () => { expect(wrapper.find(option).exists()).toBe(true); }); + it("should display 'undelete' option when is query selection mode and filtering by any deleted state", async () => { + const option = '[data-description="undelete option"]'; + // 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, + }); + expect(wrapper.find(option).exists()).toBe(true); + }); + it("should display collection building options only on visible and non-deleted items", async () => { const buildListOption = '[data-description="build list"]'; const buildPairOption = '[data-description="build pair"]'; @@ -149,6 +182,14 @@ describe("History Selection Operations", () => { 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: "deleted:any" }); + expect(wrapper.find(buildListOption).exists()).toBe(false); + expect(wrapper.find(buildPairOption).exists()).toBe(false); + expect(wrapper.find(buildListOfPairsOption).exists()).toBe(false); }); it("should display list building option when all are selected", async () => { From bc0b3eb98d0d7426cc425ce84a64a24c83efd364 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 7 Feb 2024 14:52:53 +0100 Subject: [PATCH 5/6] Fix menu display conditions to adapt to "any" filter --- .../HistoryOperations/SelectionOperations.vue | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.vue b/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.vue index 4954da962ccd..27e478cea34b 100644 --- a/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.vue +++ b/client/src/components/History/CurrentHistory/HistoryOperations/SelectionOperations.vue @@ -25,7 +25,10 @@ data-description="undelete option"> Undelete - + Delete @@ -193,10 +196,14 @@ export default { computed: { /** @returns {Boolean} */ showHidden() { - return HistoryFilters.checkFilter(this.filterText, "visible", false); + return !HistoryFilters.checkFilter(this.filterText, "visible", true); }, /** @returns {Boolean} */ showDeleted() { + return !HistoryFilters.checkFilter(this.filterText, "deleted", false); + }, + /** @returns {Boolean} */ + showStrictDeleted() { return HistoryFilters.checkFilter(this.filterText, "deleted", true); }, /** @returns {Boolean} */ From f60f0ae6e3710a85fca7110f55bd722e8d0fe43c Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 7 Feb 2024 14:58:52 +0100 Subject: [PATCH 6/6] Fix new edge case where all items might be already deleted This case was previously prevented at the UI level but should be considered here. --- lib/galaxy/webapps/galaxy/services/history_contents.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/services/history_contents.py b/lib/galaxy/webapps/galaxy/services/history_contents.py index 669361389068..68f5c789ea22 100644 --- a/lib/galaxy/webapps/galaxy/services/history_contents.py +++ b/lib/galaxy/webapps/galaxy/services/history_contents.py @@ -1417,8 +1417,12 @@ def _unhide(self, item: HistoryItemModel): def _delete(self, item: HistoryItemModel, trans: ProvidesHistoryContext): if isinstance(item, HistoryDatasetCollectionAssociation): - return self.dataset_collection_manager.delete(trans, "history", item.id, recursive=True, purge=False) - return self.hda_manager.delete(item, flush=self.flush) + self.dataset_collection_manager.delete(trans, "history", item.id, recursive=True, purge=False) + else: + self.hda_manager.delete(item, flush=self.flush) + # In the edge case where all selected items are already deleted we need to force an update + # otherwise the history will wait indefinitely for the items to be deleted + item.update() def _undelete(self, item: HistoryItemModel): if getattr(item, "purged", False):