fix: user has wrong discussion read status (#3591)

* test: deleting last post(s) then posting new replies works as expected
* fix: user has wrong discussion read status

extensions/subscriptions/tests/integration/api/discussions/ReplyNotificationTest.php

@@ -51,7 +51,7 @@ class ReplyNotificationTest extends TestCase
         /** @var User $mainUser */
         $mainUser = User::query()->find(1);
 
-        $this->assertEquals(0, $this->getUnreadNotificationCount($mainUser));
+        $this->assertEquals(0, $mainUser->getUnreadNotificationCount());
 
         $this->send(
             $this->request('POST', '/api/posts', [
@@ -69,7 +69,7 @@ class ReplyNotificationTest extends TestCase
             ])
         );
 
-        $this->assertEquals(1, $this->getUnreadNotificationCount($mainUser));
+        $this->assertEquals(1, $mainUser->getUnreadNotificationCount());
     }
 
     /** @test */
@@ -108,7 +108,7 @@ class ReplyNotificationTest extends TestCase
             ])
         );
 
-        $this->assertEquals(0, $this->getUnreadNotificationCount($mainUser));
+        $this->assertEquals(0, $mainUser->getUnreadNotificationCount());
 
         $this->send(
             $this->request('POST', '/api/posts', [
@@ -126,17 +126,72 @@ class ReplyNotificationTest extends TestCase
             ])
         );
 
-        $this->assertEquals(1, $this->getUnreadNotificationCount($mainUser));
+        $this->assertEquals(1, $mainUser->getUnreadNotificationCount());
     }
 
-    /** @todo change after core no longer statically caches unread notification in the User class */
+    /**
-    protected function getUnreadNotificationCount(User $user)
+     * @dataProvider deleteLastPostsProvider
+     * @test
+     */
+    public function deleting_last_posts_then_posting_new_one_sends_reply_notification(array $postIds)
     {
-        return $user->notifications()
+        $this->prepareDatabase([
-            ->where('type', 'newPost')
+            'users' => [
-            ->whereNull('read_at')
+                ['id' => 3, 'username' => 'acme', 'email' => 'acme@machine.local', 'is_email_confirmed' => 1],
-            ->where('is_deleted', false)
+            ],
-            ->whereSubjectVisibleTo($user)
+            'discussions' => [
-            ->count();
+                ['id' => 3, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 2, 'first_post_id' => 1, 'comment_count' => 5, 'last_post_number' => 5, 'last_post_id' => 10],
+            ],
+            'posts' => [
+                ['id' => 5, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 2, 'type' => 'comment', 'content' => '<t><p>foo bar</p></t>', 'number' => 1],
+                ['id' => 6, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>foo bar</p></t>', 'number' => 2],
+                ['id' => 7, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>foo bar</p></t>', 'number' => 3],
+                ['id' => 8, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>foo bar</p></t>', 'number' => 4],
+                ['id' => 9, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>foo bar</p></t>', 'number' => 5],
+                ['id' => 10, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>foo bar</p></t>', 'number' => 6],
+            ],
+            'discussion_user' => [
+                ['discussion_id' => 3, 'user_id' => 2, 'last_read_post_number' => 6, 'subscription' => 'follow'],
+            ]
+        ]);
+
+        // Delete the last 3 posts.
+        foreach ($postIds as $postId) {
+            $this->send(
+                $this->request('DELETE', '/api/posts/'.$postId, ['authenticatedAs' => 1])
+            );
+        }
+
+        /** @var User $mainUser */
+        $mainUser = User::query()->find(2);
+
+        $this->assertEquals(0, $mainUser->getUnreadNotificationCount());
+
+        // Reply as another user
+        $this->send(
+            $this->request('POST', '/api/posts', [
+                'authenticatedAs' => 3,
+                'json' => [
+                    'data' => [
+                        'attributes' => [
+                            'content' => 'reply with predetermined content for automated testing - too-obscure',
+                        ],
+                        'relationships' => [
+                            'discussion' => ['data' => ['id' => 3]],
+                        ],
+                    ],
+                ],
+            ])
+        );
+
+        $this->assertEquals(1, $mainUser->getUnreadNotificationCount());
+    }
+
+    public function deleteLastPostsProvider(): array
+    {
+        return [
+            [[10, 9, 8]],
+            [[8, 9, 10]]
+        ];
     }
 }

framework/core/src/Discussion/DiscussionServiceProvider.php

@@ -19,6 +19,7 @@ class DiscussionServiceProvider extends AbstractServiceProvider
     public function boot(Dispatcher $events)
     {
         $events->subscribe(DiscussionMetadataUpdater::class);
+        $events->subscribe(UserStateUpdater::class);
 
         $events->listen(
             Renamed::class,

framework/core/src/Discussion/UserState.php

@@ -46,6 +46,13 @@ class UserState extends AbstractModel
      */
     protected $dates = ['last_read_at'];
 
+    /**
+     * The attributes that are mass assignable.
+     *
+     * @var string[]
+     */
+    protected $fillable = ['last_read_post_number'];
+
     /**
      * Mark the discussion as being read up to a certain point. Raises the
      * DiscussionWasRead event.

framework/core/src/Discussion/UserStateUpdater.php

@@ -0,0 +1,39 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Discussion;
+
+use Flarum\Post\Event\Deleted;
+use Illuminate\Contracts\Events\Dispatcher;
+
+class UserStateUpdater
+{
+    public function subscribe(Dispatcher $events)
+    {
+        $events->listen(Deleted::class, [$this, 'whenPostWasDeleted']);
+    }
+
+    /**
+     * Updates a user state relative to a discussion.
+     * If user A read a discussion all the way to post number N, and X last posts were deleted,
+     * then we need to update user A's read status to the new N-X post number so that they get notified by new posts.
+     */
+    public function whenPostWasDeleted(Deleted $event)
+    {
+        /*
+         * We check if it's greater because at this point the DiscussionMetadataUpdater should have updated the last post.
+         */
+        if ($event->post->number > $event->post->discussion->last_post_number) {
+            UserState::query()
+                ->where('discussion_id', $event->post->discussion_id)
+                ->where('last_read_post_number', '>', $event->post->discussion->last_post_number)
+                ->update(['last_read_post_number' => $event->post->discussion->last_post_number]);
+        }
+    }
+}

framework/core/tests/integration/api/posts/DeleteTest.php

@@ -0,0 +1,94 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Tests\integration\api\posts;
+
+use Carbon\Carbon;
+use Flarum\Discussion\UserState;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+
+class DeleteTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+
+    /**
+     * @inheritDoc
+     */
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->prepareDatabase([
+            'users' => [
+                ['id' => 3, 'username' => 'acme', 'email' => 'acme@machine.local', 'is_email_confirmed' => 1],
+                ['id' => 4, 'username' => 'acme2', 'email' => 'acme2@machine.local', 'is_email_confirmed' => 1],
+            ],
+            'discussions' => [
+                ['id' => 3, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 2, 'first_post_id' => 1, 'comment_count' => 5, 'last_post_number' => 5, 'last_post_id' => 10],
+            ],
+            'posts' => [
+                ['id' => 5, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 2, 'type' => 'comment', 'content' => '<t><p>foo bar</p></t>', 'number' => 1],
+                ['id' => 6, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>foo bar</p></t>', 'number' => 2],
+                ['id' => 7, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>foo bar</p></t>', 'number' => 3],
+                ['id' => 8, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>foo bar</p></t>', 'number' => 4],
+                ['id' => 9, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>foo bar</p></t>', 'number' => 5],
+                ['id' => 10, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>foo bar</p></t>', 'number' => 6],
+            ],
+            'discussion_user' => [
+                ['discussion_id' => 3, 'user_id' => 2, 'last_read_post_number' => 6],
+                ['discussion_id' => 3, 'user_id' => 4, 'last_read_post_number' => 3],
+            ]
+        ]);
+    }
+
+    /**
+     * @dataProvider deleteLastPostsProvider
+     * @test
+     */
+    public function deleting_last_posts_syncs_discussion_state_for_other_users(array $postIds, int $newLastReadNumber, int $userId)
+    {
+        // Delete the last post.
+        foreach ($postIds as $postId) {
+            $this->send(
+                $this->request('DELETE', '/api/posts/'.$postId, ['authenticatedAs' => 1])
+            );
+        }
+
+        // User 2 should now have last_read_post_number set to the new last one
+        $this->assertEquals(
+            $newLastReadNumber,
+            UserState::query()
+                ->where('discussion_id', 3)
+                ->where('user_id', $userId)
+                ->first()
+                ->last_read_post_number
+        );
+    }
+
+    public function deleteLastPostsProvider(): array
+    {
+        return [
+            // User 2
+            [[10], 5, 2],
+            [[9, 10], 4, 2],
+            [[10, 9, 8], 3, 2],
+            [[8, 9, 10], 3, 2],
+            [[7, 8, 9, 10], 2, 2],
+
+            // User 4
+            [[10], 3, 4],
+            [[9, 10], 3, 4],
+            [[10, 9, 8], 3, 4],
+            [[8, 9, 10], 3, 4],
+            [[10, 9, 8, 7], 2, 4],
+            [[7, 8, 9, 10], 2, 4],
+        ];
+    }
+}