From a812ab022e910ba4f81159a68ed3d890848af7ec Mon Sep 17 00:00:00 2001 From: Isaac Hill <71404865+isaachilly@users.noreply.github.com> Date: Fri, 16 Jan 2026 15:08:39 +0100 Subject: [PATCH 1/3] [O2B-1512] Refactor environment filtering tests into a single case Consolidated multiple granular filtering tests into one comprehensive test that applies all filters in sequence. This streamlines the test suite. --- test/public/envs/overview.test.js | 164 +++--------------------------- 1 file changed, 12 insertions(+), 152 deletions(-) diff --git a/test/public/envs/overview.test.js b/test/public/envs/overview.test.js index 1b9fa872c6..f4613bbe8f 100644 --- a/test/public/envs/overview.test.js +++ b/test/public/envs/overview.test.js @@ -279,7 +279,7 @@ module.exports = () => { expect(loadCallCount).to.equal(0); }); - it('should successfully open the filtering panel', async () => { + it('should successfully filter environments utilising all filters in the process', async () => { // Get the popover key from the filter button's parent const filterButton = await page.waitForSelector('#openFilterToggle'); const popoverKey = await filterButton.evaluate((button) => { @@ -287,170 +287,30 @@ module.exports = () => { }); const filterPanelSelector = `.popover[data-popover-key="${popoverKey}"]`; - // Initially the filtering panel should be closed await page.waitForSelector(filterPanelSelector, { hidden: true }); - // Open the filtering panel await openFilteringPanel(page); await page.waitForSelector(filterPanelSelector, { visible: true }); - }); - - it('should successfully filter environments by their related run numbers', async () => { - /** - * This is the sequence to test filtering the environments based on their related run numbers. - * - * @param {string} selector the filter input selector - * @param {string} inputValue the value to type in the filter input - * @param {string[]} expectedIds the list of expected environment IDs after filtering - * @return {void} - */ - const filterOnRunNumbers = async (selector, inputValue, expectedIds) => { - await fillInput(page, selector, inputValue, ['change']); - await waitForTableLength(page, expectedIds.length); - expect(await page.$$eval('tbody tr', (rows) => rows.map((row) => row.id))).to.eql(expectedIds.map(id => `row${id}`)); - } + await expectAttributeValue(page, '.id-filter input', 'placeholder', 'e.g. CmCvjNbg, TDI59So3d...'); await expectAttributeValue(page, '.runs-filter input', 'placeholder', 'e.g. 553203, 553221, ...'); - - await filterOnRunNumbers('.runs-filter input', '10', ['TDI59So3d', 'Dxi029djX']); - await resetFilters(page); - - await filterOnRunNumbers('.runs-filter input', '103', ['TDI59So3d']); - await resetFilters(page); - - await filterOnRunNumbers('.runs-filter input', '86, 91', ['KGIS12DS', 'VODdsO12d']); - await resetFilters(page); - }); - - it('should successfully filter environments by their status history', async () => { - /** - * This is the sequence to test filtering the environments on their status history. - * - * @param {string} selector the filter input selector - * @param {string} inputValue the value to type in the filter input - * @param {string[]} expectedIds the list of expected environment IDs after filtering - * @return {void} - */ - const filterOnStatusHistory = async (selector, inputValue, expectedIds) => { - await fillInput(page, selector, inputValue, ['change']); - await waitForTableLength(page, expectedIds.length); - expect(await page.$$eval('tbody tr', (rows) => rows.map((row) => row.id))).to.eql(expectedIds.map(id => `row${id}`)); - }; - await expectAttributeValue(page, '.historyItems-filter input', 'placeholder', 'e.g. D-R-X'); - await filterOnStatusHistory('.historyItems-filter input', 'C-R-D-X', ['TDI59So3d']); - await resetFilters(page); - - await filterOnStatusHistory('.historyItems-filter input', 'S-E', ['EIDO13i3D', '8E4aZTjY']); - await resetFilters(page); - - await filterOnStatusHistory('.historyItems-filter input', 'D-E', ['KGIS12DS']); - await resetFilters(page); - }); - - it('should successfully filter environments by their current status', async () => { - /** - * Checks that all the rows of the given table have a valid current status - * - * @param {string[]} authorizedCurrentStatuses the list of valid current statuses - * @return {void} - */ - const checkTableCurrentStatuses = async (authorizedCurrentStatuses) => { - const rows = await page.$$('tbody tr'); - for (const row of rows) { - expect(await row.evaluate((rowItem) => { - const rowId = rowItem.id; - return document.querySelector(`#${rowId}-status-text`).innerText; - })).to.be.oneOf(authorizedCurrentStatuses); - } - }; - - const currentStatusSelectorPrefix = '.status-filter #checkboxes-checkbox-'; - const getCurrentStatusCheckboxSelector = (statusName) => `${currentStatusSelectorPrefix}${statusName}`; + await fillInput(page, '.id-filter input', 'Dxi029djX, TDI59So3d', ['change']); + await page.$eval('.status-filter #checkboxes-checkbox-DESTROYED', (element) => element.click()); + await fillInput(page, '.runs-filter input', '10', ['change']); + await fillInput(page, '.historyItems-filter input', 'C-R-D-X', ['change']); - await page.$eval(getCurrentStatusCheckboxSelector("RUNNING"), (element) => element.click()); - await waitForTableLength(page, 2); - await checkTableCurrentStatuses(["RUNNING"]); - - await page.$eval(getCurrentStatusCheckboxSelector("DEPLOYED"), (element) => element.click()); - await waitForTableLength(page, 3); - await checkTableCurrentStatuses(["RUNNING", "DEPLOYED"]); - }); - - it('should successfully filter environments by their IDs', async () => { - /** - * This is the sequence to test filtering the environments on IDs. - * - * @param {string} selector the filter input selector - * @param {string} inputValue the value to type in the filter input - * @param {string[]} expectedIds the list of expected environment IDs after filtering - * @return {void} - */ - const filterOnID = async (selector, inputValue, expectedIds) => { - await fillInput(page, selector, inputValue, ['change']); - await waitForTableLength(page, expectedIds.length); - expect(await page.$$eval('tbody tr', (rows) => rows.map((row) => row.id))).to.eql(expectedIds.map(id => `row${id}`)); - }; - - await expectAttributeValue(page, '.id-filter input', 'placeholder', 'e.g. CmCvjNbg, TDI59So3d...'); - - await filterOnID('.id-filter input', 'CmCvjNbg', ['CmCvjNbg']); - await resetFilters(page); - - await filterOnID('.id-filter input', 'CmCvjNbg, TDI59So3d', ['CmCvjNbg', 'TDI59So3d']); - await resetFilters(page); - - await filterOnID('.id-filter input', 'j', ['CmCvjNbg', 'GIDO1jdkD', '8E4aZTjY', 'Dxi029djX']); - await resetFilters(page); - }); - - it('should successfully filter environments by their createdAt date', async () => { - /** - * This is the sequence to test filtering the environments based on their createdAt date - * - * @param {string} selector the filter input selector - * @param {string} fromDate the from date string - * @param {string} fromTime the from time string - * @param {string} toDate the to date string - * @param {string} toTime the to time string - * @param {string[]} expectedIds the list of expected environment IDs after filtering - * @return {void} - */ - const filterOnCreatedAt = async (selector, fromDate, fromTime, toDate, toTime, expectedIds) => { - await fillInput(page, selector.fromTimeSelector, fromTime, ['change']); - await fillInput(page, selector.toTimeSelector, toTime, ['change']); - - await fillInput(page, selector.fromDateSelector, fromDate, ['change']); - await fillInput(page, selector.toDateSelector, toDate, ['change']); - - await waitForTableLength(page, expectedIds.length); - expect(await page.$$eval('tbody tr', (rows) => rows.map((row) => row.id))).to.eql(expectedIds.map(id => `row${id}`)); - }; - - await openFilteringPanel(page); - const createdAtPopoverSelector = await getPopoverSelector(await page.$('.createdAt-filter .popover-trigger')); const periodInputsSelectors = getPeriodInputsSelectors(createdAtPopoverSelector); + await fillInput(page, periodInputsSelectors.fromDateSelector, '2019-08-09', ['change']); + await fillInput(page, periodInputsSelectors.toDateSelector, '2019-08-10', ['change']); + await fillInput(page, periodInputsSelectors.fromTimeSelector, '00:00', ['change']); + await fillInput(page, periodInputsSelectors.toTimeSelector, '23:59', ['change']); - await filterOnCreatedAt( - periodInputsSelectors, - '2019-05-08', - '00:00', - '2019-05-10', - '00:00', - ['eZF99lH6'], - ); - await resetFilters(page); + await waitForTableLength(page, 1); + expect(await page.$$eval('tbody tr', (rows) => rows.map((row) => row.id))).to.eql(['rowTDI59So3d']); - await filterOnCreatedAt( - periodInputsSelectors, - '2019-08-09', - '00:00', - '2019-08-09', - '14:00', - ['GIDO1jdkD', '8E4aZTjY', 'Dxi029djX'], - ); await resetFilters(page); }); }; From 4a9810279543dfbe0c9f8e35271feb6d6552a5c6 Mon Sep 17 00:00:00 2001 From: Isaac Hill <71404865+isaachilly@users.noreply.github.com> Date: Fri, 16 Jan 2026 17:15:49 +0100 Subject: [PATCH 2/3] [O2B-1520] Enhance runNumbers filter with range support Updated GetAllEnvironmentsDto and GetAllEnvironmentsUseCase to support runNumbers filtering using ranges. Added range to large filter test case. --- lib/domain/dtos/GetAllEnvironmentsDto.js | 5 ++++- .../environment/GetAllEnvironmentsUseCase.js | 14 +++++++++----- test/public/envs/overview.test.js | 8 +++++++- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/lib/domain/dtos/GetAllEnvironmentsDto.js b/lib/domain/dtos/GetAllEnvironmentsDto.js index 8eb1cfba85..2c5e96bffa 100644 --- a/lib/domain/dtos/GetAllEnvironmentsDto.js +++ b/lib/domain/dtos/GetAllEnvironmentsDto.js @@ -14,6 +14,7 @@ const Joi = require('joi'); const PaginationDto = require('./PaginationDto'); const { FromToFilterDto } = require('./filters/FromToFilterDto'); +const { validateRange } = require('../../utilities/rangeUtils.js'); /** * Separate filter DTO for get all environments, because EnvironmentsFilterDto @@ -23,7 +24,9 @@ const FilterDto = Joi.object({ ids: Joi.string().trim().optional(), currentStatus: Joi.string().trim().optional(), statusHistory: Joi.string().trim().optional(), - runNumbers: Joi.string().trim().optional(), + runNumbers: Joi.string().trim().custom(validateRange).messages({ + 'any.invalid': '{{#message}}', + }).optional(), created: FromToFilterDto, }); diff --git a/lib/usecases/environment/GetAllEnvironmentsUseCase.js b/lib/usecases/environment/GetAllEnvironmentsUseCase.js index fd01f05813..c742c53b62 100644 --- a/lib/usecases/environment/GetAllEnvironmentsUseCase.js +++ b/lib/usecases/environment/GetAllEnvironmentsUseCase.js @@ -21,6 +21,8 @@ const { ApiConfig } = require('../../config/index.js'); const { Op } = require('sequelize'); const { dataSource } = require('../../database/DataSource.js'); const { statusAcronyms } = require('../../domain/enums/StatusAcronyms.js'); +const { unpackNumberRange } = require('../../utilities/rangeUtils.js'); +const { splitStringToStringsTrimmed } = require('../../utilities/stringUtils.js'); /** * Subquery to select the latest history item for each environment. @@ -182,19 +184,21 @@ class GetAllEnvironmentsUseCase { } if (runNumbersExpression) { - // Convert the string of run numbers to an array of numbers - const filters = runNumbersExpression.split(',').map((filter) => Number(filter.trim())); + const runNumberCriteria = splitStringToStringsTrimmed(runNumbersExpression, ','); - if (filters.length) { + const finalRunNumberList = Array.from(unpackNumberRange(runNumberCriteria)); + + // Check that the final run numbers list contains at least one valid run number + if (finalRunNumberList.length > 0) { filterQueryBuilder.include({ association: 'runs', where: { // Filter should be like with only one filter and exact with more than one filter - runNumber: { [filters.length === 1 ? Op.substring : Op.in]: filters }, + runNumber: { [finalRunNumberList.length === 1 ? Op.substring : Op.in]: finalRunNumberList }, }, }); } - } + }; const filteredEnvironmentsIds = (await EnvironmentRepository.findAll(filterQueryBuilder)).map(({ id }) => id); // If no environments match the filter, return an empty result diff --git a/test/public/envs/overview.test.js b/test/public/envs/overview.test.js index f4613bbe8f..73eecd9a4f 100644 --- a/test/public/envs/overview.test.js +++ b/test/public/envs/overview.test.js @@ -296,9 +296,15 @@ module.exports = () => { await expectAttributeValue(page, '.runs-filter input', 'placeholder', 'e.g. 553203, 553221, ...'); await expectAttributeValue(page, '.historyItems-filter input', 'placeholder', 'e.g. D-R-X'); + // range of runNumbers + await fillInput(page, '.runs-filter input', '103-104', ['change']); + await waitForTableLength(page, 1); + // substring of a runNumber + await fillInput(page, '.runs-filter input', '10', ['change']); + await fillInput(page, '.id-filter input', 'Dxi029djX, TDI59So3d', ['change']); await page.$eval('.status-filter #checkboxes-checkbox-DESTROYED', (element) => element.click()); - await fillInput(page, '.runs-filter input', '10', ['change']); + await fillInput(page, '.historyItems-filter input', 'C-R-D-X', ['change']); const createdAtPopoverSelector = await getPopoverSelector(await page.$('.createdAt-filter .popover-trigger')); From 5d612c0c485e52857990497f93ad3eb3304c1a7f Mon Sep 17 00:00:00 2001 From: Isaac Hill <71404865+isaachilly@users.noreply.github.com> Date: Wed, 21 Jan 2026 16:22:05 +0100 Subject: [PATCH 3/3] [O2B-1520] Improve range validation and error handling in DTOs Refactored range validation logic to use a custom error code (RANGE_INVALID) and improved error messages for invalid input formats in DTOs. Enhanced rangeUtils to handle empty values, multiple dashes, and non-numeric input more robustly. Updated related tests and API validation to reflect new error handling and messaging. --- lib/domain/dtos/GetAllEnvironmentsDto.js | 5 ++- lib/domain/dtos/filters/LhcFillsFilterDto.js | 5 ++- lib/domain/dtos/filters/RunFilterDto.js | 5 ++- lib/utilities/rangeUtils.js | 39 +++++++++++++------- test/api/environments.test.js | 11 ++++++ test/lib/utilities/rangeUtils.test.js | 4 +- 6 files changed, 48 insertions(+), 21 deletions(-) diff --git a/lib/domain/dtos/GetAllEnvironmentsDto.js b/lib/domain/dtos/GetAllEnvironmentsDto.js index 2c5e96bffa..e1ebbd60f2 100644 --- a/lib/domain/dtos/GetAllEnvironmentsDto.js +++ b/lib/domain/dtos/GetAllEnvironmentsDto.js @@ -14,7 +14,7 @@ const Joi = require('joi'); const PaginationDto = require('./PaginationDto'); const { FromToFilterDto } = require('./filters/FromToFilterDto'); -const { validateRange } = require('../../utilities/rangeUtils.js'); +const { validateRange, RANGE_INVALID } = require('../../utilities/rangeUtils.js'); /** * Separate filter DTO for get all environments, because EnvironmentsFilterDto @@ -25,7 +25,8 @@ const FilterDto = Joi.object({ currentStatus: Joi.string().trim().optional(), statusHistory: Joi.string().trim().optional(), runNumbers: Joi.string().trim().custom(validateRange).messages({ - 'any.invalid': '{{#message}}', + [RANGE_INVALID]: '{{#message}}', + 'string.base': 'Run numbers must be comma-separated numbers or ranges (e.g. 12,15-18)', }).optional(), created: FromToFilterDto, }); diff --git a/lib/domain/dtos/filters/LhcFillsFilterDto.js b/lib/domain/dtos/filters/LhcFillsFilterDto.js index d1d2af5929..3f36fe0820 100644 --- a/lib/domain/dtos/filters/LhcFillsFilterDto.js +++ b/lib/domain/dtos/filters/LhcFillsFilterDto.js @@ -11,11 +11,12 @@ * or submit itself to any jurisdiction. */ const Joi = require('joi'); -const { validateRange } = require('../../../utilities/rangeUtils'); +const { validateRange, RANGE_INVALID } = require('../../../utilities/rangeUtils'); exports.LhcFillsFilterDto = Joi.object({ hasStableBeams: Joi.boolean(), fillNumbers: Joi.string().trim().custom(validateRange).messages({ - 'any.invalid': '{{#message}}', + [RANGE_INVALID]: '{{#message}}', + 'string.base': 'Fill numbers must be comma-separated numbers or ranges (e.g. 12,15-18)', }), }); diff --git a/lib/domain/dtos/filters/RunFilterDto.js b/lib/domain/dtos/filters/RunFilterDto.js index 0feda0ddbc..2dc9ce98ad 100644 --- a/lib/domain/dtos/filters/RunFilterDto.js +++ b/lib/domain/dtos/filters/RunFilterDto.js @@ -18,7 +18,7 @@ const { IntegerComparisonDto, FloatComparisonDto } = require('./NumericalCompari const { RUN_CALIBRATION_STATUS } = require('../../enums/RunCalibrationStatus.js'); const { RUN_DEFINITIONS } = require('../../enums/RunDefinition.js'); const { singleRunsCollectionCustomCheck } = require('../utils.js'); -const { validateRange } = require('../../../utilities/rangeUtils.js'); +const { validateRange, RANGE_INVALID } = require('../../../utilities/rangeUtils.js'); const DetectorsFilterDto = Joi.object({ operator: Joi.string().valid('or', 'and', 'none').required(), @@ -33,7 +33,8 @@ const EorReasonFilterDto = Joi.object({ exports.RunFilterDto = Joi.object({ runNumbers: Joi.string().trim().custom(validateRange).messages({ - 'any.invalid': '{{#message}}', + [RANGE_INVALID]: '{{#message}}', + 'string.base': 'Run numbers must be comma-separated numbers or ranges (e.g. 12,15-18)', }), calibrationStatuses: Joi.array().items(...RUN_CALIBRATION_STATUS), definitions: CustomJoi.stringArray().items(Joi.string().uppercase().trim().valid(...RUN_DEFINITIONS)), diff --git a/lib/utilities/rangeUtils.js b/lib/utilities/rangeUtils.js index 4cc9a385de..dea1155878 100644 --- a/lib/utilities/rangeUtils.js +++ b/lib/utilities/rangeUtils.js @@ -19,30 +19,43 @@ * @param {*} helpers The helpers object * @returns {Object} The value if validation passes */ +export const RANGE_INVALID = 'range.invalid'; + export const validateRange = (value, helpers) => { const MAX_RANGE_SIZE = 100; const numbers = value.split(',').map((number) => number.trim()); for (const number of numbers) { + // Check for empty strings (e.g., from trailing commas or double commas) + if (number === '') { + return helpers.error(RANGE_INVALID, { message: 'Empty value found' }); + } + if (number.includes('-')) { // Check if '-' occurs more than once in this part of the range if (number.lastIndexOf('-') !== number.indexOf('-')) { - return helpers.error('any.invalid', { message: `Invalid range: ${number}` }); + return helpers.error(RANGE_INVALID, { message: `Invalid range: ${number}` }); + } + const parts = number.split('-'); + // Ensure exactly 2 parts and both are non-empty + if (parts.length !== 2 || parts[0].trim() === '' || parts[1].trim() === '') { + return helpers.error(RANGE_INVALID, { message: `Invalid range: ${number}` }); } - const [start, end] = number.split('-').map((n) => Number(n)); + const [start, end] = parts.map((n) => Number(n)); if (Number.isNaN(start) || Number.isNaN(end) || start > end) { - return helpers.error('any.invalid', { message: `Invalid range: ${number}` }); + return helpers.error(RANGE_INVALID, { message: `Invalid range: ${number}` }); } const rangeSize = end - start + 1; if (rangeSize > MAX_RANGE_SIZE) { - return helpers.error('any.invalid', { message: `Given range exceeds max size of ${MAX_RANGE_SIZE} range: ${number}` }); + return helpers.error(RANGE_INVALID, { message: `Given range exceeds max size of ${MAX_RANGE_SIZE} range: ${number}` }); } } else { - // Prevent non-numeric input. - if (isNaN(number)) { - return helpers.error('any.invalid', { message: `Invalid number: ${number}` }); + // Single number - prevent non-numeric input using Number.isNaN for consistency + const num = Number(number); + if (Number.isNaN(num)) { + return helpers.error(RANGE_INVALID, { message: `Invalid number: ${number}` }); } } } @@ -58,20 +71,20 @@ export const validateRange = (value, helpers) => { * @returns {Set} set containing the unpacked range. */ export function unpackNumberRange(numbersRanges, rangeSplitter = '-') { - // Set to prevent duplicate values. const resultNumbers = new Set(); numbersRanges.forEach((number) => { if (number.includes(rangeSplitter)) { - const [start, end] = number.split(rangeSplitter).map((n) => parseInt(n, 10)); - if (!Number.isNaN(start) && !Number.isNaN(end)) { + const [start, end] = number.split(rangeSplitter).map((n) => Number(n)); + if (!Number.isNaN(start) && !Number.isNaN(end) && start <= end) { for (let i = start; i <= end; i++) { - resultNumbers.add(Number(i)); + resultNumbers.add(i); } } } else { - if (!isNaN(number)) { - resultNumbers.add(Number(number)); + const num = Number(number); + if (!Number.isNaN(num)) { + resultNumbers.add(num); } } }); diff --git a/test/api/environments.test.js b/test/api/environments.test.js index 770df255a9..201c31a94e 100644 --- a/test/api/environments.test.js +++ b/test/api/environments.test.js @@ -225,6 +225,17 @@ module.exports = () => { expect(environments[0].id).to.be.equal('TDI59So3d'); expect(environments[1].id).to.be.equal('Dxi029djX'); }); + + it('should successfully filter environments with query on run number range', async () => { + const response = await request(server).get('/api/environments?filter[runNumbers]=100-105'); + + expect(response.status).to.equal(200); + const environments = response.body.data; + expect(environments.length).to.be.equal(2); + // Should include all environments with run numbers between 100 and 105 + expect(environments[0].id).to.be.equal('TDI59So3d'); + expect(environments[1].id).to.be.equal('Dxi029djX'); + }); }); describe('POST /api/environments', () => { it('should return 201 if valid data is provided', async () => { diff --git a/test/lib/utilities/rangeUtils.test.js b/test/lib/utilities/rangeUtils.test.js index 509db85a4f..a7802888ca 100644 --- a/test/lib/utilities/rangeUtils.test.js +++ b/test/lib/utilities/rangeUtils.test.js @@ -12,7 +12,7 @@ */ const Sinon = require('sinon'); -const { validateRange, unpackNumberRange } = require('../../../lib/utilities/rangeUtils.js'); +const { validateRange, unpackNumberRange, RANGE_INVALID } = require('../../../lib/utilities/rangeUtils.js'); const { expect } = require('chai'); module.exports = () => { @@ -65,7 +65,7 @@ module.exports = () => { const input = '5,a,7'; validateRange(input, helpers); expect(helpers.error.calledOnce).to.be.true; - expect(helpers.error.firstCall.args[0]).to.equal('any.invalid'); + expect(helpers.error.firstCall.args[0]).to.equal(RANGE_INVALID); expect(helpers.error.firstCall.args[1]).to.deep.equal({ message: 'Invalid number: a' }); });