Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Electrophysiology Browser] New module #5230

Merged
merged 97 commits into from
Feb 11, 2020
Merged

[Electrophysiology Browser] New module #5230

merged 97 commits into from
Feb 11, 2020

Conversation

cmadjar
Copy link
Collaborator

@cmadjar cmadjar commented Sep 17, 2019

Brief summary of changes

This PR includes SQL changes and code required for the new Electrophysiology Browser module.

Screenshot of the menu filter page (reactified and data frameworked):
Screen Shot 2019-09-17 at 5 20 43 PM

Screenshot of the view session page:
Screen Shot 2019-09-17 at 5 21 07 PM

IMPORTANT

This has been a wonderful team work and I want the authors to be recognized as most of the work to create the module has been done by them :):

  • @um4r12 for the original back end PHP code
  • @maltheism for the react front end code
  • @christinerogers, Derek for the UI and design
  • @cmadjar, upgrade of the code to 21.0, to use the latest react for the menu filter, to use the data framework for the menu filter, few minor bug fixes, design, SQL etc...

Testing instructions (if applicable)

  1. First, make sure to reload the latest RaisinBread that include EEG datasets (should be included in this branch since I rebased it after the RB dataset got merged)
  2. Go to the Electrophysiology menu tab and select the Electrophysiology Browser to direct you to the new module.
  3. Have fun clicking and checking the pages :)

Links to related tickets (GitHub, Redmine, ...)

@cmadjar cmadjar added Release: Add to release notes PR whose changes should be highlighted in the release notes Category: Feature PR or issue that aims to introduce a new feature Language: SQL PR or issue that update SQL code Release: Document at release PR whose changes need to be documented in the wiki (or other documentation) at release labels Sep 17, 2019
christinerogers
christinerogers previously approved these changes Sep 17, 2019
View reviewed changes
Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

Bravo Cecile. i'll re-review later

modules/electrophysiology_browser/README.md Show resolved Hide resolved
johnsaigle
johnsaigle reviewed Sep 18, 2019
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
johnsaigle
johnsaigle previously requested changes Sep 18, 2019
View reviewed changes
modules/electrophysiology_browser/ajax/BIDSFile.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/ajax/BIDSFile.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/ajax/BIDSFile.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/ajax/BIDSFile.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/ajax/BIDSFile.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/electrophysiologybrowserrow.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/electrophysiologybrowserrow.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/electrophysiologybrowserrowprovisioner.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/module.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/module.class.inc Outdated Show resolved Hide resolved
@johnsaigle johnsaigle added the State: Needs work PR awaiting additional work by the author to proceed label Sep 18, 2019
cmadjar
cmadjar commented Sep 18, 2019
View reviewed changes
Copy link
Collaborator Author

@cmadjar cmadjar left a comment

Choose a reason for hiding this comment

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

@johnsaigle Thank you so much for your review! I think I integrated all comments except one that I did not understand completely.

.gitignore Outdated Show resolved Hide resolved
SQL/0000-00-02-Menus.sql Outdated Show resolved Hide resolved
modules/electrophysiology_browser/README.md Show resolved Hide resolved
modules/electrophysiology_browser/ajax/get_electrophysiology_session_data.php Outdated
@@ -0,0 +1,203 @@
<?php
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you! Will be added in the next commit.

modules/electrophysiology_browser/ajax/get_electrophysiology_session_data.php Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/electrophysiologybrowserrow.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/electrophysiologybrowserrow.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/electrophysiologybrowserrowprovisioner.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/module.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/module.class.inc Outdated Show resolved Hide resolved
@maltheism maltheism self-assigned this Sep 18, 2019
@cmadjar cmadjar removed the State: Needs work PR awaiting additional work by the author to proceed label Sep 18, 2019
@johnsaigle
Copy link
Contributor

johnsaigle commented Sep 18, 2019 •
edited
Loading

@cmadjar In Alex's presentation, she explained that

$db = \Database::singleton();

results in difficult unit testing.

Instead, we should be using:

$db = \NDB_Factory::singleton()->database();

This is because the Factory class does extra checking to figure out whether the code is running in a testing environment or not.

I believe the same goes for config, timepoint, ..., etc.

So in my review I was recommending that each of these singletons be declared using Factory so that it'll be easier to eventually write unit tests.

@cmadjar
Copy link
Collaborator Author

cmadjar commented Sep 18, 2019

@johnsaigle thank you for the clarification! The last commit replaces all occurence with calls to the NDB_Factory. Thanks!!

driusan
driusan reviewed Sep 19, 2019
View reviewed changes
.eslintignore Outdated Show resolved Hide resolved
@cmadjar cmadjar added the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Sep 19, 2019
@cmadjar
Copy link
Collaborator Author

cmadjar commented Sep 19, 2019

@driusan @kongtiaowang @johnsaigle I don't any error on the Travis log but it is still failing Travis. Could you enlighten me on what I am missing please? Thank you!

@driusan
Copy link
Collaborator

driusan commented Sep 19, 2019

It looks like it's freezing/timing out after building a docker image and trying to start mysql..

WARNING: Image for service integration-tests was built because it did not already exist. To rebuild this image you must use `docker-compose build` or `docker-compose up --build`.

Waiting for mysqld...

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received

The build has been terminated

@cmadjar
Copy link
Collaborator Author

cmadjar commented Sep 19, 2019

@driusan Thank you! Is there anything that can be done to resolve that or just restart the build again? (Second time it is failing in a row)

@driusan
Copy link
Collaborator

driusan commented Sep 19, 2019

I can't think of anything that would have changed lately regarding that.. I restarted it just in case

@driusan
Copy link
Collaborator

driusan commented Sep 19, 2019

Looks like it's stuck again.. I don't understand Travis on this one either, I don't see your PR doing anything that should affect mysql starting.

@kongtiaowang
Copy link
Contributor

restart it to debug Travis

@kongtiaowang kongtiaowang reopened this Sep 20, 2019
kongtiaowang
kongtiaowang requested changes Sep 20, 2019
View reviewed changes
SQL/0000-00-01-Permission.sql Outdated Show resolved Hide resolved
@kongtiaowang kongtiaowang removed the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Sep 20, 2019
@cmadjar
Moving ElectrophysioFile to models directory
83736dd 
moved ElectrophysiologyFile in models

phpcbf

travis

easy comments from Dave

Update modules/electrophysiology_browser/README.md

Co-Authored-By: PapillonMcGill <34311645+PapillonMcGill@users.noreply.github.com>

Update modules/electrophysiology_browser/README.md

Co-Authored-By: PapillonMcGill <34311645+PapillonMcGill@users.noreply.github.com>

Update modules/electrophysiology_browser/README.md

Co-Authored-By: PapillonMcGill <34311645+PapillonMcGill@users.noreply.github.com>

Last review comments

remove unecessary lines

got rid of the ajax

fix previous and next links to add outputType

fetch instead of ajax call

phpcbf

travis

phan
@cmadjar cmadjar added the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Feb 7, 2020
@cmadjar
Copy link
Collaborator Author

cmadjar commented Feb 7, 2020

@kongtiaowang I have no idea why Travis is failing. I don't see any error message but that could just be me not able to spot the error ;).

kongtiaowang
kongtiaowang reviewed Feb 10, 2020
View reviewed changes
SQL/0000-00-01-Permission.sql Outdated
@@ -103,6 +103,8 @@ INSERT INTO `permissions` VALUES
(55,'publication_propose', 'Publication - Propose a project', 2),
(56,'publication_approve', 'Publication - Approve or reject proposed publication projects', 2),
(57, 'candidate_dob_edit', 'Edit dates of birth', 2);
Copy link
Contributor

@kongtiaowang kongtiaowang Feb 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@cmadjar line 105, change semicolon to comma. " ; " => " , "
It should pass travis after changing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kongtiaowang thank you!! Fixed.

@kongtiaowang kongtiaowang removed the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Feb 10, 2020
@cmadjar
Copy link
Collaborator Author

cmadjar commented Feb 10, 2020

Wouhou, passing Travis, @driusan I believe the ball is in your court now :)

driusan
driusan reviewed Feb 10, 2020
View reviewed changes
modules/electrophysiology_browser/jsx/components/electrophysiology_session_panels.js Outdated
'download_fdt_file',
];
for (let i=0; i<this.state.data.downloads.length; i++) {
console.log(this.state.data.downloads[i]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leftover console.log?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oooops. Will be removed in the next commit

driusan
driusan reviewed Feb 10, 2020
View reviewed changes
modules/electrophysiology_browser/php/models/ElectrophysioFile.class.inc Outdated

$db = \NDB_Factory::singleton()->database();

$query = "SELECT *
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be an explicit list of files, not a SELECT *

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok. Done

driusan
driusan requested changes Feb 10, 2020
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.

see previous comments.. almost there!

@cmadjar
Copy link
Collaborator Author

cmadjar commented Feb 10, 2020

@driusan Your comments have been taken care of. Thanks!

@cmadjar
Copy link
Collaborator Author

cmadjar commented Feb 10, 2020

@driusan Edit to the README done :). Back to you.

PS: subliminal message: merge, merge, merge ;)

@johnsaigle johnsaigle added the State: Needs changelog update PR that needs an update in the CHANGELOG file to proceed label Feb 10, 2020
@driusan
Copy link
Collaborator

driusan commented Feb 11, 2020

Closing and re-opening this to try and get it to start Travis..

@driusan driusan closed this Feb 11, 2020
@driusan driusan reopened this Feb 11, 2020
@driusan driusan merged commit e53e808 into aces:master Feb 11, 2020
@cmadjar
Copy link
Collaborator Author

cmadjar commented Feb 11, 2020

image

jesscall pushed a commit to jesscall/Loris that referenced this pull request Feb 13, 2020
@cmadjar
[Electrophysiology Browser] New module (aces#5230)
4ea4606 
Add new EEG module.

The Electrophysiology Browser is intended to allow users to view candidate
electrophysiology (EEG, MEG...) sessions collected for a study.

The Electrophysiology Browser displays electrophysiology datasets that have been
inserted into LORIS from a BIDS-format collection. Derived or processed electrophysiology
datasets can also be accessed via this module.
@ridz1208 ridz1208 added this to the 23.0.0 milestone Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xlecours xlecours xlecours approved these changes

@kongtiaowang kongtiaowang kongtiaowang left review comments

@johnsaigle johnsaigle johnsaigle left review comments

@christinerogers christinerogers christinerogers left review comments

@PapillonMcGill PapillonMcGill PapillonMcGill approved these changes

@driusan driusan driusan approved these changes

Assignees

@cmadjar cmadjar

@maltheism maltheism

Labels
Category: Feature PR or issue that aims to introduce a new feature Language: SQL PR or issue that update SQL code Passed manual tests PR has been successfully tested by at least one peer Release: Add to release notes PR whose changes should be highlighted in the release notes Release: Document at release PR whose changes need to be documented in the wiki (or other documentation) at release State: Needs changelog update PR that needs an update in the CHANGELOG file to proceed
Projects
None yet
Milestone
23.0.0
Development

Successfully merging this pull request may close these issues.
9 participants
@cmadjar @johnsaigle @driusan @kongtiaowang @maltheism @PapillonMcGill @christinerogers @xlecours @ridz1208