TASK-5964 - Clinical Improvements #2474
Conversation
opencga-catalog/src/main/java/org/opencb/opencga/catalog/managers/InterpretationManager.java
Outdated
Show resolved
Hide resolved
opencga-catalog/src/main/java/org/opencb/opencga/catalog/managers/InterpretationManager.java
Outdated
Show resolved
Hide resolved
|
|
||
| private Document fillStatusValues(String entity, String id, Document status, List<ClinicalStatusValue> statusValueList) { | ||
| ClinicalStatus clinicalStatus = new ClinicalStatus(); | ||
| String clinicalId = status != null ? status.getString("id") : null; |
There was a problem hiding this comment.
This is "clinicalStatusId", right?
| } else { | ||
| for (ClinicalStatusValue clinicalStatusValue : statusValueList) { | ||
| if (clinicalStatusValue.getId().equals(clinicalId)) { | ||
| clinicalStatus.setId(clinicalStatusValue.getId()); |
There was a problem hiding this comment.
What if the current study clinical configuration doesn't match the "default" one?
Is it possible for the clinicalStatus to get a description different from the values on the configuration?
Shouldn't this just transfer the "id" and "description" blindly, and then "translate" the type into the new values?
There was a problem hiding this comment.
I don't really understand your question. You can only change the status for one that exists in the configuration. OpenCGA will not allow users to change to a non-defined status. Also, users can only set the new status id and everything else (description, type) is filled automatically from the status configuration values.
| break; | ||
| } | ||
| } | ||
| if (clinicalStatus.getType() == null) { |
There was a problem hiding this comment.
Risky point here... we'd overwrite/remove the old values
There was a problem hiding this comment.
That scenario is extremely unlikely but I will change to keep the original values instead of removing them. You're right
|
|
||
| // Recalculate indexes | ||
| logger.info("Installing new indexes..."); | ||
| catalogManager.installIndexes(organizationId, token); |
There was a problem hiding this comment.
This will create multiple new indexes, not just related to this particular migration... shouldn't this be an independent migration?
There was a problem hiding this comment.
That will only add new indexes which are the ones that I want to create. Those new indexes have been added for this particular feature so I'd say it makes some sense having it there. I wouldn't create an independent migration just for this. If those indexes already exist, it will not do anything.
| (document, bulk) -> { | ||
| Document internal = document.get("internal", Document.class); | ||
| if (internal == null) { | ||
| throw new MigrationRuntimeException("'internal' field not found in study '" + document.get("fqn") + "'"); |
There was a problem hiding this comment.
These == null checks are super ultra mega unlikely, right? Like a very unexpected status, right?
There was a problem hiding this comment.
Highly highly highly unlikely. That configuration ALWAYS exist so the only possibility for not getting that information would be because we have a catastrophic bug (in the migration because we haven't included those fields which I did or because the studies don't have that field which would be extremely worrying). I just need to add conditions, don't panic !!!
| @Override | ||
| protected void run() throws Exception { | ||
| ClinicalAnalysisStudyConfiguration clinicalAnalysisStudyConfiguration = ClinicalAnalysisStudyConfiguration.defaultConfiguration(); | ||
| Document clinicalConfigurationDocument = convertToDocument(clinicalAnalysisStudyConfiguration); |
There was a problem hiding this comment.
Why would you use the defaultValue instead of reading the value from the study itself?
There was a problem hiding this comment.
Because we want to override the values from the study with the default ones
No description provided.