feat: Implement Daily Treatment Note backend + fix base-branch test and migration failures (RQ-DN-001 through RQ-DN-022)#68
Conversation
…-022) Agent-Logs-Url: https://github.com/BlackHouseDeveloper/PTDoc/sessions/78632f5d-a6b1-44ad-b198-670c03770912 Co-authored-by: BlackHouseDeveloper <20420616+BlackHouseDeveloper@users.noreply.github.com>
…cast simplification, comment Agent-Logs-Url: https://github.com/BlackHouseDeveloper/PTDoc/sessions/78632f5d-a6b1-44ad-b198-670c03770912 Co-authored-by: BlackHouseDeveloper <20420616+BlackHouseDeveloper@users.noreply.github.com>
|
@copilot 6 failing checks |
…roviders Agent-Logs-Url: https://github.com/BlackHouseDeveloper/PTDoc/sessions/92a75825-7449-4cf5-9973-dbac2b8573aa Co-authored-by: BlackHouseDeveloper <20420616+BlackHouseDeveloper@users.noreply.github.com>
Investigated all 6 failing checks. Here's the breakdown: Pre-existing failures (not caused by my changes):
Fixed:
The migration drift check now returns exit code 0: |
|
@copilot 5 failed checks |
…tification center commit Agent-Logs-Url: https://github.com/BlackHouseDeveloper/PTDoc/sessions/72f22668-2d79-4bf4-9206-7248066de6d8 Co-authored-by: BlackHouseDeveloper <20420616+BlackHouseDeveloper@users.noreply.github.com>
Found and fixed all remaining test failures in commit 7 FK constraint failures ( 1 type assertion failure ( 1 PHI substring collision ( Full test suite: 713 passed, 0 failed (10 skipped — SQL Server/Postgres skipped, no live DB in CI). |
There was a problem hiding this comment.
Pull request overview
Implements the server-side Daily Treatment Note workflow (new models/DTOs/service/endpoints), and fixes base-branch EF Core migration drift + several authorization/compliance test failures related to the notification center feature.
Changes:
- Added Daily Treatment Note backend surface area: domain model + DTOs +
IDailyNoteService+DailyNoteService+/api/daily-notesminimal API endpoints. - Added
AddUserNotificationsmigrations (SQLite/SQL Server/Postgres) and updated provider model snapshots to eliminate migration drift. - Updated RBAC route inventory and fixed several failing security/compliance tests (FK setup + result type assertion + PHI assertion).
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/PTDoc.Tests/Security/PfptRoleComplianceTests.cs | Fixes failing RBAC/compliance tests by ensuring required FK setup and correcting result type assertions. |
| tests/PTDoc.Tests/Security/AuthorizationCoverageTests.cs | Extends authorization inventory to include /api/daily-notes routes. |
| tests/PTDoc.Tests/Application/IntakeConsentJsonTests.cs | Adjusts PHI-exclusion assertion (but currently introduces a redundant duplicate assertion). |
| src/PTDoc.Infrastructure/Services/DailyNoteService.cs | Implements Daily Note draft upsert, eval carry-forward parsing, AI narrative generation, CPT time calc, and medical-necessity checks. |
| src/PTDoc.Infrastructure.Migrations.Sqlite/Migrations/ApplicationDbContextModelSnapshot.cs | Updates SQLite snapshot for UserNotifications/UserNotificationPreferences. |
| src/PTDoc.Infrastructure.Migrations.Sqlite/Migrations/20260330035949_AddUserNotifications.cs | Adds SQLite migration for notifications (namespace currently inconsistent with existing migrations). |
| src/PTDoc.Infrastructure.Migrations.Sqlite/Migrations/20260330035949_AddUserNotifications.Designer.cs | Auto-generated SQLite designer for the migration. |
| src/PTDoc.Infrastructure.Migrations.SqlServer/Migrations/ApplicationDbContextModelSnapshot.cs | Updates SQL Server snapshot for notification entities (relationship config needs to match DbContext). |
| src/PTDoc.Infrastructure.Migrations.SqlServer/Migrations/20260330000001_AddUserNotifications.cs | Adds SQL Server migration for notifications. |
| src/PTDoc.Infrastructure.Migrations.Postgres/Migrations/ApplicationDbContextModelSnapshot.cs | Updates Postgres snapshot for notification entities (relationship config needs to match DbContext). |
| src/PTDoc.Infrastructure.Migrations.Postgres/Migrations/20260330000001_AddUserNotifications.cs | Adds Postgres migration for notifications. |
| src/PTDoc.Core/Models/DailyNoteContent.cs | Adds the domain model for Daily Note JSON content. |
| src/PTDoc.Application/Services/IDailyNoteService.cs | Introduces the service contract for Daily Note operations. |
| src/PTDoc.Application/DTOs/DailyNoteDtos.cs | Adds DTO surface area for Daily Note endpoints. |
| src/PTDoc.Api/Program.cs | Registers IDailyNoteService and maps Daily Note endpoints. |
| src/PTDoc.Api/Notes/DailyNoteEndpoints.cs | Adds /api/daily-notes endpoint group with policy requirements. |
Files not reviewed (1)
- src/PTDoc.Infrastructure.Migrations.Sqlite/Migrations/20260330035949_AddUserNotifications.Designer.cs: Language not supported
|
|
||
| #nullable disable | ||
|
|
||
| namespace PTDoc.Infrastructure.Data.Migrations.Migrations |
There was a problem hiding this comment.
This migration’s namespace (PTDoc.Infrastructure.Data.Migrations.Migrations) doesn’t match the established SQLite migrations namespace (PTDoc.Infrastructure.Data.Migrations, e.g., 20260319214057_AddExternalIdentityMapping.cs). Aligning the namespace with the rest of the migrations keeps tooling/maintenance consistent and avoids confusion when scanning migrations across providers.
| namespace PTDoc.Infrastructure.Data.Migrations.Migrations | |
| namespace PTDoc.Infrastructure.Data.Migrations |
| modelBuilder.Entity("PTDoc.Core.Models.UserNotificationPreferences", b => | ||
| { | ||
| b.HasOne("PTDoc.Core.Models.User", "User") | ||
| .WithMany() | ||
| .HasForeignKey("UserId") | ||
| .OnDelete(DeleteBehavior.Cascade) | ||
| .IsRequired(); |
There was a problem hiding this comment.
In the Postgres model snapshot, UserNotificationPreferences is configured as a one-to-many (.WithMany()) relationship. ApplicationDbContext configures this as one-to-one (HasOne(...).WithOne().HasForeignKey<UserNotificationPreferences>(...)), and the SQLite snapshot reflects that. Update this snapshot to match the actual model configuration to avoid provider-specific drift and future migration inconsistencies.
| public async Task<DailyNoteResponse> SaveDraftAsync(SaveDailyNoteRequest request, CancellationToken ct = default) | ||
| { | ||
| var startOfDay = request.DateOfService.Date; | ||
| var startOfNextDay = startOfDay.AddDays(1); | ||
| var note = await _db.ClinicalNotes | ||
| .FirstOrDefaultAsync(n => | ||
| n.PatientId == request.PatientId && | ||
| n.NoteType == NoteType.Daily && | ||
| n.DateOfService >= startOfDay && | ||
| n.DateOfService < startOfNextDay && | ||
| n.SignedUtc == null, ct); |
There was a problem hiding this comment.
SaveDraftAsync can throw a DbUpdateException (FK violation) and return a 500 when PatientId doesn’t exist (or AppointmentId is invalid) because it writes the ClinicalNote without validating foreign keys first. Consider matching the existing note endpoints’ pattern: validate PatientId != Guid.Empty, DateOfService != default, verify the patient exists in the current tenant scope, and if AppointmentId is provided verify it exists and belongs to the same patient, returning a 404/422 rather than failing at SaveChanges.
| catch (Exception) | ||
| { | ||
| // AI generation unavailable — return template narrative | ||
| return templateResult; | ||
| } |
There was a problem hiding this comment.
GenerateAssessmentNarrativeAsync catches all exceptions, which will also swallow OperationCanceledException and return a fallback response even when the request was cancelled. Cancellation should propagate so callers can abort work promptly; update the catch to let OperationCanceledException bubble (or catch it explicitly and rethrow).
| var templateResult = BuildAssessmentNarrativeFromTemplate(content); | ||
| if (!string.IsNullOrWhiteSpace(templateResult)) | ||
| return templateResult; |
There was a problem hiding this comment.
The implementation returns the template narrative whenever it can build one, so the AI service is only invoked when the template result is empty. This contradicts the PR description (“falls back to a deterministic template narrative when the AI service is unavailable or returns empty”). If the intended behavior is AI-first with template fallback, invert the decision order; if template-first is intended, please update the PR description (and any consuming expectations) accordingly.
| public static WebApplication MapDailyNoteEndpoints(this WebApplication app) | ||
| { | ||
| var group = app.MapGroup("/api/daily-notes").RequireAuthorization(); | ||
|
|
||
| group.MapGet("/patient/{patientId:guid}", async (Guid patientId, IDailyNoteService service, CancellationToken ct) => | ||
| Results.Ok(await service.GetForPatientAsync(patientId, ct: ct))) | ||
| .RequireAuthorization(AuthorizationPolicies.NoteRead) | ||
| .WithName("GetDailyNotesForPatient"); | ||
|
|
||
| group.MapGet("/{noteId:guid}", async (Guid noteId, IDailyNoteService service, CancellationToken ct) => | ||
| { | ||
| var note = await service.GetByIdAsync(noteId, ct); | ||
| return note is null ? Results.NotFound() : Results.Ok(note); | ||
| }).RequireAuthorization(AuthorizationPolicies.NoteRead) | ||
| .WithName("GetDailyNoteById"); | ||
|
|
||
| group.MapPost("/", async ([FromBody] SaveDailyNoteRequest request, IDailyNoteService service, CancellationToken ct) => | ||
| Results.Ok(await service.SaveDraftAsync(request, ct))) | ||
| .RequireAuthorization(AuthorizationPolicies.NoteWrite) | ||
| .WithName("SaveDailyNoteDraft"); | ||
|
|
||
| group.MapPost("/generate-assessment", async ([FromBody] DailyNoteContentDto content, IDailyNoteService service, CancellationToken ct) => | ||
| { | ||
| var narrative = await service.GenerateAssessmentNarrativeAsync(content, ct); | ||
| return Results.Ok(new { narrative }); | ||
| }).RequireAuthorization(AuthorizationPolicies.NoteWrite) | ||
| .WithName("GenerateAssessmentNarrative"); | ||
|
|
||
| group.MapPost("/cpt-time", ([FromBody] CptTimeCalculationRequest request, IDailyNoteService service) => | ||
| Results.Ok(service.CalculateCptTime(request))) | ||
| .RequireAuthorization(AuthorizationPolicies.ClinicalStaff) | ||
| .WithName("CalculateCptTime"); | ||
|
|
||
| group.MapPost("/check-medical-necessity", ([FromBody] DailyNoteContentDto content, IDailyNoteService service) => | ||
| Results.Ok(service.CheckMedicalNecessity(content))) | ||
| .RequireAuthorization(AuthorizationPolicies.ClinicalStaff) | ||
| .WithName("CheckMedicalNecessity"); | ||
|
|
||
| group.MapGet("/eval-carry-forward/{patientId:guid}", async (Guid patientId, IDailyNoteService service, CancellationToken ct) => | ||
| Results.Ok(await service.GetEvalCarryForwardAsync(patientId, ct))) | ||
| .RequireAuthorization(AuthorizationPolicies.NoteRead) | ||
| .WithName("GetEvalCarryForward"); |
There was a problem hiding this comment.
No automated tests exercise the new /api/daily-notes endpoints or DailyNoteService behavior (upsert-by-date, tenant scoping/ClinicId assignment, signed-note immutability, CPT unit calculation, medical-necessity checks). The repo has existing integration/security tests for other endpoints, so this feature should add at least a small set of endpoint-level tests (WebApplicationFactory) to prevent regressions.
| modelBuilder.Entity("PTDoc.Core.Models.UserNotificationPreferences", b => | ||
| { | ||
| b.HasOne("PTDoc.Core.Models.User", "User") | ||
| .WithMany() |
There was a problem hiding this comment.
In the SQL Server model snapshot, UserNotificationPreferences is configured as a one-to-many (.WithMany()) relationship. ApplicationDbContext configures this as one-to-one (HasOne(...).WithOne().HasForeignKey<UserNotificationPreferences>(...)), and the SQLite snapshot reflects that. Update this snapshot to match the actual model configuration to avoid provider-specific drift and future migration inconsistencies.
| .WithMany() | |
| .WithOne() |
| note = new ClinicalNote | ||
| { | ||
| PatientId = request.PatientId, | ||
| AppointmentId = request.AppointmentId, | ||
| NoteType = NoteType.Daily, | ||
| DateOfService = request.DateOfService, | ||
| ContentJson = contentJson, | ||
| CptCodesJson = cptCodesJson | ||
| }; |
There was a problem hiding this comment.
When creating/updating the ClinicalNote draft, required tracking + tenant fields aren’t set (e.g., ClinicId, LastModifiedUtc, ModifiedByUserId). Because ClinicalNote is tenant-scoped via a global query filter, leaving ClinicId null makes the newly created note invisible to tenant-scoped reads and breaks the upsert (future saves won’t find it). Also, LastModifiedUtc drives client sync pull filters, so the default value will prevent notes from syncing. Set ClinicId from the current tenant context, set LastModifiedUtc/ModifiedByUserId on both insert and update, and enqueue the corresponding sync operation (create/update) like NoteEndpoints does.
| .WithName("GetDailyNoteById"); | ||
|
|
||
| group.MapPost("/", async ([FromBody] SaveDailyNoteRequest request, IDailyNoteService service, CancellationToken ct) => | ||
| Results.Ok(await service.SaveDraftAsync(request, ct))) |
There was a problem hiding this comment.
The save-draft endpoint always returns 200 OK and doesn’t validate basic request invariants (e.g., PatientId != Guid.Empty, DateOfService provided). Since SaveDraftAsync currently relies on EF constraints, invalid requests can surface as 500s. Consider mirroring the existing notes endpoints’ approach: return ValidationProblem for missing fields and a NotFound/UnprocessableEntity for missing/mismatched Patient/Appointment IDs.
| Results.Ok(await service.SaveDraftAsync(request, ct))) | |
| { | |
| var errors = new Dictionary<string, string[]>(); | |
| if (request.PatientId == Guid.Empty) | |
| { | |
| errors["PatientId"] = new[] { "PatientId is required." }; | |
| } | |
| if (request.DateOfService == default) | |
| { | |
| errors["DateOfService"] = new[] { "DateOfService is required." }; | |
| } | |
| if (errors.Count > 0) | |
| { | |
| return Results.ValidationProblem(errors); | |
| } | |
| var result = await service.SaveDraftAsync(request, ct); | |
| return Results.Ok(result); | |
| }) |
| Assert.DoesNotContain("patient@example.com", json, StringComparison.OrdinalIgnoreCase); | ||
| Assert.DoesNotContain("communicationPhoneNumber", json, StringComparison.OrdinalIgnoreCase); | ||
| Assert.DoesNotContain("communicationEmail", json, StringComparison.OrdinalIgnoreCase); | ||
| Assert.DoesNotContain("patient@example.com", json, StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
This test now asserts the same email value exclusion twice, which is redundant and likely accidental (it previously checked the presence of the email field name). Remove the duplicate assertion or replace it with a more specific check that still verifies the intended behavior without duplicating the same condition.
| Assert.DoesNotContain("patient@example.com", json, StringComparison.OrdinalIgnoreCase); | |
| Assert.DoesNotContain("communicationEmail", json, StringComparison.OrdinalIgnoreCase); |
|
@copilot apply changes based on the comments in this thread |
…ation, AI-first logic, input validation, duplicate assertion Agent-Logs-Url: https://github.com/BlackHouseDeveloper/PTDoc/sessions/6b4eb7cb-475f-4aee-bc74-f7b67e075a41 Co-authored-by: BlackHouseDeveloper <20420616+BlackHouseDeveloper@users.noreply.github.com>
|
@copilot create New unit/integration tests for /api/daily-notes endpoints |
… signed immutability, CPT billing, medical necessity, AI-first fallback) Agent-Logs-Url: https://github.com/BlackHouseDeveloper/PTDoc/sessions/9e8240a7-8726-4006-880d-fa0ad5e4b6c2 Co-authored-by: BlackHouseDeveloper <20420616+BlackHouseDeveloper@users.noreply.github.com>
Added 23 unit tests for
All 736 tests pass (0 failed). |
9160784
into
Client-Data-Implementation
Implements the complete server-side backend for the Daily Treatment Note workflow and fixes pre-existing EF Core migration drift and test failures introduced by the notification center feature on the base branch. Daily note content serializes into the existing
ClinicalNote.ContentJsonblob — no new application tables required.Summary
Title
Implement Daily Treatment Note backend + fix base-branch test and migration failures (RQ-DN-001 through RQ-DN-022)
Description
What Changed
src/PTDoc.Core/Models/DailyNoteContent.cs— New domain model covering all SOAP sections (Subjective/Objective/Education/Assessment/Plan).CptCodeEntryrenamed toDailyNoteCptCodeto avoid collision with the existingPTDoc.Application.Compliance.CptCodeEntry.src/PTDoc.Application/DTOs/DailyNoteDtos.cs— Full DTO surface:SaveDailyNoteRequest,DailyNoteResponse,DailyNoteContentDto,MedicalNecessityCheckResult,EvalCarryForwardResponse, CPT billing types.src/PTDoc.Application/Services/IDailyNoteService.cs— Service contract updated:SaveDraftAsyncreturns(DailyNoteResponse?, string?)tuple to communicate FK-validation errors without exceptions.src/PTDoc.Infrastructure/Services/DailyNoteService.cs— Full implementation with post-review improvements:ITenantContextAccessor,IIdentityContextAccessor,ISyncEngine; setsClinicId,LastModifiedUtc,ModifiedByUserId,SyncStateon every create/update; enqueuesSyncOperation.Create/UpdateafterSaveChangesAsync.PatientIdexists (404) andAppointmentIdexists and belongs to the same patient (404/mismatch) before writing to the database — mirrors the existingNoteEndpointspattern.IAiClinicalGenerationService.GenerateAssessmentAsyncfirst and falls back to the deterministic template only when AI is unavailable or returns empty.OperationCanceledExceptionre-throws instead of being swallowed.(PatientId, NoteType.Daily, date)using date-range predicate (index-friendly; avoids.Datein EF predicate); signed notes are excluded from the upsert predicate to preserve immutability.minutes/15 + (minutes%15 >= 8 ? 1 : 0)).src/PTDoc.Api/Notes/DailyNoteEndpoints.cs— Minimal API group at/api/daily-noteswith 7 endpoints. Save-draft endpoint now validatesPatientId != Guid.EmptyandDateOfService != default(returnsValidationProblem) and returns404on FK errors reported by the service.src/PTDoc.Api/Program.cs— RegistersIDailyNoteService → DailyNoteService; callsapp.MapDailyNoteEndpoints().tests/PTDoc.Tests/Security/AuthorizationCoverageTests.cs— 7 new inventory entries + structural coverage assertion for/api/daily-notes.tests/PTDoc.Tests/Notes/DailyNoteServiceTests.cs— 23 new unit tests covering:ClinicIdset fromITenantContextAccessor)LastModifiedUtc/ModifiedByUserIdset on create and update)SyncOperation.Createon insert,SyncOperation.Updateon upsert)MissingElementsOperationCanceledExceptionpropagatesGetByIdAsync/GetEvalCarryForwardAsyncnull/empty-result pathssrc/PTDoc.Infrastructure.Migrations.Sqlite/Migrations/20260330035949_AddUserNotifications.cs— New migration forUserNotificationsandUserNotificationPreferencestables (SQLite); namespace corrected toPTDoc.Infrastructure.Data.Migrations(matching all other SQLite migrations).src/PTDoc.Infrastructure.Migrations.SqlServer/Migrations/20260330000001_AddUserNotifications.cs— Same migration for SQL Server.src/PTDoc.Infrastructure.Migrations.Postgres/Migrations/20260330000001_AddUserNotifications.cs— Same migration for PostgreSQL.ApplicationDbContextModelSnapshot.csfiles — Updated to includeUserNotificationandUserNotificationPreferencesentity definitions. SQL Server and Postgres snapshots corrected:UserNotificationPreferencesrelationship changed from.WithMany()to.WithOne()withHasForeignKey("PTDoc.Core.Models.UserNotificationPreferences", "UserId")to match the actual one-to-one configuration inApplicationDbContext.tests/PTDoc.Tests/Security/PfptRoleComplianceTests.cs— Fixed 8 pre-existing test failures introduced by the base-branch notification center commit.tests/PTDoc.Tests/Application/IntakeConsentJsonTests.cs— Removed duplicateDoesNotContain("patient@example.com")assertion; replaced withDoesNotContain("communicationEmail")to verify field-name exclusion without redundancy.Why
AddUserNotificationsmigrations fix thedotnet-ef migrations has-pending-model-changesCI gate failure that was blocking the Database Provider CI workflow.How
ICarryForwardService/CarryForwardServiceintentionally left untouched.ClinicalNote.ContentJson; no new application schema required.Type of Change
Impact Areas
Checklist
Core Requirements
[Unreleased]section in docs/CHANGELOG.mdCode Quality & Testing
dotnet format --verify-no-changes)roslynator analyze PTDoc.sln --severity-level info)Build Artifacts
Functional Testing
Platform-Specific Validation
Documentation & Communication
Testing Instructions
1. Setup
2. Functional Testing
Expected Behavior:
PatientId = Guid.Emptyor missingDateOfServicereturns400 ValidationProblemPatientIdreturns404(not a 500)passes: falsewithmissingElementslist when required fields absentClinicIdset correctly)dotnet-ef migrations has-pending-model-changesreturns exit code 0 (no model drift)3. Accessibility Testing (if UI changes)
No UI changes.
4. Cross-Platform Testing (if applicable)
# API / net8.0 only — no MAUI-specific changes dotnet build src/PTDoc.Api/PTDoc.Api.csproj -c ReleaseVerification Commands
Review Feedback
For Reviewers: Please comment on the following areas (check all that you verified):
Design & Usability (if UI changes)
N/A — backend only.
Code Quality
Performance & Testing
Documentation
Additional Context
ICarryForwardService/CarryForwardServicewere intentionally not replaced — the existing implementations cover a broader contract than the spec stub and should not regress.IDailyNoteService.SaveDraftAsyncnow returns(DailyNoteResponse?, string?)— callers receive a structured error string instead of catchingDbUpdateException. The endpoint translates a non-null error to404 NotFound.DailyNoteServicenow injectsITenantContextAccessor,IIdentityContextAccessor, andISyncEngine(constructor injection, Scoped lifetime) to mirror the tenant/sync behavior ofNoteEndpoints.GenerateAssessmentNarrativeAsyncnow calls the AI service first and falls back to the template narrative only when AI is unavailable or returns empty.OperationCanceledExceptionis no longer swallowed.AddUserNotificationsSQLite migration namespace was corrected fromPTDoc.Infrastructure.Data.Migrations.MigrationstoPTDoc.Infrastructure.Data.Migrationsto match all other migrations in that assembly.UserNotificationPreferencesnow uses.WithOne()+ qualifiedHasForeignKey(...)to match the one-to-one configuration inApplicationDbContext.Healthcare Context Reminder: PTDoc is a HIPAA-conscious application. Ensure all patient data handling includes:
Original prompt
Implement the complete backend for the Daily Treatment Note workflow on the
Client-Data-Implementationbranch. All requirements are sourced fromExtracted_Requirements_v1.md(RQ-DN-001 through RQ-DN-022).Key Architectural Rules
ClinicalNote.ContentJsonblob is the storage pattern — do NOT add new tables or migrations.NoteType.Daily = 1already exists inClinicalNote.cs.CancellationToken.File 1:
src/PTDoc.Core/Models/DailyNoteContent.csCreate a strongly-typed content model that serializes into
ClinicalNote.ContentJson: