mirror of
https://github.com/suitenumerique/messages.git
synced 2026-04-25 17:15:21 +02:00
🐛(backend) fix threads ordering (#617)
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.
This commit is contained in:
committed by
GitHub
parent
c1666b45fe
commit
d72df6c77d
@@ -3,6 +3,7 @@
|
||||
from django.conf import settings
|
||||
from django.db import transaction
|
||||
from django.db.models import Count, Exists, OuterRef, Q
|
||||
from django.db.models.functions import Coalesce
|
||||
|
||||
import rest_framework as drf
|
||||
from drf_spectacular.types import OpenApiTypes
|
||||
@@ -131,13 +132,13 @@ class ThreadViewSet(
|
||||
else:
|
||||
queryset = queryset.filter(**{filter_field: False})
|
||||
|
||||
order_field = self._get_order_field(query_params)
|
||||
queryset = queryset.order_by(f"-{order_field}", "-created_at")
|
||||
order_expression = self._get_order_expression(query_params)
|
||||
queryset = queryset.order_by(order_expression, "-created_at")
|
||||
return queryset
|
||||
|
||||
@staticmethod
|
||||
def _get_order_field(query_params):
|
||||
"""Return the appropriate ordering field based on the active view filter."""
|
||||
def _get_order_expression(query_params):
|
||||
"""Return the ordering expression based on the active view filter."""
|
||||
view_field_map = {
|
||||
"has_trashed": "trashed_messaged_at",
|
||||
"has_draft": "draft_messaged_at",
|
||||
@@ -148,8 +149,10 @@ class ThreadViewSet(
|
||||
}
|
||||
for param, field in view_field_map.items():
|
||||
if query_params.get(param) == "1":
|
||||
return field
|
||||
return "messaged_at"
|
||||
return f"-{field}"
|
||||
|
||||
# Draft-only threads have messaged_at=NULL, fall back to draft_messaged_at
|
||||
return Coalesce("messaged_at", "draft_messaged_at").desc()
|
||||
|
||||
def destroy(self, request, *args, **kwargs):
|
||||
"""Delete a thread, requiring EDITOR role on the thread."""
|
||||
|
||||
@@ -30,6 +30,211 @@ pytestmark = pytest.mark.django_db
|
||||
API_URL = reverse("threads-list")
|
||||
|
||||
|
||||
# --- Tests for thread ordering ---
|
||||
|
||||
|
||||
def test_list_threads_default_ordering_by_messaged_at(api_client):
|
||||
"""Test that threads are ordered by messaged_at descending by default."""
|
||||
user = UserFactory()
|
||||
api_client.force_authenticate(user=user)
|
||||
mailbox = MailboxFactory(users_read=[user])
|
||||
now = timezone.now()
|
||||
|
||||
thread_old = ThreadFactory(messaged_at=now - timedelta(hours=2), has_messages=True)
|
||||
ThreadAccessFactory(
|
||||
mailbox=mailbox,
|
||||
thread=thread_old,
|
||||
role=enums.ThreadAccessRoleChoices.EDITOR,
|
||||
)
|
||||
|
||||
thread_recent = ThreadFactory(
|
||||
messaged_at=now - timedelta(hours=1), has_messages=True
|
||||
)
|
||||
ThreadAccessFactory(
|
||||
mailbox=mailbox,
|
||||
thread=thread_recent,
|
||||
role=enums.ThreadAccessRoleChoices.EDITOR,
|
||||
)
|
||||
|
||||
response = api_client.get(API_URL, {"mailbox_id": str(mailbox.id)})
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
result_ids = [r["id"] for r in response.data["results"]]
|
||||
assert result_ids == [str(thread_recent.id), str(thread_old.id)]
|
||||
|
||||
|
||||
def test_list_threads_draft_only_thread_not_first(api_client):
|
||||
"""Test that a draft-only thread (messaged_at=NULL) does not appear before
|
||||
threads with actual messages. The Coalesce fallback to draft_messaged_at
|
||||
should position it according to its draft creation time."""
|
||||
user = UserFactory()
|
||||
api_client.force_authenticate(user=user)
|
||||
mailbox = MailboxFactory(users_read=[user])
|
||||
now = timezone.now()
|
||||
|
||||
# Thread with a regular message (messaged_at=now-1h)
|
||||
thread_with_message = ThreadFactory(
|
||||
messaged_at=now - timedelta(hours=1), has_messages=True
|
||||
)
|
||||
ThreadAccessFactory(
|
||||
mailbox=mailbox,
|
||||
thread=thread_with_message,
|
||||
role=enums.ThreadAccessRoleChoices.EDITOR,
|
||||
)
|
||||
|
||||
# Thread with only a draft (messaged_at=NULL, draft_messaged_at=now-2h)
|
||||
thread_draft_only = ThreadFactory(
|
||||
messaged_at=None,
|
||||
draft_messaged_at=now - timedelta(hours=2),
|
||||
has_draft=True,
|
||||
has_messages=True,
|
||||
)
|
||||
ThreadAccessFactory(
|
||||
mailbox=mailbox,
|
||||
thread=thread_draft_only,
|
||||
role=enums.ThreadAccessRoleChoices.EDITOR,
|
||||
)
|
||||
|
||||
response = api_client.get(API_URL, {"mailbox_id": str(mailbox.id)})
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
result_ids = [r["id"] for r in response.data["results"]]
|
||||
# messaged_at(1h ago) > draft_messaged_at(2h ago) → regular thread first
|
||||
assert result_ids == [str(thread_with_message.id), str(thread_draft_only.id)]
|
||||
|
||||
|
||||
def test_list_threads_draft_only_ordered_by_draft_date(api_client):
|
||||
"""Test that among draft-only threads, the most recent draft comes first."""
|
||||
user = UserFactory()
|
||||
api_client.force_authenticate(user=user)
|
||||
mailbox = MailboxFactory(users_read=[user])
|
||||
now = timezone.now()
|
||||
|
||||
thread_old_draft = ThreadFactory(
|
||||
messaged_at=None,
|
||||
draft_messaged_at=now - timedelta(hours=2),
|
||||
has_draft=True,
|
||||
has_messages=True,
|
||||
)
|
||||
ThreadAccessFactory(
|
||||
mailbox=mailbox,
|
||||
thread=thread_old_draft,
|
||||
role=enums.ThreadAccessRoleChoices.EDITOR,
|
||||
)
|
||||
|
||||
thread_recent_draft = ThreadFactory(
|
||||
messaged_at=None,
|
||||
draft_messaged_at=now - timedelta(hours=1),
|
||||
has_draft=True,
|
||||
has_messages=True,
|
||||
)
|
||||
ThreadAccessFactory(
|
||||
mailbox=mailbox,
|
||||
thread=thread_recent_draft,
|
||||
role=enums.ThreadAccessRoleChoices.EDITOR,
|
||||
)
|
||||
|
||||
response = api_client.get(API_URL, {"mailbox_id": str(mailbox.id)})
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
result_ids = [r["id"] for r in response.data["results"]]
|
||||
assert result_ids == [str(thread_recent_draft.id), str(thread_old_draft.id)]
|
||||
|
||||
|
||||
def test_list_threads_has_messages_filter_ordering(api_client):
|
||||
"""Test that has_messages=1 returns threads ordered correctly, with
|
||||
draft-only threads positioned by their draft date, not at the top."""
|
||||
user = UserFactory()
|
||||
api_client.force_authenticate(user=user)
|
||||
mailbox = MailboxFactory(users_read=[user])
|
||||
now = timezone.now()
|
||||
|
||||
# Thread with regular message (recent)
|
||||
thread_recent_msg = ThreadFactory(
|
||||
messaged_at=now - timedelta(minutes=30), has_messages=True
|
||||
)
|
||||
ThreadAccessFactory(
|
||||
mailbox=mailbox,
|
||||
thread=thread_recent_msg,
|
||||
role=enums.ThreadAccessRoleChoices.EDITOR,
|
||||
)
|
||||
|
||||
# Thread with only a draft (oldest)
|
||||
thread_draft = ThreadFactory(
|
||||
messaged_at=None,
|
||||
draft_messaged_at=now - timedelta(hours=2),
|
||||
has_draft=True,
|
||||
has_messages=True,
|
||||
)
|
||||
ThreadAccessFactory(
|
||||
mailbox=mailbox,
|
||||
thread=thread_draft,
|
||||
role=enums.ThreadAccessRoleChoices.EDITOR,
|
||||
)
|
||||
|
||||
# Thread with regular message (old)
|
||||
thread_old_msg = ThreadFactory(
|
||||
messaged_at=now - timedelta(hours=1), has_messages=True
|
||||
)
|
||||
ThreadAccessFactory(
|
||||
mailbox=mailbox,
|
||||
thread=thread_old_msg,
|
||||
role=enums.ThreadAccessRoleChoices.EDITOR,
|
||||
)
|
||||
|
||||
response = api_client.get(
|
||||
API_URL, {"mailbox_id": str(mailbox.id), "has_messages": "1"}
|
||||
)
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
result_ids = [r["id"] for r in response.data["results"]]
|
||||
# Expected: recent msg (30min), old msg (1h), draft (2h)
|
||||
assert result_ids == [
|
||||
str(thread_recent_msg.id),
|
||||
str(thread_old_msg.id),
|
||||
str(thread_draft.id),
|
||||
]
|
||||
|
||||
|
||||
def test_list_threads_view_specific_ordering(api_client):
|
||||
"""Test that view-specific filters use their dedicated ordering field."""
|
||||
user = UserFactory()
|
||||
api_client.force_authenticate(user=user)
|
||||
mailbox = MailboxFactory(users_read=[user])
|
||||
now = timezone.now()
|
||||
|
||||
# Thread with an old messaged_at but recent draft
|
||||
thread_a = ThreadFactory(
|
||||
messaged_at=now - timedelta(hours=2),
|
||||
draft_messaged_at=now,
|
||||
has_draft=True,
|
||||
has_messages=True,
|
||||
)
|
||||
ThreadAccessFactory(
|
||||
mailbox=mailbox,
|
||||
thread=thread_a,
|
||||
role=enums.ThreadAccessRoleChoices.EDITOR,
|
||||
)
|
||||
|
||||
# Thread with a less recent draft
|
||||
thread_b = ThreadFactory(
|
||||
messaged_at=None,
|
||||
draft_messaged_at=now - timedelta(hours=1),
|
||||
has_draft=True,
|
||||
has_messages=True,
|
||||
)
|
||||
ThreadAccessFactory(
|
||||
mailbox=mailbox,
|
||||
thread=thread_b,
|
||||
role=enums.ThreadAccessRoleChoices.EDITOR,
|
||||
)
|
||||
|
||||
# has_draft=1 should order by draft_messaged_at desc
|
||||
response = api_client.get(
|
||||
API_URL, {"mailbox_id": str(mailbox.id), "has_draft": "1"}
|
||||
)
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
result_ids = [r["id"] for r in response.data["results"]]
|
||||
# thread_a draft_messaged_at(now) > thread_b draft_messaged_at(1h ago)
|
||||
assert result_ids == [str(thread_a.id), str(thread_b.id)]
|
||||
|
||||
|
||||
def test_delete_thread_viewer_should_be_forbidden(api_client):
|
||||
"""Test that a user with only VIEWER access cannot delete a thread, but EDITOR can."""
|
||||
user = UserFactory()
|
||||
|
||||
@@ -47,7 +47,9 @@ export const ThreadItem = ({ thread, isSelected, onToggleSelection, selectedThre
|
||||
if (ViewHelper.isTrashedView() && thread.trashed_messaged_at) {
|
||||
return thread.trashed_messaged_at;
|
||||
}
|
||||
return thread.messaged_at;
|
||||
|
||||
// Draft-only threads have messaged_at=null, fall back to draft_messaged_at
|
||||
return thread.messaged_at || thread.draft_messaged_at;
|
||||
}, [thread])
|
||||
|
||||
const hasUnread = useMemo(() => {
|
||||
|
||||
Reference in New Issue
Block a user