From bee71652338ca5f50776c66ae9767b0944214018 Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 1 Oct 2026 16:46:03 +0200 Subject: [PATCH 1/3] 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 2/3] 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 3/3] 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;