Merge pull request #831 from openzim/missing_cancel_requester

Missing cancel requester

dispatcher/backend/src/common/utils.py

@@ -206,15 +206,15 @@ def task_failed_event_handler(session: so.Session, task_id: UUID, payload: dict)
 def task_cancel_requested_event_handler(
     session: so.Session, task_id: UUID, payload: dict
 ):
-    requested_by = payload.get("canceled_by")
-    logger.info(f"Task Cancellation Requested: {task_id}, by: {requested_by}")
+    canceled_by = payload.get("canceled_by")
+    logger.info(f"Task Cancellation Requested: {task_id}, by: {canceled_by}")
 
     save_event(
         session,
         task_id,
         TaskStatus.cancel_requested,
         get_timestamp_from_event(payload),
-        canceled_by=requested_by,
+        canceled_by=canceled_by,
     )
 
 
@@ -223,18 +223,18 @@ def task_canceled_event_handler(session: so.Session, task_id: UUID, payload: dic
 
     # if canceled event carries a `canceled_by` and we have none on the task
     # then store it, otherwise keep what's in the task (manual request)
-    canceled_by = None
     task = dbm.Task.get(session, task_id, TaskNotFound)
-    if payload.get("canceled_by") and task and not task.canceled_by:
-        canceled_by = payload.get("canceled_by")
+    kwargs = {}
+    if not task.canceled_by and payload.get("canceled_by"):
+        kwargs["canceled_by"] = payload.get("canceled_by")
 
     save_event(
         session,
         task_id,
         TaskStatus.canceled,
         get_timestamp_from_event(payload),
         task_log=payload.get("log"),
-        canceled_by=canceled_by,
+        **kwargs,
     )
 
 

dispatcher/backend/src/tests/integration/conftest.py

@@ -76,9 +76,17 @@ def _make_access_token(username: str, role: str = "editor"):
     yield _make_access_token
 
 
+USERNAME = "username"
+
+
 @pytest.fixture(scope="session")
 def access_token(make_access_token):
-    yield make_access_token("username", "admin")
+    yield make_access_token(USERNAME, "admin")
+
+
+@pytest.fixture(scope="session")
+def username():
+    yield USERNAME
 
 
 class GarbageCollector:

dispatcher/backend/src/tests/integration/routes/business_logic/test_task_cancel_bl.py

@@ -0,0 +1,130 @@
+import json
+
+import pytest
+
+import db.models as dbm
+from db import Session
+
+SCHEDULE_NAME = "a_schedule_for_tasks"
+
+
+class TestTaskBusiness:
+    @pytest.fixture(scope="module")
+    def headers(self, access_token):
+        return {"Authorization": access_token, "Content-Type": "application/json"}
+
+    @pytest.fixture()
+    def task(self, client, headers, worker, make_requested_task, garbage_collector):
+        requested_task = make_requested_task(SCHEDULE_NAME)
+        url = "/tasks/{}".format(str(requested_task["_id"]))
+        response = client.post(
+            url, headers=headers, query_string={"worker_name": "worker_name"}
+        )
+        assert response.status_code == 201
+        assert "_id" in response.json
+        task_id = response.json["_id"]
+        garbage_collector.add_task_id(task_id)
+        yield task_id
+
+    def test_task_cancel_requested(self, client, headers, username, task):
+        url = f"/tasks/{task}"
+
+        def get_task():
+            response = client.get(
+                url,
+                headers=headers,
+            )
+            assert response.status_code == 200
+            return response
+
+        def patch_to_status(status, payload={}):
+            response = client.patch(
+                url,
+                headers=headers,
+                data=json.dumps({"event": status, "payload": payload}),
+            )
+            assert response.status_code == 204
+
+        response = get_task()
+        assert "canceled_by" in response.json
+        assert response.json["canceled_by"] is None
+
+        patch_to_status("started")
+        response = get_task()
+        assert "canceled_by" in response.json
+        assert response.json["canceled_by"] is None
+
+        patch_to_status("scraper_started")
+        response = get_task()
+        assert "canceled_by" in response.json
+        assert response.json["canceled_by"] is None
+
+        response = client.post(
+            f"{url}/cancel",
+            headers=headers,
+        )
+        assert response.status_code == 204
+        response = get_task()
+        assert "status" in response.json
+        assert response.json["status"] == "cancel_requested"
+        assert "canceled_by" in response.json
+        # canceled_by is automatically populated based on value in access token
+        assert response.json["canceled_by"] == username
+
+        patch_to_status("canceled", payload={"canceled_by": "bob"})
+        response = get_task()
+        assert "canceled_by" in response.json
+        # canceled_by is not computed based on payload because a value is already there
+        assert response.json["canceled_by"] == username
+
+    def test_task_cancel_in_db(self, client, headers, username, task):
+        url = f"/tasks/{task}"
+
+        def get_task():
+            response = client.get(
+                url,
+                headers=headers,
+            )
+            assert response.status_code == 200
+            return response
+
+        def patch_to_status(status, payload={}):
+            response = client.patch(
+                url,
+                headers=headers,
+                data=json.dumps({"event": status, "payload": payload}),
+            )
+            assert response.status_code == 204
+
+        response = get_task()
+        assert "canceled_by" in response.json
+        assert response.json["canceled_by"] is None
+
+        patch_to_status("started")
+        response = get_task()
+        assert "canceled_by" in response.json
+        assert response.json["canceled_by"] is None
+
+        patch_to_status("scraper_started")
+        response = get_task()
+        assert "canceled_by" in response.json
+        assert response.json["canceled_by"] is None
+
+        with Session.begin() as session:
+            task = dbm.Task.get(session, task)
+            task.status = "cancel_requested"
+
+        response = get_task()
+        assert "canceled_by" in response.json
+        assert response.json["canceled_by"] is None
+        assert "status" in response.json
+        assert response.json["status"] == "cancel_requested"
+
+        canceled_by = "bob"
+        patch_to_status("canceled", payload={"canceled_by": canceled_by})
+        response = get_task()
+        assert "canceled_by" in response.json
+        # canceled_by is populated based on payload because we have no access token
+        # this mimics the situation where it is the periodic scheduler which requests
+        # this cancelation directly in the code, not through the API
+        assert response.json["canceled_by"] == canceled_by

dispatcher/frontend-ui/src/views/TaskView.vue

@@ -56,6 +56,7 @@
               <tr v-for="event in task.events" v-bind:key="event.code">
                 <td><code>{{ event.code }}</code></td><td>{{ event.timestamp | format_dt }}</td>
                 <td v-if="event.code == 'requested'">{{ task.requested_by }}</td>
+                <td v-else-if="event.code == 'cancel_requested' && task.status =='cancel_requested'">{{ task.canceled_by }}</td>
                 <td v-else-if="event.code == 'canceled'">{{ task.canceled_by }}</td>
                 <td v-else />
               </tr>