Remove unnecessary trips to the DB (#1189)

* Use subquery to get submission IDs

* Add TODO

* Fix n+1 query problem

* Fix typo

* Fix tests

* Comment Sentry DSN in example secrets

* Fix admin assessments controller

* fix: Fix n + 1 query in assessments_view

* Fix format

* Fix format

Author: RichDom2185

config/dev.secrets.exs.example

@@ -104,8 +104,8 @@ config :cadet,
   # optional, passed to [HTTPoison.Request](https://hexdocs.pm/httpoison/HTTPoison.Request.html) options
   http_options: [recv_timeout: 170_0000]
 
-config :sentry,
-  dsn: "https://public_key/sentry.io/somethingsomething"
+# config :sentry,
+#   dsn: "https://public_key/sentry.io/somethingsomething"
 
 # # Additional configuration for SAML authentication
 # # For more details, see https://github.com/handnot2/samly

lib/cadet/assessments/assessments.ex

@@ -113,7 +113,7 @@ defmodule Cadet.Assessments do
       Submission
       |> where(
         [s],
-        s.id in ^submission_ids
+        s.id in subquery(submission_ids)
       )
       |> where(is_grading_published: true)
       |> join(:inner, [s], a in Answer, on: s.id == a.submission_id)
@@ -337,7 +337,7 @@ defmodule Cadet.Assessments do
       |> join(:left, [s], ans in Answer, on: ans.submission_id == s.id)
       |> where(
         [s],
-        s.id in ^submission_ids
+        s.id in subquery(submission_ids)
       )
       |> group_by([s], s.assessment_id)
       |> select([s, ans], %{
@@ -351,7 +351,7 @@ defmodule Cadet.Assessments do
       Submission
       |> where(
         [s],
-        s.id in ^submission_ids
+        s.id in subquery(submission_ids)
       )
       |> select([s], [:assessment_id, :status, :is_grading_published])
 
@@ -382,21 +382,46 @@ defmodule Cadet.Assessments do
   end
 
   defp get_submission_ids(cr_id, teams) do
-    query =
-      from(s in Submission,
-        where: s.student_id == ^cr_id or s.team_id in ^Enum.map(teams, & &1.id),
-        select: s.id
-      )
+    from(s in Submission,
+      where: s.student_id == ^cr_id or s.team_id in ^Enum.map(teams, & &1.id),
+      select: s.id
+    )
+  end
 
-    Repo.all(query)
+  defp is_voting_assigned(assessment_ids) do
+    voting_assigned_question_ids =
+      SubmissionVotes
+      |> select([v], v.question_id)
+      |> Repo.all()
+
+    # Map of assessment_id to boolean
+    voting_assigned_assessment_ids =
+      Question
+      |> where(type: :voting)
+      |> where([q], q.id in ^voting_assigned_question_ids)
+      |> where([q], q.assessment_id in ^assessment_ids)
+      |> select([q], q.assessment_id)
+      |> distinct(true)
+      |> Repo.all()
+
+    Enum.reduce(assessment_ids, %{}, fn id, acc ->
+      Map.put(acc, id, Enum.member?(voting_assigned_assessment_ids, id))
+    end)
   end
 
   @doc """
   A helper function which removes grading information from all assessments
   if it's grading is not published.
   """
   def format_all_assessments(assessments) do
+    is_voting_assigned_map =
+      assessments
+      |> Enum.map(& &1.id)
+      |> is_voting_assigned()
+
     Enum.map(assessments, fn a ->
+      a = Map.put(a, :is_voting_published, Map.get(is_voting_assigned_map, a.id, false))
+
       if a.is_grading_published do
         a
       else
@@ -653,17 +678,16 @@ defmodule Cadet.Assessments do
     end
   end
 
-  def is_voting_published(assessment_id) do
+  defp is_voting_published(assessment_id) do
     voting_assigned_question_ids =
       SubmissionVotes
       |> select([v], v.question_id)
-      |> Repo.all()
 
     Question
     |> where(type: :voting)
     |> where(assessment_id: ^assessment_id)
-    |> where([q], q.id in ^voting_assigned_question_ids)
-    |> Repo.exists?()
+    |> where([q], q.id in subquery(voting_assigned_question_ids))
+    |> Repo.exists?() || false
   end
 
   def update_final_contest_entries do
@@ -1442,6 +1466,7 @@ defmodule Cadet.Assessments do
 
   @spec update_xp_bonus(Submission.t()) ::
           {:ok, Submission.t()} | {:error, Ecto.Changeset.t()}
+  # TODO: Should destructure and pattern match on the function
   defp update_xp_bonus(submission = %Submission{id: submission_id}) do
     # to ensure backwards compatibility
     if submission.xp_bonus == 0 do

lib/cadet_web/admin_controllers/admin_assessments_controller.ex

@@ -14,7 +14,7 @@ defmodule CadetWeb.AdminAssessmentsController do
   def index(conn, %{"course_reg_id" => course_reg_id}) do
     course_reg = Repo.get(CourseRegistration, course_reg_id)
     {:ok, assessments} = Assessments.all_assessments(course_reg)
-
+    assessments = Assessments.format_all_assessments(assessments)
     render(conn, "index.json", assessments: assessments)
   end
 

lib/cadet_web/admin_views/admin_assessments_view.ex

@@ -2,9 +2,6 @@ defmodule CadetWeb.AdminAssessmentsView do
   use CadetWeb, :view
   use Timex
   import CadetWeb.AssessmentsHelpers
-  import Ecto.Query
-  alias Cadet.Assessments.{Question, SubmissionVotes}
-  alias Cadet.Repo
 
   def render("index.json", %{assessments: assessments}) do
     render_many(assessments, CadetWeb.AdminAssessmentsView, "overview.json", as: :assessment)
@@ -35,7 +32,7 @@ defmodule CadetWeb.AdminAssessmentsView do
       maxTeamSize: :max_team_size,
       hasVotingFeatures: :has_voting_features,
       hasTokenCounter: :has_token_counter,
-      isVotingPublished: &is_voting_assigned(&1.id)
+      isVotingPublished: :is_voting_published
     })
   end
 
@@ -84,17 +81,4 @@ defmodule CadetWeb.AdminAssessmentsView do
   defp password_protected?(nil), do: false
 
   defp password_protected?(_), do: true
-
-  defp is_voting_assigned(assessment_id) do
-    voting_assigned_question_ids =
-      SubmissionVotes
-      |> select([v], v.question_id)
-      |> Repo.all()
-
-    Question
-    |> where(type: :voting)
-    |> where(assessment_id: ^assessment_id)
-    |> where([q], q.id in ^voting_assigned_question_ids)
-    |> Repo.exists?()
-  end
 end

lib/cadet_web/views/assessments_view.ex

@@ -1,9 +1,6 @@
 defmodule CadetWeb.AssessmentsView do
   use CadetWeb, :view
   use Timex
-  import Ecto.Query
-  alias Cadet.Assessments.{Question, SubmissionVotes}
-  alias Cadet.Repo
 
   import CadetWeb.AssessmentsHelpers
 
@@ -37,7 +34,7 @@ defmodule CadetWeb.AssessmentsView do
       maxTeamSize: :max_team_size,
       hasVotingFeatures: :has_voting_features,
       hasTokenCounter: :has_token_counter,
-      isVotingPublished: &is_voting_assigned(&1.id),
+      isVotingPublished: :is_voting_published,
       hoursBeforeEarlyXpDecay: & &1.config.hours_before_early_xp_decay
     })
   end
@@ -87,17 +84,4 @@ defmodule CadetWeb.AssessmentsView do
   defp password_protected?(nil), do: false
 
   defp password_protected?(_), do: true
-
-  defp is_voting_assigned(assessment_id) do
-    voting_assigned_question_ids =
-      SubmissionVotes
-      |> select([v], v.question_id)
-      |> Repo.all()
-
-    Question
-    |> where(type: :voting)
-    |> where(assessment_id: ^assessment_id)
-    |> where([q], q.id in ^voting_assigned_question_ids)
-    |> Repo.exists?()
-  end
 end

test/cadet_web/admin_controllers/admin_assessments_controller_test.exs

@@ -90,11 +90,11 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
             "isPublished" => &1.is_published,
             "gradedCount" => 0,
             "questionCount" => 9,
-            "xp" => (800 + 500 + 100) * 3,
+            "xp" => if(&1.is_grading_published, do: (800 + 500 + 100) * 3, else: 0),
             "earlySubmissionXp" => &1.config.early_submission_xp,
             "hasVotingFeatures" => &1.has_voting_features,
             "hasTokenCounter" => &1.has_token_counter,
-            "isVotingPublished" => Assessments.is_voting_published(&1.id)
+            "isVotingPublished" => false
           }
         )
 
@@ -145,7 +145,7 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
             "earlySubmissionXp" => &1.config.early_submission_xp,
             "hasVotingFeatures" => &1.has_voting_features,
             "hasTokenCounter" => &1.has_token_counter,
-            "isVotingPublished" => Assessments.is_voting_published(&1.id)
+            "isVotingPublished" => false
           }
         )
 

test/cadet_web/controllers/assessments_controller_test.exs

@@ -83,7 +83,7 @@ defmodule CadetWeb.AssessmentsControllerTest do
               "earlySubmissionXp" => &1.config.early_submission_xp,
               "hasVotingFeatures" => &1.has_voting_features,
               "hasTokenCounter" => &1.has_token_counter,
-              "isVotingPublished" => Assessments.is_voting_published(&1.id),
+              "isVotingPublished" => false,
               "hoursBeforeEarlyXpDecay" => &1.config.hours_before_early_xp_decay
             }
           )
@@ -174,7 +174,7 @@ defmodule CadetWeb.AssessmentsControllerTest do
             "earlySubmissionXp" => &1.config.early_submission_xp,
             "hasVotingFeatures" => &1.has_voting_features,
             "hasTokenCounter" => &1.has_token_counter,
-            "isVotingPublished" => Assessments.is_voting_published(&1.id),
+            "isVotingPublished" => false,
             "hoursBeforeEarlyXpDecay" => &1.config.hours_before_early_xp_decay
           }
         )
@@ -288,7 +288,7 @@ defmodule CadetWeb.AssessmentsControllerTest do
               "questionCount" => 9,
               "hasVotingFeatures" => &1.has_voting_features,
               "hasTokenCounter" => &1.has_token_counter,
-              "isVotingPublished" => Assessments.is_voting_published(&1.id),
+              "isVotingPublished" => false,
               "earlySubmissionXp" => &1.config.early_submission_xp,
               "isGradingPublished" => nil,
               "hoursBeforeEarlyXpDecay" => &1.config.hours_before_early_xp_decay,