From 70956f9ecdd5f4cca8e9c81d65407bd5ad9d7146 Mon Sep 17 00:00:00 2001 From: Antonis Date: Mon, 22 Sep 2025 17:10:29 +0300 Subject: [PATCH] Change tags to use uids instead of ids. (#351) * Small cleanups * Change tags to use uid instead of id. --------- Co-authored-by: antanst <> --- backend/routes/notes.js | 2 +- backend/routes/projects.js | 2 +- backend/routes/tags.js | 110 ++++++------------ backend/routes/tasks.js | 2 +- .../validation.js => services/tagsService.js} | 0 backend/services/taskEventService.js | 11 +- backend/tests/integration/tags.test.js | 36 +++--- backend/tests/unit/utils/validation.test.js | 2 +- frontend/Layout.tsx | 6 +- frontend/components/Inbox/InboxModal.tsx | 6 +- .../components/Project/ProjectDetails.tsx | 2 +- frontend/components/Tag/TagInput.tsx | 2 +- frontend/components/Tag/TagModal.tsx | 8 +- frontend/components/Tags.tsx | 26 +++-- frontend/components/Task/TaskDetails.tsx | 1 + frontend/components/Task/TaskTags.tsx | 6 +- frontend/utils/tagsService.ts | 8 +- 17 files changed, 97 insertions(+), 133 deletions(-) rename backend/{utils/validation.js => services/tagsService.js} (100%) diff --git a/backend/routes/notes.js b/backend/routes/notes.js index 3afa843..f6b7fd3 100644 --- a/backend/routes/notes.js +++ b/backend/routes/notes.js @@ -2,7 +2,7 @@ const express = require('express'); const { Note, Tag, Project, sequelize } = require('../models'); const { Op } = require('sequelize'); const { extractUidFromSlug } = require('../utils/slug-utils'); -const { validateTagName } = require('../utils/validation'); +const { validateTagName } = require('../services/tagsService'); const router = express.Router(); // Helper function to update note tags diff --git a/backend/routes/projects.js b/backend/routes/projects.js index d43fbab..e99cd05 100644 --- a/backend/routes/projects.js +++ b/backend/routes/projects.js @@ -7,7 +7,7 @@ const fs = require('fs'); const { Project, Task, Tag, Area, Note, sequelize } = require('../models'); const { Op } = require('sequelize'); const { extractUidFromSlug } = require('../utils/slug-utils'); -const { validateTagName } = require('../utils/validation'); +const { validateTagName } = require('../services/tagsService'); const { uid } = require('../utils/uid'); const router = express.Router(); diff --git a/backend/routes/tags.js b/backend/routes/tags.js index ed17754..778b0e0 100644 --- a/backend/routes/tags.js +++ b/backend/routes/tags.js @@ -1,16 +1,17 @@ const express = require('express'); const { Tag, Task, Note, Project, sequelize } = require('../models'); const { extractUidFromSlug } = require('../utils/slug-utils'); -const { validateTagName } = require('../utils/validation'); +const { validateTagName } = require('../services/tagsService'); const router = express.Router(); const _ = require('lodash'); +const { Op } = require('sequelize'); // GET /api/tags router.get('/tags', async (req, res) => { try { const tags = await Tag.findAll({ where: { user_id: req.currentUser.id }, - attributes: ['id', 'name', 'uid'], + attributes: ['name', 'uid'], order: [['name', 'ASC']], }); res.json(tags); @@ -20,17 +21,14 @@ router.get('/tags', async (req, res) => { } }); -// GET /api/tag/:identifier (supports both ID, name, and uid-slug) +// GET /api/tag/:identifier (supports name and uid) router.get('/tag', async (req, res) => { try { - const { id, uid, name } = req.query; + const { uid, name } = req.query; let whereClause = { user_id: req.currentUser.id, }; - if (!_.isEmpty(id)) { - whereClause.id = parseInt(id, 10); - } if (!_.isEmpty(uid)) { whereClause.uid = uid; } @@ -43,7 +41,7 @@ router.get('/tag', async (req, res) => { attributes: ['name', 'uid'], }); - if (!tag) { + if (_.isEmpty(tag)) { return res.status(404).json({ error: 'Tag not found' }); } @@ -70,8 +68,7 @@ router.post('/tag', async (req, res) => { }); res.status(201).json({ - id: tag.id, - uid: tag.uid, // Explicitly include uid + uid: tag.uid, name: tag.name, }); } catch (error) { @@ -85,21 +82,13 @@ router.post('/tag', async (req, res) => { // PATCH /api/tag/:identifier (supports both ID and name) router.patch('/tag/:identifier', async (req, res) => { try { - const identifier = req.params.identifier; - let whereClause; - - // Check if identifier is a number (ID) or string (name) - if (/^\d+$/.test(identifier)) { - // It's a numeric ID - whereClause = { - id: parseInt(identifier), - user_id: req.currentUser.id, - }; - } else { - // It's a tag name - decode URI component to handle special characters - const tagName = decodeURIComponent(identifier); - whereClause = { name: tagName, user_id: req.currentUser.id }; - } + const param = decodeURIComponent(req.params.identifier); + let whereClause = { + [Op.or]: [ + { name: param, user_id: req.currentUser.id }, + { uid: param, user_id: req.currentUser.id }, + ], + }; const tag = await Tag.findOne({ where: whereClause, @@ -110,7 +99,6 @@ router.patch('/tag/:identifier', async (req, res) => { } const { name } = req.body; - const validation = validateTagName(name); if (!validation.valid) { return res.status(400).json({ error: validation.error }); @@ -130,79 +118,51 @@ router.patch('/tag/:identifier', async (req, res) => { } }); -// DELETE /api/tag/:identifier (supports both ID and name) +// DELETE /api/tag/:identifier (supports uid and name) router.delete('/tag/:identifier', async (req, res) => { const transaction = await sequelize.transaction(); try { - const identifier = req.params.identifier; - let whereClause; - - // Check if identifier is a number (ID) or string (name) - if (/^\d+$/.test(identifier)) { - // It's a numeric ID - whereClause = { - id: parseInt(identifier), - user_id: req.currentUser.id, - }; - } else { - // It's a tag name - decode URI component to handle special characters - const tagName = decodeURIComponent(identifier); - whereClause = { name: tagName, user_id: req.currentUser.id }; - } + const param = decodeURIComponent(req.params.identifier); + let whereClause = { + [Op.or]: [ + { name: param, user_id: req.currentUser.id }, + { uid: param, user_id: req.currentUser.id }, + ], + }; const tag = await Tag.findOne({ where: whereClause, }); - if (!tag) { + if (_.isEmpty(tag)) { await transaction.rollback(); return res.status(404).json({ error: 'Tag not found' }); } - // Use transaction to ensure all deletions happen atomically // Remove all associations before deleting the tag by manually deleting from junction tables // Only delete from tables that exist - try { - await sequelize.query('DELETE FROM tasks_tags WHERE tag_id = ?', { + await Promise.all([ + sequelize.query('DELETE FROM tasks_tags WHERE tag_id = ?', { replacements: [tag.id], type: sequelize.QueryTypes.DELETE, transaction, - }); - } catch (error) { - // Ignore if table doesn't exist - console.log('tasks_tags table not found, skipping'); - } - - try { - await sequelize.query('DELETE FROM notes_tags WHERE tag_id = ?', { + }), + sequelize.query('DELETE FROM notes_tags WHERE tag_id = ?', { replacements: [tag.id], type: sequelize.QueryTypes.DELETE, transaction, - }); - } catch (error) { - // Ignore if table doesn't exist - console.log('notes_tags table not found, skipping'); - } + }), + sequelize.query('DELETE FROM projects_tags WHERE tag_id = ?', { + replacements: [tag.id], + type: sequelize.QueryTypes.DELETE, + transaction, + }), + ]); - try { - await sequelize.query( - 'DELETE FROM projects_tags WHERE tag_id = ?', - { - replacements: [tag.id], - type: sequelize.QueryTypes.DELETE, - transaction, - } - ); - } catch (error) { - // Ignore if table doesn't exist - console.log('projects_tags table not found, skipping'); - } - - // Now safely delete the tag await tag.destroy({ transaction }); - await transaction.commit(); + res.json({ message: 'Tag successfully deleted' }); } catch (error) { await transaction.rollback(); diff --git a/backend/routes/tasks.js b/backend/routes/tasks.js index bc9c3c3..3afec13 100644 --- a/backend/routes/tasks.js +++ b/backend/routes/tasks.js @@ -18,7 +18,7 @@ const { logTaskUpdate, getTaskTodayMoveCount, } = require('../services/taskEventService'); -const { validateTagName } = require('../utils/validation'); +const { validateTagName } = require('../services/tagsService'); const { getSafeTimezone, getUpcomingRangeInUTC, diff --git a/backend/utils/validation.js b/backend/services/tagsService.js similarity index 100% rename from backend/utils/validation.js rename to backend/services/tagsService.js diff --git a/backend/services/taskEventService.js b/backend/services/taskEventService.js index 6a7ef39..77e16e5 100644 --- a/backend/services/taskEventService.js +++ b/backend/services/taskEventService.js @@ -1,11 +1,5 @@ const { TaskEvent } = require('../models'); -// Helper function to add default source to metadata -const addDefaultSource = (metadata) => ({ - source: 'web', - ...metadata, -}); - // Helper function to create value object const createValueObject = (fieldName, value) => value ? { [fieldName || 'value']: value } : null; @@ -31,7 +25,10 @@ const logEvent = async ({ metadata = {}, }) => { try { - const finalMetadata = addDefaultSource(metadata); + const finalMetadata = { + source: 'web', + ...metadata, + }; const event = await TaskEvent.create({ task_id: taskId, diff --git a/backend/tests/integration/tags.test.js b/backend/tests/integration/tags.test.js index b7d5174..593e070 100644 --- a/backend/tests/integration/tags.test.js +++ b/backend/tests/integration/tags.test.js @@ -29,7 +29,7 @@ describe('Tags Routes', () => { expect(response.status).toBe(201); expect(response.body.name).toBe(tagData.name); - expect(response.body.id).toBeDefined(); + expect(response.body.uid).toBeDefined(); }); it('should require authentication', async () => { @@ -61,7 +61,7 @@ describe('Tags Routes', () => { expect(response.status).toBe(201); expect(response.body.name).toBe('project:frontend'); - expect(response.body.id).toBeDefined(); + expect(response.body.uid).toBeDefined(); }); it('should allow hyphen (-) in tag names', async () => { @@ -73,7 +73,7 @@ describe('Tags Routes', () => { expect(response.status).toBe(201); expect(response.body.name).toBe('project-frontend'); - expect(response.body.id).toBeDefined(); + expect(response.body.uid).toBeDefined(); }); it('should reject tags with invalid characters', async () => { @@ -108,8 +108,8 @@ describe('Tags Routes', () => { expect(response.status).toBe(200); expect(response.body).toHaveLength(2); - expect(response.body.map((t) => t.id)).toContain(tag1.id); - expect(response.body.map((t) => t.id)).toContain(tag2.id); + expect(response.body.map((t) => t.uid)).toContain(tag1.uid); + expect(response.body.map((t) => t.uid)).toContain(tag2.uid); }); it('should order tags by name', async () => { @@ -138,15 +138,15 @@ describe('Tags Routes', () => { }); }); - it('should get tag by id', async () => { - const response = await agent.get(`/api/tag?id=${tag.id}`); + it('should get tag by uid', async () => { + const response = await agent.get(`/api/tag?uid=${tag.uid}`); expect(response.status).toBe(200); expect(response.body.name).toBe(tag.name); }); it('should return 404 for non-existent tag', async () => { - const response = await agent.get('/api/tag?id=999999'); + const response = await agent.get('/api/tag?uid=non-existent-uid'); expect(response.status).toBe(404); expect(response.body.error).toBe('Tag not found'); @@ -164,14 +164,14 @@ describe('Tags Routes', () => { user_id: otherUser.id, }); - const response = await agent.get(`/api/tag?id=${otherTag.id}`); + const response = await agent.get(`/api/tag?uid=${otherTag.uid}`); expect(response.status).toBe(404); expect(response.body.error).toBe('Tag not found'); }); it('should require authentication', async () => { - const response = await request(app).get(`/api/tag?id=${tag.id}`); + const response = await request(app).get(`/api/tag?uid=${tag.uid}`); expect(response.status).toBe(401); expect(response.body.error).toBe('Authentication required'); @@ -194,7 +194,7 @@ describe('Tags Routes', () => { }; const response = await agent - .patch(`/api/tag/${tag.id}`) + .patch(`/api/tag/${tag.uid}`) .send(updateData); expect(response.status).toBe(200); @@ -203,7 +203,7 @@ describe('Tags Routes', () => { it('should return 404 for non-existent tag', async () => { const response = await agent - .patch('/api/tag/999999') + .patch('/api/tag/non-existent-uid') .send({ name: 'Updated' }); expect(response.status).toBe(404); @@ -223,7 +223,7 @@ describe('Tags Routes', () => { }); const response = await agent - .patch(`/api/tag/${otherTag.id}`) + .patch(`/api/tag/${otherTag.uid}`) .send({ name: 'Updated' }); expect(response.status).toBe(404); @@ -232,7 +232,7 @@ describe('Tags Routes', () => { it('should require authentication', async () => { const response = await request(app) - .patch(`/api/tag/${tag.id}`) + .patch(`/api/tag/${tag.uid}`) .send({ name: 'Updated' }); expect(response.status).toBe(401); @@ -251,7 +251,7 @@ describe('Tags Routes', () => { }); it('should delete tag', async () => { - const response = await agent.delete(`/api/tag/${tag.id}`); + const response = await agent.delete(`/api/tag/${tag.uid}`); expect(response.status).toBe(200); expect(response.body.message).toBe('Tag successfully deleted'); @@ -262,7 +262,7 @@ describe('Tags Routes', () => { }); it('should return 404 for non-existent tag', async () => { - const response = await agent.delete('/api/tag/999999'); + const response = await agent.delete('/api/tag/non-existent-uid'); expect(response.status).toBe(404); expect(response.body.error).toBe('Tag not found'); @@ -280,14 +280,14 @@ describe('Tags Routes', () => { user_id: otherUser.id, }); - const response = await agent.delete(`/api/tag/${otherTag.id}`); + const response = await agent.delete(`/api/tag/${otherTag.uid}`); expect(response.status).toBe(404); expect(response.body.error).toBe('Tag not found'); }); it('should require authentication', async () => { - const response = await request(app).delete(`/api/tag/${tag.id}`); + const response = await request(app).delete(`/api/tag/${tag.uid}`); expect(response.status).toBe(401); expect(response.body.error).toBe('Authentication required'); diff --git a/backend/tests/unit/utils/validation.test.js b/backend/tests/unit/utils/validation.test.js index b3c4d78..f7bde24 100644 --- a/backend/tests/unit/utils/validation.test.js +++ b/backend/tests/unit/utils/validation.test.js @@ -1,4 +1,4 @@ -const { validateTagName } = require('../../../utils/validation'); +const { validateTagName } = require('../../../services/tagsService'); describe('validation utils', () => { describe('validateTagName', () => { diff --git a/frontend/Layout.tsx b/frontend/Layout.tsx index 7eea09a..f8d318b 100644 --- a/frontend/Layout.tsx +++ b/frontend/Layout.tsx @@ -288,15 +288,15 @@ const Layout: React.FC = ({ const handleSaveTag = async (tagData: Tag) => { try { let result: Tag; - if (tagData.id) { - result = await updateTag(tagData.id, tagData); + if (tagData.uid) { + result = await updateTag(tagData.uid, tagData); // Update existing tag in global store const currentTags = useStore.getState().tagsStore.tags; useStore .getState() .tagsStore.setTags( currentTags.map((tag) => - tag.id === result.id ? result : tag + tag.uid === result.uid ? result : tag ) ); } else { diff --git a/frontend/components/Inbox/InboxModal.tsx b/frontend/components/Inbox/InboxModal.tsx index 3e68283..6cc9bcc 100644 --- a/frontend/components/Inbox/InboxModal.tsx +++ b/frontend/components/Inbox/InboxModal.tsx @@ -1644,7 +1644,11 @@ const InboxModal: React.FC = ({ > {filteredTags.map((tag, index) => ( @@ -385,7 +385,7 @@ const Tags: React.FC = () => { className={`text-gray-500 hover:text-red-700 dark:hover:text-red-300 focus:outline-none transition-opacity ${hoveredTagId === tag.id ? 'opacity-100' : 'opacity-0'}`} aria-label={`Delete ${tag.name}`} title={`Delete ${tag.name}`} - data-testid={`tag-delete-${tag.id}`} + data-testid={`tag-delete-${tag.uid || tag.id}`} > @@ -409,14 +409,16 @@ const Tags: React.FC = () => { setSelectedTag(null); }} onSave={handleSaveTag} - onDelete={async (tagId) => { + onDelete={async (tagUid) => { try { - await apiDeleteTag(tagId); - setTags(tags.filter((tag) => tag.id !== tagId)); + await apiDeleteTag(tagUid); + setTags( + tags.filter((tag) => tag.uid !== tagUid) + ); setTagMetrics((prev) => { const newMetrics = { ...prev }; const deletedTag = tags.find( - (t) => t.id === tagId + (t) => t.uid === tagUid ); if (deletedTag) { delete newMetrics[deletedTag.name]; diff --git a/frontend/components/Task/TaskDetails.tsx b/frontend/components/Task/TaskDetails.tsx index eab5e61..5e947ac 100644 --- a/frontend/components/Task/TaskDetails.tsx +++ b/frontend/components/Task/TaskDetails.tsx @@ -484,6 +484,7 @@ const TaskDetails: React.FC = () => { ) => ( void; + onTagRemove?: (tagUid: string | undefined) => void; className?: string; } @@ -38,7 +38,7 @@ const TaskTags: React.FC = ({
{tags.map((tag, index) => (