Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

[EEG] EEG uploader module #8409

Merged
merged 7 commits into from
Apr 5, 2023
Merged

[EEG] EEG uploader module #8409

merged 7 commits into from
Apr 5, 2023

Conversation

laemtl
Copy link
Contributor

@laemtl laemtl commented Feb 24, 2023

No description provided.

@laemtl laemtl added the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Feb 24, 2023
@laemtl laemtl force-pushed the EEG-uploader branch 2 times, most recently from 28dd3c2 to 66babb4 Compare February 24, 2023 07:51
@laemtl laemtl added this to the 25.0.0 milestone Feb 28, 2023
@laemtl laemtl self-assigned this Mar 7, 2023
@laemtl laemtl force-pushed the EEG-uploader branch 3 times, most recently from 01fc09d to 285f5c3 Compare March 22, 2023 17:55
@laemtl laemtl removed the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Mar 22, 2023
@laemtl
Copy link
Contributor Author

laemtl commented Mar 22, 2023

@jeffersoncasimir, ready for testing

@laemtl laemtl assigned jeffersoncasimir and unassigned laemtl Mar 22, 2023
@laemtl laemtl force-pushed the EEG-uploader branch 2 times, most recently from 6589b7b to 3e98457 Compare March 23, 2023 06:53
jeffersoncasimir
jeffersoncasimir approved these changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@jeffersoncasimir jeffersoncasimir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I successfully uploaded a recording to a candidate. Works as intended.

Review notes:

  • The uploaded file exceeds the upload_max_filesize directive in php.ini was properly reported on the front-end but an error that came before, which was due to the post_max_size being too low, reported as a generic failure on the front-end but was adequately reported in the server log.

  • After uploading successfully, the only way to upload again or to have an updated view of the browser is by refreshing the page.

  • Did you mean to hard-code the notification e-mails here?

@jeffersoncasimir jeffersoncasimir added the Passed Manual Tests PR has undergone proper testing by at least one peer label Mar 28, 2023
@laemtl laemtl added Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix and removed Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix labels Mar 30, 2023
@laemtl
Copy link
Contributor Author

laemtl commented Mar 30, 2023 •
edited
Loading

Thanks @jeffersoncasimir for this very good review, I addressed your comments.
To test the notification system, you can assign yourself the permission monitor_eeg_uploads.

@laemtl laemtl added the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Mar 30, 2023
@laemtl laemtl removed the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Mar 30, 2023
jeffersoncasimir
jeffersoncasimir requested changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@jeffersoncasimir jeffersoncasimir left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the new changes and the only issue remaining is that the new e-mail template was not pushed to the PR.

This actually does not trigger an error on the front-end (no success/fail modal, hangs forever after 100%, unobvious 500 response) nor the server log (nothing unusual reported). The file does still get uploaded.

I have also tested it with success by replacing the e-mail template reference with an existing one.

@laemtl
Copy link
Contributor Author

laemtl commented Mar 31, 2023

Oups, thought I did. Solved :)

@laemtl laemtl added the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Apr 4, 2023
@laemtl laemtl changed the base branch from main to 25.0-release April 4, 2023 15:10
@laemtl laemtl removed the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Apr 4, 2023
driusan
driusan requested changes Apr 4, 2023
View reviewed changes
Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the couple comments, this seems to be a new module, in which case it's missing the standardized README that every module in LORIS is supposed to have.

modules/electrophysiology_uploader/php/upload.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_uploader/php/upload.class.inc Outdated Show resolved Hide resolved
driusan
driusan reviewed Apr 4, 2023
View reviewed changes
modules/electrophysiology_uploader/php/upload.class.inc
'UploadUrl' => null,
];
foreach ($emails as $email) {
\Email::send(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be using the notifier class? @ridz1208 Is there a difference between Notify and Email::send?

@laemtl laemtl requested a review from driusan April 4, 2023 18:44
driusan
driusan requested changes Apr 4, 2023
View reviewed changes
Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still missing a standardized README for the module.

@laemtl laemtl requested a review from driusan April 4, 2023 19:52
driusan
driusan reviewed Apr 4, 2023
View reviewed changes
modules/electrophysiology_uploader/README.md Outdated Show resolved Hide resolved
modules/electrophysiology_uploader/README.md Outdated Show resolved Hide resolved
modules/electrophysiology_uploader/README.md
**Note**: Uploaded recordings are erased from the
`EEGUploadIncomingPath` following a successful archival and insertion
through the LORIS-MRI pipeline.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any interactions with other modules / LORIS that need to be defined? EEG Browser presumably?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, no other module interactions yet.

@laemtl @driusan
Apply README changes from code review
70cd323 
Co-authored-by: Dave MacFarlane <driusan@gmail.com>
@laemtl laemtl requested a review from driusan April 4, 2023 20:26
@driusan driusan merged commit 983a0cf into aces:25.0-release Apr 5, 2023
cmadjar pushed a commit to cmadjar/Loris that referenced this pull request Apr 11, 2023
@laemtl @cmadjar
[EEG] EEG uploader module (aces#8409)
9bcd3b8 
Add new module intended to allow users to upload and browse electrophysiology files.
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jeffersoncasimir jeffersoncasimir jeffersoncasimir approved these changes

@driusan driusan Awaiting requested review from driusan

Assignees

@jeffersoncasimir jeffersoncasimir

Labels
Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Milestone
25.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@laemtl @driusan @jeffersoncasimir