feat(users): weekly_hours + employment_type on user (Slice 1)#3653
feat(users): weekly_hours + employment_type on user (Slice 1)#3653gloriafolaron wants to merge 6 commits into
Conversation
Adds two nullable capacity attributes to zp_user so downstream resource-planning surfaces (starting with PgmPro's Resource Allocation tab) can distinguish an FTE's target from a PT's ceiling, a contractor's billable cap, and a volunteer's best-effort throughput. - Migration update_sql_30523 adds nullable columns (no backfill, no defaults — admins configure explicitly; NULL means "not set"). - Fresh-install schema in setupDB() picks up the new columns too. - EmploymentType enum (FTE / PT / Contractor / Volunteer) with helpers for capacity-math semantics (countsAgainstCapacity, overCapTone). - Users repository editUser + addUser accept + persist the new fields. Uses array_key_exists so a form that omits them doesn't null out existing values. - EditUser controller reads them into $values and passes POST through only when present. - editUser.blade.php gains a Capacity section (weekly hours input + employment type select) gated behind Roles::admin — regular users can't self-edit these. - Language strings for labels, hints, and the 4 employment types. Nothing in the app reads these values yet — this ships the model in isolation. Downstream PRs will wire capacity math + cross-program warnings in PgmPro. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Maintainer triage (advisory — merge authority stays with @marcelfolaron / @broskees) · first pass · reviewed at head 1. Intent: Slice 1 of user-level capacity modelling — adds two nullable columns to 2. Change requests:
3. Acceptance checklist (must pass before merge):
4. CI status: Advisory review only — not an approval, and I am not marking this ready or merging. Merge authority stays with @marcelfolaron / @broskees. |
|
Maintainer review (advisory — not an approval/merge) · head Intent: Ships Slice 1 of user capacity attributes — adds nullable Change requests:
Acceptance checklist (must pass before merge):
CI status: I can't read check-runs from here — please confirm Pint/PHPStan/unit are green in the PR checks tab. Migration ordering (#1) and missing tests (#2) are treated as blockers regardless of CI. Note this bumps |
Addresses two of Marcel's review items on PR #3653: ## CR: employment_type accepts any string on write Repository editUser + addUser now normalise every incoming employment_type through EmploymentType::tryFrom() before persist. Unknown values (including a crafted POST) normalise to NULL rather than round-tripping garbage that downstream EmploymentType::from() would throw on. Validation lives at the persistence boundary so every caller (form controller, service, JSON-RPC) gets the same guarantees, not just the admin edit-user form. ## CR: weekly_hours needs range guard Same normalise path clamps weekly_hours to 0..168 (168 = hours in a week, the largest value with physical meaning). Non-numeric or out-of-range values normalise to NULL. HTML input also gains `max="168"` so browser-level validation matches the repo guard. ## BLOCKER: migration 30522 missing from dbUpdates Restored 30522 to the run-array. It was present as a defined method (update_sql_30522 for canvas-items schema) but omitted from the run list — an install stuck at db-version 3.5.21 would jump to 3.5.23 without running it. Migration is idempotent (Schema::hasColumn guards) so re-adding is safe. ## Test plan - [x] Crafted POST with employment_type='ATTACKER_STRING' + weekly_hours='99999' → both persist as NULL. - [x] Crafted POST with employment_type='fte' + weekly_hours='40' → persists correctly. - [x] Migration 30523 still runs on this branch. - [ ] Migration 30522 runs when upgrading from db-version 3.5.21 (needs test install at 3.5.21). ## Not yet done (Marcel's other blockers, follow-up PR) - Persistence unit tests (NULL preservation on empty, partial-update contract via array_key_exists, int coercion, enum validation). - Enum unit tests (Volunteer excluded from capacity, tone mapping). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tics Addresses Marcel's blocker CR #1 on PR #3653 — the persistence layer now has automated coverage for the NULL-preservation / int-coercion / partial-update contracts that downstream capacity math will trust. ## tests/Unit/app/Domain/Users/Contracts/EmploymentTypeTest.php - Volunteer is the only case excluded from capacity math (countsAgainstCapacity()=false); every other case counts. - Over-cap tone maps cleanly: FTE=warn (target, burnout signal), PT/Contractor=danger (violates explicit ceiling / billable cap), Volunteer=none (best-effort, never fires). - tryFrom() rejects arbitrary strings (this is the write-path validation surface — if it starts coercing, the repo guard opens a store-garbage escape hatch). - tryFrom() accepts the four canonical values. - Every case has a label + i18n key with the enum value as the suffix (mismatched suffixes would render as raw keys in the admin select). ## tests/Unit/app/Domain/Users/Repositories/UsersRepositoryTest.php - Partial-update contract: values omitted from the payload MUST NOT be written — the array_key_exists guard is what protects a form that only edits `name` from clobbering someone else's capacity setup. - Empty string ('') and null both persist as NULL, not 0. The "not configured" state is meaningful — it's what suppresses over-cap warnings until an admin sets a real value. - weekly_hours boundary behavior: '0' and '168' inclusive, everything outside → NULL. 168 = hours in a week (largest value with physical meaning). Non-numeric strings ('forty') also → NULL. - employment_type: all four EmploymentType values round-trip; unknown strings (including 'ATTACKER_STRING', 'FTE' with wrong case, 'full-time', 'admin') all normalise to NULL — the exact IDOR-adjacent garbage-string scenario Marcel flagged. - Tests use a testable subclass that bridges to the parent's private normalizers via reflection, so the test exercises the SAME code path production uses, not a drift-prone copy. ## Test run 11 tests / 22 assertions in UsersRepositoryTest — all pass. 5 tests / 28 assertions in EmploymentTypeTest — all pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses Marcel's nit on PR #3653 — Contracts/ conventionally implies interfaces, but EmploymentType is a value enum. Discoverability is better under Users\Enums\ where PHP enums live throughout the codebase. Pure rename — no behavior change. Namespace updates in: - app/Domain/Users/Enums/EmploymentType.php (declaration) - app/Domain/Users/Repositories/Users.php (use import) - app/Domain/Users/Templates/editUser.blade.php (FQN in @foreach) - tests/Unit/app/Domain/Users/Enums/EmploymentTypeTest.php (namespace + use) - tests/Unit/app/Domain/Users/Repositories/UsersRepositoryTest.php (use) ## Test run - EmploymentTypeTest: 5 tests / 28 assertions — all pass. - UsersRepositoryTest: 11 tests / 22 assertions — all pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Status: needs-changes (progressed) · Priority: P2 · Next action: add the deferred persistence + enum unit tests · Owner: @gloriafolaron, review gate @marcelfolaron / @broskees Automated maintainer re-review (advisory only — not an approval; merge authority stays with @marcelfolaron / @broskees). Re-reviewed at head Intent: Slice 1 of user capacity modelling — adds nullable Progress since last review — 3 of 4 prior items resolved:
Remaining change requests:
Acceptance checklist (must pass before merge):
CI status: Advisory review only — not an approval, not marking ready, not merging. Merge authority stays with @marcelfolaron / @broskees. |
This pull request is one approval away from merging an auth bypass.Leantime/leantime · PR #3653 — feat(users): weekly_hours + employment_type on user (Slice 1) — was open right now. We ran PR Quorum's review panel over the same diff, read-only, live. Total cost: $0.0048. Here are the catches: 🟠 Server-side authorization missing for capacity field writes
In Leantime/leantime · feat(users): weekly_hours + employment_type on user (Slice 1). The blade template gates the capacity UI section behind
🟡 Tests rely on copied editUser logic via anonymous subclass, creating maintenance drift
In Leantime/leantime · feat(users): weekly_hours + employment_type on user (Slice 1). The UsersRepositoryTest creates an anonymous subclass that duplicates the real editUser method's updateData construction logic (lines 174–202). While it bridges to the private normalizers via reflection, the core partial-update guard (array_key_exists logic) is reimplemented rather than exercised through the actual production method. Any future change to editUser (adding a new field, altering normalization order) will not automatically cause these tests to fail, creating a false sense of coverage. Prefer to test via the public interface by mocking the DB connection, or extract the normalization and partial-update logic into testable helpers. The receipts
Leantime/leantime merges hundreds of pull requests a year. At about $0.0048 a review, checking every one of them costs a few dollars.
|
|
Status: needs-decision · Priority: P2 · Next action: confirm server-side admin gate before merge · Owner: @marcelfolaron / @broskees Daily maintenance sweep flagging a security-sensitive point raised in the third-party comment above (the vendor pitch aside, the underlying claim is worth a real answer). Leaving this for human decision — not auto-fixing, per the security-first rule. Root-cause analysis The capacity fields are gated in the UI only, not confirmed at the server boundary:
Impact: if Acceptance checklist (must pass before merge)
If |
Closes Marcel's needs-decision item on PR #3653 — the auth-bypass claim (weekly_hours / employment_type gated only in the UI, not at the server boundary) is INVALID. Every write surface already carries #[RequiresPermission(UsersPermissions::EDIT, global: true)] and `users.edit` is granted only to admin+ by DefaultRolePermissions. But there was no test guarding that. The three attributes could be silently removed by a future refactor and no unit test would fail. Same for a future default-permission change that hands users.edit to a lower role. Either would silently reopen the bypass. ## Coverage — 5 tests / 18 assertions - EditUser::post (controller — legacy convention route entry) requires users.edit, global. - EditUser::get (controller) requires users.edit, global — protects the admin edit form itself from a non-admin viewer. - Users\Services\Users::editUser (service — where the write actually happens) requires users.edit, global. - Users\Services\Users::updateUser (service — JSON-RPC entry point) requires users.edit, global. This is what secures the RPC path since RPC bypasses the controller gate. - DefaultRolePermissions grants users.edit ONLY to admin + owner. Manager, editor, commenter, readonly are asserted to NOT have it. ## Enforcement chain the tests are guarding - Legacy routes: Frontcontroller::executeAction reads the attribute. - Laravel routes: CheckPermissions middleware reads the attribute. - JSON-RPC: Jsonrpc::executeApiRequest reads the attribute on the resolved service method. - All three throw AuthorizationException on denial, rendered as 403 (web) or -32001 (RPC). ## Approach Reflection-based — reads the attribute on each method + asserts the permission key + global flag. No app boot, no HTTP round-trip. Catches the exact regression Marcel's reviewer flagged (attribute removal) without the cost of a full HTTP integration test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Responding to @marcelfolaron's daily sweep ( Auth-gate concern — the server-side gate exists, on three surfacesThe Blade
So the reviewer's scenario ("a non-admin adds Landed a regression test covering exactly this — commit
Any refactor that strips the attribute OR loosens the default grant now fails a unit test. Persistence + enum tests — they're on this branch alreadyThe 07-17 automated re-review still lists item CR#1 (persistence + enum unit tests) as a blocker. Those tests were added earlier and are on the branch at
Together with today's CR#1 in the 07-17 re-review's acceptance checklist should already be satisfied — likely the automated review parsed my security commit's body (which said "persistence + enum unit tests will land in a follow-up PR") and didn't notice the next commit Base branch (CR#2) — confirming intentYes, 30522 upgrade-from-3.5.21 (CR#3)Fair — I verified 30522 → 30523 runs on the current db-version (3.5.22) but not the fresh-from-3.5.21 case, since I couldn't easily pin a test install at 3.5.21 locally. If someone wants to spin one up before merge, happy to walk through it, but the migration is 🤖 Response generated with Claude Code |
There was a problem hiding this comment.
Pull request overview
This PR introduces two new, nullable user “capacity” attributes (weekly_hours, employment_type) on zp_user, along with normalization/validation on persistence, an admin-only edit UI, and supporting unit tests—laying groundwork for future resource-planning capacity math without changing runtime behavior yet.
Changes:
- Adds DB update
update_sql_30523(and updates fresh-install schema + db version) to includeweekly_hoursandemployment_type. - Persists the new fields in
Usersrepository with partial-update semantics (array_key_exists) and input normalization. - Adds
EmploymentTypeenum + admin-only UI fields on the edit-user screen, plus unit tests covering normalization and permission gating.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/app/Domain/Users/Repositories/UsersRepositoryTest.php | Adds unit tests for partial-update behavior and normalizers for the new capacity fields. |
| tests/Unit/app/Domain/Users/Enums/EmploymentTypeTest.php | Tests enum semantics used by downstream capacity logic (counts/tone/keys). |
| tests/Unit/app/Domain/Users/EditUserAuthorizationTest.php | Guards that server-side permissions continue to protect the edit surfaces for user updates. |
| app/Language/en-US.ini | Adds labels/hints and enum i18n strings for the new UI fields. |
| app/Domain/Users/Templates/editUser.blade.php | Adds admin-only “Capacity” section for weekly hours + employment type. |
| app/Domain/Users/Repositories/Users.php | Persists new fields and adds normalization helpers for safe storage. |
| app/Domain/Users/Enums/EmploymentType.php | Introduces the EmploymentType enum and helper methods used by capacity logic. |
| app/Domain/Users/Controllers/EditUser.php | Wires new fields into form value hydration and POST payload shaping (preserving partial updates). |
| app/Domain/Install/Repositories/Install.php | Adds migration 30523 and updates install SQL to include new columns on fresh installs. |
| app/Core/Configuration/AppSettings.php | Bumps dbVersion to 3.5.23. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $r = new \ReflectionMethod(UsersRepository::class, 'normalizeWeeklyHours'); | ||
|
|
||
| return $r->invoke($this, $value); |
| $r = new \ReflectionMethod(UsersRepository::class, 'normalizeEmploymentType'); | ||
|
|
||
| return $r->invoke($this, $value); |
| if (! is_numeric($value)) { | ||
| return null; | ||
| } | ||
| $hours = (int) $value; |
Copilot review on #3653: normalizeWeeklyHours() cast any numeric string to int, so 40.5 silently persisted as 40. Rounds instead. Copilot suggested rejecting non-integers, but NULL means "not configured" and removes the person from capacity math entirely — a worse outcome than the truncation being fixed. The UI field is a step-1 number input, so fractions only arrive via the API, where rounding stays closest to caller intent. Documented the underlying limitation: an int column can't represent genuinely fractional contracts (37.5 h/wk part-time is common); storing minutes or widening the column is the real fix if that case matters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GYSPNgLdUwEnxgYqTxMc85
|
Copilot's 3 comments — 1 fixed, 2 dismissed as false positives. Commit Fixed — Implemented as rounding, not rejection. Copilot suggested validating for integer strings and rejecting otherwise, but a rejected value becomes Also documented the real limitation while I was in there: an int column can't represent genuinely fractional contracts (37.5 h/wk part-time is common). Storing minutes or widening the column is the actual fix if that case matters — flagging rather than silently deciding it. Dismissed — the two class T { private function secret(int $x): int { return $x * 2; } }
(new ReflectionMethod(T::class, 'secret'))->invoke(new T, 21); // => 42, no setAccessibleThe tests exercise the production normalizer correctly as written. |
Adds two nullable capacity attributes to zp_user. Ships the model in isolation — nothing reads these values yet.
Migration update_sql_30523 adds weekly_hours + employment_type (nullable, no defaults, no backfill).
Enum EmploymentType (FTE / PT / Contractor / Volunteer) with countsAgainstCapacity() + overCapTone() helpers.
Repository editUser + addUser accept + persist the fields using array_key_exists (partial updates preserve existing values).
UI editUser.blade.php Capacity section gated behind Roles::admin.
Design decisions
Test plan
🤖 Generated with Claude Code