diff --git a/airlock/business_logic.py b/airlock/business_logic.py
index c252aa8c..d1f3e24d 100644
--- a/airlock/business_logic.py
+++ b/airlock/business_logic.py
@@ -386,6 +386,16 @@ def output_files_set(self):
for request_file in filegroup.output_files
}
+ def get_file_review_for_reviewer(self, urlpath: UrlPath, reviewer: str):
+ return next(
+ (
+ r
+ for r in self.get_request_file(urlpath).reviews
+ if r.reviewer == reviewer
+ ),
+ None,
+ )
+
def request_filetype(self, urlpath: UrlPath):
try:
return self.get_request_file(urlpath).filetype
diff --git a/airlock/templates/file_browser/contents.html b/airlock/templates/file_browser/contents.html
index 5c454e3d..b215a42b 100644
--- a/airlock/templates/file_browser/contents.html
+++ b/airlock/templates/file_browser/contents.html
@@ -54,7 +54,7 @@
{% endif %}
{% else %}{# request #}
{% if path_item.is_supporting %}
- {% #description_item title="Supporting file" %}This is a supporting file and will not be releaed.{% /description_item%}
+ {% #description_item title="Supporting file" %}This is a supporting file and will not be released.{% /description_item%}
{% elif path_item.is_withdrawn %}
{% #description_item title="Withdrawn file" %}This file has been withdrawn and will not be released.{% /description_item%}
{% endif %}
@@ -68,6 +68,24 @@
{% endif %}
{% elif is_output_checker %}
+ {% if file_approve_url %}
+
+ {% else %}
+ {% csrf_token %}
+ {% #button type="submit" variant="success" disabled=True id="file-approve-button" %}Approve File{% /button %}
+ {% endif %}
+ {% if file_reject_url %}
+
+ {% else %}
+ {% csrf_token %}
+ {% #button type="submit" variant="danger" disabled=True href=file_reject_url id="file-reject-button" %}Reject File{% /button %}
+ {% endif %}
{% #button variant="primary" type="link" href=path_item.download_url id="download-button" %}Download file{% /button %}
{% endif %}
{% endif %}
diff --git a/airlock/urls.py b/airlock/urls.py
index d2565a95..4948de7e 100644
--- a/airlock/urls.py
+++ b/airlock/urls.py
@@ -77,6 +77,16 @@
airlock.views.request_contents,
name="request_contents",
),
+ path(
+ "requests/approve//",
+ airlock.views.file_approve,
+ name="file_approve",
+ ),
+ path(
+ "requests/reject//",
+ airlock.views.file_reject,
+ name="file_reject",
+ ),
path(
"requests/release/",
airlock.views.request_release_files,
diff --git a/airlock/views/__init__.py b/airlock/views/__init__.py
index 58780900..9bf08760 100644
--- a/airlock/views/__init__.py
+++ b/airlock/views/__init__.py
@@ -1,5 +1,7 @@
from .auth import login, logout
from .request import (
+ file_approve,
+ file_reject,
request_contents,
request_index,
request_reject,
@@ -20,6 +22,8 @@
"login",
"logout",
"index",
+ "file_approve",
+ "file_reject",
"request_contents",
"request_index",
"request_reject",
diff --git a/airlock/views/request.py b/airlock/views/request.py
index 55f69562..bf968258 100644
--- a/airlock/views/request.py
+++ b/airlock/views/request.py
@@ -11,7 +11,13 @@
from django.views.decorators.vary import vary_on_headers
from opentelemetry import trace
-from airlock.business_logic import RequestStatus, UrlPath, bll
+from airlock.business_logic import (
+ FileReviewStatus,
+ RequestFileType,
+ RequestStatus,
+ UrlPath,
+ bll,
+)
from airlock.file_browser_api import get_request_tree
from services.tracing import instrument
@@ -87,6 +93,33 @@ def request_view(request, request_id: str, path: str = ""):
kwargs={"request_id": request_id},
)
+ if (
+ is_directory_url
+ or release_request.request_filetype(path) == RequestFileType.SUPPORTING
+ ):
+ file_approve_url = None
+ file_reject_url = None
+ else:
+ file_approve_url = reverse(
+ "file_approve",
+ kwargs={"request_id": request_id, "path": path},
+ )
+ file_reject_url = reverse(
+ "file_reject",
+ kwargs={"request_id": request_id, "path": path},
+ )
+
+ existing_review = release_request.get_file_review_for_reviewer(
+ path, request.user.username
+ )
+ if existing_review:
+ if existing_review.status == FileReviewStatus.APPROVED:
+ file_approve_url = None
+ elif existing_review.status == FileReviewStatus.REJECTED:
+ file_reject_url = None
+ else:
+ assert False, "Invalid FileReviewStatus value"
+
context = {
"workspace": bll.get_workspace(release_request.workspace, request.user),
"release_request": release_request,
@@ -97,6 +130,8 @@ def request_view(request, request_id: str, path: str = ""):
# TODO file these in from user/models
"is_author": is_author,
"is_output_checker": request.user.output_checker,
+ "file_approve_url": file_approve_url,
+ "file_reject_url": file_reject_url,
"request_submit_url": request_submit_url,
"request_reject_url": request_reject_url,
"release_files_url": release_files_url,
@@ -191,6 +226,44 @@ def request_withdraw(request, request_id):
return redirect(redirect_url)
+@instrument(func_attributes={"release_request": "request_id"})
+@require_http_methods(["POST"])
+def file_approve(request, request_id, path: str):
+ release_request = get_release_request_or_raise(request.user, request_id)
+
+ try:
+ relpath = release_request.get_request_file(path).relpath
+ except bll.FileNotFound:
+ raise Http404()
+
+ try:
+ bll.approve_file(release_request, relpath, request.user)
+ except bll.ApprovalPermissionDenied as exc:
+ raise PermissionDenied(str(exc))
+
+ messages.success(request, "File has been approved")
+ return redirect(release_request.get_url(path))
+
+
+@instrument(func_attributes={"release_request": "request_id"})
+@require_http_methods(["POST"])
+def file_reject(request, request_id, path: str):
+ release_request = get_release_request_or_raise(request.user, request_id)
+
+ try:
+ relpath = release_request.get_request_file(path).relpath
+ except bll.FileNotFound:
+ raise Http404()
+
+ try:
+ bll.reject_file(release_request, relpath, request.user)
+ except bll.ApprovalPermissionDenied as exc:
+ raise PermissionDenied(str(exc))
+
+ messages.success(request, "File has been rejected")
+ return redirect(release_request.get_url(path))
+
+
@instrument(func_attributes={"release_request": "request_id"})
@require_http_methods(["POST"])
def request_release_files(request, request_id):
diff --git a/tests/functional/test_e2e.py b/tests/functional/test_e2e.py
index 620f5864..adfbeace 100644
--- a/tests/functional/test_e2e.py
+++ b/tests/functional/test_e2e.py
@@ -101,7 +101,10 @@ def test_e2e_release_files(page, live_server, dev_users, release_files_stubber):
- View requests list
- Click and view submitted request
- View output file
+ - Reject output file
+ - Approve output file
- Download output file
+ - View supporting file
- Release files
- View requests list again and confirm released request is not shown
"""
@@ -320,12 +323,33 @@ def test_e2e_release_files(page, live_server, dev_users, release_files_stubber):
"src", release_request.get_contents_url("my-new-group/subdir/file.txt")
)
+ # Reject the file
+ expect(page.locator("#file-reject-button")).not_to_have_attribute("disabled", "")
+ find_and_click(page.locator("#file-reject-button"))
+ expect(page.locator("#file-reject-button")).to_have_attribute("disabled", "")
+
+ # Change our minds & approve the file
+ expect(page.locator("#file-approve-button")).not_to_have_attribute("disabled", "")
+ find_and_click(page.locator("#file-approve-button"))
+ expect(page.locator("#file-approve-button")).to_have_attribute("disabled", "")
+
# Download the file
with page.expect_download() as download_info:
find_and_click(page.locator("#download-button"))
download = download_info.value
assert download.suggested_filename == "file.txt"
+ # Look at a supporting file & verify it can't be approved
+ supporting_file_link = page.get_by_role("link", name="supporting.txt").locator(
+ ".file:scope"
+ )
+ find_and_click(supporting_file_link)
+ expect(page.locator("iframe")).to_have_attribute(
+ "src", release_request.get_contents_url("my-new-group/subdir/supporting.txt")
+ )
+ expect(page.locator("#file-approve-button")).to_have_attribute("disabled", "")
+ expect(page.locator("#file-reject-button")).to_have_attribute("disabled", "")
+
# Mock the responses from job-server
release_request = bll.get_release_request(request_id, admin_user)
release_files_stubber(release_request)
diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py
index a60027f5..58639b9d 100644
--- a/tests/integration/views/test_request.py
+++ b/tests/integration/views/test_request.py
@@ -5,6 +5,7 @@
from airlock.business_logic import (
AuditEventType,
+ FileReviewStatus,
RequestFileType,
RequestStatus,
UrlPath,
@@ -111,6 +112,72 @@ def test_request_view_with_authored_request_file(airlock_client):
assert "Withdraw this file" in response.rendered_content
+def test_request_view_with_submitted_file(airlock_client):
+ airlock_client.login(output_checker=True)
+ release_request = factories.create_release_request(
+ "workspace",
+ status=RequestStatus.SUBMITTED,
+ )
+ factories.write_request_file(release_request, "group", "file.txt", "foobar")
+ response = airlock_client.get(
+ f"/requests/view/{release_request.id}/group/file.txt", follow=True
+ )
+ assert "Remove this file" not in response.rendered_content
+ assert "Approve File" in response.rendered_content
+ assert "Reject File" in response.rendered_content
+
+
+def test_request_view_with_submitted_supporting_file(airlock_client):
+ airlock_client.login(output_checker=True)
+ release_request = factories.create_release_request(
+ "workspace",
+ status=RequestStatus.SUBMITTED,
+ )
+ factories.write_request_file(
+ release_request,
+ "group",
+ "supporting_file.txt",
+ filetype=RequestFileType.SUPPORTING,
+ )
+ response = airlock_client.get(
+ f"/requests/view/{release_request.id}/group/supporting_file.txt", follow=True
+ )
+ assert "Remove this file" not in response.rendered_content
+ # these buttons currently exist but are both disabled
+ assert "Approve File" in response.rendered_content
+ assert "Reject File" in response.rendered_content
+
+
+def test_request_view_with_submitted_file_approved(airlock_client):
+ airlock_client.login(output_checker=True)
+ release_request = factories.create_release_request(
+ "workspace",
+ status=RequestStatus.SUBMITTED,
+ )
+ factories.write_request_file(release_request, "group", "file.txt", "foobar")
+ airlock_client.post(f"/requests/approve/{release_request.id}/group/file.txt")
+ response = airlock_client.get(
+ f"/requests/view/{release_request.id}/group/file.txt", follow=True
+ )
+ assert "Approve File" in response.rendered_content
+ assert "Reject File" in response.rendered_content
+
+
+def test_request_view_with_submitted_file_rejected(airlock_client):
+ airlock_client.login(output_checker=True)
+ release_request = factories.create_release_request(
+ "workspace",
+ status=RequestStatus.SUBMITTED,
+ )
+ factories.write_request_file(release_request, "group", "file.txt", "foobar")
+ airlock_client.post(f"/requests/reject/{release_request.id}/group/file.txt")
+ response = airlock_client.get(
+ f"/requests/view/{release_request.id}/group/file.txt", follow=True
+ )
+ assert "Approve File" in response.rendered_content
+ assert "Reject File" in response.rendered_content
+
+
def test_request_view_with_404(airlock_client):
airlock_client.login(output_checker=True)
release_request = factories.create_release_request("workspace")
@@ -376,6 +443,116 @@ def test_request_submit_not_author(airlock_client):
assert persisted_request.status == RequestStatus.PENDING
+@pytest.mark.parametrize("review", [("approve"), ("reject")])
+def test_file_review_bad_user(airlock_client, review):
+ workspace = "test1"
+ airlock_client.login(workspaces=[workspace], output_checker=False)
+ author = factories.create_user("author", [workspace], False)
+ release_request = factories.create_release_request(
+ workspace,
+ user=author,
+ status=RequestStatus.SUBMITTED,
+ )
+ path = "path/test.txt"
+ factories.write_request_file(release_request, "group", path, contents="test")
+
+ response = airlock_client.post(
+ f"/requests/{review}/{release_request.id}/group/{path}"
+ )
+ assert response.status_code == 403
+ relpath = UrlPath(path)
+ assert (
+ len(
+ factories.bll.get_release_request(release_request.id, author)
+ .filegroups["group"]
+ .files[relpath]
+ .reviews
+ )
+ == 0
+ )
+
+
+@pytest.mark.parametrize("review", [("approve"), ("reject")])
+def test_file_review_bad_file(airlock_client, review):
+ airlock_client.login(output_checker=True)
+ author = factories.create_user("author", ["test1"], False)
+ release_request = factories.create_release_request(
+ "test1",
+ user=author,
+ status=RequestStatus.SUBMITTED,
+ )
+ path = "path/test.txt"
+ factories.write_request_file(release_request, "group", path, contents="test")
+
+ bad_path = "path/bad.txt"
+ response = airlock_client.post(
+ f"/requests/{review}/{release_request.id}/group/{bad_path}"
+ )
+ assert response.status_code == 404
+ relpath = UrlPath(path)
+ assert (
+ len(
+ factories.bll.get_release_request(release_request.id, author)
+ .filegroups["group"]
+ .files[relpath]
+ .reviews
+ )
+ == 0
+ )
+
+
+def test_file_approve(airlock_client):
+ airlock_client.login(output_checker=True)
+ author = factories.create_user("author", ["test1"], False)
+ release_request = factories.create_release_request(
+ "test1",
+ user=author,
+ status=RequestStatus.SUBMITTED,
+ )
+ path = "path/test.txt"
+ factories.write_request_file(release_request, "group", path, contents="test")
+
+ response = airlock_client.post(
+ f"/requests/approve/{release_request.id}/group/{path}"
+ )
+ assert response.status_code == 302
+ relpath = UrlPath(path)
+ review = (
+ factories.bll.get_release_request(release_request.id, author)
+ .filegroups["group"]
+ .files[relpath]
+ .reviews[0]
+ )
+ assert review.status == FileReviewStatus.APPROVED
+ assert review.reviewer == "testuser"
+
+
+def test_file_reject(airlock_client):
+ airlock_client.login(output_checker=True)
+ author = factories.create_user("author", ["test1"], False)
+ release_request = factories.create_release_request(
+ "test1",
+ user=author,
+ status=RequestStatus.SUBMITTED,
+ )
+ path = "path/test.txt"
+ factories.write_request_file(release_request, "group", path, contents="test")
+
+ response = airlock_client.post(
+ f"/requests/reject/{release_request.id}/group/{path}"
+ )
+ assert response.status_code == 302
+ relpath = UrlPath(path)
+ review = (
+ factories.bll.get_release_request(release_request.id, author)
+ .filegroups["group"]
+ .files[relpath]
+ .reviews[0]
+ )
+ assert review.status == FileReviewStatus.REJECTED
+ assert review.reviewer == "testuser"
+
+
def test_request_reject_output_checker(airlock_client):
airlock_client.login(output_checker=True)
author = factories.create_user("author", ["test1"], False)
diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py
index 03cd8f60..e70acd61 100644
--- a/tests/unit/test_business_logic.py
+++ b/tests/unit/test_business_logic.py
@@ -925,6 +925,47 @@ def test_approve_then_reject_file(bll):
assert len(current_reviews) == 1
+def test_get_file_review_for_reviewer(bll):
+ release_request, path, author = setup_empty_release_request()
+ checker = factories.create_user("checker", [], True)
+ checker2 = factories.create_user("checker2", [], True)
+
+ bll.add_file_to_request(release_request, path, author, "default")
+ bll.set_status(
+ release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author
+ )
+
+ assert len(_get_current_file_reviews(bll, release_request, path, checker)) == 0
+ fullpath = "default" / path
+ assert release_request.get_file_review_for_reviewer(fullpath, "checker") is None
+
+ bll.approve_file(release_request, path, checker)
+ bll.reject_file(release_request, path, checker2)
+ release_request = factories.refresh_release_request(release_request, author)
+
+ assert (
+ release_request.get_file_review_for_reviewer(fullpath, "checker").status
+ is FileReviewStatus.APPROVED
+ )
+ assert (
+ release_request.get_file_review_for_reviewer(fullpath, "checker2").status
+ is FileReviewStatus.REJECTED
+ )
+
+ bll.reject_file(release_request, path, checker)
+ bll.approve_file(release_request, path, checker2)
+ release_request = factories.refresh_release_request(release_request, author)
+
+ assert (
+ release_request.get_file_review_for_reviewer(fullpath, "checker").status
+ is FileReviewStatus.REJECTED
+ )
+ assert (
+ release_request.get_file_review_for_reviewer(fullpath, "checker2").status
+ is FileReviewStatus.APPROVED
+ )
+
+
# add DAL method names to this if they do not require auditing
DAL_AUDIT_EXCLUDED = {
"get_release_request",