Easy metrics based on replicate and precursor annotation values#1210
Easy metrics based on replicate and precursor annotation values#1210ankurjuneja wants to merge 9 commits into
Conversation
labkey-jeckels
left a comment
labkey-jeckels
left a comment
There was a problem hiding this comment.
A few minor suggestions and a larger request to make one of the other metric dialogs consistent with this new approach.
| @@ -0,0 +1 @@ | |||
| ALTER TABLE targetedms.QCMetricConfiguration ADD COLUMN AnnotationName VARCHAR(200); | |||
There was a problem hiding this comment.
We should match other tables.
| ALTER TABLE targetedms.QCMetricConfiguration ADD COLUMN AnnotationName VARCHAR(200); | |
| ALTER TABLE targetedms.QCMetricConfiguration ADD COLUMN AnnotationName VARCHAR(255); |
| }); | ||
| jQuery('#createNewTraceMetricButton').click(function() { | ||
| LABKEY.internal.ConfigureQCMetrics.addNewMetric('trace') | ||
| LABKEY.internal.ConfigureQCMetrics.addNewMetric('trace'); |
There was a problem hiding this comment.
Use a constant here and other places, including for annotation
| filterArray.push(LABKEY.Filter.create('id', _config.metric.id, LABKEY.Filter.Types.NOT_EQUAL)); | ||
| } | ||
| LABKEY.Query.selectRows({ | ||
| containerPath: LABKEY.container.id, |
There was a problem hiding this comment.
While this should work in practice, better to use the path.
| containerPath: LABKEY.container.id, | |
| containerPath: LABKEY.container.path, |
| else if (configuration.getAnnotationName() != null) | ||
| { | ||
| // annotation-backed metrics: escape the annotation name for SQL string literal | ||
| String escapedName = configuration.getAnnotationName().replace("'", "''"); |
There was a problem hiding this comment.
Let's use the metric name instead of the annotation name.
| function getAnnotationTarget() { | ||
| return $('input[name="annotationType"]:checked').val() === 'precursor' | ||
| ? 'precursor_result' | ||
| : 'replicate'; |
There was a problem hiding this comment.
In the custom query metric dialog and in the table of metrics we use "run" instead of "replicate". I think "replicate" is better. Can you update the other dialog and table to be consistent with the Precursor and Replicate?
| const isPrecursor = op === 'update' && metric.PrecursorScoped; | ||
| const title = op === 'insert' ? 'Add Annotation-Backed Metric' : 'Edit Annotation-Backed Metric'; | ||
|
|
||
| return '<div id="' + DIALOG_ID + '" style="position:fixed;top:0;left:0;width:100%;height:100%;background:rgba(0,0,0,0.5);z-index:9999;display:flex;align-items:center;justify-content:center;">' |
There was a problem hiding this comment.
This is somewhat frustratingly inconsistent with the query based custom metric dialog, which has almost identical controls. The fields are in different orders, we use a radio vs a drop-down for replicate vs precursor, and the button layout is different.
While I'd like to refactor all of this into React soon, can you adapt the query metric dialog to use this plain HTML/JS approach too?
2 participants
Rationale
https://github.com/LabKey/internal-issues/issues/1046
Related Pull Requests
Changes