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.

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

[mri_violations] Convert module to React and update UI #7473

Merged
merged 48 commits into from
Feb 20, 2023

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Jun 10, 2021

This converts the MRI Violations module to React. While doing so, is simplifies the subpages. They are no longer menu filters, but modal popup windows that display all details of errors for a scan when the link is clicked. All violations for the SeriesUID are shown to provide context. The maintenance should also be simplified by having converted most types of queries to DataProvisioners, which makes it easier to see the query and copy/paste into SQL. This should also result in more consistency with how CenterID/ProjectID matches are handled with the rest of LORIS.

Fixes #7470
Fixes #5714 (the pages no longer exist)
Fixes #6576 (there is only one page with filters now)
Fixes #6839 (the pages no longer exist, so don't have tabs..)

@driusan driusan added Release: Add to release notes PR whose changes should be highlighted in the release notes Cleanup PR or issue introducing/requiring at least one clean-up operation Area: UI PR or issue related to the user interface labels Jun 10, 2021
@cmadjar cmadjar linked an issue Jun 11, 2021 that may be closed by this pull request
@laemtl
Copy link
Contributor

laemtl commented Jun 11, 2021

@driusan Failing tests

jsx/Modal.js Outdated Show resolved Hide resolved
@cmadjar
Copy link
Collaborator

cmadjar commented Jun 11, 2021

Discussed at the imaging meeting of June 11th, 2021:

Thank you :)

Copy link
Collaborator

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

@driusan Thank you for reactifying the module! :-)

I did an initial round of testing and found the following:

  • The resolution status drop down gets truncated on smaller windows:
    Screen Shot 2021-06-14 at 9 47 40 AM
  • the data table behaviour when shrinking laterally the window behaves differently in the two different tabs.
    • For the resolved tab, the table shows the left right navigation bars or column (not sure how they are named, lol)
      Screen Shot 2021-06-14 at 2 22 27 PM
    • For the "Not Resolved" tab, the data table gets out of the border
      Screen Shot 2021-06-14 at 2 23 52 PM
  • the filter called "Minc File" and column name should probably be renamed "Image File" (or at minimum, MINC should be spelled all upper case)
  • for the time run filter, I notice you cannot filter for year/month: if you type in 2019-11, it does not restrict the list to all scans run in Nov 2019 for example. Do you know if this is feasible with the date picker? Not a huge deal but I could see this being handy :-)
  • I find confusion the term Violations for 'Could not identify scan type' pop up
  • I would tend to rearrange the content of the 'Could not identify scan type' pop up window as follows:
    • Title: use the MINC location instead of the series UID since this is the only information available in the main filter page.
    • Move the table with the image protocol before the Study protocol table and label it: image protocol instead of Violations
    • add the SeriesUID and Echo Time to the
      Screen Shot 2021-06-14 at 11 18 01 AM
  • For the Protocol Violation pop up, I would modify the title to contain the image name instead of the seriesUID. It might also be good to have a raw like for the "Could not identify scan type" pop up with the CandID, PSCID, visit, seriesUID etc... information

Let me know if something isn't clear in my fuzzy explanations ;). Thank you!

jsx/StaticDataTable.js Show resolved Hide resolved
modules/mri_violations/jsx/mriViolationsIndex.js Outdated Show resolved Hide resolved
jsx/Modal.js Show resolved Hide resolved
@driusan
Copy link
Collaborator Author

driusan commented Jun 14, 2021

The resolution status drop down gets truncated on smaller windows:

Does the non-reactified version work that way? I didn't do anything special with the column sizes, so I'm not sure what I should do about this.

the data table behaviour when shrinking laterally the window behaves differently in the two different tabs.
For the resolved tab, the table shows the left right navigation bars or column (not sure how they are named, lol)
For the "Not Resolved" tab, the data table gets out of the border

The DynamicTable jQuery plugin doesn't work with multiple tables on a page, which is why I had to disable it in the popup windows and it seems that the two tabs also have this problem since it's a single page react app now. I'm not sure what to do about this.

/ the filter called "Minc File" and column name should probably be renamed "Image File" (or at minimum, MINC should be spelled all upper case)

Okay, that should be a small change.

for the time run filter, I notice you cannot filter for year/month: if you type in 2019-11, it does not restrict the list to all scans run in Nov 2019 for example. Do you know if this is feasible with the date picker? Not a huge deal but I could see this being handy :-)

I think the components just use a <input type="date"> element, I don't think we have any way to do partial dates.

I find confusion the term Violations for 'Could not identify scan type' pop up

Okay, I can change it. I think I just started by basing the popup on the other popup when I was writing it and didn't update the title.

I would tend to rearrange the content of the 'Could not identify scan type' pop up window as follows:
Title: use the MINC location instead of the series UID since this is the only information available in the main filter page.

Okay.

   Move the table with the image protocol before the Study protocol table and label it: image protocol instead of Violations
   add the SeriesUID and Echo Time to the

I can add SeriesUID but echo time is already in the protocol table?

For the Protocol Violation pop up, I would modify the title to contain the image name instead of the seriesUID. It might also be good to have a raw like for the "Could not identify scan type" pop up with the CandID, PSCID, visit, seriesUID etc... information

I don't think I understand this part.

@cmadjar
Copy link
Collaborator

cmadjar commented Jun 14, 2021

The resolution status drop down gets truncated on smaller windows:

The non reactified version does not do that. I just checked on demo.

I can add SeriesUID but echo time is already in the protocol table?

ooops. I guess I forgot to remote the echo time from my comment when I realized what you mention here...

For the Protocol Violation pop up, I would modify the title to contain the image name instead of the seriesUID. It might also be good to have a raw like for the "Could not identify scan type" pop up with the CandID, PSCID, visit, seriesUID etc... information

Sorry, I meant, there could also be a section above the table with the following (like in the other pop up):
Screen Shot 2021-06-14 at 3 54 34 PM

Is that clearer?

@ridz1208
Copy link
Collaborator

ridz1208 commented Jul 8, 2021

@driusan any movement on this ?

modules/mri_violations/jsx/mriViolationsIndex.js Outdated Show resolved Hide resolved
modules/mri_violations/jsx/protocolModal.js Outdated Show resolved Hide resolved
modules/mri_violations/jsx/protocolModal.js Outdated Show resolved Hide resolved
modules/mri_violations/jsx/protocolModal.js Outdated Show resolved Hide resolved
@driusan driusan force-pushed the ReactMRIViolations branch from ace8958 to 96e090b Compare February 6, 2023 15:42
@driusan
Copy link
Collaborator Author

driusan commented Feb 15, 2023

@cmadjar I think I addressed all your comments and its passing tests now.. can you re-review?

Copy link
Collaborator

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

@driusan thank you for rebasing the branch and implemented the changes requested. Having used this module a lot lately, I appreciate that this module is so much more reactive!! :-)

I noticed the following bugs when going through the testing plan:

  • in the "Resolved" tab:

    • the problem column is not a clickable link that would open a pop with details about the violation. Works well in the "Not Resolved" tab.
    • when filter for SeriesDescription = 'ep2d_diff 25dir av1-updated_FA' (which corresponds to one of the file listed in the table), there is no results shown (data from RB) Looks like filter works for 'ep2d_diff ' but when I add the '2' after the space, the filter do not show anything. Not sure why. Hopefully this will help figure out what the bug is.
  • in the "Could not identify scan type" pop up:

  • in the "Protocol Violation" pop up:

    • all good :-)
  • Permissions:

    • when user only belong to one site, the user can still see the full list of sites in the Site drop down menu filter (in Resolved and Not Resolved tabs)
    • when user only belong to one project, the user can still see the full list of projects in the Project drop down menu filter (in Resolved and Not Resolved tabs)
  • Help text

    • MincFile string in help text needs to be replaced by Image File
  • Dashboard

    • in the dashboard, the number of violations listed do not match the number of rows in the MRI violation module (if user belongs to site Ottawa and project Pumpernickle, the dashboard displays 168 violations to resolve but there are 169 rows displayed in the violation module)
  • violation resolving feature: I find it not super user friendly, maybe because I am used to the save button... I am starting to wonder whether we need to tabs for this module. Couldn't it be only one page with two columns: one with the resolution status displayed and one where the user can change the resolution status. Does not need to be on that PR but just wanted to see your thoughts on that workflow. Would also allow users to change the resolution status. Let's say a resolution status is "Emailed site", eventually this would need to be changed and there is no way to do that on the frontend. I can create an issue for that but I am curious about your thoughts on this first.

@driusan
Copy link
Collaborator Author

driusan commented Feb 20, 2023

when filter for SeriesDescription = 'ep2d_diff 25dir av1-updated_FA' (which corresponds to one of the file listed in the table), there is no results shown (data from RB) Looks like filter works for 'ep2d_diff ' but when I add the '2' after the space, the filter do not show anything. Not sure why. Hopefully this will help figure out what the bug is.

I traced through this and the Series Description is actually ep2d_diff 25dir av1-updated_FA (note: 2 spaces between ep2d_diff and 25dir)

Permissions:

when user only belong to one site, the user can still see the full list of sites in the Site drop down menu filter (in Resolved and Not Resolved tabs)
when user only belong to one project, the user can still see the full list of projects in the Project drop down menu filter (in Resolved and Not Resolved tabs)

Isn't this usually the case in the rest of LORIS too?

@driusan
Copy link
Collaborator Author

driusan commented Feb 20, 2023

Created issue #8385 for the whitespace thing since it's related to the data table and not the MRI violations module.

@driusan
Copy link
Collaborator Author

driusan commented Feb 20, 2023

Dashboard

in the dashboard, the number of violations listed do not match the number of rows in the MRI violation module (if user belongs to site Ottawa and project Pumpernickle, the dashboard displays 168 violations to resolve but there are 169 rows displayed in the violation module)

One was only filtering for user center match and the other only for user project match. They should both do both now (and therefore match.. )

@driusan
Copy link
Collaborator Author

driusan commented Feb 20, 2023

@cmadjar Links should be clickable on resolved tab now. I think this is in your court again.

@cmadjar
Copy link
Collaborator

cmadjar commented Feb 20, 2023

@driusan see below for the weird display of the main table when the width of the browser gets smaller. Noticed that out of luck.
Screen Shot 2023-02-20 at 2 01 19 PM

Another little thing because I am a little neurotic... In the top table for the could not identify scan type, every header has the first letter of the words capitalized but in the Default MRI protocol group table, xspace, yspace, zspace, xstep, ystep, zstep, time range headers do not have the first letter of their words capitalized. Any chance it could be harmonized?

Thanks!

@cmadjar
Copy link
Collaborator

cmadjar commented Feb 20, 2023

@driusan I just realized an important feature is missing from the MRI violation module that used to be there. Sorry for the multiple messages... The image file link used to be clickable and that opened the image in brainbrowser. Any reason why this feature does not exist anymore?

@driusan
Copy link
Collaborator Author

driusan commented Feb 20, 2023

Looks like the link disappeared because the column was renamed from Minc File to Image File and the formatter wasn't updated.. should be fixed now. I also fixed the header capitalization.

I'm not sure what I can do about the overflowing table. That's usually handled by DynamicTable adding the overflow bars, but DynamicTable doesn't work if there's more than 1 table on the page. It had to be removed since there are multiple tables (one on each tab)

Copy link
Collaborator

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

LGTM 👍 Thank you!!

@driusan driusan merged commit 2ad76f9 into aces:main Feb 20, 2023
driusan pushed a commit that referenced this pull request Feb 21, 2023
…dule (#8392)

The links to BrainBrowser from the MRI violations module were not working for two reasons:

- queried the wrong table name for the protocol violations (mri_violations_log should have been used instead of mri_violation_log)
- the FileDownloadHandler class was using the basename of the file instead of the relative path which resulted in BrainBrowser not finding the file on the file system.

These bugs were found when testing #7473 but have been present since 24.0.
@ridz1208 ridz1208 added this to the 25.0.0 milestone Mar 6, 2023
zaliqarosli pushed a commit to zaliqarosli/Loris that referenced this pull request Mar 6, 2023
…dule (aces#8392)

The links to BrainBrowser from the MRI violations module were not working for two reasons:

- queried the wrong table name for the protocol violations (mri_violations_log should have been used instead of mri_violation_log)
- the FileDownloadHandler class was using the basename of the file instead of the relative path which resulted in BrainBrowser not finding the file on the file system.

These bugs were found when testing aces#7473 but have been present since 24.0.
driusan pushed a commit that referenced this pull request Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: UI PR or issue related to the user interface Cleanup PR or issue introducing/requiring at least one clean-up operation Release: Add to release notes PR whose changes should be highlighted in the release notes
Projects
None yet
7 participants