From 7081a3fdcad2ef1c9352e75356f7cbe7a2606483 Mon Sep 17 00:00:00 2001
From: Ryan Johnson <escattone@gmail.com>
Date: Tue, 5 Dec 2023 12:16:32 -0800
Subject: [PATCH] sub-class OrderingFilter to ensure nulls ordered last (#5800)

---
 kitsune/kpi/api.py                  |  5 +++--
 kitsune/questions/api.py            |  7 ++++---
 kitsune/questions/tests/test_api.py | 17 ++++++++++++++---
 kitsune/sumo/api_utils.py           | 23 ++++++++++++++++++++---
 kitsune/users/api.py                |  6 +++---
 5 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/kitsune/kpi/api.py b/kitsune/kpi/api.py
index b6c3bea4312..112c0d937be 100644
--- a/kitsune/kpi/api.py
+++ b/kitsune/kpi/api.py
@@ -10,7 +10,7 @@
 
 import django_filters
 from django_filters.rest_framework import DjangoFilterBackend
-from rest_framework import filters, serializers, viewsets
+from rest_framework import serializers, viewsets
 from rest_framework.views import APIView
 from rest_framework.response import Response
 
@@ -29,6 +29,7 @@
     EXIT_SURVEY_DONT_KNOW_CODE,
 )
 from kitsune.questions.models import Question, Answer, AnswerVote
+from kitsune.sumo.api_utils import OrderingFilter
 from kitsune.wiki.models import HelpfulVote
 from functools import reduce
 
@@ -491,7 +492,7 @@ class CohortViewSet(viewsets.ReadOnlyModelViewSet):
     filterset_class = CohortFilter
     filter_backends = [
         DjangoFilterBackend,
-        filters.OrderingFilter,
+        OrderingFilter,
     ]
     ordering_fields = [
         "start",
diff --git a/kitsune/questions/api.py b/kitsune/questions/api.py
index c1ff44f859d..52752c706ab 100644
--- a/kitsune/questions/api.py
+++ b/kitsune/questions/api.py
@@ -6,7 +6,7 @@
 from django import forms
 from django.db.models import Q
 from django_filters.rest_framework import DjangoFilterBackend
-from rest_framework import filters, pagination, permissions, serializers, status, viewsets
+from rest_framework import pagination, permissions, serializers, status, viewsets
 from rest_framework.decorators import action
 from rest_framework.response import Response
 from taggit.models import Tag
@@ -26,6 +26,7 @@
     DateTimeUTCField,
     GenericAPIException,
     OnlyCreatorEdits,
+    OrderingFilter,
     SplitSourceField,
 )
 from kitsune.sumo.utils import is_ratelimited
@@ -246,7 +247,7 @@ class QuestionViewSet(viewsets.ModelViewSet):
     filterset_class = QuestionFilter
     filter_backends = [
         DjangoFilterBackend,
-        filters.OrderingFilter,
+        OrderingFilter,
     ]
     ordering_fields = [
         "id",
@@ -486,7 +487,7 @@ class AnswerViewSet(viewsets.ModelViewSet):
     filterset_class = AnswerFilter
     filter_backends = [
         DjangoFilterBackend,
-        filters.OrderingFilter,
+        OrderingFilter,
     ]
     filterset_fields = [
         "question",
diff --git a/kitsune/questions/tests/test_api.py b/kitsune/questions/tests/test_api.py
index bd609b62b43..48ef17f25f3 100644
--- a/kitsune/questions/tests/test_api.py
+++ b/kitsune/questions/tests/test_api.py
@@ -350,18 +350,29 @@ def test_helpful_question_not_editable(self):
     def test_ordering(self):
         q1 = QuestionFactory()
         q2 = QuestionFactory()
+        q3 = QuestionFactory()
+        AnswerFactory(question=q1)
+        AnswerFactory(question=q2)
 
         res = self.client.get(reverse("question-list"))
-        self.assertEqual(res.data["results"][0]["id"], q2.id)
-        self.assertEqual(res.data["results"][1]["id"], q1.id)
+        self.assertEqual(res.data["results"][0]["id"], q3.id)
+        self.assertEqual(res.data["results"][1]["id"], q2.id)
+        self.assertEqual(res.data["results"][2]["id"], q1.id)
 
         res = self.client.get(reverse("question-list") + "?ordering=id")
         self.assertEqual(res.data["results"][0]["id"], q1.id)
         self.assertEqual(res.data["results"][1]["id"], q2.id)
+        self.assertEqual(res.data["results"][2]["id"], q3.id)
 
-        res = self.client.get(reverse("question-list") + "?ordering=-id")
+        res = self.client.get(reverse("question-list") + "?ordering=-last_answer")
         self.assertEqual(res.data["results"][0]["id"], q2.id)
         self.assertEqual(res.data["results"][1]["id"], q1.id)
+        self.assertEqual(res.data["results"][2]["id"], q3.id)
+
+        res = self.client.get(reverse("question-list") + "?ordering=last_answer")
+        self.assertEqual(res.data["results"][0]["id"], q1.id)
+        self.assertEqual(res.data["results"][1]["id"], q2.id)
+        self.assertEqual(res.data["results"][2]["id"], q3.id)
 
     def test_filter_product_with_slug(self):
         p1 = ProductFactory()
diff --git a/kitsune/sumo/api_utils.py b/kitsune/sumo/api_utils.py
index cb797b97f29..0bb0c348189 100644
--- a/kitsune/sumo/api_utils.py
+++ b/kitsune/sumo/api_utils.py
@@ -3,14 +3,14 @@
 from django import forms
 from django.contrib.contenttypes.models import ContentType
 from django.contrib.auth.models import User
+from django.db.models import F
 from django.http import HttpResponse
 from django.utils import translation
 from django.utils.translation import pgettext
 
-from rest_framework import fields, permissions, serializers
+from rest_framework import fields, filters, permissions, serializers
 from rest_framework.authentication import SessionAuthentication, CSRFCheck
 from rest_framework.exceptions import APIException, AuthenticationFailed
-from rest_framework.filters import BaseFilterBackend
 from rest_framework.renderers import JSONRenderer as DRFJSONRenderer
 
 from kitsune.users.models import Profile
@@ -184,7 +184,7 @@ def to_representation(self, value):
         return data
 
 
-class InequalityFilterBackend(BaseFilterBackend):
+class InequalityFilterBackend(filters.BaseFilterBackend):
     """A filter backend that allows for field__gt style filtering."""
 
     def filter_queryset(self, request, queryset, view):
@@ -350,3 +350,20 @@ def render(self, data, accepted_media_type=None, renderer_context=None):
         # JSON spec: http://json.org/
 
         return json.replace(b"</", b"<\\/")
+
+
+class OrderingFilter(filters.OrderingFilter):
+    """
+    Sub-class of rest_framework.filters.OrderingFilter that simply ensures that
+    any null values in fields requested in descending order are sorted last.
+    """
+
+    def get_ordering(self, request, queryset, view):
+        """
+        Replaces any ordering fields requested in descending order with an F()
+        expression that ensures any null values are sorted last.
+        """
+        return [
+            F(field[1:]).desc(nulls_last=True) if field.startswith("-") else field
+            for field in super().get_ordering(request, queryset, view)
+        ]
diff --git a/kitsune/users/api.py b/kitsune/users/api.py
index 4839ba8c495..dd8f439ff4d 100644
--- a/kitsune/users/api.py
+++ b/kitsune/users/api.py
@@ -8,7 +8,7 @@
 from django.utils.encoding import force_str
 from django.views.decorators.http import require_GET
 from django_filters.rest_framework import DjangoFilterBackend
-from rest_framework import filters, mixins, permissions, serializers, viewsets
+from rest_framework import mixins, permissions, serializers, viewsets
 from rest_framework.decorators import api_view, permission_classes
 from rest_framework.permissions import IsAuthenticated
 from rest_framework.response import Response
@@ -16,7 +16,7 @@
 from kitsune.access.decorators import login_required
 from kitsune.questions.models import Answer
 from kitsune.questions.utils import num_answers, num_questions, num_solutions
-from kitsune.sumo.api_utils import DateTimeUTCField, PermissionMod
+from kitsune.sumo.api_utils import DateTimeUTCField, OrderingFilter, PermissionMod
 from kitsune.sumo.decorators import json_view
 from kitsune.users.models import Profile, Setting
 from kitsune.users.templatetags.jinja_helpers import profile_avatar
@@ -248,7 +248,7 @@ class ProfileViewSet(
     ]
     filter_backends = [
         DjangoFilterBackend,
-        filters.OrderingFilter,
+        OrderingFilter,
     ]
     filterset_fields: list[str] = []
     ordering_fields: list[str] = []