refactor: refactor job routes and fix after e2e tests

Author: rebecamurillo

specs/admin-refresh-all/DOCS.md

@@ -1,10 +1,25 @@
 # Admin Refresh All — Architecture Docs
 
-Visual overview of the jobs API after the admin refresh feature was added. Shows the existing per-job route, the new admin refresh route, the shared services they reuse, and how a single admin trigger fans out into seven child job runs.
+Visual overview of the jobs API after the admin refresh feature was added **and refactored**. Shows the existing per-job route, the new admin refresh route, the shared services they reuse, how a single admin trigger fans out into seven child job runs, and the clean-code improvements applied in the post-feature refactor.
+
+---
+
+## Refactor summary (post-feature)
+
+Six clean-code improvements applied after the initial feature ship — all non-breaking:
+
+| Area | What changed |
+|---|---|
+| `services/jobs/base.py` | New `BaseJob` ABC: all 4 job classes extend it; lifecycle (STARTED + FAILURE) is no longer duplicated |
+| `services/jobs/__init__.py` | New `jobs` subpackage exposes `BaseJob` at a single canonical import path |
+| `services/full_impact_refresh_service.py` | `FullImpactRefreshOrchestrator` class groups orchestration helpers; module-level shims keep all imports backward-compatible |
+| `schemas/job_run.py` + `schemas/full_impact_refresh.py` | `schemas/job.py` split into two focused modules; original file is now a compatibility shim |
+| `utils/time.py` | `utc_now()` helper replaces deprecated `datetime.utcnow()` calls across all job and service files |
+| `utils/exceptions.py` | `@translate_errors` decorator converts unhandled exceptions to HTTP 500 on the three `router.*` handlers |
 
 ## 1. Component overview
 
-Highlights what is reused vs. newly introduced. New components added by this feature are tagged `NEW`.
+Highlights what is reused vs. newly introduced. Components added by the feature are tagged `NEW`; refactored components are tagged `REFACTOR`.
 
 ```mermaid
 flowchart LR
@@ -24,6 +39,7 @@ flowchart LR
         Allowlist["enforce_ip_allowlist<br/>NEW"]
         RateLimit["check_rate_limit<br/>NEW"]
         Idempotency["validate_idempotency_key<br/>NEW"]
+        TranslateErrors["@translate_errors decorator<br/>REFACTOR"]
     end
 
     subgraph Routes[Route handlers]
@@ -36,19 +52,31 @@ flowchart LR
 
     subgraph Services[Services]
         DispatchHelpers["job_dispatch_service<br/>resolve_actual_job_name<br/>execute_job_in_background<br/>NEW module"]
-        RefreshService["full_impact_refresh_service<br/>build_dispatch_plan<br/>dispatch_full_refresh<br/>build_*_response<br/>NEW"]
+        RefreshService["full_impact_refresh_service<br/>FullImpactRefreshOrchestrator CLASS<br/>+ module-level shims<br/>REFACTOR"]
         Registry[jobs.JOB_REGISTRY<br/>get_job_class]
-        KpiJob[KpiMeasuresAnalysisJob]
-        QuantJob[McdaQuantitativeJob]
-        QualJob[McdaQualitativeJob]
-        CustomJob[McdaCustomJob]
+        BaseJob["BaseJob ABC<br/>run → _execute<br/>REFACTOR"]
+        KpiJob["KpiMeasuresAnalysisJob<br/>extends BaseJob"]
+        QuantJob["McdaQuantitativeJob<br/>extends BaseJob"]
+        QualJob["McdaQualitativeJob<br/>extends BaseJob"]
+        CustomJob["McdaCustomJob<br/>extends BaseJob"]
     end
 
     subgraph Persistence
         JobRepo["JobRepository<br/>+ get_in_progress_full_refresh NEW<br/>+ update_dispatched_job NEW"]
         JobRunsTable[(job_runs table)]
     end
 
+    subgraph Utils[Utilities]
+        UtcNow["utils/time.py<br/>utc_now()<br/>REFACTOR"]
+        Exceptions["utils/exceptions.py<br/>@translate_errors<br/>REFACTOR"]
+    end
+
+    subgraph Schemas[Schemas]
+        JobRunSchema["schemas/job_run.py<br/>JobNameEnum · JobStatusEnum<br/>TriggerJobRequest · JobRunResponse<br/>REFACTOR"]
+        RefreshSchema["schemas/full_impact_refresh.py<br/>FullRefreshDispatchedJob<br/>FullImpactRefresh*Response<br/>REFACTOR"]
+        JobShim["schemas/job.py<br/>← compatibility shim<br/>REFACTOR"]
+    end
+
     Config["settings.JOB_RUN_CONFIGURATION<br/>NEW"]
 
     Backoffice -->|X-Admin-Refresh-Key| AdminRouter
@@ -64,6 +92,10 @@ flowchart LR
     JobsRouter --> GetJob
     JobsRouter --> ListJobs
 
+    TriggerJob --> TranslateErrors
+    GetJob --> TranslateErrors
+    ListJobs --> TranslateErrors
+
     TriggerRefresh --> RateLimit
     TriggerRefresh --> Idempotency
 
@@ -87,30 +119,47 @@ flowchart LR
     Registry --> QualJob
     Registry --> CustomJob
 
+    KpiJob --> BaseJob
+    QuantJob --> BaseJob
+    QualJob --> BaseJob
+    CustomJob --> BaseJob
+
+    BaseJob --> UtcNow
+    RefreshService --> UtcNow
+    TriggerRefresh --> UtcNow
+
+    TranslateErrors --> Exceptions
+
+    JobShim --> JobRunSchema
+    JobShim --> RefreshSchema
+
     JobRepo --> JobRunsTable
 ```
 
 ## 2. Existing per-job trigger flow
 
-`POST /jobs/runs/{job_name}` — unchanged behavior, now using the extracted `resolve_actual_job_name` and `execute_job_in_background` helpers from `job_dispatch_service`.
+`POST /jobs/runs/{job_name}` — unchanged behavior, now using the extracted `resolve_actual_job_name` and `execute_job_in_background` helpers from `job_dispatch_service`. Route handlers are now wrapped by `@translate_errors` (refactored).
 
 ```mermaid
 sequenceDiagram
     autonumber
     participant Client
     participant Router as jobs.router
     participant Verify as verify_api_key
+    participant TE as @translate_errors
     participant Handler as trigger_job
     participant Dispatch as job_dispatch_service
     participant Repo as JobRepository
     participant DB as job_runs table
     participant BG as FastAPI BackgroundTasks
-    participant Job as JobClass.run
+    participant Base as BaseJob.run
+    participant Job as JobClass._execute
 
     Client->>Router: POST /jobs/runs/{job_name}<br/>X-Internal-API-Key
     Router->>Verify: verify_api_key(headers)
     Verify-->>Router: ok
-    Router->>Handler: trigger_job(job_name, params)
+    Router->>TE: @translate_errors wraps handler
+    TE->>Handler: trigger_job(job_name, params)
     Handler->>Dispatch: resolve_actual_job_name(job_name, params)
     Dispatch-->>Handler: actual_job_name
     Handler->>Repo: create_job_run(actual_job_name)
@@ -120,8 +169,11 @@ sequenceDiagram
     Handler->>BG: add_task(execute_job_in_background, job_name, job_id, params)
     Handler-->>Client: 201 JobRunResponse
     BG->>Dispatch: execute_job_in_background(job_name, job_id, params)
-    Dispatch->>Job: JobClass.run(job_id, db, params)
-    Job->>Repo: update_job_status / update_job_data
+    Dispatch->>Base: JobClass.run(job_id, db, params)
+    Base->>Repo: update_job_status(STARTED)
+    Base->>Job: cls._execute(job_id, db, params, job_repo)
+    Job-->>Base: success / exception
+    Base->>Repo: update_job_status(SUCCESS or FAILURE)
     Repo->>DB: UPDATE job_run
 ```
 
@@ -211,7 +263,7 @@ sequenceDiagram
 
 ## 5. Class-level interactions
 
-How the new modules collaborate with the existing job classes and repository. Reuse is intentional: the admin orchestration calls the same dispatch helpers as the per-job route.
+How the new modules collaborate with the existing job classes and repository. Reuse is intentional: the admin orchestration calls the same dispatch helpers as the per-job route. Job lifecycle management is now centralised in `BaseJob` (refactored).
 
 ```mermaid
 classDiagram
@@ -225,18 +277,24 @@ classDiagram
         +trigger_full_impact_refresh(headers, body)
         +get_full_impact_refresh_status(run_id)
     }
+    class TranslateErrors {
+        <<REFACTOR decorator>>
+        +__call__(func) wraps route handlers
+        re-raises HTTPException
+        converts other exceptions → HTTP 500
+    }
     class JobDispatchService {
         <<NEW module>>
         +resolve_actual_job_name(job_name, params)
         +execute_job_in_background(job_name, job_id, params)
     }
-    class FullImpactRefreshService {
-        <<NEW>>
-        +build_dispatch_plan(started_at)
-        +build_initial_dispatch_state(plan)
-        +dispatch_full_refresh(parent_run_id, plan)
-        +build_trigger_response(plan, run_id, started_at)
-        +build_status_response(parent_run, repo)
+    class FullImpactRefreshOrchestrator {
+        <<REFACTOR class>>
+        build_dispatch_plan : staticmethod
+        build_initial_dispatch_state : staticmethod
+        dispatch : staticmethod
+        build_trigger_response : staticmethod
+        build_status_response : staticmethod
     }
     class AdminRefreshGuards {
         <<NEW>>
@@ -250,6 +308,10 @@ classDiagram
         +verify_api_key(header)
         +verify_admin_refresh_api_key(header) <<NEW>>
     }
+    class TimeUtils {
+        <<REFACTOR utils/time.py>>
+        +utc_now() datetime
+    }
     class JobRepository {
         +create_job_run(job_name)
         +get_job_run(job_id)
@@ -259,14 +321,32 @@ classDiagram
         +get_in_progress_full_refresh() <<NEW>>
         +update_dispatched_job(parent_id, sequence, updates) <<NEW>>
     }
+    class BaseJob {
+        <<REFACTOR ABC services/jobs/base.py>>
+        +run(job_id, db, params)$ classmethod
+        +_execute(job_id, db, params, job_repo)$ abstract classmethod
+        lifecycle: STARTED → SUCCESS/FAILURE
+    }
+    class KpiMeasuresAnalysisJob {
+        <<extends BaseJob>>
+        +_execute(job_id, db, params, job_repo)$
+    }
+    class McdaQuantitativeJob {
+        <<extends BaseJob>>
+        +_execute(job_id, db, params, job_repo)$
+    }
+    class McdaQualitativeJob {
+        <<extends BaseJob>>
+        +_execute(job_id, db, params, job_repo)$
+    }
+    class McdaCustomJob {
+        <<extends BaseJob>>
+        +_execute(job_id, db, params, job_repo)$
+    }
     class JobRegistry {
         +JOB_REGISTRY: Dict
         +get_job_class(job_name)
     }
-    class KpiMeasuresAnalysisJob
-    class McdaQuantitativeJob
-    class McdaQualitativeJob
-    class McdaCustomJob
     class Settings {
         +ADMIN_REFRESH_API_KEY <<NEW>>
         +ADMIN_REFRESH_ALLOWED_IPS <<NEW>>
@@ -276,24 +356,36 @@ classDiagram
         +JOB_RUN_CONFIGURATION <<NEW>>
     }
 
+    JobsRouter --> TranslateErrors : decorates handlers
     JobsRouter --> AuthDependencies : verify_api_key
     JobsRouter --> JobDispatchService : resolve + execute
     JobsRouter --> JobRepository
+    JobsRouter --> TimeUtils : utc_now()
 
     AdminRouter --> AuthDependencies : verify_admin_refresh_api_key
     AdminRouter --> AdminRefreshGuards
-    AdminRouter --> FullImpactRefreshService
+    AdminRouter --> FullImpactRefreshOrchestrator
     AdminRouter --> JobRepository
+    AdminRouter --> TimeUtils : utc_now()
 
-    FullImpactRefreshService --> Settings : JOB_RUN_CONFIGURATION
-    FullImpactRefreshService --> JobDispatchService
-    FullImpactRefreshService --> JobRepository
+    FullImpactRefreshOrchestrator --> Settings : JOB_RUN_CONFIGURATION
+    FullImpactRefreshOrchestrator --> JobDispatchService
+    FullImpactRefreshOrchestrator --> JobRepository
+    FullImpactRefreshOrchestrator --> TimeUtils : utc_now()
 
     JobDispatchService --> JobRegistry
     JobRegistry --> KpiMeasuresAnalysisJob
     JobRegistry --> McdaQuantitativeJob
     JobRegistry --> McdaQualitativeJob
     JobRegistry --> McdaCustomJob
+
+    KpiMeasuresAnalysisJob --|> BaseJob
+    McdaQuantitativeJob --|> BaseJob
+    McdaQualitativeJob --|> BaseJob
+    McdaCustomJob --|> BaseJob
+
+    BaseJob --> JobRepository : instantiates
+    BaseJob --> TimeUtils : utc_now()
 ```
 
 ## 6. Persistence model

src/sum_impact_assessment/api/dependencies/auth.py

@@ -6,8 +6,16 @@
 from ...config.settings import settings
 
 
-api_key_header = APIKeyHeader(name="X-Internal-API-Key", auto_error=False)
-admin_refresh_api_key_header = APIKeyHeader(name="X-Admin-Refresh-Key", auto_error=False)
+api_key_header = APIKeyHeader(
+    name="X-Internal-API-Key",
+    scheme_name="InternalApiKey",
+    auto_error=False,
+)
+admin_refresh_api_key_header = APIKeyHeader(
+    name="X-Admin-Refresh-Key",
+    scheme_name="AdminRefreshApiKey",
+    auto_error=False,
+)
 
 
 async def verify_api_key(api_key: str = Security(api_key_header)) -> str: