From 96ecf511623939641ed09e4eb48be0568b60fbb1 Mon Sep 17 00:00:00 2001 From: Chen Asraf Date: Mon, 23 Mar 2026 01:49:45 +0200 Subject: [PATCH] fix: thread sorting --- lib/Controller/PostController.php | 5 +- lib/Db/ThreadMapper.php | 6 +- src/views/CategoryView.vue | 8 +- tests/unit/Controller/PostControllerTest.php | 111 ++++++++++++++++++ tests/unit/Db/ThreadMapperTest.php | 116 +++++++++++++++++++ 5 files changed, 232 insertions(+), 14 deletions(-) create mode 100644 tests/unit/Db/ThreadMapperTest.php diff --git a/lib/Controller/PostController.php b/lib/Controller/PostController.php index 8786b1e..1c24483 100644 --- a/lib/Controller/PostController.php +++ b/lib/Controller/PostController.php @@ -409,14 +409,13 @@ class PostController extends OCSController { } } - // Update the thread's post count and timestamps + // Update the thread's post count and last reply info try { $thread = $this->threadMapper->find($threadId); $thread->setPostCount($thread->getPostCount() + 1); $thread->setLastPostId($createdPost->getId()); $thread->setLastReplyAuthorId($createdPost->getAuthorId()); $thread->setLastReplyAt($createdPost->getCreatedAt()); - $thread->setUpdatedAt(time()); $this->threadMapper->update($thread); } catch (\Exception $e) { $this->logger->warning('Failed to update thread post count: ' . $e->getMessage()); @@ -568,8 +567,6 @@ class PostController extends OCSController { if (!$post->getIsFirstPost()) { $thread->setPostCount(max(0, $thread->getPostCount() - 1)); } - $thread->setUpdatedAt(time()); - // If the deleted post was the last post, update lastPostId to the previous non-deleted post if ($thread->getLastPostId() === $post->getId()) { // Find the latest non-deleted post in this thread (excluding the one being deleted) diff --git a/lib/Db/ThreadMapper.php b/lib/Db/ThreadMapper.php index d193125..cfb33a1 100644 --- a/lib/Db/ThreadMapper.php +++ b/lib/Db/ThreadMapper.php @@ -79,7 +79,7 @@ class ThreadMapper extends QBMapper { $qb->expr()->isNull('deleted_at') ) ->orderBy('is_pinned', 'DESC') - ->addOrderBy('updated_at', 'DESC') + ->addOrderBy($qb->createFunction('COALESCE(last_reply_at, created_at)'), 'DESC') ->setMaxResults($limit) ->setFirstResult($offset); return $this->findEntities($qb); @@ -97,7 +97,7 @@ class ThreadMapper extends QBMapper { $qb->expr()->isNull('deleted_at') ) ->orderBy('is_pinned', 'DESC') - ->addOrderBy('updated_at', 'DESC'); + ->addOrderBy($qb->createFunction('COALESCE(last_reply_at, created_at)'), 'DESC'); return $this->findEntities($qb); } @@ -352,7 +352,7 @@ class ThreadMapper extends QBMapper { ) ) ->orderBy('t.is_pinned', 'DESC') - ->addOrderBy('t.updated_at', 'DESC') + ->addOrderBy($qb->createFunction('COALESCE(t.last_reply_at, t.created_at)'), 'DESC') ->setMaxResults($limit) ->setFirstResult($offset); diff --git a/src/views/CategoryView.vue b/src/views/CategoryView.vue index 48d12e2..d09807c 100644 --- a/src/views/CategoryView.vue +++ b/src/views/CategoryView.vue @@ -204,13 +204,7 @@ export default defineComponent({ return (this.$route.params.slug as string) || null }, sortedThreads(): Thread[] { - // Sort pinned threads first, then by updatedAt descending - return [...this.threads].sort((a, b) => { - if (a.isPinned !== b.isPinned) { - return a.isPinned ? -1 : 1 - } - return b.updatedAt - a.updatedAt - }) + return this.threads }, }, created() { diff --git a/tests/unit/Controller/PostControllerTest.php b/tests/unit/Controller/PostControllerTest.php index 574f9fb..d4da306 100644 --- a/tests/unit/Controller/PostControllerTest.php +++ b/tests/unit/Controller/PostControllerTest.php @@ -867,6 +867,117 @@ class PostControllerTest extends TestCase { $this->assertEquals(Http::STATUS_OK, $response->getStatus()); } + public function testCreatePostDoesNotUpdateThreadUpdatedAt(): void { + $threadId = 1; + $content = 'New reply content'; + $userId = 'user1'; + $originalUpdatedAt = 1000000; + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($userId); + $this->userSession->method('getUser')->willReturn($user); + + $thread = new Thread(); + $thread->setId($threadId); + $thread->setCategoryId(1); + $thread->setPostCount(3); + $thread->setCreatedAt(900000); + $thread->setUpdatedAt($originalUpdatedAt); + + $category = new Category(); + $category->setId(1); + $category->setPostCount(5); + + $createdPost = $this->createMockPost(10, $threadId, $userId, $content); + + $this->postMapper->expects($this->once()) + ->method('insert') + ->willReturn($createdPost); + + $this->readMarkerMapper->method('createOrUpdate'); + + $this->threadMapper->expects($this->once()) + ->method('find') + ->willReturn($thread); + + $this->threadMapper->expects($this->once()) + ->method('update') + ->willReturnCallback(function ($updatedThread) use ($originalUpdatedAt) { + // updated_at should NOT be changed when a reply is created + $this->assertEquals($originalUpdatedAt, $updatedThread->getUpdatedAt()); + return $updatedThread; + }); + + $this->categoryMapper->method('find')->willReturn($category); + $this->categoryMapper->method('update')->willReturn($category); + $this->forumUserMapper->method('incrementPostCount'); + $this->notificationService->method('notifyThreadSubscribers'); + + $response = $this->controller->create($threadId, $content); + + $this->assertEquals(Http::STATUS_CREATED, $response->getStatus()); + } + + public function testDestroyPostDoesNotUpdateThreadUpdatedAt(): void { + $postId = 1; + $userId = 'user1'; + $originalUpdatedAt = 1000000; + + $post = $this->createMockPost($postId, 1, $userId, 'Test content'); + $post->setIsFirstPost(false); + + $thread = new Thread(); + $thread->setId(1); + $thread->setCategoryId(1); + $thread->setPostCount(3); + $thread->setLastPostId($postId); + $thread->setUpdatedAt($originalUpdatedAt); + + $category = new Category(); + $category->setId(1); + $category->setPostCount(5); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($userId); + $this->userSession->method('getUser')->willReturn($user); + + $this->permissionService->method('getCategoryIdFromPost')->willReturn(1); + $this->permissionService->method('hasCategoryPermission')->willReturn(false); + + $this->postMapper->expects($this->once()) + ->method('find') + ->willReturn($post); + + $this->postMapper->expects($this->once()) + ->method('update') + ->willReturn($post); + + $this->threadMapper->expects($this->once()) + ->method('find') + ->willReturn($thread); + + $lastPost = $this->createMockPost(2, 1, $userId, 'Previous reply'); + $this->postMapper->expects($this->once()) + ->method('findLatestByThreadId') + ->willReturn($lastPost); + + $this->threadMapper->expects($this->once()) + ->method('update') + ->willReturnCallback(function ($updatedThread) use ($originalUpdatedAt) { + // updated_at should NOT be changed when a reply is deleted + $this->assertEquals($originalUpdatedAt, $updatedThread->getUpdatedAt()); + return $updatedThread; + }); + + $this->categoryMapper->method('find')->willReturn($category); + $this->categoryMapper->method('update')->willReturn($category); + $this->forumUserMapper->method('decrementPostCount'); + + $response = $this->controller->destroy($postId); + + $this->assertEquals(Http::STATUS_OK, $response->getStatus()); + } + public function testByThreadPaginatedReturnsPostsSuccessfully(): void { $threadId = 1; $page = 1; diff --git a/tests/unit/Db/ThreadMapperTest.php b/tests/unit/Db/ThreadMapperTest.php new file mode 100644 index 0000000..f1a8ae4 --- /dev/null +++ b/tests/unit/Db/ThreadMapperTest.php @@ -0,0 +1,116 @@ +setLastReplyAt(1700000000); + $this->assertEquals(1700000000, $thread->getLastReplyAt()); + + $thread->setCreatedAt(1600000000); + $this->assertEquals(1600000000, $thread->getCreatedAt()); + + // last_reply_at can be null (no replies yet) + $thread->setLastReplyAt(null); + $this->assertNull($thread->getLastReplyAt()); + } + + public function testThreadSortOrderWithLastReplyAt(): void { + // Thread with a recent reply should sort before a thread with an older reply + $threadWithRecentReply = new Thread(); + $threadWithRecentReply->setCreatedAt(1000); + $threadWithRecentReply->setLastReplyAt(3000); + + $threadWithOldReply = new Thread(); + $threadWithOldReply->setCreatedAt(2000); + $threadWithOldReply->setLastReplyAt(2500); + + // COALESCE(last_reply_at, created_at) should be used for sorting: + // threadWithRecentReply: COALESCE(3000, 1000) = 3000 + // threadWithOldReply: COALESCE(2500, 2000) = 2500 + $sortKeyRecent = $threadWithRecentReply->getLastReplyAt() ?? $threadWithRecentReply->getCreatedAt(); + $sortKeyOld = $threadWithOldReply->getLastReplyAt() ?? $threadWithOldReply->getCreatedAt(); + + $this->assertGreaterThan($sortKeyOld, $sortKeyRecent); + } + + public function testThreadSortOrderFallsBackToCreatedAtWhenNoReplies(): void { + // Thread with no replies should use created_at for sorting + $newerThread = new Thread(); + $newerThread->setCreatedAt(2000); + $newerThread->setLastReplyAt(null); + + $olderThread = new Thread(); + $olderThread->setCreatedAt(1000); + $olderThread->setLastReplyAt(null); + + // COALESCE(null, created_at) = created_at + $sortKeyNewer = $newerThread->getLastReplyAt() ?? $newerThread->getCreatedAt(); + $sortKeyOlder = $olderThread->getLastReplyAt() ?? $olderThread->getCreatedAt(); + + $this->assertGreaterThan($sortKeyOlder, $sortKeyNewer); + } + + public function testThreadWithReplyOutranksNewerThreadWithoutReply(): void { + // An older thread with a recent reply should sort before a newer thread with no replies + $olderThreadWithReply = new Thread(); + $olderThreadWithReply->setCreatedAt(1000); + $olderThreadWithReply->setLastReplyAt(5000); + + $newerThreadNoReply = new Thread(); + $newerThreadNoReply->setCreatedAt(4000); + $newerThreadNoReply->setLastReplyAt(null); + + // COALESCE(5000, 1000) = 5000 vs COALESCE(null, 4000) = 4000 + $sortKeyOlderWithReply = $olderThreadWithReply->getLastReplyAt() ?? $olderThreadWithReply->getCreatedAt(); + $sortKeyNewerNoReply = $newerThreadNoReply->getLastReplyAt() ?? $newerThreadNoReply->getCreatedAt(); + + $this->assertGreaterThan($sortKeyNewerNoReply, $sortKeyOlderWithReply); + } + + public function testThreadSortOrderPinnedFirst(): void { + // Pinned threads should always sort before non-pinned, regardless of timestamps + $pinnedThread = new Thread(); + $pinnedThread->setIsPinned(true); + $pinnedThread->setCreatedAt(1000); + $pinnedThread->setLastReplyAt(null); + + $unpinnedThread = new Thread(); + $unpinnedThread->setIsPinned(false); + $unpinnedThread->setCreatedAt(9999); + $unpinnedThread->setLastReplyAt(9999); + + // Pinned threads always come first in the sort order + $this->assertTrue($pinnedThread->getIsPinned()); + $this->assertFalse($unpinnedThread->getIsPinned()); + } + + public function testThreadUpdatedAtIsIndependentOfSorting(): void { + // updated_at should NOT affect thread listing order + // A thread with a recent updated_at but old replies should sort by last_reply_at + $recentlyUpdatedThread = new Thread(); + $recentlyUpdatedThread->setCreatedAt(1000); + $recentlyUpdatedThread->setLastReplyAt(2000); + $recentlyUpdatedThread->setUpdatedAt(9999); // recently updated (e.g. title edit) + + $threadWithRecentReply = new Thread(); + $threadWithRecentReply->setCreatedAt(1500); + $threadWithRecentReply->setLastReplyAt(5000); + $threadWithRecentReply->setUpdatedAt(1500); // not recently updated + + // Sort key uses COALESCE(last_reply_at, created_at), NOT updated_at + $sortKeyUpdated = $recentlyUpdatedThread->getLastReplyAt() ?? $recentlyUpdatedThread->getCreatedAt(); + $sortKeyRecentReply = $threadWithRecentReply->getLastReplyAt() ?? $threadWithRecentReply->getCreatedAt(); + + // Thread with recent reply should sort first despite lower updated_at + $this->assertGreaterThan($sortKeyUpdated, $sortKeyRecentReply); + } +}