diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f64d42199..e49cbc7e96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ **BREAKING CHANGES & MIGRATIONS**: FEATURES: +* Added filtering and sorting to Airlock UI ([#2511](https://github.com/microsoft/AzureTRE/issues/2511)) ENHANCEMENTS: diff --git a/api_app/_version.py b/api_app/_version.py index 7d47b23d55..cd3616b2fc 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.4.51" +__version__ = "0.4.52" diff --git a/api_app/api/routes/airlock.py b/api_app/api/routes/airlock.py index 41b98653da..8985b22c89 100644 --- a/api_app/api/routes/airlock.py +++ b/api_app/api/routes/airlock.py @@ -47,10 +47,12 @@ 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, type: AirlockRequestType = None, status: AirlockRequestStatus = None, awaiting_current_user_review: bool = None) -> AirlockRequestWithAllowedUserActionsInList: + creator_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=type, status=status, awaiting_current_user_review=awaiting_current_user_review) + creator_user_id=creator_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: logging.error(f"Failed retrieving all the airlock requests for a workspace: {e}") diff --git a/api_app/api/routes/airlock_resource_helpers.py b/api_app/api/routes/airlock_resource_helpers.py index e5b1fd5e4a..9ee9caa141 100644 --- a/api_app/api/routes/airlock_resource_helpers.py +++ b/api_app/api/routes/airlock_resource_helpers.py @@ -83,13 +83,10 @@ 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, awaiting_current_user_review: bool = None) -> List[AirlockRequest]: - if awaiting_current_user_review: - if "AirlockManager" not in user.roles: - return [] - status = AirlockRequestStatus.InReview - - return airlock_request_repo.get_airlock_requests(workspace_id=workspace.id, user_id=creator_user_id, type=type, status=status) + 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, user_id=creator_user_id, type=type, status=status, + order_by=order_by, order_ascending=order_ascending) def get_allowed_actions(request: AirlockRequest, user: User, airlock_request_repo: AirlockRequestRepository) -> AirlockRequestWithAllowedUserActions: diff --git a/api_app/db/repositories/airlock_requests.py b/api_app/db/repositories/airlock_requests.py index 770db85ec4..8ff6eb8242 100644 --- a/api_app/db/repositories/airlock_requests.py +++ b/api_app/db/repositories/airlock_requests.py @@ -101,7 +101,7 @@ 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) -> List[AirlockRequest]: + 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}"' # optional filters @@ -112,6 +112,11 @@ def get_airlock_requests(self, workspace_id: str, user_id: str = None, type: Air if type: query += ' AND c.requestType=@type' + # optional sorting + if order_by: + query += ' ORDER BY c.' + order_by + query += ' ASC' if order_ascending else ' DESC' + parameters = [ {"name": "@user_id", "value": user_id}, {"name": "@status", "value": status}, 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 ec69646c92..73b4982f74 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 @@ -258,30 +258,16 @@ async def test_update_and_publish_event_airlock_request_without_status_change_sh assert send_airlock_notification_event_mock.call_count == 0 -async def test_get_airlock_requests_by_user_and_workspace_with_awaiting_current_user_review_and_status_arguments_should_ignore_status(airlock_request_repo_mock): +async def test_get_airlock_requests_by_user_and_workspace_with_status_filter_calls_repo(airlock_request_repo_mock): workspace = sample_workspace() user = create_workspace_airlock_manager_user() airlock_request_repo_mock.get_airlock_requests = MagicMock() get_airlock_requests_by_user_and_workspace(user=user, workspace=workspace, airlock_request_repo=airlock_request_repo_mock, - status=AirlockRequestStatus.Approved, awaiting_current_user_review=True) + status=AirlockRequestStatus.InReview) - airlock_request_repo_mock.get_airlock_requests.assert_called_once_with(workspace_id=workspace.id, user_id=None, type=None, status=AirlockRequestStatus.InReview) - - -async def test_get_airlock_requests_by_user_and_workspace_with_awaiting_current_user_review_argument_by_non_airlock_manger_should_return_empty_list(airlock_request_repo_mock): - user = create_test_user() - airlock_requests = get_airlock_requests_by_user_and_workspace(user=user, workspace=sample_workspace(), airlock_request_repo=airlock_request_repo_mock, awaiting_current_user_review=True) - assert airlock_requests == [] - - -@pytest.mark.parametrize("role", get_required_roles(endpoint=create_airlock_review)) -async def test_get_airlock_requests_by_user_and_workspace_with_awaiting_current_user_review_argument_requires_same_roles_as_review_endpoint(role, airlock_request_repo_mock): - airlock_request_repo_mock.get_airlock_requests = MagicMock() - user = create_test_user() - user.roles = [role] - get_airlock_requests_by_user_and_workspace(user=user, workspace=sample_workspace(), airlock_request_repo=airlock_request_repo_mock, awaiting_current_user_review=True) - airlock_request_repo_mock.get_airlock_requests.assert_called_once() + airlock_request_repo_mock.get_airlock_requests.assert_called_once_with(workspace_id=workspace.id, user_id=None, type=None, + status=AirlockRequestStatus.InReview, order_by=None, order_ascending=True) @pytest.mark.parametrize("action, required_roles, airlock_request_repo_mock", [ diff --git a/ui/app/src/components/shared/airlock/Airlock.tsx b/ui/app/src/components/shared/airlock/Airlock.tsx index 16e5d0de88..c3d1fb1382 100644 --- a/ui/app/src/components/shared/airlock/Airlock.tsx +++ b/ui/app/src/components/shared/airlock/Airlock.tsx @@ -1,9 +1,9 @@ import React, { useCallback, useContext, useEffect, useState } from 'react'; -import { CommandBarButton, DetailsList, getTheme, IColumn, Persona, PersonaSize, SelectionMode, Spinner, SpinnerSize, Stack } from '@fluentui/react'; +import { ColumnActionsMode, CommandBar, CommandBarButton, ContextualMenu, DirectionalHint, getTheme, IColumn, ICommandBarItemProps, IContextualMenuItem, IContextualMenuProps, Label, Persona, PersonaSize, SelectionMode, ShimmeredDetailsList, Stack } from '@fluentui/react'; import { HttpMethod, useAuthApiCall } from '../../../hooks/useAuthApiCall'; import { ApiEndpoint } from '../../../models/apiEndpoints'; import { WorkspaceContext } from '../../../contexts/WorkspaceContext'; -import { AirlockRequest, AirlockRequestAction } from '../../../models/airlock'; +import { AirlockRequest, AirlockRequestAction, AirlockRequestStatus, AirlockRequestType } from '../../../models/airlock'; import moment from 'moment'; import { Route, Routes, useNavigate } from 'react-router-dom'; import { AirlockViewRequest } from './AirlockViewRequest'; @@ -11,93 +11,142 @@ import { LoadingState } from '../../../models/loadingState'; import { APIError } from '../../../models/exceptions'; import { ExceptionLayout } from '../ExceptionLayout'; import { AirlockNewRequest } from './AirlockNewRequest'; +import { WorkspaceRoleName } from '../../../models/roleNames'; +import { useAccount, useMsal } from '@azure/msal-react'; -interface AirlockProps { -} - -export const Airlock: React.FunctionComponent = (props: AirlockProps) => { +export const Airlock: React.FunctionComponent = () => { const [airlockRequests, setAirlockRequests] = useState([] as AirlockRequest[]); const [requestColumns, setRequestColumns] = useState([] as IColumn[]); - const [orderBy, setOrderBy] = useState(); + const [orderBy, setOrderBy] = useState('updatedWhen'); + const [orderAscending, setOrderAscending] = useState(false); + const [filters, setFilters] = useState(new Map()); const [loadingState, setLoadingState] = useState(LoadingState.Loading); + const [contextMenuProps, setContextMenuProps] = useState(); + const [apiError, setApiError] = useState(); const workspaceCtx = useContext(WorkspaceContext); const apiCall = useAuthApiCall(); - const [apiError, setApiError] = useState({} as APIError); const theme = getTheme(); const navigate = useNavigate(); + const { instance, accounts } = useMsal(); + const account = useAccount(accounts[0] || {}); - useEffect(() => { - const getAirlockRequests = async () => { + // Get the airlock request data from API + const getAirlockRequests = useCallback(async () => { + setApiError(undefined); + setLoadingState(LoadingState.Loading); + + try { let requests: AirlockRequest[]; + if (workspaceCtx.workspace) { - try { - if (workspaceCtx.workspace) { - const result = await apiCall( - `${ApiEndpoint.Workspaces}/${workspaceCtx.workspace.id}/${ApiEndpoint.AirlockRequests}`, - HttpMethod.Get, - workspaceCtx.workspaceApplicationIdURI - ); - requests = result.airlockRequests.map((r: { - airlockRequest: AirlockRequest, - allowed_user_actions: Array - }) => { - const request = r.airlockRequest; - request.allowed_user_actions = r.allowed_user_actions; - return request; - }); - } else { - // TODO: Get all requests across workspaces - requests = []; + // Add any selected filters and orderBy + let query = '?'; + filters.forEach((value, key) => { + query += `${key}=${value}&`; + }); + if (orderBy) { + query += `order_by=${orderBy}&order_ascending=${orderAscending}&`; } - // Order by updatedWhen for initial view - requests.sort((a, b) => a.updatedWhen < b.updatedWhen ? 1 : -1); - setAirlockRequests(requests); - setLoadingState(LoadingState.Ok); - } catch (err: any) { - err.userMessage = 'Error fetching airlock requests'; - setApiError(err); - setLoadingState(LoadingState.Error); + + // Call the Airlock requests API + const result = await apiCall( + `${ApiEndpoint.Workspaces}/${workspaceCtx.workspace.id}/${ApiEndpoint.AirlockRequests}${query.slice(0, -1)}`, + HttpMethod.Get, + workspaceCtx.workspaceApplicationIdURI + ); + + // Map the inner requests and the allowed user actions to state + requests = result.airlockRequests.map((r: { + airlockRequest: AirlockRequest, + allowed_user_actions: Array + }) => { + const request = r.airlockRequest; + request.allowed_user_actions = r.allowed_user_actions; + return request; + }); + } else { + // TODO: Get all requests across workspaces + requests = []; } + + setAirlockRequests(requests); + setLoadingState(LoadingState.Ok); + } catch (err: any) { + err.userMessage = 'Error fetching airlock requests'; + setApiError(err); + setLoadingState(LoadingState.Error); } + }, [apiCall, workspaceCtx.workspace, workspaceCtx.workspaceApplicationIdURI, filters, orderBy, orderAscending]); + + // Fetch new requests on first load and whenever filters/orderBy selection changes + useEffect(() => { getAirlockRequests(); - }, [apiCall, workspaceCtx.workspace, workspaceCtx.workspaceApplicationIdURI]); + }, [filters, orderBy, orderAscending, getAirlockRequests]); - // Order data by selected column - const reorderRequests = useCallback((orderByColumn, invert = true) => { - if (orderByColumn) { - setOrderBy(orderByColumn); - // Reset sorting on other columns and invert selected column if already sorted asc/desc - setRequestColumns(columns => { - const orderedColumns: IColumn[] = columns.slice(); - const selectedColumn: IColumn = orderedColumns.filter(selCol => orderByColumn.key === selCol.key)[0]; - orderedColumns.forEach((newCol: IColumn) => { - if (newCol === selectedColumn) { - if (invert) selectedColumn.isSortedDescending = !selectedColumn.isSortedDescending; - selectedColumn.isSorted = true; - } else { - newCol.isSorted = false; - newCol.isSortedDescending = true; - } - }); - return orderedColumns; - }); + const orderRequests = (column: IColumn) => { + setOrderBy((o) => { + // If already selected, invert ordering + if (o === column.key) { + setOrderAscending((previous) => !previous); + return column.key; + } + return column.key; + }); + }; - // Re-order airlock requests - setAirlockRequests(requests => { - const key = orderByColumn.fieldName! as keyof AirlockRequest; - return requests - .slice(0) - .sort((a: AirlockRequest, b: AirlockRequest) => ( - (orderByColumn.isSortedDescending ? a[key] < b[key] : a[key] > b[key]) ? 1 : -1) - ); - }); - } - }, []); + // Open a context menu in the requests list for filtering and sorting + const openContextMenu = useCallback((column: IColumn, ev: React.MouseEvent, options: Array) => { + const filterOptions = options.map(option => { + return { + key: option, + name: option, + canCheck: true, + checked: filters?.has(column.key) && filters.get(column.key) === option, + onClick: () => { + // Set filter or unset if already selected + setFilters((f) => { + if (f.get(column.key) === option) { + f.delete(column.key); + } else { + f.set(column.key, option); + } + // Return as a new map to trigger re-rendering + return new Map(f); + }); + } + } + }); + + const items: IContextualMenuItem[] = [ + { + key: 'sort', + name: 'Sort', + iconProps: { iconName: 'Sort' }, + onClick: () => orderRequests(column) + }, + { + key: 'filter', + name: 'Filter', + iconProps: { iconName: 'Filter' }, + subMenuProps: { + items: filterOptions, + } + } + ]; + + setContextMenuProps({ + items: items, + target: ev.currentTarget as HTMLElement, + directionalHint: DirectionalHint.bottomCenter, + gapSpace: 0, + onDismiss: () => setContextMenuProps(undefined), + }); + }, [filters]); // Set the columns on initial render useEffect(() => { const orderByColumn = (ev: React.MouseEvent, column: IColumn) => { - reorderRequests(column); + orderRequests(column); }; const columns: IColumn[] = [ @@ -112,24 +161,31 @@ export const Airlock: React.FunctionComponent = (props: AirlockPro } }, { - key: 'initiator', + key: 'creator_user_id', name: 'Initiator', ariaLabel: 'Creator of the airlock request', minWidth: 150, maxWidth: 200, isResizable: true, + fieldName: 'initiator', onRender: (request: AirlockRequest) => request.user?.name, - onColumnClick: orderByColumn + isFiltered: filters.has('creator_user_id') }, { - key: 'type', + key: 'requestType', name: 'Type', ariaLabel: 'Whether the request is import or export', minWidth: 70, maxWidth: 100, isResizable: true, fieldName: 'requestType', - onColumnClick: orderByColumn + columnActionsMode: ColumnActionsMode.hasDropdown, + isSorted: orderBy === 'requestType', + 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') }, { key: 'status', @@ -138,31 +194,39 @@ export const Airlock: React.FunctionComponent = (props: AirlockPro minWidth: 70, isResizable: true, fieldName: 'status', - onColumnClick: orderByColumn + columnActionsMode: ColumnActionsMode.hasDropdown, + isSorted: orderBy === 'status', + isSortedDescending: !orderAscending, + onColumnClick: (ev, column) => openContextMenu(column, ev, Object.values(AirlockRequestStatus)), + onColumnContextMenu: (column, ev) => + (column && ev) && openContextMenu(column, ev, Object.values(AirlockRequestStatus)), + isFiltered: filters.has('status') }, { - key: 'created', + key: 'createdTime', name: 'Created', ariaLabel: 'When the request was created', minWidth: 120, data: 'number', isResizable: true, fieldName: 'createdTime', + isSorted: orderBy === 'createdTime', + isSortedDescending: !orderAscending, onRender: (request: AirlockRequest) => { return { moment.unix(request.creationTime).format('DD/MM/YYYY') }; }, onColumnClick: orderByColumn }, { - key: 'updated', + key: 'updatedWhen', name: 'Updated', ariaLabel: 'When the request was last updated', minWidth: 120, data: 'number', isResizable: true, fieldName: 'updatedWhen', - isSorted: false, - isSortedDescending: true, + isSorted: orderBy === 'updatedWhen', + isSortedDescending: !orderAscending, onRender: (request: AirlockRequest) => { return { moment.unix(request.updatedWhen).fromNow() }; }, @@ -170,67 +234,63 @@ export const Airlock: React.FunctionComponent = (props: AirlockPro } ]; setRequestColumns(columns); - reorderRequests(columns[5]); - }, [reorderRequests]); + }, [openContextMenu, filters, orderAscending, orderBy]); - let requestsList; - switch (loadingState) { - case LoadingState.Ok: - if (airlockRequests.length > 0) { - requestsList = ( - item.id} - onItemInvoked={(item) => navigate(item.id)} - className="tre-table-rows-align-centre" - /> - ); - } else { - requestsList = ( -
-

No requests found

- Looks like there are no airlock requests yet. Create a new request to get started. -
- ) + const handleNewRequest = async (newRequest: AirlockRequest) => { + await getAirlockRequests(); + navigate(newRequest.id); + }; + + const quickFilters: ICommandBarItemProps[] = [ + { + key: 'reset', + text: 'Clear filters', + iconProps: { iconName: 'ClearFilter' }, + onClick: () => setFilters(new Map()) + } + ]; + + // If we can access the user's msal account, give option to filter by their user id + if (account) { + quickFilters.unshift({ + key: 'myRequests', + text: 'My requests', + iconProps: { iconName: 'EditContact' }, + onClick: () => { + const userId = account.localAccountId.split('.')[0]; + setFilters((f) => { + f.set('creator_user_id', userId); + return new Map(f); + }); } - break; - case LoadingState.Error: - requestsList = ( - - ); break; - default: - requestsList = ( -
- -
- ); break; + }); } - const updateRequest = (updatedRequest: AirlockRequest) => { - setAirlockRequests(requests => { - const i = requests.findIndex(r => r.id === updatedRequest.id); - const updatedRequests = [...requests]; - updatedRequests[i] = updatedRequest; - return updatedRequests; + // Only show "Awaiting my review" filter if user in airlock manager role + if (workspaceCtx.roles?.includes(WorkspaceRoleName.AirlockManager)) { + quickFilters.unshift({ + key: 'awaitingMyReview', + text: 'Awaiting my review', + iconProps: { iconName: 'TemporaryUser' }, + onClick: () => { + setFilters((f) => { + // Currently we don't have assigned reviewers so this will be all requests in review status + f.set('status', 'in_review'); + return new Map(f); + }); + } }); - // Trigger re-ordering to ensure request appears in the right order - reorderRequests(orderBy, false); - }; - - const handleNewRequest = (newRequest: AirlockRequest) => { - setAirlockRequests(requests => [...requests, newRequest]); - reorderRequests(orderBy, false); - navigate(newRequest.id); - }; + } return ( <> -

Airlock

+

Airlock

+ + + = (props: AirlockPro - + { + apiError && + }
- { requestsList } + item?.id} + onItemInvoked={(item) => navigate(item.id)} + className="tre-table-rows-align-centre" + enableShimmer={loadingState === LoadingState.Loading} + /> + { + contextMenuProps && + } + { + airlockRequests.length === 0 && loadingState !== LoadingState.Loading &&
+

No requests found

+ { + filters.size > 0 + ? There are no requests matching your selected filter(s). + : Looks like there are no airlock requests yet. Create a new request to get started. + } +
+ }
@@ -250,11 +333,10 @@ export const Airlock: React.FunctionComponent = (props: AirlockPro } /> + } /> ); - }; diff --git a/ui/app/src/components/shared/airlock/AirlockViewRequest.tsx b/ui/app/src/components/shared/airlock/AirlockViewRequest.tsx index 3e00e49682..90bc4c3675 100644 --- a/ui/app/src/components/shared/airlock/AirlockViewRequest.tsx +++ b/ui/app/src/components/shared/airlock/AirlockViewRequest.tsx @@ -33,9 +33,19 @@ export const AirlockViewRequest: React.FunctionComponent { // Get the selected request from the router param and find in the requests prop - const req = props.requests.find(r => r.id === requestId) as AirlockRequest; - setRequest(req); - }, [requestId, props.requests]); + let req = props.requests.find(r => r.id === requestId) as AirlockRequest; + + // If not found, fetch it from the API + if (!req) { + apiCall( + `${ApiEndpoint.Workspaces}/${workspaceCtx.workspace.id}/${ApiEndpoint.AirlockRequests}/${requestId}`, + HttpMethod.Get, + workspaceCtx.workspaceApplicationIdURI + ).then((result) => setRequest(result.airlockRequest)); + } else { + setRequest(req); + } + }, [apiCall, requestId, props.requests]); const generateFilesLink = useCallback(async () => { // Retrieve a link to view/edit the airlock files @@ -141,8 +151,8 @@ export const AirlockViewRequest: React.FunctionComponent } { - request.errorMessage &&
- {request.errorMessage} + request.statusMessage &&
+ {request.statusMessage}
}
@@ -179,6 +189,15 @@ export const AirlockViewRequest: React.FunctionComponent { request ? <> + + Id + + +

{request.id}

+ + + + Initiator @@ -242,7 +261,6 @@ export const AirlockViewRequest: React.FunctionComponent{request.businessJustification}

 - { !AirlockFilesLinkInvalidStatus.includes(request.status) && <> @@ -252,7 +270,11 @@ export const AirlockViewRequest: React.FunctionComponent - Generate a storage container SAS URL to view/modify the request file(s). + { + request.status === AirlockRequestStatus.Draft + ? Generate a storage container SAS URL to upload your request file. + : Generate a storage container SAS URL to view the request file. + } @@ -266,6 +288,11 @@ export const AirlockViewRequest: React.FunctionComponent + { + request.status === AirlockRequestStatus.Draft && + Please upload a single file. Only single-file imports (including zip files) are supported. + + } { filesLinkError && } @@ -327,7 +354,7 @@ export const AirlockViewRequest: React.FunctionComponent setHideSubmitDialog(true)} dialogContentProps={{ title: 'Submit request?', - subText: 'Make sure you have uploaded your file(s) to the request\'s storage account before submitting.', + subText: 'Make sure you have uploaded your file to the request\'s storage account before submitting.', }} > { @@ -387,7 +414,7 @@ export const AirlockViewRequest: React.FunctionComponent + ? : ; businessJustification: string; - errorMessage: null | string; + statusMessage: null | string; status: AirlockRequestStatus; creationTime: number; reviews: Array;