Remove on_commit for display set saving and deletion

Author: jmsmkn

app/grandchallenge/archives/signals.py

@@ -1,6 +1,5 @@
 from django.db.models.signals import m2m_changed, post_save, pre_delete
 from django.dispatch import receiver
-from guardian.shortcuts import assign_perm
 
 from grandchallenge.archives.models import ArchiveItem
 from grandchallenge.cases.models import Image
@@ -24,15 +23,6 @@ def update_permissions_on_archive_item_changed(
         else:
             archive_items = model.objects.filter(pk__in=pk_set)
 
-        archive_items = archive_items.select_related(
-            "archive__editors_group",
-            "archive__uploaders_group",
-            "archive__users_group",
-        ).only(
-            "archive__editors_group",
-            "archive__uploaders_group",
-            "archive__users_group",
-        )
     else:
         archive_items = [instance]
 
@@ -48,29 +38,18 @@ def update_permissions_on_archive_item_changed(
                 componentinterfacevalue__pk__in=pk_set
             )
 
-    if action == "post_add":
-        for archive_item in archive_items:
-            for image in images:
-                groups = [
-                    archive_item.archive.editors_group,
-                    archive_item.archive.uploaders_group,
-                    archive_item.archive.users_group,
-                ]
-                assign_perm("view_image", groups, image)
+    exclude_archive_items = archive_items if action == "pre_clear" else None
 
-    elif action in {"post_remove", "pre_clear"}:
-        for image in images:
-            image.update_viewer_groups_permissions(
-                exclude_archive_items=archive_items
-            )
-    else:
-        raise NotImplementedError
+    for image in images:
+        image.update_viewer_groups_permissions(
+            exclude_archive_items=exclude_archive_items
+        )
 
 
 @receiver(pre_delete, sender=ArchiveItem)
 @receiver(post_save, sender=ArchiveItem)
 def update_permissions_on_archive_item_change(
-    *_, instance: ArchiveItem, signal, **__
+    *, instance: ArchiveItem, signal, **__
 ):
     images = Image.objects.filter(
         componentinterfacevalue__archive_items=instance

app/grandchallenge/cases/models.py

@@ -674,6 +674,7 @@ def update_viewer_groups_permissions(  # noqa: C901
         *,
         exclude_jobs=None,
         exclude_archive_items=None,
+        exclude_display_sets=None,
     ):
         """
         Update the permissions for the algorithm jobs viewers groups to
@@ -689,7 +690,7 @@ def update_viewer_groups_permissions(  # noqa: C901
         """
         from grandchallenge.algorithms.models import Job
         from grandchallenge.archives.models import ArchiveItem
-        from grandchallenge.reader_studies.models import ReaderStudy
+        from grandchallenge.reader_studies.models import DisplaySet
 
         expected_groups = set()
 
@@ -740,13 +741,28 @@ def update_viewer_groups_permissions(  # noqa: C901
                 ]
             )
 
-        for rs in ReaderStudy.objects.filter(
-            display_sets__values__image=self
-        ).select_related("editors_group", "readers_group"):
+        display_set_queryset = (
+            DisplaySet.objects.filter(values__image=self)
+            .select_related(
+                "reader_study__editors_group",
+                "reader_study__readers_group",
+            )
+            .only(
+                "reader_study__editors_group",
+                "reader_study__readers_group",
+            )
+        )
+
+        if exclude_display_sets is not None:
+            display_set_queryset = display_set_queryset.exclude(
+                pk__in={ds.pk for ds in exclude_display_sets}
+            )
+
+        for display_set in display_set_queryset:
             expected_groups.update(
                 [
-                    rs.editors_group,
-                    rs.readers_group,
+                    display_set.reader_study.editors_group,
+                    display_set.reader_study.readers_group,
                 ]
             )
 

app/grandchallenge/reader_studies/signals.py

@@ -5,7 +5,6 @@
     pre_delete,
     pre_save,
 )
-from django.db.transaction import on_commit
 from django.dispatch import receiver
 
 from grandchallenge.cases.models import Image
@@ -14,15 +13,25 @@
 
 @receiver(m2m_changed, sender=DisplaySet.values.through)
 def update_permissions_on_display_set_changed(
-    instance, action, reverse, pk_set, **_
+    *, instance, action, reverse, model, pk_set, **_
 ):
     if action not in ["post_add", "post_remove", "pre_clear"]:
         # nothing to do for the other actions
         return
 
     if reverse:
         images = Image.objects.filter(componentinterfacevalue__pk=instance.pk)
+
+        if pk_set is None:
+            # When using a _clear action, pk_set is None
+            # https://docs.djangoproject.com/en/2.2/ref/signals/#m2m-changed
+            display_sets = instance.display_sets.all()
+        else:
+            display_sets = model.objects.filter(pk__in=pk_set)
+
     else:
+        display_sets = [instance]
+
         if pk_set is None:
             # When using a _clear action, pk_set is None
             # https://docs.djangoproject.com/en/2.2/ref/signals/#m2m-changed
@@ -35,23 +44,26 @@ def update_permissions_on_display_set_changed(
                 componentinterfacevalue__pk__in=pk_set
             )
 
-    def update_permissions():
-        for image in images:
-            image.update_viewer_groups_permissions()
+    exclude_display_sets = display_sets if action == "pre_clear" else None
 
-    on_commit(update_permissions)
+    for image in images:
+        image.update_viewer_groups_permissions(
+            exclude_display_sets=exclude_display_sets
+        )
 
 
 @receiver(pre_delete, sender=DisplaySet)
 @receiver(post_save, sender=DisplaySet)
-def update_view_image_permissions(*_, instance: DisplaySet, **__):
-    images = [civ.image for civ in instance.values.filter(image__isnull=False)]
-
-    def update_permissions():
-        for image in images:
-            image.update_viewer_groups_permissions()
-
-    on_commit(update_permissions)
+def update_view_image_permissions(*, instance: DisplaySet, signal, **__):
+    images = Image.objects.filter(
+        componentinterfacevalue__display_sets=instance
+    ).distinct()
+    exclude_display_sets = [instance] if signal is pre_delete else None
+
+    for image in images:
+        image.update_viewer_groups_permissions(
+            exclude_display_sets=exclude_display_sets
+        )
 
 
 @receiver(m2m_changed, sender=DisplaySet.values.through)