Skip to content

Commit 1d8898e

Browse files
committed
fix: three real bugs found in PR #82's post-merge code review
- MessageCreated and ParticipantCreated notifications had the same relation-loss-through-serialization bug ThreadCreated was fixed for in #82, but were never patched themselves: their models' "user" relation, attached only via setRelation(), is dropped when the ShouldQueue notification round-trips through SerializesModels (even on the sync connection). Every live-broadcast message/participant event was silently missing "user", breaking Thread.vue's sender name and MessageBubble's online dot. Fixed with the same explicit ->load(). - MessageService::threads() gained a 'messages' => latest()->limit(1) eager-load in #82 to power the Dashboard's message preview, but the method is shared with MessageController::index() (the GET /message JSON API), which silently started returning only 1 message per thread instead of the full history. Added an $onlyLatestMessage flag so only the Dashboard opts into the preview behavior; the API keeps its original full-list contract. - Dashboard.vue's live-update handlers under- and over-reported unread counts: a live ThreadCreated always showed unread_count 0 even when the recipient (not the creator) genuinely had 1 unread opening message, and a live MessageCreated bumped unread_count even for messages the viewer sent themselves in another tab. Both now compare the message's sender against the current user. All three came out of a code review of the already-merged #82; caught via CI-matching PHPUnit runs (no local Reverb server) plus TDD-style verification that each new test fails without its corresponding fix.
1 parent d5373fe commit 1d8898e

8 files changed

Lines changed: 139 additions & 9 deletions

File tree

app/Http/Controllers/FrontEnd/DashboardController.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ public function index(MessageServiceInterface $messages)
2222
{
2323
/** @var User $user */
2424
$user = Auth::user();
25-
$threads = $messages->threads($user);
25+
$threads = $messages->threads($user, onlyLatestMessage: true);
2626

2727
return view('dashboard', [
2828
'threads' => $threads->map(fn ($thread) => [

app/Interfaces/MessageServiceInterface.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,11 @@ interface MessageServiceInterface
1515
/**
1616
* All threads that user is participating in.
1717
*
18+
* @param bool $onlyLatestMessage Eager-load only each thread's single latest message
19+
* (for a preview), instead of its full message history.
1820
* @return LengthAwarePaginator<int, Thread>
1921
*/
20-
public function threads(User $user): LengthAwarePaginator;
22+
public function threads(User $user, bool $onlyLatestMessage = false): LengthAwarePaginator;
2123

2224
/**
2325
* All threads that user is participating in, with new messages.

app/Notifications/MessageCreated.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public function via($notifiable)
4646
public function toArray($notifiable)
4747
{
4848
return [
49-
'payload' => (new MessageResource($this->message))->resolve(),
49+
'payload' => (new MessageResource($this->message->load('user')))->resolve(),
5050
];
5151
}
5252

app/Notifications/ParticipantCreated.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ public function via($notifiable)
4545
public function toArray($notifiable)
4646
{
4747
return [
48-
'payload' => (new ParticipantResource($this->participant))->resolve(),
48+
'payload' => (new ParticipantResource($this->participant->load('user')))->resolve(),
4949
];
5050
}
5151
}

app/Services/MessageService.php

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,18 @@ public function __construct(ContactServiceInterface $contactService)
3333
/**
3434
* All threads that user is participating in.
3535
*
36+
* @param bool $onlyLatestMessage Eager-load only each thread's single latest message
37+
* (for a preview), instead of its full message history.
3638
* @return LengthAwarePaginator<int, Thread>
3739
*/
38-
public function threads(User $user): LengthAwarePaginator
40+
public function threads(User $user, bool $onlyLatestMessage = false): LengthAwarePaginator
3941
{
42+
$messagesRelation = $onlyLatestMessage
43+
? ['messages' => fn (Relation $query) => $query->latest()->limit(1)]
44+
: ['messages'];
45+
4046
return Thread::forUser($user->id)
41-
->with(['participants.user', 'messages' => fn (Relation $query) => $query->latest()->limit(1)])
47+
->with(array_merge(['participants.user'], $messagesRelation))
4248
->withCount('messages')
4349
->latest('updated_at')
4450
->paginate();

resources/js/pages/Dashboard.vue

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,15 @@ const threads = ref(props.threads)
1414
const { onlineUserIds } = useOnlinePresence()
1515
1616
function transformThread(payload) {
17+
const openingMessage = payload.attributes.messages?.[0]
18+
1719
return {
1820
id: payload.id,
1921
subject: payload.attributes.subject,
2022
updated_at: 'just now',
21-
unread_count: 0,
23+
unread_count: openingMessage?.attributes?.user_id === props.auth?.id ? 0 : 1,
2224
messages_count: payload.attributes.messages?.length ?? 0,
23-
last_message: payload.attributes.messages?.[0]?.attributes?.body ?? null,
25+
last_message: openingMessage?.attributes?.body ?? null,
2426
participants: (payload.attributes.participants ?? []).map((p) => ({
2527
user: { id: p.attributes?.user?.attributes?.id, name: p.attributes?.user?.attributes?.name },
2628
})),
@@ -47,10 +49,14 @@ if (props.auth) {
4749
return
4850
}
4951
52+
const isOwnMessage = payload.attributes?.user_id === props.auth?.id
53+
5054
const updated = {
5155
...threads.value[index],
5256
updated_at: 'just now',
53-
unread_count: (threads.value[index].unread_count ?? 0) + 1,
57+
unread_count: isOwnMessage
58+
? (threads.value[index].unread_count ?? 0)
59+
: (threads.value[index].unread_count ?? 0) + 1,
5460
messages_count: (threads.value[index].messages_count ?? 0) + 1,
5561
last_message: payload.attributes?.body ?? threads.value[index].last_message,
5662
}

tests/Feature/MessageTest.php

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,44 @@ public function test_message_controller_index_method()
5050
]);
5151
}
5252

53+
/**
54+
* The index endpoint must return each thread's full message history, not
55+
* just a preview — MessageService::threads() is shared with the
56+
* dashboard, which only wants a single-message preview, but this API
57+
* endpoint (consumed outside the web dashboard) must keep receiving the
58+
* complete list it always has.
59+
*
60+
* @return void
61+
*/
62+
public function test_message_controller_index_method_returns_full_message_history()
63+
{
64+
$sender = User::factory()->create(['notify_via' => []]);
65+
$recipient = User::factory()->create(['notify_via' => []]);
66+
67+
$thread = $this->service->newThread(
68+
'Full history',
69+
$sender,
70+
['type' => 'mood', 'payload' => ['mood' => 'happy', 'intensity' => 1], 'version' => '1.0'],
71+
[$recipient->id]
72+
);
73+
$this->service->newMessage($thread, $sender, ['type' => 'mood', 'payload' => ['mood' => 'happy', 'intensity' => 2], 'version' => '1.0']);
74+
$this->service->newMessage($thread, $sender, ['type' => 'mood', 'payload' => ['mood' => 'happy', 'intensity' => 3], 'version' => '1.0']);
75+
76+
$response = $this
77+
->actingAs($sender, 'api')
78+
->get(route('message'));
79+
80+
$response->assertOk();
81+
$response->assertJson([
82+
'data' => [
83+
[
84+
'id' => $thread->id,
85+
],
86+
],
87+
]);
88+
$response->assertJsonCount(3, 'data.0.attributes.messages');
89+
}
90+
5391
/**
5492
* Check store method of MessageController.
5593
*

tests/Unit/MessageTest.php

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use App\Interfaces\ContactServiceInterface;
66
use App\Interfaces\MessageServiceInterface;
7+
use App\Models\Message;
78
use App\Models\Participant;
89
use App\Models\Thread;
910
use App\Models\User;
@@ -294,6 +295,26 @@ public function test_service_method_new_message()
294295
$this->assertEquals($lastMessage, $lastInsertedMessage);
295296
}
296297

298+
/**
299+
* Sending a message must not leave it counted as unread for its own
300+
* sender — without marking the sender's own participant as read,
301+
* userUnreadMessagesCount() would count the sender's own message against
302+
* themselves.
303+
*
304+
* @return void
305+
*/
306+
public function test_new_message_does_not_count_as_unread_for_its_sender()
307+
{
308+
$sender = User::factory()->create();
309+
$recipient = User::factory()->create();
310+
311+
$thread = $this->service->newThread('Own message', $sender, $this->envelope(), [$recipient->id]);
312+
$this->service->newMessage($thread, $sender, $this->envelope('meh'));
313+
314+
$this->assertSame(0, $thread->userUnreadMessagesCount($sender->id));
315+
$this->assertSame(2, $thread->userUnreadMessagesCount($recipient->id));
316+
}
317+
297318
/**
298319
* Check if markAsRead method updates last read attribute.
299320
*
@@ -482,4 +503,61 @@ public function test_thread_created_toarray_reloads_participant_user_after_seria
482503

483504
$this->assertCount(2, $participantUserNames);
484505
}
506+
507+
/**
508+
* Same serialization round-trip gap as ThreadCreated, but for the
509+
* message's "user" relation: MessageService::newMessage() only attaches
510+
* it via setRelation(), so MessageCreated::toArray() must reload it
511+
* itself rather than assume it survives dispatch.
512+
*
513+
* @return void
514+
*/
515+
public function test_message_created_toarray_reloads_user_after_serialization_round_trip()
516+
{
517+
Notification::fake();
518+
519+
$sender = User::factory()->create(['notify_via' => ['broadcast']]);
520+
$recipient = User::factory()->create(['notify_via' => ['broadcast']]);
521+
522+
$thread = $this->service->newThread('Serialization Round Trip', $sender, $this->envelope(), [$recipient->id]);
523+
$message = $this->service->newMessage($thread, $sender, $this->envelope('meh'));
524+
525+
// Fetch a bare copy with "user" NOT loaded, mirroring what
526+
// SerializesModels restores after unserialize().
527+
$bareMessage = Message::findOrFail($message->id);
528+
529+
$notification = new MessageCreated($bareMessage);
530+
$payload = $this->resolvedPayload($notification->toArray($recipient));
531+
532+
$this->assertSame($sender->name, Arr::get($payload, 'payload.attributes.user.attributes.name'));
533+
}
534+
535+
/**
536+
* Same serialization round-trip gap as ThreadCreated, but for the
537+
* participant's "user" relation: MessageService::addParticipant() only
538+
* attaches it via setRelation(), so ParticipantCreated::toArray() must
539+
* reload it itself rather than assume it survives dispatch.
540+
*
541+
* @return void
542+
*/
543+
public function test_participant_created_toarray_reloads_user_after_serialization_round_trip()
544+
{
545+
Notification::fake();
546+
547+
$sender = User::factory()->create(['notify_via' => ['broadcast']]);
548+
$recipient = User::factory()->create(['notify_via' => ['broadcast']]);
549+
$newParticipant = User::factory()->create(['notify_via' => ['broadcast']]);
550+
551+
$thread = $this->service->newThread('Serialization Round Trip', $sender, $this->envelope(), [$recipient->id]);
552+
$participant = $this->service->addParticipant($thread, $newParticipant);
553+
554+
// Fetch a bare copy with "user" NOT loaded, mirroring what
555+
// SerializesModels restores after unserialize().
556+
$bareParticipant = Participant::findOrFail($participant->id);
557+
558+
$notification = new ParticipantCreated($bareParticipant);
559+
$payload = $this->resolvedPayload($notification->toArray($recipient));
560+
561+
$this->assertSame($newParticipant->name, Arr::get($payload, 'payload.attributes.user.attributes.name'));
562+
}
485563
}

0 commit comments

Comments
 (0)