VPR-20 - Manual instructor import, edit and delete #89

rlorenzo · 2025-12-24T19:33:19Z

No description provided.

- Add instructor list/detail pages with search, create, edit, delete - Enforce department-level authorization on search and create - Add server-side search term validation (min 2 chars) - Include InstructorService with AAUD integration for dept/title lookup

VueApp/src/Effort/__tests__/instructor-edit-dialog.test.ts

+
+        it("should handle null report units from API", () => {
+            const apiReportUnit: string | null = null
+            const reportUnits = apiReportUnit ? apiReportUnit.split(",") : []


web/Areas/Effort/Services/InstructorService.cs

+        foreach (var instructor in instructors)
+        {
+            if (string.IsNullOrWhiteSpace(instructor.EffortTitleCode)) continue;
+
+            var paddedCode = instructor.EffortTitleCode.PadLeft(6, '0');
+            if (titleLookup.TryGetValue(paddedCode, out var title))
+            {
+                instructor.Title = title;
+            }
+        }


Copilot

Pull request overview

This PR implements manual instructor management functionality for the Effort system, allowing authorized users to import, edit, and delete instructors for specific terms.

Key Changes:

Backend service layer with full CRUD operations for instructors, including search, department resolution, and audit logging
REST API endpoints with authorization checks based on department access and permissions
Vue.js UI components for instructor list, detail view, and add/edit dialogs with comprehensive error handling

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
web/Program.cs	Registers InstructorService in DI container
web/Areas/Effort/Services/InstructorService.cs	Core service implementing instructor CRUD operations with department determination logic and title lookup
web/Areas/Effort/Services/IInstructorService.cs	Service interface defining instructor management contracts
web/Areas/Effort/Services/EffortAuditService.cs	Adds AddPersonChangeAudit method for instructor audit logging
web/Areas/Effort/Services/IEffortAuditService.cs	Interface update for person change auditing
web/Areas/Effort/Controllers/InstructorsController.cs	API controller with authorization, department filtering, and error handling
web/Areas/Effort/Models/DTOs/Requests/*.cs	Request DTOs for creating and updating instructors with validation attributes
web/Areas/Effort/Models/DTOs/Responses/*.cs	Response DTOs for instructor data, departments, report units, and AAUD person search
web/Areas/Effort/Models/AutoMapperProfileEffort.cs	Mapping configuration for VolunteerWos byte-to-bool conversion
test/Effort/InstructorServiceTests.cs	Unit tests for service layer operations
test/Effort/InstructorsControllerTests.cs	Unit tests for API controller endpoints
VueApp/src/Effort/types/index.ts	TypeScript type definitions for instructor-related DTOs
VueApp/src/Effort/services/effort-service.ts	Frontend API client methods for instructor operations
VueApp/src/Effort/router/routes.ts	Routes for instructor list and detail pages
VueApp/src/Effort/pages/InstructorList.vue	Instructor list UI with filtering, grouping by department, and action buttons
VueApp/src/Effort/pages/InstructorDetail.vue	Instructor detail page showing effort records
VueApp/src/Effort/components/InstructorAddDialog.vue	Dialog for searching and adding instructors with autocomplete
VueApp/src/Effort/components/InstructorEditDialog.vue	Dialog for editing instructor details with department and report unit selection
VueApp/src/Effort/components/EffortLeftNav.vue	Navigation update to include instructor link
VueApp/src/Effort/composables/use-effort-permissions.ts	Permission composable additions for instructor operations
VueApp/src/Effort/tests/*.test.ts	Frontend unit tests for dialog error handling and state management

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

web/Areas/Effort/Controllers/InstructorsController.cs

web/Areas/Effort/Models/DTOs/Requests/UpdateInstructorRequest.cs

Copilot · 2025-12-24T19:37:45Z

web/Areas/Effort/Models/DTOs/Responses/PersonDto.cs

-    public string FullName => string.IsNullOrEmpty(MiddleInitial)
-        ? $"{LastName}, {FirstName}"
-        : $"{LastName}, {FirstName} {MiddleInitial}.";
+    public string FullName => $"{LastName}, {FirstName}";


The FullName property no longer includes the middle initial, which is a breaking change from the original implementation. The original code showed "LastName, FirstName M." when a middle initial was present. This inconsistency could affect display logic and user expectations throughout the application, especially since AaudPersonDto still includes middle initials in its FullName formatting. Consider keeping the original logic or documenting this intentional change.

Suggested change

public string FullName => $"{LastName}, {FirstName}";

public string FullName => string.IsNullOrWhiteSpace(MiddleInitial)

? $"{LastName}, {FirstName}"

: $"{LastName}, {FirstName} {MiddleInitial}.";

Copilot · 2025-12-24T19:37:45Z

web/Areas/Effort/Services/InstructorService.cs

+        instructor.ReportUnit = request.ReportUnits != null && request.ReportUnits.Count > 0
+            ? string.Join(",", request.ReportUnits)
+            : null;
+        instructor.VolunteerWos = request.VolunteerWos ? (byte)1 : null;


The VolunteerWos database field is stored as byte? where 1=true, but the UpdateInstructorAsync method sets it as (byte)1 when true and null when false. This means there's no way to represent a false value (0) versus an unset value (null), which could lead to data loss when updating an instructor who was previously marked as non-volunteer. Consider using 0 for false instead of null to preserve the three-state distinction, or document why null is the intended representation for false.

Suggested change

instructor.VolunteerWos = request.VolunteerWos ? (byte)1 : null;

instructor.VolunteerWos = request.VolunteerWos ? (byte)1 : (byte)0;

Copilot · 2025-12-24T19:37:46Z

web/Areas/Effort/Services/InstructorService.cs

+    public async Task<List<AaudPersonDto>> SearchPossibleInstructorsAsync(int termCode, string? searchTerm = null, CancellationToken ct = default)
+    {
+        // Enforce minimum search length server-side to prevent employee enumeration
+        if (string.IsNullOrWhiteSpace(searchTerm) || searchTerm.Trim().Length < 2)
+        {
+            return [];
+        }


The search enforces a minimum of 2 characters server-side to prevent employee enumeration, but the frontend InstructorAddDialog also enforces this check client-side. However, the error case where searchTerm has fewer than 2 characters returns an empty array without any indication to the user that their input was too short. While the UI template shows "Type at least 2 characters to search", this behavior could be confusing if the backend validation changes. Consider adding a more explicit validation response or documented contract between frontend and backend.

Copilot · 2025-12-24T19:37:46Z

web/Areas/Effort/Models/AutoMapperProfileEffort.cs

+        // VolunteerWos: byte? (1=true) → bool
+        CreateMap<EffortPerson, PersonDto>()
+            .ForMember(d => d.VolunteerWos, opt => opt.MapFrom(s => s.VolunteerWos == 1));


The AutoMapper configuration maps VolunteerWos from byte? to bool using the expression "s.VolunteerWos == 1", but this only handles the true case. When VolunteerWos is 0 or null, both will map to false, which loses the distinction between "explicitly set to false" (0) and "never set" (null). This is inconsistent with how UpdateInstructorAsync sets the value (1 for true, null for false). The mapping should handle all three states consistently, or the database schema should be clarified.

Suggested change

// VolunteerWos: byte? (1=true) → bool

CreateMap<EffortPerson, PersonDto>()

.ForMember(d => d.VolunteerWos, opt => opt.MapFrom(s => s.VolunteerWos == 1));

// VolunteerWos: byte? (1=true, 0=false, null=unset) → bool?

CreateMap<EffortPerson, PersonDto>()

.ForMember(

d => d.VolunteerWos,

opt => opt.MapFrom(s =>

s.VolunteerWos == 1 ? (bool?)true :

s.VolunteerWos == 0 ? (bool?)false :

(bool?)null

)

);

Copilot · 2025-12-24T19:37:46Z

web/Areas/Effort/Controllers/InstructorsController.cs

+            if (string.IsNullOrEmpty(dept))
+            {
+                var allInstructors = new List<PersonDto>();
+                foreach (var authorizedDept in authorizedDepts)
+                {
+                    var deptInstructors = await _instructorService.GetInstructorsAsync(termCode, authorizedDept, ct);
+                    allInstructors.AddRange(deptInstructors);
+                }
+                return Ok(allInstructors.OrderBy(i => i.LastName).ThenBy(i => i.FirstName));


The GetInstructorsAsync method when called without department filter for non-full-access users loops through authorized departments and makes individual database queries for each department. For users with many authorized departments, this creates an N+1 query problem. Consider using a single query with an IN clause on the department codes instead of multiple sequential queries to improve performance.

Copilot · 2025-12-24T19:37:47Z

web/Areas/Effort/Services/InstructorService.cs

+        foreach (var instructor in instructors)
+        {
+            if (string.IsNullOrWhiteSpace(instructor.EffortTitleCode)) continue;
+
+            var paddedCode = instructor.EffortTitleCode.PadLeft(6, '0');
+            if (titleLookup.TryGetValue(paddedCode, out var title))
+            {
+                instructor.Title = title;
+            }
+        }


This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

Copilot · 2025-12-24T19:37:47Z

VueApp/src/Effort/__tests__/instructor-edit-dialog.test.ts

+
+        it("should handle null report units from API", () => {
+            const apiReportUnit: string | null = null
+            const reportUnits = apiReportUnit ? apiReportUnit.split(",") : []


This guard always evaluates to false.

Copilot AI review requested due to automatic review settings December 24, 2025 19:33

rlorenzo changed the base branch from main to VPR-19-effort-course-relationships December 24, 2025 19:33

Copilot started reviewing on behalf of rlorenzo December 24, 2025 19:33 View session

github-code-quality bot found potential problems Dec 24, 2025

View reviewed changes

VueApp/src/Effort/__tests__/instructor-edit-dialog.test.ts

it("should handle null report units from API", () => {

const apiReportUnit: string | null = null

const reportUnits = apiReportUnit ? apiReportUnit.split(",") : []

github-code-quality bot found potential problems Dec 24, 2025

View reviewed changes

Copilot AI reviewed Dec 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

VPR-20 - Manual instructor import, edit and delete #89

VPR-20 - Manual instructor import, edit and delete #89

Uh oh!

rlorenzo commented Dec 24, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 24, 2025

Uh oh!

Copilot AI Dec 24, 2025

Uh oh!

Copilot AI Dec 24, 2025

Uh oh!

Copilot AI Dec 24, 2025

Uh oh!

Copilot AI Dec 24, 2025

Uh oh!

Copilot AI Dec 24, 2025

Uh oh!

Copilot AI Dec 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	instructor.VolunteerWos = request.VolunteerWos ? (byte)1 : null;
	instructor.VolunteerWos = request.VolunteerWos ? (byte)1 : (byte)0;

VPR-20 - Manual instructor import, edit and delete #89

Are you sure you want to change the base?

VPR-20 - Manual instructor import, edit and delete #89

Uh oh!

Conversation

rlorenzo commented Dec 24, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants