Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip sending step result events for Airlock updates that do not include a status change #2547

Merged
merged 9 commits into from
Sep 7, 2022
2 changes: 1 addition & 1 deletion api_app/_version.py
Original file line number Diff line number Diff line change
@@ -1 +1 @@
__version__ = "0.4.25"
__version__ = "0.4.26"
4 changes: 4 additions & 0 deletions api_app/api/routes/airlock_resource_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ async def update_and_publish_event_airlock_request(airlock_request: AirlockReque
raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=strings.AIRLOCK_REQUEST_ILLEGAL_STATUS_CHANGE)
raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=strings.STATE_STORE_ENDPOINT_NOT_RESPONDING)

if not new_status:
logging.debug(f"Skipping sending 'status changed' event for airlock request item: {airlock_request.id} - there is no status change")
return updated_airlock_request

try:
logging.debug(f"Sending status changed event for airlock request item: {airlock_request.id}")
await send_status_changed_event(updated_airlock_request)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,24 @@ async def test_update_and_publish_event_airlock_request_updates_item(_, event_gr
assert actual_airlock_notification_event.data == airlock_notification_event_mock.data


@patch("api.routes.airlock_resource_helpers.send_status_changed_event")
@patch("api.routes.airlock_resource_helpers.send_airlock_notification_event")
@patch("services.aad_authentication.AzureADAuthorization.get_workspace_role_assignment_details")
async def test_update_and_publish_event_airlock_request_sends_status_changed_event(_, send_airlock_notification_event_mock, send_status_changed_event_mock, airlock_request_repo_mock):
new_status = AirlockRequestStatus.Submitted
airlock_request_repo_mock.update_airlock_request = MagicMock()

await update_and_publish_event_airlock_request(
airlock_request=sample_airlock_request(),
airlock_request_repo=airlock_request_repo_mock,
user=create_test_user(),
new_status=new_status,
workspace=sample_workspace())

assert send_status_changed_event_mock.call_count == 1
assert send_airlock_notification_event_mock.call_count == 1


@patch("services.aad_authentication.AzureADAuthorization.get_workspace_role_assignment_details", return_value={"WorkspaceResearcher": ["researcher@outlook.com"], "WorkspaceOwner": ["owner@outlook.com"], "AirlockManager": ["manager@outlook.com"]})
async def test_update_and_publish_event_airlock_request_raises_400_if_status_update_invalid(_, airlock_request_repo_mock):
airlock_request_mock = sample_airlock_request()
Expand Down Expand Up @@ -222,6 +240,24 @@ async def test_update_and_publish_event_airlock_request_raises_503_if_publish_ev
assert ex.value.status_code == status.HTTP_503_SERVICE_UNAVAILABLE


@patch("api.routes.airlock_resource_helpers.send_status_changed_event")
@patch("api.routes.airlock_resource_helpers.send_airlock_notification_event")
@patch("services.aad_authentication.AzureADAuthorization.get_workspace_role_assignment_details")
async def test_update_and_publish_event_airlock_request_without_status_change_should_not_send_status_changed_event(_, send_airlock_notification_event_mock, send_status_changed_event_mock, airlock_request_repo_mock):
new_status = None
airlock_request_repo_mock.update_airlock_request = MagicMock()

await update_and_publish_event_airlock_request(
airlock_request=sample_airlock_request(),
airlock_request_repo=airlock_request_repo_mock,
user=create_test_user(),
new_status=new_status,
workspace=sample_workspace())

assert send_status_changed_event_mock.call_count == 0
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):
workspace = sample_workspace()
user = create_workspace_airlock_manager_user()
Expand Down
6 changes: 5 additions & 1 deletion e2e_tests/airlock/request.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,12 @@ async def wait_for_status(
200,
)
current_status = request_result[strings.AIRLOCK_REQUEST][strings.AIRLOCK_REQUEST_STATUS]
if (current_status == request_status):
if (current_status == request_status or is_final_status(current_status)):
break

LOGGER.info(f"Waiting for request status: {request_status}, current status is {current_status}")
await asyncio.sleep(2)


def is_final_status(status):
return status in [strings.APPROVED_STATUS, strings.REJECTED_STATUS, strings.CANCELLED_STATUS, strings.BLOCKED_STATUS, strings.FAILED_STATUS]
4 changes: 4 additions & 0 deletions e2e_tests/airlock/strings.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
SUBMITTED_STATUS = "submitted"
IN_REVIEW_STATUS = "in_review"
APPROVED_STATUS = "approved"
REJECTED_STATUS = "rejected"
CANCELLED_STATUS = "cancelled"
BLOCKED_STATUS = "blocked_by_scan"
FAILED_STATUS = "failed"

AIRLOCK_REQUEST = "airlockRequest"
AIRLOCK_REQUEST_STATUS = "status"