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

[MRI violations] Fix multiple rows for file protocol violations not resolvable #8662

Conversation

cmadjar
Copy link
Collaborator

@cmadjar cmadjar commented Apr 24, 2023

Brief summary of changes

This fixes a bug where the violation could not be resolved when there is more than one Protocol Violation returned for a file/timerun combination. The fixes include the following:

  • only show one row per pname/timeRun/mincFile/scanType in the main menu filter table when there are multiple Protocol Violation for a given file (the list of violations will be displayed when clicking on the Protocol Violation link.
  • when selecting a resolution, loop through all entries in mri_violations_log matching the timeRun/mincFile and update the resolution status for all matching entries in the violations_resolved table.

Testing instructions

For the raisinbread dataset, the following images match what this PR is fixing:

  • assembly/300170/V1/mri/native/demo_300170_V1_dwi65_001.mnc
  • assembly/300170/V1/mri/native/demo_300170_V1_dwi25_001.mnc

In current 25 code, should see two rows displayed in the main menu filter with the same content for the example images listed above and it should not be possible to save the resolution status for those files.

In code pulled from this PR, only one row per file will be displayed in the main menu filter and the resolution status will save. Look into the violations_resolved table and you should see 2 entries added for the file that was resolved.

Link(s) to related issue(s)

@cmadjar cmadjar added 25.0.0 - Bugs Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) labels Apr 24, 2023
@cmadjar cmadjar linked an issue Apr 24, 2023 that may be closed by this pull request
@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
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.

Code looks good. The test detailed in the PR description works fin on my VM after I pulled Cecile's code. Approving.

@nicolasbrossard nicolasbrossard added the Passed Manual Tests PR has undergone proper testing by at least one peer label May 2, 2023
@cmadjar cmadjar force-pushed the fix_multiple_rows_for_file_protocol_violations branch from 222fe37 to 1226e72 Compare May 2, 2023 17:06
@cmadjar
Copy link
Collaborator Author

cmadjar commented May 2, 2023

@driusan ready for you :-)

@driusan driusan merged commit 2f52b10 into aces:25.0-release May 11, 2023
@ridz1208 ridz1208 added this to the 25.0.0 milestone Jun 20, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
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 Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mri_violations] Cannot resolve any MRI violation
4 participants