From 79c4c00f70cb72d61b21423da71b250885223ea7 Mon Sep 17 00:00:00 2001 From: Jordan Brock <115588091+jorbrock@users.noreply.github.com> Date: Fri, 21 Aug 2026 11:55:11 -0700 Subject: [PATCH] feat(web): tag renaming v2 (#27909) * implement updated rename tag feature * Fix tag update handler * Ensure toast displays updated tag name * Ensure toast displays updated tag name * Handle updates to tag closure table, prevent slash in API filename, test case updates * delete child tags on parent delete * Update frontend for better handling of nested tags * pr checklist fixes * fix unit test for tag controller * Hide path for top-level tags * regenerate SQL files * name updates optional for tag update operations * e2e tests for new tag api changes * changes from code review, additional test coverage * test fixes, web component refactor * test assets * test asset updates * open-api rebuild * update unit tests * fix tag dto tests * Update web/src/lib/modals/TagEditModal.svelte Fix missing color bug Co-authored-by: Ray <3608878+Pecacheu@users.noreply.github.com> * fix tag e2e test * remove closure table bug fixes * remove nullish from tag name update schema * regenerate openapi and sql * fix test and e2e test * simplify tag repository update process * minor fix and sql regenerate * fix tests, regen open-api, update dto * Remove unused function * Remove accidental test asset ref * re-add test assets * pin submodule to main branch commit * Update test cases based on feedback * Server fixes based on feedback * regenerate sql --------- Co-authored-by: Ray <3608878+Pecacheu@users.noreply.github.com> --- i18n/en.json | 4 +- open-api/immich-openapi-specs.json | 5 ++ packages/sdk/src/fetch-client.ts | 2 + server/src/controllers/tag.controller.spec.ts | 41 ++++++++++ server/src/dtos/tag.dto.ts | 7 +- server/src/queries/tag.repository.sql | 13 +++- server/src/repositories/tag.repository.ts | 65 +++++++++++++++- server/src/services/tag.service.spec.ts | 17 ++++- server/src/services/tag.service.ts | 15 +++- server/test/medium.factory.ts | 6 ++ .../specs/repositories/tag.repository.spec.ts | 74 +++++++++++++++++++ web/src/lib/constants.ts | 1 + web/src/lib/modals/TagCreateModal.svelte | 16 ++-- web/src/lib/modals/TagEditModal.svelte | 11 ++- web/src/lib/services/tag.service.ts | 2 +- .../[[assetId=id]]/+page.svelte | 10 ++- 16 files changed, 267 insertions(+), 22 deletions(-) create mode 100644 server/test/medium/specs/repositories/tag.repository.spec.ts diff --git a/i18n/en.json b/i18n/en.json index 32be9d6c7..a6dc28fba 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -807,7 +807,7 @@ "create_shared_album_page_share_select_photos": "Select Photos", "create_shared_link": "Create shared link", "create_tag": "Create tag", - "create_tag_description": "Create a new tag. For nested tags, please enter the full path of the tag including forward slashes.", + "create_tag_description": "Create a new tag. For nested tags, the full path where your new tag will be placed is printed below.", "create_user": "Create user", "create_workflow": "Create workflow", "created": "Created", @@ -2128,6 +2128,8 @@ "tag_created": "Created tag: {tag}", "tag_face": "Tag face", "tag_feature_description": "Browsing photos and videos grouped by logical tag topics", + "tag_full_path": "Full path: {tag}", + "tag_not_found_question": "Cannot find a tag? Create a new tag.", "tag_people": "Tag People", "tag_plus_more_tags": "{tag} + {count, plural, one {# tag} other {# tags}}", "tag_updated": "Updated tag: {tag}", diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index 0b832e3c0..66a430ae1 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -28192,6 +28192,11 @@ "nullable": true, "pattern": "^#?([0-9A-Fa-f]{3}|[0-9A-Fa-f]{4}|[0-9A-Fa-f]{6}|[0-9A-Fa-f]{8})$", "type": "string" + }, + "name": { + "description": "Tag name", + "pattern": "^[^/]*$", + "type": "string" } }, "type": "object" diff --git a/packages/sdk/src/fetch-client.ts b/packages/sdk/src/fetch-client.ts index 2834c6366..98444f4c4 100644 --- a/packages/sdk/src/fetch-client.ts +++ b/packages/sdk/src/fetch-client.ts @@ -2876,6 +2876,8 @@ export type TagBulkAssetsResponseDto = { export type TagUpdateDto = { /** Tag color (hex) */ color?: string | null; + /** Tag name */ + name?: string; }; export type TimeBucketAssetResponseDto = { /** Array of city names extracted from EXIF GPS data */ diff --git a/server/src/controllers/tag.controller.spec.ts b/server/src/controllers/tag.controller.spec.ts index a89ce14dd..432ef78f1 100644 --- a/server/src/controllers/tag.controller.spec.ts +++ b/server/src/controllers/tag.controller.spec.ts @@ -2,6 +2,7 @@ import { TagController } from 'src/controllers/tag.controller'; import { TagService } from 'src/services/tag.service'; import request from 'supertest'; import { errorDto } from 'test/medium/responses'; +import { factory } from 'test/small.factory'; import { ControllerContext, controllerSetup, mockBaseService } from 'test/utils'; describe(TagController.name, () => { @@ -33,6 +34,46 @@ describe(TagController.name, () => { }); }); + describe('PUT /tags/:id', () => { + it('should require a valid uuid', async () => { + const { status, body } = await request(ctx.getHttpServer()) + .put(`/tags/123`) + .send({ name: 'tag', color: '#000000' }); + expect(status).toBe(400); + expect(body).toEqual(errorDto.validationError([{ path: ['id'], message: 'Invalid UUID' }])); + }); + it('should require a valid color', async () => { + const { status, body } = await request(ctx.getHttpServer()) + .put(`/tags/${factory.uuid()}`) + .send({ name: 'tag', color: 'invalid-color' }); + expect(status).toBe(400); + expect(body).toEqual( + errorDto.validationError([ + { + path: ['color'], + message: + 'Invalid string: must match pattern /^#?([0-9A-Fa-f]{3}|[0-9A-Fa-f]{4}|[0-9A-Fa-f]{6}|[0-9A-Fa-f]{8})$/', + }, + ]), + ); + }); + it('should throw an error if a slash is in tag name', async () => { + const { status, body } = await request(ctx.getHttpServer()) + .put(`/tags/${factory.uuid()}`) + .send({ name: 'tagA/tagB', color: '#000000' }); + expect(status).toBe(400); + expect(body).toEqual( + errorDto.validationError([{ path: ['name'], message: 'Tag name cannot contain slash characters ("/")' }]), + ); + }); + it('should accept a null color', async () => { + const { status } = await request(ctx.getHttpServer()) + .put(`/tags/${factory.uuid()}`) + .send({ name: 'tagA', color: null }); + expect(status).toBe(200); + }); + }); + describe('DELETE /tags/:id', () => { it('should require a valid uuid', async () => { const { status, body } = await request(ctx.getHttpServer()).delete(`/tags/123`); diff --git a/server/src/dtos/tag.dto.ts b/server/src/dtos/tag.dto.ts index cfbae551a..1c376b9c3 100644 --- a/server/src/dtos/tag.dto.ts +++ b/server/src/dtos/tag.dto.ts @@ -13,8 +13,13 @@ const TagCreateSchema = z }) .meta({ id: 'TagCreateDto' }); -const TagUpdateSchema = z +export const TagUpdateSchema = z .object({ + name: z + .string() + .regex(/^[^/]*$/, `Tag name cannot contain slash characters ("/")`) + .optional() + .describe('Tag name'), color: hexColor.nullable().optional().describe('Tag color (hex)'), }) .meta({ id: 'TagUpdateDto' }); diff --git a/server/src/queries/tag.repository.sql b/server/src/queries/tag.repository.sql index c3b46dd9f..317298bea 100644 --- a/server/src/queries/tag.repository.sql +++ b/server/src/queries/tag.repository.sql @@ -69,13 +69,22 @@ returning * -- TagRepository.update +begin +select + "value" +from + "tag" +where + "id" = $1 update "tag" set - "color" = $1 + "value" = $1, + "color" = $2 where - "id" = $2 + "id" = $3 returning * +rollback -- TagRepository.delete delete from "tag" diff --git a/server/src/repositories/tag.repository.ts b/server/src/repositories/tag.repository.ts index d4572886a..c101f6b68 100644 --- a/server/src/repositories/tag.repository.ts +++ b/server/src/repositories/tag.repository.ts @@ -78,9 +78,68 @@ export class TagRepository { return this.db.insertInto('tag').values(tag).returningAll().executeTakeFirstOrThrow(); } - @GenerateSql({ params: [DummyValue.UUID, { color: DummyValue.STRING }] }) - update(id: string, dto: Updateable) { - return this.db.updateTable('tag').set(dto).where('id', '=', id).returningAll().executeTakeFirstOrThrow(); + @GenerateSql({ params: [DummyValue.UUID, { value: DummyValue.STRING, color: DummyValue.STRING }] }) + async update(id: string, dto: Updateable) { + return this.db.transaction().execute(async (tx) => { + // Get previous tag value for reference if the current update contains a new value + const previousTag = + dto.value === undefined + ? undefined + : await tx.selectFrom('tag').select('value').where('id', '=', id).executeTakeFirst(); + + // Perform main tag update + const updated = await tx + .updateTable('tag') + .set(dto) + .where('id', '=', id) + .returningAll() + .executeTakeFirstOrThrow(); + + // Check if value has changed, trigger value updates on all children if so + if (previousTag && dto.value !== previousTag.value) { + await tx + // Use a recursive cte to get all levels of nested child tags that need to be updated + .withRecursive('descendants(id, value)', (qb) => { + const directChildren = qb + .selectFrom('tag as child') + .select((eb) => [ + 'child.id as id', + eb + .fn('concat', [ + eb.cast(eb.val(updated.value), 'text'), + eb.cast(eb.val('/'), 'text'), + eb.fn('regexp_replace', ['child.value', eb.val('^.*/'), eb.val('')]), + ]) + .as('value'), + ]) + .where('child.parentId', '=', id); + + const nestedChildren = qb + .selectFrom('tag as child') + .innerJoin('descendants as parent', 'parent.id', 'child.parentId') + .select((eb) => [ + 'child.id as id', + eb + .fn('concat', [ + 'parent.value', + eb.cast(eb.val('/'), 'text'), + eb.fn('regexp_replace', ['child.value', eb.val('^.*/'), eb.val('')]), + ]) + .as('value'), + ]); + + return directChildren.unionAll(nestedChildren); + }) + .updateTable('tag') + .from('descendants') + .set((eb) => ({ + value: eb.ref('descendants.value'), + })) + .whereRef('tag.id', '=', 'descendants.id') + .execute(); + } + return updated; + }); } @GenerateSql({ params: [DummyValue.UUID] }) diff --git a/server/src/services/tag.service.spec.ts b/server/src/services/tag.service.spec.ts index 0c748fded..ddc0f388f 100644 --- a/server/src/services/tag.service.spec.ts +++ b/server/src/services/tag.service.spec.ts @@ -104,7 +104,15 @@ describe(TagService.name, () => { describe('update', () => { it('should throw an error for no update permission', async () => { mocks.access.tag.checkOwnerAccess.mockResolvedValue(new Set()); - await expect(sut.update(authStub.admin, 'tag-1', { color: '#000000' })).rejects.toBeInstanceOf( + await expect(sut.update(authStub.admin, 'tag-1', { name: 'tag', color: '#000000' })).rejects.toBeInstanceOf( + BadRequestException, + ); + expect(mocks.tag.update).not.toHaveBeenCalled(); + }); + + it('should throw an error if updated tag name has a slash', async () => { + mocks.access.tag.checkOwnerAccess.mockResolvedValue(new Set(['tag-parent'])); + await expect(sut.update(authStub.admin, 'tag-1', { name: 'tag/test2', color: '#000000' })).rejects.toBeInstanceOf( BadRequestException, ); expect(mocks.tag.update).not.toHaveBeenCalled(); @@ -113,8 +121,11 @@ describe(TagService.name, () => { it('should update a tag', async () => { mocks.access.tag.checkOwnerAccess.mockResolvedValue(new Set(['tag-1'])); mocks.tag.update.mockResolvedValue(tagStub.colorCreate); - await expect(sut.update(authStub.admin, 'tag-1', { color: '#000000' })).resolves.toEqual(tagResponseStub.color1); - expect(mocks.tag.update).toHaveBeenCalledWith('tag-1', { color: '#000000' }); + mocks.tag.get.mockResolvedValue(tagStub.tag); + await expect(sut.update(authStub.admin, 'tag-1', { name: 'tag', color: '#000000' })).resolves.toEqual( + tagResponseStub.color1, + ); + expect(mocks.tag.update).toHaveBeenCalledWith('tag-1', { value: 'tag', color: '#000000' }); }); }); diff --git a/server/src/services/tag.service.ts b/server/src/services/tag.service.ts index 08a9b0010..f1fadd5c7 100644 --- a/server/src/services/tag.service.ts +++ b/server/src/services/tag.service.ts @@ -59,8 +59,19 @@ export class TagService extends BaseService { async update(auth: AuthDto, id: string, dto: TagUpdateDto): Promise { await this.requireAccess({ auth, permission: Permission.TagUpdate, ids: [id] }); - const { color } = dto; - const tag = await this.tagRepository.update(id, { color }); + const { name, color } = dto; + const existing = await this.findOrFail(id); + + let value; + if (name) { + const parts = existing.value.split('/'); + parts[parts.length - 1] = name; + value = parts.join('/'); + } else { + value = existing.value; + } + + const tag = await this.tagRepository.update(id, { value, color }); return mapTag(tag); } diff --git a/server/test/medium.factory.ts b/server/test/medium.factory.ts index f6a2497e7..886f090b5 100644 --- a/server/test/medium.factory.ts +++ b/server/test/medium.factory.ts @@ -316,6 +316,12 @@ export class MediumTestContext = }; } + async newTag(dto: Insertable) { + const tag = mediumFactory.tagInsert(dto); + const result = await this.get(TagRepository).create(tag); + return { tag, result }; + } + async newTagAsset(tagBulkAssets: { tagIds: string[]; assetIds: string[] }) { const tagsAssets: Insertable[] = []; for (const tagId of tagBulkAssets.tagIds) { diff --git a/server/test/medium/specs/repositories/tag.repository.spec.ts b/server/test/medium/specs/repositories/tag.repository.spec.ts new file mode 100644 index 000000000..24a89961e --- /dev/null +++ b/server/test/medium/specs/repositories/tag.repository.spec.ts @@ -0,0 +1,74 @@ +import { Kysely } from 'kysely'; +import { LoggingRepository } from 'src/repositories/logging.repository'; +import { TagRepository } from 'src/repositories/tag.repository'; +import { DB } from 'src/schema'; +import { BaseService } from 'src/services/base.service'; +import { newMediumService } from 'test/medium.factory'; +import { getKyselyDB } from 'test/utils'; + +let defaultDatabase: Kysely; + +const setup = (db?: Kysely) => { + const { ctx } = newMediumService(BaseService, { + database: db || defaultDatabase, + real: [], + mock: [LoggingRepository], + }); + return { ctx, sut: ctx.get(TagRepository) }; +}; + +beforeAll(async () => { + defaultDatabase = await getKyselyDB(); +}); + +describe(TagRepository.name, () => { + afterEach(async () => { + const { ctx } = setup(); + await ctx.database.deleteFrom('tag_closure').execute(); + await ctx.database.deleteFrom('tag_asset').execute(); + await ctx.database.deleteFrom('tag').execute(); + }); + + describe('update', () => { + it('should update a tag color', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + + const { tag } = await ctx.newTag({ + userId: user.id, + value: 'tagA', + color: '#000000', + }); + + await sut.update(tag.id, { color: '#FFFFFF' }); + + await expect( + ctx.database + .selectFrom('tag') + .select(['userId', 'value', 'color', 'parentId']) + .where('id', '=', tag.id) + .executeTakeFirstOrThrow(), + ).resolves.toEqual({ userId: user.id, value: 'tagA', color: '#FFFFFF', parentId: null }); + }); + it('should update a top-level tag value', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + + const { tag } = await ctx.newTag({ + userId: user.id, + value: 'tagA', + color: '#000000', + }); + + await sut.update(tag.id, { value: 'updatedTagA' }); + + await expect( + ctx.database + .selectFrom('tag') + .select(['userId', 'value', 'color', 'parentId']) + .where('id', '=', tag.id) + .executeTakeFirstOrThrow(), + ).resolves.toEqual({ userId: user.id, value: 'updatedTagA', color: '#000000', parentId: null }); + }); + }); +}); diff --git a/web/src/lib/constants.ts b/web/src/lib/constants.ts index 530db1e8d..af63ea5ae 100644 --- a/web/src/lib/constants.ts +++ b/web/src/lib/constants.ts @@ -257,6 +257,7 @@ export enum SettingInputFieldType { NUMBER = 'number', PASSWORD = 'password', COLOR = 'color', + NAME = 'name', } export const AlbumPageViewMode = { diff --git a/web/src/lib/modals/TagCreateModal.svelte b/web/src/lib/modals/TagCreateModal.svelte index 268053a4c..c9c8810ee 100644 --- a/web/src/lib/modals/TagCreateModal.svelte +++ b/web/src/lib/modals/TagCreateModal.svelte @@ -1,7 +1,9 @@ - {$t('create_tag_description')} - - - + {$t('create_tag_description')} + + {#if tagValue !== ''} + {$t('tag_full_path', { values: { tag: tagValue } })}{tagName} + {/if} diff --git a/web/src/lib/modals/TagEditModal.svelte b/web/src/lib/modals/TagEditModal.svelte index d1d467064..06f7f0339 100644 --- a/web/src/lib/modals/TagEditModal.svelte +++ b/web/src/lib/modals/TagEditModal.svelte @@ -3,7 +3,7 @@ import { SettingInputFieldType } from '$lib/constants'; import { handleUpdateTag } from '$lib/services/tag.service'; import type { TreeNode } from '$lib/utils/tree-utils'; - import { FormModal } from '@immich/ui'; + import { FormModal, Text } from '@immich/ui'; import { mdiTag } from '@mdi/js'; import { t } from 'svelte-i18n'; @@ -15,9 +15,12 @@ const { tag, onClose }: Props = $props(); let tagColor = $state(tag.color ?? ''); + let tagName = $state(tag.value ?? ''); + let tagPath = $state(tag.path ?? ''); + const tagPathDisplay = $state(tagPath.endsWith(tagName) ? tagPath.slice(0, -tagName.length) : tagPath); const onSubmit = async () => { - const success = await handleUpdateTag(tag, { color: tagColor }); + const success = await handleUpdateTag(tag, { color: tagColor || null, name: tagName }); if (success) { onClose(); } @@ -26,4 +29,8 @@ + + {#if tagPathDisplay !== ''} + {$t('tag_full_path', { values: { tag: tagPathDisplay } })}{tagName} + {/if} diff --git a/web/src/lib/services/tag.service.ts b/web/src/lib/services/tag.service.ts index ccd05f412..6d3324311 100644 --- a/web/src/lib/services/tag.service.ts +++ b/web/src/lib/services/tag.service.ts @@ -61,7 +61,7 @@ export const handleUpdateTag = async (tag: TreeNode, dto: TagUpdateDto) => { try { const response = await updateTag({ id: tag.id, tagUpdateDto: dto }); - toastManager.primary($t('tag_updated', { values: { tag: tag.value } })); + toastManager.primary($t('tag_updated', { values: { tag: response?.value || tag.value } })); eventManager.emit('TagUpdate', response); return true; diff --git a/web/src/routes/(user)/tags/[[photos=photos]]/[[assetId=id]]/+page.svelte b/web/src/routes/(user)/tags/[[photos=photos]]/[[assetId=id]]/+page.svelte index 635359240..6f215d775 100644 --- a/web/src/routes/(user)/tags/[[photos=photos]]/[[assetId=id]]/+page.svelte +++ b/web/src/routes/(user)/tags/[[photos=photos]]/[[assetId=id]]/+page.svelte @@ -63,6 +63,14 @@ tags = await getAllTags(); }; + const onTagUpdate = async (response: TagResponseDto) => { + if (response.value !== tag.path) { + await navigateToView(response.value || ''); + } + + await onRefresh(); + }; + const onTagDelete = async (response: TreeNode) => { if (response.path === tag.path) { await navigateToView(tag.parent ? tag.parent.path : ''); @@ -74,7 +82,7 @@ const { Create, Update, Delete } = $derived(getTagActions($t, tag)); - + {#snippet sidebar()}