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

[dicom archive] Fix dicom archive details links #9081

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

maximemulder
Copy link
Contributor

@maximemulder maximemulder commented Feb 22, 2024

Brief summary of changes

Replace the obsolete link patches for the dicom-archive/view-details page by proper links.

The first patch in view_details_link.js looks like it is supposed to patch the current links, but its mode of action is obsolete.
The second patch refers to an HTML class that currently does not exist in Loris, and is therefore very likely dead code.

Testing instructions (if applicable)

  1. Go to the dicom-archive/view-details and click the links. They should send the user to the MRI violations page with the corresponding filters applied, which can lead to an empty result.

Links to related issues

@maximemulder maximemulder added the 26.0.0-bugs Issues that were raised during the release testing for 26.0.0 label Feb 27, 2024
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.

I think we should redefine where the link takes you. With this PR, the link will take you to the violations module and show all violations that have the same patient name as the archive that is displayed on the DICOM archive page. Problem is, there can be more than one archive with the same patient name since there can be more than one upload for a given session. Consequently, you might see in the MRI violations module records that are not associated to the archive in the DICOM archive module. @cmadjar @driusan Thoughts?

If we choose to leave the behavior as is, I'll approve the PR since it works as expected on my VM.

@cmadjar
Copy link
Collaborator

cmadjar commented Feb 27, 2024

@nicolasbrossard I agree with you that the ideal behaviour would be to filter by StudyUID, unfortunately, there is no StudyUID filter in the MRI violation module at the moment. Could be nice to add it though!

In the meantime, I think the proposed fixed will at least allow users to filter for PatientName instead of having all the MRI violation displayed.

I had no idea this link even existed, lol.

@driusan thoughts? Should we modify the MRI violation module to add a StudyUID filter that could be used for that link or can we consider that that fix is already much better than current behaviour?

@driusan
Copy link
Collaborator

driusan commented Feb 28, 2024

If this is the current behaviour we should just fix the bug here and discuss / possibly change the link elsewhere.

(I believe the reason it uses patient name is because of the case where the scan is mislabeled. StudyUID might work, but would need to think it through whether there are cases where it doesn't...)

@driusan driusan merged commit cde99c2 into aces:main Mar 11, 2024
28 checks passed
@ridz1208 ridz1208 added this to the 26.0.0 milestone Apr 9, 2024
@maximemulder maximemulder deleted the fix-acquisition-id-buttom branch October 29, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
26.0.0-bugs Issues that were raised during the release testing for 26.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dicom archive] Acquisition ID link not working correctly
6 participants