From 6530aa86b70327a54c20eab206c0637422a58f6b Mon Sep 17 00:00:00 2001 From: Yuze Fu Date: Thu, 24 Sep 2026 09:49:40 +0900 Subject: [PATCH 1/3] feat: add trainee self reflection API (T-313) --- e2e/lib/api/schema.ts | 83 +++++++ .../atc/trainings/id/self-reflection.test.ts | 203 ++++++++++++++++++ ...921000000_add_training_self_reflection.sql | 17 ++ src/modules/training/dto.rs | 7 + src/modules/training/models.rs | 1 + src/modules/training/repository/training.rs | 47 +++- src/modules/training/routes/trainings.rs | 53 ++++- src/modules/training/service.rs | 42 ++++ 8 files changed, 449 insertions(+), 4 deletions(-) create mode 100644 e2e/src/atc/trainings/id/self-reflection.test.ts create mode 100644 migrations/20260921000000_add_training_self_reflection.sql diff --git a/e2e/lib/api/schema.ts b/e2e/lib/api/schema.ts index 0fad5f7..c2b2872 100644 --- a/e2e/lib/api/schema.ts +++ b/e2e/lib/api/schema.ts @@ -404,6 +404,22 @@ export type paths = { patch?: never; trace?: never; }; + "/api/atc/trainings/self-reflection-sheet": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get: operations["get_self_reflection_sheet"]; + put?: never; + post?: never; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/api/atc/trainings/{id}": { parameters: { query?: never; @@ -436,6 +452,22 @@ export type paths = { patch?: never; trace?: never; }; + "/api/atc/trainings/{id}/self-reflection": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put: operations["set_self_reflection"]; + post?: never; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/api/compat/euroscope/metar/metar.php": { parameters: { query?: never; @@ -1682,6 +1714,8 @@ export type components = { name: string; record_sheet_filing?: components["schemas"]["SheetFieldAnswerDto"][] | null; record_sheet_filing_id?: string | null; + self_reflection_sheet_filing?: components["schemas"]["SheetFieldAnswerDto"][] | null; + self_reflection_sheet_filing_id?: string | null; /** Format: date-time */ start_at: string; trainee: components["schemas"]["UserDto"]; @@ -2565,6 +2599,27 @@ export interface operations { 500: components["responses"]["InternalServerError"]; }; }; + get_self_reflection_sheet: { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Successful response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["SheetDto"]; + }; + }; + 500: components["responses"]["InternalServerError"]; + }; + }; get_training: { parameters: { query?: never; @@ -2667,6 +2722,34 @@ export interface operations { 500: components["responses"]["InternalServerError"]; }; }; + set_self_reflection: { + parameters: { + query?: never; + header?: never; + path: { + /** @description Training ULID */ + id: string; + }; + cookie?: never; + }; + requestBody: { + content: { + "application/json": components["schemas"]["TrainingRecordRequest"]; + }; + }; + responses: { + /** @description Successful response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["TrainingDto"]; + }; + }; + 500: components["responses"]["InternalServerError"]; + }; + }; get_metar_by_query: { parameters: { query?: { diff --git a/e2e/src/atc/trainings/id/self-reflection.test.ts b/e2e/src/atc/trainings/id/self-reflection.test.ts new file mode 100644 index 0000000..de1365d --- /dev/null +++ b/e2e/src/atc/trainings/id/self-reflection.test.ts @@ -0,0 +1,203 @@ +import { expect, test as baseTest } from "vitest"; +import { getClient } from "../../../../lib/backend.js"; + +const test = baseTest + .extend("mentor", async () => getClient(["controller-training-mentor"])) + .extend("trainee", async () => getClient([])) + .extend("training", async ({ mentor, trainee }) => { + const trainerSession = await mentor.GET("/api/session"); + const traineeSession = await trainee.GET("/api/session"); + const result = await mentor.POST("/api/atc/trainings", { + body: { + name: "Self reflection test", + trainer_id: trainerSession.data!.user!.id, + trainee_id: traineeSession.data!.user!.id, + start_at: "2031-06-01T10:00:00Z", + end_at: "2031-06-01T11:00:00Z", + }, + }); + expect(result.response.status).toBe(200); + return result.data!; + }); +const body = (answer: string) => ({ + request_answers: [{ id: "reflection", answer }], +}); + +test("trainee saves before training and edits after training without changing mentor feedback", async ({ + trainee, + mentor, + training, +}) => { + const params = { path: { id: training.id } }; + const sheet = await trainee.GET("/api/atc/trainings/self-reflection-sheet"); + expect(sheet.data?.fields).toEqual([ + expect.objectContaining({ id: "reflection", kind: "long-text" }), + ]); + expect(training.self_reflection_sheet_filing).toBeNull(); + const first = await trainee.PUT("/api/atc/trainings/{id}/self-reflection", { + params, + body: body("初次反思\n需要改进协调"), + }); + expect(first.response.status).toBe(200); + expect(first.data?.self_reflection_sheet_filing?.[0].answer).toBe( + "初次反思\n需要改进协调", + ); + const ended = await mentor.PUT("/api/atc/trainings/{id}", { + params, + body: { + ...training, + start_at: "2020-06-01T10:00:00Z", + end_at: "2020-06-01T11:00:00Z", + }, + }); + expect(ended.response.status).toBe(200); + const second = await trainee.PUT("/api/atc/trainings/{id}/self-reflection", { + params, + body: body("修改后的反思"), + }); + expect(second.response.status).toBe(200); + expect(second.data?.self_reflection_sheet_filing_id).toBe( + first.data?.self_reflection_sheet_filing_id, + ); + const loaded = await trainee.GET("/api/atc/trainings/{id}", { params }); + expect(loaded.data?.self_reflection_sheet_filing?.[0].answer).toBe( + "修改后的反思", + ); + expect(loaded.data?.record_sheet_filing).toBeNull(); +}); + +test("training viewers can read but only the trainee can edit", async ({ + trainee, + mentor, + training, +}) => { + const params = { path: { id: training.id } }; + expect( + ( + await trainee.PUT("/api/atc/trainings/{id}/self-reflection", { + params, + body: body("Private reflection"), + }) + ).response.status, + ).toBe(200); + for (const viewer of [ + mentor, + await getClient(["controller-training-mentor"]), + await getClient(["controller-training-director-assistant"]), + ]) { + const read = await viewer.GET("/api/atc/trainings/{id}", { params }); + expect(read.response.status).toBe(200); + expect(read.data?.self_reflection_sheet_filing?.[0].answer).toBe( + "Private reflection", + ); + expect( + ( + await viewer.PUT("/api/atc/trainings/{id}/self-reflection", { + params, + body: body("Denied"), + }) + ).response.status, + ).toBe(403); + } + const stranger = await getClient([]); + expect( + (await stranger.GET("/api/atc/trainings/{id}", { params })).response.status, + ).toBe(403); + expect( + ( + await stranger.PUT("/api/atc/trainings/{id}/self-reflection", { + params, + body: body("Denied"), + }) + ).response.status, + ).toBe(403); + const anonymous = await getClient(); + expect( + ( + await anonymous.PUT("/api/atc/trainings/{id}/self-reflection", { + params, + body: body("Denied"), + }) + ).response.status, + ).toBe(401); + expect( + (await trainee.GET("/api/atc/trainings/{id}", { params })).data + ?.self_reflection_sheet_filing?.[0].answer, + ).toBe("Private reflection"); +}); + +test("concurrent initial saves share a filing; invalid fields do not overwrite answers", async ({ + trainee, + training, +}) => { + const params = { path: { id: training.id } }; + const results = await Promise.all( + ["First", "Second"].map((answer) => + trainee.PUT("/api/atc/trainings/{id}/self-reflection", { + params, + body: body(answer), + }), + ), + ); + results.forEach((result) => expect(result.response.status).toBe(200)); + expect(results[0].data?.self_reflection_sheet_filing_id).toBe( + results[1].data?.self_reflection_sheet_filing_id, + ); + const before = await trainee.GET("/api/atc/trainings/{id}", { params }); + const invalid = await trainee.PUT("/api/atc/trainings/{id}/self-reflection", { + params, + body: { request_answers: [{ id: "unknown", answer: "Bad field" }] }, + }); + expect(invalid.response.ok).toBe(false); + const after = await trainee.GET("/api/atc/trainings/{id}", { params }); + expect(after.data?.self_reflection_sheet_filing).toEqual( + before.data?.self_reflection_sheet_filing, + ); +}); + +test("uses configured fields and excludes deleted fields", async ({ + trainee, + training, +}) => { + const staff = await getClient(["staff"]); + const params = { path: { sheetId: "training-self-reflection" } }; + const original = await staff.GET("/api/sheets/{sheetId}", { params }); + expect(original.response.status).toBe(200); + const custom = { + ...original.data!.fields[0], + id: "next-steps", + name_zh: "改进计划", + name_en: "Next steps", + }; + try { + const configured = await staff.PUT("/api/sheets/{sheetId}", { + params, + body: { name: original.data!.name, fields: [custom] }, + }); + expect(configured.response.status).toBe(200); + const sheet = await trainee.GET("/api/atc/trainings/self-reflection-sheet"); + expect(sheet.data?.fields.map((field) => field.id)).toEqual(["next-steps"]); + const saved = await trainee.PUT("/api/atc/trainings/{id}/self-reflection", { + params: { path: { id: training.id } }, + body: { + request_answers: [ + { id: "next-steps", answer: "Practice coordination" }, + ], + }, + }); + expect(saved.response.status).toBe(200); + expect(saved.data?.self_reflection_sheet_filing?.[0]).toMatchObject({ + field: { id: "next-steps" }, + answer: "Practice coordination", + }); + } finally { + const restored = await staff.PUT("/api/sheets/{sheetId}", { + params, + body: { + name: original.data!.name, + fields: original.data!.fields.filter((field) => !field.is_deleted), + }, + }); + expect(restored.response.status).toBe(200); + } +}); diff --git a/migrations/20260921000000_add_training_self_reflection.sql b/migrations/20260921000000_add_training_self_reflection.sql new file mode 100644 index 0000000..360fd84 --- /dev/null +++ b/migrations/20260921000000_add_training_self_reflection.sql @@ -0,0 +1,17 @@ +ALTER TABLE public.training + ADD COLUMN self_reflection_sheet_filing_id uuid + REFERENCES public.sheet_filing(id); + +CREATE INDEX ix_training_self_reflection_sheet_filing_id + ON public.training (self_reflection_sheet_filing_id); + +INSERT INTO public.sheet (id, name) +VALUES ('training-self-reflection', 'Training Self Reflection'); + +INSERT INTO public.sheet_field ( + sheet_id, id, sequence, name_zh, name_en, kind, single_choice_options +) +VALUES ( + 'training-self-reflection', 'reflection', 0, + '自我反思', 'Self Reflection', 'long-text', ARRAY[]::text[] +); diff --git a/src/modules/training/dto.rs b/src/modules/training/dto.rs index 1a574f3..1119189 100644 --- a/src/modules/training/dto.rs +++ b/src/modules/training/dto.rs @@ -55,6 +55,8 @@ pub struct TrainingDto { pub deleted_at: Option>, pub record_sheet_filing_id: Option, pub record_sheet_filing: Option>, + pub self_reflection_sheet_filing_id: Option, + pub self_reflection_sheet_filing: Option>, } impl TrainingDto { @@ -63,6 +65,7 @@ impl TrainingDto { trainer: UserSummary, trainee: UserSummary, record_sheet_filing: Option>, + self_reflection_sheet_filing: Option>, ) -> Self { Self { id: Ulid::from(training.id).to_string(), @@ -80,6 +83,10 @@ impl TrainingDto { .record_sheet_filing_id .map(|id| Ulid::from(id).to_string()), record_sheet_filing, + self_reflection_sheet_filing_id: training + .self_reflection_sheet_filing_id + .map(|id| Ulid::from(id).to_string()), + self_reflection_sheet_filing, } } } diff --git a/src/modules/training/models.rs b/src/modules/training/models.rs index a457ea4..849d94b 100644 --- a/src/modules/training/models.rs +++ b/src/modules/training/models.rs @@ -14,6 +14,7 @@ pub struct Training { pub updated_at: DateTime, pub deleted_at: Option>, pub record_sheet_filing_id: Option, + pub self_reflection_sheet_filing_id: Option, } #[derive(Debug, Clone)] diff --git a/src/modules/training/repository/training.rs b/src/modules/training/repository/training.rs index 69aa0eb..dfe4c47 100644 --- a/src/modules/training/repository/training.rs +++ b/src/modules/training/repository/training.rs @@ -20,7 +20,8 @@ fn training_select_sql_from(source: &str, where_clause: &str) -> String { training.created_at, training.updated_at, training.deleted_at, - training.record_sheet_filing_id + training.record_sheet_filing_id, + training.self_reflection_sheet_filing_id FROM {source} {where_clause} "# @@ -44,6 +45,8 @@ pub(crate) trait TrainingRepository<'executor> { async fn find_training_by_id(self, id: Uuid) -> Result, sqlx::Error>; + async fn lock_training_by_id(self, id: Uuid) -> Result, sqlx::Error>; + async fn create_training(self, training: TrainingSave) -> Result; async fn update_training( @@ -58,6 +61,12 @@ pub(crate) trait TrainingRepository<'executor> { filing_id: Uuid, ) -> Result, sqlx::Error>; + async fn set_training_self_reflection_filing( + self, + id: Uuid, + filing_id: Uuid, + ) -> Result, sqlx::Error>; + async fn mark_training_deleted(self, id: Uuid) -> Result; } @@ -121,6 +130,12 @@ where .fetch_optional(self) .await } + async fn lock_training_by_id(self, id: Uuid) -> Result, sqlx::Error> { + sqlx::query_as::<_, Training>(&training_select_sql("WHERE training.id = $1 FOR UPDATE")) + .bind(id) + .fetch_optional(self) + .await + } async fn create_training(self, training: TrainingSave) -> Result { tracing::info!( operation = "create", @@ -216,6 +231,36 @@ where .fetch_optional(self) .await } + async fn set_training_self_reflection_filing( + self, + id: Uuid, + filing_id: Uuid, + ) -> Result, sqlx::Error> { + tracing::info!( + operation = "set_self_reflection_filing", + repository = "src/modules/training/repository/training.rs", + "modifying data" + ); + + let query = format!( + r#" + WITH updated AS ( + UPDATE public.training + SET self_reflection_sheet_filing_id = $2, updated_at = $3 + WHERE id = $1 + RETURNING * + ) + {} + "#, + training_select_sql_from("updated AS training", "WHERE training.id = $1"), + ); + sqlx::query_as::<_, Training>(&query) + .bind(id) + .bind(filing_id) + .bind(Utc::now()) + .fetch_optional(self) + .await + } async fn mark_training_deleted(self, id: Uuid) -> Result { let result = sqlx::query( r#" diff --git a/src/modules/training/routes/trainings.rs b/src/modules/training/routes/trainings.rs index a33977b..231e90c 100644 --- a/src/modules/training/routes/trainings.rs +++ b/src/modules/training/routes/trainings.rs @@ -5,12 +5,12 @@ use axum::{Json, Router}; use ulid::Ulid; use crate::error::ApiError; -use crate::modules::user::models::UserRole; use crate::modules::sheet::dto::{SheetDto, SheetFieldAnswerDto}; use crate::modules::sheet::models::SheetAnswerSave; use crate::modules::training::dto::{TrainingDto, TrainingRecordRequest, TrainingSaveRequest}; -use crate::modules::training::service::TrainingView; +use crate::modules::training::service::{SELF_REFLECTION_SHEET_ID, TrainingView}; use crate::modules::user::middleware::CurrentUser; +use crate::modules::user::models::UserRole; use crate::services::Services; #[derive(utoipa::OpenApi)] @@ -23,7 +23,9 @@ use crate::services::Services; get_training, update_training, delete_training, - set_record_sheet + set_record_sheet, + get_self_reflection_sheet, + set_self_reflection ))] pub(crate) struct ApiDoc; @@ -36,6 +38,11 @@ pub fn build_training_routes() -> Router { .route("/by-user/{user_id}", get(list_by_user)) .route("/finished", get(list_finished)) .route("/record-sheet", get(get_record_sheet)) + .route("/self-reflection-sheet", get(get_self_reflection_sheet)) + .route( + "/{id}/self-reflection", + axum::routing::put(set_self_reflection), + ) .route( "/{id}", get(get_training) @@ -201,6 +208,40 @@ async fn set_record_sheet( Ok(Json(training_to_dto(training))) } +#[utoipa::path(get, path = "api/atc/trainings/self-reflection-sheet", tag = "Training", security(("oauth2" = [])), responses((status = 200, description = "Successful response", body = SheetDto)))] +async fn get_self_reflection_sheet( + State(services): State, +) -> Result, ApiError> { + let view = services.sheet().find(SELF_REFLECTION_SHEET_ID).await?; + Ok(Json(SheetDto::from_entities( + view.sheet, + view.fields + .into_iter() + .filter(|field| !field.is_deleted) + .collect(), + ))) +} + +#[utoipa::path(put, path = "api/atc/trainings/{id}/self-reflection", tag = "Training", security(("oauth2" = [])), params(("id" = String, Path, description = "Training ULID")), request_body = TrainingRecordRequest, responses((status = 200, description = "Successful response", body = TrainingDto)))] +async fn set_self_reflection( + State(services): State, + current_user: CurrentUser, + Path(id): Path, + Json(request): Json, +) -> Result, ApiError> { + let user_id = current_user.user_id.ok_or(ApiError::Unauthorized)?; + let answers = request + .request_answers + .into_iter() + .map(SheetAnswerSave::from) + .collect::>(); + let training = services + .training() + .set_self_reflection(id.parse::()?.into(), &answers, user_id) + .await?; + Ok(Json(training_to_dto(training))) +} + #[utoipa::path(delete, path = "api/atc/trainings/{id}", tag = "Training", security(("oauth2" = [])), params(("id" = String, Path, description = "Training ULID")), responses((status = 204, description = "No content")))] async fn delete_training( State(services): State, @@ -231,6 +272,12 @@ fn training_to_dto(view: TrainingView) -> TrainingDto { .map(|view| SheetFieldAnswerDto::from_entities(view.answer, view.field)) .collect() }), + view.self_reflection_sheet_filing.map(|answers| { + answers + .into_iter() + .map(|view| SheetFieldAnswerDto::from_entities(view.answer, view.field)) + .collect() + }), ) } diff --git a/src/modules/training/service.rs b/src/modules/training/service.rs index 4f8a112..36c4889 100644 --- a/src/modules/training/service.rs +++ b/src/modules/training/service.rs @@ -24,6 +24,8 @@ use super::repository::training_application_response::{ }; use super::repository::training_application_slot::TrainingApplicationSlotRepository; +pub const SELF_REFLECTION_SHEET_ID: &str = "training-self-reflection"; + const RECORD_SHEET_ID: &str = "training-record"; #[derive(Clone)] @@ -159,6 +161,40 @@ impl TrainingService { self.with_filing(training).await } + pub async fn set_self_reflection( + &self, + id: Uuid, + answers: &[SheetAnswerSave], + current_user_id: Uuid, + ) -> Result { + let mut transaction = self.db.begin().await?; + // Serialize edits, including the first filing, for this training. + let training = (&mut *transaction) + .lock_training_by_id(id) + .await? + .ok_or(TrainingServiceError::NotFound(id))?; + if training.trainee_id != current_user_id { + return Err(TrainingServiceError::NotOwned { + entity: "training", + id, + }); + } + let filing_id = transaction + .set_sheet_filing( + SELF_REFLECTION_SHEET_ID, + training.self_reflection_sheet_filing_id, + current_user_id, + answers, + ) + .await?; + let training = (&mut *transaction) + .set_training_self_reflection_filing(id, filing_id) + .await? + .ok_or(TrainingServiceError::NotFound(id))?; + transaction.commit().await?; + self.with_filing(training).await + } + pub async fn delete( &self, id: Uuid, @@ -197,6 +233,10 @@ impl TrainingService { Some(filing_id) => Some(self.sheet.filing_answers(filing_id).await?), None => None, }; + let self_reflection_sheet_filing = match training.self_reflection_sheet_filing_id { + Some(filing_id) => Some(self.sheet.filing_answers(filing_id).await?), + None => None, + }; let trainer = self .user .find_summary_by_id(training.trainer_id) @@ -212,6 +252,7 @@ impl TrainingService { trainer, trainee, record_sheet_filing, + self_reflection_sheet_filing, }) } } @@ -222,6 +263,7 @@ pub struct TrainingView { pub trainer: UserSummary, pub trainee: UserSummary, pub record_sheet_filing: Option>, + pub self_reflection_sheet_filing: Option>, } fn ensure_trainer_access( From b16a88a71a969166c469bfdc07e81798aedf8928 Mon Sep 17 00:00:00 2001 From: Yuze Fu Date: Thu, 24 Sep 2026 19:26:13 +0900 Subject: [PATCH 2/3] fix: restrict reflection reads and allow training admins to write --- e2e/lib/api/schema.ts | 31 ++--- .../atc/trainings/id/self-reflection.test.ts | 117 ++++++++++++++---- src/modules/training/routes/trainings.rs | 56 ++++++--- src/modules/training/service.rs | 5 +- 4 files changed, 151 insertions(+), 58 deletions(-) diff --git a/e2e/lib/api/schema.ts b/e2e/lib/api/schema.ts index c2b2872..f9e7c18 100644 --- a/e2e/lib/api/schema.ts +++ b/e2e/lib/api/schema.ts @@ -404,39 +404,39 @@ export type paths = { patch?: never; trace?: never; }; - "/api/atc/trainings/self-reflection-sheet": { + "/api/atc/trainings/{id}": { parameters: { query?: never; header?: never; path?: never; cookie?: never; }; - get: operations["get_self_reflection_sheet"]; - put?: never; + get: operations["get_training"]; + put: operations["update_training"]; post?: never; - delete?: never; + delete: operations["delete_training"]; options?: never; head?: never; patch?: never; trace?: never; }; - "/api/atc/trainings/{id}": { + "/api/atc/trainings/{id}/record": { parameters: { query?: never; header?: never; path?: never; cookie?: never; }; - get: operations["get_training"]; - put: operations["update_training"]; + get?: never; + put: operations["set_record_sheet"]; post?: never; - delete: operations["delete_training"]; + delete?: never; options?: never; head?: never; patch?: never; trace?: never; }; - "/api/atc/trainings/{id}/record": { + "/api/atc/trainings/{id}/self-reflection": { parameters: { query?: never; header?: never; @@ -444,7 +444,7 @@ export type paths = { cookie?: never; }; get?: never; - put: operations["set_record_sheet"]; + put: operations["set_self_reflection"]; post?: never; delete?: never; options?: never; @@ -452,15 +452,15 @@ export type paths = { patch?: never; trace?: never; }; - "/api/atc/trainings/{id}/self-reflection": { + "/api/atc/trainings/{id}/self-reflection-sheet": { parameters: { query?: never; header?: never; path?: never; cookie?: never; }; - get?: never; - put: operations["set_self_reflection"]; + get: operations["get_self_reflection_sheet"]; + put?: never; post?: never; delete?: never; options?: never; @@ -2603,7 +2603,10 @@ export interface operations { parameters: { query?: never; header?: never; - path?: never; + path: { + /** @description Training ULID */ + id: string; + }; cookie?: never; }; requestBody?: never; diff --git a/e2e/src/atc/trainings/id/self-reflection.test.ts b/e2e/src/atc/trainings/id/self-reflection.test.ts index de1365d..43c3d5e 100644 --- a/e2e/src/atc/trainings/id/self-reflection.test.ts +++ b/e2e/src/atc/trainings/id/self-reflection.test.ts @@ -29,7 +29,10 @@ test("trainee saves before training and edits after training without changing me training, }) => { const params = { path: { id: training.id } }; - const sheet = await trainee.GET("/api/atc/trainings/self-reflection-sheet"); + const sheet = await trainee.GET( + "/api/atc/trainings/{id}/self-reflection-sheet", + { params: { path: { id: training.id } } }, + ); expect(sheet.data?.fields).toEqual([ expect.objectContaining({ id: "reflection", kind: "long-text" }), ]); @@ -66,52 +69,103 @@ test("trainee saves before training and edits after training without changing me expect(loaded.data?.record_sheet_filing).toBeNull(); }); -test("training viewers can read but only the trainee can edit", async ({ +test("reflection access is limited to the trainee, assigned trainer and training director assistant", async ({ trainee, mentor, training, }) => { const params = { path: { id: training.id } }; - expect( - ( - await trainee.PUT("/api/atc/trainings/{id}/self-reflection", { - params, - body: body("Private reflection"), - }) - ).response.status, - ).toBe(200); - for (const viewer of [ - mentor, - await getClient(["controller-training-mentor"]), - await getClient(["controller-training-director-assistant"]), - ]) { + const admin = await getClient(["controller-training-director-assistant"]); + const first = await admin.PUT("/api/atc/trainings/{id}/self-reflection", { + params, + body: body("Admin draft"), + }); + expect(first.response.status).toBe(200); + const saved = await trainee.PUT("/api/atc/trainings/{id}/self-reflection", { + params, + body: body("Private reflection"), + }); + expect(saved.response.status).toBe(200); + expect(saved.data?.self_reflection_sheet_filing_id).toBe( + first.data?.self_reflection_sheet_filing_id, + ); + for (const viewer of [trainee, mentor, admin]) { + expect( + ( + await viewer.GET("/api/atc/trainings/{id}/self-reflection-sheet", { + params, + }) + ).response.status, + ).toBe(200); const read = await viewer.GET("/api/atc/trainings/{id}", { params }); - expect(read.response.status).toBe(200); expect(read.data?.self_reflection_sheet_filing?.[0].answer).toBe( "Private reflection", ); + } + const otherMentor = await getClient(["controller-training-mentor"]); + const stranger = await getClient([]); + const staff = await getClient(["staff"]); + for (const denied of [otherMentor, stranger, staff]) { expect( ( - await viewer.PUT("/api/atc/trainings/{id}/self-reflection", { + await denied.GET("/api/atc/trainings/{id}/self-reflection-sheet", { + params, + }) + ).response.status, + ).toBe(403); + } + for (const denied of [mentor, otherMentor, stranger, staff]) { + expect( + ( + await denied.PUT("/api/atc/trainings/{id}/self-reflection", { params, body: body("Denied"), }) ).response.status, ).toBe(403); } - const stranger = await getClient([]); + const hidden = await otherMentor.GET("/api/atc/trainings/{id}", { params }); + expect(hidden.response.status).toBe(200); + expect(hidden.data?.self_reflection_sheet_filing).toBeNull(); + expect(hidden.data?.self_reflection_sheet_filing_id).toBeNull(); + const active = await otherMentor.GET("/api/atc/trainings/active"); + expect( + active.data?.find((row) => row.id === training.id) + ?.self_reflection_sheet_filing, + ).toBeNull(); + const history = await otherMentor.GET("/api/atc/trainings/by-user/{userId}", { + params: { path: { userId: training.trainee_id } }, + }); + expect( + history.data?.find((row) => row.id === training.id) + ?.self_reflection_sheet_filing, + ).toBeNull(); + const updated = await otherMentor.PUT("/api/atc/trainings/{id}", { + params, + body: training, + }); + expect(updated.response.status).toBe(200); + expect(updated.data?.self_reflection_sheet_filing).toBeNull(); + await mentor.GET("/api/atc/trainings/record-sheet"); + const recorded = await otherMentor.PUT("/api/atc/trainings/{id}/record", { + params, + body: { request_answers: [] }, + }); + expect(recorded.response.status).toBe(200); + expect(recorded.data?.self_reflection_sheet_filing).toBeNull(); + const finished = await otherMentor.GET("/api/atc/trainings/finished"); expect( - (await stranger.GET("/api/atc/trainings/{id}", { params })).response.status, - ).toBe(403); + finished.data?.find((row) => row.id === training.id) + ?.self_reflection_sheet_filing, + ).toBeNull(); + const anonymous = await getClient(); expect( ( - await stranger.PUT("/api/atc/trainings/{id}/self-reflection", { + await anonymous.GET("/api/atc/trainings/{id}/self-reflection-sheet", { params, - body: body("Denied"), }) ).response.status, - ).toBe(403); - const anonymous = await getClient(); + ).toBe(401); expect( ( await anonymous.PUT("/api/atc/trainings/{id}/self-reflection", { @@ -120,10 +174,18 @@ test("training viewers can read but only the trainee can edit", async ({ }) ).response.status, ).toBe(401); + const edited = await admin.PUT("/api/atc/trainings/{id}/self-reflection", { + params, + body: body("Admin edit"), + }); + expect(edited.response.status).toBe(200); + expect(edited.data?.self_reflection_sheet_filing_id).toBe( + first.data?.self_reflection_sheet_filing_id, + ); expect( (await trainee.GET("/api/atc/trainings/{id}", { params })).data ?.self_reflection_sheet_filing?.[0].answer, - ).toBe("Private reflection"); + ).toBe("Admin edit"); }); test("concurrent initial saves share a filing; invalid fields do not overwrite answers", async ({ @@ -175,7 +237,10 @@ test("uses configured fields and excludes deleted fields", async ({ body: { name: original.data!.name, fields: [custom] }, }); expect(configured.response.status).toBe(200); - const sheet = await trainee.GET("/api/atc/trainings/self-reflection-sheet"); + const sheet = await trainee.GET( + "/api/atc/trainings/{id}/self-reflection-sheet", + { params: { path: { id: training.id } } }, + ); expect(sheet.data?.fields.map((field) => field.id)).toEqual(["next-steps"]); const saved = await trainee.PUT("/api/atc/trainings/{id}/self-reflection", { params: { path: { id: training.id } }, diff --git a/src/modules/training/routes/trainings.rs b/src/modules/training/routes/trainings.rs index 231e90c..e5dbdcb 100644 --- a/src/modules/training/routes/trainings.rs +++ b/src/modules/training/routes/trainings.rs @@ -1,6 +1,6 @@ use axum::extract::{Path, State}; use axum::http::StatusCode; -use axum::routing::get; +use axum::routing::{get, put}; use axum::{Json, Router}; use ulid::Ulid; @@ -38,18 +38,18 @@ pub fn build_training_routes() -> Router { .route("/by-user/{user_id}", get(list_by_user)) .route("/finished", get(list_finished)) .route("/record-sheet", get(get_record_sheet)) - .route("/self-reflection-sheet", get(get_self_reflection_sheet)) .route( - "/{id}/self-reflection", - axum::routing::put(set_self_reflection), + "/{id}/self-reflection-sheet", + get(get_self_reflection_sheet), ) + .route("/{id}/self-reflection", put(set_self_reflection)) .route( "/{id}", get(get_training) .put(update_training) .delete(delete_training), ) - .route("/{id}/record", axum::routing::put(set_record_sheet)) + .route("/{id}/record", put(set_record_sheet)) } #[utoipa::path(get, path = "api/atc/trainings/active", tag = "Training", security(("oauth2" = [])), responses((status = 200, description = "Successful response", body = Vec)))] @@ -64,7 +64,7 @@ async fn list_active( .list_active(user_id, is_training_history_admin(¤t_user)) .await? .into_iter() - .map(training_to_dto) + .map(|training| training_to_dto(training, ¤t_user)) .collect(), )) } @@ -87,7 +87,7 @@ async fn list_by_user( ) .await? .into_iter() - .map(training_to_dto) + .map(|training| training_to_dto(training, ¤t_user)) .collect(), )) } @@ -104,7 +104,7 @@ async fn list_finished( .list_finished(user_id, is_training_history_admin(¤t_user)) .await? .into_iter() - .map(training_to_dto) + .map(|training| training_to_dto(training, ¤t_user)) .collect(), )) } @@ -124,7 +124,7 @@ async fn get_training( is_training_history_admin(¤t_user), ) .await?; - Ok(Json(training_to_dto(training))) + Ok(Json(training_to_dto(training, ¤t_user))) } #[utoipa::path(post, path = "api/atc/trainings", tag = "Training", security(("oauth2" = [])), request_body = TrainingSaveRequest, responses((status = 200, description = "Successful response", body = TrainingDto)))] @@ -143,7 +143,7 @@ async fn create_training( current_user.has_role(UserRole::ControllerTrainingDirectorAssistant), ) .await?; - Ok(Json(training_to_dto(training))) + Ok(Json(training_to_dto(training, ¤t_user))) } #[utoipa::path(put, path = "api/atc/trainings/{id}", tag = "Training", security(("oauth2" = [])), params(("id" = String, Path, description = "Training ULID")), request_body = TrainingSaveRequest, responses((status = 200, description = "Successful response", body = TrainingDto)))] @@ -164,7 +164,7 @@ async fn update_training( current_user.has_role(UserRole::ControllerTrainingMentor), ) .await?; - Ok(Json(training_to_dto(training))) + Ok(Json(training_to_dto(training, ¤t_user))) } #[utoipa::path(get, path = "api/atc/trainings/record-sheet", tag = "Training", security(("oauth2" = [])), responses((status = 200, description = "Successful response", body = SheetDto)))] @@ -205,13 +205,24 @@ async fn set_record_sheet( current_user.has_role(UserRole::ControllerTrainingMentor), ) .await?; - Ok(Json(training_to_dto(training))) + Ok(Json(training_to_dto(training, ¤t_user))) } -#[utoipa::path(get, path = "api/atc/trainings/self-reflection-sheet", tag = "Training", security(("oauth2" = [])), responses((status = 200, description = "Successful response", body = SheetDto)))] +#[utoipa::path(get, path = "api/atc/trainings/{id}/self-reflection-sheet", tag = "Training", security(("oauth2" = [])), params(("id" = String, Path, description = "Training ULID")), responses((status = 200, description = "Successful response", body = SheetDto)))] async fn get_self_reflection_sheet( State(services): State, + current_user: CurrentUser, + Path(id): Path, ) -> Result, ApiError> { + let user_id = current_user.user_id.ok_or(ApiError::Unauthorized)?; + services + .training() + .find_visible( + id.parse::()?.into(), + user_id, + current_user.has_role(UserRole::ControllerTrainingDirectorAssistant), + ) + .await?; let view = services.sheet().find(SELF_REFLECTION_SHEET_ID).await?; Ok(Json(SheetDto::from_entities( view.sheet, @@ -237,9 +248,14 @@ async fn set_self_reflection( .collect::>(); let training = services .training() - .set_self_reflection(id.parse::()?.into(), &answers, user_id) + .set_self_reflection( + id.parse::()?.into(), + &answers, + user_id, + current_user.has_role(UserRole::ControllerTrainingDirectorAssistant), + ) .await?; - Ok(Json(training_to_dto(training))) + Ok(Json(training_to_dto(training, ¤t_user))) } #[utoipa::path(delete, path = "api/atc/trainings/{id}", tag = "Training", security(("oauth2" = [])), params(("id" = String, Path, description = "Training ULID")), responses((status = 204, description = "No content")))] @@ -261,7 +277,15 @@ async fn delete_training( Ok(StatusCode::NO_CONTENT) } -fn training_to_dto(view: TrainingView) -> TrainingDto { +fn training_to_dto(mut view: TrainingView, current_user: &CurrentUser) -> TrainingDto { + // Training history is visible to other mentors, but self reflection is not. + if current_user.user_id != Some(view.training.trainee_id) + && current_user.user_id != Some(view.training.trainer_id) + && !current_user.has_role(UserRole::ControllerTrainingDirectorAssistant) + { + view.self_reflection_sheet_filing = None; + view.training.self_reflection_sheet_filing_id = None; + } TrainingDto::from_entity( view.training, view.trainer, diff --git a/src/modules/training/service.rs b/src/modules/training/service.rs index 36c4889..7d30fa4 100644 --- a/src/modules/training/service.rs +++ b/src/modules/training/service.rs @@ -166,6 +166,7 @@ impl TrainingService { id: Uuid, answers: &[SheetAnswerSave], current_user_id: Uuid, + is_admin: bool, ) -> Result { let mut transaction = self.db.begin().await?; // Serialize edits, including the first filing, for this training. @@ -173,7 +174,7 @@ impl TrainingService { .lock_training_by_id(id) .await? .ok_or(TrainingServiceError::NotFound(id))?; - if training.trainee_id != current_user_id { + if training.trainee_id != current_user_id && !is_admin { return Err(TrainingServiceError::NotOwned { entity: "training", id, @@ -183,7 +184,7 @@ impl TrainingService { .set_sheet_filing( SELF_REFLECTION_SHEET_ID, training.self_reflection_sheet_filing_id, - current_user_id, + training.trainee_id, answers, ) .await?; From 6771b78b6b7c21136e7bdd53297c07bf5d3d358a Mon Sep 17 00:00:00 2001 From: Yuze Fu Date: Thu, 24 Sep 2026 19:40:04 +0900 Subject: [PATCH 3/3] fix: align self reflection visibility with training records --- .../atc/trainings/id/self-reflection.test.ts | 34 ++++++++++++------- src/modules/training/routes/trainings.rs | 28 ++++++--------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/e2e/src/atc/trainings/id/self-reflection.test.ts b/e2e/src/atc/trainings/id/self-reflection.test.ts index 43c3d5e..0d8a28c 100644 --- a/e2e/src/atc/trainings/id/self-reflection.test.ts +++ b/e2e/src/atc/trainings/id/self-reflection.test.ts @@ -69,7 +69,7 @@ test("trainee saves before training and edits after training without changing me expect(loaded.data?.record_sheet_filing).toBeNull(); }); -test("reflection access is limited to the trainee, assigned trainer and training director assistant", async ({ +test("mentors can read reflection through training endpoints but only the trainee and training director assistant can write", async ({ trainee, mentor, training, @@ -89,7 +89,8 @@ test("reflection access is limited to the trainee, assigned trainer and training expect(saved.data?.self_reflection_sheet_filing_id).toBe( first.data?.self_reflection_sheet_filing_id, ); - for (const viewer of [trainee, mentor, admin]) { + const otherMentor = await getClient(["controller-training-mentor"]); + for (const viewer of [trainee, mentor, otherMentor, admin]) { expect( ( await viewer.GET("/api/atc/trainings/{id}/self-reflection-sheet", { @@ -102,10 +103,9 @@ test("reflection access is limited to the trainee, assigned trainer and training "Private reflection", ); } - const otherMentor = await getClient(["controller-training-mentor"]); const stranger = await getClient([]); const staff = await getClient(["staff"]); - for (const denied of [otherMentor, stranger, staff]) { + for (const denied of [stranger, staff]) { expect( ( await denied.GET("/api/atc/trainings/{id}/self-reflection-sheet", { @@ -124,40 +124,48 @@ test("reflection access is limited to the trainee, assigned trainer and training ).response.status, ).toBe(403); } - const hidden = await otherMentor.GET("/api/atc/trainings/{id}", { params }); - expect(hidden.response.status).toBe(200); - expect(hidden.data?.self_reflection_sheet_filing).toBeNull(); - expect(hidden.data?.self_reflection_sheet_filing_id).toBeNull(); + const visible = await otherMentor.GET("/api/atc/trainings/{id}", { params }); + expect(visible.response.status).toBe(200); + expect(visible.data?.self_reflection_sheet_filing).toEqual( + saved.data?.self_reflection_sheet_filing, + ); + expect(visible.data?.self_reflection_sheet_filing_id).toBe( + saved.data?.self_reflection_sheet_filing_id, + ); const active = await otherMentor.GET("/api/atc/trainings/active"); expect( active.data?.find((row) => row.id === training.id) ?.self_reflection_sheet_filing, - ).toBeNull(); + ).toEqual(saved.data?.self_reflection_sheet_filing); const history = await otherMentor.GET("/api/atc/trainings/by-user/{userId}", { params: { path: { userId: training.trainee_id } }, }); expect( history.data?.find((row) => row.id === training.id) ?.self_reflection_sheet_filing, - ).toBeNull(); + ).toEqual(saved.data?.self_reflection_sheet_filing); const updated = await otherMentor.PUT("/api/atc/trainings/{id}", { params, body: training, }); expect(updated.response.status).toBe(200); - expect(updated.data?.self_reflection_sheet_filing).toBeNull(); + expect(updated.data?.self_reflection_sheet_filing).toEqual( + saved.data?.self_reflection_sheet_filing, + ); await mentor.GET("/api/atc/trainings/record-sheet"); const recorded = await otherMentor.PUT("/api/atc/trainings/{id}/record", { params, body: { request_answers: [] }, }); expect(recorded.response.status).toBe(200); - expect(recorded.data?.self_reflection_sheet_filing).toBeNull(); + expect(recorded.data?.self_reflection_sheet_filing).toEqual( + saved.data?.self_reflection_sheet_filing, + ); const finished = await otherMentor.GET("/api/atc/trainings/finished"); expect( finished.data?.find((row) => row.id === training.id) ?.self_reflection_sheet_filing, - ).toBeNull(); + ).toEqual(saved.data?.self_reflection_sheet_filing); const anonymous = await getClient(); expect( ( diff --git a/src/modules/training/routes/trainings.rs b/src/modules/training/routes/trainings.rs index e5dbdcb..d3f82b4 100644 --- a/src/modules/training/routes/trainings.rs +++ b/src/modules/training/routes/trainings.rs @@ -64,7 +64,7 @@ async fn list_active( .list_active(user_id, is_training_history_admin(¤t_user)) .await? .into_iter() - .map(|training| training_to_dto(training, ¤t_user)) + .map(training_to_dto) .collect(), )) } @@ -87,7 +87,7 @@ async fn list_by_user( ) .await? .into_iter() - .map(|training| training_to_dto(training, ¤t_user)) + .map(training_to_dto) .collect(), )) } @@ -104,7 +104,7 @@ async fn list_finished( .list_finished(user_id, is_training_history_admin(¤t_user)) .await? .into_iter() - .map(|training| training_to_dto(training, ¤t_user)) + .map(training_to_dto) .collect(), )) } @@ -124,7 +124,7 @@ async fn get_training( is_training_history_admin(¤t_user), ) .await?; - Ok(Json(training_to_dto(training, ¤t_user))) + Ok(Json(training_to_dto(training))) } #[utoipa::path(post, path = "api/atc/trainings", tag = "Training", security(("oauth2" = [])), request_body = TrainingSaveRequest, responses((status = 200, description = "Successful response", body = TrainingDto)))] @@ -143,7 +143,7 @@ async fn create_training( current_user.has_role(UserRole::ControllerTrainingDirectorAssistant), ) .await?; - Ok(Json(training_to_dto(training, ¤t_user))) + Ok(Json(training_to_dto(training))) } #[utoipa::path(put, path = "api/atc/trainings/{id}", tag = "Training", security(("oauth2" = [])), params(("id" = String, Path, description = "Training ULID")), request_body = TrainingSaveRequest, responses((status = 200, description = "Successful response", body = TrainingDto)))] @@ -164,7 +164,7 @@ async fn update_training( current_user.has_role(UserRole::ControllerTrainingMentor), ) .await?; - Ok(Json(training_to_dto(training, ¤t_user))) + Ok(Json(training_to_dto(training))) } #[utoipa::path(get, path = "api/atc/trainings/record-sheet", tag = "Training", security(("oauth2" = [])), responses((status = 200, description = "Successful response", body = SheetDto)))] @@ -205,7 +205,7 @@ async fn set_record_sheet( current_user.has_role(UserRole::ControllerTrainingMentor), ) .await?; - Ok(Json(training_to_dto(training, ¤t_user))) + Ok(Json(training_to_dto(training))) } #[utoipa::path(get, path = "api/atc/trainings/{id}/self-reflection-sheet", tag = "Training", security(("oauth2" = [])), params(("id" = String, Path, description = "Training ULID")), responses((status = 200, description = "Successful response", body = SheetDto)))] @@ -220,7 +220,7 @@ async fn get_self_reflection_sheet( .find_visible( id.parse::()?.into(), user_id, - current_user.has_role(UserRole::ControllerTrainingDirectorAssistant), + is_training_history_admin(¤t_user), ) .await?; let view = services.sheet().find(SELF_REFLECTION_SHEET_ID).await?; @@ -255,7 +255,7 @@ async fn set_self_reflection( current_user.has_role(UserRole::ControllerTrainingDirectorAssistant), ) .await?; - Ok(Json(training_to_dto(training, ¤t_user))) + Ok(Json(training_to_dto(training))) } #[utoipa::path(delete, path = "api/atc/trainings/{id}", tag = "Training", security(("oauth2" = [])), params(("id" = String, Path, description = "Training ULID")), responses((status = 204, description = "No content")))] @@ -277,15 +277,7 @@ async fn delete_training( Ok(StatusCode::NO_CONTENT) } -fn training_to_dto(mut view: TrainingView, current_user: &CurrentUser) -> TrainingDto { - // Training history is visible to other mentors, but self reflection is not. - if current_user.user_id != Some(view.training.trainee_id) - && current_user.user_id != Some(view.training.trainer_id) - && !current_user.has_role(UserRole::ControllerTrainingDirectorAssistant) - { - view.self_reflection_sheet_filing = None; - view.training.self_reflection_sheet_filing_id = None; - } +fn training_to_dto(view: TrainingView) -> TrainingDto { TrainingDto::from_entity( view.training, view.trainer,