🐛 #636 - fix: fix a bug that allowed the change reviewer form to be i…

…nteractive without a valid comment

backend/src/openarchiefbeheer/destruction/tests/e2e/features/test_feature_list_reassign.py

@@ -1,11 +1,10 @@
 # fmt: off
 from django.test import tag
 
+from openarchiefbeheer.destruction.constants import ListStatus
 from openarchiefbeheer.utils.tests.e2e import browser_page
 from openarchiefbeheer.utils.tests.gherkin import GherkinLikeTestCase
 
-from openarchiefbeheer.destruction.constants import ListStatus
-
 
 @tag("e2e")
 class FeatureListReassignTests(GherkinLikeTestCase):
@@ -32,3 +31,28 @@ async def test_scenario_record_manager_updates_reviewer(self):
 
             await self.then.page_should_contain_text(page, "Destruction list to update")
             await self.then.list_should_have_assignee(page, destruction_list, reviewer2)
+
+    @tag("gh-636")
+    async def test_scenario_reopening_modal_preserves_state(self):
+        async with browser_page() as page:
+            await self.given.record_manager_exists()
+            await self.given.list_exists(name="Destruction list to update", status=ListStatus.ready_to_review)
+            await self.given.reviewer_exists(username="reviewer2", first_name="Jane", last_name="Doe")
+
+            await self.when.record_manager_logs_in(page)
+            await self.then.path_should_be(page, "/destruction-lists")
+
+            await self.when.user_clicks_button(page, "Destruction list to update")
+            await self.when.user_clicks_button(page, "Beoordelaar bewerken")
+            await self.when.user_fills_form_field(page, "Beoordelaar", "Jane Doe (reviewer2)")
+            await self.then.button_should_be_disabled(page, "Toewijzen")
+
+            await self.when.user_clicks_button(page, "Annuleren")
+            await self.when.user_clicks_button(page, "Beoordelaar bewerken")
+            await self.then.button_should_be_disabled(page, "Toewijzen")
+
+            await self.when.user_fills_form_field(page, "Reden", "gh-636")
+            await self.when.user_clicks_button(page, "Annuleren")
+            await self.when.user_clicks_button(page, "Beoordelaar bewerken")
+            await self.then.form_field_should_have_value(page, "Reden", "gh-636")
+            await self.then.button_should_be_enabled(page, "Toewijzen")

backend/src/openarchiefbeheer/utils/tests/gherkin.py

@@ -734,6 +734,36 @@ async def page_should_contain_element_with_title(
         async def path_should_be(self, page, path):
             await self.url_should_be(page, self.testcase.live_server_url + path)
 
+        async def button_should_be_enabled(self, page, name, index=0):
+            element = page.get_by_role("button", name=name)
+            await expect(element).to_be_enabled()
+
+        async def button_should_be_disabled(self, page, name, index=0):
+            element = page.get_by_role("button", name=name)
+            await expect(element).to_be_disabled()
+
+        async def form_field_should_have_value(
+            self, page, label, value, role=None, index=0
+        ):
+            selects = await page.query_selector_all(
+                f'.mykn-select[aria-label="{label}"]'
+            )
+            try:
+                select = selects[index]
+
+                if select:  # has content so select?
+                    raise NotImplementedError("Selects are not supported yet")
+            except IndexError:
+                pass
+
+            if role:
+                locator = page.get_by_label(label).and_(page.get_by_role("textbox"))
+            else:
+                locator = page.get_by_label(label)
+
+            elements = await locator.all()
+            await expect(elements[index]).to_have_value(value)
+
         async def url_should_be(self, page, url):
             await expect(page).to_have_url(url)
 

frontend/src/components/DestructionListReviewer/DestructionListReviewer.stories.tsx

@@ -7,7 +7,7 @@ import {
   ClearSessionStorageDecorator,
   ReactRouterDecorator,
 } from "../../../.storybook/decorators";
-import { fillForm } from "../../../.storybook/playFunctions";
+import { clickButton, fillForm } from "../../../.storybook/playFunctions";
 import { coReviewFactory } from "../../fixtures/coReview";
 import { destructionListFactory } from "../../fixtures/destructionList";
 import {
@@ -542,3 +542,123 @@ export const ReassignDestructionListErrorShowsErrorMessage: Story = {
     );
   },
 };
+
+export const ReopeningModalPreservesState: Story = {
+  args: { destructionList: DESTRUCTION_LIST_NEW },
+  parameters: {
+    moduleMock: {
+      mock: () => {
+        const reassignDestructionList = createMock(
+          libDestructionList,
+          "reassignDestructionList",
+        );
+        reassignDestructionList.mockImplementation(
+          async () => ({}) as Response,
+        );
+
+        const updateCoReviewers = createMock(
+          libDestructionList,
+          "updateCoReviewers",
+        );
+        updateCoReviewers.mockImplementation(async () => ({}) as Response);
+
+        const useWhoAmI = createMock(hooksUseWhoAmI, "useWhoAmI");
+        useWhoAmI.mockImplementation(() => RECORD_MANAGER);
+
+        return [reassignDestructionList, updateCoReviewers, useWhoAmI];
+      },
+    },
+  },
+  play: async (context) => {
+    await clickButton({
+      ...context,
+      parameters: {
+        clickButton: {
+          name: "Beoordelaar bewerken",
+        },
+      },
+    });
+
+    await fillForm({
+      ...context,
+      parameters: {
+        fillForm: {
+          formValues: {
+            Beoordelaar: "Proces ei Genaar (Proces ei Genaar)",
+          },
+          submitForm: false,
+        },
+      },
+    });
+
+    await expect(
+      await within(context.canvasElement).findByRole("button", {
+        name: "Toewijzen",
+      }),
+    ).toBeDisabled();
+
+    await clickButton({
+      ...context,
+      parameters: {
+        clickButton: {
+          name: "Annuleren",
+        },
+      },
+    });
+
+    await clickButton({
+      ...context,
+      parameters: {
+        clickButton: {
+          name: "Beoordelaar bewerken",
+        },
+      },
+    });
+
+    await expect(
+      await within(context.canvasElement).findByRole("button", {
+        name: "Toewijzen",
+      }),
+    ).toBeDisabled();
+
+    await fillForm({
+      ...context,
+      parameters: {
+        fillForm: {
+          formValues: {
+            Reden: "gh-636",
+          },
+          submitForm: false,
+        },
+      },
+    });
+
+    await clickButton({
+      ...context,
+      parameters: {
+        clickButton: {
+          name: "Annuleren",
+        },
+      },
+    });
+
+    await clickButton({
+      ...context,
+      parameters: {
+        clickButton: {
+          name: "Beoordelaar bewerken",
+        },
+      },
+    });
+
+    await expect(
+      await within(context.canvasElement).findByLabelText("Reden"),
+    ).toHaveValue("gh-636");
+
+    await expect(
+      await within(context.canvasElement).findByRole("button", {
+        name: "Toewijzen",
+      }),
+    ).toBeEnabled();
+  },
+};

frontend/src/components/DestructionListReviewer/DestructionListReviewer.tsx

@@ -66,7 +66,8 @@ export function DestructionListReviewer({
   const [assignReviewersFormState, setAssignReviewersFormState] = useState<{
     reviewer: string;
     coReviewer: string[];
-  }>({ reviewer: "", coReviewer: [] });
+    comment: string;
+  }>({ reviewer: "", coReviewer: [], comment: "" });
 
   /**
    * Pre-populates `assignCoReviewersFormValuesState` with the current
@@ -91,12 +92,11 @@ export function DestructionListReviewer({
    * @param values
    */
   const handleValidate = (values: TypedSerializedFormData) => {
-    // Ignore first run.
-    if (!Object.keys(values).length) {
-      return;
+    const _values = Object.assign({ ...assignReviewersFormState }, values);
+    if (Object.keys(_values).length) {
+      setAssignReviewersFormState(_values as typeof assignReviewersFormState);
     }
-    setAssignReviewersFormState(values as typeof assignReviewersFormState);
-    return validateForm(values, fields);
+    return validateForm(_values, fields);
   };
 
   /**
@@ -184,7 +184,7 @@ export function DestructionListReviewer({
       name: "comment",
       type: "text",
       required: true,
-      // value: assignReviewersFormState.comment,
+      value: assignReviewersFormState.comment,
     };
 
     const activeCoReviewerFields =
@@ -197,7 +197,7 @@ export function DestructionListReviewer({
         name: "coReviewer",
         required: false,
         type: "string",
-        value: assignReviewersFormState.coReviewer[i],
+        value: assignReviewersFormState.coReviewer?.[i],
         options: coReviewers
           // Don't show the co-reviewer as option if:
           // - The co-reviewer is already selected AND
@@ -273,7 +273,7 @@ export function DestructionListReviewer({
       "Toewijzen",
       "Annuleren",
       handleSubmit,
-      undefined,
+      () => setAssignCoReviewerModalOpenState(false),
       {
         allowClose: true,
         open: assignCoReviewerModalOpenState,