From f425d98b4d7593445ab2fda74f827ad8b5abe759 Mon Sep 17 00:00:00 2001 From: kolaente Date: Fri, 13 Sep 2024 18:42:42 +0200 Subject: [PATCH] fix(filters): do not replace filter or project values when the id value resolves to undefined This change fixes a bug where the label title in the query string would be replaced to undefined, resulting in an invalid filter. The underlying problem was the resolved filter query string got re-parsed and the id value of the labels were resolved to undefined (and rendered as that string) in the process. Resolves https://community.vikunja.io/t/filtering-by-label-ux-issues/2393/14 --- .../project/partials/FilterInput.vue | 54 ++++++++++--------- .../components/project/partials/Filters.vue | 2 +- frontend/src/helpers/filters.test.ts | 40 ++++++++++++++ frontend/src/helpers/filters.ts | 8 +-- 4 files changed, 73 insertions(+), 31 deletions(-) diff --git a/frontend/src/components/project/partials/FilterInput.vue b/frontend/src/components/project/partials/FilterInput.vue index d9c39d149..61a0281f4 100644 --- a/frontend/src/components/project/partials/FilterInput.vue +++ b/frontend/src/components/project/partials/FilterInput.vue @@ -218,33 +218,35 @@ function handleFieldInput() { // eslint-disable-next-line @typescript-eslint/no-unused-vars const [matched, prefix, operator, space, keyword] = match - if (keyword) { - let search = keyword - if (operator === 'in' || operator === '?=') { - const keywords = keyword.split(',') - search = keywords[keywords.length - 1].trim() - } - if (matched.startsWith('label')) { - autocompleteResultType.value = 'labels' - autocompleteResults.value = labelStore.filterLabelsByQuery([], search) - } - if (matched.startsWith('assignee')) { - autocompleteResultType.value = 'assignees' - if (props.projectId) { - projectUserService.getAll({projectId: props.projectId}, {s: search}) - .then(users => autocompleteResults.value = users.length > 1 ? users : []) - } else { - userService.getAll({}, {s: search}) - .then(users => autocompleteResults.value = users.length > 1 ? users : []) - } - } - if (!props.projectId && matched.startsWith('project')) { - autocompleteResultType.value = 'projects' - autocompleteResults.value = projectStore.searchProject(search) - } - autocompleteMatchText.value = keyword - autocompleteMatchPosition.value = match.index + prefix.length - 1 + keyword.replace(search, '').length + if(!keyword) { + return } + + let search = keyword + if (operator === 'in' || operator === '?=') { + const keywords = keyword.split(',') + search = keywords[keywords.length - 1].trim() + } + if (matched.startsWith('label')) { + autocompleteResultType.value = 'labels' + autocompleteResults.value = labelStore.filterLabelsByQuery([], search) + } + if (matched.startsWith('assignee')) { + autocompleteResultType.value = 'assignees' + if (props.projectId) { + projectUserService.getAll({projectId: props.projectId}, {s: search}) + .then(users => autocompleteResults.value = users.length > 1 ? users : []) + } else { + userService.getAll({}, {s: search}) + .then(users => autocompleteResults.value = users.length > 1 ? users : []) + } + } + if (!props.projectId && matched.startsWith('project')) { + autocompleteResultType.value = 'projects' + autocompleteResults.value = projectStore.searchProject(search) + } + autocompleteMatchText.value = keyword + autocompleteMatchPosition.value = match.index + prefix.length - 1 + keyword.replace(search, '').length }) } diff --git a/frontend/src/components/project/partials/Filters.vue b/frontend/src/components/project/partials/Filters.vue index 2a540e97b..7d6531482 100644 --- a/frontend/src/components/project/partials/Filters.vue +++ b/frontend/src/components/project/partials/Filters.vue @@ -112,7 +112,7 @@ watch( const val = {...value} val.filter = transformFilterStringFromApi( val?.filter || '', - labelId => labelStore.getLabelById(labelId)?.title, + labelId => labelStore.getLabelById(labelId)?.title || null, projectId => projectStore.projects[projectId]?.title || null, ) params.value = val diff --git a/frontend/src/helpers/filters.test.ts b/frontend/src/helpers/filters.test.ts index 3804c8a8e..ec933f296 100644 --- a/frontend/src/helpers/filters.test.ts +++ b/frontend/src/helpers/filters.test.ts @@ -199,6 +199,26 @@ describe('Filter Transformation', () => { expect(transformed).toBe('labels in lorem, ipsum') }) + it('should not touch the label value when it is undefined', () => { + const transformed = transformFilterStringFromApi( + 'labels = one', + (id: number) => undefined, + nullIdToTitleResolver, + ) + + expect(transformed).toBe('labels = one') + }) + + it('should not touch the label value when it is null', () => { + const transformed = transformFilterStringFromApi( + 'labels = one', + (id: number) => null, + nullIdToTitleResolver, + ) + + expect(transformed).toBe('labels = one') + }) + it('should correctly resolve projects', () => { const transformed = transformFilterStringFromApi( 'project = 1', @@ -228,6 +248,26 @@ describe('Filter Transformation', () => { expect(transformed).toBe('project in lorem, ipsum') }) + + it('should not touch the project value when it is undefined', () => { + const transformed = transformFilterStringFromApi( + 'project = one', + nullIdToTitleResolver, + (id: number) => undefined, + ) + + expect(transformed).toBe('project = one') + }) + + it('should not touch the project value when it is null', () => { + const transformed = transformFilterStringFromApi( + 'project = one', + nullIdToTitleResolver, + (id: number) => null, + ) + + expect(transformed).toBe('project = one') + }) it('should transform the same attribute multiple times', () => { const transformed = transformFilterStringFromApi( diff --git a/frontend/src/helpers/filters.ts b/frontend/src/helpers/filters.ts index 029d3536b..e63814a6a 100644 --- a/frontend/src/helpers/filters.ts +++ b/frontend/src/helpers/filters.ts @@ -141,8 +141,8 @@ export function transformFilterStringForApi( export function transformFilterStringFromApi( filter: string, - labelResolver: (id: number) => string | null, - projectResolver: (id: number) => string | null, + labelResolver: (id: number) => string | null | undefined, + projectResolver: (id: number) => string | null | undefined, ): string { if (filter.trim() === '') { @@ -170,7 +170,7 @@ export function transformFilterStringFromApi( keywords.forEach(k => { const labelTitle = labelResolver(parseInt(k)) - if (labelTitle !== null) { + if (labelTitle) { filter = filter.replace(k, labelTitle) } }) @@ -194,7 +194,7 @@ export function transformFilterStringFromApi( keywords.forEach(k => { const project = projectResolver(parseInt(k)) - if (project !== null) { + if (project) { filter = filter.replace(k, project) } })