Refactor environment review out of test executions

backend/migrations/versions/2024_10_02_1114-91e7e3f437a0_create_artefactbuildenvironmentreview_.py

@@ -0,0 +1,93 @@
+"""Create ArtefactBuildEnvironmentReview table
+
+Revision ID: 91e7e3f437a0
+Revises: 9d7eed94a543
+Create Date: 2024-10-02 11:14:42.211503+00:00
+
+"""
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.dialects import postgresql
+
+# revision identifiers, used by Alembic.
+revision = "91e7e3f437a0"
+down_revision = "9d7eed94a543"
+branch_labels = None
+depends_on = None
+
+
+copy_cmd = """
+INSERT INTO artefact_build_environment_review (review_decision, review_comment, 
+        artefact_build_id, environment_id, created_at, updated_at)
+    SELECT review_decision::TEXT[]::artefactbuildenvironmentreviewdecision[], 
+        review_comment, artefact_build_id, environment_id, created_at, updated_at
+    FROM test_execution
+    ORDER BY id
+"""
+
+reverse_copy_cmd = """
+UPDATE test_execution te
+    SET review_comment = aber.review_comment, 
+    review_decision = aber.review_decision::TEXT[]::testexecutionreviewdecision[]
+    FROM artefact_build_environment_review aber
+    WHERE te.environment_id = aber.environment_id 
+    AND te.artefact_build_id = aber.artefact_build_id
+"""
+
+
+def upgrade() -> None:
+    op.create_table(
+        "artefact_build_environment_review",
+        sa.Column(
+            "review_decision",
+            postgresql.ARRAY(
+                sa.Enum(
+                    "REJECTED",
+                    "APPROVED_INCONSISTENT_TEST",
+                    "APPROVED_UNSTABLE_PHYSICAL_INFRA",
+                    "APPROVED_CUSTOMER_PREREQUISITE_FAIL",
+                    "APPROVED_FAULTY_HARDWARE",
+                    "APPROVED_ALL_TESTS_PASS",
+                    name="artefactbuildenvironmentreviewdecision",
+                )
+            ),
+            nullable=False,
+        ),
+        sa.Column("review_comment", sa.String(), nullable=False),
+        sa.Column("environment_id", sa.Integer(), nullable=False),
+        sa.Column("artefact_build_id", sa.Integer(), nullable=False),
+        sa.Column("id", sa.Integer(), autoincrement=True, nullable=False),
+        sa.Column("created_at", sa.DateTime(), nullable=False),
+        sa.Column("updated_at", sa.DateTime(), nullable=False),
+        sa.ForeignKeyConstraint(
+            ["artefact_build_id"],
+            ["artefact_build.id"],
+            name=op.f("artefact_build_environment_review_artefact_build_id_fkey"),
+            ondelete="CASCADE",
+        ),
+        sa.ForeignKeyConstraint(
+            ["environment_id"],
+            ["environment.id"],
+            name=op.f("artefact_build_environment_review_environment_id_fkey"),
+            ondelete="CASCADE",
+        ),
+        sa.PrimaryKeyConstraint(
+            "id", name=op.f("artefact_build_environment_review_pkey")
+        ),
+        sa.UniqueConstraint(
+            "artefact_build_id",
+            "environment_id",
+            name=op.f(
+                "artefact_build_environment_review_artefact_build_id_environment_id_key"
+            ),
+        ),
+    )
+
+    op.execute(copy_cmd)
+
+
+def downgrade() -> None:
+    op.execute(reverse_copy_cmd)
+
+    op.drop_table("artefact_build_environment_review")
+    op.execute("DROP TYPE artefactbuildenvironmentreviewdecision")

backend/pyproject.toml

@@ -54,15 +54,7 @@ select = [
     "PLE",
     "TID252",
 ]
-ignore = ["ANN201", "ANN003", "N999", "ANN101", "ANN204", "N818"]
-
-[tool.ruff.flake8-bugbear]
-extend-immutable-calls = [
-    "fastapi.Depends",
-    "fastapi.params.Depends",
-    "fastapi.Query",
-    "fastapi.params.Query",
-]
+ignore = ["ANN201", "ANN003", "N999", "ANN101", "ANN204", "N818", "B008"]
 
 [[tool.mypy.overrides]]
 module = "celery.*"

backend/test_observer/controllers/artefacts/artefact_retriever.py

@@ -0,0 +1,23 @@
+from fastapi import Depends, HTTPException
+from sqlalchemy import select
+from sqlalchemy.orm import Session
+from sqlalchemy.sql.base import ExecutableOption
+
+from test_observer.data_access.models import (
+    Artefact,
+)
+from test_observer.data_access.setup import get_db
+
+
+class ArtefactRetriever:
+    def __init__(self, *options: ExecutableOption):
+        self._options = options
+
+    def __call__(self, artefact_id: int, db: Session = Depends(get_db)):
+        artefact = db.scalar(
+            select(Artefact).where(Artefact.id == artefact_id).options(*self._options)
+        )
+        if artefact is None:
+            msg = f"Artefact with id {artefact_id} not found"
+            raise HTTPException(status_code=404, detail=msg)
+        return artefact

backend/test_observer/controllers/artefacts/artefacts.py

@@ -19,45 +19,31 @@
 #        Nadzeya Hutsko <[email protected]>
 from fastapi import APIRouter, Depends, HTTPException
 from sqlalchemy import select
-from sqlalchemy.orm import Session, joinedload
+from sqlalchemy.orm import Session, selectinload
 
-from test_observer.data_access import queries
+from test_observer.controllers.artefacts.artefact_retriever import ArtefactRetriever
 from test_observer.data_access.models import (
     Artefact,
     ArtefactBuild,
-    TestExecution,
 )
 from test_observer.data_access.models_enums import ArtefactStatus, FamilyName
 from test_observer.data_access.repository import get_artefacts_by_family
 from test_observer.data_access.setup import get_db
 
+from . import builds, environment_reviews
 from .logic import (
-    are_all_test_executions_approved,
-    is_there_a_rejected_test_execution,
+    are_all_environments_approved,
+    is_there_a_rejected_environment,
 )
 from .models import (
-    ArtefactBuildDTO,
     ArtefactDTO,
     ArtefactPatch,
     ArtefactVersionDTO,
 )
 
 router = APIRouter(tags=["artefacts"])
-
-
-def _get_artefact_from_db(artefact_id: int, db: Session = Depends(get_db)) -> Artefact:
-    a = (
-        db.query(Artefact)
-        .options(
-            joinedload(Artefact.builds).joinedload(ArtefactBuild.test_executions),
-        )
-        .filter(Artefact.id == artefact_id)
-        .one_or_none()
-    )
-    if a is None:
-        msg = f"Artefact with id {artefact_id} not found"
-        raise HTTPException(status_code=404, detail=msg)
-    return a
+router.include_router(environment_reviews.router)
+router.include_router(builds.router)
 
 
 @router.get("", response_model=list[ArtefactDTO])
@@ -71,7 +57,7 @@ def get_artefacts(family: FamilyName | None = None, db: Session = Depends(get_db
             db,
             family,
             load_stage=True,
-            load_test_executions=True,
+            load_environment_reviews=True,
             order_by_columns=order_by,
         )
     else:
@@ -80,7 +66,7 @@ def get_artefacts(family: FamilyName | None = None, db: Session = Depends(get_db
                 db,
                 family,
                 load_stage=True,
-                load_test_executions=True,
+                load_environment_reviews=True,
                 order_by_columns=order_by,
             )
 
@@ -89,7 +75,13 @@ def get_artefacts(family: FamilyName | None = None, db: Session = Depends(get_db
 
 @router.get("/{artefact_id}", response_model=ArtefactDTO)
 def get_artefact(
-    artefact: Artefact = Depends(_get_artefact_from_db),
+    artefact: Artefact = Depends(
+        ArtefactRetriever(
+            selectinload(Artefact.builds).selectinload(
+                ArtefactBuild.environment_reviews
+            )
+        )
+    ),
 ):
     return artefact
 
@@ -98,17 +90,15 @@ def get_artefact(
 def patch_artefact(
     request: ArtefactPatch,
     db: Session = Depends(get_db),
-    artefact: Artefact = Depends(_get_artefact_from_db),
+    artefact: Artefact = Depends(
+        ArtefactRetriever(
+            selectinload(Artefact.builds).selectinload(
+                ArtefactBuild.environment_reviews
+            )
+        )
+    ),
 ):
-    latest_builds = list(
-        db.scalars(
-            queries.latest_artefact_builds.where(
-                ArtefactBuild.artefact_id == artefact.id
-            ).options(joinedload(ArtefactBuild.test_executions))
-        ).unique()
-    )
-
-    _validate_artefact_status(latest_builds, request.status)
+    _validate_artefact_status(artefact.latest_builds, request.status)
 
     artefact.status = request.status
     db.commit()
@@ -118,58 +108,27 @@ def patch_artefact(
 def _validate_artefact_status(
     builds: list[ArtefactBuild], status: ArtefactStatus
 ) -> None:
-    if status == ArtefactStatus.APPROVED and not are_all_test_executions_approved(
-        builds
-    ):
+    ...
+    if status == ArtefactStatus.APPROVED and not are_all_environments_approved(builds):
         raise HTTPException(
             status_code=400,
             detail="All test executions need to be approved",
         )
 
     if (
         status == ArtefactStatus.MARKED_AS_FAILED
-        and not is_there_a_rejected_test_execution(builds)
+        and not is_there_a_rejected_environment(builds)
     ):
         raise HTTPException(
             400,
             detail="At least one test execution needs to be rejected",
         )
 
 
-@router.get("/{artefact_id}/builds", response_model=list[ArtefactBuildDTO])
-def get_artefact_builds(artefact_id: int, db: Session = Depends(get_db)):
-    """Get latest artefact builds of an artefact together with their test executions"""
-    latest_builds = list(
-        db.scalars(
-            queries.latest_artefact_builds.where(
-                ArtefactBuild.artefact_id == artefact_id
-            ).options(
-                joinedload(ArtefactBuild.test_executions).joinedload(
-                    TestExecution.rerun_request
-                )
-            )
-        ).unique()
-    )
-
-    for artefact_build in latest_builds:
-        artefact_build.test_executions.sort(
-            key=lambda test_execution: test_execution.environment.name
-        )
-
-    return latest_builds
-
-
 @router.get("/{artefact_id}/versions", response_model=list[ArtefactVersionDTO])
 def get_artefact_versions(
-    artefact_id: int,
-    db: Session = Depends(get_db),
+    artefact: Artefact = Depends(ArtefactRetriever()), db: Session = Depends(get_db)
 ):
-    artefact = db.get(Artefact, artefact_id)
-
-    if not artefact:
-        msg = f"Artefact with id {artefact_id} not found"
-        raise HTTPException(status_code=404, detail=msg)
-
     return db.scalars(
         select(Artefact)
         .where(Artefact.name == artefact.name)

backend/test_observer/controllers/artefacts/builds.py

@@ -0,0 +1,37 @@
+from fastapi import APIRouter, Depends
+from sqlalchemy.orm import selectinload
+
+from test_observer.controllers.artefacts.artefact_retriever import ArtefactRetriever
+from test_observer.data_access.models import (
+    Artefact,
+    ArtefactBuild,
+    TestExecution,
+)
+
+from .models import (
+    ArtefactBuildDTO,
+)
+
+router = APIRouter(tags=["artefact-builds"])
+
+
+@router.get("/{artefact_id}/builds", response_model=list[ArtefactBuildDTO])
+def get_artefact_builds(
+    artefact: Artefact = Depends(
+        ArtefactRetriever(
+            selectinload(Artefact.builds)
+            .selectinload(ArtefactBuild.test_executions)
+            .options(
+                selectinload(TestExecution.environment),
+                selectinload(TestExecution.rerun_request),
+            )
+        )
+    ),
+):
+    """Get latest artefact builds of an artefact together with their test executions"""
+    for artefact_build in artefact.latest_builds:
+        artefact_build.test_executions.sort(
+            key=lambda test_execution: test_execution.environment.name
+        )
+
+    return artefact.latest_builds