Majority of DB Tables#15
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds new core domain entities and EF Core schema, changes leave-request identifiers to ChangesCore domain and persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Program
participant DbInitializer
participant AppDbContext
participant Database
Program->>DbInitializer: InitializeAsync()
DbInitializer->>Database: Apply migrations
DbInitializer->>AppDbContext: Seed people and assignments
DbInitializer->>AppDbContext: Expand leave requests into days
DbInitializer->>AppDbContext: Seed outbound messages
AppDbContext->>Database: Save changes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 3
🧹 Nitpick comments (1)
server/Program.cs (1)
93-97: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer
CreateAsyncScopewhen resolving services within an async context.Using
CreateAsyncScopeensures that any underlying scoped dependencies implementingIAsyncDisposable(such asAppDbContext) are disposed asynchronously, avoiding potential "sync-over-async" blocking issues during startup.♻️ Proposed refactor
- using (var scope = app.Services.CreateScope()) + await using (var scope = app.Services.CreateAsyncScope()) { var init = scope.ServiceProvider.GetRequiredService<IDbInitializer>(); await init.InitializeAsync(); }🤖 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/Program.cs` around lines 93 - 97, Update the startup service scope around IDbInitializer.InitializeAsync in Program.cs to use the asynchronous scope creation API, ensuring the scope is disposed asynchronously while preserving the existing initialization flow.
🤖 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/DbInitializer.cs`:
- Line 934: Update the seeding assignments in the DbInitializer flow, including
the locations around the shown assignment and the additional 1015-1026
occurrences, to use the supplied PersonSeed.IsCampusEmployee value rather than
deriving IsCampusEmployee from seed.IsEmployee. Preserve the existing output
mapping and apply the correction consistently to every affected seed path.
- Line 9: Gate the development seed block in DbInitializer.InitializeAsync so
deterministic users, admin assignments, fake records, and pending outbound
messages run only when an explicit configuration flag is enabled. Make the flag
default to disabled, preserving production-safe behavior while allowing
intentional development seeding without changing the unconditional startup call
in Program.cs.
In `@server.core/Domain/ClusterCaoAssignment.cs`:
- Around line 52-56: Add unique indexes enforcing each initializer identity: in
server.core/Domain/ClusterCaoAssignment.cs lines 52-56 use (ClusterId, IamId,
EffectiveStartDate); in server.core/Domain/DepartmentChairAssignment.cs lines
54-58 use (DepartmentCode, IamId, EffectiveStartDate); and in
server.core/Domain/EmployeeReportingDepartmentOverride.cs lines 58-62 use
(IamId, EffectiveStartDate). Regenerate
server.core/Migrations/20260717155630_AddCoreDomainTables.cs to include these
constraints.
---
Nitpick comments:
In `@server/Program.cs`:
- Around line 93-97: Update the startup service scope around
IDbInitializer.InitializeAsync in Program.cs to use the asynchronous scope
creation API, ensuring the scope is disposed asynchronously while preserving the
existing initialization flow.
🪄 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: 1e846d80-4548-4945-9a2f-ae6eee7a898b
📒 Files selected for processing (19)
server.core/Data/AppDbContext.csserver.core/Data/DbInitializer.csserver.core/Domain/AppSetting.csserver.core/Domain/AppUser.csserver.core/Domain/Cluster.csserver.core/Domain/ClusterCaoAssignment.csserver.core/Domain/Department.csserver.core/Domain/DepartmentChairAssignment.csserver.core/Domain/EmployeeReportingDepartmentOverride.csserver.core/Domain/LeaveRequest.csserver.core/Domain/LeaveRequestDay.csserver.core/Domain/OutboundMessage.csserver.core/Domain/OutboundMessageStatus.csserver.core/Domain/Person.csserver.core/Migrations/20260717155630_AddCoreDomainTables.Designer.csserver.core/Migrations/20260717155630_AddCoreDomainTables.csserver.core/Migrations/AppDbContextModelSnapshot.csserver/Program.cstests/server.tests/Data/EmployeeAccrualBalanceTests.cs
| public interface IDbInitializer | ||
| { | ||
| Task InitializeAsync(bool includeDevSeed, CancellationToken cancellationToken = default); | ||
| Task InitializeAsync(CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not run development seeding in every environment.
Line 169 now inserts deterministic development users, admin assignments, fake records, and pending outbound messages whenever startup initialization runs. Because server/Program.cs calls this unconditionally, production databases can receive development principals and privileges. Restore an explicit production-safe gate that defaults off.
Proposed fix
public interface IDbInitializer
{
- Task InitializeAsync(CancellationToken cancellationToken = default);
+ Task InitializeAsync(
+ bool includeDevelopmentSeed = false,
+ CancellationToken cancellationToken = default);
}
-public async Task InitializeAsync(CancellationToken cancellationToken = default)
+public async Task InitializeAsync(
+ bool includeDevelopmentSeed = false,
+ CancellationToken cancellationToken = default)
{
// Apply migrations...
- await SeedDevelopmentAsync(cancellationToken);
+ if (includeDevelopmentSeed)
+ {
+ await SeedDevelopmentAsync(cancellationToken);
+ }
}Also applies to: 154-169
🤖 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` at line 9, Gate the development seed block
in DbInitializer.InitializeAsync so deterministic users, admin assignments, fake
records, and pending outbound messages run only when an explicit configuration
flag is enabled. Make the flag default to disabled, preserving production-safe
behavior while allowing intentional development seeding without changing the
unconditional startup call in Program.cs.
| IsStaff = seed.IsStaff, | ||
| IsExternal = false, | ||
| PrivacyCode = "N", | ||
| IsCampusEmployee = seed.IsEmployee ? "Y" : "N", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use the supplied campus-employee flag.
PersonSeed.IsCampusEmployee is ignored, and Line 934 instead derives this value from IsEmployee. That conflates non-campus employees with campus employees.
Proposed fix
- IsCampusEmployee = seed.IsEmployee ? "Y" : "N",
+ IsCampusEmployee = seed.IsCampusEmployee ? "Y" : "N",Also applies to: 1015-1026
🤖 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` at line 934, Update the seeding
assignments in the DbInitializer flow, including the locations around the shown
assignment and the additional 1015-1026 occurrences, to use the supplied
PersonSeed.IsCampusEmployee value rather than deriving IsCampusEmployee from
seed.IsEmployee. Preserve the existing output mapping and apply the correction
consistently to every affected seed path.
Comment on lines
+52
to
+56
| entity.HasIndex(e => new { e.ClusterId, e.EffectiveStartDate, e.EffectiveEndDateExclusive }) | ||
| .HasDatabaseName("IX_CaoAssignment_Cluster_EffectiveDates"); | ||
|
|
||
| entity.HasIndex(e => e.IamId) | ||
| .HasDatabaseName("IX_CaoAssignment_IamId"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce initializer identity keys with unique database indexes.
The three seed workflows use identity tuples that the schema does not enforce, allowing duplicate rows during concurrent initialization or ordinary writes.
server.core/Domain/ClusterCaoAssignment.cs#L52-L56: add a unique index on(ClusterId, IamId, EffectiveStartDate).server.core/Domain/DepartmentChairAssignment.cs#L54-L58: add a unique index on(DepartmentCode, IamId, EffectiveStartDate).server.core/Domain/EmployeeReportingDepartmentOverride.cs#L58-L62: add a unique index on(IamId, EffectiveStartDate).
Regenerate server.core/Migrations/20260717155630_AddCoreDomainTables.cs afterward.
📍 Affects 3 files
server.core/Domain/ClusterCaoAssignment.cs#L52-L56(this comment)server.core/Domain/DepartmentChairAssignment.cs#L54-L58server.core/Domain/EmployeeReportingDepartmentOverride.cs#L58-L62
🤖 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/ClusterCaoAssignment.cs` around lines 52 - 56, Add unique
indexes enforcing each initializer identity: in
server.core/Domain/ClusterCaoAssignment.cs lines 52-56 use (ClusterId, IamId,
EffectiveStartDate); in server.core/Domain/DepartmentChairAssignment.cs lines
54-58 use (DepartmentCode, IamId, EffectiveStartDate); and in
server.core/Domain/EmployeeReportingDepartmentOverride.cs lines 58-62 use
(IamId, EffectiveStartDate). Regenerate
server.core/Migrations/20260717155630_AddCoreDomainTables.cs to include these
constraints.
srkirkland
requested changes
Jul 17, 2026
| public class OutboundMessage | ||
| { | ||
| [Key] | ||
| public long Id { get; set; } |
Member
There was a problem hiding this comment.
let's make our identity IDs all int
| { | ||
| public DbSet<AppUser> AppUsers => Set<AppUser>(); | ||
| public DbSet<AppAdminAssignment> AppAdminAssignments => Set<AppAdminAssignment>(); | ||
| public DbSet<AppSetting> AppSettings => Set<AppSetting>(); |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Migrations/20260717181106_AddCoreDomainTables.cs`:
- Around line 68-72: Update the CreateIndex call for
UX_LeaveRequestAction_LeaveRequestId in the migration so the LeaveRequestId
index is non-unique, allowing multiple LeaveRequestAction records for the same
request while preserving the existing index and table configuration.
- Around line 30-56: Update the migration before the AlterColumn calls for
LeaveRequest.Id, LeaveRequestAction.Id, and LeaveRequestAction.LeaveRequestId to
safely handle existing bigint values: add a SQL data check/backfill that
guarantees all values fit within int, or retain these columns as bigint. Also
remove the unique constraint/index on LeaveRequestAction.LeaveRequestId so
multiple actions can reference the same LeaveRequest, matching the
LeaveRequest.Actions collection.
🪄 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: f02a5139-3008-4e1b-80e0-e278bd368984
📒 Files selected for processing (10)
server.core/Data/AppDbContext.csserver.core/Data/DbInitializer.csserver.core/Domain/AppUser.csserver.core/Domain/LeaveRequest.csserver.core/Domain/LeaveRequestAction.csserver.core/Domain/LeaveRequestDay.csserver.core/Domain/OutboundMessage.csserver.core/Migrations/20260717181106_AddCoreDomainTables.Designer.csserver.core/Migrations/20260717181106_AddCoreDomainTables.csserver.core/Migrations/AppDbContextModelSnapshot.cs
💤 Files with no reviewable changes (2)
- server.core/Domain/AppUser.cs
- server.core/Data/AppDbContext.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- server.core/Domain/LeaveRequestDay.cs
- server.core/Domain/OutboundMessage.cs
Comment on lines
+30
to
+56
| migrationBuilder.AlterColumn<int>( | ||
| name: "Id", | ||
| table: "LeaveRequest", | ||
| type: "int", | ||
| nullable: false, | ||
| oldClrType: typeof(long), | ||
| oldType: "bigint") | ||
| .Annotation("SqlServer:Identity", "1, 1") | ||
| .OldAnnotation("SqlServer:Identity", "1, 1"); | ||
|
|
||
| migrationBuilder.AlterColumn<int>( | ||
| name: "LeaveRequestId", | ||
| table: "LeaveRequestAction", | ||
| type: "int", | ||
| nullable: false, | ||
| oldClrType: typeof(long), | ||
| oldType: "bigint"); | ||
|
|
||
| migrationBuilder.AlterColumn<int>( | ||
| name: "Id", | ||
| table: "LeaveRequestAction", | ||
| type: "int", | ||
| nullable: false, | ||
| oldClrType: typeof(long), | ||
| oldType: "bigint") | ||
| .Annotation("SqlServer:Identity", "1, 1") | ||
| .OldAnnotation("SqlServer:Identity", "1, 1"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the migration and related entity/model definitions.
git ls-files 'server.core/**' | rg '20260717181106_AddCoreDomainTables\.cs|LeaveRequest|LeaveRequestAction|DbContext|ModelCreating|EntityTypeConfiguration'
echo '--- migration outline ---'
ast-grep outline server.core/Migrations/20260717181106_AddCoreDomainTables.cs --view expanded || true
echo '--- related files (filtered) ---'
for f in $(git ls-files 'server.core/**' | rg 'LeaveRequest(Action)?|DbContext|ModelCreating|EntityTypeConfiguration'); do
echo "### $f"
wc -l "$f"
done
echo '--- search for unique index definitions ---'
rg -n "LeaveRequestId|HasIndex|IsUnique|LeaveRequestAction" server.core -g '!**/bin/**' -g '!**/obj/**'Repository: ucdavis/leaves
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- migration slice ---'
sed -n '20,90p' server.core/Migrations/20260717181106_AddCoreDomainTables.cs | cat -n
echo '--- LeaveRequest entity ---'
sed -n '1,140p' server.core/Domain/LeaveRequest.cs | cat -n
echo '--- LeaveRequestAction entity ---'
sed -n '1,120p' server.core/Domain/LeaveRequestAction.cs | cat -n
echo '--- DbContext ---'
sed -n '1,80p' server.core/Data/AppDbContext.cs | cat -n
echo '--- snapshot relationship slice ---'
sed -n '708,765p' server.core/Migrations/AppDbContextModelSnapshot.cs | cat -nRepository: ucdavis/leaves
Length of output: 15869
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- LeaveRequestAction usages ---'
rg -n "new LeaveRequestAction|LeaveRequestActions|Actions\b|LeaveRequestActionType" server.core -g '!**/bin/**' -g '!**/obj/**'
echo '--- seed slice ---'
sed -n '630,690p' server.core/Data/DbInitializer.cs | cat -n
echo '--- maybe action creation helpers/usages ---'
rg -n "LeaveRequestAction" server.core -g '!**/bin/**' -g '!**/obj/**' | sed -n '1,120p'Repository: ucdavis/leaves
Length of output: 22933
Guard the bigint→int narrowing before this migration runs.
- Existing
LeaveRequest/LeaveRequestActionkeys outsideintwill make the column alters fail mid-deployment; add a data check/backfill or keep these IDs asbigint. LeaveRequestAction.LeaveRequestIdis still recreated asUNIQUEwhileLeaveRequest.Actionsis modeled as a collection, so the schema still only allows one action per request.
🤖 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/Migrations/20260717181106_AddCoreDomainTables.cs` around lines 30
- 56, Update the migration before the AlterColumn calls for LeaveRequest.Id,
LeaveRequestAction.Id, and LeaveRequestAction.LeaveRequestId to safely handle
existing bigint values: add a SQL data check/backfill that guarantees all values
fit within int, or retain these columns as bigint. Also remove the unique
constraint/index on LeaveRequestAction.LeaveRequestId so multiple actions can
reference the same LeaveRequest, matching the LeaveRequest.Actions collection.
Comment on lines
+68
to
+72
| migrationBuilder.CreateIndex( | ||
| name: "UX_LeaveRequestAction_LeaveRequestId", | ||
| table: "LeaveRequestAction", | ||
| column: "LeaveRequestId", | ||
| unique: true); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove uniqueness from the action foreign-key index.
LeaveRequest.Actions is a collection, but this unique index permits only one action per request. A second workflow action will violate the constraint.
Proposed fix
migrationBuilder.CreateIndex(
- name: "UX_LeaveRequestAction_LeaveRequestId",
+ name: "IX_LeaveRequestAction_LeaveRequestId",
table: "LeaveRequestAction",
- column: "LeaveRequestId",
- unique: true);
+ column: "LeaveRequestId");📝 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
| migrationBuilder.CreateIndex( | |
| name: "UX_LeaveRequestAction_LeaveRequestId", | |
| table: "LeaveRequestAction", | |
| column: "LeaveRequestId", | |
| unique: true); | |
| migrationBuilder.CreateIndex( | |
| name: "IX_LeaveRequestAction_LeaveRequestId", | |
| table: "LeaveRequestAction", | |
| column: "LeaveRequestId"); |
🤖 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/Migrations/20260717181106_AddCoreDomainTables.cs` around lines 68
- 72, Update the CreateIndex call for UX_LeaveRequestAction_LeaveRequestId in
the migration so the LeaveRequestId index is non-unique, allowing multiple
LeaveRequestAction records for the same request while preserving the existing
index and table configuration.
srkirkland
approved these changes
Jul 17, 2026
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.
Summary by CodeRabbit