From d02dfbe36827d1ba03df2b19c083681fbcecc2ec Mon Sep 17 00:00:00 2001 From: Sven van de Scheur Date: Thu, 19 Dec 2024 15:53:30 +0100 Subject: [PATCH 1/4] :sparkles: #561 - feat: filter select options on co reviewer assignment dialog based on active selection --- .../destruction/api/serializers.py | 19 ++- .../DestructionListReviewer.tsx | 135 ++++++++++++++---- 2 files changed, 120 insertions(+), 34 deletions(-) diff --git a/backend/src/openarchiefbeheer/destruction/api/serializers.py b/backend/src/openarchiefbeheer/destruction/api/serializers.py index fc825618..64f30259 100644 --- a/backend/src/openarchiefbeheer/destruction/api/serializers.py +++ b/backend/src/openarchiefbeheer/destruction/api/serializers.py @@ -114,12 +114,19 @@ def validate(self, attrs): .assignees.filter(role=ListRole.co_reviewer) .count() ) - if ( - current_number_co_reviewers - + len(attrs.get("add", [])) - - len(attrs.get("remove", [])) - > MAX_NUMBER_CO_REVIEWERS - ): + + # (New) number of co reviewers depends on whether a partial update has been mode or a full update is provided. + number_of_co_reviewers = ( + ( + current_number_co_reviewers + + len(attrs.get("add", [])) + - len(attrs.get("remove", [])) + ) + if self.partial + else len(attrs.get("add", [])) + ) + + if number_of_co_reviewers > MAX_NUMBER_CO_REVIEWERS: raise ValidationError( _("The maximum number of allowed co-reviewers is %(max_co_reviewers)s.") % {"max_co_reviewers": MAX_NUMBER_CO_REVIEWERS} diff --git a/frontend/src/components/DestructionListReviewer/DestructionListReviewer.tsx b/frontend/src/components/DestructionListReviewer/DestructionListReviewer.tsx index a932be80..2cd4fe8c 100644 --- a/frontend/src/components/DestructionListReviewer/DestructionListReviewer.tsx +++ b/frontend/src/components/DestructionListReviewer/DestructionListReviewer.tsx @@ -3,11 +3,13 @@ import { Button, FormField, P, + SerializedFormData, Solid, useAlert, useFormDialog, + validateForm, } from "@maykin-ui/admin-ui"; -import { useMemo } from "react"; +import { useEffect, useMemo, useState } from "react"; import { useNavigation, useRevalidator } from "react-router-dom"; import { @@ -57,11 +59,47 @@ export function DestructionListReviewer({ const assignedCoReviewers = useDestructionListCoReviewers(destructionList); const user = useWhoAmI(); + const [ + assignCoReviewersFormValuesState, + setAssignCoReviewersFormValuesState, + ] = useState({}); + + /** + * Pre-populates `assignCoReviewersFormValuesState` with the current + * co-reviewers. + */ + useEffect(() => { + const coReviewerPks = assignedCoReviewers.map((r) => r.user.pk.toString()); + + setAssignCoReviewersFormValuesState({ + ...assignCoReviewersFormValuesState, + coReviewer: coReviewerPks, + }); + }, [assignedCoReviewers.map((r) => r.user.pk).join()]); + + const [assignCoReviewerModalOpenState, setAssignCoReviewerModalOpenState] = + useState(false); + + /** + * Updates `assignCoReviewersFormValuesState` with `values`. + * This allows `field` to use filtered options based on it's value. + * @param values + */ + const handleValidate = (values: SerializedFormData) => { + // Ignore first run. + if (!Object.keys(values).length) { + return; + } + setAssignCoReviewersFormValuesState(values); + return validateForm(values, fields); + }; + /** * Gets called when the change is confirmed. */ const handleSubmit = (data: DestructionListReviewerFormType) => { const { coReviewer, reviewer, comment } = data; + setAssignCoReviewerModalOpenState(false); const promises: Promise[] = []; @@ -122,6 +160,10 @@ export function DestructionListReviewer({ (assignee) => assignee.role === "main_reviewer", ); + /** + * The fields to show in the form dialog, can be either for (re)assigning a + * reviewer or for (re)assigning co-reviewers. + */ const fields = useMemo(() => { if (!user) return []; @@ -145,29 +187,52 @@ export function DestructionListReviewer({ required: true, }; + const activeCoReviewers = + (assignCoReviewersFormValuesState.coReviewer as string[]) || []; + if (canReviewDestructionList(user, destructionList)) { - const coReviewerFields = new Array(5) - .fill({ - label: "Medebeoordelaar", - name: "coReviewer", - type: "string", - options: coReviewers.map((user) => ({ - label: formatUser(user), - value: user.pk, - })), - required: false, - }) - .map((f, i) => ({ + const coReviewerFields = new Array(5).fill(null).map((f, i) => { + return { ...f, label: `Medebeoordelaar ${1 + i}`, - value: assignedCoReviewers[i]?.user.pk, - })); + name: "coReviewer", + required: false, + type: "string", + value: activeCoReviewers?.[i], + options: coReviewers + // Don't show the co-reviewer as option if: + // - The co-reviewer is already selected AND + // - The co-reviewer is not selected as value for the current + // field. + .filter((c) => { + const selectedIndex = activeCoReviewers.indexOf(c.pk.toString()); + if (selectedIndex < 0 || selectedIndex === i) { + return true; + } + return false; + }) + .map((user) => ({ + label: formatUser(user), + value: user.pk, + })), + }; + }); return [...coReviewerFields, comment]; } return [reviewer, comment]; - }, [user, destructionList, reviewers, assignedCoReviewers]); + }, [ + user, + destructionList, + reviewers, + assignedCoReviewers, + assignCoReviewersFormValuesState, + ]); + /** + * Contains the co-reviewers that are assigned to the destruction list as + * items for the AttributeTable. + */ const coReviewerItems = useMemo( () => assignedCoReviewers.reduce((acc, coReviewer, i) => { @@ -194,6 +259,29 @@ export function DestructionListReviewer({ [assignedCoReviewers, coReviews], ); + /** + * Opens a dialog to assign a co-reviewer and updates it when `fields` change. + */ + useEffect(() => { + formDialog( + "Beoordelaar toewijzen", + null, + fields, + "Toewijzen", + "Annuleren", + handleSubmit, + undefined, + { + allowClose: true, + open: assignCoReviewerModalOpenState, + onClose: () => setAssignCoReviewerModalOpenState(false), + }, + { + validate: handleValidate, + }, + ); + }, [assignCoReviewerModalOpenState, JSON.stringify(fields)]); + return ( <> {reviewer && ( @@ -217,18 +305,9 @@ export function DestructionListReviewer({ } size="xs" variant="secondary" - onClick={(e) => { - formDialog( - "Beoordelaar toewijzen", - null, - fields, - "Toewijzen", - "Annuleren", - handleSubmit, - undefined, - { allowClose: true }, - ); - }} + onClick={() => + setAssignCoReviewerModalOpenState(true) + } > From 8dc3a0dde29c725d69e547b24593f58aebaf9124 Mon Sep 17 00:00:00 2001 From: Sven van de Scheur Date: Thu, 19 Dec 2024 16:44:24 +0100 Subject: [PATCH 2/4] :white_check_mark: #561 - test: attempt to make the tests less flaky --- .../destructionlist/review/DestructionListReview.stories.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/pages/destructionlist/review/DestructionListReview.stories.tsx b/frontend/src/pages/destructionlist/review/DestructionListReview.stories.tsx index 1eda330e..7c8cf7c3 100644 --- a/frontend/src/pages/destructionlist/review/DestructionListReview.stories.tsx +++ b/frontend/src/pages/destructionlist/review/DestructionListReview.stories.tsx @@ -208,7 +208,7 @@ export const ReviewerCanExcludeZaak: Story = { name: "Uitzonderen", }); const exclude = excludes[0]; - await userEvent.click(exclude); + await userEvent.click(exclude, { delay: 60 }); const reason = await canvas.findByLabelText("Reden"); const submitExclude = await canvas.findByRole("button", { From 5893deeccd6994899a93708e9b5e8b6bba844fb1 Mon Sep 17 00:00:00 2001 From: Sven van de Scheur Date: Fri, 20 Dec 2024 15:32:27 +0100 Subject: [PATCH 3/4] :pencil2: #561 - refactor: fix typo in CoReviewerAssignmentSerializer --- .../src/openarchiefbeheer/destruction/api/serializers.py | 2 +- backend/src/openarchiefbeheer/destruction/api/viewsets.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/backend/src/openarchiefbeheer/destruction/api/serializers.py b/backend/src/openarchiefbeheer/destruction/api/serializers.py index 64f30259..26c2f0c4 100644 --- a/backend/src/openarchiefbeheer/destruction/api/serializers.py +++ b/backend/src/openarchiefbeheer/destruction/api/serializers.py @@ -97,7 +97,7 @@ def validate(self, attrs: dict) -> dict: return attrs -class CoReviewerAssignementSerializer(serializers.Serializer): +class CoReviewerAssignmentSerializer(serializers.Serializer): comment = serializers.CharField(required=True, allow_blank=False) add = CoReviewerSerializer(many=True) remove = CoReviewerSerializer(many=True, required=False) diff --git a/backend/src/openarchiefbeheer/destruction/api/viewsets.py b/backend/src/openarchiefbeheer/destruction/api/viewsets.py index ba66d2fe..a5363f33 100644 --- a/backend/src/openarchiefbeheer/destruction/api/viewsets.py +++ b/backend/src/openarchiefbeheer/destruction/api/viewsets.py @@ -58,7 +58,7 @@ ) from .serializers import ( AbortDestructionSerializer, - CoReviewerAssignementSerializer, + CoReviewerAssignmentSerializer, DestructionListAssigneeReadSerializer, DestructionListCoReviewSerializer, DestructionListItemReadSerializer, @@ -555,7 +555,7 @@ def get_permissions(self): description=_( "Full update of the co-reviewers assigned to a destruction list." ), - request=CoReviewerAssignementSerializer, + request=CoReviewerAssignmentSerializer, responses={200: DestructionListAssigneeReadSerializer}, ), partial_update=extend_schema( @@ -564,7 +564,7 @@ def get_permissions(self): description=_( "Partial update of the co-reviewers assigned to a destruction list." ), - request=CoReviewerAssignementSerializer, + request=CoReviewerAssignmentSerializer, responses={200: DestructionListAssigneeReadSerializer}, ), ) @@ -605,7 +605,7 @@ def get_serializer_class(self): case "list": return DestructionListAssigneeReadSerializer case "update" | "partial_update" | "create": - return CoReviewerAssignementSerializer + return CoReviewerAssignmentSerializer case default: # noqa return DestructionListAssigneeReadSerializer From dc3233aea5dbec9fab9f4d7b43fdc78393022393 Mon Sep 17 00:00:00 2001 From: Sven van de Scheur Date: Fri, 20 Dec 2024 16:00:40 +0100 Subject: [PATCH 4/4] :white_check_mark: #561 - test: add tests for CoReviewerAssignmentSerializer --- .../destruction/tests/test_serializers.py | 139 ++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/backend/src/openarchiefbeheer/destruction/tests/test_serializers.py b/backend/src/openarchiefbeheer/destruction/tests/test_serializers.py index 3166334b..375157fe 100644 --- a/backend/src/openarchiefbeheer/destruction/tests/test_serializers.py +++ b/backend/src/openarchiefbeheer/destruction/tests/test_serializers.py @@ -13,7 +13,9 @@ from openarchiefbeheer.emails.models import EmailConfig from ...zaken.tests.factories import ZaakFactory +from ..api.constants import MAX_NUMBER_CO_REVIEWERS from ..api.serializers import ( + CoReviewerAssignmentSerializer, DestructionListCoReviewSerializer, DestructionListReviewSerializer, DestructionListWriteSerializer, @@ -1282,3 +1284,140 @@ def test_create_review(self): }, message, ) + + +class CoReviewerAssignmentSerializerTests(TestCase): + def test_validate_max_co_reviewers_invalid_full(self): + destruction_list = DestructionListFactory.create( + status=ListStatus.ready_to_review + ) + + request = factory.put("/foo") + request.user = UserFactory.create() + + serializer = CoReviewerAssignmentSerializer( + instance=destruction_list, + data={ + "add": [ + {"user": user.pk} + for user in UserFactory.create_batch( + 10, post__can_co_review_destruction=True + ) + ], + "comment": "gh-561", + }, + context={ + "request": request, + "destruction_list": destruction_list, + }, + partial=False, + ) + + self.assertFalse(serializer.is_valid()) + self.assertEqual( + serializer.errors["non_field_errors"][0], + _("The maximum number of allowed co-reviewers is %(max_co_reviewers)s.") + % {"max_co_reviewers": MAX_NUMBER_CO_REVIEWERS}, + ) + + def test_validate_max_co_reviewers_valid_full(self): + destruction_list = DestructionListFactory.create( + status=ListStatus.ready_to_review + ) + + request = factory.put("/foo") + request.user = UserFactory.create() + + serializer = CoReviewerAssignmentSerializer( + instance=destruction_list, + data={ + "add": [ + {"user": user.pk} + for user in UserFactory.create_batch( + 5, post__can_co_review_destruction=True + ) + ], + "comment": "gh-561", + }, + context={ + "request": request, + "destruction_list": destruction_list, + }, + partial=False, + ) + + self.assertTrue(serializer.is_valid()) + + def test_validate_max_co_reviewers_invalid_partial(self): + destruction_list = DestructionListFactory.create( + status=ListStatus.ready_to_review + ) + DestructionListAssigneeFactory.create_batch( + 5, destruction_list=destruction_list, role=ListRole.co_reviewer + ) + + request = factory.patch("/foo") + request.user = UserFactory.create() + + serializer = CoReviewerAssignmentSerializer( + instance=destruction_list, + data={ + "add": [ + { + "user": UserFactory.create( + post__can_co_review_destruction=True + ).pk + } + ], + "comment": "gh-561", + }, + context={ + "request": request, + "destruction_list": destruction_list, + }, + partial=True, + ) + + self.assertFalse(serializer.is_valid()) + self.assertEqual( + serializer.errors["non_field_errors"][0], + _("The maximum number of allowed co-reviewers is %(max_co_reviewers)s.") + % {"max_co_reviewers": MAX_NUMBER_CO_REVIEWERS}, + ) + + def test_validate_max_co_reviewers_valid_partial(self): + destruction_list = DestructionListFactory.create( + status=ListStatus.ready_to_review + ) + assignees = DestructionListAssigneeFactory.create_batch( + 5, destruction_list=destruction_list, role=ListRole.co_reviewer + ) + + request = factory.patch("/foo") + request.user = UserFactory.create() + + serializer = CoReviewerAssignmentSerializer( + instance=destruction_list, + data={ + "add": [ + { + "user": UserFactory.create( + post__can_co_review_destruction=True + ).pk + } + ], + "remove": [ + { + "user": assignees[0].user.pk, + } + ], + "comment": "gh-561", + }, + context={ + "request": request, + "destruction_list": destruction_list, + }, + partial=True, + ) + + self.assertTrue(serializer.is_valid())