Data Classification: Fund SFN, chart-segment hierarchy, grid UX, and ERN tab#27
Merged
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix AuthorizedUserHandlerTests to pass required IHostEnvironment parameter to handler. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds HierarchySeed.EnsureSeededAsync to seed dev data for the DepartmentHierarchy, AccountHierarchy, FundHierarchy, and ActivityHierarchy tables, wired into DbInitializer's dev seed path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Load the four hierarchy tables from embedded CSV exports (Account/Fund/Activity direct; Department synthesized from the FinancialDept D-G columns with fabricated A-C). Chart-string segments are then derived from the hierarchy so every row shows a full breadcrumb. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Unset-first ordering is captured when a tab is entered and stays stable while classifying (classified rows no longer jump to the bottom). All columns are sortable, and switching chart-string type remounts the table so the search box and pagination reset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract a shared SeedCsv reader (reused by HierarchySeed), add ErnCodes.csv, and seed each ERN code to its source IncludeInAD419FTE value, leaving the first few unset for demo. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Included funds are now seeded with a valid SFN so the Fund dropdown reflects a classified state instead of reading Unset. DataTable no longer resets to page 1 when the row data updates in place (autoResetPageIndex: false). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds chart string segment classification across the data model, API, seeding, and workflow UI. It introduces hierarchy-backed segment storage, a classification endpoint, and a workflow stage for tabbed segment review and progression. ChangesData Classification Workflow
Estimated code review effort: 4 (Complex) | ~75 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
server.core/Data/ChartStringSegmentSeed.cs (1)
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sourcing the SFN list from a single shared constant.
FundSfnPoolduplicates the list also defined inFundSfns.cs(per your own comment). Sinceserver.coreis presumably a lower-layer project referenced byserver, moving the canonical SFN list intoserver.core(e.g., alongsideChartStringSegment) and having both the seed andFundSfns.csreference it would prevent drift if the valid SFN set changes.🤖 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/ChartStringSegmentSeed.cs` around lines 18 - 22, The SFN values are duplicated in ChartStringSegmentSeed and FundSfns, so move the canonical SFN list into a shared constant in server.core near ChartStringSegment and have both ChartStringSegmentSeed.FundSfnPool and FundSfns.cs reference that single source. Update the seed logic to use the shared list instead of its local array so the valid SFN set stays consistent and avoids drift.server.core/Domain/ActivityHierarchy.cs (1)
1-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider deduplicating identical hierarchy classes.
ActivityHierarchy,AccountHierarchy, andFundHierarchyare structurally identical (same fields, sameLevels()implementation). A shared abstract base class or a single genericLevelBasedHierarchyimplementingISegmentHierarchywould remove this triplication and centralize future changes (e.g., adding a level or changing the levels-building logic).🤖 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/ActivityHierarchy.cs` around lines 1 - 34, The hierarchy model logic is duplicated across ActivityHierarchy, AccountHierarchy, and FundHierarchy, so consolidate the shared fields and Levels() implementation into a common abstraction such as a base class or a generic LevelBasedHierarchy that implements ISegmentHierarchy. Move the repeated ParentLevel0Code/Name through ParentLevel5Code/Name properties and the current Levels() builder logic there, then have ActivityHierarchy inherit from it so future changes only happen once.server.core/Data/SeedCsv.cs (2)
20-21: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNaive comma split will silently corrupt data if any field contains a comma.
line.Split(',')has no quote/escape handling. The doc comment asserts source CSVs contain no embedded commas, but description/name fields (e.g. fund/department descriptions) are exactly the kind of free text prone to commas, and a violation would silently shift columns rather than throw — hard to detect during seeding.Consider using a lightweight, well-tested CSV parser (e.g.
CsvHelper, already common in .NET ecosystems) instead of rolling a manual splitter, especially since this seeds multiple hierarchy tables from external exports.🤖 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/SeedCsv.cs` around lines 20 - 21, The CSV parsing in SeedCsv is using a naive line.Split(',') approach that can corrupt rows when fields contain commas. Update the row parsing in SeedCsv to use a proper CSV parser with quote/escape handling instead of manual splitting, and make sure the mapping logic that feeds rows.Add(map(...)) still receives the correct column values for all seeded hierarchy exports.
33-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Single()resource lookup will throw if names collide.
GetManifestResourceNames().Single(candidate => candidate.EndsWith(fileName, ...))throws an unhelpfulInvalidOperationException("sequence contains more than one element") if two embedded resources happen to share a filename suffix. Low risk given current file naming, but worth confirming resource naming stays unique as more CSVs are added.🤖 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/SeedCsv.cs` around lines 33 - 40, The resource lookup in OpenResource currently uses Single() with an EndsWith match, so it will fail with an unhelpful exception if multiple embedded resources share the same filename suffix. Update SeedCsv.OpenResource to use a lookup that guards against collisions by ensuring the matched resource name is unique, and if multiple matches exist, throw a clear InvalidOperationException that includes the conflicting names; keep the existing SeedCsv/Assembly resource loading flow intact.server.core/Data/HierarchySeed.cs (1)
78-94: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDuplicate G-codes are silently dropped via
GroupBy(...).First().If
DepartmentSource.csvhas multiple rows sharing the same level-G code but differing parent chains, only the first-encountered row's D-F ancestry survives; the rest are dropped without warning. Since this is dev-only seed data derived from an external export, worth a quick sanity check that G codes are actually unique in the source, or add a comment/assertion documenting the assumption.🤖 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 78 - 94, The DepartmentRows seeding logic is deduplicating level-G records with GroupBy(...).First(), so any duplicate G-code rows with different ancestry are silently discarded. Update DepartmentRows in HierarchySeed to either verify and assert that f[6] is unique in DepartmentSource.csv or add an explicit comment/guard documenting the uniqueness assumption before selecting the first row. Keep the fix centered on the DepartmentRows / SeedCsv.ReadRows pipeline so the assumption is obvious to future maintainers.tests/server.tests/ChartStringSegments/HierarchyMappingTests.cs (1)
26-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name implies null-filtering but no null case is exercised.
The test asserts behavior only when both parent-level pairs are populated. Since the name claims "non_null_pairs," consider adding a case where one
ParentLevelXCode/Namepair is null to confirmLevels()actually filters it out rather than including a partially-null entry.🧪 Suggested additional test case
[Fact] public void Department_levels_filters_out_null_parent_levels() { var dept = new DepartmentHierarchy { Code = "031000", Description = "Plant Sciences", ParentLevelACode = "CAES", ParentLevelAName = "Ag & Env Sciences", ParentLevelBCode = null, ParentLevelBName = null, }; dept.Levels().Should().Equal( new HierarchyLevel("A", "CAES", "Ag & Env Sciences")); }🤖 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 26 - 40, The HierarchyMappingTests.Department_levels_return_ordered_non_null_pairs test only covers fully populated parent levels, so it does not verify the null-filtering behavior implied by the name. Update the test suite around DepartmentHierarchy.Levels() to add a case where one ParentLevelXCode/ParentLevelXName pair is null and assert that Levels() omits that entry instead of returning a partially null HierarchyLevel, keeping the existing ordered pair check intact.server.core/Data/DbInitializer.cs (1)
37-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding logging around dev seeding for consistency.
InitializeAsynclogs before/after migrations, butSeedDevelopmentAsyncseeds two tables silently. Adding log statements would help diagnose seeding issues in dev environments.📝 Suggested logging
private async Task SeedDevelopmentAsync(CancellationToken ct) { // Hierarchy first: chart-string segments are derived from the hierarchy tables. + _logger.LogInformation("Seeding development data..."); await HierarchySeed.EnsureSeededAsync(_db, ct); await ChartStringSegmentSeed.EnsureSeededAsync(_db, ct); + _logger.LogInformation("Development data seeded."); }🤖 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/DbInitializer.cs` around lines 37 - 42, Add consistent dev-seeding logs around the silent work in SeedDevelopmentAsync. In DbInitializer.SeedDevelopmentAsync, log when hierarchy and chart-string seeding starts and when it completes, similar to the before/after logging already done in InitializeAsync. Use clear messages tied to the SeedDevelopmentAsync flow so failures in HierarchySeed.EnsureSeededAsync and ChartStringSegmentSeed.EnsureSeededAsync are easier to diagnose in development.database/data/Tables/ChartStringSegments.sql (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment implies a continuous range, but valid SFNs are a sparse set.
The comment
SFN code (201..223)reads as an inclusive range, but per the PR only201, 202, 203, 205, 220, 221, 223are valid (e.g.204,206-219,222are not). Consider rewording to avoid implying intermediate values are valid, since this comment is the primary DB-level documentation of the column's semantics.✏️ Suggested comment wording
- [Sfn] NVARCHAR(10) NULL, -- SFN code (201..223) or 'Multiple'; only for Fund + [Sfn] NVARCHAR(10) NULL, -- SFN code (201, 202, 203, 205, 220, 221, 223) or 'Multiple'; only for Fund🤖 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 7, The column comment on Sfn currently implies a continuous SFN range, but the valid values are a sparse set. Update the documentation in ChartStringSegments.sql near the Sfn definition to describe the allowed discrete codes explicitly (or as a non-contiguous set) instead of “201..223”, so the schema comment matches the semantics used by the related fund/Sfn handling.
🤖 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/Tables/DepartmentHierarchy.sql`:
- Line 3: The `Code` column in `DepartmentHierarchy` is mismatched with the join
key type used by `ChartStringSegments.Code`, which can cause implicit
conversions and missed matches. Update the hierarchy table schema to align
`Code` with the `ChartStringSegments.Code` definition, and apply the same fix to
the related hierarchy tables referenced by `HierarchyMappingTests`
(`AccountHierarchy`, `ActivityHierarchy`, and `FundHierarchy`). Keep the `Code`
definition consistent across these tables so joins and lookups on `Code` remain
sargable and accept the full expected length.
---
Nitpick comments:
In `@database/data/Tables/ChartStringSegments.sql`:
- Line 7: The column comment on Sfn currently implies a continuous SFN range,
but the valid values are a sparse set. Update the documentation in
ChartStringSegments.sql near the Sfn definition to describe the allowed discrete
codes explicitly (or as a non-contiguous set) instead of “201..223”, so the
schema comment matches the semantics used by the related fund/Sfn handling.
In `@server.core/Data/ChartStringSegmentSeed.cs`:
- Around line 18-22: The SFN values are duplicated in ChartStringSegmentSeed and
FundSfns, so move the canonical SFN list into a shared constant in server.core
near ChartStringSegment and have both ChartStringSegmentSeed.FundSfnPool and
FundSfns.cs reference that single source. Update the seed logic to use the
shared list instead of its local array so the valid SFN set stays consistent and
avoids drift.
In `@server.core/Data/DbInitializer.cs`:
- Around line 37-42: Add consistent dev-seeding logs around the silent work in
SeedDevelopmentAsync. In DbInitializer.SeedDevelopmentAsync, log when hierarchy
and chart-string seeding starts and when it completes, similar to the
before/after logging already done in InitializeAsync. Use clear messages tied to
the SeedDevelopmentAsync flow so failures in HierarchySeed.EnsureSeededAsync and
ChartStringSegmentSeed.EnsureSeededAsync are easier to diagnose in development.
In `@server.core/Data/HierarchySeed.cs`:
- Around line 78-94: The DepartmentRows seeding logic is deduplicating level-G
records with GroupBy(...).First(), so any duplicate G-code rows with different
ancestry are silently discarded. Update DepartmentRows in HierarchySeed to
either verify and assert that f[6] is unique in DepartmentSource.csv or add an
explicit comment/guard documenting the uniqueness assumption before selecting
the first row. Keep the fix centered on the DepartmentRows / SeedCsv.ReadRows
pipeline so the assumption is obvious to future maintainers.
In `@server.core/Data/SeedCsv.cs`:
- Around line 20-21: The CSV parsing in SeedCsv is using a naive line.Split(',')
approach that can corrupt rows when fields contain commas. Update the row
parsing in SeedCsv to use a proper CSV parser with quote/escape handling instead
of manual splitting, and make sure the mapping logic that feeds
rows.Add(map(...)) still receives the correct column values for all seeded
hierarchy exports.
- Around line 33-40: The resource lookup in OpenResource currently uses Single()
with an EndsWith match, so it will fail with an unhelpful exception if multiple
embedded resources share the same filename suffix. Update SeedCsv.OpenResource
to use a lookup that guards against collisions by ensuring the matched resource
name is unique, and if multiple matches exist, throw a clear
InvalidOperationException that includes the conflicting names; keep the existing
SeedCsv/Assembly resource loading flow intact.
In `@server.core/Domain/ActivityHierarchy.cs`:
- Around line 1-34: The hierarchy model logic is duplicated across
ActivityHierarchy, AccountHierarchy, and FundHierarchy, so consolidate the
shared fields and Levels() implementation into a common abstraction such as a
base class or a generic LevelBasedHierarchy that implements ISegmentHierarchy.
Move the repeated ParentLevel0Code/Name through ParentLevel5Code/Name properties
and the current Levels() builder logic there, then have ActivityHierarchy
inherit from it so future changes only happen once.
In `@tests/server.tests/ChartStringSegments/HierarchyMappingTests.cs`:
- Around line 26-40: The
HierarchyMappingTests.Department_levels_return_ordered_non_null_pairs test only
covers fully populated parent levels, so it does not verify the null-filtering
behavior implied by the name. Update the test suite around
DepartmentHierarchy.Levels() to add a case where one
ParentLevelXCode/ParentLevelXName pair is null and assert that Levels() omits
that entry instead of returning a partially null HierarchyLevel, keeping the
existing ordered pair check intact.
🪄 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: 69ad2a19-2c95-421a-bbea-034001d46d74
⛔ Files ignored due to path filters (5)
server.core/Data/Seed/AccountHierarchy.csvis excluded by!**/*.csvserver.core/Data/Seed/ActivityHierarchy.csvis excluded by!**/*.csvserver.core/Data/Seed/DepartmentSource.csvis excluded by!**/*.csvserver.core/Data/Seed/ErnCodes.csvis excluded by!**/*.csvserver.core/Data/Seed/FundHierarchy.csvis excluded by!**/*.csv
📒 Files selected for processing (37)
client/src/components/dataClassification/DataClassificationStage.tsxclient/src/components/dataClassification/SegmentClassificationControl.tsxclient/src/components/dataClassification/SegmentGrid.tsxclient/src/components/dataClassification/segments.tsclient/src/queries/chartStringSegments.tsclient/src/routes/(authenticated)/workflow.$stageId.tsxclient/src/shared/dataTable.tsxclient/src/test/components/dataClassification/SegmentClassificationControl.test.tsxclient/src/test/components/dataClassification/SegmentGrid.test.tsxclient/src/test/components/dataClassification/segments.test.tsclient/src/test/routes/(authenticated)/dataClassification.test.tsxclient/src/test/setup.tsdatabase/data/Tables/AccountHierarchy.sqldatabase/data/Tables/ActivityHierarchy.sqldatabase/data/Tables/ChartStringSegments.sqldatabase/data/Tables/DepartmentHierarchy.sqldatabase/data/Tables/FundHierarchy.sqlserver.core/Data/AppDbContext.csserver.core/Data/ChartStringSegmentSeed.csserver.core/Data/DbInitializer.csserver.core/Data/HierarchySeed.csserver.core/Data/SeedCsv.csserver.core/Domain/AccountHierarchy.csserver.core/Domain/ActivityHierarchy.csserver.core/Domain/ChartStringSegment.csserver.core/Domain/DepartmentHierarchy.csserver.core/Domain/FundHierarchy.csserver.core/Domain/SegmentHierarchy.csserver.core/server.core.csprojserver/Controllers/ChartStringSegmentsController.csserver/Models/ChartStringSegments/ChartStringSegmentDtos.csserver/Models/ChartStringSegments/FundSfns.cstests/server.tests/ChartStringSegments/ChartStringSegmentMappingTests.cstests/server.tests/ChartStringSegments/ChartStringSegmentSeedTests.cstests/server.tests/ChartStringSegments/ChartStringSegmentsControllerTests.cstests/server.tests/ChartStringSegments/HierarchyMappingTests.cstests/server.tests/ChartStringSegments/HierarchySeedTests.cs
The hierarchy tables' Code join key was VARCHAR(20) while ChartStringSegments.Code is NVARCHAR(50), risking implicit conversions in SQL joins and truncating codes longer than 20 chars. Widen Code to NVARCHAR(50) across the four hierarchy tables and match the EF max length; parent-level codes stay VARCHAR(20). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <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
Builds out the Data Classification workflow: reviewers classify chart-string segments and earnings codes across five tabs, with the segment hierarchy shown inline.
Features
201, 202, 203, 205, 220, 221, 223),Multiple, orExcluded. Choosing an SFN/Multiple includes the fund; Excluded clears the SFN. Server-validated.data-schema hierarchy tables (Department A–G, Account/Fund/Activity 0–5), EF entities, and a segments-API join. The grid shows one column per level; each code hovers (instant daisyUI tooltip) to reveal its description.Data
Hierarchy and ERN tables are populated from embedded CSV dev seed only. Chart-string segments are derived from the hierarchy so every row shows a full breadcrumb; most are pre-classified with a few left unset per tab.
Deploy / testing notes
database/data/publish-local.shbefore testing against a local/shared dev SQL instance. The new hierarchy tables and the widenedChartStringSegments.Sfncolumn (NVARCHAR(3)→NVARCHAR(10)) are DACPAC-managed and excluded from EF migrations — EF will not create/alter them. The Azure deploy workflow already runs the DACPAC publish.Multipleis a special SFN literal, not a real SFN code.Summary by CodeRabbit
Summary by CodeRabbit