From b7afead0f8a19807d23577b93a02ac7556a738ca Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Tue, 16 Jun 2026 11:27:56 -0700 Subject: [PATCH] chore(codeql): readonly fields and TryGetValue cleanups Mark single-assignment static fields readonly (RolesTests SQLite connection/options) and replace ContainsKey + indexer double lookups with TryGetValue (RotationsController recent-clinicians map, EmergencyContact test). Make BuildRecentCliniciansList internal static (it holds no instance state) and cover its TryGetValue name lookup and mothraId dedup with unit tests. Resolves cs/missed-readonly-modifier and cs/inefficient-containskey. Claude-Session: https://claude.ai/code/session_01LtL41k9i168SMvku6kdTfd --- .../RotationsControllerTest.cs | 65 +++++++++++++------ test/RAPS/RolesTests.cs | 4 +- .../EmergencyContactControllerTests.cs | 4 +- .../Controllers/RotationsController.cs | 6 +- 4 files changed, 52 insertions(+), 27 deletions(-) diff --git a/test/ClinicalScheduler/RotationsControllerTest.cs b/test/ClinicalScheduler/RotationsControllerTest.cs index 0ca3bf9d..aa74cd42 100644 --- a/test/ClinicalScheduler/RotationsControllerTest.cs +++ b/test/ClinicalScheduler/RotationsControllerTest.cs @@ -1,7 +1,6 @@ using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; -using MockQueryable.NSubstitute; using NSubstitute; using Viper.Areas.ClinicalScheduler.Controllers; using Viper.Areas.ClinicalScheduler.Models.DTOs.Responses; @@ -334,25 +333,6 @@ public async Task GetRotation_AccessDeniedRotation_ReturnsForbidden() #region BuildWeekScheduleItem Tests (lines 522-528) - // Empty InstructorSchedules avoids the Week navigation property NPE that occurs in - // GetRecentCliniciansAsync when MockQueryable doesn't load navigation properties. - private void SetupForScheduleResponse() - { - var instSched = new List().BuildMockDbSet(); - var rwp = new List().BuildMockDbSet(); - - MockContext.InstructorSchedules.Returns(instSched); - MockContext.RotationWeeklyPrefs.Returns(rwp); - - var baseDate = new DateTime(TestYear, 6, 1, 0, 0, 0, DateTimeKind.Utc); - _mockWeekService.GetWeeksAsync(Arg.Any(), Arg.Any(), Arg.Any()) - .Returns( - [ - new() { WeekId = 1, WeekNum = 1, DateStart = baseDate, DateEnd = baseDate.AddDays(6), TermCode = TestTermCode }, - new() { WeekId = 2, WeekNum = 2, DateStart = baseDate.AddDays(7), DateEnd = baseDate.AddDays(13), TermCode = TestTermCode } - ]); - } - private static RotationDto CardiologyRotationWithMinConsecutiveWeeks(int? minConsecutiveWeeks) => new() { RotId = CardiologyRotationId, @@ -411,6 +391,51 @@ public async Task GetRotationSchedule_WhenNoWeeks_AndServiceIsNull_ServiceIsNull #endregion + #region BuildRecentCliniciansList Tests + + [Fact] + public void BuildRecentCliniciansList_MapsKnownPersonName_AndFallsBackForUnknown() + { + // personData contains the first clinician but not the second, so each branch + // of the lookup is exercised: a hit resolves to the display name, a miss falls + // back to the "Clinician {mothraId}" label. + var personData = new Dictionary + { + ["known1"] = new Person { IdsMothraId = "known1", PersonDisplayFullName = "Known, Clinician" } + }; + + var result = RotationsController.BuildRecentCliniciansList(["known1", "missing1"], personData); + + var byMothraId = result + .Select(ReadClinician) + .ToDictionary(c => c.MothraId, c => c.FullName); + + Assert.Equal(2, byMothraId.Count); + Assert.Equal("Known, Clinician", byMothraId["known1"]); // found in personData + Assert.Equal("Clinician missing1", byMothraId["missing1"]); // missing from personData + } + + [Fact] + public void BuildRecentCliniciansList_DeduplicatesMothraIds() + { + var result = RotationsController.BuildRecentCliniciansList( + ["missing1", "missing1"], + new Dictionary()); + + Assert.Single(result); + } + + // Recent-clinician entries are anonymous objects on the response; read them by reflection. + private static (string MothraId, string FullName) ReadClinician(object item) + { + var type = item.GetType(); + var mothraId = (string)type.GetProperty("mothraId")!.GetValue(item)!; + var fullName = (string)type.GetProperty("fullName")!.GetValue(item)!; + return (mothraId, fullName); + } + + #endregion + #region GetRotationsWithScheduledWeeks Security Tests [Fact] diff --git a/test/RAPS/RolesTests.cs b/test/RAPS/RolesTests.cs index c3d57d60..ea6d64af 100644 --- a/test/RAPS/RolesTests.cs +++ b/test/RAPS/RolesTests.cs @@ -19,8 +19,8 @@ public class RolesTests /// /// Use the _sqlLiteConnection (see RejectAddRole_WhenDuplicateRoleId() to validate unique contraints or to do create, edit, delete actions /// - static SqliteConnection _sqlLiteConnection = new SqliteConnection("Filename=:memory:"); - static DbContextOptions _sqlLiteContextOptions = new DbContextOptionsBuilder() + static readonly SqliteConnection _sqlLiteConnection = new SqliteConnection("Filename=:memory:"); + static readonly DbContextOptions _sqlLiteContextOptions = new DbContextOptionsBuilder() .UseSqlite(_sqlLiteConnection) .Options; diff --git a/test/Students/EmergencyContactControllerTests.cs b/test/Students/EmergencyContactControllerTests.cs index ed48b494..2efc6b19 100644 --- a/test/Students/EmergencyContactControllerTests.cs +++ b/test/Students/EmergencyContactControllerTests.cs @@ -318,8 +318,8 @@ public async Task UpdateStudentContact_ArgumentException_ReturnsValidationProble // but in unit tests without an HttpContext it stays null — assert the body shape. var objectResult = Assert.IsType(result.Result); var problem = Assert.IsType(objectResult.Value); - Assert.True(problem.Errors.ContainsKey("PhoneValidation")); - Assert.Contains("Invalid phone number: 12345", problem.Errors["PhoneValidation"]); + Assert.True(problem.Errors.TryGetValue("PhoneValidation", out var phoneErrors)); + Assert.Contains("Invalid phone number: 12345", phoneErrors); } [Fact] diff --git a/web/Areas/ClinicalScheduler/Controllers/RotationsController.cs b/web/Areas/ClinicalScheduler/Controllers/RotationsController.cs index e0bf5c64..3ded479c 100644 --- a/web/Areas/ClinicalScheduler/Controllers/RotationsController.cs +++ b/web/Areas/ClinicalScheduler/Controllers/RotationsController.cs @@ -555,15 +555,15 @@ private List GroupSchedulesBySemester(IEnumerable weekSchedules /// /// Builds recent clinicians list with full names /// - private List BuildRecentCliniciansList(IEnumerable mothraIds, Dictionary personData) + internal static List BuildRecentCliniciansList(IEnumerable mothraIds, Dictionary personData) { return mothraIds .Distinct() .Select(mothraId => new { mothraId, - fullName = personData.ContainsKey(mothraId) - ? personData[mothraId].PersonDisplayFullName + fullName = personData.TryGetValue(mothraId, out var person) + ? person.PersonDisplayFullName : $"Clinician {mothraId}" }) .OrderBy(c => c.fullName)