From 74a08c4f5cf89ff6d3f271401f89f5babc16b8dc Mon Sep 17 00:00:00 2001 From: guerler Date: Wed, 28 Sep 2022 14:37:04 -0400 Subject: [PATCH 1/4] Add search param to force refresh for specific routes --- .../components/History/Content/ContentItem.vue | 2 +- .../History/Content/Dataset/DatasetActions.vue | 4 ++-- client/src/entry/analysis/router.js | 15 +++++++++++---- client/src/utils/url.js | 7 +++++++ 4 files changed, 21 insertions(+), 7 deletions(-) diff --git a/client/src/components/History/Content/ContentItem.vue b/client/src/components/History/Content/ContentItem.vue index 090dbf5bad1..79e4cf5aad0 100644 --- a/client/src/components/History/Content/ContentItem.vue +++ b/client/src/components/History/Content/ContentItem.vue @@ -203,7 +203,7 @@ export default { } }, onDisplay() { - this.$router.push(this.itemUrls.display, this.name); + this.$router.push(this.itemUrls.display, { title: this.name }); }, onDragStart(evt) { evt.dataTransfer.dropEffect = "move"; diff --git a/client/src/components/History/Content/Dataset/DatasetActions.vue b/client/src/components/History/Content/Dataset/DatasetActions.vue index bf0f4bad48e..f0a3697ae24 100644 --- a/client/src/components/History/Content/Dataset/DatasetActions.vue +++ b/client/src/components/History/Content/Dataset/DatasetActions.vue @@ -136,11 +136,11 @@ export default { this.$router.push(this.itemUrls.showDetails); }, onRerun() { - this.$router.push(`/root?job_id=${this.item.creating_job}`); + this.$router.push(`/root?job_id=${this.item.creating_job}`, { force: true }); }, onVisualize() { const title = `Visualization of ${this.item.name || ""}`; - this.$router.push(this.itemUrls.visualize, title); + this.$router.push(this.itemUrls.visualize, { title }); }, onHighlight() { this.$emit("toggleHighlights"); diff --git a/client/src/entry/analysis/router.js b/client/src/entry/analysis/router.js index 58b1850d109..6b08be9e623 100644 --- a/client/src/entry/analysis/router.js +++ b/client/src/entry/analysis/router.js @@ -55,12 +55,18 @@ import { CloudAuth } from "components/User/CloudAuth"; import { ExternalIdentities } from "components/User/ExternalIdentities"; import { HistoryExport } from "components/HistoryExport/index"; import { StorageDashboardRouter } from "components/User/DiskUsage"; +import { addSearchParams } from "utils/url"; Vue.use(VueRouter); // patches $router.push() to trigger an event and hide duplication warnings const originalPush = VueRouter.prototype.push; -VueRouter.prototype.push = function push(location, windowManagerTitle = null) { +VueRouter.prototype.push = function push(location, options = {}) { + const { title, force } = options; + // location + if (force) { + location = addSearchParams(location, { vkey: Date.now() }); + } // verify if confirmation is required console.debug("VueRouter - push: ", location); if (this.confirmation) { @@ -72,12 +78,13 @@ VueRouter.prototype.push = function push(location, windowManagerTitle = null) { } // show location in window manager const Galaxy = getGalaxyInstance(); - if (windowManagerTitle && Galaxy.frame && Galaxy.frame.active) { - Galaxy.frame.add({ title: windowManagerTitle, url: location }); + if (title && Galaxy.frame && Galaxy.frame.active) { + Galaxy.frame.add({ title: title, url: location }); return; } - // always emit event when a route is pushed + // always emit event when a duplicate route is pushed this.app.$emit("router-push"); + // avoid console warning when user clicks to revisit same route return originalPush.call(this, location).catch((err) => { if (err.name !== "NavigationDuplicated") { diff --git a/client/src/utils/url.js b/client/src/utils/url.js index a8e4840e2ec..098911520c3 100644 --- a/client/src/utils/url.js +++ b/client/src/utils/url.js @@ -13,3 +13,10 @@ export async function urlData({ url, headers, params }) { rethrowSimple(e); } } + +export function addSearchParams(url, query) { + const placeholder = url.indexOf("?") == -1 ? "?" : "&"; + console.log(placeholder); + const params = new URLSearchParams(query) + return `${url}${placeholder}${params.toString()}`; +} \ No newline at end of file From e8a838daf51dfcac368ae6016c5ecfa52fb33c6f Mon Sep 17 00:00:00 2001 From: guerler Date: Wed, 28 Sep 2022 14:52:40 -0400 Subject: [PATCH 2/4] Fix reload, move router push addition to individual file --- client/src/entry/analysis/router-push.js | 45 ++++++++++++++++++++++++ client/src/entry/analysis/router.js | 35 ++---------------- client/src/utils/url.js | 5 ++- 3 files changed, 49 insertions(+), 36 deletions(-) create mode 100644 client/src/entry/analysis/router-push.js diff --git a/client/src/entry/analysis/router-push.js b/client/src/entry/analysis/router-push.js new file mode 100644 index 00000000000..8fdfbf65e96 --- /dev/null +++ b/client/src/entry/analysis/router-push.js @@ -0,0 +1,45 @@ +import { getGalaxyInstance } from "app"; +import { addSearchParams } from "utils/url"; + +/** + * Is called before the regular router.push and allows us to provide logs, + * handle the window manager, avoid duplication warnings, and force a component + * refresh if needed. + * + * @param {String} Location as parsed to original router.push() + * @param {Object} Custom options, to provide a title and/or force reload + */ +export function patchRouterPush(VueRouter) { + const originalPush = VueRouter.prototype.push; + VueRouter.prototype.push = function push(location, options = {}) { + // add key to location to force component refresh + const { title, force } = options; + if (force) { + location = addSearchParams(location, { __vkey__: Date.now() }); + } + console.debug("VueRouter - push: ", location); + // verify if confirmation is required + if (this.confirmation) { + if (confirm("There are unsaved changes which will be lost.")) { + this.confirmation = undefined; + } else { + return; + } + } + // show location in window manager + const Galaxy = getGalaxyInstance(); + if (title && Galaxy.frame && Galaxy.frame.active) { + Galaxy.frame.add({ title: title, url: location }); + return; + } + // always emit event when a duplicate route is pushed + this.app.$emit("router-push"); + + // avoid console warning when user clicks to revisit same route + return originalPush.call(this, location).catch((err) => { + if (err.name !== "NavigationDuplicated") { + throw err; + } + }); + }; +} diff --git a/client/src/entry/analysis/router.js b/client/src/entry/analysis/router.js index 6b08be9e623..9cd10b901bd 100644 --- a/client/src/entry/analysis/router.js +++ b/client/src/entry/analysis/router.js @@ -2,6 +2,7 @@ import Vue from "vue"; import VueRouter from "vue-router"; import { getAppRoot } from "onload/loadConfig"; import { getGalaxyInstance } from "app"; +import { patchRouterPush } from "./router-push"; // these modules are mounted below the masthead. import Analysis from "entry/analysis/modules/Analysis"; @@ -55,43 +56,11 @@ import { CloudAuth } from "components/User/CloudAuth"; import { ExternalIdentities } from "components/User/ExternalIdentities"; import { HistoryExport } from "components/HistoryExport/index"; import { StorageDashboardRouter } from "components/User/DiskUsage"; -import { addSearchParams } from "utils/url"; Vue.use(VueRouter); // patches $router.push() to trigger an event and hide duplication warnings -const originalPush = VueRouter.prototype.push; -VueRouter.prototype.push = function push(location, options = {}) { - const { title, force } = options; - // location - if (force) { - location = addSearchParams(location, { vkey: Date.now() }); - } - // verify if confirmation is required - console.debug("VueRouter - push: ", location); - if (this.confirmation) { - if (confirm("There are unsaved changes which will be lost.")) { - this.confirmation = undefined; - } else { - return; - } - } - // show location in window manager - const Galaxy = getGalaxyInstance(); - if (title && Galaxy.frame && Galaxy.frame.active) { - Galaxy.frame.add({ title: title, url: location }); - return; - } - // always emit event when a duplicate route is pushed - this.app.$emit("router-push"); - - // avoid console warning when user clicks to revisit same route - return originalPush.call(this, location).catch((err) => { - if (err.name !== "NavigationDuplicated") { - throw err; - } - }); -}; +patchRouterPush(VueRouter); // redirect anon users function redirectAnon() { diff --git a/client/src/utils/url.js b/client/src/utils/url.js index 098911520c3..7781ab4421f 100644 --- a/client/src/utils/url.js +++ b/client/src/utils/url.js @@ -16,7 +16,6 @@ export async function urlData({ url, headers, params }) { export function addSearchParams(url, query) { const placeholder = url.indexOf("?") == -1 ? "?" : "&"; - console.log(placeholder); - const params = new URLSearchParams(query) + const params = new URLSearchParams(query); return `${url}${placeholder}${params.toString()}`; -} \ No newline at end of file +} From 7936523d6d909b3122101c4d04ec44212b7af8b0 Mon Sep 17 00:00:00 2001 From: guerler Date: Wed, 28 Sep 2022 15:24:02 -0400 Subject: [PATCH 3/4] Add test case and comments --- client/src/entry/analysis/router-push.js | 4 ++-- client/src/utils/url.js | 14 +++++++++++--- client/src/utils/url.test.js | 9 +++++++++ 3 files changed, 22 insertions(+), 5 deletions(-) create mode 100644 client/src/utils/url.test.js diff --git a/client/src/entry/analysis/router-push.js b/client/src/entry/analysis/router-push.js index 8fdfbf65e96..da0a97ae7b4 100644 --- a/client/src/entry/analysis/router-push.js +++ b/client/src/entry/analysis/router-push.js @@ -2,7 +2,7 @@ import { getGalaxyInstance } from "app"; import { addSearchParams } from "utils/url"; /** - * Is called before the regular router.push and allows us to provide logs, + * Is called before the regular router.push() and allows us to provide logs, * handle the window manager, avoid duplication warnings, and force a component * refresh if needed. * @@ -17,6 +17,7 @@ export function patchRouterPush(VueRouter) { if (force) { location = addSearchParams(location, { __vkey__: Date.now() }); } + // log upcoming location console.debug("VueRouter - push: ", location); // verify if confirmation is required if (this.confirmation) { @@ -34,7 +35,6 @@ export function patchRouterPush(VueRouter) { } // always emit event when a duplicate route is pushed this.app.$emit("router-push"); - // avoid console warning when user clicks to revisit same route return originalPush.call(this, location).catch((err) => { if (err.name !== "NavigationDuplicated") { diff --git a/client/src/utils/url.js b/client/src/utils/url.js index 7781ab4421f..32db2d84192 100644 --- a/client/src/utils/url.js +++ b/client/src/utils/url.js @@ -14,8 +14,16 @@ export async function urlData({ url, headers, params }) { } } -export function addSearchParams(url, query) { +/** + * Adds search parameters to url. + * + * @param {String} original url + * @param {Object} params which will be added to the url + * @returns + */ +export function addSearchParams(url, params) { const placeholder = url.indexOf("?") == -1 ? "?" : "&"; - const params = new URLSearchParams(query); - return `${url}${placeholder}${params.toString()}`; + const searchParams = new URLSearchParams(params); + const searchString = searchParams.toString(); + return searchString ? `${url}${placeholder}${searchString}` : url; } diff --git a/client/src/utils/url.test.js b/client/src/utils/url.test.js new file mode 100644 index 00000000000..6fcaf8bab99 --- /dev/null +++ b/client/src/utils/url.test.js @@ -0,0 +1,9 @@ +import { addSearchParams } from "./url"; + +describe("test url utilities", () => { + it("adding parameters to url", async () => { + expect(addSearchParams("/test?name=value")).toBe("/test?name=value"); + expect(addSearchParams("/test", { name: "value", and: "this" })).toBe("/test?name=value&and=this"); + expect(addSearchParams("/test?exists=value", { name: "value" })).toBe("/test?exists=value&name=value"); + }); +}); From 38137e1bc856e918abf2ee7bb08004689ba58c92 Mon Sep 17 00:00:00 2001 From: guerler Date: Wed, 28 Sep 2022 17:29:49 -0400 Subject: [PATCH 4/4] Add push router handling test --- client/src/app/monitor.js | 2 +- client/src/entry/analysis/router-push.js | 2 +- client/src/entry/analysis/router-push.test.js | 72 +++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 client/src/entry/analysis/router-push.test.js diff --git a/client/src/app/monitor.js b/client/src/app/monitor.js index e4e86d2d830..f74015a9219 100644 --- a/client/src/app/monitor.js +++ b/client/src/app/monitor.js @@ -19,7 +19,7 @@ if (!window.Galaxy) { if (!config.testBuild === true) { console.warn("accessing (get) window.Galaxy", serverPath()); } - return getGalaxyInstance() || galaxyStub; + return (getGalaxyInstance && getGalaxyInstance()) || galaxyStub; }, set: function (newValue) { console.warn("accessing (set) window.Galaxy", serverPath()); diff --git a/client/src/entry/analysis/router-push.js b/client/src/entry/analysis/router-push.js index da0a97ae7b4..e4f6b201483 100644 --- a/client/src/entry/analysis/router-push.js +++ b/client/src/entry/analysis/router-push.js @@ -33,7 +33,7 @@ export function patchRouterPush(VueRouter) { Galaxy.frame.add({ title: title, url: location }); return; } - // always emit event when a duplicate route is pushed + // always emit event, even when a duplicate route is pushed this.app.$emit("router-push"); // avoid console warning when user clicks to revisit same route return originalPush.call(this, location).catch((err) => { diff --git a/client/src/entry/analysis/router-push.test.js b/client/src/entry/analysis/router-push.test.js new file mode 100644 index 00000000000..91b027a897d --- /dev/null +++ b/client/src/entry/analysis/router-push.test.js @@ -0,0 +1,72 @@ +import { getGalaxyInstance } from "app/singleton"; +import { patchRouterPush } from "./router-push"; + +// mock Galaxy object +jest.mock("app/singleton"); +const mockGalaxy = { + frame: { + active: false, + add: jest.fn(), + }, +}; +getGalaxyInstance.mockImplementation(() => mockGalaxy); + +// router push handling tests +describe("router push changes", () => { + it("pushing routes", async () => { + window.confirm = jest.fn(); + let currentLocation = null; + const mockComplete = jest.fn(); + const mockContext = { + confirmation: true, + app: { + $emit: jest.fn(), + }, + }; + const mockRouter = { + prototype: { + push: (location) => { + currentLocation = location; + return { catch: mockComplete }; + }, + }, + }; + patchRouterPush(mockRouter); + const push = mockRouter.prototype.push; + const results = mockComplete.mock.results; + const windowResults = mockGalaxy.frame.add.mock.results; + // route will be rejected while confirmation is required + push.call(mockContext, "/test/name"); + expect(currentLocation).toBe(null); + expect(results.length).toBe(0); + expect(window.confirm.mock.calls[0][0]).toBe("There are unsaved changes which will be lost."); + // route should properly parse to original push + mockContext.confirmation = false; + push.call(mockContext, "/test/other"); + expect(currentLocation).toBe("/test/other"); + expect(results.length).toBe(1); + // route should properly parse to original push despite title + const title = "test title"; + push.call(mockContext, "/test/something", { title }); + expect(currentLocation).toBe("/test/something"); + expect(results.length).toBe(2); + // route should be handled by calling the window manager + mockGalaxy.frame.active = true; + push.call(mockContext, "/test/tryagain", { title }); + expect(currentLocation).toBe("/test/something"); + push.call(mockContext, "/test/openanotherone", { title }); + expect(results.length).toBe(2); + expect(windowResults.length).toBe(2); + // route should be handled by router again + mockGalaxy.frame.active = false; + push.call(mockContext, "/test/regularagain", { title }); + expect(currentLocation).toBe("/test/regularagain"); + push.call(mockContext, "/test/openanotherone", { title }); + expect(results.length).toBe(4); + expect(windowResults.length).toBe(2); + // force route should modify location by adding key + mockGalaxy.frame.active = false; + push.call(mockContext, "/test/forceroute", { force: true }); + expect(currentLocation).toMatch(new RegExp(`/test/forceroute?.*vkey.*`)); + }); +});