Skip to content

Commit

Permalink
[O2B-955] Create a log when EoR reason DETECTOR is chosen (#1742)
Browse files Browse the repository at this point in the history
* [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
  • Loading branch information
martinboulais authored Sep 3, 2024
1 parent d7d914c commit a999dbc
Show file tree
Hide file tree
Showing 8 changed files with 149 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/public/views/Runs/format/displayRunEorReasons.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
/**
* Format the given EoR reason to string
*
* @param {Partial<EorReason>} 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) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
131 changes: 109 additions & 22 deletions lib/server/services/run/RunService.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down Expand Up @@ -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<void>} 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
*/
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<undefined|Error>} - 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<number|string, SequelizeReasonType>}
*/
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<number|string, number>}
* @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<SequelizeEorReason>[]}
*/
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,
);
}
};

/**
Expand Down
31 changes: 31 additions & 0 deletions test/lib/server/services/run/RunService.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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 });
Expand Down
7 changes: 3 additions & 4 deletions test/public/runs/detail.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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'));
Expand Down
3 changes: 1 addition & 2 deletions test/public/runs/overview.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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"]');
Expand Down

0 comments on commit a999dbc

Please sign in to comment.