Sep dept head form by sbagg · Pull Request #140 · UCDavisLibrary/ucdlib-iam-support

sbagg · 2026-07-13T07:26:13Z

Copilot

Pull request overview

Adds support for reassigning a department head during an employee separation workflow (UI capture/display plus backend/CLI deprovision handling), and extends “direct reports” fetching to optionally target a specific supervisor IAM ID.

Changes:

Adds UI for selecting and displaying a “New Department Head” on separation create/single pages.
Introduces models.admin.separateDepartmentHead(separationId) and calls it during deprovision (API + CLI).
Updates the direct-reports API/client plumbing to accept an optional iamId query parameter.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
services/lib/models/admin.js	Adds `separateDepartmentHead` and group head reassignment logic
services/lib/cork/services/EmployeeService.js	Adds `iamId` parameter support for direct reports requests
services/lib/cork/models/EmployeeModel.js	Propagates optional `iamId` argument + updates docs
services/cli/lib/employees.js	Calls `separateDepartmentHead` during CLI separation flow
services/app/client/src/elements/pages/ucdlib-iam-page-separation-single.tpl.js	Displays “New Department Head” on separation detail
services/app/client/src/elements/pages/ucdlib-iam-page-separation-single.js	Fetches/display data for the new department head
services/app/client/src/elements/pages/ucdlib-iam-page-separation-new.tpl.js	Conditionally shows employee search for new dept head
services/app/client/src/elements/pages/ucdlib-iam-page-separation-new.js	Captures/validates new dept head selection and persists to additionalData
services/app/api/separation.js	Calls `separateDepartmentHead` during deprovision
services/app/api/employees.js	Allows optional query override for direct reports IAM id

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

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

services/app/client/src/elements/pages/ucdlib-iam-page-separation-new.js:145

When the selected employee is cleared (or changed), isDepartmentHead / employeeNewHead are not reset. This can leave the "New Department Head" field visible (and/or a stale employeeNewHead value) from a previous selection, which breaks the conditional UI and submit validation.

  _onEmployeeStatusChange(e){
    if ( e.detail.employee ){
      this.employeeRecord = e.detail.employee;
      this._isDepartmentHead();
    } else {
      this.employeeRecord = {};
    }
  }

Copilot

Pull request overview

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

Comments suppressed due to low confidence (3)

services/lib/cork/models/EmployeeModel.js:28

The updated JSDoc for getDirectReports() is inaccurate: the method does not accept a supervisor IAM id parameter and it returns the Cork store record (via EmployeeService.getDirectReports()), not an Array<Object>. This can mislead callers and future maintenance.

  /**
    * @description Returns all direct reports for the current user (or for the provided supervisor IAM id)
    * @returns {Array<Object>} direct report employee records
   */
  getDirectReports(){
    return this.service.getDirectReports();
  }

services/app/api/separation.js:48

Server-side validation for department-head separations currently only checks that additionalData.newDepartmentHead is present, but it does not verify that the selected employee exists in the local DB and is actually in the same department where the separating employee is head. This allows invalid newDepartmentHead values to be stored and later silently skipped during deprovisioning, which conflicts with the requirement that the new head must be selected and in the respective department.

      //if the employee is a department head, add the department head info to the separation request
      const isDepartmentHead = payload.additionalData.employeeRecord.groups.some(g => g.type === 'Department' && g.isHead === true);
      if ( isDepartmentHead ) {
        const newHeadId = payload.additionalData?.newDepartmentHead;
        if ( !newHeadId ) {
          res.status(400).json({error: true, message: 'Missing new department head id for department head separation or selected head was not in the same department as the separated employee.'});
          return;
        }
      }

services/app/api/separation.js:292

The deprovision endpoint logs departmentSeparationResult to stdout. This can add noisy logs and may expose internal details/identifiers in production logs. Prefer structured logging behind an explicit debug flag, or remove this line.

      // separate department head if applicable
      const departmentSeparationResult = await models.admin.separateDepartmentHead(separationId);
      console.log('departmentSeparationResult', departmentSeparationResult);
      if ( departmentSeparationResult.error ) {

spelkey-ucd

In addition to the comments, please add a way for the submitter to bypass the department head checks.

Put a checkbox below the lookup tool with the label and description:

Do not assign new/interim head
If you do not assign a new head now, you will need to contact ITIS as soon as the information becomes available.

And then make a new CLI command:
separation replace-department-head <separation-id> <new-department-head>

which will set the appropriate data in the separation record

spelkey-ucd · 2026-07-29T14:41:04Z

   * Returns blank array if no reports
   */
  api.get('/employees/direct-reports', async (req, res) => {
-    const iamId = req.auth.token.iamId;


looks like all changes in this file slipped in from another issue. please revert.

spelkey-ucd · 2026-07-29T14:55:17Z

when you merged sandbox into your feature branch, you threw out several legitimate updates. please fix.

spelkey-ucd · 2026-07-29T15:15:18Z

+      if ( isDepartmentHead ) {
+        const newHeadId = payload.additionalData?.newDepartmentHead;
+        if ( !newHeadId ) {
+          res.status(400).json({error: true, message: 'Missing new department head id for department head separation or selected head was not in the same department as the separated employee.'});


you dont actually validate that the new department head is in the department

spelkey-ucd · 2026-07-29T15:24:29Z

+    if ( !departmentRecord.res.rows.length ) {
+      return {error: true, message: `No department record found for id ${currentDepartmentId}`};
+    }
+    const currentDepartment = departmentRecord.res.rows;


should be departmentRecord.res.rows[0]. Then you don't have to do this flatMap stuff

spelkey-ucd · 2026-07-29T15:45:00Z

+   * @description Checks if the employee is a department head
+   * @returns {boolean}
+   */
+  async _isDepartmentHead(){


isDepartmentHead depends solely on employeeRecord. A better pattern is to hook onto willUpdate. That way you always know it is never stale.

spelkey-ucd · 2026-07-29T16:05:56Z

    additionalData.employeeId = this.employeeId;
    additionalData.employeeUserId = this.userId;
+    this.employeeNewHead && this.employeeNewHead.iamId 
+      ? additionalData.newDepartmentHead = this.employeeNewHead.iamId : delete additionalData.newDepartmentHead;


this is a weird pattern.

additionalData.newDepartmentHead = this.employeeNewHead?.iamId || null would be much simpler, yes?

spelkey-ucd · 2026-07-29T16:10:08Z

              <input id='sp-separation-date' type="date" required .value=${this.separationDate} @input=${(e) => {this.separationDate = e.target.value;}}>
            </div>
+            <div class="field-container" ?hidden=${!this.isDepartmentHead}>
+              <label for="sp-department-head">New Department Head <abbr title="Required">*</abbr></label>


change label to "New or Interim Department Head"

add a blurb that the user must be in the same department

spelkey-ucd · 2026-07-29T16:21:33Z

+       this.employeeNewHead = null;
+       return;
+    }
+    const r = await this.EmployeeModel.get(iamId, "iamId");


this will fail if:

the new department head leaves the library, so when you go to historical separation requests there is a chance you will get an error bar when nothing is actually wrong.

If the user does not have admin or hr access (aka for the supervisor)

instead of doing an api call, set some additional properties on the separation object with the new department head's name in the POST api endpoint.

spelkey-ucd · 2026-07-29T16:23:51Z

      utils.logObject(keycloakUser);
    }

+    // separate department head if applicable


this is a local database action. so it should be under options.rm

Sabrina Baggett added 4 commits June 18, 2026 10:36

add direct report section #127

8fc9372

requested change from copilot

3a11d67

stash

a5749d2

initial ticket update

26d377b

sbagg self-assigned this Jul 13, 2026

sbagg requested a review from Copilot July 13, 2026 07:26

Copilot started reviewing on behalf of sbagg July 13, 2026 07:26 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

suggested copilot changes

bf58039

sbagg requested a review from Copilot July 13, 2026 18:36

Copilot started reviewing on behalf of sbagg July 13, 2026 18:37 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Sabrina Baggett added 4 commits July 22, 2026 11:54

changes added

43717d4

merge changes

f833327

merge conflicts solve and changes copilot

bc31ec5

uncomment message

91128c4

sbagg requested a review from Copilot July 22, 2026 19:13

Copilot started reviewing on behalf of sbagg July 22, 2026 19:13 View session

Copilot AI reviewed Jul 22, 2026

View reviewed changes

Comment thread services/app/client/src/elements/pages/ucdlib-iam-page-separation-new.tpl.js

Comment thread services/app/client/src/elements/pages/ucdlib-iam-page-separation-single.js

Comment thread services/lib/models/admin.js

Comment thread services/lib/models/admin.js Outdated

changes for copilot

f8283de

sbagg requested a review from spelkey-ucd July 22, 2026 19:47

created single transaction

c99cb3e

spelkey-ucd requested changes Jul 29, 2026

View reviewed changes

Uh oh!

Conversation

sbagg commented Jul 13, 2026

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spelkey-ucd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants