From 987bbf9be58fa69669251b06452cc56b2d07b285 Mon Sep 17 00:00:00 2001 From: Ahmed Awan Date: Tue, 3 Sep 2024 11:59:22 -0500 Subject: [PATCH] [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) {