New balances table w/ test examples#14
Conversation
📝 WalkthroughWalkthroughAdds the ChangesEmployee accrual balances
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DbInitializer
participant AppDbContext
participant Database
DbInitializer->>AppDbContext: Query existing EmployeeAccrualBalance keys
AppDbContext->>Database: Read EmployeeAccrualBalances
Database-->>AppDbContext: Return existing keys
AppDbContext-->>DbInitializer: Identify missing seed records
DbInitializer->>AppDbContext: Add mapped balances
DbInitializer->>Database: SaveChangesAsync
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server.core/Domain/EmployeeAccrualBalance.cs (1)
76-89: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConfirm
nvarchar(max)is intentional for short status/flag codes.
HrStatus,EmployeeStatus,EmployeeType, andApproachingMaxare mapped asnvarchar(max), yetDbInitializer.CreateEmployeeAccrualBalanceonly ever populates them with single-character values ("A", "E", "Y"/"N"). Sibling short codes on this same entity (UnionCode,EmployeeClassCode) are correctly bounded (nvarchar(3)). Since this table's purpose is to "establish the intended shape" for Fabric-sourced data, worth confirming these widths actually mirror the source columns — unboundednvarchar(max)wastes storage and blocks efficient indexing/filtering if these fields are ever queried.♻️ Example bound if source values are short codes
- entity.Property(e => e.HrStatus).HasColumnType("nvarchar(max)"); - entity.Property(e => e.EmployeeStatus).HasColumnType("nvarchar(max)"); + entity.Property(e => e.HrStatus).HasColumnType("nvarchar(1)").HasMaxLength(1); + entity.Property(e => e.EmployeeStatus).HasColumnType("nvarchar(1)").HasMaxLength(1);🤖 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/EmployeeAccrualBalance.cs` around lines 76 - 89, Update the EmployeeAccrualBalance mappings for HrStatus, EmployeeStatus, EmployeeType, and ApproachingMax to use bounded nvarchar lengths matching their Fabric source columns and short-code values, consistent with UnionCode and EmployeeClassCode; do not leave them as nvarchar(max) unless the source schema confirms they are intentionally unbounded.server.core/Data/DbInitializer.cs (1)
528-588: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify the sign convention of
HoursOverUnderPolicyMax.
HoursOverUnderPolicyMax = AccrualLimit - CalculatedBal(line 571) produces a positive value when the employee is well under the cap — e.g. the test expects+152.62for a balance of87.38against a240.00limit. That's the inverse of what the property name typically implies (positive = over the max, negative = under). If a real employee ever exceeds the cap, this formula goes negative, which downstream consumers reading the field literally ("hours over policy max") would likely misinterpret.Since this table is meant to mirror the Fabric source shape, please confirm the source system's actual sign convention for this column before this dev-seed placeholder becomes the reference implementation for real ingestion 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/Data/DbInitializer.cs` around lines 528 - 588, Verify the Fabric source sign convention for HoursOverUnderPolicyMax, then update the assignment in CreateEmployeeAccrualBalance to match it rather than assuming AccrualLimit minus CalculatedBal. Preserve the zero-limit handling and ensure under-cap and over-cap values have the source-defined signs.
🤖 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.
Nitpick comments:
In `@server.core/Data/DbInitializer.cs`:
- Around line 528-588: Verify the Fabric source sign convention for
HoursOverUnderPolicyMax, then update the assignment in
CreateEmployeeAccrualBalance to match it rather than assuming AccrualLimit minus
CalculatedBal. Preserve the zero-limit handling and ensure under-cap and
over-cap values have the source-defined signs.
In `@server.core/Domain/EmployeeAccrualBalance.cs`:
- Around line 76-89: Update the EmployeeAccrualBalance mappings for HrStatus,
EmployeeStatus, EmployeeType, and ApproachingMax to use bounded nvarchar lengths
matching their Fabric source columns and short-code values, consistent with
UnionCode and EmployeeClassCode; do not leave them as nvarchar(max) unless the
source schema confirms they are intentionally unbounded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 608826c4-a10d-422f-82f8-fdf8b84c10dd
📒 Files selected for processing (7)
server.core/Data/AppDbContext.csserver.core/Data/DbInitializer.csserver.core/Domain/EmployeeAccrualBalance.csserver.core/Migrations/20260716171119_AddEmployeeAccrualBalances.Designer.csserver.core/Migrations/20260716171119_AddEmployeeAccrualBalances.csserver.core/Migrations/AppDbContextModelSnapshot.cstests/server.tests/Data/EmployeeAccrualBalanceTests.cs
3 participants
Will eventually be populated by fabric but this is the right shape and gives us something to work with
Summary by CodeRabbit
New Features
Bug Fixes
Tests