Feat/reviewqueue (#451)

Author: PGijsbers

docs/developer/schema/index.md

@@ -19,6 +19,9 @@ following figure:
 
 ![AIoD Metadata model](../../media/AIoD_Metadata_Model.drawio.png)
 
+In addition to the implementation of the metadata schema, we maintain some metadata about that metadata.
+Specifically, a system which stores users, permissions, and reviews that relate to the metadata assets uploaded to AIoD.
+More on this can be found in the ["User Model"](../users.md) documentation.
 
 ## Reading the Conceptual Metadata Schema
 Tools and documentation on how to read the conceptual metadata model are currently being written.

docs/developer/users.md

@@ -0,0 +1,43 @@
+# User Model
+
+The user model in AIoD allows users to maintain and share ownership over assets, 
+and to allow a review process for new assets.
+The components of this user model are defined in [src/database/authorization.py](https://github.com/aiondemand/AIOD-rest-api/blob/develop/src/database/authorization.py)
+and [src/database/review.py](https://github.com/aiondemand/AIOD-rest-api/blob/develop/src/database/review.py).
+
+[//]: # (Add a diagram overview once the model is more final, e.g., groups are added)
+
+## Users
+User credentials are kept in keycloak or by identity providers, in any case the user is uniquely identified
+by the unique "sub" key that keycloak provides in its token.
+In our user model we keep as little information about the user as we can.
+This avoids a scenario were data is kept twice and may desynchronize, and also respects the users' privacy.
+Currently, that is only the "sub", but in the future we may consider adding e.g., usernames.
+
+## Permissions
+A user may have certain permissions for an asset, these are: 
+ 
+ - read: a user may retrieve this metadata asset.
+ - write: a user may modify this metadata asset.
+ - admin: a user may delete this metadata asset, submit it for review, and grant other users permissions for the asset.
+
+The only notion of "ownership" over a particular metadata asset is having administrator rights.
+When user A uploads an asset and assigns user B administrator rights, they have completely equal rights.
+There is no special privilege for user A, and this also means that e.g., user B may remove administrator rights from user A.
+
+## Reviews
+An asset uploaded by a user is by default in `draft` state.
+The user may request the asset to be `published` by submitting it for review through the REST API.
+To do this, they submit the identifier of the asset to the `ASSET_TYPE/submit/v1/{identifier}` endpoint.
+When it is submitted for review, reviewers will be able to see the request and make a decision.
+As long as no decision has been made, the user may retract their submission from review using the `ASSET_TYPE/retract/v1/{identifier}`
+endpoint.
+Each asset may only have one concurrent review request, and the asset may not be modified while it is under review.
+
+Reviewers can find pending submissions using the `submissions/v1` endpoint,
+and get information on a particular submission with the `submissions/v1/{identifier}` endpoint, 
+where the identifier denotes the identifier of the submission (i.e., review request).
+Reviewers can submit a review using the `ASSET_TYPE/review/v1/{identifier}` endpoint.
+
+If the submission is accepted, the asset will be published without further action from the original uploader.
+If the submission is rejected, the asset will move back to `draft` state and may be submitted again.

mkdocs.yaml

@@ -17,6 +17,7 @@ nav:
   - 'Developer Resources':
       - developer/index.md
       - 'Authentication': developer/authentication.md
+      - 'User Model': developer/users.md
       - 'Metadata Schema':
           - developer/schema/index.md
           - 'Attributes': developer/schema/attributes.md

src/database/authorization.py

@@ -65,11 +65,14 @@ def register_user(kc_user: KeycloakUser, session: Session) -> User:
 
 
 def add_administrator(user: KeycloakUser, resource: AIoDConcept, session: Session):
-    permission = Permission(
-        type_=PermissionType.ADMIN,
-        user_identifier=user._subject_identifier,
-        aiod_entry=resource.aiod_entry,
-    )
+    query = select(Permission).where(User.subject_identifier == user._subject_identifier)
+    permission = session.scalars(query).first()
+    if permission is None:
+        permission = Permission(
+            user_identifier=user._subject_identifier,
+            aiod_entry=resource.aiod_entry,
+        )
+    permission.type_ = PermissionType.ADMIN
     session.add(permission)
 
 

src/database/review.py

@@ -30,6 +30,7 @@ class Review(SQLModel, table=True):  # type: ignore [call-arg]
 
     reviewer_identifier: str = Field(
         foreign_key="user.subject_identifier",
+        exclude=True,
     )
     submission_identifier: int = Field(
         foreign_key="submission.identifier",
@@ -56,7 +57,9 @@ class Submission(SQLModel, table=True):  # type: ignore [call-arg]
     # We do not want the review to be deleted when the original requestee is deleted,
     # there could be e.g., shared ownership in which case the review data should be preserved.
     requestee_identifier: str | None = Field(
-        foreign_key="user.subject_identifier", ondelete="SET NULL"
+        foreign_key="user.subject_identifier",
+        ondelete="SET NULL",
+        exclude=True,
     )
 
     # On the other hand, if the entry corresponding to the thing it reviews is removed,

src/main.py

@@ -24,8 +24,14 @@
 from database.session import EngineSingleton, DbSession
 from database.setup import create_database, database_exists
 from error_handling import http_exception_handler
-from routers import resource_routers, parent_routers, enum_routers, uploader_routers
-from routers import search_routers
+from routers import (
+    resource_routers,
+    parent_routers,
+    enum_routers,
+    uploader_routers,
+    search_routers,
+    review_router,
+)
 from setup_logger import setup_logger
 
 
@@ -95,6 +101,7 @@ def counts() -> dict:
         + enum_routers.router_list
         + search_routers.router_list
         + uploader_routers.router_list
+        + [review_router]
     ):
         app.include_router(router.create(url_prefix))
 

src/routers/review_router.py

@@ -0,0 +1,83 @@
+import enum
+from http import HTTPStatus
+from typing import Sequence, Literal
+
+from fastapi import APIRouter, HTTPException
+from sqlmodel import select
+
+from database.session import DbSession
+from database.review import Submission, Review
+
+
+def create(url_prefix: str) -> APIRouter:
+    router = APIRouter()
+    version = "v1"
+
+    router.get(
+        f"{url_prefix}/submissions/{version}/",
+        tags=["Reviewing"],
+        description="List all assets submitted for review.",
+    )(list_submissions)
+
+    router.get(
+        f"{url_prefix}/submissions/{version}/{{identifier}}",
+        tags=["Reviewing"],
+        description="Retrieve a specific submission.",
+    )(get_submission)
+
+    # Add MiddleWare which requires authentication as reviewer role
+    return router
+
+
+class ListMode(enum.StrEnum):
+    OLDEST = enum.auto()
+    NEWEST = enum.auto()
+    ALL = enum.auto()
+    PENDING = enum.auto()
+    COMPLETED = enum.auto()
+
+
+def _get_single_submission(
+    *, which: Literal[ListMode.NEWEST, ListMode.OLDEST]
+) -> Submission | None:
+    with DbSession() as session:
+        has_review = select(1).where(Submission.identifier == Review.submission_identifier).exists()
+        order = Submission.request_date
+        if which == ListMode.NEWEST:
+            order = Submission.request_date.desc()  # type: ignore[attr-defined]
+        query = select(Submission).order_by(order).where(~has_review)
+        return session.scalars(query).first()
+
+
+def _get_submissions_by_state(
+    *, which: Literal[ListMode.COMPLETED, ListMode.PENDING]
+) -> Sequence[Submission]:
+    with DbSession() as session:
+        has_review = select(1).where(Submission.identifier == Review.submission_identifier).exists()
+        if which == ListMode.PENDING:
+            submissions = select(Submission).where(~has_review)
+        if which == ListMode.COMPLETED:
+            submissions = select(Submission).where(has_review)
+        return session.scalars(submissions).all()
+
+
+def list_submissions(mode: ListMode = ListMode.NEWEST) -> Sequence[Submission]:
+    # mypy does not do type narrowing properly: https://github.com/python/mypy/issues/12535
+    if mode in [ListMode.NEWEST, ListMode.OLDEST]:
+        submission = _get_single_submission(which=mode)  # type: ignore[arg-type]
+        return [submission] if submission else []
+    if mode in [ListMode.PENDING, ListMode.COMPLETED]:
+        return _get_submissions_by_state(which=mode)  # type: ignore[arg-type]
+    raise ValueError(f"`mode` should be one of {ListMode!r} but is {mode!r}.")
+
+
+def get_submission(identifier: int) -> Submission:
+    with DbSession() as session:
+        query = select(Submission).where(Submission.identifier == identifier)
+        submission = session.scalars(query).first()
+    if submission:
+        return submission
+    raise HTTPException(
+        status_code=HTTPStatus.NOT_FOUND,
+        detail=f"No submission with identifier {identifier} found.",
+    )

src/tests/authorization/test_authorization.py

@@ -13,7 +13,7 @@
 from database.model.concept.concept import AIoDConcept
 from database.review import Review, Decision, ReviewCreate, Submission
 from database.session import DbSession
-
+from routers.review_router import ListMode
 
 ALICE = KeycloakUser("Alice", {"edit_aiod_resources"}, "alice-sub")
 BOB = KeycloakUser("Bob", {"edit_aiod_resources"}, "bob-sub")
@@ -98,6 +98,12 @@ def test_user_can_submit_draft_for_review(client, publication):
         assert submission.status_code == HTTPStatus.OK, submission.json()
         assert "submission_identifier" in submission.json()
 
+    with logged_in_user(REVIEWER):
+        queue = client.get("/submissions/v1", headers={"Authorization": "Fake token"})
+        assert queue.status_code == HTTPStatus.OK, queue.json()
+        assert len(queue.json()) == 1, "A successful request should result in a submission."
+        assert "requestee_identifier" not in queue.json()
+
 
 def test_user_can_not_submit_other_for_review(client, publication):
     identifier = register_asset(publication, owner=ALICE, status=EntryStatus.DRAFT)
@@ -109,6 +115,102 @@ def test_user_can_not_submit_other_for_review(client, publication):
         )
         assert submission.status_code == HTTPStatus.FORBIDDEN, submission.json()
 
+    with logged_in_user(REVIEWER):
+        queue = client.get("/submissions/v1", headers={"Authorization": "Fake token"})
+        assert queue.status_code == HTTPStatus.OK, queue.json()
+        assert len(queue.json()) == 0, "A rejected request should not result in a submission."
+
+
+def test_a_draft_is_not_pending_for_review(client, publication):
+    register_asset(publication, owner=ALICE, status=EntryStatus.DRAFT)
+
+    with logged_in_user(REVIEWER):
+        queue = client.get("/submissions/v1", headers={"Authorization": "Fake token"})
+        assert queue.status_code == HTTPStatus.OK, queue.json()
+        assert len(queue.json()) == 0, "An asset is only pending for review after submission."
+
+
+def test_a_submitted_asset_is_pending_for_review(client, publication):
+    register_asset(publication, owner=ALICE, status=EntryStatus.SUBMITTED)
+
+    with logged_in_user(REVIEWER):
+        queue = client.get("/submissions/v1", headers={"Authorization": "Fake token"})
+        assert queue.status_code == HTTPStatus.OK, queue.json()
+        assert len(queue.json()) == 1, "A submitted asset should be pending until a review is done."
+
+
+def test_get_submission_by_id(client, publication):
+    register_asset(publication, owner=ALICE, status=EntryStatus.PUBLISHED)
+
+    with logged_in_user(REVIEWER):
+        queue = client.get("/submissions/v1/1", headers={"Authorization": "Fake token"})
+        assert queue.status_code == HTTPStatus.OK, queue.json()
+        assert queue.json()["identifier"] == 1
+
+
+def test_unknown_submission_raises_404(client):
+    with logged_in_user(REVIEWER):
+        queue = client.get("/submissions/v1/1", headers={"Authorization": "Fake token"})
+        assert queue.status_code == HTTPStatus.NOT_FOUND
+
+
+@pytest.mark.skip("Requiring reviewer role to be added through middleware")
+def test_getting_submission_requires_review_role(client):
+    queue = client.get("/submissions/v1/1")
+    assert queue.status_code == HTTPStatus.UNAUTHORIZED
+
+    with logged_in_user(ALICE):
+        queue = client.get("/submissions/v1/1", headers={"Authorization": "Fake token"})
+        assert queue.status_code == HTTPStatus.FORBIDDEN
+
+
+def test_an_published_asset_is_not_pending_for_review(client, publication):
+    register_asset(publication, owner=ALICE, status=EntryStatus.PUBLISHED)
+
+    with logged_in_user(REVIEWER):
+        queue = client.get(
+            f"/submissions/v1?mode={ListMode.PENDING}", headers={"Authorization": "Fake token"}
+        )
+        assert queue.status_code == HTTPStatus.OK, queue.json()
+        assert (
+            len(queue.json()) == 0
+        ), "After publication, the submission is no longer pending a review."
+
+        queue = client.get(
+            f"/submissions/v1?mode={ListMode.COMPLETED}", headers={"Authorization": "Fake token"}
+        )
+        assert queue.status_code == HTTPStatus.OK, queue.json()
+        assert len(queue.json()) == 1, "After publication, the review request is completed."
+
+
+def test_retrieving_single_submission_works(client, publication_factory):
+    publication = publication_factory()
+    oldest = publication_factory()
+    oldest.platform_resource_identifier = "OLDEST"
+    newest = publication_factory()
+    newest.platform_resource_identifier = "NEWEST"
+
+    register_asset(publication, owner=ALICE, status=EntryStatus.PUBLISHED)
+    register_asset(oldest, owner=ALICE, status=EntryStatus.SUBMITTED)
+    register_asset(newest, owner=ALICE, status=EntryStatus.SUBMITTED)
+
+    with logged_in_user(REVIEWER):
+        queue = client.get(
+            f"/submissions/v1?mode={ListMode.OLDEST}", headers={"Authorization": "Fake token"}
+        )
+        assert queue.status_code == HTTPStatus.OK, queue.json()
+        assert (
+            queue.json()[0]["aiod_entry_identifier"] == 2
+        ), "The oldest pending submission is the second one."
+
+        queue = client.get(
+            f"/submissions/v1?mode={ListMode.NEWEST}", headers={"Authorization": "Fake token"}
+        )
+        assert queue.status_code == HTTPStatus.OK, queue.json()
+        assert (
+            queue.json()[0]["aiod_entry_identifier"] == 3
+        ), "The newest pending submission is the third one."
+
 
 def test_user_can_retract_assets(client, publication):
     identifier = register_asset(publication, owner=ALICE, status=EntryStatus.SUBMITTED)
@@ -119,6 +221,13 @@ def test_user_can_retract_assets(client, publication):
         assert "review_identifier" in response.json()
         assert Decision.RETRACTED == response.json()["decision"]
 
+    with logged_in_user(REVIEWER):
+        queue = client.get(
+            f"/submissions/v1?mode={ListMode.PENDING}", headers={"Authorization": "Fake token"}
+        )
+        assert queue.status_code == HTTPStatus.OK, queue.json()
+        assert len(queue.json()) == 0, "A retracted request should not remain pending."
+
 
 def test_other_user_can_not_retract_assets(client, publication):
     identifier = register_asset(publication, owner=ALICE, status=EntryStatus.SUBMITTED)

src/tests/testutils/default_instances.py

@@ -6,6 +6,8 @@
 
 import copy
 import json
+from functools import partial
+from typing import Callable
 
 import pytest
 from sqlalchemy.engine import Engine
@@ -73,8 +75,7 @@ def body_agent(body_resource: dict, load_body_agent: dict) -> dict:
     return copy.deepcopy(body)
 
 
-@pytest.fixture
-def publication(body_asset: dict) -> Publication:
+def make_publication(body_asset: dict) -> Publication:
     body = copy.deepcopy(body_asset)
     body["permanent_identifier"] = "http://dx.doi.org/10.1093/ajae/aaq063"
     body["isbn"] = "9783161484100"
@@ -83,6 +84,16 @@ def publication(body_asset: dict) -> Publication:
     return _create_class_with_body(Publication, body)
 
 
+@pytest.fixture
+def publication(body_asset: dict) -> Publication:
+    return make_publication(body_asset)
+
+
+@pytest.fixture
+def publication_factory(body_asset: dict) -> Callable[[], Publication]:
+    return partial(make_publication, body_asset)
+
+
 @pytest.fixture
 def contact(body_concept, engine: Engine) -> Contact:
     body = copy.deepcopy(body_concept)