[Merge first!]: Laravel and refactor to thin Controllers (app code only) by nielsdrost7 · Pull Request #459 · Bottelet/DaybydayCRM

nielsdrost7 · 2026-07-25T06:32:36Z

Summary

Application-code portion of #455, split out into its own PR (merge this one first — #460 stacks on top of it) so the Laravel 12 upgrade and refactor can be reviewed independently of the test-infrastructure changes.

133 files changed (+9,837 / -1,836): controllers, services, requests, models, migrations, views, and the config/env/composer files directly coupled to those changes, plus a small number of pre-existing tests whose assertions were stale against this branch's own app-code changes.

What's in here

Laravel 12 upgrade. Fat controllers refactored to delegate to existing, previously-unused service classes. Routes use [Controller::class, 'method'] syntax throughout instead of magic route strings. rector.php modernized to target Laravel 12.
Elasticsearch removed. Abandoned integration pulled out of composer.json, config/services.php, and its observer — dead code, not a functional change.
Test environment hygiene. .env.testing is no longer committed; it's generated from .env.testing.example. SQLite removed from the test config in favor of real MySQL, so tests exercise the same engine as production.
Security consistency fix. RolesController's constructor gated every role-management action behind the user.is.admin middleware except store — creating a role was the one action reachable without that check (the route still required auth via StoreRoleRequest::authorize(), but through a different, less consistent path than every sibling action). store is now in the same only list as the rest.
Bug fix. AbsenceReason's constructor now falls back to the reason string when no display value is given, instead of returning null from a method typed to return string (was a latent TypeError on read).
Three new locales. French, Indonesian, and Polish translation files (resources/lang/{fr,id,pl}.json), each pulled directly from its source PR (Add french support #266, Add indonesian translation & currency, add some refactoring #302, Added polish translation #346) as a locale-file blob only — no other changes from those PRs (no blade wiring, no Country/Currency enum edits, no incidental edits to existing lang files).
9 pre-existing tests repaired where this branch's own changes (enum value changes, FormRequest field renames, validation-response-shape changes, exception-handler behavior) had made their assertions stale.

Explicitly out of scope (lives in #460 instead)

The Playwright e2e suite, the reorganized/hardened PHPUnit suite, and root *.md docs — pure test-infrastructure/tooling reviewed separately as "does the suite pass" rather than line-by-line.

Test plan

Full PHPUnit suite green (985/985) on this branch
CI (phpunit workflow) passes against this branch
Manual smoke test of the refactored controllers (Absence, Appointments, Clients, Comment, Departments, Documents, Invoices, Leads, Offers, Pages, Payments, Products, Projects, Roles, Settings, Tasks, Users)
Confirm elasticsearch removal doesn't break search (replaced by SearchService/SearchableTrait)

Pulls the parts of #459 that are pure config/tooling and don't depend on any of its still-under-review app code, so they can land independently and shrink that PR's diff: - .coderabbit.yml (new) - rector.php: fluent RectorConfig::configure() API, Laravel set bumped from LARAVEL_80 to LaravelLevelSetList::UP_TO_LARAVEL_120 - vite.config.mjs: fixes Vue never mounting (broken vite-plugin-vue2 alias) plus jquery externalization — verified buildable standalone against develop's current app code - .env.example / .env.ci: adds inert TOUR_DISABLED (unused by any code yet, safe regardless of branch) - .env.testing -> .env.testing.example, gitignore the real file, and regenerate it verbatim in CI (phpunit.yml) before tests run, since ProjectFilesConfigurationTest asserts on its exact content Deliberately NOT touched: .env.ci's CACHE_STORE stays "array" here (the "database" value needs the create_cache_table migration, which is still only on #459) and phpunit.xml's SQLite <env> override stays out entirely (needs the DropColumnsIfExist.php Doctrine-API fix, also still only on #459) - both would break develop's current CI if merged ahead of their app-code dependency.

Test-infrastructure portion of feature/playwright-tests, split out from PR Bottelet#455/Bottelet#459 for separate review: the full PHPUnit reorg (Feature/Unit split, new coverage) plus the Playwright e2e suite (132 tests) and its config. Stacked on feature/laravel12-refactor (Bottelet#459) because a portion of these tests exercise app-code fixes that only exist on that branch (Setting::cached(), the Currency USD-separator fix, the new task/lead/project edit routes, etc.) — without it, ~34 tests fail.

…from code review - Restored tests/Feature/Clients/ClientAuthorizationTest.php and ClientPerformanceTest.php, deleted during the Feature/Unit consolidation with no replacement anywhere - lost the only test gating client-delete-by-permission and 6 N+1-query regression tests. Also fixed their non-it_-prefixed method names (userWithoutClientCreatePermission... -> it_...) and added missing : void return types while restoring. - AbstractTestCase::setUp() silently skipped user creation/auth setup if the `users` table didn't exist after its (SQLite-workaround) migrate:fresh-skip logic, rather than failing loud. Now throws a clear RuntimeException explaining what to check, instead of letting downstream tests fail confusingly on a null $this->user. Also updated the stale SQLite-specific comment now that SQLite is eliminated from this suite - the skip-when-RefreshDatabase-is-used behavior itself is still correct (avoids a redundant/conflicting second migration), just the old comment's specific justification no longer applied. - Restored it_posting_to_appointments_resource_route_returns_not_found into AppointmentsTest.php - the one test with real regression value (asserts POST /appointments still 404s/405s) that got dropped when AppointmentsStoreRemovedTest.php was deleted. Left the two dropped method_exists() reflection checks out, per review. - CommentControllerTest had zero negative-authorization coverage. Investigated: StoreCommentRequest::authorize() only checks auth()->check() on both develop and this branch (unchanged - not a regression), and no COMMENT_* permission exists anywhere in this app, consistent with its permission-based (not per-resource- ownership) authorization model elsewhere. Added tests documenting that actual behavior (any authenticated user can comment on any task) plus the unauthenticated-request 401/403 case, rather than inventing new authorization logic that isn't part of this app's existing model. - Fixed : void return type on the 5 test methods added earlier this session (LeadsTest.php x3, OffersTest.php x2). Fixing two pre-existing tests broken by Bottelet#459's Handler::render() change (AuthorizationException now redirects+flashes for non-JSON requests instead of returning a raw 403): ClientAuthorizationTest's storeClient permission-denial test and UsersTest's it_only_owner_role_can_update_user both asserted the old 403 status; updated to assert the new (intentionally improved) redirect+flash behavior. Also fixed my own new CommentControllerTest assertion that didn't account for CommentService's clean() wrapping plain text in <p> tags. Verified: full suite (950 tests, 2334 assertions) passes against real MySQL.

Test-infrastructure portion of feature/playwright-tests, split out from PR Bottelet#455/Bottelet#459 for separate review: the full PHPUnit reorg (Feature/Unit split, new coverage) plus the Playwright e2e suite (132 tests) and its config. Stacked on feature/laravel12-refactor (Bottelet#459) because a portion of these tests exercise app-code fixes that only exist on that branch (Setting::cached(), the Currency USD-separator fix, the new task/lead/project edit routes, etc.) — without it, ~34 tests fail.

…from code review - Restored tests/Feature/Clients/ClientAuthorizationTest.php and ClientPerformanceTest.php, deleted during the Feature/Unit consolidation with no replacement anywhere - lost the only test gating client-delete-by-permission and 6 N+1-query regression tests. Also fixed their non-it_-prefixed method names (userWithoutClientCreatePermission... -> it_...) and added missing : void return types while restoring. - AbstractTestCase::setUp() silently skipped user creation/auth setup if the `users` table didn't exist after its (SQLite-workaround) migrate:fresh-skip logic, rather than failing loud. Now throws a clear RuntimeException explaining what to check, instead of letting downstream tests fail confusingly on a null $this->user. Also updated the stale SQLite-specific comment now that SQLite is eliminated from this suite - the skip-when-RefreshDatabase-is-used behavior itself is still correct (avoids a redundant/conflicting second migration), just the old comment's specific justification no longer applied. - Restored it_posting_to_appointments_resource_route_returns_not_found into AppointmentsTest.php - the one test with real regression value (asserts POST /appointments still 404s/405s) that got dropped when AppointmentsStoreRemovedTest.php was deleted. Left the two dropped method_exists() reflection checks out, per review. - CommentControllerTest had zero negative-authorization coverage. Investigated: StoreCommentRequest::authorize() only checks auth()->check() on both develop and this branch (unchanged - not a regression), and no COMMENT_* permission exists anywhere in this app, consistent with its permission-based (not per-resource- ownership) authorization model elsewhere. Added tests documenting that actual behavior (any authenticated user can comment on any task) plus the unauthenticated-request 401/403 case, rather than inventing new authorization logic that isn't part of this app's existing model. - Fixed : void return type on the 5 test methods added earlier this session (LeadsTest.php x3, OffersTest.php x2). Fixing two pre-existing tests broken by Bottelet#459's Handler::render() change (AuthorizationException now redirects+flashes for non-JSON requests instead of returning a raw 403): ClientAuthorizationTest's storeClient permission-denial test and UsersTest's it_only_owner_role_can_update_user both asserted the old 403 status; updated to assert the new (intentionally improved) redirect+flash behavior. Also fixed my own new CommentControllerTest assertion that didn't account for CommentService's clean() wrapping plain text in <p> tags. Verified: full suite (950 tests, 2334 assertions) passes against real MySQL.

…p-code changes feature/laravel12-refactor (Bottelet#459) is the "app-code only" half of a PR split, so its own diff never touches tests/ - but several of the app-code changes on this branch changed real behavior, and the inherited (pre-split) copies of the tests exercising that behavior were never updated to match. Running the full suite standalone on this branch (rather than only via Bottelet#460, which already carries fixed copies of these files) surfaced 3 errors + 18 failures, all traced to one of four root causes: - AbsenceReason enum values changed from freeform strings ('Sick') to a fixed set ('sick_leave' etc.) - AbsenceControllerTest still sent the old value, so every absence-creation request now fails validation before reaching the code path under test. (tests/Feature/Absenses/AbsenceControllerTest.php) - StoreUserRequest's role/department validation keys were fixed from the never-actually-sent plural 'roles'/'departments' to the singular 'role'/'department' that the form actually submits (see the fix's own comment in the class) - UsersControllerTest's payload helper still built the old plural keys, so user-creation requests failed validation before ever reaching the exception path the two affected tests exist to exercise. Fixing the payload also resolved the "did not remove its own error/exception handlers" risky-test warnings on both, which were a downstream symptom of the request never reaching the code that installs and removes them. (tests/Feature/Users/UsersControllerTest.php) - Status-update validation moved from a manual check + custom 400/session-flash response to a proper FormRequest, which Laravel turns into an automatic 422 (JSON) or redirect+assertSessionHasErrors (web) response - four tests across two controllers still asserted the old 400/flash_message_warning shape. (tests/Feature/Projects/ProjectSecurityTest.php, tests/Feature/Tasks/TaskSecurityTest.php) - The exception Handler now converts a FormRequest's authorize() failure into a flash+redirect-back for non-JSON requests (matching the rest of the app's permission-denial pattern) instead of Laravel's generic 403 page - one test still asserted the old assertForbidden() shape. (tests/Feature/Clients/ClientAuthorizationTest.php) Two more failures were pre-existing test bugs unrelated to any behavior change: - ClientServiceTest::it_gets_{tasks,projects,leads}_with_relations called assertCount() directly on the HasMany relation builder returned by ClientService (never resolved with ->get()), which throws a TypeError under this PHPUnit/Laravel version rather than counting the underlying query. - DepartmentsControllerTest::it_can_create_department and OfferAuthorizationTest's won/lost tests sent requests via $this->json(), which sets an Accept: application/json header and made the controllers take their JSON-response branch (200/201) instead of the redirect branch (302) the tests assert - switched to plain $this->post(). Also fixed, unrelated to any of the above: ProjectFilesConfigurationTest asserted .env.ci's CACHE_STORE must be "array", but .env.ci is intentionally "database" (it drives real HTTP requests in the Playwright suite, and "array" never serializes cached values - see the updated test's own message for the specific failure mode this guards against). The test's expectation was stale, not the config; flipped the assertion and renamed the test to match. Brittleness note for the record: none of these are hardcoded-count style brittle tests (that pattern exists elsewhere, e.g. CountryTest's hardcoded country count - flagged separately, not touched here) - all nine were behavior assertions that fell out of sync with intentional app-code changes because the test-fixing work for this refactor landed exclusively on the stacked tests PR instead of also being back-ported here. Verified: full suite standalone on this branch, 895 tests, 2191 assertions, 0 errors, 0 failures (9 pre-existing notices/8 incomplete unrelated and unchanged).

Test-infrastructure portion of feature/playwright-tests, split out from PR Bottelet#455/Bottelet#459 for separate review: the full PHPUnit reorg (Feature/Unit split, new coverage) plus the Playwright e2e suite (132 tests) and its config. Stacked on feature/laravel12-refactor (Bottelet#459) because a portion of these tests exercise app-code fixes that only exist on that branch (Setting::cached(), the Currency USD-separator fix, the new task/lead/project edit routes, etc.) — without it, ~34 tests fail.

…from code review - Restored tests/Feature/Clients/ClientAuthorizationTest.php and ClientPerformanceTest.php, deleted during the Feature/Unit consolidation with no replacement anywhere - lost the only test gating client-delete-by-permission and 6 N+1-query regression tests. Also fixed their non-it_-prefixed method names (userWithoutClientCreatePermission... -> it_...) and added missing : void return types while restoring. - AbstractTestCase::setUp() silently skipped user creation/auth setup if the `users` table didn't exist after its (SQLite-workaround) migrate:fresh-skip logic, rather than failing loud. Now throws a clear RuntimeException explaining what to check, instead of letting downstream tests fail confusingly on a null $this->user. Also updated the stale SQLite-specific comment now that SQLite is eliminated from this suite - the skip-when-RefreshDatabase-is-used behavior itself is still correct (avoids a redundant/conflicting second migration), just the old comment's specific justification no longer applied. - Restored it_posting_to_appointments_resource_route_returns_not_found into AppointmentsTest.php - the one test with real regression value (asserts POST /appointments still 404s/405s) that got dropped when AppointmentsStoreRemovedTest.php was deleted. Left the two dropped method_exists() reflection checks out, per review. - CommentControllerTest had zero negative-authorization coverage. Investigated: StoreCommentRequest::authorize() only checks auth()->check() on both develop and this branch (unchanged - not a regression), and no COMMENT_* permission exists anywhere in this app, consistent with its permission-based (not per-resource- ownership) authorization model elsewhere. Added tests documenting that actual behavior (any authenticated user can comment on any task) plus the unauthenticated-request 401/403 case, rather than inventing new authorization logic that isn't part of this app's existing model. - Fixed : void return type on the 5 test methods added earlier this session (LeadsTest.php x3, OffersTest.php x2). Fixing two pre-existing tests broken by Bottelet#459's Handler::render() change (AuthorizationException now redirects+flashes for non-JSON requests instead of returning a raw 403): ClientAuthorizationTest's storeClient permission-denial test and UsersTest's it_only_owner_role_can_update_user both asserted the old 403 status; updated to assert the new (intentionally improved) redirect+flash behavior. Also fixed my own new CommentControllerTest assertion that didn't account for CommentService's clean() wrapping plain text in <p> tags. Verified: full suite (950 tests, 2334 assertions) passes against real MySQL.

…xes, and 3 new locales - Upgrade the application to Laravel 12; refactor fat controllers to delegate to existing, previously-unused service classes. - Use [Controller::class, 'method'] route syntax throughout instead of magic route strings; modernize rector.php to target Laravel 12. - Stop committing .env.testing; generate it from .env.testing.example instead, and eliminate SQLite from the test suite in favor of real MySQL so tests exercise the same engine as production. - Address Critical/Important findings from a senior-Laravel code review pass across controllers, views, and FormRequests. - Fix AbsenceReason's constructor to fall back to the reason string when no display value is given, avoiding a TypeError on read. - Add French, Indonesian, and Polish translation files, each pulled directly from its source PR's locale blob with no other changes from those PRs (no blade wiring, no unrelated app-code changes). - Repair 9 tests whose assertions had gone stale against this branch's own app-code changes (enum value changes, FormRequest field renames, validation-response-shape changes, exception-handler behavior).

nielsdrost7 · 2026-07-26T05:56:52Z

@coderabbitai full review

coderabbitai · 2026-07-26T05:56:56Z

✅ Action performed

Full review finished.

anyData(), taskDataTable(), projectDataTable(), and leadDataTable() still built their linked columns from raw Blade-template strings interpolating $company_name/$title directly, unlike every sibling controller (Clients, Projects, Tasks, Leads, Users), which already use e()-escaped closures. Converted to the same closure + e() pattern used elsewhere in this codebase.

Ported from feature/playwright-tests-suite (excludes its Roles/Clients route-naming and middleware changes, which stay scoped to that branch): - Absence: redirect to dashboard instead of the view-gated absence index when the acting user can create but not view absences, so the success flow doesn't immediately bounce through a second permission check. The existing JSON-request tests are untouched (that branch short-circuits before the redirect); added a new non-JSON test that actually exercises the redirect path. - Documents: extract the repeated file-size-in-MB calculation into a helper, and return the JSON response client uploads were missing (uploadToTask/uploadToProject already returned it; upload() fell through with no response, which Laravel can't turn into a valid HTTP response). - Invoices: wrap invoice line creation in a DB transaction so a mid-loop validation failure rolls back the whole batch instead of leaving partial lines; use firstOrFail() for lookups that were crashing instead of 404ing on a bad external_id. New AddInvoiceLineTest covers the single and batch endpoints, including the rollback case. - Offers: wrap invoice line replacement (delete + recreate) in a DB transaction for the same reason. - Users/Tasks/Invoices: guard Setting::cached() being null (no settings row yet, e.g. fresh installs/tests) instead of crashing on ->company/->max_users. New UserServiceTest covers the image-upload path throwing when no settings exist. - Migration: add the invoice_lines/offers foreign keys that were dropped during the SQLite table-rebuild workaround. Verified: the specific tests covering these changes (17 tests) and the full suite (899 tests, 2197 assertions, 0 failures - the 8 incomplete/9 notices are pre-existing and unrelated) both pass clean.

This was referenced Jul 25, 2026

Add Playwright e2e suite and reorganize PHPUnit tests #460

Draft

Upgrade Laravel and refactor to thin Controllers (to develop branch) #455

Closed

nielsdrost7 marked this pull request as draft July 25, 2026 06:45

nielsdrost7 changed the title ~~Upgrade Laravel and refactor to thin Controllers (app code only)~~ [Merge first!]: Laravel and refactor to thin Controllers (app code only) Jul 25, 2026

This comment was marked as outdated.

Sign in to view

Repository owner deleted a comment from coderabbitai Bot Jul 25, 2026

This was referenced Jul 25, 2026

rtl support and persian translate #257

Closed

Add french support #266

Closed

Add indonesian translation & currency, add some refactoring #302

Closed

Added polish translation #346

Closed

nielsdrost7 force-pushed the feature/laravel12-refactor branch from 23b6ecf to afe8013 Compare July 26, 2026 03:50

nielsdrost7 force-pushed the feature/laravel12-refactor branch from afe8013 to 0d56dca Compare July 26, 2026 04:03

This comment was marked as outdated.

Sign in to view

Repository owner deleted a comment from coderabbitai Bot Jul 26, 2026

Delete resources/.DS_Store

6df7f32

nielsdrost7 and others added 3 commits July 26, 2026 20:37

Delete docs directory

470a359

Navigation Menu

Uh oh!

[Merge first!]: Laravel and refactor to thin Controllers (app code only) - #459

[Merge first!]: Laravel and refactor to thin Controllers (app code only)#459
nielsdrost7 wants to merge 5 commits into
Bottelet:developfrom
underdogg-forks:feature/laravel12-refactor

nielsdrost7 commented Jul 25, 2026 •

edited

Loading

Uh oh!

This comment was marked as outdated.

Uh oh!

nielsdrost7 commented Jul 26, 2026

Uh oh!

coderabbitai Bot commented Jul 26, 2026 •

edited

Loading

Uh oh!

This comment was marked as outdated.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

nielsdrost7 commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What's in here

Explicitly out of scope (lives in #460 instead)

Test plan

Uh oh!

This comment was marked as outdated.

Uh oh!

nielsdrost7 commented Jul 26, 2026

Uh oh!

coderabbitai Bot commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

nielsdrost7 commented Jul 25, 2026 •

edited

Loading

coderabbitai Bot commented Jul 26, 2026 •

edited

Loading