fix(clinical): detect duplicate schedules by SQL error number by rlorenzo · Pull Request #229 · ucdavis/VIPER

rlorenzo · 2026-06-17T01:14:19Z

What

In ScheduleEditService.AddInstructorAsync, the post-SaveChanges catch decides whether a DB failure means "instructor already scheduled" by SQL Server error number (2627 unique-constraint, 2601 duplicate-key-in-unique-index) instead of substring-matching the exception message.

private static bool IsDuplicateKeyViolation(Exception ex)
{
    var sqlEx = ex as SqlException ?? ex.InnerException as SqlException;
    return sqlEx is not null
        && sqlEx.Errors.Cast<SqlError>().Any(e => e.Number is 2627 or 2601);
}

Why

The old check matched "duplicate" / "unique" / "constraint" / "violation of primary key" against message.ToLower(). Two problems:

Latent bug: other constraint failures — foreign-key or check-constraint violations (error 547, whose message contains "constraint") — were mis-reported as "already scheduled." Now only true duplicate/unique violations get that message; everything else falls through to the generic "database operation failed" message.
Locale fragility: ToLower() plus English error-text matching is culture-sensitive (Turkish-I) and breaks if SQL messages are localized. Error numbers are stable and locale-invariant.

Notes

Behavior-preserving for the real duplicate case; only re-routes non-duplicate constraint errors to the correct (generic) message.
The duplicate pre-check (before SaveChanges) is unchanged; AddInstructorAsync_WithScheduleConflicts and all 2084 tests pass.
Supersedes the message-matching helper currently on Development via refactor(codeql): decompose complex conditions + harden Vite web-root check #227; the overlap will be resolved (keeping this version) at the Development merge.

codecov-commenter · 2026-06-17T01:15:00Z

Bundle Report

Bundle size has no change ✅

codecov-commenter · 2026-06-17T01:15:40Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 44.60%. Comparing base (ae61c44) to head (9114f3c).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #229      +/-   ##
==========================================
+ Coverage   44.58%   44.60%   +0.02%     
==========================================
  Files         896      896              
  Lines       51733    51737       +4     
  Branches     4834     4835       +1     
==========================================
+ Hits        23063    23078      +15     
+ Misses      28098    28087      -11     
  Partials      572      572

Flag	Coverage Δ
backend	`44.71% <100.00%> (+0.02%)`	⬆️
frontend	`42.36% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Copilot

Pull request overview

Refines error handling in the Clinical Scheduler’s ScheduleEditService.AddInstructorAsync so that “already scheduled” is only returned for true SQL Server duplicate/unique key violations, using stable SQL error numbers instead of locale-dependent substring matching on exception messages.

Changes:

Replaces exception message substring matching with SQL Server error-number detection (2627, 2601) to identify duplicate-key violations.
Adds a focused helper (IsDuplicateKeyViolation) to unwrap EF/SQL exceptions and check SqlException.Errors.

rlorenzo · 2026-06-26T04:11:34Z

@coderabbitai full review

coderabbitai · 2026-06-26T04:11:38Z

✅ Action performed

Full review finished.

coderabbitai · 2026-06-26T04:13:58Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0aff4536-bd88-44af-8084-67a2f464bda3

📥 Commits

Reviewing files that changed from the base of the PR and between c881eba and 84afaba.

📒 Files selected for processing (4)

test/ClinicalScheduler/ScheduleEditServiceTest.cs
test/ClinicalScheduler/SqlExceptionFactory.cs
test/ClinicalScheduler/ToggleThrowContext.cs
web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs

📝 Walkthrough

Walkthrough

AddInstructorAsync now classifies SQL Server duplicate-key failures by error number instead of exception-message text, returning the existing “already scheduled” error for those cases and a generic database failure for other exceptions. Tests add a one-shot save-failure context and SQL exception factory to cover the new paths.

Changes

Duplicate-key error handling

Layer / File(s)	Summary
Duplicate-key classification `web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs`	`AddInstructorAsync` now calls `IsDuplicateKeyViolation` in its database-exception catch path, and the new helper checks `SqlException.Errors` for SQL Server error numbers `2627` and `2601`.
Test save-failure flow `test/ClinicalScheduler/ToggleThrowContext.cs`, `test/ClinicalScheduler/SqlExceptionFactory.cs`, `test/ClinicalScheduler/ScheduleEditServiceTest.cs`	`ToggleThrowContext` adds one-shot save exceptions, `SqlExceptionFactory` constructs SQL Server exceptions by error number, and the service tests verify duplicate-key and generic database failure mapping.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

ucdavis/VIPER#193: Related ScheduleEditService exception-handling changes around SQL-specific duplicate/unique key handling.

Suggested reviewers: bsedwards

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 18.75% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: detecting duplicate schedules by SQL error number.
Description check	✅ Passed	The description accurately explains the SQL error-number change and why it replaces message matching.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/clinical-duplicate-key-detection

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

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 `@web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs`:
- Around line 250-252: Add targeted tests for the exception classification logic
in IsDuplicateKeyViolation within ScheduleEditService so both branches are
covered. Create tests that feed the service a duplicate-key SqlException with
error numbers 2627 and 2601 and verify the duplicate-schedule path, plus a
non-duplicate SqlException/DbUpdateException that verifies the generic
database-failure path. Make sure the tests assert the downstream API error
mapping contract stays distinct for these two cases.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 109ef738-ea05-4f15-b163-60f3a498908e

📥 Commits

Reviewing files that changed from the base of the PR and between 1a0996e and 06ab238.

📒 Files selected for processing (1)

web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

rlorenzo · 2026-07-01T05:01:52Z

@coderabbitai full review

coderabbitai · 2026-07-01T05:01:57Z

✅ Action performed

Full review finished.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

rlorenzo · 2026-07-01T06:13:55Z

@coderabbitai review

coderabbitai · 2026-07-01T06:14:00Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

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 `@web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs`:
- Around line 262-269: The duplicate-key detection in IsDuplicateKeyViolation
only checks one inner exception level, which is brittle if the SqlException is
wrapped more than once. Update the method to walk the full exception chain
starting from ex until you find a SqlException, then keep the existing 2627/2601
error-number check so the duplicate-key path still works even with deeper
wrapping.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 55cf54a0-368a-4cd5-9873-058d30a19dea

📥 Commits

Reviewing files that changed from the base of the PR and between 85efd3c and c881eba.

📒 Files selected for processing (4)

test/ClinicalScheduler/ScheduleEditServiceTest.cs
test/ClinicalScheduler/SqlExceptionFactory.cs
test/ClinicalScheduler/ToggleThrowContext.cs
web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs

rlorenzo · 2026-07-01T08:05:07Z

@coderabbitai review

coderabbitai · 2026-07-01T08:05:17Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Replace locale-fragile message-substring matching with SQL Server error numbers 2627/2601 (unique constraint / duplicate key in a unique index) to decide whether a save failure means the instructor is already scheduled. Other database errors (foreign-key, check constraints) now fall through to the generic message instead of mis-reporting "already scheduled", and the check no longer depends on English error text or a culture-specific ToLower().

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

rlorenzo requested a review from Copilot June 17, 2026 01:14

Copilot started reviewing on behalf of rlorenzo June 17, 2026 01:14 View session

Copilot AI reviewed Jun 17, 2026

View reviewed changes

rlorenzo mentioned this pull request Jun 17, 2026

refactor(codeql): decompose complex conditions + harden Vite web-root check #227

Merged

rlorenzo force-pushed the fix/clinical-duplicate-key-detection branch from 384a05b to 06ab238 Compare June 26, 2026 03:06

coderabbitai Bot reviewed Jun 26, 2026

View reviewed changes

Comment thread web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs

rlorenzo force-pushed the fix/clinical-duplicate-key-detection branch from 06ab238 to b9765ea Compare June 26, 2026 05:30

rlorenzo requested a review from Copilot June 26, 2026 05:31

Copilot started reviewing on behalf of rlorenzo June 26, 2026 05:31 View session

Copilot AI reviewed Jun 26, 2026

View reviewed changes

Comment thread test/ClinicalScheduler/ToggleThrowContext.cs Outdated

rlorenzo force-pushed the fix/clinical-duplicate-key-detection branch from b9765ea to 85efd3c Compare July 1, 2026 05:01

rlorenzo requested a review from Copilot July 1, 2026 05:12

Copilot started reviewing on behalf of rlorenzo July 1, 2026 05:13 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

rlorenzo force-pushed the fix/clinical-duplicate-key-detection branch from 85efd3c to c881eba Compare July 1, 2026 06:13

coderabbitai Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs

rlorenzo force-pushed the fix/clinical-duplicate-key-detection branch from c881eba to 84afaba Compare July 1, 2026 08:04

rlorenzo requested a review from Copilot July 1, 2026 18:23

rlorenzo force-pushed the fix/clinical-duplicate-key-detection branch from 84afaba to 9114f3c Compare July 1, 2026 18:24

Copilot started reviewing on behalf of rlorenzo July 1, 2026 18:24 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

Comment thread test/ClinicalScheduler/SqlExceptionFactory.cs

Uh oh!

Conversation

rlorenzo commented Jun 17, 2026

What

Why

Notes

Uh oh!

codecov-commenter commented Jun 17, 2026

Bundle Report

Uh oh!

codecov-commenter commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

rlorenzo commented Jun 26, 2026

Uh oh!

coderabbitai Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

rlorenzo commented Jul 1, 2026

Uh oh!

coderabbitai Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

rlorenzo commented Jul 1, 2026

Uh oh!

coderabbitai Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rlorenzo commented Jul 1, 2026

Uh oh!

coderabbitai Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Jun 17, 2026 •

edited

Loading

coderabbitai Bot commented Jun 26, 2026 •

edited

Loading

coderabbitai Bot commented Jun 26, 2026 •

edited

Loading

coderabbitai Bot commented Jul 1, 2026 •

edited

Loading

coderabbitai Bot commented Jul 1, 2026 •

edited

Loading

coderabbitai Bot commented Jul 1, 2026 •

edited

Loading