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

Check if image exists before deleting civs for archive items #3116

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

amickan
Copy link
Contributor

@amickan amickan commented Dec 1, 2023

When uploading a new image to an existing archive item, all non-image CIVs of that same item used to get deleted. The reason for this was that we checked for existing CIVs with the same image and deleted those. In the case of new image uploads, however, civ.image is still empty at the time of checking, and so it matched all CIVs with image=None.

I was confused why we need this check at all. After some digging, I realized that it's necessary to enable updating of the interface of an image through the api (say from generic medical image to something more specific). This was a common use case when it was added. I'm in doubt whether we still need or want to offer this feature. It means that you can never have an archive item with the same image attached to different interfaces. Seems like a reasonable restriction to me, but it's a restriction that we don't enforce anywhere else (i.e. for display sets or jobs) as far as I know.

Finally, adding a new interface to an archive item through the dropdown menu no longer worked since it used inline js. Fixed that as well.

Closes #3104

@amickan amickan requested review from HarmvZ and jmsmkn as code owners December 1, 2023 08:11
@jmsmkn
Copy link
Member

jmsmkn commented Dec 1, 2023

It means that you can never have an archive item with the same image attached to different interfaces. Seems like a reasonable restriction to me, but it's a restriction that we don't enforce anywhere else (i.e. for display sets or jobs) as far as I know.

Test it out, I don't think that restriction is necessary. If we do allow this, do users have an easy way of removing values from a display set?

@amickan
Copy link
Contributor Author

amickan commented Dec 1, 2023

Test it out, I don't think that restriction is necessary. If we do allow this, do users have an easy way of removing values from a display set?

It's possible to attach the same image twice to one display set (with different interfaces of course), and no, users cannot delete images from display sets. Deleting only works for everything else. Enabling that would require some changes to the FlexibleImageWidget, which we use in multiple places, so we would need to be careful with making changes.

jmsmkn
jmsmkn previously approved these changes Dec 1, 2023
@amickan amickan merged commit fc1fca7 into main Dec 4, 2023
8 checks passed
@amickan amickan deleted the 3104_archive_item_updating branch December 4, 2023 07:28
amickan added a commit that referenced this pull request Jan 11, 2024
…play sets (#3156)

Closes #3138 
Part of DIAGNijmegen/rse-roadmap#274 

With this change to use the same logic and code for CIV creation for
display sets and archive items, it is no longer possible to update the
interface of an image in an archive item. We already discussed this
[here ](#3116
and had decided that this was no longer necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Annotation CIV disapears after uploading image CIV
2 participants