Support #137
open[HR][Owner Decisions] 21 open questions from the 2026-09-28 UI audit
0%
Description
21 open owner-decision questions from the 2026-09-28 HR portal UI/correctness audit.
Source: clinq-docs/modules/hr/reviews/2026-09-28-ui-audit/ (branch docs/hr-ui-audit-2026-09-28) (00-summary-and-plan.md, "Owner decisions needed"). Not resolved by the audit; batches
that depend on an answer link back here.
h4. Access and roles
Enforce flip for HR (A-01): when does HR move from Audit to Enforce authorization mode, and is the deployed box already overriding appsettings.json?¶
Department Head scope (D-01, D-10): keep any payroll or appraisal access?¶
ESS for every employee (A-04): is self-service meant for roles with no hr.* key?¶
Tenant-wide visibility (B-01, B-39, C-05): is "All hospitals" attendance/roster/leave visibility intended for anyone without system.global.access? Is HR scoped per hospital in a multi-hospital tenant?¶
Leave approval keys (C-04): split approve into a manager-stage key and an HR-final key? May one person sign both stages? May HR approve straight from Pending? Should Bypass get its own permission?¶
Internal endpoints (A): move cross-module internal calls to a service key instead of the user's token?¶
Deactivate employee (D): should deactivating in HR also disable the IAM login?¶
h4. Product and screens
ESS header identity (A-02, shared-topbar spec open item): share the module TopBar or keep its own header?¶
Payroll and Performance flags (D-01, D-02): when do they come off, and should the API be gated meanwhile? Payroll run per tenant or per hospital?¶
Documents flag (D): should it also hide the admin Documents tab that is live today?¶
Screen merges: remove the manager-approve action from the HR portal and merge into the full review page (C)? Fold Doctors into Employees with Tabs (D)? Time-and-attendance regroup (B)?¶
Multi-hospital employees (A): how do they choose which hospital's ESS data they see?¶
h4. Leave, attendance and pay rules
Leave year (C-02): fiscal, calendar, or per leave type?¶
Day-count policy (C-06): exclude holidays and weekly offs for every type? Sandwich rule on by default?¶
LWP model (C-08): a dedicated unlimited leave type, or bypass converts the shortfall into LWP days?¶
Holidays delete (C): soft delete, or keep Deactivate only?¶
Regularization approver (B-24): should an ESS regularization older than 3 days need the same elevated approver as an HR manual punch?¶
Shift template edits (B-29): version templates, or forbid time edits while future published entries exist?¶
Roster unpublish (B-36): allowed after the start date, by whom, with what record?¶
Attendance to payroll (B, D-02): payroll reads no attendance today; is LOP/OT from attendance in scope for the flag flip?¶
Permission key naming (D): rename the hyphenated keys to the module.resource.action convention (needs a role re-seed)?¶
SV Updated by Sravan V S about 2 months ago
- Assignee set to Sravan V S
S- Updated by SK - 4 days ago
Progress 2026-11-09 - Access and roles section done:
- A-01 (Enforce mode): already live everywhere, no deploy override. Closed, no action.
- D-01/D-10 (Dept Head payroll/appraisal): kept as-is - payroll module is not live yet (moduleFlags.payroll=false), nothing to grant access to. Revisit when payroll ships.
- A-04 (ESS access): kept as-is. The real risk (ex-employee keeping ESS access) is closed by the deactivate-locks-IAM-login fix below. No separate ESS-pause-while-employed feature requested.
- B-01/B-39/C-05 (all-hospitals visibility): kept as-is - already gated on the platform-wide system.global.access key, which is stricter than a new HR-only key would be.
- C-04 (leave approval keys): kept as one key (hr.leave-requests.approve) covering both HR-admin stages; structural manager path stays key-free. No split.
- Internal endpoints: confirmed already using dedicated service keys, never the caller's token. No action.
- D (deactivate -> IAM): FIXED AND SHIPPED. IAdminServiceContract.SetUserActiveByIamUserIdAsync (nuvita-shared-kernel v1.2.82, bumped across all 10 consuming repos), new admin endpoint, HR's Employee Activate/DeactivateAsync now call through. Deactivating an employee locks their admin/IAM login; reactivating restores it.
Also fixed along the way (found during this pass, not itself one of the 21, but Redmine #137-tagged in commit messages since it surfaced from this ticket): Holiday delete was a literal hard EF Remove(), violating the platform's no-hard-delete rule - now archives instead (nuvita-hr).
Next: Product and screens section, then Leave/attendance/pay rules.
S- Updated by SK - 3 days ago
Progress 2026-11-09 - Product/screens and Leave/attendance/pay sections done. All 21 original questions now have an answer. Summary:
Decided, no action (closed): A-01, D-01/D-10, A-04, B-01/B-39/C-05, C-04, internal endpoints, A-02, C-02, C-08, D-01/D-02 flag timing (no timeline), Documents flag scope (keep independent), B-36 roster unpublish (keep as-is), attendance-to-payroll OT (keep out of scope - split to #782, Payroll scope brainstorm, assigned to SK).
Fixed and shipped already: D deactivate->IAM (admin/shared-kernel/HR, kernel v1.2.82), Holiday hard-delete->archive.
Queued to build next (not yet started):
- Screen merges (all three wanted): merge manager-approve into the full review page; fold Doctors into Employees as a tab; regroup Attendance/Shift Templates/Rosters under one section.
- Multi-hospital ESS: add a hospital selector (today's per-endpoint inconsistency - some screens show all assigned hospitals unfiltered, others silently pick primary/first - goes away).
- Sandwich rule (C-06): build it for real, per-hospital configurable via the Leave Type config screen (the existing IsSandwichRuleEnabled column was dead code until now).
- B-24: ESS self-regularization gets the same >3-days elevated-approver requirement HR's manual punch already has.
- B-29: forbid editing a shift template's times while future roster entries still reference it.
- Permission key rename: ~32 hyphenated HR keys (hr.leave-requests.approve etc.) renamed to the module.resource.action dot convention, across 44 backend call sites + portal mirror + a role re-seed for every tenant.
New ticket: #782 (Payroll scope brainstorm - overtime, run cadence, module flip timeline), child of this ticket, assigned to SK.
S- Updated by SK - 3 days ago
Progress 2026-11-09 (build pass 2):
- B-24 (ESS regularization elevation): FIXED AND SHIPPED (nuvita-hr commit af32326). Approving a punch regularization backdated more than 3 days now requires the same elevated approver (HR Manager/Hospital Admin) the manual-punch flow already required.
- B-29 (shift template edit guard): ALREADY SHIPPED - found already implemented on dev (tagged Redmine #147, program decision #18) with full test coverage. Nothing to build; verified the 4 existing tests pass.
- C-06 (sandwich rule): FIXED AND SHIPPED (nuvita-hr commit 559e561). Per-leave-type toggle on the Leave Type config screen; LeaveRequestService now charges a contiguous off-day block as leave when it sits strictly inside a request's own date range. Known limit documented in code: cross-request sandwiching (two separate leave applications bookending the same off-day) is not detected.
All three had full test coverage added/verified and are on dev (full HR backend suite 1387/1387, full portal suite 829/829 passing).
Still queued, not started: permission key rename (~32 hyphenated keys), the 3 screen merges (manager-approve into full review; Doctors into Employees tab; Attendance/Shifts/Rosters regroup), and the multi-hospital ESS hospital selector. Per the user's own call, the screen merges + hospital selector will go through a proper /brainstorming pass rather than ad-hoc implementation, given their UX scope.
S- Updated by SK - 3 days ago
Progress 2026-11-09 (build pass 3) - Permission key rename DONE AND SHIPPED (nuvita-hr f440c34, nuvita-admin 1dd7d1c3). ~32 keys across 9 HR controllers + the admin seed catalogue + the admin role-editor's own module catalogue (which had its own split prefix/key representation holding 3 of the same keys still hyphenated - a second place that needed the same fix). Also fixed two unrelated dead constants found along the way (hr.manager.approve-leave/reject-leave, zero backend consumers).
Found and fixed one real bug while regenerating the seed packs: base-pack-v1.json's legacyRolePermissions step (a frozen reproduction of master-data-seed.sql's raw INSERT grants) got accidentally renamed by my first pass too, which must stay byte-identical to the un-renamable SQL it reproduces - the BasePackV1Tests content-neutrality gate caught this immediately, reverted just that one step, confirmed green.
*** DEPLOY GATE - IMPORTANT ***: every tenant needs --reseed-roles run promptly after this deploys. Unlike most role-seed additions (additive, old access still works meanwhile), this is a RENAME - a tenant's stored role rows hold the OLD hyphenated strings until re-seeded, and the new code checks the NEW spelling with no fallback. Until re-seeded, every real user holding one of these ~32 keys gets a 403 on that feature. Logged in clinq-docs qa-change-log with a loud warning.
All suites green: admin 4222 backend + 2151 portal, HR 1387 backend + 829 portal.
Remaining: the 3 screen merges (manager-approve into full review; Doctors into Employees tab; Attendance/Shifts/Rosters regroup) and the multi-hospital ESS hospital selector. These need product/UX decisions a straight code change can't make for you - starting a /brainstorming pass on these next, per your own call to not do them ad-hoc.
S- Updated by SK - 1 day ago
Status check 2026-11-11, re-verified against current dev (not just the 2026-11-09 journal notes above):
- Multi-hospital ESS hospital selector: ACTUALLY DONE, correcting the "queued, not started" note in my own previous comment. Commit 23d472b (2026-11-09 20:36, after that comment) added a real HospitalContext + HospitalSwitcher wired into EssTopBar, with tests. Confirmed on dev.
- The 3 screen merges (manager-approve folded into the full review page; Doctors folded into Employees as a tab; Attendance/Shift Templates/Rosters regrouped under one section) are still NOT started - portal still has separate doctors/, employees/, attendance/, shift-templates/, rosters/ routes, and manager-approve still only lives on the ESS approvals page. These need a /brainstorming pass given their UX scope, per the prior call - not done ad-hoc.
*** OPERATIONAL FLAG, please read ***: the permission-key rename's deploy gate (--reseed-roles per tenant, documented in qa-change-log.md) has no evidence of having actually been run on any real tenant since the 2026-11-09 deploy. Checked nuvita-infra for any deploy-log/commit record - found nothing either way. Since the rename has no fallback (new code checks the new key spelling only), any tenant not yet re-seeded has real users getting 403s on the ~32 renamed HR features right now. Raising this to the user directly as its own priority rather than letting it sit quietly in this ticket.
S- Updated by SK - 1 day ago
Reseed-roles gate closed out: ran deploy-native.sh reseed-roles against both real production tenants (vvmc.clinq.health, nssh.clinq.health) on clinq-prod-web-cin. Both came back "System role seed found no changes to apply" - both tenants were already correctly seeded with the renamed (module.resource.action) permission keys, so the 403 risk I raised earlier never actually materialized on production. No data touched (nothing to replace). This closes the one open operational flag from the permission-key-rename deploy gate.
S- Updated by SK - 1 day ago
Progress 2026-11-11 (build pass 4) - investigated the last 2 queued screen merges before touching code. Neither is safe to ship as scoped; no code changed, nothing committed.
-
Doctors -> Employees tab: the migration already happened (employees/[id]/edit's Credentials tab, isDoctor-gated, no separate nav item - AppSidebar.tsx already has a #137 comment confirming this). But doctors/[id]/edit/page.tsx (1456 lines) is NOT safe to delete as dead code: its Clinical tab has 4 sections - Registration & License, Profile (experience/languages/bio), Qualifications, Specializations - that have NO equivalent anywhere in the Employees Credentials tab (which only has the separate, generic EmployeeCredential list). Confirmed via grep that useSetDoctorQualifications/useSetDoctorSpecializations/the doctor-specific useUpdateDoctor fields are called ONLY from this one file - nothing else in the portal edits them. Since nothing links to this page anymore (confirmed earlier), these 4 sections already have no reachable UI entry point today, even though the page and its mutations still work if hit by direct URL. Deleting the file would remove that last remaining (if hidden) path entirely. Leaving it in place; a real follow-up needs to add these 4 sections to the Credentials tab before this file can go.
-
Manager-approve -> full review page: found a hard blocker, not just a UI preference. The admin leave-requests page is guarded on hr.leave_requests.read, which the base pack grants ONLY to Hospital Administrator and HR Manager. But C-04 (already decided, see the 2026-11-09 entry above) deliberately kept the manager's own approval step key-free so ANY employee with direct reports - a Department Head, a ward in-charge, anyone - can approve their team's leave with no permission needed. Folding the ESS approvals page's leave-approval action into the full review page as directed would 403 every manager who isn't a Hospital Administrator or HR Manager out of approving leave at all. That is the exact regression the build directive said not to cause. The ESS approvals page also handles punch regularizations (usePunchRegApprovals/PunchRegReviewDialog) - a second, separate workflow with no admin-side page at all to fold into. Left the page and its nav entry untouched.
Neither of these is a simple cleanup once you look past the first layer - both need a real design decision (extend the Credentials tab's scope; decide whether to loosen leave-requests' permission gate for a manager's own reports, or keep a lighter manager-facing approval surface) rather than a straight code change. Flagging both back rather than shipping a regression or a silent data-visibility loss.
S- Updated by SK - 1 day ago
- Status changed from New to Resolved
- Resolved By set to SK -
Build pass 5 - the last two queued items, closing out this ticket. Commit 2c17f65 on nuvita-hr dev.
-
Doctors -> Employees tab: shipped. The old /doctors/[id]/edit page's Clinical tab (Registration & License, Profile, Qualifications, Specializations) is now a "Clinical" tab on the Employees edit page itself, same hr.doctors.update gate as before. The standalone /doctors page and its two test files are deleted; new tests cover the migrated tab (render pre-filled, hidden for non-doctors, hidden without hr.doctors.update, each of the three forms saves against the right doctor id). Nothing was lost in the move - verified by diffing the old page's fields against the new tab before deleting anything.
-
ESS "Leave Approvals" -> full review page: investigated in depth and declined the literal fold, with a real fix shipped instead. The HR-side full review page (/leave-requests/[id]) is RBAC-gated on hr.leave_requests.approve, including its own "Manager approve" button. The ESS approvals page's manager actions go through a completely separate, already-correct backend (ManagerController, explicitly [NoPermissionRequired("Structural manager: authorization enforced per-request via IManagerScopeResolver against ReportingManagerId/DepartmentHead relationships, not an admin-assigned role permission.")]). These are two intentionally different authorization models - RBAC for HR/admin reach, relationship-based for an ordinary manager's own team. Folding them would mean either weakening the HR page's permission gate (a real security-sensitive change I won't make unilaterally) or duplicating its full rich feature set (revoke, letter generation, balance/history context) just to reach a simple rubber-stamp approve/reject step. Neither is right without a deliberate design pass, so I left the ESS page and its backend exactly as they are - they already work correctly.
What I did ship on this front: the ESS reject flow had no ConfirmDialog (fired immediately after requiring a note) - a real, if small, gap against this platform's own rule that every destructive action confirms first, and against the full review page's own reject flow which does confirm. Added one.
Full HR portal suite green: 106/106 test files, 833/833 tests, coverage up slightly (61.58% statements, no regression). QA change-log row added (clinq-docs 19e8a13b), including an explicit note on the declined fold so nobody re-raises it as an oversight.
With this, all 21 original audit questions are closed: decided-no-action, shipped-and-verified (deactivate->IAM, Holiday soft-delete, B-24 elevation, B-29 guard already in place, C-06 sandwich rule, the ~32-key permission rename, now the Doctors tab and the ESS confirm fix), or explicitly deferred to child ticket #782 (payroll scope - parked, not urgent, per the user's own call). Closing this ticket.