From de741a915c6499d7b88595c7d160094ef71d5a7c Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 1 Oct 2026 09:17:07 +0200 Subject: [PATCH 1/8] Update model and DB with new attribute for userid --- docs/data-model.md | 2 +- lib/database/adapters/TagAdapter.js | 5 +- lib/database/adapters/index.js | 2 + ...60930100000-tags-last-edited-by-user-id.js | 56 +++++++++++++++++++ lib/database/models/tag.js | 5 +- lib/database/models/typedefs/SequelizeTag.js | 2 +- lib/domain/entities/Tag.js | 2 +- 7 files changed, 67 insertions(+), 7 deletions(-) create mode 100644 lib/database/migrations/v1/20260930100000-tags-last-edited-by-user-id.js diff --git a/docs/data-model.md b/docs/data-model.md index 8f7b38fc21..3e5278a837 100644 --- a/docs/data-model.md +++ b/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 diff --git a/lib/database/adapters/TagAdapter.js b/lib/database/adapters/TagAdapter.js index 5f44a81e26..208bed20ca 100644 --- a/lib/database/adapters/TagAdapter.js +++ b/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.toEntity(lastEditedBy) : null, email, mattermost, archived, diff --git a/lib/database/adapters/index.js b/lib/database/adapters/index.js index 5ff6404b3d..fc9a81a696 100644 --- a/lib/database/adapters/index.js +++ b/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; diff --git a/lib/database/migrations/v1/20260930100000-tags-last-edited-by-user-id.js b/lib/database/migrations/v1/20260930100000-tags-last-edited-by-user-id.js new file mode 100644 index 0000000000..2f0e28b514 --- /dev/null +++ b/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 }); + }), +}; diff --git a/lib/database/models/tag.js b/lib/database/models/tag.js index 7bacf4a9b4..b6165c7a4f 100644 --- a/lib/database/models/tag.js +++ b/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; diff --git a/lib/database/models/typedefs/SequelizeTag.js b/lib/database/models/typedefs/SequelizeTag.js index 6257015d9c..35477c301a 100644 --- a/lib/database/models/typedefs/SequelizeTag.js +++ b/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 diff --git a/lib/domain/entities/Tag.js b/lib/domain/entities/Tag.js index f19448a94c..9bb36bec15 100644 --- a/lib/domain/entities/Tag.js +++ b/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 {User|null} lastEditedBy * @property {boolean} archived * @property {number|null} archivedAt * @property {number} createdAt From 0abebc078fd6c0d12b0c946d5c4bdbc3544cb296 Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 1 Oct 2026 09:20:10 +0200 Subject: [PATCH 2/8] Update usecases to fetch with userId --- lib/usecases/tag/GetAllTagsUseCase.js | 2 +- lib/usecases/tag/GetTagByNameUseCase.js | 3 ++- lib/usecases/tag/GetTagUseCase.js | 3 ++- .../usecases/tag/GetAllTagsUseCase.test.js | 15 +++++++++++++ test/lib/usecases/tag/GetTagUseCase.test.js | 21 +++++++++++++++++++ 5 files changed, 41 insertions(+), 3 deletions(-) diff --git a/lib/usecases/tag/GetAllTagsUseCase.js b/lib/usecases/tag/GetAllTagsUseCase.js index c23fe653da..580262f9f4 100644 --- a/lib/usecases/tag/GetAllTagsUseCase.js +++ b/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' }); const { query = {} } = dto; const { filter = {} } = query; diff --git a/lib/usecases/tag/GetTagByNameUseCase.js b/lib/usecases/tag/GetTagByNameUseCase.js index 029fc40d3e..1587fb0160 100644 --- a/lib/usecases/tag/GetTagByNameUseCase.js +++ b/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' }); return TagRepository.findOne(queryBuilder); }); diff --git a/lib/usecases/tag/GetTagUseCase.js b/lib/usecases/tag/GetTagUseCase.js index 181c5f70c0..e1bb5da689 100644 --- a/lib/usecases/tag/GetTagUseCase.js +++ b/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' }); return TagRepository.findOne(queryBuilder); }); diff --git a/test/lib/usecases/tag/GetAllTagsUseCase.test.js b/test/lib/usecases/tag/GetAllTagsUseCase.test.js index 4f58839cfe..ad0ecbed17 100644 --- a/test/lib/usecases/tag/GetAllTagsUseCase.test.js +++ b/test/lib/usecases/tag/GetAllTagsUseCase.test.js @@ -28,6 +28,21 @@ module.exports = () => { expect(tags).to.be.an('array'); }); + + it('should return the user who last edited each tag', async () => { + const { tags } = await new GetAllTagsUseCase().execute(); + + for (const tag of tags) { + expect(tag).to.have.property('lastEditedByUserId'); + expect(tag).to.have.property('lastEditedBy'); + expect(tag).to.not.have.property('lastEditedName'); + if (tag.lastEditedByUserId === null) { + expect(tag.lastEditedBy).to.be.null; + } else { + expect(tag.lastEditedBy.id).to.equal(tag.lastEditedByUserId); + } + } + }); it('should return tags sorted by text', async () => { const { tags } = await new GetAllTagsUseCase().execute(); diff --git a/test/lib/usecases/tag/GetTagUseCase.test.js b/test/lib/usecases/tag/GetTagUseCase.test.js index 4395c47afb..a876946b90 100644 --- a/test/lib/usecases/tag/GetTagUseCase.test.js +++ b/test/lib/usecases/tag/GetTagUseCase.test.js @@ -12,6 +12,7 @@ */ const { tag: { GetTagUseCase } } = require('../../../../lib/usecases/index.js'); +const { repositories: { TagRepository }, utilities: { QueryBuilder } } = require('../../../../lib/database/index.js'); const { dtos: { GetTagDto } } = require('../../../../lib/domain/index.js'); const chai = require('chai'); @@ -35,4 +36,24 @@ module.exports = () => { expect(result).to.have.ownProperty('id'); expect(result.id).to.equal(1); }); + + it('should return the user who last edited the tag', async () => { + const createdTag = await TagRepository.insert({ text: `TAG-LAST-EDITED-${Date.now()}`, lastEditedByUserId: 2 }); + const result = await new GetTagUseCase().execute({ params: { tagId: createdTag.id } }); + + expect(result.lastEditedByUserId).to.equal(2); + expect(result.lastEditedBy).to.deep.equal({ id: 2, externalId: 456, name: 'Jan Jansen' }); + + await TagRepository.removeAll(new QueryBuilder().where('id').is(createdTag.id)); + }); + + it('should return null last editor for a tag that was never edited', async () => { + const createdTag = await TagRepository.insert({ text: `TAG-NEVER-EDITED-${Date.now()}` }); + const result = await new GetTagUseCase().execute({ params: { tagId: createdTag.id } }); + + expect(result.lastEditedByUserId).to.be.null; + expect(result.lastEditedBy).to.be.null; + + await TagRepository.removeAll(new QueryBuilder().where('id').is(createdTag.id)); + }); }; From 789c809989ddf4ee563f5345c984e08a4a7a635e Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 1 Oct 2026 09:39:11 +0200 Subject: [PATCH 3/8] Improve integrity of created tags --- lib/server/controllers/tags.controller.js | 23 +++------- lib/usecases/tag/CreateTagUseCase.js | 27 +++++++---- lib/usecases/tag/GetTagUseCase.js | 2 +- .../lib/usecases/tag/CreateTagUseCase.test.js | 45 ++++++++++++++++--- test/public/tags/create.test.js | 2 +- 5 files changed, 67 insertions(+), 32 deletions(-) diff --git a/lib/server/controllers/tags.controller.js b/lib/server/controllers/tags.controller.js index 76c480e629..2a915e9e9c 100644 --- a/lib/server/controllers/tags.controller.js +++ b/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); } }; diff --git a/lib/usecases/tag/CreateTagUseCase.js b/lib/usecases/tag/CreateTagUseCase.js index 7d2ddb4f4f..aadc1d4529 100644 --- a/lib/usecases/tag/CreateTagUseCase.js +++ b/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} 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); } } diff --git a/lib/usecases/tag/GetTagUseCase.js b/lib/usecases/tag/GetTagUseCase.js index e1bb5da689..e76d1fc689 100644 --- a/lib/usecases/tag/GetTagUseCase.js +++ b/lib/usecases/tag/GetTagUseCase.js @@ -38,7 +38,7 @@ class GetTagUseCase { const tag = await TransactionHelper.provide(async () => { const queryBuilder = new QueryBuilder() - .where('id').is(tagId) + .where('id').is(tagId) .include({ association: 'lastEditedBy' }); return TagRepository.findOne(queryBuilder); diff --git a/test/lib/usecases/tag/CreateTagUseCase.test.js b/test/lib/usecases/tag/CreateTagUseCase.test.js index 6a5b39bfc1..47c0f91f9e 100644 --- a/test/lib/usecases/tag/CreateTagUseCase.test.js +++ b/test/lib/usecases/tag/CreateTagUseCase.test.js @@ -15,6 +15,10 @@ const { repositories: { TagRepository } } = require('../../../../lib/database/in const { tag: { CreateTagUseCase } } = require('../../../../lib/usecases/index.js'); const { dtos: { CreateTagDto } } = require('../../../../lib/domain/index.js'); const chai = require('chai'); +const assert = require('assert'); +const { BadParameterError } = require('../../../../lib/server/errors/BadParameterError.js'); +const { NotFoundError } = require('../../../../lib/server/errors/NotFoundError.js'); +const { ConflictError } = require('../../../../lib/server/errors/ConflictError.js'); const { expect } = chai; @@ -27,6 +31,7 @@ module.exports = () => { text: `TAG#${new Date().getTime()}`, }, }); + createTagDto.session = { id: 1, externalId: 1, name: 'John Doe' }; }); it('should insert a new Tag', async () => { @@ -49,7 +54,7 @@ module.exports = () => { expect(result.text).to.equal(expectedTitle); }); - it('should return null if we are trying to create the same tag again', async () => { + it('should reject with a ConflictError if we are trying to create the same tag again', async () => { const nTagsBefore = await TagRepository.count(); await new CreateTagUseCase() @@ -58,13 +63,43 @@ module.exports = () => { const nTagsAfter = await TagRepository.count(); expect(nTagsAfter).to.be.greaterThan(nTagsBefore); - const result = await new CreateTagUseCase() - .execute(createTagDto); - - expect(result).to.be.null; + await assert.rejects( + () => new CreateTagUseCase().execute(createTagDto), + new ConflictError('The provided entity already exists'), + ); expect(await TagRepository.count()).to.equal(nTagsAfter); }); + it('should store the id of the user creating the tag', async () => { + const tag = await new CreateTagUseCase().execute(createTagDto); + expect(tag).to.not.have.property('lastEditedName'); + + const storedTag = await TagRepository.findOne({ where: { id: tag.id } }); + expect(storedTag.lastEditedByUserId).to.equal(1); + }); + + it('should reject the creation if no user is provided', async () => { + delete createTagDto.session; + const nTagsBefore = await TagRepository.count(); + + await assert.rejects( + () => new CreateTagUseCase().execute(createTagDto), + new BadParameterError('A user is required to create a tag'), + ); + expect(await TagRepository.count()).to.equal(nTagsBefore); + }); + + it('should reject the creation if the user does not exist', async () => { + createTagDto.session = { id: 9999, externalId: 9999, name: 'Ghost' }; + const nTagsBefore = await TagRepository.count(); + + await assert.rejects( + () => new CreateTagUseCase().execute(createTagDto), + new NotFoundError('User with this id (9999) could not be found'), + ); + expect(await TagRepository.count()).to.equal(nTagsBefore); + }); + it('should successfully create a new tag with a description', async () => { createTagDto.body.description = 'A description'; const tag = await new CreateTagUseCase().execute(createTagDto); diff --git a/test/public/tags/create.test.js b/test/public/tags/create.test.js index b7a573e136..c29ac477f8 100644 --- a/test/public/tags/create.test.js +++ b/test/public/tags/create.test.js @@ -70,7 +70,7 @@ module.exports = () => { await pressElement(page, 'button#submit'); // Because this tag already exists, we expect an error message to appear - await expectInnerText(page, '.alert', 'Conflict: The provided entity already exists'); + await expectInnerText(page, '.alert', 'The request conflicts with existing data: The provided entity already exists'); }); it('Should show no fields when having no admin roles', async () => { From 98641f9d32a7924609bcd5b349c1599b0847fc88 Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 1 Oct 2026 15:31:33 +0200 Subject: [PATCH 4/8] Update backend of tags usecase to return user association --- lib/server/controllers/tags.controller.js | 12 ++-- lib/usecases/tag/UpdateTagUseCase.js | 54 ++++++++--------- proto/log.proto | 4 +- test/api/logs.test.js | 2 +- test/api/tags.test.js | 23 ++++++++ .../lib/usecases/tag/DeleteTagUseCase.test.js | 1 + .../usecases/tag/GetAllTagsUseCase.test.js | 8 +-- test/lib/usecases/tag/GetTagUseCase.test.js | 2 - .../lib/usecases/tag/UpdateTagUseCase.test.js | 58 ++++++++++++------- 9 files changed, 101 insertions(+), 63 deletions(-) diff --git a/lib/server/controllers/tags.controller.js b/lib/server/controllers/tags.controller.js index 2a915e9e9c..553f2db9d5 100644 --- a/lib/server/controllers/tags.controller.js +++ b/lib/server/controllers/tags.controller.js @@ -278,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); } }; diff --git a/lib/usecases/tag/UpdateTagUseCase.js b/lib/usecases/tag/UpdateTagUseCase.js index 4b88e98c36..b1017674f5 100644 --- a/lib/usecases/tag/UpdateTagUseCase.js +++ b/lib/usecases/tag/UpdateTagUseCase.js @@ -21,6 +21,9 @@ 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 @@ -28,40 +31,39 @@ const GetTagUseCase = require('./GetTagUseCase'); 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} 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 } }); } } diff --git a/proto/log.proto b/proto/log.proto index e035ff91ce..c97cf4d447 100644 --- a/proto/log.proto +++ b/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; } diff --git a/test/api/logs.test.js b/test/api/logs.test.js index 9d2f774ad3..a8e9fde03f 100644 --- a/test/api/logs.test.js +++ b/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([ { diff --git a/test/api/tags.test.js b/test/api/tags.test.js index 096a5a3606..68fc4e41a7 100644 --- a/test/api/tags.test.js +++ b/test/api/tags.test.js @@ -312,6 +312,7 @@ module.exports = () => { }, }); + createTagDto.session = { id: 1, externalId: 1, name: 'John Doe' }; createdTag = await new CreateTagUseCase() .execute(createTagDto); }); @@ -543,6 +544,7 @@ module.exports = () => { text: `TAG#${new Date().getTime()}`, }, }); + createTagDto.session = { id: 1, externalId: 1, name: 'John Doe' }; createdTag = await new CreateTagUseCase() .execute(createTagDto); }); @@ -580,6 +582,18 @@ module.exports = () => { done(); }); }); + it('should store the user who edited the tag and return it', async () => { + const putResponse = await request(server) + .put(`/api/tags/${createdTag.id}?token=admin`) + .send({ email: 'groupa@cern.ch', mattermost: 'groupa' }); + expect(putResponse.status).to.equal(201); + + const getResponse = await request(server).get(`/api/tags/${createdTag.id}`); + expect(getResponse.status).to.equal(200); + const { data } = getResponse.body; + expect(data.lastEditedBy).to.deep.equal({ id: 1, externalId: 1, name: 'John Doe' }); + expect(data).to.not.have.property('lastEditedName'); + }); it('should return 400 if invalid email is given', (done) => { request(server) .put(`/api/tags/${createdTag.id}?token=admin`) @@ -632,6 +646,15 @@ module.exports = () => { done(); }); }); + it('should return 404 if the tag could not be found', async () => { + const response = await request(server) + .put('/api/tags/999999999?token=admin') + .send({ email: 'groupa@cern.ch', mattermost: 'groupa' }); + + expect(response.status).to.equal(404); + expect(response.body.errors[0].detail).to.equal('Tag with this id (999999999) could not be found'); + }); + it('should successfully archive the given tag', async () => { const now = Date.now(); const response = await request(server).put(`/api/tags/${createdTag.id}?token=admin`).send({ archivedAt: now }); diff --git a/test/lib/usecases/tag/DeleteTagUseCase.test.js b/test/lib/usecases/tag/DeleteTagUseCase.test.js index bad598f919..336b38818e 100644 --- a/test/lib/usecases/tag/DeleteTagUseCase.test.js +++ b/test/lib/usecases/tag/DeleteTagUseCase.test.js @@ -28,6 +28,7 @@ module.exports = () => { }, }); + createTagDto.session = { id: 1, externalId: 1, name: 'John Doe' }; createdTag = await new CreateTagUseCase() .execute(createTagDto); }); diff --git a/test/lib/usecases/tag/GetAllTagsUseCase.test.js b/test/lib/usecases/tag/GetAllTagsUseCase.test.js index ad0ecbed17..3181f04ba5 100644 --- a/test/lib/usecases/tag/GetAllTagsUseCase.test.js +++ b/test/lib/usecases/tag/GetAllTagsUseCase.test.js @@ -33,13 +33,11 @@ module.exports = () => { const { tags } = await new GetAllTagsUseCase().execute(); for (const tag of tags) { - expect(tag).to.have.property('lastEditedByUserId'); expect(tag).to.have.property('lastEditedBy'); expect(tag).to.not.have.property('lastEditedName'); - if (tag.lastEditedByUserId === null) { - expect(tag.lastEditedBy).to.be.null; - } else { - expect(tag.lastEditedBy.id).to.equal(tag.lastEditedByUserId); + expect(tag).to.not.have.property('lastEditedByUserId'); + if (tag.lastEditedBy !== null) { + expect(tag.lastEditedBy).to.have.all.keys('id', 'externalId', 'name'); } } }); diff --git a/test/lib/usecases/tag/GetTagUseCase.test.js b/test/lib/usecases/tag/GetTagUseCase.test.js index a876946b90..733357431a 100644 --- a/test/lib/usecases/tag/GetTagUseCase.test.js +++ b/test/lib/usecases/tag/GetTagUseCase.test.js @@ -41,7 +41,6 @@ module.exports = () => { const createdTag = await TagRepository.insert({ text: `TAG-LAST-EDITED-${Date.now()}`, lastEditedByUserId: 2 }); const result = await new GetTagUseCase().execute({ params: { tagId: createdTag.id } }); - expect(result.lastEditedByUserId).to.equal(2); expect(result.lastEditedBy).to.deep.equal({ id: 2, externalId: 456, name: 'Jan Jansen' }); await TagRepository.removeAll(new QueryBuilder().where('id').is(createdTag.id)); @@ -51,7 +50,6 @@ module.exports = () => { const createdTag = await TagRepository.insert({ text: `TAG-NEVER-EDITED-${Date.now()}` }); const result = await new GetTagUseCase().execute({ params: { tagId: createdTag.id } }); - expect(result.lastEditedByUserId).to.be.null; expect(result.lastEditedBy).to.be.null; await TagRepository.removeAll(new QueryBuilder().where('id').is(createdTag.id)); diff --git a/test/lib/usecases/tag/UpdateTagUseCase.test.js b/test/lib/usecases/tag/UpdateTagUseCase.test.js index 959c830213..bf3d0fe736 100644 --- a/test/lib/usecases/tag/UpdateTagUseCase.test.js +++ b/test/lib/usecases/tag/UpdateTagUseCase.test.js @@ -14,6 +14,9 @@ const { tag: { UpdateTagUseCase } } = require('../../../../lib/usecases/index.js'); const { dtos: { UpdateTagDto } } = require('../../../../lib/domain/index.js'); const chai = require('chai'); +const assert = require('assert'); +const { BadParameterError } = require('../../../../lib/server/errors/BadParameterError.js'); +const { NotFoundError } = require('../../../../lib/server/errors/NotFoundError.js'); const { expect } = chai; @@ -39,33 +42,48 @@ module.exports = () => { }; }); it('should save the correct values', async () => { - const { result } = await new UpdateTagUseCase() + const result = await new UpdateTagUseCase() .execute(updateTagDto); expect(result.mattermost).to.equal('tag,tag,tag'); - expect(result.lastEditedName).to.equal('John Doe'); + expect(result.lastEditedBy).to.deep.equal({ id: 1, externalId: 1, name: 'John Doe' }); + expect(result).to.not.have.property('lastEditedName'); expect(result.email).to.equal('cern@tag.ch,cern@othertag.ch'); expect(result.description).to.equal('The new tag\'s description'); expect(result.archived).to.be.true; }); - it('should return an error when values do not match', async () => { - const newTagDto = await UpdateTagDto.validateAsync({ - body: { - mattermost: 'tag,tag,tag', - email: 'cern@tag.ch', - }, - params: { - tagId: 9999, - }, - }); - newTagDto.session = { - personid: 1, - id: 1, - name: 'John Do', + it('should store the id of the user performing the update', async () => { + updateTagDto.session = { + personid: 456, + id: 2, + name: 'Jan Jansen', }; - const { error } = await new UpdateTagUseCase() - .execute(newTagDto); - expect(error.status).to.equal('400'); - expect(error.title).to.equal('this tag with this tag id: (9999) could not be found.'); + const result = await new UpdateTagUseCase() + .execute(updateTagDto); + expect(result.lastEditedBy).to.deep.equal({ id: 2, externalId: 456, name: 'Jan Jansen' }); + }); + + it('should reject when no user is provided in the session', async () => { + delete updateTagDto.session; + await assert.rejects( + () => new UpdateTagUseCase().execute(updateTagDto), + new BadParameterError('A user is required to update a tag'), + ); + }); + + it('should reject when the session user does not exist', async () => { + updateTagDto.session = { id: 9999, externalId: 9999, name: 'Ghost' }; + await assert.rejects( + () => new UpdateTagUseCase().execute(updateTagDto), + new NotFoundError('User with this id (9999) could not be found'), + ); + }); + + it('should reject when the tag does not exist', async () => { + updateTagDto.params.tagId = 9999; + await assert.rejects( + () => new UpdateTagUseCase().execute(updateTagDto), + new NotFoundError('Tag with this id (9999) could not be found'), + ); }); }; From 4bd7b9be07933d1dace57f1db9623816ab14f54b Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 1 Oct 2026 15:40:03 +0200 Subject: [PATCH 5/8] API endpoint should return only needed information on backend --- lib/database/adapters/TagAdapter.js | 2 +- lib/domain/entities/Tag.js | 2 +- lib/public/components/tag/tagDetail.js | 4 ++-- .../Tags/ActiveColumns/tagsActiveColumns.js | 4 ++-- lib/usecases/tag/GetAllTagsUseCase.js | 2 +- lib/usecases/tag/GetTagByNameUseCase.js | 2 +- lib/usecases/tag/GetTagUseCase.js | 2 +- test/api/tags.test.js | 2 +- .../usecases/tag/GetAllTagsUseCase.test.js | 2 +- test/lib/usecases/tag/GetTagUseCase.test.js | 2 +- .../lib/usecases/tag/UpdateTagUseCase.test.js | 4 ++-- test/public/tags/detail.test.js | 14 ++++++++++++- test/public/tags/overview.test.js | 20 +++++++++++++++++++ 13 files changed, 47 insertions(+), 15 deletions(-) diff --git a/lib/database/adapters/TagAdapter.js b/lib/database/adapters/TagAdapter.js index 208bed20ca..af8ad5affa 100644 --- a/lib/database/adapters/TagAdapter.js +++ b/lib/database/adapters/TagAdapter.js @@ -35,7 +35,7 @@ class TagAdapter { id, text, description, - lastEditedBy: lastEditedBy ? this.userAdapter.toEntity(lastEditedBy) : null, + lastEditedBy: lastEditedBy ? this.userAdapter.toNameOnly(lastEditedBy) : null, email, mattermost, archived, diff --git a/lib/domain/entities/Tag.js b/lib/domain/entities/Tag.js index 9bb36bec15..3aa8ba76a5 100644 --- a/lib/domain/entities/Tag.js +++ b/lib/domain/entities/Tag.js @@ -18,7 +18,7 @@ * @property {string} description * @property {string|null} email * @property {string|null} mattermost - * @property {User|null} lastEditedBy + * @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 diff --git a/lib/public/components/tag/tagDetail.js b/lib/public/components/tag/tagDetail.js index 83d9190e5e..b89f60995d 100644 --- a/lib/public/components/tag/tagDetail.js +++ b/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', diff --git a/lib/public/views/Tags/ActiveColumns/tagsActiveColumns.js b/lib/public/views/Tags/ActiveColumns/tagsActiveColumns.js index 8d5cb05c43..4551c2757a 100644 --- a/lib/public/views/Tags/ActiveColumns/tagsActiveColumns.js +++ b/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', diff --git a/lib/usecases/tag/GetAllTagsUseCase.js b/lib/usecases/tag/GetAllTagsUseCase.js index 580262f9f4..fae6ea973b 100644 --- a/lib/usecases/tag/GetAllTagsUseCase.js +++ b/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().include({ association: 'lastEditedBy' }); + const queryBuilder = new QueryBuilder().include({ association: 'lastEditedBy', attributes: ['name'] }); const { query = {} } = dto; const { filter = {} } = query; diff --git a/lib/usecases/tag/GetTagByNameUseCase.js b/lib/usecases/tag/GetTagByNameUseCase.js index 1587fb0160..56b12591dd 100644 --- a/lib/usecases/tag/GetTagByNameUseCase.js +++ b/lib/usecases/tag/GetTagByNameUseCase.js @@ -39,7 +39,7 @@ class GetTagByNameUseCase { const tag = await TransactionHelper.provide(async () => { const queryBuilder = new QueryBuilder() .where('text').is(name) - .include({ association: 'lastEditedBy' }); + .include({ association: 'lastEditedBy', attributes: ['name'] }); return TagRepository.findOne(queryBuilder); }); diff --git a/lib/usecases/tag/GetTagUseCase.js b/lib/usecases/tag/GetTagUseCase.js index e76d1fc689..b2bc9bb604 100644 --- a/lib/usecases/tag/GetTagUseCase.js +++ b/lib/usecases/tag/GetTagUseCase.js @@ -39,7 +39,7 @@ class GetTagUseCase { const tag = await TransactionHelper.provide(async () => { const queryBuilder = new QueryBuilder() .where('id').is(tagId) - .include({ association: 'lastEditedBy' }); + .include({ association: 'lastEditedBy', attributes: ['name'] }); return TagRepository.findOne(queryBuilder); }); diff --git a/test/api/tags.test.js b/test/api/tags.test.js index 68fc4e41a7..f657a14ce9 100644 --- a/test/api/tags.test.js +++ b/test/api/tags.test.js @@ -591,7 +591,7 @@ module.exports = () => { const getResponse = await request(server).get(`/api/tags/${createdTag.id}`); expect(getResponse.status).to.equal(200); const { data } = getResponse.body; - expect(data.lastEditedBy).to.deep.equal({ id: 1, externalId: 1, name: 'John Doe' }); + expect(data.lastEditedBy).to.deep.equal({ name: 'John Doe' }); expect(data).to.not.have.property('lastEditedName'); }); it('should return 400 if invalid email is given', (done) => { diff --git a/test/lib/usecases/tag/GetAllTagsUseCase.test.js b/test/lib/usecases/tag/GetAllTagsUseCase.test.js index 3181f04ba5..a9c5882c24 100644 --- a/test/lib/usecases/tag/GetAllTagsUseCase.test.js +++ b/test/lib/usecases/tag/GetAllTagsUseCase.test.js @@ -37,7 +37,7 @@ module.exports = () => { expect(tag).to.not.have.property('lastEditedName'); expect(tag).to.not.have.property('lastEditedByUserId'); if (tag.lastEditedBy !== null) { - expect(tag.lastEditedBy).to.have.all.keys('id', 'externalId', 'name'); + expect(tag.lastEditedBy).to.have.all.keys('name'); } } }); diff --git a/test/lib/usecases/tag/GetTagUseCase.test.js b/test/lib/usecases/tag/GetTagUseCase.test.js index 733357431a..b8b94e98b0 100644 --- a/test/lib/usecases/tag/GetTagUseCase.test.js +++ b/test/lib/usecases/tag/GetTagUseCase.test.js @@ -41,7 +41,7 @@ module.exports = () => { const createdTag = await TagRepository.insert({ text: `TAG-LAST-EDITED-${Date.now()}`, lastEditedByUserId: 2 }); const result = await new GetTagUseCase().execute({ params: { tagId: createdTag.id } }); - expect(result.lastEditedBy).to.deep.equal({ id: 2, externalId: 456, name: 'Jan Jansen' }); + expect(result.lastEditedBy).to.deep.equal({ name: 'Jan Jansen' }); await TagRepository.removeAll(new QueryBuilder().where('id').is(createdTag.id)); }); diff --git a/test/lib/usecases/tag/UpdateTagUseCase.test.js b/test/lib/usecases/tag/UpdateTagUseCase.test.js index bf3d0fe736..0ef90e6267 100644 --- a/test/lib/usecases/tag/UpdateTagUseCase.test.js +++ b/test/lib/usecases/tag/UpdateTagUseCase.test.js @@ -45,7 +45,7 @@ module.exports = () => { const result = await new UpdateTagUseCase() .execute(updateTagDto); expect(result.mattermost).to.equal('tag,tag,tag'); - expect(result.lastEditedBy).to.deep.equal({ id: 1, externalId: 1, name: 'John Doe' }); + expect(result.lastEditedBy).to.deep.equal({ name: 'John Doe' }); expect(result).to.not.have.property('lastEditedName'); expect(result.email).to.equal('cern@tag.ch,cern@othertag.ch'); expect(result.description).to.equal('The new tag\'s description'); @@ -60,7 +60,7 @@ module.exports = () => { }; const result = await new UpdateTagUseCase() .execute(updateTagDto); - expect(result.lastEditedBy).to.deep.equal({ id: 2, externalId: 456, name: 'Jan Jansen' }); + expect(result.lastEditedBy).to.deep.equal({ name: 'Jan Jansen' }); }); it('should reject when no user is provided in the session', async () => { diff --git a/test/public/tags/detail.test.js b/test/public/tags/detail.test.js index 716e324fa7..fbe34c9c79 100644 --- a/test/public/tags/detail.test.js +++ b/test/public/tags/detail.test.js @@ -12,8 +12,9 @@ */ const chai = require('chai'); -const { defaultBefore, defaultAfter, expectInnerText, pressElement, getFirstRow, goToPage, waitForNavigation } = require('../defaults.js'); +const { defaultBefore, defaultAfter, expectInnerText, expectInnerTextTo, pressElement, getFirstRow, goToPage, waitForNavigation } = require('../defaults.js'); const { resetDatabaseContent } = require('../../utilities/resetDatabaseContent.js'); +const { repositories: { TagRepository } } = require('../../../lib/database/index.js'); const { expect } = chai; @@ -60,6 +61,17 @@ module.exports = () => { expect(await emails[0].evaluate((element) => element.innerText)).to.equal('food-group@cern.ch'); }); + it('should display the name of the user who last edited the tag', async () => { + await goToPage(page, 'tag-detail', { queryParameters: { id: 1, panel: 'logs' } }); + await expectInnerTextTo(page, '#tag-lastEditedBy', (text) => text.trim().endsWith('-')); + + await TagRepository.updateAll({ lastEditedByUserId: 1 }, { where: { id: 1 } }); + await goToPage(page, 'tag-detail', { queryParameters: { id: 1, panel: 'logs' } }); + await expectInnerTextTo(page, '#tag-lastEditedBy', (text) => text.trim().endsWith('John Doe')); + + await TagRepository.updateAll({ lastEditedByUserId: null }, { where: { id: 1 } }); + }); + it('notifies if a specified tag id is invalid', async () => { // Navigate to a tag detail view with an id that cannot exist await goToPage(page, 'tag-detail', { queryParameters: { id: 'abc' } }); diff --git a/test/public/tags/overview.test.js b/test/public/tags/overview.test.js index 6b28633b6a..266493c062 100644 --- a/test/public/tags/overview.test.js +++ b/test/public/tags/overview.test.js @@ -23,6 +23,7 @@ const { waitForNavigation, } = require('../defaults.js'); const { resetDatabaseContent } = require('../../utilities/resetDatabaseContent.js'); +const { repositories: { TagRepository } } = require('../../../lib/database/index.js'); const { expect } = chai; @@ -73,6 +74,25 @@ module.exports = () => { expect(headers[4]).to.equal('Email'); }); + it('should display the name of the user who last edited the tag', async () => { + await page.waitForSelector('tbody tr[id^="row"]'); + table = await page.$$('tbody tr'); + const editedRowId = await getFirstRow(table, page); + const editedTagId = parseInt(editedRowId.slice('row'.length), 10); + + await TagRepository.updateAll({ lastEditedByUserId: 2 }, { where: { id: editedTagId } }); + await page.reload({ waitUntil: 'networkidle0' }); + + await page.waitForSelector(`#${editedRowId}-lastEditedBy`); + expect(await page.$eval(`#${editedRowId}-lastEditedBy`, (cell) => cell.innerText)).to.equal('Jan Jansen'); + + // Tags never edited display a placeholder + const uneditedCellText = await page.$eval('tbody tr:nth-child(2) td:nth-child(2)', (cell) => cell.innerText); + expect(uneditedCellText).to.equal('-'); + + await TagRepository.updateAll({ lastEditedByUserId: null }, { where: { id: editedTagId } }); + }); + it('can navigate to a tag detail page', async () => { table = await page.$$('tr'); firstRowId = await getFirstRow(table, page); From bee71652338ca5f50776c66ae9767b0944214018 Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 1 Oct 2026 16:46:03 +0200 Subject: [PATCH 6/8] Update data model to use user id and not name in eor reasons --- docs/data-model.md | 2 +- lib/database/adapters/EorReasonAdapter.js | 11 +++- lib/database/adapters/index.js | 1 + ...0000-eor-reasons-last-edited-by-user-id.js | 56 +++++++++++++++++++ lib/database/models/eorreason.js | 6 +- .../models/typedefs/SequelizeEorReason.js | 3 +- .../seeders/20220513153907-eor-reason.js | 8 +-- lib/domain/entities/EorReason.js | 2 +- 8 files changed, 77 insertions(+), 12 deletions(-) create mode 100644 lib/database/migrations/v1/20261001100000-eor-reasons-last-edited-by-user-id.js diff --git a/docs/data-model.md b/docs/data-model.md index 3e5278a837..f2e31df33d 100644 --- a/docs/data-model.md +++ b/docs/data-model.md @@ -101,7 +101,7 @@ Concerning the **Update mode** of the fields: | `description` | Other information on the reason | `Run stopped due to faulty detector` | AT COE | `description` | Insert | | `reason_type_id` | Id of the general reason type belonging to | '1' | AT COE | `reason_type_id` | Insert | | `run_id` | RUN id for which the reason was added | `500540` | AT COE | `run_id` | Insert | -| `last_edited_name` | Name of the person who last edited the fields | `Anonymous`, `Jan Janssen` | When fields are edited | `id` | Update | +| `last_edited_by_user_id` | Id (in `users` table) of the user who last edited the fields | `1`, `2` | When fields are edited | `id` | Update | | `created_at` | When the entity is created | | AT COE | `created_at` | Insert | | `updated_at` | When entity is edited | | When fields are edited | `updated_at` | Update | diff --git a/lib/database/adapters/EorReasonAdapter.js b/lib/database/adapters/EorReasonAdapter.js index d88bdd6c14..816534e7ea 100644 --- a/lib/database/adapters/EorReasonAdapter.js +++ b/lib/database/adapters/EorReasonAdapter.js @@ -24,6 +24,11 @@ class EorReasonAdapter { */ this.reasonTypeAdapter = null; + /** + * @type {UserAdapter|null} + */ + this.userAdapter = null; + this.toEntity = this.toEntity.bind(this); this.toDatabase = this.toDatabase.bind(this); } @@ -33,11 +38,11 @@ class EorReasonAdapter { * @param {SequelizeEorReason} databaseObject Object to convert. * @returns {EorReason} Converted entity object. */ - toEntity({ id, description, runId, reasonTypeId, reasonType, lastEditedName, createdAt, updatedAt }) { + toEntity({ id, description, runId, reasonTypeId, reasonType, lastEditedBy, createdAt, updatedAt }) { const entityObject = { id, description, - lastEditedName, + lastEditedBy: lastEditedBy ? this.userAdapter.toNameOnly(lastEditedBy) : null, reasonTypeId, runId, createdAt: new Date(createdAt).getTime(), @@ -62,7 +67,7 @@ class EorReasonAdapter { return { id: entityObject.id, description: entityObject.description, - lastEditedName: entityObject.lastEditedName, + lastEditedByUserId: entityObject.lastEditedByUserId, reasonTypeId: entityObject.reasonTypeId, runId: entityObject.runId, }; diff --git a/lib/database/adapters/index.js b/lib/database/adapters/index.js index fc9a81a696..15e653444b 100644 --- a/lib/database/adapters/index.js +++ b/lib/database/adapters/index.js @@ -105,6 +105,7 @@ environmentAdapter.environmentHistoryItemAdapter = environmentHistoryItemAdapter environmentAdapter.runAdapter = runAdapter; eorReasonAdapter.reasonTypeAdapter = reasonTypeAdapter; +eorReasonAdapter.userAdapter = userAdapter; flpRoleAdapter.runAdapter = runAdapter; diff --git a/lib/database/migrations/v1/20261001100000-eor-reasons-last-edited-by-user-id.js b/lib/database/migrations/v1/20261001100000-eor-reasons-last-edited-by-user-id.js new file mode 100644 index 0000000000..c99de58752 --- /dev/null +++ b/lib/database/migrations/v1/20261001100000-eor-reasons-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('eor_reasons', 'last_edited_by_user_id', { + type: Sequelize.INTEGER, + allowNull: true, + references: { + model: 'users', + key: 'id', + }, + onUpdate: 'CASCADE', + onDelete: 'SET NULL', + }, { transaction }); + + // Link existing EoR reasons to the user matching the stored name (ambiguous names resolve to the oldest user) + await queryInterface.sequelize.query( + `UPDATE eor_reasons e + SET e.last_edited_by_user_id = (SELECT MIN(u.id) FROM users u WHERE u.name = e.last_edited_name) + WHERE e.last_edited_name IS NOT NULL`, + { transaction }, + ); + + await queryInterface.removeColumn('eor_reasons', 'last_edited_name', { transaction }); + }), + + down: async (queryInterface, Sequelize) => queryInterface.sequelize.transaction(async (transaction) => { + await queryInterface.addColumn('eor_reasons', 'last_edited_name', { + type: Sequelize.STRING, + allowNull: true, + }, { transaction }); + + await queryInterface.sequelize.query( + `UPDATE eor_reasons e + INNER JOIN users u ON u.id = e.last_edited_by_user_id + SET e.last_edited_name = u.name`, + { transaction }, + ); + + await queryInterface.removeColumn('eor_reasons', 'last_edited_by_user_id', { transaction }); + }), +}; diff --git a/lib/database/models/eorreason.js b/lib/database/models/eorreason.js index d6a8d3578b..0844d9e4fc 100644 --- a/lib/database/models/eorreason.js +++ b/lib/database/models/eorreason.js @@ -24,8 +24,9 @@ module.exports = (sequelize) => { description: { type: Sequelize.STRING, }, - lastEditedName: { - type: Sequelize.STRING, + lastEditedByUserId: { + type: Sequelize.INTEGER, + allowNull: true, }, reasonTypeId: { type: Sequelize.INTEGER, @@ -38,6 +39,7 @@ module.exports = (sequelize) => { EorReason.associate = (models) => { EorReason.belongsTo(models.Run); EorReason.belongsTo(models.ReasonType, { as: 'reasonType' }); + EorReason.belongsTo(models.User, { as: 'lastEditedBy', foreignKey: 'lastEditedByUserId' }); }; return EorReason; diff --git a/lib/database/models/typedefs/SequelizeEorReason.js b/lib/database/models/typedefs/SequelizeEorReason.js index 67b06d315a..953dc31466 100644 --- a/lib/database/models/typedefs/SequelizeEorReason.js +++ b/lib/database/models/typedefs/SequelizeEorReason.js @@ -16,7 +16,8 @@ * * @property {number} id * @property {string|null} description - * @property {string|null} lastEditedName + * @property {number|null} lastEditedByUserId + * @property {SequelizeUser|null} [lastEditedBy] * @property {number} reasonTypeId * @property {number} runId * @property {string} createdAt diff --git a/lib/database/seeders/20220513153907-eor-reason.js b/lib/database/seeders/20220513153907-eor-reason.js index d927206ba5..55fef0b5f0 100644 --- a/lib/database/seeders/20220513153907-eor-reason.js +++ b/lib/database/seeders/20220513153907-eor-reason.js @@ -20,7 +20,7 @@ module.exports = { queryInterface.bulkInsert('eor_reasons', [ { description: 'Some Reason other than selected', - last_edited_name: 'Anonymous', + last_edited_by_user_id: 3, run_id: 1, reason_type_id: 1, created_at: new Date('2022-08-09'), @@ -28,21 +28,21 @@ module.exports = { }, { description: 'Some Reason other than selected plus one', - last_edited_name: 'Anonymous', + last_edited_by_user_id: 3, reason_type_id: 2, run_id: 1, created_at: new Date('2022-08-09'), updated_at: new Date('2022-08-10 05:00:00'), }, { - last_edited_name: 'Anonymous', + last_edited_by_user_id: 3, reason_type_id: 3, run_id: 56, created_at: new Date('2021-08-09'), updated_at: new Date('2021-08-10 15:00:00'), }, { - last_edited_name: 'Anonymous', + last_edited_by_user_id: 3, reason_type_id: 1, run_id: 56, created_at: new Date('2021-07-09'), diff --git a/lib/domain/entities/EorReason.js b/lib/domain/entities/EorReason.js index 570693db3a..58271a1777 100644 --- a/lib/domain/entities/EorReason.js +++ b/lib/domain/entities/EorReason.js @@ -16,7 +16,7 @@ * * @property {number} id * @property {string|null} description - * @property {string|null} lastEditedName + * @property {{name: string}|null} lastEditedBy the name of the user who last edited the EoR reason * @property {number} reasonTypeId * @property {number} runId * @property {number} [createdAt] From 98db1035aa39e722edc23ae409117e19b749baed Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 1 Oct 2026 16:48:59 +0200 Subject: [PATCH 7/8] Update front-end to use new data model --- lib/public/views/Runs/Details/RunPatch.js | 8 ++++---- lib/public/views/Runs/format/editRunEorReasons.js | 4 ++-- lib/public/views/Runs/format/formatRunEorReason.js | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/public/views/Runs/Details/RunPatch.js b/lib/public/views/Runs/Details/RunPatch.js index 54f7e347c6..e8188771ba 100644 --- a/lib/public/views/Runs/Details/RunPatch.js +++ b/lib/public/views/Runs/Details/RunPatch.js @@ -9,7 +9,7 @@ import { RunQualities } from '../../../domain/enums/RunQualities.js'; * @property {string} category * @property {string} title * @property {string} description - * @property {string|null} [lastEditedName] + * @property {{name: string}|null} [lastEditedBy] */ /** @@ -76,7 +76,7 @@ export class RunPatch extends Observable { } if (this._eorReasons.length !== this._run.eorReasons.length || this._eorReasons.some(({ id }) => id === undefined)) { - // Strip lastEditedName — the server's EorReasonDto only accepts id, reasonTypeId, and description + // Strip lastEditedBy — the server's EorReasonDto only accepts id, reasonTypeId, and description ret.eorReasons = this._eorReasons.map(({ id, reasonTypeId, description }) => ({ id, reasonTypeId, description })); } @@ -128,11 +128,11 @@ export class RunPatch extends Observable { } = this._run || {}; this._runQuality = runQuality; - this._eorReasons = eorReasons.map(({ id, description, reasonTypeId, lastEditedName }) => ({ + this._eorReasons = eorReasons.map(({ id, description, reasonTypeId, lastEditedBy }) => ({ id, description, reasonTypeId, - lastEditedName, + lastEditedBy, })); this._tags = tags.map(({ text }) => text); diff --git a/lib/public/views/Runs/format/editRunEorReasons.js b/lib/public/views/Runs/format/editRunEorReasons.js index 6ba0d59e24..601e5d3685 100644 --- a/lib/public/views/Runs/format/editRunEorReasons.js +++ b/lib/public/views/Runs/format/editRunEorReasons.js @@ -94,7 +94,7 @@ export const editRunEorReasons = (runDetailsModel) => { */ runDetailsModel.runPatch.eorReasons.length > 0 ? runDetailsModel.runPatch.eorReasons.map((eorReason) => { - const { reasonTypeId, description, lastEditedName } = eorReason; + const { reasonTypeId, description, lastEditedBy } = eorReason; const { category = '-', title } = eorReasonTypes.find((eorReasonType) => eorReasonType.id === reasonTypeId) || {}; const titleString = title ? ` - ${title}` : ''; const descriptionString = description ? ` - ${description}` : ''; @@ -110,7 +110,7 @@ export const editRunEorReasons = (runDetailsModel) => { }, iconTrash()), h('.w-wrapped', `${category} ${titleString} ${descriptionString}`), ]), - h('.w-wrapped', lastEditedName || null), + h('.w-wrapped', lastEditedBy?.name || null), ], ); }) diff --git a/lib/public/views/Runs/format/formatRunEorReason.js b/lib/public/views/Runs/format/formatRunEorReason.js index b97ab4a223..f6f676e42e 100644 --- a/lib/public/views/Runs/format/formatRunEorReason.js +++ b/lib/public/views/Runs/format/formatRunEorReason.js @@ -16,21 +16,21 @@ import { tooltip } from '../../../../components/common/popover/tooltip.js'; import { formatEorReason } from './formatEorReason.mjs'; /** - * Display the given EoR reason as a vnode component with lastEditedName tooltip + * Display the given EoR reason as a vnode component with a tooltip containing the name of its last editor * * @param {Partial<{ * category: string, * title: string, * description: string, - * lastEditedName: string, + * lastEditedBy: {name: string}|null, * }>} eorReason the EoR reason to display * @return {VNode} the vnode component */ export const formatRunEorReason = (eorReason) => { - const { lastEditedName } = eorReason; + const lastEditorName = eorReason.lastEditedBy?.name; const reasonText = formatEorReason(eorReason); return h('.w-100.flex-row.justify-between', [ h('', reasonText), - lastEditedName ? tooltip(h('.w-wrapped', lastEditedName), 'Last edited by') : null, + lastEditorName ? tooltip(h('.w-wrapped', lastEditorName), 'Last edited by') : null, ]); }; From d3d9ccb1f7d282216270f940bbda57605641de48 Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 1 Oct 2026 16:51:17 +0200 Subject: [PATCH 8/8] Update backend service and usecase to use new structure for user associated to eor reason --- lib/server/controllers/runs.controller.js | 18 +-- lib/server/services/run/RunService.js | 18 ++- lib/usecases/run/GetRunUseCase.js | 14 +- lib/usecases/run/UpdateRunUseCase.js | 81 +++++----- test/api/runs.test.js | 61 ++++---- .../server/services/run/RunService.test.js | 26 ++++ .../lib/usecases/run/UpdateRunUseCase.test.js | 139 +++++++++++------- test/public/runs/detail.test.js | 2 +- 8 files changed, 218 insertions(+), 141 deletions(-) diff --git a/lib/server/controllers/runs.controller.js b/lib/server/controllers/runs.controller.js index 380899dff4..58ab18df07 100644 --- a/lib/server/controllers/runs.controller.js +++ b/lib/server/controllers/runs.controller.js @@ -241,12 +241,11 @@ const updateRun = async (request, response) => { return; } - const { result: run, error } = await new UpdateRunUseCase().execute(value); - - if (error) { - response.status(Number(error.status)).json({ errors: [error] }); - } else { + try { + const run = await new UpdateRunUseCase().execute(value); response.status(201).json({ data: runToHttpView(run) }); + } catch (error) { + updateExpressResponseFromNativeError(response, error); } }; @@ -265,12 +264,11 @@ const updateRunByRunNumber = async (request, response) => { return; } - const { result: run, error } = await new UpdateRunUseCase().execute(value); - - if (error) { - response.status(Number(error.status)).json({ errors: [error] }); - } else { + try { + const run = await new UpdateRunUseCase().execute(value); response.status(200).json({ data: runToHttpView(run) }); + } catch (error) { + updateExpressResponseFromNativeError(response, error); } }; diff --git a/lib/server/services/run/RunService.js b/lib/server/services/run/RunService.js index 6d0443ef1f..913b653fdd 100644 --- a/lib/server/services/run/RunService.js +++ b/lib/server/services/run/RunService.js @@ -384,7 +384,7 @@ class RunService { // Update EOR reasons if they are provided if (eorReasons) { - await updateEorReasonsOnRun(run.id, run.runNumber, user?.name, eorReasons, transaction); + await updateEorReasonsOnRun(run.id, run.runNumber, user, eorReasons, transaction); } // Update detector qualities if they are provided @@ -434,7 +434,13 @@ class RunService { queryBuilder.include('runType'); } if (relations.eorReasons) { - queryBuilder.include({ association: 'eorReasons', include: { model: ReasonType, as: 'reasonType' } }); + queryBuilder.include({ + association: 'eorReasons', + include: [ + { model: ReasonType, as: 'reasonType' }, + { association: 'lastEditedBy', attributes: ['name'] }, + ], + }); } if (relations.flpRoles) { queryBuilder.include('flpRoles'); @@ -570,13 +576,13 @@ class RunService { * * @param {number} runId - id of the run that is due to be modified * @param {number} runNumber - run number of the run that is due to be modified - * @param {string} userName - name of the user editing the EOR reasons + * @param {SequelizeUser|null} user - the user editing the EOR reasons, null if the change is not done by a user (e.g. automatic EoR reasons) * @param {EorReasonPatch[]} eorReasonsPatches - full list of EoR reasons to apply on the run (any existing EoR reason not in the list will be * removed) * @param {import('sequelize').Transaction} [transaction] optional transaction in which operations must be wrapped * @returns {Promise} - promise on result of db queries */ -const updateEorReasonsOnRun = async (runId, runNumber, userName, eorReasonsPatches, transaction) => { +const updateEorReasonsOnRun = async (runId, runNumber, user, eorReasonsPatches, transaction) => { const reasonTypes = await ReasonTypeRepository.findAll(); const idsOfReasonTypesToLog = []; @@ -628,7 +634,7 @@ const updateEorReasonsOnRun = async (runId, runNumber, userName, eorReasonsPatch if (id) { toKeepEorReasonsIds.push(id); } else { - newEorReasons.push({ runId, reasonTypeId, description, lastEditedName: userName }); + newEorReasons.push({ runId, reasonTypeId, description, lastEditedByUserId: user?.id ?? null }); if (idsOfReasonTypesToLog.includes(reasonTypeId)) { needLoggingForEorReason = true; @@ -642,7 +648,7 @@ const updateEorReasonsOnRun = async (runId, runNumber, userName, eorReasonsPatch if (needLoggingForEorReason) { await logEorReasonChange( runNumber, - userName, + user?.name, eorReasons.map(({ reasonTypeId, description }) => { const eorReasonType = reasonTypesMap.get(reasonTypeId) ?? {}; return { category: eorReasonType.category, title: eorReasonType.title, description }; diff --git a/lib/usecases/run/GetRunUseCase.js b/lib/usecases/run/GetRunUseCase.js index 56921988bc..7fe350e061 100644 --- a/lib/usecases/run/GetRunUseCase.js +++ b/lib/usecases/run/GetRunUseCase.js @@ -47,10 +47,16 @@ class GetRunUseCase { .include({ model: EorReason, as: 'eorReasons', - include: { - model: ReasonType, - as: 'reasonType', - }, + include: [ + { + model: ReasonType, + as: 'reasonType', + }, + { + association: 'lastEditedBy', + attributes: ['name'], + }, + ], }) .include('lhcFill') .include('lhcPeriod') diff --git a/lib/usecases/run/UpdateRunUseCase.js b/lib/usecases/run/UpdateRunUseCase.js index 0a62134288..b0abec81e2 100644 --- a/lib/usecases/run/UpdateRunUseCase.js +++ b/lib/usecases/run/UpdateRunUseCase.js @@ -12,6 +12,7 @@ */ const { runService } = require('../../server/services/run/RunService.js'); +const { BadParameterError } = require('../../server/errors/BadParameterError.js'); /** * Update a run with provided values. For now we update only RunQuality @@ -21,57 +22,53 @@ class UpdateRunUseCase { * Executes this use case. * * @param {UpdateRunDto} dto containing all data. - * @returns {Promise} Promise object represents the result of this use case. + * @returns {Promise} resolves with the updated run + * @throws {BadParameterError} if end of run reasons are updated without a user in the session + * @throws {NotFoundError} if the run or the session user does not exist */ async execute(dto) { const { body, params = {}, query = {} } = dto; const { runNumber = query.runNumber } = params; - try { - const { - eorReasons, - tags: tagsTexts, - detectorsQualities, - runQualityChangeReason, - calibrationStatusChangeReason, - detectorsQualitiesChangeReason, - phaseShiftAtStart, - phaseShiftAtEnd, - } = body; - delete body.eorReasons; - delete body.tags; - delete body.detectorsQualities; - delete body.phaseShiftAtStart; - delete body.phaseShiftAtEnd; + const { + eorReasons, + tags: tagsTexts, + detectorsQualities, + runQualityChangeReason, + calibrationStatusChangeReason, + detectorsQualitiesChangeReason, + phaseShiftAtStart, + phaseShiftAtEnd, + } = body; + delete body.eorReasons; + delete body.tags; + delete body.detectorsQualities; + delete body.phaseShiftAtStart; + delete body.phaseShiftAtEnd; - body.phaseShiftAtStartBeam1 = phaseShiftAtStart?.beam1; - body.phaseShiftAtStartBeam2 = phaseShiftAtStart?.beam2; - body.phaseShiftAtEndBeam1 = phaseShiftAtEnd?.beam1; - body.phaseShiftAtEndBeam2 = phaseShiftAtEnd?.beam2; + const externalUserId = dto?.session?.externalId; + if (eorReasons && (externalUserId === undefined || externalUserId === null)) { + throw new BadParameterError('A user is required to update the end of run reasons'); + } - const run = await runService.update( - { runNumber }, - { - runPatch: body, - relations: { tagsTexts, eorReasons, userIdentifier: { externalUserId: dto?.session?.externalId }, detectorsQualities }, - metadata: { - runQualityChangeReason: runQualityChangeReason?.trim(), - calibrationStatusChangeReason: calibrationStatusChangeReason?.trim(), - detectorsQualitiesChangeReason: detectorsQualitiesChangeReason?.trim(), - }, - }, - ); + body.phaseShiftAtStartBeam1 = phaseShiftAtStart?.beam1; + body.phaseShiftAtStartBeam2 = phaseShiftAtStart?.beam2; + body.phaseShiftAtEndBeam1 = phaseShiftAtEnd?.beam1; + body.phaseShiftAtEndBeam2 = phaseShiftAtEnd?.beam2; - return { result: run }; - } catch (error) { - return { - error: { - status: 500, - title: 'ServiceUnavailable', - detail: error.message || `Unable to update run with runNumber ${runNumber}`, + // The existence of the user is checked by the run service + return runService.update( + { runNumber }, + { + runPatch: body, + relations: { tagsTexts, eorReasons, userIdentifier: { externalUserId }, detectorsQualities }, + metadata: { + runQualityChangeReason: runQualityChangeReason?.trim(), + calibrationStatusChangeReason: calibrationStatusChangeReason?.trim(), + detectorsQualitiesChangeReason: detectorsQualitiesChangeReason?.trim(), }, - }; - } + }, + ); } } diff --git a/test/api/runs.test.js b/test/api/runs.test.js index 4ada026ce5..5fdca423cc 100644 --- a/test/api/runs.test.js +++ b/test/api/runs.test.js @@ -1145,13 +1145,13 @@ module.exports = () => { }); describe('PUT /api/runs/:runNumber', () => { - it('should return 500 when run could not be found', (done) => { + it('should return 404 when run could not be found', (done) => { request(server) .put('/api/runs/9999999999') .send({ runQuality: RunQualities.BAD, }) - .expect(500) + .expect(404) .end((err, res) => { if (err) { done(err); @@ -1198,19 +1198,19 @@ module.exports = () => { expect(body.errors[0].detail).to.equal('"body.runQuality" must be one of [good, bad, test, none]'); }); - it('should return 500 when trying to update the run quality without justification', async () => { + it('should return 400 when trying to update the run quality without justification', async () => { const { body, status } = await request(server) .put('/api/runs/1') .send({ runQuality: RunQualities.BAD }); - expect(status).to.equal(500); + expect(status).to.equal(400); expect(body.errors[0].detail).to.equal('Run quality change require a reason'); }); - it('should return 500 when trying to update the run quality with an empty justification', async () => { + it('should return 400 when trying to update the run quality with an empty justification', async () => { const { body, status } = await request(server) .put('/api/runs/1') .send({ runQuality: RunQualities.BAD }); - expect(status).to.equal(500); + expect(status).to.equal(400); expect(body.errors[0].detail).to.equal('Run quality change require a reason'); }); @@ -1248,13 +1248,18 @@ module.exports = () => { expect(body.data.runNumber).to.equal(106); expect(body.data.eorReasons).to.have.lengthOf(1); expect(body.data.eorReasons[0].description).to.equal('Some'); + expect(body.data.eorReasons[0].lastEditedBy).to.deep.equal({ name: 'John Doe' }); + expect(body.data.eorReasons[0]).to.not.have.property('lastEditedName'); expect(body.data.runQuality).to.equal(RunQualities.GOOD); + + const { body: { data: fetchedRun } } = await request(server).get('/api/runs/106').expect(200); + expect(fetchedRun.eorReasons[0].lastEditedBy).to.deep.equal({ name: 'John Doe' }); }); it('should give a proper error when a detectorId does not exists', async () => { const { body } = await request(server) .put('/api/runs/1') - .expect(500) + .expect(404) .send({ detectorsQualities: [ { @@ -1286,30 +1291,30 @@ module.exports = () => { expect(body.data.detectorsQualities[0].quality).to.equal(RunDetectorQualities.GOOD); }); - it('should return 500 when trying to update the detector\'s quality of a run that has not ended yet', async () => { + it('should return 400 when trying to update the detector\'s quality of a run that has not ended yet', async () => { const { body, status } = await request(server) .put('/api/runs/105') .send({ detectorsQualities: [{ detectorId: 1, quality: RunDetectorQualities.GOOD }], detectorsQualitiesChangeReason: 'Justification', }); - expect(status).to.equal(500); + expect(status).to.equal(400); expect(body.errors[0].detail).to.equal('Detector quality can not be updated on a run that has not ended yet'); }); - it('should return 500 when trying to update the detector\'s quality without justification', async () => { + it('should return 400 when trying to update the detector\'s quality without justification', async () => { const { body, status } = await request(server) .put('/api/runs/1') .send({ detectorsQualities: [{ detectorId: 1, quality: RunDetectorQualities.GOOD }] }); - expect(status).to.equal(500); + expect(status).to.equal(400); expect(body.errors[0].detail).to.equal('Detector quality change reason is required when updating detector quality'); }); - it('should return 500 when trying to update the detector\'s quality with an empty justification', async () => { + it('should return 400 when trying to update the detector\'s quality with an empty justification', async () => { const { body, status } = await request(server) .put('/api/runs/1') .send({ detectorsQualities: [{ detectorId: 1, quality: RunDetectorQualities.GOOD }], detectorsQualitiesChangeReason: ' ' }); - expect(status).to.equal(500); + expect(status).to.equal(400); expect(body.errors[0].detail).to.equal('Detector quality change reason is required when updating detector quality'); }); @@ -1323,42 +1328,42 @@ module.exports = () => { expect(body.data.calibrationStatus).to.equal(RunCalibrationStatus.SUCCESS); }); - it('should successfully return 500 when trying to set calibration status for non-calibration run', async () => { + it('should return 400 when trying to set calibration status for non-calibration run', async () => { const { body, status } = await request(server) .put('/api/runs/106') .send({ calibrationStatus: RunCalibrationStatus.SUCCESS }); - expect(status).to.equal(500); + expect(status).to.equal(400); expect(body.errors[0].detail).to.equal('Calibration status is reserved to calibration runs'); }); - it('should successfully return 500 when trying to set calibration status change reason for non-failed calibration', async () => { + it('should return 400 when trying to set calibration status change reason for non-failed calibration', async () => { const { body, status } = await request(server) .put('/api/runs/40') .send({ calibrationStatus: RunCalibrationStatus.NO_STATUS, calibrationStatusChangeReason: 'A spurious reason' }); - expect(status).to.equal(500); + expect(status).to.equal(400); expect(body.errors[0].detail) .to.equal(`Calibration status change reason can only be specified when changing from/to ${RunCalibrationStatus.FAILED}`); }); - it('should successfully return 500 when trying to set calibration status to FAILED without reason', async () => { + it('should return 400 when trying to set calibration status to FAILED without reason', async () => { const { body, status } = await request(server) .put('/api/runs/40') .send({ calibrationStatus: RunCalibrationStatus.FAILED }); - expect(status).to.equal(500); + expect(status).to.equal(400); expect(body.errors[0].detail) .to.equal(`Calibration status change require a reason when changing from/to ${RunCalibrationStatus.FAILED}`); }); - it('should successfully return 500 when trying to set calibration status to FAILED with an empty', async () => { + it('should return 400 when trying to set calibration status to FAILED with an empty', async () => { const { body, status } = await request(server) .put('/api/runs/40') .send({ calibrationStatus: RunCalibrationStatus.FAILED, calibrationStatusChangeReason: ' ' }); - expect(status).to.equal(500); + expect(status).to.equal(400); expect(body.errors[0].detail) .to.equal(`Calibration status change require a reason when changing from/to ${RunCalibrationStatus.FAILED}`); }); - it('should successfully return 500 when trying to set calibration status from FAILED without reason', async () => { + it('should return 400 when trying to set calibration status from FAILED without reason', async () => { await updateRun( { runNumber: 40 }, { runPatch: { calibrationStatus: RunCalibrationStatus.FAILED }, metadata: { calibrationStatusChangeReason: 'A reason' } }, @@ -1366,16 +1371,16 @@ module.exports = () => { const { body, status } = await request(server) .put('/api/runs/40') .send({ calibrationStatus: RunCalibrationStatus.SUCCESS }); - expect(status).to.equal(500); + expect(status).to.equal(400); expect(body.errors[0].detail) .to.equal(`Calibration status change require a reason when changing from/to ${RunCalibrationStatus.FAILED}`); }); - it('should successfully return 500 when trying to set calibration status from FAILED with an empty reason', async () => { + it('should return 400 when trying to set calibration status from FAILED with an empty reason', async () => { const { body, status } = await request(server) .put('/api/runs/40') .send({ calibrationStatus: RunCalibrationStatus.SUCCESS, calibrationStatusChangeReason: ' ' }); - expect(status).to.equal(500); + expect(status).to.equal(400); expect(body.errors[0].detail) .to.equal(`Calibration status change require a reason when changing from/to ${RunCalibrationStatus.FAILED}`); }); @@ -1423,7 +1428,7 @@ module.exports = () => { }); describe('PATCH api/runs query:runNumber', () => { - it('should return 500 if the wrong id is given', (done) => { + it('should return 404 if the wrong id is given', (done) => { request(server) .patch('/api/runs?runNumber=99999') .send({ @@ -1435,13 +1440,13 @@ module.exports = () => { aliceDipoleCurrent: 45654.1, aliceDipolePolarity: 'NEGATIVE', }) - .expect(500) + .expect(404) .end((err, res) => { if (err) { done(err); return; } - expect(res.body.errors[0].title).to.equal('ServiceUnavailable'); + expect(res.body.errors[0].detail).to.equal('Run with this run number (99999) could not be found'); done(); }); diff --git a/test/lib/server/services/run/RunService.test.js b/test/lib/server/services/run/RunService.test.js index 314ca29dab..ece31ed051 100644 --- a/test/lib/server/services/run/RunService.test.js +++ b/test/lib/server/services/run/RunService.test.js @@ -371,6 +371,32 @@ module.exports = () => { } }); + it('should store EoR reasons without editor when no user is given (automatic EoR reasons)', async () => { + const run = await runService.update( + { runNumber: 1 }, + { relations: { eorReasons: [{ category: 'DETECTORS', title: 'CPV', description: 'automatic' }] } }, + ); + + expect(run.eorReasons).to.lengthOf(1); + expect(run.eorReasons[0].description).to.equal('automatic'); + expect(run.eorReasons[0].lastEditedBy).to.be.null; + }); + + it('should store the user who created the EoR reasons', async () => { + const run = await runService.update( + { runNumber: 1 }, + { + relations: { + eorReasons: [{ category: 'DETECTORS', title: 'CPV', description: 'by user' }], + userIdentifier: { externalUserId: 456 }, + }, + }, + ); + + expect(run.eorReasons).to.lengthOf(1); + expect(run.eorReasons[0].lastEditedBy).to.deep.equal({ name: 'Jan Jansen' }); + }); + it('should successfully update run with eorReasons with category and title', async () => { const runNumber = 1; diff --git a/test/lib/usecases/run/UpdateRunUseCase.test.js b/test/lib/usecases/run/UpdateRunUseCase.test.js index eeb78063aa..3295b7075c 100644 --- a/test/lib/usecases/run/UpdateRunUseCase.test.js +++ b/test/lib/usecases/run/UpdateRunUseCase.test.js @@ -15,6 +15,9 @@ const { run: { UpdateRunUseCase, GetRunUseCase } } = require('../../../../lib/usecases/index.js'); const { dtos: { UpdateRunDto, GetRunDto, UpdateRunByRunNumberDto } } = require('../../../../lib/domain/index.js'); const chai = require('chai'); +const assert = require('assert'); +const { BadParameterError } = require('../../../../lib/server/errors/BadParameterError.js'); +const { NotFoundError } = require('../../../../lib/server/errors/NotFoundError.js'); const { GetAllLogsUseCase } = require('../../../../lib/usecases/log/index.js'); const { RunQualities } = require('../../../../lib/domain/enums/RunQualities.js'); const { RunDetectorQualities } = require('../../../../lib/domain/enums/RunDetectorQualities.js'); @@ -77,9 +80,10 @@ module.exports = () => { describe('updates with runNumber parameter.', () => { it('Should give an error when the id of the environment can not be found', async () => { updateRunDto.params.runNumber = wrongRunNumber; - const { error } = await new UpdateRunUseCase().execute(updateRunDto); - expect(error.status).to.equal(500); - expect(error.detail).to.equal(`Run with this run number (${wrongRunNumber}) could not be found`); + await assert.rejects( + () => new UpdateRunUseCase().execute(updateRunDto), + new NotFoundError(`Run with this run number (${wrongRunNumber}) could not be found`), + ); }); it('should successfully retrieve run via run number, \ @@ -91,9 +95,8 @@ module.exports = () => { updateRunDto.body.runQuality = RunQualities.BAD; updateRunDto.body.runQualityChangeReason = 'Change reason'; - const { result, error } = await new UpdateRunUseCase().execute(updateRunDto); + const result = await new UpdateRunUseCase().execute(updateRunDto); - expect(error).to.be.an('undefined'); expect(result).to.be.an('object'); expect(result.id).to.equal(106); expect(result.runQuality).to.equal(RunQualities.BAD); @@ -109,10 +112,10 @@ module.exports = () => { updateRunDto.params.runNumber = 105; updateRunDto.body.runQuality = RunQualities.BAD; updateRunDto.body.runQualityChangeReason = 'Change reason'; - const { error } = await new UpdateRunUseCase().execute(updateRunDto); - - expect(error).to.be.an('object'); - expect(error.detail).to.equal('Run quality can not be updated on a run that has not ended yet'); + await assert.rejects( + () => new UpdateRunUseCase().execute(updateRunDto), + new BadParameterError('Run quality can not be updated on a run that has not ended yet'), + ); }); it('should successfully create a log when run quality change', async () => { @@ -200,9 +203,9 @@ module.exports = () => { }, ], }; - const { result, error } = await new UpdateRunUseCase().execute(updateRunDto); + updateRunDto.session = { id: 2, externalId: 456, name: 'Jan Jansen' }; + const result = await new UpdateRunUseCase().execute(updateRunDto); - expect(error).to.be.an('undefined'); expect(result).to.be.an('object'); expect(result.id).to.equal(1); expect(result.eorReasons).to.have.lengthOf(2); @@ -210,13 +213,52 @@ module.exports = () => { expect(result.eorReasons[0].description).to.equal('Some Reason other than selected plus one'); expect(result.eorReasons[1].id).to.equal(7); expect(result.eorReasons[1].description).to.be.null; + + // Kept EoR reason keeps its original editor, the new one is attributed to the session user + expect(result.eorReasons[0].lastEditedBy).to.deep.equal({ name: 'Anonymous' }); + expect(result.eorReasons[1].lastEditedBy).to.deep.equal({ name: 'Jan Jansen' }); + expect(result.eorReasons[1]).to.not.have.property('lastEditedName'); + }); + + it('should reject an update of the end of run reasons without a user', async () => { + updateRunDto.params.runNumber = 1; + updateRunDto.body = { eorReasons: [{ reasonTypeId: 1 }] }; + const { eorReasons: eorReasonsBefore } = await new GetRunUseCase().execute({ params: { runNumber: 1 } }); + + await assert.rejects( + () => new UpdateRunUseCase().execute(updateRunDto), + new BadParameterError('A user is required to update the end of run reasons'), + ); + + const { eorReasons: eorReasonsAfter } = await new GetRunUseCase().execute({ params: { runNumber: 1 } }); + expect(eorReasonsAfter).to.eql(eorReasonsBefore); + }); + + it('should reject an update of the end of run reasons if the user does not exist', async () => { + updateRunDto.params.runNumber = 1; + updateRunDto.body = { eorReasons: [{ reasonTypeId: 1 }] }; + updateRunDto.session = { id: 9999, externalId: 9999, name: 'Ghost' }; + const { eorReasons: eorReasonsBefore } = await new GetRunUseCase().execute({ params: { runNumber: 1 } }); + + await assert.rejects( + () => new UpdateRunUseCase().execute(updateRunDto), + new NotFoundError('User with this external id (9999) could not be found'), + ); + + const { eorReasons: eorReasonsAfter } = await new GetRunUseCase().execute({ params: { runNumber: 1 } }); + expect(eorReasonsAfter).to.eql(eorReasonsBefore); + }); + + it('should allow updating other run fields without a user', async () => { + updateRunDto.body.tags = ['ECS']; + const result = await new UpdateRunUseCase().execute(updateRunDto); + expect(result.tags.map((tag) => tag.text)).to.be.eql(['ECS']); }); it('Should successfully update the run tags', async () => { updateRunDto.body.tags = ['ECS', 'ECS Shifter']; - const { result, error } = await new UpdateRunUseCase().execute(updateRunDto); + const result = await new UpdateRunUseCase().execute(updateRunDto); - expect(error).to.be.undefined; expect(result.tags.map((tag) => tag.text)).to.be.eql(['ECS', 'ECS Shifter']); }); @@ -226,12 +268,10 @@ module.exports = () => { updateRunDto.body.tags = ['FOOD', 'DO-NOT-EXIST', 'DO-NOT-EXIST-EITHER']; const originalRun = await new GetRunUseCase().execute({ params: { runNumber: runNumber } }); - const { result, error } = await new UpdateRunUseCase().execute(updateRunDto); - - expect(result).to.be.undefined; - expect(error).to.be.an('object'); - expect(error.status).to.equal(500); - expect(error.detail).to.equal('Tags DO-NOT-EXIST, DO-NOT-EXIST-EITHER could not be found'); + await assert.rejects( + () => new UpdateRunUseCase().execute(updateRunDto), + { message: 'Tags DO-NOT-EXIST, DO-NOT-EXIST-EITHER could not be found' }, + ); // Expect run to have other fields unchanged const run = await new GetRunUseCase().execute({ params: { runNumber: runNumber } }); @@ -254,14 +294,13 @@ module.exports = () => { expect(log.tags.map(({ text }) => text)).to.eql(['CPV']); }; - const { result, error } = await new UpdateRunUseCase().execute({ + const result = await new UpdateRunUseCase().execute({ params: { runNumber: 1 }, body: { detectorsQualities: [{ detectorId: 1, quality: RunDetectorQualities.BAD }], detectorsQualitiesChangeReason: justification, }, }); - expect(error).to.be.undefined; expect(result).to.be.an('object'); expect(result.detectorsQualities).to.lengthOf(1); expect(result.detectorsQualities[0].id).to.equal(1); @@ -280,35 +319,35 @@ module.exports = () => { }); it('should throw an error when trying to update the quality of a non-existing detector', async () => { - const { result, error } = await new UpdateRunUseCase().execute({ - params: { runNumber: 1 }, - body: { - detectorsQualities: [{ detectorId: 2, quality: RunDetectorQualities.BAD }], - detectorsQualitiesChangeReason: 'Justification', - }, - }); - expect(result).to.be.undefined; - expect(error).to.be.an('object'); - expect(error.detail).to.equal('This run\'s detector with runNumber: (1) and with detector Id: (2) could not be found'); + await assert.rejects( + () => new UpdateRunUseCase().execute({ + params: { runNumber: 1 }, + body: { + detectorsQualities: [{ detectorId: 2, quality: RunDetectorQualities.BAD }], + detectorsQualitiesChangeReason: 'Justification', + }, + }), + new NotFoundError('This run\'s detector with runNumber: (1) and with detector Id: (2) could not be found'), + ); }); it('should throw an error when trying to update the quality of a run not ended yet', async () => { - const { result, error } = await new UpdateRunUseCase().execute({ - params: { runNumber: 105 }, - body: { - detectorsQualities: [{ detectorId: 1, quality: RunDetectorQualities.BAD }], - detectorsQualitiesChangeReason: 'Justification', - }, - }); - expect(result).to.be.undefined; - expect(error).to.be.an('object'); - expect(error.detail).to.equal('Detector quality can not be updated on a run that has not ended yet'); + await assert.rejects( + () => new UpdateRunUseCase().execute({ + params: { runNumber: 105 }, + body: { + detectorsQualities: [{ detectorId: 1, quality: RunDetectorQualities.BAD }], + detectorsQualitiesChangeReason: 'Justification', + }, + }), + new BadParameterError('Detector quality can not be updated on a run that has not ended yet'), + ); }); }); describe('updates with run number', () => { it('Should be able to update the run with correct values', async () => { - const { result } = await new UpdateRunUseCase().execute(updateRunByRunNumberDto); + const result = await new UpdateRunUseCase().execute(updateRunByRunNumberDto); expect(result.runNumber).to.equal(72); expect(result.lhcBeamEnergy).to.equal(232.156); @@ -339,18 +378,18 @@ module.exports = () => { it('Should give an error when the id of the run can not be found', async () => { updateRunByRunNumberDto.query.runNumber = wrongRunNumber; - const { error } = await new UpdateRunUseCase() - .execute(updateRunByRunNumberDto); - expect(error.status).to.equal(500); - expect(error.detail).to.equal(`Run with this run number (${wrongRunNumber}) could not be found`); + await assert.rejects( + () => new UpdateRunUseCase().execute(updateRunByRunNumberDto), + new NotFoundError(`Run with this run number (${wrongRunNumber}) could not be found`), + ); }); it('Should give an error when the id of the lhcFill cannot be found', async () => { updateRunByRunNumberDto.body.fillNumber = wrongRunNumber; - const { error } = await new UpdateRunUseCase() - .execute(updateRunByRunNumberDto); - expect(error.status).to.equal(500); - expect(error.detail).to.equal('LhcFill with id (\'9999999999\') could not be found'); + await assert.rejects( + () => new UpdateRunUseCase().execute(updateRunByRunNumberDto), + new Error('LhcFill with id (\'9999999999\') could not be found'), + ); }); }); }; diff --git a/test/public/runs/detail.test.js b/test/public/runs/detail.test.js index 515f36d8d8..18030b21ea 100644 --- a/test/public/runs/detail.test.js +++ b/test/public/runs/detail.test.js @@ -240,7 +240,7 @@ module.exports = () => { .to.equal('DETECTORS - CPV - A new EOR reason\nAnonymous'); }); - it('should display lastEditedName tooltip with "Last edited by" on formatRunEorReason', async () => { + it('should display the last editor tooltip with "Last edited by" on formatRunEorReason', async () => { const eorReasonElement = await page.$('#eor-reasons .eor-reason'); const popoverTrigger = await eorReasonElement.$('.popover-trigger'); expect(popoverTrigger).to.not.be.null;