From 9ad5c2e6438aa232c727a104f9e88c231a40b409 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Wed, 28 Aug 2024 13:49:15 +0100 Subject: [PATCH 1/4] Move STATUS_OWNERS into permissions This is really a permission definition anyway. But also, it cuts a dependency loop, and allows further refactoring of the BLL. --- airlock/business_logic.py | 32 ++++--------------------- airlock/permissions.py | 22 +++++++++++++++++ airlock/policies.py | 6 ++++- local_db/data_access.py | 8 +++---- tests/integration/views/test_request.py | 4 ++-- tests/unit/test_business_logic.py | 4 ++-- 6 files changed, 38 insertions(+), 38 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index a3a03093..17fc3f15 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -1007,7 +1007,7 @@ def submitted_reviews_count(self): return len(self.submitted_reviews) def status_owner(self) -> RequestStatusOwner: - return BusinessLogicLayer.STATUS_OWNERS[self.status] + return permissions.STATUS_OWNERS[self.status] def can_be_released(self) -> bool: return ( @@ -1016,19 +1016,13 @@ def can_be_released(self) -> bool: ) def is_final(self): - return ( - BusinessLogicLayer.STATUS_OWNERS[self.status] == RequestStatusOwner.SYSTEM - ) + return self.status_owner() == RequestStatusOwner.SYSTEM def is_under_review(self): - return ( - BusinessLogicLayer.STATUS_OWNERS[self.status] == RequestStatusOwner.REVIEWER - ) + return self.status_owner() == RequestStatusOwner.REVIEWER def is_editing(self): - return ( - BusinessLogicLayer.STATUS_OWNERS[self.status] == RequestStatusOwner.AUTHOR - ) + return self.status_owner() == RequestStatusOwner.AUTHOR def store_file(release_request: ReleaseRequest, abspath: Path) -> str: @@ -1401,24 +1395,6 @@ def get_approved_requests(self, user: User): ], } - # The following lists should a) include every status and b) be disjoint - # This is validated in tests - # - STATUS_OWNERS = { - # states where only the author can edit this request - RequestStatus.PENDING: RequestStatusOwner.AUTHOR, - RequestStatus.RETURNED: RequestStatusOwner.AUTHOR, - # states where only an output-checker can edit this request - RequestStatus.SUBMITTED: RequestStatusOwner.REVIEWER, - RequestStatus.PARTIALLY_REVIEWED: RequestStatusOwner.REVIEWER, - RequestStatus.REVIEWED: RequestStatusOwner.REVIEWER, - # states where no user can edit - RequestStatus.WITHDRAWN: RequestStatusOwner.SYSTEM, - RequestStatus.APPROVED: RequestStatusOwner.SYSTEM, - RequestStatus.REJECTED: RequestStatusOwner.SYSTEM, - RequestStatus.RELEASED: RequestStatusOwner.SYSTEM, - } - STATUS_AUDIT_EVENT = { RequestStatus.PENDING: AuditEventType.REQUEST_CREATE, RequestStatus.SUBMITTED: AuditEventType.REQUEST_SUBMIT, diff --git a/airlock/permissions.py b/airlock/permissions.py index d4d21ebf..c9f18ee5 100644 --- a/airlock/permissions.py +++ b/airlock/permissions.py @@ -5,6 +5,10 @@ from typing import TYPE_CHECKING from airlock import exceptions, policies +from airlock.enums import ( + RequestStatus, + RequestStatusOwner, +) from airlock.types import UrlPath from airlock.users import User @@ -18,6 +22,24 @@ from airlock.business_logic import Comment, ReleaseRequest, Workspace +# The following lists should a) include every status and b) be disjoint +# This is validated in tests +STATUS_OWNERS = { + # states where only the author can edit this request + RequestStatus.PENDING: RequestStatusOwner.AUTHOR, + RequestStatus.RETURNED: RequestStatusOwner.AUTHOR, + # states where only an output-checker can edit this request + RequestStatus.SUBMITTED: RequestStatusOwner.REVIEWER, + RequestStatus.PARTIALLY_REVIEWED: RequestStatusOwner.REVIEWER, + RequestStatus.REVIEWED: RequestStatusOwner.REVIEWER, + # states where no user can edit + RequestStatus.WITHDRAWN: RequestStatusOwner.SYSTEM, + RequestStatus.APPROVED: RequestStatusOwner.SYSTEM, + RequestStatus.REJECTED: RequestStatusOwner.SYSTEM, + RequestStatus.RELEASED: RequestStatusOwner.SYSTEM, +} + + def check_user_can_view_workspace(user: User | None, workspace_name: str): """ This user can access and view the workspace and requests for it. diff --git a/airlock/policies.py b/airlock/policies.py index 6bc71502..12692d86 100644 --- a/airlock/policies.py +++ b/airlock/policies.py @@ -16,7 +16,11 @@ from typing import TYPE_CHECKING from airlock import exceptions -from airlock.enums import RequestFileVote, RequestStatus, WorkspaceFileStatus +from airlock.enums import ( + RequestFileVote, + RequestStatus, + WorkspaceFileStatus, +) from airlock.types import UrlPath from airlock.utils import is_valid_file_type diff --git a/local_db/data_access.py b/local_db/data_access.py index 52d4c56d..c3eafb6d 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -1,10 +1,9 @@ from django.db import transaction from django.utils import timezone -from airlock import exceptions +from airlock import exceptions, permissions from airlock.business_logic import ( AuditEvent, - BusinessLogicLayer, DataAccessLayerProtocol, ) from airlock.enums import ( @@ -72,7 +71,7 @@ def get_active_requests_for_workspace_by_user(self, workspace: str, username: st # author or a reviewer, and are considered active editable_status = [ status - for status, owner in BusinessLogicLayer.STATUS_OWNERS.items() + for status, owner in permissions.STATUS_OWNERS.items() if owner != RequestStatusOwner.SYSTEM ] return [ @@ -202,8 +201,7 @@ def delete_file_from_request( request = self._find_metadata(request_id) assert ( - BusinessLogicLayer.STATUS_OWNERS[request.status] - == RequestStatusOwner.AUTHOR + permissions.STATUS_OWNERS[request.status] == RequestStatusOwner.AUTHOR ) try: diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py index 59dff0bb..98e64f06 100644 --- a/tests/integration/views/test_request.py +++ b/tests/integration/views/test_request.py @@ -5,7 +5,7 @@ from django.contrib.messages import get_messages from django.template.response import TemplateResponse -from airlock import exceptions +from airlock import exceptions, permissions from airlock.business_logic import bll from airlock.enums import ( AuditEventType, @@ -361,7 +361,7 @@ def test_request_view_with_authored_request_file(airlock_client, status): response = airlock_client.get( f"/requests/view/{release_request.id}/group/file.txt", follow=True ) - can_withdraw = bll.STATUS_OWNERS[status] == RequestStatusOwner.AUTHOR + can_withdraw = permissions.STATUS_OWNERS[status] == RequestStatusOwner.AUTHOR assert ("Withdraw this file" in response.rendered_content) == can_withdraw diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index bc88a951..1ca895be 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -11,7 +11,7 @@ from django.utils.dateparse import parse_datetime import old_api -from airlock import exceptions +from airlock import exceptions, permissions from airlock.business_logic import ( AuditEvent, BusinessLogicLayer, @@ -1226,7 +1226,7 @@ def test_set_status(current, future, valid_author, valid_checker, withdrawn_afte def test_request_status_ownership(bll): """Test every RequestStatus has been assigned an ownership""" - missing_states = set(RequestStatus) - BusinessLogicLayer.STATUS_OWNERS.keys() + missing_states = set(RequestStatus) - permissions.STATUS_OWNERS.keys() assert missing_states == set() From 0baa6f153df9891bd604a9c321339123f239dca1 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Wed, 28 Aug 2024 15:41:15 +0100 Subject: [PATCH 2/4] Move visibility logic into its own module This carves off a small chunk of the BLL logic into its own module for handling the complexities around vote/comment/audit visibility. Also moves the main integration test for this logic into its own test file. This is not exactly a unit test - due the nature of the complex state manipulation, its more of an integration tests, so its been moved into that test directory. It is our longest and most complex test, with a whole support class to aid, so breaking it out makes a certain amount of sense. --- airlock/business_logic.py | 73 +----- airlock/file_browser_api.py | 2 +- airlock/visibility.py | 82 +++++++ tests/integration/test_visibility.py | 339 +++++++++++++++++++++++++++ tests/unit/test_business_logic.py | 329 +------------------------- 5 files changed, 425 insertions(+), 400 deletions(-) create mode 100644 airlock/visibility.py create mode 100644 tests/integration/test_visibility.py diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 17fc3f15..25899ded 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -9,7 +9,7 @@ from datetime import datetime from functools import cached_property from pathlib import Path -from typing import Any, Protocol, Self, Sequence, cast +from typing import Any, Protocol, Self, cast from django.conf import settings from django.urls import reverse @@ -41,6 +41,7 @@ from airlock.types import FileMetadata, UrlPath from airlock.users import User from airlock.utils import is_valid_file_type +from airlock.visibility import RequestFileStatus, filter_visible_items ROOT_PATH = UrlPath() # empty path @@ -48,15 +49,6 @@ logger = logging.getLogger(__name__) -@dataclass -class RequestFileStatus: - """The current visible decision and inidividual vote for a specific user.""" - - user: User - decision: RequestFileDecision - vote: RequestFileVote | None - - READONLY_EVENTS = { AuditEventType.WORKSPACE_FILE_VIEW, AuditEventType.REQUEST_FILE_VIEW, @@ -164,67 +156,6 @@ def visibility(self) -> Visibility: return Visibility.PUBLIC -class VisibleItem(Protocol): - @property - def author(self) -> str: - raise NotImplementedError() - - @property - def review_turn(self) -> int: - raise NotImplementedError() - - @property - def visibility(self) -> Visibility: - raise NotImplementedError() - - -def filter_visible_items( - items: Sequence[VisibleItem], - current_turn: int, - current_phase: ReviewTurnPhase, - user_can_review: bool, - user: User, -): - """Filter a list of items to only include items this user is allowed to view. - - This depends on the current turn, phase, and whether the user is the author - of said item. - """ - for item in items: - # you can always see things you've authored. Doing this first - # simplifies later logic, and avoids potential bugs with users adding - # items but then they can not see the item they just added - if item.author == user.username: - yield item - continue - - match item.visibility: - case Visibility.PUBLIC: - # can always see public items from previous turns and completed turns - if ( - item.review_turn < current_turn - or current_phase == ReviewTurnPhase.COMPLETE - ): - yield item - # can see public items for other users if CONSOLIDATING and can review - elif current_phase == ReviewTurnPhase.CONSOLIDATING and user_can_review: - yield item - case Visibility.PRIVATE: - # have to be able to review this request to see *any* private items - if user_can_review: - # can always see private items from previous turns - if ( - item.review_turn < current_turn - or current_phase == ReviewTurnPhase.COMPLETE - ): - yield item - # can only see private items from current turn if we are not INDEPENDENT - elif current_phase != ReviewTurnPhase.INDEPENDENT: - yield item - case _: # pragma: nocover - assert False - - class AirlockContainer(Protocol): """Structural typing class for a instance of a Workspace or ReleaseRequest diff --git a/airlock/file_browser_api.py b/airlock/file_browser_api.py index 8f8ec96c..b758b656 100644 --- a/airlock/file_browser_api.py +++ b/airlock/file_browser_api.py @@ -10,13 +10,13 @@ AirlockContainer, CodeRepo, ReleaseRequest, - RequestFileStatus, Workspace, ) from airlock.enums import PathType, RequestFileType, WorkspaceFileStatus from airlock.types import FileMetadata, UrlPath from airlock.users import User from airlock.utils import is_valid_file_type +from airlock.visibility import RequestFileStatus from services.tracing import instrument diff --git a/airlock/visibility.py b/airlock/visibility.py new file mode 100644 index 00000000..7e6ab2d5 --- /dev/null +++ b/airlock/visibility.py @@ -0,0 +1,82 @@ +from __future__ import annotations + +from dataclasses import dataclass +from typing import Protocol, Sequence + +from airlock.enums import ( + RequestFileDecision, + RequestFileVote, + ReviewTurnPhase, + Visibility, +) +from airlock.users import User + + +@dataclass +class RequestFileStatus: + """The current visible decision and individual vote for a specific user.""" + + user: User + decision: RequestFileDecision + vote: RequestFileVote | None + + +class VisibleItem(Protocol): + @property + def author(self) -> str: + raise NotImplementedError() + + @property + def review_turn(self) -> int: + raise NotImplementedError() + + @property + def visibility(self) -> Visibility: + raise NotImplementedError() + + +def filter_visible_items( + items: Sequence[VisibleItem], + current_turn: int, + current_phase: ReviewTurnPhase, + user_can_review: bool, + user: User, +): + """Filter a list of items to only include items this user is allowed to view. + + This depends on the current turn, phase, and whether the user is the author + of said item. + """ + for item in items: + # you can always see things you've authored. Doing this first + # simplifies later logic, and avoids potential bugs with users adding + # items but then they can not see the item they just added + if item.author == user.username: + yield item + continue + + match item.visibility: + case Visibility.PUBLIC: + # can always see public items from previous turns and completed turns + if ( + item.review_turn < current_turn + or current_phase == ReviewTurnPhase.COMPLETE + ): + yield item + # can see public items for other users if CONSOLIDATING and can review + elif current_phase == ReviewTurnPhase.CONSOLIDATING and user_can_review: + yield item + case Visibility.PRIVATE: + # have to be able to review this request to see *any* private items + if user_can_review: + # can always see private items from previous turns + if ( + item.review_turn < current_turn + or current_phase == ReviewTurnPhase.COMPLETE + ): + yield item + # can only see private items from current turn if we are not INDEPENDENT + elif current_phase != ReviewTurnPhase.INDEPENDENT: + yield item + case _: # pragma: nocover + assert False diff --git a/tests/integration/test_visibility.py b/tests/integration/test_visibility.py new file mode 100644 index 00000000..0e4fd058 --- /dev/null +++ b/tests/integration/test_visibility.py @@ -0,0 +1,339 @@ +from dataclasses import dataclass + +from airlock.business_logic import ( + AuditEvent, + BusinessLogicLayer, + Comment, + ReleaseRequest, +) +from airlock.enums import ( + AuditEventType, + RequestFileVote, + RequestStatus, + ReviewTurnPhase, + Visibility, +) +from airlock.users import User +from tests import factories + + +@dataclass +class VisibleItemsHelper: + """Helper class to make assertions about visiblity of comments and audit logs. + + It will fetch comments and audits that are visible for a specific request and + user, and store them to make assertions about. + """ + + comments: list[tuple[Comment, str]] + audits: list[AuditEvent] + + @classmethod + def for_group( + cls, request: ReleaseRequest, user: User, group, bll: BusinessLogicLayer + ): + return cls( + comments=request.get_visible_comments_for_group(group, user), + audits=bll.get_request_audit_log(user, request, group), + ) + + def is_comment_visible(self, text: str, author: User) -> bool: + for c, _ in self.comments: + if c.comment == text and c.author == author.username: + return True + + return False + + def is_audit_visible(self, type_: AuditEventType, author: User) -> bool: + for audit in self.audits: + if audit.type == type_ and audit.user == author.username: + return True + + return False + + def comment(self, text: str, author: User) -> bool: + """Is this comment in the list of visible items we have?""" + if not self.is_comment_visible(text=text, author=author): + return False + + return self.is_audit_visible( + type_=AuditEventType.REQUEST_COMMENT, author=author + ) + + def vote(self, vote: RequestFileVote, author: User) -> bool: + """Is this vote in the list of visible items we have?""" + match vote: + case RequestFileVote.APPROVED: + event_type = AuditEventType.REQUEST_FILE_APPROVE + case RequestFileVote.CHANGES_REQUESTED: + event_type = AuditEventType.REQUEST_FILE_REQUEST_CHANGES + case _: # pragma: nocover + assert False + + return self.is_audit_visible(type_=event_type, author=author) + + +def test_request_comment_and_audit_visibility(bll): + # This test is long and complex. + # + # It tests both get_visible_comments_for_group() and + # get_request_audit_log(), and uses the custom VisibleItems helper above. + # + # It walks through a couple of rounds and turns of back and forth review, + # validating that the comments that are visibile to various users at + # different points in the process are correct. + # + author = factories.create_user("author1", workspaces=["workspace"]) + checkers = factories.get_default_output_checkers() + + release_request = factories.create_request_at_status( + "workspace", + status=RequestStatus.SUBMITTED, + author=author, + files=[factories.request_file(group="group", path="file.txt", approved=True)], + ) + + bll.group_comment_create( + release_request, + "group", + "turn 1 checker 0 private", + Visibility.PRIVATE, + checkers[0], + ) + bll.group_comment_create( + release_request, + "group", + "turn 1 checker 1 private", + Visibility.PRIVATE, + checkers[1], + ) + + release_request = factories.refresh_release_request(release_request) + + assert release_request.review_turn == 1 + assert release_request.get_turn_phase() == ReviewTurnPhase.INDEPENDENT + + def get_visible_items(user): + return VisibleItemsHelper.for_group(release_request, user, "group", bll) + + # in ReviewTurnPhase.INDEPENDENT, checkers can only see own comments, author can see nothing + visible = get_visible_items(checkers[0]) + assert visible.comment("turn 1 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert not visible.comment("turn 1 checker 1 private", checkers[1]) + assert not visible.vote(RequestFileVote.APPROVED, checkers[1]) + + visible = get_visible_items(checkers[1]) + assert not visible.comment("turn 1 checker 0 private", checkers[0]) + assert not visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert visible.comment("turn 1 checker 1 private", checkers[1]) + assert visible.vote(RequestFileVote.APPROVED, checkers[1]) + + visible = get_visible_items(author) + assert not visible.comment("turn 1 checker 0 private", checkers[0]) + assert not visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert not visible.comment("turn 1 checker 1 private", checkers[1]) + assert not visible.vote(RequestFileVote.APPROVED, checkers[1]) + + factories.submit_independent_review(release_request) + bll.group_comment_create( + release_request, + "group", + "turn 1 checker 0 public", + Visibility.PUBLIC, + checkers[0], + ) + release_request = factories.refresh_release_request(release_request) + + assert release_request.review_turn == 1 + assert release_request.get_turn_phase() == ReviewTurnPhase.CONSOLIDATING + + # in ReviewTurnPhase.CONSOLIDATING, checkers should see all private comments + # and pending public comments, but author should not see any yet + for checker in checkers: + visible = get_visible_items(checker) + assert visible.comment("turn 1 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert visible.comment("turn 1 checker 1 private", checkers[1]) + assert visible.vote(RequestFileVote.APPROVED, checkers[1]) + assert visible.comment("turn 1 checker 0 public", checkers[0]) + + # author still cannot see anything + visible = get_visible_items(author) + assert not visible.comment("turn 1 checker 0 private", checkers[0]) + assert not visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert not visible.comment("turn 1 checker 1 private", checkers[1]) + assert not visible.vote(RequestFileVote.APPROVED, checkers[1]) + assert not visible.comment("turn 1 checker 0 public", checkers[0]) + + bll.return_request(release_request, checkers[0]) + release_request = factories.refresh_release_request(release_request) + bll.group_comment_create( + release_request, + "group", + "turn 2 author public", + Visibility.PUBLIC, + author, + ) + release_request = factories.refresh_release_request(release_request) + + assert release_request.review_turn == 2 + assert release_request.get_turn_phase() == ReviewTurnPhase.AUTHOR + + # in ReviewTurnPhase.AUTHOR, checkers should see turn 1 comments, but not authors turn 2 comments. + # Author should turn 1 and 2 public comments + for checker in checkers: + visible = get_visible_items(checker) + assert visible.comment("turn 1 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert visible.comment("turn 1 checker 1 private", checkers[1]) + assert visible.vote(RequestFileVote.APPROVED, checkers[1]) + assert visible.comment("turn 1 checker 0 public", checkers[0]) + assert not visible.comment("turn 2 author public", author) + + # author can see al turn 1 public comments and votes, their turn 2 public comments, but no private comments. + visible = get_visible_items(author) + assert not visible.comment("turn 1 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert not visible.comment("turn 1 checker 1 private", checkers[1]) + assert visible.vote(RequestFileVote.APPROVED, checkers[1]) + assert visible.comment("turn 1 checker 0 public", checkers[0]) + assert visible.comment("turn 2 author public", author) + + bll.submit_request(release_request, author) + release_request = factories.refresh_release_request(release_request) + assert release_request.review_turn == 3 + + bll.group_comment_create( + release_request, + "group", + "turn 3 checker 0 private", + Visibility.PRIVATE, + checkers[0], + ) + bll.group_comment_create( + release_request, + "group", + "turn 3 checker 1 private", + Visibility.PRIVATE, + checkers[1], + ) + # checker0 requests changes to the file now. Not realistic, but we want to check that + # the audit log for later round votes is hidden + bll.request_changes_to_file( + release_request, + release_request.get_request_file_from_output_path("file.txt"), + checkers[0], + ) + release_request = factories.refresh_release_request(release_request) + + # in ReviewTurnPhase.INDEPENDENT for a 2nd round + # Checkers should see previous round's private comments, but not this rounds + # Author should see previous round's public comments, but not any this round + visible = get_visible_items(checkers[0]) + assert visible.comment("turn 1 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert visible.comment("turn 1 checker 1 private", checkers[1]) + assert visible.vote(RequestFileVote.APPROVED, checkers[1]) + assert visible.comment("turn 1 checker 0 public", checkers[0]) + assert visible.comment("turn 2 author public", author) + assert visible.comment("turn 3 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) + assert not visible.comment("turn 3 checker 1 private", checkers[1]) + + visible = get_visible_items(checkers[1]) + assert visible.comment("turn 1 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert visible.comment("turn 1 checker 1 private", checkers[1]) + assert visible.vote(RequestFileVote.APPROVED, checkers[1]) + assert visible.comment("turn 1 checker 0 public", checkers[0]) + assert visible.comment("turn 2 author public", author) + assert not visible.comment("turn 3 checker 0 private", checkers[0]) + assert not visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) + assert visible.comment("turn 3 checker 1 private", checkers[1]) + + visible = get_visible_items(author) + assert not visible.comment("turn 1 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert not visible.comment("turn 1 checker 1 private", checkers[1]) + assert visible.vote(RequestFileVote.APPROVED, checkers[1]) + assert visible.comment("turn 1 checker 0 public", checkers[0]) + assert visible.comment("turn 2 author public", author) + assert not visible.comment("turn 3 checker 0 private", checkers[0]) + assert not visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) + assert not visible.comment("turn 3 checker 1 private", checkers[1]) + + factories.submit_independent_review(release_request) + release_request = factories.refresh_release_request(release_request) + bll.group_comment_create( + release_request, + "group", + "turn 3 checker 0 public", + Visibility.PUBLIC, + checkers[0], + ) + release_request = factories.refresh_release_request(release_request) + + # in ReviewTurnPhase.CONSOLIDATING for a 2nd round + # Checkers should see previous and current round's private comments, + # Author should see previous round's public comments, but not any private comments + for checker in checkers: + visible = get_visible_items(checker) + assert visible.comment("turn 1 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert visible.comment("turn 1 checker 1 private", checkers[1]) + assert visible.vote(RequestFileVote.APPROVED, checkers[1]) + assert visible.comment("turn 1 checker 0 public", checkers[0]) + assert visible.comment("turn 2 author public", author) + assert visible.comment("turn 3 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) + assert visible.comment("turn 3 checker 1 private", checkers[1]) + assert visible.comment("turn 3 checker 0 public", checkers[0]) + + # author sees no private comments, and no turn 3 things. + visible = get_visible_items(author) + assert not visible.comment("turn 1 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert not visible.comment("turn 1 checker 1 private", checkers[1]) + assert visible.vote(RequestFileVote.APPROVED, checkers[1]) + assert visible.comment("turn 1 checker 0 public", checkers[0]) + assert visible.comment("turn 2 author public", author) + assert not visible.comment("turn 3 checker 0 private", checkers[0]) + assert not visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) + assert not visible.comment("turn 3 checker 1 private", checkers[1]) + assert not visible.comment("turn 3 checker 0 public", checkers[0]) + + # reject the request + bll.set_status(release_request, RequestStatus.REJECTED, checkers[0]) + release_request = factories.refresh_release_request(release_request) + # no increment, as there was no return to author + assert release_request.review_turn == 3 + assert release_request.get_turn_phase() == ReviewTurnPhase.COMPLETE + + # COMPLETE has special handling - test it works + # checkers can see all things + for checker in checkers: + visible = get_visible_items(checker) + assert visible.comment("turn 1 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert visible.comment("turn 1 checker 1 private", checkers[1]) + assert visible.vote(RequestFileVote.APPROVED, checkers[1]) + assert visible.comment("turn 1 checker 0 public", checkers[0]) + assert visible.comment("turn 2 author public", author) + assert visible.comment("turn 3 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) + assert visible.comment("turn 3 checker 1 private", checkers[1]) + assert visible.comment("turn 3 checker 0 public", checkers[0]) + + # Author should see all public comments, regardless of round, but not any private comments + visible = get_visible_items(author) + assert not visible.comment("turn 1 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.APPROVED, checkers[0]) + assert not visible.comment("turn 1 checker 1 private", checkers[1]) + assert visible.vote(RequestFileVote.APPROVED, checkers[1]) + assert visible.comment("turn 1 checker 0 public", checkers[0]) + assert visible.comment("turn 2 author public", author) + assert not visible.comment("turn 3 checker 0 private", checkers[0]) + assert visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) + assert not visible.comment("turn 3 checker 1 private", checkers[1]) + assert visible.comment("turn 3 checker 0 public", checkers[0]) diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index 1ca895be..efa2feb3 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -1,7 +1,6 @@ import hashlib import inspect import json -from dataclasses import dataclass from hashlib import file_digest from io import BytesIO from unittest.mock import MagicMock, patch @@ -14,12 +13,8 @@ from airlock import exceptions, permissions from airlock.business_logic import ( AuditEvent, - BusinessLogicLayer, CodeRepo, - Comment, DataAccessLayerProtocol, - ReleaseRequest, - RequestFileStatus, Workspace, ) from airlock.enums import ( @@ -34,7 +29,7 @@ WorkspaceFileStatus, ) from airlock.types import FileMetadata, UrlPath -from airlock.users import User +from airlock.visibility import RequestFileStatus from tests import factories @@ -2207,328 +2202,6 @@ def setup_empty_release_request(): return release_request, path, author -@dataclass -class VisibleItemsHelper: - """Helper class to make assertions about visiblity of comments and audit logs. - - It will fetch comments and audits that are visible for a specific request and - user, and store them to make assertions about. - """ - - comments: list[tuple[Comment, str]] - audits: list[AuditEvent] - - @classmethod - def for_group( - cls, request: ReleaseRequest, user: User, group, bll: BusinessLogicLayer - ): - return cls( - comments=request.get_visible_comments_for_group(group, user), - audits=bll.get_request_audit_log(user, request, group), - ) - - def is_comment_visible(self, text: str, author: User) -> bool: - for c, _ in self.comments: - if c.comment == text and c.author == author.username: - return True - - return False - - def is_audit_visible(self, type_: AuditEventType, author: User) -> bool: - for audit in self.audits: - if audit.type == type_ and audit.user == author.username: - return True - - return False - - def comment(self, text: str, author: User) -> bool: - """Is this comment in the list of visible items we have?""" - if not self.is_comment_visible(text=text, author=author): - return False - - return self.is_audit_visible( - type_=AuditEventType.REQUEST_COMMENT, author=author - ) - - def vote(self, vote: RequestFileVote, author: User) -> bool: - """Is this vote in the list of visible items we have?""" - match vote: - case RequestFileVote.APPROVED: - event_type = AuditEventType.REQUEST_FILE_APPROVE - case RequestFileVote.CHANGES_REQUESTED: - event_type = AuditEventType.REQUEST_FILE_REQUEST_CHANGES - case _: # pragma: nocover - assert False - - return self.is_audit_visible(type_=event_type, author=author) - - -def test_request_comment_and_audit_visibility(bll): - # This test is long and complex. - # - # It tests both get_visible_comments_for_group() and - # get_request_audit_log(), and uses the custom VisibleItems helper above. - # - # It walks through a couple of rounds and turns of back and forth review, - # validating that the comments that are visibile to various users at - # different points in the process are correct. - # - author = factories.create_user("author1", workspaces=["workspace"]) - checkers = factories.get_default_output_checkers() - - release_request = factories.create_request_at_status( - "workspace", - status=RequestStatus.SUBMITTED, - author=author, - files=[factories.request_file(group="group", path="file.txt", approved=True)], - ) - - bll.group_comment_create( - release_request, - "group", - "turn 1 checker 0 private", - Visibility.PRIVATE, - checkers[0], - ) - bll.group_comment_create( - release_request, - "group", - "turn 1 checker 1 private", - Visibility.PRIVATE, - checkers[1], - ) - - release_request = factories.refresh_release_request(release_request) - - assert release_request.review_turn == 1 - assert release_request.get_turn_phase() == ReviewTurnPhase.INDEPENDENT - - def get_visible_items(user): - return VisibleItemsHelper.for_group(release_request, user, "group", bll) - - # in ReviewTurnPhase.INDEPENDENT, checkers can only see own comments, author can see nothing - visible = get_visible_items(checkers[0]) - assert visible.comment("turn 1 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert not visible.comment("turn 1 checker 1 private", checkers[1]) - assert not visible.vote(RequestFileVote.APPROVED, checkers[1]) - - visible = get_visible_items(checkers[1]) - assert not visible.comment("turn 1 checker 0 private", checkers[0]) - assert not visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert visible.comment("turn 1 checker 1 private", checkers[1]) - assert visible.vote(RequestFileVote.APPROVED, checkers[1]) - - visible = get_visible_items(author) - assert not visible.comment("turn 1 checker 0 private", checkers[0]) - assert not visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert not visible.comment("turn 1 checker 1 private", checkers[1]) - assert not visible.vote(RequestFileVote.APPROVED, checkers[1]) - - factories.submit_independent_review(release_request) - bll.group_comment_create( - release_request, - "group", - "turn 1 checker 0 public", - Visibility.PUBLIC, - checkers[0], - ) - release_request = factories.refresh_release_request(release_request) - - assert release_request.review_turn == 1 - assert release_request.get_turn_phase() == ReviewTurnPhase.CONSOLIDATING - - # in ReviewTurnPhase.CONSOLIDATING, checkers should see all private comments - # and pending public comments, but author should not see any yet - for checker in checkers: - visible = get_visible_items(checker) - assert visible.comment("turn 1 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert visible.comment("turn 1 checker 1 private", checkers[1]) - assert visible.vote(RequestFileVote.APPROVED, checkers[1]) - assert visible.comment("turn 1 checker 0 public", checkers[0]) - - # author still cannot see anything - visible = get_visible_items(author) - assert not visible.comment("turn 1 checker 0 private", checkers[0]) - assert not visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert not visible.comment("turn 1 checker 1 private", checkers[1]) - assert not visible.vote(RequestFileVote.APPROVED, checkers[1]) - assert not visible.comment("turn 1 checker 0 public", checkers[0]) - - bll.return_request(release_request, checkers[0]) - release_request = factories.refresh_release_request(release_request) - bll.group_comment_create( - release_request, - "group", - "turn 2 author public", - Visibility.PUBLIC, - author, - ) - release_request = factories.refresh_release_request(release_request) - - assert release_request.review_turn == 2 - assert release_request.get_turn_phase() == ReviewTurnPhase.AUTHOR - - # in ReviewTurnPhase.AUTHOR, checkers should see turn 1 comments, but not authors turn 2 comments. - # Author should turn 1 and 2 public comments - for checker in checkers: - visible = get_visible_items(checker) - assert visible.comment("turn 1 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert visible.comment("turn 1 checker 1 private", checkers[1]) - assert visible.vote(RequestFileVote.APPROVED, checkers[1]) - assert visible.comment("turn 1 checker 0 public", checkers[0]) - assert not visible.comment("turn 2 author public", author) - - # author can see al turn 1 public comments and votes, their turn 2 public comments, but no private comments. - visible = get_visible_items(author) - assert not visible.comment("turn 1 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert not visible.comment("turn 1 checker 1 private", checkers[1]) - assert visible.vote(RequestFileVote.APPROVED, checkers[1]) - assert visible.comment("turn 1 checker 0 public", checkers[0]) - assert visible.comment("turn 2 author public", author) - - bll.submit_request(release_request, author) - release_request = factories.refresh_release_request(release_request) - assert release_request.review_turn == 3 - - bll.group_comment_create( - release_request, - "group", - "turn 3 checker 0 private", - Visibility.PRIVATE, - checkers[0], - ) - bll.group_comment_create( - release_request, - "group", - "turn 3 checker 1 private", - Visibility.PRIVATE, - checkers[1], - ) - # checker0 requests changes to the file now. Not realistic, but we want to check that - # the audit log for later round votes is hidden - bll.request_changes_to_file( - release_request, - release_request.get_request_file_from_output_path("file.txt"), - checkers[0], - ) - release_request = factories.refresh_release_request(release_request) - - # in ReviewTurnPhase.INDEPENDENT for a 2nd round - # Checkers should see previous round's private comments, but not this rounds - # Author should see previous round's public comments, but not any this round - visible = get_visible_items(checkers[0]) - assert visible.comment("turn 1 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert visible.comment("turn 1 checker 1 private", checkers[1]) - assert visible.vote(RequestFileVote.APPROVED, checkers[1]) - assert visible.comment("turn 1 checker 0 public", checkers[0]) - assert visible.comment("turn 2 author public", author) - assert visible.comment("turn 3 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) - assert not visible.comment("turn 3 checker 1 private", checkers[1]) - - visible = get_visible_items(checkers[1]) - assert visible.comment("turn 1 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert visible.comment("turn 1 checker 1 private", checkers[1]) - assert visible.vote(RequestFileVote.APPROVED, checkers[1]) - assert visible.comment("turn 1 checker 0 public", checkers[0]) - assert visible.comment("turn 2 author public", author) - assert not visible.comment("turn 3 checker 0 private", checkers[0]) - assert not visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) - assert visible.comment("turn 3 checker 1 private", checkers[1]) - - visible = get_visible_items(author) - assert not visible.comment("turn 1 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert not visible.comment("turn 1 checker 1 private", checkers[1]) - assert visible.vote(RequestFileVote.APPROVED, checkers[1]) - assert visible.comment("turn 1 checker 0 public", checkers[0]) - assert visible.comment("turn 2 author public", author) - assert not visible.comment("turn 3 checker 0 private", checkers[0]) - assert not visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) - assert not visible.comment("turn 3 checker 1 private", checkers[1]) - - factories.submit_independent_review(release_request) - release_request = factories.refresh_release_request(release_request) - bll.group_comment_create( - release_request, - "group", - "turn 3 checker 0 public", - Visibility.PUBLIC, - checkers[0], - ) - release_request = factories.refresh_release_request(release_request) - - # in ReviewTurnPhase.CONSOLIDATING for a 2nd round - # Checkers should see previous and current round's private comments, - # Author should see previous round's public comments, but not any private comments - for checker in checkers: - visible = get_visible_items(checker) - assert visible.comment("turn 1 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert visible.comment("turn 1 checker 1 private", checkers[1]) - assert visible.vote(RequestFileVote.APPROVED, checkers[1]) - assert visible.comment("turn 1 checker 0 public", checkers[0]) - assert visible.comment("turn 2 author public", author) - assert visible.comment("turn 3 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) - assert visible.comment("turn 3 checker 1 private", checkers[1]) - assert visible.comment("turn 3 checker 0 public", checkers[0]) - - # author sees no private comments, and no turn 3 things. - visible = get_visible_items(author) - assert not visible.comment("turn 1 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert not visible.comment("turn 1 checker 1 private", checkers[1]) - assert visible.vote(RequestFileVote.APPROVED, checkers[1]) - assert visible.comment("turn 1 checker 0 public", checkers[0]) - assert visible.comment("turn 2 author public", author) - assert not visible.comment("turn 3 checker 0 private", checkers[0]) - assert not visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) - assert not visible.comment("turn 3 checker 1 private", checkers[1]) - assert not visible.comment("turn 3 checker 0 public", checkers[0]) - - # reject the request - bll.set_status(release_request, RequestStatus.REJECTED, checkers[0]) - release_request = factories.refresh_release_request(release_request) - # no increment, as there was no return to author - assert release_request.review_turn == 3 - assert release_request.get_turn_phase() == ReviewTurnPhase.COMPLETE - - # COMPLETE has special handling - test it works - # checkers can see all things - for checker in checkers: - visible = get_visible_items(checker) - assert visible.comment("turn 1 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert visible.comment("turn 1 checker 1 private", checkers[1]) - assert visible.vote(RequestFileVote.APPROVED, checkers[1]) - assert visible.comment("turn 1 checker 0 public", checkers[0]) - assert visible.comment("turn 2 author public", author) - assert visible.comment("turn 3 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) - assert visible.comment("turn 3 checker 1 private", checkers[1]) - assert visible.comment("turn 3 checker 0 public", checkers[0]) - - # Author should see all public comments, regardless of round, but not any private comments - visible = get_visible_items(author) - assert not visible.comment("turn 1 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.APPROVED, checkers[0]) - assert not visible.comment("turn 1 checker 1 private", checkers[1]) - assert visible.vote(RequestFileVote.APPROVED, checkers[1]) - assert visible.comment("turn 1 checker 0 public", checkers[0]) - assert visible.comment("turn 2 author public", author) - assert not visible.comment("turn 3 checker 0 private", checkers[0]) - assert visible.vote(RequestFileVote.CHANGES_REQUESTED, checkers[0]) - assert not visible.comment("turn 3 checker 1 private", checkers[1]) - assert visible.comment("turn 3 checker 0 public", checkers[0]) - - def test_get_visible_comments_for_group_class(bll): author = factories.create_user("author", workspaces=["workspace"]) checkers = factories.get_default_output_checkers() From 55fd1cea5f337efea671d2f341b4a0e6a2927551 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Wed, 28 Aug 2024 16:13:35 +0100 Subject: [PATCH 3/4] Move AirlockContainer into file_browser_api.py AirlockContainer defines and abstraction that the file browser API can use to render and navigate through a tree like structure. The fact that AirlockContainer was defined in business_logic.py is due to some awkward import cycles. Those issues have all been fixed, so AirlockContainer can now be defined in the same module its mainly used. The Workspace and ReleaseRequest classes still need to implement it, and mypy will complain if they don't. But that is all now a concern of the file_browser_api module, and business_logic does need to care about it anymore. --- airlock/business_logic.py | 38 --------------------------------- airlock/file_browser_api.py | 42 ++++++++++++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 39 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 25899ded..76f2865c 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -156,44 +156,6 @@ def visibility(self) -> Visibility: return Visibility.PUBLIC -class AirlockContainer(Protocol): - """Structural typing class for a instance of a Workspace or ReleaseRequest - - Provides a uniform interface for accessing information about the paths and files - contained within this instance. - """ - - def get_id(self) -> str: - """Get the human name for this container.""" - - def get_url(self, relpath: UrlPath = ROOT_PATH) -> str: - """Get the url for the container object with path""" - - def get_contents_url( - self, relpath: UrlPath, download: bool = False, plaintext: bool = False - ) -> str: - """Get the url for the contents of the container object with path""" - - def request_filetype(self, relpath: UrlPath) -> RequestFileType | None: - """What kind of file is this, e.g. output, supporting, etc.""" - - def get_renderer( - self, relpath: UrlPath, plaintext: bool = False - ) -> renderers.Renderer: - """Create and return the correct renderer for this path.""" - - def get_file_metadata(self, relpath: UrlPath) -> FileMetadata | None: - """Get the file metadata""" - - def get_workspace_file_status(self, relpath: UrlPath) -> WorkspaceFileStatus | None: - """Get workspace state of file.""" - - def get_request_file_status( - self, relpath: UrlPath, user: User - ) -> RequestFileStatus | None: - """Get request status of file.""" - - @dataclass(frozen=True) class Project: name: str diff --git a/airlock/file_browser_api.py b/airlock/file_browser_api.py index b758b656..842e0185 100644 --- a/airlock/file_browser_api.py +++ b/airlock/file_browser_api.py @@ -4,10 +4,11 @@ from dataclasses import dataclass, field from datetime import UTC, datetime from pathlib import Path +from typing import Protocol +from airlock import renderers from airlock.business_logic import ( ROOT_PATH, - AirlockContainer, CodeRepo, ReleaseRequest, Workspace, @@ -20,6 +21,45 @@ from services.tracing import instrument +class AirlockContainer(Protocol): + """Structural typing class for a instance of a Workspace or ReleaseRequest + + Provides a uniform interface for the file browser to accessing information + about the paths and files contained within this container, whichever kind + it is. + """ + + def get_id(self) -> str: + """Get the human name for this container.""" + + def get_url(self, relpath: UrlPath = ROOT_PATH) -> str: + """Get the url for the container object with path""" + + def get_contents_url( + self, relpath: UrlPath, download: bool = False, plaintext: bool = False + ) -> str: + """Get the url for the contents of the container object with path""" + + def request_filetype(self, relpath: UrlPath) -> RequestFileType | None: + """What kind of file is this, e.g. output, supporting, etc.""" + + def get_renderer( + self, relpath: UrlPath, plaintext: bool = False + ) -> renderers.Renderer: + """Create and return the correct renderer for this path.""" + + def get_file_metadata(self, relpath: UrlPath) -> FileMetadata | None: + """Get the file metadata""" + + def get_workspace_file_status(self, relpath: UrlPath) -> WorkspaceFileStatus | None: + """Get workspace state of file.""" + + def get_request_file_status( + self, relpath: UrlPath, user: User + ) -> RequestFileStatus | None: + """Get request status of file.""" + + @dataclass class PathItem: """ From 3c7374fca1be57efe6c955bea0201da043545168 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Wed, 28 Aug 2024 16:58:20 +0100 Subject: [PATCH 4/4] Break out the half of business_logic.py into models.py This breaks out the Workspace, ReleaseRequest, and CodeRepo classes (and their dependant sub-objects) into a new models.py file. This is just moving code around and changing imports, there's no actualy code changes. It also splits half the even larger test_business_logic.py file into a test_models.py file to match. It moves ROOT_PATH to types.py, as that was needed by both modules, and it arguably belongs there, were UrlPath is defined. This and the preceding changes takes the currently 2200 line business_logic.py file down to 1198, with models.py clocking in at 896. Likewise, the test_business_logic.py is currently at 4008 lines, but this change splits it into two files, 2935 and 797 respectively. There may be futher changes and cleanups possible here - this commit limits itself to mechanical changes to split files. --- airlock/business_logic.py | 907 +--------------------- airlock/file_browser_api.py | 7 +- airlock/forms.py | 2 +- airlock/models.py | 896 +++++++++++++++++++++ airlock/permissions.py | 2 +- airlock/policies.py | 2 +- airlock/types.py | 3 + airlock/views/code.py | 3 +- airlock/views/request.py | 4 +- local_db/data_access.py | 6 +- tests/factories.py | 16 +- tests/integration/test_visibility.py | 12 +- tests/integration/views/test_workspace.py | 2 +- tests/local_db/test_data_access.py | 3 +- tests/unit/test_business_logic.py | 768 +----------------- tests/unit/test_models.py | 774 ++++++++++++++++++ 16 files changed, 1731 insertions(+), 1676 deletions(-) create mode 100644 airlock/models.py create mode 100644 tests/unit/test_models.py diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 76f2865c..c3865f12 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -5,919 +5,41 @@ import logging import secrets import shutil -from dataclasses import dataclass, field from datetime import datetime -from functools import cached_property from pathlib import Path -from typing import Any, Protocol, Self, cast +from typing import Protocol, cast from django.conf import settings -from django.urls import reverse -from django.utils import timezone from django.utils.functional import SimpleLazyObject from django.utils.module_loading import import_string import old_api -from airlock import exceptions, permissions, policies, renderers +from airlock import exceptions, permissions, policies from airlock.enums import ( AuditEventType, NotificationEventType, - RequestFileDecision, RequestFileType, - RequestFileVote, RequestStatus, RequestStatusOwner, - ReviewTurnPhase, Visibility, - WorkspaceFileStatus, ) -from airlock.lib.git import ( - GitError, - list_files_from_repo, - project_name_from_url, - read_file_from_repo, +from airlock.models import ( + AuditEvent, + FileReview, + ReleaseRequest, + RequestFile, + Workspace, ) from airlock.notifications import send_notification_event -from airlock.types import FileMetadata, UrlPath +from airlock.types import UrlPath from airlock.users import User from airlock.utils import is_valid_file_type -from airlock.visibility import RequestFileStatus, filter_visible_items +from airlock.visibility import filter_visible_items -ROOT_PATH = UrlPath() # empty path - logger = logging.getLogger(__name__) -READONLY_EVENTS = { - AuditEventType.WORKSPACE_FILE_VIEW, - AuditEventType.REQUEST_FILE_VIEW, - AuditEventType.REQUEST_FILE_UNDECIDED, -} - - -AUDIT_MSG_FORMATS = { - AuditEventType.WORKSPACE_FILE_VIEW: "Viewed file", - AuditEventType.REQUEST_FILE_VIEW: "Viewed file", - AuditEventType.REQUEST_FILE_DOWNLOAD: "Downloaded file", - AuditEventType.REQUEST_CREATE: "Created request", - AuditEventType.REQUEST_SUBMIT: "Submitted request", - AuditEventType.REQUEST_WITHDRAW: "Withdrew request", - AuditEventType.REQUEST_REVIEW: "Submitted review", - AuditEventType.REQUEST_APPROVE: "Approved request", - AuditEventType.REQUEST_REJECT: "Rejected request", - AuditEventType.REQUEST_RETURN: "Returned request", - AuditEventType.REQUEST_RELEASE: "Released request", - AuditEventType.REQUEST_EDIT: "Edited the Context/Controls", - AuditEventType.REQUEST_COMMENT: "Commented", - AuditEventType.REQUEST_COMMENT_DELETE: "Comment deleted", - AuditEventType.REQUEST_COMMENT_VISIBILITY_PUBLIC: "Private comment made public", - AuditEventType.REQUEST_FILE_ADD: "Added file", - AuditEventType.REQUEST_FILE_UPDATE: "Updated file", - AuditEventType.REQUEST_FILE_WITHDRAW: "Withdrew file from group", - AuditEventType.REQUEST_FILE_APPROVE: "Approved file", - AuditEventType.REQUEST_FILE_REQUEST_CHANGES: "Changes requested to file", - AuditEventType.REQUEST_FILE_RESET_REVIEW: "Reset review of file", - AuditEventType.REQUEST_FILE_UNDECIDED: "File with changes requested moved to undecided", - AuditEventType.REQUEST_FILE_RELEASE: "File released", -} - - -@dataclass -class AuditEvent: - type: AuditEventType - user: str - workspace: str | None = None - request: str | None = None - path: UrlPath | None = None - extra: dict[str, str] = field(default_factory=dict) - # this is used when querying the db for audit log times - created_at: datetime = field(default_factory=timezone.now, compare=False) - - WIDTH = max(len(k.name) for k in AuditEventType) - - @classmethod - def from_request( - cls, - request: ReleaseRequest, - type: AuditEventType, # noqa: A002 - user: User, - path: UrlPath | None = None, - **kwargs: str, - ): - # Note: kwargs go straight to extra - # set review_turn from request - kwargs["review_turn"] = str(request.review_turn) - event = cls( - type=type, - user=user.username, - workspace=request.workspace, - request=request.id, - extra=kwargs, - ) - if path: - event.path = path - - return event - - def __str__(self): - ts = self.created_at.isoformat()[:-13] # seconds precision - msg = [ - f"{ts}: {self.type.name:<{self.WIDTH}} user={self.user} workspace={self.workspace}" - ] - - if self.request: - msg.append(f"request={self.request}") - if self.path: - msg.append(f"path={self.path}") - - for k, v in self.extra.items(): - msg.append(f"{k}={v}") - - return " ".join(msg) - - def description(self): - return AUDIT_MSG_FORMATS[self.type] - - @property - def review_turn(self) -> int: - return int(self.extra.get("review_turn", 0)) - - @property - def author(self) -> str: - return self.user - - @property - def visibility(self) -> Visibility: - v = self.extra.get("visibility") - if v: - return Visibility[v.upper()] - else: - return Visibility.PUBLIC - - -@dataclass(frozen=True) -class Project: - name: str - is_ongoing: bool - - def display_name(self): - # helper for templates - if not self.is_ongoing: - return f"{self.name} (INACTIVE)" - return self.name - - -@dataclass(order=True) -class Workspace: - """Simple wrapper around a workspace directory on disk. - - Deliberately a dumb python object - the only operations are about accessing - filepaths within the workspace directory, and related urls. - """ - - name: str - manifest: dict[str, Any] - metadata: dict[str, Any] - current_request: ReleaseRequest | None - released_files: set[str] - - @classmethod - def from_directory( - cls, - name: str, - metadata: dict[str, str] | None = None, - current_request: ReleaseRequest | None = None, - released_files: set[str] | None = None, - ) -> Workspace: - root = settings.WORKSPACE_DIR / name - if not root.exists(): - raise exceptions.WorkspaceNotFound(name) - - manifest_path = root / "metadata/manifest.json" - if not manifest_path.exists(): - raise exceptions.ManifestFileError(f"{manifest_path} does not exist") - - try: - manifest = json.loads(manifest_path.read_text()) - except json.JSONDecodeError as exc: - raise exceptions.ManifestFileError( - f"Could not parse manifest.json file: {manifest_path}:\n{exc}" - ) - - if metadata is None: # pragma: no cover - metadata = {} - - return cls( - name, - manifest=manifest, - metadata=metadata, - current_request=current_request, - released_files=released_files or set(), - ) - - def __str__(self): - return self.get_id() - - def project(self) -> Project: - details = self.metadata.get("project_details", {}) - return Project( - name=details.get("name", "Unknown project"), - is_ongoing=details.get("ongoing", True), - ) - - def is_archived(self): - return self.metadata.get("archived") - - # helpers for templates - def is_active(self): - return self.project().is_ongoing and not self.is_archived() - - def display_name(self): - # helper for templates - if self.is_archived(): - return f"{self.name} (ARCHIVED)" - return self.name - - def root(self): - return settings.WORKSPACE_DIR / self.name - - def manifest_path(self): - return self.root() / "metadata/manifest.json" - - def get_id(self) -> str: - return self.name - - def get_url(self, relpath: UrlPath = ROOT_PATH) -> str: - kwargs = {"workspace_name": self.name} - if relpath != ROOT_PATH: - kwargs["path"] = str(relpath) - return reverse("workspace_view", kwargs=kwargs) - - def get_workspace_file_status(self, relpath: UrlPath) -> WorkspaceFileStatus | None: - # get_file_metadata will throw FileNotFound if we have a bad file path - metadata = self.get_file_metadata(relpath) - - # check if file has been released once we can do that - if metadata and metadata.content_hash in self.released_files: - return WorkspaceFileStatus.RELEASED - - if self.current_request: - try: - rfile = self.current_request.get_request_file_from_output_path(relpath) - except exceptions.FileNotFound: - return WorkspaceFileStatus.UNRELEASED - - if metadata is None: # pragma: no cover - raise exceptions.ManifestFileError( - f"no file metadata available for {relpath}" - ) - if rfile.filetype is RequestFileType.WITHDRAWN: - return WorkspaceFileStatus.WITHDRAWN - elif rfile.file_id == metadata.content_hash: - return WorkspaceFileStatus.UNDER_REVIEW - else: - return WorkspaceFileStatus.CONTENT_UPDATED - - return WorkspaceFileStatus.UNRELEASED - - def get_request_file_status( - self, relpath: UrlPath, user: User - ) -> RequestFileStatus | None: - return None # pragma: nocover - - def get_requests_url(self): - return reverse( - "requests_for_workspace", - kwargs={"workspace_name": self.name}, - ) - - def get_contents_url( - self, relpath: UrlPath, download: bool = False, plaintext: bool = False - ) -> str: - url = reverse( - "workspace_contents", - kwargs={"workspace_name": self.name, "path": relpath}, - ) - - renderer = self.get_renderer(relpath, plaintext=plaintext) - plaintext_param = "&plaintext=true" if plaintext else "" - url += f"?cache_id={renderer.cache_id}{plaintext_param}" - - return url - - def get_renderer( - self, relpath: UrlPath, plaintext: bool = False - ) -> renderers.Renderer: - renderer_class = renderers.get_renderer(relpath, plaintext=plaintext) - return renderer_class.from_file( - self.abspath(relpath), - relpath=relpath, - ) - - def get_manifest_for_file(self, relpath: UrlPath): - try: - return self.manifest["outputs"][str(relpath)] - except KeyError: - raise exceptions.ManifestFileError( - f"No entry for {relpath} from manifest.json file" - ) - - def get_file_metadata(self, relpath: UrlPath) -> FileMetadata | None: - """Get file metadata, i.e. size, timestamp, hash""" - try: - return FileMetadata.from_manifest(self.get_manifest_for_file(relpath)) - except exceptions.ManifestFileError: - pass - - # not in manifest, e.g. log file. Check disk - return FileMetadata.from_path(self.abspath(relpath)) - - def abspath(self, relpath): - """Get absolute path for file - - Protects against traversal, and ensures the path exists.""" - root = self.root() - path = root / relpath - - # protect against traversal - path.resolve().relative_to(root) - - # validate path exists - if not path.exists(): - raise exceptions.FileNotFound(path) - - return path - - def request_filetype(self, relpath: UrlPath) -> None: - return None - - -@dataclass(frozen=True) -class CodeRepo: - workspace: str - repo: str - name: str - commit: str - directory: Path - pathlist: list[UrlPath] - - class RepoNotFound(Exception): - pass - - class CommitNotFound(Exception): - pass - - @classmethod - def from_workspace(cls, workspace: Workspace, commit: str) -> CodeRepo: - try: - repo = list(workspace.manifest["outputs"].values())[0]["repo"] - except (exceptions.ManifestFileError, IndexError, KeyError) as exc: - raise cls.RepoNotFound( - f"Could not parse manifest.json file: {workspace.manifest_path()}:\n{exc}" - ) - - try: - pathlist = list_files_from_repo(repo, commit) - except GitError as exc: - raise CodeRepo.CommitNotFound(str(exc)) - - return cls( - workspace=workspace.name, - repo=repo, - name=project_name_from_url(repo), - commit=commit, - directory=settings.GIT_REPO_DIR / workspace.name, - pathlist=pathlist, - ) - - def get_id(self) -> str: - return f"{self.name}@{self.commit[:7]}" - - def get_url(self, relpath: UrlPath = ROOT_PATH) -> str: - kwargs = { - "workspace_name": self.name, - "commit": self.commit, - } - if relpath != ROOT_PATH: - kwargs["path"] = str(relpath) - return reverse( - "code_view", - kwargs=kwargs, - ) - - def get_file_state(self, relpath: UrlPath) -> WorkspaceFileStatus | None: - """Get state of path.""" - return None # pragma: no cover - - def get_contents_url( - self, - relpath: UrlPath = ROOT_PATH, - download: bool = False, - plaintext: bool = False, - ) -> str: - url = reverse( - "code_contents", - kwargs={ - "workspace_name": self.workspace, - "commit": self.commit, - "path": relpath, - }, - ) - - renderer = self.get_renderer(relpath, plaintext=plaintext) - plaintext_param = "&plaintext=true" if plaintext else "" - url += f"?cache_id={renderer.cache_id}{plaintext_param}" - - return url - - def get_renderer(self, relpath: UrlPath, plaintext=False) -> renderers.Renderer: - # we do not care about valid file types here, so we just get the base renderers - - try: - contents = read_file_from_repo(self.repo, self.commit, relpath) - except GitError as exc: - raise exceptions.FileNotFound(str(exc)) - - renderer_class = renderers.get_code_renderer(relpath, plaintext=plaintext) - # note: we don't actually need an explicit cache_id here, as the commit is - # already in the url. But we want to add the template version to the - # cache id, so pass an empty string. - return renderer_class.from_contents( - contents=contents, - relpath=relpath, - cache_id="", - ) - - def get_file_metadata(self, relpath: UrlPath) -> FileMetadata | None: - """Get the size of a file""" - return None # pragma: no cover - - def request_filetype(self, relpath: UrlPath) -> RequestFileType | None: - return RequestFileType.CODE - - def get_workspace_file_status(self, relpath: UrlPath) -> WorkspaceFileStatus | None: - return None - - def get_request_file_status( - self, relpath: UrlPath, user: User - ) -> RequestFileStatus | None: - return None # pragma: nocover - - -@dataclass(frozen=True) -class FileReview: - """ - Represents a review of a file in the context of a release request - """ - - reviewer: str - status: RequestFileVote - created_at: datetime - updated_at: datetime - - @classmethod - def from_dict(cls, attrs): - return cls(**attrs) - - -@dataclass(frozen=True) -class RequestFile: - """ - Represents a single file within a release request - """ - - relpath: UrlPath - group: str - file_id: str - reviews: dict[str, FileReview] - timestamp: int - size: int - job_id: str - commit: str - repo: str - released_by: str | None = None - row_count: int | None = None - col_count: int | None = None - filetype: RequestFileType = RequestFileType.OUTPUT - released_at: datetime | None = None - - @classmethod - def from_dict(cls, attrs) -> Self: - return cls( - **{k: v for k, v in attrs.items() if k != "reviews"}, - reviews={ - value["reviewer"]: FileReview.from_dict(value) - for value in attrs.get("reviews", ()) - }, - ) - - def get_decision(self, submitted_reviewers) -> RequestFileDecision: - """ - The status of RequestFile, based on multiple reviews. - - Disclosivity can only be assessed by considering all files in a release - together. Therefore an overall decision on a file is based on votes from - from submitted reviews only. - - We specificially only require 2 APPROVED votes (within submitted reviews), - rather than all votes being APPROVED, as this allows a 3rd review to mark - a file APPROVED to unblock things if one of the initial reviewers is unavailable. - """ - all_reviews = [ - v.status for v in self.reviews.values() if v.reviewer in submitted_reviewers - ] - - if len(all_reviews) < 2: - # not enough votes yet - return RequestFileDecision.INCOMPLETE - - # if we have 2+ APPROVED reviews, we are APPROVED - if all_reviews.count(RequestFileVote.APPROVED) >= 2: - return RequestFileDecision.APPROVED - - # do the reviews disagree? - if len(set(all_reviews)) > 1: - return RequestFileDecision.CONFLICTED - - # only case left is all reviews are CHANGES_REQUESTED - return RequestFileDecision.CHANGES_REQUESTED - - def get_file_vote_for_user(self, user: User) -> RequestFileVote | None: - if user.username in self.reviews: - return self.reviews[user.username].status - else: - return None - - def changes_requested_reviews(self): - return [ - review - for review in self.reviews.values() - if review.status == RequestFileVote.CHANGES_REQUESTED - ] - - -@dataclass(frozen=True) -class FileGroup: - """ - Represents a group of one or more files within a release request - """ - - name: str - files: dict[UrlPath, RequestFile] - context: str = "" - controls: str = "" - updated_at: datetime = field(default_factory=timezone.now) - comments: list[Comment] = field(default_factory=list) - - @property - def output_files(self): - return [f for f in self.files.values() if f.filetype == RequestFileType.OUTPUT] - - @property - def supporting_files(self): - return [ - f for f in self.files.values() if f.filetype == RequestFileType.SUPPORTING - ] - - @classmethod - def from_dict(cls, attrs) -> Self: - return cls( - **{k: v for k, v in attrs.items() if k not in ["files", "comments"]}, - files={ - UrlPath(value["relpath"]): RequestFile.from_dict(value) - for value in attrs.get("files", ()) - }, - comments=[Comment.from_dict(c) for c in attrs.get("comments", [])], - ) - - -@dataclass(frozen=True) -class Comment: - """A user comment on a group""" - - id: str - comment: str - author: str - created_at: datetime - visibility: Visibility - review_turn: int - - @classmethod - def from_dict(cls, attrs): - # `id` is implemented as an `int` in the current DAL, and as a `str` - # in the BLL, so we need to add a conversion here (instead of just passing - # it straight through with the other `attrs`) - return cls( - **{k: v for k, v in attrs.items() if k not in ["id"]}, - id=str(attrs["id"]), - ) - - -@dataclass -class ReleaseRequest: - """Represents a release request made by a user. - - Deliberately a dumb python object. Does not operate on the state of request, - except for the request directory on disk and the files stored within. - - All other state is provided at instantiation by the provider. - """ - - id: str - workspace: str - author: str - created_at: datetime - status: RequestStatus = RequestStatus.PENDING - filegroups: dict[str, FileGroup] = field(default_factory=dict) - submitted_reviews: dict[str, str] = field(default_factory=dict) - turn_reviewers: set[str] = field(default_factory=set) - review_turn: int = 0 - - @classmethod - def from_dict(cls, attrs) -> Self: - return cls( - **{k: v for k, v in attrs.items() if k != "filegroups"}, - filegroups=cls._filegroups_from_dict(attrs.get("filegroups", {})), - ) - - @staticmethod - def _filegroups_from_dict(attrs): - return {key: FileGroup.from_dict(value) for key, value in attrs.items()} - - def __post_init__(self): - self.root().mkdir(parents=True, exist_ok=True) - - def __str__(self): - return self.get_id() - - def root(self): - return settings.REQUEST_DIR / self.workspace / self.id - - def get_id(self): - return self.id - - def get_url(self, relpath=""): - return reverse( - "request_view", - kwargs={ - "request_id": self.id, - "path": relpath, - }, - ) - - def get_contents_url( - self, relpath: UrlPath, download: bool = False, plaintext: bool = False - ): - url = reverse( - "request_contents", - kwargs={"request_id": self.id, "path": relpath}, - ) - if download: - url += "?download" - else: - # what renderer would render this file? - renderer = self.get_renderer(relpath, plaintext=plaintext) - plaintext_param = "&plaintext=true" if plaintext else "" - url += f"?cache_id={renderer.cache_id}{plaintext_param}" - - return url - - def get_renderer( - self, relpath: UrlPath, plaintext: bool = False - ) -> renderers.Renderer: - request_file = self.get_request_file_from_urlpath(relpath) - renderer_class = renderers.get_renderer(relpath, plaintext=plaintext) - return renderer_class.from_file( - self.abspath(relpath), - relpath=request_file.relpath, - cache_id=request_file.file_id, - ) - - def get_file_metadata(self, relpath: UrlPath) -> FileMetadata | None: - rfile = self.get_request_file_from_urlpath(relpath) - return FileMetadata( - rfile.size, - rfile.timestamp, - _content_hash=rfile.file_id, - ) - - def get_workspace_file_status(self, relpath: UrlPath) -> WorkspaceFileStatus | None: - return None - - def get_request_file_status( - self, relpath: UrlPath, user: User - ) -> RequestFileStatus | None: - rfile = self.get_request_file_from_urlpath(relpath) - phase = self.get_turn_phase() - decision = RequestFileDecision.INCOMPLETE - can_review = permissions.user_can_review_request(user, self) - submitted_reviewers_this_turn = self.submitted_reviews.keys() - - # If we're in the AUTHOR phase of a turn (i.e. the request is being - # edited by the author, it's not in an under-review status), we need - # to show the decision from the previous turn, if there is one. For - # all other (reviewing) phases, we show the current decision based on - # the submitted reviews in this turn. - match phase: - case ReviewTurnPhase.INDEPENDENT: - # already set - no one knows the current status - pass - case ReviewTurnPhase.CONSOLIDATING: - # only users who can review this request know the current status - if can_review: - decision = rfile.get_decision(submitted_reviewers_this_turn) - case ReviewTurnPhase.COMPLETE: - # everyone knows the current status - decision = rfile.get_decision(submitted_reviewers_this_turn) - case ReviewTurnPhase.AUTHOR: - # everyone can the status at the end of the previous turn - decision = rfile.get_decision(self.turn_reviewers) - case _: # pragma: nocover - assert False - - return RequestFileStatus( - user=user, - decision=decision, - vote=rfile.get_file_vote_for_user(user), - ) - - def get_visible_comments_for_group( - self, group: str, user: User - ) -> list[tuple[Comment, str]]: - filegroup = self.filegroups[group] - current_phase = self.get_turn_phase() - - comments = [] - visible_comments = filter_visible_items( - filegroup.comments, - self.review_turn, - current_phase, - permissions.user_can_review_request(user, self), - user, - ) - - for comment in visible_comments: - # does this comment need to be blinded? - if ( - comment.review_turn == self.review_turn - and current_phase == ReviewTurnPhase.INDEPENDENT - ): - html_class = "comment_blinded" - else: - html_class = f"comment_{comment.visibility.name.lower()}" - - comments.append((comment, html_class)) - - return comments - - def get_request_file_from_urlpath(self, relpath: UrlPath | str) -> RequestFile: - """Get the request file from the url, which includes the group.""" - relpath = UrlPath(relpath) - group = relpath.parts[0] - file_relpath = UrlPath(*relpath.parts[1:]) - - if not (filegroup := self.filegroups.get(group)): - raise exceptions.FileNotFound(f"bad group {group} in url {relpath}") - - if not (request_file := filegroup.files.get(file_relpath)): - raise exceptions.FileNotFound(relpath) - - return request_file - - def get_request_file_from_output_path(self, relpath: UrlPath | str): - """Get the request file from the output path, which does not include the group""" - relpath = UrlPath(relpath) - if relpath in self.all_files_by_name: - return self.all_files_by_name[relpath] - - raise exceptions.FileNotFound(relpath) - - def get_turn_phase(self) -> ReviewTurnPhase: - if self.status in [RequestStatus.PENDING, RequestStatus.RETURNED]: - return ReviewTurnPhase.AUTHOR - - if self.status in [RequestStatus.SUBMITTED, RequestStatus.PARTIALLY_REVIEWED]: - return ReviewTurnPhase.INDEPENDENT - - if self.status in [RequestStatus.REVIEWED]: - return ReviewTurnPhase.CONSOLIDATING - - return ReviewTurnPhase.COMPLETE - - def get_writable_comment_visibilities_for_user( - self, user: User - ) -> list[Visibility]: - """What comment visibilities should this user be able to write for this request?""" - is_author = user.username == self.author - - # author can only ever create public comments - if is_author: - return [Visibility.PUBLIC] - - # non-author non-output-checker, also only public - if not user.output_checker: - return [Visibility.PUBLIC] - - # all other cases - the output-checker can choose to write public or private comments - return [Visibility.PRIVATE, Visibility.PUBLIC] - - def abspath(self, relpath): - """Returns abspath to the file on disk. - - The first part of the relpath is the group, so we parse and validate that first. - """ - request_file = self.get_request_file_from_urlpath(relpath) - return self.root() / request_file.file_id - - @cached_property - def all_files_by_name(self) -> dict[UrlPath, RequestFile]: - """Return the relpaths for all files on the request, of any filetype""" - return { - request_file.relpath: request_file - for filegroup in self.filegroups.values() - for request_file in filegroup.files.values() - } - - def output_files(self) -> dict[UrlPath, RequestFile]: - """Return the relpaths for output files on the request""" - return { - rfile.relpath: rfile - for rfile in self.all_files_by_name.values() - if rfile.filetype == RequestFileType.OUTPUT - } - - def supporting_files_count(self): - return len( - [ - 1 - for rfile in self.all_files_by_name.values() - if rfile.filetype == RequestFileType.SUPPORTING - ] - ) - - def request_filetype(self, urlpath: UrlPath): - try: - return self.get_request_file_from_urlpath(urlpath).filetype - except exceptions.FileNotFound: - # this includes the case when urlpath is an output directory - # e.g. `foo` when the request contains `foo/bar.txt` - return None - - def set_filegroups_from_dict(self, attrs): - self.filegroups = self._filegroups_from_dict(attrs) - - def get_output_file_paths(self): - paths = [] - for file_group in self.filegroups.values(): - for request_file in file_group.output_files: - relpath = request_file.relpath - abspath = self.abspath(file_group.name / relpath) - paths.append((relpath, abspath)) - return paths - - def all_files_approved(self): - return all( - request_file.get_decision(self.submitted_reviews.keys()) - == RequestFileDecision.APPROVED - for request_file in self.output_files().values() - ) - - def files_reviewed_by_reviewer_count(self, reviewer: User) -> int: - return sum( - 1 - for rfile in self.output_files().values() - if rfile.get_file_vote_for_user(reviewer) - not in [None, RequestFileVote.UNDECIDED] - ) - - def all_files_reviewed_by_reviewer(self, reviewer: User) -> bool: - return self.files_reviewed_by_reviewer_count(reviewer) == len( - self.output_files() - ) - - def submitted_reviews_count(self): - return len(self.submitted_reviews) - - def status_owner(self) -> RequestStatusOwner: - return permissions.STATUS_OWNERS[self.status] - - def can_be_released(self) -> bool: - return ( - self.status in [RequestStatus.REVIEWED, RequestStatus.APPROVED] - and self.all_files_approved() - ) - - def is_final(self): - return self.status_owner() == RequestStatusOwner.SYSTEM - - def is_under_review(self): - return self.status_owner() == RequestStatusOwner.REVIEWER - - def is_editing(self): - return self.status_owner() == RequestStatusOwner.AUTHOR - - def store_file(release_request: ReleaseRequest, abspath: Path) -> str: # Make a "staging" copy of the file under a temporary name so we know it can't be # modified underneath us @@ -1940,6 +1062,13 @@ def group_comment_visibility_public( release_request.id, group, comment_id, user.username, audit ) + # can filter out these audit events + READONLY_EVENTS = { + AuditEventType.WORKSPACE_FILE_VIEW, + AuditEventType.REQUEST_FILE_VIEW, + AuditEventType.REQUEST_FILE_UNDECIDED, + } + def get_request_audit_log( self, user: User, @@ -1953,7 +1082,7 @@ def get_request_audit_log( audits = self._dal.get_audit_log( request=request.id, group=group, - exclude=READONLY_EVENTS if exclude_readonly else set(), + exclude=self.READONLY_EVENTS if exclude_readonly else set(), size=size, ) diff --git a/airlock/file_browser_api.py b/airlock/file_browser_api.py index 842e0185..4a37e34c 100644 --- a/airlock/file_browser_api.py +++ b/airlock/file_browser_api.py @@ -7,14 +7,13 @@ from typing import Protocol from airlock import renderers -from airlock.business_logic import ( - ROOT_PATH, +from airlock.enums import PathType, RequestFileType, WorkspaceFileStatus +from airlock.models import ( CodeRepo, ReleaseRequest, Workspace, ) -from airlock.enums import PathType, RequestFileType, WorkspaceFileStatus -from airlock.types import FileMetadata, UrlPath +from airlock.types import ROOT_PATH, FileMetadata, UrlPath from airlock.users import User from airlock.utils import is_valid_file_type from airlock.visibility import RequestFileStatus diff --git a/airlock/forms.py b/airlock/forms.py index 7de53748..93f00fc1 100644 --- a/airlock/forms.py +++ b/airlock/forms.py @@ -1,8 +1,8 @@ from django import forms from django.forms.formsets import BaseFormSet, formset_factory -from airlock.business_logic import FileGroup from airlock.enums import RequestFileType, Visibility +from airlock.models import FileGroup class ListField(forms.Field): diff --git a/airlock/models.py b/airlock/models.py new file mode 100644 index 00000000..13cc0381 --- /dev/null +++ b/airlock/models.py @@ -0,0 +1,896 @@ +from __future__ import annotations + +import json +from dataclasses import dataclass, field +from datetime import datetime +from functools import cached_property +from pathlib import Path +from typing import Any, Self + +from django.conf import settings +from django.urls import reverse +from django.utils import timezone + +from airlock import exceptions, permissions, renderers +from airlock.enums import ( + AuditEventType, + RequestFileDecision, + RequestFileType, + RequestFileVote, + RequestStatus, + RequestStatusOwner, + ReviewTurnPhase, + Visibility, + WorkspaceFileStatus, +) +from airlock.lib.git import ( + GitError, + list_files_from_repo, + project_name_from_url, + read_file_from_repo, +) +from airlock.types import ROOT_PATH, FileMetadata, UrlPath +from airlock.users import User +from airlock.visibility import RequestFileStatus, filter_visible_items + + +AUDIT_MSG_FORMATS = { + AuditEventType.WORKSPACE_FILE_VIEW: "Viewed file", + AuditEventType.REQUEST_FILE_VIEW: "Viewed file", + AuditEventType.REQUEST_FILE_DOWNLOAD: "Downloaded file", + AuditEventType.REQUEST_CREATE: "Created request", + AuditEventType.REQUEST_SUBMIT: "Submitted request", + AuditEventType.REQUEST_WITHDRAW: "Withdrew request", + AuditEventType.REQUEST_REVIEW: "Submitted review", + AuditEventType.REQUEST_APPROVE: "Approved request", + AuditEventType.REQUEST_REJECT: "Rejected request", + AuditEventType.REQUEST_RETURN: "Returned request", + AuditEventType.REQUEST_RELEASE: "Released request", + AuditEventType.REQUEST_EDIT: "Edited the Context/Controls", + AuditEventType.REQUEST_COMMENT: "Commented", + AuditEventType.REQUEST_COMMENT_DELETE: "Comment deleted", + AuditEventType.REQUEST_COMMENT_VISIBILITY_PUBLIC: "Private comment made public", + AuditEventType.REQUEST_FILE_ADD: "Added file", + AuditEventType.REQUEST_FILE_UPDATE: "Updated file", + AuditEventType.REQUEST_FILE_WITHDRAW: "Withdrew file from group", + AuditEventType.REQUEST_FILE_APPROVE: "Approved file", + AuditEventType.REQUEST_FILE_REQUEST_CHANGES: "Changes requested to file", + AuditEventType.REQUEST_FILE_RESET_REVIEW: "Reset review of file", + AuditEventType.REQUEST_FILE_UNDECIDED: "File with changes requested moved to undecided", + AuditEventType.REQUEST_FILE_RELEASE: "File released", +} + + +@dataclass +class AuditEvent: + type: AuditEventType + user: str + workspace: str | None = None + request: str | None = None + path: UrlPath | None = None + extra: dict[str, str] = field(default_factory=dict) + # this is used when querying the db for audit log times + created_at: datetime = field(default_factory=timezone.now, compare=False) + + WIDTH = max(len(k.name) for k in AuditEventType) + + @classmethod + def from_request( + cls, + request: ReleaseRequest, + type: AuditEventType, # noqa: A002 + user: User, + path: UrlPath | None = None, + **kwargs: str, + ): + # Note: kwargs go straight to extra + # set review_turn from request + kwargs["review_turn"] = str(request.review_turn) + event = cls( + type=type, + user=user.username, + workspace=request.workspace, + request=request.id, + extra=kwargs, + ) + if path: + event.path = path + + return event + + def __str__(self): + ts = self.created_at.isoformat()[:-13] # seconds precision + msg = [ + f"{ts}: {self.type.name:<{self.WIDTH}} user={self.user} workspace={self.workspace}" + ] + + if self.request: + msg.append(f"request={self.request}") + if self.path: + msg.append(f"path={self.path}") + + for k, v in self.extra.items(): + msg.append(f"{k}={v}") + + return " ".join(msg) + + def description(self): + return AUDIT_MSG_FORMATS[self.type] + + @property + def review_turn(self) -> int: + return int(self.extra.get("review_turn", 0)) + + @property + def author(self) -> str: + return self.user + + @property + def visibility(self) -> Visibility: + v = self.extra.get("visibility") + if v: + return Visibility[v.upper()] + else: + return Visibility.PUBLIC + + +@dataclass(frozen=True) +class Project: + name: str + is_ongoing: bool + + def display_name(self): + # helper for templates + if not self.is_ongoing: + return f"{self.name} (INACTIVE)" + return self.name + + +@dataclass(order=True) +class Workspace: + """Simple wrapper around a workspace directory on disk. + + Deliberately a dumb python object - the only operations are about accessing + filepaths within the workspace directory, and related urls. + """ + + name: str + manifest: dict[str, Any] + metadata: dict[str, Any] + current_request: ReleaseRequest | None + released_files: set[str] + + @classmethod + def from_directory( + cls, + name: str, + metadata: dict[str, str] | None = None, + current_request: ReleaseRequest | None = None, + released_files: set[str] | None = None, + ) -> Workspace: + root = settings.WORKSPACE_DIR / name + if not root.exists(): + raise exceptions.WorkspaceNotFound(name) + + manifest_path = root / "metadata/manifest.json" + if not manifest_path.exists(): + raise exceptions.ManifestFileError(f"{manifest_path} does not exist") + + try: + manifest = json.loads(manifest_path.read_text()) + except json.JSONDecodeError as exc: + raise exceptions.ManifestFileError( + f"Could not parse manifest.json file: {manifest_path}:\n{exc}" + ) + + if metadata is None: # pragma: no cover + metadata = {} + + return cls( + name, + manifest=manifest, + metadata=metadata, + current_request=current_request, + released_files=released_files or set(), + ) + + def __str__(self): + return self.get_id() + + def project(self) -> Project: + details = self.metadata.get("project_details", {}) + return Project( + name=details.get("name", "Unknown project"), + is_ongoing=details.get("ongoing", True), + ) + + def is_archived(self): + return self.metadata.get("archived") + + # helpers for templates + def is_active(self): + return self.project().is_ongoing and not self.is_archived() + + def display_name(self): + # helper for templates + if self.is_archived(): + return f"{self.name} (ARCHIVED)" + return self.name + + def root(self): + return settings.WORKSPACE_DIR / self.name + + def manifest_path(self): + return self.root() / "metadata/manifest.json" + + def get_id(self) -> str: + return self.name + + def get_url(self, relpath: UrlPath = ROOT_PATH) -> str: + kwargs = {"workspace_name": self.name} + if relpath != ROOT_PATH: + kwargs["path"] = str(relpath) + return reverse("workspace_view", kwargs=kwargs) + + def get_workspace_file_status(self, relpath: UrlPath) -> WorkspaceFileStatus | None: + # get_file_metadata will throw FileNotFound if we have a bad file path + metadata = self.get_file_metadata(relpath) + + # check if file has been released once we can do that + if metadata and metadata.content_hash in self.released_files: + return WorkspaceFileStatus.RELEASED + + if self.current_request: + try: + rfile = self.current_request.get_request_file_from_output_path(relpath) + except exceptions.FileNotFound: + return WorkspaceFileStatus.UNRELEASED + + if metadata is None: # pragma: no cover + raise exceptions.ManifestFileError( + f"no file metadata available for {relpath}" + ) + if rfile.filetype is RequestFileType.WITHDRAWN: + return WorkspaceFileStatus.WITHDRAWN + elif rfile.file_id == metadata.content_hash: + return WorkspaceFileStatus.UNDER_REVIEW + else: + return WorkspaceFileStatus.CONTENT_UPDATED + + return WorkspaceFileStatus.UNRELEASED + + def get_request_file_status( + self, relpath: UrlPath, user: User + ) -> RequestFileStatus | None: + return None # pragma: nocover + + def get_requests_url(self): + return reverse( + "requests_for_workspace", + kwargs={"workspace_name": self.name}, + ) + + def get_contents_url( + self, relpath: UrlPath, download: bool = False, plaintext: bool = False + ) -> str: + url = reverse( + "workspace_contents", + kwargs={"workspace_name": self.name, "path": relpath}, + ) + + renderer = self.get_renderer(relpath, plaintext=plaintext) + plaintext_param = "&plaintext=true" if plaintext else "" + url += f"?cache_id={renderer.cache_id}{plaintext_param}" + + return url + + def get_renderer( + self, relpath: UrlPath, plaintext: bool = False + ) -> renderers.Renderer: + renderer_class = renderers.get_renderer(relpath, plaintext=plaintext) + return renderer_class.from_file( + self.abspath(relpath), + relpath=relpath, + ) + + def get_manifest_for_file(self, relpath: UrlPath): + try: + return self.manifest["outputs"][str(relpath)] + except KeyError: + raise exceptions.ManifestFileError( + f"No entry for {relpath} from manifest.json file" + ) + + def get_file_metadata(self, relpath: UrlPath) -> FileMetadata | None: + """Get file metadata, i.e. size, timestamp, hash""" + try: + return FileMetadata.from_manifest(self.get_manifest_for_file(relpath)) + except exceptions.ManifestFileError: + pass + + # not in manifest, e.g. log file. Check disk + return FileMetadata.from_path(self.abspath(relpath)) + + def abspath(self, relpath): + """Get absolute path for file + + Protects against traversal, and ensures the path exists.""" + root = self.root() + path = root / relpath + + # protect against traversal + path.resolve().relative_to(root) + + # validate path exists + if not path.exists(): + raise exceptions.FileNotFound(path) + + return path + + def request_filetype(self, relpath: UrlPath) -> None: + return None + + +@dataclass(frozen=True) +class CodeRepo: + workspace: str + repo: str + name: str + commit: str + directory: Path + pathlist: list[UrlPath] + + class RepoNotFound(Exception): + pass + + class CommitNotFound(Exception): + pass + + @classmethod + def from_workspace(cls, workspace: Workspace, commit: str) -> CodeRepo: + try: + repo = list(workspace.manifest["outputs"].values())[0]["repo"] + except (exceptions.ManifestFileError, IndexError, KeyError) as exc: + raise cls.RepoNotFound( + f"Could not parse manifest.json file: {workspace.manifest_path()}:\n{exc}" + ) + + try: + pathlist = list_files_from_repo(repo, commit) + except GitError as exc: + raise CodeRepo.CommitNotFound(str(exc)) + + return cls( + workspace=workspace.name, + repo=repo, + name=project_name_from_url(repo), + commit=commit, + directory=settings.GIT_REPO_DIR / workspace.name, + pathlist=pathlist, + ) + + def get_id(self) -> str: + return f"{self.name}@{self.commit[:7]}" + + def get_url(self, relpath: UrlPath = ROOT_PATH) -> str: + kwargs = { + "workspace_name": self.name, + "commit": self.commit, + } + if relpath != ROOT_PATH: + kwargs["path"] = str(relpath) + return reverse( + "code_view", + kwargs=kwargs, + ) + + def get_file_state(self, relpath: UrlPath) -> WorkspaceFileStatus | None: + """Get state of path.""" + return None # pragma: no cover + + def get_contents_url( + self, + relpath: UrlPath = ROOT_PATH, + download: bool = False, + plaintext: bool = False, + ) -> str: + url = reverse( + "code_contents", + kwargs={ + "workspace_name": self.workspace, + "commit": self.commit, + "path": relpath, + }, + ) + + renderer = self.get_renderer(relpath, plaintext=plaintext) + plaintext_param = "&plaintext=true" if plaintext else "" + url += f"?cache_id={renderer.cache_id}{plaintext_param}" + + return url + + def get_renderer(self, relpath: UrlPath, plaintext=False) -> renderers.Renderer: + # we do not care about valid file types here, so we just get the base renderers + + try: + contents = read_file_from_repo(self.repo, self.commit, relpath) + except GitError as exc: + raise exceptions.FileNotFound(str(exc)) + + renderer_class = renderers.get_code_renderer(relpath, plaintext=plaintext) + # note: we don't actually need an explicit cache_id here, as the commit is + # already in the url. But we want to add the template version to the + # cache id, so pass an empty string. + return renderer_class.from_contents( + contents=contents, + relpath=relpath, + cache_id="", + ) + + def get_file_metadata(self, relpath: UrlPath) -> FileMetadata | None: + """Get the size of a file""" + return None # pragma: no cover + + def request_filetype(self, relpath: UrlPath) -> RequestFileType | None: + return RequestFileType.CODE + + def get_workspace_file_status(self, relpath: UrlPath) -> WorkspaceFileStatus | None: + return None + + def get_request_file_status( + self, relpath: UrlPath, user: User + ) -> RequestFileStatus | None: + return None # pragma: nocover + + +@dataclass(frozen=True) +class FileReview: + """ + Represents a review of a file in the context of a release request + """ + + reviewer: str + status: RequestFileVote + created_at: datetime + updated_at: datetime + + @classmethod + def from_dict(cls, attrs): + return cls(**attrs) + + +@dataclass(frozen=True) +class RequestFile: + """ + Represents a single file within a release request + """ + + relpath: UrlPath + group: str + file_id: str + reviews: dict[str, FileReview] + timestamp: int + size: int + job_id: str + commit: str + repo: str + released_by: str | None = None + row_count: int | None = None + col_count: int | None = None + filetype: RequestFileType = RequestFileType.OUTPUT + released_at: datetime | None = None + + @classmethod + def from_dict(cls, attrs) -> Self: + return cls( + **{k: v for k, v in attrs.items() if k != "reviews"}, + reviews={ + value["reviewer"]: FileReview.from_dict(value) + for value in attrs.get("reviews", ()) + }, + ) + + def get_decision(self, submitted_reviewers) -> RequestFileDecision: + """ + The status of RequestFile, based on multiple reviews. + + Disclosivity can only be assessed by considering all files in a release + together. Therefore an overall decision on a file is based on votes from + from submitted reviews only. + + We specificially only require 2 APPROVED votes (within submitted reviews), + rather than all votes being APPROVED, as this allows a 3rd review to mark + a file APPROVED to unblock things if one of the initial reviewers is unavailable. + """ + all_reviews = [ + v.status for v in self.reviews.values() if v.reviewer in submitted_reviewers + ] + + if len(all_reviews) < 2: + # not enough votes yet + return RequestFileDecision.INCOMPLETE + + # if we have 2+ APPROVED reviews, we are APPROVED + if all_reviews.count(RequestFileVote.APPROVED) >= 2: + return RequestFileDecision.APPROVED + + # do the reviews disagree? + if len(set(all_reviews)) > 1: + return RequestFileDecision.CONFLICTED + + # only case left is all reviews are CHANGES_REQUESTED + return RequestFileDecision.CHANGES_REQUESTED + + def get_file_vote_for_user(self, user: User) -> RequestFileVote | None: + if user.username in self.reviews: + return self.reviews[user.username].status + else: + return None + + def changes_requested_reviews(self): + return [ + review + for review in self.reviews.values() + if review.status == RequestFileVote.CHANGES_REQUESTED + ] + + +@dataclass(frozen=True) +class FileGroup: + """ + Represents a group of one or more files within a release request + """ + + name: str + files: dict[UrlPath, RequestFile] + context: str = "" + controls: str = "" + updated_at: datetime = field(default_factory=timezone.now) + comments: list[Comment] = field(default_factory=list) + + @property + def output_files(self): + return [f for f in self.files.values() if f.filetype == RequestFileType.OUTPUT] + + @property + def supporting_files(self): + return [ + f for f in self.files.values() if f.filetype == RequestFileType.SUPPORTING + ] + + @classmethod + def from_dict(cls, attrs) -> Self: + return cls( + **{k: v for k, v in attrs.items() if k not in ["files", "comments"]}, + files={ + UrlPath(value["relpath"]): RequestFile.from_dict(value) + for value in attrs.get("files", ()) + }, + comments=[Comment.from_dict(c) for c in attrs.get("comments", [])], + ) + + +@dataclass(frozen=True) +class Comment: + """A user comment on a group""" + + id: str + comment: str + author: str + created_at: datetime + visibility: Visibility + review_turn: int + + @classmethod + def from_dict(cls, attrs): + # `id` is implemented as an `int` in the current DAL, and as a `str` + # in the BLL, so we need to add a conversion here (instead of just passing + # it straight through with the other `attrs`) + return cls( + **{k: v for k, v in attrs.items() if k not in ["id"]}, + id=str(attrs["id"]), + ) + + +@dataclass +class ReleaseRequest: + """Represents a release request made by a user. + + Deliberately a dumb python object. Does not operate on the state of request, + except for the request directory on disk and the files stored within. + + All other state is provided at instantiation by the provider. + """ + + id: str + workspace: str + author: str + created_at: datetime + status: RequestStatus = RequestStatus.PENDING + filegroups: dict[str, FileGroup] = field(default_factory=dict) + submitted_reviews: dict[str, str] = field(default_factory=dict) + turn_reviewers: set[str] = field(default_factory=set) + review_turn: int = 0 + + @classmethod + def from_dict(cls, attrs) -> Self: + return cls( + **{k: v for k, v in attrs.items() if k != "filegroups"}, + filegroups=cls._filegroups_from_dict(attrs.get("filegroups", {})), + ) + + @staticmethod + def _filegroups_from_dict(attrs): + return {key: FileGroup.from_dict(value) for key, value in attrs.items()} + + def __post_init__(self): + self.root().mkdir(parents=True, exist_ok=True) + + def __str__(self): + return self.get_id() + + def root(self): + return settings.REQUEST_DIR / self.workspace / self.id + + def get_id(self): + return self.id + + def get_url(self, relpath=""): + return reverse( + "request_view", + kwargs={ + "request_id": self.id, + "path": relpath, + }, + ) + + def get_contents_url( + self, relpath: UrlPath, download: bool = False, plaintext: bool = False + ): + url = reverse( + "request_contents", + kwargs={"request_id": self.id, "path": relpath}, + ) + if download: + url += "?download" + else: + # what renderer would render this file? + renderer = self.get_renderer(relpath, plaintext=plaintext) + plaintext_param = "&plaintext=true" if plaintext else "" + url += f"?cache_id={renderer.cache_id}{plaintext_param}" + + return url + + def get_renderer( + self, relpath: UrlPath, plaintext: bool = False + ) -> renderers.Renderer: + request_file = self.get_request_file_from_urlpath(relpath) + renderer_class = renderers.get_renderer(relpath, plaintext=plaintext) + return renderer_class.from_file( + self.abspath(relpath), + relpath=request_file.relpath, + cache_id=request_file.file_id, + ) + + def get_file_metadata(self, relpath: UrlPath) -> FileMetadata | None: + rfile = self.get_request_file_from_urlpath(relpath) + return FileMetadata( + rfile.size, + rfile.timestamp, + _content_hash=rfile.file_id, + ) + + def get_workspace_file_status(self, relpath: UrlPath) -> WorkspaceFileStatus | None: + return None + + def get_request_file_status( + self, relpath: UrlPath, user: User + ) -> RequestFileStatus | None: + rfile = self.get_request_file_from_urlpath(relpath) + phase = self.get_turn_phase() + decision = RequestFileDecision.INCOMPLETE + can_review = permissions.user_can_review_request(user, self) + submitted_reviewers_this_turn = self.submitted_reviews.keys() + + # If we're in the AUTHOR phase of a turn (i.e. the request is being + # edited by the author, it's not in an under-review status), we need + # to show the decision from the previous turn, if there is one. For + # all other (reviewing) phases, we show the current decision based on + # the submitted reviews in this turn. + match phase: + case ReviewTurnPhase.INDEPENDENT: + # already set - no one knows the current status + pass + case ReviewTurnPhase.CONSOLIDATING: + # only users who can review this request know the current status + if can_review: + decision = rfile.get_decision(submitted_reviewers_this_turn) + case ReviewTurnPhase.COMPLETE: + # everyone knows the current status + decision = rfile.get_decision(submitted_reviewers_this_turn) + case ReviewTurnPhase.AUTHOR: + # everyone can the status at the end of the previous turn + decision = rfile.get_decision(self.turn_reviewers) + case _: # pragma: nocover + assert False + + return RequestFileStatus( + user=user, + decision=decision, + vote=rfile.get_file_vote_for_user(user), + ) + + def get_visible_comments_for_group( + self, group: str, user: User + ) -> list[tuple[Comment, str]]: + filegroup = self.filegroups[group] + current_phase = self.get_turn_phase() + + comments = [] + visible_comments = filter_visible_items( + filegroup.comments, + self.review_turn, + current_phase, + permissions.user_can_review_request(user, self), + user, + ) + + for comment in visible_comments: + # does this comment need to be blinded? + if ( + comment.review_turn == self.review_turn + and current_phase == ReviewTurnPhase.INDEPENDENT + ): + html_class = "comment_blinded" + else: + html_class = f"comment_{comment.visibility.name.lower()}" + + comments.append((comment, html_class)) + + return comments + + def get_request_file_from_urlpath(self, relpath: UrlPath | str) -> RequestFile: + """Get the request file from the url, which includes the group.""" + relpath = UrlPath(relpath) + group = relpath.parts[0] + file_relpath = UrlPath(*relpath.parts[1:]) + + if not (filegroup := self.filegroups.get(group)): + raise exceptions.FileNotFound(f"bad group {group} in url {relpath}") + + if not (request_file := filegroup.files.get(file_relpath)): + raise exceptions.FileNotFound(relpath) + + return request_file + + def get_request_file_from_output_path(self, relpath: UrlPath | str): + """Get the request file from the output path, which does not include the group""" + relpath = UrlPath(relpath) + if relpath in self.all_files_by_name: + return self.all_files_by_name[relpath] + + raise exceptions.FileNotFound(relpath) + + def get_turn_phase(self) -> ReviewTurnPhase: + if self.status in [RequestStatus.PENDING, RequestStatus.RETURNED]: + return ReviewTurnPhase.AUTHOR + + if self.status in [RequestStatus.SUBMITTED, RequestStatus.PARTIALLY_REVIEWED]: + return ReviewTurnPhase.INDEPENDENT + + if self.status in [RequestStatus.REVIEWED]: + return ReviewTurnPhase.CONSOLIDATING + + return ReviewTurnPhase.COMPLETE + + def get_writable_comment_visibilities_for_user( + self, user: User + ) -> list[Visibility]: + """What comment visibilities should this user be able to write for this request?""" + is_author = user.username == self.author + + # author can only ever create public comments + if is_author: + return [Visibility.PUBLIC] + + # non-author non-output-checker, also only public + if not user.output_checker: + return [Visibility.PUBLIC] + + # all other cases - the output-checker can choose to write public or private comments + return [Visibility.PRIVATE, Visibility.PUBLIC] + + def abspath(self, relpath): + """Returns abspath to the file on disk. + + The first part of the relpath is the group, so we parse and validate that first. + """ + request_file = self.get_request_file_from_urlpath(relpath) + return self.root() / request_file.file_id + + @cached_property + def all_files_by_name(self) -> dict[UrlPath, RequestFile]: + """Return the relpaths for all files on the request, of any filetype""" + return { + request_file.relpath: request_file + for filegroup in self.filegroups.values() + for request_file in filegroup.files.values() + } + + def output_files(self) -> dict[UrlPath, RequestFile]: + """Return the relpaths for output files on the request""" + return { + rfile.relpath: rfile + for rfile in self.all_files_by_name.values() + if rfile.filetype == RequestFileType.OUTPUT + } + + def supporting_files_count(self): + return len( + [ + 1 + for rfile in self.all_files_by_name.values() + if rfile.filetype == RequestFileType.SUPPORTING + ] + ) + + def request_filetype(self, urlpath: UrlPath): + try: + return self.get_request_file_from_urlpath(urlpath).filetype + except exceptions.FileNotFound: + # this includes the case when urlpath is an output directory + # e.g. `foo` when the request contains `foo/bar.txt` + return None + + def set_filegroups_from_dict(self, attrs): + self.filegroups = self._filegroups_from_dict(attrs) + + def get_output_file_paths(self): + paths = [] + for file_group in self.filegroups.values(): + for request_file in file_group.output_files: + relpath = request_file.relpath + abspath = self.abspath(file_group.name / relpath) + paths.append((relpath, abspath)) + return paths + + def all_files_approved(self): + return all( + request_file.get_decision(self.submitted_reviews.keys()) + == RequestFileDecision.APPROVED + for request_file in self.output_files().values() + ) + + def files_reviewed_by_reviewer_count(self, reviewer: User) -> int: + return sum( + 1 + for rfile in self.output_files().values() + if rfile.get_file_vote_for_user(reviewer) + not in [None, RequestFileVote.UNDECIDED] + ) + + def all_files_reviewed_by_reviewer(self, reviewer: User) -> bool: + return self.files_reviewed_by_reviewer_count(reviewer) == len( + self.output_files() + ) + + def submitted_reviews_count(self): + return len(self.submitted_reviews) + + def status_owner(self) -> RequestStatusOwner: + return permissions.STATUS_OWNERS[self.status] + + def can_be_released(self) -> bool: + return ( + self.status in [RequestStatus.REVIEWED, RequestStatus.APPROVED] + and self.all_files_approved() + ) + + def is_final(self): + return self.status_owner() == RequestStatusOwner.SYSTEM + + def is_under_review(self): + return self.status_owner() == RequestStatusOwner.REVIEWER + + def is_editing(self): + return self.status_owner() == RequestStatusOwner.AUTHOR diff --git a/airlock/permissions.py b/airlock/permissions.py index c9f18ee5..19346340 100644 --- a/airlock/permissions.py +++ b/airlock/permissions.py @@ -19,7 +19,7 @@ # imports are not executed at runtime. # https://peps.python.org/pep-0484/#forward-references # https://mypy.readthedocs.io/en/stable/runtime_troubles.html#import-cycles` - from airlock.business_logic import Comment, ReleaseRequest, Workspace + from airlock.models import Comment, ReleaseRequest, Workspace # The following lists should a) include every status and b) be disjoint diff --git a/airlock/policies.py b/airlock/policies.py index 12692d86..c124b756 100644 --- a/airlock/policies.py +++ b/airlock/policies.py @@ -31,7 +31,7 @@ # imports are not executed at runtime. # https://peps.python.org/pep-0484/#forward-references # https://mypy.readthedocs.io/en/stable/runtime_troubles.html#import-cycles` - from airlock.business_logic import Comment, FileReview, ReleaseRequest, Workspace + from airlock.models import Comment, FileReview, ReleaseRequest, Workspace def check_can_edit_request(request: "ReleaseRequest"): diff --git a/airlock/types.py b/airlock/types.py index 76c1e894..4e0836f7 100644 --- a/airlock/types.py +++ b/airlock/types.py @@ -19,6 +19,9 @@ class UrlPath(PurePosixPath): ... UrlPath = PurePosixPath +ROOT_PATH = UrlPath() # empty path + + @dataclass class FileMetadata: """Represents the base properties of file metadata. diff --git a/airlock/views/code.py b/airlock/views/code.py index 40791e74..3f244580 100644 --- a/airlock/views/code.py +++ b/airlock/views/code.py @@ -7,8 +7,9 @@ from django.views.decorators.vary import vary_on_headers from airlock import exceptions -from airlock.business_logic import CodeRepo, Workspace, bll +from airlock.business_logic import bll from airlock.file_browser_api import get_code_tree +from airlock.models import CodeRepo, Workspace from airlock.types import UrlPath from airlock.views.helpers import ( get_path_item_from_tree_or_404, diff --git a/airlock/views/request.py b/airlock/views/request.py index fa678d7e..711c7130 100644 --- a/airlock/views/request.py +++ b/airlock/views/request.py @@ -15,7 +15,7 @@ from opentelemetry import trace from airlock import exceptions, permissions -from airlock.business_logic import ROOT_PATH, bll +from airlock.business_logic import bll from airlock.enums import RequestFileType, RequestFileVote, RequestStatus, Visibility from airlock.file_browser_api import get_request_tree from airlock.forms import ( @@ -24,7 +24,7 @@ GroupEditForm, MultiselectForm, ) -from airlock.types import UrlPath +from airlock.types import ROOT_PATH, UrlPath from services.tracing import instrument from .helpers import ( diff --git a/local_db/data_access.py b/local_db/data_access.py index c3eafb6d..f115d3f9 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -2,10 +2,7 @@ from django.utils import timezone from airlock import exceptions, permissions -from airlock.business_logic import ( - AuditEvent, - DataAccessLayerProtocol, -) +from airlock.business_logic import DataAccessLayerProtocol from airlock.enums import ( AuditEventType, RequestFileType, @@ -14,6 +11,7 @@ RequestStatusOwner, Visibility, ) +from airlock.models import AuditEvent from airlock.types import UrlPath from local_db.models import ( AuditLog, diff --git a/tests/factories.py b/tests/factories.py index 0897bbc0..d1aaf44e 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -11,20 +11,20 @@ from django.conf import settings from airlock import exceptions -from airlock.business_logic import ( - AuditEvent, - CodeRepo, - ReleaseRequest, - RequestFile, - Workspace, - bll, -) +from airlock.business_logic import bll from airlock.enums import ( RequestFileType, RequestFileVote, RequestStatus, ) from airlock.lib.git import ensure_git_init +from airlock.models import ( + AuditEvent, + CodeRepo, + ReleaseRequest, + RequestFile, + Workspace, +) from airlock.types import UrlPath from airlock.users import User diff --git a/tests/integration/test_visibility.py b/tests/integration/test_visibility.py index 0e4fd058..53fcab65 100644 --- a/tests/integration/test_visibility.py +++ b/tests/integration/test_visibility.py @@ -1,11 +1,6 @@ from dataclasses import dataclass -from airlock.business_logic import ( - AuditEvent, - BusinessLogicLayer, - Comment, - ReleaseRequest, -) +from airlock.business_logic import BusinessLogicLayer from airlock.enums import ( AuditEventType, RequestFileVote, @@ -13,6 +8,11 @@ ReviewTurnPhase, Visibility, ) +from airlock.models import ( + AuditEvent, + Comment, + ReleaseRequest, +) from airlock.users import User from tests import factories diff --git a/tests/integration/views/test_workspace.py b/tests/integration/views/test_workspace.py index 3d610b86..4851621f 100644 --- a/tests/integration/views/test_workspace.py +++ b/tests/integration/views/test_workspace.py @@ -4,8 +4,8 @@ from django.urls import reverse from airlock import policies -from airlock.business_logic import Project from airlock.enums import RequestFileType, RequestStatus, WorkspaceFileStatus +from airlock.models import Project from airlock.types import UrlPath from tests import factories from tests.conftest import get_trace diff --git a/tests/local_db/test_data_access.py b/tests/local_db/test_data_access.py index 49ab736c..51c04d06 100644 --- a/tests/local_db/test_data_access.py +++ b/tests/local_db/test_data_access.py @@ -1,13 +1,14 @@ import pytest from airlock import exceptions -from airlock.business_logic import AuditEvent, store_file +from airlock.business_logic import store_file from airlock.enums import ( AuditEventType, RequestFileType, RequestStatus, Visibility, ) +from airlock.models import AuditEvent from airlock.types import UrlPath from local_db import data_access, models from tests import factories diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index efa2feb3..d7626d30 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -1,22 +1,13 @@ -import hashlib import inspect import json -from hashlib import file_digest -from io import BytesIO from unittest.mock import MagicMock, patch import pytest -from django.conf import settings from django.utils.dateparse import parse_datetime import old_api -from airlock import exceptions, permissions -from airlock.business_logic import ( - AuditEvent, - CodeRepo, - DataAccessLayerProtocol, - Workspace, -) +from airlock import exceptions +from airlock.business_logic import DataAccessLayerProtocol from airlock.enums import ( AuditEventType, NotificationEventType, @@ -24,11 +15,13 @@ RequestFileType, RequestFileVote, RequestStatus, - ReviewTurnPhase, Visibility, WorkspaceFileStatus, ) -from airlock.types import FileMetadata, UrlPath +from airlock.models import ( + AuditEvent, +) +from airlock.types import UrlPath from airlock.visibility import RequestFileStatus from tests import factories @@ -57,427 +50,16 @@ def assert_last_notification(mock_notifications, event_type): assert get_last_notification(mock_notifications)["event_type"] == event_type -def test_workspace_container(): - workspace = factories.create_workspace("workspace") - factories.write_workspace_file(workspace, "foo/bar.html") - - foo_bar_relpath = UrlPath("foo/bar.html") - - assert workspace.root() == settings.WORKSPACE_DIR / "workspace" - assert workspace.get_id() == "workspace" - assert workspace.released_files == set() - assert ( - workspace.get_url(foo_bar_relpath) == "/workspaces/view/workspace/foo/bar.html" - ) - assert ( - "/workspaces/content/workspace/foo/bar.html?cache_id=" - in workspace.get_contents_url(foo_bar_relpath) - ) - plaintext_contents_url = workspace.get_contents_url(foo_bar_relpath, plaintext=True) - assert ( - "/workspaces/content/workspace/foo/bar.html?cache_id=" in plaintext_contents_url - ) - assert "&plaintext=true" in plaintext_contents_url - - assert workspace.request_filetype(UrlPath("path")) is None # type: ignore - - -def test_workspace_from_directory_errors(): - with pytest.raises(exceptions.WorkspaceNotFound): - Workspace.from_directory("workspace", {}) - - (settings.WORKSPACE_DIR / "workspace").mkdir() - with pytest.raises(exceptions.ManifestFileError): - Workspace.from_directory("workspace") - - manifest_path = settings.WORKSPACE_DIR / "workspace/metadata/manifest.json" - manifest_path.parent.mkdir(parents=True) - manifest_path.write_text(":") - with pytest.raises(exceptions.ManifestFileError): - Workspace.from_directory("workspace") - - -def test_workspace_request_filetype(bll): - workspace = factories.create_workspace("workspace") - factories.write_workspace_file(workspace, "foo/bar.txt") - assert workspace.request_filetype(UrlPath("foo/bar.txt")) is None # type: ignore - - -def test_workspace_manifest_for_file(): - workspace = factories.create_workspace("workspace") - factories.write_workspace_file(workspace, "foo/bar.csv", "c1,c2,c3\n1,2,3\n4,5,6") - - file_manifest = workspace.get_manifest_for_file(UrlPath("foo/bar.csv")) - assert file_manifest["row_count"] == 2 - assert file_manifest["col_count"] == 3 - - -def test_workspace_manifest_for_file_not_found(bll): - workspace = factories.create_workspace("workspace") - factories.write_workspace_file(workspace, "foo/bar.txt") - manifest_path = workspace.root() / "metadata/manifest.json" - manifest_data = json.loads(manifest_path.read_text()) - manifest_data["outputs"] = {} - manifest_path.write_text(json.dumps(manifest_data)) - - workspace = bll.get_workspace( - "workspace", factories.create_user(workspaces=["workspace"]) - ) - with pytest.raises(exceptions.ManifestFileError): - workspace.get_manifest_for_file(UrlPath("foo/bar.txt")) - - -def test_get_file_metadata(): - workspace = factories.create_workspace("workspace") - - # non-existent file - with pytest.raises(exceptions.FileNotFound): - workspace.get_file_metadata(UrlPath("metadata/foo.log")) - - # directory - (workspace.root() / "directory").mkdir() - with pytest.raises(AssertionError): - workspace.get_file_metadata(UrlPath("directory")) is None - - # small log file - factories.write_workspace_file( - workspace, "metadata/foo.log", contents="foo", manifest=False - ) - - from_file = workspace.get_file_metadata(UrlPath("metadata/foo.log")) - assert isinstance(from_file, FileMetadata) - assert from_file.size == 3 - assert from_file.timestamp is not None - assert from_file.content_hash == hashlib.sha256(b"foo").hexdigest() - - # larger output file - contents = "x," * 1024 * 1024 - factories.write_workspace_file( - workspace, "output/bar.csv", contents=contents, manifest=True - ) - - from_metadata = workspace.get_file_metadata(UrlPath("output/bar.csv")) - assert isinstance(from_metadata, FileMetadata) - assert from_metadata.size == len(contents) - assert from_metadata.timestamp is not None - assert ( - from_metadata.content_hash - == hashlib.sha256(contents.encode("utf8")).hexdigest() - ) - - -def test_workspace_get_workspace_archived_ongoing(bll): - factories.create_workspace("normal_workspace") - factories.create_workspace("archived_workspace") - factories.create_workspace("not_ongoing") - user = factories.create_user_from_dict( - "user", - workspaces_dict={ - "normal_workspace": { - "project": "project-1", - "project_details": {"name": "project-1", "ongoing": True}, - "archived": False, - }, - "archived_workspace": { - "project": "project-1", - "project_details": {"name": "project-1", "ongoing": True}, - "archived": True, - }, - "not_ongoing": { - "project": "project-2", - "project_details": {"name": "project-2", "ongoing": False}, - "archived": False, - }, - }, - ) - checker = factories.create_user("checker", output_checker=True) - - active_workspace = bll.get_workspace("normal_workspace", user) - archived_workspace = bll.get_workspace("archived_workspace", user) - inactive_project = bll.get_workspace("not_ongoing", user) - assert not active_workspace.is_archived() - assert active_workspace.project().is_ongoing - assert active_workspace.is_active() - assert active_workspace.display_name() == "normal_workspace" - assert active_workspace.project().display_name() == "project-1" - - assert archived_workspace.is_archived() - assert archived_workspace.project().is_ongoing - assert not archived_workspace.is_active() - assert archived_workspace.display_name() == "archived_workspace (ARCHIVED)" - assert archived_workspace.project().display_name() == "project-1" - - assert not inactive_project.is_archived() - assert not inactive_project.project().is_ongoing - assert not inactive_project.is_active() - assert inactive_project.display_name() == "not_ongoing" - assert inactive_project.project().display_name() == "project-2 (INACTIVE)" - - for workspace_name in ["normal_workspace", "archived_workspace", "not_ongoing"]: - workspace = bll.get_workspace(workspace_name, checker) - assert workspace.is_archived() is None - assert bll.get_workspace(workspace_name, checker).project().is_ongoing - assert workspace.display_name() == workspace_name - assert "INACTIVE" not in workspace.project().display_name() - - -def test_workspace_get_workspace_file_status(bll): - path = UrlPath("foo/bar.txt") - workspace = factories.create_workspace("workspace") - user = factories.create_user(workspaces=["workspace"]) - - with pytest.raises(exceptions.FileNotFound): - workspace.get_workspace_file_status(path) - - factories.write_workspace_file(workspace, path, contents="foo") - assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNRELEASED - - release_request = factories.create_release_request(workspace, user=user) - # refresh workspace - workspace = bll.get_workspace("workspace", user) - assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNRELEASED - - factories.add_request_file(release_request, "group", path) - # refresh workspace - workspace = bll.get_workspace("workspace", user) - assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNDER_REVIEW - - factories.write_workspace_file(workspace, path, contents="changed") - assert ( - workspace.get_workspace_file_status(path) == WorkspaceFileStatus.CONTENT_UPDATED - ) - - -def test_workspace_get_released_files(bll): - path = UrlPath("foo/bar.txt") - path1 = UrlPath("foo/supporting_bar.txt") - factories.create_request_at_status( - "workspace", - RequestStatus.RELEASED, - files=[ - factories.request_file( - path=path, - contents="foo", - approved=True, - filetype=RequestFileType.OUTPUT, - ), - factories.request_file( - path=path1, - contents="bar", - filetype=RequestFileType.SUPPORTING, - ), - ], - ) - user = factories.create_user("test", workspaces=["workspace"]) - workspace = bll.get_workspace("workspace", user) - # supporting file is not considered a released file - assert len(workspace.released_files) == 1 - assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.RELEASED - assert workspace.get_workspace_file_status(path1) == WorkspaceFileStatus.UNRELEASED - - -def test_request_returned_get_workspace_file_status(bll): - user = factories.create_user(workspaces=["workspace"]) - path = "file1.txt" - workspace_file = factories.request_file( - group="group", - contents="1", - approved=True, - path=path, - ) - factories.create_request_at_status( - "workspace", - status=RequestStatus.RETURNED, - files=[ - workspace_file, - ], - ) - - # refresh workspace - workspace = bll.get_workspace("workspace", user) - assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNRELEASED - - -def test_request_pending_not_author_get_workspace_file_status(bll): - user = factories.create_user(workspaces=["workspace"]) - path = "file1.txt" - workspace_file = factories.request_file( - group="group", - contents="1", - path=path, - ) - factories.create_request_at_status( - "workspace", - status=RequestStatus.PENDING, - files=[ - workspace_file, - ], - ) - - # refresh workspace - workspace = bll.get_workspace("workspace", user) - assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNRELEASED - - -def test_request_pending_author_get_workspace_file_status(bll): - status = RequestStatus.PENDING - +def setup_empty_release_request(): author = factories.create_user("author", ["workspace"], False) - workspace = factories.create_workspace("workspace") path = UrlPath("path/file.txt") - - workspace_file = factories.request_file( - group="group", - path=path, - contents="1", - user=author, - changes_requested=True, - ) - - factories.create_request_at_status( - workspace.name, - author=author, - status=status, - files=[ - workspace_file, - ], - ) - - # refresh workspace - workspace = bll.get_workspace("workspace", author) - assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNDER_REVIEW - - -def test_request_returned_author_get_workspace_file_status(bll): - status = RequestStatus.RETURNED - - author = factories.create_user("author", ["workspace"], False) workspace = factories.create_workspace("workspace") - path = UrlPath("path/file.txt") - - workspace_file = factories.request_file( - group="group", - path=path, - contents="1", + factories.write_workspace_file(workspace, path) + release_request = factories.create_release_request( + "workspace", user=author, - changes_requested=True, - ) - - factories.create_request_at_status( - workspace.name, - author=author, - status=status, - files=[ - workspace_file, - ], - ) - - # refresh workspace - workspace = bll.get_workspace("workspace", author) - assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNDER_REVIEW - - -def test_request_container(mock_notifications): - release_request = factories.create_release_request("workspace") - release_request = factories.add_request_file(release_request, "group", "bar.html") - rid = release_request.get_id() - - assert release_request.root() == settings.REQUEST_DIR / "workspace" / rid - - assert ( - release_request.get_url("group/bar.html") - == f"/requests/view/{rid}/group/bar.html" - ) - assert ( - f"/requests/content/{rid}/group/bar.html?cache_id=" - in release_request.get_contents_url(UrlPath("group/bar.html")) - ) - plaintext_contents_url = release_request.get_contents_url( - UrlPath("group/bar.html"), plaintext=True - ) - assert f"/requests/content/{rid}/group/bar.html?cache_id=" in plaintext_contents_url - assert "&plaintext=true" in plaintext_contents_url - - assert_no_notifications(mock_notifications) - - -def test_request_file_manifest_data(mock_notifications, bll): - workspace = factories.create_workspace("workspace") - factories.write_workspace_file(workspace, "bar.txt") - user = factories.create_user(workspaces=["workspace"]) - release_request = factories.create_release_request(workspace, user=user) - - # modify the manifest data to known values for asserts - manifest_path = workspace.root() / "metadata/manifest.json" - manifest_data = json.loads(manifest_path.read_text()) - file_manifest = manifest_data["outputs"]["bar.txt"] - file_manifest.update( - { - "job_id": "job-bar", - "size": 10, - "commit": "abcd", - "timestamp": 1715000000, - } - ) - manifest_path.write_text(json.dumps(manifest_data)) - - bll.add_file_to_request(release_request, UrlPath("bar.txt"), user, "group") - - request_file = release_request.filegroups["group"].files[UrlPath("bar.txt")] - assert request_file.timestamp == 1715000000 - assert request_file.commit == "abcd" - assert request_file.job_id == "job-bar" - assert request_file.size == 10 - assert request_file.row_count is None - assert request_file.col_count is None - - -def test_request_file_manifest_data_content_hash_mismatch(mock_notifications, bll): - workspace = factories.create_workspace("workspace") - factories.write_workspace_file(workspace, "bar.txt") - user = factories.create_user(workspaces=["workspace"]) - release_request = factories.create_release_request(workspace, user=user) - - # modify the manifest data to known values for asserts - manifest = workspace.root() / "metadata/manifest.json" - manifest_data = json.loads(manifest.read_text()) - file_manifest = manifest_data["outputs"]["bar.txt"] - file_manifest.update( - { - "content_hash": file_digest(BytesIO(b"foo"), "sha256").hexdigest(), - } - ) - manifest.write_text(json.dumps(manifest_data)) - - with pytest.raises(AssertionError): - bll.add_file_to_request(release_request, UrlPath("bar.txt"), user, "group") - - -def test_code_repo_container(): - workspace = factories.create_workspace("workspace") - factories.write_workspace_file(workspace, "foo.txt") - repo = factories.create_repo(workspace) - - assert repo.get_id() == f"workspace@{repo.commit[:7]}" - assert ( - repo.get_url(UrlPath("project.yaml")) - == f"/code/view/workspace/{repo.commit}/project.yaml" - ) - assert ( - f"/code/contents/workspace/{repo.commit}/project.yaml?cache_id=" - in repo.get_contents_url(UrlPath("project.yaml")) - ) - - plaintext_contents_url = repo.get_contents_url( - UrlPath("project.yaml"), plaintext=True - ) - assert ( - f"/code/contents/workspace/{repo.commit}/project.yaml?cache_id=" - in plaintext_contents_url ) - assert "&plaintext=true" in plaintext_contents_url - - assert repo.request_filetype(UrlPath("path")) == RequestFileType.CODE + return release_request, path, author @pytest.mark.parametrize("output_checker", [False, True]) @@ -1219,12 +801,6 @@ def test_set_status(current, future, valid_author, valid_checker, withdrawn_afte bll.set_status(release_request2, future, user=checker) -def test_request_status_ownership(bll): - """Test every RequestStatus has been assigned an ownership""" - missing_states = set(RequestStatus) - permissions.STATUS_OWNERS.keys() - assert missing_states == set() - - @pytest.mark.parametrize( "current,future,user,notification_event_type", [ @@ -2092,303 +1668,6 @@ def test_withdraw_file_from_request_not_author(bll, status): ) -def test_request_all_files_by_name(bll): - author = factories.create_user(username="author", workspaces=["workspace"]) - path = UrlPath("path/file.txt") - supporting_path = UrlPath("path/supporting_file.txt") - - release_request = factories.create_request_at_status( - "workspace", - author=author, - status=RequestStatus.PENDING, - files=[ - factories.request_file( - group="default", - path=supporting_path, - filetype=RequestFileType.SUPPORTING, - ), - factories.request_file(group="default", path=path), - ], - ) - - # all_files_by_name consists of output files and supporting files - assert release_request.all_files_by_name.keys() == {path, supporting_path} - - filegroup = release_request.filegroups["default"] - assert len(filegroup.files) == 2 - assert len(filegroup.output_files) == 1 - assert len(filegroup.supporting_files) == 1 - - -def test_request_release_get_request_file_from_urlpath(bll): - path = UrlPath("foo/bar.txt") - supporting_path = UrlPath("foo/bar1.txt") - - release_request = factories.create_request_at_status( - "workspace", - status=RequestStatus.PENDING, - files=[ - factories.request_file( - group="default", - path=supporting_path, - filetype=RequestFileType.SUPPORTING, - ), - factories.request_file(group="default", path=path), - ], - ) - - with pytest.raises(exceptions.FileNotFound): - release_request.get_request_file_from_urlpath("badgroup" / path) - - with pytest.raises(exceptions.FileNotFound): - release_request.get_request_file_from_urlpath("default/does/not/exist") - - request_file = release_request.get_request_file_from_urlpath("default" / path) - assert request_file.relpath == path - - -def test_request_release_abspath(bll): - path = UrlPath("foo/bar.txt") - supporting_path = UrlPath("foo/bar1.txt") - release_request = factories.create_request_at_status( - "workspace", - status=RequestStatus.PENDING, - files=[ - factories.request_file( - group="default", - path=supporting_path, - filetype=RequestFileType.SUPPORTING, - ), - factories.request_file(group="default", path=path), - ], - ) - - assert release_request.abspath("default" / path).exists() - assert release_request.abspath("default" / supporting_path).exists() - - -def test_request_release_request_filetype(bll): - path = UrlPath("foo/bar.txt") - supporting_path = UrlPath("foo/bar1.txt") - release_request = factories.create_request_at_status( - "workspace", - status=RequestStatus.PENDING, - files=[ - factories.request_file( - group="default", - path=supporting_path, - filetype=RequestFileType.SUPPORTING, - ), - factories.request_file(group="default", path=path), - ], - ) - - assert release_request.request_filetype("default" / path) == RequestFileType.OUTPUT - assert ( - release_request.request_filetype("default" / supporting_path) - == RequestFileType.SUPPORTING - ) - - -def setup_empty_release_request(): - author = factories.create_user("author", ["workspace"], False) - path = UrlPath("path/file.txt") - workspace = factories.create_workspace("workspace") - factories.write_workspace_file(workspace, path) - release_request = factories.create_release_request( - "workspace", - user=author, - ) - return release_request, path, author - - -def test_get_visible_comments_for_group_class(bll): - author = factories.create_user("author", workspaces=["workspace"]) - checkers = factories.get_default_output_checkers() - - release_request = factories.create_request_at_status( - "workspace", - status=RequestStatus.SUBMITTED, - author=author, - files=[factories.request_file(group="group", path="file.txt", approved=True)], - ) - - bll.group_comment_create( - release_request, - "group", - "turn 1 checker 0 private", - Visibility.PRIVATE, - checkers[0], - ) - bll.group_comment_create( - release_request, - "group", - "turn 1 checker 1 private", - Visibility.PRIVATE, - checkers[1], - ) - bll.group_comment_create( - release_request, - "group", - "turn 1 checker 0 public", - Visibility.PUBLIC, - checkers[0], - ) - - release_request = factories.refresh_release_request(release_request) - assert release_request.review_turn == 1 - assert release_request.get_turn_phase() == ReviewTurnPhase.INDEPENDENT - - def get_comment_metadata(user): - return [ - m for _, m in release_request.get_visible_comments_for_group("group", user) - ] - - assert get_comment_metadata(checkers[0]) == ["comment_blinded", "comment_blinded"] - assert get_comment_metadata(checkers[1]) == ["comment_blinded"] - assert get_comment_metadata(author) == [] - - factories.submit_independent_review(release_request) - - release_request = factories.refresh_release_request(release_request) - assert release_request.review_turn == 1 - assert release_request.get_turn_phase() == ReviewTurnPhase.CONSOLIDATING - - for checker in checkers: - assert get_comment_metadata(checker) == [ - "comment_private", - "comment_private", - "comment_public", - ] - - assert get_comment_metadata(author) == [] - - bll.return_request(release_request, checkers[0]) - release_request = factories.refresh_release_request(release_request) - assert release_request.review_turn == 2 - assert release_request.get_turn_phase() == ReviewTurnPhase.AUTHOR - - for checker in checkers: - assert get_comment_metadata(checker) == [ - "comment_private", - "comment_private", - "comment_public", - ] - - assert get_comment_metadata(author) == ["comment_public"] - - bll.submit_request(release_request, author) - release_request = factories.refresh_release_request(release_request) - assert release_request.review_turn == 3 - assert release_request.get_turn_phase() == ReviewTurnPhase.INDEPENDENT - - bll.group_comment_create( - release_request, - "group", - "turn 3 checker 0 private", - Visibility.PRIVATE, - checkers[0], - ) - bll.group_comment_create( - release_request, - "group", - "turn 3 checker 1 private", - Visibility.PRIVATE, - checkers[1], - ) - - release_request = factories.refresh_release_request(release_request) - - # comments from previous round are visible - assert get_comment_metadata(checkers[0]) == [ - "comment_private", - "comment_private", - "comment_public", - "comment_blinded", - ] - assert get_comment_metadata(checkers[1]) == [ - "comment_private", - "comment_private", - "comment_public", - "comment_blinded", - ] - - -def test_release_request_filegroups_with_no_files(bll): - release_request, _, _ = setup_empty_release_request() - assert release_request.filegroups == {} - - -def test_release_request_filegroups_default_filegroup(bll): - release_request, path, author = setup_empty_release_request() - assert release_request.filegroups == {} - bll.add_file_to_request(release_request, path, author) - assert len(release_request.filegroups) == 1 - filegroup = release_request.filegroups["default"] - assert filegroup.name == "default" - assert len(filegroup.files) == 1 - assert path in filegroup.files - - -def test_release_request_filegroups_named_filegroup(bll): - release_request, path, author = setup_empty_release_request() - assert release_request.filegroups == {} - bll.add_file_to_request(release_request, path, author, "test_group") - assert len(release_request.filegroups) == 1 - filegroup = release_request.filegroups["test_group"] - assert filegroup.name == "test_group" - assert len(filegroup.files) == 1 - assert path in filegroup.files - - -def test_release_request_filegroups_multiple_filegroups(bll): - release_request, path, author = setup_empty_release_request() - bll.add_file_to_request(release_request, path, author, "test_group") - assert len(release_request.filegroups) == 1 - - workspace = bll.get_workspace("workspace", author) - path1 = UrlPath("path/file1.txt") - path2 = UrlPath("path/file2.txt") - factories.write_workspace_file(workspace, path1) - factories.write_workspace_file(workspace, path2) - bll.add_file_to_request(release_request, path1, author, "test_group") - bll.add_file_to_request(release_request, path2, author, "test_group1") - - release_request = bll.get_release_request(release_request.id, author) - assert len(release_request.filegroups) == 2 - - release_request_files = { - filegroup.name: list(filegroup.files) - for filegroup in release_request.filegroups.values() - } - - assert release_request_files == { - "test_group": [UrlPath("path/file.txt"), UrlPath("path/file1.txt")], - "test_group1": [UrlPath("path/file2.txt")], - } - - -def test_release_request_add_same_file(bll): - release_request, path, author = setup_empty_release_request() - assert release_request.filegroups == {} - bll.add_file_to_request(release_request, path, author) - assert len(release_request.filegroups) == 1 - assert len(release_request.filegroups["default"].files) == 1 - - # Adding the same file again should not create a new RequestFile - with pytest.raises(exceptions.APIException): - bll.add_file_to_request(release_request, path, author) - - # We also can't add the same file to a different group - with pytest.raises(exceptions.APIException): - bll.add_file_to_request(release_request, path, author, "new_group") - - release_request = bll.get_release_request(release_request.id, author) - # No additional files or groups have been created - assert len(release_request.filegroups) == 1 - assert len(release_request.filegroups["default"].files) == 1 - - def _get_request_file(release_request, path): """Syntactic sugar to make the tests a little more readable""" # refresh @@ -3654,28 +2933,3 @@ def test_group_comment_delete_invalid_params(bll): bll.group_comment_delete(release_request, "badgroup", test_comment.id, author) assert len(release_request.filegroups["group"].comments) == 1 - - -@pytest.mark.parametrize( - "manifest", - [ - {}, - {"repo": None, "outputs": {}}, - {"repo": None, "outputs": {"file.txt": {"commit": "commit"}}}, - ], -) -def test_coderepo_from_workspace_no_repo_in_manifest(bll, manifest): - workspace = factories.create_workspace("workspace") - workspace.manifest = manifest - with pytest.raises(CodeRepo.RepoNotFound): - CodeRepo.from_workspace(workspace, "commit") - - -def test_coderepo_from_workspace(bll): - workspace = factories.create_workspace("workspace") - factories.create_repo(workspace) - # No root repo, retrieved from first output in manifest instead - workspace.manifest["repo"] = None - CodeRepo.from_workspace( - workspace, workspace.manifest["outputs"]["foo.txt"]["commit"] - ) diff --git a/tests/unit/test_models.py b/tests/unit/test_models.py new file mode 100644 index 00000000..a74c6f2d --- /dev/null +++ b/tests/unit/test_models.py @@ -0,0 +1,774 @@ +import hashlib +import json +from hashlib import file_digest +from io import BytesIO + +import pytest +from django.conf import settings + +from airlock import exceptions, permissions +from airlock.enums import ( + RequestFileType, + RequestStatus, + ReviewTurnPhase, + Visibility, + WorkspaceFileStatus, +) +from airlock.models import ( + CodeRepo, + Workspace, +) +from airlock.types import FileMetadata, UrlPath +from tests import factories + + +pytestmark = pytest.mark.django_db + + +def test_workspace_container(): + workspace = factories.create_workspace("workspace") + factories.write_workspace_file(workspace, "foo/bar.html") + + foo_bar_relpath = UrlPath("foo/bar.html") + + assert workspace.root() == settings.WORKSPACE_DIR / "workspace" + assert workspace.get_id() == "workspace" + assert workspace.released_files == set() + assert ( + workspace.get_url(foo_bar_relpath) == "/workspaces/view/workspace/foo/bar.html" + ) + assert ( + "/workspaces/content/workspace/foo/bar.html?cache_id=" + in workspace.get_contents_url(foo_bar_relpath) + ) + plaintext_contents_url = workspace.get_contents_url(foo_bar_relpath, plaintext=True) + assert ( + "/workspaces/content/workspace/foo/bar.html?cache_id=" in plaintext_contents_url + ) + assert "&plaintext=true" in plaintext_contents_url + + assert workspace.request_filetype(UrlPath("path")) is None # type: ignore + + +def test_workspace_from_directory_errors(): + with pytest.raises(exceptions.WorkspaceNotFound): + Workspace.from_directory("workspace", {}) + + (settings.WORKSPACE_DIR / "workspace").mkdir() + with pytest.raises(exceptions.ManifestFileError): + Workspace.from_directory("workspace") + + manifest_path = settings.WORKSPACE_DIR / "workspace/metadata/manifest.json" + manifest_path.parent.mkdir(parents=True) + manifest_path.write_text(":") + with pytest.raises(exceptions.ManifestFileError): + Workspace.from_directory("workspace") + + +def test_workspace_request_filetype(bll): + workspace = factories.create_workspace("workspace") + factories.write_workspace_file(workspace, "foo/bar.txt") + assert workspace.request_filetype(UrlPath("foo/bar.txt")) is None # type: ignore + + +def test_workspace_manifest_for_file(): + workspace = factories.create_workspace("workspace") + factories.write_workspace_file(workspace, "foo/bar.csv", "c1,c2,c3\n1,2,3\n4,5,6") + + file_manifest = workspace.get_manifest_for_file(UrlPath("foo/bar.csv")) + assert file_manifest["row_count"] == 2 + assert file_manifest["col_count"] == 3 + + +def test_workspace_manifest_for_file_not_found(bll): + workspace = factories.create_workspace("workspace") + factories.write_workspace_file(workspace, "foo/bar.txt") + manifest_path = workspace.root() / "metadata/manifest.json" + manifest_data = json.loads(manifest_path.read_text()) + manifest_data["outputs"] = {} + manifest_path.write_text(json.dumps(manifest_data)) + + workspace = bll.get_workspace( + "workspace", factories.create_user(workspaces=["workspace"]) + ) + with pytest.raises(exceptions.ManifestFileError): + workspace.get_manifest_for_file(UrlPath("foo/bar.txt")) + + +def test_get_file_metadata(): + workspace = factories.create_workspace("workspace") + + # non-existent file + with pytest.raises(exceptions.FileNotFound): + workspace.get_file_metadata(UrlPath("metadata/foo.log")) + + # directory + (workspace.root() / "directory").mkdir() + with pytest.raises(AssertionError): + workspace.get_file_metadata(UrlPath("directory")) is None + + # small log file + factories.write_workspace_file( + workspace, "metadata/foo.log", contents="foo", manifest=False + ) + + from_file = workspace.get_file_metadata(UrlPath("metadata/foo.log")) + assert isinstance(from_file, FileMetadata) + assert from_file.size == 3 + assert from_file.timestamp is not None + assert from_file.content_hash == hashlib.sha256(b"foo").hexdigest() + + # larger output file + contents = "x," * 1024 * 1024 + factories.write_workspace_file( + workspace, "output/bar.csv", contents=contents, manifest=True + ) + + from_metadata = workspace.get_file_metadata(UrlPath("output/bar.csv")) + assert isinstance(from_metadata, FileMetadata) + assert from_metadata.size == len(contents) + assert from_metadata.timestamp is not None + assert ( + from_metadata.content_hash + == hashlib.sha256(contents.encode("utf8")).hexdigest() + ) + + +def test_workspace_get_workspace_archived_ongoing(bll): + factories.create_workspace("normal_workspace") + factories.create_workspace("archived_workspace") + factories.create_workspace("not_ongoing") + user = factories.create_user_from_dict( + "user", + workspaces_dict={ + "normal_workspace": { + "project": "project-1", + "project_details": {"name": "project-1", "ongoing": True}, + "archived": False, + }, + "archived_workspace": { + "project": "project-1", + "project_details": {"name": "project-1", "ongoing": True}, + "archived": True, + }, + "not_ongoing": { + "project": "project-2", + "project_details": {"name": "project-2", "ongoing": False}, + "archived": False, + }, + }, + ) + checker = factories.create_user("checker", output_checker=True) + + active_workspace = bll.get_workspace("normal_workspace", user) + archived_workspace = bll.get_workspace("archived_workspace", user) + inactive_project = bll.get_workspace("not_ongoing", user) + assert not active_workspace.is_archived() + assert active_workspace.project().is_ongoing + assert active_workspace.is_active() + assert active_workspace.display_name() == "normal_workspace" + assert active_workspace.project().display_name() == "project-1" + + assert archived_workspace.is_archived() + assert archived_workspace.project().is_ongoing + assert not archived_workspace.is_active() + assert archived_workspace.display_name() == "archived_workspace (ARCHIVED)" + assert archived_workspace.project().display_name() == "project-1" + + assert not inactive_project.is_archived() + assert not inactive_project.project().is_ongoing + assert not inactive_project.is_active() + assert inactive_project.display_name() == "not_ongoing" + assert inactive_project.project().display_name() == "project-2 (INACTIVE)" + + for workspace_name in ["normal_workspace", "archived_workspace", "not_ongoing"]: + workspace = bll.get_workspace(workspace_name, checker) + assert workspace.is_archived() is None + assert bll.get_workspace(workspace_name, checker).project().is_ongoing + assert workspace.display_name() == workspace_name + assert "INACTIVE" not in workspace.project().display_name() + + +def test_workspace_get_workspace_file_status(bll): + path = UrlPath("foo/bar.txt") + workspace = factories.create_workspace("workspace") + user = factories.create_user(workspaces=["workspace"]) + + with pytest.raises(exceptions.FileNotFound): + workspace.get_workspace_file_status(path) + + factories.write_workspace_file(workspace, path, contents="foo") + assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNRELEASED + + release_request = factories.create_release_request(workspace, user=user) + # refresh workspace + workspace = bll.get_workspace("workspace", user) + assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNRELEASED + + factories.add_request_file(release_request, "group", path) + # refresh workspace + workspace = bll.get_workspace("workspace", user) + assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNDER_REVIEW + + factories.write_workspace_file(workspace, path, contents="changed") + assert ( + workspace.get_workspace_file_status(path) == WorkspaceFileStatus.CONTENT_UPDATED + ) + + +def test_workspace_get_released_files(bll): + path = UrlPath("foo/bar.txt") + path1 = UrlPath("foo/supporting_bar.txt") + factories.create_request_at_status( + "workspace", + RequestStatus.RELEASED, + files=[ + factories.request_file( + path=path, + contents="foo", + approved=True, + filetype=RequestFileType.OUTPUT, + ), + factories.request_file( + path=path1, + contents="bar", + filetype=RequestFileType.SUPPORTING, + ), + ], + ) + user = factories.create_user("test", workspaces=["workspace"]) + workspace = bll.get_workspace("workspace", user) + # supporting file is not considered a released file + assert len(workspace.released_files) == 1 + assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.RELEASED + assert workspace.get_workspace_file_status(path1) == WorkspaceFileStatus.UNRELEASED + + +def test_request_returned_get_workspace_file_status(bll): + user = factories.create_user(workspaces=["workspace"]) + path = "file1.txt" + workspace_file = factories.request_file( + group="group", + contents="1", + approved=True, + path=path, + ) + factories.create_request_at_status( + "workspace", + status=RequestStatus.RETURNED, + files=[ + workspace_file, + ], + ) + + # refresh workspace + workspace = bll.get_workspace("workspace", user) + assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNRELEASED + + +def test_request_pending_not_author_get_workspace_file_status(bll): + user = factories.create_user(workspaces=["workspace"]) + path = "file1.txt" + workspace_file = factories.request_file( + group="group", + contents="1", + path=path, + ) + factories.create_request_at_status( + "workspace", + status=RequestStatus.PENDING, + files=[ + workspace_file, + ], + ) + + # refresh workspace + workspace = bll.get_workspace("workspace", user) + assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNRELEASED + + +def test_request_pending_author_get_workspace_file_status(bll): + status = RequestStatus.PENDING + + author = factories.create_user("author", ["workspace"], False) + workspace = factories.create_workspace("workspace") + path = UrlPath("path/file.txt") + + workspace_file = factories.request_file( + group="group", + path=path, + contents="1", + user=author, + changes_requested=True, + ) + + factories.create_request_at_status( + workspace.name, + author=author, + status=status, + files=[ + workspace_file, + ], + ) + + # refresh workspace + workspace = bll.get_workspace("workspace", author) + assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNDER_REVIEW + + +def test_request_returned_author_get_workspace_file_status(bll): + status = RequestStatus.RETURNED + + author = factories.create_user("author", ["workspace"], False) + workspace = factories.create_workspace("workspace") + path = UrlPath("path/file.txt") + + workspace_file = factories.request_file( + group="group", + path=path, + contents="1", + user=author, + changes_requested=True, + ) + + factories.create_request_at_status( + workspace.name, + author=author, + status=status, + files=[ + workspace_file, + ], + ) + + # refresh workspace + workspace = bll.get_workspace("workspace", author) + assert workspace.get_workspace_file_status(path) == WorkspaceFileStatus.UNDER_REVIEW + + +def test_request_container(): + release_request = factories.create_release_request("workspace") + release_request = factories.add_request_file(release_request, "group", "bar.html") + rid = release_request.get_id() + + assert release_request.root() == settings.REQUEST_DIR / "workspace" / rid + + assert ( + release_request.get_url("group/bar.html") + == f"/requests/view/{rid}/group/bar.html" + ) + assert ( + f"/requests/content/{rid}/group/bar.html?cache_id=" + in release_request.get_contents_url(UrlPath("group/bar.html")) + ) + plaintext_contents_url = release_request.get_contents_url( + UrlPath("group/bar.html"), plaintext=True + ) + assert f"/requests/content/{rid}/group/bar.html?cache_id=" in plaintext_contents_url + assert "&plaintext=true" in plaintext_contents_url + + +def test_request_file_manifest_data(mock_notifications, bll): + workspace = factories.create_workspace("workspace") + factories.write_workspace_file(workspace, "bar.txt") + user = factories.create_user(workspaces=["workspace"]) + release_request = factories.create_release_request(workspace, user=user) + + # modify the manifest data to known values for asserts + manifest_path = workspace.root() / "metadata/manifest.json" + manifest_data = json.loads(manifest_path.read_text()) + file_manifest = manifest_data["outputs"]["bar.txt"] + file_manifest.update( + { + "job_id": "job-bar", + "size": 10, + "commit": "abcd", + "timestamp": 1715000000, + } + ) + manifest_path.write_text(json.dumps(manifest_data)) + + bll.add_file_to_request(release_request, UrlPath("bar.txt"), user, "group") + + request_file = release_request.filegroups["group"].files[UrlPath("bar.txt")] + assert request_file.timestamp == 1715000000 + assert request_file.commit == "abcd" + assert request_file.job_id == "job-bar" + assert request_file.size == 10 + assert request_file.row_count is None + assert request_file.col_count is None + + +def test_request_file_manifest_data_content_hash_mismatch(mock_notifications, bll): + workspace = factories.create_workspace("workspace") + factories.write_workspace_file(workspace, "bar.txt") + user = factories.create_user(workspaces=["workspace"]) + release_request = factories.create_release_request(workspace, user=user) + + # modify the manifest data to known values for asserts + manifest = workspace.root() / "metadata/manifest.json" + manifest_data = json.loads(manifest.read_text()) + file_manifest = manifest_data["outputs"]["bar.txt"] + file_manifest.update( + { + "content_hash": file_digest(BytesIO(b"foo"), "sha256").hexdigest(), + } + ) + manifest.write_text(json.dumps(manifest_data)) + + with pytest.raises(AssertionError): + bll.add_file_to_request(release_request, UrlPath("bar.txt"), user, "group") + + +def test_code_repo_container(): + workspace = factories.create_workspace("workspace") + factories.write_workspace_file(workspace, "foo.txt") + repo = factories.create_repo(workspace) + + assert repo.get_id() == f"workspace@{repo.commit[:7]}" + assert ( + repo.get_url(UrlPath("project.yaml")) + == f"/code/view/workspace/{repo.commit}/project.yaml" + ) + assert ( + f"/code/contents/workspace/{repo.commit}/project.yaml?cache_id=" + in repo.get_contents_url(UrlPath("project.yaml")) + ) + + plaintext_contents_url = repo.get_contents_url( + UrlPath("project.yaml"), plaintext=True + ) + assert ( + f"/code/contents/workspace/{repo.commit}/project.yaml?cache_id=" + in plaintext_contents_url + ) + assert "&plaintext=true" in plaintext_contents_url + + assert repo.request_filetype(UrlPath("path")) == RequestFileType.CODE + + +def test_request_status_ownership(bll): + """Test every RequestStatus has been assigned an ownership""" + missing_states = set(RequestStatus) - permissions.STATUS_OWNERS.keys() + assert missing_states == set() + + +def test_request_all_files_by_name(bll): + author = factories.create_user(username="author", workspaces=["workspace"]) + path = UrlPath("path/file.txt") + supporting_path = UrlPath("path/supporting_file.txt") + + release_request = factories.create_request_at_status( + "workspace", + author=author, + status=RequestStatus.PENDING, + files=[ + factories.request_file( + group="default", + path=supporting_path, + filetype=RequestFileType.SUPPORTING, + ), + factories.request_file(group="default", path=path), + ], + ) + + # all_files_by_name consists of output files and supporting files + assert release_request.all_files_by_name.keys() == {path, supporting_path} + + filegroup = release_request.filegroups["default"] + assert len(filegroup.files) == 2 + assert len(filegroup.output_files) == 1 + assert len(filegroup.supporting_files) == 1 + + +def test_request_release_get_request_file_from_urlpath(bll): + path = UrlPath("foo/bar.txt") + supporting_path = UrlPath("foo/bar1.txt") + + release_request = factories.create_request_at_status( + "workspace", + status=RequestStatus.PENDING, + files=[ + factories.request_file( + group="default", + path=supporting_path, + filetype=RequestFileType.SUPPORTING, + ), + factories.request_file(group="default", path=path), + ], + ) + + with pytest.raises(exceptions.FileNotFound): + release_request.get_request_file_from_urlpath("badgroup" / path) + + with pytest.raises(exceptions.FileNotFound): + release_request.get_request_file_from_urlpath("default/does/not/exist") + + request_file = release_request.get_request_file_from_urlpath("default" / path) + assert request_file.relpath == path + + +def test_request_release_abspath(bll): + path = UrlPath("foo/bar.txt") + supporting_path = UrlPath("foo/bar1.txt") + release_request = factories.create_request_at_status( + "workspace", + status=RequestStatus.PENDING, + files=[ + factories.request_file( + group="default", + path=supporting_path, + filetype=RequestFileType.SUPPORTING, + ), + factories.request_file(group="default", path=path), + ], + ) + + assert release_request.abspath("default" / path).exists() + assert release_request.abspath("default" / supporting_path).exists() + + +def test_request_release_request_filetype(bll): + path = UrlPath("foo/bar.txt") + supporting_path = UrlPath("foo/bar1.txt") + release_request = factories.create_request_at_status( + "workspace", + status=RequestStatus.PENDING, + files=[ + factories.request_file( + group="default", + path=supporting_path, + filetype=RequestFileType.SUPPORTING, + ), + factories.request_file(group="default", path=path), + ], + ) + + assert release_request.request_filetype("default" / path) == RequestFileType.OUTPUT + assert ( + release_request.request_filetype("default" / supporting_path) + == RequestFileType.SUPPORTING + ) + + +def setup_empty_release_request(): + author = factories.create_user("author", ["workspace"], False) + path = UrlPath("path/file.txt") + workspace = factories.create_workspace("workspace") + factories.write_workspace_file(workspace, path) + release_request = factories.create_release_request( + "workspace", + user=author, + ) + return release_request, path, author + + +def test_get_visible_comments_for_group_class(bll): + author = factories.create_user("author", workspaces=["workspace"]) + checkers = factories.get_default_output_checkers() + + release_request = factories.create_request_at_status( + "workspace", + status=RequestStatus.SUBMITTED, + author=author, + files=[factories.request_file(group="group", path="file.txt", approved=True)], + ) + + bll.group_comment_create( + release_request, + "group", + "turn 1 checker 0 private", + Visibility.PRIVATE, + checkers[0], + ) + bll.group_comment_create( + release_request, + "group", + "turn 1 checker 1 private", + Visibility.PRIVATE, + checkers[1], + ) + bll.group_comment_create( + release_request, + "group", + "turn 1 checker 0 public", + Visibility.PUBLIC, + checkers[0], + ) + + release_request = factories.refresh_release_request(release_request) + assert release_request.review_turn == 1 + assert release_request.get_turn_phase() == ReviewTurnPhase.INDEPENDENT + + def get_comment_metadata(user): + return [ + m for _, m in release_request.get_visible_comments_for_group("group", user) + ] + + assert get_comment_metadata(checkers[0]) == ["comment_blinded", "comment_blinded"] + assert get_comment_metadata(checkers[1]) == ["comment_blinded"] + assert get_comment_metadata(author) == [] + + factories.submit_independent_review(release_request) + + release_request = factories.refresh_release_request(release_request) + assert release_request.review_turn == 1 + assert release_request.get_turn_phase() == ReviewTurnPhase.CONSOLIDATING + + for checker in checkers: + assert get_comment_metadata(checker) == [ + "comment_private", + "comment_private", + "comment_public", + ] + + assert get_comment_metadata(author) == [] + + bll.return_request(release_request, checkers[0]) + release_request = factories.refresh_release_request(release_request) + assert release_request.review_turn == 2 + assert release_request.get_turn_phase() == ReviewTurnPhase.AUTHOR + + for checker in checkers: + assert get_comment_metadata(checker) == [ + "comment_private", + "comment_private", + "comment_public", + ] + + assert get_comment_metadata(author) == ["comment_public"] + + bll.submit_request(release_request, author) + release_request = factories.refresh_release_request(release_request) + assert release_request.review_turn == 3 + assert release_request.get_turn_phase() == ReviewTurnPhase.INDEPENDENT + + bll.group_comment_create( + release_request, + "group", + "turn 3 checker 0 private", + Visibility.PRIVATE, + checkers[0], + ) + bll.group_comment_create( + release_request, + "group", + "turn 3 checker 1 private", + Visibility.PRIVATE, + checkers[1], + ) + + release_request = factories.refresh_release_request(release_request) + + # comments from previous round are visible + assert get_comment_metadata(checkers[0]) == [ + "comment_private", + "comment_private", + "comment_public", + "comment_blinded", + ] + assert get_comment_metadata(checkers[1]) == [ + "comment_private", + "comment_private", + "comment_public", + "comment_blinded", + ] + + +def test_release_request_filegroups_with_no_files(bll): + release_request, _, _ = setup_empty_release_request() + assert release_request.filegroups == {} + + +def test_release_request_filegroups_default_filegroup(bll): + release_request, path, author = setup_empty_release_request() + assert release_request.filegroups == {} + bll.add_file_to_request(release_request, path, author) + assert len(release_request.filegroups) == 1 + filegroup = release_request.filegroups["default"] + assert filegroup.name == "default" + assert len(filegroup.files) == 1 + assert path in filegroup.files + + +def test_release_request_filegroups_named_filegroup(bll): + release_request, path, author = setup_empty_release_request() + assert release_request.filegroups == {} + bll.add_file_to_request(release_request, path, author, "test_group") + assert len(release_request.filegroups) == 1 + filegroup = release_request.filegroups["test_group"] + assert filegroup.name == "test_group" + assert len(filegroup.files) == 1 + assert path in filegroup.files + + +def test_release_request_filegroups_multiple_filegroups(bll): + release_request, path, author = setup_empty_release_request() + bll.add_file_to_request(release_request, path, author, "test_group") + assert len(release_request.filegroups) == 1 + + workspace = bll.get_workspace("workspace", author) + path1 = UrlPath("path/file1.txt") + path2 = UrlPath("path/file2.txt") + factories.write_workspace_file(workspace, path1) + factories.write_workspace_file(workspace, path2) + bll.add_file_to_request(release_request, path1, author, "test_group") + bll.add_file_to_request(release_request, path2, author, "test_group1") + + release_request = bll.get_release_request(release_request.id, author) + assert len(release_request.filegroups) == 2 + + release_request_files = { + filegroup.name: list(filegroup.files) + for filegroup in release_request.filegroups.values() + } + + assert release_request_files == { + "test_group": [UrlPath("path/file.txt"), UrlPath("path/file1.txt")], + "test_group1": [UrlPath("path/file2.txt")], + } + + +def test_release_request_add_same_file(bll): + release_request, path, author = setup_empty_release_request() + assert release_request.filegroups == {} + bll.add_file_to_request(release_request, path, author) + assert len(release_request.filegroups) == 1 + assert len(release_request.filegroups["default"].files) == 1 + + # Adding the same file again should not create a new RequestFile + with pytest.raises(exceptions.APIException): + bll.add_file_to_request(release_request, path, author) + + # We also can't add the same file to a different group + with pytest.raises(exceptions.APIException): + bll.add_file_to_request(release_request, path, author, "new_group") + + release_request = bll.get_release_request(release_request.id, author) + # No additional files or groups have been created + assert len(release_request.filegroups) == 1 + assert len(release_request.filegroups["default"].files) == 1 + + +@pytest.mark.parametrize( + "manifest", + [ + {}, + {"repo": None, "outputs": {}}, + {"repo": None, "outputs": {"file.txt": {"commit": "commit"}}}, + ], +) +def test_coderepo_from_workspace_no_repo_in_manifest(bll, manifest): + workspace = factories.create_workspace("workspace") + workspace.manifest = manifest + with pytest.raises(CodeRepo.RepoNotFound): + CodeRepo.from_workspace(workspace, "commit") + + +def test_coderepo_from_workspace(bll): + workspace = factories.create_workspace("workspace") + factories.create_repo(workspace) + # No root repo, retrieved from first output in manifest instead + workspace.manifest["repo"] = None + CodeRepo.from_workspace( + workspace, workspace.manifest["outputs"]["foo.txt"]["commit"] + )