grand-challenge.org icon indicating copy to clipboard operation
grand-challenge.org copied to clipboard

Displayset view shows wrong widget for interfaces with super-kind File

Open miriam-groeneveld opened this issue 2 years ago • 11 comments

Describe the bug Interfaces can be defined with super_kind File. For annotations this means that the annotation should be provided as a json file. For these interfaces, the "Edit display set" view in a reader study shows the wrong widget (JSON widget instead of file upload widget). This also leads to conflicting database objects:

image

To Reproduce Steps to reproduce the behavior:

  1. Go to a reader study with a question of type Multiple 2D bounding boxes that is linked to interface Regions of interest, for example this reader study
  2. Edit a display set
  3. The widget for the default answer for the 2d Bounding box is a JSON widget.

Expected behavior A file upload widget

miriam-groeneveld avatar Jun 28 '22 09:06 miriam-groeneveld

There's a discrepancy between that interface and what is defined in the code, as Multiple 2D bounding boxes is marked as a json type in the code: https://github.com/comic/grand-challenge.org/blob/main/app/grandchallenge/components/models.py#L427, but this interface has superkind file, because it has saved_in_object_store set to True: https://github.com/comic/grand-challenge.org/blob/main/app/grandchallenge/components/models.py#L611.

The question then is, which of these two definitions is leading and which needs to be changed?

MikeOverkamp-diag avatar Jun 28 '22 10:06 MikeOverkamp-diag

The interface is correct, values can either be saved as files or in the db, see how it is handled in Archive Items.

jmsmkn avatar Jun 28 '22 10:06 jmsmkn

This is what you need to use https://github.com/comic/grand-challenge.org/blob/cace4b153b67ec93e50c9fd706ff8c1d805a94c3/app/grandchallenge/components/models.py#L629-L648

jmsmkn avatar Jun 28 '22 10:06 jmsmkn

But then I wouldn't expect a file upload widget for the interface in this issue, as the initial value is also defined in a json editor. The interface_type_json and interface_type_file functions are used to determine which widget to use, which is what is also done in the display set form.

MikeOverkamp-diag avatar Jun 28 '22 10:06 MikeOverkamp-diag

It does not matter. The issue is with the file saving, not the widget.

jmsmkn avatar Jun 28 '22 11:06 jmsmkn

This issue focuses specifically on the widget though, which imo is not what needs te be changed, but rather, like you said, the way the value is saved. I'd rather be clear on that before implementing a fix :slightly_smiling_face:

MikeOverkamp-diag avatar Jun 28 '22 11:06 MikeOverkamp-diag

That's my bad, sorry for confusing matters. I don't really care what kind of widget is shown. Although it's a bit weird maybe, to paste a huge json object, I think a file upload makes more sense (especially when users download these from other answers or algorithm outputs already anyway)

miriam-groeneveld avatar Jun 28 '22 11:06 miriam-groeneveld

I'd rather be clear on that before implementing a fix

I don't know if I can be more clear: just fix the saving.

jmsmkn avatar Jun 28 '22 11:06 jmsmkn

That's my bad, sorry for confusing matters. I don't really care what kind of widget is shown. Although it's a bit weird maybe, to paste a huge json object, I think a file upload makes more sense (especially when users download these from other answers or algorithm outputs already anyway)

For this issue, I'll just leave the widget as is, as there are no upload widgets in the update view for now. The plan is to introduce those for https://github.com/DIAGNijmegen/rse-roadmap/issues/159, so we could switch them around when that has been implemented.

MikeOverkamp-diag avatar Jun 28 '22 11:06 MikeOverkamp-diag

There's a few CIV's stored as value already, not sure what to do with those. I know Leander made a few (Multiple regions of interest), and I added some for testing.

miriam-groeneveld avatar Jun 28 '22 14:06 miriam-groeneveld

Oof, this issue is a bit bigger than I initially thought. Because the value will now be saved in the file field, rather than the value field, the value rendering in the form will not work properly in the form either. This in turn also makes it difficult to determine whether the value has changed and a new civ needs to be created or not. The quickest fix for the current situation would be to read the file contents and display them in the widget, but I really think this needs to be reworked a bit as part of https://github.com/DIAGNijmegen/rse-roadmap/issues/159. Especially as things currently should be workable for users (things should work when updating the field in the json widget), with the only issue being that currently any value for an interface that isn't an explicit file or image type will not be written to a file.

MikeOverkamp-diag avatar Jun 28 '22 14:06 MikeOverkamp-diag