Merge pull request #85 from opensafely-core/split-requests

Split API for authored requests vs requests to review

Author: bloodearnest

airlock/api.py

@@ -224,8 +224,12 @@ def get_current_request(
         """
         raise NotImplementedError()
 
-    def get_requests_for_user(self, user: User) -> list[ReleaseRequest]:
-        """Get all current requests authored by user"""
+    def get_requests_authored_by_user(self, user: User) -> list[ReleaseRequest]:
+        """Get all current requests authored by user."""
+        raise NotImplementedError()
+
+    def get_outstanding_requests_for_review(self, user: User):
+        """Get all request that need review."""
         raise NotImplementedError()
 
     VALID_STATE_TRANSITIONS = {

airlock/templates/requests.html

@@ -1,11 +1,23 @@
 {% extends "base.html" %}
 
 {% block content %}
-{% #card title="Requests for "|add:request.user.username %}
+
+{% #card title="Requests by "|add:request.user.username %}
   {% #list_group %}
-    {% for request in requests %} 
-    {% #list_group_item href=request.get_absolute_url %}{{ request.id }}{% /list_group_item %}
+    {% for request in authored_requests %} 
+    {% #list_group_item href=request.get_absolute_url %}{{ request.workspace }}: {{ request.status.name }}{% /list_group_item %}
     {% endfor %}
   {% /list_group %}
 {% /card %}
+
+{% if request.user.output_checker %}
+{% #card title="Outstanding requests awaiting review" %}
+  {% #list_group %}
+    {% for request in outstanding_requests %} 
+    {% #list_group_item href=request.get_absolute_url %}{{ request.workspace }} by {{ request.author }}{% /list_group_item %}
+    {% endfor %}
+  {% /list_group %}
+{% /card %}
+{% endif %}
+
 {% endblock content %}

airlock/views.py

@@ -161,8 +161,20 @@ def workspace_add_file_to_request(request, workspace_name):
 
 
 def request_index(request):
-    requests = api.get_requests_for_user(request.user)
-    return TemplateResponse(request, "requests.html", {"requests": requests})
+    authored_requests = api.get_requests_authored_by_user(request.user)
+
+    outstanding_requests = []
+    if request.user.output_checker:
+        outstanding_requests = api.get_outstanding_requests_for_review(request.user)
+
+    return TemplateResponse(
+        request,
+        "requests.html",
+        {
+            "authored_requests": authored_requests,
+            "outstanding_requests": outstanding_requests,
+        },
+    )
 
 
 def request_view(request, request_id: str, path: str = ""):

local_db/api.py

@@ -60,17 +60,30 @@ def get_current_request(self, workspace: str, user: User, create=False):
                 author=user.username,
             )
 
-    def get_requests_for_user(self, user: User):
+    def get_requests_authored_by_user(self, user: User):
+        requests = []
+
+        for metadata in RequestMetadata.objects.filter(author=user.username).order_by(
+            "status"
+        ):
+            # to create a request, user *must* have explicit workspace
+            # permissions - being an output checker is not enough
+            if metadata.workspace in user.workspaces:
+                requests.append(self._request(metadata))
+
+        return requests
+
+    def get_outstanding_requests_for_review(self, user: User):
         requests = []
-        filter_args = {}
 
-        # if not output checker, can only see your own requests
         if not user.output_checker:
-            filter_args = {"author": user.username}
+            return []
 
-        for metadata in RequestMetadata.objects.filter(**filter_args):
-            if user.output_checker or metadata.workspace in user.workspaces:
+        for metadata in RequestMetadata.objects.filter(status=Status.SUBMITTED):
+            # do not show output_checker their own requests
+            if metadata.author != user.username:
                 requests.append(self._request(metadata))
+
         return requests
 
     def set_status(self, request: ReleaseRequest, status: Status, user: User):

tests/integration/test_views.py

@@ -248,17 +248,30 @@ def test_request_index_user_permitted_requests(client_with_user):
     user = User.from_session(permitted_client.session)
     release_request = factories.create_release_request("test1", user)
     response = permitted_client.get("/requests/")
-    request_ids = {r.id for r in response.context["requests"]}
-    assert request_ids == {release_request.id}
+    authored_ids = {r.id for r in response.context["authored_requests"]}
+    outstanding_ids = {r.id for r in response.context["outstanding_requests"]}
+    assert authored_ids == {release_request.id}
+    assert outstanding_ids == set()
 
 
 def test_request_index_user_output_checker(client_with_user):
-    r1 = factories.create_release_request("test1")
-    r2 = factories.create_release_request("test2")
-    permitted_client = client_with_user({"workspaces": [], "output_checker": True})
+    permitted_client = client_with_user(
+        {"workspaces": ["test_workspace"], "output_checker": True}
+    )
+    user = User.from_session(permitted_client.session)
+    other = User(1, "other")
+    r1 = factories.create_release_request(
+        "test_workspace", user=user, status=Status.SUBMITTED
+    )
+    r2 = factories.create_release_request(
+        "other_workspace", user=other, status=Status.SUBMITTED
+    )
     response = permitted_client.get("/requests/")
-    request_ids = {r.id for r in response.context["requests"]}
-    assert request_ids == {r1.id, r2.id}
+
+    authored_ids = {r.id for r in response.context["authored_requests"]}
+    outstanding_ids = {r.id for r in response.context["outstanding_requests"]}
+    assert authored_ids == {r1.id}
+    assert outstanding_ids == {r2.id}
 
 
 def test_request_submit_author(client_with_user):

tests/unit/test_api.py

@@ -101,29 +101,25 @@ def test_provider_request_release_files(mock_old_api):
     [
         ([], False, []),
         (["allowed"], False, ["r1"]),
-        (
-            [],
-            True,
-            [
-                "r1",
-                "r2",
-                "r3",
-            ],
-        ),
+        # output checkers can't create requests unless explit permission for workspace
+        ([], True, []),
+        (["allowed"], True, ["r1"]),
         (["allowed", "notexist"], False, ["r1"]),
-        (["notexist", "notexist"], False, []),
+        (["notexist"], False, []),
         (["no-request-dir", "notexist"], False, []),
     ],
 )
-def test_provider_get_requests_for_user(workspaces, output_checker, expected, api):
+def test_provider_get_requests_authored_by_user(
+    workspaces, output_checker, expected, api
+):
     user = User(1, "test", workspaces, output_checker)
     other_user = User(1, "other", [], False)
     factories.create_release_request("allowed", user, id="r1")
     factories.create_release_request("allowed", other_user, id="r2")
     factories.create_release_request("not-allowed", user, id="r3")
     factories.create_workspace("no-request-dir")
 
-    assert set(r.id for r in api.get_requests_for_user(user)) == set(expected)
+    assert set(r.id for r in api.get_requests_authored_by_user(user)) == set(expected)
 
 
 def test_provider_get_current_request_for_user(api):