Add project identification workflow setup checklist#39
Open
sprucely wants to merge 7 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a persisted project-identification workflow with fiscal-period selection, checklist progression, flat-file and PGM import integration, project-list APIs, SQL projections, and a checklist-driven client UI with validation coverage. ChangesProject identification workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ProjectIdentificationStage
participant ProjectIdentificationController
participant ProjectIdentificationService
participant AppDbContext
participant ProjectListService
User->>ProjectIdentificationStage: open project-identification stage
ProjectIdentificationStage->>ProjectIdentificationController: GET setup
ProjectIdentificationController->>ProjectIdentificationService: load workflow setup
ProjectIdentificationService->>AppDbContext: load or create current workflow
AppDbContext-->>ProjectIdentificationService: workflow state and imports
ProjectIdentificationService-->>ProjectIdentificationStage: setup checklist
ProjectIdentificationStage->>ProjectListService: request project list for fiscal year
ProjectListService-->>ProjectIdentificationStage: rows and summary
User->>ProjectIdentificationStage: confirm fiscal period or complete checklist item
ProjectIdentificationStage->>ProjectIdentificationController: PUT workflow update
ProjectIdentificationController->>ProjectIdentificationService: apply checklist transition
ProjectIdentificationService->>AppDbContext: persist updated state
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
server/Models/FiscalYearCycle.cs (1)
10-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider using
out FiscalYearCycleinstead ofout FiscalYearCycle?forTryParse.The standard .NET
TryParsepattern uses a non-nullableoutparameter (e.g.,DateTime.TryParse(out DateTime result)). Sincecycleis only set to a non-null value when the method returnstrue, usingout FiscalYearCycle?forces every caller to use the null-forgiving operator (cycle!). Usingout FiscalYearCycle(withcycle = defaultorcycle = default!as the initial value) would eliminate the need for!at call sites and better match BCL conventions.As per coding guidelines, C# nullable reference types should be used throughout backend code. The current nullable
outannotation is technically correct but degrades ergonomics at every call site.♻️ Proposed refactor
public sealed record FiscalYearCycle( string FiscalYear, DateOnly CycleStart, DateOnly CycleEnd) { - public static bool TryParse(string? fiscalYear, out FiscalYearCycle? cycle) + public static bool TryParse(string? fiscalYear, out FiscalYearCycle cycle) { - cycle = null; + cycle = default!; if (string.IsNullOrWhiteSpace(fiscalYear)) { return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/Models/FiscalYearCycle.cs` around lines 10 - 42, Change FiscalYearCycle.TryParse to use out FiscalYearCycle instead of out FiscalYearCycle?, initializing cycle to default or default! on failure paths while assigning a non-null instance on success. Update all callers to remove unnecessary null-forgiving operators and rely on the TryParse return value for flow safety.Source: Coding guidelines
server/ProjectIdentification/ProjectIdentificationService.cs (1)
441-454: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCorrelated subquery in
GetLatestImportsAsynccan be simplified.The per-row correlated subquery to find the latest import log per dataset translates to a SQL subquery executed for each matching
ImportLogrow. Fetching the logs and grouping in memory is simpler and avoids the correlated subquery overhead, especially since the dataset count is small.♻️ Proposed refactor using in-memory grouping
private async Task<IReadOnlyDictionary<string, RecentImportResponse>> GetLatestImportsAsync( CancellationToken cancellationToken) { var datasetIds = importRegistry.Datasets.Select(dataset => dataset.Id).ToList(); - var recentLogs = await dbContext.ImportLogs - .AsNoTracking() - .Where(log => datasetIds.Contains(log.Dataset)) - .Where(log => log.Id == dbContext.ImportLogs - .Where(candidate => candidate.Dataset == log.Dataset) - .OrderByDescending(candidate => candidate.CompletedAt) - .ThenByDescending(candidate => candidate.Id) - .Select(candidate => candidate.Id) - .First()) - .ToListAsync(cancellationToken); + + var logs = await dbContext.ImportLogs + .AsNoTracking() + .Where(log => datasetIds.Contains(log.Dataset)) + .OrderByDescending(log => log.CompletedAt) + .ThenByDescending(log => log.Id) + .ToListAsync(cancellationToken); + + var recentLogs = logs + .GroupBy(log => log.Dataset) + .Select(g => g.First()) + .ToList(); return recentLogs.ToDictionary(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/ProjectIdentification/ProjectIdentificationService.cs` around lines 441 - 454, Refactor GetLatestImportsAsync to remove the correlated subquery that selects the latest ImportLog per dataset. Query the filtered logs with AsNoTracking and cancellation, materialize them, then group by Dataset and select the latest entry from each group ordered by CompletedAt descending and Id descending before building the response dictionary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server.core/Data/AppDbContext.cs`:
- Around line 23-30: Add an explicit foreign-key relationship for
WorkflowChecklistItemState.SourceImportLogId to ImportLog in AppDbContext,
including the appropriate navigation and delete behavior, and update the model
snapshot/migration as needed so the database constraint is represented. If
SourceImportLogId is intentionally a soft link, document that decision instead
and avoid adding the constraint.
- Around line 26-30: Ensure only one WorkflowRun can have IsCurrent = true:
define a filtered unique index for IsCurrent = 1 in the model configuration, and
update GetOrCreateCurrentRunAsync to handle existing current runs safely before
inserting a new one.
In `@server/Controllers/PgmProjectsController.cs`:
- Around line 32-34: Handle failures from
_projectIdentificationService.RecordPgmImportAsync after
_importService.ImportAsync succeeds: wrap the workflow-recording call in a
try-catch, log the exception with appropriate context, and still return
Ok(result) so clients are not told the committed import failed.
In `@server/ProjectIdentification/ProjectIdentificationService.cs`:
- Around line 254-276: Handle failed FiscalYearCycle.TryParse results explicitly
in both CreateFiscalPeriodOptions and CurrentFiscalYearCycle. Check the boolean
return value before accessing cycle, remove the null-forgiving operator, and
apply the project’s appropriate fallback or clear exception behavior when
parsing fails so no null dereference is possible.
- Around line 202-234: Add concurrency protection to GetOrCreateCurrentRunAsync
so only one WorkflowRun with IsCurrent = true can exist. Prefer configuring a
filtered unique database index on WorkflowRun.IsCurrent (for true values) and
handle duplicate-key SaveChangesAsync failures by reloading and returning the
existing current run; alternatively serialize this creation path with equivalent
retry handling.
---
Nitpick comments:
In `@server/Models/FiscalYearCycle.cs`:
- Around line 10-42: Change FiscalYearCycle.TryParse to use out FiscalYearCycle
instead of out FiscalYearCycle?, initializing cycle to default or default! on
failure paths while assigning a non-null instance on success. Update all callers
to remove unnecessary null-forgiving operators and rely on the TryParse return
value for flow safety.
In `@server/ProjectIdentification/ProjectIdentificationService.cs`:
- Around line 441-454: Refactor GetLatestImportsAsync to remove the correlated
subquery that selects the latest ImportLog per dataset. Query the filtered logs
with AsNoTracking and cancellation, materialize them, then group by Dataset and
select the latest entry from each group ordered by CompletedAt descending and Id
descending before building the response dictionary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d4bca21-82da-4460-abe2-ca78405c2505
📒 Files selected for processing (34)
client/src/components/FlatFileImportPanel.tsxclient/src/components/ProjectIdentificationSetupChecklist.tsxclient/src/components/ProjectIdentificationStage.tsxclient/src/main.cssclient/src/queries/projectIdentification.tsclient/src/queries/projectList.tsclient/src/routes/(authenticated)/workflow.$stageId.tsxclient/src/test/components/FlatFileImportPanel.test.tsxclient/src/test/routes/(authenticated)/workflow.test.tsxdatabase/data/StoredProcedures/GetProjectList.sqlserver.core/Data/AppDbContext.csserver.core/Domain/WorkflowChecklistItemState.csserver.core/Domain/WorkflowRun.csserver.core/Migrations/20260710192848_ProjectIdentificationWorkflowState.Designer.csserver.core/Migrations/20260710192848_ProjectIdentificationWorkflowState.csserver.core/Migrations/AppDbContextModelSnapshot.csserver/Controllers/PgmProjectsController.csserver/Controllers/ProjectIdentificationController.csserver/Controllers/ProjectListController.csserver/Models/FiscalYearCycle.csserver/Models/ProjectIdentification/ProjectIdentificationDtos.csserver/Models/ProjectList/ProjectListDtos.csserver/Program.csserver/ProjectIdentification/IProjectIdentificationService.csserver/ProjectIdentification/ProjectIdentificationService.csserver/ProjectList/IProjectListService.csserver/ProjectList/ProjectListResponseFactory.csserver/ProjectList/ProjectListService.cstests/server.tests/Data/AppDbContextTests.cstests/server.tests/Import/PgmProjectsControllerTests.cstests/server.tests/Models/FiscalYearCycleTests.cstests/server.tests/ProjectIdentification/ProjectIdentificationServiceTests.cstests/server.tests/ProjectList/ProjectListControllerTests.cstests/server.tests/ProjectList/ProjectListResponseFactoryTests.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/Models/FiscalYearCycle.cs`:
- Around line 10-12: Update FiscalYearCycle.TryParse to use an out
FiscalYearCycle? cycle parameter and initialize it with default/null instead of
default!. Preserve the existing successful parse assignment and ensure callers
handle the nullable result when parsing fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bcb841b-f1ea-4448-a885-b0158b10f609
📒 Files selected for processing (15)
server.core/Data/AppDbContext.csserver.core/Domain/WorkflowChecklistItemState.csserver.core/Domain/WorkflowRun.csserver.core/Migrations/20260710214615_ProjectIdentificationWorkflowState.Designer.csserver.core/Migrations/20260710214615_ProjectIdentificationWorkflowState.csserver.core/Migrations/AppDbContextModelSnapshot.csserver.core/updateDatabase.shserver/Controllers/PgmProjectsController.csserver/Controllers/ProjectListController.csserver/Models/FiscalYearCycle.csserver/ProjectIdentification/ProjectIdentificationService.cstests/server.tests/Data/AppDbContextTests.cstests/server.tests/Import/PgmProjectsControllerTests.cstests/server.tests/Models/FiscalYearCycleTests.cstests/server.tests/ProjectList/ProjectListResponseFactoryTests.cs
💤 Files with no reviewable changes (1)
- server.core/Domain/WorkflowRun.cs
✅ Files skipped from review due to trivial changes (1)
- server.core/Migrations/20260710214615_ProjectIdentificationWorkflowState.Designer.cs
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/server.tests/ProjectList/ProjectListResponseFactoryTests.cs
- server/Controllers/ProjectListController.cs
- server.core/Domain/WorkflowChecklistItemState.cs
- server.core/Migrations/AppDbContextModelSnapshot.cs
- server/Controllers/PgmProjectsController.cs
- server.core/Data/AppDbContext.cs
- tests/server.tests/Import/PgmProjectsControllerTests.cs
- tests/server.tests/Models/FiscalYearCycleTests.cs
- server/ProjectIdentification/ProjectIdentificationService.cs
| ap.Title, | ||
| a.ProjectDirector AS ActiveProjectDirector, | ||
| ap.ProjectDirector AS AllProjectDirector, | ||
| ap.Department AS Orgr, |
Contributor
There was a problem hiding this comment.
Not sure we can depend on Department for OrgR. It's pretty messy in the spreadsheet and not always populated.
Last year we loaded financial dept from PGM master data and created a mapping to OrgR. That's probably the best way forward here.
| ap.AwardNumber AS NifaAwardNumber, | ||
| ap.Title, | ||
| a.ProjectDirector AS ActiveProjectDirector, | ||
| ap.ProjectDirector AS AllProjectDirector, |
Contributor
There was a problem hiding this comment.
We'll want to keep the UCP Employee ID and UCPath Name so we can join projects to UCPath data later.
Cast the six LISTAGG people columns to VARCHAR(255) instead of VARCHAR(8000) in the PGM master-data pass-through query. The Redshift ODBC driver reports a wider VARCHAR as SQL_LONGVARCHAR, which EXEC ... AT streams and cannot random-access, failing with error 7341 "cannot get the current row value". Real values top out around 43 characters, so 255 has ample headroom. Also add a note to the PGM import checklist step warning that the import may take several minutes and the browser should not be reloaded while it runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server.core/Import/PgmProjectsImportService.cs`:
- Around line 185-190: Add post-import validation in PgmProjectsImportService
around the project-name aggregation query to detect LISTAGG results exceeding
VARCHAR(255). Compare each source aggregation’s full length with the stored
value, and log a warning identifying affected project IDs and fields when
truncation is detected; preserve the existing import behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c0b414b-9f59-4ae7-ba2f-352621ac9cc7
📒 Files selected for processing (5)
client/src/components/ProjectIdentificationSetupChecklist.tsxserver.core/Import/PgmProjectsImportService.csserver/Models/FiscalYearCycle.cstests/server.tests/Models/FiscalYearCycleTests.cstests/server.tests/ProjectList/ProjectListResponseFactoryTests.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/server.tests/ProjectList/ProjectListResponseFactoryTests.cs
- server/Models/FiscalYearCycle.cs
- client/src/components/ProjectIdentificationSetupChecklist.tsx
- tests/server.tests/Models/FiscalYearCycleTests.cs
Comment on lines
+185
to
+190
| CAST(LISTAGG(DISTINCT principal_investigator_person_name, '; ') AS VARCHAR(255)) AS principal_investigator_names, | ||
| CAST(LISTAGG(DISTINCT piperson, '; ') AS VARCHAR(255)) AS pi_persons, | ||
| CAST(LISTAGG(DISTINCT award_copi_name, '; ') AS VARCHAR(255)) AS award_copi_names, | ||
| CAST(LISTAGG(DISTINCT project_manager_name, '; ') AS VARCHAR(255)) AS project_manager_names, | ||
| CAST(LISTAGG(DISTINCT grant_administrator, '; ') AS VARCHAR(255)) AS grant_administrators, | ||
| CAST(LISTAGG(DISTINCT contractadmin, '; ') AS VARCHAR(255)) AS contract_admins |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Silent truncation risk if LISTAGG result exceeds 255 characters.
Redshift CAST(... AS VARCHAR(255)) silently truncates when the concatenated string exceeds 255 chars. The comment documents observed max ~43 chars, which gives ~6× headroom, but there's no runtime detection if a future project has enough distinct people to exceed the limit. Consider adding a post-import validation query that flags any rows where the source LISTAGG length differs from the stored value, or at minimum log a warning if LENGTH(LISTAGG(...)) > 255 is observed for any project during the next few imports.
🛡️ Suggested post-import truncation check
-- Run after import to detect any projects whose aggregated names
-- may have been silently truncated by the VARCHAR(255) cast.
SELECT project_id,
LISTAGG(DISTINCT principal_investigator_person_name, '; ') AS full_names,
LENGTH(LISTAGG(DISTINCT principal_investigator_person_name, '; ')) AS full_len
FROM ae_dwh.pgm_master_data
WHERE project_id IS NOT NULL
GROUP BY project_id
HAVING LENGTH(LISTAGG(DISTINCT principal_investigator_person_name, '; ')) > 255;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server.core/Import/PgmProjectsImportService.cs` around lines 185 - 190, Add
post-import validation in PgmProjectsImportService around the project-name
aggregation query to detect LISTAGG results exceeding VARCHAR(255). Compare each
source aggregation’s full length with the stored value, and log a warning
identifying affected project IDs and fields when truncation is detected;
preserve the existing import behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is branched from swe/ProjectList, which has yet to be merged to main. PR #38 should be completed first.
Adds the project identification setup workflow, including a guided checklist for fiscal period confirmation, flat file uploads, PGM master-data import, issue review, and future finalization.
Summary:
Summary by CodeRabbit