[O2B-1620] Tags should associate user by id and not direct name

docs/data-model.md

@@ -184,7 +184,7 @@ Concerning the **Update mode** of the fields:
 | `text`             | Tag name.                                                      | `TPC`, `COSMICS`, `RC`                        |                                 | `id`           | Insert          |
 | `Mattermost`       | Mattermost channels                                            | `Food`, `Bookkeeping updates`                 |                                 | `id`           | Update          |
 | `email`            | Email groups                                                   | `food@cern.ch`, `Bookkeeping-updates@cern.ch` |                                 | `id`           | Update          |
-| `last_edited_name` | Name of the person who last edited the email/mattermost fields | `Anonymous`, `Jan Janssen`                    | When email/mattermost is edited | `id`           | Update          |
+| `last_edited_by_user_id` | Id (in `users` table) of the user who last edited the tag | `1`, `2`                                      | When the tag is edited          | `id`           | Update          |
 
 ## Environments
 

lib/database/adapters/TagAdapter.js

@@ -21,6 +21,7 @@ class TagAdapter {
     constructor() {
         this.toEntity = this.toEntity.bind(this);
         this.toDatabase = this.toDatabase.bind(this);
+        this.userAdapter = null;
     }
 
     /**
@@ -29,12 +30,12 @@ class TagAdapter {
      * @param {SequelizeTag} databaseObject Object to convert.
      * @returns {Tag} Converted entity object.
      */
-    toEntity({ id, text, description, email, mattermost, last_edited_name, archived, color, archivedAt, updatedAt }) {
+    toEntity({ id, text, description, email, mattermost, lastEditedBy, archived, color, archivedAt, updatedAt }) {
         return {
             id,
             text,
             description,
-            lastEditedName: last_edited_name,
+            lastEditedBy: lastEditedBy ? this.userAdapter.toNameOnly(lastEditedBy) : null,
             email,
             mattermost,
             archived,

lib/database/adapters/index.js

@@ -139,6 +139,8 @@ runAdapter.logAdapter = logAdapter;
 runAdapter.runTypeAdapter = runTypeAdapter;
 runAdapter.tagAdapter = tagAdapter;
 runAdapter.userAdapter = userAdapter;
+
+tagAdapter.userAdapter = userAdapter;
 runAdapter.qcFlagAdapter = qcFlagAdapter;
 
 simulationPassQcFlagAdapter.simulationPassAdapter = simulationPassAdapter;

lib/database/migrations/v1/20260930100000-tags-last-edited-by-user-id.js

@@ -0,0 +1,56 @@
+/*
+ *  @license
+ *  Copyright CERN and copyright holders of ALICE O2. This software is
+ *  distributed under the terms of the GNU General Public License v3 (GPL
+ *  Version 3), copied verbatim in the file "COPYING".
+ *
+ *  See http://alice-o2.web.cern.ch/license for full licensing information.
+ *
+ *  In applying this license CERN does not waive the privileges and immunities
+ *  granted to it by virtue of its status as an Intergovernmental Organization
+ *  or submit itself to any jurisdiction.
+ */
+
+'use strict';
+
+/** @type {import('sequelize-cli').Migration} */
+module.exports = {
+    up: async (queryInterface, Sequelize) => queryInterface.sequelize.transaction(async (transaction) => {
+        await queryInterface.addColumn('tags', 'last_edited_by_user_id', {
+            type: Sequelize.INTEGER,
+            allowNull: true,
+            references: {
+                model: 'users',
+                key: 'id',
+            },
+            onUpdate: 'CASCADE',
+            onDelete: 'SET NULL',
+        }, { transaction });
+
+        // Link existing tags to the user matching the stored name (ambiguous names resolve to the oldest user)
+        await queryInterface.sequelize.query(
+            `UPDATE tags t
+            SET t.last_edited_by_user_id = (SELECT MIN(u.id) FROM users u WHERE u.name = t.last_edited_name)
+            WHERE t.last_edited_name IS NOT NULL`,
+            { transaction },
+        );
+
+        await queryInterface.removeColumn('tags', 'last_edited_name', { transaction });
+    }),
+
+    down: async (queryInterface, Sequelize) => queryInterface.sequelize.transaction(async (transaction) => {
+        await queryInterface.addColumn('tags', 'last_edited_name', {
+            type: Sequelize.STRING,
+            allowNull: true,
+        }, { transaction });
+
+        await queryInterface.sequelize.query(
+            `UPDATE tags t
+            INNER JOIN users u ON u.id = t.last_edited_by_user_id
+            SET t.last_edited_name = u.name`,
+            { transaction },
+        );
+
+        await queryInterface.removeColumn('tags', 'last_edited_by_user_id', { transaction });
+    }),
+};

lib/database/models/tag.js

@@ -31,8 +31,8 @@ module.exports = (sequelize) => {
             type: Sequelize.STRING,
             allowNull: true,
         },
-        last_edited_name: {
-            type: Sequelize.STRING,
+        lastEditedByUserId: {
+            type: Sequelize.INTEGER,
             allowNull: true,
         },
         archivedAt: {
@@ -66,6 +66,7 @@ module.exports = (sequelize) => {
     Tag.associate = (models) => {
         Tag.belongsToMany(models.Log, { through: 'log_tags' });
         Tag.belongsToMany(models.Run, { through: 'run_tags', as: 'runs' });
+        Tag.belongsTo(models.User, { as: 'lastEditedBy', foreignKey: 'lastEditedByUserId' });
     };
 
     return Tag;

lib/database/models/typedefs/SequelizeTag.js

@@ -19,7 +19,7 @@
  * @property {string} description
  * @property {string|null} email
  * @property {string|null} mattermost
- * @property {string|null} last_edited_name
+ * @property {SequelizeUser|null} [lastEditedBy]
  * @property {string|null} archivedAt
  * @property {string} createdAt
  * @property {string} updatedAt

lib/domain/entities/Tag.js

@@ -18,7 +18,7 @@
  * @property {string} description
  * @property {string|null} email
  * @property {string|null} mattermost
- * @property {string|null} lastEditedName
+ * @property {{name: string}|null} lastEditedBy the name of the user who last edited the tag
  * @property {boolean} archived
  * @property {number|null} archivedAt
  * @property {number} createdAt

lib/public/components/tag/tagDetail.js

@@ -78,11 +78,11 @@ const activeFields = (detailsModel) => ({
         size: 'cell-m',
         format: (timestamp) => formatTimestamp(timestamp),
     },
-    lastEditedName: {
+    lastEditedBy: {
         name: 'Last modified by',
         visible: true,
         size: 'cell-m',
-        format: (name) => name ? name : '-',
+        format: (user) => user?.name || '-',
     },
     archived: {
         name: 'Archived',

lib/public/views/Tags/ActiveColumns/tagsActiveColumns.js

@@ -36,11 +36,11 @@ export const tagsActiveColumns = {
             placeholder: 'Filter by name',
         },
     },
-    lastEditedName: {
+    lastEditedBy: {
         name: 'Last Edited by',
         visible: true,
         classes: 'w-10 f6',
-        format: (name) => name || '-',
+        format: (user) => user?.name || '-',
     },
     updatedAt: {
         name: 'Updated at',

lib/server/controllers/tags.controller.js

@@ -24,6 +24,7 @@ const {
 } = require('../../usecases');
 const { dtos: { CreateTagDto, GetAllTagsDto, GetTagDto, UpdateTagDto, GetTagByNameDto } } = require('../../domain');
 const { dtoValidator } = require('../utilities');
+const { updateExpressResponseFromNativeError } = require('../express/updateExpressResponseFromNativeError.js');
 const { ApiConfig } = require('../../config/index.js');
 const GetAllLogsByTagDto = require('../../domain/dtos/GetAllLogsByTagDto.js');
 
@@ -42,23 +43,11 @@ const createTag = async (request, response) => {
         return;
     }
 
-    const tag = await new CreateTagUseCase().execute(value);
-
-    if (tag) {
-        response.status(201).json({
-            data: tag,
-        });
-    } else {
-        response.status(409).json({
-            errors: [
-                {
-                    status: '409',
-                    source: { pointer: '/data/attributes/body/text' },
-                    title: 'Conflict',
-                    detail: 'The provided entity already exists',
-                },
-            ],
-        });
+    try {
+        const tag = await new CreateTagUseCase().execute(value);
+        response.status(201).json({ data: tag });
+    } catch (error) {
+        updateExpressResponseFromNativeError(response, error);
     }
 };
 
@@ -289,13 +278,11 @@ const updateTagById = async (request, response) => {
         return;
     }
 
-    const { result, error } = await new UpdateTagUseCase()
-        .execute(value);
-
-    if (error) {
-        response.status(Number(error.status)).json({ errors: [error] });
-    } else {
-        response.status(201).json({ data: result });
+    try {
+        const tag = await new UpdateTagUseCase().execute(value);
+        response.status(201).json({ data: tag });
+    } catch (error) {
+        updateExpressResponseFromNativeError(response, error);
     }
 };
 

lib/usecases/tag/CreateTagUseCase.js

@@ -21,6 +21,9 @@ const {
     },
 } = require('../../database');
 const { tagAdapter } = require('../../database/adapters/index.js');
+const { BadParameterError } = require('../../server/errors/BadParameterError.js');
+const { ConflictError } = require('../../server/errors/ConflictError.js');
+const { getUserOrFail } = require('../../server/services/user/getUserOrFail.js');
 
 /**
  * CreateTagUseCase
@@ -30,23 +33,31 @@ class CreateTagUseCase {
      * Executes this use case.
      *
      * @param {Object} dto The CreateTagDto containing all data.
-     * @returns {Promise} Promise object represents the result of this use case.
+     * @returns {Promise<Tag>} resolves with the created tag
+     * @throws {BadParameterError} if no user is provided in the session
+     * @throws {NotFoundError} if the session user does not exist
+     * @throws {ConflictError} if a tag with the same text already exists
      */
     async execute(dto) {
         const { body } = dto;
-        body.last_edited_name = dto?.session?.name;
+        const userId = dto?.session?.id;
+        if (userId === undefined || userId === null) {
+            throw new BadParameterError('A user is required to create a tag');
+        }
+
         const tag = await TransactionHelper.provide(async () => {
-            const queryBuilder = new QueryBuilder()
-                .where('text').is(body.text);
-            const tag = await TagRepository.findOne(queryBuilder);
-            if (tag) {
-                return null;
+            const user = await getUserOrFail({ userId });
+            body.lastEditedByUserId = user.id;
+
+            const existingTag = await TagRepository.findOne(new QueryBuilder().where('text').is(body.text));
+            if (existingTag) {
+                throw new ConflictError('The provided entity already exists');
             }
 
             return TagRepository.insert(tagAdapter.toDatabase(body));
         });
 
-        return tag ? tagAdapter.toEntity(tag) : null;
+        return tagAdapter.toEntity(tag);
     }
 }
 

lib/usecases/tag/GetAllTagsUseCase.js

@@ -33,7 +33,7 @@ class GetAllTagsUseCase {
      * @returns {Promise} Promise object represents the result of this use case.
      */
     async execute(dto = {}) {
-        const queryBuilder = new QueryBuilder();
+        const queryBuilder = new QueryBuilder().include({ association: 'lastEditedBy', attributes: ['name'] });
         const { query = {} } = dto;
         const { filter = {} } = query;
 

lib/usecases/tag/GetTagByNameUseCase.js

@@ -38,7 +38,8 @@ class GetTagByNameUseCase {
         name = decodeURIComponent(name);
         const tag = await TransactionHelper.provide(async () => {
             const queryBuilder = new QueryBuilder()
-                .where('text').is(name);
+                .where('text').is(name)
+                .include({ association: 'lastEditedBy', attributes: ['name'] });
 
             return TagRepository.findOne(queryBuilder);
         });

lib/usecases/tag/GetTagUseCase.js

@@ -38,7 +38,8 @@ class GetTagUseCase {
 
         const tag = await TransactionHelper.provide(async () => {
             const queryBuilder = new QueryBuilder()
-                .where('id').is(tagId);
+                .where('id').is(tagId)
+                .include({ association: 'lastEditedBy', attributes: ['name'] });
 
             return TagRepository.findOne(queryBuilder);
         });

lib/usecases/tag/UpdateTagUseCase.js

@@ -21,47 +21,49 @@ const {
     },
 } = require('../../database');
 const GetTagUseCase = require('./GetTagUseCase');
+const { BadParameterError } = require('../../server/errors/BadParameterError.js');
+const { NotFoundError } = require('../../server/errors/NotFoundError.js');
+const { getUserOrFail } = require('../../server/services/user/getUserOrFail.js');
 
 /**
  * Update tag use case
  */
 class UpdateTagUseCase {
     /**
      * Executes this use case
+     *
      * @param {Object} dto The UpdateTagDto containing the values that needs to be updated.
-     * @returns {Promise} Promise object that represents the result of the update.
+     * @returns {Promise<Tag>} resolves with the updated tag
+     * @throws {BadParameterError} if no user is provided in the session
+     * @throws {NotFoundError} if the tag or the session user does not exist
      */
     async execute(dto) {
         const { body, params } = dto;
         const { tagId } = params;
         const { description, email, mattermost, archivedAt, color } = body;
-        const tag = await TransactionHelper.provide(async () => {
-            const username = dto?.session?.name;
-            const queryBuilder = new QueryBuilder().where('id').is(tagId);
-            const tagObject = await TagRepository.findOne(queryBuilder);
-            if (tagObject && username) {
-                tagObject.description = description;
-                tagObject.email = email;
-                tagObject.color = color;
-                tagObject.mattermost = mattermost;
-                tagObject.archivedAt = archivedAt;
-                tagObject.last_edited_name = username;
-                await tagObject.save();
-                return tagObject;
-            } else {
-                return {
-                    error: {
-                        status: '400',
-                        title: `this tag with this tag id: (${tagId}) could not be found.`,
-                    },
-                };
+        const userId = dto?.session?.id;
+        if (userId === undefined || userId === null) {
+            throw new BadParameterError('A user is required to update a tag');
+        }
+
+        await TransactionHelper.provide(async () => {
+            const tag = await TagRepository.findOne(new QueryBuilder().where('id').is(tagId));
+            if (!tag) {
+                throw new NotFoundError(`Tag with this id (${tagId}) could not be found`);
             }
+
+            const user = await getUserOrFail({ userId });
+
+            tag.description = description;
+            tag.email = email;
+            tag.color = color;
+            tag.mattermost = mattermost;
+            tag.archivedAt = archivedAt;
+            tag.lastEditedByUserId = user.id;
+            await tag.save();
         });
-        if (tag.error) {
-            return tag;
-        }
-        const result = await new GetTagUseCase().execute({ params: { tagId } });
-        return { result };
+
+        return new GetTagUseCase().execute({ params: { tagId } });
     }
 }
 

proto/log.proto

@@ -87,8 +87,8 @@ message Tag {
   string mattermost = 5;
   // Unix timestamp when this entity was last updated.
   int64 updatedAt = 6;
-  // The last person that edited the email/mattermost fields
-  string lastEditedName = 7;
+  reserved 7;
+  reserved "lastEditedName";
   // The description of the tag
   optional string description = 8;
 }

test/api/logs.test.js

@@ -1224,7 +1224,7 @@ module.exports = () => {
             expect(response.body.data.text).to.equal('Text of yet another run');
             for (const tag of response.body.data.tags) {
                 delete tag.updatedAt;
-                delete tag.lastEditedName;
+                delete tag.lastEditedBy;
             }
             expect(response.body.data.tags).to.deep.equal([
                 {