fix: do not allow verifying & moving reports at the same time (#1124)

* fix(backend): do not allow verifying & moving reports at the same time
* fix(frontend/analysis): validate verified field

Author: c0rydoras

backend/timed/tracking/tests/test_report.py

@@ -1,6 +1,9 @@
 """Tests for the reports endpoint."""
 
+from __future__ import annotations
+
 from datetime import date, timedelta
+from typing import TYPE_CHECKING
 
 import pyexcel
 import pytest
@@ -16,6 +19,10 @@
     TaskFactory,
 )
 
+if TYPE_CHECKING:
+    from timed.projects.models import Project, Task
+    from timed.tracking.models import Report
+
 
 def test_report_list(
     internal_employee_client,
@@ -1245,46 +1252,49 @@ def test_report_export_max_count(
     assert response.status_code == expected_status
 
 
-def test_report_update_bulk_verify_reviewer_multiple_notify(
+def test_report_update_bulk_reviewer_multiple_notify(
     internal_employee_client,
-    task,
+    task: Task,
     task_factory,
-    project,
+    project: Project,
     report_factory,
     user_factory,
+    project_assignee_factory,
     mailoutbox,
 ):
     reviewer = internal_employee_client.user
-    ProjectAssigneeFactory.create(user=reviewer, project=project, is_reviewer=True)
+    project_assignee_factory(user=reviewer, project=project, is_reviewer=True)
 
     user1, user2, user3 = user_factory.create_batch(3)
     report1_1 = report_factory(user=user1, task=task)
     report1_2 = report_factory(user=user1, task=task)
     report2 = report_factory(user=user2, task=task)
     report3 = report_factory(user=user3, task=task)
 
-    other_task = task_factory()
+    other_task = task_factory(project=project)
 
     url = reverse("report-bulk")
 
     data = {
         "data": {
             "type": "report-bulks",
             "id": None,
-            "attributes": {"verified": True, "comment": "some comment"},
+            "attributes": {"comment": "some comment"},
             "relationships": {"task": {"data": {"type": "tasks", "id": other_task.id}}},
         }
     }
 
-    query_params = f"?editable=1&reviewer={reviewer.id}&id=" + ",".join(
-        str(r.id) for r in [report1_1, report1_2, report2, report3]
-    )
-    response = internal_employee_client.post(url + query_params, data)
+    reports = [report1_1, report1_2, report2, report3]
+    params = {
+        "editable": 1,
+        "reviewer": reviewer.id,
+        "id": ",".join(str(r.id) for r in reports),
+    }
+    response = internal_employee_client.post(url, data, query_params=params)
     assert response.status_code == status.HTTP_204_NO_CONTENT
 
-    for report in [report1_1, report1_2, report2, report3]:
+    for report in reports:
         report.refresh_from_db()
-        assert report.verified_by == reviewer
         assert report.comment == "some comment"
         assert report.task == other_task
 
@@ -2111,3 +2121,47 @@ def test_report_list_filter_comment(
     assert json["data"][0]["id"] == str(report_1.id)
     assert json["data"][1]["id"] == str(report_2.id)
     assert json["data"][2]["id"] == str(report_3.id)
+
+
+def test_report_bulk_edit_move_and_verify(
+    internal_employee_client, user, report_factory, project_assignee_factory, task: Task
+):
+    reviewer = internal_employee_client.user
+
+    assert reviewer != user
+
+    # create a report owned by another user
+    report: Report = report_factory(user=user)
+
+    # assign the test employee as reviewer to the project of the report
+    project_assignee_factory(
+        user=reviewer, project=report.task.project, is_reviewer=True
+    )
+
+    url = reverse("report-bulk")
+
+    # raw request body for verifying and moving a report at the same time
+    data = {
+        "data": {
+            "type": "report-bulks",
+            "id": None,
+            "attributes": {"verified": True},
+            "relationships": {
+                "task": {"data": {"type": "tasks", "id": task.pk}},
+            },
+        }
+    }
+
+    # query params as required by the action
+    params = {"editable": 1, "reviewer": reviewer.id, "id": report.id}
+
+    response = internal_employee_client.post(
+        url,
+        data=data,
+        query_params=params,
+    )
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+    assert (
+        response.json()["errors"][0]["detail"]
+        == "Reports can't be moved and verified at the same time."
+    )

backend/timed/tracking/views.py

@@ -256,6 +256,11 @@ def bulk(self, request):
                     _("Reports can't both be set as `review` and `verified`.")
                 )
 
+            if fields.get("task"):
+                raise exceptions.ParseError(
+                    _("Reports can't be moved and verified at the same time.")
+                )
+
         if serializer.validated_data.get("billed", None) is not None and not (
             user.is_superuser or user.is_accountant
         ):

frontend/app/analysis/edit/template.hbs

@@ -209,7 +209,12 @@
                       </Checkbox>
                     </f.input>
 
-                    <f.input @name="verified" @labelComponent="void" as |fi|>
+                    <f.input
+                      @name="verified"
+                      @labelComponent="void"
+                      @dirty={{true}}
+                      as |fi|
+                    >
                       <Checkbox
                         data-test-verified
                         @checked={{fi.value}}

frontend/app/validations/intersection.js

@@ -1,5 +1,57 @@
-import validateIntersectionTask from "timed/validators/intersection-task";
+/**
+ * Given `changes` and `content` from a changeset validator, return wether a given `relation` has been changed.
+ * @param {string} relation
+ * @returns {(changes: Record<string, unknown>, content: Record<string, unknown>) => boolean}
+ */
+const relationChanged = (relation) => (changes, content) =>
+  Object.keys(changes).includes(relation) &&
+  (changes[relation]?.get("id") || null) !==
+    (content[relation]?.get("id") || null);
+
+const customerChanged = relationChanged("customer");
+const projectChanged = relationChanged("project");
+
+const validateVerified = (key, newValue, oldValue, changes, content) => {
+  if (!newValue) {
+    // when verified is false, it is valid
+    return true;
+  }
+
+  // the task can be null when verifying multiple reports, therefore we
+  // also validate against a change in customer/project
+  if (
+    (changes.task?.id && content.task?.id !== changes.task?.id) ||
+    customerChanged(changes, content) ||
+    projectChanged(changes, content)
+  ) {
+    return "Cannot verify and move report(s) at the same time.";
+  }
+
+  return true;
+};
+
+const validateIntersectionTask = (
+  key,
+  newValue,
+  oldValue,
+  changes,
+  content,
+) => {
+  const hasTask = !!(newValue && newValue.id);
+
+  // different customer -> different project -> different task
+  // a report belongs to a task, therefore when customer/project are changed
+  // task is no longer optional
+  return (
+    hasTask ||
+    (!hasTask &&
+      !customerChanged(changes, content) &&
+      !projectChanged(changes, content)) ||
+    "Task must not be empty"
+  );
+};
 
 export default {
-  task: validateIntersectionTask(),
+  verified: validateVerified,
+  task: validateIntersectionTask,
 };

frontend/app/validators/intersection-task.js

@@ -1,5 +1,6 @@
 import { click, fillIn, currentURL, visit } from "@ember/test-helpers";
 import { setupMirage } from "ember-cli-mirage/test-support";
+import { selectChoose } from "ember-power-select/test-support";
 import { setupApplicationTest } from "ember-qunit";
 import { authenticateSession } from "ember-simple-auth/test-support";
 import { module, test } from "qunit";
@@ -149,4 +150,58 @@ module("Acceptance | analysis edit", function (hooks) {
         "Please select yourself as 'reviewer' to verify reports. Please review selected reports before verifying.",
       );
   });
+  test("cannot verify report and move it at the same time", async function (assert) {
+    // we set the name of the task to something that faker will not produce
+    // otherwise this is flaky as tasks with the same name could exist (and we select them in the dropdown by name)
+    const task = this.server.create("task", { name: "the task" });
+    const otherTasks = this.server.createList("task", 10);
+
+    // giving `project`/`projectId` as an argument to `createList` does not work
+    // this is a workaround
+    otherTasks.forEach((t) => {
+      t.update({ project: task.project });
+    });
+
+    this.reportIntersection.update({
+      review: false,
+      customer: task.project.customer,
+      project: task.project,
+      taskId: task.id,
+    });
+    await visit(`/analysis/edit?id=${task.id}`);
+
+    // verified checkbox is not disabled
+    assert.dom("[data-test-verified] input").isNotDisabled();
+    assert.dom("[data-test-verified] label").hasAttribute("title", "");
+
+    // select another task
+    await selectChoose("[data-test-task]", otherTasks.at(-1).name);
+
+    // set verified to `true`
+    await click("[data-test-verified] input");
+
+    // the error message is visible
+    assert
+      .dom("[data-test-verified] + .invalid-feedback")
+      .hasText("Cannot verify and move report(s) at the same time.");
+
+    // set verified back to`false`
+    await click("[data-test-verified] input");
+    // the error message no longer exists (as there is no error)
+    assert.dom("[data-test-verified] + .invalid-feedback").doesNotExist();
+
+    // set verified back to `true`
+    await click("[data-test-verified] input");
+
+    // the error message is visible again
+    assert
+      .dom("[data-test-verified] + .invalid-feedback")
+      .hasText("Cannot verify and move report(s) at the same time.");
+
+    // select the original task
+    await selectChoose("[data-test-task]", task.name);
+
+    // the error message is gone
+    assert.dom("[data-test-verified] + .invalid-feedback").doesNotExist();
+  });
 });