From 0ea87e3980e00183c7e2c7ccf694b6d68c5acd75 Mon Sep 17 00:00:00 2001 From: Sebastian Urchs Date: Fri, 1 Dec 2023 16:11:31 -0500 Subject: [PATCH] [FIX] allow users to use the browser-back button (#650) * Rename index to home * Reorganize the routing * Remove superfluous page setting methods * Remove unnecessary getters from store * Must rename tests too * Mock routing in the next-page test * Add test for navbar * Ensure next-page button does not appear on download page Also remove some unnecessary cypress comands * Remove deprecated store methods from test No longer needed * Remove these files as well * Add browser-back test to e2e test --- components/next-page.vue | 25 ++++-- components/tool-navbar.vue | 28 +++--- .../{index_page.cy.js => home_page.cy.js} | 12 +-- cypress/component/next-page.cy.js | 87 ++++++++----------- cypress/component/tool-navbar.cy.js | 43 +++++++++ cypress/e2e/app/simple-e2etest.cy.js | 26 +++--- cypress/support/commands.js | 8 -- cypress/unit/store-getter-getNextPage.cy.js | 30 ------- .../unit/store-mutation-setCurrentPage.cy.js | 24 ----- layouts/default.vue | 14 +-- nuxt.config.js | 15 ++++ pages/{index.vue => home.vue} | 0 store/index.js | 68 +-------------- 13 files changed, 151 insertions(+), 229 deletions(-) rename cypress/component/{index_page.cy.js => home_page.cy.js} (95%) create mode 100644 cypress/component/tool-navbar.cy.js delete mode 100644 cypress/unit/store-getter-getNextPage.cy.js delete mode 100644 cypress/unit/store-mutation-setCurrentPage.cy.js rename pages/{index.vue => home.vue} (100%) diff --git a/components/next-page.vue b/components/next-page.vue index f3815146..1749254d 100644 --- a/components/next-page.vue +++ b/components/next-page.vue @@ -8,7 +8,7 @@

+ v-if="!isPageAccessible(nextPage)"> {{ uiText.instructions[currentPage] }}

@@ -17,9 +17,9 @@ + @click="navigateToPage(nextPage);"> {{ uiText.button[currentPage] }} @@ -69,20 +69,29 @@ ...mapGetters([ - "getNextPage", "isPageAccessible" ]), ...mapState([ - "currentPage", - "pageData" + "pageOrder" ]), - nextPageButtonColor() { + currentPage() { + return this.$route.name; + }, + nextPage() { + const pageIndex = this.pageOrder.indexOf(this.currentPage); + if (pageIndex < this.pageOrder.length - 1) { + return this.pageOrder[pageIndex + 1]; + } else { + return ""; + } + }, + nextPageButtonColor() { // Bootstrap variant color of the button leading to the next page - return this.isPageAccessible(this.getNextPage) ? "success" : "secondary"; + return this.isPageAccessible(this.nextPage) ? "success" : "secondary"; } }, diff --git a/components/tool-navbar.vue b/components/tool-navbar.vue index abcef912..11502d8e 100644 --- a/components/tool-navbar.vue +++ b/components/tool-navbar.vue @@ -34,14 +34,14 @@ - {{ navItem.fullName }} + v-for="(pageName) in pageOrder" + :active="currentPageName === pageName" + :class="getNavItemColor(pageName)" + :data-cy="'menu-item-' + pageName" + :disabled="!isPageAccessible(pageName)" + :key="pageName" + @click="navigateToPage(pageName)"> + {{ pageName }} | { +describe("The Home page", () => { // Setup beforeEach(() => { @@ -27,7 +27,7 @@ describe("The index page", () => { it("Mounts empty and displays all UI elements", () => { // Act - cy.mount(indexPage, { + cy.mount(homePage, { mocks: { $store: store }, stubs: stubs, @@ -44,7 +44,7 @@ describe("The index page", () => { it("Correctly displays previews of the loaded data", () => { // Act - cy.mount(indexPage, { + cy.mount(homePage, { mocks: { $store: Object.assign({}, store, { @@ -87,7 +87,7 @@ describe("The index page", () => { // Setup cy.fixture("examples/good/example_short.tsv").as("exampleTable"); cy.spy(store, "dispatch").as("dispatchSpy"); - cy.mount(indexPage, { + cy.mount(homePage, { mocks: { $store: store }, stubs: stubs, @@ -119,7 +119,7 @@ describe("The index page", () => { ]; cy.fixture("examples/good/example_short.json").as("exampleDictionary"); cy.spy(store, "dispatch").as("dispatchSpy"); - cy.mount(indexPage, { + cy.mount(homePage, { mocks: { $store: store }, stubs: stubs, diff --git a/cypress/component/next-page.cy.js b/cypress/component/next-page.cy.js index 892c60aa..3e710b2d 100644 --- a/cypress/component/next-page.cy.js +++ b/cypress/component/next-page.cy.js @@ -19,59 +19,34 @@ describe("next page button", () => { commit: () => {}, - getters: { - - getNextPage: () => { - - return "mynextpage"; - } - }, - - mutations: { - - setCurrentPage: () => (p_pageName) => { - - store.state.currentPage = p_pageName; - } - }, - state: { currentPage: "mypage", dataTable: [], - pageData: { - - mypage: { - - location: "mypage", - pageName: "mypage" - }, - - mynextpage: { - - location: "mynextpage", - pageName: "mynextpage" - } - } + pageOrder: ["mypage", "mynextpage"] } }; }); it("Does instruction text appear above the next page button when the button is disabled", () => { - // Setup - The next page after 'mypage' is currently inaccessible - store.getters.isPageAccessible = () => (p_pageName) => false; - // Act - Mount the next page button with mocks cy.mount(nextPage, { - computed: store.getters, + computed: { + isPageAccessible: () => (p_pageName) => false + }, data() { return { uiText: uiText }; }, - mocks: { $store: store } + mocks: { + $store: store, + $route: { + name: "mypage" + } + } }); // Assert - Correct instructions are visible (since button is disabled due to next page inaccessibility) @@ -80,19 +55,23 @@ describe("next page button", () => { it("Button is enabled when next page is accessible and vice-versa", () => { - // Setup - Mock the page accessibility getter to test effects on the next page button - store.getters.isPageAccessible = () => (p_pageName) => true; - // Act - Mount the next page button with mocks cy.mount(nextPage, { - computed: store.getters, + computed: { + isPageAccessible: () => (p_pageName) => true + }, data() { return { uiText: uiText }; }, - mocks: { $store: store } + mocks: { + $store: store, + $route: { + name: "mypage" + } + } }); // Assert - Button is enabled when next page is accessible @@ -101,19 +80,23 @@ describe("next page button", () => { it("Button is disabled when next page is not accessible", () => { - // Setup - Mock the page accessibility getter to test effects on the next page button - store.getters.isPageAccessible = () => (p_pageName) => false; - // Act - Mount the next page button with mocks cy.mount(nextPage, { - computed: store.getters, + computed: { + isPageAccessible: () => (p_pageName) => false + }, data() { return { uiText: uiText }; }, - mocks: { $store: store } + mocks: { + $store: store, + $route: { + name: "mypage" + } + } }); // Assert - Button is enabled when next page is accessible @@ -122,19 +105,23 @@ describe("next page button", () => { it("Does the label on the next page button correspond to the text for the current page?", () => { - // Setup - Mock the page accessibility getter to test effects on the next page button - store.getters.isPageAccessible = () => (p_pageName) => true; - // Act - Mount the next page button with mocks cy.mount(nextPage, { - computed: store.getters, + computed: { + isPageAccessible: () => (p_pageName) => true + }, data() { return { uiText: uiText }; }, - mocks: { $store: store } + mocks: { + $store: store, + $route: { + name: "mypage" + } + } }); // Assert - Check button text corresponds to the recently set page diff --git a/cypress/component/tool-navbar.cy.js b/cypress/component/tool-navbar.cy.js new file mode 100644 index 00000000..5433c825 --- /dev/null +++ b/cypress/component/tool-navbar.cy.js @@ -0,0 +1,43 @@ +import toolNavbar from '~/components/tool-navbar.vue'; + + +describe('Navbar Component', () => { + beforeEach(() => { + const $route = { name: 'home' }; + const store = { + state: { + pageOrder: ['home', 'about', 'contact'] + }, + getters: { + isPageAccessible: () => true + }, + dispatch: () => {} + }; + + cy.spy(store, "dispatch").as("dispatchSpy"); + + cy.mount(toolNavbar, { + mocks: { + $route, + $store: store + } + }); + }); + + it('lists all nav items', () => { + cy.get('[data-cy=menu-item-home]').should('exist'); + cy.get('[data-cy=menu-item-about]').should('exist'); + cy.get('[data-cy=menu-item-contact]').should('exist'); + }); + + it('the current page rout is highlighted with a different css style', () => { + cy.get('[data-cy=menu-item-home]').should('have.class', 'dark'); + cy.get('[data-cy=menu-item-about]').should('not.have.class', 'dark'); + cy.get('[data-cy=menu-item-contact]').should('not.have.class', 'dark'); + }); + + it('navigates to a page on nav item click', () => { + cy.get('[data-cy=menu-item-about]').click(); + cy.get('@dispatchSpy').should('have.been.calledWith', 'navigateToPage', 'about'); + }); +}); \ No newline at end of file diff --git a/cypress/e2e/app/simple-e2etest.cy.js b/cypress/e2e/app/simple-e2etest.cy.js index 95f21b3d..caa9de03 100644 --- a/cypress/e2e/app/simple-e2etest.cy.js +++ b/cypress/e2e/app/simple-e2etest.cy.js @@ -54,9 +54,7 @@ describe("End to end test using a simple UI path through the app", () => { .selectFile(dataFolder + p_dataset.data_dictionary); // E. Click the next page button to proceed to the categorization page - cy.nextPageByButton(); - - cy.commitToVuexStore("setCurrentPage", "categorization"); + cy.get("[data-cy='button-nextpage']").click(); if ( cy.datasetMeetsTestCriteria("categorization", p_dataset, testCriteria) ) { @@ -87,10 +85,17 @@ describe("End to end test using a simple UI path through the app", () => { // annotation page is no accessible. cy.assertNextPageAccess("annotation", true); - // E. Click the next page button to proceed to the categorization page - cy.nextPageByButton(); + // E. Click the next page button to proceed to the annotation page + cy.get("[data-cy='button-nextpage']").click(); + + // Make sure that we can go back with the browser back button and the + // app state also updates accordingly + cy.get("[data-cy='button-nextpage']").contains('Review and download'); + cy.go('back'); + cy.get("[data-cy='button-nextpage']").contains('Annotate columns'); + cy.go('forward'); + cy.get("[data-cy='button-nextpage']").contains('Review and download'); - cy.commitToVuexStore("setCurrentPage", "annotation"); if ( cy.datasetMeetsTestCriteria("annotation", p_dataset, testCriteria)) { @@ -123,18 +128,15 @@ describe("End to end test using a simple UI path through the app", () => { cy.assertNextPageAccess("download", true); // E. Click the next page button to proceed to the download page - cy.nextPageByButton(); + cy.get("[data-cy='button-nextpage']").click(); // 4. Go through the download page, downloading the output annotation file - // A. Click the download button + cy.get("[data-cy='button-nextpage']").should('not.exist'); + cy.get("[data-cy='download-button']") .click(); - // B. Assert that csv file has downloaded - // cy.verifyDownload(".json", { contains: true }); - - // C. Check the contents of the output cy.task("downloads", "cypress/downloads").then(folderStateAfter => { cy.readFile('cypress/downloads/' + folderStateAfter[folderStateAfter.length - 1]).then((fileContent) => { expect(fileContent.group.Annotations.Levels.HC.Label).to.eq("Healthy Control"); diff --git a/cypress/support/commands.js b/cypress/support/commands.js index e75f05d8..1c8b27af 100644 --- a/cypress/support/commands.js +++ b/cypress/support/commands.js @@ -261,14 +261,6 @@ Cypress.Commands.add("loadTestDataIntoStore", (p_dataset) => { }); }); -// Go to the next page by clicking the next page button -Cypress.Commands.add("nextPageByButton", () => { - - // Click the next page button to proceed to the next page - cy.get("[data-cy='button-nextpage']") - .click(); -}); - // Go to the next page by clicking the nav bar link Cypress.Commands.add("nextPageByNav", (p_navItemName) => { diff --git a/cypress/unit/store-getter-getNextPage.cy.js b/cypress/unit/store-getter-getNextPage.cy.js deleted file mode 100644 index ad3e2fee..00000000 --- a/cypress/unit/store-getter-getNextPage.cy.js +++ /dev/null @@ -1,30 +0,0 @@ -import { getters } from "~/store"; - -let state = {}; - -describe("getNextPage", () => { - - beforeEach(() => { - - // Setup - state = { - - currentPage: "home" - }; - }); - - it("Checks each next page after given page is correct", () => { - - // Setup - let pageNames = ["home", "categorization", "annotation", "download"]; - - for ( let index = 0; index < pageNames.length - 1; index++ ) { - - // Act - state.currentPage = pageNames[index]; - - // Assert - expect(getters.getNextPage(state, pageNames[index])).to.equal(pageNames[index + 1]); - } - }); -}); \ No newline at end of file diff --git a/cypress/unit/store-mutation-setCurrentPage.cy.js b/cypress/unit/store-mutation-setCurrentPage.cy.js deleted file mode 100644 index 41ea545b..00000000 --- a/cypress/unit/store-mutation-setCurrentPage.cy.js +++ /dev/null @@ -1,24 +0,0 @@ -import { mutations } from "~/store"; - -let state = {}; - -describe("setCurrentPage", () => { - - beforeEach(() => { - - // Setup - state = { - - currentPage: "home" - }; - }); - - it("Set the annotation tool's current page", () => { - - // Act - mutations.setCurrentPage(state, "categorization"); - - // Assert - expect(state.currentPage).to.equal("categorization"); - }); -}); \ No newline at end of file diff --git a/layouts/default.vue b/layouts/default.vue index 35910395..02c10c93 100644 --- a/layouts/default.vue +++ b/layouts/default.vue @@ -9,7 +9,7 @@ - + @@ -17,19 +17,9 @@ \ No newline at end of file diff --git a/nuxt.config.js b/nuxt.config.js index 08770bd2..5a26f240 100644 --- a/nuxt.config.js +++ b/nuxt.config.js @@ -1,4 +1,19 @@ export default { + // We need to override the default page name from index.vue to home.vue + // because we use the auto-generated routes for navigation and want + // the first page to be called "home" + router: { + extendRoutes(routes, resolve) { + routes.push({ + path: '/', + name: 'home', + components: { + // eslint-disable-next-line no-undef + default: resolve(__dirname, 'pages/home.vue') + } + }); + } + }, // Global page headers: https://go.nuxtjs.dev/config-head head: { diff --git a/pages/index.vue b/pages/home.vue similarity index 100% rename from pages/index.vue rename to pages/home.vue diff --git a/store/index.js b/store/index.js index 1e14964e..8d429144 100644 --- a/store/index.js +++ b/store/index.js @@ -92,8 +92,6 @@ export const state = () => ({ columnToCategoryMap: {}, - currentPage: "home", - dataDictionary: { // stores the data dictionary loaded by the user (if available) in userProvided @@ -107,36 +105,7 @@ export const state = () => ({ dataTable: [], - pageData: { - - home: { - - fullName: "Home", - location: "/", - pageName: "home" - }, - - categorization: { - - fullName: "Categorization", - location: "categorization", - pageName: "categorization" - }, - - annotation: { - - fullName: "Annotation", - location: "annotation", - pageName: "annotation" - }, - - download: { - - fullName: "Download", - location: "download", - pageName: "download" - } - }, + pageOrder: ['home', 'categorization', 'annotation', 'download'], // TODO: Assess whether this is the best place and configuration for storing // transformation heuristics @@ -525,29 +494,6 @@ export const getters = { return missingValues; }, - getNextPage(p_state) { - - let nextPage = ""; - - switch ( p_state.currentPage ) { - - case "home": - nextPage = "categorization"; - break; - case "categorization": - nextPage = "annotation"; - break; - case "annotation": - nextPage = "download"; - break; - case "download": - nextPage = ""; - break; - } - - return nextPage; - }, - getSelectedCategoricalOption: (p_state) => (p_columnName, p_rawValue) => { // 0. If raw value does not exist in the value map, returns "" @@ -652,7 +598,6 @@ export const getters = { }, isPageAccessible: (p_state, p_getters) => (p_pageName) => { - let pageAccessible = false; switch ( p_pageName ) { @@ -707,9 +652,9 @@ export const getters = { export const actions = { - navigateToPage({ state, commit }, pageName) { - this.$router.push(state.pageData[pageName].location); - commit("setCurrentPage", pageName); + navigateToPage( _, pageName) { + const targetRoute = this.$router.options.routes.filter((route) => route.name === pageName)[0]; + this.$router.push(targetRoute.path); }, processDataDictionary({ state, commit, getters }, { data, filename }) { @@ -868,11 +813,6 @@ export const mutations = { } }, - setCurrentPage(p_state, p_pageName) { - - p_state.currentPage = p_pageName; - }, - setDataDictionary(p_state, { newDataDictionary, storeColumns }) { // 1. Update values to existing columns in the data dictionary, but ignore any new columns