-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Questionnaire completion log #118
Conversation
nivemaham
commented
Jul 19, 2018
•
edited
Loading
edited
- exposes questionnaire completion log data
- refactor existing monitor api to expose all monitor data (e.g. application-status from pRMT and questionnaire-completion-log from aRMT).
private String sourceId; | ||
|
||
@JsonProperty | ||
private MonitorCategory monitorCategory; |
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'm wondering if you can't just use the org.radarcns.domain.restapi.header.Header
, leaving some values empty. Or use extension, with a base Header
with projectId, subjectId, sourceId, effectiveTimeFrame, timeFrame
, and DataSetHeader
including also DescriptiveStatistic
while the MonitorHeader could contain the monitorCategory
.
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.
We can make an extension from the header, but at the moment monitor APIs are not designed with timeFrame, effectiveTimeFrame as part of the request. As mentioned here RADAR-base/RADAR-Schemas#132, the records gets overridden. so timeFrame based requests are not supported from backend either. Having said that, it makes sense to do the extension assuming we will have this support from backend in the future.
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.
Ah okay, good point... Yes I think for future extensibility, it makes sense to have the records returned in an array, and to have a null time frame for now. If we start enabling monitoring history, clients can easily switch.
} | ||
return application; | ||
return result; |
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.
this will return null. Perhaps you could return 404 for missing subjects? The subjectService has a weird syntax though... It returns boolean
but never returns false
, instead throws a BadRequestException
where I'd expect a 404
. Perhaps it can return void
and map to NotFoundException
?
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 checkSubjectInProject
should return 404 when the subject not found as well as when not found the project.
I will change the subjectService