From 38c53b37a3eb029df3e7d64932987ad3efcd2634 Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Wed, 28 Feb 2024 06:51:17 +0100 Subject: [PATCH 1/3] PB-259: Improved startup performance by postponing the catalogue and active layer rendering The rendering of catalogue is very costly and store events would re-trigger it therefore wait that the map has been rendered before rendering the catalogue, it is anyway not visible at startup until the user click on the menu entry. It still needs to be always rendered once the application has been started, otherwise the toggling of the menu would be too slow. Also differ the rendering and opening of the active layers for the same reason and also because if the menu contains external layer (e.g. kml, gpx, ...) the application needs to load its metadata/data before rendering the correct name in the list. For kml and GPX it would use at startup the default KML or GPX name until the name has been loading. By defering the active layers list we avoid having the name changing. --- src/modules/menu/components/menu/MenuTray.vue | 5 ++++- src/modules/menu/components/topics/MenuTopicSection.vue | 4 ++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/modules/menu/components/menu/MenuTray.vue b/src/modules/menu/components/menu/MenuTray.vue index bc4624991..e7d495cad 100644 --- a/src/modules/menu/components/menu/MenuTray.vue +++ b/src/modules/menu/components/menu/MenuTray.vue @@ -49,12 +49,14 @@ :compact="compact" @open-menu-section="onOpenMenuSection" /> + @@ -121,6 +123,7 @@ export default { is3dMode: (state) => state.cesium.active, showImportFile: (state) => state.ui.importFile, showDrawingOverlay: (state) => state.ui.showDrawingOverlay, + mapModuleReady: (state) => state.app.isMapReady, }), ...mapGetters(['isPhoneMode', 'hasDevSiteWarning']), }, diff --git a/src/modules/menu/components/topics/MenuTopicSection.vue b/src/modules/menu/components/topics/MenuTopicSection.vue index c010410c1..bc3d96d2c 100644 --- a/src/modules/menu/components/topics/MenuTopicSection.vue +++ b/src/modules/menu/components/topics/MenuTopicSection.vue @@ -35,6 +35,7 @@ const showTopicTree = computed(() => { // If we have defined catalog themes to be opened in the URL, it makes sense to open the catalog return !isDefaultTopic.value || openThemesIds.value.length > 0 }) +const mapModuleReady = computed(() => store.state.app.isMapReady) function setShowTopicSelectionPopup() { showTopicSelectionPopup.value = true @@ -78,7 +79,10 @@ defineExpose({ close }) @close="showTopicSelectionPopup = false" /> + Date: Wed, 28 Feb 2024 06:38:10 +0100 Subject: [PATCH 2/3] PB-259: Fix e2e test race condition and improve drawing test The drawing test `keeps the kml after a page reload, ...` was flaky and failed due to a race condition. This test has been improved to avoid race condition. Other tests using the cy.reload() have been also slightly improve by waiting the mapIsReady before continuing avoiding race condition. --- tests/cypress/support/commands.js | 8 ++ tests/cypress/tests-e2e/drawing.cy.js | 139 +++++++++++++------ tests/cypress/tests-e2e/importToolFile.cy.js | 2 + tests/cypress/tests-e2e/importToolMaps.cy.js | 1 + 4 files changed, 104 insertions(+), 46 deletions(-) diff --git a/tests/cypress/support/commands.js b/tests/cypress/support/commands.js index d81ec17f1..a0299f746 100644 --- a/tests/cypress/support/commands.js +++ b/tests/cypress/support/commands.js @@ -252,6 +252,14 @@ Cypress.Commands.add( } ) +/** + * Wait until the map has been rendered and is ready. This is useful and needed during the + * application startup phase + */ +Cypress.Commands.add('waitMapIsReady', ({ timeout = 15000 } = {}) => { + cy.waitUntilState((state) => state.app.isMapReady, { timeout: timeout }) +}) + /** * Changes a URL parameter without reloading the app. * diff --git a/tests/cypress/tests-e2e/drawing.cy.js b/tests/cypress/tests-e2e/drawing.cy.js index 9bf71c992..094177be7 100644 --- a/tests/cypress/tests-e2e/drawing.cy.js +++ b/tests/cypress/tests-e2e/drawing.cy.js @@ -413,63 +413,110 @@ describe('Drawing module tests', () => { }) it('keeps the KML after a page reload, and creates a copy if it is then edited', () => { cy.goToDrawing() + cy.log('Create a simple drawing with a marker') cy.clickDrawingTool(EditableFeatureTypes.MARKER) cy.get('[data-cy="ol-map"]').click() - let kmlId cy.wait('@post-kml').then((interception) => { - kmlId = interception.response.body.id - }) - cy.waitUntilState((state) => { - return state.layers.activeLayers.find( - (layer) => layer.type === LayerTypes.KML && layer.fileId === kmlId - ) - }) + const kmlId = interception.response.body.id - cy.reload() - cy.wait('@get-kml') - cy.waitUntilState((state) => { - return state.layers.activeLayers.find( - (layer) => layer.type === LayerTypes.KML && layer.fileId === kmlId + cy.get('[data-cy="drawing-toolbox-close-button"]:visible').click() + cy.wait('@update-kml') + + cy.log('Check that the drawings has been added to the active layers') + cy.get( + `[data-cy="active-layer-name-KML|https://sys-public.dev.bgdi.ch/api/kml/files/${kmlId}"]` ) - }) - // checking that the KML is correctly loaded in the drawing module, even though it doesn't carry an adminId - cy.openDrawingMode() + .should('be.visible') + .contains('Drawing') + cy.waitUntilState((state) => { + return state.layers.activeLayers.find( + (layer) => layer.type === LayerTypes.KML && layer.fileId === kmlId + ) + }) - // if closing the drawing module without changing anything, no copy must be made - cy.get('[data-cy="drawing-toolbox-close-button"]').click() - cy.readStoreValue('getters.activeKmlLayer').then((activeKmlLayer) => { - expect(activeKmlLayer).to.haveOwnProperty('fileId') - expect(activeKmlLayer.fileId).to.eq(kmlId) - }) + cy.log('Reload the page') + cy.reload() + cy.waitMapIsReady() + cy.wait('@get-kml') + cy.clickOnMenuButtonIfMobile() - // re-opening the drawing module - cy.get('[data-cy="menu-tray-drawing-section"]').should('be.visible').click() + cy.log(`Check that the KML file ${kmlId} is present on the active layer list`) + cy.get( + `[data-cy="active-layer-name-KML|https://sys-public.dev.bgdi.ch/api/kml/files/${kmlId}"]` + ) + .should('be.visible') + .contains('Drawing') + cy.waitUntilState((state) => { + return state.layers.activeLayers.find( + (layer) => layer.type === LayerTypes.KML && layer.fileId === kmlId + ) + }) - // deleting all features (clearing/emptying the KML) - cy.get('[data-cy="drawing-toolbox-delete-button"]').click() - cy.get('[data-cy="modal-confirm-button"]').click() - // checking that it creates a copy of the KML, and doesn't edit/clear the existing one (no adminId) - let newKmlId - cy.wait('@post-kml').then((interception) => { - newKmlId = interception.response.body.id - expect(newKmlId).to.not.eq(kmlId) - }) - // there should be only one KML layer left in the layers, and it's the one just saved - cy.readStoreValue('state.layers.activeLayers').then((activeLayers) => { - expect(activeLayers.filter((layer) => layer.type === LayerTypes.KML).length).to.eq( - 1, - 'There should only be one KMl layer left in the layers' + cy.log('Open and close the drawing mode and check that the KML is not altered') + // checking that the KML is correctly loaded in the drawing module, even though it doesn't carry an adminId + cy.get('[data-cy="menu-tray-drawing-section"]').should('be.visible').click() + + // if closing the drawing module without changing anything, no copy must be made + cy.get('[data-cy="drawing-toolbox-close-button"]').click() + cy.get( + `[data-cy="active-layer-name-KML|https://sys-public.dev.bgdi.ch/api/kml/files/${kmlId}"]` ) - const kmlLayer = activeLayers.find((layer) => layer.type === LayerTypes.KML) - expect(kmlLayer.fileId).to.eq(newKmlId) - }) + .should('be.visible') + .contains('Drawing') + cy.readStoreValue('getters.activeKmlLayer').then((activeKmlLayer) => { + expect(activeKmlLayer).to.haveOwnProperty('fileId') + expect(activeKmlLayer.fileId).to.eq(kmlId) + }) - // Add another feature and checking that we do not create subsequent copies (we now have the adminId for this KML) - cy.clickDrawingTool(EditableFeatureTypes.ANNOTATION) - cy.get('[data-cy="ol-map"]').click('center') - cy.wait('@update-kml').then((interception) => { - expect(interception.response.body.id).to.eq(newKmlId) + cy.log('Open again the drawing mode and edit the kml') + // re-opening the drawing module + cy.get('[data-cy="menu-tray-drawing-section"]').should('be.visible').click() + + cy.log('deleting all features (clearing/emptying the KML)') + // deleting all features (clearing/emptying the KML) + cy.get('[data-cy="drawing-toolbox-delete-button"]').click() + cy.get('[data-cy="modal-confirm-button"]').click() + // checking that it creates a copy of the KML, and doesn't edit/clear the existing one (no adminId) + + cy.log('Check that a new kml has been saved') + cy.wait('@post-kml').then((interception) => { + const newKmlId = interception.response.body.id + expect(newKmlId).to.not.eq(kmlId) + + // there should be only one KML layer left in the layers, and it's the one just saved + cy.readStoreValue('state.layers.activeLayers').then((activeLayers) => { + cy.wrap( + activeLayers.filter((layer) => layer.type === LayerTypes.KML) + ).should('have.length', 1) + cy.wrap( + activeLayers.find((layer) => layer.type === LayerTypes.KML).fileId + ).should('be.eq', newKmlId) + }) + + cy.log(`Check that adding a new feature update the new kml ${newKmlId}`) + // Add another feature and checking that we do not create subsequent copies (we now have the adminId for this KML) + cy.clickDrawingTool(EditableFeatureTypes.ANNOTATION) + cy.get('[data-cy="ol-map"]').click('center') + cy.wait('@update-kml').then((interception) => { + expect(interception.response.body.id).to.eq(newKmlId) + }) + + cy.log('Check the active layer list making sure that there is only the new') + cy.get('[data-cy="drawing-toolbox-close-button"]').click() + + cy.log( + `Check that the old kml has been removed from the active layer and that the new one has been added` + ) + cy.get( + `[data-cy="active-layer-name-KML|https://sys-public.dev.bgdi.ch/api/kml/files/${kmlId}"]` + ).should('not.exist') + cy.get( + `[data-cy="active-layer-name-KML|https://sys-public.dev.bgdi.ch/api/kml/files/${newKmlId}"]` + ) + .should('be.visible') + .contains('Drawing') + }) }) }) it('manages the KML layer correctly if it comes attached with an adminId at startup', () => { diff --git a/tests/cypress/tests-e2e/importToolFile.cy.js b/tests/cypress/tests-e2e/importToolFile.cy.js index d483b8d88..b8170913c 100644 --- a/tests/cypress/tests-e2e/importToolFile.cy.js +++ b/tests/cypress/tests-e2e/importToolFile.cy.js @@ -205,6 +205,7 @@ describe('The Import File Tool', () => { // Test reloading the page cy.log('Test reloading the page should only keep online external layers') cy.reload() + cy.waitMapIsReady() cy.clickOnMenuButtonIfMobile() cy.get('[data-cy="menu-section-active-layers"]:visible').children().should('have.length', 1) cy.get(`[data-cy="active-layer-name-KML|${secondValidOnlineUrl}"]`).should('be.visible') @@ -536,6 +537,7 @@ describe('The Import File Tool', () => { // Test reloading the page cy.log('Test reloading the page should only keep online external layers') cy.reload() + cy.waitMapIsReady() cy.wait('@getGpxFile') // only the URL GPX should be kept while reloading cy.checkOlLayer(gpxOnlineLayerId) diff --git a/tests/cypress/tests-e2e/importToolMaps.cy.js b/tests/cypress/tests-e2e/importToolMaps.cy.js index 8a89ea8b5..83f9f5ebc 100644 --- a/tests/cypress/tests-e2e/importToolMaps.cy.js +++ b/tests/cypress/tests-e2e/importToolMaps.cy.js @@ -349,6 +349,7 @@ describe('The Import Maps Tool', () => { //--------------------------------------------------------------------- cy.log('Reload should keep the layers') cy.reload() + cy.waitMapIsReady() cy.clickOnMenuButtonIfMobile() cy.get('[data-cy="menu-section-active-layers"]:visible').children().should('have.length', 3) cy.get(`[data-cy="active-layer-name-${singleLayerFullId}"]`).should('be.visible') From 53c43f1768334790af58dd3a0780c0fc0d13c2cd Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Wed, 28 Feb 2024 07:05:40 +0100 Subject: [PATCH 3/3] PB-259: Improved logging of the router store subscriber Do not log for non watched mutation because this is missleading in the logs. There is already anther global mutation watcher that logs all mutations. --- src/router/storeSync/storeSync.routerPlugin.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/router/storeSync/storeSync.routerPlugin.js b/src/router/storeSync/storeSync.routerPlugin.js index 05e2a141a..900108ec0 100644 --- a/src/router/storeSync/storeSync.routerPlugin.js +++ b/src/router/storeSync/storeSync.routerPlugin.js @@ -49,15 +49,15 @@ let routeChangeIsTriggeredByThisModule = false * @param {Router} router */ function storeMutationWatcher(store, mutation, router) { - log.debug( - '[store sync router] store mutation', - mutation, - 'Current route', - router.currentRoute.value - ) - // Ignore mutation that has been triggered by the router.beforeEach below. if (mutationNotTriggeredByModule(mutation) && mutationWatched(mutation)) { + log.debug( + '[store sync router] store mutation', + mutation, + 'Current route', + router.currentRoute.value + ) + // if the value in the store differs from the one in the URL if (isRoutePushNeeded(store, router.currentRoute.value)) { const query = {} @@ -73,7 +73,7 @@ function storeMutationWatcher(store, mutation, router) { query, }) .catch((error) => { - log.info('Error while routing to', query, error) + log.error('Error while routing to', query, error) }) .finally(() => { routeChangeIsTriggeredByThisModule = false