-
Notifications
You must be signed in to change notification settings - Fork 12
Add serializer for DataType retrieval #36
base: gb-dev
Are you sure you want to change the base?
Conversation
) | ||
throw new NoSuchResourceException("No parameter named concepts was given.") | ||
} | ||
if (params.get('concepts') == "") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when it is null? You can also use StringUtils.isEmpty() from apache.commons.lang to test for both in one go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it with "/v1/export/datatypes?concepts=" and "/v1/export/datatypes?concepts". Both return "Parameter concepts has no value."
def conceptArguments = jsonSlurper.parseText(conceptParameters) | ||
List dataTypes = [] | ||
int cohortNumber = 1 | ||
conceptArguments.each { it -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is quite some logics in this call, preferably you would put as much transformation and business logic in a Service. Leaving Http handling and top level control flow to the Controler
} | ||
respond(restExportService.formatDataTypes(datatypes)) | ||
respond(dataTypes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stylistic remark: in this call you use brackets - in the other one you use groovy bracketless call (index call). This is inconsistent, would look better to use just one way everywhere. (but this is not a big deal)
def cohortsMap = [:] | ||
dataTypeRetrieved.OntologyTermsMap.each { ID, terms -> | ||
terms.collect { term -> | ||
if (ID in cohortsMap.keySet()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you describe the desired control flow here? I am not sure if I get it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The frontend wants to know which concepts are in which cohort. Now a cohort can only contain one study, so the most logical thing to do would be to differentiate between studies. However in the near future cross studies will be added, which would result in this serialiser not functioning properly. So there needed to be a way to easily differentiate between the cohorts. This way it also supports multiple cohorts, which is also planned for the future. So based on (cohort)ID I create the structure for cohorts.
String noPatients = "There are no saved patient sets by $currentUser.username" | ||
def patientSets = queriesResource.getQueryResultsSummaryByUsername(currentUser.getUsername() ) | ||
patientSets = patientSets == null ? noPatients : patientSets | ||
respond patientSets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it wrap the noPatients string in a JSON object here?
Why not return an empty Json object instead?
8b0d78f
to
4a4784e
Compare
Along with:
thehyve/transmart-core-db#66