From 922296549b690b59f13030529cfa7fcf28ed7f1d Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Mon, 6 May 2024 16:43:04 +0200 Subject: [PATCH 01/30] Copy display set naming changes, adapted for archive items --- app/grandchallenge/archives/forms.py | 45 +++++++++++++++- .../0018_archiveitem_title_and_more.py | 25 +++++++++ app/grandchallenge/archives/models.py | 16 +++++- app/grandchallenge/archives/serializers.py | 1 + app/grandchallenge/archives/views.py | 53 +++++++++++++++---- .../templates/components/civ_set_row.html | 7 ++- ...remove_displayset_unique_title_and_more.py | 24 +++++++++ app/grandchallenge/reader_studies/models.py | 19 ++++--- app/grandchallenge/reader_studies/views.py | 2 +- app/tests/archives_tests/test_forms.py | 44 +++++++++++++++ 10 files changed, 214 insertions(+), 22 deletions(-) create mode 100644 app/grandchallenge/archives/migrations/0018_archiveitem_title_and_more.py create mode 100644 app/grandchallenge/reader_studies/migrations/0056_remove_displayset_unique_title_and_more.py diff --git a/app/grandchallenge/archives/forms.py b/app/grandchallenge/archives/forms.py index 861f2458ae..1d15482851 100644 --- a/app/grandchallenge/archives/forms.py +++ b/app/grandchallenge/archives/forms.py @@ -1,7 +1,8 @@ from dal import autocomplete from django.conf import settings -from django.core.exceptions import ObjectDoesNotExist +from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.forms import ( + CharField, Form, ModelChoiceField, ModelForm, @@ -17,6 +18,7 @@ ArchivePermissionRequest, ) from grandchallenge.cases.forms import UploadRawImagesForm +from grandchallenge.components.forms import MultipleCIVForm from grandchallenge.components.models import ComponentInterface, InterfaceKind from grandchallenge.core.forms import ( PermissionRequestUpdateForm, @@ -225,3 +227,44 @@ def save(self, *args, **kwargs): {"interface_pk": self.cleaned_data["interface"].pk} ) return super().save(*args, **kwargs) + + +class ArchiveItemCreateForm(MultipleCIVForm): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + self.fields["title"] = CharField( + required=False, + initial=self.instance and self.instance.title or "", + max_length=ArchiveItem._meta.get_field("title").max_length, + ) + + class Meta: + non_civ_fields = ("title",) + + def clean_title(self): + title = self.cleaned_data.get("title") + if title and self._title_query(title).exists(): + raise ValidationError( + "An archive item already exists with this title" + ) + return title + + def _title_query(self, title): + return ArchiveItem.objects.filter( + title=title, + archive=self.base_obj, + ) + + +class ArchiveItemUpdateForm(ArchiveItemCreateForm): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + if not self.instance.is_editable: + for _, field in self.fields.items(): + field.disabled = True + + def _title_query(self, *args, **kwargs): + return ( + super()._title_query(*args, **kwargs).exclude(id=self.instance.pk) + ) diff --git a/app/grandchallenge/archives/migrations/0018_archiveitem_title_and_more.py b/app/grandchallenge/archives/migrations/0018_archiveitem_title_and_more.py new file mode 100644 index 0000000000..119977dcd9 --- /dev/null +++ b/app/grandchallenge/archives/migrations/0018_archiveitem_title_and_more.py @@ -0,0 +1,25 @@ +# Generated by Django 4.2.11 on 2024-05-06 13:29 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("archives", "0017_archive_item_delete_permission_granting"), + ] + + operations = [ + migrations.AddField( + model_name="archiveitem", + name="title", + field=models.CharField(blank=True, default="", max_length=255), + ), + migrations.AddConstraint( + model_name="archiveitem", + constraint=models.UniqueConstraint( + condition=models.Q(("title", ""), _negated=True), + fields=("title", "archive"), + name="unique_archive_item_title", + ), + ), + ] diff --git a/app/grandchallenge/archives/models.py b/app/grandchallenge/archives/models.py index ef9b363d2a..4eb6a5047f 100644 --- a/app/grandchallenge/archives/models.py +++ b/app/grandchallenge/archives/models.py @@ -5,6 +5,7 @@ from django.contrib.auth.models import Group from django.contrib.contenttypes.models import ContentType from django.db import models +from django.db.models import Q from django_extensions.db.models import TitleSlugDescriptionModel from guardian.models import GroupObjectPermissionBase, UserObjectPermissionBase from guardian.shortcuts import assign_perm, remove_perm @@ -290,15 +291,19 @@ class ArchiveItem(CIVForObjectMixin, UUIDModel): values = models.ManyToManyField( ComponentInterfaceValue, blank=True, related_name="archive_items" ) + title = models.CharField(max_length=255, default="", blank=True) def __str__(self): + result = f"{self.title!r}" if self.title else str(self.pk) + values = [] for value in self.values.all(): if value.image: values.append(value.image.name) if value.file: values.append(os.path.basename(value.file.file.name)) - return ", ".join(values) + values = ", ".join(values) + return f"{result} ({values})" if values else f"Empty {result}" def save(self, *args, **kwargs): adding = self._state.adding @@ -308,6 +313,15 @@ def save(self, *args, **kwargs): if adding: self.assign_permissions() + class Meta: + constraints = [ + models.UniqueConstraint( + fields=["title", "archive"], + name="unique_archive_item_title", + condition=~Q(title=""), + ) + ] + def assign_permissions(self): # Archive editors, uploaders and users can view this archive item assign_perm( diff --git a/app/grandchallenge/archives/serializers.py b/app/grandchallenge/archives/serializers.py index 138e30606f..b38c9c0778 100644 --- a/app/grandchallenge/archives/serializers.py +++ b/app/grandchallenge/archives/serializers.py @@ -34,6 +34,7 @@ class Meta: model = ArchiveItem fields = ( "pk", + "title", "archive", "values", "hanging_protocol", diff --git a/app/grandchallenge/archives/views.py b/app/grandchallenge/archives/views.py index 9d27408e64..0130cc3473 100644 --- a/app/grandchallenge/archives/views.py +++ b/app/grandchallenge/archives/views.py @@ -5,7 +5,7 @@ from django.http import Http404, HttpResponseRedirect from django.shortcuts import get_object_or_404 from django.utils.datastructures import MultiValueDictKeyError -from django.utils.functional import cached_property +from django.utils.functional import cached_property, empty from django.utils.html import format_html from django.utils.safestring import mark_safe from django.utils.timezone import now @@ -30,7 +30,9 @@ from grandchallenge.archives.forms import ( AddCasesForm, ArchiveForm, + ArchiveItemCreateForm, ArchiveItemsToReaderStudyForm, + ArchiveItemUpdateForm, ArchivePermissionRequestUpdateForm, UploadersForm, UsersForm, @@ -365,7 +367,7 @@ def get_context_data(self, **kwargs): class ArchiveItemUpdate(CIVSetFormMixin, MultipleCIVProcessingBaseView): - form_class = MultipleCIVForm + form_class = ArchiveItemUpdateForm permission_required = ( f"{ArchiveItem._meta.app_label}.change_{ArchiveItem._meta.model_name}" ) @@ -413,7 +415,20 @@ def new_interface_url(self): def process_data_for_object(self, data): """Updates the archive item""" instance = self.object + non_civ_data_keys = self.form_class.Meta.non_civ_fields + + save = False + for key in non_civ_data_keys: + value = data.get(key, empty) + if value is not empty and value != getattr(instance, key): + setattr(instance, key, value) + save = True + if save: + instance.save() + for ci_slug, new_value in data.items(): + if ci_slug in non_civ_data_keys: + continue instance.create_civ( ci_slug=ci_slug, new_value=new_value, user=self.request.user ) @@ -427,10 +442,19 @@ class ArchiveItemsList(CivSetListView): ) columns = [ Column(title=""), - Column(title="ArchiveItem ID", sort_field="pk"), + Column(title="ID", sort_field="pk"), + Column( + title="Title", + sort_field="title", + optional_condition=lambda obj: bool(obj.title), + ), *CivSetListView.columns, ] - default_sort_column = 1 + default_sort_column = 2 + search_fields = [ + "title", + *CivSetListView.search_fields, + ] @cached_property def base_object(self): @@ -479,7 +503,10 @@ def form_valid(self, form): for item in items: values = item.values.all() - ds = DisplaySet.objects.create(reader_study=reader_study) + ds = DisplaySet.objects.create( + reader_study=reader_study, + title=item.title, + ) ds.values.set(values) self.success_url = reader_study.get_absolute_url() @@ -554,7 +581,7 @@ class ArchiveItemCreateView( CIVSetFormMixin, MultipleCIVProcessingBaseView, ): - form_class = MultipleCIVForm + form_class = ArchiveItemCreateForm permission_required = ( f"{Archive._meta.app_label}.change_{Archive._meta.model_name}" ) @@ -592,10 +619,18 @@ def new_interface_url(self): def process_data_for_object(self, data): """Creates an archive item""" - instance = ArchiveItem.objects.create(archive=self.base_object) - for slug in data: + non_civ_data_keys = self.form_class.Meta.non_civ_fields + + instance = ArchiveItem.objects.create( + archive=self.base_object, + **{k: data.get(k) for k in non_civ_data_keys}, + ) + + for key in data: + if key in non_civ_data_keys: + continue instance.create_civ( - ci_slug=slug, new_value=data[slug], user=self.request.user + ci_slug=key, new_value=data[key], user=self.request.user ) return instance diff --git a/app/grandchallenge/components/templates/components/civ_set_row.html b/app/grandchallenge/components/templates/components/civ_set_row.html index cbbd96df21..f2a12e7a0a 100644 --- a/app/grandchallenge/components/templates/components/civ_set_row.html +++ b/app/grandchallenge/components/templates/components/civ_set_row.html @@ -19,11 +19,10 @@ {{ object.pk }} -{% if base_object|model_name == base_model_options.READER_STUDY %} - - {{ object.title }} - +{{ object.title }} + +{% if base_object|model_name == base_model_options.READER_STUDY %} {{ object.order }} {% endif %} diff --git a/app/grandchallenge/reader_studies/migrations/0056_remove_displayset_unique_title_and_more.py b/app/grandchallenge/reader_studies/migrations/0056_remove_displayset_unique_title_and_more.py new file mode 100644 index 0000000000..7ed8a92522 --- /dev/null +++ b/app/grandchallenge/reader_studies/migrations/0056_remove_displayset_unique_title_and_more.py @@ -0,0 +1,24 @@ +# Generated by Django 4.2.11 on 2024-05-06 13:29 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("reader_studies", "0055_displayset_title_displayset_unique_title"), + ] + + operations = [ + migrations.RemoveConstraint( + model_name="displayset", + name="unique_title", + ), + migrations.AddConstraint( + model_name="displayset", + constraint=models.UniqueConstraint( + condition=models.Q(("title", ""), _negated=True), + fields=("title", "reader_study"), + name="unique_display_set_title", + ), + ), + ] diff --git a/app/grandchallenge/reader_studies/models.py b/app/grandchallenge/reader_studies/models.py index 304373aa33..68937a0e42 100644 --- a/app/grandchallenge/reader_studies/models.py +++ b/app/grandchallenge/reader_studies/models.py @@ -1,3 +1,5 @@ +import os + from actstream.models import Follow from django.conf import settings from django.contrib.auth import get_user_model @@ -798,11 +800,16 @@ class DisplaySet(CIVForObjectMixin, UUIDModel): title = models.CharField(max_length=255, default="", blank=True) def __str__(self): - parts = [self._meta.verbose_name.title()] - if self.title: - parts.append(f"{self.title!r}") - parts.append(f"(ID: {self.id})") - return " ".join(parts) + result = f"{self.title!r}" if self.title else str(self.pk) + + values = [] + for value in self.values.all(): + if value.image: + values.append(value.image.name) + if value.file: + values.append(os.path.basename(value.file.file.name)) + values = ", ".join(values) + return f"{result} ({values})" if values else f"Empty {result}" def save(self, *args, **kwargs): adding = self._state.adding @@ -838,7 +845,7 @@ class Meta: constraints = [ models.UniqueConstraint( fields=["title", "reader_study"], - name="unique_title", + name="unique_display_set_title", condition=~Q(title=""), ) ] diff --git a/app/grandchallenge/reader_studies/views.py b/app/grandchallenge/reader_studies/views.py index 3f94f714d7..ada4892532 100644 --- a/app/grandchallenge/reader_studies/views.py +++ b/app/grandchallenge/reader_studies/views.py @@ -395,7 +395,7 @@ class ReaderStudyDisplaySetList(CivSetListView): *CivSetListView.columns, ] - default_sort_column = 3 + default_sort_column = 2 search_fields = [ "title", "order", diff --git a/app/tests/archives_tests/test_forms.py b/app/tests/archives_tests/test_forms.py index 78e0feb85e..e8e40a9401 100644 --- a/app/tests/archives_tests/test_forms.py +++ b/app/tests/archives_tests/test_forms.py @@ -3,6 +3,10 @@ from django.contrib.auth.models import Permission from grandchallenge.algorithms.models import Job +from grandchallenge.archives.forms import ( + ArchiveItemCreateForm, + ArchiveItemUpdateForm, +) from grandchallenge.archives.models import Archive, ArchivePermissionRequest from grandchallenge.components.models import ( ComponentInterface, @@ -201,6 +205,46 @@ def create_archive(): assert str(archive.social_image.x20.url) in response.content.decode() +@pytest.mark.django_db +@pytest.mark.parametrize( + "form_class", + (ArchiveItemCreateForm, ArchiveItemUpdateForm), +) +@pytest.mark.parametrize( + "existing_title,new_title,expected_validity", + ( + ("", "", True), + ("Foo", "", True), + ("", "Bar", True), + ("Foo", "Bar", True), + ("Foo", "Foo", False), + ), +) +def test_archive_item_form_unique_title( + form_class, existing_title, new_title, expected_validity +): + archive = ArchiveFactory() + user = UserFactory() + archive.add_editor(user) + + ArchiveItemFactory(archive=archive, title=existing_title) + + instance = None + if form_class == ArchiveItemUpdateForm: + instance = ArchiveItemFactory(archive=archive) + + form = form_class( + user=user, + instance=instance, + base_obj=archive, + data={ + "title": new_title, + }, + ) + + assert form.is_valid() is expected_validity + + @pytest.mark.django_db def test_archive_item_update_permissions(client): archive = ArchiveFactory() From b2869397472eeffc8c139daae546311fc270befc Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Mon, 6 May 2024 16:54:57 +0200 Subject: [PATCH 02/30] Add test for copy to reader study --- app/tests/archives_tests/test_forms.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/app/tests/archives_tests/test_forms.py b/app/tests/archives_tests/test_forms.py index e8e40a9401..2aea571030 100644 --- a/app/tests/archives_tests/test_forms.py +++ b/app/tests/archives_tests/test_forms.py @@ -402,8 +402,8 @@ def test_archive_items_to_reader_study_update_form(client, settings): ComponentInterfaceValueFactory(interface=overlay, image=im4), ) - ai1 = ArchiveItemFactory(archive=archive) - ai2 = ArchiveItemFactory(archive=archive) + ai1 = ArchiveItemFactory(archive=archive, title="archive item 1") + ai2 = ArchiveItemFactory(archive=archive, title="archive item 2") ai1.values.add(civ1) ai2.values.add(civ2) @@ -435,12 +435,21 @@ def test_archive_items_to_reader_study_update_form(client, settings): assert response.status_code == 200 assert rs.display_sets.count() == 2 + + assert sorted([ds.title for ds in rs.display_sets.all()]) == sorted( + [ai1.title, ai2.title] + ) assert sorted( list(rs.display_sets.values_list("values", flat=True)) ) == sorted([civ1.pk, civ2.pk]) + ai1.title = "New title 1" ai1.values.add(civ3) + ai1.save() + + ai2.title = "New title 2" ai2.values.add(civ4) + ai2.save() response = get_view_for_user( viewname="archives:items-reader-study-update", From a60547d247b4b4cfd3fa13675c39490ccbdb93ae Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Mon, 6 May 2024 17:04:05 +0200 Subject: [PATCH 03/30] Add test that checks if title changes to not fire new jobs --- app/tests/archives_tests/test_forms.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/tests/archives_tests/test_forms.py b/app/tests/archives_tests/test_forms.py index 2aea571030..5d2d576b29 100644 --- a/app/tests/archives_tests/test_forms.py +++ b/app/tests/archives_tests/test_forms.py @@ -378,6 +378,11 @@ def test_archive_item_update_triggers_algorithm_jobs( assert Job.objects.count() == 3 assert ComponentInterfaceValue.objects.count() == civ_count + 2 + # A change to the title should not fire-off a new job + ai.title = "A new title" + ai.save() + assert Job.objects.count() == 3 + @pytest.mark.django_db def test_archive_items_to_reader_study_update_form(client, settings): From af12a728329b8099fe44412dc878750ddab9d01a Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Mon, 6 May 2024 17:17:36 +0200 Subject: [PATCH 04/30] Add test for archive item title integrity --- app/tests/archives_tests/test_models.py | 38 +++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/app/tests/archives_tests/test_models.py b/app/tests/archives_tests/test_models.py index 6253592143..8292040134 100644 --- a/app/tests/archives_tests/test_models.py +++ b/app/tests/archives_tests/test_models.py @@ -1,4 +1,5 @@ import pytest +from django.db import IntegrityError from tests.archives_tests.factories import ArchiveFactory, ArchiveItemFactory from tests.components_tests.factories import ComponentInterfaceValueFactory @@ -15,3 +16,40 @@ def create_archive_items_for_images(images, archive): civ = ComponentInterfaceValueFactory(image=image) ai = ArchiveItemFactory(archive=archive) ai.values.add(civ) + + +@pytest.mark.django_db(transaction=True) +def test_archive_item_set_title(): + archive = ArchiveFactory() + ai0 = ArchiveItemFactory(archive=archive) + + # Default + assert ai0.title == "" + + # Update + ai0.title = "An archive item title" + ai0.save() + + # Sanity + ai1 = ArchiveItemFactory( + archive=ai0.archive, + title="Another archive item title", + ) + + # Duplication attempt via edit + ai1.title = ai0.title + with pytest.raises(IntegrityError): + ai1.save() + + # Duplication attempt via save + with pytest.raises(IntegrityError): + ArchiveItemFactory( + archive=archive, + title=ai1.title, + ) + + # Other archive no problem + ArchiveItemFactory( + archive=ArchiveFactory(), + title=ai0.title, + ) From c0865605cd234a714a16f6b5aec2ac60bf4af639 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Tue, 7 May 2024 10:15:31 +0200 Subject: [PATCH 05/30] Fix archive item create view test --- app/tests/archives_tests/test_views.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/tests/archives_tests/test_views.py b/app/tests/archives_tests/test_views.py index c05bc9081b..00ff60a36d 100644 --- a/app/tests/archives_tests/test_views.py +++ b/app/tests/archives_tests/test_views.py @@ -914,9 +914,10 @@ def test_archive_item_create_view( client=client, user=editor, ) - assert len(response.context["form"].fields) == 2 + assert len(response.context["form"].fields) == 3 assert response.context["form"].fields[ci_str.slug] assert response.context["form"].fields[ci_img.slug] + assert response.context["form"].fields["title"] im_upload = create_upload_from_file( file_path=RESOURCE_PATH / "test_grayscale.jpg", @@ -945,6 +946,7 @@ def test_archive_item_create_view( f"WidgetChoice-{ci_img2.slug}": WidgetChoices.IMAGE_SEARCH.name, ci_json.slug: str(upload.pk), ci_json2.slug: '{"some": "content"}', + "title": "archive-item title", }, user=editor, method=client.post, @@ -953,6 +955,7 @@ def test_archive_item_create_view( assert response.status_code == 302 assert ArchiveItem.objects.count() == 2 ai2 = ArchiveItem.objects.order_by("created").last() + assert ai2.title == "archive-item title" assert ai2.values.count() == 5 assert ai2.values.get(interface=ci_str).value == "new-title" assert ai2.values.get(interface=ci_img).image.name == "test_grayscale.jpg" From a1a9200e99fa826742f99042632315cac8a2fe47 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Tue, 7 May 2024 11:58:10 +0200 Subject: [PATCH 06/30] Abstract the updating and creating of CIVSets --- app/grandchallenge/archives/urls.py | 4 +- app/grandchallenge/archives/views.py | 50 +++++---------------- app/grandchallenge/components/views.py | 48 +++++++++++++++++++- app/grandchallenge/evaluation/tasks.py | 2 +- app/grandchallenge/reader_studies/views.py | 52 +++++----------------- 5 files changed, 70 insertions(+), 86 deletions(-) diff --git a/app/grandchallenge/archives/urls.py b/app/grandchallenge/archives/urls.py index 919a2f138a..b3e02a848d 100644 --- a/app/grandchallenge/archives/urls.py +++ b/app/grandchallenge/archives/urls.py @@ -10,7 +10,7 @@ ArchiveItemInterfaceCreate, ArchiveItemsList, ArchiveItemsToReaderStudyUpdate, - ArchiveItemUpdate, + ArchiveItemUpdateView, ArchiveList, ArchivePermissionRequestCreate, ArchivePermissionRequestList, @@ -81,7 +81,7 @@ ), path( "/items//edit/", - ArchiveItemUpdate.as_view(), + ArchiveItemUpdateView.as_view(), name="item-edit", ), path( diff --git a/app/grandchallenge/archives/views.py b/app/grandchallenge/archives/views.py index 0130cc3473..2a097f09ba 100644 --- a/app/grandchallenge/archives/views.py +++ b/app/grandchallenge/archives/views.py @@ -5,7 +5,7 @@ from django.http import Http404, HttpResponseRedirect from django.shortcuts import get_object_or_404 from django.utils.datastructures import MultiValueDictKeyError -from django.utils.functional import cached_property, empty +from django.utils.functional import cached_property from django.utils.html import format_html from django.utils.safestring import mark_safe from django.utils.timezone import now @@ -52,9 +52,10 @@ from grandchallenge.components.forms import MultipleCIVForm, NewFileUploadForm from grandchallenge.components.views import ( CIVSetBulkDeleteView, + CIVSetCreateViewMixin, CIVSetDeleteView, - CIVSetFormMixin, CivSetListView, + CIVSetUpdateViewMixin, InterfacesCreateBaseView, MultipleCIVProcessingBaseView, ) @@ -366,7 +367,9 @@ def get_context_data(self, **kwargs): return context -class ArchiveItemUpdate(CIVSetFormMixin, MultipleCIVProcessingBaseView): +class ArchiveItemUpdateView( + CIVSetUpdateViewMixin, MultipleCIVProcessingBaseView +): form_class = ArchiveItemUpdateForm permission_required = ( f"{ArchiveItem._meta.app_label}.change_{ArchiveItem._meta.model_name}" @@ -412,28 +415,6 @@ def new_interface_url(self): kwargs={"slug": self.base_object.slug, "pk": self.object.pk}, ) - def process_data_for_object(self, data): - """Updates the archive item""" - instance = self.object - non_civ_data_keys = self.form_class.Meta.non_civ_fields - - save = False - for key in non_civ_data_keys: - value = data.get(key, empty) - if value is not empty and value != getattr(instance, key): - setattr(instance, key, value) - save = True - if save: - instance.save() - - for ci_slug, new_value in data.items(): - if ci_slug in non_civ_data_keys: - continue - instance.create_civ( - ci_slug=ci_slug, new_value=new_value, user=self.request.user - ) - return instance - class ArchiveItemsList(CivSetListView): model = ArchiveItem @@ -578,7 +559,7 @@ def get_serializer_class(self): class ArchiveItemCreateView( - CIVSetFormMixin, + CIVSetCreateViewMixin, MultipleCIVProcessingBaseView, ): form_class = ArchiveItemCreateForm @@ -617,23 +598,12 @@ def new_interface_url(self): kwargs={"slug": self.base_object.slug}, ) - def process_data_for_object(self, data): - """Creates an archive item""" - non_civ_data_keys = self.form_class.Meta.non_civ_fields - - instance = ArchiveItem.objects.create( + def create_object(self, non_civ_data): + return ArchiveItem.objects.create( archive=self.base_object, - **{k: data.get(k) for k in non_civ_data_keys}, + **non_civ_data, ) - for key in data: - if key in non_civ_data_keys: - continue - instance.create_civ( - ci_slug=key, new_value=data[key], user=self.request.user - ) - return instance - def get_success_url(self): return self.return_url diff --git a/app/grandchallenge/components/views.py b/app/grandchallenge/components/views.py index b35d29a41c..4d8902e4d3 100644 --- a/app/grandchallenge/components/views.py +++ b/app/grandchallenge/components/views.py @@ -5,7 +5,7 @@ from django.db.models import Q, TextChoices from django.forms import Media from django.http import HttpResponse -from django.utils.functional import cached_property +from django.utils.functional import cached_property, empty from django.utils.html import format_html from django.views.generic import DeleteView, FormView, ListView, TemplateView from django_filters.rest_framework import DjangoFilterBackend @@ -244,6 +244,52 @@ def new_interface_url(self): raise NotImplementedError +class CIVSetUpdateViewMixin(CIVSetFormMixin): + def process_data_for_object(self, data): + """Updates the object""" + instance = self.object + non_civ_data_keys = self.form_class.Meta.non_civ_fields + + save = False + for key in non_civ_data_keys: + value = data.get(key, empty) + if value is not empty and value != getattr(instance, key): + setattr(instance, key, value) + save = True + if save: + instance.save() + + for key in data: + if key in non_civ_data_keys: + continue + instance.create_civ( + ci_slug=key, new_value=data[key], user=self.request.user + ) + + return instance + + +class CIVSetCreateViewMixin(CIVSetFormMixin): + def create_object(self, non_civ_data): + raise NotImplementedError + + def process_data_for_object(self, data): + """Creates the object, including interfaces""" + non_civ_data_keys = self.form_class.Meta.non_civ_fields + + instance = self.create_object( + non_civ_data={k: data.get(k) for k in non_civ_data_keys} + ) + + for key in data: + if key in non_civ_data_keys: + continue + instance.create_civ( + ci_slug=key, new_value=data[key], user=self.request.user + ) + return instance + + class InterfacesCreateBaseView(ObjectPermissionRequiredMixin, TemplateView): form_class = SingleCIVForm raise_exception = True diff --git a/app/grandchallenge/evaluation/tasks.py b/app/grandchallenge/evaluation/tasks.py index b66626e65e..d48904872b 100644 --- a/app/grandchallenge/evaluation/tasks.py +++ b/app/grandchallenge/evaluation/tasks.py @@ -356,7 +356,7 @@ def set_evaluation_inputs(*, evaluation_pk): civ for civ_set in civ_sets for civ in civ_set } ), - ) + ), ) .filter( inputs_match_count=evaluation.submission.phase.algorithm_inputs.count() diff --git a/app/grandchallenge/reader_studies/views.py b/app/grandchallenge/reader_studies/views.py index ada4892532..a4c3de3949 100644 --- a/app/grandchallenge/reader_studies/views.py +++ b/app/grandchallenge/reader_studies/views.py @@ -19,7 +19,7 @@ JsonResponse, ) from django.shortcuts import get_object_or_404 -from django.utils.functional import cached_property, empty +from django.utils.functional import cached_property from django.utils.html import format_html from django.utils.safestring import mark_safe from django.utils.timezone import now @@ -59,9 +59,10 @@ ) from grandchallenge.components.views import ( CIVSetBulkDeleteView, + CIVSetCreateViewMixin, CIVSetDeleteView, - CIVSetFormMixin, CivSetListView, + CIVSetUpdateViewMixin, FileUpdateBaseView, InterfacesCreateBaseView, MultipleCIVProcessingBaseView, @@ -1261,7 +1262,9 @@ def get(self, request, slug): return HttpResponse(form["widget"]) -class DisplaySetUpdateView(CIVSetFormMixin, MultipleCIVProcessingBaseView): +class DisplaySetUpdateView( + CIVSetUpdateViewMixin, MultipleCIVProcessingBaseView +): form_class = DisplaySetUpdateForm permission_required = ( f"{ReaderStudy._meta.app_label}.change_{DisplaySet._meta.model_name}" @@ -1308,28 +1311,6 @@ def new_interface_url(self): kwargs={"slug": self.base_object.slug, "pk": self.object.pk}, ) - def process_data_for_object(self, data): - """Updates the display set""" - instance = self.object - non_civ_data_keys = self.form_class.Meta.non_civ_fields - - save = False - for key in non_civ_data_keys: - value = data.get(key, empty) - if value is not empty and value != getattr(instance, key): - setattr(instance, key, value) - save = True - if save: - instance.save() - for key in data: - if key in non_civ_data_keys: - continue - instance.create_civ( - ci_slug=key, new_value=data[key], user=self.request.user - ) - - return instance - class DisplaySetFilesUpdate(FileUpdateBaseView): permission_required = ( @@ -1386,7 +1367,7 @@ def get_htmx_url(self): class DisplaySetCreateView( - CIVSetFormMixin, + CIVSetCreateViewMixin, MultipleCIVProcessingBaseView, ): form_class = DisplaySetCreateForm @@ -1427,24 +1408,11 @@ def new_interface_url(self): kwargs={"slug": self.base_object.slug}, ) - def process_data_for_object(self, data): - """Creates a display set""" - non_civ_data_keys = self.form_class.Meta.non_civ_fields - - instance = DisplaySet.objects.create( - reader_study=self.base_object, - **{k: data.get(k) for k in non_civ_data_keys}, + def create_object(self, non_civ_data): + return DisplaySet.objects.create( + reader_study=self.base_object, **non_civ_data ) - for key in data: - if key in non_civ_data_keys: - continue - instance.create_civ( - ci_slug=key, new_value=data[key], user=self.request.user - ) - - return instance - def get_success_url(self): return self.return_url From 507da529641a3f6c39499c089f686053136d7131 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Fri, 10 May 2024 15:32:58 +0200 Subject: [PATCH 07/30] Revert to sorting by Order --- app/grandchallenge/reader_studies/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/grandchallenge/reader_studies/views.py b/app/grandchallenge/reader_studies/views.py index a4c3de3949..dc431e864d 100644 --- a/app/grandchallenge/reader_studies/views.py +++ b/app/grandchallenge/reader_studies/views.py @@ -396,7 +396,7 @@ class ReaderStudyDisplaySetList(CivSetListView): *CivSetListView.columns, ] - default_sort_column = 2 + default_sort_column = 3 search_fields = [ "title", "order", From 74c9776fb05d0a8113cbb82feeeae4317dbb0696 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Fri, 10 May 2024 16:24:49 +0200 Subject: [PATCH 08/30] Refactor Update/Create forms display set and archive items --- app/grandchallenge/archives/forms.py | 48 +++++----------------- app/grandchallenge/core/forms.py | 47 ++++++++++++++++++++- app/grandchallenge/reader_studies/forms.py | 43 ++++++------------- 3 files changed, 69 insertions(+), 69 deletions(-) diff --git a/app/grandchallenge/archives/forms.py b/app/grandchallenge/archives/forms.py index 1d15482851..ca6cfeaa6b 100644 --- a/app/grandchallenge/archives/forms.py +++ b/app/grandchallenge/archives/forms.py @@ -1,8 +1,7 @@ from dal import autocomplete from django.conf import settings -from django.core.exceptions import ObjectDoesNotExist, ValidationError +from django.core.exceptions import ObjectDoesNotExist from django.forms import ( - CharField, Form, ModelChoiceField, ModelForm, @@ -21,8 +20,10 @@ from grandchallenge.components.forms import MultipleCIVForm from grandchallenge.components.models import ComponentInterface, InterfaceKind from grandchallenge.core.forms import ( + CreateTitleFormMixin, PermissionRequestUpdateForm, SaveFormInitMixin, + UpdateTitleFormMixin, WorkstationUserFilterMixin, ) from grandchallenge.core.guardian import get_objects_for_user @@ -229,42 +230,13 @@ def save(self, *args, **kwargs): return super().save(*args, **kwargs) -class ArchiveItemCreateForm(MultipleCIVForm): - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) +class ArchiveItemCreateForm(CreateTitleFormMixin, MultipleCIVForm): + model = ArchiveItem - self.fields["title"] = CharField( - required=False, - initial=self.instance and self.instance.title or "", - max_length=ArchiveItem._meta.get_field("title").max_length, - ) + def _unique_title_query(self, *args, **kwargs): + query = super()._unique_title_query(*args, **kwargs) + return query.filter(archive=self.base_obj) - class Meta: - non_civ_fields = ("title",) - def clean_title(self): - title = self.cleaned_data.get("title") - if title and self._title_query(title).exists(): - raise ValidationError( - "An archive item already exists with this title" - ) - return title - - def _title_query(self, title): - return ArchiveItem.objects.filter( - title=title, - archive=self.base_obj, - ) - - -class ArchiveItemUpdateForm(ArchiveItemCreateForm): - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - if not self.instance.is_editable: - for _, field in self.fields.items(): - field.disabled = True - - def _title_query(self, *args, **kwargs): - return ( - super()._title_query(*args, **kwargs).exclude(id=self.instance.pk) - ) +class ArchiveItemUpdateForm(UpdateTitleFormMixin, ArchiveItemCreateForm): + pass diff --git a/app/grandchallenge/core/forms.py b/app/grandchallenge/core/forms.py index 4319ba7f95..ceb72806eb 100644 --- a/app/grandchallenge/core/forms.py +++ b/app/grandchallenge/core/forms.py @@ -1,7 +1,8 @@ from crispy_forms.helper import FormHelper from crispy_forms.layout import Submit from django.conf import settings -from django.forms import ModelForm +from django.core.exceptions import ValidationError +from django.forms import CharField, ModelForm from grandchallenge.core.guardian import get_objects_for_user from grandchallenge.workstation_configs.models import WorkstationConfig @@ -51,3 +52,47 @@ def __init__(self, *args, **kwargs): class Meta: fields = ("status", "rejection_text") + + +class CreateTitleFormMixin: + model = None + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + self.fields["title"] = CharField( + required=False, + initial=self.instance and self.instance.title or "", + max_length=self.model._meta.get_field("title").max_length, + ) + + class Meta: + non_civ_fields = ("title",) + + def clean_title(self): + title = self.cleaned_data.get("title") + if title and self._unique_title_query(title).exists(): + raise ValidationError( + f"An {self.model._meta.verbose_name} already exists with this title" + ) + return title + + def _unique_title_query(self, title): + return self.model.objects.filter( + title=title, + ) + + +class UpdateTitleFormMixin(CreateTitleFormMixin): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + if not self.instance.is_editable: + for _, field in self.fields.items(): + field.disabled = True + + def _unique_title_query(self, *args, **kwargs): + return ( + super() + ._unique_title_query(*args, **kwargs) + .exclude(id=self.instance.pk) + ) diff --git a/app/grandchallenge/reader_studies/forms.py b/app/grandchallenge/reader_studies/forms.py index b5f9b662db..3af3359a1e 100644 --- a/app/grandchallenge/reader_studies/forms.py +++ b/app/grandchallenge/reader_studies/forms.py @@ -36,8 +36,10 @@ from grandchallenge.components.forms import MultipleCIVForm from grandchallenge.components.models import ComponentInterface from grandchallenge.core.forms import ( + CreateTitleFormMixin, PermissionRequestUpdateForm, SaveFormInitMixin, + UpdateTitleFormMixin, WorkstationUserFilterMixin, ) from grandchallenge.core.layout import Formset @@ -653,15 +655,12 @@ def save_answers(self): answer.save() -class DisplaySetCreateForm(MultipleCIVForm): +class DisplaySetCreateForm(CreateTitleFormMixin, MultipleCIVForm): + model = DisplaySet + def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.fields["title"] = CharField( - required=False, - initial=self.instance and self.instance.title or "", - max_length=DisplaySet._meta.get_field("title").max_length, - ) self.fields["order"] = IntegerField( initial=( self.instance.order @@ -672,31 +671,15 @@ def __init__(self, *args, **kwargs): ) class Meta: - non_civ_fields = ("title", "order") - - def clean_title(self): - title = self.cleaned_data.get("title") - if title and self._title_query(title).exists(): - raise ValidationError( - "A display set already exists with this title" - ) - return title - - def _title_query(self, title): - return DisplaySet.objects.filter( - title=title, - reader_study=self.base_obj, + non_civ_fields = ( + "order", + *CreateTitleFormMixin.Meta.non_civ_fields, ) + def _unique_title_query(self, *args, **kwargs): + query = super()._unique_title_query(*args, **kwargs) + return query.filter(reader_study=self.base_obj) -class DisplaySetUpdateForm(DisplaySetCreateForm): - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - if not self.instance.is_editable: - for _, field in self.fields.items(): - field.disabled = True - def _title_query(self, *args, **kwargs): - return ( - super()._title_query(*args, **kwargs).exclude(id=self.instance.pk) - ) +class DisplaySetUpdateForm(UpdateTitleFormMixin, DisplaySetCreateForm): + pass From 42b743544df1cc62757d7b3fdbc9449db97132d9 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Mon, 13 May 2024 09:12:12 +0200 Subject: [PATCH 09/30] Pull out some logic from the unique title mixins --- app/grandchallenge/archives/forms.py | 9 +- app/grandchallenge/core/forms.py | 9 -- app/grandchallenge/reader_studies/forms.py | 18 ++-- app/tests/archives_tests/test_forms.py | 60 +++++++---- app/tests/reader_studies_tests/test_forms.py | 108 +++++++++++++++---- 5 files changed, 142 insertions(+), 62 deletions(-) diff --git a/app/grandchallenge/archives/forms.py b/app/grandchallenge/archives/forms.py index ca6cfeaa6b..534e1d354c 100644 --- a/app/grandchallenge/archives/forms.py +++ b/app/grandchallenge/archives/forms.py @@ -233,10 +233,17 @@ def save(self, *args, **kwargs): class ArchiveItemCreateForm(CreateTitleFormMixin, MultipleCIVForm): model = ArchiveItem + class Meta: + non_civ_fields = ("title",) + def _unique_title_query(self, *args, **kwargs): query = super()._unique_title_query(*args, **kwargs) return query.filter(archive=self.base_obj) class ArchiveItemUpdateForm(UpdateTitleFormMixin, ArchiveItemCreateForm): - pass + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + if not self.instance.is_editable: + for _, field in self.fields.items(): + field.disabled = True diff --git a/app/grandchallenge/core/forms.py b/app/grandchallenge/core/forms.py index ceb72806eb..8c4fd9f512 100644 --- a/app/grandchallenge/core/forms.py +++ b/app/grandchallenge/core/forms.py @@ -66,9 +66,6 @@ def __init__(self, *args, **kwargs): max_length=self.model._meta.get_field("title").max_length, ) - class Meta: - non_civ_fields = ("title",) - def clean_title(self): title = self.cleaned_data.get("title") if title and self._unique_title_query(title).exists(): @@ -84,12 +81,6 @@ def _unique_title_query(self, title): class UpdateTitleFormMixin(CreateTitleFormMixin): - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - if not self.instance.is_editable: - for _, field in self.fields.items(): - field.disabled = True - def _unique_title_query(self, *args, **kwargs): return ( super() diff --git a/app/grandchallenge/reader_studies/forms.py b/app/grandchallenge/reader_studies/forms.py index 3af3359a1e..f0487e503a 100644 --- a/app/grandchallenge/reader_studies/forms.py +++ b/app/grandchallenge/reader_studies/forms.py @@ -658,6 +658,12 @@ def save_answers(self): class DisplaySetCreateForm(CreateTitleFormMixin, MultipleCIVForm): model = DisplaySet + class Meta: + non_civ_fields = ( + "order", + "title", + ) + def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) @@ -670,16 +676,14 @@ def __init__(self, *args, **kwargs): min_value=0, ) - class Meta: - non_civ_fields = ( - "order", - *CreateTitleFormMixin.Meta.non_civ_fields, - ) - def _unique_title_query(self, *args, **kwargs): query = super()._unique_title_query(*args, **kwargs) return query.filter(reader_study=self.base_obj) class DisplaySetUpdateForm(UpdateTitleFormMixin, DisplaySetCreateForm): - pass + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + if not self.instance.is_editable: + for _, field in self.fields.items(): + field.disabled = True diff --git a/app/tests/archives_tests/test_forms.py b/app/tests/archives_tests/test_forms.py index 5d2d576b29..0e0df3c652 100644 --- a/app/tests/archives_tests/test_forms.py +++ b/app/tests/archives_tests/test_forms.py @@ -210,39 +210,55 @@ def create_archive(): "form_class", (ArchiveItemCreateForm, ArchiveItemUpdateForm), ) -@pytest.mark.parametrize( - "existing_title,new_title,expected_validity", - ( - ("", "", True), - ("Foo", "", True), - ("", "Bar", True), - ("Foo", "Bar", True), - ("Foo", "Foo", False), - ), -) -def test_archive_item_form_unique_title( - form_class, existing_title, new_title, expected_validity -): - archive = ArchiveFactory() +def test_archive_item_form_unique_title(form_class): + archive1 = ArchiveFactory() + user = UserFactory() - archive.add_editor(user) + archive1.add_editor(user) - ArchiveItemFactory(archive=archive, title=existing_title) + ai1 = ArchiveItemFactory(archive=archive1, title="Title in archive 1") - instance = None + instance1 = None if form_class == ArchiveItemUpdateForm: - instance = ArchiveItemFactory(archive=archive) + instance1 = ArchiveItemFactory(archive=archive1) + # Adding a unique title in archive 1 is allowed form = form_class( user=user, - instance=instance, - base_obj=archive, + instance=instance1, + base_obj=archive1, data={ - "title": new_title, + "title": "A unique title", }, ) + assert form.is_valid() + + # Adding an existing title in archive 1 is not allowed + form = form_class( + user=user, + instance=instance1, + base_obj=archive1, + data={"title": ai1.title}, + ) + assert not form.is_valid() - assert form.is_valid() is expected_validity + # However, it is allowed if it's in another archive all together + archive2 = ArchiveFactory() + archive2.add_editor(user) + + instance2 = None + if form_class == ArchiveItemUpdateForm: + instance2 = ArchiveItemFactory(archive=archive2) + + form = form_class( + user=user, + instance=instance2, + base_obj=archive2, + data={ + "title": ai1.title, + }, + ) + assert form.is_valid() @pytest.mark.django_db diff --git a/app/tests/reader_studies_tests/test_forms.py b/app/tests/reader_studies_tests/test_forms.py index cdf0a6b6c6..6a265318b2 100644 --- a/app/tests/reader_studies_tests/test_forms.py +++ b/app/tests/reader_studies_tests/test_forms.py @@ -1164,40 +1164,102 @@ def test_display_set_update_form(form_class, file_widget): "form_class", (DisplaySetCreateForm, DisplaySetUpdateForm), ) -@pytest.mark.parametrize( - "existing_ds_title,new_ds_title,expected_validity", - ( - ("", "", True), - ("Foo", "", True), - ("", "Bar", True), - ("Foo", "Bar", True), - ("Foo", "Foo", False), - ), -) -def test_display_set_form_unique_title( - form_class, existing_ds_title, new_ds_title, expected_validity -): - rs = ReaderStudyFactory() +def test_display_set_form_unique_title(form_class): + rs1 = ReaderStudyFactory() + user = UserFactory() - rs.add_editor(user) + rs1.add_editor(user) - DisplaySetFactory(reader_study=rs, title=existing_ds_title) + ds1 = DisplaySetFactory(reader_study=rs1, title="Title in reader study 1") - instance = None + instance1 = None if form_class == DisplaySetUpdateForm: - instance = DisplaySetFactory(reader_study=rs) + instance1 = DisplaySetFactory(reader_study=rs1) + # Adding a unique title in reader study 1 is allowed form = form_class( user=user, - instance=instance, - base_obj=rs, + instance=instance1, + base_obj=rs1, data={ - "order": 1, - "title": new_ds_title, + "title": "A unique title", + "order": 10, + }, + ) + assert form.is_valid() + + # Adding an existing title in reader study 1 is not allowed + form = form_class( + user=user, + instance=instance1, + base_obj=rs1, + data={ + "title": ds1.title, + "order": 10, + }, + ) + assert not form.is_valid() + + # However, it is allowed if it's in another archive all together + rs2 = ReaderStudyFactory() + rs2.add_editor(user) + + instance2 = None + if form_class == DisplaySetUpdateForm: + instance2 = DisplaySetFactory(reader_study=rs2) + + form = form_class( + user=user, + instance=instance2, + base_obj=rs2, + data={ + "title": ds1.title, + "order": 10, }, ) + assert form.is_valid() + - assert form.is_valid() is expected_validity +# TODO: transform this into a Mixin test +# @pytest.mark.django_db +# @pytest.mark.parametrize( +# "form_class", +# (DisplaySetCreateForm, DisplaySetUpdateForm), +# ) +# @pytest.mark.parametrize( +# "existing_ds_title,new_ds_title,expected_validity", +# ( +# ("", "", True), +# ("Foo", "", True), +# ("", "Bar", True), +# ("Foo", "Bar", True), +# ("Foo", "Foo", False), +# ), +# ) +# def test_display_set_form_unique_title( +# form_class, existing_ds_title, new_ds_title, expected_validity +# ): +# rs = ReaderStudyFactory() +# user = UserFactory() +# rs.add_editor(user) +# +# DisplaySetFactory(reader_study=rs, title=existing_ds_title) +# +# instance = None +# if form_class == DisplaySetUpdateForm: +# instance = DisplaySetFactory(reader_study=rs) +# +# form = form_class( +# user=user, +# instance=instance, +# base_obj=rs, +# data={ +# "order": 1, +# "title": new_ds_title, +# }, +# ) +# +# assert form.is_valid() is expected_validity @pytest.mark.parametrize( From 882b6b11eb2816537e2b646d73e750ed8b649c3e Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Mon, 13 May 2024 09:17:33 +0200 Subject: [PATCH 10/30] Replace transaction db marked tests by local atomic contextmanagers --- app/tests/archives_tests/test_models.py | 20 ++++++++++--------- app/tests/reader_studies_tests/test_models.py | 19 ++++++++++-------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/app/tests/archives_tests/test_models.py b/app/tests/archives_tests/test_models.py index 8292040134..cc225400c2 100644 --- a/app/tests/archives_tests/test_models.py +++ b/app/tests/archives_tests/test_models.py @@ -1,5 +1,5 @@ import pytest -from django.db import IntegrityError +from django.db import IntegrityError, transaction from tests.archives_tests.factories import ArchiveFactory, ArchiveItemFactory from tests.components_tests.factories import ComponentInterfaceValueFactory @@ -18,7 +18,7 @@ def create_archive_items_for_images(images, archive): ai.values.add(civ) -@pytest.mark.django_db(transaction=True) +@pytest.mark.django_db def test_archive_item_set_title(): archive = ArchiveFactory() ai0 = ArchiveItemFactory(archive=archive) @@ -38,15 +38,17 @@ def test_archive_item_set_title(): # Duplication attempt via edit ai1.title = ai0.title - with pytest.raises(IntegrityError): - ai1.save() + with transaction.atomic(): + with pytest.raises(IntegrityError): + ai1.save() # Duplication attempt via save - with pytest.raises(IntegrityError): - ArchiveItemFactory( - archive=archive, - title=ai1.title, - ) + with transaction.atomic(): + with pytest.raises(IntegrityError): + ArchiveItemFactory( + archive=archive, + title=ai1.title, + ) # Other archive no problem ArchiveItemFactory( diff --git a/app/tests/reader_studies_tests/test_models.py b/app/tests/reader_studies_tests/test_models.py index 3cc60d862b..c0e4fa6e55 100644 --- a/app/tests/reader_studies_tests/test_models.py +++ b/app/tests/reader_studies_tests/test_models.py @@ -2,6 +2,7 @@ import pytest from django.core.exceptions import ObjectDoesNotExist, ValidationError +from django.db import transaction from django.db.models import ProtectedError from django.db.utils import IntegrityError @@ -677,7 +678,7 @@ def test_display_set_description(): assert ds.description == result[ds.pk] -@pytest.mark.django_db(transaction=True) +@pytest.mark.django_db def test_display_set_title(): rs = ReaderStudyFactory() ds0 = DisplaySetFactory(reader_study=rs) @@ -697,15 +698,17 @@ def test_display_set_title(): # Duplication attempt via edit ds1.title = ds0.title - with pytest.raises(IntegrityError): - ds1.save() + with transaction.atomic(): + with pytest.raises(IntegrityError): + ds1.save() # Duplication attempt via save - with pytest.raises(IntegrityError): - DisplaySetFactory( - reader_study=rs, - title=ds0.title, - ) + with transaction.atomic(): + with pytest.raises(IntegrityError): + DisplaySetFactory( + reader_study=rs, + title=ds0.title, + ) # Other reader study is no problem DisplaySetFactory( From 6ec009e2a3658fbea60579a3d2b90e5e3aeedf61 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Mon, 13 May 2024 11:37:35 +0200 Subject: [PATCH 11/30] Add test for form mixin --- app/grandchallenge/archives/forms.py | 11 ++-- app/grandchallenge/core/forms.py | 16 +++--- app/grandchallenge/reader_studies/forms.py | 11 ++-- app/tests/core_tests/test_forms.py | 55 ++++++++++++++++++++ app/tests/reader_studies_tests/test_forms.py | 42 --------------- 5 files changed, 75 insertions(+), 60 deletions(-) create mode 100644 app/tests/core_tests/test_forms.py diff --git a/app/grandchallenge/archives/forms.py b/app/grandchallenge/archives/forms.py index 534e1d354c..65740cae76 100644 --- a/app/grandchallenge/archives/forms.py +++ b/app/grandchallenge/archives/forms.py @@ -20,10 +20,10 @@ from grandchallenge.components.forms import MultipleCIVForm from grandchallenge.components.models import ComponentInterface, InterfaceKind from grandchallenge.core.forms import ( - CreateTitleFormMixin, + CreateUniqueTitleFormMixin, PermissionRequestUpdateForm, SaveFormInitMixin, - UpdateTitleFormMixin, + UpdateUniqueTitleFormMixin, WorkstationUserFilterMixin, ) from grandchallenge.core.guardian import get_objects_for_user @@ -230,10 +230,9 @@ def save(self, *args, **kwargs): return super().save(*args, **kwargs) -class ArchiveItemCreateForm(CreateTitleFormMixin, MultipleCIVForm): - model = ArchiveItem - +class ArchiveItemCreateForm(CreateUniqueTitleFormMixin, MultipleCIVForm): class Meta: + model = ArchiveItem non_civ_fields = ("title",) def _unique_title_query(self, *args, **kwargs): @@ -241,7 +240,7 @@ def _unique_title_query(self, *args, **kwargs): return query.filter(archive=self.base_obj) -class ArchiveItemUpdateForm(UpdateTitleFormMixin, ArchiveItemCreateForm): +class ArchiveItemUpdateForm(UpdateUniqueTitleFormMixin, ArchiveItemCreateForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) if not self.instance.is_editable: diff --git a/app/grandchallenge/core/forms.py b/app/grandchallenge/core/forms.py index 8c4fd9f512..4c920e9762 100644 --- a/app/grandchallenge/core/forms.py +++ b/app/grandchallenge/core/forms.py @@ -54,8 +54,12 @@ class Meta: fields = ("status", "rejection_text") -class CreateTitleFormMixin: - model = None +class CreateUniqueTitleFormMixin: + """ + Form mixing creating an item with a unique title. + + Base class should have the `Meta.model` and `instance` attributes + """ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) @@ -63,24 +67,24 @@ def __init__(self, *args, **kwargs): self.fields["title"] = CharField( required=False, initial=self.instance and self.instance.title or "", - max_length=self.model._meta.get_field("title").max_length, + max_length=self.Meta.model._meta.get_field("title").max_length, ) def clean_title(self): title = self.cleaned_data.get("title") if title and self._unique_title_query(title).exists(): raise ValidationError( - f"An {self.model._meta.verbose_name} already exists with this title" + f"An {self.Meta.model._meta.verbose_name} already exists with this title" ) return title def _unique_title_query(self, title): - return self.model.objects.filter( + return self.Meta.model.objects.filter( title=title, ) -class UpdateTitleFormMixin(CreateTitleFormMixin): +class UpdateUniqueTitleFormMixin(CreateUniqueTitleFormMixin): def _unique_title_query(self, *args, **kwargs): return ( super() diff --git a/app/grandchallenge/reader_studies/forms.py b/app/grandchallenge/reader_studies/forms.py index f0487e503a..b1bee00451 100644 --- a/app/grandchallenge/reader_studies/forms.py +++ b/app/grandchallenge/reader_studies/forms.py @@ -36,10 +36,10 @@ from grandchallenge.components.forms import MultipleCIVForm from grandchallenge.components.models import ComponentInterface from grandchallenge.core.forms import ( - CreateTitleFormMixin, + CreateUniqueTitleFormMixin, PermissionRequestUpdateForm, SaveFormInitMixin, - UpdateTitleFormMixin, + UpdateUniqueTitleFormMixin, WorkstationUserFilterMixin, ) from grandchallenge.core.layout import Formset @@ -655,10 +655,9 @@ def save_answers(self): answer.save() -class DisplaySetCreateForm(CreateTitleFormMixin, MultipleCIVForm): - model = DisplaySet - +class DisplaySetCreateForm(CreateUniqueTitleFormMixin, MultipleCIVForm): class Meta: + model = DisplaySet non_civ_fields = ( "order", "title", @@ -681,7 +680,7 @@ def _unique_title_query(self, *args, **kwargs): return query.filter(reader_study=self.base_obj) -class DisplaySetUpdateForm(UpdateTitleFormMixin, DisplaySetCreateForm): +class DisplaySetUpdateForm(UpdateUniqueTitleFormMixin, DisplaySetCreateForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) if not self.instance.is_editable: diff --git a/app/tests/core_tests/test_forms.py b/app/tests/core_tests/test_forms.py new file mode 100644 index 0000000000..bb8cd49018 --- /dev/null +++ b/app/tests/core_tests/test_forms.py @@ -0,0 +1,55 @@ +import pytest +from django.forms import Form + +from grandchallenge.core.forms import ( + CreateUniqueTitleFormMixin, + UpdateUniqueTitleFormMixin, +) +from grandchallenge.reader_studies.models import DisplaySet +from tests.reader_studies_tests.factories import DisplaySetFactory + + +@pytest.mark.django_db +@pytest.mark.parametrize( + "form_class", + (CreateUniqueTitleFormMixin, UpdateUniqueTitleFormMixin), +) +@pytest.mark.parametrize( + "existing_title,new_title,expected_validity", + ( + ("", "", True), + ("Foo", "", True), + ("", "Bar", True), + ("Foo", "Bar", True), + ("Foo", "Foo", False), + ), +) +def test_unique_title_mixin( + form_class, existing_title, new_title, expected_validity +): + + class TestForm(form_class, Form): + class Meta: + model = DisplaySet # For ease, use an existing model with a title + + def __init__(self, *args, instance, **kwargs): + self.instance = instance + + super().__init__(*args, **kwargs) + + # Create an existing item + DisplaySetFactory(title=existing_title) + + # Adapt for updating + instance = None + if form_class == UpdateUniqueTitleFormMixin: + instance = DisplaySetFactory() + + form = TestForm( + instance=instance, + data={ + "title": new_title, + }, + ) + + assert form.is_valid() is expected_validity diff --git a/app/tests/reader_studies_tests/test_forms.py b/app/tests/reader_studies_tests/test_forms.py index 6a265318b2..b3b60db0e0 100644 --- a/app/tests/reader_studies_tests/test_forms.py +++ b/app/tests/reader_studies_tests/test_forms.py @@ -1220,48 +1220,6 @@ def test_display_set_form_unique_title(form_class): assert form.is_valid() -# TODO: transform this into a Mixin test -# @pytest.mark.django_db -# @pytest.mark.parametrize( -# "form_class", -# (DisplaySetCreateForm, DisplaySetUpdateForm), -# ) -# @pytest.mark.parametrize( -# "existing_ds_title,new_ds_title,expected_validity", -# ( -# ("", "", True), -# ("Foo", "", True), -# ("", "Bar", True), -# ("Foo", "Bar", True), -# ("Foo", "Foo", False), -# ), -# ) -# def test_display_set_form_unique_title( -# form_class, existing_ds_title, new_ds_title, expected_validity -# ): -# rs = ReaderStudyFactory() -# user = UserFactory() -# rs.add_editor(user) -# -# DisplaySetFactory(reader_study=rs, title=existing_ds_title) -# -# instance = None -# if form_class == DisplaySetUpdateForm: -# instance = DisplaySetFactory(reader_study=rs) -# -# form = form_class( -# user=user, -# instance=instance, -# base_obj=rs, -# data={ -# "order": 1, -# "title": new_ds_title, -# }, -# ) -# -# assert form.is_valid() is expected_validity - - @pytest.mark.parametrize( "form_class", ( From 570672aaab83ce0c44440bdd7f509c476cf42988 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Mon, 13 May 2024 12:59:36 +0200 Subject: [PATCH 12/30] Remove is_editable logic from archive items --- app/grandchallenge/archives/forms.py | 6 +----- app/grandchallenge/archives/models.py | 5 ----- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/app/grandchallenge/archives/forms.py b/app/grandchallenge/archives/forms.py index 65740cae76..75c0fa86a1 100644 --- a/app/grandchallenge/archives/forms.py +++ b/app/grandchallenge/archives/forms.py @@ -241,8 +241,4 @@ def _unique_title_query(self, *args, **kwargs): class ArchiveItemUpdateForm(UpdateUniqueTitleFormMixin, ArchiveItemCreateForm): - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - if not self.instance.is_editable: - for _, field in self.fields.items(): - field.disabled = True + pass diff --git a/app/grandchallenge/archives/models.py b/app/grandchallenge/archives/models.py index 4eb6a5047f..278602ea1a 100644 --- a/app/grandchallenge/archives/models.py +++ b/app/grandchallenge/archives/models.py @@ -353,11 +353,6 @@ def assign_permissions(self): def base_object(self): return self.archive - @property - def is_editable(self): - # always editable - return True - @property def update_url(self): return reverse( From 6cbe25113e0b0f1d1c5bb733088f7aed8f0c2ed6 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Mon, 13 May 2024 13:01:30 +0200 Subject: [PATCH 13/30] Change and refactor the civ_set remove confirmations --- app/grandchallenge/archives/models.py | 17 ++-------- app/grandchallenge/components/models.py | 24 ++++++++++++++ .../components/civset_confirm_delete.html | 31 ++++++++++++++++--- app/grandchallenge/reader_studies/models.py | 17 ++-------- 4 files changed, 54 insertions(+), 35 deletions(-) diff --git a/app/grandchallenge/archives/models.py b/app/grandchallenge/archives/models.py index 278602ea1a..dd1daf8994 100644 --- a/app/grandchallenge/archives/models.py +++ b/app/grandchallenge/archives/models.py @@ -1,5 +1,3 @@ -import os - from actstream.models import Follow from django.conf import settings from django.contrib.auth.models import Group @@ -15,6 +13,7 @@ from grandchallenge.anatomy.models import BodyStructure from grandchallenge.components.models import ( CIVForObjectMixin, + CIVSetContainerMixin, ComponentInterfaceValue, ValuesForInterfacesMixin, ) @@ -284,7 +283,7 @@ class ArchiveGroupObjectPermission(GroupObjectPermissionBase): content_object = models.ForeignKey(Archive, on_delete=models.CASCADE) -class ArchiveItem(CIVForObjectMixin, UUIDModel): +class ArchiveItem(CIVSetContainerMixin, CIVForObjectMixin, UUIDModel): archive = models.ForeignKey( Archive, related_name="items", on_delete=models.PROTECT ) @@ -293,18 +292,6 @@ class ArchiveItem(CIVForObjectMixin, UUIDModel): ) title = models.CharField(max_length=255, default="", blank=True) - def __str__(self): - result = f"{self.title!r}" if self.title else str(self.pk) - - values = [] - for value in self.values.all(): - if value.image: - values.append(value.image.name) - if value.file: - values.append(os.path.basename(value.file.file.name)) - values = ", ".join(values) - return f"{result} ({values})" if values else f"Empty {result}" - def save(self, *args, **kwargs): adding = self._state.adding diff --git a/app/grandchallenge/components/models.py b/app/grandchallenge/components/models.py index e8eb12fe3a..38167a1620 100644 --- a/app/grandchallenge/components/models.py +++ b/app/grandchallenge/components/models.py @@ -1962,6 +1962,30 @@ def update_size_in_storage(self): self.size_in_registry = self.calculate_size_in_registry() +class CIVSetContainerMixin: + def __str__(self): + result = [str(self.pk)] + + if self.title: + result.append(f"{self.title!r}") + + result.append(self.__content_str) + return ", ".join(result) + + @property + def __content_str(self): + civs = self.values.all() + nr = len(civs) + if nr == 0: + return "No content" + + if nr > 5: + return "5+ items" + + content = [f"{civ.interface.title}: {civ.title[:30]}" for civ in civs] + return ", ".join(content) + + class CIVForObjectMixin: def create_civ(self, *, ci_slug, new_value, user=None): ci = ComponentInterface.objects.get(slug=ci_slug) diff --git a/app/grandchallenge/components/templates/components/civset_confirm_delete.html b/app/grandchallenge/components/templates/components/civset_confirm_delete.html index cce9f9e299..480cdd6c28 100644 --- a/app/grandchallenge/components/templates/components/civset_confirm_delete.html +++ b/app/grandchallenge/components/templates/components/civset_confirm_delete.html @@ -16,13 +16,34 @@ {% endblock %} {% block content %} -

Confirm Deletion

+

Confirm {{ object|verbose_name|title }} Deletion

{% csrf_token %} -

Are you sure that you want to delete {{ object }}?
- WARNING: - You are not able to undo this action. Once the {{ object }} is deleted - it is deleted forever.

+

Are you sure that you want to delete {{ object|verbose_name }} {{ object.pk }}?

+

Content

+
+
+ {% if object.title %} +
Title
+
{{ object.title }}
+ {% endif %} + {% if object.values.exists %} + {% for civ in object.values.all %} +
{{ civ.interface.title }}
+
{{ civ.title|truncatechars:30 }}
+ {% endfor %} + {% else %} + + No content + + {% endif %} +
+
+

+ WARNING: + You are not able to undo this action. Once the {{ object|verbose_name }} is deleted + it is deleted forever. +

Cancel diff --git a/app/grandchallenge/reader_studies/models.py b/app/grandchallenge/reader_studies/models.py index 68937a0e42..67708862cc 100644 --- a/app/grandchallenge/reader_studies/models.py +++ b/app/grandchallenge/reader_studies/models.py @@ -1,5 +1,3 @@ -import os - from actstream.models import Follow from django.conf import settings from django.contrib.auth import get_user_model @@ -28,6 +26,7 @@ from grandchallenge.anatomy.models import BodyStructure from grandchallenge.components.models import ( CIVForObjectMixin, + CIVSetContainerMixin, ComponentInterface, ComponentInterfaceValue, InterfaceKind, @@ -789,7 +788,7 @@ def delete_reader_study_groups_hook(*_, instance: ReaderStudy, using, **__): pass -class DisplaySet(CIVForObjectMixin, UUIDModel): +class DisplaySet(CIVSetContainerMixin, CIVForObjectMixin, UUIDModel): reader_study = models.ForeignKey( ReaderStudy, related_name="display_sets", on_delete=models.PROTECT ) @@ -799,18 +798,6 @@ class DisplaySet(CIVForObjectMixin, UUIDModel): order = models.PositiveIntegerField(default=0) title = models.CharField(max_length=255, default="", blank=True) - def __str__(self): - result = f"{self.title!r}" if self.title else str(self.pk) - - values = [] - for value in self.values.all(): - if value.image: - values.append(value.image.name) - if value.file: - values.append(os.path.basename(value.file.file.name)) - values = ", ".join(values) - return f"{result} ({values})" if values else f"Empty {result}" - def save(self, *args, **kwargs): adding = self._state.adding super().save(*args, **kwargs) From b888ea2b3630ad38ba360cae8b407d32fe1141c3 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Mon, 13 May 2024 13:41:51 +0200 Subject: [PATCH 14/30] Ensure order and title are always first in CIVSet forms renders --- app/grandchallenge/core/forms.py | 13 ++++++++----- app/grandchallenge/reader_studies/forms.py | 17 ++++++++++------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/app/grandchallenge/core/forms.py b/app/grandchallenge/core/forms.py index 4c920e9762..dd0ba187d3 100644 --- a/app/grandchallenge/core/forms.py +++ b/app/grandchallenge/core/forms.py @@ -64,11 +64,14 @@ class CreateUniqueTitleFormMixin: def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.fields["title"] = CharField( - required=False, - initial=self.instance and self.instance.title or "", - max_length=self.Meta.model._meta.get_field("title").max_length, - ) + self.fields = { + "title": CharField( + required=False, + initial=self.instance and self.instance.title or "", + max_length=self.Meta.model._meta.get_field("title").max_length, + ), + **self.fields, + } def clean_title(self): title = self.cleaned_data.get("title") diff --git a/app/grandchallenge/reader_studies/forms.py b/app/grandchallenge/reader_studies/forms.py index b1bee00451..15b07a7e01 100644 --- a/app/grandchallenge/reader_studies/forms.py +++ b/app/grandchallenge/reader_studies/forms.py @@ -666,14 +666,17 @@ class Meta: def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.fields["order"] = IntegerField( - initial=( - self.instance.order - if self.instance - else self.base_obj.next_display_set_order + self.fields = { + "order": IntegerField( + initial=( + self.instance.order + if self.instance + else self.base_obj.next_display_set_order + ), + min_value=0, ), - min_value=0, - ) + **self.fields, + } def _unique_title_query(self, *args, **kwargs): query = super()._unique_title_query(*args, **kwargs) From 88b08a3c52ee7b9348a704f1c231aa95111b4e1a Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Mon, 13 May 2024 13:59:22 +0200 Subject: [PATCH 15/30] Remove full object print in breadcrumb --- .../components/templates/components/civset_confirm_delete.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/grandchallenge/components/templates/components/civset_confirm_delete.html b/app/grandchallenge/components/templates/components/civset_confirm_delete.html index 480cdd6c28..ba2af6ccba 100644 --- a/app/grandchallenge/components/templates/components/civset_confirm_delete.html +++ b/app/grandchallenge/components/templates/components/civset_confirm_delete.html @@ -11,7 +11,7 @@
  • {{ object.base_object|verbose_name_plural|title }}
  • {{ object.base_object.title }}
  • {{ object|verbose_name_plural|title }}
    • -
  • Delete {{ object }}
    • +
  • Delete {{ object.pk }}
    • {% endblock %} From 26fbfb37c4b539cbef14f3cc436bc92e9a428827 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Mon, 13 May 2024 14:08:36 +0200 Subject: [PATCH 16/30] Fix archive item test --- app/tests/archives_tests/test_views.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/app/tests/archives_tests/test_views.py b/app/tests/archives_tests/test_views.py index 00ff60a36d..d050263ff8 100644 --- a/app/tests/archives_tests/test_views.py +++ b/app/tests/archives_tests/test_views.py @@ -648,8 +648,14 @@ def test_archive_items_to_reader_study_update(client, settings): assert response.status_code == 200 - assert f"{im1.name}, {im3.name}" in response.rendered_content - assert f"{im2.name}, {im4.name}" in response.rendered_content + assert ( + f"{image.title}: {im1.name}, {overlay.title}: {im3.name}" + in response.rendered_content + ) + assert ( + f"{image.title}: {im2.name}, {overlay.title}: {im4.name}" + in response.rendered_content + ) @pytest.mark.django_db From 24fbe4b0893de1e7b1173aed2bb4d76e7a016100 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Mon, 13 May 2024 14:19:01 +0200 Subject: [PATCH 17/30] Also add warning about forever deletion to list view --- .../templates/components/civ_set_delete_confirm.html | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/grandchallenge/components/templates/components/civ_set_delete_confirm.html b/app/grandchallenge/components/templates/components/civ_set_delete_confirm.html index dc4f60fa2c..f234da23e2 100644 --- a/app/grandchallenge/components/templates/components/civ_set_delete_confirm.html +++ b/app/grandchallenge/components/templates/components/civ_set_delete_confirm.html @@ -16,6 +16,11 @@

    Delete {{ base_object.title }} {{ base_object.civ_set_model|verbose_name_plu

    No editable {{ base_object.civ_set_model|verbose_name_plural }} selected.

    {% else %}

    Are you sure you want to delete the following {{ delete_count }} {{ base_object.civ_set_model|verbose_name_plural }}?

    +

    + WARNING: + You are not able to undo this action. Once the {{ base_object.civ_set_model|verbose_name_plural }} are deleted + they are deleted forever. +

    {% crispy form %} {% endif %} {% endblock %} From 1a87ca8f37a5f2ba37367b150213c612f5a8ddf6 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Tue, 14 May 2024 10:00:24 +0200 Subject: [PATCH 18/30] Be more explicit about field ordering --- app/grandchallenge/core/forms.py | 15 +++++++-------- app/grandchallenge/reader_studies/forms.py | 19 +++++++++---------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/app/grandchallenge/core/forms.py b/app/grandchallenge/core/forms.py index dd0ba187d3..77a0e97517 100644 --- a/app/grandchallenge/core/forms.py +++ b/app/grandchallenge/core/forms.py @@ -64,14 +64,13 @@ class CreateUniqueTitleFormMixin: def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.fields = { - "title": CharField( - required=False, - initial=self.instance and self.instance.title or "", - max_length=self.Meta.model._meta.get_field("title").max_length, - ), - **self.fields, - } + field_order = list(self.field_order or self.fields.keys()) + self.fields["title"] = CharField( + required=False, + initial=self.instance and self.instance.title or "", + max_length=self.Meta.model._meta.get_field("title").max_length, + ) + self.order_fields(["title", *field_order]) def clean_title(self): title = self.cleaned_data.get("title") diff --git a/app/grandchallenge/reader_studies/forms.py b/app/grandchallenge/reader_studies/forms.py index 15b07a7e01..7525c932d0 100644 --- a/app/grandchallenge/reader_studies/forms.py +++ b/app/grandchallenge/reader_studies/forms.py @@ -666,17 +666,16 @@ class Meta: def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.fields = { - "order": IntegerField( - initial=( - self.instance.order - if self.instance - else self.base_obj.next_display_set_order - ), - min_value=0, + field_order = list(self.field_order or self.fields.keys()) + self.fields["order"] = IntegerField( + initial=( + self.instance.order + if self.instance + else self.base_obj.next_display_set_order ), - **self.fields, - } + min_value=0, + ) + self.order_fields(["order", *field_order]) def _unique_title_query(self, *args, **kwargs): query = super()._unique_title_query(*args, **kwargs) From 651fbf21fb6922edf4ef3b86d16d39e88db19799 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Tue, 14 May 2024 12:02:56 +0200 Subject: [PATCH 19/30] Refactor update and create mixins --- app/grandchallenge/archives/forms.py | 3 +- app/grandchallenge/archives/views.py | 12 +++-- app/grandchallenge/components/forms.py | 9 ++++ app/grandchallenge/components/views.py | 61 ++++++++++------------ app/grandchallenge/reader_studies/forms.py | 5 +- app/grandchallenge/reader_studies/views.py | 12 +++-- 6 files changed, 57 insertions(+), 45 deletions(-) diff --git a/app/grandchallenge/archives/forms.py b/app/grandchallenge/archives/forms.py index 75c0fa86a1..73b4aeeacb 100644 --- a/app/grandchallenge/archives/forms.py +++ b/app/grandchallenge/archives/forms.py @@ -233,7 +233,8 @@ def save(self, *args, **kwargs): class ArchiveItemCreateForm(CreateUniqueTitleFormMixin, MultipleCIVForm): class Meta: model = ArchiveItem - non_civ_fields = ("title",) + base_object_model_field = "archive" + non_interface_fields = ("title",) def _unique_title_query(self, *args, **kwargs): query = super()._unique_title_query(*args, **kwargs) diff --git a/app/grandchallenge/archives/views.py b/app/grandchallenge/archives/views.py index 2a097f09ba..71eb3ae666 100644 --- a/app/grandchallenge/archives/views.py +++ b/app/grandchallenge/archives/views.py @@ -52,10 +52,11 @@ from grandchallenge.components.forms import MultipleCIVForm, NewFileUploadForm from grandchallenge.components.views import ( CIVSetBulkDeleteView, - CIVSetCreateViewMixin, + CIVSetCreateMixin, CIVSetDeleteView, + CIVSetFormMixin, CivSetListView, - CIVSetUpdateViewMixin, + CIVSetUpdateMixin, InterfacesCreateBaseView, MultipleCIVProcessingBaseView, ) @@ -368,7 +369,9 @@ def get_context_data(self, **kwargs): class ArchiveItemUpdateView( - CIVSetUpdateViewMixin, MultipleCIVProcessingBaseView + CIVSetUpdateMixin, + CIVSetFormMixin, + MultipleCIVProcessingBaseView, ): form_class = ArchiveItemUpdateForm permission_required = ( @@ -559,7 +562,8 @@ def get_serializer_class(self): class ArchiveItemCreateView( - CIVSetCreateViewMixin, + CIVSetCreateMixin, + CIVSetFormMixin, MultipleCIVProcessingBaseView, ): form_class = ArchiveItemCreateForm diff --git a/app/grandchallenge/components/forms.py b/app/grandchallenge/components/forms.py index 6be6b67b19..dcc01a2d52 100644 --- a/app/grandchallenge/components/forms.py +++ b/app/grandchallenge/components/forms.py @@ -249,6 +249,15 @@ def _get_default_field(self, *, interface, current_value): user=self.user, ).field + def process_interface_data(self, data): + for slug, value in data.items(): + self.instance.create_civ( + ci_slug=slug, + new_value=value, + user=self.user, + ) + return self.instance + class SingleCIVForm(Form): _possible_widgets = { diff --git a/app/grandchallenge/components/views.py b/app/grandchallenge/components/views.py index 4d8902e4d3..43b36d0be1 100644 --- a/app/grandchallenge/components/views.py +++ b/app/grandchallenge/components/views.py @@ -126,15 +126,18 @@ class MultipleCIVProcessingBaseView( raise_exception = True success_message = None - def process_data_for_object(self, data): - raise NotImplementedError - @property def base_object(self): raise NotImplementedError def form_valid(self, form): - form.instance = self.process_data_for_object(form.cleaned_data) + form.instance = form.process_interface_data( + data={ + k: v + for k, v in form.cleaned_data.items() + if k not in form.Meta.non_interface_fields + } + ) response = super().form_valid(form) return HttpResponse( response.url, @@ -244,14 +247,13 @@ def new_interface_url(self): raise NotImplementedError -class CIVSetUpdateViewMixin(CIVSetFormMixin): - def process_data_for_object(self, data): - """Updates the object""" - instance = self.object - non_civ_data_keys = self.form_class.Meta.non_civ_fields +class CIVSetUpdateMixin: + def form_valid(self, form): + instance = form.instance + data = form.cleaned_data save = False - for key in non_civ_data_keys: + for key in form.Meta.non_interface_fields: value = data.get(key, empty) if value is not empty and value != getattr(instance, key): setattr(instance, key, value) @@ -259,35 +261,26 @@ def process_data_for_object(self, data): if save: instance.save() - for key in data: - if key in non_civ_data_keys: - continue - instance.create_civ( - ci_slug=key, new_value=data[key], user=self.request.user - ) - - return instance - + return super().form_valid(form) -class CIVSetCreateViewMixin(CIVSetFormMixin): - def create_object(self, non_civ_data): - raise NotImplementedError - def process_data_for_object(self, data): - """Creates the object, including interfaces""" - non_civ_data_keys = self.form_class.Meta.non_civ_fields +class CIVSetCreateMixin: + def form_valid(self, form): + model = form.Meta.model + non_civ_data = { + k: v + for k, v in form.cleaned_data.items() + if k in form.Meta.non_interface_fields + } - instance = self.create_object( - non_civ_data={k: data.get(k) for k in non_civ_data_keys} + form.instance = model.objects.create( + **{ + form.Meta.base_object_model_field: self.base_object, + **non_civ_data, + } ) - for key in data: - if key in non_civ_data_keys: - continue - instance.create_civ( - ci_slug=key, new_value=data[key], user=self.request.user - ) - return instance + return super().form_valid(form) class InterfacesCreateBaseView(ObjectPermissionRequiredMixin, TemplateView): diff --git a/app/grandchallenge/reader_studies/forms.py b/app/grandchallenge/reader_studies/forms.py index 7525c932d0..bca73efa70 100644 --- a/app/grandchallenge/reader_studies/forms.py +++ b/app/grandchallenge/reader_studies/forms.py @@ -658,9 +658,10 @@ def save_answers(self): class DisplaySetCreateForm(CreateUniqueTitleFormMixin, MultipleCIVForm): class Meta: model = DisplaySet - non_civ_fields = ( - "order", + base_object_model_field = "reader_study" + non_interface_fields = ( "title", + "order", ) def __init__(self, *args, **kwargs): diff --git a/app/grandchallenge/reader_studies/views.py b/app/grandchallenge/reader_studies/views.py index dc431e864d..d63e4a76ed 100644 --- a/app/grandchallenge/reader_studies/views.py +++ b/app/grandchallenge/reader_studies/views.py @@ -59,10 +59,11 @@ ) from grandchallenge.components.views import ( CIVSetBulkDeleteView, - CIVSetCreateViewMixin, + CIVSetCreateMixin, CIVSetDeleteView, + CIVSetFormMixin, CivSetListView, - CIVSetUpdateViewMixin, + CIVSetUpdateMixin, FileUpdateBaseView, InterfacesCreateBaseView, MultipleCIVProcessingBaseView, @@ -1263,7 +1264,9 @@ def get(self, request, slug): class DisplaySetUpdateView( - CIVSetUpdateViewMixin, MultipleCIVProcessingBaseView + CIVSetUpdateMixin, + CIVSetFormMixin, + MultipleCIVProcessingBaseView, ): form_class = DisplaySetUpdateForm permission_required = ( @@ -1367,7 +1370,8 @@ def get_htmx_url(self): class DisplaySetCreateView( - CIVSetCreateViewMixin, + CIVSetCreateMixin, + CIVSetFormMixin, MultipleCIVProcessingBaseView, ): form_class = DisplaySetCreateForm From f31e436239d74b39835946a300e34c0f62c89043 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Tue, 14 May 2024 12:44:41 +0200 Subject: [PATCH 20/30] Remove overly eager Meta additions --- app/grandchallenge/archives/forms.py | 6 ++++-- app/grandchallenge/archives/models.py | 3 +++ app/grandchallenge/components/views.py | 10 +--------- app/grandchallenge/core/forms.py | 6 +++--- app/grandchallenge/reader_studies/forms.py | 7 ++++--- app/grandchallenge/reader_studies/models.py | 3 +++ 6 files changed, 18 insertions(+), 17 deletions(-) diff --git a/app/grandchallenge/archives/forms.py b/app/grandchallenge/archives/forms.py index 73b4aeeacb..0e10366115 100644 --- a/app/grandchallenge/archives/forms.py +++ b/app/grandchallenge/archives/forms.py @@ -232,10 +232,12 @@ def save(self, *args, **kwargs): class ArchiveItemCreateForm(CreateUniqueTitleFormMixin, MultipleCIVForm): class Meta: - model = ArchiveItem - base_object_model_field = "archive" non_interface_fields = ("title",) + @property + def model(self): + return self.base_obj.civ_set_model + def _unique_title_query(self, *args, **kwargs): query = super()._unique_title_query(*args, **kwargs) return query.filter(archive=self.base_obj) diff --git a/app/grandchallenge/archives/models.py b/app/grandchallenge/archives/models.py index dd1daf8994..cfd94ffaa5 100644 --- a/app/grandchallenge/archives/models.py +++ b/app/grandchallenge/archives/models.py @@ -266,6 +266,9 @@ def civ_sets_related_manager(self): def civ_set_model(self): return ArchiveItem + def create_civ_set(self, data): + self.civ_set_model.objects.create(archive=self, **data) + @property def create_civ_set_url(self): return reverse("archives:item-create", kwargs={"slug": self.slug}) diff --git a/app/grandchallenge/components/views.py b/app/grandchallenge/components/views.py index 43b36d0be1..4762ec504e 100644 --- a/app/grandchallenge/components/views.py +++ b/app/grandchallenge/components/views.py @@ -266,20 +266,12 @@ def form_valid(self, form): class CIVSetCreateMixin: def form_valid(self, form): - model = form.Meta.model non_civ_data = { k: v for k, v in form.cleaned_data.items() if k in form.Meta.non_interface_fields } - - form.instance = model.objects.create( - **{ - form.Meta.base_object_model_field: self.base_object, - **non_civ_data, - } - ) - + self.base_object.create_civ_set(data=non_civ_data) return super().form_valid(form) diff --git a/app/grandchallenge/core/forms.py b/app/grandchallenge/core/forms.py index 77a0e97517..31d79c703d 100644 --- a/app/grandchallenge/core/forms.py +++ b/app/grandchallenge/core/forms.py @@ -68,7 +68,7 @@ def __init__(self, *args, **kwargs): self.fields["title"] = CharField( required=False, initial=self.instance and self.instance.title or "", - max_length=self.Meta.model._meta.get_field("title").max_length, + max_length=self.model._meta.get_field("title").max_length, ) self.order_fields(["title", *field_order]) @@ -76,12 +76,12 @@ def clean_title(self): title = self.cleaned_data.get("title") if title and self._unique_title_query(title).exists(): raise ValidationError( - f"An {self.Meta.model._meta.verbose_name} already exists with this title" + f"An {self.model._meta.verbose_name} already exists with this title" ) return title def _unique_title_query(self, title): - return self.Meta.model.objects.filter( + return self.model.objects.filter( title=title, ) diff --git a/app/grandchallenge/reader_studies/forms.py b/app/grandchallenge/reader_studies/forms.py index bca73efa70..75b1c6aa68 100644 --- a/app/grandchallenge/reader_studies/forms.py +++ b/app/grandchallenge/reader_studies/forms.py @@ -57,7 +57,6 @@ Answer, AnswerType, CategoricalOption, - DisplaySet, Question, ReaderStudy, ReaderStudyPermissionRequest, @@ -657,13 +656,15 @@ def save_answers(self): class DisplaySetCreateForm(CreateUniqueTitleFormMixin, MultipleCIVForm): class Meta: - model = DisplaySet - base_object_model_field = "reader_study" non_interface_fields = ( "title", "order", ) + @property + def model(self): + return self.base_obj.civ_set_model + def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) diff --git a/app/grandchallenge/reader_studies/models.py b/app/grandchallenge/reader_studies/models.py index 67708862cc..07122f49d7 100644 --- a/app/grandchallenge/reader_studies/models.py +++ b/app/grandchallenge/reader_studies/models.py @@ -736,6 +736,9 @@ def civ_sets_related_manager(self): def civ_set_model(self): return DisplaySet + def create_civ_set(self, data): + return self.civ_set_model.objects.create(reader_study=self, **data) + @property def create_civ_set_url(self): return reverse( From 4ac85204793e953a309c8ddb8907314bcb39c03e Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Tue, 14 May 2024 14:32:35 +0200 Subject: [PATCH 21/30] Push civ data processing into the form --- app/grandchallenge/archives/forms.py | 33 ++++++++++++--- app/grandchallenge/archives/models.py | 2 +- app/grandchallenge/archives/views.py | 4 -- app/grandchallenge/components/forms.py | 49 ++++++++++++++++++---- app/grandchallenge/components/views.py | 41 +++--------------- app/grandchallenge/core/forms.py | 14 +++---- app/grandchallenge/reader_studies/forms.py | 33 ++++++++++++--- app/grandchallenge/reader_studies/views.py | 4 -- 8 files changed, 107 insertions(+), 73 deletions(-) diff --git a/app/grandchallenge/archives/forms.py b/app/grandchallenge/archives/forms.py index 0e10366115..16563cb0aa 100644 --- a/app/grandchallenge/archives/forms.py +++ b/app/grandchallenge/archives/forms.py @@ -17,7 +17,11 @@ ArchivePermissionRequest, ) from grandchallenge.cases.forms import UploadRawImagesForm -from grandchallenge.components.forms import MultipleCIVForm +from grandchallenge.components.forms import ( + CIVSetCreateFormMixin, + CIVSetUpdateFormMixin, + MultipleCIVForm, +) from grandchallenge.components.models import ComponentInterface, InterfaceKind from grandchallenge.core.forms import ( CreateUniqueTitleFormMixin, @@ -230,7 +234,7 @@ def save(self, *args, **kwargs): return super().save(*args, **kwargs) -class ArchiveItemCreateForm(CreateUniqueTitleFormMixin, MultipleCIVForm): +class ArchiveItemFormMixin(MultipleCIVForm): class Meta: non_interface_fields = ("title",) @@ -238,10 +242,27 @@ class Meta: def model(self): return self.base_obj.civ_set_model - def _unique_title_query(self, *args, **kwargs): - query = super()._unique_title_query(*args, **kwargs) - return query.filter(archive=self.base_obj) + def unique_title_query(self, *args, **kwargs): + return ( + super() + .unique_title_query(*args, **kwargs) + .filter(archive=self.base_obj) + ) -class ArchiveItemUpdateForm(UpdateUniqueTitleFormMixin, ArchiveItemCreateForm): +class ArchiveItemCreateForm( + ArchiveItemFormMixin, + CreateUniqueTitleFormMixin, + CIVSetCreateFormMixin, + MultipleCIVForm, +): + pass + + +class ArchiveItemUpdateForm( + ArchiveItemFormMixin, + UpdateUniqueTitleFormMixin, + CIVSetUpdateFormMixin, + MultipleCIVForm, +): pass diff --git a/app/grandchallenge/archives/models.py b/app/grandchallenge/archives/models.py index cfd94ffaa5..f5cbc38cd8 100644 --- a/app/grandchallenge/archives/models.py +++ b/app/grandchallenge/archives/models.py @@ -267,7 +267,7 @@ def civ_set_model(self): return ArchiveItem def create_civ_set(self, data): - self.civ_set_model.objects.create(archive=self, **data) + return self.civ_set_model.objects.create(archive=self, **data) @property def create_civ_set_url(self): diff --git a/app/grandchallenge/archives/views.py b/app/grandchallenge/archives/views.py index 71eb3ae666..605eb4be36 100644 --- a/app/grandchallenge/archives/views.py +++ b/app/grandchallenge/archives/views.py @@ -52,11 +52,9 @@ from grandchallenge.components.forms import MultipleCIVForm, NewFileUploadForm from grandchallenge.components.views import ( CIVSetBulkDeleteView, - CIVSetCreateMixin, CIVSetDeleteView, CIVSetFormMixin, CivSetListView, - CIVSetUpdateMixin, InterfacesCreateBaseView, MultipleCIVProcessingBaseView, ) @@ -369,7 +367,6 @@ def get_context_data(self, **kwargs): class ArchiveItemUpdateView( - CIVSetUpdateMixin, CIVSetFormMixin, MultipleCIVProcessingBaseView, ): @@ -562,7 +559,6 @@ def get_serializer_class(self): class ArchiveItemCreateView( - CIVSetCreateMixin, CIVSetFormMixin, MultipleCIVProcessingBaseView, ): diff --git a/app/grandchallenge/components/forms.py b/app/grandchallenge/components/forms.py index dcc01a2d52..0e8fce41c4 100644 --- a/app/grandchallenge/components/forms.py +++ b/app/grandchallenge/components/forms.py @@ -13,6 +13,7 @@ ModelForm, ModelMultipleChoiceField, ) +from django.utils.functional import empty from grandchallenge.algorithms.models import AlgorithmImage from grandchallenge.components.form_fields import InterfaceFormField @@ -106,6 +107,9 @@ class MultipleCIVForm(Form): SelectUploadWidget, } + class Meta: + non_interface_fields = tuple() + def __init__(self, *args, instance, base_obj, user, **kwargs): super().__init__(*args, **kwargs) self.instance = instance @@ -249,14 +253,43 @@ def _get_default_field(self, *, interface, current_value): user=self.user, ).field - def process_interface_data(self, data): - for slug, value in data.items(): - self.instance.create_civ( - ci_slug=slug, - new_value=value, - user=self.user, - ) - return self.instance + def process_object_data(self): + for key, value in self.cleaned_data.items(): + if key not in self.Meta.non_interface_fields: + self.instance.create_civ( + ci_slug=key, + new_value=value, + user=self.user, + ) + + +class CIVSetCreateFormMixin: + instance = None + + def process_object_data(self): + non_civ_data = { + k: v + for k, v in self.cleaned_data.items() + if k in self.Meta.non_interface_fields + } + self.instance = self.base_obj.create_civ_set(data=non_civ_data) + super().process_object_data() + + +class CIVSetUpdateFormMixin: + def process_object_data(self): + instance = self.instance + + save = False + for key in self.Meta.non_interface_fields: + value = self.cleaned_data.get(key, empty) + if value is not empty and value != getattr(instance, key): + setattr(instance, key, value) + save = True + if save: + instance.save() + + super().process_object_data() class SingleCIVForm(Form): diff --git a/app/grandchallenge/components/views.py b/app/grandchallenge/components/views.py index 4762ec504e..99d93aa9b0 100644 --- a/app/grandchallenge/components/views.py +++ b/app/grandchallenge/components/views.py @@ -5,7 +5,7 @@ from django.db.models import Q, TextChoices from django.forms import Media from django.http import HttpResponse -from django.utils.functional import cached_property, empty +from django.utils.functional import cached_property from django.utils.html import format_html from django.views.generic import DeleteView, FormView, ListView, TemplateView from django_filters.rest_framework import DjangoFilterBackend @@ -130,14 +130,11 @@ class MultipleCIVProcessingBaseView( def base_object(self): raise NotImplementedError + def process_data(self, data): + raise NotImplementedError + def form_valid(self, form): - form.instance = form.process_interface_data( - data={ - k: v - for k, v in form.cleaned_data.items() - if k not in form.Meta.non_interface_fields - } - ) + form.process_object_data() response = super().form_valid(form) return HttpResponse( response.url, @@ -247,34 +244,6 @@ def new_interface_url(self): raise NotImplementedError -class CIVSetUpdateMixin: - def form_valid(self, form): - instance = form.instance - data = form.cleaned_data - - save = False - for key in form.Meta.non_interface_fields: - value = data.get(key, empty) - if value is not empty and value != getattr(instance, key): - setattr(instance, key, value) - save = True - if save: - instance.save() - - return super().form_valid(form) - - -class CIVSetCreateMixin: - def form_valid(self, form): - non_civ_data = { - k: v - for k, v in form.cleaned_data.items() - if k in form.Meta.non_interface_fields - } - self.base_object.create_civ_set(data=non_civ_data) - return super().form_valid(form) - - class InterfacesCreateBaseView(ObjectPermissionRequiredMixin, TemplateView): form_class = SingleCIVForm raise_exception = True diff --git a/app/grandchallenge/core/forms.py b/app/grandchallenge/core/forms.py index 31d79c703d..1720812a1a 100644 --- a/app/grandchallenge/core/forms.py +++ b/app/grandchallenge/core/forms.py @@ -58,7 +58,7 @@ class CreateUniqueTitleFormMixin: """ Form mixing creating an item with a unique title. - Base class should have the `Meta.model` and `instance` attributes + Base class should have the `model` and `instance` attributes """ def __init__(self, *args, **kwargs): @@ -74,22 +74,20 @@ def __init__(self, *args, **kwargs): def clean_title(self): title = self.cleaned_data.get("title") - if title and self._unique_title_query(title).exists(): + if title and self.unique_title_query(title).exists(): raise ValidationError( f"An {self.model._meta.verbose_name} already exists with this title" ) return title - def _unique_title_query(self, title): - return self.model.objects.filter( - title=title, - ) + def unique_title_query(self, title): + return self.model.objects.filter(title=title) class UpdateUniqueTitleFormMixin(CreateUniqueTitleFormMixin): - def _unique_title_query(self, *args, **kwargs): + def unique_title_query(self, *args, **kwargs): return ( super() - ._unique_title_query(*args, **kwargs) + .unique_title_query(*args, **kwargs) .exclude(id=self.instance.pk) ) diff --git a/app/grandchallenge/reader_studies/forms.py b/app/grandchallenge/reader_studies/forms.py index 75b1c6aa68..b160fb016a 100644 --- a/app/grandchallenge/reader_studies/forms.py +++ b/app/grandchallenge/reader_studies/forms.py @@ -33,7 +33,11 @@ from django_select2.forms import Select2MultipleWidget from dynamic_forms import DynamicField, DynamicFormMixin -from grandchallenge.components.forms import MultipleCIVForm +from grandchallenge.components.forms import ( + CIVSetCreateFormMixin, + CIVSetUpdateFormMixin, + MultipleCIVForm, +) from grandchallenge.components.models import ComponentInterface from grandchallenge.core.forms import ( CreateUniqueTitleFormMixin, @@ -654,7 +658,7 @@ def save_answers(self): answer.save() -class DisplaySetCreateForm(CreateUniqueTitleFormMixin, MultipleCIVForm): +class DisplaySetFormMixin: class Meta: non_interface_fields = ( "title", @@ -679,12 +683,29 @@ def __init__(self, *args, **kwargs): ) self.order_fields(["order", *field_order]) - def _unique_title_query(self, *args, **kwargs): - query = super()._unique_title_query(*args, **kwargs) - return query.filter(reader_study=self.base_obj) + def unique_title_query(self, *args, **kwargs): + return ( + super() + .unique_title_query(*args, **kwargs) + .filter(reader_study=self.base_obj) + ) + + +class DisplaySetCreateForm( + DisplaySetFormMixin, + CreateUniqueTitleFormMixin, + CIVSetCreateFormMixin, + MultipleCIVForm, +): + pass -class DisplaySetUpdateForm(UpdateUniqueTitleFormMixin, DisplaySetCreateForm): +class DisplaySetUpdateForm( + DisplaySetFormMixin, + UpdateUniqueTitleFormMixin, + CIVSetUpdateFormMixin, + MultipleCIVForm, +): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) if not self.instance.is_editable: diff --git a/app/grandchallenge/reader_studies/views.py b/app/grandchallenge/reader_studies/views.py index d63e4a76ed..6238b28828 100644 --- a/app/grandchallenge/reader_studies/views.py +++ b/app/grandchallenge/reader_studies/views.py @@ -59,11 +59,9 @@ ) from grandchallenge.components.views import ( CIVSetBulkDeleteView, - CIVSetCreateMixin, CIVSetDeleteView, CIVSetFormMixin, CivSetListView, - CIVSetUpdateMixin, FileUpdateBaseView, InterfacesCreateBaseView, MultipleCIVProcessingBaseView, @@ -1264,7 +1262,6 @@ def get(self, request, slug): class DisplaySetUpdateView( - CIVSetUpdateMixin, CIVSetFormMixin, MultipleCIVProcessingBaseView, ): @@ -1370,7 +1367,6 @@ def get_htmx_url(self): class DisplaySetCreateView( - CIVSetCreateMixin, CIVSetFormMixin, MultipleCIVProcessingBaseView, ): From 0e146cf5c999f1b40ae51a69d7395408ff1a8a7c Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Tue, 14 May 2024 14:35:34 +0200 Subject: [PATCH 22/30] Fix unique title mixins name and test --- app/grandchallenge/archives/forms.py | 8 ++++---- app/grandchallenge/core/forms.py | 4 ++-- app/grandchallenge/reader_studies/forms.py | 8 ++++---- app/tests/core_tests/test_forms.py | 12 ++++++------ 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/app/grandchallenge/archives/forms.py b/app/grandchallenge/archives/forms.py index 16563cb0aa..c88e456a65 100644 --- a/app/grandchallenge/archives/forms.py +++ b/app/grandchallenge/archives/forms.py @@ -24,10 +24,10 @@ ) from grandchallenge.components.models import ComponentInterface, InterfaceKind from grandchallenge.core.forms import ( - CreateUniqueTitleFormMixin, PermissionRequestUpdateForm, SaveFormInitMixin, - UpdateUniqueTitleFormMixin, + UniqueTitleCreateFormMixin, + UniqueTitleUpdateFormMixin, WorkstationUserFilterMixin, ) from grandchallenge.core.guardian import get_objects_for_user @@ -252,7 +252,7 @@ def unique_title_query(self, *args, **kwargs): class ArchiveItemCreateForm( ArchiveItemFormMixin, - CreateUniqueTitleFormMixin, + UniqueTitleCreateFormMixin, CIVSetCreateFormMixin, MultipleCIVForm, ): @@ -261,7 +261,7 @@ class ArchiveItemCreateForm( class ArchiveItemUpdateForm( ArchiveItemFormMixin, - UpdateUniqueTitleFormMixin, + UniqueTitleUpdateFormMixin, CIVSetUpdateFormMixin, MultipleCIVForm, ): diff --git a/app/grandchallenge/core/forms.py b/app/grandchallenge/core/forms.py index 1720812a1a..44aa6a6c98 100644 --- a/app/grandchallenge/core/forms.py +++ b/app/grandchallenge/core/forms.py @@ -54,7 +54,7 @@ class Meta: fields = ("status", "rejection_text") -class CreateUniqueTitleFormMixin: +class UniqueTitleCreateFormMixin: """ Form mixing creating an item with a unique title. @@ -84,7 +84,7 @@ def unique_title_query(self, title): return self.model.objects.filter(title=title) -class UpdateUniqueTitleFormMixin(CreateUniqueTitleFormMixin): +class UniqueTitleUpdateFormMixin(UniqueTitleCreateFormMixin): def unique_title_query(self, *args, **kwargs): return ( super() diff --git a/app/grandchallenge/reader_studies/forms.py b/app/grandchallenge/reader_studies/forms.py index b160fb016a..08268cef4e 100644 --- a/app/grandchallenge/reader_studies/forms.py +++ b/app/grandchallenge/reader_studies/forms.py @@ -40,10 +40,10 @@ ) from grandchallenge.components.models import ComponentInterface from grandchallenge.core.forms import ( - CreateUniqueTitleFormMixin, PermissionRequestUpdateForm, SaveFormInitMixin, - UpdateUniqueTitleFormMixin, + UniqueTitleCreateFormMixin, + UniqueTitleUpdateFormMixin, WorkstationUserFilterMixin, ) from grandchallenge.core.layout import Formset @@ -693,7 +693,7 @@ def unique_title_query(self, *args, **kwargs): class DisplaySetCreateForm( DisplaySetFormMixin, - CreateUniqueTitleFormMixin, + UniqueTitleCreateFormMixin, CIVSetCreateFormMixin, MultipleCIVForm, ): @@ -702,7 +702,7 @@ class DisplaySetCreateForm( class DisplaySetUpdateForm( DisplaySetFormMixin, - UpdateUniqueTitleFormMixin, + UniqueTitleUpdateFormMixin, CIVSetUpdateFormMixin, MultipleCIVForm, ): diff --git a/app/tests/core_tests/test_forms.py b/app/tests/core_tests/test_forms.py index bb8cd49018..896acdc190 100644 --- a/app/tests/core_tests/test_forms.py +++ b/app/tests/core_tests/test_forms.py @@ -2,8 +2,8 @@ from django.forms import Form from grandchallenge.core.forms import ( - CreateUniqueTitleFormMixin, - UpdateUniqueTitleFormMixin, + UniqueTitleCreateFormMixin, + UniqueTitleUpdateFormMixin, ) from grandchallenge.reader_studies.models import DisplaySet from tests.reader_studies_tests.factories import DisplaySetFactory @@ -12,7 +12,7 @@ @pytest.mark.django_db @pytest.mark.parametrize( "form_class", - (CreateUniqueTitleFormMixin, UpdateUniqueTitleFormMixin), + (UniqueTitleCreateFormMixin, UniqueTitleUpdateFormMixin), ) @pytest.mark.parametrize( "existing_title,new_title,expected_validity", @@ -29,8 +29,8 @@ def test_unique_title_mixin( ): class TestForm(form_class, Form): - class Meta: - model = DisplaySet # For ease, use an existing model with a title + + model = DisplaySet # For ease, use an existing model with a title def __init__(self, *args, instance, **kwargs): self.instance = instance @@ -42,7 +42,7 @@ def __init__(self, *args, instance, **kwargs): # Adapt for updating instance = None - if form_class == UpdateUniqueTitleFormMixin: + if form_class == UniqueTitleUpdateFormMixin: instance = DisplaySetFactory() form = TestForm( From 2b011fa3ff972ca342544aac10cb6f1ce28c339e Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Fri, 24 May 2024 10:35:10 +0200 Subject: [PATCH 23/30] Cleanup old unused function --- app/grandchallenge/archives/views.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/app/grandchallenge/archives/views.py b/app/grandchallenge/archives/views.py index 605eb4be36..423a7932ce 100644 --- a/app/grandchallenge/archives/views.py +++ b/app/grandchallenge/archives/views.py @@ -598,12 +598,6 @@ def new_interface_url(self): kwargs={"slug": self.base_object.slug}, ) - def create_object(self, non_civ_data): - return ArchiveItem.objects.create( - archive=self.base_object, - **non_civ_data, - ) - def get_success_url(self): return self.return_url From 90ba723177f44e04ed6999672a816c58f108c34a Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Fri, 24 May 2024 10:37:05 +0200 Subject: [PATCH 24/30] Rename CIVSetContainerMixin --- app/grandchallenge/archives/models.py | 6 ++++-- app/grandchallenge/components/models.py | 2 +- app/grandchallenge/reader_studies/models.py | 6 ++++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/grandchallenge/archives/models.py b/app/grandchallenge/archives/models.py index f5cbc38cd8..3eacede8f3 100644 --- a/app/grandchallenge/archives/models.py +++ b/app/grandchallenge/archives/models.py @@ -13,7 +13,7 @@ from grandchallenge.anatomy.models import BodyStructure from grandchallenge.components.models import ( CIVForObjectMixin, - CIVSetContainerMixin, + CIVSetStringRepresentationMixin, ComponentInterfaceValue, ValuesForInterfacesMixin, ) @@ -286,7 +286,9 @@ class ArchiveGroupObjectPermission(GroupObjectPermissionBase): content_object = models.ForeignKey(Archive, on_delete=models.CASCADE) -class ArchiveItem(CIVSetContainerMixin, CIVForObjectMixin, UUIDModel): +class ArchiveItem( + CIVSetStringRepresentationMixin, CIVForObjectMixin, UUIDModel +): archive = models.ForeignKey( Archive, related_name="items", on_delete=models.PROTECT ) diff --git a/app/grandchallenge/components/models.py b/app/grandchallenge/components/models.py index 38167a1620..633e88c57e 100644 --- a/app/grandchallenge/components/models.py +++ b/app/grandchallenge/components/models.py @@ -1962,7 +1962,7 @@ def update_size_in_storage(self): self.size_in_registry = self.calculate_size_in_registry() -class CIVSetContainerMixin: +class CIVSetStringRepresentationMixin: def __str__(self): result = [str(self.pk)] diff --git a/app/grandchallenge/reader_studies/models.py b/app/grandchallenge/reader_studies/models.py index 07122f49d7..d9de2378fc 100644 --- a/app/grandchallenge/reader_studies/models.py +++ b/app/grandchallenge/reader_studies/models.py @@ -26,7 +26,7 @@ from grandchallenge.anatomy.models import BodyStructure from grandchallenge.components.models import ( CIVForObjectMixin, - CIVSetContainerMixin, + CIVSetStringRepresentationMixin, ComponentInterface, ComponentInterfaceValue, InterfaceKind, @@ -791,7 +791,9 @@ def delete_reader_study_groups_hook(*_, instance: ReaderStudy, using, **__): pass -class DisplaySet(CIVSetContainerMixin, CIVForObjectMixin, UUIDModel): +class DisplaySet( + CIVSetStringRepresentationMixin, CIVForObjectMixin, UUIDModel +): reader_study = models.ForeignKey( ReaderStudy, related_name="display_sets", on_delete=models.PROTECT ) From 24b6c1f66659e24d3b8a6fb192e1fd6d0fd0c0cf Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Fri, 24 May 2024 11:15:40 +0200 Subject: [PATCH 25/30] Remove useless function --- app/grandchallenge/reader_studies/views.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/grandchallenge/reader_studies/views.py b/app/grandchallenge/reader_studies/views.py index 6238b28828..e140c92373 100644 --- a/app/grandchallenge/reader_studies/views.py +++ b/app/grandchallenge/reader_studies/views.py @@ -1408,11 +1408,6 @@ def new_interface_url(self): kwargs={"slug": self.base_object.slug}, ) - def create_object(self, non_civ_data): - return DisplaySet.objects.create( - reader_study=self.base_object, **non_civ_data - ) - def get_success_url(self): return self.return_url From e981e06ea06859c10e92cd52ce3c21ce932f0d79 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Fri, 24 May 2024 11:16:20 +0200 Subject: [PATCH 26/30] Remove useless function --- app/grandchallenge/components/views.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/grandchallenge/components/views.py b/app/grandchallenge/components/views.py index 99d93aa9b0..2dee9da176 100644 --- a/app/grandchallenge/components/views.py +++ b/app/grandchallenge/components/views.py @@ -130,9 +130,6 @@ class MultipleCIVProcessingBaseView( def base_object(self): raise NotImplementedError - def process_data(self, data): - raise NotImplementedError - def form_valid(self, form): form.process_object_data() response = super().form_valid(form) From 9c3c737855e848215ade617e8cf1da1904f17906 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Fri, 24 May 2024 14:40:34 +0200 Subject: [PATCH 27/30] Remove extra baseclass of MultipleCIVForm --- app/grandchallenge/archives/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/grandchallenge/archives/forms.py b/app/grandchallenge/archives/forms.py index c88e456a65..7c878bb65e 100644 --- a/app/grandchallenge/archives/forms.py +++ b/app/grandchallenge/archives/forms.py @@ -234,7 +234,7 @@ def save(self, *args, **kwargs): return super().save(*args, **kwargs) -class ArchiveItemFormMixin(MultipleCIVForm): +class ArchiveItemFormMixin: class Meta: non_interface_fields = ("title",) From 3419766887c921730ae5dca50687893fad347fcf Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Tue, 28 May 2024 13:14:17 +0200 Subject: [PATCH 28/30] Remove 'View' suffix to better match current code style --- app/grandchallenge/archives/urls.py | 12 ++++++------ app/grandchallenge/archives/views.py | 10 +++++----- app/grandchallenge/components/views.py | 4 ++-- app/grandchallenge/reader_studies/urls.py | 12 ++++++------ app/grandchallenge/reader_studies/views.py | 10 +++++----- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/app/grandchallenge/archives/urls.py b/app/grandchallenge/archives/urls.py index b3e02a848d..8027c7e089 100644 --- a/app/grandchallenge/archives/urls.py +++ b/app/grandchallenge/archives/urls.py @@ -4,13 +4,13 @@ ArchiveCreate, ArchiveDetail, ArchiveEditorsUpdate, - ArchiveItemBulkDeleteView, + ArchiveItemBulkDelete, ArchiveItemCreateView, - ArchiveItemDeleteView, + ArchiveItemDelete, ArchiveItemInterfaceCreate, ArchiveItemsList, ArchiveItemsToReaderStudyUpdate, - ArchiveItemUpdateView, + ArchiveItemUpdate, ArchiveList, ArchivePermissionRequestCreate, ArchivePermissionRequestList, @@ -66,7 +66,7 @@ ), path( "/items/delete/", - ArchiveItemBulkDeleteView.as_view(), + ArchiveItemBulkDelete.as_view(), name="items-bulk-delete", ), path( @@ -76,12 +76,12 @@ ), path( "/items//delete/", - ArchiveItemDeleteView.as_view(), + ArchiveItemDelete.as_view(), name="item-delete", ), path( "/items//edit/", - ArchiveItemUpdateView.as_view(), + ArchiveItemUpdate.as_view(), name="item-edit", ), path( diff --git a/app/grandchallenge/archives/views.py b/app/grandchallenge/archives/views.py index 423a7932ce..d7f4780853 100644 --- a/app/grandchallenge/archives/views.py +++ b/app/grandchallenge/archives/views.py @@ -51,8 +51,8 @@ from grandchallenge.cases.models import Image, RawImageUploadSession from grandchallenge.components.forms import MultipleCIVForm, NewFileUploadForm from grandchallenge.components.views import ( - CIVSetBulkDeleteView, - CIVSetDeleteView, + CIVSetBulkDelete, + CIVSetDelete, CIVSetFormMixin, CivSetListView, InterfacesCreateBaseView, @@ -366,7 +366,7 @@ def get_context_data(self, **kwargs): return context -class ArchiveItemUpdateView( +class ArchiveItemUpdate( CIVSetFormMixin, MultipleCIVProcessingBaseView, ): @@ -640,14 +640,14 @@ def get_htmx_url(self): ) -class ArchiveItemDeleteView(CIVSetDeleteView): +class ArchiveItemDelete(CIVSetDelete): model = ArchiveItem permission_required = ( f"{Archive._meta.app_label}.delete_{ArchiveItem._meta.model_name}" ) -class ArchiveItemBulkDeleteView(CIVSetBulkDeleteView): +class ArchiveItemBulkDelete(CIVSetBulkDelete): model = ArchiveItem @property diff --git a/app/grandchallenge/components/views.py b/app/grandchallenge/components/views.py index 2dee9da176..970cb0ad92 100644 --- a/app/grandchallenge/components/views.py +++ b/app/grandchallenge/components/views.py @@ -270,7 +270,7 @@ def get_context_data(self, *args, **kwargs): return context -class CIVSetDeleteView( +class CIVSetDelete( LoginRequiredMixin, ObjectPermissionRequiredMixin, SuccessMessageMixin, @@ -341,7 +341,7 @@ def get_queryset(self): ) -class CIVSetBulkDeleteView(LoginRequiredMixin, FormView): +class CIVSetBulkDelete(LoginRequiredMixin, FormView): form_class = CIVSetDeleteForm model = None template_name = "components/civ_set_delete_confirm.html" diff --git a/app/grandchallenge/reader_studies/urls.py b/app/grandchallenge/reader_studies/urls.py index e5966885bd..16fd4e538f 100644 --- a/app/grandchallenge/reader_studies/urls.py +++ b/app/grandchallenge/reader_studies/urls.py @@ -6,9 +6,9 @@ AddQuestionToReaderStudy, AnswersRemoveForUser, AnswersRemoveGroundTruth, - DisplaySetBulkDeleteView, - DisplaySetCreateView, - DisplaySetDeleteView, + DisplaySetBulkDelete, + DisplaySetCreate, + DisplaySetDelete, DisplaySetFilesUpdate, DisplaySetInterfacesCreate, DisplaySetUpdateView, @@ -85,12 +85,12 @@ ), path( "/display-sets/delete/", - DisplaySetBulkDeleteView.as_view(), + DisplaySetBulkDelete.as_view(), name="display-sets-bulk-delete", ), path( "/display-sets/create-single/", - DisplaySetCreateView.as_view(), + DisplaySetCreate.as_view(), name="display-set-create", ), path( @@ -100,7 +100,7 @@ ), path( "/display-sets//delete/", - DisplaySetDeleteView.as_view(), + DisplaySetDelete.as_view(), name="display-set-delete", ), path( diff --git a/app/grandchallenge/reader_studies/views.py b/app/grandchallenge/reader_studies/views.py index e140c92373..7d0b154f73 100644 --- a/app/grandchallenge/reader_studies/views.py +++ b/app/grandchallenge/reader_studies/views.py @@ -58,8 +58,8 @@ ComponentInterfaceValuePostSerializer, ) from grandchallenge.components.views import ( - CIVSetBulkDeleteView, - CIVSetDeleteView, + CIVSetBulkDelete, + CIVSetDelete, CIVSetFormMixin, CivSetListView, FileUpdateBaseView, @@ -1366,7 +1366,7 @@ def get_htmx_url(self): ) -class DisplaySetCreateView( +class DisplaySetCreate( CIVSetFormMixin, MultipleCIVProcessingBaseView, ): @@ -1412,14 +1412,14 @@ def get_success_url(self): return self.return_url -class DisplaySetDeleteView(CIVSetDeleteView): +class DisplaySetDelete(CIVSetDelete): model = DisplaySet permission_required = ( f"{ReaderStudy._meta.app_label}.delete_{DisplaySet._meta.model_name}" ) -class DisplaySetBulkDeleteView(CIVSetBulkDeleteView): +class DisplaySetBulkDelete(CIVSetBulkDelete): model = DisplaySet @property From e5faecc96db2ac6baaffbc63d6902d6cc06cff6b Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Tue, 28 May 2024 14:22:20 +0200 Subject: [PATCH 29/30] Remove transactional contexts for when testing display set title manipulations --- app/tests/reader_studies_tests/test_models.py | 56 +++++++++++-------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/app/tests/reader_studies_tests/test_models.py b/app/tests/reader_studies_tests/test_models.py index c0e4fa6e55..39bcde0e67 100644 --- a/app/tests/reader_studies_tests/test_models.py +++ b/app/tests/reader_studies_tests/test_models.py @@ -2,7 +2,6 @@ import pytest from django.core.exceptions import ObjectDoesNotExist, ValidationError -from django.db import transaction from django.db.models import ProtectedError from django.db.utils import IntegrityError @@ -678,42 +677,51 @@ def test_display_set_description(): assert ds.description == result[ds.pk] -@pytest.mark.django_db -def test_display_set_title(): +@pytest.fixture(scope="function") +def display_set_with_title(db): rs = ReaderStudyFactory() - ds0 = DisplaySetFactory(reader_study=rs) + ds = DisplaySetFactory(reader_study=rs) # Default - assert ds0.title == "" + assert ds.title == "" # Update - ds0.title = "A Display Set Title" - ds0.save() + ds.title = "A Display Set Title" + ds.save() + + return ds + +@pytest.mark.django_db +def test_display_set_duplicate_title_edit(display_set_with_title): # Sanity - ds1 = DisplaySetFactory( - reader_study=ds0.reader_study, + ds = DisplaySetFactory( + reader_study=display_set_with_title.reader_study, title="Another Display Set", ) - # Duplication attempt via edit - ds1.title = ds0.title - with transaction.atomic(): - with pytest.raises(IntegrityError): - ds1.save() - - # Duplication attempt via save - with transaction.atomic(): - with pytest.raises(IntegrityError): - DisplaySetFactory( - reader_study=rs, - title=ds0.title, - ) + ds.title = display_set_with_title.title + with pytest.raises(IntegrityError): + ds.save() - # Other reader study is no problem + +@pytest.mark.django_db +def test_display_set_duplicate_title_create(display_set_with_title): + with pytest.raises(IntegrityError): + DisplaySetFactory( + reader_study=display_set_with_title.reader_study, + title=display_set_with_title.title, + ) + + +@pytest.mark.django_db +def test_display_set_duplicate_title_other_reader_study( + display_set_with_title, +): + # Another reader study is not problem DisplaySetFactory( reader_study=ReaderStudyFactory(), - title=ds0.title, + title=display_set_with_title.title, ) From d05dc882c8a8f56ca52b7f941650d8050fac8b49 Mon Sep 17 00:00:00 2001 From: Chris van Run Date: Tue, 28 May 2024 14:30:43 +0200 Subject: [PATCH 30/30] Remove transactional contexts for when testing archive item title manipulations --- app/tests/archives_tests/test_models.py | 48 +++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/app/tests/archives_tests/test_models.py b/app/tests/archives_tests/test_models.py index cc225400c2..f7f5a739ff 100644 --- a/app/tests/archives_tests/test_models.py +++ b/app/tests/archives_tests/test_models.py @@ -18,6 +18,54 @@ def create_archive_items_for_images(images, archive): ai.values.add(civ) +@pytest.fixture(scope="function") +def archive_item_with_title(db): + archive = ArchiveFactory() + ai = ArchiveItemFactory(archive=archive) + + # Default + assert ai.title == "" + + # Update + ai.title = "An Archive Item Title" + ai.save() + + return ai + + +@pytest.mark.django_db +def test_archive_item_duplicate_title_edit(archive_item_with_title): + # Sanity + ai = ArchiveItemFactory( + archive=archive_item_with_title.archive, + title="Another Archive Item", + ) + + ai.title = archive_item_with_title.title + with pytest.raises(IntegrityError): + ai.save() + + +@pytest.mark.django_db +def test_archive_item_duplicate_title_create(archive_item_with_title): + with pytest.raises(IntegrityError): + ArchiveItemFactory( + archive=archive_item_with_title.archive, + title=archive_item_with_title.title, + ) + + +@pytest.mark.django_db +def test_archive_item_duplicate_title_other_archive( + archive_item_with_title, +): + # Another archive is not a problem + ArchiveItemFactory( + archive=ArchiveFactory(), + title=archive_item_with_title.title, + ) + + @pytest.mark.django_db def test_archive_item_set_title(): archive = ArchiveFactory()