fix audit log + add Pest test #1786
Conversation
WalkthroughVersion bump and build-step addition; controller import switched to Symfony Response; audit-log route renamed and URL-generation calls refactored across views/models; AppServiceProvider debug SQL logging enabled; seeder insert approach adjusted; new feature tests added for audit logs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.32)Installing dependencies from lock file (including require-dev)
... [truncated 7754 characters] ... p line 48: Source path "../mercator-core" is not found for package sourcentis/mercator install [--prefer-source] [--prefer-dist] [--prefer-install PREFER-INSTALL] [--dry-run] [--download-only] [--dev] [--no-suggest] [--no-dev] [--no-autoloader] [--no-progress] [--no-install] [--audit] [--audit-format AUDIT-FORMAT] [-v|vv|vvv|--verbose] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--apcu-autoloader-prefix APCU-AUTOLOADER-PREFIX] [--ignore-platform-req IGNORE-PLATFORM-REQ] [--ignore-platform-reqs] [--] [...] Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
build.sh (1)
29-34: Consider hardening the new Composer step (global config + set -e)The new step does what it says, but there are two potential gotchas:
composer config --global --unset repositories.mercator-core/repositories.mercator-dummymutate the global Composer config for the user running the script. If those repositories are only meant for this project, you may prefer a project‑local config instead of--global.- With
set -e, if those keys are not present in the global config and Composer returns a non‑zero exit code, the whole build will abort. To make the script more robust you could allow these unsets to be best‑effort.For example:
-### --- 2b. composer update -composer config --global --unset repositories.mercator-core -composer config --global --unset repositories.mercator-dummy -composer update +### --- 2b. composer update +composer config --global --unset repositories.mercator-core || true +composer config --global --unset repositories.mercator-dummy || true +composer updateAnd keep (or drop)
--globaldepending on whether you truly want to clean the user’s global Composer setup.tests/Feature/Controller/AuditLogControllerTest.php (2)
37-37: Fix typo in comment."objets" should be "objects".
- // create some objets + // create some objects
64-64: Update test description to match the test scope.The test description says "can display logs index page" but this test is actually for the show page, not the index page.
- test('can display logs index page', function () { + test('can display log show page', function () {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
app/Http/Controllers/Admin/AuditLogsController.php(1 hunks)build.sh(1 hunks)package.json(1 hunks)resources/views/admin/auditLogs/history.blade.php(1 hunks)resources/views/admin/auditLogs/index.blade.php(2 hunks)resources/views/admin/auditLogs/show.blade.php(1 hunks)routes/web.php(1 hunks)tests/Feature/Controller/AuditLogControllerTest.php(1 hunks)version.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
routes/web.php (1)
app/Http/Controllers/Admin/AuditLogsController.php (1)
AuditLogsController(12-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
package.json (1)
15-15: Version bump aligns with release metadataThe updated
"version": "2025.12.01"is consistent with version.txt and the build script’s use of that file. Nothing else to flag here.app/Http/Controllers/Admin/AuditLogsController.php (1)
9-10: Model namespace and Response import look correctUsing
Mercator\Core\Models\AuditLogfor theshow(AuditLog $auditLog)type-hint and switching toSymfony\Component\HttpFoundation\Responsefor HTTP status constants are both consistent and should be functionally equivalent to the previous setup.version.txt (1)
1-1: version.txt kept in syncThe version string
2025.12.01matches package.json and the build script’s date-based versioning. All good.resources/views/admin/auditLogs/index.blade.php (1)
54-55: Index view updates are consistent with the new URL helper and route
Str::afterLast($log->subject_type, '\\')is a nice improvement for displaying the short class name instead of the full FQCN.- Using
\Mercator\Core\Models\AuditLog::URL($log->subject_type, $log->subject_id)centralizes subject URL generation and keeps the view thin.- The history button now targets
route('admin.audit-logs.history', ['type' => $log->subject_type, 'id' => $log->subject_id]), which matches the new route name and parameters.All of this looks coherent with the routing and model changes.
Also applies to: 57-58, 76-77
resources/views/admin/auditLogs/history.blade.php (1)
22-24: History view correctly reuses the centralized AuditLog URL helperThe subject link now uses
\Mercator\Core\Models\AuditLog::URL($auditLogs->first()->subject_type, $auditLogs->first()->subject_id), which aligns with the new URL helper used elsewhere. Given the controller aborts when$auditLogsis empty, callingfirst()here is safe.resources/views/admin/auditLogs/show.blade.php (1)
29-30: Show view subject display and URL delegation look good
Str::afterLast($auditLog->subject_type, '\\')makes the subject type much more readable and matches the index view behavior.- Using
$auditLog->subjectURL()pushes URL generation into the model, which is cleaner than building it in the view and keeps usage consistent with the new static helper elsewhere.Assuming the new
subjectURL()method is covered by the added tests, this looks solid.Also applies to: 37-38
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/Providers/AppServiceProvider.php (1)
34-39: Tighten SQL logging scope and improve log structureEnabling
DB::listenonconfig('app.debug')is useful, but a couple of tweaks would make this safer and more usable:
- Consider also scoping this to non‑production environments to avoid accidentally flooding prod logs or leaking sensitive data if
APP_DEBUGis ever misconfigured.- Log queries as structured data (SQL + bindings + time) rather than a concatenated string to improve readability and searchability.
For example:
- if (config('app.debug')) { - // Log SQL Queries - \DB::listen(function ($query): void { - \Log::info($query->time.':'.$query->sql); - }); - } + if (config('app.debug') && App::environment(['local', 'testing'])) { + // Log SQL queries (debug only, non-production) + \DB::listen(function ($query): void { + \Log::info('sql_query', [ + 'sql' => $query->sql, + 'bindings' => $query->bindings, + 'time_ms' => $query->time, + 'connection' => $query->connectionName ?? null, + ]); + }); + }This keeps the behavior you want while reducing prod risk and making logs more actionable.
database/seeders/RolesTableSeeder.php (1)
15-32:Role::query()->insert($roles)bypasses model events and will leave timestamps as NULLThe
insert()method skips Eloquent, socreated_at/updated_atwon't be auto-set—they'll remain NULL since the seed array doesn't include them. If timestamps should be tracked for roles, either add them explicitly to each array element or use a per-row approach likefirstOrCreate()(though that still requires timestamps unless relying on database defaults). For straightforward initial role seeding where timestamps don't matter, the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
app/Providers/AppServiceProvider.php(1 hunks)database/seeders/RolesTableSeeder.php(2 hunks)resources/views/admin/certificates/show.blade.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
database/seeders/RolesTableSeeder.php (3)
database/seeders/RoleUserTableSeeder.php (1)
run(11-22)database/seeders/PermissionRoleTableSeeder.php (1)
run(13-749)database/seeders/PermissionsTableSeeder.php (1)
run(13-329)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
resources/views/admin/certificates/show.blade.php (1)
18-22: Verify route name for audit log history and prefer::classover raw FQCNThe new button wiring looks correct conceptually, but two details are worth tightening up:
Route name consistency
This view callsroute('admin.audit-logs.history', [...]). Please verify the actual route definition inroutes/web.phpto ensure the route name (with or withoutadmin.prefix) matches exactly; otherwise this will 500 at runtime with a "route not defined" error.Model type string robustness
Instead of the hardcoded'App\Models\Certificate', use the class constant:
<a class="btn btn-secondary" href="{{ route('admin.audit-logs.history',['type' => 'App\Models\Certificate', 'id' => $certificate->id]) }}">
<a class="btn btn-secondary" href="{{ route('admin.audit-logs.history', ['type' => \App\Models\Certificate::class,'id' => $certificate->id,]) }}">database/seeders/RolesTableSeeder.php (2)
7-7: ImportingMercator\Core\Models\Roleis consistent with the core model namespace shiftThe updated import aligns this seeder with the
Mercator\Core\Modelsnamespace used elsewhere; no issues here.
13-15: Spacing after the log improves readabilityThe blank line after
\Log::info('RolesTableSeeder');visually separates logging from the main seeding logic and matches the style used in other seeders.
Summary by CodeRabbit
Chores
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.