Add user visible errors when permission error and request dismission happen (#126)

* handle permission error and messages to dismissed requests

* fix

* commit

* fix

* debug

* debug

* debug

* debug

* debug

* debug

* fix

* qa

Author: francesconazzaro

alembic/versions/a4e8be715296_add_deleted_as_new_status.py

@@ -5,9 +5,8 @@
 Create Date: 2024-07-25 13:13:11.955119
 
 """
-import sqlalchemy as sa
 
-from sqlalchemy.dialects.postgresql import ENUM
+import sqlalchemy as sa
 
 from alembic import op
 

cads_broker/database.py

@@ -641,13 +641,25 @@ def set_successful_request(
     return request
 
 
-def set_dismissed_request(request_uid: str, session: sa.orm.Session) -> SystemRequest:
+def set_dismissed_request(
+    request_uid: str,
+    session: sa.orm.Session,
+    message: str = "Dismissed by the user.",
+    reason: str = "DismissedRequest",
+) -> SystemRequest:
     request = get_request(request_uid=request_uid, session=session)
     metadata = dict(request.request_metadata)
-    metadata.update({"previous_status": request.status})
+    metadata.update(
+        {
+            "dismission": {
+                "previous_status": request.status,
+                "message": message,
+                "reason": reason,
+            }
+        }
+    )
     request.request_metadata = metadata
     request.status = "dismissed"
-    request.response_error = {"reason": "Dismissed by the user"}
     session.commit()
     logger.info("dismissed job by the user.", **logger_kwargs(request=request))
     return request

cads_broker/dispatcher.py

@@ -327,6 +327,34 @@ def set_request_error_status(
             )
         return request
 
+    def manage_dismissed_request(self, request, session):
+        dismission_metadata = request.request_metadata.get("dismission", {})
+        db.add_event(
+            event_type="user_visible_error",
+            request_uid=request.request_uid,
+            message=dismission_metadata.get("message", ""),
+            session=session,
+        )
+        previous_status = dismission_metadata.get("previous_status", "accepted")
+        if previous_status == "running":
+            self.qos.notify_end_of_request(
+                request, session, scheduler=self.internal_scheduler
+            )
+            request.status = "deleted"
+        elif previous_status == "accepted":
+            self.queue.pop(request.request_uid, None)
+            self.qos.notify_dismission_of_request(
+                request, session, scheduler=self.internal_scheduler
+            )
+            if (
+                reason := dismission_metadata.get("reason", "DismissedRequest")
+            ) == "DismissedRequest":
+                request.status = "deleted"
+            elif reason == "PermissionError":
+                request.status = "failed"
+                request.finished_at = datetime.datetime.now()
+        return session
+
     @cachetools.cachedmethod(lambda self: self.ttl_cache)
     @perf_logger
     def sync_database(self, session: sa.orm.Session) -> None:
@@ -346,20 +374,7 @@ def sync_database(self, session: sa.orm.Session) -> None:
         for request in dismissed_requests:
             if future := self.futures.pop(request.request_uid, None):
                 future.cancel()
-            if (
-                previous_status := request.request_metadata.get(
-                    "previous_status", "accepted"
-                )
-            ) == "running":
-                self.qos.notify_end_of_request(
-                    request, session, scheduler=self.internal_scheduler
-                )
-            elif previous_status == "accepted":
-                self.queue.pop(request.request_uid, None)
-                self.qos.notify_dismission_of_request(
-                    request, session, scheduler=self.internal_scheduler
-                )
-            request.status = "deleted"
+            session = self.manage_dismissed_request(request, session)
         session.commit()
 
         statement = sa.select(db.SystemRequest).where(

cads_broker/entry_points.py

@@ -118,11 +118,13 @@ class RequestStatus(str, Enum):
 @app.command()
 def delete_requests(
     status: RequestStatus = RequestStatus.running,
+    user_uid: Optional[str] = None,
     connection_string: Optional[str] = None,
     minutes: float = 0,
     seconds: float = 0,
     hours: float = 0,
     days: float = 0,
+    message: Optional[str] = "The request has been dismissed by the administrator.",
     skip_confirmation: Annotated[bool, typer.Option("--yes", "-y")] = False,
 ) -> None:
     """Set the status of records in the system_requests table to 'dismissed'.
@@ -142,7 +144,18 @@ def delete_requests(
             sa.update(database.SystemRequest)
             .where(database.SystemRequest.status == status)
             .where(database.SystemRequest.created_at < timestamp)
-            .values(status="dismissed")
+        )
+        if user_uid:
+            statement = statement.where(database.SystemRequest.user_uid == user_uid)
+        statement = statement.values(
+            status="dismissed",
+            request_metadata={
+                "dismission": {
+                    "reason": "DismissedRequest",
+                    "message": message,
+                    "previous_status": status,
+                }
+            },
         )
         number_of_requests = session.execute(statement).rowcount
         if not skip_confirmation:

cads_broker/qos/QoS.py

@@ -140,14 +140,11 @@ def _properties(self, request, session):
             if rule.match(request):
                 properties.permissions.append(rule)
                 if not rule.evaluate(request):
-                    database.set_request_status(
+                    database.set_dismissed_request(
                         request_uid=request.request_uid,
-                        status="failed",
                         session=session,
-                        error_message=rule.info.evaluate(
-                            Context(request, self.environment)
-                        ),
-                        error_reason="Permission error.",
+                        message=rule.info.evaluate(Context(request, self.environment)),
+                        reason="PermissionError",
                     )
                     break
 
@@ -254,20 +251,24 @@ def user_limit(self, request):
 
         limits = self.per_user_limits.get(user, [])
         applied_limits = []
-        for limit in limits:
-            if limit.match(request):
-                applied_limits.append(limit)
+        for user_limit in limits:
+            if user_limit.match(request):
+                applied_limits.append(user_limit)
 
-        for limit in self.rules.user_limits:
-            if limit.match(request):
+        for user_limit in self.rules.user_limits:
+            if user_limit.match(request):
                 """
                 We clone the rule because we need one instance per different
                 user otherwise all users will share that limit
                 """
-                if limit.get_uid(request) not in [l.get_uid(request) for l in limits]:
-                    limit = limit.clone()
-                    applied_limits.append(limit)
-                    self.per_user_limits[user] = self.per_user_limits.get(user, []) + [limit]
+                if user_limit.get_uid(request) not in [
+                    limit.get_uid(request) for limit in limits
+                ]:
+                    user_limit = user_limit.clone()
+                    applied_limits.append(user_limit)
+                    self.per_user_limits[user] = self.per_user_limits.get(user, []) + [
+                        user_limit
+                    ]
         return applied_limits
 
     @locked