VPR-64 feat(phone): schoolwide and unit phone lists by bniedzie · Pull Request #323 · ucdavis/VIPER

bniedzie · 2026-08-25T19:05:54Z

This PR migrates the schoolwide and Dean's Office phone lists from Viper 1. Viewing the lists requires only basic permissions, while specific permissions allow users to edit and maintain the lists. The lists are now housed in the new Personnel area.

The migration makes the following functional changes from the Viper 1 version:

Rather than using the PhoneList database, creates a phones schema in the VIPER database.
The database structure is different, normalizing the existing data. Phone is now stored at a person level, meaning that the same person will have the same data every time they occur across lists. Name data now comes from the user.Person table, which more easily handles name changes. The primary person identifier is now IAM ID rather than Mothra ID, per an initial meeting with Brandon.
Change history is now tracked, rather than showing only the latest changes. This fixes a bug where deletions would not affect the last modified date shown to users.
A new SVMSecure.PhoneLists.SVMMaintain permission guards editing the SVM list. In Viper 1, anyone with the link could make changes.
Removed supervisor from the VMDO data, as it is not surfaced anywhere. This can be pulled from existing UCPath data if needed at a later date.
Generalized the VMDO table to support the creation of arbitrary department/unit phone lists. These are keyed by a code (e.g., VMDO), allowing the list name to change without breaking links. Each allows separate permissions. VMDO is the only list currently present.
Changed the general Phone List view to display direct numbers to those with list maintenance permissions as well. Previously, these users could view the data on the maintenance page only.
Fixed a broken link in the general Phone List view.
Changed phone lists to be filterable and (for relevant fields) sortable.

This PR also does some refactoring around Person selection and dialog boxes. There should be no end user impact to CMS, but a few files are affected.

This PR requires schema changes to the Production database:

CREATE SCHEMA phones;


CREATE TABLE [phones].[Person](
	[PersonIam] [VARCHAR](10) NOT NULL,
	[Phone] [NVARCHAR](25) NOT NULL,
	[ModifiedDate] [DATETIME] NULL,
	[ModifiedBy] [VARCHAR](10) NULL,
	[DirectPhone] [NVARCHAR](25) NULL,
	[Office] [NVARCHAR](100) NULL,
 CONSTRAINT [PK_Person] PRIMARY KEY CLUSTERED 
(
	[PersonIam] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY];


CREATE TABLE [phones].[PhoneList](
	[PhoneListId] [INT] IDENTITY(1,1) NOT NULL,
	[Name] [NVARCHAR](100) NOT NULL,
	[MaintainRole] [VARCHAR](100) NOT NULL,
	[Code] [NVARCHAR](20) NOT NULL,
 CONSTRAINT [PK_PhoneList] PRIMARY KEY CLUSTERED 
(
	[PhoneListId] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY];

CREATE UNIQUE NONCLUSTERED INDEX [UX_PhoneList_Code] ON [phones].[PhoneList]
(
	[Code] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, SORT_IN_TEMPDB = OFF, IGNORE_DUP_KEY = OFF, DROP_EXISTING = OFF, ONLINE = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY];


CREATE TABLE [phones].[PhoneListUnit](
	[PhoneListUnitId] [INT] IDENTITY(1,1) NOT NULL,
	[PhoneListId] [INT] NOT NULL,
	[Name] [NVARCHAR](100) NOT NULL,
	[SortOrder] [INT] NULL,
 CONSTRAINT [PK_PhoneListUnit] PRIMARY KEY CLUSTERED 
(
	[PhoneListUnitId] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY];

ALTER TABLE [phones].[PhoneListUnit]  WITH CHECK ADD  CONSTRAINT [FK_PhoneListUnit_PhoneListId] FOREIGN KEY([PhoneListId])
REFERENCES [phones].[PhoneList] ([PhoneListId]);
ALTER TABLE [phones].[PhoneListUnit] CHECK CONSTRAINT [FK_PhoneListUnit_PhoneListId];


CREATE TABLE [phones].[PhoneListUnitPerson](
	[PhoneListUnitPersonId] [INT] IDENTITY(1,1) NOT NULL,
	[PhoneListUnitId] [INT] NOT NULL,
	[PersonIam] [VARCHAR](10) NOT NULL,
	[ListFirst] [BIT] NOT NULL,
	[IsActive] [BIT] NOT NULL,
	[ModifiedBy] [VARCHAR](10) NULL,
	[ModifiedDate] [DATETIME] NULL,
 CONSTRAINT [PK_PhoneListUnitPerson] PRIMARY KEY CLUSTERED 
(
	[PhoneListUnitPersonId] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY];

ALTER TABLE [phones].[PhoneListUnitPerson] ADD  DEFAULT ((0)) FOR [ListFirst];
ALTER TABLE [phones].[PhoneListUnitPerson] ADD  DEFAULT ((1)) FOR [IsActive];
ALTER TABLE [phones].[PhoneListUnitPerson]  WITH CHECK ADD  CONSTRAINT [FK_PhoneListUnitPerson_PersonIam] FOREIGN KEY([PersonIam])
REFERENCES [phones].[Person] ([PersonIam]);
ALTER TABLE [phones].[PhoneListUnitPerson] CHECK CONSTRAINT [FK_PhoneListUnitPerson_PersonIam];
ALTER TABLE [phones].[PhoneListUnitPerson]  WITH CHECK ADD  CONSTRAINT [FK_PhoneListUnitPerson_PhoneListUnitId] FOREIGN KEY([PhoneListUnitId])
REFERENCES [phones].[PhoneListUnit] ([PhoneListUnitId]);
ALTER TABLE [phones].[PhoneListUnitPerson] CHECK CONSTRAINT [FK_PhoneListUnitPerson_PhoneListUnitId];


CREATE TABLE [phones].[SVMSection](
	[SectionId] [INT] IDENTITY(1,1) NOT NULL,
	[Name] [NVARCHAR](100) NULL,
	[IncludeAbbrv] [BIT] NOT NULL,
	[DirectorTitle] [NVARCHAR](50) NOT NULL,
	[SortOrder] [INT] NULL,
	[UnitName] [NVARCHAR](50) NULL,
 CONSTRAINT [PK_SVMSection] PRIMARY KEY CLUSTERED 
(
	[SectionId] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY];


CREATE TABLE [phones].[SVMUnit](
	[UnitId] [INT] IDENTITY(1,1) NOT NULL,
	[SectionId] [INT] NOT NULL,
	[Name] [NVARCHAR](100) NOT NULL,
	[Abbrv] [NVARCHAR](20) NULL,
	[SortOrder] [INT] NULL,
	[Fax] [NVARCHAR](25) NULL,
	[ModifiedBy] [VARCHAR](10) NULL,
	[ModifiedDate] [DATETIME] NULL,
 CONSTRAINT [PK_SVMUnit] PRIMARY KEY CLUSTERED 
(
	[UnitId] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY];

ALTER TABLE [phones].[SVMUnit]  WITH CHECK ADD  CONSTRAINT [FK_SVMUnit_SectionId] FOREIGN KEY([SectionId])
REFERENCES [phones].[SVMSection] ([SectionId]);
ALTER TABLE [phones].[SVMUnit] CHECK CONSTRAINT [FK_SVMUnit_SectionId];


CREATE TABLE [phones].[SVMUnitPerson](
	[UnitPersonId] [INT] IDENTITY(1,1) NOT NULL,
	[UnitId] [INT] NOT NULL,
	[PersonIam] [VARCHAR](10) NOT NULL,
	[ModifiedDate] [DATETIME] NULL,
	[ModifiedBy] [VARCHAR](10) NULL,
	[Office] [NVARCHAR](50) NULL,
	[PosType] [NVARCHAR](25) NULL,
	[Interim] [NVARCHAR](10) NULL,
	[IsActive] [BIT] NOT NULL,
 CONSTRAINT [PK_SVMUnitPerson] PRIMARY KEY CLUSTERED 
(
	[UnitPersonId] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY];

ALTER TABLE [phones].[SVMUnitPerson] ADD  DEFAULT ((1)) FOR [IsActive];
ALTER TABLE [phones].[SVMUnitPerson]  WITH CHECK ADD  CONSTRAINT [FK_SVMUnitPerson_PersonId] FOREIGN KEY([PersonIam])
REFERENCES [phones].[Person] ([PersonIam]);
ALTER TABLE [phones].[SVMUnitPerson] CHECK CONSTRAINT [FK_SVMUnitPerson_PersonId];
ALTER TABLE [phones].[SVMUnitPerson]  WITH CHECK ADD  CONSTRAINT [FK_SVMUnitPerson_UnitId] FOREIGN KEY([UnitId])
REFERENCES [phones].[SVMUnit] ([UnitId]);
ALTER TABLE [phones].[SVMUnitPerson] CHECK CONSTRAINT [FK_SVMUnitPerson_UnitId];


CREATE TABLE [phones].[SVMFrequentNumber](
	[NumberID] [INT] IDENTITY(1,1) NOT NULL,
	[Label] [NVARCHAR](100) NOT NULL,
	[Phone] [NVARCHAR](25) NOT NULL,
	[SortOrder] [INT] NULL,
	[ModifiedBy] [VARCHAR](10) NULL,
	[ModifiedDate] [DATETIME] NULL,
	[IsActive] [BIT] NOT NULL,
 CONSTRAINT [PK_SVMFrequentNumber] PRIMARY KEY CLUSTERED 
(
	[NumberID] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY];

ALTER TABLE [phones].[SVMFrequentNumber] ADD  DEFAULT ((1)) FOR [IsActive];

This PR requires creating a new permission on Production: SVMSecure.PhoneLists.SVMMaintain.

This PR requires running the migration script .\RunMigrateData.bat Production for a dry run, and then .\RunMigrateData.bat Production --apply to migrate data into the new schema.

This PR will require a change to Viper 1 redirecting two Personnel left nav items and adding the SVMMaintain permission check to one.

codecov-commenter · 2026-08-25T19:07:14Z

Bundle Report

Changes will increase total bundle size by 50.13kB (2.19%) ⬆️. This is within the configured threshold ✅

Detailed changes

Bundle name	Size	Change
viper-frontend-esm	2.34MB	50.13kB (2.19%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: viper-frontend-esm

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`assets/GenericError-*.css`	164 bytes	208.06kB	0.08%
`assets/GenericError-*.js`	13.04kB	58.03kB	28.99% ⚠️
`assets/schedule-*.js`	8 bytes	55.03kB	0.01%
`assets/SortableList-*.js`	8 bytes	45.54kB	0.02%
`assets/PhotoGallery-*.js`	8 bytes	36.16kB	0.02%
`assets/InstructorList-*.js`	-32 bytes	26.04kB	-0.12%
`assets/effort-*.js`	-5 bytes	23.76kB	-0.02%
`assets/Files-*.js`	-1.77kB	20.8kB	-7.86%
`assets/ContentBlockEdit-*.js`	-32 bytes	20.74kB	-0.15%
`assets/MultiYearReport-*.js`	8 bytes	18.81kB	0.04%
`assets/EmergencyContactForm-*.js`	-32 bytes	17.2kB	-0.19%
`assets/CmsHome-*.js`	8 bytes	11.43kB	0.07%
`assets/ViperFetch-*.js`	53 bytes	11.22kB	0.47%
*`assets/SVMPhonesMaintain-.js`** (New)	10.6kB	10.6kB	100.0% 🚀
*`assets/SVMFrequentNumberTable-.js`** (New)	10.15kB	10.15kB	100.0% 🚀
`assets/ReportFilterForm-*.js`	8 bytes	8.87kB	0.09%
*`assets/RecordActionCell-.js`** (New)	5.92kB	5.92kB	100.0% 🚀
*`assets/PhoneListMaintain-.js`** (New)	5.61kB	5.61kB	100.0% 🚀
`assets/ClinicalEffort-*.js`	8 bytes	5.33kB	0.15%
*`assets/PhoneListUnitTable-.js`** (New)	5.01kB	5.01kB	100.0% 🚀
`assets/ManageCourseCompetencies-*.js`	3 bytes	3.71kB	0.08%
*`assets/RecordFormDialog-.js`** (New)	3.69kB	3.69kB	100.0% 🚀
`assets/students-*.js`	-5 bytes	3.5kB	-0.14%
~~*`assets/dist-.js`*~~ (Deleted)*	-11.57kB	0 bytes	-100.0% 🗑️
`assets/ExportToolbar-*.js`	8 bytes	2.95kB	0.27%
*`assets/SVMPhones-.js`** (New)	2.65kB	2.65kB	100.0% 🚀
*`assets/PhoneList-.js`** (New)	2.12kB	2.12kB	100.0% 🚀
*`assets/personnel-.js`** (New)	2.05kB	2.05kB	100.0% 🚀
*`assets/PersonSelector-.js`** (New)	1.79kB	1.79kB	100.0% 🚀
*`assets/CmsContent-.js`** (New)	462 bytes	462 bytes	100.0% 🚀
`assets/Home-*.js`	-343 bytes	221 bytes	-60.82%
*`assets/Home-.js`** (New)	221 bytes	221 bytes	100.0% 🚀
*`assets/RecordActionCell-.css`** (New)	215 bytes	215 bytes	100.0% 🚀
*`assets/SVMPhones-.css`** (New)	110 bytes	110 bytes	100.0% 🚀

Files in assets/Files-*.js:

./src/CMS/components/PersonSelector.vue → Total Size: 150 bytes
./src/CMS/components/FileFormDialog.vue → Total Size: 150 bytes

Files in assets/personnel-*.js:

./src/Personnel/App.vue → Total Size: 112 bytes

codecov-commenter · 2026-08-25T19:08:22Z

Codecov Report

❌ Patch coverage is 92.58216% with 158 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.38%. Comparing base (c6f64b5) to head (dcd328c).

Files with missing lines	Patch %	Lines
...s/Personnel/Controllers/PhoneListUnitController.cs	69.13%	23 Missing and 2 partials ⚠️
...el/Controllers/PhoneSVMFrequentNumberController.cs	50.00%	24 Missing ⚠️
...as/Personnel/Controllers/PhoneSVMUnitController.cs	68.75%	15 Missing ⚠️
VueApp/src/Personnel/composables/svm-data-fetch.ts	92.15%	3 Missing and 5 partials ⚠️
web/Areas/Personnel/Models/PhoneDtos.cs	87.30%	8 Missing ⚠️
VueApp/src/Personnel/pages/PhoneListMaintain.vue	87.93%	4 Missing and 3 partials ⚠️
...b/Areas/Personnel/Services/PhoneListUnitService.cs	96.51%	2 Missing and 5 partials ⚠️
.../Personnel/components/PhoneListAddRecordDialog.vue	82.35%	5 Missing and 1 partial ⚠️
VueApp/src/Personnel/pages/SVMPhonesMaintain.vue	92.95%	2 Missing and 3 partials ⚠️
VueApp/src/components/PersonSearchSelect.vue	70.58%	4 Missing and 1 partial ⚠️
... and 25 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #323      +/-   ##
==========================================
+ Coverage   42.38%   44.38%   +1.99%     
==========================================
  Files         994     1065      +71     
  Lines       49877    51933    +2056     
  Branches     5887     6115     +228     
==========================================
+ Hits        21142    23049    +1907     
- Misses      27798    27912     +114     
- Partials      937      972      +35

Flag	Coverage Δ
backend	`41.79% <92.46%> (+1.42%)`	⬆️
frontend	`63.38% <92.75%> (+4.42%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...App/src/Personnel/components/MobileSortControl.vue	`100.00% <100.00%> (ø)`
...ueApp/src/Personnel/components/ModifiedSummary.vue	`100.00% <100.00%> (ø)`
VueApp/src/Personnel/components/PersonSelector.vue	`100.00% <100.00%> (ø)`
...ueApp/src/Personnel/components/PhoneListFilter.vue	`100.00% <100.00%> (ø)`
...pp/src/Personnel/components/RecordActionButton.vue	`100.00% <100.00%> (ø)`
...eApp/src/Personnel/components/RecordActionCell.vue	`100.00% <100.00%> (ø)`
...pp/src/Personnel/components/SVMAddRecordDialog.vue	`100.00% <100.00%> (ø)`
...eApp/src/Personnel/components/SectionJumpLinks.vue	`100.00% <100.00%> (ø)`
...src/Personnel/composables/phone-list-data-fetch.ts	`100.00% <100.00%> (ø)`
...App/src/Personnel/composables/svm-phone-columns.ts	`100.00% <100.00%> (ø)`
... and 68 more

... and 2 files with indirect coverage changes

rlorenzo · 2026-08-25T22:44:05Z

@coderabbitai full review

coderabbitai · 2026-08-25T22:44:11Z

⚠️ Action not completed

Review skipped: 122 files exceed the limit of 100.

Copilot

Pull request overview

This PR migrates the schoolwide (SVM) and Dean's Office (VMDO) phone lists from Viper 1 into a new Personnel area, backed by a new normalized phones schema in the VIPER database. Viewing requires basic SVMSecure permission, while a new SVMSecure.PhoneLists.SVMMaintain permission (and per-list MaintainRole) gates editing. It adds EF Core models/services/controllers plus a full Vue 3/Quasar SPA, and refactors shared person-search logic into a reusable PersonSearchHelper used by both CMS and Personnel.

Changes:

New phones schema + PhonesDbContext, EF models, area services and /api/phones/... controllers with dynamic per-list maintain permissions and direct-number masking.
New Personnel Vue SPA (lists, maintenance views, person selector, record dialogs) plus data-migration scripts from the legacy PhoneList database.
Shared PersonSearchHelper extracted and adopted by CMS's SearchPeople, forcing EF parameterization (ESCAPE clause) to prevent per-term query plans and %/_ wildcard injection.

Reviewed changes

Copilot reviewed 121 out of 122 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
web/Viper.csproj	Excludes `Areas\Personnel\Scripts\**` (separate migration project) from the web build, mirroring the Effort area.
web/Program.cs	Registers `PhonesDbContext`, adds `Personnel` SPA name and the Personnel services namespace to Scrutor registration.
web/Classes/Utilities/PersonSearchHelper.cs	New shared expression-tree helper for name-search autocomplete with parameterized `Contains` matching.
web/Areas/Personnel/Services/PhoneSVMSectionService.cs	Read-only query for SVM sections, ordered with null-safe sort.
web/Areas/Personnel/Services/PhoneSVMFrequentNumberService.cs	CRUD + soft-delete for SVM frequent numbers, with modified-date tracking.
web/Areas/Personnel/Services/PhonePersonLookupService.cs	Looks up phone people by IAM IDs (direct number masked unless maintainer) and current-employee search.
web/Areas/Personnel/Services/PhonePermissionsService.cs	Resolves edit permission from the list's `MaintainRole` column.
web/Areas/Personnel/Controllers/PhonePersonController.cs	Person-picker endpoint merging Viper and phone data; uses `foreach`/`Add` where `.Select()` is preferred.
web/Areas/Personnel/Controllers/PhoneSVMModifiedDateController.cs	Returns latest SVM modified date; contains a comment typo ("Identfies").
web/Areas/Personnel/Models/, VueApp/src/Personnel/*	New EF models/DTOs/Mapperly mapper and the Personnel Vue SPA (services, composables, components, tests).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

rlorenzo

Solid work, and the parts that are easy to get wrong are right: ResolveListForMaintain, VerifyUnitInList, and GetUnitPersonInList each re-scope by list rather than trusting the id in the request, with a test proving one list's role grants nothing on another. I ran the branch against dev, so the inline notes are reproductions. Four things block deployment, none of them in the code:

The DDL won't run. CREATE SCHEMA Inventory; should be phones, so every CREATE TABLE [phones].[...] fails. Four ALTER TABLE [phones].[SVMUnitPerson] CHECK CONSTRAINT statements also name the wrong table, and three run before that table exists.
The DDL is missing the unique index on PhoneList.Code, and dev already has it. UX_PhoneList_Code was added to dev by hand, so Production won't get it and a duplicate code would resolve arbitrarily, including for the permission check.
A permission is missing from the steps. VMDO's MaintainRole is SVMSecure.PhoneLists.VMDOMaintain, but only SVMMaintain is listed, so nobody could maintain VMDO.
The pages aren't reachable from the nav. MainNav.cs:29 and MiniNav/Default.cshtml:105-109 still point Personnel at VIPER 1, though App.vue sets highlighted-top-nav="Personnel".

Also Home.vue needs a personnel-home CMS record per environment, or redirected non-maintainers land on a blank page. Everything else is inline, tagged minor where it's a nit rather than a fix I'd hold the PR for.

bniedzie · 2026-08-28T22:22:53Z

Solid work, and the parts that are easy to get wrong are right: ResolveListForMaintain, VerifyUnitInList, and GetUnitPersonInList each re-scope by list rather than trusting the id in the request, with a test proving one list's role grants nothing on another. I ran the branch against dev, so the inline notes are reproductions. Four things block deployment, none of them in the code:

The DDL won't run. CREATE SCHEMA Inventory; should be phones, so every CREATE TABLE [phones].[...] fails. Four ALTER TABLE [phones].[SVMUnitPerson] CHECK CONSTRAINT statements also name the wrong table, and three run before that table exists.

The DDL is missing the unique index on PhoneList.Code, and dev already has it. UX_PhoneList_Code was added to dev by hand, so Production won't get it and a duplicate code would resolve arbitrarily, including for the permission check.

A permission is missing from the steps. VMDO's MaintainRole is SVMSecure.PhoneLists.VMDOMaintain, but only SVMMaintain is listed, so nobody could maintain VMDO.

The pages aren't reachable from the nav. MainNav.cs:29 and MiniNav/Default.cshtml:105-109 still point Personnel at VIPER 1, though App.vue sets highlighted-top-nav="Personnel".

Also Home.vue needs a personnel-home CMS record per environment, or redirected non-maintainers land on a blank page. Everything else is inline, tagged minor where it's a nit rather than a fix I'd hold the PR for.

I've addressed 1 and 2 in the DDL in the description.

3 is a non-issue - SVMSecure.PhoneLists.VMDOMaintain is a legacy permission, present in all environments, and assigned to users with no changes to scope. SVMMaintain is being added to address a legacy security issue.

I've fixed the nav; personnel-home is an existing CMS record on both test and prod, so no changes should be needed.

bniedzie · 2026-08-28T23:20:05Z

Tagging @JasonRobertFrancis mostly so he is aware of what I've done in Personnel as part of this task

rlorenzo · 2026-08-29T03:35:34Z

+// oxlint-disable-next-line unicorn/import-style
 import path, { resolve } from "node:path"


Mixing the default and named import is what trips import-style; every other file in the repo imports path only, so no suppression is needed.

Suggested change

// oxlint-disable-next-line unicorn/import-style

import path, { resolve } from "node:path"

import path from "node:path"

rlorenzo · 2026-08-29T03:35:34Z

                    cahfs: resolve(import.meta.dirname, "src/CAHFS/index.html"),
                    clinicalscheduler: resolve(import.meta.dirname, "src/ClinicalScheduler/index.html"),
                    effort: resolve(import.meta.dirname, "src/Effort/index.html"),
+                    personnel: resolve(import.meta.dirname, "src/Personnel/index.html"),


Same for the five entries above.

Suggested change

cahfs: resolve(import.meta.dirname, "src/CAHFS/index.html"),

clinicalscheduler: resolve(import.meta.dirname, "src/ClinicalScheduler/index.html"),

effort: resolve(import.meta.dirname, "src/Effort/index.html"),

personnel: resolve(import.meta.dirname, "src/Personnel/index.html"),

cahfs: path.resolve(import.meta.dirname, "src/CAHFS/index.html"),

clinicalscheduler: path.resolve(import.meta.dirname, "src/ClinicalScheduler/index.html"),

effort: path.resolve(import.meta.dirname, "src/Effort/index.html"),

personnel: path.resolve(import.meta.dirname, "src/Personnel/index.html"),

rlorenzo · 2026-08-29T03:38:54Z

        <q-item clickable>
            <q-item-section>
-                <q-btn stretch flat label="Personnel" href="@Url.Content(ViewData["OldViperURL"] + "/personnel/default.cfm")"></q-btn>
+                <q-btn stretch flat label="Personnel" href="/2/Personnel")"></q-btn>


Hardcoded /2 breaks local dev (no PathBase): /2/Personnel is a 404 here while /Personnel is 200. The trailing )" is left over from the removed @Url.Content(...) and parses as a junk attribute.

Suggested change

<q-btn stretch flat label="Personnel" href="/2/Personnel")"></q-btn>

<q-btn stretch flat label="Personnel" href="@Url.Content("~/Personnel")"></q-btn>

rlorenzo · 2026-08-29T04:02:17Z

        <q-item clickable>
            <q-item-section>
-                <q-btn stretch flat label="Personnel" href="@Url.Content(ViewData["OldViperURL"] + "/personnel/default.cfm")"></q-btn>
+                <q-btn stretch flat label="Personnel" href="/2/Personnel")"></q-btn>


Hardcoded /2 404s locally (no PathBase), and the trailing )" is left over from the removed @Url.Content(...). Verified on dev: this renders as href="/2/Personnel")" while MainNav renders /Personnel.

Suggested change

<q-btn stretch flat label="Personnel" href="/2/Personnel")"></q-btn>

<q-btn stretch flat label="Personnel" href="@Url.Content("~/Personnel")"></q-btn>

rlorenzo

Re-review of d4aaacb9..dcd328cf. The earlier threads are all resolved; these are new in the delta. Items on PhoneListFilter.vue, SectionJumpLinks.vue, SVMPhones.vue and PhoneListUnitService.cs I'd hold for, the rest are marked minor. The three UI ones reproduced in the browser at 390px.

rlorenzo · 2026-08-29T06:17:19Z

+// header above it. The height is not a constant: the field is a fixed 40px but the padding is in
+// rem, and the root font-size steps from 14px to 16px at 768px.
+const barRef = useTemplateRef<HTMLElement>("bar")
+const { height } = useElementSize(barRef)


useElementSize measures the content box by default, so the published height leaves out this bar's own padding-block (line 72). Measured at 390px: --phone-list-filter-height is 207px, the bar's border box is 221px. .sticky-filter-offset is built from the variable, so every jump target lands 14px under the bar (16px at 768px and up), with focus on a hidden h2.

Suggested change

const { height } = useElementSize(barRef)

const { height } = useElementSize(barRef, undefined, { box: "border-box" })

rlorenzo · 2026-08-29T06:17:19Z

+<template>
+    <!-- Nothing to navigate between with one destination, and nothing to offer when a search has
+         emptied them all. -->
+    <q-expansion-item


Nothing collapses this after a link is followed, and it sits inside the sticky bar, so the open panel stays pinned over the section it just jumped to (about 140px of a 390px viewport). A v-model set to false in the link's click handler closes it and lets the bar shrink back before the scroll.

rlorenzo · 2026-08-29T06:17:19Z

+        {{ errorMessage }}
+    </StatusBanner>
+
+    <template v-if="!loading && !errorMessage">


This is the guard I asked for on the phone list page, where a failed read means no data. Here loadPhoneData renders whatever arrived, so a failed frequent-numbers read hides the filter and jump links over a full page of sections. v-if="!loading" is enough; the banner already sits above it.

rlorenzo · 2026-08-29T06:17:19Z

+        {{ errorMessage }}
+    </StatusBanner>
+
+    <template v-if="!loading && !errorMessage">


Same as SVMPhones.vue:11: the tables below render on a partial failure, so this hides the filter over the data.

rlorenzo · 2026-08-29T06:17:19Z

+            var userIam = _userHelper.GetCurrentUser()?.IamId;
+            var updateTimestamp = DateTime.Now;
+
+            await AddOrUpdatePhonePerson(request, userIam, updateTimestamp, ct);


This upserts the shared phones.Person row keyed by request.EmployeeIam, but the list row's PersonIam never changes, so a PUT carrying a different (real) IAM rewrites a third party's phone, direct phone and office while the row still shows the original person. Not reachable from the dialog (v-if="!isEdit" on the picker), but VerifyPersonExists right above implies the field is honoured. Key the write off modifiedPhoneListPerson.PersonIam, or 400 on a mismatch.

rlorenzo · 2026-08-29T06:17:19Z

+            {
+                return BadRequest(ex.Message);
+            }
+            catch (DbUpdateException ex)


Minor, and my suggestion, so on me: DbUpdateException also wraps deadlock victims (1205), timeouts and dropped connections, which now return "check all field values" at Warning. With [MaxLength] and the person check in front, most of what reaches this catch is infrastructure. when (ex.InnerException is SqlException { Number: 2601 or 2627 or 547 or 8152 }) for the 400, LogError + 500 otherwise. Same in the other eight write actions.

rlorenzo · 2026-08-29T06:17:19Z

+    /// <summary>
+    /// Maps the phones entities to the shapes the API returns. Every property an entity has and
+    /// its DTO does not is named in a [MapperIgnoreSource] below, so the omissions are decisions
+    /// rather than oversights and a column added to an entity is a build error here before it can


Minor: RequiredMappingStrategy.None (line 11, the project default) turns off the unmapped-member diagnostic, so "a column added to an entity is a build error here" does not hold; the [MapperIgnoreSource] list only bites on a rename. The DTOs are what pin the wire shape, so the comment can say that instead.

rlorenzo · 2026-08-29T06:17:19Z

+            entity.Property(e => e.DirectPhone).HasColumnName("DirectPhone").HasMaxLength(25);
+            entity.Property(e => e.Office).HasColumnName("Office").HasMaxLength(100);
+            entity.Property(e => e.ModifiedDate).HasColumnName("ModifiedDate");
+            entity.Property(e => e.ModifiedBy).HasColumnName("ModifiedBy");


Minor: only ModifiedBy in this file without the cap; lines 89, 114, 142 and 188 all have it.

Suggested change

entity.Property(e => e.ModifiedBy).HasColumnName("ModifiedBy");

entity.Property(e => e.ModifiedBy).HasColumnName("ModifiedBy").HasMaxLength(10);

rlorenzo · 2026-08-29T06:17:19Z

+    }
+
+    [Fact]
+    public async Task GetUnits_SerializesWithoutTheEntityNavigationProperties()


Minor: the fixture seeds no unit-person rows, so phoneListUnitPersons serializes as [] and the isActive / phoneListUnit assertions never see a PhoneListUnitPersonDto. AddUnitPersonRow(1, 1) at the top makes the test pin what its comment says it pins.

rlorenzo · 2026-08-29T06:17:20Z

+        <!-- tabindex allows a jump link to land focus here, not just the viewport. -->
+        <h2
+            :id="anchorId"
+            class="text-h6 q-mt-none q-mb-sm sticky-filter-offset"


Minor: .q-page-container .text-h6 (1rem) out-specifies .q-page-container h2 (1.2rem), so this renders a step smaller than DESIGN.md's h2.

Suggested change

class="text-h6 q-mt-none q-mb-sm sticky-filter-offset"

class="q-mt-none q-mb-sm sticky-filter-offset"

VPR-64 feat(phone): schoolwide and unit phone lists

12baad2

VPR-64 feat(phone): improve test coverage and fix incorrect class name

3283e1b

rlorenzo requested a balanced review from Copilot August 25, 2026 22:50

Copilot started reviewing on behalf of rlorenzo August 25, 2026 22:50 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

Comment thread web/Areas/Personnel/Controllers/PhonePersonController.cs Outdated

Comment thread web/Areas/Personnel/Controllers/PhoneSVMModifiedDateController.cs Outdated

VPR-64 feat(phone): Fix code quality errors

d4aaacb

rlorenzo requested changes Aug 26, 2026

View reviewed changes

bniedzie added 2 commits August 26, 2026 08:51

VPR-64 feat(phone): Fix code quality errors and typos

092824f

VPR-64 feat(phone): Make Code Review Changes

a98086e

VPR-64 feat(phone): Clarify EF relationship for phones.Person

97f699b

bniedzie requested review from JasonRobertFrancis, bsedwards and rlorenzo August 28, 2026 23:19

VPR-64 feat(phone): Ensure new pages are built by vite

dcd328c

rlorenzo reviewed Aug 29, 2026

View reviewed changes

		// oxlint-disable-next-line unicorn/import-style
		import path, { resolve } from "node:path"

	// oxlint-disable-next-line unicorn/import-style
	import path, { resolve } from "node:path"
	import path from "node:path"

	<q-btn stretch flat label="Personnel" href="/2/Personnel")"></q-btn>
	<q-btn stretch flat label="Personnel" href="@Url.Content("~/Personnel")"></q-btn>

	const { height } = useElementSize(barRef)
	const { height } = useElementSize(barRef, undefined, { box: "border-box" })

	entity.Property(e => e.ModifiedBy).HasColumnName("ModifiedBy");
	entity.Property(e => e.ModifiedBy).HasColumnName("ModifiedBy").HasMaxLength(10);

	class="text-h6 q-mt-none q-mb-sm sticky-filter-offset"
	class="q-mt-none q-mb-sm sticky-filter-offset"

Uh oh!

Conversation

bniedzie commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle Report

Affected Assets, Files, and Routes:

Assets Changed:

Uh oh!

codecov-commenter commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rlorenzo commented Aug 25, 2026

Uh oh!

coderabbitai Bot commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

rlorenzo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bniedzie commented Aug 28, 2026

Uh oh!

bniedzie commented Aug 28, 2026

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!

rlorenzo 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

bniedzie commented Aug 25, 2026 •

edited

Loading

codecov-commenter commented Aug 25, 2026 •

edited

Loading

codecov-commenter commented Aug 25, 2026 •

edited

Loading

coderabbitai Bot commented Aug 25, 2026 •

edited

Loading