[O2B-1365] Disable GAQ for not fully covered runs #1784
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1784 +/- ##
==========================================
+ Coverage 43.78% 43.83% +0.05%
==========================================
Files 893 893
Lines 15959 15981 +22
Branches 3005 3013 +8
==========================================
+ Hits 6987 7006 +19
- Misses 8972 8975 +3 ☔ View full report in Codecov by Sentry. |
xsalonx
changed the title
| @@ -27,6 +27,26 @@ const GAQ_PERIODS_VIEW = ` | |||
| ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING | |||
| ) AS \`to\` | |||
| FROM ( | |||
| -- Two selects for runs' timestamps (in case QC flag's eff. period doesn't start at run's start or end at run's end ) | |||
There was a problem hiding this comment.
This raw SQL querries start to be very complex. Could you try to create a view that would simplify this?
First, you should already use time_start and time_end that already coalesce time_trg_start and time_trg_end
Also, coalesce null timestamp to 0 works but is a bit wrong in my opinion.
There was a problem hiding this comment.
applied in xsalonx/gaq/O2B-1365/disable-gaq-if-detector-is-missing---with-views
| for (const runSummaries of Object.values(summary)) { | ||
| for (const runDetectorSummary of Object.values(runSummaries)) { | ||
| this._fillQcSummaryMissingValues(runDetectorSummary); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this would fit in a function on its own. I would use the function name _fillQcSummaryMissingValues to wrap this double for loop, then the per-unit function would be named _fillQcFummaryUnitMissingValues
There was a problem hiding this comment.
Why not to wrap single-for-loop with this filling method which occur in getGaqSummary(...) ?
| const QC_SUMMARY_PROPERTIES = { | ||
| badEffectiveRunCoverage: 'badEffectiveRunCoverage', | ||
| explicitlyNotBadEffectiveRunCoverage: 'explicitlyNotBadEffectiveRunCoverage', | ||
| qualityNotDefinedEffectiveRunCoverage: 'qualityNotDefinedEffectiveRunCoverage', | ||
| missingVerificationsCount: 'missingVerificationsCount', | ||
| mcReproducible: 'mcReproducible', | ||
| }; |
There was a problem hiding this comment.
I am not sure this is a correct way to do things. Don't you think that wrapping all this in a class would make more sense?
| .to.be.equal('Missing 3 verifications'); | ||
| await expectInnerText(page, '#row106-globalAggregatedQuality', 'GAQ'); | ||
|
|
||
| await goToPage(page, 'runs-per-data-pass', { queryParameters: { dataPassId: 3 } }); |
There was a problem hiding this comment.
We should avoid goToPage, could you try to use a different way?
| }); | ||
|
|
||
| it('should switch mcReproducibleAsNotBad', async () => { | ||
| await goToPage(page, 'runs-per-data-pass', { queryParameters: { dataPassId: 1 } }); |
There was a problem hiding this comment.
Same, we should avoid goToPage
I have a JIRA ticket
Notable changes for users:
Notable changes for developers:
Changes made to the database: