Skip to content

Conversation

rlorenzo
Copy link
Contributor

Still in draft, but read-only mode is working and ready for review

@rlorenzo rlorenzo requested a review from bsedwards August 13, 2025 15:07
@rlorenzo rlorenzo self-assigned this Aug 13, 2025
@Copilot Copilot AI review requested due to automatic review settings August 13, 2025 15:07
Copilot

This comment was marked as outdated.

@rlorenzo rlorenzo force-pushed the clinical-scheduler-ui branch 3 times, most recently from 3d6dfd5 to 17e193e Compare August 15, 2025 17:55
Copy link
Collaborator

@bsedwards bsedwards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good start - I left a lot of comments, so please feel free to reach out with questions or if you want to chat.

One thing I see that should be addressed is the lack of permission checking- if you're not ready to implement detailed permission checking, checking the most restrictive permission on the controllers would be appropriate (in this case, SVMSecure.ClnSched.Manage) until you're ready.

Unless there's a use I'm not seeing, code like the database testing code should not be committed and merged. Having API endpoints doing those things seems like a risk.

Hopefully the comments about the context class make sense. The CTS app is accessing clinical scheduler data through views, while the clinical scheduler context should map those tables directly.

@rlorenzo rlorenzo force-pushed the clinical-scheduler-ui branch from 17e193e to f8ea22c Compare August 15, 2025 20:27
@rlorenzo rlorenzo requested a review from Copilot August 17, 2025 20:08
Copilot

This comment was marked as outdated.

@rlorenzo rlorenzo requested a review from Copilot August 17, 2025 20:47
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces the Clinical Scheduler UI module with read-only functionality for viewing rotation schedules. The implementation adds a new Vue.js application area focused on clinical scheduling with complete backend services and API endpoints.

  • Adds a new ClinicalScheduler area with comprehensive backend services for managing weeks, rotations, persons, and graduation years
  • Implements Entity Framework models for Clinical Scheduler database schema with proper navigation properties
  • Creates API controllers with full CRUD operations for rotations, clinicians, and schedules
  • Establishes Vue.js application structure for the Clinical Scheduler frontend

Reviewed Changes

Copilot reviewed 62 out of 63 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
web/Program.cs Registers Clinical Scheduler services and adds Vue app configuration
web/Classes/SQLContext/ClinicalSchedulerContext.cs Defines EF context with complete Clinical Scheduler database mapping
web/Areas/ClinicalScheduler/Services/* Implements service layer with business logic for weeks, rotations, persons, and graduation years
web/Areas/ClinicalScheduler/Controllers/* Creates API controllers with comprehensive endpoints for scheduler data
web/Models/ClinicalScheduler/* Defines entity models mapping to Clinical Scheduler database tables
VueApp/vite.config.ts Adds Clinical Scheduler proxy configuration for development
test/ClinicalScheduler/* Provides comprehensive unit tests for services and controllers
Files not reviewed (1)
  • VueApp/package-lock.json: Language not supported

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

/// <param name="contextProperty">Optional context property name for logging</param>
/// <param name="contextValue">Optional context value for logging</param>
/// <returns>A standardized 500 Internal Server Error response</returns>
protected ObjectResult HandleException(Exception ex, string message, string? contextProperty = null, object? contextValue = null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this logic be combined into the ApiExceptionFilter class? If so, it would make exception handling more standardized across apps.

/// <param name="year">Year to filter clinicians by (optional, defaults to current year behavior)</param>
/// <param name="includeAllAffiliates">If true, includes all affiliates instead of just active clinicians</param>
/// <returns>List of clinicians with their basic info</returns>
[HttpGet]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I was reading too fast on this one. :)

@rlorenzo rlorenzo force-pushed the clinical-scheduler-ui branch from 7c6bdf1 to d5bc156 Compare August 27, 2025 08:44
rlorenzo and others added 15 commits September 1, 2025 11:07
Phase 1 - Add Clinical Scheduler backend infrastructure

- Add dedicated database context for Clinical Scheduler
- Configure app to use the new database connection

Phase 2 - VueJS Ui with dual scheduling views
- Schedule by Rotation
- Schedule by Clinician
- Landing page
Phase 3: Backend API

Phase 4: Frontend integration
- Connect frontend to live API with loading and
  error states

Phase 5: Real schedule display

- Replace all sample data with actual database
  queries
- Display real instructor assignments with
  primary evaluator indicators (★/☆)
- Show actual week dates and numbers from
  database
- Add visual alerts (⚠️) for even weeks requiring
  primary evaluators
- Implement bookmarkable URLs with rotation
  parameters
- Enable responsive week grid layout for mobile
  devices
- Implemented complete clinician scheduling view with search and filtering
- Added ClinicianService and CliniciansController
- Changed clinician selectors to Quasar QSelect components
- Added "Include all affiliates" option to clinician filters
- Added TermCodes utility class for semester/term management
- Enhanced RotationSelector and ClinicianSelector with improved UX
- Implemented year filtering and historical data viewing
- Integrated with academic calendar
- Added EvaluationPolicyService for primary evaluator business rules
- Comprehensive unit tests for AcademicYearService with Vitest
- Removed development endpoints
- Added additional frontend tests
- TODO: Some UX issues with "Schedule by Clinician"
- Add service interfaces (IGradYearService, IPersonService, IRotationService, IWeekService) for better dependency
  injection
- Rename AcademicYearService to GradYearService for consistency
- Migrate database querying from CTS to ClinicalScheduler database
- Move models to proper Viper.Models.ClinicalScheduler namespace
- Refactor controllers to use centralized exception handling and structured logging
- Replace custom styles with Quasar components for consistency
- Add shared CSS styling for schedule components
- Added UC Davis colors as css variables for global usage
- Add comprehensive test coverage for new services
- Change app.RunAsync() to app.Run() in Program.cs for standard ASP.NET Core pattern
- Added additional documentation
…oring

- Deleted unused WeekGrid, InstructorScheduleController,  StudentScheduleController controllers.
- Add new reusable components: ScheduleBanner, SchedulerNavigation, WeekScheduleCard
- Fixed "Recent Clinicians" to get those assigned to a given rotation in this AND last year.
- Moved ClinicalScheduleSecurityService.cs from CTS area to ClinicalScheduler
- Fix hardcoded API paths and standardize URL configuration across components
- Aligned Clinician dropdown and year in "Schedule by Clinician"
- Made scheduler items stretch across the entire screen
… editing

- NOTE: The permissions system is not yet integrated with the frontend
- Add PermissionService and controller for service-level edit permissions
- Create permission store for managing user access state
- Add comprehensive test coverage with base test class and builders
- Fix ViperFetch header handling with safe property access
- Refactor services to use VITE_API_URL consistently
- Implement full CRUD API for clinician schedule assignments
- Add comprehensive audit logging for all schedule modifications
- Create schedule edit service with conflict detection and validation
- Integrate granular permission system throughout edit operations
- Ensure atomic transactions with automatic audit trail
- Add primary evaluator management with business rule enforcement
- Include extensive unit test coverage for all new services
- Introduce interactive schedule editing with conflict detection and audit trail support
- Add RecentSelections component for quick clinician and rotation access
- Extract scheduling logic into composables (management, display, permissions, state updates)
- Implement InstructorScheduleService with CRUD operations and backend integration
- Enhance ClinicianScheduleView and RotationScheduleView with interactive features
- Add primary evaluator management and permission-based editing controls
- Reorganize styles (site.css → styles/index.css) with centralized brand color system
- Improve navigation with parameterized route handling and active state fixes
- Standardize error handling with consistent user feedback
- Also enabling detailed logging for all databases
- Split monolithic RotationSelector test into focused modules for
  better maintainability
- Fix code formatting across Vue components to match project
  ESLint standards
- Reorganize ClinicalScheduler services with kebab-case naming
  convention
- Consolidate type definitions into dedicated modules for better
  organization
- Update composables structure for improved reusability
…lector

- Add TypeScript support for .ts files in ESLint config
- Improve service response handling for affiliate mode
- Fix duplicate CSS properties and add modern Object.hasOwn
- Configure TypeScript target to ES2022 for modern features
- Improve backend clinician controller name handling
@rlorenzo rlorenzo force-pushed the clinical-scheduler-ui branch from 3097f57 to 57f4f4e Compare September 2, 2025 16:13
- Updated verify build script to perform same type checking as in
  Jenkins
@rlorenzo rlorenzo force-pushed the clinical-scheduler-ui branch from 7e00626 to 39a7ade Compare September 2, 2025 17:36
- Added running tests for precommit hook
- Added VueApp tests to Jenkins file
- Added backend support for fine-grain permissions
- Added email notifications for primary evaluator changes
- Filter rotations/clinicians in UI depending on rotation access or can
  edit your own schedule
- Introduce permissions store (actions/helpers/utils) plus constants/messages
- Send notification on primary evaluator removal in ScheduleEditService
- Centralize UC Davis colors; preload CSS vars via ucdavis-colors.js to prevent FOUC
- Added related frontend/backend tests
},
"EmailNotifications": {
"PrimaryEvaluatorRemoved": {
"To": ["[email protected]"],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsedwards, I am unsure if it is okay to put the email address in here on a public repo. Is there a way to inject the config into the Jenkins build?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add config to the AWS parameter store, either as single parameters, or json that could be deserialized into a settings object.

- Also fixing "dev:stop" to not error if a process cannot be stopped
permissions

- Also added email notification tests
- Fixed error with build-cache.js in which modified file times were not
  properly being handled
…lidation

- Consolidate ClinicalScheduleSecurityService into SchedulePermissionService
- Add ClinicalSchedulePermissions constants class with XML documentation
- Update CTS controller to use unified permission service with async pattern
- Remove duplicate permission service interfaces and registrations
…nPolicyService to instance-based

- Move request/response classes from controllers to separate DTO files
- Add entity-to-DTO mapping extensions for clean separation
- Convert EvaluationPolicyService from static to dependency injection
- Add comprehensive unit tests for validation logic
…oring

- Extract validation logic into dedicated validator classes
- Split ClinicalScheduleService into focused service interfaces
- Move AAUD data access from controllers to PersonService
- Replace anonymous objects with proper DTOs throughout
- Remove direct DbContext usage from all controllers
…ring

- Split the schedule store into modular files (state, actions, cache)
- Created specialized composables for CRUD, dialogs, notifications, and
  operations
- Extracted PermissionFeedbackChip and WeekCell as reusable components
- Moved component-specific styles to scoped blocks, cleaned up shared CSS
- Improve the responsiveness of the sticky "Recents" box
- Refactor schedule view components to eliminate duplication and improve
  maintainability
- Add comprehensive test coverage for normalization, permissions, and
  optimistic updates
- Adjusting linting rules and added oxlint-tsgolint
- Ignoring precommit hook on rebases, since rebases should just to used to
  change commit messages or reorder code. No new code should be added.
@rlorenzo rlorenzo force-pushed the clinical-scheduler-ui branch from f70534e to 98d68c0 Compare September 9, 2025 10:06
- Add 25 comprehensive integration tests covering permission flows and service interactions
- Extend TestDataBuilder with IntegrationHelpers for complex test scenarios
- Add DTO mapping unit tests and enhance controller test coverage significantly
- Remove obsolete StudentSchedule entity and clean up model references
- Refactor error transformer with improved HTTP status handling and cleaner structure
- Enhance build verification with better output formatting and test compilation checks
// Get antiforgery token from user store
const userStore = useUserStore()
const token = userStore.userInfo.token || ""
addHeader(options, "X-CSRF-TOKEN", token)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsedwards FYI: I noticed that the CSRF token was not being set by the frontend. This is a security patch.

… notifications

- Send email notifications when primary evaluators are changed or removed
- Simplify error handling across all Clinical Scheduler controllers
- Add tracking IDs to errors for easier troubleshooting
- Adjust build scripts to ensure email service start for dev:build
- Update Node.js from 20.6.1 to 20.19.4 (npm 9.8.1 →
  10.8.2)
- Upgrade Vite from 6.3.5 to 7.1.5 for improved build
  performance
- Update @vitejs/plugin-vue from 5.2.4 to 6.0.1 for
  Vite 7 compatibility
- Upgrade Quasar from 2.16.11 to 2.18.2
- Update Vitest from 2.0.0 to 3.2.4
- Update all other packages to latest compatible
  versions
- Configure Volta and Jenkins to use Node.js 20.19.4
   multiple clinicians and rotations

- Enable multi-select mode for clinicians and rotations in
  schedule views
- Add loading indicators per week during bulk operations
- Show selection count badge and helper text for bulk
  actions
- Batch conflict checks and display single confirmation
  dialog
- Fix email notification name format to use (LastName,
  FirstName)
- Suppress build warnings in test:backend script for
  cleaner output
- Add Alt/Option+click range selection for scheduling multiple weeks at
  once
- Implement mobile-friendly long-press selection for touch devices
- Add visual selection indicators and "Schedule Selected" confirmation UI
- Enhance accessibility with ARIA attributes and keyboard navigation
  support
- Changed oxlint to ignore linting .vue files
Copy link
Collaborator

@bsedwards bsedwards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple things:

  • Rotation and clinician selects could use options-dense property
  • I think the chip below the rotation select saying "Showing X of X items" can be removed
  • RotationsController - I didn't see permission checking at first glance. Maybe it's happening in the functions that build the schedule.
  • For clinicians that can only edit their own schedule:
  • Don't show the access descriptions
  • Change Schedule by Clinician to "Edit My Schedule" or similar
  • Prefill the clinician select, or replace with static text with their name
  • Editing their own schedule is failing
  • For service editors: Don't show the limited access message
  • Remove the confirmation dialog when removing a clinician or rotation from the schedule (maybe keep if they're the primary)
  • Primary evaluator rules are excluding extended rotation weeks

- Add bulk deletion for both rotation and clinician schedule views
  if you select weeks that already contain the given clinicians or
  rotations
- Implement visual animations for add/remove operations
- Add primary evaluator removal warnings with specific instructor and week information
- Standardize notification timeouts across success, warning, and error states
- Updated tests
- Added dense-options to dropdowns
- Removed permission notices/labels
- Showing "Edit My Schedule" for users with
  EditOwnSchedule permission
- Removing "Make Primary Evaluator" checkbox
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants