feat(pay-rule-sets): non-admin selection + pay-code columns on empty periods - #1674
Merged
renemadsen merged 2 commits intoAug 7, 2026
Merged
Conversation
… columns on empty periods Selecting a pay rule set for an assigned site required the Admin role, so a non-admin could not set it at all: GET pay-rule-sets was [Authorize(Roles = Admin)] and its 403 was escalated by the global HttpErrorInterceptor into a forced logout, which is why the UI hid the selector entirely. Listing and reading a rule set are now open to any authenticated user; creating, editing and deleting stay admin-only. The selector is ungated in the assigned-site dialog, and the Pay Rule Sets page hides its create/edit/delete actions for non-admins so the page — now reachable for them — cannot hit an admin-only endpoint and log them out. Note the write path was never admin-gated: the assigned-site PUT already wrote PayRuleSetId for anyone with time_planning_working_hours_get, so this closes a read/write asymmetry rather than widening what can be changed. Excel exports built their pay-code column list from the pay lines OBSERVED in the selected period, so a period with no registered time produced no pay-code columns. Both exports now seed from the codes DECLARED by the site's pay rule set (the all-workers per-site sheets already did), keeping the observed codes as a defensive union. Column order therefore follows the rule set's structure rather than first-observed order — payroll imports that read by column position rather than header name need checking. Also filters soft-deleted rules out of both export eager-loads. PayDayRule, PayTierRule, PayDayTypeRule and PayTimeBandRule are soft-deleted, but neither the loads nor PayLineGenerator filtered WorkflowState, so a tier the admin had already deleted still produced columns AND still took part in pay calculation. Tests: reflection tests pinning the authorization split, three empty-period export tests, and an updated Jest spec (it asserted the old non-admin behaviour). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Enables non-admin users to select (and view) pay rule sets while keeping pay rule set authoring admin-only, and updates Excel exports so pay-code columns are present even when the selected period contains no registrations (plus filtering out soft-deleted rule components).
Changes:
- Backend: relax
PayRuleSetControllerGET authorization (authenticated-only), retain admin-only mutations; export loads now filter out removed day rules/tiers/time bands and seed columns from declared pay codes. - Frontend: expose pay rule set selector for non-admins; hide pay rule set create/edit/delete UI for non-admins to avoid 403→logout behavior.
- Tests: add controller authorization reflection tests and add empty-period export coverage (including total sheet behavior).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs | Seeds export pay-code columns from declared rule-set codes and filters out removed rule components; hardens totals lookup. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Controllers/PayRuleSetController.cs | Removes admin role requirement from Index/Read while keeping class-level auth and admin-only mutations. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PayRuleSetControllerTests.cs | Adds reflection tests pinning auth split (class-level auth, GET open, mutations admin-only). |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DagsoversigtWorksheetExportTests.cs | Adds tests ensuring declared pay-code columns exist for empty periods (single-worker and all-workers exports). |
| eform-client/src/app/plugins/modules/time-planning-pn/modules/pay-rule-sets/components/pay-rule-sets-table/pay-rule-sets-table.component.ts | Adds admin-check observable for UI gating of mutating actions. |
| eform-client/src/app/plugins/modules/time-planning-pn/modules/pay-rule-sets/components/pay-rule-sets-table/pay-rule-sets-table.component.html | Hides create/edit/delete actions for non-admins. |
| eform-client/src/app/plugins/modules/time-planning-pn/components/plannings/time-planning-actions/assigned-site/assigned-site-dialog.component.ts | Always fetches pay rule sets (now readable by any authenticated user). |
| eform-client/src/app/plugins/modules/time-planning-pn/components/plannings/time-planning-actions/assigned-site/assigned-site-dialog.component.html | Ungates the payroll rules selector from admin-only visibility. |
| eform-client/src/app/plugins/modules/time-planning-pn/components/plannings/time-planning-actions/assigned-site/assigned-site-dialog.component.spec.ts | Updates spec to assert non-admins also fetch pay rule sets. |
Suppressed comments (1)
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs:3266
- Same soft-delete concern as the single-worker export: PayRuleSetForCache is loaded by Id without filtering WorkflowState. This can reintroduce deleted rule sets into the all-workers export if AssignedSite.PayRuleSetId points at a removed row. Add a root WorkflowState != Removed filter to match PayRuleSetService behavior.
payRuleSetForCache = await dbContext.PayRuleSets
.Include(p => p.DayRules.Where(d => d.WorkflowState != Constants.WorkflowStates.Removed))
.ThenInclude(d => d.Tiers.Where(t => t.WorkflowState != Constants.WorkflowStates.Removed))
.Include(p => p.DayTypeRules.Where(d => d.WorkflowState != Constants.WorkflowStates.Removed))
.ThenInclude(d => d.TimeBandRules.Where(b => b.WorkflowState != Constants.WorkflowStates.Removed))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
2629
to
+2633
| payRuleSet = await dbContext.PayRuleSets | ||
| .Include(p => p.DayRules) | ||
| .ThenInclude(d => d.Tiers) | ||
| .Include(p => p.DayTypeRules) | ||
| .ThenInclude(d => d.TimeBandRules) | ||
| .Include(p => p.DayRules.Where(d => d.WorkflowState != Constants.WorkflowStates.Removed)) | ||
| .ThenInclude(d => d.Tiers.Where(t => t.WorkflowState != Constants.WorkflowStates.Removed)) | ||
| .Include(p => p.DayTypeRules.Where(d => d.WorkflowState != Constants.WorkflowStates.Removed)) | ||
| .ThenInclude(d => d.TimeBandRules.Where(b => b.WorkflowState != Constants.WorkflowStates.Removed)) |
The temp file was named from a second-precision UTC timestamp only, so two exports started within the same second resolved to the same path. The first export returns an open FileStream over that file, so the second one fails to create it and the whole export is reported as ErrorWhileCreatingExcelFile. Surfaced by the new empty-period test, which is the first test to call the single-worker export twice in a row, but it is a real collision: two users (or one user double-clicking) exporting in the same second hit it in production. Both exports now append a GUID. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent changes, both requested by @rene.
1. Non-admins can select a pay rule set
Setting a pay rule set on an assigned site required the Admin role.
GET api/time-planning-pn/pay-rule-setswas[Authorize(Roles = Admin)], and its 403 is escalated by the globalHttpErrorInterceptorinto a forced logout, which is why the UI hid the selector rather than letting it 403.IndexandReaddrop the role requirement (class-level[Authorize]retained — anonymous still blocked).Create/Update/Deletestay admin-only.Indexmakes that page reachable for them and its mutations would otherwise 403 → logout.Worth knowing: the write path was never gated — the assigned-site PUT already wrote
PayRuleSetIdfor anyone withtime_planning_working_hours_get. This closes a read/write asymmetry rather than widening what can be changed.Readreturns the full tier/time-band structure of the agreement (no monetary rates) to any authenticated user. Accepted deliberately; say the word if you'd rather keepReadadmin-only and hide the eye button.2. Pay-code columns survive an empty period
Both exports built their column list from pay lines observed in the period, so no registered time meant no pay-code columns. They now seed from the codes declared by the site's rule set (the all-workers per-site sheets already did this), with observed codes unioned in as a fallback.
Bonus fix found in review
Pay rules are soft-deleted, but neither the export eager-loads nor
PayLineGeneratorfilteredWorkflowState. A tier an admin had already deleted still produced columns and still took part in pay calculation. Both loads now filter removed rows.Tests
Reflection tests pinning the authorization split (shard d), three empty-period export tests (shard g), and an updated Jest spec that previously asserted the old non-admin behaviour.
Verified locally
Full-solution build clean. Against the running backend with a real non-admin user: 200 on list and read, 403 still on create and delete. An export for a period with zero registrations now carries all ten declared pay codes (
NORMAL,OVERTIME_30,OVERTIME_80,SAT_NORMAL,SAT_ANIMAL_AFTERNOON,ANIMAL_SUN_HOLIDAY,GRUNDLOVSDAG,ANIMAL_NIGHT,SHIFTED_MORNING,SHIFTED_EVENING) where it previously carried none.Merge order
This PR must merge before the matching backend-configuration PR. BC's CI checks out this repo at
ref: stableand builds it into the test container, so the BC change is red — and would regress production to the forced-logout bug — until this is onstable.🤖 Generated with Claude Code