Move retract endpoint to reviewing router (#477)

Author: PGijsbers

src/routers/resource_router.py

@@ -142,14 +142,6 @@ def create(self, url_prefix: str) -> APIRouter:
             description=f"Submit a {self.resource_name} for review.",
             **default_kwargs,
         )
-        router.add_api_route(
-            path=f"{url_prefix}/{self.resource_name_plural}/retract/{version}/{{identifier}}",
-            methods={"POST"},
-            endpoint=self.get_retract_func(),
-            name=self.resource_name,
-            description=f"Retract a {self.resource_name}, setting its status to 'draft'.",
-            **default_kwargs,
-        )
         router.add_api_route(
             path=f"{url_prefix}/{self.resource_name_plural}/{version}",
             methods={"POST"},
@@ -567,55 +559,6 @@ def submit_resource(
 
         return submit_resource
 
-    def get_retract_func(self):
-        """Return a function that can be used to retract a single resource."""
-
-        def retract_resource(
-            identifier: str,
-            user: KeycloakUser = Depends(get_user_or_raise),
-        ):
-            with DbSession() as session:
-                resource = self._retrieve_resource(identifier=identifier, session=session)  # type: ignore
-
-                if not user_can_administer(user, resource.aiod_entry):
-                    # Could choose to instead give same error as if resource does not exist.
-                    msg = (
-                        f"You do not have permission to retract {self.resource_name} {identifier}."
-                    )
-                    raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=msg)
-
-                query = (
-                    select(Submission)
-                    .where(
-                        Submission.aiod_entry_identifier == resource.aiod_entry.identifier,
-                    )
-                    .order_by(Submission.request_date.desc())  # type: ignore [attr-defined]
-                )
-                current_request = session.scalars(query).first()
-                if current_request is None or not current_request.is_pending:
-                    raise HTTPException(
-                        status_code=status.HTTP_400_BAD_REQUEST,
-                        detail="Cannot retract this asset, as it is not under review.",
-                    )
-
-                retraction = Review(
-                    decision=Decision.RETRACTED,
-                    reviewer_identifier=user._subject_identifier,
-                    submission_identifier=current_request.identifier,
-                )
-                resource.aiod_entry.status = EntryStatus.DRAFT
-                session.add(retraction)
-                session.commit()
-                return self._wrap_with_headers(
-                    {
-                        "review_identifier": retraction.identifier,
-                        "submission_identifier": current_request.identifier,
-                        "decision": retraction.decision,
-                    }
-                )
-
-        return retract_resource
-
     def _retrieve_resource(
         self,
         session: Session,

src/routers/review_router.py

@@ -24,6 +24,12 @@ def create(url_prefix: str) -> APIRouter:
     router = APIRouter()
     version = "v1"
 
+    router.post(
+        f"{url_prefix}/submissions/retract/{version}/{{submission_identifier}}",
+        tags=["Reviewing"],
+        description="Retract an asset under review, setting its status to 'draft'.",
+    )(retract_submission)
+
     router.get(
         f"{url_prefix}/submissions/{version}/",
         tags=["Reviewing"],
@@ -64,27 +70,27 @@ def _get_single_submission(
 ) -> Submission | None:
     with DbSession() as session:
         has_review = select(1).where(Submission.identifier == Review.submission_identifier).exists()
-        query = select(Submission).where(~has_review)
-
+        submissions = select(Submission).where(~has_review)
         if which == ListMode.NEWEST:
-            query = query.order_by(Submission.request_date.desc())  # type: ignore[attr-defined]
+            submissions = submissions.order_by(Submission.request_date.desc())  # type: ignore[attr-defined]
         if from_requestee is not None:
-            query = query.where(Submission.requestee_identifier == from_requestee)
+            submissions = submissions.where(Submission.requestee_identifier == from_requestee)
 
-        return session.scalars(query).first()
+        return session.scalars(submissions).first()
 
 
 def _get_submissions_by_state(
     *,
-    which: Literal[ListMode.COMPLETED, ListMode.PENDING],
+    which: Literal[ListMode.COMPLETED, ListMode.PENDING, ListMode.ALL],
     from_requestee: str | None = None,
 ) -> Sequence[Submission]:
     with DbSession() as session:
         has_review = select(1).where(Submission.identifier == Review.submission_identifier).exists()
+        submissions = select(Submission)
         if which == ListMode.PENDING:
-            submissions = select(Submission).where(~has_review)
+            submissions = submissions.where(~has_review)
         if which == ListMode.COMPLETED:
-            submissions = select(Submission).where(has_review)
+            submissions = submissions.where(has_review)
         if from_requestee is not None:
             submissions = submissions.where(Submission.requestee_identifier == from_requestee)
         return session.scalars(submissions).all()
@@ -98,7 +104,7 @@ def list_submissions(
     if mode in [ListMode.NEWEST, ListMode.OLDEST]:
         submission = _get_single_submission(which=mode, from_requestee=user_filter)  # type: ignore[arg-type]
         return [submission] if submission else []
-    if mode in [ListMode.PENDING, ListMode.COMPLETED]:
+    if mode in [ListMode.PENDING, ListMode.COMPLETED, ListMode.ALL]:
         return _get_submissions_by_state(which=mode, from_requestee=user_filter)  # type: ignore[arg-type]
     raise ValueError(f"`mode` should be one of {ListMode!r} but is {mode!r}.")
 
@@ -108,8 +114,7 @@ def get_submission(
     user: KeycloakUser = Depends(get_user_or_raise),
     session: Session = Depends(get_session),
 ) -> Submission:
-    query = select(Submission).where(Submission.identifier == identifier)
-    submission = session.scalars(query).first()
+    submission = session.get(Submission, identifier)
     if not submission:
         raise HTTPException(
             status_code=HTTPStatus.NOT_FOUND,
@@ -169,3 +174,35 @@ def _review_resource(
     session.add(review)
     session.commit()
     return review
+
+
+def retract_submission(
+    submission_identifier: str,
+    user: KeycloakUser = Depends(get_user_or_raise),
+):
+    with DbSession() as session:
+        submission = session.get(Submission, submission_identifier)
+        if not user_can_administer(user, submission.asset.aiod_entry):
+            # Could choose to instead give same error as if resource does not exist.
+            msg = f"You do not have permission to retract {submission.asset_type} {submission.asset.identifier}."
+            raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=msg)
+
+        if submission is None or not submission.is_pending:
+            raise HTTPException(
+                status_code=status.HTTP_400_BAD_REQUEST,
+                detail="Cannot retract this asset, as it is not under review.",
+            )
+
+        retraction = Review(
+            decision=Decision.RETRACTED,
+            reviewer_identifier=user._subject_identifier,
+            submission_identifier=submission.identifier,
+        )
+        submission.asset.aiod_entry.status = EntryStatus.DRAFT
+        session.add(retraction)
+        session.commit()
+        return {
+            "review_identifier": retraction.identifier,
+            "submission_identifier": submission.identifier,
+            "decision": retraction.decision,
+        }

src/tests/authorization/test_authorization.py

@@ -206,10 +206,13 @@ def test_unknown_submission_raises_404(client):
     [
         (REVIEWER, ListMode.PENDING, [2, 3], "Reviewer can see all pending reviews."),
         (REVIEWER, ListMode.COMPLETED, [1], "Reviewer can see all completed reviews."),
+        (REVIEWER, ListMode.ALL, [1,2,3], "Reviewer can see all reviews."),
         (ALICE, ListMode.PENDING, [2], "Alice has one pending submission and can not see Bob's."),
         (ALICE, ListMode.COMPLETED, [1], "Alice only has one completed submission."),
+        (ALICE, ListMode.ALL, [1,2], "Alice can see all her reviews, but not Bob's."),
         (BOB, ListMode.PENDING, [3], "Bob has one pending submission and can not see Alice's."),
         (BOB, ListMode.COMPLETED, [], "Bob has no completed submission."),
+        (BOB, ListMode.ALL, [3], "Bob can see all his reviews, but not Alice's."),
     ]
 )
 def test_submission_by_state_respects_privacy(user: KeycloakUser, mode: ListMode, assets: list[int], reason: str, client: TestClient, publication):
@@ -271,10 +274,10 @@ def test_retrieving_single_submission_works(user: KeycloakUser, mode: ListMode,
 
 
 def test_user_can_retract_assets(client, publication):
-    identifier = register_asset(publication, owner=ALICE, status=EntryStatus.SUBMITTED)
+    register_asset(publication, owner=ALICE, status=EntryStatus.SUBMITTED)
     with logged_in_user(ALICE):
         response = client.post(
-            f"/publications/retract/v1/{identifier}", headers={"Authorization": "Fake token"}
+            f"/submissions/retract/v1/1", headers={"Authorization": "Fake token"}
         )
         assert "review_identifier" in response.json()
         assert Decision.RETRACTED == response.json()["decision"]
@@ -292,7 +295,7 @@ def test_other_user_can_not_retract_assets(client, publication):
 
     with logged_in_user(BOB):
         response = client.post(
-            f"/publications/retract/v1/{identifier}", headers={"Authorization": "Fake token"}
+            f"/submissions/retract/v1/{identifier}", headers={"Authorization": "Fake token"}
         )
         assert response.status_code == HTTPStatus.FORBIDDEN, response.json()