-
Notifications
You must be signed in to change notification settings - Fork 183
[redcap] Add REDCap automatic session creation #9840
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
base: main
Are you sure you want to change the base?
[redcap] Add REDCap automatic session creation #9840
Conversation
64a5f5b
to
49d3b90
Compare
The CI errors are on main and not from this PR. Fixed the remaining TODO and successfully tested on C-BIG dev. Ready for review. I only added the last commit and rebased on main since last review. PR history is just messy because of the rebases, the commit history is still clean. |
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.
One comment, but LGTM if that was already tested on C-BIG dev.
Most of the name changes are coming from the previous PR.
modules/redcap/php/client/models/records/redcaprecord.class.inc
Outdated
Show resolved
Hide resolved
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.
@ridz1208 let me know, but if already tested on C-BIG, LGTM.
I am interested to port it to IBIS too for more tests, if possible in few weeks.
@regisoc functional on CBIG! |
Blocked by the following:
EDIT: Last two points are done, see last two commits. |
b23d00d
to
86345aa
Compare
86345aa
to
ae2db94
Compare
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 do not think there is any major issues on the few last additions, and it was already reviewed few days ago. Last additions are focused the points mentioned above, notably:
- Make the timestamp field optional and add fallback options.
- Separate session creation from next step registration.
LGTM as it was already tested on CBIG.
One important note that was part of discussions on slack: the optional _timestamp
field should always appear when importing data from REDCap instrument because it is embedded in the survey metadata parameter exportSurveyFields
of the payload.
As this field is always present (correct timestamp or empty string) on projects such as HBCD and IBIS, but was mentioned to be absent on some others (CBIG/MPN?), a new PR is required to control that this field is indeed present in imported REDCap record data.
if ($redcap_record->datetime !== null) { | ||
$datetime = $redcap_record->datetime; | ||
} else { | ||
$datetime = new \DateTimeImmutable(); |
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.
Note here: that is the LORIS server datetime, not the REDCap one (timezone might be different).
First two commits are #9839.
Successfully tested in C-BIG dev (in a backported version).