From a999dbce1d60af3b7726dfcd89c8d7a7ed7c00b6 Mon Sep 17 00:00:00 2001 From: Martin Boulais <31805063+martinboulais@users.noreply.github.com> Date: Tue, 3 Sep 2024 14:41:19 +0200 Subject: [PATCH] [O2B-955] Create a log when EoR reason DETECTOR is chosen (#1742) * [O2B-955] Create a log when EoR reason DETECTOR is chosen * Revert dumb change * Fix test * Fixing tests attempt * Try to fix flaky test * Fix test more --- .../format/displayRunEorReasonOverview.js | 2 +- .../views/Runs/format/displayRunEorReasons.js | 2 +- ...formatEorReason.js => formatEorReason.mjs} | 3 +- .../eorReasonOccurrencesBarChartComponent.js | 2 +- lib/server/services/run/RunService.js | 131 +++++++++++++++--- .../server/services/run/RunService.test.js | 31 +++++ test/public/runs/detail.test.js | 7 +- test/public/runs/overview.test.js | 3 +- 8 files changed, 149 insertions(+), 32 deletions(-) rename lib/public/views/Runs/format/{formatEorReason.js => formatEorReason.mjs} (87%) diff --git a/lib/public/views/Runs/format/displayRunEorReasonOverview.js b/lib/public/views/Runs/format/displayRunEorReasonOverview.js index 744f73f18d..efbb0fca6b 100644 --- a/lib/public/views/Runs/format/displayRunEorReasonOverview.js +++ b/lib/public/views/Runs/format/displayRunEorReasonOverview.js @@ -12,7 +12,7 @@ */ import { h } from '/js/src/index.js'; -import { formatEorReason } from './formatEorReason.js'; +import { formatEorReason } from './formatEorReason.mjs'; /** * Display a list of EOR reasons diff --git a/lib/public/views/Runs/format/displayRunEorReasons.js b/lib/public/views/Runs/format/displayRunEorReasons.js index 0644e6c2da..d0f068a11f 100644 --- a/lib/public/views/Runs/format/displayRunEorReasons.js +++ b/lib/public/views/Runs/format/displayRunEorReasons.js @@ -12,7 +12,7 @@ */ import { h } from '/js/src/index.js'; -import { formatEorReason } from './formatEorReason.js'; +import { formatEorReason } from './formatEorReason.mjs'; /** * Display a list of EOR reasons diff --git a/lib/public/views/Runs/format/formatEorReason.js b/lib/public/views/Runs/format/formatEorReason.mjs similarity index 87% rename from lib/public/views/Runs/format/formatEorReason.js rename to lib/public/views/Runs/format/formatEorReason.mjs index a5491493ef..2cca2ba47a 100644 --- a/lib/public/views/Runs/format/formatEorReason.js +++ b/lib/public/views/Runs/format/formatEorReason.mjs @@ -14,7 +14,8 @@ /** * Format the given EoR reason to string * - * @param {Partial} eorReason the EoR reason to format + * @param {Partial<{category: string, title: string, description: string}>} eorReason the EoR reason to + * format * @return {string} the result */ export const formatEorReason = (eorReason) => { diff --git a/lib/public/views/Statistics/charts/eorReasonOccurrencesBarChartComponent.js b/lib/public/views/Statistics/charts/eorReasonOccurrencesBarChartComponent.js index b8589dc9a6..8560c8815f 100644 --- a/lib/public/views/Statistics/charts/eorReasonOccurrencesBarChartComponent.js +++ b/lib/public/views/Statistics/charts/eorReasonOccurrencesBarChartComponent.js @@ -13,7 +13,7 @@ import { ChartColors } from '../chartColors.js'; import { barChartComponent } from '../../../components/common/chart/barChart/barChartComponent.js'; -import { formatEorReason } from '../../Runs/format/formatEorReason.js'; +import { formatEorReason } from '../../Runs/format/formatEorReason.mjs'; /** * Bar chart displaying EoR reason occurrences diff --git a/lib/server/services/run/RunService.js b/lib/server/services/run/RunService.js index 2404db5236..d9251ae619 100644 --- a/lib/server/services/run/RunService.js +++ b/lib/server/services/run/RunService.js @@ -58,6 +58,8 @@ const { flpRoleService } = require('../flp/FlpRoleService.js'); * @property {boolean|{where: object}} [logs] if true, related logs will be fetched alongside the run */ +const EOR_REASON_CATEGORIES_TO_LOG = ['DETECTORS']; + /** * Create a log stating the detector's quality change * @@ -103,6 +105,54 @@ const logRunDetectorQualityChange = async (runNumber, runDetectors, transaction, } }; +/** + * Create a log to register run end of run reason + * + * @param {number} runNumber the run number of the run for which EoR reason has changed + * @param {string} userName the username of the user that changed the EoR reason + * @param {array<{category: string, title: string, description: string}>} eorReasons the new EoR reasons + * @param {object|null} transaction optionally the transaction in which the log creation must be wrapped + * @return {Promise} resolves once the log has been created + */ +const logEorReasonChange = async (runNumber, userName, eorReasons, transaction) => { + const { formatEorReason } = await import('../../../public/views/Runs/format/formatEorReason.mjs'); + + const headerParts = [`End of run reason for the run ${runNumber} has been changed`]; + + if (userName) { + headerParts.push(`by ${userName}`); + } + + // Use EoR reason's title as text, as it probably contains detector name + const tags = (await getTagsByText(eorReasons.map(({ title }) => title))).map(({ text }) => text); + + const textParts = [`${headerParts.join(' ')}.`]; + + textParts.push('The new EoR reasons are:'); + for (const eorReason of eorReasons) { + textParts.push(`- ${formatEorReason(eorReason)}`); + } + + const { error } = await createLog( + { + title: `EoR reason has changed for run ${runNumber}`, + text: textParts.join('\n'), + subtype: 'run', + origin: 'process', + }, + [runNumber], + tags, + [], + [], + [], + transaction, + ); + + if (error) { + // TODO log the failed log creation + } +}; + /** * Global service to handle runs instances */ @@ -303,7 +353,7 @@ class RunService { // Update EOR reasons if they are provided if (eorReasons) { - await updateEorReasonsOnRun(run.id, user?.name, eorReasons); + await updateEorReasonsOnRun(run.id, run.runNumber, user?.name, eorReasons, transaction); } // Update detector qualities if they are provided @@ -471,55 +521,92 @@ class RunService { } } +// eslint-disable-next-line valid-jsdoc /** - * Method to remove existing reason_type - run_id from `eor_reasons` table and insert new ones + * Update the EoR reasons of a given run * * @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 {EorReasonPatch[]} eorReasonPatches - list of eor_reasons patches to be updated on the RUN + * @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, userName, eorReasonPatches) => { +const updateEorReasonsOnRun = async (runId, runNumber, userName, eorReasonsPatches, transaction) => { const reasonTypes = await ReasonTypeRepository.findAll(); + const idsOfReasonTypesToLog = []; + + /** + * @type {Map} + */ + const reasonTypesMap = new Map(); + for (const reasonType of reasonTypes) { + // Index the reason type by its id + reasonTypesMap.set(reasonType.id, reasonType); + // Index the reason type by its category and title + reasonTypesMap.set(JSON.stringify([reasonType.category, reasonType.title]), reasonType); + + if (EOR_REASON_CATEGORIES_TO_LOG.includes(reasonType.category)) { + idsOfReasonTypesToLog.push(reasonType.id); + } + } /** - * @type {Map} + * @type {SequelizeEorReason[]} */ - const reasonTypesMap = new Map(reasonTypes - .map(({ id, category, title }) => [ - // Set in the same map possibility to index by reasonTypeId or by title/category - [id, id], - [JSON.stringify([category, title]), id], - ]) - // Convert the array of two values into 2 values in the main array - .flat()); - - const validPatches = []; - for (const patch of eorReasonPatches) { + const eorReasons = []; + for (const eorReasonPatch of eorReasonsPatches) { // Use reasonTypeId if it exists, otherwise use category+title if it exists to get the reasonTypeId - const reasonTypeId = reasonTypesMap.get(patch.reasonTypeId ?? JSON.stringify([patch.category, patch.title])) ?? null; - // Add to the validPatches array if valid, if the patch is invalid ignore it - if (reasonTypeId !== null) { + const reasonTypeKey = eorReasonPatch.reasonTypeId ?? JSON.stringify([eorReasonPatch.category, eorReasonPatch.title]); + const reasonType = reasonTypesMap.get(reasonTypeKey) ?? null; + // Add to the eorReasons array if valid, if the patch is invalid ignore it + if (reasonType !== null) { // Set the reasonTypeId for further processing - patch.reasonTypeId = reasonTypeId; - validPatches.push(patch); + eorReasons.push({ + id: eorReasonPatch.id, + reasonTypeId: reasonType.id, + description: eorReasonPatch.description, + }); } } + /** + * @type {number[]} + */ const toKeepEorReasonsIds = []; // EorReasons with an ID already, means exist in DB; + /** * @type {Partial[]} */ const newEorReasons = []; // EorReasons with no ID, need to be added in DB; - validPatches.forEach(({ id, reasonTypeId, description }) => { + let needLoggingForEorReason = false; + eorReasons.forEach(({ id, reasonTypeId, description }) => { if (id) { toKeepEorReasonsIds.push(id); } else { newEorReasons.push({ runId, reasonTypeId, description, lastEditedName: userName }); + + if (idsOfReasonTypesToLog.includes(reasonTypeId)) { + needLoggingForEorReason = true; + } } }); + await EorReasonRepository.removeByRunIdAndKeepIds(runId, toKeepEorReasonsIds); await EorReasonRepository.addMany(newEorReasons); + + if (needLoggingForEorReason) { + await logEorReasonChange( + runNumber, + userName, + eorReasons.map(({ reasonTypeId, description }) => { + const eorReasonType = reasonTypesMap.get(reasonTypeId) ?? {}; + return { category: eorReasonType.category, title: eorReasonType.title, description }; + }), + transaction, + ); + } }; /** diff --git a/test/lib/server/services/run/RunService.test.js b/test/lib/server/services/run/RunService.test.js index 344b8fd36b..c4032fec9b 100644 --- a/test/lib/server/services/run/RunService.test.js +++ b/test/lib/server/services/run/RunService.test.js @@ -245,6 +245,9 @@ module.exports = () => { { runPatch: { calibrationStatus: RunCalibrationStatus.FAILED }, metadata: { calibrationStatusChangeReason: 'A reason' } }, ); + // A new log has been created + lastLogId++; + const run = await getRun({ runNumber }); expect(run.calibrationStatus).to.equal(RunCalibrationStatus.FAILED); @@ -254,6 +257,34 @@ module.exports = () => { ); }); + it('should successfully create a log when updating EoR reason to DETECTOR', async () => { + const runNumber = 1; + + await runService.update( + { runNumber }, + { relations: { eorReasons: [{ reasonTypeId: 2, description: 'A description' }] } }, + ); + + const lastLog = await getLog(++lastLogId, (qb) => { + qb.include('tags'); + }); + expect(lastLog.title).to.equal('EoR reason has changed for run 1'); + expect(lastLog.text).to.equal('End of run reason for the run 1 has been changed.\nThe new EoR reasons are:' + + '\n- DETECTORS - TPC - A description'); + expect(lastLog.tags).to.lengthOf(0); + }); + + it('should successfully NOT create a log when updating EoR reason for something else than DETECTOR', async () => { + const runNumber = 1; + + await runService.update( + { runNumber }, + { relations: { eorReasons: [{ reasonTypeId: 3, description: 'A description' }] } }, + ); + + expect(await getLog(lastLogId + 1, (qb) => qb.include('tags'))).to.be.null; + }); + it('should successfully consider current patch to allow/disallow calibration status update', async () => { const runNumber = 106; let run = await getRun({ runNumber }); diff --git a/test/public/runs/detail.test.js b/test/public/runs/detail.test.js index 5451a71951..79b1f3dab9 100644 --- a/test/public/runs/detail.test.js +++ b/test/public/runs/detail.test.js @@ -191,7 +191,7 @@ module.exports = () => { await page.waitForSelector('#Run-eorReasons select'); await page.select('#Run-eorReasons select', 'DETECTORS'); - await page.waitForSelector('#Run-eorReasons select:nth-child(2)'); + await page.waitForSelector('#Run-eorReasons select:nth-child(2) option:nth-of-type(2)'); await page.select('#Run-eorReasons select:nth-child(2)', 'CPV'); await page.type('#Run-eorReasons input', 'A new EOR reason'); await pressElement(page, '#add-eor-reason', true); @@ -204,7 +204,6 @@ module.exports = () => { await pressElement(page, '#save-run'); await page.waitForSelector('#edit-run'); - await waitForTableLength(page, 5); const eorReasons = await page.$$('#Run-eorReasons .eor-reason'); expect(eorReasons).to.lengthOf(2); @@ -231,7 +230,6 @@ module.exports = () => { await pressElement(page, '#cancel-run'); await page.waitForSelector('#save-run', { hidden: true }); - await waitForTableLength(page, 5); const eorReasons = await page.$$('#Run-eorReasons .eor-reason'); expect(eorReasons).to.lengthOf(2); @@ -296,7 +294,8 @@ module.exports = () => { }); it('can navigate to a log detail page', async () => { - await waitForTableLength(page, 5); + // Lengh of 6 because of the test to change EoR reason which creates a log + await waitForTableLength(page, 6); // We expect the entry page to have the same id as the id from the run overview await waitForNavigation(page, () => pressElement(page, '#row1 .btn-redirect')); diff --git a/test/public/runs/overview.test.js b/test/public/runs/overview.test.js index 13d57a6789..96ccef4408 100644 --- a/test/public/runs/overview.test.js +++ b/test/public/runs/overview.test.js @@ -941,9 +941,8 @@ module.exports = () => { expect(eorDescriptionInput).to.exist; // Expect there to be one result that contains a certain description - await page.focus('#eorDescription'); const descriptionInput = 'some'; - await page.keyboard.type(descriptionInput); + await fillInput(page, '#eorDescription', descriptionInput); await waitForTableLength(page, 2); let eorReasons = await page.$$('table td[id$="eorReasons"]');