From 6af2876ba712b6d55ffc403a82ae99a447b723d8 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Fri, 20 Sep 2024 20:53:43 +0800 Subject: [PATCH 01/10] Use subquery to get submission IDs --- lib/cadet/assessments/assessments.ex | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 55d0dff4d..8dc0be209 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/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,13 +382,10 @@ 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 - ) - - Repo.all(query) + from(s in Submission, + where: s.student_id == ^cr_id or s.team_id in ^Enum.map(teams, & &1.id), + select: s.id + ) end @doc """ From d67d93f9fe29fe4ef9349694cea1028e25232675 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Fri, 20 Sep 2024 20:53:55 +0800 Subject: [PATCH 02/10] Add TODO --- lib/cadet/assessments/assessments.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 8dc0be209..27230aec5 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1439,6 +1439,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 From 079807e362123f63e6d2dc301307e71f7d82f03a Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Fri, 20 Sep 2024 21:39:31 +0800 Subject: [PATCH 03/10] Fix n+1 query problem --- lib/cadet/assessments/assessments.ex | 28 +++++++++++++++++++ .../admin_views/admin_assessments_view.ex | 18 +----------- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 27230aec5..0a49e6f10 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -388,12 +388,40 @@ defmodule Cadet.Assessments do ) end + 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() + |> 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 diff --git a/lib/cadet_web/admin_views/admin_assessments_view.ex b/lib/cadet_web/admin_views/admin_assessments_view.ex index 159c5b848..33af48629 100644 --- a/lib/cadet_web/admin_views/admin_assessments_view.ex +++ b/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 From f8187e32fd4835ee578556332c9ad321d9670120 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Fri, 20 Sep 2024 22:31:14 +0800 Subject: [PATCH 04/10] Fix typo --- lib/cadet/assessments/assessments.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 0a49e6f10..b53b8b62e 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -401,7 +401,7 @@ defmodule Cadet.Assessments do |> where([q], q.id in ^voting_assigned_question_ids) |> where([q], q.assessment_id in ^assessment_ids) |> select([q], q.assessment_id) - |> distinct() + |> distinct(true) |> Repo.all() Enum.reduce(assessment_ids, %{}, fn id, acc -> From bd430a8c7c433fc30ca2efd7dd96a94864d91256 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Fri, 20 Sep 2024 23:01:27 +0800 Subject: [PATCH 05/10] Fix tests --- lib/cadet/assessments/assessments.ex | 7 +++---- .../admin_assessments_controller_test.exs | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index b53b8b62e..5af40d9c2 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -678,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?() == true end def update_final_contest_entries do diff --git a/test/cadet_web/admin_controllers/admin_assessments_controller_test.exs b/test/cadet_web/admin_controllers/admin_assessments_controller_test.exs index 0b0dc1483..2f32cc172 100644 --- a/test/cadet_web/admin_controllers/admin_assessments_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_assessments_controller_test.exs @@ -94,7 +94,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 } ) @@ -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 } ) From c0ae608a732be1dd5c64231fab277fe97fcc76e1 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Fri, 20 Sep 2024 23:06:20 +0800 Subject: [PATCH 06/10] Comment Sentry DSN in example secrets --- config/dev.secrets.exs.example | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/dev.secrets.exs.example b/config/dev.secrets.exs.example index 088f27727..42a87b674 100644 --- a/config/dev.secrets.exs.example +++ b/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 From 45b47d412afc5ef4ba4e1ba474daf165b399a2b5 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Fri, 20 Sep 2024 23:39:24 +0800 Subject: [PATCH 07/10] Fix admin assessments controller --- lib/cadet/assessments/assessments.ex | 2 +- .../admin_controllers/admin_assessments_controller.ex | 2 +- .../admin_controllers/admin_assessments_controller_test.exs | 2 +- test/cadet_web/controllers/assessments_controller_test.exs | 6 +++--- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 5af40d9c2..8e5956f8c 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -687,7 +687,7 @@ defmodule Cadet.Assessments do |> where(type: :voting) |> where(assessment_id: ^assessment_id) |> where([q], q.id in subquery(voting_assigned_question_ids)) - |> Repo.exists?() == true + |> Repo.exists?() || false end def update_final_contest_entries do diff --git a/lib/cadet_web/admin_controllers/admin_assessments_controller.ex b/lib/cadet_web/admin_controllers/admin_assessments_controller.ex index d9a992267..9263195f2 100644 --- a/lib/cadet_web/admin_controllers/admin_assessments_controller.ex +++ b/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 diff --git a/test/cadet_web/admin_controllers/admin_assessments_controller_test.exs b/test/cadet_web/admin_controllers/admin_assessments_controller_test.exs index 2f32cc172..4d2a7601b 100644 --- a/test/cadet_web/admin_controllers/admin_assessments_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_assessments_controller_test.exs @@ -90,7 +90,7 @@ 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, diff --git a/test/cadet_web/controllers/assessments_controller_test.exs b/test/cadet_web/controllers/assessments_controller_test.exs index f1cc9beb0..d17eee839 100644 --- a/test/cadet_web/controllers/assessments_controller_test.exs +++ b/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, From 5d5b90253841e57b462463a527ecaba0fff46f48 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 20 Sep 2024 23:34:29 +0800 Subject: [PATCH 08/10] fix: Fix n + 1 query in assessments_view --- lib/cadet_web/views/assessments_view.ex | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/lib/cadet_web/views/assessments_view.ex b/lib/cadet_web/views/assessments_view.ex index 700f13019..4c8fb3641 100644 --- a/lib/cadet_web/views/assessments_view.ex +++ b/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 @@ -88,16 +85,4 @@ defmodule CadetWeb.AssessmentsView do 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 From d548f71bdb496e4336467d9c60b796beeadc0516 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Fri, 20 Sep 2024 23:44:04 +0800 Subject: [PATCH 09/10] Fix format --- .../admin_controllers/admin_assessments_controller_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cadet_web/admin_controllers/admin_assessments_controller_test.exs b/test/cadet_web/admin_controllers/admin_assessments_controller_test.exs index 4d2a7601b..10e9c3331 100644 --- a/test/cadet_web/admin_controllers/admin_assessments_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_assessments_controller_test.exs @@ -90,7 +90,7 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do "isPublished" => &1.is_published, "gradedCount" => 0, "questionCount" => 9, - "xp" => (if &1.is_grading_published, do: (800 + 500 + 100) * 3, else: 0), + "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, From 35d3fd245331a808845264c7f345cdafa97d7671 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Fri, 20 Sep 2024 23:45:22 +0800 Subject: [PATCH 10/10] Fix format --- lib/cadet_web/views/assessments_view.ex | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/cadet_web/views/assessments_view.ex b/lib/cadet_web/views/assessments_view.ex index 4c8fb3641..a00c61aa3 100644 --- a/lib/cadet_web/views/assessments_view.ex +++ b/lib/cadet_web/views/assessments_view.ex @@ -84,5 +84,4 @@ defmodule CadetWeb.AssessmentsView do defp password_protected?(nil), do: false defp password_protected?(_), do: true - end