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 all 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
88 changes: 69 additions & 19 deletions lib/public/components/Filters/RunsFilter/durationFilter.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,76 @@
* or submit itself to any jurisdiction.
*/

import { amountFilter } from '../common/filters/amountFilter.js';
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 {RunsOverviewModel} runModel the runs model object
*
* @return {vnode} the duration filter
* @param {DurationFilterModel} durationFilterModel the duration filter model
* @return {Component} the duration filter
*/
export const durationFilter = (runModel) => amountFilter(
runModel.runDurationFilter,
(filter) => {
runModel.runDurationFilter = filter;
},
{
operatorAttributes: {
id: 'duration-operator',
},
limitAttributes: {
id: 'duration-limit',
},
},
);
export const durationFilter = (durationFilterModel) => {
const { durationInputModel, operatorSelectionModel } = durationFilterModel;

Check warning on line 24 in lib/public/components/Filters/RunsFilter/durationFilter.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/RunsFilter/durationFilter.js#L23-L24

Added lines #L23 - L24 were not covered by tests

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

Check warning on line 26 in lib/public/components/Filters/RunsFilter/durationFilter.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/RunsFilter/durationFilter.js#L26

Added line #L26 was not covered by tests

/**
* 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();

Check warning on line 40 in lib/public/components/Filters/RunsFilter/durationFilter.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/RunsFilter/durationFilter.js#L34-L40

Added lines #L34 - L40 were not covered by tests
}
};

const hoursInput = h('input.flex-grow', {

Check warning on line 44 in lib/public/components/Filters/RunsFilter/durationFilter.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/RunsFilter/durationFilter.js#L44

Added line #L44 was not covered by tests
id: 'hours-input',
type: 'number',
min: 0,
value: hours,
oninput: updateInputModelHandlerByTimeUnit('hours'),
});
const minutesInput = h('input.flex-grow', {

Check warning on line 51 in lib/public/components/Filters/RunsFilter/durationFilter.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/RunsFilter/durationFilter.js#L51

Added line #L51 was not covered by tests
id: 'minutes-input',
type: 'number',
min: 0,
max: 59,
value: minutes,
oninput: updateInputModelHandlerByTimeUnit('minutes'),
}, 'm');
const secondsInput = h('input.flex-grow', {

Check warning on line 59 in lib/public/components/Filters/RunsFilter/durationFilter.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/RunsFilter/durationFilter.js#L59

Added line #L59 was not covered by tests
id: 'seconds-input',
type: 'number',
min: 0,
max: 59,
value: seconds,
oninput: updateInputModelHandlerByTimeUnit('seconds'),
}, 's');

const inputs = h('.flex-row.w-100', [

Check warning on line 68 in lib/public/components/Filters/RunsFilter/durationFilter.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/RunsFilter/durationFilter.js#L68

Added line #L68 was not covered by tests
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,
h('.flex-row.items-center.p2', [
h('label', { for: 'hours-input' }, 'h'),
':',
h('label', { for: 'minutes-input' }, 'm'),
':',
h('label', { for: 'seconds-input' }, 's'),
]),
]);

return comparisonOperatorFilter(

Check warning on line 81 in lib/public/components/Filters/RunsFilter/durationFilter.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/RunsFilter/durationFilter.js#L81

Added line #L81 was not covered by tests
inputs,
operatorSelectionModel.selected[0],
(operator) => operatorSelectionModel.select(operator),

Check warning on line 84 in lib/public/components/Filters/RunsFilter/durationFilter.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/RunsFilter/durationFilter.js#L84

Added line #L84 was not covered by tests
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* @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 { SelectionModel } from '../../../common/selection/SelectionModel.js';

/**
* Model storing state of a selection of predefined comparison operators
*/
export class ComparisonOperatorSelectionModel extends SelectionModel {
/**
* Constructor
* @param {string} [defaultOperator = '='] one of ['<', '<=', '=', '>=', '>'] operators
*/
constructor(defaultOperator = '=') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you don't override this parameter anywhere, simply don't add it

super({
availableOptions: ['<', '<=', '=', '>=', '>'].map((operator) => ({ value: operator })),

Check warning on line 26 in lib/public/components/Filters/common/filters/ComparisonOperatorSelectionModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/common/filters/ComparisonOperatorSelectionModel.js#L24-L26

Added lines #L24 - L26 were not covered by tests
multiple: false,
allowEmpty: false,
defaultSelection: [{ value: defaultOperator }],
});
}
}
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();

Check warning on line 28 in lib/public/components/Filters/common/filters/DurationFilterModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/common/filters/DurationFilterModel.js#L26-L28

Added lines #L26 - L28 were not covered by tests

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();

Check warning on line 37 in lib/public/components/Filters/common/filters/DurationFilterModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/common/filters/DurationFilterModel.js#L30-L37

Added lines #L30 - L37 were not covered by tests
}
});
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$;

Check warning on line 48 in lib/public/components/Filters/common/filters/DurationFilterModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/common/filters/DurationFilterModel.js#L47-L48

Added lines #L47 - L48 were not covered by tests
}

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

Check warning on line 57 in lib/public/components/Filters/common/filters/DurationFilterModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/common/filters/DurationFilterModel.js#L56-L57

Added lines #L56 - L57 were not covered by tests
}

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

Check warning on line 66 in lib/public/components/Filters/common/filters/DurationFilterModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/common/filters/DurationFilterModel.js#L65-L66

Added lines #L65 - L66 were not covered by tests
}

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

Check warning on line 75 in lib/public/components/Filters/common/filters/DurationFilterModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/common/filters/DurationFilterModel.js#L74-L75

Added lines #L74 - L75 were not covered by tests
}

/**
* Reset the filter to its default value
*
* @return {void}
*/
reset() {
this._durationInputModel.reset();
this._operatorSelectionModel.reset();

Check warning on line 85 in lib/public/components/Filters/common/filters/DurationFilterModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/Filters/common/filters/DurationFilterModel.js#L83-L85

Added lines #L83 - L85 were not covered by tests
}
}
98 changes: 98 additions & 0 deletions lib/public/components/common/form/inputs/DurationInputModel.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/**
* @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';

/**
* @typedef DurationInputRawData
* @property {number} hours the number of hours
* @property {number} minutes the number of minutes, from range [0, 59]
* @property {number} seconds the number of seconds, from range [0, 59]
*/

/**
* Store the duration input
*/
export class DurationInputModel extends Observable {
/**
* Constructor
*/
constructor() {
super();

Check warning on line 31 in lib/public/components/common/form/inputs/DurationInputModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/common/form/inputs/DurationInputModel.js#L30-L31

Added lines #L30 - L31 were not covered by tests

this._raw = {

Check warning on line 33 in lib/public/components/common/form/inputs/DurationInputModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/common/form/inputs/DurationInputModel.js#L33

Added line #L33 was not covered by tests
hours: null,
minutes: null,
seconds: null,
};

/**
* Timestamp (ms)
* @type {number|null}
* @private
*/
this._value = null;

Check warning on line 44 in lib/public/components/common/form/inputs/DurationInputModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/common/form/inputs/DurationInputModel.js#L44

Added line #L44 was not covered by tests
}

/**
* Update the inputs raw values
* @param {DurationInputRawData} raw the input raw values
* @return {void}
*/
update(raw) {
try {
this._raw = { ...this._raw, ...raw };
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;

Check warning on line 61 in lib/public/components/common/form/inputs/DurationInputModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/common/form/inputs/DurationInputModel.js#L52-L61

Added lines #L52 - L61 were not covered by tests
}
} catch {
this._value = null;

Check warning on line 64 in lib/public/components/common/form/inputs/DurationInputModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/common/form/inputs/DurationInputModel.js#L64

Added line #L64 was not covered by tests
}

this.notify();

Check warning on line 67 in lib/public/components/common/form/inputs/DurationInputModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/common/form/inputs/DurationInputModel.js#L67

Added line #L67 was not covered by tests
}

/**
* Reset the inputs to its initial state
* @return {void}
*/
reset() {
this._raw = {

Check warning on line 75 in lib/public/components/common/form/inputs/DurationInputModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/common/form/inputs/DurationInputModel.js#L74-L75

Added lines #L74 - L75 were not covered by tests
hours: null,
minutes: null,
seconds: null,
};
this._value = null;

Check warning on line 80 in lib/public/components/common/form/inputs/DurationInputModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/common/form/inputs/DurationInputModel.js#L80

Added line #L80 was not covered by tests
}

/**
* Returns the raw input values
* @return {DurationInputRawData} the raw values
*/
get raw() {
return this._raw;

Check warning on line 88 in lib/public/components/common/form/inputs/DurationInputModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/common/form/inputs/DurationInputModel.js#L87-L88

Added lines #L87 - L88 were not covered by tests
}

/**
* Returns the current date represented by the inputs (null if no valid value is represented)
* @return {number|null} the current value
*/
get value() {
return this._value;

Check warning on line 96 in lib/public/components/common/form/inputs/DurationInputModel.js

View check run for this annotation

Codecov / codecov/patch

lib/public/components/common/form/inputs/DurationInputModel.js#L95-L96

Added lines #L95 - L96 were not covered by tests
}
}
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 @@ -308,8 +308,7 @@
noEllipsis: true,
format: (_duration, run) => displayRunDuration(run),
exportFormat: (_duration, run) => formatRunDuration(run),
filter: durationFilter,
filterTooltip: 'Run duration (in minutes)',
filter: ({ runDurationFilterModel }) => durationFilter(runDurationFilterModel),

Check warning on line 311 in lib/public/views/Runs/ActiveColumns/runsActiveColumns.js

View check run for this annotation

Codecov / codecov/patch

lib/public/views/Runs/ActiveColumns/runsActiveColumns.js#L311

Added line #L311 was not covered by tests
},
environmentId: {
name: 'Environment ID',
Expand Down
Loading
Loading