Prevent submitting a release request with empty context & controls (#517

)

* Add sample context & controls to test setup
* Add context & controls to e2e test
* Add context & controls to existing request tests
* Add context & controls to business logic tests
* Prevent submitting a request if context & controls are empty
* Add a test to verify submit doesn't work if missing context&controls

airlock/business_logic.py

@@ -1270,6 +1270,9 @@ class WorkspacePermissionDenied(APIException):
     class RequestPermissionDenied(APIException):
         pass
 
+    class IncompleteContextOrControls(RequestPermissionDenied):
+        pass
+
     class RequestReviewDenied(APIException):
         pass
 
@@ -1560,6 +1563,16 @@ def check_status(
                 f"cannot change status from {release_request.status.name} to {to_status.name}"
             )
 
+        if to_status == RequestStatus.SUBMITTED:
+            for filegroup in release_request.filegroups:
+                if (
+                    release_request.filegroups[filegroup].context == ""
+                    or release_request.filegroups[filegroup].controls == ""
+                ):
+                    raise self.IncompleteContextOrControls(
+                        f"Incomplete context and/or controls for filegroup '{filegroup}'"
+                    )
+
         # check permissions
         owner = release_request.status_owner()
         # author transitions

airlock/views/request.py

@@ -322,6 +322,9 @@ def request_submit(request, request_id):
     try:
         bll.submit_request(release_request, request.user)
     except bll.RequestPermissionDenied as exc:
+        if isinstance(exc, bll.IncompleteContextOrControls):
+            messages.error(request, str(exc))
+            return redirect(release_request.get_url())
         raise PermissionDenied(str(exc))
 
     messages.success(request, "Request has been submitted")

tests/factories.py

@@ -325,6 +325,15 @@ def create_request_at_status(
         add_file(request)
 
     request = refresh_release_request(request)
+
+    # if we add files, we should add context & controls
+    if files:
+        for filegroup in request.filegroups:
+            dummy_context = "This is some testing context"
+            dummy_controls = "I got rid of all the small numbers"
+            bll.group_edit(request, filegroup, dummy_context, dummy_controls, author)
+        request = refresh_release_request(request)
+
     if status == RequestStatus.PENDING:
         return request
 

tests/functional/test_e2e.py

@@ -321,6 +321,21 @@ def test_e2e_release_files(
         "src", release_request.get_contents_url(UrlPath("my-new-group/subdir/file.txt"))
     )
 
+    # Add context & controls to the filegroup
+    filegroup_link = page.get_by_role("link").locator(".filegroup:scope")
+    find_and_click(filegroup_link)
+
+    context_input = page.locator("#id_context")
+    find_and_click(context_input)
+    context_input.fill("some context")
+
+    controls_input = page.locator("#id_controls")
+    find_and_click(controls_input)
+    controls_input.fill("some controls")
+
+    save_button = page.locator("#edit-group-button")
+    find_and_click(save_button)
+
     # Submit request
     submit_button = page.locator("button[data-modal=submitRequest]")
     find_and_click(submit_button)

tests/integration/views/test_request.py

@@ -605,6 +605,9 @@ def test_request_submit_author(airlock_client):
         "test1", user=airlock_client.user
     )
     factories.write_request_file(release_request, "group", "path/test.txt")
+    bll.group_edit(
+        release_request, "group", "my context", "my controls", airlock_client.user
+    )
 
     response = airlock_client.post(f"/requests/submit/{release_request.id}")
 
@@ -620,6 +623,7 @@ def test_request_submit_not_author(airlock_client):
         "test1", user=other_author, status=RequestStatus.PENDING
     )
     factories.write_request_file(release_request, "group", "path/test.txt")
+    bll.group_edit(release_request, "group", "my context", "my controls", other_author)
 
     response = airlock_client.post(f"/requests/submit/{release_request.id}")
 
@@ -628,6 +632,21 @@ def test_request_submit_not_author(airlock_client):
     assert persisted_request.status == RequestStatus.PENDING
 
 
+def test_request_submit_missing_context_controls(airlock_client):
+    airlock_client.login(workspaces=["test1"])
+    release_request = factories.create_release_request(
+        "test1", user=airlock_client.user
+    )
+    factories.write_request_file(release_request, "group", "path/test.txt")
+
+    response = airlock_client.post(f"/requests/submit/{release_request.id}")
+
+    assert response.status_code == 302
+    persisted_request = bll.get_release_request(release_request.id, airlock_client.user)
+    # request has not been submitted
+    assert persisted_request.status == RequestStatus.PENDING
+
+
 def test_request_withdraw_author(airlock_client):
     airlock_client.login(workspaces=["test1"])
     release_request = factories.create_release_request(

tests/unit/test_business_logic.py

@@ -604,6 +604,8 @@ def test_provider_request_release_files(mock_old_api, mock_notifications, bll, f
         AuditEventType.REQUEST_FILE_ADD,
         AuditEventType.REQUEST_FILE_ADD,
         AuditEventType.REQUEST_FILE_ADD,
+        # add default context & controls
+        AuditEventType.REQUEST_EDIT,
         # submit request
         AuditEventType.REQUEST_SUBMIT,
         # checker reviews
@@ -617,11 +619,11 @@ def test_provider_request_release_files(mock_old_api, mock_notifications, bll, f
     ]
     assert [log.type for log in audit_log] == expected_audit_logs
 
-    checker_review_log = audit_log[5]
-    checker1_review_log = audit_log[6]
-    approve_log = audit_log[7]
-    release_file_log = audit_log[8]
-    release_log = audit_log[9]
+    checker_review_log = audit_log[6]
+    checker1_review_log = audit_log[7]
+    approve_log = audit_log[8]
+    release_file_log = audit_log[9]
+    release_log = audit_log[10]
 
     assert checker_review_log.type == AuditEventType.REQUEST_REVIEW
     assert checker_review_log.user == checker.username
@@ -1328,6 +1330,7 @@ def test_notification_error(bll, notifications_stubber, caplog):
     factories.write_request_file(
         release_request, "group", "test/file.txt", approved=True
     )
+    bll.group_edit(release_request, "group", "foo", "bar", author)
     release_request = factories.refresh_release_request(release_request)
     bll.set_status(release_request, RequestStatus.SUBMITTED, author)
     notifications_responses = parse_notification_responses(mock_notifications)
@@ -1415,6 +1418,8 @@ def test_submit_request(bll, mock_notifications):
         "workspace", user=author, status=RequestStatus.PENDING
     )
     factories.write_request_file(release_request, "group", "test/file.txt")
+    bll.group_edit(release_request, "group", "foo", "bar", author)
+    release_request = bll.get_release_request(release_request.id, author)
     bll.submit_request(release_request, author)
     assert release_request.status == RequestStatus.SUBMITTED
     assert_last_notification(mock_notifications, "request_submitted")
@@ -2383,6 +2388,9 @@ def test_approve_file_not_your_own(bll):
     release_request, path, author = setup_empty_release_request()
 
     bll.add_file_to_request(release_request, path, author)
+    bll.group_edit(release_request, "default", "foo", "bar", author)
+    release_request = bll.get_release_request(release_request.id, author)
+
     bll.set_status(
         release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author
     )
@@ -2401,6 +2409,9 @@ def test_approve_file_not_checker(bll):
     author2 = factories.create_user("author2", [], False)
 
     bll.add_file_to_request(release_request, path, author)
+    bll.group_edit(release_request, "default", "foo", "bar", author)
+    release_request = bll.get_release_request(release_request.id, author)
+
     bll.set_status(
         release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author
     )