From 914452b57d2b324cdf36d4795f365dd7cec3cebd Mon Sep 17 00:00:00 2001 From: amickan Date: Thu, 12 Dec 2024 11:02:36 +0100 Subject: [PATCH] Remove add functionality from admin, refactor form --- app/grandchallenge/algorithms/admin.py | 17 ++--- app/grandchallenge/algorithms/forms.py | 88 +++++++++--------------- app/grandchallenge/algorithms/views.py | 8 +-- app/tests/algorithms_tests/test_forms.py | 65 +++++++++-------- app/tests/algorithms_tests/test_views.py | 20 +----- 5 files changed, 78 insertions(+), 120 deletions(-) diff --git a/app/grandchallenge/algorithms/admin.py b/app/grandchallenge/algorithms/admin.py index 03a32c6d6..2ed5eb570 100644 --- a/app/grandchallenge/algorithms/admin.py +++ b/app/grandchallenge/algorithms/admin.py @@ -2,14 +2,13 @@ from django.contrib import admin from django.contrib.admin import ModelAdmin -from django.core.exceptions import ObjectDoesNotExist, ValidationError +from django.core.exceptions import ObjectDoesNotExist from django.db.models import Count, Sum from django.forms import ModelForm from django.urls import reverse from django.utils.html import format_html from guardian.admin import GuardedModelAdmin -from grandchallenge.algorithms.forms import AlgorithmInterfaceBaseForm from grandchallenge.algorithms.models import ( Algorithm, AlgorithmAlgorithmInterface, @@ -258,16 +257,6 @@ class AlgorithmModelAdmin(GuardedModelAdmin): readonly_fields = ("creator", "algorithm", "sha256", "size_in_storage") -class AlgorithmInterfaceAdminForm(AlgorithmInterfaceBaseForm): - def clean(self): - cleaned_data = super().clean() - if cleaned_data["existing_io"]: - raise ValidationError( - "An AlgorithmIO with the same combination of inputs and outputs already exists." - ) - return cleaned_data - - @admin.register(AlgorithmInterface) class AlgorithmInterfaceAdmin(GuardedModelAdmin): list_display = ( @@ -279,7 +268,6 @@ class AlgorithmInterfaceAdmin(GuardedModelAdmin): "inputs__slug", "outputs__slug", ) - form = AlgorithmInterfaceAdminForm def algorithm_inputs(self, obj): return oxford_comma(obj.inputs.all()) @@ -293,6 +281,9 @@ def has_change_permission(self, request, obj=None): def has_delete_permission(self, request, obj=None): return False + def has_add_permission(self, request, obj=None): + return False + @admin.register(AlgorithmAlgorithmInterface) class AlgorithmAlgorithmInterfaceAdmin(GuardedModelAdmin): diff --git a/app/grandchallenge/algorithms/forms.py b/app/grandchallenge/algorithms/forms.py index 3da1daf3c..a08319d35 100644 --- a/app/grandchallenge/algorithms/forms.py +++ b/app/grandchallenge/algorithms/forms.py @@ -52,7 +52,6 @@ AlgorithmModel, AlgorithmPermissionRequest, Job, - get_existing_interface_for_inputs_and_outputs, ) from grandchallenge.algorithms.serializers import ( AlgorithmImageSerializer, @@ -1348,7 +1347,7 @@ def clean_algorithm_model(self): return algorithm_model -class AlgorithmInterfaceBaseForm(SaveFormInitMixin, ModelForm): +class AlgorithmInterfaceForm(SaveFormInitMixin, ModelForm): inputs = ModelMultipleChoiceField( queryset=ComponentInterface.objects.exclude( slug__in=[*NON_ALGORITHM_INTERFACES, "results-json-file"] @@ -1361,10 +1360,22 @@ class AlgorithmInterfaceBaseForm(SaveFormInitMixin, ModelForm): ), widget=Select2MultipleWidget, ) + set_as_default = BooleanField(required=False) class Meta: model = AlgorithmInterface - fields = ("inputs", "outputs") + fields = ( + "inputs", + "outputs", + "set_as_default", + ) + + def __init__(self, *args, algorithm, **kwargs): + super().__init__(*args, **kwargs) + self._algorithm = algorithm + + if not self._algorithm.default_interface: + self.fields["set_as_default"].initial = True def clean(self): cleaned_data = super().clean() @@ -1385,56 +1396,12 @@ def clean(self): f"{oxford_comma(duplicate_interfaces)} present in both" ) - cleaned_data["existing_io"] = ( - get_existing_interface_for_inputs_and_outputs( - inputs=inputs, - outputs=outputs, - ) - ) - - return cleaned_data - - -class AlgorithmInterfaceGetOrCreateForm(AlgorithmInterfaceBaseForm): - set_as_default = BooleanField(required=False) - - class Meta(AlgorithmInterfaceBaseForm.Meta): - fields = ( - *AlgorithmInterfaceBaseForm.Meta.fields, - "set_as_default", - ) - - def __init__(self, *args, algorithm, **kwargs): - super().__init__(*args, **kwargs) - self._algorithm = algorithm - - if not self._algorithm.default_interface: - self.fields["set_as_default"].initial = True - - def clean(self): - cleaned_data = super().clean() - - if ( - cleaned_data["existing_io"] - and AlgorithmAlgorithmInterface.objects.filter( - algorithm=self._algorithm, - interface=cleaned_data["existing_io"], - ).exists() - ): - raise ValidationError( - "Your algorithm already has an interface with these inputs " - "and outputs. If you would like to update the 'is_default' " - "property of the interface, you can do so by updating the " - "existing interface on the interface list." - ) - return cleaned_data def save(self): - io = ( - self.cleaned_data["existing_io"] - if self.cleaned_data["existing_io"] - else super().save() + interface = AlgorithmInterface.objects.create( + inputs=self.cleaned_data["inputs"], + outputs=self.cleaned_data["outputs"], ) if self.cleaned_data["set_as_default"]: @@ -1442,11 +1409,18 @@ def save(self): algorithm=self._algorithm ).update(is_default=False) - self._algorithm.interfaces.add( - io, - through_defaults={ - "is_default": self.cleaned_data["set_as_default"] - }, - ) + if AlgorithmAlgorithmInterface.objects.filter( + algorithm=self._algorithm, interface=interface + ).exists(): + AlgorithmAlgorithmInterface.objects.filter( + algorithm=self._algorithm, interface=interface + ).update(is_default=self.cleaned_data["set_as_default"]) + else: + self._algorithm.interfaces.add( + interface, + through_defaults={ + "is_default": self.cleaned_data["set_as_default"] + }, + ) - return io + return interface diff --git a/app/grandchallenge/algorithms/views.py b/app/grandchallenge/algorithms/views.py index 361722226..db9f837d6 100644 --- a/app/grandchallenge/algorithms/views.py +++ b/app/grandchallenge/algorithms/views.py @@ -47,7 +47,7 @@ AlgorithmImageForm, AlgorithmImageUpdateForm, AlgorithmImportForm, - AlgorithmInterfaceGetOrCreateForm, + AlgorithmInterfaceForm, AlgorithmModelForm, AlgorithmModelUpdateForm, AlgorithmModelVersionControlForm, @@ -1097,9 +1097,7 @@ def get(self, *_, **__): ) -class AlgorithmInterfacePermissionMixin( - VerificationRequiredMixin, AccessMixin -): +class AlgorithmInterfacePermissionMixin(AccessMixin): @property def algorithm(self): return get_object_or_404(Algorithm, slug=self.kwargs["slug"]) @@ -1117,7 +1115,7 @@ class AlgorithmInterfaceForAlgorithmCreate( AlgorithmInterfacePermissionMixin, CreateView ): model = AlgorithmInterface - form_class = AlgorithmInterfaceGetOrCreateForm + form_class = AlgorithmInterfaceForm success_message = "Algorithm interface successfully added" def get_success_url(self): diff --git a/app/tests/algorithms_tests/test_forms.py b/app/tests/algorithms_tests/test_forms.py index fa627200d..f416fe8b1 100644 --- a/app/tests/algorithms_tests/test_forms.py +++ b/app/tests/algorithms_tests/test_forms.py @@ -5,11 +5,10 @@ from django.core.validators import MaxValueValidator, MinValueValidator from factory.django import ImageField -from grandchallenge.algorithms.admin import AlgorithmInterfaceAdminForm from grandchallenge.algorithms.forms import ( AlgorithmForm, AlgorithmForPhaseForm, - AlgorithmInterfaceGetOrCreateForm, + AlgorithmInterfaceForm, AlgorithmModelForm, AlgorithmModelVersionControlForm, AlgorithmPublishForm, @@ -1299,27 +1298,10 @@ def test_algorithm_for_phase_form_memory_limited(): assert max_validator.limit_value == 32 -@pytest.mark.django_db -def test_algorithm_interface_check_for_uniqueness(): - interface = AlgorithmInterfaceFactory() - ci1, ci2 = ComponentInterfaceFactory.create_batch(2) - interface.inputs.set([ci1]) - interface.outputs.set([ci2]) - - form = AlgorithmInterfaceAdminForm( - data={"inputs": [ci1], "outputs": [ci2]} - ) - assert form.is_valid() is False - assert ( - "An AlgorithmIO with the same combination of inputs and outputs already exists." - in str(form.errors) - ) - - @pytest.mark.django_db def test_algorithm_interface_disjoint_interfaces(): ci = ComponentInterfaceFactory() - form = AlgorithmInterfaceAdminForm(data={"inputs": [ci], "outputs": [ci]}) + form = AlgorithmInterfaceForm(data={"inputs": [ci], "outputs": [ci]}) assert form.is_valid() is False assert "The sets of Inputs and Outputs must be unique" in str(form.errors) @@ -1351,12 +1333,12 @@ def test_algorithm_for_phase_form_memory(): assert max_validator.limit_value == 42 -class TestAlgorithmInterfaceGetOrCreateForm: +class TestAlgorithmInterfaceForm: @pytest.mark.django_db def test_set_as_default_initial_value(self): alg = AlgorithmFactory() - form = AlgorithmInterfaceGetOrCreateForm( + form = AlgorithmInterfaceForm( algorithm=alg, ) assert form.fields["set_as_default"].initial @@ -1367,7 +1349,7 @@ def test_set_as_default_initial_value(self): del alg.default_interface - form = AlgorithmInterfaceGetOrCreateForm( + form = AlgorithmInterfaceForm( algorithm=alg, ) assert not form.fields["set_as_default"].initial @@ -1382,7 +1364,7 @@ def test_existing_io_is_reused(self): alg = AlgorithmFactory() - form = AlgorithmInterfaceGetOrCreateForm( + form = AlgorithmInterfaceForm( algorithm=alg, data={ "inputs": [inp.pk], @@ -1404,7 +1386,7 @@ def test_new_default_interface_updates_related_interfaces(self): old_iot = AlgorithmAlgorithmInterface.objects.get() - form = AlgorithmInterfaceGetOrCreateForm( + form = AlgorithmInterfaceForm( algorithm=alg, data={ "inputs": [ci_1.pk], @@ -1422,7 +1404,7 @@ def test_new_default_interface_updates_related_interfaces(self): assert not old_iot.is_default @pytest.mark.django_db - def test_default_interface_for_algorithm_not_altered_when_adding_new_non_default_interface( + def test_default_interface_for_algorithm_not_updated_when_adding_new_non_default_interface( self, ): alg = AlgorithmFactory() @@ -1432,7 +1414,7 @@ def test_default_interface_for_algorithm_not_altered_when_adding_new_non_default ci_1, ci_2 = ComponentInterfaceFactory.create_batch(2) - form = AlgorithmInterfaceGetOrCreateForm( + form = AlgorithmInterfaceForm( algorithm=alg, data={ "inputs": [ci_1.pk], @@ -1448,3 +1430,32 @@ def test_default_interface_for_algorithm_not_altered_when_adding_new_non_default assert new_io != io assert not new_iot.is_default assert old_iot.is_default + + @pytest.mark.django_db + def test_is_default_is_updated_when_adding_an_already_existing_interface( + self, + ): + alg = AlgorithmFactory() + ci_1, ci_2 = ComponentInterfaceFactory.create_batch(2) + + io = AlgorithmInterfaceFactory() + io.inputs.set([ci_1]) + io.outputs.set([ci_2]) + + alg.interfaces.add(io, through_defaults={"is_default": True}) + old_iot = AlgorithmAlgorithmInterface.objects.get() + + form = AlgorithmInterfaceForm( + algorithm=alg, + data={ + "inputs": [ci_1.pk], + "outputs": [ci_2.pk], + "set_as_default": False, + }, + ) + form.is_valid() + new_io = form.save() + old_iot.refresh_from_db() + + assert new_io == io + assert not old_iot.is_default diff --git a/app/tests/algorithms_tests/test_views.py b/app/tests/algorithms_tests/test_views.py index 66baafe55..822110fbe 100644 --- a/app/tests/algorithms_tests/test_views.py +++ b/app/tests/algorithms_tests/test_views.py @@ -2157,33 +2157,19 @@ def test_algorithm_interface_view_permission(client, viewname): user_without_alg_add_perm, algorithm_editor_with_alg_add, algorithm_editor_without_alg_add, - verified_alg_editor_with_alg_add, - verified_alg_editor_without_alg_add, - ) = UserFactory.create_batch(6) + ) = UserFactory.create_batch(4) assign_perm("algorithms.add_algorithm", user_with_alg_add_perm) assign_perm("algorithms.add_algorithm", algorithm_editor_with_alg_add) - assign_perm("algorithms.add_algorithm", verified_alg_editor_with_alg_add) alg = AlgorithmFactory() alg.add_editor(algorithm_editor_with_alg_add) alg.add_editor(algorithm_editor_without_alg_add) - alg.add_editor(verified_alg_editor_with_alg_add) - alg.add_editor(verified_alg_editor_without_alg_add) - - VerificationFactory( - user=verified_alg_editor_with_alg_add, is_verified=True - ) - VerificationFactory( - user=verified_alg_editor_without_alg_add, is_verified=True - ) for user, status in [ [user_with_alg_add_perm, 403], [user_without_alg_add_perm, 403], - [algorithm_editor_with_alg_add, 403], + [algorithm_editor_with_alg_add, 200], [algorithm_editor_without_alg_add, 403], - [verified_alg_editor_with_alg_add, 200], - [verified_alg_editor_without_alg_add, 403], ]: response = get_view_for_user( viewname=viewname, @@ -2201,8 +2187,6 @@ def test_algorithm_interface_create(client): alg = AlgorithmFactory() alg.add_editor(user) - VerificationFactory(user=user, is_verified=True) - ci_1 = ComponentInterfaceFactory() ci_2 = ComponentInterfaceFactory()