From 258e7137d7616a55356b65cad677fa7e937d4f0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20K=C3=BCnzi?= Date: Tue, 26 Aug 2025 09:10:06 +0200 Subject: [PATCH 1/3] PB-1895: Stop opening drawing mode at startup when the KML is not valid Issue: When starting the mapviewer with an edit link for a KML that was deleted, the drawing module is open and can't save to a new KML as the presence of the initial admin ID is interfering with that function. Fix: We now ignore the admin ID and display an error when someone is trying to load a KML that was deleted. Also: We altered the structure of messages a bit so that the same error given by the same source is not shown, but the same error from a different source would be displayed Also Also : We added some tests to check that the message equality function was working correctly. Also Also Also : We added an acknowledgement system which stops the same source from sending the same errors multiple times --- packages/geoadmin-log/src/Message.ts | 15 +++- .../geoadmin-log/src/__test__/message.spec.js | 69 +++++++++++++++++++ packages/mapviewer/src/App.vue | 2 +- .../map/components/cesium/CesiumWMTSLayer.vue | 7 +- ...generateErrorMessageFromErrorType.utils.js | 13 ++-- .../storeSync/LayerParamConfig.class.js | 7 -- .../src/store/modules/layers.store.js | 2 +- .../mapviewer/src/store/modules/ui.store.js | 24 ++++++- .../store/plugins/external-layers.plugin.js | 5 +- .../src/store/plugins/load-gpx-data.plugin.js | 4 +- .../store/plugins/load-kml-kmz-data.plugin.js | 33 +++++++-- .../src/utils/components/FeedbackPopup.vue | 18 ++--- 12 files changed, 158 insertions(+), 41 deletions(-) create mode 100644 packages/geoadmin-log/src/__test__/message.spec.js diff --git a/packages/geoadmin-log/src/Message.ts b/packages/geoadmin-log/src/Message.ts index ad6e767c51..d7c0ebb29f 100644 --- a/packages/geoadmin-log/src/Message.ts +++ b/packages/geoadmin-log/src/Message.ts @@ -3,22 +3,31 @@ export class Message { msg: string params: Record - + /** + * Flag telling that the user has seen the error and closed it, to avoid seeing again + */ + isAcknowledged: boolean + sourceId? : string /** * @param {string} msg Translation key message * @param {any} params Translation params to pass to i18n (used for message formatting) */ - constructor(msg: string, params: Record | null = null) { + constructor(msg: string, params: Record | null = null, sourceId? : string, isAcknowledged: boolean = false ) { this.msg = msg this.params = params ?? {} + this.isAcknowledged = isAcknowledged + this.sourceId = sourceId } + // It is important that the Acknowledgement of the message is not part of the equality check. + // As this is used to stop new errors that have the same source to be dispatched. isEquals(object: Message) { return ( object instanceof Message && object.msg === this.msg && Object.keys(this.params).length === Object.keys(object.params).length && - Object.keys(this.params).every((key) => this.params[key] === object.params[key]) + Object.keys(this.params).every((key) => this.params[key] === object.params[key]) && + this.sourceId === object.sourceId ) } } diff --git a/packages/geoadmin-log/src/__test__/message.spec.js b/packages/geoadmin-log/src/__test__/message.spec.js new file mode 100644 index 0000000000..c94d28efcb --- /dev/null +++ b/packages/geoadmin-log/src/__test__/message.spec.js @@ -0,0 +1,69 @@ +import { expect } from 'chai' +import { ErrorMessage } from 'packages/geoadmin-log/src/Message' +import { describe, it } from 'vitest' + +describe('Unit Tests for messages', () => { + describe('checking equality between messages', () => { + it('Ensure that errors with no sources are considered equal too when the rest is equal', () => { + const msg1 = new ErrorMessage('test text', {}, null) + const msg2 = new ErrorMessage('test text', {}, null) + const msg3 = new ErrorMessage('test text', { key1: 'value1', key2: 'value2' }, null) + const msg4 = new ErrorMessage('test text', { key1: 'value1', key2: 'value2' }, null) + expect(msg1.isEquals(msg1)).to.equal(true) + expect(msg1.isEquals(msg2)).to.equal(true) + expect(msg3.isEquals(msg3)).to.equal(true) + expect(msg3.isEquals(msg4)).to.equal(true) + }) + it('detects equality and inequality between two messages', () => { + // messages without parameter + const simpleTextMsg1 = new ErrorMessage('test text', {}, 'test_source_12345') + const copyOfimpleTextMsg1 = new ErrorMessage('test text', {}, 'test_source_12345') + const simpleTextMsg1OtherSource = new ErrorMessage('test text', {}, 'test_source_02345') + const simpleTextMsg2 = new ErrorMessage('Another test text', {}, 'test_source_12345') + + expect(simpleTextMsg1.isEquals(simpleTextMsg1)).to.be.true + expect(simpleTextMsg1.isEquals(copyOfimpleTextMsg1)).to.be.true + expect(simpleTextMsg1.isEquals(simpleTextMsg1OtherSource)).to.be.false // different source + expect(simpleTextMsg1.isEquals(simpleTextMsg2)).to.be.false // different msg + + // messages with parameters + const paramMsg1 = new ErrorMessage( + 'test text', + { key1: 'value1', key2: 'value2' }, + 'test_source_12345' + ) + const copyOfParamMsg1 = new ErrorMessage( + 'test text', + { key1: 'value1', key2: 'value2' }, + 'test_source_12345' + ) + const paramMsg2 = new ErrorMessage( + 'test text', + { key1: 'value2', key2: 'value2' }, + 'test_source_12345' + ) + + const paramMsg3 = new ErrorMessage( + 'test text', + { key2: 'value1', key3: 'value2' }, + 'test_source_12345' + ) + const paramMsg4 = new ErrorMessage( + 'Another test text', + { key1: 'value1', key2: 'value2' }, + 'test_source_12345' + ) + expect(simpleTextMsg1.isEquals(paramMsg1)).to.be.false // empty params vs params + expect(paramMsg1.isEquals(paramMsg1)).to.be.true + expect(paramMsg1.isEquals(copyOfParamMsg1)).to.be.true + expect(paramMsg1.isEquals(paramMsg4)).to.be.false // different text with params + expect(paramMsg1.isEquals(paramMsg2)).to.be.false // different values in params + expect(paramMsg1.isEquals(paramMsg3)).to.be.false // different keys in params + }) + it('ensure that acknowledement of a message does not change equality', () => { + const msg1 = new ErrorMessage('test', {}, null, false) + const msg2 = new ErrorMessage('test', {}, null, true) + expect(msg1.isEquals(msg2)).to.be.true + }) + }) +}) diff --git a/packages/mapviewer/src/App.vue b/packages/mapviewer/src/App.vue index b98c0792d9..49eac03f39 100644 --- a/packages/mapviewer/src/App.vue +++ b/packages/mapviewer/src/App.vue @@ -23,7 +23,7 @@ const dispatcher = { dispatcher: 'App.vue' } let debouncedOnResize const showFeedbackPopup = computed(() => { - return store.state.ui.errors.size + store.state.ui.warnings.size > 0 + return store.getters.getFirstError || store.getters.getFirstWarning }) const showDebugToolbar = computed(() => !IS_TESTING_WITH_CYPRESS && store.getters.hasDevSiteWarning) diff --git a/packages/mapviewer/src/modules/map/components/cesium/CesiumWMTSLayer.vue b/packages/mapviewer/src/modules/map/components/cesium/CesiumWMTSLayer.vue index a15d3991a8..533a942024 100644 --- a/packages/mapviewer/src/modules/map/components/cesium/CesiumWMTSLayer.vue +++ b/packages/mapviewer/src/modules/map/components/cesium/CesiumWMTSLayer.vue @@ -15,7 +15,11 @@ import { getWmtsXyzUrl } from '@/utils/layerUtils' const dispatcher = { dispatcher: 'CesiumWMTSLayer.vue' } const MAXIMUM_LEVEL_OF_DETAILS = 18 -const unsupportedProjectionError = new ErrorMessage('3d_unsupported_projection') +const unsupportedProjectionError = new ErrorMessage( + '3d_unsupported_projection', + {}, + wmtsLayerConfig.id +) const { wmtsLayerConfig, zIndex, parentLayerOpacity } = defineProps({ wmtsLayerConfig: { @@ -59,6 +63,7 @@ const tileMatrixSet = computed(() => { error: unsupportedProjectionError, ...dispatcher, }) + store.dispatch('addErrors', [unsupportedProjectionError], dispatcher) } return wmtsLayerConfig.tileMatrixSets }) diff --git a/packages/mapviewer/src/modules/menu/components/advancedTools/ImportFile/parser/errors/generateErrorMessageFromErrorType.utils.js b/packages/mapviewer/src/modules/menu/components/advancedTools/ImportFile/parser/errors/generateErrorMessageFromErrorType.utils.js index bb48708561..36dd30675b 100644 --- a/packages/mapviewer/src/modules/menu/components/advancedTools/ImportFile/parser/errors/generateErrorMessageFromErrorType.utils.js +++ b/packages/mapviewer/src/modules/menu/components/advancedTools/ImportFile/parser/errors/generateErrorMessageFromErrorType.utils.js @@ -10,17 +10,18 @@ import UnknownProjectionError from '@/modules/menu/components/advancedTools/Impo * function). * * @param {Error} error An error raised by a FileParser, or the parseAll + * @param {AbstractLayer} layer The layer which originated the error * @returns {ErrorMessage} */ -export default function generateErrorMessageFromErrorType(error) { +export default function generateErrorMessageFromErrorType(error, layer) { if (error instanceof AxiosError || /fetch/.test(error.message)) { - return new ErrorMessage('loading_error_network_failure') + return new ErrorMessage('loading_error_network_failure', {}, layer?.id) } else if (error instanceof OutOfBoundsError) { - return new ErrorMessage('imported_file_out_of_bounds') + return new ErrorMessage('imported_file_out_of_bounds', {}, layer?.id) } else if (error instanceof EmptyFileContentError) { - return new ErrorMessage('kml_gpx_file_empty') + return new ErrorMessage('kml_gpx_file_empty', {}, layer?.id) } else if (error instanceof UnknownProjectionError) { - return new ErrorMessage('unknown_projection_error', { epsg: error.epsg }) + return new ErrorMessage('unknown_projection_error', { epsg: error.epsg }, layer?.id) } - return new ErrorMessage('invalid_import_file_error') + return new ErrorMessage('invalid_import_file_error', {}, layer?.id) } diff --git a/packages/mapviewer/src/router/storeSync/LayerParamConfig.class.js b/packages/mapviewer/src/router/storeSync/LayerParamConfig.class.js index 57a725c216..d45f4ec10e 100644 --- a/packages/mapviewer/src/router/storeSync/LayerParamConfig.class.js +++ b/packages/mapviewer/src/router/storeSync/LayerParamConfig.class.js @@ -185,13 +185,6 @@ function dispatchLayersFromUrlIntoStore(to, store, urlParamValue) { ) if (layerObject) { if (layerObject.type === LayerTypes.KML && layerObject.adminId) { - promisesForAllDispatch.push( - store.dispatch('setShowDrawingOverlay', { - show: true, - dispatcher: STORE_DISPATCHER_ROUTER_PLUGIN, - }) - ) - promisesForAllDispatch.push( store.dispatch('setIsVisitWithAdminId', { value: true, diff --git a/packages/mapviewer/src/store/modules/layers.store.js b/packages/mapviewer/src/store/modules/layers.store.js index cca6b0a9a9..1d8309339a 100644 --- a/packages/mapviewer/src/store/modules/layers.store.js +++ b/packages/mapviewer/src/store/modules/layers.store.js @@ -694,7 +694,7 @@ const actions = { * @param {ErrorMessage} error Error translation key to add * @param {string} dispatcher Action dispatcher name */ - addLayerError({ commit, getters }, { layerId, isExternal, baseUrl, error, dispatcher }) { + addLayerError({ commit, store, getters }, { layerId, isExternal, baseUrl, error, dispatcher }) { const layers = getters.getLayersById(layerId, isExternal, baseUrl) if (layers.length === 0) { throw new Error( diff --git a/packages/mapviewer/src/store/modules/ui.store.js b/packages/mapviewer/src/store/modules/ui.store.js index 3e6d428168..66fb25e01f 100644 --- a/packages/mapviewer/src/store/modules/ui.store.js +++ b/packages/mapviewer/src/store/modules/ui.store.js @@ -316,6 +316,12 @@ export default { hideEmbedUI(state) { return state.hideEmbedUI }, + getFirstError(state) { + return Array.from(state.errors).find((error) => !error.isAcknowledged) + }, + getFirstWarning(state) { + return Array.from(state.warnings).find((warning) => !warning.isAcknowledged) + }, }, actions: { setSize({ commit, state }, { width, height, dispatcher }) { @@ -453,14 +459,21 @@ export default { } }, - removeError({ commit, state }, { error, dispatcher }) { + acknowledgedError({ commit, state }, { error, dispatcher }) { if (!(error instanceof ErrorMessage)) { throw new Error( `Error ${error} dispatched by ${dispatcher} is not of type ErrorMessage` ) } if (state.errors.has(error)) { + const acknowledgedError = new ErrorMessage( + error.msg, + error.params, + error.sourceId, + true + ) commit('removeError', { error, dispatcher }) + commit('addErrors', { errors: [acknowledgedError], dispatcher }) } }, addWarnings({ commit, state }, { warnings, dispatcher }) { @@ -482,14 +495,21 @@ export default { ) } }, - removeWarning({ commit, state }, { warning, dispatcher }) { + acknowledgeWarning({ commit, state }, { warning, dispatcher }) { if (!(warning instanceof WarningMessage)) { throw new Error( `Warning ${warning} dispatched by ${dispatcher} is not of type WarningMessage` ) } if (state.warnings.has(warning)) { + const acknowledgedWarning = new WarningMessage( + warning.msg, + warning.params, + warning.sourceId, + true + ) commit('removeWarning', { warning, dispatcher }) + commit('addWarnings', { warnings: [acknowledgedWarning], dispatcher }) } }, setShowDragAndDropOverlay({ commit }, { showDragAndDropOverlay, dispatcher }) { diff --git a/packages/mapviewer/src/store/plugins/external-layers.plugin.js b/packages/mapviewer/src/store/plugins/external-layers.plugin.js index 4dd19d5be3..da90238957 100644 --- a/packages/mapviewer/src/store/plugins/external-layers.plugin.js +++ b/packages/mapviewer/src/store/plugins/external-layers.plugin.js @@ -112,13 +112,16 @@ async function updateExternalLayer(store, capabilities, layer, projection) { return updated } catch (error) { log.error(`Failed to update external layer ${layer.id}: `, error) + const errorMessage = new ErrorMessage(error.key ?? 'error', {}, layer.id) store.dispatch('addLayerError', { layerId: layer.id, isExternal: layer.isExternal, baseUrl: layer.baseUrl, - error: new ErrorMessage(error.key ?? 'error'), + error: errorMessage, ...dispatcher, }) + store.dispatch('addErrors', [errorMessage], dispatcher) + return null } } diff --git a/packages/mapviewer/src/store/plugins/load-gpx-data.plugin.js b/packages/mapviewer/src/store/plugins/load-gpx-data.plugin.js index f0f6e5053c..3d685ebad7 100644 --- a/packages/mapviewer/src/store/plugins/load-gpx-data.plugin.js +++ b/packages/mapviewer/src/store/plugins/load-gpx-data.plugin.js @@ -31,13 +31,15 @@ async function loadGpx(store, gpxLayer) { }) } catch (error) { log.error(`Error while fetching GPX data for layer ${gpxLayer?.id}`, error) + const errorMessage = new ErrorMessage('loading_error_network_failure', {}, gpxLayer.id) store.dispatch('addLayerError', { layerId: gpxLayer.id, isExternal: gpxLayer.isExternal, baseUrl: gpxLayer.baseUrl, - error: new ErrorMessage('loading_error_network_failure'), + error: errorMessage, ...dispatcher, }) + store.dispatch('addErrors', [errorMessage], dispatcher) } } diff --git a/packages/mapviewer/src/store/plugins/load-kml-kmz-data.plugin.js b/packages/mapviewer/src/store/plugins/load-kml-kmz-data.plugin.js index fa5a35de42..991c36328e 100644 --- a/packages/mapviewer/src/store/plugins/load-kml-kmz-data.plugin.js +++ b/packages/mapviewer/src/store/plugins/load-kml-kmz-data.plugin.js @@ -34,7 +34,26 @@ async function loadMetadata(store, kmlLayer) { }, ...dispatcher, }) + if (kmlLayer.adminId) { + store.dispatch('setShowDrawingOverlay', { + show: true, + ...dispatcher, + }) + } + // if admin id dispatch open drawing } catch (error) { + // ajouter error message here + if (kmlLayer?.adminId) { + //kmlLayer.adminId = null + kmlLayer.addErrorMessage( + new ErrorMessage( + 'BONJOUR EDITEUR', + { layerName: kmlLayer.name ?? kmlLayer.id }, + kmlLayer.id + ) + ) + } + // TODO set admin Id to null log.error(`Error while fetching KML metadata for layer ${kmlLayer?.id}`, error) } } @@ -95,15 +114,19 @@ async function loadData(store, kmlLayer) { } if (!mimeType && !loadedContent) { log.error('[load-kml-kmz-data] could not get content for KML', kmlLayer.kmlFileUrl) + const errorMessage = new ErrorMessage( + kmlLayer.isExternal ? 'loading_error_network_failure' : 'loading_error_file_deleted', + {}, + kmlLayer.id + ) store.dispatch('addLayerError', { layerId: kmlLayer.id, isExternal: kmlLayer.isExternal, baseUrl: kmlLayer.baseUrl, - error: new ErrorMessage( - kmlLayer.isExternal ? 'loading_error_network_failure' : 'loading_error_file_deleted' - ), + error: errorMessage, ...dispatcher, }) + store.dispatch('addErrors', { errors: [errorMessage] }, dispatcher) // stopping there, there won't be anything to do with this file return } @@ -142,13 +165,15 @@ async function loadData(store, kmlLayer) { log.error( `[load-kml-kmz-data] Error while fetching KML data for layer ${kmlLayer?.id}: ${error}` ) + const errorMessage = generateErrorMessageFromErrorType(error, kmlLayer) store.dispatch('addLayerError', { layerId: kmlLayer.id, isExternal: kmlLayer.isExternal, baseUrl: kmlLayer.baseUrl, - error: generateErrorMessageFromErrorType(error), + error: errorMessage, ...dispatcher, }) + store.dispatch('addErrors', [errorMessage], dispatcher) } } diff --git a/packages/mapviewer/src/utils/components/FeedbackPopup.vue b/packages/mapviewer/src/utils/components/FeedbackPopup.vue index e56245e267..e7d3fec970 100644 --- a/packages/mapviewer/src/utils/components/FeedbackPopup.vue +++ b/packages/mapviewer/src/utils/components/FeedbackPopup.vue @@ -7,18 +7,8 @@ import ErrorWindow from '@/utils/components/ErrorWindow.vue' import WarningWindow from '@/utils/components/WarningWindow.vue' const store = useStore() const { t } = useI18n() -const error = computed(() => { - if (store.state.ui.errors.size > 0) { - return store.state.ui.errors.values().next().value - } - return null -}) -const warning = computed(() => { - if (store.state.ui.warnings.size > 0) { - return store.state.ui.warnings.values().next().value - } - return null -}) +const error = computed(() => store.getters.getFirstError) +const warning = computed(() => store.getters.getFirstWarning)