Homework crosschecks now work between neighbour questions (#2751)

Other improvements:
* Reduced side effect probability in the TestUtilsMixin.update() method
* QuestionAdmin speedup
* Minor test readility improvement

Author: f213

src/apps/homework/admin/question/admin.py

@@ -1,19 +1,23 @@
-from django.db.models import QuerySet
+from typing import no_type_check
+
+from django.db.models import OuterRef, QuerySet, Subquery
 from django.http import HttpRequest
 from django.utils.translation import gettext_lazy as _
 from rest_framework.request import Request
 
 from apps.homework import tasks
 from apps.homework.admin.question.form import QuestionForm
 from apps.homework.models import Question
+from apps.products.models import Course
 from core.admin import ModelAdmin, admin
 
 
 @admin.register(Question)
 class QuestionAdmin(ModelAdmin):
     list_display = [
         "name",
-        "course",
+        "product",
+        "tariff",
     ]
     fields = [
         "module",
@@ -29,15 +33,29 @@ class QuestionAdmin(ModelAdmin):
     form = QuestionForm
 
     @admin.display(description=_("Course"))
-    def course(self, question: Question) -> str:
-        lesson = question.lesson_set.select_related("module", "module__course", "module__course__group").first()
-        if lesson is None:
-            return "-"
+    def product(self, question: Question) -> str:
+        return getattr(question, "product_name", "-") or "-"
 
-        return str(lesson.module.course)
+    @admin.display(description=_("Tariff Name"))
+    def tariff(self, question: Question) -> str:
+        return getattr(question, "course_name", "-") or "-"
 
+    @no_type_check
     def get_queryset(self, request: HttpRequest) -> QuerySet[Question]:
-        return super().get_queryset(request).prefetch_related("lesson_set")
+        queryset = super().get_queryset(request)
+
+        tariff = Course.objects.filter(
+            modules__lesson__question=OuterRef("pk"),
+        ).values("tariff_name")[:1]
+
+        group = Course.objects.filter(
+            modules__lesson__question=OuterRef("pk"),
+        ).values("group__name")[:1]
+
+        return queryset.annotate(
+            course_name=Subquery(tariff),
+            product_name=Subquery(group),
+        )
 
     @admin.action(description=_("Dispatch crosscheck"))
     def dispatch_crosscheck(self, request: Request, queryset: QuerySet) -> None:

src/apps/homework/factory.py

@@ -4,8 +4,11 @@
 
 
 @register
-def question(self: FixtureFactory, course: Course | None = None, **kwargs: dict) -> Question:
-    question = self.mixer.blend("homework.Question", _legacy_course=None, **kwargs)
+def question(self: FixtureFactory, course: Course | None = None, name: str | None = None, **kwargs: dict) -> Question:
+    question = Question.objects.create(
+        name=name or f"Please {self.faker.bs()} two times",
+        **kwargs,
+    )
 
     if course is not None:
         module = self.module(course=course)

src/apps/homework/services/question_crosscheck_dispatcher.py

@@ -4,67 +4,89 @@
 
 from apps.homework.models import Answer, AnswerCrossCheck, Question
 from apps.homework.services.answer_crosscheck_dispatcher import AnswerCrossCheckDispatcher
+from apps.lms.models import Lesson
 from apps.mailing.tasks import send_mail
 from apps.users.models import User
 from core.services import BaseService
 
 
 @dataclass
 class QuestionCrossCheckDispatcher(BaseService):
-    question: Question
-    answers_per_user: int = 3
+    """Dispatches crosschecks for given question (and its neighbours).
 
-    def __post_init__(self) -> None:
-        self.dispatcher = AnswerCrossCheckDispatcher(
-            answers=self.get_answers_to_check(),
-            answers_per_user=self.answers_per_user,
-        )
+    To improve readability, the user story is split in 2 services:
+        - QuestionCrossCheckDispatcher (this one) finds the answers to mix between students
+        - AnswerCrossCheckDispatcher mixes them between students
+    """
 
-        self.checks: list[AnswerCrossCheck] = list()
+    question: Question
+    answers_per_user: int = 3
 
     def act(self) -> int:
-        self.dispatch_crosschecks()
+        self.crosschecks = self.dispatch_crosschecks()
         self.notify_users()
 
         return self.get_users_to_notify().count()
 
-    def dispatch_crosschecks(self) -> None:
-        self.checks = self.dispatcher()
+    def dispatch_crosschecks(self) -> list[AnswerCrossCheck]:
+        dispatcher = AnswerCrossCheckDispatcher(
+            answers=self.get_answers_to_check(),
+            answers_per_user=self.answers_per_user,
+        )
+        return dispatcher()
 
     def notify_users(self) -> None:
         for user in self.get_users_to_notify():
-            user_checks_list = self.get_checks_for_user(user)
+            user_crosschecks_list = self.get_crosschecks_for_user(user)
             send_mail.delay(
                 to=user.email,
                 template_id="new-answers-to-check",
-                ctx=self.get_notification_context(user_checks_list),
+                ctx=self.get_notification_context(user_crosschecks_list),
                 disable_antispam=True,
             )
 
     def get_users_to_notify(self) -> QuerySet[User]:
-        return User.objects.filter(pk__in=[check.checker_id for check in self.checks])
+        return User.objects.filter(pk__in=[check.checker_id for check in self.crosschecks])
 
-    def get_checks_for_user(self, user: User) -> list[AnswerCrossCheck]:
-        return [check for check in self.checks if check.checker == user]
+    def get_crosschecks_for_user(self, user: User) -> list[AnswerCrossCheck]:
+        return [crosscheck for crosscheck in self.crosschecks if crosscheck.checker == user]
 
     def get_answers_to_check(self) -> QuerySet[Answer]:
+        questions = self.get_questions_to_check()
         return (
-            Answer.objects.filter(question=self.question)
+            Answer.objects.filter(question__in=questions)
             .root_only()
             .exclude(
                 do_not_crosscheck=True,
             )
         )
 
+    def get_questions_to_check(self) -> QuerySet[Question]:
+        """
+        Questions with the same name that belong to the same product.group
+        """
+        current_lesson = Lesson.objects.filter(question=self.question).first()
+        if current_lesson is None:
+            return Question.objects.filter(pk=self.question.pk)
+
+        same_group_lessons = Lesson.objects.filter(
+            module__course__group=current_lesson.module.course.group,
+            question__name__iexact=self.question.name,
+        )
+
+        return Question.objects.filter(
+            pk__in=same_group_lessons.values_list("question_id"),
+        )
+
     @staticmethod
-    def get_notification_context(checks: list[AnswerCrossCheck]) -> dict:
+    def get_notification_context(crosschecks: list[AnswerCrossCheck]) -> dict:
         answers = list()
 
-        for check in checks:
+        for crosscheck in crosschecks:
             answers.append(
                 {
-                    "url": check.answer.get_absolute_url(),
-                    "text": str(check.answer),
+                    "url": crosscheck.answer.get_absolute_url(),
+                    "text": str(crosscheck.answer),
                 }
             )
 

src/apps/homework/tests/homework/api/answers/tests_answer_create.py

@@ -214,7 +214,7 @@ def test_404_for_not_purchased_users(api, question):
 
 
 @pytest.mark.usefixtures("_no_purchase")
-def test_ok_for_users_that_purchased_another_course_from_the_group(api, question, course, factory):
+def test_ok_for_users_that_purchased_another_course_from_the_same_group(api, question, course, factory):
     """Create a purchase of the item with the same group as the course, and check if user can post an answer"""
     another_course = factory.course(group=course.group)
     factory.order(item=another_course, user=api.user, is_paid=True)
@@ -229,6 +229,23 @@ def test_ok_for_users_that_purchased_another_course_from_the_group(api, question
     )
 
 
+@pytest.mark.usefixtures("_no_purchase")
+def test_parent_answer_ok_for_users_that_purchased_another_course_from_the_same_group(api, question, course, another_answer, factory):
+    """Same as above, but for the non-root answer"""
+    another_course = factory.course(group=course.group)
+    factory.order(item=another_course, user=api.user, is_paid=True)
+
+    api.post(
+        "/api/v2/homework/answers/",
+        {
+            "question": question.slug,
+            "parent": another_answer.slug,
+            "content": JSON,
+        },
+        expected_status_code=201,
+    )
+
+
 @pytest.mark.usefixtures("_no_purchase")
 @pytest.mark.parametrize(
     "permission",
@@ -275,7 +292,7 @@ def test_ok_for_superusers(api, question):
     assert created.study is None
 
 
-def test_404_if_user_has_not_purchase_record_at_all(api, question, purchase):
+def test_404_if_user_has_no_order_at_all(api, question, purchase):
     purchase.delete()
 
     api.post(

src/apps/homework/tests/homework/cross_check/conftest.py

@@ -6,18 +6,41 @@
 
 
 @pytest.fixture
-def dispatcher():
-    return AnswerCrossCheckDispatcher
+def module(factory, course):
+    return factory.module(course=course)
 
 
 @pytest.fixture
-def question_dispatcher(question):
-    return QuestionCrossCheckDispatcher(question=question, answers_per_user=1)
+def another_module(factory, another_course):
+    return factory.module(course=another_course)
+
+
+@pytest.fixture
+def lesson(factory, module):
+    return factory.lesson(module=module)
 
 
 @pytest.fixture
-def question(factory):
-    return factory.question()
+def another_lesson(factory, another_module):
+    return factory.lesson(module=another_module)
+
+
+@pytest.fixture
+def question(factory, lesson):
+    question = factory.question()
+
+    lesson.update(question=question)
+
+    return question
+
+
+@pytest.fixture
+def another_question(factory, another_lesson):
+    question = factory.question()
+
+    another_lesson.update(question=question)
+
+    return question
 
 
 @pytest.fixture(autouse=True)
@@ -26,3 +49,22 @@ def answers(mixer, question, user, another_user):
         mixer.blend("homework.Answer", question=question, author=user, parent=None),
         mixer.blend("homework.Answer", question=question, author=another_user, parent=None),
     ]
+
+
+@pytest.fixture
+def answers_to_another_question(mixer, another_question):
+    even_more_users = mixer.cycle(2).blend("users.User")
+    return [
+        mixer.blend("homework.Answer", question=another_question, author=even_more_users[0], parent=None),
+        mixer.blend("homework.Answer", question=another_question, author=even_more_users[1], parent=None),
+    ]
+
+
+@pytest.fixture
+def answer_dispatcher():
+    return AnswerCrossCheckDispatcher
+
+
+@pytest.fixture
+def question_dispatcher(question):
+    return QuestionCrossCheckDispatcher(question=question, answers_per_user=1)

src/apps/homework/tests/homework/cross_check/tests_crosscheck_dispatcher.py

@@ -6,8 +6,8 @@
 
 
 @pytest.fixture
-def dispatcher(dispatcher, answers):
-    return dispatcher(answers=answers, answers_per_user=1)
+def dispatcher(answer_dispatcher, answers):
+    return answer_dispatcher(answers=answers, answers_per_user=1)
 
 
 @pytest.mark.repeat(10)

src/apps/homework/tests/homework/cross_check/tests_dispatch_answers_of_the_same_user.py

@@ -16,8 +16,8 @@ def answers(mixer, question, user, another_user):
 
 
 @pytest.fixture
-def dispatcher(dispatcher, answers):
-    return dispatcher(answers=answers)
+def dispatcher(answer_dispatcher, answers):
+    return answer_dispatcher(answers=answers)
 
 
 @pytest.fixture

src/apps/homework/tests/homework/cross_check/tests_get_answer_to_check.py

@@ -9,9 +9,9 @@
 
 
 @pytest.fixture
-def get(dispatcher, answers):
+def get(answer_dispatcher, answers):
     def _get(user):
-        service = dispatcher(answers=answers)
+        service = answer_dispatcher(answers=answers)
 
         return service.get_answer_to_check(user)
 

src/apps/homework/tests/homework/cross_check/tests_get_crosscheck_count.py

@@ -9,21 +9,21 @@ def get_crosscheck_count(answer, dispatcher):
     return answer.crosscheck_count
 
 
-def test_no_crosschecks(dispatcher, answers):
-    dispatcher = dispatcher(answers=answers)
+def test_no_crosschecks(answer_dispatcher, answers):
+    dispatcher = answer_dispatcher(answers=answers)
 
     assert get_crosscheck_count(answers[0], dispatcher) == 0
 
 
-def test_no_crosschecks_from_non_dispatched_users(dispatcher, mixer, answers):
+def test_no_crosschecks_from_non_dispatched_users(answer_dispatcher, mixer, answers):
     mixer.blend("homework.AnswerCrossCheck", answer=answers[1])
-    dispatcher = dispatcher(answers)
+    dispatcher = answer_dispatcher(answers)
 
     assert get_crosscheck_count(answers[1], dispatcher) == 0
 
 
-def test_crosschecks(dispatcher, mixer, answers):
+def test_crosschecks(answer_dispatcher, mixer, answers):
     mixer.blend("homework.AnswerCrossCheck", answer=answers[1], checker=answers[0].author)
-    dispatcher = dispatcher(answers)
+    dispatcher = answer_dispatcher(answers)
 
     assert get_crosscheck_count(answers[1], dispatcher) == 1