Merge pull request #590 from opensafely-core/exceptions

Move BusinessLayerLogic exceptions to a separate module

airlock/exceptions.py

@@ -0,0 +1,37 @@
+class ActionDenied(Exception): ...
+
+
+class APIException(Exception): ...
+
+
+class WorkspaceNotFound(APIException): ...
+
+
+class WorkspacePermissionDenied(APIException): ...
+
+
+class ReleaseRequestNotFound(APIException): ...
+
+
+class FileNotFound(APIException): ...
+
+
+class FileReviewNotFound(APIException): ...
+
+
+class InvalidStateTransition(APIException): ...
+
+
+class RequestPermissionDenied(APIException): ...
+
+
+class IncompleteContextOrControls(RequestPermissionDenied): ...
+
+
+class RequestReviewDenied(APIException): ...
+
+
+class ApprovalPermissionDenied(APIException): ...
+
+
+class ManifestFileError(APIException): ...

airlock/file_browser_api.py

@@ -2,7 +2,7 @@
 
 import os
 from dataclasses import dataclass, field
-from datetime import datetime
+from datetime import UTC, datetime
 from enum import Enum
 from pathlib import Path
 
@@ -176,7 +176,7 @@ def modified_at(self) -> datetime | None:
         if metadata is None:
             return None
 
-        return datetime.utcfromtimestamp(metadata.timestamp)
+        return datetime.fromtimestamp(metadata.timestamp, tz=UTC)
 
     def breadcrumbs(self):
         item = self

airlock/users.py

@@ -2,8 +2,7 @@
 import time
 from typing import Any
 
-
-class ActionDenied(Exception): ...
+from airlock.exceptions import ActionDenied
 
 
 @dataclasses.dataclass(frozen=True)

airlock/views/code.py

@@ -6,6 +6,7 @@
 from django.views.decorators.http import require_http_methods
 from django.views.decorators.vary import vary_on_headers
 
+from airlock import exceptions
 from airlock.business_logic import CodeRepo, Workspace, bll
 from airlock.file_browser_api import get_code_tree
 from airlock.types import UrlPath
@@ -93,7 +94,7 @@ def contents(request, workspace_name: str, commit: str, path: str):
     plaintext = request.GET.get("plaintext", False)
     try:
         renderer = repo.get_renderer(UrlPath(path), plaintext=plaintext)
-    except bll.FileNotFound:
+    except exceptions.FileNotFound:
         raise Http404()
 
     return serve_file(request, renderer)

airlock/views/helpers.py

@@ -5,6 +5,7 @@
 from django.http import FileResponse, Http404, HttpResponseNotModified
 from django.utils.safestring import mark_safe
 
+from airlock import exceptions
 from airlock.business_logic import bll
 from airlock.file_browser_api import PathItem
 from airlock.types import UrlPath
@@ -23,9 +24,9 @@ def get_workspace_or_raise(user, workspace_name):
     """Get the workspace, converting any errors to http codes."""
     try:
         workspace = bll.get_workspace(workspace_name, user)
-    except bll.WorkspaceNotFound:
+    except exceptions.WorkspaceNotFound:
         raise Http404()
-    except bll.WorkspacePermissionDenied:
+    except exceptions.WorkspacePermissionDenied:
         raise PermissionDenied()
 
     return workspace
@@ -35,9 +36,9 @@ def get_release_request_or_raise(user, request_id):
     """Get the release request, converting any errors to http codes."""
     try:
         release_request = bll.get_release_request(request_id, user)
-    except bll.ReleaseRequestNotFound:
+    except exceptions.ReleaseRequestNotFound:
         raise Http404()
-    except bll.WorkspacePermissionDenied:
+    except exceptions.WorkspacePermissionDenied:
         raise PermissionDenied()
 
     return release_request

airlock/views/request.py

@@ -14,6 +14,7 @@
 from django.views.decorators.vary import vary_on_headers
 from opentelemetry import trace
 
+from airlock import exceptions
 from airlock.business_logic import (
     ROOT_PATH,
     RequestFileType,
@@ -355,7 +356,7 @@ def request_contents(request, request_id: str, path: str):
 
     try:
         abspath = release_request.abspath(path)
-    except bll.FileNotFound:
+    except exceptions.FileNotFound:
         raise Http404()
 
     download = "download" in request.GET
@@ -384,10 +385,10 @@ def request_submit(request, request_id):
 
     try:
         bll.submit_request(release_request, request.user)
-    except bll.IncompleteContextOrControls as exc:
+    except exceptions.IncompleteContextOrControls as exc:
         messages.error(request, str(exc))
         return redirect(release_request.get_url())
-    except bll.RequestPermissionDenied as exc:
+    except exceptions.RequestPermissionDenied as exc:
         raise PermissionDenied(str(exc))
 
     messages.success(request, "Request has been submitted")
@@ -401,9 +402,9 @@ def request_review(request, request_id):
 
     try:
         bll.review_request(release_request, request.user)
-    except bll.RequestPermissionDenied as exc:
+    except exceptions.RequestPermissionDenied as exc:
         raise PermissionDenied(str(exc))
-    except bll.RequestReviewDenied as exc:
+    except exceptions.RequestReviewDenied as exc:
         messages.error(request, str(exc))
     else:
         messages.success(request, "Your review has been submitted")
@@ -416,7 +417,7 @@ def _action_request(request, request_id, new_status):
 
     try:
         bll.set_status(release_request, new_status, request.user)
-    except bll.RequestPermissionDenied as exc:
+    except exceptions.RequestPermissionDenied as exc:
         raise PermissionDenied(str(exc))
 
     messages.error(request, f"Request has been {new_status.name.lower()}")
@@ -441,7 +442,7 @@ def request_return(request, request_id):
     release_request = get_release_request_or_raise(request.user, request_id)
     try:
         bll.return_request(release_request, request.user)
-    except bll.RequestPermissionDenied as exc:
+    except exceptions.RequestPermissionDenied as exc:
         raise PermissionDenied(str(exc))
 
     messages.success(request, "Request has been returned to author")
@@ -456,17 +457,17 @@ def file_withdraw(request, request_id, path: str):
 
     try:
         release_request.get_request_file_from_urlpath(grouppath)
-    except bll.FileNotFound:
+    except exceptions.FileNotFound:
         raise Http404()
 
     try:
         bll.withdraw_file_from_request(release_request, grouppath, request.user)
-    except bll.RequestPermissionDenied as exc:
+    except exceptions.RequestPermissionDenied as exc:
         raise PermissionDenied(str(exc))
 
     try:
         release_request.get_request_file_from_urlpath(grouppath)
-    except bll.FileNotFound:
+    except exceptions.FileNotFound:
         # its been removed - redirect to group that contained
         redirect_url = release_request.get_url(grouppath.parts[0])
     else:
@@ -500,12 +501,12 @@ def file_approve(request, request_id, path: str):
 
     try:
         request_file = release_request.get_request_file_from_urlpath(path)
-    except bll.FileNotFound:
+    except exceptions.FileNotFound:
         raise Http404()
 
     try:
         bll.approve_file(release_request, request_file, request.user)
-    except bll.ApprovalPermissionDenied as exc:
+    except exceptions.ApprovalPermissionDenied as exc:
         raise PermissionDenied(str(exc))
 
     return redirect(release_request.get_url(path))
@@ -518,12 +519,12 @@ def file_request_changes(request, request_id, path: str):
 
     try:
         request_file = release_request.get_request_file_from_urlpath(path)
-    except bll.FileNotFound:
+    except exceptions.FileNotFound:
         raise Http404()
 
     try:
         bll.request_changes_to_file(release_request, request_file, request.user)
-    except bll.ApprovalPermissionDenied as exc:
+    except exceptions.ApprovalPermissionDenied as exc:
         raise PermissionDenied(str(exc))
 
     return redirect(release_request.get_url(path))
@@ -536,14 +537,14 @@ def file_reset_review(request, request_id, path: str):
 
     try:
         relpath = release_request.get_request_file_from_urlpath(path).relpath
-    except bll.FileNotFound:
+    except exceptions.FileNotFound:
         raise Http404()
 
     try:
         bll.reset_review_file(release_request, relpath, request.user)
-    except bll.ApprovalPermissionDenied as exc:
+    except exceptions.ApprovalPermissionDenied as exc:
         raise PermissionDenied(str(exc))
-    except bll.FileReviewNotFound:
+    except exceptions.FileReviewNotFound:
         raise Http404()
 
     messages.success(request, "File review has been reset")
@@ -564,9 +565,9 @@ def request_release_files(request, request_id):
         if release_request.status != RequestStatus.APPROVED:
             bll.set_status(release_request, RequestStatus.APPROVED, request.user)
         bll.release_files(release_request, request.user)
-    except bll.RequestPermissionDenied as exc:
+    except exceptions.RequestPermissionDenied as exc:
         messages.error(request, f"Error releasing files: {str(exc)}")
-    except bll.InvalidStateTransition as exc:
+    except exceptions.InvalidStateTransition as exc:
         messages.error(request, f"Error releasing files: {str(exc)}")
     except requests.HTTPError as err:
         if settings.DEBUG:
@@ -629,7 +630,7 @@ def group_edit(request, request_id, group):
                 controls=form.cleaned_data["controls"],
                 user=request.user,
             )
-        except bll.RequestPermissionDenied as exc:  # pragma: nocover
+        except exceptions.RequestPermissionDenied as exc:  # pragma: nocover
             # currently, we can't hit this because of get_release_request_or_raise above.
             # However, that may change, so handle it anyway.
             raise PermissionDenied(str(exc))
@@ -660,11 +661,11 @@ def group_comment_create(request, request_id, group):
                 visibility=Visibility[form.cleaned_data["visibility"]],
                 user=request.user,
             )
-        except bll.RequestPermissionDenied as exc:  # pragma: nocover
+        except exceptions.RequestPermissionDenied as exc:  # pragma: nocover
             # currently, we can't hit this because of get_release_request_or_raise above.
             # However, that may change, so handle it anyway.
             raise PermissionDenied(str(exc))
-        except bll.FileNotFound:
+        except exceptions.FileNotFound:
             messages.error(request, f"Invalid group: {group}")
         else:
             messages.success(request, "Comment added")
@@ -691,11 +692,11 @@ def group_comment_delete(request, request_id, group):
                 comment_id=comment_id,
                 user=request.user,
             )
-        except bll.RequestPermissionDenied as exc:  # pragma: nocover
+        except exceptions.RequestPermissionDenied as exc:  # pragma: nocover
             # currently, we can't hit this because of get_release_request_or_raise above.
             # However, that may change, so handle it anyway.
             raise PermissionDenied(str(exc))
-        except bll.FileNotFound:
+        except exceptions.FileNotFound:
             raise Http404(
                 request, f"Comment not found in group {group} with id {comment_id}"
             )

airlock/views/workspace.py

@@ -9,14 +9,14 @@
 from django.views.decorators.vary import vary_on_headers
 from opentelemetry import trace
 
+from airlock import exceptions
 from airlock.business_logic import (
     RequestFileType,
     bll,
 )
 from airlock.file_browser_api import get_workspace_tree
 from airlock.forms import AddFileForm, FileTypeFormSet, MultiselectForm
 from airlock.types import UrlPath, WorkspaceFileStatus
-from airlock.users import ActionDenied
 from airlock.views.helpers import (
     display_form_errors,
     display_multiple_messages,
@@ -86,7 +86,7 @@ def workspace_view(request, workspace_name: str, path: str = ""):
     try:
         request.user.verify_can_action_request(workspace_name)
         can_action_request = True
-    except ActionDenied:
+    except exceptions.ActionDenied:
         can_action_request = False
 
     multiselect_add = can_action_request and (
@@ -161,7 +161,7 @@ def workspace_contents(request, workspace_name: str, path: str):
 
     try:
         abspath = workspace.abspath(path)
-    except bll.FileNotFound:
+    except exceptions.FileNotFound:
         raise Http404()
 
     if not abspath.is_file():
@@ -288,7 +288,7 @@ def workspace_add_file_to_request(request, workspace_name):
         for formset_form in formset:
             relpath = formset_form.cleaned_data["file"]
             workspace.abspath(relpath)
-    except bll.FileNotFound:
+    except exceptions.FileNotFound:
         raise Http404(f"file {relpath} does not exist")
 
     group_name = (
@@ -311,7 +311,7 @@ def workspace_add_file_to_request(request, workspace_name):
                 bll.update_file_in_request(
                     release_request, relpath, request.user, group_name, filetype
                 )
-            except bll.APIException as err:  # pragma: no cover
+            except exceptions.APIException as err:  # pragma: no cover
                 # it's pretty difficult to hit this error
                 msgs.append(f"{relpath}: {err}")
             else:
@@ -328,7 +328,7 @@ def workspace_add_file_to_request(request, workspace_name):
                 bll.add_file_to_request(
                     release_request, relpath, request.user, group_name, filetype
                 )
-            except bll.APIException as err:
+            except exceptions.APIException as err:
                 # This exception is raised if the file has already been added
                 # (to any group on the request)
                 msgs.append(f"{relpath}: {err}")