diff --git a/VueApp/src/ClinicalScheduler/__tests__/audit-log-page-access.test.ts b/VueApp/src/ClinicalScheduler/__tests__/audit-log-page-access.test.ts new file mode 100644 index 000000000..9d578cbdd --- /dev/null +++ b/VueApp/src/ClinicalScheduler/__tests__/audit-log-page-access.test.ts @@ -0,0 +1,97 @@ +import { usePermissionsStore } from "../stores/permissions" +import { AuditLogService } from "../services/audit-log-service" +import { RotationService } from "../services/rotation-service" +import { PageDataService } from "../services/page-data-service" +import AuditLogPage from "../pages/AuditLogPage.vue" +import { + setupTest, + createTestWrapper, + waitForAsync, + createMockPermissionsStore, + createMockUserPermissions, +} from "./test-utils" +import type { UserPermissions } from "../types" + +type MockPermissionsStore = Omit, "userPermissions"> & { + hasManagePermission?: boolean + userPermissions: UserPermissions | null +} + +// Mock the permissions store and the services the page calls +vi.mock("../stores/permissions") +vi.mock("../services/audit-log-service") +vi.mock("../services/rotation-service") +vi.mock("../services/page-data-service") + +// Stub child components that hit the network or rely on named routes +vi.mock("../components/AccessDeniedCard.vue", () => ({ + default: { + name: "AccessDeniedCard", + props: ["message", "subtitle"], + template: '
Access Denied
{{message}}
', + }, +})) +vi.mock("../components/SchedulerNavigation.vue", () => ({ + default: { name: "SchedulerNavigation", template: "
" }, +})) + +describe("auditLogPage - Access Control", () => { + let mockPermissionsStore: MockPermissionsStore = {} as never + let router: any = {} + + beforeEach(() => { + const { router: testRouter, mockStore } = setupTest() + router = testRouter + mockPermissionsStore = mockStore + vi.mocked(usePermissionsStore).mockReturnValue(mockPermissionsStore as any) + vi.mocked(AuditLogService.getAuditLog).mockResolvedValue({ result: [], success: true, errors: [] }) + vi.mocked(AuditLogService.getModifiers).mockResolvedValue({ result: [], success: true, errors: [] }) + vi.mocked(AuditLogService.getPersons).mockResolvedValue({ result: [], success: true, errors: [] }) + vi.mocked(AuditLogService.getTerms).mockResolvedValue({ result: [], success: true, errors: [] }) + vi.mocked(RotationService.getRotations).mockResolvedValue({ result: [], success: true, errors: [] }) + vi.mocked(PageDataService.getPageData).mockResolvedValue({ currentGradYear: 2026, availableGradYears: [2026] }) + }) + + it("shows access denied when user lacks the manage permission", () => { + mockPermissionsStore.hasManagePermission = false + mockPermissionsStore.userPermissions = createMockUserPermissions() + + const wrapper = createTestWrapper({ component: AuditLogPage, router }) + + expect(wrapper.find(".no-access-card").exists()).toBeTruthy() + // The results table (and its "Modified By" column) only renders for managers. + expect(wrapper.text()).not.toContain("Modified By") + }) + + it("shows the audit trail and loads filter options for managers", async () => { + mockPermissionsStore.hasManagePermission = true + mockPermissionsStore.userPermissions = createMockUserPermissions({ + permissions: { + hasAdminPermission: false, + hasManagePermission: true, + hasEditClnSchedulesPermission: false, + hasEditOwnSchedulePermission: false, + servicePermissions: {}, + editableServiceCount: 0, + }, + }) + + const wrapper = createTestWrapper({ component: AuditLogPage, router }) + await waitForAsync() + + expect(wrapper.find(".no-access-card").exists()).toBeFalsy() + expect(wrapper.text()).toContain("Modified By") + expect(RotationService.getRotations).toHaveBeenCalledWith() + expect(AuditLogService.getModifiers).toHaveBeenCalledWith() + }) + + it("initializes the permissions store on mount when not yet loaded", async () => { + mockPermissionsStore.hasManagePermission = true + mockPermissionsStore.userPermissions = null + + createTestWrapper({ component: AuditLogPage, router }) + await waitForAsync() + + expect(mockPermissionsStore.initialize).toHaveBeenCalledWith() + }) +}) diff --git a/VueApp/src/ClinicalScheduler/__tests__/audit-log-results-table.test.ts b/VueApp/src/ClinicalScheduler/__tests__/audit-log-results-table.test.ts new file mode 100644 index 000000000..fcfb4aa82 --- /dev/null +++ b/VueApp/src/ClinicalScheduler/__tests__/audit-log-results-table.test.ts @@ -0,0 +1,66 @@ +import AuditLogResultsTable from "../components/AuditLogResultsTable.vue" +import StatusBadge from "@/components/StatusBadge.vue" +import { createTestWrapper } from "./test-utils" +import type { AuditLogEntry } from "../types/audit-types" + +function makeEntry(overrides: Partial = {}): AuditLogEntry { + return { + scheduleAuditId: 1, + area: "Clinicians", + mothraId: "abc123", + personName: "Dr. Example", + action: "Added to rotation", + rotationId: 5, + rotationName: "Cardiology", + weekId: 10, + weekNum: 3, + weekStart: "2026-01-05", + term: "Spring 2026", + modifiedBy: "mgr456", + modifiedByName: "Manager Person", + timeStamp: "2026-01-06T08:30:00", + ...overrides, + } +} + +function mountTable(entries: AuditLogEntry[], isLoading = false) { + return createTestWrapper({ + component: AuditLogResultsTable, + props: { entries, isLoading }, + }) +} + +describe("auditLogResultsTable", () => { + it("renders audit entries", () => { + expect.assertions(4) + + const wrapper = mountTable([makeEntry()]) + + expect(wrapper.text()).toContain("Dr. Example") + expect(wrapper.text()).toContain("Added to rotation") + expect(wrapper.text()).toContain("Cardiology") + expect(wrapper.text()).toContain("Manager Person") + }) + + it("renders one row per entry", () => { + expect.assertions(3) + + const wrapper = mountTable([ + makeEntry(), + makeEntry({ scheduleAuditId: 2, personName: "Dr. Second", action: "Removed from rotation" }), + ]) + + // One action badge renders per entry across every responsive table variant. + expect(wrapper.findAllComponents(StatusBadge)).toHaveLength(2) + expect(wrapper.text()).toContain("Dr. Second") + expect(wrapper.text()).toContain("Removed from rotation") + }) + + it("marks the table as loading", () => { + expect.assertions(1) + + const wrapper = mountTable([], true) + + expect(wrapper.findComponent({ name: "QTable" }).props("loading")).toBeTruthy() + }) +}) diff --git a/VueApp/src/ClinicalScheduler/__tests__/week-history-content.test.ts b/VueApp/src/ClinicalScheduler/__tests__/week-history-content.test.ts new file mode 100644 index 000000000..096c23ca0 --- /dev/null +++ b/VueApp/src/ClinicalScheduler/__tests__/week-history-content.test.ts @@ -0,0 +1,153 @@ +import WeekHistoryContent from "../components/WeekHistoryContent.vue" +import { createTestWrapper } from "./test-utils" +import { useDateFunctions } from "@/composables/DateFunctions" +import type { AuditLogEntry } from "../types/audit-types" + +function makeEntry(overrides: Partial = {}): AuditLogEntry { + return { + scheduleAuditId: 1, + area: "Clinicians", + mothraId: "abc123", + personName: "Dr. Example", + action: "Added to rotation", + rotationId: 5, + rotationName: "Cardiology", + weekId: 10, + weekNum: 3, + weekStart: "2026-01-05", + term: "Spring 2026", + modifiedBy: "mgr456", + modifiedByName: "Manager Person", + timeStamp: "2026-01-06T08:30:00", + ...overrides, + } +} + +function mountContent(props: Record = {}) { + return createTestWrapper({ + component: WeekHistoryContent, + props: { + titleId: "week-history-title-test", + viewMode: "rotation", + weekNumber: 3, + weekDateStart: "2026-01-05", + contextLabel: "Cardiology", + entries: [], + isLoading: false, + error: null, + ...props, + }, + }) +} + +describe("weekHistoryContent", () => { + it("shows skeleton rows while loading", () => { + expect.assertions(2) + + const wrapper = mountContent({ isLoading: true }) + + expect(wrapper.findAll(".week-history__skeleton-row")).toHaveLength(3) + expect(wrapper.attributes("aria-busy")).toBe("true") + }) + + it("shows the error state and emits retry", async () => { + expect.assertions(2) + + const wrapper = mountContent({ error: "Failed to load the audit trail" }) + + expect(wrapper.find("[role='alert']").text()).toContain("Failed to load the audit trail") + + await wrapper.find("[role='alert'] button").trigger("click") + + expect(wrapper.emitted("retry")).toHaveLength(1) + }) + + it("shows an empty state when there are no entries", () => { + expect.assertions(1) + + const wrapper = mountContent({ entries: [] }) + + expect(wrapper.text()).toContain("No audit entries for this week.") + }) + + it("lists entries with the clinician as subject in rotation view", () => { + expect.assertions(3) + + const wrapper = mountContent({ viewMode: "rotation", entries: [makeEntry()] }) + + expect(wrapper.find(".week-history__subject").text()).toBe("Dr. Example") + expect(wrapper.text()).toContain("Added to rotation") + expect(wrapper.text()).toContain("Manager Person") + }) + + it("lists entries with the rotation as subject in clinician view", () => { + expect.assertions(1) + + const wrapper = mountContent({ viewMode: "clinician", entries: [makeEntry()] }) + + expect(wrapper.find(".week-history__subject").text()).toBe("Cardiology") + }) + + it("labels the week navigation with the current week and date", () => { + expect.assertions(2) + + const { formatDate } = useDateFunctions() + const wrapper = mountContent({ weekNumber: 3, weekDateStart: "2026-01-05" }) + + const label = wrapper.find(".week-history__nav-label").text() + expect(label).toContain("Week 3") + expect(label).toContain(formatDate("2026-01-05")) + }) + + it("emits prev/next when the enabled nav controls are clicked", async () => { + expect.assertions(2) + + const wrapper = mountContent({ canPrev: true, canNext: true }) + + await wrapper.find("[aria-label='Previous week']").trigger("click") + await wrapper.find("[aria-label='Next week']").trigger("click") + + expect(wrapper.emitted("prev")).toHaveLength(1) + expect(wrapper.emitted("next")).toHaveLength(1) + }) + + it("disables nav controls at the schedule ends", () => { + expect.assertions(2) + + const wrapper = mountContent({ canPrev: false, canNext: false }) + + expect(wrapper.find("[aria-label='Previous week']").classes()).toContain("disabled") + expect(wrapper.find("[aria-label='Next week']").classes()).toContain("disabled") + }) + + it("shows skeleton rows only on the initial load (no entries yet)", () => { + expect.assertions(2) + + const wrapper = mountContent({ isLoading: true, entries: [] }) + + expect(wrapper.findAll(".week-history__skeleton-row")).toHaveLength(3) + expect(wrapper.find(".week-history__list").exists()).toBeFalsy() + }) + + it("keeps the current rows (dimmed) instead of a skeleton while paging weeks", () => { + expect.assertions(3) + + // Entries already present while loading = paging to another week + const wrapper = mountContent({ isLoading: true, entries: [makeEntry()] }) + + expect(wrapper.findAll(".week-history__skeleton-row")).toHaveLength(0) + const list = wrapper.find(".week-history__list") + expect(list.exists()).toBeTruthy() + expect(list.classes()).toContain("week-history__list--loading") + }) + + it("shows the loading progress bar only while a fetch is in flight", () => { + expect.assertions(2) + + const loading = mountContent({ isLoading: true, entries: [makeEntry()] }) + const idle = mountContent({ isLoading: false, entries: [makeEntry()] }) + + expect(loading.find(".week-history__progress").exists()).toBeTruthy() + expect(idle.find(".week-history__progress").exists()).toBeFalsy() + }) +}) diff --git a/VueApp/src/ClinicalScheduler/components/AuditLogResultsTable.vue b/VueApp/src/ClinicalScheduler/components/AuditLogResultsTable.vue new file mode 100644 index 000000000..54e1c6409 --- /dev/null +++ b/VueApp/src/ClinicalScheduler/components/AuditLogResultsTable.vue @@ -0,0 +1,227 @@ + + + diff --git a/VueApp/src/ClinicalScheduler/components/ScheduleLegend.vue b/VueApp/src/ClinicalScheduler/components/ScheduleLegend.vue index 5ca82545a..0756daee6 100644 --- a/VueApp/src/ClinicalScheduler/components/ScheduleLegend.vue +++ b/VueApp/src/ClinicalScheduler/components/ScheduleLegend.vue @@ -97,6 +97,20 @@ /> Needs primary evaluator + +
+
@@ -110,12 +124,14 @@ import { useQuasar } from "quasar" interface Props { showWarning?: boolean showBulkGuide?: boolean + showHistory?: boolean itemType?: "rotation" | "clinician" } withDefaults(defineProps(), { showWarning: false, showBulkGuide: false, + showHistory: false, itemType: "rotation", }) diff --git a/VueApp/src/ClinicalScheduler/components/ScheduleView.vue b/VueApp/src/ClinicalScheduler/components/ScheduleView.vue index 849c7e7f9..65fcd462b 100644 --- a/VueApp/src/ClinicalScheduler/components/ScheduleView.vue +++ b/VueApp/src/ClinicalScheduler/components/ScheduleView.vue @@ -41,7 +41,7 @@ class="q-mb-md" > -
+
@@ -69,8 +71,20 @@ v-if="showLegend && schedulesBySemester && schedulesBySemester.length > 0" :show-warning="showWarningInLegend" :show-bulk-guide="enableWeekSelection" + :show-history="canViewHistory" :item-type="viewMode === 'rotation' ? 'clinician' : 'rotation'" /> + + +
@@ -78,6 +92,7 @@ import { computed, ref } from "vue" import { useKeyModifier, useEventListener } from "@vueuse/core" import WeekCell from "./WeekCell.vue" +import WeekHistoryDialog from "./WeekHistoryDialog.vue" import ScheduleLegend from "./ScheduleLegend.vue" import StatusBanner from "@/components/StatusBanner.vue" import type { WeekItem, ScheduleAssignment, ScheduleSemester, ViewMode } from "./schedule-view-types" @@ -110,6 +125,11 @@ interface Props { // Selection mode enableWeekSelection?: boolean + // Inline per-week history (manager-only) + canViewHistory?: boolean + historyContextId?: number | string | null + historyContextLabel?: string + // Custom messages noDataMessage?: string emptyStateMessage?: string @@ -141,6 +161,9 @@ const props = withDefaults(defineProps(), { showPrimaryToggle: true, requiresPrimaryForWeek: false, enableWeekSelection: false, + canViewHistory: false, + historyContextId: null, + historyContextLabel: "", noDataMessage: "No schedule data available", emptyStateMessage: "Click to add assignment", readOnlyEmptyMessage: "No assignments", @@ -179,6 +202,22 @@ const allWeeks = computed(() => { return weeks }) +// Gate once here (context is schedule-level), then feed the flat week list to one shared dialog. +const canShowHistory = computed( + () => + props.canViewHistory && + props.historyContextId !== null && + props.historyContextId !== undefined && + props.historyContextId !== "", +) +const historyOpen = ref(false) +const historyStartWeekId = ref(null) + +function openWeekHistory(week: WeekItem) { + historyStartWeekId.value = week.weekId + historyOpen.value = true +} + // Check if a week is selected function isWeekSelected(weekId: number): boolean { return selectedWeekIds.value.has(weekId) @@ -350,4 +389,12 @@ defineExpose({ .cursor-pointer { cursor: pointer; } + +/* Mobile single column: q-gutter-md adds only a left margin, which left-pins the card */ +@media (max-width: 599.98px) { + .schedule-week-grid, + .schedule-week-grid > * { + margin-left: 0; + } +} diff --git a/VueApp/src/ClinicalScheduler/components/SchedulerNavigation.vue b/VueApp/src/ClinicalScheduler/components/SchedulerNavigation.vue index 1e03023d1..aca8155b6 100644 --- a/VueApp/src/ClinicalScheduler/components/SchedulerNavigation.vue +++ b/VueApp/src/ClinicalScheduler/components/SchedulerNavigation.vue @@ -35,6 +35,14 @@ :id="`clinician-tab`" role="tab" /> + diff --git a/VueApp/src/ClinicalScheduler/components/WeekCell.vue b/VueApp/src/ClinicalScheduler/components/WeekCell.vue index e20190f61..5f8ca424c 100644 --- a/VueApp/src/ClinicalScheduler/components/WeekCell.vue +++ b/VueApp/src/ClinicalScheduler/components/WeekCell.vue @@ -28,6 +28,11 @@ title="Primary evaluator required for this week" /> Week {{ week.weekNumber }} ({{ formatDate(week.dateStart) }}) + @@ -162,6 +167,7 @@ import { useTimeoutFn } from "@vueuse/core" import { inflect } from "inflection" import { useDateFunctions } from "@/composables/DateFunctions" import { ANIMATIONS } from "../constants/app-constants" +import WeekHistoryButton from "./WeekHistoryButton.vue" const { formatDate } = useDateFunctions() @@ -201,6 +207,8 @@ interface Props { isLoading?: boolean selectable?: boolean selected?: boolean + // Manager-only audit-trail trigger; ScheduleView owns the dialog and its context. + canViewHistory?: boolean } const props = withDefaults(defineProps(), { @@ -213,6 +221,7 @@ const props = withDefaults(defineProps(), { isLoading: false, selectable: false, selected: false, + canViewHistory: false, }) const emit = defineEmits<{ @@ -220,6 +229,7 @@ const emit = defineEmits<{ "remove-assignment": [assignmentId: number, displayName: string, isPrimary?: boolean] "toggle-primary": [assignmentId: number, isPrimary: boolean, displayName: string] "shift-click": [week: Props["week"]] + "view-history": [week: Props["week"]] }>() // Computed properties @@ -269,7 +279,20 @@ const { } = useTimeoutFn(() => emit("click", props.week), 500, { immediate: false }) // Methods + +// Touch taps don't bubble through a child control's @click.stop, so ignore any +// tap/click that starts on an in-cell control (it must not also schedule the week). +function originatesOnControl(event?: Event): boolean { + if (!event) return false + // composedPath covers text-node targets (some touch taps) that event.target misses + return event + .composedPath() + .some((node) => node instanceof Element && node.matches("button, [role='button'], a, input, select, textarea")) +} + function handleClick(event?: MouseEvent) { + if (originatesOnControl(event)) return + // Don't handle click during selection mode if just selecting if (props.selectable && !props.canEdit) { if (event?.shiftKey) { @@ -296,7 +319,8 @@ function handleClick(event?: MouseEvent) { } // Touch event handlers for mobile long-press -function handleTouchStart() { +function handleTouchStart(event: TouchEvent) { + if (originatesOnControl(event)) return if (props.selectable && !props.isPastYear) { startLongPress() } @@ -360,6 +384,14 @@ const cardClasses = computed(() => { min-width: 200px; } +/* Mobile single column: fill the width instead of capping at 280px */ +@media (width <= 599.98px) { + .week-schedule-card { + max-width: none; + min-width: 0; + } +} + .week-schedule-card .q-card { height: 100%; } diff --git a/VueApp/src/ClinicalScheduler/components/WeekHistoryBody.vue b/VueApp/src/ClinicalScheduler/components/WeekHistoryBody.vue new file mode 100644 index 000000000..3fe54b346 --- /dev/null +++ b/VueApp/src/ClinicalScheduler/components/WeekHistoryBody.vue @@ -0,0 +1,174 @@ + + + + + diff --git a/VueApp/src/ClinicalScheduler/components/WeekHistoryButton.vue b/VueApp/src/ClinicalScheduler/components/WeekHistoryButton.vue new file mode 100644 index 000000000..66797f699 --- /dev/null +++ b/VueApp/src/ClinicalScheduler/components/WeekHistoryButton.vue @@ -0,0 +1,45 @@ + + + + + diff --git a/VueApp/src/ClinicalScheduler/components/WeekHistoryContent.vue b/VueApp/src/ClinicalScheduler/components/WeekHistoryContent.vue new file mode 100644 index 000000000..a7ce03ea7 --- /dev/null +++ b/VueApp/src/ClinicalScheduler/components/WeekHistoryContent.vue @@ -0,0 +1,340 @@ + + + + + diff --git a/VueApp/src/ClinicalScheduler/components/WeekHistoryDialog.vue b/VueApp/src/ClinicalScheduler/components/WeekHistoryDialog.vue new file mode 100644 index 000000000..7ed19d63c --- /dev/null +++ b/VueApp/src/ClinicalScheduler/components/WeekHistoryDialog.vue @@ -0,0 +1,123 @@ + + + + + diff --git a/VueApp/src/ClinicalScheduler/composables/use-audit-entries.ts b/VueApp/src/ClinicalScheduler/composables/use-audit-entries.ts new file mode 100644 index 000000000..753aec6f2 --- /dev/null +++ b/VueApp/src/ClinicalScheduler/composables/use-audit-entries.ts @@ -0,0 +1,70 @@ +import { ref } from "vue" +import type { Ref } from "vue" +import type { ApiResult } from "../types/api" +import type { AuditLogEntry } from "../types/audit-types" + +const LOAD_FAILED = "Failed to load the audit trail" +const LOAD_ERROR = "An error occurred while loading the audit trail" + +export interface UseAuditEntries { + entries: Ref + isLoading: Ref + error: Ref + load: (request: () => Promise>) => Promise + /** Clear entries/error so the next load shows a clean loading state, not stale content */ + reset: () => void +} + +/** + * Shared loader for audit-trail surfaces (the full page and the inline per-week + * popover): owns the entries/loading/error state and applies one consistent + * success/failure handling, so each caller stays a thin wrapper around its request. + */ +export function useAuditEntries(): UseAuditEntries { + const entries = ref([]) + const isLoading = ref(false) + const error = ref(null) + let latestRequestId = 0 + + async function load(request: () => Promise>): Promise { + // Overlapping loads (debounced filter edits, retry) can resolve out of order; + // only the latest request may commit state, or stale results win the race. + latestRequestId += 1 + const requestId = latestRequestId + isLoading.value = true + error.value = null + try { + const response = await request() + if (requestId !== latestRequestId) { + return + } + if (response.success) { + entries.value = response.result + } else { + entries.value = [] + error.value = response.errors?.join(", ") || LOAD_FAILED + } + } catch (err) { + if (requestId !== latestRequestId) { + return + } + entries.value = [] + error.value = err instanceof Error ? err.message : LOAD_ERROR + } finally { + if (requestId === latestRequestId) { + isLoading.value = false + } + } + } + + // Drop the current result (and invalidate any in-flight load) so a reopened + // surface shows its loading state, not stale rows. + function reset(): void { + latestRequestId += 1 + entries.value = [] + error.value = null + isLoading.value = false + } + + return { entries, isLoading, error, load, reset } +} diff --git a/VueApp/src/ClinicalScheduler/constants/permission-messages.ts b/VueApp/src/ClinicalScheduler/constants/permission-messages.ts index 6ab4b589b..71ee5dcc2 100644 --- a/VueApp/src/ClinicalScheduler/constants/permission-messages.ts +++ b/VueApp/src/ClinicalScheduler/constants/permission-messages.ts @@ -8,6 +8,7 @@ const ACCESS_DENIED_MESSAGES = { ROTATION_VIEW: "You do not have permission to access the Schedule by Rotation view.", CLINICIAN_VIEW: "You do not have permission to access the Schedule by Clinician view.", UNAUTHORIZED_ROTATION: "You do not have permission to view this rotation.", + AUDIT_LOG: "You do not have permission to view the Clinical Scheduler audit trail.", } as const // Subtitle messages for access denied components @@ -18,6 +19,7 @@ const ACCESS_DENIED_SUBTITLES = { CLINICIAN_VIEW: "This feature is not available with rotation-specific permissions. Contact your administrator if you need full access to scheduling features.", UNAUTHORIZED_ROTATION: "You can only access rotations that you have been granted permission to edit.", + AUDIT_LOG: "The audit trail is available to schedule managers. Contact your administrator if you need access.", } as const // Error messages for schedule operations diff --git a/VueApp/src/ClinicalScheduler/pages/AuditLogPage.vue b/VueApp/src/ClinicalScheduler/pages/AuditLogPage.vue new file mode 100644 index 000000000..5e5fc8b6d --- /dev/null +++ b/VueApp/src/ClinicalScheduler/pages/AuditLogPage.vue @@ -0,0 +1,408 @@ + + + diff --git a/VueApp/src/ClinicalScheduler/pages/ClinicianScheduleView.vue b/VueApp/src/ClinicalScheduler/pages/ClinicianScheduleView.vue index 5f2f0dc20..b25e2e223 100644 --- a/VueApp/src/ClinicalScheduler/pages/ClinicianScheduleView.vue +++ b/VueApp/src/ClinicalScheduler/pages/ClinicianScheduleView.vue @@ -140,6 +140,9 @@ :is-loading="loadingSchedule" :error="scheduleError" :can-edit="!isPastYear" + :can-view-history="permissionsStore.hasManagePermission" + :history-context-id="selectedClinician?.mothraId ?? null" + :history-context-label="selectedClinician?.fullName ?? ''" :show-legend="true" :show-warning-icon="false" :show-primary-toggle="true" diff --git a/VueApp/src/ClinicalScheduler/pages/RotationScheduleView.vue b/VueApp/src/ClinicalScheduler/pages/RotationScheduleView.vue index d29934a5a..b4990f64a 100644 --- a/VueApp/src/ClinicalScheduler/pages/RotationScheduleView.vue +++ b/VueApp/src/ClinicalScheduler/pages/RotationScheduleView.vue @@ -170,6 +170,9 @@ :is-loading="isLoadingSchedule" :error="scheduleError" :can-edit="canEditRotation" + :can-view-history="permissionsStore.hasManagePermission" + :history-context-id="selectedRotation.rotId" + :history-context-label="selectedRotation.name" :show-legend="true" :show-warning-in-legend="true" :show-warning-icon="true" diff --git a/VueApp/src/ClinicalScheduler/router/routes.ts b/VueApp/src/ClinicalScheduler/router/routes.ts index 9a25d957a..e165cc837 100644 --- a/VueApp/src/ClinicalScheduler/router/routes.ts +++ b/VueApp/src/ClinicalScheduler/router/routes.ts @@ -36,6 +36,12 @@ const routes = [ component: () => import("@/ClinicalScheduler/pages/ClinicianScheduleView.vue"), name: "ClinicianScheduleWithId", }, + { + path: "/ClinicalScheduler/audit", + meta: { layout: ViperLayout }, + component: () => import("@/ClinicalScheduler/pages/AuditLogPage.vue"), + name: "AuditLog", + }, ] export { routes as clinicalSchedulerRoutes } diff --git a/VueApp/src/ClinicalScheduler/services/audit-log-service.ts b/VueApp/src/ClinicalScheduler/services/audit-log-service.ts new file mode 100644 index 000000000..0a2b8df32 --- /dev/null +++ b/VueApp/src/ClinicalScheduler/services/audit-log-service.ts @@ -0,0 +1,132 @@ +import { useFetch } from "../../composables/ViperFetch" +import type { ApiResult } from "../types/api" +import type { AuditLogEntry, AuditModifier, AuditTerm } from "../types/audit-types" + +interface AuditLogFilters { + year?: number | null + rotationId?: number | null + termCode?: number | null + person?: string | null + modifiedBy?: string | null + area?: string | null + from?: string | null + to?: string | null +} + +class AuditLogService { + private static readonly BASE_URL = `${import.meta.env.VITE_API_URL}clinicalscheduler/audit` + + private static buildUrl(baseUrl: string, params: Record): string { + const search = new URLSearchParams() + for (const [key, value] of Object.entries(params)) { + if (value !== undefined && value !== null && `${value}`.length > 0) { + search.set(key, value.toString()) + } + } + const queryString = search.toString() + return queryString ? `${baseUrl}?${queryString}` : baseUrl + } + + /** + * Get the filtered audit log. An empty/omitted year defaults to the current grad year server-side. + */ + static async getAuditLog(filters: AuditLogFilters = {}): Promise> { + try { + const url = AuditLogService.buildUrl(AuditLogService.BASE_URL, { ...filters }) + const { get } = useFetch() + return await get(url) + } catch (error) { + return { + result: [], + success: false, + errors: [error instanceof Error ? error.message : "Unknown error occurred"], + } + } + } + + /** + * Get the distinct users who have made audited changes, for the "Modified By" filter. + */ + static async getModifiers(): Promise> { + try { + const { get } = useFetch() + return await get(`${AuditLogService.BASE_URL}/modifiers`) + } catch (error) { + return { + result: [], + success: false, + errors: [error instanceof Error ? error.message : "Unknown error occurred"], + } + } + } + + /** + * Get the terms (semesters) within a grad year, for the "Term" filter. + */ + static async getTerms(year?: number): Promise> { + try { + const url = AuditLogService.buildUrl(`${AuditLogService.BASE_URL}/terms`, { year }) + const { get } = useFetch() + return await get(url) + } catch (error) { + return { + result: [], + success: false, + errors: [error instanceof Error ? error.message : "Unknown error occurred"], + } + } + } + + /** + * Get the distinct affected students/clinicians in the audit trail, for the "Person" filter. + */ + static async getPersons(): Promise> { + try { + const { get } = useFetch() + return await get(`${AuditLogService.BASE_URL}/persons`) + } catch (error) { + return { + result: [], + success: false, + errors: [error instanceof Error ? error.message : "Unknown error occurred"], + } + } + } + + /** + * Get the audit trail for a single rotation + week (Schedule-by-Rotation inline popover). + */ + static async getRotationWeekHistory(rotationId: number, weekId: number): Promise> { + try { + const url = AuditLogService.buildUrl(`${AuditLogService.BASE_URL}/rotation-week`, { rotationId, weekId }) + const { get } = useFetch() + return await get(url) + } catch (error) { + return { + result: [], + success: false, + errors: [error instanceof Error ? error.message : "Unknown error occurred"], + } + } + } + + /** + * Get the audit trail for a single clinician + week (Schedule-by-Clinician inline popover). + */ + static async getClinicianWeekHistory(mothraId: string, weekId: number): Promise> { + try { + const url = AuditLogService.buildUrl(`${AuditLogService.BASE_URL}/clinician-week`, { mothraId, weekId }) + const { get } = useFetch() + return await get(url) + } catch (error) { + return { + result: [], + success: false, + errors: [error instanceof Error ? error.message : "Unknown error occurred"], + } + } + } +} + +export { AuditLogService } +export type { AuditLogFilters } diff --git a/VueApp/src/ClinicalScheduler/types/audit-types.ts b/VueApp/src/ClinicalScheduler/types/audit-types.ts new file mode 100644 index 000000000..b7c6be6d5 --- /dev/null +++ b/VueApp/src/ClinicalScheduler/types/audit-types.ts @@ -0,0 +1,35 @@ +/** + * Types for the schedule-change audit log. + */ + +interface AuditLogEntry { + scheduleAuditId: number + area: string + mothraId: string | null + personName: string + action: string + rotationId: number | null + rotationName: string + weekId: number | null + weekNum: number + weekStart: string | null + term: string + modifiedBy: string + modifiedByName: string + timeStamp: string +} + +// A selectable person option in the audit trail filters — reused for both the +// "Modified By" (change author) and "Person" (affected student/clinician) dropdowns. +interface AuditModifier { + mothraId: string + displayName: string +} + +// A selectable term option for the audit trail "Term" filter, scoped to a grad year. +interface AuditTerm { + termCode: number + term: string +} + +export type { AuditLogEntry, AuditModifier, AuditTerm } diff --git a/VueApp/src/ClinicalScheduler/utils/audit-actions.ts b/VueApp/src/ClinicalScheduler/utils/audit-actions.ts new file mode 100644 index 000000000..fff72b4d7 --- /dev/null +++ b/VueApp/src/ClinicalScheduler/utils/audit-actions.ts @@ -0,0 +1,17 @@ +/** + * Shared mapping of schedule-audit actions to semantic Quasar colors, used by both the + * standalone Audit Trail page and the inline per-week history popover so the colored + * action badges stay consistent across the feature. + */ +const ACTION_COLORS: Record = { + "Added to rotation": "positive", + "Removed from rotation": "negative", + "Made primary evaluator": "primary", + "Primary evaluator flag removed": "warning", +} + +function getAuditActionColor(action: string): string { + return ACTION_COLORS[action] ?? "grey-8" +} + +export { getAuditActionColor } diff --git a/VueApp/src/Effort/__tests__/audit-change-detail.test.ts b/VueApp/src/Effort/__tests__/audit-change-detail.test.ts new file mode 100644 index 000000000..2190f5da5 --- /dev/null +++ b/VueApp/src/Effort/__tests__/audit-change-detail.test.ts @@ -0,0 +1,49 @@ +import { mount } from "@vue/test-utils" +import AuditChangeDetail from "../components/AuditChangeDetail.vue" +import type { ChangeDetail } from "../types" + +function mountDetail(name: string, detail: ChangeDetail) { + return mount(AuditChangeDetail, { props: { name, detail } }) +} + +describe("auditChangeDetail", () => { + it("shows a reference value once without diff styling", () => { + expect.assertions(3) + + const wrapper = mountDetail("Course", { oldValue: "VET 410", newValue: "VET 410" }) + + expect(wrapper.text()).toContain("Course:") + expect(wrapper.text()).toContain("VET 410") + expect(wrapper.find(".text-negative").exists()).toBeFalsy() + }) + + it("shows old and new values with diff styling when changed", () => { + expect.assertions(3) + + const wrapper = mountDetail("Hours", { oldValue: "10", newValue: "12" }) + + expect(wrapper.find(".text-negative").text()).toBe("10") + expect(wrapper.find(".text-positive").text()).toBe("12") + expect(wrapper.text()).toContain("→") + }) + + it("omits the old value and arrow when the value was added", () => { + expect.assertions(3) + + const wrapper = mountDetail("Role", { oldValue: null, newValue: "Instructor" }) + + expect(wrapper.find(".text-negative").exists()).toBeFalsy() + expect(wrapper.find(".text-positive").text()).toBe("Instructor") + expect(wrapper.text()).not.toContain("→") + }) + + it("omits the new value and arrow when the value was removed", () => { + expect.assertions(3) + + const wrapper = mountDetail("Role", { oldValue: "Instructor", newValue: null }) + + expect(wrapper.find(".text-negative").text()).toBe("Instructor") + expect(wrapper.find(".text-positive").exists()).toBeFalsy() + expect(wrapper.text()).not.toContain("→") + }) +}) diff --git a/VueApp/src/Effort/components/AuditChangeDetail.vue b/VueApp/src/Effort/components/AuditChangeDetail.vue new file mode 100644 index 000000000..98366020b --- /dev/null +++ b/VueApp/src/Effort/components/AuditChangeDetail.vue @@ -0,0 +1,30 @@ + + + diff --git a/VueApp/src/Effort/pages/AuditList.vue b/VueApp/src/Effort/pages/AuditList.vue index 7574edd91..051b8ac10 100644 --- a/VueApp/src/Effort/pages/AuditList.vue +++ b/VueApp/src/Effort/pages/AuditList.vue @@ -21,6 +21,7 @@ option-label="termName" option-value="termCode" label="Term" + :display-value="filter.termCode == null ? 'All terms' : undefined" dense options-dense outlined @@ -35,6 +36,7 @@ v-model="filter.action" :options="actions" label="Action" + :display-value="filter.action == null ? 'All actions' : undefined" dense options-dense outlined @@ -48,6 +50,8 @@ option-label="fullName" option-value="personId" label="Modified By" + stack-label + placeholder="All users" dense options-dense outlined @@ -66,6 +70,8 @@ option-label="fullName" option-value="personId" label="Instructor" + stack-label + placeholder="All instructors" dense options-dense outlined @@ -77,15 +83,6 @@ @filter="filterInstructors" /> -
- -
+ +
+ + + +
+ - {{ key }}: - - - - +
- {{ key }}: - - - - +
entry.MothraId == "test456"); } + [Fact] + public async Task GetRotationWeekAuditAsync_ExcludesStudentAreaEntries() + { + // The shared audit table also records student schedule changes for the + // same rotation/week; the clinician-page popover must not show them. + var clinicianEntry = CreateAuditEntry("test123", 1, 1, "modifier", ScheduleAuditActions.InstructorAdded); + var studentEntry = CreateAuditEntry("student1", 1, 1, "modifier", ScheduleAuditActions.InstructorAdded, ScheduleAuditAreas.Students); + await _context.ScheduleAudits.AddRangeAsync(clinicianEntry, studentEntry); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + var result = await _service.GetRotationWeekAuditAsync(1, 1, TestContext.Current.CancellationToken); + + var entry = Assert.Single(result); + Assert.Equal("test123", entry.MothraId); + Assert.Equal(ScheduleAuditAreas.Clinicians, entry.Area); + } + + [Fact] + public async Task GetClinicianWeekAuditAsync_ExcludesStudentAreaEntries() + { + // A clinician who was previously a UCD student must not see their + // student-era entries mixed into their clinician week history. + var clinicianEntry = CreateAuditEntry("test123", 1, 1, "modifier", ScheduleAuditActions.InstructorAdded); + var studentEntry = CreateAuditEntry("test123", 2, 1, "modifier", ScheduleAuditActions.InstructorAdded, ScheduleAuditAreas.Students); + await _context.ScheduleAudits.AddRangeAsync(clinicianEntry, studentEntry); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + var result = await _service.GetClinicianWeekAuditAsync("test123", 1, TestContext.Current.CancellationToken); + + var entry = Assert.Single(result); + Assert.Equal(1, entry.RotationId); + Assert.Equal(ScheduleAuditAreas.Clinicians, entry.Area); + } + [Fact] public async Task GetInstructorScheduleAuditHistoryAsync_EmptyResult_ReturnsEmptyList() { @@ -202,7 +238,7 @@ public async Task GetRotationWeekAuditHistoryAsync_EmptyResult_ReturnsEmptyList( Assert.Empty(result); } - private ScheduleAudit CreateAuditEntry(string mothraId, int rotationId, int weekId, string modifiedBy, string action) + private ScheduleAudit CreateAuditEntry(string mothraId, int rotationId, int weekId, string modifiedBy, string action, string area = ScheduleAuditAreas.Clinicians) { return new ScheduleAudit { @@ -212,7 +248,7 @@ private ScheduleAudit CreateAuditEntry(string mothraId, int rotationId, int week Action = action, ModifiedBy = modifiedBy, TimeStamp = DateTime.UtcNow, - Area = "Clinicians" + Area = area }; } diff --git a/test/ClinicalScheduler/ScheduleEditServiceRollbackTest.cs b/test/ClinicalScheduler/ScheduleEditServiceRollbackTest.cs new file mode 100644 index 000000000..7d08884f6 --- /dev/null +++ b/test/ClinicalScheduler/ScheduleEditServiceRollbackTest.cs @@ -0,0 +1,149 @@ +using Microsoft.Data.Sqlite; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using NSubstitute; +using NSubstitute.ExceptionExtensions; +using Viper.Areas.ClinicalScheduler.Services; +using Viper.Classes.SQLContext; +using Viper.EmailTemplates.Services; +using Viper.Services; + +namespace Viper.test.ClinicalScheduler +{ + /// + /// Runs the real ScheduleEditService (with its production transaction handling) + /// against SQLite, which supports transactions, unlike the in-memory provider + /// used by ScheduleEditServiceTest via TestableScheduleEditService. These tests + /// prove a failed audit write rolls the schedule change back, not just that the + /// exception surfaces. + /// + public class ScheduleEditServiceRollbackTest : IDisposable + { + private readonly SqliteConnection _connection; + private readonly ClinicalSchedulerContext _context; + private readonly IScheduleAuditService _mockAuditService; + private readonly ScheduleEditService _service; + private bool _disposed; + + public ScheduleEditServiceRollbackTest() + { + _connection = new SqliteConnection("Filename=:memory:"); + _connection.Open(); + var options = new DbContextOptionsBuilder() + .UseSqlite(_connection) + .Options; + _context = new ClinicalSchedulerContext(options); + _context.Database.EnsureCreated(); + + var gradYear = DateTime.Now.Year + 1; + _context.Services.Add(TestDataBuilder.CreateService(1, "Cardiology")); + _context.Rotations.Add(TestDataBuilder.CreateRotation(1, "Cardiology Rotation", 1)); + var (week, weekGradYear) = TestDataBuilder.CreateWeekScenario(1, gradYear); + _context.Weeks.Add(week); + _context.WeekGradYears.Add(weekGradYear); + _context.Persons.Add(TestDataBuilder.CreatePerson("test123")); + _context.SaveChanges(); + + _mockAuditService = Substitute.For(); + + var permissionValidator = Substitute.For(); + permissionValidator.ValidateEditPermissionAndGetUserAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(TestDataBuilder.CreateUser("currentuser")); + + var gradYearService = Substitute.For(); + gradYearService.GetCurrentGradYearAsync().Returns(DateTime.Now.Year); + + var emailNotificationOptions = Substitute.For>(); + emailNotificationOptions.Value.Returns(new EmailNotificationSettings()); + var emailSettingsOptions = Substitute.For>(); + emailSettingsOptions.Value.Returns(new EmailSettings()); + + _service = new ScheduleEditService( + _context, + _mockAuditService, + Substitute.For>(), + Substitute.For(), + emailNotificationOptions, + emailSettingsOptions, + gradYearService, + permissionValidator, + Substitute.For()); + } + + [Fact] + public async Task AddInstructorAsync_AuditWriteFails_RollsBackScheduleInsert() + { + _mockAuditService.LogInstructorAddedAsync(Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any()) + .ThrowsAsync(new InvalidOperationException("Failed to create audit entry")); + + await Assert.ThrowsAsync(() => + _service.AddInstructorAsync("test123", 1, new[] { 1 }, DateTime.Now.Year + 1, false, TestContext.Current.CancellationToken)); + + // The insert committed nothing: the schedule row must not survive the rollback + Assert.Empty(await _context.InstructorSchedules.AsNoTracking().ToListAsync(TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task RemoveInstructorScheduleAsync_AuditWriteFails_RollsBackDelete() + { + var schedule = TestDataBuilder.CreateInstructorSchedule("test123", 1, 1); + _context.InstructorSchedules.Add(schedule); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + _mockAuditService.LogInstructorRemovedAsync(Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any()) + .ThrowsAsync(new InvalidOperationException("Failed to create audit entry")); + + await Assert.ThrowsAsync(() => + _service.RemoveInstructorScheduleAsync(schedule.InstructorScheduleId, TestContext.Current.CancellationToken)); + + // The delete rolled back: no unaudited removals + Assert.NotNull(await _context.InstructorSchedules.AsNoTracking() + .FirstOrDefaultAsync(s => s.InstructorScheduleId == schedule.InstructorScheduleId, TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task SetPrimaryEvaluatorAsync_AuditWriteFails_RollsBackEvaluatorFlag() + { + var schedule = TestDataBuilder.CreateInstructorSchedule("test123", 1, 1); + _context.InstructorSchedules.Add(schedule); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + _mockAuditService.LogPrimaryEvaluatorSetAsync(Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any()) + .ThrowsAsync(new InvalidOperationException("Failed to create audit entry")); + + await Assert.ThrowsAsync(() => + _service.SetPrimaryEvaluatorAsync(schedule.InstructorScheduleId, true, TestContext.Current.CancellationToken)); + + // The flag update rolled back + var reloaded = await _context.InstructorSchedules.AsNoTracking() + .FirstAsync(s => s.InstructorScheduleId == schedule.InstructorScheduleId, TestContext.Current.CancellationToken); + Assert.False(reloaded.Evaluator); + } + + protected virtual void Dispose(bool disposing) + { + if (_disposed) + { + return; + } + + if (disposing) + { + _context.Dispose(); + _connection.Dispose(); + } + + _disposed = true; + } + + public void Dispose() + { + Dispose(disposing: true); + GC.SuppressFinalize(this); + } + } +} diff --git a/test/ClinicalScheduler/ScheduleEditServiceTest.cs b/test/ClinicalScheduler/ScheduleEditServiceTest.cs index 06f14baa3..999376d16 100644 --- a/test/ClinicalScheduler/ScheduleEditServiceTest.cs +++ b/test/ClinicalScheduler/ScheduleEditServiceTest.cs @@ -206,6 +206,69 @@ await _mockAuditService.Received(2).LogInstructorAddedAsync(mothraId, rotationId user.MothraId, Arg.Any()); } + [Fact] + public async Task AddInstructorAsync_AuditWriteFails_ThrowsInsteadOfSwallowing() + { + // Audit writes run inside the schedule transaction, so a failed audit + // must fail the whole operation (rolled back in production) rather + // than being silently swallowed. + var user = TestDataBuilder.CreateUser("currentuser"); + _mockPermissionValidator.ValidateEditPermissionAndGetUserAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(user); + _mockAuditService.LogInstructorAddedAsync(Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any()) + .ThrowsAsync(new InvalidOperationException("Failed to create audit entry")); + + var ex = await Assert.ThrowsAsync(() => + _service.AddInstructorAsync("test123", 1, new[] { 1 }, DateTime.Now.Year + 1, false, TestContext.Current.CancellationToken)); + + Assert.Contains("audit", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public async Task RemoveInstructorScheduleAsync_AuditWriteFails_ThrowsInsteadOfSwallowing() + { + // Same fail-closed guarantee for removals: no unaudited deletes. + var schedule = TestDataBuilder.CreateInstructorSchedule("test123", 1, 1); + await _context.InstructorSchedules.AddAsync(schedule, TestContext.Current.CancellationToken); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + var user = TestDataBuilder.CreateUser("currentuser"); + _mockPermissionValidator.ValidateEditPermissionAndGetUserAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(user); + _mockAuditService.LogInstructorRemovedAsync(Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any()) + .ThrowsAsync(new InvalidOperationException("Failed to create audit entry")); + + var ex = await Assert.ThrowsAsync(() => + _service.RemoveInstructorScheduleAsync(schedule.InstructorScheduleId, TestContext.Current.CancellationToken)); + + Assert.Contains("audit", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public async Task SetPrimaryEvaluatorAsync_AuditWriteFails_ThrowsInsteadOfSwallowing() + { + // Same fail-closed guarantee for primary evaluator changes. This method wraps + // failures in its generic operation-failed message, so the audit cause is + // asserted on the inner exception. + var schedule = TestDataBuilder.CreateInstructorSchedule("test123", 1, 1); + await _context.InstructorSchedules.AddAsync(schedule, TestContext.Current.CancellationToken); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + var user = TestDataBuilder.CreateUser("currentuser"); + _mockPermissionValidator.ValidateEditPermissionAndGetUserAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(user); + _mockAuditService.LogPrimaryEvaluatorSetAsync(Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any()) + .ThrowsAsync(new InvalidOperationException("Failed to create audit entry")); + + var ex = await Assert.ThrowsAsync(() => + _service.SetPrimaryEvaluatorAsync(schedule.InstructorScheduleId, true, TestContext.Current.CancellationToken)); + + Assert.Contains("audit", ex.InnerException!.Message, StringComparison.OrdinalIgnoreCase); + } + [Fact] public async Task AddInstructorAsync_WithPrimaryEvaluator_ClearsPreviousPrimaryAndSetsNew() { @@ -218,8 +281,8 @@ public async Task AddInstructorAsync_WithPrimaryEvaluator_ClearsPreviousPrimaryA // Create existing primary evaluator var existingPrimary = TestDataBuilder.CreateInstructorSchedule("existing456", rotationId, weekId, true); - await _context.InstructorSchedules.AddAsync(existingPrimary); - await _context.SaveChangesAsync(); + await _context.InstructorSchedules.AddAsync(existingPrimary, TestContext.Current.CancellationToken); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); // Override default permission validator to return the specific user for this test _mockPermissionValidator.ValidateEditPermissionAndGetUserAsync(Arg.Any(), Arg.Any(), Arg.Any()) @@ -244,7 +307,7 @@ public async Task AddInstructorAsync_WithPrimaryEvaluator_ClearsPreviousPrimaryA Assert.True(newSchedule.Evaluator); // Check that existing primary evaluator was cleared - var updatedExisting = await _context.InstructorSchedules.FindAsync(existingPrimary.InstructorScheduleId); + var updatedExisting = await _context.InstructorSchedules.FindAsync(new object?[] { existingPrimary.InstructorScheduleId }, TestContext.Current.CancellationToken); Assert.False(updatedExisting!.Evaluator); // Verify audit logging @@ -349,8 +412,8 @@ public async Task RemoveInstructorScheduleAsync_WithoutPermissionForOwnSchedule_ { // Arrange - User tries to remove their own schedule but lacks EditOwnSchedule permission var schedule = TestDataBuilder.CreateInstructorSchedule("test123", 1, 1); - await _context.InstructorSchedules.AddAsync(schedule); - await _context.SaveChangesAsync(); + await _context.InstructorSchedules.AddAsync(schedule, TestContext.Current.CancellationToken); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); // Set up audit service mock _mockAuditService.LogInstructorRemovedAsync(Arg.Any(), Arg.Any(), Arg.Any(), @@ -363,7 +426,7 @@ public async Task RemoveInstructorScheduleAsync_WithoutPermissionForOwnSchedule_ // Act & Assert - Should throw UnauthorizedAccessException var exception = await Assert.ThrowsAsync(() => - _service.RemoveInstructorScheduleAsync(schedule.InstructorScheduleId)); + _service.RemoveInstructorScheduleAsync(schedule.InstructorScheduleId, TestContext.Current.CancellationToken)); Assert.Contains("does not have permission", exception.Message); } @@ -373,8 +436,8 @@ public async Task RemoveInstructorScheduleAsync_UserTriesToRemoveOtherUserSchedu { // Arrange - User tries to remove another user's schedule without general rotation permission var schedule = TestDataBuilder.CreateInstructorSchedule("other456", 1, 1); - await _context.InstructorSchedules.AddAsync(schedule); - await _context.SaveChangesAsync(); + await _context.InstructorSchedules.AddAsync(schedule, TestContext.Current.CancellationToken); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); // Set up audit service mock _mockAuditService.LogInstructorRemovedAsync(Arg.Any(), Arg.Any(), Arg.Any(), @@ -387,7 +450,7 @@ public async Task RemoveInstructorScheduleAsync_UserTriesToRemoveOtherUserSchedu // Act & Assert - Should throw UnauthorizedAccessException var exception = await Assert.ThrowsAsync(() => - _service.RemoveInstructorScheduleAsync(schedule.InstructorScheduleId)); + _service.RemoveInstructorScheduleAsync(schedule.InstructorScheduleId, TestContext.Current.CancellationToken)); Assert.Contains("does not have permission", exception.Message); } @@ -404,8 +467,8 @@ public async Task AddInstructorAsync_WithScheduleConflicts_ThrowsInvalidOperatio // Create conflicting schedule - same instructor, same rotation, same week var conflictingSchedule = TestDataBuilder.CreateInstructorSchedule(mothraId, rotationId, weekId); - await _context.InstructorSchedules.AddAsync(conflictingSchedule); - await _context.SaveChangesAsync(); + await _context.InstructorSchedules.AddAsync(conflictingSchedule, TestContext.Current.CancellationToken); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); // Override default permission validator to return the specific user for this test _mockPermissionValidator.ValidateEditPermissionAndGetUserAsync(Arg.Any(), Arg.Any(), Arg.Any()) @@ -483,8 +546,8 @@ public async Task RemoveInstructorScheduleAsync_ValidSchedule_RemovesSuccessfull { // Arrange var schedule = TestDataBuilder.CreateInstructorSchedule("test123", 1, 1); - await _context.InstructorSchedules.AddAsync(schedule); - await _context.SaveChangesAsync(); + await _context.InstructorSchedules.AddAsync(schedule, TestContext.Current.CancellationToken); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); var user = TestDataBuilder.CreateUser("currentuser"); _mockPermissionValidator.ValidateEditPermissionAndGetUserAsync(Arg.Any(), Arg.Any(), Arg.Any()) @@ -494,11 +557,11 @@ public async Task RemoveInstructorScheduleAsync_ValidSchedule_RemovesSuccessfull .Returns(new ScheduleAudit()); // Act - var result = await _service.RemoveInstructorScheduleAsync(schedule.InstructorScheduleId); + var result = await _service.RemoveInstructorScheduleAsync(schedule.InstructorScheduleId, TestContext.Current.CancellationToken); // Assert Assert.True(result.success); - var removedSchedule = await _context.InstructorSchedules.FindAsync(schedule.InstructorScheduleId); + var removedSchedule = await _context.InstructorSchedules.FindAsync(new object?[] { schedule.InstructorScheduleId }, TestContext.Current.CancellationToken); Assert.Null(removedSchedule); // Verify the permission validator was called @@ -517,7 +580,7 @@ public async Task RemoveInstructorScheduleAsync_PrimaryEvaluatorWithOtherInstruc var otherSchedule = TestDataBuilder.CreateInstructorSchedule("other456", 1, 1); await _context.InstructorSchedules.AddRangeAsync(primarySchedule, otherSchedule); - await _context.SaveChangesAsync(); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); var user = TestDataBuilder.CreateUser("currentuser"); _mockPermissionValidator.ValidateEditPermissionAndGetUserAsync(Arg.Any(), Arg.Any(), Arg.Any()) @@ -530,11 +593,11 @@ public async Task RemoveInstructorScheduleAsync_PrimaryEvaluatorWithOtherInstruc .Returns(new ScheduleAudit()); // Act - var result = await _service.RemoveInstructorScheduleAsync(primarySchedule.InstructorScheduleId); + var result = await _service.RemoveInstructorScheduleAsync(primarySchedule.InstructorScheduleId, TestContext.Current.CancellationToken); // Assert Assert.True(result.success); - var removedSchedule = await _context.InstructorSchedules.FindAsync(primarySchedule.InstructorScheduleId); + var removedSchedule = await _context.InstructorSchedules.FindAsync(new object?[] { primarySchedule.InstructorScheduleId }, TestContext.Current.CancellationToken); Assert.Null(removedSchedule); // Verify primary evaluator unset audit log @@ -547,8 +610,8 @@ public async Task RemoveInstructorScheduleAsync_PrimaryEvaluatorWithoutOtherInst { // Arrange var primarySchedule = TestDataBuilder.CreateInstructorSchedule("primary123", 2, 2, true); - await _context.InstructorSchedules.AddAsync(primarySchedule); - await _context.SaveChangesAsync(); + await _context.InstructorSchedules.AddAsync(primarySchedule, TestContext.Current.CancellationToken); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); var user = TestDataBuilder.CreateUser("currentuser"); _mockPermissionValidator.ValidateEditPermissionAndGetUserAsync(2, "primary123", Arg.Any()) @@ -561,11 +624,11 @@ public async Task RemoveInstructorScheduleAsync_PrimaryEvaluatorWithoutOtherInst .Returns(new ScheduleAudit()); // Act - var result = await _service.RemoveInstructorScheduleAsync(primarySchedule.InstructorScheduleId); + var result = await _service.RemoveInstructorScheduleAsync(primarySchedule.InstructorScheduleId, TestContext.Current.CancellationToken); // Assert Assert.True(result.success); - var removedSchedule = await _context.InstructorSchedules.FindAsync(primarySchedule.InstructorScheduleId); + var removedSchedule = await _context.InstructorSchedules.FindAsync(new object?[] { primarySchedule.InstructorScheduleId }, TestContext.Current.CancellationToken); Assert.Null(removedSchedule); } @@ -574,8 +637,8 @@ public async Task SetPrimaryEvaluatorAsync_ValidSchedule_UpdatesSuccessfully() { // Arrange var schedule = TestDataBuilder.CreateInstructorSchedule("test123", 1, 1); - await _context.InstructorSchedules.AddAsync(schedule); - await _context.SaveChangesAsync(); + await _context.InstructorSchedules.AddAsync(schedule, TestContext.Current.CancellationToken); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); var user = TestDataBuilder.CreateUser("currentuser"); _mockPermissionValidator.ValidateEditPermissionAndGetUserAsync(Arg.Any(), Arg.Any(), Arg.Any()) @@ -585,11 +648,11 @@ public async Task SetPrimaryEvaluatorAsync_ValidSchedule_UpdatesSuccessfully() .Returns(new ScheduleAudit()); // Act - var result = await _service.SetPrimaryEvaluatorAsync(schedule.InstructorScheduleId, true); + var result = await _service.SetPrimaryEvaluatorAsync(schedule.InstructorScheduleId, true, TestContext.Current.CancellationToken); // Assert Assert.True(result.success); - var updatedSchedule = await _context.InstructorSchedules.FindAsync(schedule.InstructorScheduleId); + var updatedSchedule = await _context.InstructorSchedules.FindAsync(new object?[] { schedule.InstructorScheduleId }, TestContext.Current.CancellationToken); Assert.True(updatedSchedule!.Evaluator); // Verify audit logging @@ -602,11 +665,11 @@ public async Task CanRemoveInstructorAsync_NonPrimaryEvaluator_ReturnsTrue() { // Arrange var schedule = TestDataBuilder.CreateInstructorSchedule("test123", 1, 1); - await _context.InstructorSchedules.AddAsync(schedule); - await _context.SaveChangesAsync(); + await _context.InstructorSchedules.AddAsync(schedule, TestContext.Current.CancellationToken); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); // Act - var result = await _service.CanRemoveInstructorAsync(schedule.InstructorScheduleId); + var result = await _service.CanRemoveInstructorAsync(schedule.InstructorScheduleId, TestContext.Current.CancellationToken); // Assert Assert.True(result); @@ -620,10 +683,10 @@ public async Task CanRemoveInstructorAsync_PrimaryEvaluatorWithOtherInstructors_ var otherSchedule = TestDataBuilder.CreateInstructorSchedule("other456", 1, 1); await _context.InstructorSchedules.AddRangeAsync(primarySchedule, otherSchedule); - await _context.SaveChangesAsync(); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); // Act - var result = await _service.CanRemoveInstructorAsync(primarySchedule.InstructorScheduleId); + var result = await _service.CanRemoveInstructorAsync(primarySchedule.InstructorScheduleId, TestContext.Current.CancellationToken); // Assert Assert.True(result); @@ -634,11 +697,11 @@ public async Task CanRemoveInstructorAsync_PrimaryEvaluatorWithoutOtherInstructo { // Arrange var primarySchedule = TestDataBuilder.CreateInstructorSchedule("primary123", 2, 2, true); - await _context.InstructorSchedules.AddAsync(primarySchedule); - await _context.SaveChangesAsync(); + await _context.InstructorSchedules.AddAsync(primarySchedule, TestContext.Current.CancellationToken); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); // Act - var result = await _service.CanRemoveInstructorAsync(primarySchedule.InstructorScheduleId); + var result = await _service.CanRemoveInstructorAsync(primarySchedule.InstructorScheduleId, TestContext.Current.CancellationToken); // Assert Assert.True(result); @@ -655,11 +718,11 @@ public async Task GetOtherRotationSchedulesAsync_WithConflicts_ReturnsConflictin var conflictSchedule2 = TestDataBuilder.CreateInstructorSchedule(mothraId, 2, 2); await _context.InstructorSchedules.AddRangeAsync(conflictSchedule1, conflictSchedule2); - await _context.SaveChangesAsync(); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); // Act var testYear = DateTime.Now.Year + 1; - var result = await _service.GetOtherRotationSchedulesAsync(mothraId, weekIds, testYear); + var result = await _service.GetOtherRotationSchedulesAsync(mothraId, weekIds, testYear, cancellationToken: TestContext.Current.CancellationToken); // Assert Assert.Equal(2, result.Count); @@ -678,11 +741,11 @@ public async Task GetOtherRotationSchedulesAsync_WithExcludedRotation_ExcludesSp var excludedSchedule = TestDataBuilder.CreateInstructorSchedule(mothraId, 2, 1); await _context.InstructorSchedules.AddRangeAsync(includedSchedule, excludedSchedule); - await _context.SaveChangesAsync(); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); // Act var testYear = DateTime.Now.Year + 1; - var result = await _service.GetOtherRotationSchedulesAsync(mothraId, weekIds, testYear, excludeRotationId); + var result = await _service.GetOtherRotationSchedulesAsync(mothraId, weekIds, testYear, excludeRotationId, TestContext.Current.CancellationToken); // Assert Assert.Single(result); @@ -695,8 +758,8 @@ public async Task GetOtherRotationSchedulesAsync_NoConflicts_ReturnsEmptyList() // Arrange // Create instructor schedule for different weeks (no conflicts) var scheduleNoConflict = TestDataBuilder.CreateInstructorSchedule("12345", 1, 5); - await _context.InstructorSchedules.AddAsync(scheduleNoConflict); - await _context.SaveChangesAsync(); + await _context.InstructorSchedules.AddAsync(scheduleNoConflict, TestContext.Current.CancellationToken); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); // Act - Check for conflicts on different weeks var testYear = DateTime.Now.Year + 1; @@ -715,7 +778,7 @@ public async Task GetOtherRotationSchedulesAsync_WithMultipleConflicts_ReturnsAl var schedule2 = TestDataBuilder.CreateInstructorSchedule("12345", 2, 15); var schedule3 = TestDataBuilder.CreateInstructorSchedule("12345", 3, 20); await _context.InstructorSchedules.AddRangeAsync(schedule1, schedule2, schedule3); - await _context.SaveChangesAsync(); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); // Act - Check for conflicts on all these weeks for a different rotation var testYear = DateTime.Now.Year + 1; @@ -740,7 +803,7 @@ public async Task SetPrimaryEvaluatorAsync_SwitchPrimaryEvaluator_ClearsOldAndSe var instructor2 = TestDataBuilder.CreateInstructorSchedule("test456", rotationId, weekId); // Not primary await _context.InstructorSchedules.AddRangeAsync(instructor1, instructor2); - await _context.SaveChangesAsync(); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); var user = TestDataBuilder.CreateUser("currentuser"); _mockUserHelper.GetCurrentUser().Returns(user); @@ -751,17 +814,17 @@ public async Task SetPrimaryEvaluatorAsync_SwitchPrimaryEvaluator_ClearsOldAndSe .Returns(new ScheduleAudit()); // Act - Switch primary evaluator from instructor1 to instructor2 - var result = await _service.SetPrimaryEvaluatorAsync(instructor2.InstructorScheduleId, true); + var result = await _service.SetPrimaryEvaluatorAsync(instructor2.InstructorScheduleId, true, TestContext.Current.CancellationToken); // Assert Assert.True(result.success); // Verify instructor1 is no longer primary - var updatedInstructor1 = await _context.InstructorSchedules.FindAsync(instructor1.InstructorScheduleId); + var updatedInstructor1 = await _context.InstructorSchedules.FindAsync(new object?[] { instructor1.InstructorScheduleId }, TestContext.Current.CancellationToken); Assert.False(updatedInstructor1!.Evaluator); // Verify instructor2 is now primary - var updatedInstructor2 = await _context.InstructorSchedules.FindAsync(instructor2.InstructorScheduleId); + var updatedInstructor2 = await _context.InstructorSchedules.FindAsync(new object?[] { instructor2.InstructorScheduleId }, TestContext.Current.CancellationToken); Assert.True(updatedInstructor2!.Evaluator); } diff --git a/web/Areas/ClinicalScheduler/Controllers/ScheduleAuditController.cs b/web/Areas/ClinicalScheduler/Controllers/ScheduleAuditController.cs new file mode 100644 index 000000000..431fd3027 --- /dev/null +++ b/web/Areas/ClinicalScheduler/Controllers/ScheduleAuditController.cs @@ -0,0 +1,206 @@ +using Microsoft.AspNetCore.Mvc; +using Microsoft.Data.SqlClient; +using Microsoft.EntityFrameworkCore; +using Viper.Areas.ClinicalScheduler.Models.DTOs.Responses; +using Viper.Areas.ClinicalScheduler.Services; +using Viper.Classes.Utilities; +using Web.Authorization; + +namespace Viper.Areas.ClinicalScheduler.Controllers +{ + /// + /// Read-only access to the schedule-change audit log. Replaces the legacy + /// "Schedule Changes Audit log" page and is gated by the same Manage permission. + /// + [Route("api/clinicalscheduler/audit")] + [ApiController] + [Permission(Allow = ClinicalSchedulePermissions.Manage)] + public class ScheduleAuditController : BaseClinicalSchedulerController + { + private readonly IScheduleAuditService _auditService; + + public ScheduleAuditController( + IScheduleAuditService auditService, + IGradYearService gradYearService, + ILogger logger) + : base(gradYearService, logger) + { + _auditService = auditService; + } + + /// + /// Get the filtered audit log for a grad year (defaults to the current grad year). + /// + /// Grad year; defaults to the current grad year when omitted + /// Optional rotation filter + /// Optional term (semester) filter, scoped to the grad year + /// Optional MothraID of the affected student/clinician + /// Optional MothraID of the user who made the change + /// Optional area filter (Students / Clinicians) + /// Optional inclusive lower bound on the change date + /// Optional inclusive upper bound on the change date + /// Cancellation token + [HttpGet] + [ProducesResponseType(typeof(List), 200)] + public async Task GetAuditLog( + [FromQuery] int? year = null, + [FromQuery] int? rotationId = null, + [FromQuery] int? termCode = null, + [FromQuery] string? person = null, + [FromQuery] string? modifiedBy = null, + [FromQuery] string? area = null, + [FromQuery] DateTime? from = null, + [FromQuery] DateTime? to = null, + CancellationToken cancellationToken = default) + { + try + { + var gradYear = await GetTargetYearAsync(year); + var log = await _auditService.GetAuditLogAsync( + gradYear, rotationId, termCode, person, modifiedBy, area, from, to, cancellationToken); + return Ok(log); + } + catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) + { + _logger.LogError(ex, "Error retrieving audit log for year {Year}", LogSanitizer.SanitizeYear(year)); + return StatusCode(500, "An error occurred while retrieving the audit log"); + } + } + + /// + /// Get the terms (semesters) within a grad year, for the audit trail "Term" filter. + /// + /// Grad year; defaults to the current grad year when omitted + /// Cancellation token + [HttpGet("terms")] + [ProducesResponseType(typeof(List), 200)] + public async Task GetTerms( + [FromQuery] int? year = null, + CancellationToken cancellationToken = default) + { + try + { + var gradYear = await GetTargetYearAsync(year); + var terms = await _auditService.GetAuditTermsAsync(gradYear, cancellationToken); + return Ok(terms); + } + catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) + { + _logger.LogError(ex, "Error retrieving audit terms for year {Year}", LogSanitizer.SanitizeYear(year)); + return StatusCode(500, "An error occurred while retrieving the terms"); + } + } + + /// + /// Get the distinct users who have made audited schedule changes, for the "Modified By" filter. + /// + /// Cancellation token + [HttpGet("modifiers")] + [ProducesResponseType(typeof(List), 200)] + public async Task GetModifiers(CancellationToken cancellationToken = default) + { + try + { + var modifiers = await _auditService.GetAuditModifiersAsync(cancellationToken); + return Ok(modifiers); + } + catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) + { + _logger.LogError(ex, "Error retrieving audit log modifiers"); + return StatusCode(500, "An error occurred while retrieving the audit modifiers"); + } + } + + /// + /// Get the distinct affected students/clinicians in the audit trail, for the "Person" filter. + /// + /// Cancellation token + [HttpGet("persons")] + [ProducesResponseType(typeof(List), 200)] + public async Task GetPersons(CancellationToken cancellationToken = default) + { + try + { + var persons = await _auditService.GetAuditPersonsAsync(cancellationToken); + return Ok(persons); + } + catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) + { + _logger.LogError(ex, "Error retrieving audit log persons"); + return StatusCode(500, "An error occurred while retrieving the audited persons"); + } + } + + /// + /// Get the audit trail for a single rotation + week (inline per-week audit popover, + /// Schedule-by-Rotation grid). + /// + /// Rotation the week belongs to + /// Week to scope the history to + /// Cancellation token + [HttpGet("rotation-week")] + [ProducesResponseType(typeof(List), 200)] + public async Task GetRotationWeekHistory( + [FromQuery] int rotationId, + [FromQuery] int weekId, + CancellationToken cancellationToken = default) + { + try + { + if (rotationId <= 0) + { + return BadRequest("A valid rotation is required."); + } + + if (weekId <= 0) + { + return BadRequest("A valid week is required."); + } + + var history = await _auditService.GetRotationWeekAuditAsync(rotationId, weekId, cancellationToken); + return Ok(history); + } + catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) + { + _logger.LogError(ex, "Error retrieving audit history for rotation {RotationId}, week {WeekId}", rotationId, weekId); + return StatusCode(500, "An error occurred while retrieving the week's audit trail"); + } + } + + /// + /// Get the audit trail for a single clinician + week across all rotations (inline + /// per-week audit popover, Schedule-by-Clinician grid). + /// + /// MothraID of the affected clinician + /// Week to scope the history to + /// Cancellation token + [HttpGet("clinician-week")] + [ProducesResponseType(typeof(List), 200)] + public async Task GetClinicianWeekHistory( + [FromQuery] string mothraId, + [FromQuery] int weekId, + CancellationToken cancellationToken = default) + { + try + { + if (string.IsNullOrWhiteSpace(mothraId)) + { + return BadRequest("A clinician (mothraId) is required."); + } + + if (weekId <= 0) + { + return BadRequest("A valid week is required."); + } + + var history = await _auditService.GetClinicianWeekAuditAsync(mothraId, weekId, cancellationToken); + return Ok(history); + } + catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) + { + _logger.LogError(ex, "Error retrieving audit history for clinician {MothraId}, week {WeekId}", LogSanitizer.SanitizeString(mothraId), weekId); + return StatusCode(500, "An error occurred while retrieving the week's audit trail"); + } + } + } +} diff --git a/web/Areas/ClinicalScheduler/Models/DTOs/Responses/AuditLogEntryDto.cs b/web/Areas/ClinicalScheduler/Models/DTOs/Responses/AuditLogEntryDto.cs new file mode 100644 index 000000000..0de4e9ed3 --- /dev/null +++ b/web/Areas/ClinicalScheduler/Models/DTOs/Responses/AuditLogEntryDto.cs @@ -0,0 +1,26 @@ +namespace Viper.Areas.ClinicalScheduler.Models.DTOs.Responses +{ + /// + /// A single schedule-change audit entry, enriched with display names for the + /// affected person, modifier, rotation, and week. Mirrors the legacy "Schedule + /// Changes Audit log" row (Area / Person / Action / Rotation / Week / Modified By / Date). + /// + public class AuditLogEntryDto + { + public int ScheduleAuditId { get; set; } + public string Area { get; set; } = string.Empty; + public string? MothraId { get; set; } + public string PersonName { get; set; } = string.Empty; + public string Action { get; set; } = string.Empty; + public int? RotationId { get; set; } + public string RotationName { get; set; } = string.Empty; + public int? WeekId { get; set; } + public int WeekNum { get; set; } + public DateTime? WeekStart { get; set; } + public int TermCode { get; set; } + public string Term { get; set; } = string.Empty; + public string ModifiedBy { get; set; } = string.Empty; + public string ModifiedByName { get; set; } = string.Empty; + public DateTime TimeStamp { get; set; } + } +} diff --git a/web/Areas/ClinicalScheduler/Models/DTOs/Responses/AuditModifierDto.cs b/web/Areas/ClinicalScheduler/Models/DTOs/Responses/AuditModifierDto.cs new file mode 100644 index 000000000..c36362de3 --- /dev/null +++ b/web/Areas/ClinicalScheduler/Models/DTOs/Responses/AuditModifierDto.cs @@ -0,0 +1,13 @@ +namespace Viper.Areas.ClinicalScheduler.Models.DTOs.Responses +{ + /// + /// A distinct person referenced by an audited schedule change, used to populate the + /// audit trail's person picklists — both "Modified By" (the user who made the change) + /// and "Person" (the affected student/clinician). + /// + public class AuditModifierDto + { + public string MothraId { get; set; } = string.Empty; + public string DisplayName { get; set; } = string.Empty; + } +} diff --git a/web/Areas/ClinicalScheduler/Models/DTOs/Responses/AuditTermDto.cs b/web/Areas/ClinicalScheduler/Models/DTOs/Responses/AuditTermDto.cs new file mode 100644 index 000000000..ca1ef5851 --- /dev/null +++ b/web/Areas/ClinicalScheduler/Models/DTOs/Responses/AuditTermDto.cs @@ -0,0 +1,11 @@ +namespace Viper.Areas.ClinicalScheduler.Models.DTOs.Responses +{ + /// + /// A selectable term (semester) option for the audit trail "Term" filter, scoped to a grad year. + /// + public class AuditTermDto + { + public int TermCode { get; set; } + public string Term { get; set; } = string.Empty; + } +} diff --git a/web/Areas/ClinicalScheduler/Services/ClinicalSchedulerNavMenu.cs b/web/Areas/ClinicalScheduler/Services/ClinicalSchedulerNavMenu.cs index 8136d1d26..66b58e1fc 100644 --- a/web/Areas/ClinicalScheduler/Services/ClinicalSchedulerNavMenu.cs +++ b/web/Areas/ClinicalScheduler/Services/ClinicalSchedulerNavMenu.cs @@ -20,6 +20,7 @@ public async Task Nav(CancellationToken cancellationToken = default) var userHelper = new UserHelper(); var user = userHelper.GetCurrentUser(); var hasAccess = userHelper.HasPermission(_rapsContext, user, ClinicalSchedulePermissions.Base); + var hasManage = userHelper.HasPermission(_rapsContext, user, ClinicalSchedulePermissions.Manage); var nav = new List { @@ -44,6 +45,12 @@ public async Task Nav(CancellationToken cancellationToken = default) nav.Add(new NavMenuItem { MenuItemText = clinicianLabel, MenuItemURL = "~/ClinicalScheduler/clinician", IndentLevel = 1 }); } + if (hasManage) + { + nav.Add(new NavMenuItem { MenuItemText = "Audit Trail", MenuItemURL = "~/ClinicalScheduler/audit", IndentLevel = 1 }); + } + + nav.Add(new NavMenuItem { MenuItemText = "Clinical Scheduler 1.0", MenuItemURL = $"{oldViperUrl}/clinicalScheduler/" }); } diff --git a/web/Areas/ClinicalScheduler/Services/IScheduleAuditService.cs b/web/Areas/ClinicalScheduler/Services/IScheduleAuditService.cs index 60daf7577..58f20ec41 100644 --- a/web/Areas/ClinicalScheduler/Services/IScheduleAuditService.cs +++ b/web/Areas/ClinicalScheduler/Services/IScheduleAuditService.cs @@ -1,3 +1,4 @@ +using Viper.Areas.ClinicalScheduler.Models.DTOs.Responses; using Viper.Models.ClinicalScheduler; namespace Viper.Areas.ClinicalScheduler.Services @@ -93,6 +94,87 @@ Task> GetRotationWeekAuditHistoryAsync( int weekId, CancellationToken cancellationToken = default); + /// + /// Get a filtered, display-ready audit log for a grad year. + /// Mirrors the legacy "Schedule Changes Audit log" page query. + /// + /// Grad year to scope results to (via the week's grad year) + /// Optional rotation filter + /// Optional term (semester) filter, scoped to the grad year + /// Optional MothraID of the affected student/clinician + /// Optional MothraID of the user who made the change + /// Optional area filter (Students / Clinicians) + /// Optional inclusive lower bound on the change timestamp + /// Optional inclusive upper bound on the change timestamp + /// Cancellation token + /// Up to 2500 enriched audit entries, newest first + Task> GetAuditLogAsync( + int gradYear, + int? rotationId, + int? termCode, + string? person, + string? modifiedBy, + string? area, + DateTime? fromDate, + DateTime? toDate, + CancellationToken cancellationToken = default); + + /// + /// Get the distinct terms (semesters) that fall within a grad year, for the audit + /// trail "Term" filter. + /// + /// Grad year to scope the terms to + /// Cancellation token + /// Distinct terms ordered chronologically + Task> GetAuditTermsAsync( + int gradYear, + CancellationToken cancellationToken = default); + + /// + /// Get the distinct set of users who have made an audited schedule change, + /// used to populate the audit trail "Modified By" filter. + /// + /// Cancellation token + /// Distinct modifiers ordered by display name + Task> GetAuditModifiersAsync( + CancellationToken cancellationToken = default); + + /// + /// Get the distinct set of affected students/clinicians that appear in the audit + /// trail, used to populate the audit trail "Person" filter. + /// + /// Cancellation token + /// Distinct affected persons ordered by display name + Task> GetAuditPersonsAsync( + CancellationToken cancellationToken = default); + + /// + /// Get the display-ready audit trail for a single rotation + week, newest first. + /// Powers the inline per-week audit popover in the Schedule-by-Rotation grid. + /// + /// Rotation the week belongs to + /// Week to scope the history to + /// Cancellation token + /// Enriched audit entries for the rotation/week, newest first + Task> GetRotationWeekAuditAsync( + int rotationId, + int weekId, + CancellationToken cancellationToken = default); + + /// + /// Get the display-ready audit trail for a single clinician + week, newest first + /// (across all rotations that week). Powers the inline per-week audit popover in + /// the Schedule-by-Clinician grid. + /// + /// MothraID of the affected clinician + /// Week to scope the history to + /// Cancellation token + /// Enriched audit entries for the clinician/week, newest first + Task> GetClinicianWeekAuditAsync( + string mothraId, + int weekId, + CancellationToken cancellationToken = default); + } } diff --git a/web/Areas/ClinicalScheduler/Services/ScheduleAuditService.cs b/web/Areas/ClinicalScheduler/Services/ScheduleAuditService.cs index 81f78e4e0..3c3fe784c 100644 --- a/web/Areas/ClinicalScheduler/Services/ScheduleAuditService.cs +++ b/web/Areas/ClinicalScheduler/Services/ScheduleAuditService.cs @@ -1,6 +1,8 @@ using Microsoft.Data.SqlClient; using Microsoft.EntityFrameworkCore; using Viper.Areas.ClinicalScheduler.Constants; +using Viper.Areas.ClinicalScheduler.Models.DTOs.Responses; +using Viper.Areas.Curriculum.Services; using Viper.Classes.SQLContext; using Viper.Classes.Utilities; using Viper.Models.ClinicalScheduler; @@ -106,12 +108,15 @@ public async Task> GetInstructorScheduleAuditHistoryAsync( return new List(); } - // Find audit entries matching the rotation, week, and instructor + // Find audit entries matching the rotation, week, and instructor. + // Clinicians only: the shared audit table also records student schedule + // changes, which are out of scope for an instructor schedule's history. return await _context.ScheduleAudits .AsNoTracking() .Where(a => a.RotationId == schedule.RotationId && a.WeekId == schedule.WeekId - && a.MothraId == schedule.MothraId) + && a.MothraId == schedule.MothraId + && a.Area == ScheduleAuditAreas.Clinicians) .OrderByDescending(a => a.TimeStamp) .ToListAsync(cancellationToken); } @@ -129,9 +134,10 @@ public async Task> GetRotationWeekAuditHistoryAsync( { try { + // Clinicians only; see GetInstructorScheduleAuditHistoryAsync return await _context.ScheduleAudits .AsNoTracking() - .Where(a => a.RotationId == rotationId && a.WeekId == weekId) + .Where(a => a.RotationId == rotationId && a.WeekId == weekId && a.Area == ScheduleAuditAreas.Clinicians) .OrderByDescending(a => a.TimeStamp) .ToListAsync(cancellationToken); } @@ -142,6 +148,281 @@ public async Task> GetRotationWeekAuditHistoryAsync( } } + public async Task> GetAuditLogAsync( + int gradYear, + int? rotationId, + int? termCode, + string? person, + string? modifiedBy, + string? area, + DateTime? fromDate, + DateTime? toDate, + CancellationToken cancellationToken = default) + { + try + { + // Scope to the grad year via vWeek (a week belongs to a grad year there), + // and left-join names so a missing person/rotation lookup never drops a row. + var query = + from a in _context.ScheduleAudits.AsNoTracking() + join vw in _context.VWeeks on a.WeekId equals vw.WeekId + where vw.GradYear == gradYear + join rot in _context.Rotations on a.RotationId equals rot.RotId into rotJoin + from rot in rotJoin.DefaultIfEmpty() + join ap in _context.Persons on a.MothraId equals ap.IdsMothraId into affectedJoin + from ap in affectedJoin.DefaultIfEmpty() + join mp in _context.Persons on a.ModifiedBy equals mp.IdsMothraId into modifierJoin + from mp in modifierJoin.DefaultIfEmpty() + select new { Audit = a, Week = vw, Rotation = rot, Affected = ap, Modifier = mp }; + + if (rotationId is { } selectedRotationId) + { + query = query.Where(x => x.Audit.RotationId == selectedRotationId); + } + if (termCode is { } selectedTermCode) + { + query = query.Where(x => x.Week.TermCode == selectedTermCode); + } + if (!string.IsNullOrWhiteSpace(area)) + { + query = query.Where(x => x.Audit.Area == area); + } + if (!string.IsNullOrWhiteSpace(modifiedBy)) + { + query = query.Where(x => x.Audit.ModifiedBy == modifiedBy); + } + if (fromDate is { } fromTimestamp) + { + query = query.Where(x => x.Audit.TimeStamp >= fromTimestamp); + } + if (toDate is { } toTimestamp) + { + // The date picker binds to midnight at the start of the day, so advance the + // bound a day to include changes made anytime on the selected end date + // (matches the Effort audit upper-bound handling). + var endOfDay = toTimestamp.AddDays(1); + query = query.Where(x => x.Audit.TimeStamp < endOfDay); + } + if (!string.IsNullOrWhiteSpace(person)) + { + query = query.Where(x => x.Audit.MothraId == person); + } + + var entries = await query + .OrderByDescending(x => x.Audit.TimeStamp) + .Take(2500) + .Select(x => new AuditLogEntryDto + { + ScheduleAuditId = x.Audit.ScheduleAuditId, + Area = x.Audit.Area, + MothraId = x.Audit.MothraId, + PersonName = x.Affected != null ? x.Affected.PersonDisplayFullName : (x.Audit.MothraId ?? string.Empty), + Action = x.Audit.Action, + RotationId = x.Audit.RotationId, + RotationName = x.Rotation != null ? x.Rotation.Name : string.Empty, + WeekId = x.Audit.WeekId, + WeekNum = x.Week.WeekNum, + WeekStart = x.Week.DateStart, + TermCode = x.Week.TermCode, + ModifiedBy = x.Audit.ModifiedBy, + ModifiedByName = x.Modifier != null ? x.Modifier.PersonDisplayFullName : x.Audit.ModifiedBy, + TimeStamp = x.Audit.TimeStamp, + }) + .ToListAsync(cancellationToken); + + return ApplyTermDescriptions(entries); + } + catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) + { + _logger.LogError(ex, "Error retrieving audit log for grad year {GradYear}", LogSanitizer.SanitizeYear(gradYear)); + throw new InvalidOperationException("Failed to retrieve the audit log. Please try again or contact support if the problem persists.", ex); + } + } + + public async Task> GetAuditTermsAsync( + int gradYear, + CancellationToken cancellationToken = default) + { + try + { + // Distinct terms the grad year's weeks fall into; codes sort chronologically. + var termCodes = await _context.VWeeks.AsNoTracking() + .Where(w => w.GradYear == gradYear) + .Select(w => w.TermCode) + .Distinct() + .OrderBy(t => t) + .ToListAsync(cancellationToken); + + return termCodes + .Select(termCode => new AuditTermDto + { + TermCode = termCode, + Term = TermCodeService.GetTermCodeDescription(termCode), + }) + .ToList(); + } + catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) + { + _logger.LogError(ex, "Error retrieving audit terms for grad year {GradYear}", LogSanitizer.SanitizeYear(gradYear)); + throw new InvalidOperationException("Failed to retrieve the terms for this grad year. Please try again or contact support if the problem persists.", ex); + } + } + + public async Task> GetAuditModifiersAsync( + CancellationToken cancellationToken = default) + { + try + { + // Left join so a modifier whose MothraId no longer resolves in Persons still + // appears in the filter, matching the raw-ID fallback in GetAuditLogAsync. + return await ( + from a in _context.ScheduleAudits.AsNoTracking() + where a.ModifiedBy != "" + join p in _context.Persons on a.ModifiedBy equals p.IdsMothraId into modifierJoin + from p in modifierJoin.DefaultIfEmpty() + select new AuditModifierDto + { + MothraId = a.ModifiedBy, + DisplayName = p != null ? p.PersonDisplayFullName : a.ModifiedBy, + }) + .Distinct() + .OrderBy(m => m.DisplayName) + .ToListAsync(cancellationToken); + } + catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) + { + _logger.LogError(ex, "Error retrieving audit log modifiers"); + throw new InvalidOperationException("Failed to retrieve the list of audit modifiers. Please try again or contact support if the problem persists.", ex); + } + } + + public async Task> GetAuditPersonsAsync( + CancellationToken cancellationToken = default) + { + try + { + // Left join so an affected person whose MothraId no longer resolves in Persons + // still appears in the filter, matching the raw-ID fallback in GetAuditLogAsync. + return await ( + from a in _context.ScheduleAudits.AsNoTracking() + where a.MothraId != null && a.MothraId != "" + join p in _context.Persons on a.MothraId equals p.IdsMothraId into affectedJoin + from p in affectedJoin.DefaultIfEmpty() + select new AuditModifierDto + { + MothraId = a.MothraId!, + DisplayName = p != null ? p.PersonDisplayFullName : a.MothraId!, + }) + .Distinct() + .OrderBy(m => m.DisplayName) + .ToListAsync(cancellationToken); + } + catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) + { + _logger.LogError(ex, "Error retrieving audit log persons"); + throw new InvalidOperationException("Failed to retrieve the list of audited persons. Please try again or contact support if the problem persists.", ex); + } + } + + public async Task> GetRotationWeekAuditAsync( + int rotationId, + int weekId, + CancellationToken cancellationToken = default) + { + try + { + // Clinician scheduling context only: the shared ScheduleAudit table also + // holds student schedule changes (Area = Students) for the same + // rotation/week, which would read as phantom roster entries here. + var audits = _context.ScheduleAudits.AsNoTracking() + .Where(a => a.RotationId == rotationId && a.WeekId == weekId && a.Area == ScheduleAuditAreas.Clinicians); + var entries = await BuildEnrichedAuditQuery(audits) + .OrderByDescending(x => x.TimeStamp) + .ToListAsync(cancellationToken); + return ApplyTermDescriptions(entries); + } + catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) + { + _logger.LogError(ex, "Error retrieving audit history for rotation {RotationId}, week {WeekId}", rotationId, weekId); + throw new InvalidOperationException("Failed to retrieve the audit trail for this week. Please try again or contact support if the problem persists.", ex); + } + } + + public async Task> GetClinicianWeekAuditAsync( + string mothraId, + int weekId, + CancellationToken cancellationToken = default) + { + try + { + // Clinicians only, same as GetRotationWeekAuditAsync: a clinician who was + // previously a UCD student would otherwise see student-era entries here. + var audits = _context.ScheduleAudits.AsNoTracking() + .Where(a => a.MothraId == mothraId && a.WeekId == weekId && a.Area == ScheduleAuditAreas.Clinicians); + var entries = await BuildEnrichedAuditQuery(audits) + .OrderByDescending(x => x.TimeStamp) + .ToListAsync(cancellationToken); + return ApplyTermDescriptions(entries); + } + catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) + { + _logger.LogError(ex, "Error retrieving audit history for clinician {MothraId}, week {WeekId}", LogSanitizer.SanitizeString(mothraId), weekId); + throw new InvalidOperationException("Failed to retrieve the audit trail for this week. Please try again or contact support if the problem persists.", ex); + } + } + + /// + /// Build the display-ready (name / rotation / week-enriched) projection for a + /// pre-filtered set of audit rows. Left-joins every lookup so a missing match never + /// drops a row; shared by the per-rotation-week and per-clinician-week history queries. + /// + private IQueryable BuildEnrichedAuditQuery(IQueryable audits) + { + // Base Weeks (unique per WeekId), not vWeek (one row per WeekId x grad year), + // which would duplicate every audit row for a week that spans two grad years. + // WeekNum stays 0 for the same reason: it is a per-grad-year concept, so a + // week-scoped query has no single unambiguous week number. + return + from a in audits + join w in _context.Weeks on a.WeekId equals w.WeekId into weekJoin + from w in weekJoin.DefaultIfEmpty() + join rot in _context.Rotations on a.RotationId equals rot.RotId into rotJoin + from rot in rotJoin.DefaultIfEmpty() + join ap in _context.Persons on a.MothraId equals ap.IdsMothraId into affectedJoin + from ap in affectedJoin.DefaultIfEmpty() + join mp in _context.Persons on a.ModifiedBy equals mp.IdsMothraId into modifierJoin + from mp in modifierJoin.DefaultIfEmpty() + select new AuditLogEntryDto + { + ScheduleAuditId = a.ScheduleAuditId, + Area = a.Area, + MothraId = a.MothraId, + PersonName = ap != null ? ap.PersonDisplayFullName : (a.MothraId ?? string.Empty), + Action = a.Action, + RotationId = a.RotationId, + RotationName = rot != null ? rot.Name : string.Empty, + WeekId = a.WeekId, + WeekStart = w != null ? w.DateStart : null, + TermCode = w != null ? w.TermCode : 0, + ModifiedBy = a.ModifiedBy, + ModifiedByName = mp != null ? mp.PersonDisplayFullName : a.ModifiedBy, + TimeStamp = a.TimeStamp, + }; + } + + /// + /// Fill in the term description from the SQL-populated term code. + /// GetTermCodeDescription is C#, not SQL-translatable, so this runs after materializing. + /// + private static List ApplyTermDescriptions(List entries) + { + foreach (var entry in entries) + { + entry.Term = entry.TermCode > 0 ? TermCodeService.GetTermCodeDescription(entry.TermCode) : string.Empty; + } + return entries; + } + /// /// Create a schedule audit entry with common properties /// diff --git a/web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs b/web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs index 816e32822..9f9643b3c 100644 --- a/web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs +++ b/web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs @@ -184,47 +184,25 @@ public async Task> AddInstructorAsync( // Save all schedule entities at once for better performance await _context.SaveChangesAsync(cancellationToken); + // Audit inside the transaction so a schedule change can never commit + // without its trail; an audit failure rolls the change back + // (matching the legacy scheduler's in-transaction auditing). + await WriteAddAuditEntriesAsync( + mothraId!, rotationId, isPrimaryEvaluator, createdSchedules, removedPrimarySchedules, currentUser.MothraId, cancellationToken); + return (createdSchedules, removedPrimarySchedules); }, IsolationLevel.Serializable, cancellationToken); var (createdSchedules, removedPrimarySchedules) = result; - // Send notifications and log audit entries AFTER successful transaction commit - // This ensures we don't send emails for operations that were rolled back - // Post-transaction operations are wrapped in try-catch to prevent perceived failures - try + // Notify AFTER the commit so emails never fire for rolled-back + // changes. The sender swallows its own failures. + foreach (var (_, removedSchedules) in removedPrimarySchedules) { - // Send notifications for any removed primary evaluators - foreach (var (_, removedSchedules) in removedPrimarySchedules) + foreach (var removedSchedule in removedSchedules) { - foreach (var removedSchedule in removedSchedules) - { - await HandlePrimaryEvaluatorRemovalAsync(removedSchedule, currentUser.MothraId, cancellationToken, mothraId); - } + await SendPrimaryEvaluatorRemovedNotificationAsync(removedSchedule, currentUser.MothraId, cancellationToken, mothraId); } - - // Log audit entries after successful save (so we have InstructorScheduleId) -#pragma warning disable S3267 // Loop contains async operations that cannot be simplified to Select - foreach (var schedule in createdSchedules) - { - await _auditService.LogInstructorAddedAsync( - mothraId!, rotationId, schedule.WeekId, currentUser.MothraId, cancellationToken); - - if (isPrimaryEvaluator) - { - await _auditService.LogPrimaryEvaluatorSetAsync( - mothraId!, rotationId, schedule.WeekId, currentUser.MothraId, cancellationToken); - } - } -#pragma warning restore S3267 - } -#pragma warning disable CA1031 // Intentional broad catch: post-transaction work (email/audit notifications) must not roll back the successful database changes above. - catch (Exception postTransactionEx) -#pragma warning restore CA1031 - { - // Log warning but don't fail the operation - the database changes were successful - _logger.LogWarning(postTransactionEx, "Post-transaction operations failed for instructor {MothraId} in rotation {RotationId}, but database changes were successful", - LogSanitizer.SanitizeId(mothraId), rotationId); } _logger.LogInformation("Successfully added instructor {MothraId} to rotation {RotationId} for {WeekCount} weeks (Primary: {IsPrimary})", @@ -259,6 +237,43 @@ await _auditService.LogPrimaryEvaluatorSetAsync( } } + /// + /// Writes the audit entries for an add operation. Must be called inside the + /// same transaction as the schedule changes so the trail commits atomically. + /// + private async Task WriteAddAuditEntriesAsync( + string mothraId, + int rotationId, + bool isPrimaryEvaluator, + List createdSchedules, + Dictionary> removedPrimarySchedules, + string modifiedByMothraId, + CancellationToken cancellationToken) + { +#pragma warning disable S3267 // Loop contains async operations that cannot be simplified to Select + foreach (var (_, removedSchedules) in removedPrimarySchedules) + { + foreach (var removedSchedule in removedSchedules) + { + await _auditService.LogPrimaryEvaluatorUnsetAsync( + removedSchedule.MothraId, removedSchedule.RotationId, removedSchedule.WeekId, modifiedByMothraId, cancellationToken); + } + } + + foreach (var schedule in createdSchedules) + { + await _auditService.LogInstructorAddedAsync( + mothraId, rotationId, schedule.WeekId, modifiedByMothraId, cancellationToken); + + if (isPrimaryEvaluator) + { + await _auditService.LogPrimaryEvaluatorSetAsync( + mothraId, rotationId, schedule.WeekId, modifiedByMothraId, cancellationToken); + } + } +#pragma warning restore S3267 + } + // SQL Server: 2627 = unique constraint violation, 2601 = duplicate key in a unique index. // EF wraps the provider error in DbUpdateException, so walk the inner-exception chain to find // the underlying SqlException (robust even if it is wrapped more than once). @@ -333,29 +348,25 @@ await ExecuteInTransactionAsync(async _ => // Remove the schedule _context.InstructorSchedules.Remove(schedule); await _context.SaveChangesAsync(cancellationToken); - return true; - }, cancellationToken); - // Send notifications and log audit entries AFTER successful transaction commit - // This ensures we don't send emails for operations that were rolled back - // Post-transaction operations are wrapped in try-catch to prevent perceived failures - try - { + // Audit inside the transaction; see AddInstructorAsync for rationale await _auditService.LogInstructorRemovedAsync( schedule.MothraId, schedule.RotationId, schedule.WeekId, currentUser.MothraId, cancellationToken); if (wasPrimaryEvaluator) { - await HandlePrimaryEvaluatorRemovalAsync(schedule, currentUser.MothraId, cancellationToken); + await _auditService.LogPrimaryEvaluatorUnsetAsync( + schedule.MothraId, schedule.RotationId, schedule.WeekId, currentUser.MothraId, cancellationToken); } - } -#pragma warning disable CA1031 // Intentional broad catch: post-transaction work (email/audit notifications) must not roll back the successful database changes above. - catch (Exception postTransactionEx) -#pragma warning restore CA1031 + + return true; + }, cancellationToken); + + // Notify AFTER the commit so emails never fire for rolled-back + // changes. The sender swallows its own failures. + if (wasPrimaryEvaluator) { - // Log warning but don't fail the operation - the database changes were successful - _logger.LogWarning(postTransactionEx, "Post-transaction operations failed for instructor removal {ScheduleId}, but database changes were successful", - instructorScheduleId); + await SendPrimaryEvaluatorRemovedNotificationAsync(schedule, currentUser.MothraId, cancellationToken); } _logger.LogInformation("Successfully removed instructor {MothraId} from rotation {RotationId}, week {WeekId} (WasPrimary: {WasPrimary})", @@ -444,39 +455,43 @@ await _auditService.LogInstructorRemovedAsync( await _context.SaveChangesAsync(cancellationToken); - return (previousPrimaryName, removedPrimarySchedules); - }, cancellationToken); - - var (previousPrimaryName, removedPrimarySchedules) = result; - - // Send notifications and log audit entries AFTER successful transaction commit - // This ensures we don't send emails for operations that were rolled back - // Post-transaction operations are wrapped in try-catch to prevent perceived failures - try - { + // Audit inside the transaction; see AddInstructorAsync for rationale if (isPrimary) { - // Send notifications for any removed primary evaluators +#pragma warning disable S3267 // Loop contains async operations that cannot be simplified to Select foreach (var removedSchedule in removedPrimarySchedules) { - await HandlePrimaryEvaluatorRemovalAsync(removedSchedule, currentUser.MothraId, cancellationToken, schedule.MothraId, requiresPrimaryEvaluator); + await _auditService.LogPrimaryEvaluatorUnsetAsync( + removedSchedule.MothraId, removedSchedule.RotationId, removedSchedule.WeekId, currentUser.MothraId, cancellationToken); } +#pragma warning restore S3267 await _auditService.LogPrimaryEvaluatorSetAsync( schedule.MothraId, schedule.RotationId, schedule.WeekId, currentUser.MothraId, cancellationToken); } else { - await HandlePrimaryEvaluatorRemovalAsync(schedule, currentUser.MothraId, cancellationToken, null, requiresPrimaryEvaluator); + await _auditService.LogPrimaryEvaluatorUnsetAsync( + schedule.MothraId, schedule.RotationId, schedule.WeekId, currentUser.MothraId, cancellationToken); + } + + return (previousPrimaryName, removedPrimarySchedules); + }, cancellationToken); + + var (previousPrimaryName, removedPrimarySchedules) = result; + + // Notify AFTER the commit so emails never fire for rolled-back + // changes. The sender swallows its own failures. + if (isPrimary) + { + foreach (var removedSchedule in removedPrimarySchedules) + { + await SendPrimaryEvaluatorRemovedNotificationAsync(removedSchedule, currentUser.MothraId, cancellationToken, schedule.MothraId, requiresPrimaryEvaluator); } } -#pragma warning disable CA1031 // Intentional broad catch: post-transaction work (email/audit notifications) must not roll back the successful database changes above. - catch (Exception postTransactionEx) -#pragma warning restore CA1031 + else { - // Log warning but don't fail the operation - the database changes were successful - _logger.LogWarning(postTransactionEx, "Post-transaction operations failed for primary evaluator update {ScheduleId}, but database changes were successful", - instructorScheduleId); + await SendPrimaryEvaluatorRemovedNotificationAsync(schedule, currentUser.MothraId, cancellationToken, null, requiresPrimaryEvaluator); } _logger.LogInformation("Set primary evaluator for {MothraId} on rotation {RotationId}, week {WeekId} to {IsPrimary}", @@ -624,25 +639,6 @@ private async Task> ClearPrimaryEvaluatorAsync(int rota return existingPrimary; } - /// - /// Handles the complete process of removing a primary evaluator status, - /// including audit logging and email notifications - /// - private async Task HandlePrimaryEvaluatorRemovalAsync( - InstructorSchedule schedule, - string currentUserMothraId, - CancellationToken cancellationToken, - string? newPrimaryMothraId = null, - bool requiresPrimaryEvaluator = false) - { - // Log the primary evaluator removal - await _auditService.LogPrimaryEvaluatorUnsetAsync( - schedule.MothraId, schedule.RotationId, schedule.WeekId, currentUserMothraId, cancellationToken); - - // Send email notification - await SendPrimaryEvaluatorRemovedNotificationAsync(schedule, currentUserMothraId, cancellationToken, newPrimaryMothraId, requiresPrimaryEvaluator); - } - /// /// Send email notification when a primary evaluator is removed /// Matches the ColdFusion system's notification format and recipients