From f9d64eb4862c07ebebf4c470f2aa14044693193c Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Tue, 21 Feb 2023 18:20:05 +0000 Subject: [PATCH] Fix bug where subscriptions would not update on query change --- src/mixins/subscriptionComponent.js | 9 ++++ src/mixins/subscriptionView.js | 40 ---------------- src/views/Dashboard.vue | 4 +- src/views/Graph.vue | 4 +- src/views/Guide.vue | 2 - src/views/Log.vue | 6 +-- src/views/Table.vue | 4 +- src/views/Tree.vue | 4 +- src/views/UserProfile.vue | 4 +- src/views/Workflows.vue | 4 +- src/views/WorkflowsTable.vue | 4 +- src/views/Workspace.vue | 11 +---- tests/unit/components/cylc/drawer.vue.spec.js | 3 +- .../unit/mixins/subscriptionComponent.spec.js | 47 ++++++++++--------- tests/unit/mixins/subscriptionView.spec.js | 44 ----------------- 15 files changed, 48 insertions(+), 142 deletions(-) delete mode 100644 src/mixins/subscriptionView.js delete mode 100644 tests/unit/mixins/subscriptionView.spec.js diff --git a/src/mixins/subscriptionComponent.js b/src/mixins/subscriptionComponent.js index 7edfc365d..21ae2a440 100644 --- a/src/mixins/subscriptionComponent.js +++ b/src/mixins/subscriptionComponent.js @@ -35,8 +35,17 @@ export default { ], beforeMount () { this.$workflowService.subscribe(this) + this.$workflowService.startSubscriptions() }, beforeDestroy () { this.$workflowService.unsubscribe(this) + }, + watch: { + query () { + // if the query changes, unsubscribe & re-subscribe + this.$workflowService.unsubscribe(this) + this.$workflowService.subscribe(this) + this.$workflowService.startSubscriptions() + } } } diff --git a/src/mixins/subscriptionView.js b/src/mixins/subscriptionView.js deleted file mode 100644 index f54071522..000000000 --- a/src/mixins/subscriptionView.js +++ /dev/null @@ -1,40 +0,0 @@ -/** - * Copyright (C) NIWA & British Crown (Met Office) & Contributors. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program. If not, see . - */ - -import subscriptionMixin from '@/mixins/subscription' - -/** - * A mixin used for a View (VueRouter bound component). It uses VueRouter - * navigation guards to coordinate when a subscription is started. - * - * Subscriptions are stopped via the subscriptionComponent mixin, which binds - * to lifecycle created and beforeDestroy methods of the View/Component. - * - * @see Subscription - * @see SubscriptionQuery - * @see WorkflowService - */ -export default { - mixins: [ - subscriptionMixin - ], - beforeRouteEnter (to, from, next) { - next(vm => { - vm.$workflowService.startSubscriptions() - }) - } -} diff --git a/src/views/Dashboard.vue b/src/views/Dashboard.vue index 91d10d0aa..0b57f2da4 100644 --- a/src/views/Dashboard.vue +++ b/src/views/Dashboard.vue @@ -168,7 +168,6 @@ along with this program. If not, see . import { mapState, mapGetters } from 'vuex' import { mdiBook, mdiBookMultiple, mdiBookOpenVariant, mdiCog, mdiHubspot, mdiTable } from '@mdi/js' import pageMixin from '@/mixins/index' -import subscriptionViewMixin from '@/mixins/subscriptionView' import subscriptionComponentMixin from '@/mixins/subscriptionComponent' import { createUrl } from '@/utils/urls' import { WorkflowState, WorkflowStateOrder } from '@/model/WorkflowState.model' @@ -179,8 +178,7 @@ export default { name: 'Dashboard', mixins: [ pageMixin, - subscriptionComponentMixin, - subscriptionViewMixin + subscriptionComponentMixin ], metaInfo () { return { diff --git a/src/views/Graph.vue b/src/views/Graph.vue index 4e761d3b8..4a749d1ef 100644 --- a/src/views/Graph.vue +++ b/src/views/Graph.vue @@ -96,7 +96,6 @@ import gql from 'graphql-tag' import { mapGetters } from 'vuex' import pageMixin from '@/mixins/index' import graphqlMixin from '@/mixins/graphql' -import subscriptionViewMixin from '@/mixins/subscriptionView' import subscriptionComponentMixin from '@/mixins/subscriptionComponent' import SubscriptionQuery from '@/model/SubscriptionQuery.model' // import CylcTreeCallback from '@/services/treeCallback' @@ -203,8 +202,7 @@ export default { mixins: [ pageMixin, graphqlMixin, - subscriptionComponentMixin, - subscriptionViewMixin + subscriptionComponentMixin ], name: 'Graph', components: { diff --git a/src/views/Guide.vue b/src/views/Guide.vue index 6c0d416c4..1fd5c5433 100644 --- a/src/views/Guide.vue +++ b/src/views/Guide.vue @@ -191,11 +191,9 @@ along with this program. If not, see . import Task from '@/components/cylc/Task' import Job from '@/components/cylc/Job' import { TaskStateUserOrder } from '@/model/TaskState.model' -import subscriptionViewMixin from '@/mixins/subscriptionView' export default { name: 'Guide', - mixins: [subscriptionViewMixin], components: { task: Task, job: Job diff --git a/src/views/Log.vue b/src/views/Log.vue index 2e7fa9390..6b870f260 100644 --- a/src/views/Log.vue +++ b/src/views/Log.vue @@ -89,8 +89,7 @@ along with this program. If not, see . import { mdiFileDocumentMultipleOutline } from '@mdi/js' import pageMixin from '@/mixins/index' import graphqlMixin from '@/mixins/graphql' -import subscriptionViewMixin from '@/mixins/subscriptionView' -import subscriptionMixin from '@/mixins/subscriptionComponent' +import subscriptionComponentMixin from '@/mixins/subscriptionComponent' import LogComponent from '@/components/cylc/log/Log' import SubscriptionQuery from '@/model/SubscriptionQuery.model' import { LOGS_SUBSCRIPTION } from '@/graphql/queries' @@ -116,8 +115,7 @@ export default { mixins: [ pageMixin, graphqlMixin, - subscriptionMixin, - subscriptionViewMixin + subscriptionComponentMixin ], name: 'Log', components: { diff --git a/src/views/Table.vue b/src/views/Table.vue index 55d6c2506..f3531ea6a 100644 --- a/src/views/Table.vue +++ b/src/views/Table.vue @@ -32,7 +32,6 @@ import { mapState, mapGetters } from 'vuex' import { mdiTable } from '@mdi/js' import pageMixin from '@/mixins/index' import graphqlMixin from '@/mixins/graphql' -import subscriptionViewMixin from '@/mixins/subscriptionView' import subscriptionComponentMixin from '@/mixins/subscriptionComponent' import TableComponent from '@/components/cylc/table/Table.vue' import SubscriptionQuery from '@/model/SubscriptionQuery.model' @@ -43,8 +42,7 @@ export default { mixins: [ pageMixin, graphqlMixin, - subscriptionComponentMixin, - subscriptionViewMixin + subscriptionComponentMixin ], name: 'Table', components: { diff --git a/src/views/Tree.vue b/src/views/Tree.vue index cf8895785..c40254f9e 100644 --- a/src/views/Tree.vue +++ b/src/views/Tree.vue @@ -38,7 +38,6 @@ import { mapState, mapGetters } from 'vuex' import { mdiFileTree } from '@mdi/js' import pageMixin from '@/mixins/index' import graphqlMixin from '@/mixins/graphql' -import subscriptionViewMixin from '@/mixins/subscriptionView' import subscriptionComponentMixin from '@/mixins/subscriptionComponent' import SubscriptionQuery from '@/model/SubscriptionQuery.model' import TreeComponent from '@/components/cylc/tree/Tree' @@ -48,8 +47,7 @@ export default { mixins: [ pageMixin, graphqlMixin, - subscriptionComponentMixin, - subscriptionViewMixin + subscriptionComponentMixin ], name: 'Tree', components: { diff --git a/src/views/UserProfile.vue b/src/views/UserProfile.vue index 42bf2ca65..d9e25a6bd 100644 --- a/src/views/UserProfile.vue +++ b/src/views/UserProfile.vue @@ -215,15 +215,13 @@ import { decreaseFontSize, getCurrentFontSize, increaseFontSize, resetFontSize } import { mdiCog, mdiFormatFontSizeDecrease, mdiFormatFontSizeIncrease } from '@mdi/js' import Job from '@/components/cylc/Job' import JobState from '@/model/JobState.model' -import subscriptionViewMixin from '@/mixins/subscriptionView' // TODO: update where user preferences are stored after #335 export default { name: 'UserProfile', mixins: [ - pageMixin, - subscriptionViewMixin + pageMixin ], components: { Job diff --git a/src/views/Workflows.vue b/src/views/Workflows.vue index 21486b688..ff5d9fc19 100644 --- a/src/views/Workflows.vue +++ b/src/views/Workflows.vue @@ -25,7 +25,6 @@ import GScan from '@/components/cylc/gscan/GScan' import { GSCAN_DELTAS_SUBSCRIPTION } from '@/graphql/queries' import pageMixin from '@/mixins/index' import subscriptionComponentMixin from '@/mixins/subscriptionComponent' -import subscriptionViewMixin from '@/mixins/subscriptionView' import SubscriptionQuery from '@/model/SubscriptionQuery.model' export default { @@ -33,8 +32,7 @@ export default { mixins: [ pageMixin, - subscriptionComponentMixin, - subscriptionViewMixin + subscriptionComponentMixin ], components: { diff --git a/src/views/WorkflowsTable.vue b/src/views/WorkflowsTable.vue index c7bfe7c88..a6cea2514 100644 --- a/src/views/WorkflowsTable.vue +++ b/src/views/WorkflowsTable.vue @@ -72,7 +72,7 @@ import { mapState, mapGetters } from 'vuex' import i18n from '@/i18n' import { mdiTable } from '@mdi/js' import pageMixin from '@/mixins/index' -import subscriptionViewMixin from '@/mixins/subscriptionView' +import subscriptionMixin from '@/mixins/subscription' import SubscriptionQuery from '@/model/SubscriptionQuery.model' import { WORKFLOWS_TABLE_DELTAS_SUBSCRIPTION } from '@/graphql/queries' import WorkflowIcon from '@/components/cylc/gscan/WorkflowIcon' @@ -81,7 +81,7 @@ export default { name: 'WorkflowsTable', mixins: [ pageMixin, - subscriptionViewMixin + subscriptionMixin ], metaInfo () { return { diff --git a/src/views/Workspace.vue b/src/views/Workspace.vue index b0e4d0da1..dc09a5048 100644 --- a/src/views/Workspace.vue +++ b/src/views/Workspace.vue @@ -54,7 +54,7 @@ import Vue from 'vue' import { each, iter } from '@lumino/algorithm' import pageMixin from '@/mixins' import graphqlMixin from '@/mixins/graphql' -import subscriptionViewMixin from '@/mixins/subscriptionView' +import subscriptionMixin from '@/mixins/subscription' import ViewState from '@/model/ViewState.model' import { createWidgetId } from '@/components/cylc/workflow/index' import Lumino from '@/components/cylc/workflow/Lumino' @@ -70,7 +70,7 @@ export default { mixins: [ pageMixin, graphqlMixin, - subscriptionViewMixin + subscriptionMixin ], components: { @@ -179,13 +179,6 @@ export default { createWidgetId(), { view, initialOptions } ) - this.$nextTick(() => { - // Views use navigation-guards to start the pending subscriptions. But we don't have - // this in this view. We must pretend we are doing the beforeRouteEnter here, and - // call the .startSubscriptions function, so that the WorkflowService will take care - // of loading the pending subscriptions (like the ones created by the new view). - this.$workflowService.startSubscriptions() - }) }, /** * Remove all the widgets present in the UI. diff --git a/tests/unit/components/cylc/drawer.vue.spec.js b/tests/unit/components/cylc/drawer.vue.spec.js index 82738ba2e..66588feda 100644 --- a/tests/unit/components/cylc/drawer.vue.spec.js +++ b/tests/unit/components/cylc/drawer.vue.spec.js @@ -17,7 +17,7 @@ import { expect } from 'chai' import { createLocalVue, mount } from '@vue/test-utils' -import Drawer from '../../../../src/components/cylc/Drawer' +import Drawer from '@/components/cylc/Drawer' import Vue from 'vue' import Vuetify from 'vuetify' @@ -37,6 +37,7 @@ const mountFunction = options => { register () {}, unregister () {}, subscribe () {}, + startSubscriptions () {}, introspection: Promise.resolve({ mutations: [ { args: [] } diff --git a/tests/unit/mixins/subscriptionComponent.spec.js b/tests/unit/mixins/subscriptionComponent.spec.js index 35ba20f69..775d16f79 100644 --- a/tests/unit/mixins/subscriptionComponent.spec.js +++ b/tests/unit/mixins/subscriptionComponent.spec.js @@ -16,46 +16,49 @@ */ import { expect } from 'chai' +import sinon from 'sinon' import { createLocalVue, shallowMount } from '@vue/test-utils' import subscriptionComponentMixin from '@/mixins/subscriptionComponent' +import WorkflowService from '@/services/workflow.service' const localVue = createLocalVue() describe('Subscription Component mixin', () => { - let workflowService + let workflowService, Component beforeEach(() => { - workflowService = { - subscribe (componentOrView) { - componentOrView.subscribed = true - }, - unsubscribe (componentOrView) { - componentOrView.subscribed = false - } - } + workflowService = sinon.createStubInstance(WorkflowService) localVue.prototype.$workflowService = workflowService - }) - it('should provide a hook for when the component is created', () => { - const Component = { + + Component = { mixins: [subscriptionComponentMixin], + data: () => ({ + query: { foo: 1 } + }), render () { } } + }) + + it('subscribes & unsubscribes when the component is mounted & destroyed', () => { const component = shallowMount(Component, { localVue }) - expect(component.vm.subscribed).to.equal(true) + expect(workflowService.subscribe.calledOnceWith(component.vm)).to.equal(true) + expect(workflowService.startSubscriptions.calledOnce).to.equal(true) + expect(workflowService.unsubscribe.called).to.equal(false) + component.vm.$destroy() + expect(workflowService.unsubscribe.calledOnceWith(component.vm)).to.equal(true) }) - it('should provide a hook for when the component is destroyed', () => { - const Component = { - mixins: [subscriptionComponentMixin], - render () { - } - } + + it('un- & re-subcribes when the query changes', () => { const component = shallowMount(Component, { localVue }) - expect(component.vm.subscribed).to.equal(true) - component.vm.$destroy() - expect(component.vm.subscribed).to.equal(false) + component.vm.query = { foo: 2 } + component.vm.$nextTick(() => { + expect(workflowService.unsubscribe.calledOnceWith(component.vm)).to.equal(true) + expect(workflowService.subscribe.calledTwice).to.equal(true) + expect(workflowService.startSubscriptions.calledTwice).to.equal(true) + }) }) }) diff --git a/tests/unit/mixins/subscriptionView.spec.js b/tests/unit/mixins/subscriptionView.spec.js deleted file mode 100644 index c5603fa01..000000000 --- a/tests/unit/mixins/subscriptionView.spec.js +++ /dev/null @@ -1,44 +0,0 @@ -/** - * Copyright (C) NIWA & British Crown (Met Office) & Contributors. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program. If not, see . - */ - -import { expect } from 'chai' -import { createLocalVue } from '@vue/test-utils' -import sinon from 'sinon' -import subscriptionViewMixin from '@/mixins/subscriptionView' - -const localVue = createLocalVue() - -describe('Subscription View mixin', () => { - let workflowService - beforeEach(() => { - workflowService = { - startSubscriptions: () => {} - } - sinon.spy(workflowService) - localVue.prototype.$workflowService = workflowService - }) - it('should provide a navigation guard for when the view is visited', () => { - const component = Object.assign(subscriptionViewMixin) - const vm = { - $workflowService: workflowService - } - component.beforeRouteEnter(null, null, (callback) => { - callback(vm) - }) - expect(workflowService.startSubscriptions.calledOnce).to.equal(true) - }) -})