VPR-64 feat(phone): schoolwide and unit phone lists - #323
Conversation
Bundle ReportChanges will increase total bundle size by 50.14kB (2.19%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: viper-frontend-esmAssets Changed:
Files in
Files in
|
|
@coderabbitai full review |
|
There was a problem hiding this comment.
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
phonesschema +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
PersonSearchHelperextracted and adopted by CMS'sSearchPeople, 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
left a comment
There was a problem hiding this comment.
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 bephones, so everyCREATE TABLE [phones].[...]fails. FourALTER TABLE [phones].[SVMUnitPerson] CHECK CONSTRAINTstatements 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_Codewas 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 onlySVMMaintainis listed, so nobody could maintain VMDO. - The pages aren't reachable from the nav.
MainNav.cs:29andMiniNav/Default.cshtml:105-109still point Personnel at VIPER 1, thoughApp.vuesetshighlighted-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.
|
|
Tagging @JasonRobertFrancis mostly so he is aware of what I've done in Personnel as part of this task |
rlorenzo
left a comment
There was a problem hiding this comment.
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.
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:
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:
This PR requires creating a new permission on Production: SVMSecure.PhoneLists.SVMMaintain.
This PR requires running the migration script
.\RunMigrateData.bat Productionfor a dry run, and then.\RunMigrateData.bat Production --applyto 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.