From 62985410ece2fd86c2d53107d59b9c64e44848e2 Mon Sep 17 00:00:00 2001 From: jjgriff93 Date: Mon, 24 Oct 2022 14:02:45 +0000 Subject: [PATCH 01/13] Fixed initiator queries and display --- api_app/api/routes/airlock.py | 4 ++-- api_app/api/routes/airlock_resource_helpers.py | 4 ++-- api_app/db/repositories/airlock_requests.py | 11 ++++++----- ui/app/src/components/shared/airlock/Airlock.tsx | 13 ++++++++----- .../shared/airlock/AirlockViewRequest.tsx | 8 ++++++-- 5 files changed, 24 insertions(+), 16 deletions(-) diff --git a/api_app/api/routes/airlock.py b/api_app/api/routes/airlock.py index 7d1d5d1cc5..2fc0456b9c 100644 --- a/api_app/api/routes/airlock.py +++ b/api_app/api/routes/airlock.py @@ -55,11 +55,11 @@ async def get_all_airlock_requests_by_workspace( airlock_request_repo=Depends(get_repository(AirlockRequestRepository)), workspace=Depends(get_deployed_workspace_by_id_from_path), user=Depends(get_current_workspace_owner_or_researcher_user_or_airlock_manager), - creator_user_id: str = None, requestType: AirlockRequestType = None, status: AirlockRequestStatus = None, + initiator_user_id: str = None, requestType: AirlockRequestType = None, status: AirlockRequestStatus = None, order_by: str = None, order_ascending: bool = True) -> AirlockRequestWithAllowedUserActionsInList: try: airlock_requests = get_airlock_requests_by_user_and_workspace(user=user, workspace=workspace, airlock_request_repo=airlock_request_repo, - creator_user_id=creator_user_id, type=requestType, status=status, + initiator_user_id=initiator_user_id, type=requestType, status=status, order_by=order_by, order_ascending=order_ascending) airlock_requests_with_allowed_user_actions = enrich_requests_with_allowed_actions(airlock_requests, user, airlock_request_repo) except (ValidationError, ValueError) as e: diff --git a/api_app/api/routes/airlock_resource_helpers.py b/api_app/api/routes/airlock_resource_helpers.py index eb7e4109ed..1f1117550b 100644 --- a/api_app/api/routes/airlock_resource_helpers.py +++ b/api_app/api/routes/airlock_resource_helpers.py @@ -107,9 +107,9 @@ def check_email_exists(role_assignment_details: defaultdict(list)): def get_airlock_requests_by_user_and_workspace(user: User, workspace: Workspace, airlock_request_repo: AirlockRequestRepository, - creator_user_id: str = None, type: AirlockRequestType = None, status: AirlockRequestStatus = None, + initiator_user_id: str = None, type: AirlockRequestType = None, status: AirlockRequestStatus = None, order_by: str = None, order_ascending=True) -> List[AirlockRequest]: - return airlock_request_repo.get_airlock_requests(workspace_id=workspace.id, user_id=creator_user_id, type=type, status=status, + return airlock_request_repo.get_airlock_requests(workspace_id=workspace.id, initiator_user_id=initiator_user_id, type=type, status=status, order_by=order_by, order_ascending=order_ascending) diff --git a/api_app/db/repositories/airlock_requests.py b/api_app/db/repositories/airlock_requests.py index bbbead480e..1a7292d7ea 100644 --- a/api_app/db/repositories/airlock_requests.py +++ b/api_app/db/repositories/airlock_requests.py @@ -103,12 +103,13 @@ def create_airlock_request_item(self, airlock_request_input: AirlockRequestInCre return airlock_request - def get_airlock_requests(self, workspace_id: str, user_id: str = None, type: AirlockRequestType = None, status: AirlockRequestStatus = None, order_by: str = None, order_ascending=True) -> List[AirlockRequest]: - query = self.airlock_requests_query() + f' where c.workspaceId = "{workspace_id}"' + def get_airlock_requests(self, workspace_id: str, initiator_user_id: str = None, type: AirlockRequestType = None, status: AirlockRequestStatus = None, order_by: str = None, order_ascending=True) -> List[AirlockRequest]: + query = self.airlock_requests_query() + f' WHERE c.workspaceId = "{workspace_id}"' # optional filters - if user_id: - query += ' AND c.user.id=@user_id' + if initiator_user_id: + # If the resource has history, get the first user object (initiator) + query += ' AND (ARRAY_LENGTH(c.history) > 0? c.history[0].user.id=@user_id: c.user.id=@user_id)' if status: query += ' AND c.status=@status' if type: @@ -120,7 +121,7 @@ def get_airlock_requests(self, workspace_id: str, user_id: str = None, type: Air query += ' ASC' if order_ascending else ' DESC' parameters = [ - {"name": "@user_id", "value": user_id}, + {"name": "@user_id", "value": initiator_user_id}, {"name": "@status", "value": status}, {"name": "@type", "value": type}, ] diff --git a/ui/app/src/components/shared/airlock/Airlock.tsx b/ui/app/src/components/shared/airlock/Airlock.tsx index 53b4474f2b..65a9cf82a4 100644 --- a/ui/app/src/components/shared/airlock/Airlock.tsx +++ b/ui/app/src/components/shared/airlock/Airlock.tsx @@ -178,15 +178,18 @@ export const Airlock: React.FunctionComponent = () => { fieldName: 'requestTitle' }, { - key: 'creator_user_id', + key: 'initiator', name: 'Initiator', ariaLabel: 'Creator of the airlock request', minWidth: 150, maxWidth: 200, isResizable: true, - fieldName: 'initiator', - onRender: (request: AirlockRequest) => , - isFiltered: filters.has('creator_user_id') + onRender: (request: AirlockRequest) => 0 + ? request.history[0].user.name + : request.user.name + } />, + isFiltered: filters.has('initiator_user_id') }, { key: 'requestType', @@ -275,7 +278,7 @@ export const Airlock: React.FunctionComponent = () => { iconProps: { iconName: 'EditContact' }, onClick: () => { const userId = account.localAccountId.split('.')[0]; - setFilters(new Map([['creator_user_id', userId]])); + setFilters(new Map([['initiator_user_id', userId]])); } }); } diff --git a/ui/app/src/components/shared/airlock/AirlockViewRequest.tsx b/ui/app/src/components/shared/airlock/AirlockViewRequest.tsx index d1efa59feb..98e7ce52b0 100644 --- a/ui/app/src/components/shared/airlock/AirlockViewRequest.tsx +++ b/ui/app/src/components/shared/airlock/AirlockViewRequest.tsx @@ -1,4 +1,4 @@ -import { DefaultButton, Dialog, DialogFooter, DialogType, DocumentCard, DocumentCardActivity, DocumentCardDetails, DocumentCardTitle, DocumentCardType, FontIcon, getTheme, IStackItemStyles, IStackStyles, IStackTokens, mergeStyles, MessageBar, MessageBarType, Modal, Panel, PanelType, Persona, PersonaSize, PrimaryButton, Spinner, SpinnerSize, Stack, TextField } from "@fluentui/react"; +import { DefaultButton, Dialog, DialogFooter, DocumentCard, DocumentCardActivity, DocumentCardDetails, DocumentCardTitle, DocumentCardType, FontIcon, getTheme, IStackItemStyles, IStackStyles, IStackTokens, mergeStyles, MessageBar, MessageBarType, Modal, Panel, PanelType, Persona, PersonaSize, PrimaryButton, Spinner, SpinnerSize, Stack, TextField } from "@fluentui/react"; import moment from "moment"; import React, { useCallback, useContext, useEffect, useState } from "react"; import { useNavigate, useParams } from "react-router-dom"; @@ -176,7 +176,11 @@ export const AirlockViewRequest: React.FunctionComponentInitiator - + 0 + ? request.history[0].user.name + : request.user.name + } /> From c3a5bbcff6d8a3dc2e5a96ca874b0e8edd5d5adf Mon Sep 17 00:00:00 2001 From: jjgriff93 Date: Mon, 24 Oct 2022 14:10:19 +0000 Subject: [PATCH 02/13] Update version & changelog --- CHANGELOG.md | 3 ++- api_app/_version.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 986fbc3a10..76683ef11c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ -## 0.7.0 (Unreleased) +## 0.6.1 (Unreleased) **BREAKING CHANGES & MIGRATIONS**: @@ -9,6 +9,7 @@ FEATURES: ENHANCEMENTS: BUG FIXES: +* Show the correct initiator of airlock requests in UI and in API queries ([#2779](https://github.com/microsoft/AzureTRE/pull/2779)) COMPONENTS: diff --git a/api_app/_version.py b/api_app/_version.py index dd9b22cccc..722515271f 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.5.1" +__version__ = "0.5.2" From f7cf8b68778d2b5c54c8de45d788cce9b24f1317 Mon Sep 17 00:00:00 2001 From: jjgriff93 Date: Mon, 24 Oct 2022 14:18:55 +0000 Subject: [PATCH 03/13] Fix tests --- .../test_api/test_routes/test_airlock_resource_helpers.py | 2 +- .../test_repositories/test_airlock_request_repository.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py b/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py index 33c758b63b..dc7df94d9f 100644 --- a/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py +++ b/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py @@ -266,7 +266,7 @@ async def test_get_airlock_requests_by_user_and_workspace_with_status_filter_cal get_airlock_requests_by_user_and_workspace(user=user, workspace=workspace, airlock_request_repo=airlock_request_repo_mock, status=AirlockRequestStatus.InReview) - airlock_request_repo_mock.get_airlock_requests.assert_called_once_with(workspace_id=workspace.id, user_id=None, type=None, + airlock_request_repo_mock.get_airlock_requests.assert_called_once_with(workspace_id=workspace.id, initiator_user_id=None, type=None, status=AirlockRequestStatus.InReview, order_by=None, order_ascending=True) diff --git a/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py b/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py index 754f99eac8..c4163cca0e 100644 --- a/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py +++ b/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py @@ -137,7 +137,7 @@ def test_update_airlock_request_should_retry_update_when_etag_is_not_up_to_date( def test_get_airlock_requests_queries_db(airlock_request_repo): airlock_request_repo.container.query_items = MagicMock() - expected_query = airlock_request_repo.airlock_requests_query() + f' where c.workspaceId = "{WORKSPACE_ID}"' + expected_query = airlock_request_repo.airlock_requests_query() + f' WHERE c.workspaceId = "{WORKSPACE_ID}"' expected_parameters = [ {"name": "@user_id", "value": None}, {"name": "@status", "value": None}, From 15d6987335ec1b00d87bb65b873e2475341f9496 Mon Sep 17 00:00:00 2001 From: jjgriff93 Date: Tue, 25 Oct 2022 14:30:35 +0000 Subject: [PATCH 04/13] Added createdBy field Added migrations to rename several airlock fields Migrate history[0].user to createdBy --- CHANGELOG.md | 3 +- api_app/api/routes/airlock.py | 10 +++--- .../api/routes/airlock_resource_helpers.py | 10 +++--- api_app/api/routes/migrations.py | 12 ++++++- api_app/db/migrations/airlock.py | 33 +++++++++++++++++ api_app/db/repositories/airlock_requests.py | 29 ++++++++------- api_app/event_grid/event_sender.py | 2 +- api_app/models/domain/airlock_request.py | 13 +++---- api_app/models/schemas/airlock_request.py | 18 +++++----- .../airlock_request_status_update.py | 2 +- api_app/services/airlock.py | 2 +- .../test_api/test_routes/test_airlock.py | 8 ++--- .../test_airlock_resource_helpers.py | 4 +-- .../test_airlock_request_repository.py | 4 +-- .../test_airlock_request_status_update.py | 4 +-- .../tests_ma/test_services/test_airlock.py | 2 +- .../commands/workspaces/airlock/request.py | 2 +- .../commands/workspaces/airlock/requests.py | 8 ++--- e2e_tests/test_airlock.py | 8 ++--- .../src/components/shared/airlock/Airlock.tsx | 30 +++++++--------- .../shared/airlock/AirlockNewRequest.tsx | 22 ++++++------ .../shared/airlock/AirlockReviewRequest.tsx | 2 +- .../shared/airlock/AirlockViewRequest.tsx | 22 +++++------- ui/app/src/models/airlock.ts | 35 +++++++++++++------ 24 files changed, 167 insertions(+), 118 deletions(-) create mode 100644 api_app/db/migrations/airlock.py diff --git a/CHANGELOG.md b/CHANGELOG.md index cce1627a98..cd48dbb87a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,9 +9,10 @@ FEATURES: * Automatically validate all resources have tre_id tag via TFLint [#2774](https://github.com/microsoft/AzureTRE/pull/2774) ENHANCEMENTS: +* Renamed several airlock fields to make them more descriptive and added a createdBy field. Included migration for backwards compatibility ([#2779](https://github.com/microsoft/AzureTRE/pull/2779)) BUG FIXES: -* Show the correct initiator of airlock requests in UI and in API queries ([#2779](https://github.com/microsoft/AzureTRE/pull/2779)) +* Show the correct createdBy value for airlock requests in UI and in API queries ([#2779](https://github.com/microsoft/AzureTRE/pull/2779)) COMPONENTS: diff --git a/api_app/api/routes/airlock.py b/api_app/api/routes/airlock.py index 2fc0456b9c..184ce59d80 100644 --- a/api_app/api/routes/airlock.py +++ b/api_app/api/routes/airlock.py @@ -55,11 +55,11 @@ async def get_all_airlock_requests_by_workspace( airlock_request_repo=Depends(get_repository(AirlockRequestRepository)), workspace=Depends(get_deployed_workspace_by_id_from_path), user=Depends(get_current_workspace_owner_or_researcher_user_or_airlock_manager), - initiator_user_id: str = None, requestType: AirlockRequestType = None, status: AirlockRequestStatus = None, + creator_user_id: str = None, type: AirlockRequestType = None, status: AirlockRequestStatus = None, order_by: str = None, order_ascending: bool = True) -> AirlockRequestWithAllowedUserActionsInList: try: airlock_requests = get_airlock_requests_by_user_and_workspace(user=user, workspace=workspace, airlock_request_repo=airlock_request_repo, - initiator_user_id=initiator_user_id, type=requestType, status=status, + creator_user_id=creator_user_id, type=type, status=status, order_by=order_by, order_ascending=order_ascending) airlock_requests_with_allowed_user_actions = enrich_requests_with_allowed_actions(airlock_requests, user, airlock_request_repo) except (ValidationError, ValueError) as e: @@ -103,11 +103,11 @@ async def create_review_user_resource( try: # Getting the review configuration from the airlock request's workspace properties - if airlock_request.requestType == AirlockRequestType.Import: + if airlock_request.type == AirlockRequestType.Import: config = workspace.properties["airlock_review_config"]["import"] workspace_id = config["workspace_id"] else: - assert airlock_request.requestType == AirlockRequestType.Export + assert airlock_request.type == AirlockRequestType.Export config = workspace.properties["airlock_review_config"]["export"] workspace_id = workspace.id workspace_service_id = config["workspace_service_id"] @@ -137,7 +137,7 @@ async def create_review_user_resource( templateName=user_resource_template_name, properties={ "display_name": "Airlock Review VM", - "description": f"Airlock Review VM for request {airlock_request.requestTitle} (ID {airlock_request.id})", + "description": f"Airlock Review VM for request {airlock_request.title} (ID {airlock_request.id})", "airlock_request_sas_url": airlock_request_sas_url } ) diff --git a/api_app/api/routes/airlock_resource_helpers.py b/api_app/api/routes/airlock_resource_helpers.py index 1f1117550b..b3ee8e448c 100644 --- a/api_app/api/routes/airlock_resource_helpers.py +++ b/api_app/api/routes/airlock_resource_helpers.py @@ -33,7 +33,7 @@ async def save_and_publish_event_airlock_request(airlock_request: AirlockRequest try: logging.debug(f"Saving airlock request item: {airlock_request.id}") - airlock_request.user = user + airlock_request.updatedBy = user airlock_request.updatedWhen = get_timestamp() airlock_request_repo.save_item(airlock_request) except Exception as e: @@ -53,7 +53,7 @@ async def save_and_publish_event_airlock_request(airlock_request: AirlockRequest async def update_and_publish_event_airlock_request( airlock_request: AirlockRequest, airlock_request_repo: AirlockRequestRepository, - user: User, + updated_by: User, workspace: Workspace, new_status: AirlockRequestStatus = None, request_files: List[AirlockFile] = None, @@ -64,7 +64,7 @@ async def update_and_publish_event_airlock_request( logging.debug(f"Updating airlock request item: {airlock_request.id}") updated_airlock_request = airlock_request_repo.update_airlock_request( original_request=airlock_request, - user=user, + updated_by=updated_by, new_status=new_status, request_files=request_files, status_message=status_message, @@ -107,9 +107,9 @@ def check_email_exists(role_assignment_details: defaultdict(list)): def get_airlock_requests_by_user_and_workspace(user: User, workspace: Workspace, airlock_request_repo: AirlockRequestRepository, - initiator_user_id: str = None, type: AirlockRequestType = None, status: AirlockRequestStatus = None, + creator_user_id: str = None, type: AirlockRequestType = None, status: AirlockRequestStatus = None, order_by: str = None, order_ascending=True) -> List[AirlockRequest]: - return airlock_request_repo.get_airlock_requests(workspace_id=workspace.id, initiator_user_id=initiator_user_id, type=type, status=status, + return airlock_request_repo.get_airlock_requests(workspace_id=workspace.id, creator_user_id=creator_user_id, type=type, status=status, order_by=order_by, order_ascending=order_ascending) diff --git a/api_app/api/routes/migrations.py b/api_app/api/routes/migrations.py index 7697a8c721..e5a57a6ab5 100644 --- a/api_app/api/routes/migrations.py +++ b/api_app/api/routes/migrations.py @@ -1,6 +1,7 @@ import logging from fastapi import APIRouter, Depends, HTTPException, status +from db.migrations.airlock import AirlockMigration from db.migrations.resources import ResourceMigration from db.repositories.operations import OperationRepository from services.authentication import get_current_admin_user @@ -24,7 +25,8 @@ async def migrate_database(resources_repo=Depends(get_repository(ResourceReposit operations_repo=Depends(get_repository(OperationRepository)), shared_services_migration=Depends(get_repository(SharedServiceMigration)), workspace_migration=Depends(get_repository(WorkspaceMigration)), - resource_migration=Depends(get_repository(ResourceMigration))): + resource_migration=Depends(get_repository(ResourceMigration)), + airlock_migration=Depends(get_repository(AirlockMigration))): try: migrations = list() logging.info("PR 1030") @@ -55,6 +57,14 @@ async def migrate_database(resources_repo=Depends(get_repository(ResourceReposit shared_services_migration.checkMinFirewallVersion() migrations.append(Migration(issueNumber="2371", status='Firewall version meets requirement')) + logging.info("PR 2779 - Restructure Airlock requests & add createdBy field") + airlock_migration.rename_field_name('requestType', 'type') + airlock_migration.rename_field_name('requestTitle', 'title') + airlock_migration.rename_field_name('user', 'updatedBy') + airlock_migration.rename_field_name('creationTime', 'createdWhen') + num_updated = airlock_migration.add_created_by_and_rename_in_history() + migrations.append(Migration(issueNumber="2779", status=f'Renamed fields & updated {num_updated} airlock requests with createdBy')) + return MigrationOutList(migrations=migrations) except Exception as e: logging.error("Failed to migrate database: %s", e, exc_info=True) diff --git a/api_app/db/migrations/airlock.py b/api_app/db/migrations/airlock.py new file mode 100644 index 0000000000..db5a8c3300 --- /dev/null +++ b/api_app/db/migrations/airlock.py @@ -0,0 +1,33 @@ +from azure.cosmos import CosmosClient +from db.repositories.airlock_requests import AirlockRequestRepository + + +class AirlockMigration(AirlockRequestRepository): + def __init__(self, client: CosmosClient): + super().__init__(client) + + def add_created_by_and_rename_in_history(self) -> int: + num_updated = 0 + for request in self.container.read_all_items(): + # Only migrate if createdBy isn't present + if 'createdBy' in request: + break + + # For each request, check if it has history + if len(request['history']) > 0: + # createdBy value will be first user in history + request['createdBy'] = request['history'][0]['user'] + + # Also rename user to updatedBy in each history item + for item in request['history']: + if 'user' in item: + item['updatedBy'] = item['user'] + del item['user'] + else: + # If not, the createdBy user will be the same as the updatedBy value + request['createdBy'] = request['updatedBy'] + + self.update_item_dict(request) + num_updated = num_updated + 1 + + return num_updated diff --git a/api_app/db/repositories/airlock_requests.py b/api_app/db/repositories/airlock_requests.py index 1a7292d7ea..612c6a1c69 100644 --- a/api_app/db/repositories/airlock_requests.py +++ b/api_app/db/repositories/airlock_requests.py @@ -31,18 +31,18 @@ def get_resource_base_spec_params(): def get_timestamp(self) -> float: return datetime.utcnow().timestamp() - def update_airlock_request_item(self, original_request: AirlockRequest, new_request: AirlockRequest, user: User, request_properties: dict) -> AirlockRequest: + def update_airlock_request_item(self, original_request: AirlockRequest, new_request: AirlockRequest, updated_by: User, request_properties: dict) -> AirlockRequest: history_item = AirlockRequestHistoryItem( resourceVersion=original_request.resourceVersion, updatedWhen=original_request.updatedWhen, - user=original_request.user, + updatedBy=original_request.updatedBy, properties=request_properties ) new_request.history.append(history_item) # now update the request props new_request.resourceVersion = new_request.resourceVersion + 1 - new_request.user = user + new_request.updatedBy = updated_by new_request.updatedWhen = self.get_timestamp() self.upsert_item_with_etag(new_request, new_request.etag) @@ -93,27 +93,26 @@ def create_airlock_request_item(self, airlock_request_input: AirlockRequestInCre airlock_request = AirlockRequest( id=full_airlock_request_id, workspaceId=workspace_id, - requestTitle=airlock_request_input.requestTitle, + title=airlock_request_input.title, businessJustification=airlock_request_input.businessJustification, - requestType=airlock_request_input.requestType, - creationTime=datetime.utcnow().timestamp(), + type=airlock_request_input.type, + createdWhen=datetime.utcnow().timestamp(), properties=resource_spec_parameters, reviews=[] ) return airlock_request - def get_airlock_requests(self, workspace_id: str, initiator_user_id: str = None, type: AirlockRequestType = None, status: AirlockRequestStatus = None, order_by: str = None, order_ascending=True) -> List[AirlockRequest]: + def get_airlock_requests(self, workspace_id: str, creator_user_id: str = None, type: AirlockRequestType = None, status: AirlockRequestStatus = None, order_by: str = None, order_ascending=True) -> List[AirlockRequest]: query = self.airlock_requests_query() + f' WHERE c.workspaceId = "{workspace_id}"' # optional filters - if initiator_user_id: - # If the resource has history, get the first user object (initiator) - query += ' AND (ARRAY_LENGTH(c.history) > 0? c.history[0].user.id=@user_id: c.user.id=@user_id)' + if creator_user_id: + query += ' AND c.createdBy.id=@user_id' if status: query += ' AND c.status=@status' if type: - query += ' AND c.requestType=@type' + query += ' AND c.type=@type' # optional sorting if order_by: @@ -121,7 +120,7 @@ def get_airlock_requests(self, workspace_id: str, initiator_user_id: str = None, query += ' ASC' if order_ascending else ' DESC' parameters = [ - {"name": "@user_id", "value": initiator_user_id}, + {"name": "@user_id", "value": creator_user_id}, {"name": "@status", "value": status}, {"name": "@type", "value": type}, ] @@ -138,7 +137,7 @@ def get_airlock_request_by_id(self, airlock_request_id: UUID4) -> AirlockRequest def update_airlock_request( self, original_request: AirlockRequest, - user: User, + updated_by: User, new_status: AirlockRequestStatus = None, request_files: List[AirlockFile] = None, status_message: str = None, @@ -152,12 +151,12 @@ def update_airlock_request( airlock_review=airlock_review, review_user_resource=review_user_resource) try: - db_response = self.update_airlock_request_item(original_request, updated_request, user, {"previousStatus": original_request.status}) + db_response = self.update_airlock_request_item(original_request, updated_request, updated_by, {"previousStatus": original_request.status}) except CosmosAccessConditionFailedError: logging.warning(f"ETag mismatch for request ID: '{original_request.id}'. Retrying.") original_request = self.get_airlock_request_by_id(original_request.id) updated_request = self._build_updated_request(original_request=original_request, new_status=new_status, request_files=request_files, status_message=status_message, airlock_review=airlock_review) - db_response = self.update_airlock_request_item(original_request, updated_request, user, {"previousStatus": original_request.status}) + db_response = self.update_airlock_request_item(original_request, updated_request, updated_by, {"previousStatus": original_request.status}) return db_response diff --git a/api_app/event_grid/event_sender.py b/api_app/event_grid/event_sender.py index 81ffa61dca..af582278a6 100644 --- a/api_app/event_grid/event_sender.py +++ b/api_app/event_grid/event_sender.py @@ -12,7 +12,7 @@ async def send_status_changed_event(airlock_request: AirlockRequest, previous_st request_id = airlock_request.id new_status = airlock_request.status.value previous_status = previous_status.value if previous_status else None - request_type = airlock_request.requestType.value + request_type = airlock_request.type.value short_workspace_id = airlock_request.workspaceId[-4:] status_changed_event = EventGridEvent( diff --git a/api_app/models/domain/airlock_request.py b/api_app/models/domain/airlock_request.py index 48b39b8876..76503e69f6 100644 --- a/api_app/models/domain/airlock_request.py +++ b/api_app/models/domain/airlock_request.py @@ -62,7 +62,7 @@ class AirlockRequestHistoryItem(AzureTREModel): """ resourceVersion: int updatedWhen: float - user: dict = {} + updatedBy: dict = {} properties: dict = {} @@ -81,16 +81,17 @@ class AirlockRequest(AzureTREModel): """ id: str = Field(title="Id", description="GUID identifying the resource") resourceVersion: int = 0 - user: dict = {} + createdBy: dict = {} + createdWhen: float = Field(None, title="Creation time of the request") + updatedBy: dict = {} updatedWhen: float = 0 history: List[AirlockRequestHistoryItem] = [] workspaceId: str = Field("", title="Workspace ID", description="Service target Workspace id") - requestType: AirlockRequestType = Field("", title="Airlock request type") + type: AirlockRequestType = Field("", title="Airlock request type") files: List[AirlockFile] = Field([], title="Files of the request") - requestTitle: str = Field("Airlock Request", title="Brief title for the request") - businessJustification: str = Field("Business Justifications", title="Explanation that will be provided to the request reviewer") + title: str = Field("Airlock Request", title="Brief title for the request") + businessJustification: str = Field("Business Justification", title="Explanation that will be provided to the request reviewer") status = AirlockRequestStatus.Draft - creationTime: float = Field(None, title="Creation time of the request") statusMessage: Optional[str] = Field(title="Optional - contains additional information about the current status.") reviews: Optional[List[AirlockReview]] etag: Optional[str] = Field(title="_etag", alias="_etag") diff --git a/api_app/models/schemas/airlock_request.py b/api_app/models/schemas/airlock_request.py index eb42022c17..11bcf4d942 100644 --- a/api_app/models/schemas/airlock_request.py +++ b/api_app/models/schemas/airlock_request.py @@ -20,11 +20,11 @@ def get_sample_airlock_request(workspace_id: str, airlock_request_id: str) -> di "requestId": airlock_request_id, "workspaceId": workspace_id, "status": "draft", - "requestType": "import", + "type": "import", "files": [], - "requestTitle": "a request title", + "title": "a request title", "businessJustification": "some business justification", - "creationTime": datetime.utcnow().timestamp(), + "createdWhen": datetime.utcnow().timestamp(), "reviews": [ get_sample_airlock_review("29990431-5451-40e7-a58a-02e2b7c3d7c8"), get_sample_airlock_review("02dc0f29-351a-43ec-87e7-3dd2b5177b7f")] @@ -34,7 +34,7 @@ def get_sample_airlock_request(workspace_id: str, airlock_request_id: str) -> di def get_sample_airlock_request_with_allowed_user_actions(workspace_id: str) -> dict: return { "airlockRequest": get_sample_airlock_request(workspace_id, str(uuid.uuid4())), - "allowed_user_actions": [AirlockActions.Cancel, AirlockActions.Review, AirlockActions.Submit], + "allowedUserActions": [AirlockActions.Cancel, AirlockActions.Review, AirlockActions.Submit], } @@ -64,7 +64,7 @@ class Config: class AirlockRequestWithAllowedUserActions(BaseModel): airlockRequest: AirlockRequest = Field([], title="Airlock Request") - allowed_user_actions: List[str] = Field([], title="actions that the requesting user can do on the request") + allowedUserActions: List[str] = Field([], title="actions that the requesting user can do on the request") class Config: schema_extra = { @@ -87,16 +87,16 @@ class Config: class AirlockRequestInCreate(BaseModel): - requestType: AirlockRequestType = Field("", title="Airlock request type", description="Specifies if this is an import or an export request") - requestTitle: str = Field("Airlock Request", title="Brief title for the request") + type: AirlockRequestType = Field("", title="Airlock request type", description="Specifies if this is an import or an export request") + title: str = Field("Airlock Request", title="Brief title for the request") businessJustification: str = Field("Business Justifications", title="Explanation that will be provided to the request reviewer") properties: dict = Field({}, title="Airlock request parameters", description="Values for the parameters required by the Airlock request specification") class Config: schema_extra = { "example": { - "requestType": "import", - "requestTitle": "a request title", + "type": "import", + "title": "a request title", "businessJustification": "some business justification" } } diff --git a/api_app/service_bus/airlock_request_status_update.py b/api_app/service_bus/airlock_request_status_update.py index 586ae0252c..6800e19108 100644 --- a/api_app/service_bus/airlock_request_status_update.py +++ b/api_app/service_bus/airlock_request_status_update.py @@ -65,7 +65,7 @@ async def update_status_in_database(airlock_request_repo: AirlockRequestReposito if airlock_request.status == current_status: workspace = workspace_repo.get_workspace_by_id(airlock_request.workspaceId) # update to new status and send to event grid - await update_and_publish_event_airlock_request(airlock_request=airlock_request, airlock_request_repo=airlock_request_repo, user=airlock_request.user, workspace=workspace, new_status=new_status, request_files=request_files, status_message=status_message) + await update_and_publish_event_airlock_request(airlock_request=airlock_request, airlock_request_repo=airlock_request_repo, updated_by=airlock_request.updatedBy, workspace=workspace, new_status=new_status, request_files=request_files, status_message=status_message) result = True else: error_string = strings.STEP_RESULT_MESSAGE_STATUS_DOES_NOT_MATCH.format(airlock_request_id, current_status, airlock_request.status) diff --git a/api_app/services/airlock.py b/api_app/services/airlock.py index 9bbee50432..f554565968 100644 --- a/api_app/services/airlock.py +++ b/api_app/services/airlock.py @@ -13,7 +13,7 @@ def get_account_by_request(airlock_request: AirlockRequest, workspace: Workspace) -> str: tre_id = config.TRE_ID short_workspace_id = workspace.id[-4:] - if airlock_request.requestType == constants.IMPORT_TYPE: + if airlock_request.type == constants.IMPORT_TYPE: if airlock_request.status == AirlockRequestStatus.Draft: return constants.STORAGE_ACCOUNT_NAME_IMPORT_EXTERNAL.format(tre_id) elif airlock_request.status == AirlockRequestStatus.Submitted: diff --git a/api_app/tests_ma/test_api/test_routes/test_airlock.py b/api_app/tests_ma/test_api/test_routes/test_airlock.py index 274980323f..3be25239b4 100644 --- a/api_app/tests_ma/test_api/test_routes/test_airlock.py +++ b/api_app/tests_ma/test_api/test_routes/test_airlock.py @@ -29,7 +29,7 @@ @pytest.fixture def sample_airlock_request_input_data(): return { - "requestType": "import", + "type": "import", "businessJustification": "some business justification" } @@ -45,7 +45,7 @@ def sample_airlock_review_input_data(): @pytest.fixture def sample_airlock_review_with_user_resources(): return { - "requestType": "import", + "type": "import", "businessJustification": "some business justification", "reviewUserResources": [ { @@ -61,9 +61,9 @@ def sample_airlock_request_object(status=AirlockRequestStatus.Draft, airlock_req airlock_request = AirlockRequest( id=airlock_request_id, workspaceId=workspace_id, - requestTitle="test title", + title="test title", businessJustification="test business justification", - requestType="import", + type="import", status=status, reviews=[sample_airlock_review_object()] if reviews else None, reviewUserResources=[sample_airlock_user_resource_object()] if review_user_resource else [] diff --git a/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py b/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py index dc7df94d9f..ff5183f02c 100644 --- a/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py +++ b/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py @@ -33,7 +33,7 @@ def sample_airlock_request(status=AirlockRequestStatus.Draft): airlock_request = AirlockRequest( id=AIRLOCK_REQUEST_ID, workspaceId=WORKSPACE_ID, - requestType=AirlockRequestType.Import, + type=AirlockRequestType.Import, files=[], businessJustification="some test reason", status=status @@ -266,7 +266,7 @@ async def test_get_airlock_requests_by_user_and_workspace_with_status_filter_cal get_airlock_requests_by_user_and_workspace(user=user, workspace=workspace, airlock_request_repo=airlock_request_repo_mock, status=AirlockRequestStatus.InReview) - airlock_request_repo_mock.get_airlock_requests.assert_called_once_with(workspace_id=workspace.id, initiator_user_id=None, type=None, + airlock_request_repo_mock.get_airlock_requests.assert_called_once_with(workspace_id=workspace.id, creator_user_id=None, type=None, status=AirlockRequestStatus.InReview, order_by=None, order_ascending=True) diff --git a/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py b/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py index c4163cca0e..deb80266cf 100644 --- a/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py +++ b/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py @@ -49,7 +49,7 @@ def airlock_request_repo(): @pytest.fixture def sample_airlock_request_input(): - return AirlockRequestInCreate(requestType=AirlockRequestType.Import, businessJustification="Some business justification") + return AirlockRequestInCreate(type=AirlockRequestType.Import, businessJustification="Some business justification") @pytest.fixture @@ -63,7 +63,7 @@ def airlock_request_mock(status=AirlockRequestStatus.Draft): airlock_request = AirlockRequest( id=AIRLOCK_REQUEST_ID, workspaceId=WORKSPACE_ID, - requestType=AirlockRequestType.Import, + type=AirlockRequestType.Import, files=[], businessJustification="some test reason", status=status, diff --git a/api_app/tests_ma/test_service_bus/test_airlock_request_status_update.py b/api_app/tests_ma/test_service_bus/test_airlock_request_status_update.py index 82c0246941..795ca50c77 100644 --- a/api_app/tests_ma/test_service_bus/test_airlock_request_status_update.py +++ b/api_app/tests_ma/test_service_bus/test_airlock_request_status_update.py @@ -61,7 +61,7 @@ def sample_airlock_request(status=AirlockRequestStatus.Submitted): airlock_request = AirlockRequest( id=AIRLOCK_REQUEST_ID, workspaceId=WORKSPACE_ID, - requestType=AirlockRequestType.Import, + type=AirlockRequestType.Import, files=[], businessJustification="some test reason", status=status, @@ -99,7 +99,7 @@ async def test_receiving_good_message(_, app, sb_client, logging_mock, workspace await receive_step_result_message_and_update_status(app) airlock_request_repo().get_airlock_request_by_id.assert_called_once_with(test_sb_step_result_message["data"]["request_id"]) - airlock_request_repo().update_airlock_request.assert_called_once_with(original_request=expected_airlock_request, user=expected_airlock_request.user, new_status=test_sb_step_result_message["data"]["new_status"], request_files=None, status_message=None, airlock_review=None, review_user_resource=None) + airlock_request_repo().update_airlock_request.assert_called_once_with(original_request=expected_airlock_request, updated_by=expected_airlock_request.updatedBy, new_status=test_sb_step_result_message["data"]["new_status"], request_files=None, status_message=None, airlock_review=None, review_user_resource=None) assert eg_client().send.call_count == 2 logging_mock.assert_not_called() sb_client().get_queue_receiver().complete_message.assert_called_once_with(service_bus_received_message_mock) diff --git a/api_app/tests_ma/test_services/test_airlock.py b/api_app/tests_ma/test_services/test_airlock.py index d92edbe862..9f78c555d0 100644 --- a/api_app/tests_ma/test_services/test_airlock.py +++ b/api_app/tests_ma/test_services/test_airlock.py @@ -12,7 +12,7 @@ def sample_airlock_request(status=AirlockRequestStatus.Draft): airlock_request = AirlockRequest( id="AIRLOCK_REQUEST_ID", workspaceId="WORKSPACE_ID", - requestType=AirlockRequestType.Import, + type=AirlockRequestType.Import, files=[], businessJustification="some test reason", status=status, diff --git a/cli/tre/commands/workspaces/airlock/request.py b/cli/tre/commands/workspaces/airlock/request.py index a2ec060b69..9d9d386f66 100644 --- a/cli/tre/commands/workspaces/airlock/request.py +++ b/cli/tre/commands/workspaces/airlock/request.py @@ -5,7 +5,7 @@ from tre.commands.workspaces.airlock.contexts import WorkspaceAirlockContext, pass_workspace_airlock_context from tre.output import output, output_option, query_option -_default_table_query_item = r"airlockRequest.{id:id,workspace_id:workspaceId,type:requestType, title:requestTitle,status:status,business_justification:businessJustification}" +_default_table_query_item = r"airlockRequest.{id:id,workspace_id:workspaceId,type:type, title:title,status:status,business_justification:businessJustification}" def airlock_id_completion(ctx: click.Context, param: click.Parameter, incomplete: str): diff --git a/cli/tre/commands/workspaces/airlock/requests.py b/cli/tre/commands/workspaces/airlock/requests.py index 599c3429aa..8e3524caca 100644 --- a/cli/tre/commands/workspaces/airlock/requests.py +++ b/cli/tre/commands/workspaces/airlock/requests.py @@ -5,8 +5,8 @@ from tre.commands.workspaces.contexts import pass_workspace_context from tre.output import output, output_option, query_option -_default_table_query_list = r"airlockRequests[].airlockRequest.{id:id,workspace_id:workspaceId,type:requestType,status:status,business_justification:businessJustification}" -_default_table_query_item = r"airlockRequest.{id:id,workspace_id:workspaceId,type:requestType,status:status,business_justification:businessJustification}" +_default_table_query_list = r"airlockRequests[].airlockRequest.{id:id,workspace_id:workspaceId,type:type,status:status,business_justification:businessJustification}" +_default_table_query_item = r"airlockRequest.{id:id,workspace_id:workspaceId,type:type,status:status,business_justification:businessJustification}" @click.group(name="airlock-requests", help="List/add airlock requests") @@ -60,8 +60,8 @@ def airlock_create(workspace_context, request_type, title, justification, output 'POST', f'/api/workspaces/{workspace_id}/requests', json_data={ - "requestType": request_type, - "requestTitle": title, + "type": request_type, + "title": title, "businessJustification": justification }, scope_id=workspace_scope) diff --git a/e2e_tests/test_airlock.py b/e2e_tests/test_airlock.py index 5e2a2eb09f..e0f2311ca6 100644 --- a/e2e_tests/test_airlock.py +++ b/e2e_tests/test_airlock.py @@ -53,13 +53,13 @@ async def test_airlock_flow(verify) -> None: # 2. create airlock request LOGGER.info("Creating airlock import request") payload = { - "requestType": airlock_strings.IMPORT, + "type": airlock_strings.IMPORT, "businessJustification": "some business justification" } request_result = await post_request(payload, f'/api{workspace_path}/requests', workspace_owner_token, verify, 201) - assert request_result["airlockRequest"]["requestType"] == airlock_strings.IMPORT + assert request_result["airlockRequest"]["type"] == airlock_strings.IMPORT assert request_result["airlockRequest"]["businessJustification"] == "some business justification" assert request_result["airlockRequest"]["status"] == airlock_strings.DRAFT_STATUS @@ -134,13 +134,13 @@ async def test_airlock_flow(verify) -> None: LOGGER.info("Creating airlock export request") justification = "another business justification" payload = { - "requestType": airlock_strings.EXPORT, + "type": airlock_strings.EXPORT, "businessJustification": justification } request_result = await post_request(payload, f'/api{workspace_path}/requests', workspace_owner_token, verify, 201) - assert request_result["airlockRequest"]["requestType"] == airlock_strings.EXPORT + assert request_result["airlockRequest"]["type"] == airlock_strings.EXPORT assert request_result["airlockRequest"]["businessJustification"] == justification assert request_result["airlockRequest"]["status"] == airlock_strings.DRAFT_STATUS diff --git a/ui/app/src/components/shared/airlock/Airlock.tsx b/ui/app/src/components/shared/airlock/Airlock.tsx index 65a9cf82a4..15d4c07ddd 100644 --- a/ui/app/src/components/shared/airlock/Airlock.tsx +++ b/ui/app/src/components/shared/airlock/Airlock.tsx @@ -59,10 +59,10 @@ export const Airlock: React.FunctionComponent = () => { // Map the inner requests and the allowed user actions to state requests = result.airlockRequests.map((r: { airlockRequest: AirlockRequest, - allowed_user_actions: Array + allowedUserActions: Array }) => { const request = r.airlockRequest; - request.allowed_user_actions = r.allowed_user_actions; + request.allowedUserActions = r.allowedUserActions; return request; }); } else { @@ -175,37 +175,33 @@ export const Airlock: React.FunctionComponent = () => { minWidth: 150, maxWidth: 300, isResizable: true, - fieldName: 'requestTitle' + fieldName: 'title' }, { - key: 'initiator', - name: 'Initiator', + key: 'createdBy', + name: 'Creator', ariaLabel: 'Creator of the airlock request', minWidth: 150, maxWidth: 200, isResizable: true, - onRender: (request: AirlockRequest) => 0 - ? request.history[0].user.name - : request.user.name - } />, - isFiltered: filters.has('initiator_user_id') + onRender: (request: AirlockRequest) => , + isFiltered: filters.has('creator_user_id') }, { - key: 'requestType', + key: 'type', name: 'Type', ariaLabel: 'Whether the request is import or export', minWidth: 70, maxWidth: 100, isResizable: true, - fieldName: 'requestType', + fieldName: 'type', columnActionsMode: ColumnActionsMode.hasDropdown, - isSorted: orderBy === 'requestType', + isSorted: orderBy === 'type', isSortedDescending: !orderAscending, onColumnClick: (ev, column) => openContextMenu(column, ev, Object.values(AirlockRequestType)), onColumnContextMenu: (column, ev) => (column && ev) && openContextMenu(column, ev, Object.values(AirlockRequestType)), - isFiltered: filters.has('requestType') + isFiltered: filters.has('type') }, { key: 'status', @@ -233,7 +229,7 @@ export const Airlock: React.FunctionComponent = () => { isSorted: orderBy === 'createdTime', isSortedDescending: !orderAscending, onRender: (request: AirlockRequest) => { - return { moment.unix(request.creationTime).format('DD/MM/YYYY') }; + return { moment.unix(request.createdWhen).format('DD/MM/YYYY') }; }, onColumnClick: orderByColumn }, @@ -278,7 +274,7 @@ export const Airlock: React.FunctionComponent = () => { iconProps: { iconName: 'EditContact' }, onClick: () => { const userId = account.localAccountId.split('.')[0]; - setFilters(new Map([['initiator_user_id', userId]])); + setFilters(new Map([['creator_user_id', userId]])); } }); } diff --git a/ui/app/src/components/shared/airlock/AirlockNewRequest.tsx b/ui/app/src/components/shared/airlock/AirlockNewRequest.tsx index 4c31578d15..4b774e73d8 100644 --- a/ui/app/src/components/shared/airlock/AirlockNewRequest.tsx +++ b/ui/app/src/components/shared/airlock/AirlockNewRequest.tsx @@ -23,12 +23,12 @@ export const AirlockNewRequest: React.FunctionComponent const workspaceCtx = useContext(WorkspaceContext); const apiCall = useAuthApiCall(); - const onChangeRequestTitle = useCallback( + const onChangetitle = useCallback( (event: React.FormEvent, newValue?: string) => { setNewRequest(request => { return { ...request, - requestTitle: newValue || '' + title: newValue || '' } }); }, @@ -49,7 +49,7 @@ export const AirlockNewRequest: React.FunctionComponent useEffect( () => setRequestValid( - newRequest.requestTitle?.length > 0 && + newRequest.title?.length > 0 && newRequest.businessJustification?.length > 0 ), [newRequest, setRequestValid] @@ -82,7 +82,7 @@ export const AirlockNewRequest: React.FunctionComponent const renderFooter = useCallback(() => { let footer = <> - if (newRequest.requestType) { + if (newRequest.type) { footer = <>
setNewRequest({} as NewAirlockRequest)} styles={{root:{marginRight: 8}}}>Back @@ -96,14 +96,14 @@ export const AirlockNewRequest: React.FunctionComponent let title: string; let currentStep = <>; - // Render current step depending on whether requestType has been selected - if (!newRequest.requestType) { + // Render current step depending on whether type has been selected + if (!newRequest.type) { title = "New airlock request"; currentStep = setNewRequest({ requestType: AirlockRequestType.Import } as NewAirlockRequest)}> + onClick={() => setNewRequest({ type: AirlockRequestType.Import } as NewAirlockRequest)}> @@ -118,7 +118,7 @@ export const AirlockNewRequest: React.FunctionComponent setNewRequest({ requestType: AirlockRequestType.Export } as NewAirlockRequest)}> + onClick={() => setNewRequest({ type: AirlockRequestType.Export } as NewAirlockRequest)}> @@ -131,13 +131,13 @@ export const AirlockNewRequest: React.FunctionComponent ; } else { - title = `New airlock ${newRequest.requestType} request`; + title = `New airlock ${newRequest.type} request`; currentStep = diff --git a/ui/app/src/components/shared/airlock/AirlockReviewRequest.tsx b/ui/app/src/components/shared/airlock/AirlockReviewRequest.tsx index ffeee5b84e..f811f994bd 100644 --- a/ui/app/src/components/shared/airlock/AirlockReviewRequest.tsx +++ b/ui/app/src/components/shared/airlock/AirlockReviewRequest.tsx @@ -260,7 +260,7 @@ export const AirlockReviewRequest: React.FunctionComponent
- Review: {request?.requestTitle} + Review: {request?.title} { - request.allowed_user_actions?.includes(AirlockRequestAction.Cancel) && + request.allowedUserActions?.includes(AirlockRequestAction.Cancel) && {setSubmitError(false); setHideCancelDialog(false)}} styles={destructiveButtonStyles}>Cancel request } { - request.allowed_user_actions?.includes(AirlockRequestAction.Submit) && + request.allowedUserActions?.includes(AirlockRequestAction.Submit) && {setSubmitError(false); setHideSubmitDialog(false)}}>Submit } { - request.allowed_user_actions?.includes(AirlockRequestAction.Review) && + request.allowedUserActions?.includes(AirlockRequestAction.Review) && setReviewIsOpen(true)}>Review }
@@ -151,7 +151,7 @@ export const AirlockViewRequest: React.FunctionComponent - Initiator + Creator - 0 - ? request.history[0].user.name - : request.user.name - } /> + @@ -189,7 +185,7 @@ export const AirlockViewRequest: React.FunctionComponentType -

{request.requestType}

+

{request.type}

 @@ -216,7 +212,7 @@ export const AirlockViewRequest: React.FunctionComponentCreated -

{moment.unix(request.creationTime).format('DD/MM/YYYY')}

+

{moment.unix(request.createdWhen).format('DD/MM/YYYY')}

 @@ -278,7 +274,7 @@ export const AirlockViewRequest: React.FunctionComponent } { - request.reviews.length > 0 && <> + request.reviews && request.reviews.length > 0 && <> Reviews diff --git a/ui/app/src/models/airlock.ts b/ui/app/src/models/airlock.ts index a755a3e4d8..80847b5f21 100644 --- a/ui/app/src/models/airlock.ts +++ b/ui/app/src/models/airlock.ts @@ -1,18 +1,31 @@ -import { Resource } from "./resource"; import { User } from "./user"; -export interface AirlockRequest extends Resource { +export interface AirlockRequest { + id: string; + resourceVersion: number; + createdBy: User; + createdWhen: number; + updatedBy: User; + updatedWhen: number; + history: Array; workspaceId: string; - requestType: AirlockRequestType; + type: AirlockRequestType; files: Array<{name: string, size: number}>; - requestTitle: string; + title: string; businessJustification: string; - statusMessage: null | string; status: AirlockRequestStatus; - creationTime: number; - reviews: Array; reviewUserResources: Array; - allowed_user_actions: Array; + allowedUserActions: Array; + reviews?: Array; + statusMessage?: string; + etag?: string +} + +export interface AirlockRequestHistoryItem { + resourceVersion: number; + updatedWhen: number; + updatedBy: User; + properties: {}; } export enum AirlockRequestType { @@ -23,20 +36,20 @@ export enum AirlockRequestType { export enum AirlockRequestStatus { Draft = 'draft', InReview = 'in_review', - InProgress = 'in_progress', Approved = 'approved', ApprovalInProgress = 'approval_in_progress', RejectionInProgress = 'rejection_in_progress', Rejected = 'rejected', Blocked = 'blocked', + BlockingInProgress = 'blocking_in_progress', Submitted = 'submitted', Cancelled = 'cancelled', Failed = 'failed' } export interface NewAirlockRequest { - requestType: AirlockRequestType; - requestTitle: string; + type: AirlockRequestType; + title: string; businessJustification: string; } From 00935cf3f467625a2331a24882dee241b5bff55b Mon Sep 17 00:00:00 2001 From: jjgriff93 Date: Tue, 25 Oct 2022 15:31:47 +0000 Subject: [PATCH 05/13] Fix tests --- api_app/api/routes/airlock.py | 2 +- .../test_routes/test_airlock_resource_helpers.py | 10 +++++----- .../test_api/test_routes/test_migrations.py | 14 ++++++++++---- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/api_app/api/routes/airlock.py b/api_app/api/routes/airlock.py index 184ce59d80..28222cbd14 100644 --- a/api_app/api/routes/airlock.py +++ b/api_app/api/routes/airlock.py @@ -203,7 +203,7 @@ async def create_airlock_review( elif airlock_review.reviewDecision.value == AirlockReviewDecision.Rejected: review_status = AirlockRequestStatus.RejectionInProgress - updated_airlock_request = await update_and_publish_event_airlock_request(airlock_request=airlock_request, airlock_request_repo=airlock_request_repo, user=user, workspace=workspace, new_status=review_status, airlock_review=airlock_review) + updated_airlock_request = await update_and_publish_event_airlock_request(airlock_request=airlock_request, airlock_request_repo=airlock_request_repo, updated_by=user, workspace=workspace, new_status=review_status, airlock_review=airlock_review) # If there was a VM created for the request, clean it up as it will no longer be needed # In this request, we aren't returning the operations for clean up of VMs, diff --git a/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py b/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py index ff5183f02c..eede23c413 100644 --- a/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py +++ b/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py @@ -172,7 +172,7 @@ async def test_update_and_publish_event_airlock_request_updates_item(_, event_gr actual_updated_airlock_request = await update_and_publish_event_airlock_request( airlock_request=airlock_request_mock, airlock_request_repo=airlock_request_repo_mock, - user=create_test_user(), + updated_by=create_test_user(), new_status=AirlockRequestStatus.Submitted, workspace=sample_workspace()) @@ -197,7 +197,7 @@ async def test_update_and_publish_event_airlock_request_sends_status_changed_eve await update_and_publish_event_airlock_request( airlock_request=sample_airlock_request(), airlock_request_repo=airlock_request_repo_mock, - user=create_test_user(), + updated_by=create_test_user(), new_status=new_status, workspace=sample_workspace()) @@ -213,7 +213,7 @@ async def test_update_and_publish_event_airlock_request_raises_400_if_status_upd await update_and_publish_event_airlock_request( airlock_request=airlock_request_mock, airlock_request_repo=airlock_request_repo_mock, - user=create_test_user(), + updated_by=create_test_user(), new_status=AirlockRequestStatus.Approved, workspace=sample_workspace()) @@ -234,7 +234,7 @@ async def test_update_and_publish_event_airlock_request_raises_503_if_publish_ev await update_and_publish_event_airlock_request( airlock_request=airlock_request_mock, airlock_request_repo=airlock_request_repo_mock, - user=create_test_user(), + updated_by=create_test_user(), new_status=AirlockRequestStatus.Submitted, workspace=sample_workspace()) assert ex.value.status_code == status.HTTP_503_SERVICE_UNAVAILABLE @@ -250,7 +250,7 @@ async def test_update_and_publish_event_airlock_request_without_status_change_sh await update_and_publish_event_airlock_request( airlock_request=sample_airlock_request(), airlock_request_repo=airlock_request_repo_mock, - user=create_test_user(), + updated_by=create_test_user(), new_status=new_status, workspace=sample_workspace()) diff --git a/api_app/tests_ma/test_api/test_routes/test_migrations.py b/api_app/tests_ma/test_api/test_routes/test_migrations.py index a15ed66419..a3aa3a995f 100644 --- a/api_app/tests_ma/test_api/test_routes/test_migrations.py +++ b/api_app/tests_ma/test_api/test_routes/test_migrations.py @@ -40,9 +40,11 @@ def _prepare(self, app, admin_user): @ patch("api.routes.migrations.SharedServiceMigration.deleteDuplicatedSharedServices") @ patch("api.routes.migrations.WorkspaceMigration.moveAuthInformationToProperties") @ patch("api.routes.migrations.SharedServiceMigration.checkMinFirewallVersion") - async def test_post_migrations_returns_202_on_successful(self, check_min_firewall_version, workspace_migration, - shared_services_migration, rename_field, - add_deployment_field, _, logging, client, app): + @ patch("api.routes.migrations.AirlockMigration.add_created_by_and_rename_in_history") + @ patch("api.routes.migrations.AirlockMigration.rename_field_name") + async def test_post_migrations_returns_202_on_successful(self, airlock_rename_field, add_created_by_and_rename_in_history, + check_min_firewall_version, workspace_migration, shared_services_migration, + rename_field, add_deployment_field, _, logging, client, app): response = await client.post(app.url_path_for(strings.API_MIGRATE_DATABASE)) check_min_firewall_version.assert_called_once() @@ -50,6 +52,8 @@ async def test_post_migrations_returns_202_on_successful(self, check_min_firewal workspace_migration.assert_called_once() rename_field.assert_called() add_deployment_field.assert_called() + add_created_by_and_rename_in_history.assert_called_once() + airlock_rename_field.assert_called() logging.assert_called() assert response.status_code == status.HTTP_202_ACCEPTED @@ -58,6 +62,8 @@ async def test_post_migrations_returns_202_on_successful(self, check_min_firewal @ patch("api.routes.migrations.ResourceRepository.rename_field_name", side_effect=ValueError) @ patch("api.routes.migrations.SharedServiceMigration.deleteDuplicatedSharedServices") @ patch("api.routes.migrations.WorkspaceMigration.moveAuthInformationToProperties") - async def test_post_migrations_returns_400_if_template_does_not_exist(self, workspace_migration, shared_services_migration, resources_repo, logging, client, app): + @ patch("api.routes.migrations.AirlockMigration.add_created_by_and_rename_in_history") + async def test_post_migrations_returns_400_if_template_does_not_exist(self, workspace_migration, shared_services_migration, + resources_repo, airlock_migration, logging, client, app): response = await client.post(app.url_path_for(strings.API_MIGRATE_DATABASE)) assert response.status_code == status.HTTP_400_BAD_REQUEST From e59c8d232829ef24e1ff453cfb95c138cd3e100f Mon Sep 17 00:00:00 2001 From: jjgriff93 Date: Tue, 25 Oct 2022 15:34:43 +0000 Subject: [PATCH 06/13] user -> updated_by in tests --- .../test_repositories/test_airlock_request_repository.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py b/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py index deb80266cf..f102d01921 100644 --- a/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py +++ b/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py @@ -131,7 +131,7 @@ def test_update_airlock_request_should_retry_update_when_etag_is_not_up_to_date( expected_update_attempts = 2 user = create_test_user() mock_existing_request = airlock_request_mock(status=DRAFT) - airlock_request_repo.update_airlock_request(original_request=mock_existing_request, user=user, new_status=SUBMITTED) + airlock_request_repo.update_airlock_request(original_request=mock_existing_request, updated_by=user, new_status=SUBMITTED) assert update_airlock_request_item_mock.call_count == expected_update_attempts From b7b2fb2d025f6124b7d916a5ab209c74f780ed69 Mon Sep 17 00:00:00 2001 From: jjgriff93 Date: Tue, 25 Oct 2022 15:44:12 +0000 Subject: [PATCH 07/13] Set createdBy in new requests --- api_app/api/routes/airlock.py | 2 +- api_app/db/repositories/airlock_requests.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/api_app/api/routes/airlock.py b/api_app/api/routes/airlock.py index 28222cbd14..ab9d4a42ac 100644 --- a/api_app/api/routes/airlock.py +++ b/api_app/api/routes/airlock.py @@ -38,7 +38,7 @@ async def create_draft_request(airlock_request_input: AirlockRequestInCreate, us if workspace.properties.get("enable_airlock") is False: raise HTTPException(status_code=status.HTTP_405_METHOD_NOT_ALLOWED, detail=strings.AIRLOCK_NOT_ENABLED_IN_WORKSPACE) try: - airlock_request = airlock_request_repo.create_airlock_request_item(airlock_request_input, workspace.id) + airlock_request = airlock_request_repo.create_airlock_request_item(airlock_request_input, workspace.id, user) except (ValidationError, ValueError) as e: logging.error(f"Failed creating airlock request model instance: {e}") raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) diff --git a/api_app/db/repositories/airlock_requests.py b/api_app/db/repositories/airlock_requests.py index 612c6a1c69..77a3b5349f 100644 --- a/api_app/db/repositories/airlock_requests.py +++ b/api_app/db/repositories/airlock_requests.py @@ -85,7 +85,7 @@ def validate_status_update(self, current_status: AirlockRequestStatus, new_statu return approved_condition and rejected_condition and blocked_condition and (approved_in_progress_condition or rejected_in_progress_condition or blocking_in_progress_condition or draft_condition or submit_condition or in_review_condition or cancel_condition or failed_condition) - def create_airlock_request_item(self, airlock_request_input: AirlockRequestInCreate, workspace_id: str) -> AirlockRequest: + def create_airlock_request_item(self, airlock_request_input: AirlockRequestInCreate, workspace_id: str, user) -> AirlockRequest: full_airlock_request_id = str(uuid.uuid4()) resource_spec_parameters = {**self.get_airlock_request_spec_params()} @@ -96,6 +96,7 @@ def create_airlock_request_item(self, airlock_request_input: AirlockRequestInCre title=airlock_request_input.title, businessJustification=airlock_request_input.businessJustification, type=airlock_request_input.type, + createdBy=user, createdWhen=datetime.utcnow().timestamp(), properties=resource_spec_parameters, reviews=[] From 5921ee3c31968fe2f84203fbde35ff21a066bb84 Mon Sep 17 00:00:00 2001 From: jjgriff93 Date: Tue, 25 Oct 2022 16:07:24 +0000 Subject: [PATCH 08/13] Fix enrich requests --- api_app/api/routes/airlock_resource_helpers.py | 2 +- api_app/models/schemas/airlock_request.py | 6 +++++- ui/app/src/components/shared/airlock/Airlock.tsx | 2 ++ .../src/components/shared/airlock/AirlockViewRequest.tsx | 7 ++++++- 4 files changed, 14 insertions(+), 3 deletions(-) diff --git a/api_app/api/routes/airlock_resource_helpers.py b/api_app/api/routes/airlock_resource_helpers.py index b3ee8e448c..562b94404d 100644 --- a/api_app/api/routes/airlock_resource_helpers.py +++ b/api_app/api/routes/airlock_resource_helpers.py @@ -136,7 +136,7 @@ def enrich_requests_with_allowed_actions(requests: List[AirlockRequest], user: U enriched_requests = [] for request in requests: allowed_actions = get_allowed_actions(request, user, airlock_request_repo) - enriched_requests.append(AirlockRequestWithAllowedUserActions(airlockRequest=request, allowed_user_actions=allowed_actions)) + enriched_requests.append(AirlockRequestWithAllowedUserActions(airlockRequest=request, allowedUserActions=allowed_actions)) return enriched_requests diff --git a/api_app/models/schemas/airlock_request.py b/api_app/models/schemas/airlock_request.py index 11bcf4d942..c1145897f7 100644 --- a/api_app/models/schemas/airlock_request.py +++ b/api_app/models/schemas/airlock_request.py @@ -24,6 +24,10 @@ def get_sample_airlock_request(workspace_id: str, airlock_request_id: str) -> di "files": [], "title": "a request title", "businessJustification": "some business justification", + "createdBy": { + "id": "a user id", + "name": "a user name" + }, "createdWhen": datetime.utcnow().timestamp(), "reviews": [ get_sample_airlock_review("29990431-5451-40e7-a58a-02e2b7c3d7c8"), @@ -78,7 +82,7 @@ class AirlockRequestWithAllowedUserActionsInList(BaseModel): class Config: schema_extra = { "example": { - "airlock_requests": [ + "airlockRequests": [ get_sample_airlock_request_with_allowed_user_actions("933ad738-7265-4b5f-9eae-a1a62928772e"), get_sample_airlock_request_with_allowed_user_actions("933ad738-7265-4b5f-9eae-a1a62928772e") ] diff --git a/ui/app/src/components/shared/airlock/Airlock.tsx b/ui/app/src/components/shared/airlock/Airlock.tsx index 15d4c07ddd..33708e9f17 100644 --- a/ui/app/src/components/shared/airlock/Airlock.tsx +++ b/ui/app/src/components/shared/airlock/Airlock.tsx @@ -56,6 +56,8 @@ export const Airlock: React.FunctionComponent = () => { workspaceCtx.workspaceApplicationIdURI ); + console.log(result); + // Map the inner requests and the allowed user actions to state requests = result.airlockRequests.map((r: { airlockRequest: AirlockRequest, diff --git a/ui/app/src/components/shared/airlock/AirlockViewRequest.tsx b/ui/app/src/components/shared/airlock/AirlockViewRequest.tsx index 6ffeaafcd5..c57cf7f925 100644 --- a/ui/app/src/components/shared/airlock/AirlockViewRequest.tsx +++ b/ui/app/src/components/shared/airlock/AirlockViewRequest.tsx @@ -42,10 +42,15 @@ export const AirlockViewRequest: React.FunctionComponent setRequest(result.airlockRequest)); + ).then((result) => { + const request = result.airlockRequest as AirlockRequest; + request.allowedUserActions = result.allowedUserActions; + setRequest(request); + }); } else { setRequest(req); } + console.log(req); }, [apiCall, requestId, props.requests, workspaceCtx.workspace.id, workspaceCtx.workspaceApplicationIdURI]); // Retrieve a link to view/edit the airlock files From 3ab1a7dea9fd610d70673927c69d61adcaa1628d Mon Sep 17 00:00:00 2001 From: jjgriff93 Date: Tue, 25 Oct 2022 16:25:20 +0000 Subject: [PATCH 09/13] Fix create_airlock_request_item test --- .../test_repositories/test_airlock_request_repository.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py b/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py index f102d01921..ed75a108e1 100644 --- a/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py +++ b/api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py @@ -104,9 +104,11 @@ def test_get_airlock_request_by_id_raises_entity_does_not_exist_if_no_such_reque def test_create_airlock_request_item_creates_an_airlock_request_with_the_right_values(sample_airlock_request_input, airlock_request_repo): airlock_request_item_to_create = sample_airlock_request_input - airlock_request = airlock_request_repo.create_airlock_request_item(airlock_request_item_to_create, WORKSPACE_ID) + created_by_user = {'id': 'test_user_id'} + airlock_request = airlock_request_repo.create_airlock_request_item(airlock_request_item_to_create, WORKSPACE_ID, created_by_user) assert airlock_request.workspaceId == WORKSPACE_ID + assert airlock_request.createdBy['id'] == 'test_user_id' @pytest.mark.parametrize("current_status, new_status", get_allowed_status_changes()) From 8bd573849099369736f5ae7524805cbf86480339 Mon Sep 17 00:00:00 2001 From: jjgriff93 Date: Tue, 25 Oct 2022 21:36:34 +0000 Subject: [PATCH 10/13] Fix redeclare of status and formatting --- api_app/api/routes/airlock.py | 91 +++++++++++++++++++++++------------ 1 file changed, 61 insertions(+), 30 deletions(-) diff --git a/api_app/api/routes/airlock.py b/api_app/api/routes/airlock.py index ab9d4a42ac..fe99b21f2c 100644 --- a/api_app/api/routes/airlock.py +++ b/api_app/api/routes/airlock.py @@ -1,6 +1,6 @@ import logging -from fastapi import APIRouter, Depends, HTTPException, status, Response +from fastapi import APIRouter, Depends, HTTPException, status as status_code, Response from jsonschema.exceptions import ValidationError @@ -14,16 +14,19 @@ from api.dependencies.database import get_repository from api.dependencies.workspaces import get_workspace_by_id_from_path, get_deployed_workspace_by_id_from_path from api.dependencies.airlock import get_airlock_request_by_id_from_path -from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus, AirlockRequestType, AirlockReviewDecision, AirlockReviewUserResource +from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus, AirlockRequestType, AirlockReviewDecision, \ + AirlockReviewUserResource from models.schemas.operation import OperationInResponse from models.schemas.user_resource import UserResourceInCreate from models.schemas.airlock_request_url import AirlockRequestTokenInResponse -from models.schemas.airlock_request import AirlockRequestAndOperationInResponse, AirlockRequestInCreate, AirlockRequestInResponse, AirlockRequestWithAllowedUserActionsInList, AirlockReviewInCreate +from models.schemas.airlock_request import AirlockRequestAndOperationInResponse, AirlockRequestInCreate, AirlockRequestInResponse, \ + AirlockRequestWithAllowedUserActionsInList, AirlockReviewInCreate from resources import strings -from services.authentication import get_current_workspace_owner_or_researcher_user_or_airlock_manager, get_current_workspace_owner_or_researcher_user, get_current_airlock_manager_user +from services.authentication import get_current_workspace_owner_or_researcher_user_or_airlock_manager, \ + get_current_workspace_owner_or_researcher_user, get_current_airlock_manager_user -from .airlock_resource_helpers import save_and_publish_event_airlock_request, update_and_publish_event_airlock_request, enrich_requests_with_allowed_actions, \ - get_airlock_requests_by_user_and_workspace, delete_review_user_resources +from .airlock_resource_helpers import save_and_publish_event_airlock_request, update_and_publish_event_airlock_request, \ + enrich_requests_with_allowed_actions, get_airlock_requests_by_user_and_workspace, delete_review_user_resources from .resource_helpers import save_and_deploy_resource, construct_location_header from services.airlock import validate_user_allowed_to_access_storage_account, \ @@ -33,24 +36,29 @@ # airlock -@airlock_workspace_router.post("/workspaces/{workspace_id}/requests", status_code=status.HTTP_201_CREATED, response_model=AirlockRequestInResponse, name=strings.API_CREATE_AIRLOCK_REQUEST, dependencies=[Depends(get_current_workspace_owner_or_researcher_user), Depends(get_workspace_by_id_from_path)]) -async def create_draft_request(airlock_request_input: AirlockRequestInCreate, user=Depends(get_current_workspace_owner_or_researcher_user), airlock_request_repo=Depends(get_repository(AirlockRequestRepository)), workspace=Depends(get_deployed_workspace_by_id_from_path)) -> AirlockRequestInResponse: +@airlock_workspace_router.post("/workspaces/{workspace_id}/requests", status_code=status_code.HTTP_201_CREATED, + response_model=AirlockRequestInResponse, name=strings.API_CREATE_AIRLOCK_REQUEST, + dependencies=[Depends(get_current_workspace_owner_or_researcher_user), Depends(get_workspace_by_id_from_path)]) +async def create_draft_request(airlock_request_input: AirlockRequestInCreate, user=Depends(get_current_workspace_owner_or_researcher_user), + airlock_request_repo=Depends(get_repository(AirlockRequestRepository)), + workspace=Depends(get_deployed_workspace_by_id_from_path)) -> AirlockRequestInResponse: if workspace.properties.get("enable_airlock") is False: - raise HTTPException(status_code=status.HTTP_405_METHOD_NOT_ALLOWED, detail=strings.AIRLOCK_NOT_ENABLED_IN_WORKSPACE) + raise HTTPException(status_code=status_code.HTTP_405_METHOD_NOT_ALLOWED, detail=strings.AIRLOCK_NOT_ENABLED_IN_WORKSPACE) try: airlock_request = airlock_request_repo.create_airlock_request_item(airlock_request_input, workspace.id, user) except (ValidationError, ValueError) as e: logging.error(f"Failed creating airlock request model instance: {e}") - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) + raise HTTPException(status_code=status_code.HTTP_400_BAD_REQUEST, detail=str(e)) await save_and_publish_event_airlock_request(airlock_request, airlock_request_repo, user, workspace) return AirlockRequestInResponse(airlockRequest=airlock_request) @airlock_workspace_router.get("/workspaces/{workspace_id}/requests", - status_code=status.HTTP_200_OK, + status_code=status_code.HTTP_200_OK, response_model=AirlockRequestWithAllowedUserActionsInList, name=strings.API_LIST_AIRLOCK_REQUESTS, - dependencies=[Depends(get_current_workspace_owner_or_researcher_user_or_airlock_manager), Depends(get_workspace_by_id_from_path)]) + dependencies=[Depends(get_current_workspace_owner_or_researcher_user_or_airlock_manager), + Depends(get_workspace_by_id_from_path)]) async def get_all_airlock_requests_by_workspace( airlock_request_repo=Depends(get_repository(AirlockRequestRepository)), workspace=Depends(get_deployed_workspace_by_id_from_path), @@ -64,28 +72,45 @@ async def get_all_airlock_requests_by_workspace( airlock_requests_with_allowed_user_actions = enrich_requests_with_allowed_actions(airlock_requests, user, airlock_request_repo) except (ValidationError, ValueError) as e: logging.error(f"Failed retrieving all the airlock requests for a workspace: {e}") - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) + raise HTTPException(status_code=status_code.HTTP_400_BAD_REQUEST, detail=str(e)) return AirlockRequestWithAllowedUserActionsInList(airlockRequests=airlock_requests_with_allowed_user_actions) -@airlock_workspace_router.get("/workspaces/{workspace_id}/requests/{airlock_request_id}", status_code=status.HTTP_200_OK, response_model=AirlockRequestInResponse, name=strings.API_GET_AIRLOCK_REQUEST, dependencies=[Depends(get_current_workspace_owner_or_researcher_user), Depends(get_workspace_by_id_from_path)]) +@airlock_workspace_router.get("/workspaces/{workspace_id}/requests/{airlock_request_id}", status_code=status_code.HTTP_200_OK, + response_model=AirlockRequestInResponse, name=strings.API_GET_AIRLOCK_REQUEST, + dependencies=[Depends(get_current_workspace_owner_or_researcher_user), Depends(get_workspace_by_id_from_path)]) async def retrieve_airlock_request_by_id(airlock_request=Depends(get_airlock_request_by_id_from_path)) -> AirlockRequestInResponse: return AirlockRequestInResponse(airlockRequest=airlock_request) -@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/submit", status_code=status.HTTP_200_OK, response_model=AirlockRequestInResponse, name=strings.API_SUBMIT_AIRLOCK_REQUEST, dependencies=[Depends(get_current_workspace_owner_or_researcher_user), Depends(get_workspace_by_id_from_path)]) -async def create_submit_request(airlock_request=Depends(get_airlock_request_by_id_from_path), user=Depends(get_current_workspace_owner_or_researcher_user), airlock_request_repo=Depends(get_repository(AirlockRequestRepository)), workspace=Depends(get_workspace_by_id_from_path)) -> AirlockRequestInResponse: - updated_resource = await update_and_publish_event_airlock_request(airlock_request, airlock_request_repo, user, workspace, new_status=AirlockRequestStatus.Submitted) +@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/submit", status_code=status_code.HTTP_200_OK, + response_model=AirlockRequestInResponse, name=strings.API_SUBMIT_AIRLOCK_REQUEST, + dependencies=[Depends(get_current_workspace_owner_or_researcher_user), Depends(get_workspace_by_id_from_path)]) +async def create_submit_request(airlock_request=Depends(get_airlock_request_by_id_from_path), + user=Depends(get_current_workspace_owner_or_researcher_user), + airlock_request_repo=Depends(get_repository(AirlockRequestRepository)), + workspace=Depends(get_workspace_by_id_from_path)) -> AirlockRequestInResponse: + updated_resource = await update_and_publish_event_airlock_request(airlock_request, airlock_request_repo, user, workspace, + new_status=AirlockRequestStatus.Submitted) return AirlockRequestInResponse(airlockRequest=updated_resource) -@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/cancel", status_code=status.HTTP_200_OK, response_model=AirlockRequestInResponse, name=strings.API_CANCEL_AIRLOCK_REQUEST, dependencies=[Depends(get_current_workspace_owner_or_researcher_user), Depends(get_workspace_by_id_from_path)]) -async def create_cancel_request(airlock_request=Depends(get_airlock_request_by_id_from_path), user=Depends(get_current_workspace_owner_or_researcher_user), airlock_request_repo=Depends(get_repository(AirlockRequestRepository)), workspace=Depends(get_workspace_by_id_from_path)) -> AirlockRequestInResponse: - updated_resource = await update_and_publish_event_airlock_request(airlock_request, airlock_request_repo, user, workspace, new_status=AirlockRequestStatus.Cancelled) +@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/cancel", status_code=status_code.HTTP_200_OK, + response_model=AirlockRequestInResponse, name=strings.API_CANCEL_AIRLOCK_REQUEST, + dependencies=[Depends(get_current_workspace_owner_or_researcher_user), Depends(get_workspace_by_id_from_path)]) +async def create_cancel_request(airlock_request=Depends(get_airlock_request_by_id_from_path), + user=Depends(get_current_workspace_owner_or_researcher_user), + airlock_request_repo=Depends(get_repository(AirlockRequestRepository)), + workspace=Depends(get_workspace_by_id_from_path)) -> AirlockRequestInResponse: + updated_resource = await update_and_publish_event_airlock_request(airlock_request, airlock_request_repo, user, workspace, + new_status=AirlockRequestStatus.Cancelled) return AirlockRequestInResponse(airlockRequest=updated_resource) -@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/review-user-resource", status_code=status.HTTP_202_ACCEPTED, response_model=AirlockRequestAndOperationInResponse, name=strings.API_CREATE_AIRLOCK_REVIEW_USER_RESOURCE, dependencies=[Depends(get_current_airlock_manager_user), Depends(get_workspace_by_id_from_path)]) +@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/review-user-resource", + status_code=status_code.HTTP_202_ACCEPTED, response_model=AirlockRequestAndOperationInResponse, + name=strings.API_CREATE_AIRLOCK_REVIEW_USER_RESOURCE, + dependencies=[Depends(get_current_airlock_manager_user), Depends(get_workspace_by_id_from_path)]) async def create_review_user_resource( response: Response, airlock_request=Depends(get_airlock_request_by_id_from_path), @@ -98,7 +123,7 @@ async def create_review_user_resource( resource_template_repo=Depends(get_repository(ResourceTemplateRepository))) -> OperationInResponse: if airlock_request.status != AirlockRequestStatus.InReview: - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, + raise HTTPException(status_code=status_code.HTTP_400_BAD_REQUEST, detail="Airlock request must be in 'in_review' status to create a Review User Resource") try: @@ -116,7 +141,7 @@ async def create_review_user_resource( logging.info(f"Going to create a user resource in {workspace_id} {workspace_service_id} {user_resource_template_name}") except (KeyError, TypeError) as e: logging.error(f"Failed to parse configuration: {e}") - raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + raise HTTPException(status_code=status_code.HTTP_422_UNPROCESSABLE_ENTITY, detail=f"Failed to retrieve Airlock Review configuration for workspace {workspace.id}.\ Please ask your TRE administrator to check the configuration. Details: {str(e)}") @@ -125,7 +150,7 @@ async def create_review_user_resource( workspace_service = workspace_service_repo.get_workspace_service_by_id(workspace_id=workspace_id, service_id=workspace_service_id) except EntityDoesNotExist as e: logging.error(f"Failed to get workspace service {workspace_service_id} for workspace {workspace_id}: {str(e)}") - raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + raise HTTPException(status_code=status_code.HTTP_422_UNPROCESSABLE_ENTITY, detail=f"Failed to retrieve Airlock Review configuration for workspace {workspace.id}.\ Please ask your TRE administrator to check the configuration. Details: {str(e)}") @@ -148,12 +173,12 @@ async def create_review_user_resource( user_resource_create, workspace_id, workspace_service_id, workspace_service.templateName, user.id, user.roles) except (ValidationError, ValueError) as e: logging.error(f"Failed create user resource model instance due to validation error: {e}") - raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + raise HTTPException(status_code=status_code.HTTP_500_INTERNAL_SERVER_ERROR, detail=f"Invalid configuration for creating user resource. Please contact your TRE administrator. \ Details: {str(e)}") except UserNotAuthorizedToUseTemplate as e: logging.error(f"User not authorized to use template: {e}") - raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=str(e)) + raise HTTPException(status_code=status_code.HTTP_403_FORBIDDEN, detail=str(e)) operation = await save_and_deploy_resource( resource=user_resource, @@ -180,7 +205,10 @@ async def create_review_user_resource( return AirlockRequestAndOperationInResponse(airlockRequest=updated_resource, operation=operation) -@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/review", status_code=status.HTTP_200_OK, response_model=AirlockRequestInResponse, name=strings.API_REVIEW_AIRLOCK_REQUEST, dependencies=[Depends(get_current_airlock_manager_user), Depends(get_workspace_by_id_from_path)]) +@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/review", + status_code=status_code.HTTP_200_OK, response_model=AirlockRequestInResponse, + name=strings.API_REVIEW_AIRLOCK_REQUEST, dependencies=[Depends(get_current_airlock_manager_user), + Depends(get_workspace_by_id_from_path)]) async def create_airlock_review( airlock_review_input: AirlockReviewInCreate, airlock_request=Depends(get_airlock_request_by_id_from_path), @@ -196,14 +224,17 @@ async def create_airlock_review( airlock_review = airlock_request_repo.create_airlock_review_item(airlock_review_input, user) except (ValidationError, ValueError) as e: logging.error(f"Failed creating airlock review model instance: {e}") - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) + raise HTTPException(status_code=status_code.HTTP_400_BAD_REQUEST, detail=str(e)) # Store review with new status in cosmos, and send status_changed event if airlock_review.reviewDecision.value == AirlockReviewDecision.Approved: review_status = AirlockRequestStatus.ApprovalInProgress elif airlock_review.reviewDecision.value == AirlockReviewDecision.Rejected: review_status = AirlockRequestStatus.RejectionInProgress - updated_airlock_request = await update_and_publish_event_airlock_request(airlock_request=airlock_request, airlock_request_repo=airlock_request_repo, updated_by=user, workspace=workspace, new_status=review_status, airlock_review=airlock_review) + updated_airlock_request = await update_and_publish_event_airlock_request(airlock_request=airlock_request, + airlock_request_repo=airlock_request_repo, updated_by=user, + workspace=workspace, new_status=review_status, + airlock_review=airlock_review) # If there was a VM created for the request, clean it up as it will no longer be needed # In this request, we aren't returning the operations for clean up of VMs, @@ -228,7 +259,7 @@ def get_airlock_container_link(airlock_request: AirlockRequest, user, workspace) @airlock_workspace_router.get("/workspaces/{workspace_id}/requests/{airlock_request_id}/link", - status_code=status.HTTP_200_OK, response_model=AirlockRequestTokenInResponse, + status_code=status_code.HTTP_200_OK, response_model=AirlockRequestTokenInResponse, name=strings.API_AIRLOCK_REQUEST_LINK, dependencies=[Depends(get_current_workspace_owner_or_researcher_user_or_airlock_manager)]) async def get_airlock_container_link_method(workspace=Depends(get_deployed_workspace_by_id_from_path), From 8dc2bfd9198ee68ab0832d6ddd43cfb71e85842d Mon Sep 17 00:00:00 2001 From: jjgriff93 Date: Wed, 26 Oct 2022 14:23:42 +0000 Subject: [PATCH 11/13] Add breaking change to CHANGELOG --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd48dbb87a..d21b9fd809 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,15 @@ ## 0.6.1 (Unreleased) **BREAKING CHANGES & MIGRATIONS**: +* The airlock request object has changed. Make sure you have ran the db migration step after deploying the new API image and UI (which runs automatically in `make all`/`make tre-deploy` but can be manually invoked with `make db-migrate`) so that existing requests in your DB are migrated to the new model. +* Also the model for creating new airlock requests with the API has changed slightly; this is updated in the UI and CLI but if you have written custom tools ensure you are POSTing to `/requests` with the following model: +```json +{ + "type": "'import' or 'export'", + "title": "a request title", + "businessJustification": "some business justification" +} +``` FEATURES: * Display workspace and shared services total costs for admin role in UI [#2738](https://github.com/microsoft/AzureTRE/pull/2772) From 5d36515b7496c24885024a79dab5b248e31c040e Mon Sep 17 00:00:00 2001 From: jjgriff93 Date: Wed, 26 Oct 2022 14:25:38 +0000 Subject: [PATCH 12/13] break -> continue --- api_app/db/migrations/airlock.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api_app/db/migrations/airlock.py b/api_app/db/migrations/airlock.py index db5a8c3300..8bdd858bf3 100644 --- a/api_app/db/migrations/airlock.py +++ b/api_app/db/migrations/airlock.py @@ -11,7 +11,7 @@ def add_created_by_and_rename_in_history(self) -> int: for request in self.container.read_all_items(): # Only migrate if createdBy isn't present if 'createdBy' in request: - break + continue # For each request, check if it has history if len(request['history']) > 0: From 0e594f5568dbf89a55e5d2be4cd7bc76bb865e35 Mon Sep 17 00:00:00 2001 From: jjgriff93 Date: Wed, 26 Oct 2022 14:30:17 +0000 Subject: [PATCH 13/13] Fix missing updatedBy updatedWhen in create --- api_app/db/repositories/airlock_requests.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api_app/db/repositories/airlock_requests.py b/api_app/db/repositories/airlock_requests.py index 77a3b5349f..389e62801e 100644 --- a/api_app/db/repositories/airlock_requests.py +++ b/api_app/db/repositories/airlock_requests.py @@ -98,6 +98,8 @@ def create_airlock_request_item(self, airlock_request_input: AirlockRequestInCre type=airlock_request_input.type, createdBy=user, createdWhen=datetime.utcnow().timestamp(), + updatedBy=user, + updatedWhen=datetime.utcnow().timestamp(), properties=resource_spec_parameters, reviews=[] )