Merge pull request #1090 from marksweb/fix/1079

fix: resolve N+1 issue getting questions & answers

Author: amakarudze

applications/views.py

@@ -22,7 +22,11 @@ def apply(request, page_url):
     if not event:
         raise Http404
     elif isinstance(event, tuple):
-        return render(request, "applications/event_not_live.html", {"city": event[0], "past": event[1]})
+        return render(
+            request,
+            "applications/event_not_live.html",
+            {"city": event[0], "past": event[1]},
+        )
 
     form_obj = Form.objects.filter(event=event).first()
     if form_obj is None:
@@ -39,7 +43,10 @@ def apply(request, page_url):
 
     if form.is_valid():
         form.save()
-        messages.success(request, _("Yay! Your application has been saved. You'll hear from us soon!"))
+        messages.success(
+            request,
+            _("Yay! Your application has been saved. You'll hear from us soon!"),
+        )
 
         return render(
             request,
@@ -117,7 +124,12 @@ def applications_csv(request, page_url):
     response = HttpResponse(content_type="text/csv")
     response["Content-Disposition"] = f'attachment; filename="{page_url}.csv"'
     writer = csv.writer(response)
-    csv_header = [_("Application Number"), _("Application State"), _("RSVP Status"), _("Average Score")]
+    csv_header = [
+        _("Application Number"),
+        _("Application State"),
+        _("RSVP Status"),
+        _("Average Score"),
+    ]
     question_set = event.form.question_set
 
     question_titles = question_set.values_list("title", flat=True)
@@ -149,6 +161,8 @@ def application_detail(request, page_url, app_number):
         .order_by("-created")
         .first()
     )
+    # Get the related answers and questions to avoid N+1 queries in the template.
+    answers = application.answer_set.select_related("question").all()
     try:
         score = Score.objects.get(user=request.user, application=application)
     except Score.DoesNotExist:
@@ -177,13 +191,14 @@ def application_detail(request, page_url, app_number):
         request,
         "applications/application_detail.html",
         {
-            "event": event,
+            "answers": answers,
             "application": application,
+            "event": event,
             "form": application.form,
+            "menu": get_organiser_menu(page_url),
+            "score_form": score_form,
             "scores": all_scores,
             "user_score": score,
-            "score_form": score_form,
-            "menu": get_organiser_menu(page_url),
         },
     )
 
@@ -217,7 +232,11 @@ def compose_email(request, page_url, email_id=None):
     form_obj = get_object_or_404(Form, event=event)
     emailmsg = None if not email_id else get_object_or_404(Email, form__event=event, id=email_id)
 
-    form = EmailForm(request.POST or None, instance=emailmsg, initial={"author": request.user, "form": form_obj})
+    form = EmailForm(
+        request.POST or None,
+        instance=emailmsg,
+        initial={"author": request.user, "form": form_obj},
+    )
     if form.is_valid() and request.method == "POST":
         obj = form.save(commit=False)
         obj.author = request.user
@@ -293,7 +312,11 @@ def rsvp(request, page_url, code):
     if not event:
         raise Http404
     elif isinstance(event, tuple):
-        return render(request, "applications/event_not_live.html", {"city": event[0], "past": event[1]})
+        return render(
+            request,
+            "applications/event_not_live.html",
+            {"city": event[0], "past": event[1]},
+        )
 
     application, rsvp = Application.get_by_rsvp_code(code, event)
     if not application:
@@ -337,4 +360,8 @@ def rsvp(request, page_url, code):
 
     menu = EventPageMenu.objects.filter(event=event)
 
-    return render(request, "applications/rsvp.html", {"event": event, "menu": menu, "message": message})
+    return render(
+        request,
+        "applications/rsvp.html",
+        {"event": event, "menu": menu, "message": message},
+    )

templates/applications/application_detail.html

@@ -15,7 +15,7 @@ <h3 class="page-header">Application #{{ application.number }} for {{ event.page_
 
     <div class="row">
         <div class="col-md-7 applications">
-        {% for answer in application.answer_set.all %}
+        {% for answer in answers %}
             <div class="answer">
                 <p class="question">{{ answer.question.title|safe }}</p>
                 <p>{{ answer.answer|urlize|linebreaks }}</p>

tests/applications/views/test_applications.py

@@ -4,7 +4,7 @@
 from django.urls import reverse
 from django.utils import translation
 
-from applications.models import Application, Score
+from applications.models import Answer, Application, Question, Score
 from applications.views import application_list
 
 
@@ -203,12 +203,16 @@ def test_changing_application_status_errors(client, admin_client, future_event,
 
     # lack of state parameter
     resp = admin_client.post(
-        reverse("applications:change_state", args=[future_event.page_url]), {"application": application_submitted.id}
+        reverse("applications:change_state", args=[future_event.page_url]),
+        {"application": application_submitted.id},
     )
     assert "error" in resp.json()
 
     # lack of application parameter
-    resp = admin_client.post(reverse("applications:change_state", args=[future_event.page_url]), {"state": "accepted"})
+    resp = admin_client.post(
+        reverse("applications:change_state", args=[future_event.page_url]),
+        {"state": "accepted"},
+    )
     assert "error" in resp.json()
 
 
@@ -222,13 +226,15 @@ def test_changing_application_rsvp_errors(client, admin_client, future_event, ap
 
     # lack of rsvp_status parameter
     resp = admin_client.post(
-        reverse("applications:change_rsvp", args=[future_event.page_url]), {"application": application_submitted.id}
+        reverse("applications:change_rsvp", args=[future_event.page_url]),
+        {"application": application_submitted.id},
     )
     assert "error" in resp.json()
 
     # lack of application parameter
     resp = admin_client.post(
-        reverse("applications:change_rsvp", args=[future_event.page_url]), {"rsvp_status": Application.RSVP_YES}
+        reverse("applications:change_rsvp", args=[future_event.page_url]),
+        {"rsvp_status": Application.RSVP_YES},
     )
     assert "error" in resp.json()
 
@@ -238,7 +244,10 @@ def test_changing_application_status_in_bulk(admin_client, future_event, applica
     assert application_rejected.state == "rejected"
     resp = admin_client.post(
         reverse("applications:change_state", args=[future_event.page_url]),
-        {"state": "accepted", "application": [application_submitted.id, application_rejected.id]},
+        {
+            "state": "accepted",
+            "application": [application_submitted.id, application_rejected.id],
+        },
     )
     assert resp.status_code == 200
     application_submitted = Application.objects.get(id=application_submitted.id)
@@ -258,3 +267,52 @@ def test_application_scores_is_queried_once(admin_client, future_event, scored_a
 
     # The first query is for the annotation in get_applications_for_event, the second is for the scores themselves
     assert len(score_queries) == 2
+
+
+def test_application_detail_queries_optimized(admin_client, future_event, application_submitted, future_event_form):
+    """Regression test to ensure application_detail view doesn't have N+1 query problem with answers/questions."""
+    # Create multiple questions and answers for the application
+    questions = []
+    for i in range(5):
+        question = Question.objects.create(
+            form=future_event_form,
+            title=f"Test Question {i}",
+            question_type="text",
+            order=i,
+        )
+        questions.append(question)
+        Answer.objects.create(
+            application=application_submitted,
+            question=question,
+            answer=f"Test Answer {i}",
+        )
+
+    application_detail_url = reverse(
+        "applications:application_detail",
+        kwargs={
+            "page_url": future_event.page_url,
+            "app_number": application_submitted.number,
+        },
+    )
+
+    with CaptureQueriesContext(connection) as queries:
+        resp = admin_client.get(application_detail_url)
+        assert resp.status_code == 200
+
+    # Check that we're not making separate queries for each question
+    question_table_name = Question._meta.db_table
+    answer_table_name = Answer._meta.db_table
+
+    question_queries = [q for q in queries.captured_queries if question_table_name in q["sql"]]
+    answer_queries = [q for q in queries.captured_queries if answer_table_name in q["sql"]]
+
+    # With select_related, we should have only 1 query that joins answers and questions
+    # The query should join both tables, so it will appear in both lists
+    # We're checking that there's at most 1 query involving answers (with joined questions)
+    assert len(answer_queries) <= 1, f"Expected at most 1 answer query, got {len(answer_queries)}"
+
+    # Verify that if there are question queries, they're part of the joined query with answers
+    # There should not be 5 separate queries (one per question)
+    assert len(question_queries) <= 1, (
+        f"Expected at most 1 question query (joined with answers), got {len(question_queries)}"
+    )