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

[Imaging Browser] Fix caveat bug not showing when there are pipeline protocol violations #8661

Merged
merged 6 commits into from May 2, 2023
Merged

[Imaging Browser] Fix caveat bug not showing when there are pipeline protocol violations #8661

merged 6 commits into from May 2, 2023

Conversation

cmadjar
Copy link
Collaborator

@cmadjar cmadjar commented Apr 24, 2023 •
edited

Brief summary of changes

This fixes a bug where the Caveat link was not showing when a file has protocol violations logged automatically by the pipeline in the MRI violation module. It also fixes the link to the MRI violation module with the new react links.

Related to https://github.com/aces/HBCD/issues/738

Testing instructions

Checkout this branch and follow the test plan that has been updated with the latest changes.

@cmadjar cmadjar added Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) 25.0.0 - Bugs labels Apr 24, 2023
@cmadjar cmadjar added the Critical to release PR or issue is key for the release to which it has been assigned label Apr 25, 2023
nicolasbrossard
nicolasbrossard reviewed May 2, 2023
View reviewed changes
modules/imaging_browser/test/TestPlan.md Outdated
18. Scan-level Caveat emptor are viewable to all, modifiable if and only if user has permission `imaging_browser_qc` and there are no
pipeline protocol violations associated to the image in the MRI violation module (`Manual Caveat Set by...` protocol violation
with no other protocol violations can be modified). Example in raisinbread: `OTT170_300170_V1` `fieldmapBOLD` images have
pipeline protocol violations associated to the image in the MRI violation module and therefore should Caveat should not be editable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Grammar: "...module and therefore should Caveat should not be editable."
Not part of your PR but I noticed that step 19 mentions the (old) resolved tab...

nicolasbrossard
nicolasbrossard requested changes May 2, 2023
View reviewed changes
Copy link
Contributor

@nicolasbrossard nicolasbrossard left a comment

Choose a reason for hiding this comment

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

Tested on my VM and behavior is as expected. Code looks good but minor correction suggested to the test plan. Approval is imminent.

@nicolasbrossard
Copy link
Contributor

Not sure it's related to your PR but I see this in the JS console when loading the imaging browser's session view:

Warning: Failed prop type: Invalid prop `editable` of type `string` supplied to `ImageQCDropdown`, expected `boolean`.
    at ImageQCDropdown (http://nbrossard-dev.loris.ca/imaging_browser/js/ImagePanel.js:1667:86)
    at ImagePanelQCCaveatSelector (http://nbrossard-dev.loris.ca/imaging_browser/js/ImagePanel.js:1910:86)
    at div
    at ImagePanelQCPanel (http://nbrossard-dev.loris.ca/imaging_browser/js/ImagePanel.js:2022:86)
    at div
    at div
    at div
    at ImagePanelBody (http://nbrossard-dev.loris.ca/imaging_browser/js/ImagePanel.js:2378:86)
    at div
    at div
    at ImagePanel (http://nbrossard-dev.loris.ca/imaging_browser/js/ImagePanel.js:2492:86)

@cmadjar
Copy link
Collaborator Author

cmadjar commented May 2, 2023

Not sure it's related to your PR but I see this in the JS console when loading the imaging browser's session view:

Warning: Failed prop type: Invalid prop `editable` of type `string` supplied to `ImageQCDropdown`, expected `boolean`.
    at ImageQCDropdown (http://nbrossard-dev.loris.ca/imaging_browser/js/ImagePanel.js:1667:86)
    at ImagePanelQCCaveatSelector (http://nbrossard-dev.loris.ca/imaging_browser/js/ImagePanel.js:1910:86)
    at div
    at ImagePanelQCPanel (http://nbrossard-dev.loris.ca/imaging_browser/js/ImagePanel.js:2022:86)
    at div
    at div
    at div
    at ImagePanelBody (http://nbrossard-dev.loris.ca/imaging_browser/js/ImagePanel.js:2378:86)
    at div
    at div
    at ImagePanel (http://nbrossard-dev.loris.ca/imaging_browser/js/ImagePanel.js:2492:86)

That is odd, I do not get this in the console.

@driusan driusan merged commit cb67330 into aces:25.0-release May 2, 2023
12 checks passed
@ridz1208 ridz1208 added this to the 25.0.0 milestone Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driusan driusan driusan left review comments

@nicolasbrossard nicolasbrossard nicolasbrossard approved these changes

Assignees

@nicolasbrossard nicolasbrossard

Labels
25.0.0 - Bugs Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Critical to release PR or issue is key for the release to which it has been assigned
Projects
None yet
Milestone
25.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cmadjar @nicolasbrossard @driusan @ridz1208