psa-systems/mokosh-server
Watch
9
0
Fork
You've already forked mokosh-server
 1
Code Issues Pull requests Projects Releases 10 Packages Wiki Activity Actions

refactor: collapse CreateTicketRequest + RequireAdminUser + CurrentContact::tenant (PMS-479) #352

Merged
YousifShkara merged 1 commit from feat/PMS-479-refactor-boilerplate into main 2026-06-24 09:15:33 +02:00
Conversation 0 Commits 1 Files changed 12 +81 -143
YousifShkara commented 2026-06-24 08:56:32 +02:00
Owner
Copy link

Three small but compounding boilerplate sinks fell out of the PMS-448..PMS-457 grind: every PR touching CreateTicketRequest hit a merge conflict against the same per-feature optional-field expansion; every admin-only handler spelled out a RequireAuth(u): RequireAuth, _admin: RequireAdmin pair; every portal handler spelled out crate::modules::auth::TenantId::from_trusted(contact.tenant_id). None of these are hard to fix in isolation. Land them as one refactor PR so the boilerplate stops accumulating.

What landed (no behavioural changes; the existing test suites for workflow_rules, email_intake, intake_token_admin, portal, portal_ticket_notes all pass verbatim):

  1. CreateTicketRequest derives Default. Every internal struct-literal call site (rmm intake, email intake, seed/data, seed/qa, portal-ticket creation in tickets::service) collapses the noisy priority_id: None, type_id: None, ... tail into ..Default::default(). Adding a new optional field in a future PR no longer breaks these sites; they keep the new field at its default. The HTTP path still goes through Deserialize + Validate, which enforces the non-empty title + present company_id at the boundary, so external requests cannot exploit the default.

  2. New RequireAdminUser(pub CurrentUser) extractor in auth::middleware. The extractor delegates to RequireAdmin so the role list stays a single source of truth, then unwraps the tuple to drop the PhantomData tail consumers do not want. Every existing RequireAuth(u): RequireAuth, _admin: RequireAdmin pair in email_intake::routes (4 sites) and workflows::routes (6 sites) collapses to RequireAdminUser(u): RequireAdminUser. The time_tracking::routes admin sites use RequireTimeTracking + RequireAdmin (a different first half) and are deliberately left untouched - extending the pattern to module-gated admin extractors is its own scope.

  3. New CurrentContact::tenant() -> TenantId helper that wraps the verified portal-JWT tenant_id claim. The 8 verbose crate::modules::auth::TenantId::from_trusted(contact.tenant_id) call sites in portal::routes collapse to contact.tenant(). The ResolvedTenantToken from email-intake already stores tenant_id: TenantId (not a raw Uuid), so no helper is needed there.

#PMS-479

Three small but compounding boilerplate sinks fell out of the PMS-448..PMS-457 grind: every PR touching CreateTicketRequest hit a merge conflict against the same per-feature optional-field expansion; every admin-only handler spelled out a `RequireAuth(u): RequireAuth, _admin: RequireAdmin` pair; every portal handler spelled out `crate::modules::auth::TenantId::from_trusted(contact.tenant_id)`. None of these are hard to fix in isolation. Land them as one refactor PR so the boilerplate stops accumulating. What landed (no behavioural changes; the existing test suites for workflow_rules, email_intake, intake_token_admin, portal, portal_ticket_notes all pass verbatim): 1. `CreateTicketRequest` derives `Default`. Every internal struct-literal call site (rmm intake, email intake, seed/data, seed/qa, portal-ticket creation in `tickets::service`) collapses the noisy `priority_id: None, type_id: None, ...` tail into `..Default::default()`. Adding a new optional field in a future PR no longer breaks these sites; they keep the new field at its default. The HTTP path still goes through `Deserialize + Validate`, which enforces the non-empty title + present company_id at the boundary, so external requests cannot exploit the default. 2. New `RequireAdminUser(pub CurrentUser)` extractor in `auth::middleware`. The extractor delegates to `RequireAdmin` so the role list stays a single source of truth, then unwraps the tuple to drop the `PhantomData` tail consumers do not want. Every existing `RequireAuth(u): RequireAuth, _admin: RequireAdmin` pair in `email_intake::routes` (4 sites) and `workflows::routes` (6 sites) collapses to `RequireAdminUser(u): RequireAdminUser`. The `time_tracking::routes` admin sites use `RequireTimeTracking + RequireAdmin` (a different first half) and are deliberately left untouched - extending the pattern to module-gated admin extractors is its own scope. 3. New `CurrentContact::tenant() -> TenantId` helper that wraps the verified portal-JWT `tenant_id` claim. The 8 verbose `crate::modules::auth::TenantId::from_trusted(contact.tenant_id)` call sites in `portal::routes` collapse to `contact.tenant()`. The `ResolvedTenantToken` from email-intake already stores `tenant_id: TenantId` (not a raw Uuid), so no helper is needed there. #PMS-479
refactor: collapse CreateTicketRequest + RequireAdminUser + CurrentContact::tenant (PMS-479)
All checks were successful
E2E / Playwright against staging (pull_request) Successful in 53s
Details
Check / fmt + clippy + build + tests (pull_request) Successful in 2m44s
Details
Integration / integration tests (pull_request) Successful in 9m23s
Details
Create release / Create release from merged PR (pull_request) Successful in 1s
Details
13ca35dee8 
Three small but compounding boilerplate sinks fell out of the PMS-448..PMS-457 grind: every PR touching CreateTicketRequest hit a merge conflict against the same per-feature optional-field expansion; every admin-only handler spelled out a `RequireAuth(u): RequireAuth, _admin: RequireAdmin` pair; every portal handler spelled out `crate::modules::auth::TenantId::from_trusted(contact.tenant_id)`. None of these are hard to fix in isolation. Land them as one refactor PR so the boilerplate stops accumulating.

What landed (no behavioural changes; the existing test suites for workflow_rules, email_intake, intake_token_admin, portal, portal_ticket_notes all pass verbatim):

1. `CreateTicketRequest` derives `Default`. Every internal struct-literal call site (rmm intake, email intake, seed/data, seed/qa, portal-ticket creation in `tickets::service`) collapses the noisy `priority_id: None, type_id: None, ...` tail into `..Default::default()`. Adding a new optional field in a future PR no longer breaks these sites; they keep the new field at its default. The HTTP path still goes through `Deserialize + Validate`, which enforces the non-empty title + present company_id at the boundary, so external requests cannot exploit the default.

2. New `RequireAdminUser(pub CurrentUser)` extractor in `auth::middleware`. The extractor delegates to `RequireAdmin` so the role list stays a single source of truth, then unwraps the tuple to drop the `PhantomData` tail consumers do not want. Every existing `RequireAuth(u): RequireAuth, _admin: RequireAdmin` pair in `email_intake::routes` (4 sites) and `workflows::routes` (6 sites) collapses to `RequireAdminUser(u): RequireAdminUser`. The `time_tracking::routes` admin sites use `RequireTimeTracking + RequireAdmin` (a different first half) and are deliberately left untouched - extending the pattern to module-gated admin extractors is its own scope.

3. New `CurrentContact::tenant() -> TenantId` helper that wraps the verified portal-JWT `tenant_id` claim. The 8 verbose `crate::modules::auth::TenantId::from_trusted(contact.tenant_id)` call sites in `portal::routes` collapse to `contact.tenant()`. The `ResolvedTenantToken` from email-intake already stores `tenant_id: TenantId` (not a raw Uuid), so no helper is needed there.

#PMS-479
YousifShkara deleted branch feat/PMS-479-refactor-boilerplate 2026-06-24 09:15:34 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
YousifShkara
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
psa-systems/mokosh-server!352
No description provided.