Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[O2B-643] Run duration accept format HH:MM:SS #1464

Draft
wants to merge 56 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
355ddac
works
xsalonx Mar 14, 2024
9702196
works
xsalonx Mar 14, 2024
4b87ce8
refactor
xsalonx Mar 14, 2024
d436308
add model
xsalonx Mar 14, 2024
eda937b
nEPS filter works
xsalonx Mar 15, 2024
8c18794
filters work
xsalonx Mar 15, 2024
15b5dac
filter works
xsalonx Mar 15, 2024
c3b2f86
pol
xsalonx Mar 15, 2024
e9e7420
pol
xsalonx Mar 15, 2024
5e98f04
linter
xsalonx Mar 15, 2024
3cbb87f
add default
xsalonx Mar 15, 2024
1a61b69
docs
xsalonx Mar 15, 2024
2d1731a
conf min max
xsalonx Mar 15, 2024
2475d19
docs
xsalonx Mar 15, 2024
b37573a
value
xsalonx Mar 15, 2024
78950b5
change default
xsalonx Mar 15, 2024
7f03dff
test WIP
xsalonx Mar 15, 2024
58d31e5
Test
xsalonx Mar 15, 2024
191d434
Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…
xsalonx Mar 18, 2024
fbdefc0
Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…
xsalonx Mar 25, 2024
3661588
remove override
xsalonx Mar 27, 2024
a06640d
refactor
xsalonx Mar 27, 2024
21c1cf4
use SelectionMode
xsalonx Mar 27, 2024
0ab6860
rename
xsalonx Mar 27, 2024
144317c
rename
xsalonx Mar 27, 2024
40b70e2
bug
xsalonx Mar 27, 2024
68a01d7
simp
xsalonx Mar 27, 2024
ff0da59
cherry-picked
xsalonx Mar 27, 2024
7d57685
cherry -pick
xsalonx Mar 27, 2024
71cb30d
rm
xsalonx Mar 27, 2024
14fc5a8
cleanup
xsalonx Mar 27, 2024
6dda2bc
cl
xsalonx Mar 27, 2024
e639db8
revoke
xsalonx Mar 27, 2024
1d5e0c7
remove
xsalonx Mar 27, 2024
cd9d679
ref
xsalonx May 30, 2024
c4c9fc7
ch
xsalonx May 30, 2024
b59e0d2
add model
xsalonx May 30, 2024
a4933f2
ref WIP
xsalonx May 30, 2024
5d6c941
ref
xsalonx May 30, 2024
fe67005
fix
xsalonx May 30, 2024
f165e23
fix
xsalonx May 30, 2024
52e7bf6
merge main
xsalonx May 30, 2024
318d0a5
cleanup
xsalonx May 30, 2024
7d05396
test WIP
xsalonx May 30, 2024
a4843f3
merge main
xsalonx Jun 3, 2024
6b013dd
fix
xsalonx Jun 3, 2024
c353f54
fix
xsalonx Jun 3, 2024
73425be
fix
xsalonx Jun 4, 2024
c4c70dd
Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…
xsalonx Jun 4, 2024
2507a4a
simp
xsalonx Jun 4, 2024
e6763c6
merge main
xsalonx Jun 4, 2024
a908084
Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…
xsalonx Jun 4, 2024
bf412c4
add method
xsalonx Jun 4, 2024
2ea7b4a
merge main
xsalonx Jun 10, 2024
76905be
Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…
xsalonx Jun 11, 2024
eb7ae9f
merg main
xsalonx Oct 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 26 additions & 18 deletions lib/public/components/Filters/RunsFilter/durationFilter.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,49 +15,57 @@ import { h } from '/js/src/index.js';
import { comparisonOperatorFilter } from '../common/filters/comparisonOperatorFilter.js';

/**
* Returns the run duration filter component
* Returns the duration filter component
*
* @param {DurationInputModel} durationFilterModel the duration input model
* @param {ComparisonOperatorSelectionModel} operatorSelectionModel the comparison operator selection model
* @param {DurationFilterModel} durationFilterModel the duration filter model
* @return {Component} the duration filter
*/
export const durationFilter = (durationFilterModel, operatorSelectionModel) => {
const { hours, minutes, seconds } = durationFilterModel.raw;
export const durationFilter = (durationFilterModel) => {
const { durationInputModel, operatorSelectionModel } = durationFilterModel;

const { hours, minutes, seconds } = durationInputModel.raw;

/**
* Return oninput handler for given time unit
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mention what the function should be used for, only explain what it does.

*
* @param {'hours'|'minutes'|'seconds'} unitName time unit
* @return {function(InputEvent, void)} oninput handler for input for given time unit
*/
const updateInputModelHandlerByTimeUnit = (unitName) => (e) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function seems to add more complexity than it brings value I think

const { value } = e.target;
const parsedValue = Number(value);
if (value.length > 0 || 0 <= parsedValue && (unitName === 'hours' || parsedValue < 60)) {
durationInputModel.update({ [unitName]: value.length > 0 ? parsedValue : null });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing so, if you delete a value, update will not be triggered, so it will be put back to previous value.
Plus, this kind of processing should be in the model.
What you do in oninput is simply call durationFilterModel update, pass it RAW value (i.e. the string input), you store this raw value and try to extract the actual value.
If user put 80 minutes you still need to keep this as raw value but mark the input as invalid, and not update the actual value.
Please have a look at other inputs, such as DateTimeInputComponent

} else {
durationFilterModel.visualChange$.notify();
}
};

const hoursInput = h('input.flex-grow', {
id: 'hours-input',
type: 'number',
min: 0,
value: hours,
oninput: (e) => {
const { value } = e.target;
durationFilterModel.update({ hours: value.length === 0 ? null : Number(value) });
},
oninput: updateInputModelHandlerByTimeUnit('hours'),
});
const minutesInput = h('input.flex-grow', {
id: 'minutes-input',
type: 'number',
min: 0,
max: 59,
value: minutes,
oninput: (e) => {
const { value } = e.target;
durationFilterModel.update({ minutes: value.length === 0 ? null : Number(value) });
},
oninput: updateInputModelHandlerByTimeUnit('minutes'),
}, 'm');
const secondsInput = h('input.flex-grow', {
id: 'seconds-input',
type: 'number',
min: 0,
max: 59,
value: seconds,
oninput: (e) => {
const { value } = e.target;
durationFilterModel.update({ seconds: value.length === 0 ? null : Number(value) });
},
oninput: updateInputModelHandlerByTimeUnit('seconds'),
}, 's');

const inputs = h('.flex-row.w-40', [
const inputs = h('.flex-row.w-100', [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is w-100 usefull? Can't we use flex-grow or equivalent instead?

hoursInput,
minutesInput,
secondsInput,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/**
* @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.
*/

import { Observable } from '/js/src/index.js';

import { DurationInputModel } from '../../../common/form/inputs/DurationInputModel.js';
import { ComparisonOperatorSelectionModel } from './ComparisonOperatorSelectionModel.js';

/**
* Duration filter model which stores time value and selected operator
*/
export class DurationFilterModel extends Observable {
/**
* Constructor
*/
constructor() {
super();
this._visualChange$ = new Observable();

this._durationInputModel = new DurationInputModel();
this._durationInputModel.bubbleTo(this);
this._operatorSelectionModel = new ComparisonOperatorSelectionModel();
this._operatorSelectionModel.observe(() => {
if (this._durationInputModel.value === null) {
this._visualChange$.notify();
} else {
this.notify();
}
});
Comment on lines +33 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not how it should work I suppose. If operatorSelectionModel has visualChange you need to bubleUp this to the current visualChange, but not mix the global notify and the visual change

}

/**
* Returns the observable notified any time there is a visual change which has no impact on the actual filter value
*
* @return {Observable} the observable
*/
get visualChange$() {
return this._visualChange$;
}

/**
* Retrun duration input model
*
* @return {DurationInputModel} duration input model
*/
get durationInputModel() {
return this._durationInputModel;
}

/**
* Return operator selection model
*
* @return {ComparisonOperatorSelectionModel} operator selection model
*/
get operatorSelectionModel() {
return this._operatorSelectionModel;
}

/**
* States if the filter has been filled
*
* @return {boolean} true if the filter has been filled
*/
isEmpty() {
return this._durationInputModel.value === null;
}

/**
* Reset the filter to its default value
*
* @return {void}
*/
reset() {
this._durationInputModel.reset();
this._operatorSelectionModel.reset();
}
}
20 changes: 10 additions & 10 deletions lib/public/components/common/form/inputs/DurationInputModel.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import { Observable } from '/js/src/index.js';
/**
* @typedef DurationInputRawData
* @property {number} hours the number of hours
* @property {number} minutes the number of minutes
* @property {number} seconds the number of seconds
* @property {number} minutes the number of minutes, from range [0, 59]
* @property {number} seconds the number of seconds, from range [0, 59]
*/

/**
Expand Down Expand Up @@ -52,15 +52,15 @@ export class DurationInputModel extends Observable {
update(raw) {
try {
this._raw = { ...this._raw, ...raw };
this._value =
(this._raw.hours || 0) * 60 * 60 * 1000 +
(this._raw.minutes || 0) * 60 * 1000 +
(this._raw.seconds || 0) * 1000;

if (this._value === 0) {
this.reset();
const { hours, minutes, seconds } = this._raw;
if ((hours ?? minutes ?? seconds ?? null) === null) {
this._value = null;
} else {
this._value = (hours || 0) * 60 * 60 * 1000
+ (minutes || 0) * 60 * 1000
+ (seconds || 0) * 1000;
}
} catch (_) {
} catch {
this._value = null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the try catch?


Expand Down
3 changes: 1 addition & 2 deletions lib/public/views/Runs/ActiveColumns/runsActiveColumns.js
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,7 @@ export const runsActiveColumns = {
noEllipsis: true,
format: (_duration, run) => displayRunDuration(run),
exportFormat: (_duration, run) => formatRunDuration(run),
filter: ({ runDurationFilterModel, runDurationOperatorSelectionModel }) =>
durationFilter(runDurationFilterModel, runDurationOperatorSelectionModel),
filter: ({ runDurationFilterModel }) => durationFilter(runDurationFilterModel),
},
environmentId: {
name: 'Environment ID',
Expand Down
37 changes: 11 additions & 26 deletions lib/public/views/Runs/Overview/RunsOverviewModel.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,8 @@ import { EorReasonFilterModel } from '../../../components/Filters/RunsFilter/Eor
import pick from '../../../utilities/pick.js';
import { OverviewPageModel } from '../../../models/OverviewModel.js';
import { getRemoteDataSlice } from '../../../utilities/fetch/getRemoteDataSlice.js';
import { DurationInputModel } from '../../../components/common/form/inputs/DurationInputModel.js';
import { ComparisonOperatorSelectionModel }
from '../../../components/Filters/common/filters/ComparisonOperatorSelectionModel.js';
import { DurationFilterModel } from '../../../components/Filters/common/filters/DurationFilterModel.js';
import { CombinationOperator } from '../../../components/Filters/common/CombinationOperatorChoiceModel.js';

/**
* Model representing handlers for runs page
Expand Down Expand Up @@ -56,16 +55,11 @@ export class RunsOverviewModel extends OverviewPageModel {

this._eorReasonsFilterModel = new EorReasonFilterModel();
this._eorReasonsFilterModel.observe(() => this._applyFilters());
this._eorReasonsFilterModel.visualChange$.observe(() => this.notify());
this._eorReasonsFilterModel.visualChange$.bubbleTo(this);

this._runDurationFilterModel = new DurationInputModel();
this._runDurationFilterModel = new DurationFilterModel();
this._runDurationFilterModel.observe(() => this._applyFilters());
this._runDurationOperatorSelectionModel = new ComparisonOperatorSelectionModel();
this._runDurationOperatorSelectionModel.observe(() => {
if (this._runDurationFilterModel.value !== null) {
this._applyFilters();
}
});
this._runDurationFilterModel.visualChange$.bubbleTo(this);

// Export items
this._allRuns = RemoteData.NotAsked();
Expand Down Expand Up @@ -188,7 +182,6 @@ export class RunsOverviewModel extends OverviewPageModel {
this.nFlpsFilter = null;

this._runDurationFilterModel.reset();
this._runDurationOperatorSelectionModel.reset();

this.ddflpFilter = '';

Expand Down Expand Up @@ -224,7 +217,7 @@ export class RunsOverviewModel extends OverviewPageModel {
|| this.o2endFilterTo !== ''
|| this.o2endFilterToTime !== '23:59'
|| this.o2endFilterFromTime !== '00:00'
|| this._runDurationFilterModel.value !== null
|| !this._runDurationFilterModel.isEmpty()
|| this._lhcPeriodsFilter !== null
|| this.environmentIdsFilter !== ''
|| this.runQualitiesFilters.length !== 0
Expand Down Expand Up @@ -279,7 +272,7 @@ export class RunsOverviewModel extends OverviewPageModel {
if (this.o2endFilterTo !== '') {
this.activeFilters.push('O2 End to');
}
if (this._runDurationFilterModel.value !== null) {
if (!this._runDurationFilterModel.isEmpty()) {
this.activeFilters.push('Run duration');
}
if (this._lhcPeriodsFilter !== null) {
Expand Down Expand Up @@ -530,20 +523,12 @@ export class RunsOverviewModel extends OverviewPageModel {

/**
* Returns the run duration filter model
* @return {DurationInputModel} The current run duration filter model
* @return {DurationFilterModel} The current run duration filter model
*/
get runDurationFilterModel() {
return this._runDurationFilterModel;
}

/**
* Returns the run duration filter operator selection model
* @return {ComparisonOperatorSelectionModel} selection model
*/
get runDurationOperatorSelectionModel() {
return this._runDurationOperatorSelectionModel;
}

/**
* Returns the current environment id(s) filter
* @return {String} The current environment id(s) filter
Expand Down Expand Up @@ -908,9 +893,9 @@ export class RunsOverviewModel extends OverviewPageModel {
'filter[o2end][to]':
new Date(`${this.o2endFilterTo.replace(/\//g, '-')}T${this.o2endFilterToTime}:59.999`).getTime(),
},
...this._runDurationFilterModel.value && {
'filter[runDuration][operator]': this._runDurationOperatorSelectionModel.selected,
'filter[runDuration][limit]': this._runDurationFilterModel.value,
...!this._runDurationFilterModel.isEmpty() && {
'filter[runDuration][operator]': this._runDurationFilterModel.operatorSelectionModel.selected[0],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should use current instead

'filter[runDuration][limit]': this._runDurationFilterModel.durationInputModel.value,
},
...this._lhcPeriodsFilter && {
'filter[lhcPeriods]': this._lhcPeriodsFilter,
Expand Down
49 changes: 38 additions & 11 deletions test/public/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -430,11 +430,6 @@ module.exports.expectInnerTextTo = async (page, selector, validator) => {
expect(validator(actualInnerText), `"${actualInnerText}" is invalid with respect of given validator`).to.be.true;
};

module.exports.expectSelectToBe = async (page, selector, selectedOption) => {
await page.waitForSelector(selector);
expect(await (await page.$(selector)).evaluate(({ value }) => value)).to.be.equal(selectedOption);
};

/**
* Evaluate and return the html content of a given element handler
* @param {{evaluate}} elementHandler the puppeteer handler of the element to inspect
Expand Down Expand Up @@ -616,16 +611,45 @@ module.exports.expectColumnValues = async (page, columnId, expectedInnerTextValu
}
};

/**
* Method to check cells of a row with given id have expected innerText
*
* @param {puppeteer.Page} page the puppeteer page
* @param {stirng} rowId row id
* @param {Object<string, string>} [expectedInnerTextValues] values expected in the row
*
* @return {Promise<void>} resolve once row's values were checked
*/
module.exports.expectRowValues = async (page, rowId, expectedInnerTextValues) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this used? It looks like it's not, and tbh I think it's a good thing because I don't really see where it make sense.

try {
await page.waitForFunction(async (rowId, expectedInnerTextValues) => {
for (const columnId in expectedInnerTextValues) {
const actualValue = (await document.querySelectorAll(`table tbody td:nth-of-type(${rowId}) .column-${columnId}`)).innerText;
if (expectedInnerTextValues[columnId] == actualValue) {
return false;
}
}
return true;
}, rowId, expectedInnerTextValues);
} catch {
const rowInnerTexts = {};
for (const columnId in expectedInnerTextValues) {
rowInnerTexts[columnId] = (await document.querySelectorAll(`table tbody td:nth-of-type(${rowId}) .column-${columnId}`)).innerText;
}
expect(rowInnerTexts).to.eql(expectedInnerTextValues);
}
};

/**
* Generic method to validate inner text of cells belonging column with given id.
* It checks exact match with given values
*
* @param {puppeteer.Page} page the puppeteer page
* @param {string} columnId column id
* @param {string} expectedValuesRegex string that regex constructor `RegExp(expectedValuesRegex)` returns desired regular expression
* @param {string|RegExp} expectedValuesRegex string that regex constructor `RegExp(expectedValuesRegex)` returns desired regular expression
* @param {object} options options
* @param {'every'|'some'} [options.valuesCheckingMode = 'every'] whether all values are expected to match regex or at least one
* @param {boolean} [options.negation] if true it's expected not to match given regex
* @param {boolean} [options.negation = false] if true it's expected not to match given regex
*
* @return {Promise<void>} revoled once column values were checked
*/
Expand All @@ -634,13 +658,16 @@ module.exports.checkColumnValuesWithRegex = async (page, columnId, expectedValue
valuesCheckingMode = 'every',
negation = false,
} = options;

const adjustedRegExp = new RegExp(expectedValuesRegex).toString().slice(1, -1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain why you do that?
Why not

if (typeof expectedValuesRegex === 'string') {
    expectedValuesRegex = new RegExp(expectedValuesRegex);
}


await page.waitForFunction((columnId, regexString, valuesCheckingMode, negation) => {
// Browser context, be careful when modifying
const names = [...document.querySelectorAll(`table tbody .column-${columnId}`)].map(({ innerText }) => innerText);
return names.length
&& names[valuesCheckingMode]((name) =>
const innerTexts = [...document.querySelectorAll(`table tbody .column-${columnId}`)].map(({ innerText }) => innerText);
return innerTexts.length
&& innerTexts[valuesCheckingMode]((name) =>
negation ? !RegExp(regexString).test(name) : RegExp(regexString).test(name));
}, { timeout: 1500 }, columnId, expectedValuesRegex, valuesCheckingMode, negation);
}, { timeout: 1500 }, columnId, adjustedRegExp, valuesCheckingMode, negation);
};

/**
Expand Down
Loading
Loading