Fix DB crash due to timeout (#1031)

* Create index on assessment ID in submissions table

Improves read performance.

* Create separate view layer for grading summary

* Move views under AdminGradingView

* Migrate to more optimised query

* Use subquery instead of join for answers
* Split main model query and view model generation
* Split queries into 3
* Use ORM where possible

* Fix typing

* Restore group filtering functionality

* Update Avenger backlog notification workers

Fixes compatibilty with new function signature.

* Remove old query

* Remove comment

Author: RichDom2185

lib/cadet/assessments/assessments.ex

@@ -1238,114 +1238,106 @@ defmodule Cadet.Assessments do
   {:forbidden, "Forbidden."}}
   """
   @spec all_submissions_by_grader_for_index(CourseRegistration.t()) ::
-          {:ok, String.t()}
+          {:ok, %{:assessments => [any()], :submissions => [any()], :users => [any()]}}
   def all_submissions_by_grader_for_index(
         grader = %CourseRegistration{course_id: course_id},
         group_only \\ false,
-        ungraded_only \\ false
+        _ungraded_only \\ false
       ) do
     show_all = not group_only
 
-    group_where =
+    group_filter =
       if show_all,
         do: "",
         else:
-          "where s.student_id in (select cr.id from course_registrations cr inner join groups g on cr.group_id = g.id where g.leader_id = $2) or s.student_id = $2"
+          "AND s.student_id IN (SELECT cr.id FROM course_registrations AS cr INNER JOIN groups AS g ON cr.group_id = g.id WHERE g.leader_id = #{grader.id}) OR s.student_id = #{grader.id}"
 
-    ungraded_where =
-      if ungraded_only,
-        do: "where s.\"gradedCount\" < assts.\"questionCount\"",
-        else: ""
-
-    params = if show_all, do: [course_id], else: [course_id, grader.id]
+    # TODO: Restore ungraded filtering
+    # ... or more likely, decouple email logic from this function
+    # ungraded_where =
+    #   if ungraded_only,
+    #     do: "where s.\"gradedCount\" < assts.\"questionCount\"",
+    #     else: ""
 
     # We bypass Ecto here and use a raw query to generate JSON directly from
     # PostgreSQL, because doing it in Elixir/Erlang is too inefficient.
 
-    case Repo.query(
-           """
-           select json_agg(q)::TEXT from
-           (
-             select
-               s.id,
-               s.status,
-               s."unsubmittedAt",
-               s.xp,
-               s."xpAdjustment",
-               s."xpBonus",
-               s."gradedCount",
-               assts.jsn as assessment,
-               students.jsn as student,
-               unsubmitters.jsn as "unsubmittedBy"
-             from
-               (select
-                 s.id,
-                 s.student_id,
-                 s.assessment_id,
-                 s.status,
-                 s.unsubmitted_at as "unsubmittedAt",
-                 s.unsubmitted_by_id,
-                 sum(ans.xp) as xp,
-                 sum(ans.xp_adjustment) as "xpAdjustment",
-                 s.xp_bonus as "xpBonus",
-                 count(ans.id) filter (where ans.grader_id is not null) as "gradedCount"
-               from submissions s
-                 left join
-                 answers ans on s.id = ans.submission_id
-               #{group_where}
-               group by s.id) s
-             inner join
-               (select
-                 a.id, a."questionCount", to_json(a) as jsn
-               from
-                 (select
-                   a.id,
-                   a.title,
-                   a.number as "assessmentNumber",
-                   bool_or(ac.is_manually_graded) as "isManuallyGraded",
-                   max(ac.type) as "type",
-                   sum(q.max_xp) as "maxXp",
-                   count(q.id) as "questionCount"
-                 from assessments a
-                   left join
-                   questions q on a.id = q.assessment_id
-                   inner join
-                   assessment_configs ac on ac.id = a.config_id
-                  where a.course_id = $1
-                 group by a.id) a) assts on assts.id = s.assessment_id
-             inner join
-               (select
-                 cr.id, to_json(cr) as jsn
-               from
-                 (select
-                   cr.id,
-                   u.name as "name",
-                   u.username as "username",
-                   g.name as "groupName",
-                   g.leader_id as "groupLeaderId"
-                 from course_registrations cr
-                   left join
-                   groups g on g.id = cr.group_id
-                   inner join
-                   users u on u.id = cr.user_id) cr) students on students.id = s.student_id
-             left join
-               (select
-                 cr.id, to_json(cr) as jsn
-               from
-                 (select
-                   cr.id,
-                   u.name
-                 from course_registrations cr
-                   inner join
-                   users u on u.id = cr.user_id) cr) unsubmitters on s.unsubmitted_by_id = unsubmitters.id
-             #{ungraded_where}
-           ) q
-           """,
-           params
-         ) do
-      {:ok, %{rows: [[nil]]}} -> {:ok, "[]"}
-      {:ok, %{rows: [[json]]}} -> {:ok, json}
-    end
+    submissions =
+      case Repo.query("""
+           SELECT
+             s.id,
+             s.status,
+             s.unsubmitted_at,
+             s.unsubmitted_by_id,
+             s_ans.xp,
+             s_ans.xp_adjustment,
+             s.xp_bonus,
+             s_ans.graded_count,
+             s.student_id,
+             s.assessment_id
+           FROM
+             submissions AS s
+             LEFT JOIN (
+               SELECT
+                 ans.submission_id,
+                 SUM(ans.xp) AS xp,
+                 SUM(ans.xp_adjustment) AS xp_adjustment,
+                 COUNT(ans.id) FILTER (
+                   WHERE
+                     ans.grader_id IS NOT NULL
+                 ) AS graded_count
+               FROM
+                 answers AS ans
+               GROUP BY
+                 ans.submission_id
+             ) AS s_ans ON s_ans.submission_id = s.id
+           WHERE
+             s.assessment_id IN (
+               SELECT
+                 id
+               FROM
+                 assessments
+               WHERE
+                 assessments.course_id = #{course_id}
+             ) #{group_filter};
+           """) do
+        {:ok, %{columns: columns, rows: result}} ->
+          result
+          |> Enum.map(
+            &(columns
+              |> Enum.map(fn c -> String.to_atom(c) end)
+              |> Enum.zip(&1)
+              |> Enum.into(%{}))
+          )
+      end
+
+    {:ok, generate_grading_summary_view_model(submissions, course_id)}
+  end
+
+  defp generate_grading_summary_view_model(submissions, course_id) do
+    users =
+      CourseRegistration
+      |> where([cr], cr.course_id == ^course_id)
+      |> join(:inner, [cr], u in assoc(cr, :user))
+      |> join(:left, [cr, u], g in assoc(cr, :group))
+      |> preload([cr, u, g], user: u, group: g)
+      |> Repo.all()
+
+    assessment_ids = submissions |> Enum.map(& &1.assessment_id) |> Enum.uniq()
+
+    assessments =
+      Assessment
+      |> where([a], a.id in ^assessment_ids)
+      |> join(:left, [a], q in assoc(a, :questions))
+      |> join(:inner, [a], ac in assoc(a, :config))
+      |> preload([a, q, ac], questions: q, config: ac)
+      |> Repo.all()
+
+    %{
+      users: users,
+      assessments: assessments,
+      submissions: submissions
+    }
   end
 
   @spec get_answers_in_submission(integer() | String.t()) ::

lib/cadet/workers/NotificationWorker.ex

@@ -73,15 +73,10 @@ defmodule Cadet.Workers.NotificationWorker do
           for avenger_cr <- avengers_crs do
             avenger = Cadet.Accounts.get_user(avenger_cr.user_id)
 
-            ungraded_submissions =
-              Jason.decode!(
-                elem(
-                  Cadet.Assessments.all_submissions_by_grader_for_index(avenger_cr, true, true),
-                  1
-                )
-              )
+            {:ok, %{submissions: ungraded_submissions}} =
+              Cadet.Assessments.all_submissions_by_grader_for_index(avenger_cr, true, true)
 
-            if length(ungraded_submissions) < ungraded_threshold do
+            if Enum.count(ungraded_submissions) < ungraded_threshold do
               IO.puts("[AVENGER_BACKLOG] below threshold!")
             else
               IO.puts("[AVENGER_BACKLOG] SENDING_OUT")

lib/cadet_web/admin_controllers/admin_grading_controller.ex

@@ -10,11 +10,11 @@ defmodule CadetWeb.AdminGradingController do
     group = String.to_atom(group)
 
     case Assessments.all_submissions_by_grader_for_index(course_reg, group) do
-      {:ok, submissions} ->
+      {:ok, view_model} ->
         conn
         |> put_status(:ok)
         |> put_resp_content_type("application/json")
-        |> text(submissions)
+        |> render("gradingsummaries.json", view_model)
     end
   end
 

lib/cadet_web/admin_views/admin_grading_view.ex

@@ -7,6 +7,94 @@ defmodule CadetWeb.AdminGradingView do
     render_many(answers, CadetWeb.AdminGradingView, "grading_info.json", as: :answer)
   end
 
+  def render("gradingsummaries.json", %{
+        users: users,
+        assessments: assessments,
+        submissions: submissions
+      }) do
+    for submission <- submissions do
+      user = users |> Enum.find(&(&1.id == submission.student_id))
+      assessment = assessments |> Enum.find(&(&1.id == submission.assessment_id))
+
+      render(
+        CadetWeb.AdminGradingView,
+        "gradingsummary.json",
+        %{
+          user: user,
+          assessment: assessment,
+          submission: submission,
+          unsubmitter:
+            case submission.unsubmitted_by_id do
+              nil -> nil
+              _ -> users |> Enum.find(&(&1.id == submission.unsubmitted_by_id))
+            end
+        }
+      )
+    end
+  end
+
+  def render("gradingsummary.json", %{
+        user: user,
+        assessment: a,
+        submission: s,
+        unsubmitter: unsubmitter
+      }) do
+    s
+    |> transform_map_for_view(%{
+      id: :id,
+      status: :status,
+      unsubmittedAt: :unsubmitted_at,
+      xp: :xp,
+      xpAdjustment: :xp_adjustment,
+      xpBonus: :xp_bonus,
+      gradedCount:
+        &case &1.graded_count do
+          nil -> 0
+          x -> x
+        end
+    })
+    |> Map.merge(%{
+      assessment:
+        render_one(a, CadetWeb.AdminGradingView, "gradingsummaryassessment.json", as: :assessment),
+      student: render_one(user, CadetWeb.AdminGradingView, "gradingsummaryuser.json", as: :cr),
+      unsubmittedBy:
+        case unsubmitter do
+          nil -> nil
+          cr -> transform_map_for_view(cr, %{id: :id, name: & &1.user.name})
+        end
+    })
+  end
+
+  def render("gradingsummaryassessment.json", %{assessment: a}) do
+    %{
+      id: a.id,
+      title: a.title,
+      assessmentNumber: a.number,
+      isManuallyGraded: a.config.is_manually_graded,
+      type: a.config.type,
+      maxXp: a.questions |> Enum.map(& &1.max_xp) |> Enum.sum(),
+      questionCount: a.questions |> Enum.count()
+    }
+  end
+
+  def render("gradingsummaryuser.json", %{cr: cr}) do
+    %{
+      id: cr.id,
+      name: cr.user.name,
+      username: cr.user.username,
+      groupName:
+        case cr.group do
+          nil -> nil
+          _ -> cr.group.name
+        end,
+      groupLeaderId:
+        case cr.group do
+          nil -> nil
+          _ -> cr.group.leader_id
+        end
+    }
+  end
+
   def render("grading_info.json", %{answer: answer}) do
     transform_map_for_view(answer, %{
       student:

priv/repo/migrations/20231105164101_create_submissions_assessment_index.exs

@@ -0,0 +1,11 @@
+defmodule Cadet.Repo.Migrations.CreateSubmissionsAssessmentIndex do
+  use Ecto.Migration
+
+  def up do
+    create(index(:submissions, [:assessment_id]))
+  end
+
+  def down do
+    drop(index(:submissions, [:assessment_id]))
+  end
+end

test/cadet/email_test.exs

@@ -24,10 +24,8 @@ defmodule Cadet.EmailTest do
     avenger_user = insert(:user, %{email: "[email protected]"})
     avenger = insert(:course_registration, %{user: avenger_user, role: :staff})
 
-    ungraded_submissions =
-      Jason.decode!(
-        elem(Cadet.Assessments.all_submissions_by_grader_for_index(avenger, true, true), 1)
-      )
+    {:ok, %{submissions: ungraded_submissions}} =
+      Cadet.Assessments.all_submissions_by_grader_for_index(avenger, true, true)
 
     email = Email.avenger_backlog_email("avenger_backlog", avenger_user, ungraded_submissions)
 

test/cadet/jobs/notification_worker/notification_worker_test.exs

@@ -18,10 +18,8 @@ defmodule Cadet.NotificationWorker.NotificationWorkerTest do
     submission = List.first(List.first(data.mcq_answers)).submission
 
     # setup for avenger backlog
-    ungraded_submissions =
-      Jason.decode!(
-        elem(Cadet.Assessments.all_submissions_by_grader_for_index(avenger_cr, true, true), 1)
-      )
+    {:ok, %{submissions: ungraded_submissions}} =
+      Cadet.Assessments.all_submissions_by_grader_for_index(avenger_cr, true, true)
 
     Repo.update_all(NotificationType, set: [is_enabled: true])
     Repo.update_all(NotificationConfig, set: [is_enabled: true])