👌 [#516] PR Feedback

backend/bin/liveness_celery.sh

@@ -0,0 +1,25 @@
+#!/bin/bash
+
+    # Exit immediately if a non-zero exit code is returned
+    set -e
+
+    QUEUE_NAME=${CELERY_WORKER_QUEUE:-celery}
+
+    # Check if CELERY_WORKER_NAME is set
+    if [ -z "$CELERY_WORKER_NAME" ]; then
+      # If not set
+      WORKER_NAME="${QUEUE_NAME}@${HOSTNAME}"
+    else
+      # If set and contains the '@' symbol
+      if [[ "$CELERY_WORKER_NAME" != *"@"* ]]; then
+        # If not, update CELERY_WORKER_NAME to celery@${CELERY_WORKER_NAME}
+        WORKER_NAME="celery@${CELERY_WORKER_NAME}"
+      else
+        WORKER_NAME="${CELERY_WORKER_NAME}"
+      fi
+    fi
+
+    echo "$WORKER_NAME"
+
+    # Use CELERY_WORKER_NAME to set the --destination flag
+    # celery --workdir src --app openarchiefbeheer.celery inspect --destination "$WORKER_NAME" active

backend/src/openarchiefbeheer/destruction/api/mixins.py

@@ -1,26 +1,27 @@
 from django.utils.translation import gettext_lazy as _
 
 from rest_framework.exceptions import ValidationError
-from rest_framework.request import Request
 
 from ..models import DestructionList
 
 
 class DestructionListChecksMixin:
-    def get_destruction_list(self, request: Request) -> DestructionList:
+    """Check the permissions on a destruction list
+
+    Used by APIView endpoints that create a resource related to a destruction list (for example a review/co-review)
+    and need to check based on the rights of the user, the status and the assignees of the destruction list
+    whether the action is allowed.
+    """
+
+    def create(self, request, *args, **kwargs):
         try:
-            return DestructionList.objects.get(
+            destruction_list = DestructionList.objects.get(
                 uuid=request.data.get("destruction_list")
             )
         except DestructionList.DoesNotExist:
             raise ValidationError(
                 detail={"destruction_list": _("The destruction list does not exist.")}
             )
 
-    def check_destruction_list_permissions(self, request: Request) -> None:
-        destruction_list = self.get_destruction_list(request)
         self.check_object_permissions(self.request, destruction_list)
-
-    def create(self, request, *args, **kwargs):
-        self.check_destruction_list_permissions(request)
         return super().create(request, *args, **kwargs)

backend/src/openarchiefbeheer/destruction/api/permissions.py

@@ -29,20 +29,15 @@ def has_object_permission(self, request, view, destruction_list):
 
         # User is not permitted based on role + status
         if (
-            (
-                destruction_list.status == ListStatus.ready_to_review
-                and not user.has_perm("accounts.can_review_destruction")
-            )
-            or (
-                destruction_list.status == ListStatus.ready_for_archivist
-                and not user.has_perm("accounts.can_review_final_list")
-            )
-            or destruction_list.status
-            not in [ListStatus.ready_to_review, ListStatus.ready_for_archivist]
+            destruction_list.status == ListStatus.ready_to_review
+            and user.has_perm("accounts.can_review_destruction")
+        ) or (
+            destruction_list.status == ListStatus.ready_for_archivist
+            and user.has_perm("accounts.can_review_final_list")
         ):
-            return False
+            return True
 
-        return True
+        return False
 
 
 class CanCoReviewPermission(permissions.BasePermission):
@@ -59,13 +54,13 @@ def has_object_permission(self, request, view, destruction_list):
         if user.pk not in user_assignees:
             return False
 
-        # User is not permitted based on role + status
-        if (
-            destruction_list.status == ListStatus.ready_to_review
-            and not user.has_perm("accounts.can_co_review_destruction")
-        ) or destruction_list.status != ListStatus.ready_to_review:
-            return False
-        return True
+        # User is permitted based on role + status
+        if destruction_list.status == ListStatus.ready_to_review and user.has_perm(
+            "accounts.can_co_review_destruction"
+        ):
+            return True
+
+        return False
 
 
 class CanUpdateDestructionList(permissions.BasePermission):

frontend/src/lib/auth/permissions.test.ts

@@ -8,10 +8,7 @@ import {
   roleFactory,
   userFactory,
 } from "../../fixtures/user";
-import {
-  STATUSES_ELIGIBLE_FOR_EDIT,
-  STATUSES_ELIGIBLE_FOR_REVIEW,
-} from "../../pages/constants";
+import { STATUSES_ELIGIBLE_FOR_EDIT } from "../../pages/constants";
 import { User } from "../api/auth";
 import {
   DESTRUCTION_LIST_STATUSES,
@@ -151,7 +148,7 @@ describe("canReviewDestructionList()", () => {
     });
     const list2 = destructionListFactory({
       assignee: other,
-      status: "ready_to_review",
+      status: "ready_for_archivist",
     });
 
     expect(canReviewDestructionList(me, list1)).toBeTruthy();
@@ -264,7 +261,7 @@ DESTRUCTION_LIST_STATUSES.forEach((status) => {
     });
 
     test("should allow a user to review if they have the role and status is eligible", () => {
-      const isEligible = STATUSES_ELIGIBLE_FOR_REVIEW.includes(status);
+      const isEligible = status === "ready_to_review";
       expect(canReviewDestructionList(user, destructionList)).toBe(isEligible);
     });
 

frontend/src/lib/auth/permissions.ts

@@ -1,7 +1,4 @@
-import {
-  STATUSES_ELIGIBLE_FOR_EDIT,
-  STATUSES_ELIGIBLE_FOR_REVIEW,
-} from "../../pages/constants";
+import { STATUSES_ELIGIBLE_FOR_EDIT } from "../../pages/constants";
 import { User } from "../api/auth";
 import { DestructionList } from "../api/destructionLists";
 
@@ -61,15 +58,24 @@ export const canReviewDestructionList: DestructionListPermissionCheck = (
   user,
   destructionList,
 ) => {
-  if (!(user.role.canReviewDestruction || user.role.canReviewFinalList)) {
+  if (user.pk !== destructionList.assignee.pk) {
     return false;
   }
 
-  if (!STATUSES_ELIGIBLE_FOR_REVIEW.includes(destructionList.status)) {
-    return false;
+  if (
+    user.role.canReviewDestruction &&
+    destructionList.status === "ready_to_review"
+  ) {
+    return true;
+  }
+  if (
+    user.role.canReviewFinalList &&
+    destructionList.status === "ready_for_archivist"
+  ) {
+    return true;
   }
 
-  return user.pk === destructionList.assignee.pk;
+  return false;
 };
 
 /**
@@ -85,14 +91,18 @@ export const canCoReviewDestructionList: DestructionListPermissionCheck = (
     return false;
   }
 
-  if (!STATUSES_ELIGIBLE_FOR_REVIEW.includes(destructionList.status)) {
+  if (destructionList.status !== "ready_to_review") {
     return false;
   }
 
-  return destructionList.assignees
+  const listAssignees = destructionList.assignees
     .filter((a) => a.role === "co_reviewer")
-    .map((a) => a.user.pk)
-    .includes(user.pk);
+    .map((a) => a.user.pk);
+  if (listAssignees.includes(user.pk)) {
+    return true;
+  }
+
+  return false;
 };
 
 /**

frontend/src/pages/constants.tsx

@@ -1,9 +1,6 @@
 import { BadgeProps, Outline } from "@maykin-ui/admin-ui";
 
-import {
-  DESTRUCTION_LIST_STATUSES,
-  DestructionListStatus,
-} from "../lib/api/destructionLists";
+import { DestructionListStatus } from "../lib/api/destructionLists";
 import { ProcessingStatus } from "../lib/api/processingStatus";
 import { Review } from "../lib/api/review";
 
@@ -23,8 +20,6 @@ export const REVIEW_DECISION_LEVEL_MAPPING: Record<
 };
 
 export const STATUSES_ELIGIBLE_FOR_EDIT = ["new", "changes_requested"];
-export const STATUSES_ELIGIBLE_FOR_REVIEW: (typeof DESTRUCTION_LIST_STATUSES)[number][] =
-  ["ready_to_review", "ready_for_archivist"];
 
 export const STATUS_MAPPING: { [key in DestructionListStatus]: string } = {
   new: "nieuw",