original-markdown-block.md

⚠️ Outside diff range comments (2)

tests/Unit/Entrust/EntrustUserTraitTest.php (1)

81-93: ⚠️ Potential issue | 🟠 Major

This test can pass without validating the target behavior.

At Line 87, cachedRoles() is called before any role is attached, so the collection may be empty and the foreach assertions at Lines 90-93 never execute.

Proposed fix

 #[Test]
 public function cached_roles_returns_eloquent_model_instances()
 {
-    /** Arrange */
-    // User already created in setUp()
+    /** Arrange */
+    $this->user->attachRole($this->role);

     /** Act */
     $cachedRoles = $this->user->cachedRoles();

     /** Assert */
+    $this->assertNotEmpty($cachedRoles, 'cachedRoles should contain at least one role');
     foreach ($cachedRoles as $role) {
         $this->assertIsObject($role, 'Each cached role should be an object');
         $this->assertInstanceOf(Role::class, $role, 'Each cached role should be a Role model instance');
     }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Entrust/EntrustUserTraitTest.php` around lines 81 - 93, The test
calls $this->user->cachedRoles() before any role is attached so the foreach
never runs; fix by creating and attaching a Role to $this->user (e.g., create a
Role model instance and attach it via $this->user->roles()->save($role) or the
project's assignRole helper) in the Arrange section (or assert the collection is
non-empty) before calling cachedRoles(), then keep the existing assertions that
each item is an object and an instance of Role::class.

app/Models/Payment.php (1)

40-55: ⚠️ Potential issue | 🔴 Critical

Critical: Duplicate invoice() method will cause a PHP fatal error.

The invoice() method is defined twice in this class (lines 40-43 and lines 52-55). PHP will throw a fatal error: Cannot redeclare App\Models\Payment::invoice().

Remove the duplicate method at lines 52-55.

🐛 Proposed fix

     //endregion

     public function getPriceAttribute()
     {
         return app(Money::class, ['amount' => $this->amount]);
     }
-
-    public function invoice()
-    {
-        return $this->belongsTo(Invoice::class);
-    }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Models/Payment.php` around lines 40 - 55, The class defines the invoice()
relationship twice causing a fatal redeclare; remove the duplicate invoice()
method (the second declaration of invoice()) and keep a single
belongsTo(Invoice::class) implementation, ensuring getPriceAttribute() and the
remaining invoice() method stay intact; search for the method name invoice() in
the Payment class and delete the redundant block.

♻️ Duplicate comments (1)

tests/Unit/Client/ClientNumberServiceTest.php (1)

103-118: ⚠️ Potential issue | 🟠 Major

Failure-path tests currently codify invalid non-positive client numbers as acceptable.

Both test names/comments say these inputs should be prevented, but assertions validate successful progression (0,1 and -100,-99). This makes desired validation harder to introduce safely.

Suggested test direction (align with intended behavior)

+use InvalidArgumentException;
...
-    public function set_client_number_to_zero_leads_to_duplicate_numbers()
+    public function set_client_number_to_zero_throws_exception()
     {
-        $this->clientNumberService->setClientNumber(0);
-        $firstClient = $this->clientNumberService->setNextClientNumber();
-        $secondClient = $this->clientNumberService->setNextClientNumber();
-        $this->assertEquals(0, $firstClient);
-        $this->assertEquals(1, $secondClient);
-        $this->assertNotEquals($firstClient, $secondClient, 'Client numbers should not duplicate');
+        $this->expectException(InvalidArgumentException::class);
+        $this->clientNumberService->setClientNumber(0);
     }
...
-    public function set_negative_client_number_should_be_prevented()
+    public function set_negative_client_number_throws_exception()
     {
-        $negativeNumber = -100;
-        $this->clientNumberService->setClientNumber($negativeNumber);
-        $firstNumber = $this->clientNumberService->setNextClientNumber();
-        $secondNumber = $this->clientNumberService->setNextClientNumber();
-        $this->assertEquals(-100, $firstNumber);
-        $this->assertEquals(-99, $secondNumber);
-        $this->assertLessThan(0, $firstNumber, 'Negative client numbers should not be allowed');
+        $this->expectException(InvalidArgumentException::class);
+        $this->clientNumberService->setClientNumber(-100);
     }

As per coding guidelines: **/*.php: Throw specific exception types like InvalidArgumentException instead of generic Exception.

Also applies to: 154-173

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Client/ClientNumberServiceTest.php` around lines 103 - 118, Update
the failing tests so they assert the intended validation behavior: when calling
ClientNumberService::setClientNumber with non-positive values (e.g., 0 or -100)
the test should expect a specific InvalidArgumentException instead of asserting
numeric progression; locate the tests named
set_client_number_to_zero_leads_to_duplicate_numbers and the similar
negative-number test, replace the current assertions around
setClientNumber/setNextClientNumber with an expectation of
InvalidArgumentException (use
$this->expectException(InvalidArgumentException::class)) and remove the
duplicate-number assertions so the tests reflect that setClientNumber enforces
positive values.

🟠 Major comments (22)

tests/Unit/Exceptions/HandlerTest.php-87-97 (1)

87-97: ⚠️ Potential issue | 🟠 Major

Replace placeholder test with real JSON-structure assertions.

assertTrue(true) makes this test non-functional and leaves the exception-handler contract unverified.

Proposed fix

 public function unauthenticated_json_response_has_correct_structure()
 {
     /** Arrange */
-    // Placeholder test
+    // No authentication credentials
 
     /** Act */
-    // No action
+    $response = $this->getJson('/api/users');
 
     /** Assert */
-    $this->assertTrue(true);
+    $response->assertStatus(401);
+    $response->assertJsonStructure(['error']);
+    $response->assertJson(['error' => 'Unauthenticated.']);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Exceptions/HandlerTest.php` around lines 87 - 97, The test
unauthenticated_json_response_has_correct_structure() is a placeholder and must
be replaced with a real assertion: simulate an unauthenticated request (or throw
the authentication exception) through the app/Exception/Handler pipeline,
capture the HTTP JSON response from the handler, and assert the structure and
types (e.g., top-level keys like "message", "errors"/"error" array or object,
and numeric "status" or "status_code", and proper HTTP status 401) and content
types; update the test to call the real handler method used by your app (the
same exception handling entrypoint) and perform assertions on
response->getStatusCode(), response->getHeaders()['Content-Type'], and
json_decoded body keys to verify the contract instead of assertTrue(true).

app/Models/Role.php-32-35 (1)

32-35: ⚠️ Potential issue | 🟠 Major

Remove or fix the userRole() method — it references a non-existent foreign key and is never used.

The userRole() method at lines 32–35 uses hasMany(Role::class, 'user_id', 'id'), but the roles table has no user_id column. This method is also never called anywhere in the codebase. Either remove it as dead code or, if it was intended to access users assigned to a role, replace it with the correct relationship through the role_user pivot table.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Models/Role.php` around lines 32 - 35, The userRole() method in Role
(public function userRole()) is incorrect (references non-existent user_id on
roles) and unused; either delete this dead method or replace it with a correct
many-to-many relationship to users using the role_user pivot: add a users()
method returning belongsToMany(User::class, 'role_user', 'role_id', 'user_id')
(or adjust pivot/column names to match DB), and remove the old userRole()
method; after change, run tests/grep for any callers and update them to use
users() if needed.

app/Models/PermissionRole.php-33-36 (1)

33-36: ⚠️ Potential issue | 🟠 Major

Remove the unused and broken settings() relationship.

The settings() method at lines 33-36 defines a belongsTo(Setting::class) relationship that expects a setting_id column on the permission_role table. However, the table schema (from migration 2016_08_26_205017_entrust_setup_tables.php) contains only permission_id and role_id—no setting_id column exists. This relationship is never used anywhere in the codebase and will fail at runtime if invoked. Remove it or fix the mapping if it was intended to serve a purpose.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Models/PermissionRole.php` around lines 33 - 36, Remove the unused/broken
relationship method settings() from the PermissionRole model: the method public
function settings() { return $this->belongsTo(Setting::class); } expects a
setting_id column that doesn't exist (permission_role only has permission_id and
role_id) and is not referenced; delete this method from the PermissionRole class
(or replace it with a correctly mapped relationship only if there is an intended
association) to prevent runtime errors.

tests/Unit/Offer/OffersStatusEnumTest.php-6-6 (1)

6-6: ⚠️ Potential issue | 🟠 Major

Update exception type in OfferStatus enum and align test expectations

The test expects generic Exception, but OfferStatus::fromStatus() and fromDisplayValue() should throw InvalidArgumentException per the coding guideline. Update app/Enums/OfferStatus.php to throw InvalidArgumentException (see AbsenceReason enum for the correct pattern), then update the test to expect that specific type and remove the generic use Exception; import.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Offer/OffersStatusEnumTest.php` at line 6, The OfferStatus enum
currently throws a generic Exception but should throw InvalidArgumentException;
update app/Enums/OfferStatus.php so OfferStatus::fromStatus() and
OfferStatus::fromDisplayValue() throw InvalidArgumentException (follow the
pattern used in AbsenceReason), then update the test in
tests/Unit/Offer/OffersStatusEnumTest.php to expect InvalidArgumentException
instead of Exception and remove the unused "use Exception;" import.

tests/Unit/Invoice/InvoiceStatusEnumTest.php-95-105 (1)

95-105: ⚠️ Potential issue | 🟠 Major

Fix weak string-vs-object assertion in display-value mapping test.

Line 104 compares a status string to an InvoiceStatus object. This doesn’t explicitly validate the returned enum instance and exact mapped status.

Suggested fix

     /** Act */
     $status = InvoiceStatus::fromDisplayValue($displayValue);

     /** Assert */
-    $this->assertEquals(InvoiceStatus::partialPaid()->getStatus(), $status);
+    $this->assertInstanceOf(InvoiceStatus::class, $status);
+    $this->assertSame(InvoiceStatus::partialPaid()->getStatus(), $status->getStatus());

Based on learnings: “verify object identity/instance linkage … rather than only using instanceof” and prefer strict assertions in this test suite.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Invoice/InvoiceStatusEnumTest.php` around lines 95 - 105, The test
currently compares a status string to an InvoiceStatus object; replace that weak
string-vs-object check with a strict object identity/value assertion: call
InvoiceStatus::fromDisplayValue($displayValue) and assert the returned enum
instance equals the expected enum instance by using a strict assertion such as
$this->assertSame(InvoiceStatus::partialPaid(), $status) (or $this->assertEquals
if your enum implements value equality) instead of comparing to getStatus();
update the assertion line to reference InvoiceStatus::partialPaid() and $status
directly.

tests/Unit/Repositories/RoleRepositoryTest.php-120-133 (1)

120-133: ⚠️ Potential issue | 🟠 Major

Owner-exclusion assertion can be skipped, so behavior is not truly validated.

At Line 131, the conditional allows the core assertion to be bypassed when no owner exists. With current Arrange data, owner-filtering can pass without being exercised (same risk in the owner-exclusion checks around Line 43 and Line 146).

🔧 Suggested fix

 public function list_all_roles_does_not_include_owner()
 {
     /** Arrange */
-    // Already arranged in setUp()
+    $ownerRole = Role::factory()->create([
+        'name' => 'owner',
+        'display_name' => 'Owner',
+    ]);

     /** Act */
     $roles = $this->repository->listAllRoles();
     $displayNames = $roles->toArray();
-    $ownerRole = Role::where('name', 'owner')->first();

     /** Assert */
-    if ($ownerRole) {
-        $this->assertArrayNotHasKey($ownerRole->id, $displayNames, 'listAllRoles() should not include the owner role');
-    }
-
+    $this->assertArrayNotHasKey($ownerRole->id, $displayNames, 'listAllRoles() should not include the owner role');
     $this->assertNotContains('Owner', $displayNames);
 }

As per coding guidelines: "Tests must be self-contained: create their own test data and avoid dependencies on other tests or seeders."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Repositories/RoleRepositoryTest.php` around lines 120 - 133, The
test list_all_roles_does_not_include_owner is conditional on Role::where('name',
'owner') and can skip the assertion; instead make the test self-contained by
ensuring an 'owner' role exists (create one directly in the test via
Role::create(...) or a factory) before calling
$this->repository->listAllRoles(), then assert that the returned array (from
$roles->toArray()) does not contain that owner's id; update the test to remove
the conditional and reference the Role creation so the exclusion behavior of
listAllRoles() is always exercised.

tests/Unit/Client/UpdateAssigneeTest.php-56-60 (1)

56-60: ⚠️ Potential issue | 🟠 Major

Assert persisted database side effects, not only mutated in-memory properties.

At Line 58, Line 74, Line 91, Line 111, Line 128, and Lines 145-146, assertions only read the current model instances. A failed/partial persistence path could slip by. Refresh and/or assert DB state explicitly.

Suggested hardening

 /** Act */
 $this->client->updateAssignee($this->user);

 /** Assert */
-$this->assertEquals($this->client->user_id, $this->user->id);
+$this->client->refresh();
+$this->assertSame($this->user->id, $this->client->user_id);
+$this->assertDatabaseHas('clients', [
+    'id' => $this->client->id,
+    'user_id' => $this->user->id,
+]);
 Event::assertDispatched(ClientAction::class);

Also applies to: 73-75, 90-93, 110-113, 127-130, 144-148

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Client/UpdateAssigneeTest.php` around lines 56 - 60, The
assertions in UpdateAssigneeTest rely on in-memory model properties (e.g.
$this->client, $this->user and assertions using assertEquals/assertNotEquals and
Event::assertDispatched(ClientAction::class)) and may miss failed persistence;
after performing the update action, refresh the models or assert DB state
explicitly — for example call $this->client->refresh() and
$this->user->refresh() before asserting their ids, or replace the in-memory
assertions with database assertions such as $this->assertDatabaseHas('clients',
['id' => $this->client->id, 'user_id' => $this->user->id]) (and similar for
other test cases at the referenced lines) while keeping
Event::assertDispatched(ClientAction::class).

tests/Unit/Events/LeadActionTest.php-75-86 (1)

75-86: ⚠️ Potential issue | 🟠 Major

Add channel name assertion to catch hardcoded placeholder regressions.

The test only validates type but not the channel name. Once the broadcastOn() implementation uses a lead-scoped channel name (e.g., 'lead-' . $lead->id), add an assertion: $this->assertEquals('lead-' . $lead->id, $channel->name) to prevent regressions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Events/LeadActionTest.php` around lines 75 - 86, The test
currently only asserts the channel type; update the
LeadActionTest::broadcast_on_returns_private_channel test to also assert the
actual channel name returned by LeadAction::broadcastOn() to catch hardcoded
placeholders — after obtaining $channel, add an assertion that $channel->name
equals 'lead-' . $lead->id (e.g., $this->assertEquals('lead-' . $lead->id,
$channel->name)) so the test verifies both the PrivateChannel class and the
expected lead-scoped channel name.

tests/Unit/Events/ClientActionTest.php-75-86 (1)

75-86: ⚠️ Potential issue | 🟠 Major

Add assertion for exact broadcast channel name.

The test only checks that broadcastOn() returns a PrivateChannel instance, but doesn't verify the actual channel name. This won't catch if the channel name is hardcoded as a placeholder or incorrect. Add an assertion for $channel->name to verify the channel is properly scoped to the client entity (e.g., 'client-' . $client->id or similar pattern).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Events/ClientActionTest.php` around lines 75 - 86, Update the test
method broadcast_on_returns_private_channel to also assert the exact channel
name: after creating $client and $event = new ClientAction($client, 'created')
and obtaining $channel = $event->broadcastOn(), add an assertion that
$channel->name equals the expected pattern (e.g., 'client-' . $client->id or
whatever naming convention ClientAction uses) to ensure the PrivateChannel is
correctly scoped to the specific client entity.

tests/Unit/Events/TaskActionTest.php-75-86 (1)

75-86: ⚠️ Potential issue | 🟠 Major

Add channel name assertion to catch regression on entity-scoped channel names.

The test validates only channel type but not the actual channel name value. It should assert $this->assertSame('channel-name', $channel->name) (or the proper task-scoped name once implemented) to prevent regressions when the channel naming scheme changes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Events/TaskActionTest.php` around lines 75 - 86, The test
broadcast_on_returns_private_channel only asserts the channel type; add an
assertion verifying the channel name produced by TaskAction::broadcastOn to
prevent regressions. After creating $task and $event and calling $channel =
$event->broadcastOn(), assert that $channel->name matches the expected
task-scoped channel name (e.g. built using the Task id or slug for the app's
naming scheme) so the test checks both type and exact name.

tests/Unit/Events/ProjectActionTest.php-75-86 (1)

75-86: ⚠️ Potential issue | 🟠 Major

Test should assert the channel name to catch hardcoded placeholder values.

The test currently only asserts the channel type with assertInstanceOf(). Add an assertion on $channel->name to verify it matches the expected project-scoped channel name. This will catch regressions once the channel name is properly scoped to the project.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Events/ProjectActionTest.php` around lines 75 - 86, The test
currently only checks the channel type; update it to also assert the channel's
name matches the project-scoped pattern to catch hardcoded placeholders: after
creating Project and ProjectAction and calling ProjectAction::broadcastOn(),
assert $channel->name equals "project.{$project->id}" (i.e. verify the
PrivateChannel returned by broadcastOn() is scoped to the created project's id).

tests/Unit/DemoEnvironment/CanNotAccessTest.php-90-99 (1)

90-99: ⚠️ Potential issue | 🟠 Major

Add $user->fresh() after role/permission changes and before actingAs() in these three tests.

The code attaches roles/permissions to user objects then immediately authenticates with actingAs() without reloading from the database. This leaves the user object with stale permission state. Per coding guidelines: "In tests, always call $user = $user->fresh() after attaching permissions or roles ... before authentication with actingAs($user)."

Suggested patch

@@
-        $this->actingAs($user);
+        $user = $user->fresh();
+        $this->actingAs($user);
@@
-        $this->actingAs($authUser);
+        $authUser = $authUser->fresh();
+        $this->actingAs($authUser);
@@
-        $this->actingAs($authUser);
+        $authUser = $authUser->fresh();
+        $this->actingAs($authUser);

Lines 90–99, 111–119, 132–140.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/DemoEnvironment/CanNotAccessTest.php` around lines 90 - 99, In the
tests that modify user roles/permissions (where User::factory()->create() is
followed by $user->attachRole(...) and $role->attachPermission(...)), reload the
user from the DB before authenticating: call $user = $user->fresh() immediately
after the role/permission attachment (and any Cache::tags('role_user')->flush())
and before $this->actingAs($user); update this in the three test blocks
currently using attachRole/attachPermission and actingAs in CanNotAccessTest.php
(lines referenced around the User::factory, attachRole, attachPermission,
Cache::tags(...)->flush(), and actingAs calls).

app/Models/Lead.php-149-152 (1)

149-152: ⚠️ Potential issue | 🟠 Major

Normalize the status title before comparing it.

convertToOrder() later looks up 'Closed', but isClosed() compares against 'closed'. If the stored title is capitalized, this method returns false for an actually closed lead.

💡 Proposed fix

     public function isClosed()
     {
-        // Check if status relationship exists and compare title
-        return $this->status && $this->status->title == self::LEAD_STATUS_CLOSED;
+        return strtolower($this->status?->title ?? '') === self::LEAD_STATUS_CLOSED;
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Models/Lead.php` around lines 149 - 152, The isClosed() method currently
compares $this->status->title to self::LEAD_STATUS_CLOSED with case-sensitivity
which fails when titles are capitalized; update isClosed() to normalize the
status title (e.g., lowercase or trim) before comparing so it matches the same
normalization used by convertToOrder(), e.g., compare
strtolower($this->status->title) (or use Str::lower) against the normalized
self::LEAD_STATUS_CLOSED value to ensure a case-insensitive match.

tests/Unit/Status/TypeOfStatusTest.php-44-51 (1)

44-51: ⚠️ Potential issue | 🟠 Major

Current assertions do not validate scope correctness.

At Line 49–51, non-null checks are vacuous for get() results and won’t catch broken scopes. Assert expected counts and source types instead.

Suggested patch

-        $taskStatuses = Status::typeOfTask()->get()->where('title', 'Hello');
-        $leadStatuses = Status::typeOfLead()->get()->where('title', 'Hello');
-        $projectStatuses = Status::typeOfProject()->get()->where('title', 'Hello');
+        $taskStatuses = Status::typeOfTask()->where('title', 'Hello')->get();
+        $leadStatuses = Status::typeOfLead()->where('title', 'Hello')->get();
+        $projectStatuses = Status::typeOfProject()->where('title', 'Hello')->get();

-        $this->assertNotNull($taskStatuses);
-        $this->assertNotNull($leadStatuses);
-        $this->assertNotNull($projectStatuses);
+        $this->assertCount(1, $taskStatuses);
+        $this->assertCount(1, $leadStatuses);
+        $this->assertCount(1, $projectStatuses);
+        $this->assertSame(Task::class, $taskStatuses->first()->source_type);
+        $this->assertSame(Lead::class, $leadStatuses->first()->source_type);
+        $this->assertSame(Project::class, $projectStatuses->first()->source_type);

Based on learnings: Aim to cover edge cases and keep the test suite at/near 100% coverage.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Status/TypeOfStatusTest.php` around lines 44 - 51, Replace the
vacuous assertNotNull checks with assertions that validate the scope behavior:
for the collections returned by Status::typeOfTask()->get() ($taskStatuses),
Status::typeOfLead()->get() ($leadStatuses) and Status::typeOfProject()->get()
($projectStatuses) assert the expected counts (e.g. ->count() equals the number
you expect for fixtures) and assert each model's distinguishing field (e.g. a
source_type, type, or scope-specific attribute on Status) matches the scope
(e.g. every $taskStatuses item has source_type === 'task' or equivalent); update
the assertions to loop or use collection helpers (->pluck or ->every) to
validate the source/type for all items rather than just checking non-null.

tests/Unit/Invoice/DueAtTest.php-94-104 (1)

94-104: ⚠️ Potential issue | 🟠 Major

This new expectation contradicts Invoice::pastDueAt().

scopePastDueAt() only filters on due_at < now() and unpaid status. Setting sent_at to null does not exclude the invoice with the current scope, so this test will fail unless the scope is also changed to require a sent invoice.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Invoice/DueAtTest.php` around lines 94 - 104, The test
dont_get_invoice_if_not_sent() assumes Invoice::pastDueAt() excludes invoices
with sent_at == null, but scopePastDueAt() currently only checks due_at < now()
and unpaid status; either update scopePastDueAt() to also filter sent invoices
(add ->whereNotNull('sent_at') or equivalent in the scopePastDueAt method on the
Invoice model) or change the test assertion to expect the invoice to be present;
locate scopePastDueAt() on the Invoice model and add the sent_at not-null
condition if you want the scope to require sent invoices, otherwise update the
test dont_get_invoice_if_not_sent to reflect the current scope behavior.

tests/Unit/Invoice/InvoiceNumberServiceTest.php-99-111 (1)

99-111: ⚠️ Potential issue | 🟠 Major

This edge case is seeding the wrong state.

InvoiceNumberService reads and increments settings.invoice_number; it does not calculate the next number from existing invoices rows. Seeding invoices here will not make setNextInvoiceNumber() return 10006, so this test will fail unless the service contract changes.

🛠️ Align the arrange step with the current service contract

-        Invoice::factory()->create(['invoice_number' => 10005]);
-        Invoice::factory()->create(['invoice_number' => 10003]);
+        Setting::query()->firstOrFail()->update(['invoice_number' => 10006]);
         $service = app(InvoiceNumberService::class);

         /** Act */
         $nextNumber = $service->setNextInvoiceNumber();

         /** Assert */
-        $this->assertEquals(10006, $nextNumber);
+        $this->assertSame(10006, $nextNumber);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Invoice/InvoiceNumberServiceTest.php` around lines 99 - 111, The
test seeds invoice rows but InvoiceNumberService::setNextInvoiceNumber() uses
settings.invoice_number, so change the Arrange to set the settings record to
10005 (so setNextInvoiceNumber() returns 10006) instead of creating
Invoice::factory() records; use the existing settings model or DB helper (e.g.,
updateOrCreate on Setting or DB::table('settings')->upsert) to set the
invoice_number value before resolving InvoiceNumberService and asserting the
result.

tests/Unit/Comment/GetCommentEndpointTest.php-47-50 (1)

47-50: ⚠️ Potential issue | 🟠 Major

Replace manual URL construction with the actual production route used by models.

The test assertions use url('comments/lead').DIRECTORY_SEPARATOR.$this->lead->external_id, which is platform-dependent; on Windows, DIRECTORY_SEPARATOR is \, breaking the URL. The production models already use route('comments.create', ['type' => '...', 'external_id' => ...]) to generate these endpoints. Align the test expectations with production behavior and switch to assertSame() for string comparisons.

Suggested fix

-        $this->assertEquals(url('comments/lead').DIRECTORY_SEPARATOR.$this->lead->external_id, $leadEndpoint);
-        $this->assertEquals(url('comments/task').DIRECTORY_SEPARATOR.$this->task->external_id, $taskEndpoint);
-        $this->assertEquals(url('comments/project').DIRECTORY_SEPARATOR.$this->project->external_id, $projectEndpoint);
+        $this->assertSame(route('comments.create', ['type' => 'lead', 'external_id' => $this->lead->external_id]), $leadEndpoint);
+        $this->assertSame(route('comments.create', ['type' => 'task', 'external_id' => $this->task->external_id]), $taskEndpoint);
+        $this->assertSame(route('comments.create', ['type' => 'project', 'external_id' => $this->project->external_id]), $projectEndpoint);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Comment/GetCommentEndpointTest.php` around lines 47 - 50, The test
builds expected URLs by concatenating url('comments/...') and
DIRECTORY_SEPARATOR which is platform-dependent and causes failures; change the
three assertions to use the same route helper the app uses and strict
comparison: replace the expected strings with route('comments.create', ['type'
=> 'lead'|'task'|'project', 'external_id' =>
$this->lead->external_id|$this->task->external_id|$this->project->external_id])
and use $this->assertSame(...) for each of $leadEndpoint, $taskEndpoint and
$projectEndpoint to match production behavior.

tests/Unit/Controllers/Appointment/AppointmentSecurityTest.php-118-121 (1)

118-121: ⚠️ Potential issue | 🟠 Major

Reload the role-attached user before actingAs(...).

Authenticating the model immediately after withRole('employee') can reuse stale role/permission state and make this authorization test flaky. Fresh the user first.

Suggested fix

         $this->user->roles()->detach();
         $this->user = User::factory()->withRole('employee')->create();
+        $this->user = $this->user->fresh();
         $this->actingAs($this->user);

As per coding guidelines, in tests always call $user = $user->fresh() after attaching permissions or roles before authentication with actingAs($user).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Controllers/Appointment/AppointmentSecurityTest.php` around lines
118 - 121, The test attaches a role then immediately authenticates the model
which can use stale role state; after creating/attaching the role via
User::factory()->withRole('employee')->create() (and/or after
$this->user->roles()->detach()), refresh the model instance with $this->user =
$this->user->fresh() (or assign a fresh local $user) before calling
$this->actingAs($this->user) so the authenticated user has up-to-date
roles/permissions.

tests/Unit/Invoice/GenerateInvoiceStatusTest.php-38-40 (1)

38-40: ⚠️ Potential issue | 🟠 Major

Don't depend on a seeded Setting row in setUp().

query()->update(['vat' => 0]) is a no-op when the table is empty, so this class only behaves correctly if some external seeder already created a record. Please create the setting fixture explicitly here instead of mutating ambient state.

As per coding guidelines, tests must be self-contained: create their own test data and avoid dependencies on other tests or seeders.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Invoice/GenerateInvoiceStatusTest.php` around lines 38 - 40, The
test relies on a seeded Setting row; replace the fragile call to
\App\Models\Setting::query()->update(['vat' => 0]) in
GenerateInvoiceStatusTest::setUp() with an explicit fixture creation or
retrieval so the test is self-contained—use
\App\Models\Setting::firstOrCreate(...) or Setting::factory()->create(...) to
ensure a Setting record exists with 'vat' => 0, and remove the dependence on
ambient seeders.

tests/Unit/Controllers/Appointment/AppointmentSecurityTest.php-98-149 (1)

98-149: ⚠️ Potential issue | 🟠 Major

Failure-path tests should assert no side effects.

These 403 checks stop at the status code, so they would miss a controller that mutates or deletes the appointment before denying access. Capture the original timestamps / deletion state in Arrange and assert they are unchanged after each forbidden request.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Controllers/Appointment/AppointmentSecurityTest.php` around lines
98 - 149, In the three tests unauthorized_user_cannot_update_appointment,
appointment_update_requires_permission_check, and
unauthorized_user_cannot_delete_appointment capture the appointment's original
state in Arrange (e.g. $originalStart = $this->appointment->start, $originalEnd
= $this->appointment->end, $originalDeletedAt = $this->appointment->deleted_at
or existence flag) then after the forbidden request refresh the model (e.g.
$this->appointment->refresh()) and assert the timestamps are unchanged
($this->assertEquals($originalStart, $this->appointment->start) etc.) and for
the delete test assert the record still exists / deleted_at remains null to
guarantee no mutation or deletion occurred.

tests/Unit/Controllers/Appointment/AppointmentSecurityTest.php-60-76 (1)

60-76: ⚠️ Potential issue | 🟠 Major

Assert the persisted update, not just the 200.

This still passes if the controller returns 200 without actually updating start_at / end_at. Refresh the model and assert the saved timestamps changed.

Suggested assertion update

         /** Assert */
         $response->assertStatus(200);
+        $this->appointment->refresh();
+        $this->assertSame(
+            Carbon::now()->addDay()->toISOString(),
+            $this->appointment->start_at->toISOString(),
+        );
+        $this->assertSame(
+            Carbon::now()->addDay()->addHour()->toISOString(),
+            $this->appointment->end_at->toISOString(),
+        );
     }

As per coding guidelines, in tests never compare Carbon date objects directly to strings; always normalize dates for comparison.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Controllers/Appointment/AppointmentSecurityTest.php` around lines
60 - 76, The test authorized_user_can_update_appointment currently only asserts
a 200 response; refresh $this->appointment after the request (use
$this->appointment->refresh() or reload via model query) and assert that the
persisted start_at and end_at fields changed to the expected values (compare
normalized Carbon instances or ISO strings rather than raw Carbon vs string),
referencing the request payload fields 'start' and 'end' and the model
properties start_at/end_at to verify the update occurred.

app/Models/User.php-76-79 (1)

76-79: ⚠️ Potential issue | 🟠 Major

This is a breaking change: morphMany(source_*) will not return existing appointments created with only user_id set.

The new relation filters on source_type and source_id only, but AppointmentSecurityTest.php (lines 38–46) creates appointments with only user_id populated—no source_* fields. After this change, those rows will not be returned by $user->appointments, breaking any code that relies on the old direct user relationship.

Audit all existing appointment writes. If any path creates appointments with user_id but leaves source_* null, data will become inaccessible. Either:

• Migrate existing appointments to set source_type and source_id, or
• Restore the original hasMany relationship on user_id if polymorphic source assignment is not yet applied consistently

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Models/User.php` around lines 76 - 79, The new appointments() relation
was changed to morphMany(Appointment::class, 'source') which will no longer
return Appointment rows created with only user_id set (as seen in
AppointmentSecurityTest.php); audit all code paths that create Appointment
records and either (A) migrate existing rows to populate source_type and
source_id for the polymorphic relation and update write paths to set those
fields, or (B) restore the original relation on User by changing appointments()
back to a hasMany(Appointment::class, 'user_id') (or provide a combined accessor
that queries both the polymorphic source and user_id) so legacy rows remain
accessible; update tests accordingly.

🟡 Minor comments (6)

tests/Unit/Payment/PaymentSourceEnumTest.php-126-127 (1)

126-127: ⚠️ Potential issue | 🟡 Minor

Update PaymentSource::fromSource() and PaymentSource::fromDisplayValue() to throw specific exception types.

Per coding guidelines, PaymentSource.php should throw InvalidArgumentException instead of generic Exception. Update both methods to throw the specific exception type, then update the test assertions to match:

• Line 126: $this->expectException(InvalidArgumentException::class);
• Line 138: $this->expectException(InvalidArgumentException::class);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Payment/PaymentSourceEnumTest.php` around lines 126 - 127, The
tests expect a specific exception but PaymentSource currently throws a generic
Exception; update PaymentSource::fromSource() and
PaymentSource::fromDisplayValue() to throw InvalidArgumentException instead of
Exception, and update the unit tests in PaymentSourceEnumTest (the
expectException calls) to assert InvalidArgumentException::class for both
failing cases so the thrown type matches the assertions.

tests/Unit/Client/UpdateAssigneeTest.php-59-60 (1)

59-60: ⚠️ Potential issue | 🟡 Minor

Strengthen event assertions to validate payload identity and action value.

Current checks only prove that ClientAction was dispatched, not that it carried the correct client/action. Assert event payload via callback.

Suggested payload assertion pattern

+use App\Http\Controllers\ClientsController;
+
 Event::assertDispatched(
     ClientAction::class,
     function (ClientAction $event) {
-        return true;
+        return $event->getClient()->is($this->client)
+            && $event->getAction() === ClientsController::UPDATED_ASSIGN;
     }
 );

Also applies to: 75-76, 92-93, 112-113, 129-130, 147-148

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Client/UpdateAssigneeTest.php` around lines 59 - 60, The test
currently only checks that ClientAction was dispatched; change the
Event::assertDispatched calls in UpdateAssigneeTest.php to assert the event
payload via a callback that verifies the event carries the expected client
identity and action value (e.g. compare $event->client->id or $event->clientId
and $event->action or $event->value), using
Event::assertDispatched(ClientAction::class, fn($event) => /* return true when
$event matches expected client id and action */); apply this replacement for the
occurrences around the current assertions (the ones at the referenced test
positions) so each test validates both the event type and its payload contents.

tests/Unit/Controllers/Absence/AbsenceControllerTest.php-73-79 (1)

73-79: ⚠️ Potential issue | 🟡 Minor

Either assert on $response or inline the request.

Both tests assign the JSON response and then never use it, which is exactly what PHPMD is reporting. Either drop the variable or assert the redirect/flash behavior through it.

Also applies to: 100-107

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Controllers/Absence/AbsenceControllerTest.php` around lines 73 -
79, The test currently assigns the request result to $response (from
$this->json('POST', route('absence.store'), ...)) but never asserts against it;
either drop the assignment and call $this->json(...) inline, or keep $response
and add assertions (e.g., $response->assertRedirect(...),
$response->assertSessionHas(...)) to verify redirect/flash behavior for
route('absence.store'); apply the same change to the similar occurrences around
lines 100-107 so no $response is unused.

tests/Unit/Environment/ProjectFilesConfigurationTest.php-399-427 (1)

399-427: ⚠️ Potential issue | 🟡 Minor

These rule checks are now vulnerable to comment and substring false positives.

Using str_contains() here means a comment or partial pattern can satisfy the test without the actual .gitignore/.gitattributes rule being present. For config-file validation, anchor the expected line with regex or parse the file into exact entries.

Also applies to: 549-625

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Environment/ProjectFilesConfigurationTest.php` around lines 399 -
427, The current tests (gitignore_excludes_vendor_directory,
gitignore_excludes_env_files and other similar tests) use str_contains() which
can be falsely satisfied by comments or substrings; update each test to assert
the presence of an exact anchored entry instead — e.g., get the file content via
readFile('.gitignore'), split into lines and trim/comments-filter them or use
preg_match with anchored patterns like ^/vendor/$ and ^\.env(\.production)?$ to
verify exact lines; replace the str_contains checks in the identified test
methods with these precise line/regex checks so only real
.gitignore/.gitattributes entries pass.

tests/Unit/Controllers/Appointment/AppointmentsControllerTest.php-188-207 (1)

188-207: ⚠️ Potential issue | 🟡 Minor

Test name misleads: only verifies source_id isolation, not source_type constraint.

The fixture only uses source_type => User::class for all appointments (including the setUp ones). To properly test that morphMany constrains by source type, the test should create an appointment with a different source_type (e.g., Task::class) pointing to $this->user, then verify $this->user->appointments excludes it. A bug that removed the source_type constraint would still pass this regression test as written.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Controllers/Appointment/AppointmentsControllerTest.php` around
lines 188 - 207, The test
user_appointments_morph_does_not_return_appointments_for_other_source_types
currently only verifies source_id isolation; change the fixture to create an
Appointment for $this->user but with a different source_type (e.g., Task::class)
and the same or different source_id, then assert that $this->user->appointments
does NOT contain that appointment while a Task-owner’s appointments (or lookup
by source_type) would—this verifies the morphMany filters by source_type; update
the test name or comments if needed to reflect that it now asserts exclusion by
differing source_type (referencing Appointment::factory(), User, Task, and
$this->user->appointments).

tests/Unit/Project/ProjectObserverDeleteTest.php-55-56 (1)

55-56: ⚠️ Potential issue | 🟡 Minor

Remove the unused $document variable.

It isn't used after assignment, and PHPMD is already flagging it.

Suggested cleanup

         /** Arrange */
-        $document = $this->project->documents()->first();

         /** Act */
         $this->project->delete();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/Project/ProjectObserverDeleteTest.php` around lines 55 - 56,
Remove the unused local variable assignment "$document =
$this->project->documents()->first();" in the ProjectObserverDeleteTest setup;
simply delete that line (or replace it with a direct call if side effects are
needed) so the test no longer defines an unused $document variable and PHPMD
warnings are resolved.