Refactor and improve discussion suggestions

skole/schemas/comment.py

@@ -21,6 +21,7 @@
 from skole.types import ID, ResolveInfo
 from skole.utils.constants import Messages
 from skole.utils.pagination import get_paginator
+from skole.utils.shortcuts import join_queries
 
 
 class CommentObjectType(VoteMixin, DjangoObjectType):
@@ -188,88 +189,63 @@ def resolve_discussion_suggestions(
         country = getattr(city, "country", None)
         cut = settings.DISCUSSION_SUGGESTIONS_COUNT // 3
 
-        # Include schools that:
-        # - Is the user's school.
-        # - Have been commented by the user.
-        # - Have reply comments from the user.
-        schools = (
-            School.objects.filter(
-                Q(users=user)
-                | Q(comments__user=user)
-                | Q(comments__reply_comments__user=user)
-            )
-            .order_by("-comment_count")
-            .distinct()
+        # Note: the different Q object arguments passed to `join_queries` contain
+        # different priority filterings. E.g. `Q(users=user)` on the schools query is
+        # the most important one and its results should always appear first.
+
+        # Include:
+        # - Include the the user's school.
+        # - Schools that have been commented by the user or have reply comments from the user.
+        # - The best schools from the user's city.
+        # - The best schools from the user's country.
+        schools = join_queries(
+            School,
+            Q(users=user),
+            Q(comments__user=user) | Q(comments__reply_comments__user=user),
+            *([Q(city=city)] if city else []),
+            *([Q(city__country=country)] if country else []),
+            order_by=["-comment_count"],
         )
 
-        # If there are not enough schools in the queryset, add the best schools from the user's city.
-        if len(schools) < cut:
-            schools = schools.union(
-                School.objects.filter(city=city).order_by("-comment_count")
-            ).distinct()
-
-        # If there are still not enough schools, do the same based on the user's country.
-        if len(schools) < cut:
-            schools = schools.union(
-                School.objects.filter(city__country=country).order_by("-comment_count")
-            ).distinct()
-
         # Include courses that:
         # - Are created by the user.
         # - Have been starred by the user.
         # - Have been voted by the user.
         # - Have been commented by the user.
         # - Have reply comments from the user.
         # - Have resources added by the user.
-        courses = (
-            Course.objects.filter(
-                Q(user=user)
-                | Q(stars__user=user)
-                | Q(votes__user=user)
-                | Q(comments__user=user)
-                | Q(comments__reply_comments__user=user)
-                | Q(resources__user=user)
-            )
-            .order_by("-score", "-resource_count", "-comment_count")
-            .distinct()
+        courses = join_queries(
+            Course,
+            Q(user=user)
+            | Q(stars__user=user)
+            | Q(votes__user=user)
+            | Q(comments__user=user)
+            | Q(comments__reply_comments__user=user)
+            | Q(resources__user=user),
+            *([Q(subjects=user.subject)] if user.subject else []),
+            order_by=["-score", "-resource_count", "-comment_count"],
         )
 
-        # If there are not enough courses in the queryset, add the best courses from the user's subject.
-        if len(courses) < cut:
-            courses = courses.union(
-                Course.objects.filter(subjects=user.subject).order_by(
-                    "-score", "-resource_count", "-comment_count"
-                )
-            ).distinct()
-
         # Include resources that:
         # - Are created by the user.
         # - Have their course created by the user.
         # - Have been starred by the user.
         # - Have been voted by the user.
         # - Have been commented by the user.
         # - Have reply comments from the user.
-        resources = (
-            Resource.objects.filter(
-                Q(user=user)
-                | Q(course__user=user)
-                | Q(stars__user=user)
-                | Q(votes__user=user)
-                | Q(comments__user=user)
-                | Q(comments__reply_comments__user=user)
-            )
-            .order_by("-score", "-comment_count")
-            .distinct()
+        # - The best resources from the user's subject.
+        resources = join_queries(
+            Resource,
+            Q(user=user)
+            | Q(course__user=user)
+            | Q(stars__user=user)
+            | Q(votes__user=user)
+            | Q(comments__user=user)
+            | Q(comments__reply_comments__user=user),
+            *([Q(course__subjects=user.subject)] if user.subject else []),
+            order_by=["-score", "-comment_count"],
         )
 
-        # If there are not enough resources in the queryset, add the best resources from the user's subject.
-        if len(resources) < cut:
-            resources = resources.union(
-                Resource.objects.filter(course__subjects=user.subject).order_by(
-                    "-score", "-comment_count"
-                )
-            ).distinct()
-
         return [*schools[:cut], *courses[:cut], *resources[:cut]]
 
 

skole/schemas/gdpr.py

@@ -108,54 +108,57 @@ def mutate(cls, root: None, info: ResolveInfo) -> MyDataMutation:
         cls.__send_email(user, data_url)
         return cls(success_message=Messages.DATA_REQUEST_RECEIVED)
 
-    # fmt: off
-    # black slaughters the formatting of long queries.
     @classmethod
     def _created_comments(cls, user: User) -> QuerySet[Comment]:
-        return user.comments.annotate(
-            target=cls.__target_case()
-        ).annotate(
-            uploaded_attachment=cls.__file_case("attachment")
-        ).values(
-            "id",
-            "text",
-            "target",
-            "uploaded_attachment",
-            "modified",
-            "created",
+        return (
+            user.comments.annotate(target=cls.__target_case())
+            .annotate(uploaded_attachment=cls.__file_case("attachment"))
+            .values(
+                "id",
+                "text",
+                "target",
+                "uploaded_attachment",
+                "modified",
+                "created",
+            )
         )
 
     @classmethod
     def _created_courses(cls, user: User) -> QuerySet[Course]:
-        return user.created_courses.filter(
-            subjects__translations__language_code=settings.LANGUAGE_CODE
-        ).annotate(
-            subject_names=ArrayAgg("subjects__translations__name", distinct=True)
-        ).values(
-            "id",
-            "name",
-            "code",
-            "subject_names",
-            "modified",
-            "created",
+        return (
+            user.created_courses.filter(
+                subjects__translations__language_code=settings.LANGUAGE_CODE
+            )
+            .annotate(
+                subject_names=ArrayAgg("subjects__translations__name", distinct=True)
+            )
+            .values(
+                "id",
+                "name",
+                "code",
+                "subject_names",
+                "modified",
+                "created",
+            )
         )
 
     @classmethod
     def _created_resources(cls, user: User) -> QuerySet[Resource]:
-        return user.created_resources.filter(
-            resource_type__translations__language_code=settings.LANGUAGE_CODE
-        ).annotate(
-            uploaded_file=cls.__file_case("file")
-        ).values(
-            "id",
-            "course",
-            "title",
-            "uploaded_file",
-            "modified",
-            "created",
-            type=F("resource_type__translations__name"),
+        return (
+            user.created_resources.filter(
+                resource_type__translations__language_code=settings.LANGUAGE_CODE
+            )
+            .annotate(uploaded_file=cls.__file_case("file"))
+            .values(
+                "id",
+                "course",
+                "title",
+                "uploaded_file",
+                "modified",
+                "created",
+                type=F("resource_type__translations__name"),
+            )
         )
-    # fmt: on
 
     @classmethod
     def _activities(cls, user: User) -> QuerySet[Activity]:

skole/schemas/suggestions.py

@@ -1,12 +1,11 @@
 import random
-from typing import TypeVar, Union, cast
+from typing import TypeVar, cast
 
 import graphene
 from django.conf import settings
-from django.contrib.auth.models import AnonymousUser
 from django.db.models import QuerySet
 
-from skole.models import Comment, Course, Resource, User
+from skole.models import Comment, Course, Resource
 from skole.schemas.base import SkoleObjectType
 from skole.schemas.comment import CommentObjectType
 from skole.schemas.course import CourseObjectType
@@ -16,9 +15,7 @@
 T = TypeVar("T", bound=SuggestionModel)
 
 
-def get_suggestions(
-    user: Union[User, AnonymousUser], num_results: int
-) -> list[SuggestionModel]:
+def get_suggestions(num_results: int) -> list[SuggestionModel]:
     """
     Return a selection of trending comments, courses, and resources for suggestions.
 
@@ -72,15 +69,11 @@ class Query(SkoleObjectType):
     def resolve_suggestions(root: None, info: ResolveInfo) -> list[SuggestionModel]:
         """Return suggested courses, resources and comments based on secret Skole AI-
         powered algorithms."""
-
-        user = info.context.user
-        return get_suggestions(user, settings.SUGGESTIONS_COUNT)
+        return get_suggestions(settings.SUGGESTIONS_COUNT)
 
     @staticmethod
     def resolve_suggestions_preview(
         root: None, info: ResolveInfo
     ) -> list[SuggestionModel]:
         """Return preview of the suggestions."""
-
-        user = info.context.user
-        return get_suggestions(user, settings.SUGGESTIONS_PREVIEW_COUNT)
+        return get_suggestions(settings.SUGGESTIONS_PREVIEW_COUNT)

skole/tests/helpers.py

@@ -23,7 +23,7 @@
 from graphql_jwt.utils import delete_cookie
 
 from skole.models import User
-from skole.types import ID, JsonDict
+from skole.types import ID, AnyJson, JsonDict
 
 FileData = Optional[Collection[tuple[str, File]]]
 
@@ -132,7 +132,7 @@ def query(
 
     def execute(
         self, graphql: str, *, assert_error: bool = False, **kwargs: Any
-    ) -> JsonDict:
+    ) -> Any:
         """
         Run a GraphQL query, if `assert_error` parameter is False assert that status
         code was 200 (=syntax was ok) and that the result didn't have "error" section.
@@ -150,9 +150,9 @@ def execute(
             The resulting JSON data.name section if `assert_error` is False.
             Otherwise returns both the "error" and "data" sections.
 
-            Note: The return value is type hinted for simplicity to be a `JsonDict`,
-            but it can actually be `list[JsonDict]` in cases when we're fetching a list
-            of objects. In those cases we'll do a `cast()` before accessing the data.
+            Note: The return value is type hinted for simplicity to be a `Any`,
+            so that the callers never need to do any casting.
+            In reality this can return either a `JsonDict` or a `list[JsonDict]`.
         """
 
         if self.authenticated_user:
@@ -314,20 +314,22 @@ def _assert_response_has_errors(self, response: HttpResponse) -> None:
                 assert value is None
 
 
-def get_form_error(res: JsonDict, /) -> str:
+def get_form_error(res: AnyJson, /) -> str:
     """Return the first error message from a result containing a form mutation error."""
     try:
-        return res["errors"][0]["messages"][0]
+        # Ignore: It's fine if `res` is a `list[JsonDict]` and this raises an error.
+        return res["errors"][0]["messages"][0]  # type: ignore[call-overload]
     except (IndexError, KeyError, TypeError):
         raise AssertionError(
             f"`res` didn't contain a form mutation error: \n{res}"
         ) from None
 
 
-def get_graphql_error(res: JsonDict, /) -> str:
+def get_graphql_error(res: AnyJson, /) -> str:
     """Return the first error message from a result containing a GraphQL error."""
     try:
-        return res["errors"][0]["message"]
+        # Ignore: It's fine if `res` is a `list[JsonDict]` and this raises an error.
+        return res["errors"][0]["message"]  # type: ignore[call-overload]
     except (IndexError, KeyError, TypeError):
         raise AssertionError(f"`res` didn't contain a GraphQL error: \n{res}") from None
 

skole/tests/schemas/test_city.py

@@ -1,5 +1,3 @@
-from typing import cast
-
 from skole.tests.helpers import SkoleSchemaTestCase
 from skole.types import JsonDict
 
@@ -26,7 +24,7 @@ def query_autocomplete_cities(self) -> list[JsonDict]:
             }
             """
         )
-        return cast(list[JsonDict], self.execute(graphql))
+        return self.execute(graphql)
 
     def query_city(self, *, slug: str) -> JsonDict:
         variables = {"slug": slug}