fix(test): seed finance_reports.view permission for policy tests#3839
Merged
rathorevaibhav merged 2 commits intodevelopfrom May 8, 2026
Merged
Conversation
The ProjectInvoiceTerm and ClientContract policies call
$user->hasPermissionTo('finance_reports.view'), which Spatie throws
PermissionDoesNotExist on if the permission is not registered. The
permission is defined in ReportDatabaseSeeder, but that seeder was
never wired into the central DatabaseSeeder, so neither tests
(via setUpRolesAndPermissions -> db:seed) nor non-prod environments
were registering it. As a result, every policy test that exercises
a non-super-admin user path errored before reaching the actual
authorization logic — the only passing tests were ones that called
Permission::findOrCreate('finance_reports.view') inline as a
workaround.
Wiring ReportDatabaseSeeder into DatabaseSeeder follows the existing
precedent of BooksPermissionsSeeder being seeded the same way, and
unblocks the 24 failing tests in the Project and Client modules
without changing any policy or production code paths. The seeder is
gated by app()->environment(['local','staging','UAT','testing']) so
production seeding behavior is unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WalkthroughThis PR introduces two database setup changes: a migration making the ChangesDatabase Setup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.54)PHPStan was skipped because the config uses disallowed 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 |
Employee portal
 |
||||||||||||||||||||||||||||
| Project |
Employee portal
|
| Branch Review |
refs/pull/3839/merge
|
| Run status |
|
| Run duration | 00m 23s |
| Commit |
|
| Committer | Vaibhav Rathore |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
0
|
 |
0
|
 |
4
|
| View all changes introduced in this branch ↗︎ | |
ClientController::showPdf() defensively handles a null contract_file_path by logging a warning and returning 404, and the matching feature test test_null_file_path_returns_404_and_logs_warning exercises that exact branch — but the original create_client_contracts_table migration declared the column NOT NULL, so the test errored at factory setup with an integrity-constraint violation before reaching the assertion. The analogous project_invoice_terms.delivery_report column is nullable and its equivalent null-path test passes; the column nullability here was simply a missed ->nullable() in the original migration. Aligning the schema with the existing controller behavior and test intent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Modules/Client/Database/Migrations/2026_05_08_190600_make_contract_file_path_nullable_on_client_contracts_table.php`:
- Around line 16-20: The down() migration currently changes contract_file_path
to NOT NULL but doesn't handle existing NULLs; update any rows with NULL
contract_file_path to a non-null default (e.g., empty string or a sensible
placeholder) before calling Schema::table so the alter won't fail. In other
words, in the down() method, run a DB update targeting the client_contracts
table to backfill NULL contract_file_path values, then proceed with the existing
Schema::table('client_contracts', function (Blueprint $table) {
$table->string('contract_file_path')->nullable(false)->change(); }); to enforce
NOT NULL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bb2cb74f-34cf-43b8-974b-111b95ba7a79
📒 Files selected for processing (2)
Modules/Client/Database/Migrations/2026_05_08_190600_make_contract_file_path_nullable_on_client_contracts_table.phpdatabase/Seeders/DatabaseSeeder.php
Comment on lines
+16
to
+20
| public function down() | ||
| { | ||
| Schema::table('client_contracts', function (Blueprint $table) { | ||
| $table->string('contract_file_path')->nullable(false)->change(); | ||
| }); |
There was a problem hiding this comment.
Rollback can fail if existing rows contain NULL contract_file_path
On Line 19, reverting to NOT NULL will error if any row has contract_file_path = NULL (which up() explicitly permits). Backfill nulls before altering the constraint.
Suggested fix
use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;
+use Illuminate\Support\Facades\DB;
@@
public function down()
{
+ DB::table('client_contracts')
+ ->whereNull('contract_file_path')
+ ->update(['contract_file_path' => '']);
+
Schema::table('client_contracts', function (Blueprint $table) {
$table->string('contract_file_path')->nullable(false)->change();
});
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Modules/Client/Database/Migrations/2026_05_08_190600_make_contract_file_path_nullable_on_client_contracts_table.php`
around lines 16 - 20, The down() migration currently changes contract_file_path
to NOT NULL but doesn't handle existing NULLs; update any rows with NULL
contract_file_path to a non-null default (e.g., empty string or a sensible
placeholder) before calling Schema::table so the alter won't fail. In other
words, in the down() method, run a DB update targeting the client_contracts
table to backfill NULL contract_file_path values, then proceed with the existing
Schema::table('client_contracts', function (Blueprint $table) {
$table->string('contract_file_path')->nullable(false)->change(); }); to enforce
NOT NULL.
Contributor
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
1 participant
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.
Summary
Wires
ReportDatabaseSeederinto the centralDatabaseSeederso thefinance_reports.viewpermission is registered when tests rundb:seed. Fixes the 24 failing tests acrossModules/Project/Tests/{Feature,Unit}andModules/Client/Tests/{Feature,Unit}.Root cause
Both
Modules/Project/Policies/ProjectInvoiceTermPolicy::view()andModules/Client/Policies/ClientContractPolicy::view()call:Spatie's
hasPermissionTo()throwsPermissionDoesNotExistif the permission name isn't registered in thepermissionstable. The permission is defined in Modules/Report/Database/Seeders/ReportDatabaseSeeder.php:22, but that seeder was never called from database/seeders/DatabaseSeeder.php.The four failing test classes all use the standard test bootstrap:
db:seedruns the centralDatabaseSeeder, which seeds roles, HR domains, users, and book permissions — but not finance-report permissions. So when a non-super-admin test user hit a route that invokedProjectInvoiceTermPolicy/ClientContractPolicy, the policy threwPermissionDoesNotExistbefore reaching any of the actual authorization logic, and the request 500'd.That's why the test counts (
Tests: 106, Assertions: 140, Errors: 13, Failures: 11.) have stayed identical across recent develop runs — a single configuration gap is producing all 24 failures.The one test in each class that does pass —
test_user_with_finance_reports_view_permission_can_download— explicitly callsPermission::findOrCreate('finance_reports.view')inline as a workaround, which confirms the original author was aware of the gap but only patched the local case.The fix
Add
$this->call(ReportDatabaseSeeder::class)toDatabaseSeeder::run()alongside the already-presentBooksPermissionsSeeder— there's clear precedent for wiring module-level permission seeders into the central seeder.Permission::updateOrCreateandgivePermissionToare idempotent, so re-running is safe.RolesTableSeeder(already called first inDatabaseSeeder) creates theadminandfinance-managerroles thatReportDatabaseSeederassigns permissions to, so dependency order is satisfied.Why this won't change production behavior
DatabaseSeeder::run()is gated byapp()->environment(['local', 'staging', 'UAT', 'testing']), so this seeder will only run in those environments. Production seeding behavior is unchanged. (Production presumably already hasfinance_reports.viewregistered — the policy code that uses it has been live and the affected views/routes work for real users — but how it's seeded in production is out of scope for this PR.)Test plan
unit-testing.ymlModules\Client\Tests\Feature\ClientContractPdfAccessTest(5 tests)Modules\Client\Tests\Unit\ClientContractPolicyTest(5+ tests)Modules\Project\Tests\Feature\DeliveryReportAccessTest(4 tests)Modules\Project\Tests\Unit\ProjectInvoiceTermPolicyTest(5 tests)Note on local verification
I couldn't run PHPUnit locally to verify (no MySQL on this machine), so CI is the source of truth here. The change is small, follows clear precedent, and the reasoning is grounded in reading the failing test code, the policy code, and the existing seeder wiring — but if CI surfaces something unexpected I'll iterate.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores