Skip to content

Commit 07f7826

Browse files
committed
🐛(backend) fix threads ordering
There was bug in the default thread ordering. Actually, on list that embed active and draft messages, some draft messages might have a messaged_at at None (New message) but when we were sort threads by this field, new draft message was sort at the top of the list because of null attribute. So in this case, we need to first try to sort by messaged_at then by draft_message_at.
1 parent f002a22 commit 07f7826

File tree

3 files changed

+217
-7
lines changed

3 files changed

+217
-7
lines changed

src/backend/core/api/viewsets/thread.py

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
from django.conf import settings
44
from django.db import transaction
55
from django.db.models import Count, Exists, OuterRef, Q
6+
from django.db.models.functions import Coalesce
67

78
import rest_framework as drf
89
from drf_spectacular.types import OpenApiTypes
@@ -131,13 +132,13 @@ def get_queryset(self, exclude_spam: bool = True, exclude_trashed: bool = True):
131132
else:
132133
queryset = queryset.filter(**{filter_field: False})
133134

134-
order_field = self._get_order_field(query_params)
135-
queryset = queryset.order_by(f"-{order_field}", "-created_at")
135+
order_expression = self._get_order_expression(query_params)
136+
queryset = queryset.order_by(order_expression, "-created_at")
136137
return queryset
137138

138139
@staticmethod
139-
def _get_order_field(query_params):
140-
"""Return the appropriate ordering field based on the active view filter."""
140+
def _get_order_expression(query_params):
141+
"""Return the ordering expression based on the active view filter."""
141142
view_field_map = {
142143
"has_trashed": "trashed_messaged_at",
143144
"has_draft": "draft_messaged_at",
@@ -148,8 +149,10 @@ def _get_order_field(query_params):
148149
}
149150
for param, field in view_field_map.items():
150151
if query_params.get(param) == "1":
151-
return field
152-
return "messaged_at"
152+
return f"-{field}"
153+
154+
# Draft-only threads have messaged_at=NULL, fall back to draft_messaged_at
155+
return Coalesce("messaged_at", "draft_messaged_at").desc()
153156

154157
def destroy(self, request, *args, **kwargs):
155158
"""Delete a thread, requiring EDITOR role on the thread."""

src/backend/core/tests/api/test_threads_list.py

Lines changed: 205 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,211 @@
3030
API_URL = reverse("threads-list")
3131

3232

33+
# --- Tests for thread ordering ---
34+
35+
36+
def test_list_threads_default_ordering_by_messaged_at(api_client):
37+
"""Test that threads are ordered by messaged_at descending by default."""
38+
user = UserFactory()
39+
api_client.force_authenticate(user=user)
40+
mailbox = MailboxFactory(users_read=[user])
41+
now = timezone.now()
42+
43+
thread_old = ThreadFactory(messaged_at=now - timedelta(hours=2), has_messages=True)
44+
ThreadAccessFactory(
45+
mailbox=mailbox,
46+
thread=thread_old,
47+
role=enums.ThreadAccessRoleChoices.EDITOR,
48+
)
49+
50+
thread_recent = ThreadFactory(
51+
messaged_at=now - timedelta(hours=1), has_messages=True
52+
)
53+
ThreadAccessFactory(
54+
mailbox=mailbox,
55+
thread=thread_recent,
56+
role=enums.ThreadAccessRoleChoices.EDITOR,
57+
)
58+
59+
response = api_client.get(API_URL, {"mailbox_id": str(mailbox.id)})
60+
assert response.status_code == status.HTTP_200_OK
61+
result_ids = [r["id"] for r in response.data["results"]]
62+
assert result_ids == [str(thread_recent.id), str(thread_old.id)]
63+
64+
65+
def test_list_threads_draft_only_thread_not_first(api_client):
66+
"""Test that a draft-only thread (messaged_at=NULL) does not appear before
67+
threads with actual messages. The Coalesce fallback to draft_messaged_at
68+
should position it according to its draft creation time."""
69+
user = UserFactory()
70+
api_client.force_authenticate(user=user)
71+
mailbox = MailboxFactory(users_read=[user])
72+
now = timezone.now()
73+
74+
# Thread with a regular message (messaged_at=now-1h)
75+
thread_with_message = ThreadFactory(
76+
messaged_at=now - timedelta(hours=1), has_messages=True
77+
)
78+
ThreadAccessFactory(
79+
mailbox=mailbox,
80+
thread=thread_with_message,
81+
role=enums.ThreadAccessRoleChoices.EDITOR,
82+
)
83+
84+
# Thread with only a draft (messaged_at=NULL, draft_messaged_at=now-2h)
85+
thread_draft_only = ThreadFactory(
86+
messaged_at=None,
87+
draft_messaged_at=now - timedelta(hours=2),
88+
has_draft=True,
89+
has_messages=True,
90+
)
91+
ThreadAccessFactory(
92+
mailbox=mailbox,
93+
thread=thread_draft_only,
94+
role=enums.ThreadAccessRoleChoices.EDITOR,
95+
)
96+
97+
response = api_client.get(API_URL, {"mailbox_id": str(mailbox.id)})
98+
assert response.status_code == status.HTTP_200_OK
99+
result_ids = [r["id"] for r in response.data["results"]]
100+
# messaged_at(1h ago) > draft_messaged_at(2h ago) → regular thread first
101+
assert result_ids == [str(thread_with_message.id), str(thread_draft_only.id)]
102+
103+
104+
def test_list_threads_draft_only_ordered_by_draft_date(api_client):
105+
"""Test that among draft-only threads, the most recent draft comes first."""
106+
user = UserFactory()
107+
api_client.force_authenticate(user=user)
108+
mailbox = MailboxFactory(users_read=[user])
109+
now = timezone.now()
110+
111+
thread_old_draft = ThreadFactory(
112+
messaged_at=None,
113+
draft_messaged_at=now - timedelta(hours=2),
114+
has_draft=True,
115+
has_messages=True,
116+
)
117+
ThreadAccessFactory(
118+
mailbox=mailbox,
119+
thread=thread_old_draft,
120+
role=enums.ThreadAccessRoleChoices.EDITOR,
121+
)
122+
123+
thread_recent_draft = ThreadFactory(
124+
messaged_at=None,
125+
draft_messaged_at=now - timedelta(hours=1),
126+
has_draft=True,
127+
has_messages=True,
128+
)
129+
ThreadAccessFactory(
130+
mailbox=mailbox,
131+
thread=thread_recent_draft,
132+
role=enums.ThreadAccessRoleChoices.EDITOR,
133+
)
134+
135+
response = api_client.get(API_URL, {"mailbox_id": str(mailbox.id)})
136+
assert response.status_code == status.HTTP_200_OK
137+
result_ids = [r["id"] for r in response.data["results"]]
138+
assert result_ids == [str(thread_recent_draft.id), str(thread_old_draft.id)]
139+
140+
141+
def test_list_threads_has_messages_filter_ordering(api_client):
142+
"""Test that has_messages=1 returns threads ordered correctly, with
143+
draft-only threads positioned by their draft date, not at the top."""
144+
user = UserFactory()
145+
api_client.force_authenticate(user=user)
146+
mailbox = MailboxFactory(users_read=[user])
147+
now = timezone.now()
148+
149+
# Thread with regular message (recent)
150+
thread_recent_msg = ThreadFactory(
151+
messaged_at=now - timedelta(minutes=30), has_messages=True
152+
)
153+
ThreadAccessFactory(
154+
mailbox=mailbox,
155+
thread=thread_recent_msg,
156+
role=enums.ThreadAccessRoleChoices.EDITOR,
157+
)
158+
159+
# Thread with only a draft (oldest)
160+
thread_draft = ThreadFactory(
161+
messaged_at=None,
162+
draft_messaged_at=now - timedelta(hours=2),
163+
has_draft=True,
164+
has_messages=True,
165+
)
166+
ThreadAccessFactory(
167+
mailbox=mailbox,
168+
thread=thread_draft,
169+
role=enums.ThreadAccessRoleChoices.EDITOR,
170+
)
171+
172+
# Thread with regular message (old)
173+
thread_old_msg = ThreadFactory(
174+
messaged_at=now - timedelta(hours=1), has_messages=True
175+
)
176+
ThreadAccessFactory(
177+
mailbox=mailbox,
178+
thread=thread_old_msg,
179+
role=enums.ThreadAccessRoleChoices.EDITOR,
180+
)
181+
182+
response = api_client.get(
183+
API_URL, {"mailbox_id": str(mailbox.id), "has_messages": "1"}
184+
)
185+
assert response.status_code == status.HTTP_200_OK
186+
result_ids = [r["id"] for r in response.data["results"]]
187+
# Expected: recent msg (30min), old msg (1h), draft (2h)
188+
assert result_ids == [
189+
str(thread_recent_msg.id),
190+
str(thread_old_msg.id),
191+
str(thread_draft.id),
192+
]
193+
194+
195+
def test_list_threads_view_specific_ordering(api_client):
196+
"""Test that view-specific filters use their dedicated ordering field."""
197+
user = UserFactory()
198+
api_client.force_authenticate(user=user)
199+
mailbox = MailboxFactory(users_read=[user])
200+
now = timezone.now()
201+
202+
# Thread with an old messaged_at but recent draft
203+
thread_a = ThreadFactory(
204+
messaged_at=now - timedelta(hours=2),
205+
draft_messaged_at=now,
206+
has_draft=True,
207+
has_messages=True,
208+
)
209+
ThreadAccessFactory(
210+
mailbox=mailbox,
211+
thread=thread_a,
212+
role=enums.ThreadAccessRoleChoices.EDITOR,
213+
)
214+
215+
# Thread with a less recent draft
216+
thread_b = ThreadFactory(
217+
messaged_at=None,
218+
draft_messaged_at=now - timedelta(hours=1),
219+
has_draft=True,
220+
has_messages=True,
221+
)
222+
ThreadAccessFactory(
223+
mailbox=mailbox,
224+
thread=thread_b,
225+
role=enums.ThreadAccessRoleChoices.EDITOR,
226+
)
227+
228+
# has_draft=1 should order by draft_messaged_at desc
229+
response = api_client.get(
230+
API_URL, {"mailbox_id": str(mailbox.id), "has_draft": "1"}
231+
)
232+
assert response.status_code == status.HTTP_200_OK
233+
result_ids = [r["id"] for r in response.data["results"]]
234+
# thread_a draft_messaged_at(now) > thread_b draft_messaged_at(1h ago)
235+
assert result_ids == [str(thread_a.id), str(thread_b.id)]
236+
237+
33238
def test_delete_thread_viewer_should_be_forbidden(api_client):
34239
"""Test that a user with only VIEWER access cannot delete a thread, but EDITOR can."""
35240
user = UserFactory()

src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,9 @@ export const ThreadItem = ({ thread, isSelected, onToggleSelection, selectedThre
4747
if (ViewHelper.isTrashedView() && thread.trashed_messaged_at) {
4848
return thread.trashed_messaged_at;
4949
}
50-
return thread.messaged_at;
50+
51+
// Draft-only threads have messaged_at=null, fall back to draft_messaged_at
52+
return thread.messaged_at || thread.draft_messaged_at;
5153
}, [thread])
5254

5355
const hasUnread = useMemo(() => {

0 commit comments

Comments
 (0)