Skip to content
This repository has been archived by the owner on Jan 21, 2021. It is now read-only.

Add serializer for DataType retrieval #36

Open
wants to merge 5 commits into
base: gb-dev
Choose a base branch
from

Conversation

Jarvanerp
Copy link

@Jarvanerp Jarvanerp commented Nov 8, 2016

)
throw new NoSuchResourceException("No parameter named concepts was given.")
}
if (params.get('concepts') == "") {

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

Copy link
Author

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

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)

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()) {

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

Copy link
Author

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

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?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants