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

xsalonx · 2024-03-15T09:19:54Z

I have a JIRA ticket

branch and/or PR name(s) include(s) JIRA ID
issue has "Fix version" assigned
issue "Status" is set to "In review"
PR labels are selected

Notable changes for users:

Changed run duration filter to accept number of hours, minutes and seconds

Notable changes for developers:

NA

Changes made to the database:

NA

codecov · 2024-03-15T11:07:06Z

Codecov Report

Attention: Patch coverage is 0% with 123 lines in your changes missing coverage. Please review.

Project coverage is 43.51%. Comparing base (c6ba263) to head (eb7ae9f).

Files with missing lines	Patch %	Lines
...ib/public/views/Runs/Overview/RunsOverviewModel.js	0.00%	58 Missing ⚠️
...omponents/common/form/inputs/DurationInputModel.js	0.00%	23 Missing ⚠️
...ents/Filters/common/filters/DurationFilterModel.js	0.00%	22 Missing ⚠️
...ic/components/Filters/RunsFilter/durationFilter.js	0.00%	16 Missing ⚠️
...common/filters/ComparisonOperatorSelectionModel.js	0.00%	3 Missing ⚠️
...blic/views/Runs/ActiveColumns/runsActiveColumns.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1464      +/-   ##
==========================================
- Coverage   43.82%   43.51%   -0.31%     
==========================================
  Files         889      892       +3     
  Lines       15761    15872     +111     
  Branches     2939     2973      +34     
==========================================
  Hits         6907     6907              
- Misses       8854     8965     +111

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ept-format

martinboulais

I have put several comments on things that I have already commented too much times.
Please have a look, fix what I commented, and try to fix the rest (do it carefully, especially prune from this PR everything that do not belongs to it)

lib/public/components/Filters/common/filters/NumericComparisonOperatorSelectionModel.js

lib/public/components/Filters/common/filters/NumericFilterModel.js

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

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

lib/public/components/Filters/RunsFilter/nDetectors.js

lib/public/components/Filters/RunsFilter/nEpns.js

lib/public/components/Filters/RunsFilter/nFlps.js

lib/public/views/Runs/Overview/RunsOverviewModel.js

test/public/defaults.js

…ept-format

martinboulais · 2024-06-24T13:30:37Z

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

+    const { hours, minutes, seconds } = durationInputModel.raw;
+
+    /**
+     * Return oninput handler for given time unit


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

martinboulais · 2024-06-24T13:48:35Z

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

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


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

martinboulais · 2024-06-24T13:53:39Z

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

+        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 });


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

martinboulais · 2024-06-24T13:58:51Z

lib/public/components/Filters/common/filters/DurationFilterModel.js

+        this._operatorSelectionModel.observe(() => {
+            if (this._durationInputModel.value === null) {
+                this._visualChange$.notify();
+            } else {
+                this.notify();
+            }
+        });


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

martinboulais · 2024-06-24T14:03:46Z

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

        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;
        }


Why the try catch?

martinboulais · 2024-06-24T14:07:23Z

lib/public/views/Runs/Overview/RunsOverviewModel.js

-                'filter[runDuration][operator]': this._runDurationOperatorSelectionModel.selected,
-                'filter[runDuration][limit]': this._runDurationFilterModel.value,
+            ...!this._runDurationFilterModel.isEmpty() && {
+                'filter[runDuration][operator]': this._runDurationFilterModel.operatorSelectionModel.selected[0],


you should use current instead

martinboulais · 2024-06-24T14:12:20Z

test/public/defaults.js

+ *
+ * @return {Promise<void>} resolve once row's values were checked
+ */
+module.exports.expectRowValues = async (page, rowId, expectedInnerTextValues) => {


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.

martinboulais · 2024-06-24T14:16:49Z

test/public/defaults.js

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


Please explain why you do that?
Why not

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

martinboulais · 2024-06-24T14:20:00Z

lib/public/components/Filters/common/filters/ComparisonOperatorSelectionModel.js

+     * Constructor
+     * @param {string} [defaultOperator = '='] one of ['<', '<=', '=', '>=', '>'] operators
+     */
+    constructor(defaultOperator = '=') {


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

martinboulais · 2024-06-24T14:23:21Z

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

+        oninput: updateInputModelHandlerByTimeUnit('seconds'),
+    }, 's');
+
+    const inputs = h('.flex-row.w-100', [


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

xsalonx added 10 commits March 14, 2024 12:00

works

355ddac

works

9702196

refactor

4b87ce8

add model

d436308

nEPS filter works

eda937b

filters work

8c18794

filter works

15b5dac

pol

c3b2f86

pol

e9e7420

linter

5e98f04

xsalonx added the frontend label Mar 15, 2024

xsalonx self-assigned this Mar 15, 2024

xsalonx added 5 commits March 15, 2024 10:21

add default

3cbb87f

docs

1a61b69

conf min max

2d1731a

docs

2475d19

value

b37573a

xsalonx marked this pull request as ready for review March 15, 2024 09:30

xsalonx requested review from graduta and martinboulais as code owners March 15, 2024 09:30

xsalonx marked this pull request as draft March 15, 2024 09:31

xsalonx added 3 commits March 15, 2024 10:52

change default

78950b5

test WIP

7f03dff

Test

58d31e5

xsalonx marked this pull request as ready for review March 15, 2024 10:55

xsalonx added 2 commits March 18, 2024 16:07

Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…

191d434

…ept-format

Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…

fbdefc0

…ept-format

martinboulais requested changes Mar 26, 2024

View reviewed changes

remove override

3661588

martinboulais requested changes Mar 27, 2024

View reviewed changes

xsalonx marked this pull request as draft May 30, 2024 10:01

xsalonx added 8 commits May 30, 2024 12:13

ref

cd9d679

ch

c4c9fc7

add model

b59e0d2

ref WIP

a4933f2

ref

5d6c941

fix

fe67005

fix

f165e23

merge main

52e7bf6

xsalonx requested a review from martinboulais May 30, 2024 12:13

xsalonx marked this pull request as ready for review May 30, 2024 12:13

xsalonx marked this pull request as draft May 30, 2024 12:44

xsalonx added 4 commits May 30, 2024 14:47

cleanup

318d0a5

test WIP

7d05396

merge main

a4843f3

fix

6b013dd

xsalonx marked this pull request as ready for review June 3, 2024 12:34

xsalonx and others added 9 commits June 3, 2024 15:03

fix

c353f54

fix

73425be

Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…

c4c70dd

…ept-format

simp

2507a4a

merge main

e6763c6

Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…

a908084

…ept-format

add method

bf412c4

merge main

2ea7b4a

Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…

76905be

…ept-format

martinboulais requested changes Jun 24, 2024

View reviewed changes

xsalonx marked this pull request as draft August 8, 2024 09:26

merg main

eb7ae9f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

xsalonx commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading

martinboulais left a comment

martinboulais Jun 24, 2024

martinboulais Jun 24, 2024

martinboulais Jun 24, 2024

martinboulais Jun 24, 2024

martinboulais Jun 24, 2024

martinboulais Jun 24, 2024

martinboulais Jun 24, 2024

martinboulais Jun 24, 2024

martinboulais Jun 24, 2024

martinboulais Jun 24, 2024

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

Are you sure you want to change the base?

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

Conversation

xsalonx commented Mar 15, 2024 • edited Loading

I have a JIRA ticket

codecov bot commented Mar 15, 2024 • edited Loading

Codecov Report

martinboulais left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xsalonx commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading