From 987bbf9be58fa69669251b06452cc56b2d07b285 Mon Sep 17 00:00:00 2001 From: Ahmed Awan Date: Tue, 3 Sep 2024 11:59:22 -0500 Subject: [PATCH 01/15] [24.1] Add error handling for `SwitchToHistoryLink` Yet another case where errors were not being handled; in this case if the user clicks on the `SwitchToHistoryLink` when the history isn't shared/importable, we now `Toast` an error message. Error currently reproducable if you click on the history name in this invocation: https://usegalaxy.org/workflows/invocations/a11d3f54288f2b34?from_panel=true --- .../History/SwitchToHistoryLink.test.ts | 98 +++++++++++++++++-- .../History/SwitchToHistoryLink.vue | 34 +++++-- client/src/stores/historyStore.ts | 10 +- 3 files changed, 124 insertions(+), 18 deletions(-) diff --git a/client/src/components/History/SwitchToHistoryLink.test.ts b/client/src/components/History/SwitchToHistoryLink.test.ts index e45a043e5fa3..c443a07f65d0 100644 --- a/client/src/components/History/SwitchToHistoryLink.test.ts +++ b/client/src/components/History/SwitchToHistoryLink.test.ts @@ -1,27 +1,45 @@ import { createTestingPinia } from "@pinia/testing"; import { getLocalVue } from "@tests/jest/helpers"; -import { shallowMount } from "@vue/test-utils"; +import { mount } from "@vue/test-utils"; import axios from "axios"; import MockAdapter from "axios-mock-adapter"; import flushPromises from "flush-promises"; -import { type HistorySummary } from "@/api"; +import { type HistorySummaryExtended } from "@/api"; +import { Toast } from "@/composables/toast"; +import { useUserStore } from "@/stores/userStore"; import SwitchToHistoryLink from "./SwitchToHistoryLink.vue"; +jest.mock("@/composables/toast", () => ({ + Toast: { + error: jest.fn(), + }, +})); +jest.mock("vue-router/composables", () => ({ + useRouter: jest.fn(() => ({ + resolve: jest.fn((route) => ({ + href: route, + })), + })), +})); + const localVue = getLocalVue(true); const selectors = { historyLink: ".history-link", + historyLinkClick: ".history-link > a", } as const; -function mountSwitchToHistoryLinkForHistory(history: HistorySummary) { +function mountSwitchToHistoryLinkForHistory(history: HistorySummaryExtended) { + const windowSpy = jest.spyOn(window, "open"); + windowSpy.mockImplementation(() => null); const pinia = createTestingPinia(); const axiosMock = new MockAdapter(axios); axiosMock.onGet(`/api/histories/${history.id}`).reply(200, history); - const wrapper = shallowMount(SwitchToHistoryLink as object, { + const wrapper = mount(SwitchToHistoryLink as object, { propsData: { historyId: history.id, }, @@ -31,10 +49,19 @@ function mountSwitchToHistoryLinkForHistory(history: HistorySummary) { FontAwesomeIcon: true, }, }); + + const userStore = useUserStore(); + userStore.currentUser = { + email: "email", + id: "user_id", + tags_used: [], + isAnonymous: false, + total_disk_usage: 0, + }; return wrapper; } -async function expectOptionForHistory(option: string, history: HistorySummary) { +async function expectOptionForHistory(option: string, history: HistorySummaryExtended) { const wrapper = mountSwitchToHistoryLinkForHistory(history); // Wait for the history to be loaded @@ -42,6 +69,8 @@ async function expectOptionForHistory(option: string, history: HistorySummary) { expect(wrapper.html()).toContain(option); expect(wrapper.text()).toContain(history.name); + + return wrapper; } describe("SwitchToHistoryLink", () => { @@ -52,7 +81,7 @@ describe("SwitchToHistoryLink", () => { deleted: false, archived: false, purged: false, - } as HistorySummary; + } as HistorySummaryExtended; const wrapper = mountSwitchToHistoryLinkForHistory(history); expect(wrapper.find(selectors.historyLink).exists()).toBe(false); @@ -72,7 +101,8 @@ describe("SwitchToHistoryLink", () => { deleted: false, purged: false, archived: false, - } as HistorySummary; + user_id: "user_id", + } as HistorySummaryExtended; await expectOptionForHistory("Switch", history); }); @@ -83,7 +113,8 @@ describe("SwitchToHistoryLink", () => { deleted: false, purged: true, archived: false, - } as HistorySummary; + user_id: "user_id", + } as HistorySummaryExtended; await expectOptionForHistory("View", history); }); @@ -94,7 +125,56 @@ describe("SwitchToHistoryLink", () => { deleted: false, purged: false, archived: true, - } as HistorySummary; + user_id: "user_id", + } as HistorySummaryExtended; await expectOptionForHistory("View", history); }); + + it("should view in new tab when the history is unowned and published", async () => { + const history = { + id: "unowned-history-id", + name: "History Published", + deleted: false, + purged: false, + archived: false, + published: true, + user_id: "other_user_id", + } as HistorySummaryExtended; + const wrapper = await expectOptionForHistory("View", history); + + // Wait for the history to be loaded + await flushPromises(); + + // Click on the history link + const link = wrapper.find(selectors.historyLinkClick); + await link.trigger("click"); + await wrapper.vm.$nextTick(); + + // History opens in a new tab + expect(Toast.error).toHaveBeenCalledTimes(0); + }); + + it("should display View option and show error on click when history is unowned and not published", async () => { + const history = { + id: "published-history-id", + name: "History Unowned", + deleted: false, + purged: false, + archived: false, + user_id: "other_user_id", + } as HistorySummaryExtended; + const wrapper = await expectOptionForHistory("View", history); + + // Wait for the history to be loaded + await flushPromises(); + + // Click on the history link + const link = wrapper.find(selectors.historyLinkClick); + await link.trigger("click"); + await wrapper.vm.$nextTick(); + + // History does not open in a new tab and we get a Toast error + expect(Toast.error).toHaveBeenCalledTimes(1); + expect(Toast.error).toHaveBeenCalledWith("You do not have permission to view this history."); + }); }); diff --git a/client/src/components/History/SwitchToHistoryLink.vue b/client/src/components/History/SwitchToHistoryLink.vue index 4087ccecd27c..0c3a5d404462 100644 --- a/client/src/components/History/SwitchToHistoryLink.vue +++ b/client/src/components/History/SwitchToHistoryLink.vue @@ -6,8 +6,11 @@ import { BLink } from "bootstrap-vue"; import { computed } from "vue"; import { useRouter } from "vue-router/composables"; -import { HistorySummary } from "@/api"; +import { type HistorySummary, userOwnsHistory } from "@/api"; +import { Toast } from "@/composables/toast"; import { useHistoryStore } from "@/stores/historyStore"; +import { useUserStore } from "@/stores/userStore"; +import { errorMessageAsString } from "@/utils/simple-error"; import LoadingSpan from "@/components/LoadingSpan.vue"; @@ -15,6 +18,7 @@ library.add(faArchive, faBurn); const router = useRouter(); const historyStore = useHistoryStore(); +const userStore = useUserStore(); interface Props { historyId: string; @@ -25,7 +29,13 @@ const props = defineProps(); const history = computed(() => historyStore.getHistoryById(props.historyId)); -const canSwitch = computed(() => !!history.value && !history.value.archived && !history.value.purged); +const canSwitch = computed( + () => + !!history.value && + !history.value.archived && + !history.value.purged && + userOwnsHistory(userStore.currentUser, history.value) +); const actionText = computed(() => { if (canSwitch.value) { @@ -37,12 +47,16 @@ const actionText = computed(() => { return "View in new tab"; }); -function onClick(history: HistorySummary) { +async function onClick(history: HistorySummary) { if (canSwitch.value) { if (props.filters) { historyStore.applyFilters(history.id, props.filters); } else { - historyStore.setCurrentHistory(history.id); + try { + await historyStore.setCurrentHistory(history.id); + } catch (error) { + Toast.error(errorMessageAsString(error)); + } } return; } @@ -50,8 +64,16 @@ function onClick(history: HistorySummary) { } function viewHistoryInNewTab(history: HistorySummary) { - const routeData = router.resolve(`/histories/view?id=${history.id}`); - window.open(routeData.href, "_blank"); + if ( + userOwnsHistory(userStore.currentUser, history) || + history.published || + ("importable" in history && history.importable) + ) { + const routeData = router.resolve(`/histories/view?id=${history.id}`); + window.open(routeData.href, "_blank"); + } else { + Toast.error("You do not have permission to view this history."); + } } diff --git a/client/src/stores/historyStore.ts b/client/src/stores/historyStore.ts index 38c430ead734..b15227ddde67 100644 --- a/client/src/stores/historyStore.ts +++ b/client/src/stores/historyStore.ts @@ -98,9 +98,13 @@ export const useHistoryStore = defineStore("historyStore", () => { }); async function setCurrentHistory(historyId: string) { - const currentHistory = (await setCurrentHistoryOnServer(historyId)) as HistoryDevDetailed; - selectHistory(currentHistory); - setFilterText(historyId, ""); + try { + const currentHistory = (await setCurrentHistoryOnServer(historyId)) as HistoryDevDetailed; + selectHistory(currentHistory); + setFilterText(historyId, ""); + } catch (error) { + rethrowSimple(error); + } } function setCurrentHistoryId(historyId: string) { From a9ca0691ac7f5b8c893b9b2cec054611fb43ebb7 Mon Sep 17 00:00:00 2001 From: Ahmed Awan Date: Tue, 13 Aug 2024 19:00:40 -0500 Subject: [PATCH 02/15] [24.1] Allow drag and drop for collection elements We extract the `HDAObject` from the `DCESummary` to be able to drag and drop it. For now, this works on dragging and dropping items to histories, since that process only needs the dataset's id. However, this doesn't work on tool forms; as we would need to get the full `HDASummary` to do that. --- client/src/api/datasets.ts | 2 +- client/src/api/index.ts | 17 ++++++ .../CurrentCollection/CollectionPanel.vue | 2 + .../History/CurrentHistory/HistoryPanel.vue | 59 ++++++++++--------- client/src/utils/setDrag.ts | 18 ++++++ 5 files changed, 70 insertions(+), 28 deletions(-) diff --git a/client/src/api/datasets.ts b/client/src/api/datasets.ts index c6096ea18f65..2a568f77130c 100644 --- a/client/src/api/datasets.ts +++ b/client/src/api/datasets.ts @@ -67,7 +67,7 @@ export async function purgeDataset(datasetId: string) { } const datasetCopy = fetcher.path("/api/histories/{history_id}/contents/{type}s").method("post").create(); -type HistoryContentsArgs = FetchArgType; +export type HistoryContentsArgs = FetchArgType; export async function copyDataset( datasetId: HistoryContentsArgs["content"], historyId: HistoryContentsArgs["history_id"], diff --git a/client/src/api/index.ts b/client/src/api/index.ts index d1dbc8854bbb..be992a9a504f 100644 --- a/client/src/api/index.ts +++ b/client/src/api/index.ts @@ -128,6 +128,14 @@ export interface DCECollection extends DCESummary { object: DCObject; } +/** + * DatasetCollectionElement specific type for datasets. + */ +export interface DCEDataset extends DCESummary { + element_type: "hda"; + object: HDAObject; +} + /** * Contains summary information about a HDCA (HistoryDatasetCollectionAssociation). * @@ -148,6 +156,8 @@ export type HDCADetailed = components["schemas"]["HDCADetailed"]; */ export type DCObject = components["schemas"]["DCObject"]; +export type HDAObject = components["schemas"]["HDAObject"]; + export type DatasetCollectionAttributes = components["schemas"]["DatasetCollectionAttributesResult"]; export type ConcreteObjectStoreModel = components["schemas"]["ConcreteObjectStoreModel"]; @@ -187,6 +197,13 @@ export function isCollectionElement(element: DCESummary): element is DCECollecti return element.element_type === "dataset_collection"; } +/** + * Returns true if the given element of a collection is a Dataset. + */ +export function isDatasetElement(element: DCESummary): element is DCEDataset { + return element.element_type === "hda"; +} + /** * Returns true if the given dataset entry is an instance of DatasetDetails. */ diff --git a/client/src/components/History/CurrentCollection/CollectionPanel.vue b/client/src/components/History/CurrentCollection/CollectionPanel.vue index 47ca7a497827..058c05430110 100644 --- a/client/src/components/History/CurrentCollection/CollectionPanel.vue +++ b/client/src/components/History/CurrentCollection/CollectionPanel.vue @@ -8,6 +8,7 @@ import { canMutateHistory, isCollectionElement, isHDCA } from "@/api"; import ExpandedItems from "@/components/History/Content/ExpandedItems"; import { updateContentFields } from "@/components/History/model/queries"; import { useCollectionElementsStore } from "@/stores/collectionElementsStore"; +import { setItemDragstart } from "@/utils/setDrag"; import { errorMessageAsString } from "@/utils/simple-error"; import CollectionDetails from "./CollectionDetails.vue"; @@ -164,6 +165,7 @@ watch( :expand-dataset="isExpanded(item)" :is-dataset="item.element_type == 'hda'" :filterable="filterable" + @drag-start="setItemDragstart(item, $event)" @update:expand-dataset="setExpanded(item, $event)" @view-collection="onViewDatasetCollectionElement(item)" /> diff --git a/client/src/components/History/CurrentHistory/HistoryPanel.vue b/client/src/components/History/CurrentHistory/HistoryPanel.vue index 869548649a95..57a039676408 100644 --- a/client/src/components/History/CurrentHistory/HistoryPanel.vue +++ b/client/src/components/History/CurrentHistory/HistoryPanel.vue @@ -3,8 +3,15 @@ import { BAlert } from "bootstrap-vue"; import { storeToRefs } from "pinia"; import { computed, onMounted, type Ref, ref, set as VueSet, unref, watch } from "vue"; -import { type HistoryItemSummary, type HistorySummaryExtended, isHistoryItem, userOwnsHistory } from "@/api"; -import { copyDataset } from "@/api/datasets"; +import { + type HDAObject, + type HistoryItemSummary, + type HistorySummaryExtended, + isDatasetElement, + isHistoryItem, + userOwnsHistory, +} from "@/api"; +import { copyDataset, HistoryContentsArgs } from "@/api/datasets"; import ExpandedItems from "@/components/History/Content/ExpandedItems"; import SelectedItems from "@/components/History/Content/SelectedItems"; import { HistoryFilters } from "@/components/History/HistoryFilters"; @@ -17,7 +24,7 @@ import { useHistoryItemsStore } from "@/stores/historyItemsStore"; import { useHistoryStore } from "@/stores/historyStore"; import { useUserStore } from "@/stores/userStore"; import { type Alias, getOperatorForAlias } from "@/utils/filtering"; -import { setDrag } from "@/utils/setDrag"; +import { setItemDragstart } from "@/utils/setDrag"; import HistoryCounter from "./HistoryCounter.vue"; import HistoryDetails from "./HistoryDetails.vue"; @@ -54,6 +61,8 @@ interface Props { isMultiViewItem?: boolean; } +type DraggableHistoryItem = HistoryItemSummary | HDAObject; + type ContentItemRef = Record | null>>; const props = withDefaults(defineProps(), { @@ -227,7 +236,13 @@ function getDragData() { const eventStore = useEventStore(); const dragItems = eventStore.getDragItems(); // Filter out any non-history items - const historyItems = dragItems?.filter((item: any) => isHistoryItem(item)) as HistoryItemSummary[]; + const historyItems = dragItems?.map((item: any) => { + if (isHistoryItem(item)) { + return item; + } else if (isDatasetElement(item)) { + return item.object; + } + }) as DraggableHistoryItem[]; const historyId = historyItems?.[0]?.history_id; return { data: historyItems, sameHistory: historyId === props.history.id, multiple: historyItems?.length > 1 }; } @@ -381,10 +396,18 @@ async function onDrop() { try { // iterate over the data array and copy each item to the current history for (const item of data) { - const dataSource = item.history_content_type === "dataset" ? "hda" : "hdca"; - await copyDataset(item.id, props.history.id, item.history_content_type, dataSource); + let dataSource: HistoryContentsArgs["source"]; + const type = item.history_content_type as "dataset" | "dataset_collection" | undefined; + if (type) { + // it's a `HistoryItemSummary` + dataSource = type === "dataset" ? "hda" : "hdca"; + } else { + // it's a `HDAObject` from a collection + dataSource = "hda"; + } + await copyDataset(item.id, props.history.id, type, dataSource); - if (item.history_content_type === "dataset") { + if (dataSource === "hda") { datasetCount++; if (!multiple) { Toast.info("Dataset copied to history"); @@ -451,24 +474,6 @@ function arrowNavigate(item: HistoryItemSummary, eventKey: string) { } return nextItem; } - -function setItemDragstart( - item: HistoryItemSummary, - itemIsSelected: boolean, - selectedItems: Map, - selectionSize: number, - event: DragEvent -) { - if (itemIsSelected && selectionSize > 1) { - const selectedItemsObj: any = {}; - for (const [key, value] of selectedItems) { - selectedItemsObj[key] = value; - } - setDrag(event, selectedItemsObj, true); - } else { - setDrag(event, item as any); - } -}