Data Classification feedback: Purpose segment, letter levels, SFN catalog, per-tab export#35
Open
rmartinsen-ucd wants to merge 17 commits into
Open
Data Classification feedback: Purpose segment, letter levels, SFN catalog, per-tab export#35rmartinsen-ucd wants to merge 17 commits into
rmartinsen-ucd wants to merge 17 commits into
Conversation
Database layer for the expense data import pipeline: - ChartSegments: consolidated reference table for the eight ae_dwh erp_* segment sources - AETransactions and UcPathTransactions: raw transaction detail with persisted exclusion columns for rules step 2 does not own (ExcludedByDate, ExcludedByPurpose, AccountNotInAE) - Projects: materialized project list (ActiveProjects x AllProjects x PGMProjects) built by BuildProjects, snapshotting the step 1 outcome - v_PgmProjectSfnBuckets: CFDA-to-SFN-bucket logic shared with GetProjectList's PgmClassified CTE - SeedChartStringSegments: insert-only seeding of unclassified segment codes (including the new Ern type) from imported transactions - ClassifyTransactions: stamps date, purpose and account-not-in-AE exclusions; fails closed on missing preconditions and lingering 204 SFN disagreements Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The test asserts the panel preserves flat-file column order, so the fixture's key order is meaningful and must stay unsorted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds CSV export to the segment classification UI, introduces a shared SFN catalog with descriptions replacing hardcoded fund lists, adds a Purpose segment type/hierarchy with letter-based hierarchy level keys, and adds new database tables, indexes, a view, and stored procedures for AE/UcPath transaction ingestion and classification. ChangesSegment Classification Export UI
Estimated code review effort: 3 (Moderate) | ~25 minutes SFN Catalog
Estimated code review effort: 2 (Simple) | ~12 minutes Purpose Hierarchy and Letter-Based Levels
Estimated code review effort: 3 (Moderate) | ~25 minutes Transaction Ingestion Database Schema
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DataClassificationStage
participant buildSegmentExport
participant ExportDataButton
DataClassificationStage->>buildSegmentExport: segments, activeTab
buildSegmentExport-->>DataClassificationStage: columns, rows, filename
DataClassificationStage->>ExportDataButton: exportData
sequenceDiagram
participant BuildProjects
participant Projects
participant ClassifyTransactions
participant AETransactions
participant UcPathTransactions
participant SeedChartStringSegments
BuildProjects->>Projects: rebuild rows from ActiveProjects/AllProjects/v_PgmProjectSfnBuckets/PGMProjects
ClassifyTransactions->>AETransactions: stamp ExcludedByDate/ExcludedByPurpose
ClassifyTransactions->>UcPathTransactions: stamp ExcludedByDate/ExcludedByPurpose/AccountNotInAE
SeedChartStringSegments->>AETransactions: read segment codes
SeedChartStringSegments->>UcPathTransactions: read segment codes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR #34 split the data schema entities into DataDbContext, so the PurposeHierarchy DbSet, mapping, seeds, and controller lookup follow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Disambiguates the classification table from the ChartSegments warehouse reference table: one holds what segments exist, the other how step 2 classified them. Renames the table, sproc, EF entity, controller, API route, and client module. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Purpose is a classified segment now, so purpose exclusions derive at read time from SegmentClassifications like the other step 2 types. Removes ExcludedByPurpose columns and the hardcoded 2025 list, the Projects-based 204 guards the carve-out needed, and seeds Purpose codes from transactions alongside the other segment types. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
client/src/components/dataClassification/SegmentGrid.tsx (1)
45-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared
levelKeyshelper to avoid duplication.The
levelKeyscomputation here (lines 45-51) is duplicated verbatim inexportSegments.ts(lines 24-30). Both derive the same sorted unique set of hierarchy level keys from segments. Extracting a shared helper would prevent divergence if the logic ever changes (e.g., custom sort order).♻️ Suggested shared helper
+// In a shared module, e.g. segments.ts or a hierarchy utils file: +export function hierarchyLevelKeys(segments: ChartStringSegment[]): string[] { + return [ + ...new Set( + segments.flatMap((segment) => + segment.hierarchy.map((level) => level.level) + ) + ), + ].sort(); +}Then in both
SegmentGrid.tsxandexportSegments.ts:- const levelKeys = [ - ...new Set( - segments.flatMap((segment) => - segment.hierarchy.map((level) => level.level) - ) - ), - ].sort(); + const levelKeys = hierarchyLevelKeys(segments);🤖 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 `@client/src/components/dataClassification/SegmentGrid.tsx` around lines 45 - 51, The levelKeys computation is duplicated between SegmentGrid and exportSegments, so extract it into a shared helper and have both call that single source of truth. Move the “sorted unique hierarchy level keys from segments” logic into a reusable function (using the same symbols around segments/hierarchy/level.level), then replace the inline computation in SegmentGrid and exportSegments with calls to that helper to keep behavior consistent.client/src/components/dataClassification/DataClassificationStage.tsx (1)
44-47: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider memoizing
exportDatato avoid recomputation on every render.
buildSegmentExportruns on every render, including when the user is interacting with the table (search, sort, pagination). Wrapping inuseMemokeyed ontabSegmentsandactiveTabwould avoid unnecessary recomputation.♻️ Optional memoization
+import { useMemo, useState } from 'react'; const tabSegments = segmentsForType(segments, activeType); - const exportData = buildSegmentExport(tabSegments, activeTab); + const exportData = useMemo( + () => buildSegmentExport(tabSegments, activeTab), + [tabSegments, activeTab] + );🤖 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 `@client/src/components/dataClassification/DataClassificationStage.tsx` around lines 44 - 47, `buildSegmentExport` in `DataClassificationStage` is recomputed on every render, even when only table interactions change. Memoize the `exportData` calculation with `useMemo`, keyed on `tabSegments` and `activeTab`, so the export payload is only rebuilt when the selected tab or its segment data actually changes.server.core/Data/HierarchySeed.cs (1)
98-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPurposeFrom duplicates AccountFrom, FundFrom, and ActivityFrom.
All four
*Frommethods have identical bodies — same field indices, sameSeedCsv.Nullablecalls, same six level pairs. Adding a fourth copy increases the maintenance surface: if the CSV column layout changes, four methods must be updated in lockstep. Consider extracting a shared mapping helper or generic factory to consolidate.♻️ Optional: extract a shared hierarchy-from-CSV helper
If the hierarchy types share a common base or interface exposing the
ParentLevel*properties, a single generic method could replace all four:-private static PurposeHierarchy PurposeFrom(string[] f) => new() -{ - Code = f[0], - Description = SeedCsv.Nullable(f[1]), - ParentLevel0Code = SeedCsv.Nullable(f[2]), ParentLevel0Name = SeedCsv.Nullable(f[3]), - ParentLevel1Code = SeedCsv.Nullable(f[4]), ParentLevel1Name = SeedCsv.Nullable(f[5]), - ParentLevel2Code = SeedCsv.Nullable(f[6]), ParentLevel2Name = SeedCsv.Nullable(f[7]), - ParentLevel3Code = SeedCsv.Nullable(f[8]), ParentLevel3Name = SeedCsv.Nullable(f[9]), - ParentLevel4Code = SeedCsv.Nullable(f[10]), ParentLevel4Name = SeedCsv.Nullable(f[11]), - ParentLevel5Code = SeedCsv.Nullable(f[12]), ParentLevel5Name = SeedCsv.Nullable(f[13]), -}; +// If ISegmentHierarchy (or a new IHierarchyWithLevels) exposes settable ParentLevel* properties: +private static T HierarchyFrom<T>(string[] f) where T : ISegmentHierarchy, new() + => new() + { + Code = f[0], + Description = SeedCsv.Nullable(f[1]), + ParentLevel0Code = SeedCsv.Nullable(f[2]), ParentLevel0Name = SeedCsv.Nullable(f[3]), + ParentLevel1Code = SeedCsv.Nullable(f[4]), ParentLevel1Name = SeedCsv.Nullable(f[5]), + ParentLevel2Code = SeedCsv.Nullable(f[6]), ParentLevel2Name = SeedCsv.Nullable(f[7]), + ParentLevel3Code = SeedCsv.Nullable(f[8]), ParentLevel3Name = SeedCsv.Nullable(f[9]), + ParentLevel4Code = SeedCsv.Nullable(f[10]), ParentLevel4Name = SeedCsv.Nullable(f[11]), + ParentLevel5Code = SeedCsv.Nullable(f[12]), ParentLevel5Name = SeedCsv.Nullable(f[13]), + };This depends on whether the hierarchy domain classes expose a shared settable-property interface. If they don't, keeping the separate methods is acceptable — just be aware all four must stay synchronized.
🤖 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/Data/HierarchySeed.cs` around lines 98 - 108, PurposeFrom is a duplicate of AccountFrom, FundFrom, and ActivityFrom in HierarchySeed, so consolidate the repeated CSV-to-hierarchy mapping into a shared helper or generic factory. Refactor the common field/index mapping and SeedCsv.Nullable calls into one place, then have PurposeFrom and the other *From methods delegate to it while preserving the existing ParentLevel0-5 assignments.server.core/Domain/PurposeHierarchy.cs (1)
20-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the duplicated
Levels()implementation.
AccountHierarchy,ActivityHierarchy,FundHierarchy, and nowPurposeHierarchyall share an identicalLevels()method. A shared base class or a static helper (e.g.,SegmentHierarchyLevels.Build(ParentLevel0Code, ParentLevel0Name, ...)) would eliminate this copy-paste pattern and ensure future changes apply uniformly.🤖 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/Domain/PurposeHierarchy.cs` around lines 20 - 33, The Levels() implementation in PurposeHierarchy is duplicated across multiple hierarchy classes and should be centralized. Refactor the repeated logic used by Levels() in PurposeHierarchy, AccountHierarchy, ActivityHierarchy, and FundHierarchy into a shared base class or helper such as SegmentHierarchyLevels.Build(...), then have PurposeHierarchy delegate to that shared implementation so future changes only need to be made once.database/data/StoredProcedures/BuildProjects.sql (2)
59-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant join to
PGMProjectsforPrincipalInvestigatorNames.The
LEFT JOIN [data].[PGMProjects] pgm ON pgm.[ProjectId] = pc.[ProjectId]exists solely to fetchpgm.[PrincipalInvestigatorNames], butv_PgmProjectSfnBucketsalready selects fromPGMProjects. If the view includes that column (see view review), this join can be removed.🤖 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 `@database/data/StoredProcedures/BuildProjects.sql` around lines 59 - 63, Remove the redundant LEFT JOIN to PGMProjects in BuildProjects and use PrincipalInvestigatorNames from v_PgmProjectSfnBuckets instead, since the join only exists to pull pgm.[PrincipalInvestigatorNames]. Update the query around the ap/pc/pgm join chain so the select references the value exposed by [data].[v_PgmProjectSfnBuckets] and verify no other pgm columns are needed before deleting the [data].[PGMProjects] join.
16-16: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer
TRUNCATE TABLEoverDELETEfor full-table clearing.Since this removes all rows from a staging table,
TRUNCATE TABLE [data].[Projects]is more efficient (minimally logged, resets identity seed) thanDELETE FROM. If foreign keys preventTRUNCATE, keepDELETEbut add a comment noting why.⚡ Suggested change
- DELETE FROM [data].[Projects]; + TRUNCATE TABLE [data].[Projects];🤖 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 `@database/data/StoredProcedures/BuildProjects.sql` at line 16, The full-table clearing step in BuildProjects should use a more efficient truncation approach instead of a row-by-row delete. Update the logic in the stored procedure that currently issues DELETE FROM [data].[Projects] to prefer TRUNCATE TABLE [data].[Projects], and only keep DELETE if truncation is blocked by constraints; if so, add a brief comment in the procedure explaining why truncation cannot be used.database/data/Views/v_PgmProjectSfnBuckets.sql (1)
8-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider including
PrincipalInvestigatorNamesin the view to eliminate a redundant join inBuildProjects.
BuildProjectsjoins back toPGMProjectssolely to retrievePrincipalInvestigatorNames, but this view already selects fromPGMProjects. Adding the column here would remove the extra join and keep the PGM-derived data contract in one place.♻️ Proposed addition to the view SELECT list
SELECT pgm.ProjectId, pgm.ProjectNumber, pgm.SponsorAwardNumber, REPLACE(pgm.SponsorAwardNumber, '-', '') AS AwardKey, + pgm.PrincipalInvestigatorNames, CASE🤖 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 `@database/data/Views/v_PgmProjectSfnBuckets.sql` around lines 8 - 32, Add PrincipalInvestigatorNames to the v_PgmProjectSfnBuckets view so BuildProjects no longer needs to join back to PGMProjects just to fetch that field. Update the view’s SELECT list in the v_PgmProjectSfnBuckets definition to include the PGMProjects.PrincipalInvestigatorNames column alongside the existing PGM-derived fields, and then adjust BuildProjects to read it directly from this view. Keep the existing symbols PGMProjects, v_PgmProjectSfnBuckets, and BuildProjects aligned so the data contract stays in one place.
🤖 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 `@database/data/StoredProcedures/BuildProjects.sql`:
- Around line 16-18: Wrap the DELETE and INSERT in BuildProjects.sql in a single
transaction so dbo/data.Projects is only replaced when the full refresh
succeeds. Add transaction/error handling around the DELETE FROM
[data].[Projects] and the following INSERT INTO [data].[Projects] block, and
make sure failures roll back the transaction before rethrowing or failing the
procedure.
In `@database/data/Tables/ChartStringSegments.sql`:
- Line 3: The SegmentType inline comment in ChartStringSegments.sql is missing
the new Purpose value, so update the list alongside the existing SegmentType
enum references to include Purpose with the other valid values. Keep the comment
in sync with ChartStringSegment and the seeded CSV data so the allowed segment
types are accurately documented.
In `@database/data/Tables/PurposeHierarchy.sql`:
- Around line 3-16: The PurposeHierarchy table has inconsistent code column
types because Code is NVARCHAR(50) while all ParentLevel*Code columns are
VARCHAR(20). Update the ParentLevel0Code through ParentLevel5Code definitions in
PurposeHierarchy.sql to use the same type and length as Code, so hierarchy
comparisons and joins in this table stay consistent; keep the change localized
to the table column declarations.
In `@tests/server.tests/ChartStringSegments/HierarchyMappingTests.cs`:
- Around line 42-76: Add PurposeHierarchy to the schema-mapping theory test
coverage so it is validated alongside the other hierarchy entities. Update the
Maps_to_data_schema_keyed_by_code test to include PurposeHierarchy in its
InlineData cases, ensuring it is asserted to map to the data schema with Code as
the primary key. Use the existing hierarchy type names in that test as the
reference point and keep the assertion pattern consistent.
---
Nitpick comments:
In `@client/src/components/dataClassification/DataClassificationStage.tsx`:
- Around line 44-47: `buildSegmentExport` in `DataClassificationStage` is
recomputed on every render, even when only table interactions change. Memoize
the `exportData` calculation with `useMemo`, keyed on `tabSegments` and
`activeTab`, so the export payload is only rebuilt when the selected tab or its
segment data actually changes.
In `@client/src/components/dataClassification/SegmentGrid.tsx`:
- Around line 45-51: The levelKeys computation is duplicated between SegmentGrid
and exportSegments, so extract it into a shared helper and have both call that
single source of truth. Move the “sorted unique hierarchy level keys from
segments” logic into a reusable function (using the same symbols around
segments/hierarchy/level.level), then replace the inline computation in
SegmentGrid and exportSegments with calls to that helper to keep behavior
consistent.
In `@database/data/StoredProcedures/BuildProjects.sql`:
- Around line 59-63: Remove the redundant LEFT JOIN to PGMProjects in
BuildProjects and use PrincipalInvestigatorNames from v_PgmProjectSfnBuckets
instead, since the join only exists to pull pgm.[PrincipalInvestigatorNames].
Update the query around the ap/pc/pgm join chain so the select references the
value exposed by [data].[v_PgmProjectSfnBuckets] and verify no other pgm columns
are needed before deleting the [data].[PGMProjects] join.
- Line 16: The full-table clearing step in BuildProjects should use a more
efficient truncation approach instead of a row-by-row delete. Update the logic
in the stored procedure that currently issues DELETE FROM [data].[Projects] to
prefer TRUNCATE TABLE [data].[Projects], and only keep DELETE if truncation is
blocked by constraints; if so, add a brief comment in the procedure explaining
why truncation cannot be used.
In `@database/data/Views/v_PgmProjectSfnBuckets.sql`:
- Around line 8-32: Add PrincipalInvestigatorNames to the v_PgmProjectSfnBuckets
view so BuildProjects no longer needs to join back to PGMProjects just to fetch
that field. Update the view’s SELECT list in the v_PgmProjectSfnBuckets
definition to include the PGMProjects.PrincipalInvestigatorNames column
alongside the existing PGM-derived fields, and then adjust BuildProjects to read
it directly from this view. Keep the existing symbols PGMProjects,
v_PgmProjectSfnBuckets, and BuildProjects aligned so the data contract stays in
one place.
In `@server.core/Data/HierarchySeed.cs`:
- Around line 98-108: PurposeFrom is a duplicate of AccountFrom, FundFrom, and
ActivityFrom in HierarchySeed, so consolidate the repeated CSV-to-hierarchy
mapping into a shared helper or generic factory. Refactor the common field/index
mapping and SeedCsv.Nullable calls into one place, then have PurposeFrom and the
other *From methods delegate to it while preserving the existing ParentLevel0-5
assignments.
In `@server.core/Domain/PurposeHierarchy.cs`:
- Around line 20-33: The Levels() implementation in PurposeHierarchy is
duplicated across multiple hierarchy classes and should be centralized. Refactor
the repeated logic used by Levels() in PurposeHierarchy, AccountHierarchy,
ActivityHierarchy, and FundHierarchy into a shared base class or helper such as
SegmentHierarchyLevels.Build(...), then have PurposeHierarchy delegate to that
shared implementation so future changes only need to be made once.
🪄 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: cfca0177-e470-45c8-9bf5-57c623303599
⛔ Files ignored due to path filters (1)
server.core/Data/Seed/PurposeHierarchy.csvis excluded by!**/*.csv
📒 Files selected for processing (40)
client/src/components/dataClassification/DataClassificationStage.tsxclient/src/components/dataClassification/SegmentClassificationControl.tsxclient/src/components/dataClassification/SegmentGrid.tsxclient/src/components/dataClassification/exportSegments.tsclient/src/components/dataClassification/segments.tsclient/src/queries/chartStringSegments.tsclient/src/test/components/FlatFileImportPanel.test.tsxclient/src/test/components/dataClassification/SegmentClassificationControl.test.tsxclient/src/test/components/dataClassification/SegmentGrid.test.tsxclient/src/test/components/dataClassification/exportSegments.test.tsclient/src/test/components/dataClassification/segments.test.tsdatabase/data/Indexes/IX_AETransactions_Project.sqldatabase/data/Indexes/IX_UcPathTransactions_Account.sqldatabase/data/Indexes/IX_UcPathTransactions_Project.sqldatabase/data/StoredProcedures/BuildProjects.sqldatabase/data/StoredProcedures/ClassifyTransactions.sqldatabase/data/StoredProcedures/SeedChartStringSegments.sqldatabase/data/Tables/AETransactions.sqldatabase/data/Tables/ChartSegments.sqldatabase/data/Tables/ChartStringSegments.sqldatabase/data/Tables/Projects.sqldatabase/data/Tables/PurposeHierarchy.sqldatabase/data/Tables/UcPathTransactions.sqldatabase/data/Views/v_PgmProjectSfnBuckets.sqlserver.core/Data/ChartStringSegmentSeed.csserver.core/Data/DataDbContext.csserver.core/Data/HierarchySeed.csserver.core/Domain/AccountHierarchy.csserver.core/Domain/ActivityHierarchy.csserver.core/Domain/ChartStringSegment.csserver.core/Domain/FundHierarchy.csserver.core/Domain/PurposeHierarchy.csserver/Controllers/ChartStringSegmentsController.csserver/Models/ChartStringSegments/FundSfns.csserver/Models/ChartStringSegments/SfnCatalog.cstests/server.tests/ChartStringSegments/ChartStringSegmentSeedTests.cstests/server.tests/ChartStringSegments/ChartStringSegmentsControllerTests.cstests/server.tests/ChartStringSegments/HierarchyMappingTests.cstests/server.tests/ChartStringSegments/HierarchySeedTests.cstests/server.tests/ChartStringSegments/SfnCatalogTests.cs
Comment on lines
+16
to
+18
| DELETE FROM [data].[Projects]; | ||
|
|
||
| INSERT INTO [data].[Projects] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap DELETE + INSERT in a transaction to prevent data loss on partial failure.
If the INSERT fails partway (constraint violation, timeout, etc.), the Projects table is left empty, breaking all downstream consumers. Wrap both statements in BEGIN TRAN / COMMIT with error handling so the table is only replaced on success.
🔒 Suggested transaction wrapping
DELETE FROM [data].[Projects];
+ BEGIN TRY
+ BEGIN TRAN;
+
INSERT INTO [data].[Projects]
(
[AccessionNumber], -- Row count for the import run stage.
SELECT COUNT(*) AS ProjectRowsBuilt FROM [data].[Projects];
+
+ COMMIT;
+ END TRY
+ BEGIN CATCH
+ IF @@TRANCOUNT > 0 ROLLBACK;
+ THROW;
+ END CATCH
END🤖 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 `@database/data/StoredProcedures/BuildProjects.sql` around lines 16 - 18, Wrap
the DELETE and INSERT in BuildProjects.sql in a single transaction so
dbo/data.Projects is only replaced when the full refresh succeeds. Add
transaction/error handling around the DELETE FROM [data].[Projects] and the
following INSERT INTO [data].[Projects] block, and make sure failures roll back
the transaction before rethrowing or failing the procedure.
| @@ -1,6 +1,6 @@ | |||
| CREATE TABLE [data].[ChartStringSegments] | |||
| ( | |||
| [SegmentType] NVARCHAR(20) NOT NULL, -- FinancialDepartment | Account | Fund | Activity | |||
| [SegmentType] NVARCHAR(20) NOT NULL, -- FinancialDepartment | Account | Fund | Activity | Ern | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment is missing Purpose from the SegmentType list.
The comment now includes Ern but omits Purpose, which is a new SegmentType introduced in this PR (added to the SegmentType enum in server.core/Domain/ChartStringSegment.cs and seeded via CSV). Listing it prevents confusion about valid segment types.
Add Purpose to the comment
- [SegmentType] NVARCHAR(20) NOT NULL, -- FinancialDepartment | Account | Fund | Activity | Ern
+ [SegmentType] NVARCHAR(20) NOT NULL, -- FinancialDepartment | Account | Fund | Activity | Ern | Purpose📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [SegmentType] NVARCHAR(20) NOT NULL, -- FinancialDepartment | Account | Fund | Activity | Ern | |
| [SegmentType] NVARCHAR(20) NOT NULL, -- FinancialDepartment | Account | Fund | Activity | Ern | Purpose |
🤖 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 `@database/data/Tables/ChartStringSegments.sql` at line 3, The SegmentType
inline comment in ChartStringSegments.sql is missing the new Purpose value, so
update the list alongside the existing SegmentType enum references to include
Purpose with the other valid values. Keep the comment in sync with
ChartStringSegment and the seeded CSV data so the allowed segment types are
accurately documented.
Comment on lines
+3
to
+16
| [Code] NVARCHAR(50) NOT NULL, | ||
| [Description] NVARCHAR(1000) NULL, | ||
| [ParentLevel0Code] VARCHAR(20) NULL, | ||
| [ParentLevel0Name] NVARCHAR(1000) NULL, | ||
| [ParentLevel1Code] VARCHAR(20) NULL, | ||
| [ParentLevel1Name] NVARCHAR(1000) NULL, | ||
| [ParentLevel2Code] VARCHAR(20) NULL, | ||
| [ParentLevel2Name] NVARCHAR(1000) NULL, | ||
| [ParentLevel3Code] VARCHAR(20) NULL, | ||
| [ParentLevel3Name] NVARCHAR(1000) NULL, | ||
| [ParentLevel4Code] VARCHAR(20) NULL, | ||
| [ParentLevel4Name] NVARCHAR(1000) NULL, | ||
| [ParentLevel5Code] VARCHAR(20) NULL, | ||
| [ParentLevel5Name] NVARCHAR(1000) NULL, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Inconsistent column types between Code and ParentLevel*Code.
Code is NVARCHAR(50) but all ParentLevel*Code columns are VARCHAR(20). If these columns are ever compared or joined (e.g., self-referencing hierarchy queries), the type mismatch forces implicit conversions that can degrade performance and cause unexpected collation behavior. Consider aligning all code columns to the same type.
🔧 Suggested type alignment
- [ParentLevel0Code] VARCHAR(20) NULL,
+ [ParentLevel0Code] NVARCHAR(50) NULL,
[ParentLevel0Name] NVARCHAR(1000) NULL,
- [ParentLevel1Code] VARCHAR(20) NULL,
+ [ParentLevel1Code] NVARCHAR(50) NULL,
[ParentLevel1Name] NVARCHAR(1000) NULL,
- [ParentLevel2Code] VARCHAR(20) NULL,
+ [ParentLevel2Code] NVARCHAR(50) NULL,
[ParentLevel2Name] NVARCHAR(1000) NULL,
- [ParentLevel3Code] VARCHAR(20) NULL,
+ [ParentLevel3Code] NVARCHAR(50) NULL,
[ParentLevel3Name] NVARCHAR(1000) NULL,
- [ParentLevel4Code] VARCHAR(20) NULL,
+ [ParentLevel4Code] NVARCHAR(50) NULL,
[ParentLevel4Name] NVARCHAR(1000) NULL,
- [ParentLevel5Code] VARCHAR(20) NULL,
+ [ParentLevel5Code] NVARCHAR(50) NULL,
[ParentLevel5Name] NVARCHAR(1000) NULL,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [Code] NVARCHAR(50) NOT NULL, | |
| [Description] NVARCHAR(1000) NULL, | |
| [ParentLevel0Code] VARCHAR(20) NULL, | |
| [ParentLevel0Name] NVARCHAR(1000) NULL, | |
| [ParentLevel1Code] VARCHAR(20) NULL, | |
| [ParentLevel1Name] NVARCHAR(1000) NULL, | |
| [ParentLevel2Code] VARCHAR(20) NULL, | |
| [ParentLevel2Name] NVARCHAR(1000) NULL, | |
| [ParentLevel3Code] VARCHAR(20) NULL, | |
| [ParentLevel3Name] NVARCHAR(1000) NULL, | |
| [ParentLevel4Code] VARCHAR(20) NULL, | |
| [ParentLevel4Name] NVARCHAR(1000) NULL, | |
| [ParentLevel5Code] VARCHAR(20) NULL, | |
| [ParentLevel5Name] NVARCHAR(1000) NULL, | |
| [Code] NVARCHAR(50) NOT NULL, | |
| [Description] NVARCHAR(1000) NULL, | |
| [ParentLevel0Code] NVARCHAR(50) NULL, | |
| [ParentLevel0Name] NVARCHAR(1000) NULL, | |
| [ParentLevel1Code] NVARCHAR(50) NULL, | |
| [ParentLevel1Name] NVARCHAR(1000) NULL, | |
| [ParentLevel2Code] NVARCHAR(50) NULL, | |
| [ParentLevel2Name] NVARCHAR(1000) NULL, | |
| [ParentLevel3Code] NVARCHAR(50) NULL, | |
| [ParentLevel3Name] NVARCHAR(1000) NULL, | |
| [ParentLevel4Code] NVARCHAR(50) NULL, | |
| [ParentLevel4Name] NVARCHAR(1000) NULL, | |
| [ParentLevel5Code] NVARCHAR(50) NULL, | |
| [ParentLevel5Name] NVARCHAR(1000) NULL, |
🤖 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 `@database/data/Tables/PurposeHierarchy.sql` around lines 3 - 16, The
PurposeHierarchy table has inconsistent code column types because Code is
NVARCHAR(50) while all ParentLevel*Code columns are VARCHAR(20). Update the
ParentLevel0Code through ParentLevel5Code definitions in PurposeHierarchy.sql to
use the same type and length as Code, so hierarchy comparisons and joins in this
table stay consistent; keep the change localized to the table column
declarations.
Comment on lines
+42
to
+76
| [Fact] | ||
| public void Fund_levels_use_letter_keys() | ||
| { | ||
| var fund = new FundHierarchy | ||
| { | ||
| Code = "45530", | ||
| ParentLevel0Code = "TOP", ParentLevel0Name = "Top", | ||
| ParentLevel1Code = "MID", ParentLevel1Name = "Mid", | ||
| }; | ||
|
|
||
| fund.Levels().Select(l => l.Level).Should().Equal("A", "B"); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Account_and_activity_levels_use_letter_keys() | ||
| { | ||
| var account = new AccountHierarchy { Code = "500000", ParentLevel0Code = "4X", ParentLevel5Code = "DEEP" }; | ||
| var activity = new ActivityHierarchy { Code = "000000", ParentLevel0Code = "ROOT" }; | ||
|
|
||
| account.Levels().Select(l => l.Level).Should().Equal("A", "F"); | ||
| activity.Levels().Select(l => l.Level).Should().Equal("A"); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Purpose_levels_use_letter_keys() | ||
| { | ||
| var purpose = new PurposeHierarchy | ||
| { | ||
| Code = "44", | ||
| ParentLevel0Code = "1A", ParentLevel0Name = "Purpose Categories", | ||
| ParentLevel1Code = "1D", ParentLevel1Name = "Organized Research D", | ||
| }; | ||
|
|
||
| purpose.Levels().Select(l => l.Level).Should().Equal("A", "B"); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add PurposeHierarchy to the schema-mapping theory test.
The Maps_to_data_schema_keyed_by_code test (lines 9-24) validates that each hierarchy entity maps to the data schema with a Code primary key, but PurposeHierarchy is not included in its [InlineData] cases. Since this is a new entity with its own table, it should receive the same schema-mapping coverage.
💚 Proposed fix
[InlineData(typeof(DepartmentHierarchy), "DepartmentHierarchy")]
[InlineData(typeof(AccountHierarchy), "AccountHierarchy")]
[InlineData(typeof(FundHierarchy), "FundHierarchy")]
[InlineData(typeof(ActivityHierarchy), "ActivityHierarchy")]
+[InlineData(typeof(PurposeHierarchy), "PurposeHierarchy")]
public void Maps_to_data_schema_keyed_by_code(Type clr, string table)🤖 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 `@tests/server.tests/ChartStringSegments/HierarchyMappingTests.cs` around lines
42 - 76, Add PurposeHierarchy to the schema-mapping theory test coverage so it
is validated alongside the other hierarchy entities. Update the
Maps_to_data_schema_keyed_by_code test to include PurposeHierarchy in its
InlineData cases, ensuring it is asserted to map to the data schema with Code as
the primary key. Use the existing hierarchy type names in that test as the
reference point and keep the assertion pattern consistent.
Every included active project has a PGM master data record by build time and SFN agreement is checked during Project Identification, so AEProjectNumber is required and the separate NifaSfn/PgmSfnBucket pair collapses to one Sfn column. BuildProjects fails loudly on unmatched projects instead of inserting NULL AE rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Core chart string segments, natural key components, and the fringe/salary marker are required. ERN code replaces the legacy DOS terminology throughout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Warehouse profiling of both labor ledger views over the filtered population shows every mapped column except Program and JobCode fully populated, so parent department, project, activity, position number, amounts, pay period end date, and fiscal fields are now required. Import-supplied values cover fringe gaps (hours 0, FTE 0, ERN 'XXX'); rows without a position number are excluded at import. The classify sproc drops its dead null-date branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
Implements the six Data Classification feedback items (spec reviewed 2026-07-08) plus follow-on review changes to the import pipeline schema.
Classification feedback
Follow-on review changes
Issue #31 was updated alongside (classification-driven purpose exclusion, renamed objects, ERN terminology, profiling verdicts).
Also on this branch
Test plan
🤖 Generated with Claude Code