Skip to content

Commit 1b7789c

Browse files
nielsdrost7nielsdrost7
committed
fix: address Critical/Important findings + phpunit-test-naming audit 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.
1 parent 185f03e commit 1b7789c

8 files changed

Lines changed: 556 additions & 20 deletions

File tree

‎tests/AbstractTestCase.php‎

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,10 @@ protected function setUp(): void
2727
// Reset Faker's unique state to avoid collisions with seeded data
2828
fake()->unique(true);
2929

30-
// Skip migrate:fresh when RefreshDatabase is used — that trait handles migrations itself
31-
// and calling migrate:fresh inside a transaction (which RefreshDatabase starts) fails on SQLite.
30+
// Skip migrate:fresh when RefreshDatabase is used — that trait already
31+
// handles migrations itself (via its own setUp hook, which already ran
32+
// by the time we get here), and running migrate:fresh again inside the
33+
// transaction it starts would be redundant and unsafe.
3234
$usesRefreshDatabase = in_array(
3335
\Illuminate\Foundation\Testing\RefreshDatabase::class,
3436
array_keys((function () { return class_uses_recursive($this); })->call($this))
@@ -38,19 +40,25 @@ protected function setUp(): void
3840
Artisan::call('migrate:fresh', ['--seed' => true]);
3941
}
4042

41-
// Only create user and run auth setup if the DB is available
42-
if (Schema::hasTable('users')) {
43-
$uniqueEmail = 'user_' . uniqid() . '@test.com';
44-
$this->user = User::factory()->create([
45-
'email' => $uniqueEmail,
46-
'name' => 'Admin',
47-
]);
43+
if ( ! Schema::hasTable('users')) {
44+
throw new \RuntimeException(
45+
'The `users` table does not exist after test database setup. '
46+
. ($usesRefreshDatabase
47+
? 'This test uses RefreshDatabase, which should have migrated it - check that trait\'s setup.'
48+
: 'migrate:fresh --seed just ran and should have created it - check the migration/seeder output.')
49+
);
50+
}
4851

49-
// Standardize: Every user starts as an owner to minimize boilerplate 403s
50-
$this->asOwner();
52+
$uniqueEmail = 'user_' . uniqid() . '@test.com';
53+
$this->user = User::factory()->create([
54+
'email' => $uniqueEmail,
55+
'name' => 'Admin',
56+
]);
5157

52-
$this->actingAs($this->user);
53-
}
58+
// Standardize: Every user starts as an owner to minimize boilerplate 403s
59+
$this->asOwner();
60+
61+
$this->actingAs($this->user);
5462
}
5563

5664
/**

‎tests/Feature/Clients/ClientAuthorizationTest.php‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
<?php
2+
3+
namespace Tests\Feature\Clients;
4+
5+
use App\Enums\PermissionName;
6+
use App\Http\Middleware\VerifyCsrfToken;
7+
use App\Models\Client;
8+
use App\Models\Industry;
9+
use App\Models\User;
10+
use Carbon\Carbon;
11+
use Illuminate\Foundation\Testing\RefreshDatabase;
12+
use PHPUnit\Framework\Attributes\Group;
13+
use PHPUnit\Framework\Attributes\Test;
14+
use Tests\AbstractTestCase;
15+
16+
#[Group('authorization-fix')]
17+
class ClientAuthorizationTest extends AbstractTestCase
18+
{
19+
use RefreshDatabase;
20+
21+
private Client $client;
22+
23+
private User $userWithPermission;
24+
25+
private User $userWithoutPermission;
26+
27+
protected function setUp(): void
28+
{
29+
parent::setUp();
30+
31+
Carbon::setTestNow('2024-01-15 12:00:00');
32+
33+
$this->client = Client::factory()->create();
34+
$this->userWithPermission = User::factory()->create();
35+
$this->userWithoutPermission = User::factory()->create();
36+
37+
$this->withoutMiddleware(VerifyCsrfToken::class);
38+
}
39+
40+
protected function tearDown(): void
41+
{
42+
Carbon::setTestNow();
43+
parent::tearDown();
44+
}
45+
46+
#[Test]
47+
public function it_user_with_client_delete_permission_can_delete_client(): void
48+
{
49+
/* Arrange */
50+
$this->user = $this->userWithPermission;
51+
$this->withPermissions(PermissionName::CLIENT_DELETE);
52+
53+
/* Act */
54+
$response = $this->delete(route('clients.destroy', $this->client->external_id));
55+
56+
/* Assert */
57+
$response->assertStatus(302);
58+
$this->assertSoftDeleted('clients', ['id' => $this->client->id]);
59+
}
60+
61+
#[Test]
62+
public function it_user_without_client_delete_permission_cannot_delete_client(): void
63+
{
64+
/* Arrange */
65+
$this->actingAs($this->userWithoutPermission);
66+
67+
/* Act */
68+
$response = $this->deleteJson(route('clients.destroy', $this->client->external_id));
69+
70+
/* Assert */
71+
$response->assertStatus(403);
72+
$this->assertDatabaseHas('clients', ['id' => $this->client->id, 'deleted_at' => null]);
73+
}
74+
75+
#[Test]
76+
public function it_redirects_user_without_client_create_permission_from_client_create_page(): void
77+
{
78+
/* Arrange */
79+
$this->actingAs($this->userWithoutPermission);
80+
81+
/* Act */
82+
$response = $this->get(route('clients.create'));
83+
84+
/* Assert */
85+
$response->assertRedirect(route('clients.index'));
86+
$response->assertSessionHas('flash_message_warning');
87+
}
88+
89+
#[Test]
90+
public function it_returns_forbidden_for_json_request_without_client_create_permission(): void
91+
{
92+
/* Arrange */
93+
$this->actingAs($this->userWithoutPermission);
94+
95+
/* Act */
96+
$response = $this->getJson(route('clients.create'));
97+
98+
/* Assert */
99+
$response
100+
->assertForbidden()
101+
->assertJson(['message' => __("You don't have permission to create a client")]);
102+
}
103+
104+
#[Test]
105+
public function it_prevents_user_without_client_create_permission_from_storing_client(): void
106+
{
107+
/* Arrange */
108+
$industry = Industry::factory()->create();
109+
$owner = User::factory()->create();
110+
111+
$this->actingAs($this->userWithoutPermission);
112+
113+
/* Act */
114+
$response = $this->post(route('clients.store'), [
115+
'name' => 'James Test',
116+
'email' => 'james@test.com',
117+
'primary_number' => '2342342342',
118+
'secondary_number' => '423423432',
119+
'vat' => '12312334',
120+
'company_name' => 'James & Co',
121+
'address' => 'james street',
122+
'zipcode' => '2222',
123+
'city' => 'Bond city',
124+
'company_type' => 'Aps',
125+
'industry_id' => $industry->id,
126+
'user_id' => $owner->id,
127+
]);
128+
129+
/* Assert: StoreClientRequest::authorize() failing throws AuthorizationException,
130+
* which the app's exception Handler now converts to a flash+redirect-back for
131+
* non-JSON requests (matching the rest of the app's permission-denial pattern)
132+
* instead of Laravel's generic 403 error page. */
133+
$response->assertRedirect();
134+
$response->assertSessionHas('flash_message_warning');
135+
$this->assertDatabaseMissing('clients', ['company_name' => 'James & Co']);
136+
}
137+
}

0 commit comments

Comments
 (0)