Skip to content

Commit 0d56dca

Browse files
nielsdrost7nielsdrost7
committed
refactor: upgrade to Laravel 12 with thin controllers, code review fixes, 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).
1 parent 7cba44e commit 0d56dca

133 files changed

Lines changed: 9837 additions & 1836 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.coderabbit.yml‎

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
# =======================================================
2+
# CodeRabbit AI Review Configuration
3+
# For Laravel + JS projects
4+
# Reference: https://docs.coderabbit.ai/config
5+
# =======================================================
6+
7+
language: en-US
8+
9+
reviews:
10+
# Review tone — defines style of feedback
11+
# Options: assertive | chill | friendly | strict | concise | detailed
12+
profile: assertive
13+
14+
# Require resolution before merge
15+
request_changes_workflow: true
16+
17+
# Include an AI summary of the PR
18+
high_level_summary: false
19+
20+
# Report check status to GitHub UI
21+
review_status: false
22+
commit_status: false
23+
fail_commit_status: false
24+
25+
# Scope review to the app's own PHP source and its tests. Everything
26+
# else (Blade/JS/CSS assets, vendor lockfiles, CI/tooling config,
27+
# generated artifacts, docs) is noise for a Laravel PR review and
28+
# inflates the reviewed-file count without adding review value.
29+
path_filters:
30+
- '!vendor/**'
31+
- '!node_modules/**'
32+
- '!public/**'
33+
- '!storage/**'
34+
- '!bootstrap/cache/**'
35+
- '!resources/assets/**'
36+
- '!resources/views/**'
37+
- '!*.md'
38+
- '!**/*.md'
39+
- '!yarn.lock'
40+
- '!package-lock.json'
41+
- '!composer.lock'
42+
- '!.github/**'
43+
- '!.claude/**'
44+
- '!.junie/**'
45+
- 'app/**/*.php'
46+
- 'tests/**/*.php'
47+
- 'database/**/*.php'
48+
- 'routes/**/*.php'
49+
- 'config/**/*.php'
50+
51+
path_instructions:
52+
- path: 'app/**/*.php'
53+
instructions: |
54+
This is Laravel application code. Prioritize:
55+
- Missing or inconsistent authorization checks (controller
56+
constructor middleware, FormRequest::authorize(), and route
57+
middleware are all valid enforcement points — check all three
58+
before flagging something as unauthorized).
59+
- Mass assignment via fill($request->all()) instead of validated()
60+
or explicit field lists.
61+
- N+1 queries and missing eager loading, especially in anything
62+
feeding a DataTables/index endpoint.
63+
- Raw Eloquent models or user input echoed into Blade/JSON without
64+
escaping.
65+
- path: 'tests/**/*.php'
66+
instructions: |
67+
Each test method must start with `it_` and read as a sentence,
68+
annotated with #[Test] (never a `test_` prefix). Flag tests that
69+
don't follow Arrange/Act/Assert structure, that depend on shared
70+
seeded state instead of factories, or that assert only the happy
71+
path for something security-relevant.
72+
73+
# --- collapsible: UI / Fun / Optional ------------------------------------------------
74+
collapse_walkthrough: false
75+
sequence_diagrams: false
76+
estimate_code_review_effort: true
77+
in_progress_fortune: false
78+
poem: false
79+
# ------------------------------------------------------------------------------------
80+
81+
auto_review:
82+
enabled: false
83+
auto_incremental_review: true
84+
# --- collapsible: customization ----------------------------------------------------
85+
ignore_title_keywords: ['WIP', 'draft']
86+
labels: ['needs-review']
87+
drafts: false
88+
base_branches: ['master', 'development']
89+
ignore_usernames: ['dependabot', 'renovate']
90+
# ------------------------------------------------------------------------------------
91+
92+
# --- collapsible: doc/test auto-suggestions ------------------------------------------
93+
finishing_touches:
94+
docstrings:
95+
enabled: false
96+
unit_tests:
97+
enabled: false
98+
# ------------------------------------------------------------------------------------
99+
100+
# --- collapsible: pre-merge quality gates --------------------------------------------
101+
pre_merge_checks:
102+
docstrings:
103+
mode: warning
104+
threshold: 80
105+
title:
106+
mode: warning
107+
requirements: 'Descriptive, mentions component or ticket'
108+
description:
109+
mode: warning
110+
# ------------------------------------------------------------------------------------
111+
112+
tools:
113+
# --- Core linters for Laravel (PHP) and JS -------------------------------------------
114+
phpstan:
115+
enabled: true
116+
level: "3"
117+
phpcs:
118+
enabled: true
119+
eslint:
120+
enabled: true
121+
markdownlint:
122+
enabled: true
123+
124+
# --- collapsible: supporting & infra tools -------------------------------------------
125+
shellcheck:
126+
enabled: false
127+
hadolint:
128+
enabled: false
129+
yamllint:
130+
enabled: false
131+
dotenvLint:
132+
enabled: false
133+
htmlhint:
134+
enabled: false
135+
semgrep:
136+
enabled: false
137+
gitleaks:
138+
enabled: false
139+
osvScanner:
140+
enabled: false
141+
actionlint:
142+
enabled: true
143+
# ------------------------------------------------------------------------------------
144+
145+
chat:
146+
# --- collapsible: personality + integrations -----------------------------------------
147+
art: false
148+
auto_reply: true
149+
integrations:
150+
jira:
151+
usage: disabled
152+
linear:
153+
usage: disabled
154+
# ------------------------------------------------------------------------------------
155+
156+
# --- collapsible: AI learning + docs integration ---------------------------------------
157+
knowledge_base:
158+
opt_out: true
159+
web_search:
160+
enabled: false
161+
code_guidelines:
162+
enabled: false
163+
filePatterns: ['CONTRIBUTING.md', 'docs/code-guidelines.md']
164+
learnings:
165+
scope: global
166+
issues:
167+
scope: global
168+
pull_requests:
169+
scope: global
170+
# ---------------------------------------------------------------------------------------
171+
172+
# --- collapsible: AI code generation suggestions ---------------------------------------
173+
code_generation:
174+
docstrings:
175+
language: en-US
176+
177+
unit_tests:
178+
# Instructions for AI-generated unit tests
179+
path_instructions:
180+
- path: 'tests/Unit/**'
181+
instructions: |
182+
Each test method must begin with `it_` and is in snake_case.
183+
Annotate each test method with `#[Test]`.
184+
Each test method must contain `$this->markTestIncomplete();` by default.
185+
Prefer Fakes and Fixtures over Mocks.
186+
Structure tests with `it_` functions and place happy paths last.
187+
Reusable logic (e.g., fixtures) must reside in an abstract test case.
188+
- path: 'tests/Feature/**'
189+
instructions: |
190+
Each test method must begin with `it_` and is in snake_case.
191+
Annotate each test method with `#[Test]`.
192+
Default to `$this->markTestIncomplete();` until test logic is finalized.
193+
Use Laravel's HTTP helpers (`->postJson()`, `->getJson()`, etc.).
194+
Avoid inline fixture setup; use factories and seeders.
195+
# ---------------------------------------------------------------------------------------

‎.env.ci‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ APP_ENV=testing
22
APP_DEBUG=true
33
APP_KEY=base64:xxx
44

5-
CACHE_STORE=array
5+
CACHE_STORE=database
66
SESSION_DRIVER=file
77
SESSION_DOMAIN=null
88
QUEUE_CONNECTION=sync
@@ -14,3 +14,6 @@ DB_PORT=3306
1414
DB_DATABASE=daybyday_test
1515
DB_USERNAME=root
1616
DB_PASSWORD=password
17+
18+
# Set to true to disable onboarding tours site-wide (useful for staging/e2e environments)
19+
TOUR_DISABLED=true

‎.env.example‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
APP_NAME=Laravel
1+
APP_NAME="DayByDay CRM"
22
APP_ENV=local
33
APP_KEY=
44
APP_DEBUG=true
@@ -66,3 +66,6 @@ AWS_BUCKET=
6666
AWS_USE_PATH_STYLE_ENDPOINT=false
6767

6868
VITE_APP_NAME="${APP_NAME}"
69+
70+
# Set to true to disable onboarding tours site-wide (useful for staging/e2e environments)
71+
TOUR_DISABLED=true

.env.testing renamed to .env.testing.example

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,3 +35,6 @@ SEED_DRAFT_INVOICE_EXTERNAL_ID=
3535

3636
# — one seeded appointment (since the store route doesn't exist)
3737
SEED_APPOINTMENT_EXTERNAL_ID=
38+
39+
# Set to true to disable onboarding tours site-wide (useful for staging/e2e environments)
40+
TOUR_DISABLED=true

‎.github/workflows/phpunit.yml‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -124,15 +124,19 @@ jobs:
124124
sleep 2
125125
done
126126
127-
# Only .env needs .env.ci's content, for the artisan CLI commands below
128-
# (migrate, seed) that run outside PHPUnit's own environment. .env.testing
129-
# is deliberately left as committed: phpunit.xml's <env> tags already
130-
# override every key PHPUnit's test run actually needs (DB_CONNECTION,
131-
# CACHE_STORE, etc.), and ProjectFilesConfigurationTest asserts on
132-
# .env.testing's committed content directly - overwriting it here made
133-
# that assertion fail in CI while passing everywhere else.
127+
# .env needs .env.ci's content, for the artisan CLI commands below
128+
# (migrate, seed) that run outside PHPUnit's own environment.
129+
#
130+
# .env.testing is no longer committed (it's gitignored so local dev
131+
# keys/passwords don't get overwritten by every checkout); regenerate
132+
# it verbatim from .env.testing.example. ProjectFilesConfigurationTest
133+
# asserts on .env.testing's content directly (file exists, CACHE_STORE
134+
# is "array", etc.), so this copy must happen before the test run and
135+
# .env.testing.example must stay byte-for-byte what that test expects.
134136
- name: Prepare .env
135-
run: cp .env.ci .env
137+
run: |
138+
cp .env.ci .env
139+
cp .env.testing.example .env.testing
136140
137141
- name: Regenerate autoloader
138142
run: composer dump-autoload --no-scripts

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ Thumbs.db
2626
.env
2727
.env.backup
2828
.env.production
29+
.env.testing
2930
Homestead.yaml
3031
Homestead.json
3132
/auth.json

‎app/Enums/AbsenceReason.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,11 @@ class AbsenceReason
3939

4040
public function __construct(string $reason, ?string $displayValue = null)
4141
{
42-
$this->reason = $reason;
43-
$this->displayValue = $displayValue;
42+
$this->reason = $reason;
43+
// getDisplayValue() declares a non-nullable `string` return type, so a
44+
// null $displayValue must fall back to something safe rather than
45+
// being stored as-is (which would throw a TypeError on read).
46+
$this->displayValue = $displayValue ?? $reason;
4447
}
4548

4649
/**

‎app/Exceptions/Handler.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,20 @@ public function render($request, Throwable $exception)
6868
);
6969
}
7070

71+
// FormRequest::authorize() returning false throws this directly (not
72+
// wrapped in an HttpException until Laravel's own handler converts
73+
// it), which would otherwise fall through to a generic 403 error
74+
// page instead of the flash+redirect-back pattern the rest of the
75+
// app uses for permission denials.
76+
if ($exception instanceof AuthorizationException && ! $request->expectsJson()) {
77+
session()->flash(
78+
'flash_message_warning',
79+
$exception->getMessage() ?: __('You are not authorized to perform this action.')
80+
);
81+
82+
return redirect()->back();
83+
}
84+
7185
return parent::render($request, $exception);
7286
}
7387

‎app/Http/Controllers/AbsenceController.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,10 @@
33
namespace App\Http\Controllers;
44

55
use App\Enums\AbsenceReason;
6+
use App\Http\Requests\Absence\StoreAbsenceRequest;
67
use App\Models\Absence;
78
use App\Models\User;
89
use App\Services\AbsenceService;
9-
use Illuminate\Http\Request;
1010
use Throwable;
1111
use Yajra\DataTables\DataTables;
1212

@@ -67,7 +67,7 @@ public function create()
6767
->withUsers($users);
6868
}
6969

70-
public function store(Request $request, AbsenceService $absenceService)
70+
public function store(StoreAbsenceRequest $request, AbsenceService $absenceService)
7171
{
7272
try {
7373
$result = $absenceService->storeAbsence($request);
@@ -95,7 +95,7 @@ public function store(Request $request, AbsenceService $absenceService)
9595
}
9696
session()->flash('flash_message', __('Absence registered'));
9797

98-
return redirect()->back();
98+
return redirect()->route('absence.index');
9999
}
100100

101101
public function destroy(Absence $absence)

‎app/Http/Controllers/AppointmentsController.php‎

Lines changed: 14 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,19 +5,20 @@
55
use App\Http\Requests\Appointment\UpdateAppointmentCalendarRequest;
66
use App\Models\Appointment;
77
use App\Models\User;
8-
use Carbon\Carbon;
8+
use App\Services\Appointment\AppointmentService;
99
use Throwable;
1010

1111
class AppointmentsController extends Controller
1212
{
13-
public function calendar()
13+
public function __construct(private AppointmentService $appointmentService)
1414
{
15-
if ( ! auth()->user()->can('calendar-view')) {
16-
session()->flash('flash_message_warning', __('You do not have permission to view this page'));
17-
18-
return redirect()->back();
19-
}
15+
$this->middleware('permission:calendar-view', ['only' => ['calendar']]);
16+
$this->middleware('permission:appointment-edit', ['only' => ['update']]);
17+
$this->middleware('permission:appointment-delete', ['only' => ['destroy']]);
18+
}
2019

20+
public function calendar()
21+
{
2122
return view('appointments.calendar');
2223
}
2324

@@ -32,11 +33,10 @@ public function appointmentsJson()
3233
public function update(UpdateAppointmentCalendarRequest $request, Appointment $appointment)
3334
{
3435
try {
35-
// Parse the timestamps directly - they're already in the correct format
36-
// Don't convert timezone as that would shift the time
37-
$appointment->start_at = Carbon::parse($request->start);
38-
$appointment->end_at = Carbon::parse($request->end);
39-
$assignee = User::query()->where('external_id', $request->group)->first();
36+
// Assignee existence is validated here (not in the service) because
37+
// a missing assignee is a request-validation-shaped 400/redirect
38+
// response, not something the service should decide how to render.
39+
$assignee = User::query()->where('external_id', $request->group)->first();
4040

4141
if ( ! $assignee) {
4242
$message = __('Selected assignee was not found.');
@@ -51,8 +51,7 @@ public function update(UpdateAppointmentCalendarRequest $request, Appointment $a
5151
return redirect()->back()->withInput()->withErrors(['group' => $message]);
5252
}
5353

54-
$appointment->user()->associate($assignee);
55-
$appointment->save();
54+
$this->appointmentService->updateAppointmentTime($appointment, $request->start, $request->end, $assignee->external_id);
5655
} catch (Throwable $exception) {
5756
report($exception);
5857

@@ -68,11 +67,7 @@ public function update(UpdateAppointmentCalendarRequest $request, Appointment $a
6867

6968
public function destroy(Appointment $appointment)
7069
{
71-
if ( ! auth()->user()->can('appointment-delete')) {
72-
return response('Access denied', 403);
73-
}
74-
75-
$deleted = $appointment->delete();
70+
$deleted = $this->appointmentService->deleteAppointment($appointment);
7671
if ($deleted) {
7772
return response('Success');
7873
}

0 commit comments

Comments
 (0)