From b2422a41b04330d33c02e5b446172d8ea33f81c2 Mon Sep 17 00:00:00 2001 From: kjw142857 <122250318+kjw142857@users.noreply.github.com> Date: Wed, 20 Sep 2023 10:27:07 +0800 Subject: [PATCH] Prevent deletion of contests when contest voting file is present (#1002) --- lib/cadet/assessments/assessments.ex | 61 +++++++++++---- test/cadet/assessments/assessments_test.exs | 82 +++++++++++++++++++++ 2 files changed, 127 insertions(+), 16 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 57d622ef3..a543f819e 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -36,18 +36,33 @@ defmodule Cadet.Assessments do def delete_assessment(id) do assessment = Repo.get(Assessment, id) - Submission - |> where(assessment_id: ^id) - |> delete_submission_assocation(id) + is_voted_on = + Question + |> where(type: :voting) + |> join(:inner, [q], asst in assoc(q, :assessment)) + |> where( + [q, asst], + q.question["contest_number"] == ^assessment.number and + asst.course_id == ^assessment.course_id + ) + |> Repo.exists?() - Question - |> where(assessment_id: ^id) - |> Repo.all() - |> Enum.each(fn q -> - delete_submission_votes_association(q) - end) + if is_voted_on do + {:error, {:bad_request, "Contest voting for this contest is still up"}} + else + Submission + |> where(assessment_id: ^id) + |> delete_submission_assocation(id) - Repo.delete(assessment) + Question + |> where(assessment_id: ^id) + |> Repo.all() + |> Enum.each(fn q -> + delete_submission_votes_association(q) + end) + + Repo.delete(assessment) + end end defp delete_submission_votes_association(question) do @@ -522,12 +537,26 @@ defmodule Cadet.Assessments do |> select([v], v.question_id) |> Repo.all() - Question - |> where(type: :voting) - |> where([q], q.id not in ^voting_assigned_question_ids) - |> join(:inner, [q], asst in assoc(q, :assessment)) - |> select([q, asst], %{course_id: asst.course_id, question: q.question, question_id: q.id}) - |> Repo.all() + valid_assessments = + Assessment + |> select([a], %{number: a.number, course_id: a.course_id}) + |> Repo.all() + + valid_questions = + Question + |> where(type: :voting) + |> where([q], q.id not in ^voting_assigned_question_ids) + |> join(:inner, [q], asst in assoc(q, :assessment)) + |> select([q, asst], %{course_id: asst.course_id, question: q.question, question_id: q.id}) + |> Repo.all() + + # fetch only voting where there is a corresponding contest + Enum.filter(valid_questions, fn q -> + Enum.any?( + valid_assessments, + fn a -> a.number == q.question["contest_number"] and a.course_id == q.course_id end + ) + end) end @doc """ diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index 70fe132cd..61800c289 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -311,6 +311,8 @@ defmodule Cadet.AssessmentsTest do closed_voting_assessment = insert(:assessment, %{course: course}) open_voting_assessment = insert(:assessment, %{course: course}) compiled_voting_assessment = insert(:assessment, %{course: course}) + # voting assessment that references an invalid contest number + invalid_voting_assessment = insert(:assessment, %{course: course}) closed_question = insert(:voting_question, %{ @@ -333,6 +335,12 @@ defmodule Cadet.AssessmentsTest do build(:voting_question_content, contest_number: compiled_contest_assessment.number) }) + invalid_question = + insert(:voting_question, %{ + assessment: invalid_voting_assessment, + question: build(:voting_question_content, contest_number: "test_invalid") + }) + students = insert_list(10, :course_registration, %{ role: :student, @@ -375,6 +383,9 @@ defmodule Cadet.AssessmentsTest do |> Repo.all() |> length() == 4 * 3 + 6 * 4 + assert SubmissionVotes |> where(question_id: ^invalid_question.id) |> Repo.all() |> length() == + 0 + Enum.map(students, fn student -> submission = insert(:submission, @@ -420,6 +431,15 @@ defmodule Cadet.AssessmentsTest do ) end) + # fetching all unassigned voting questions should only yield open and closed questions + unassigned_voting_questions = Assessments.fetch_unassigned_voting_questions() + assert Enum.count(unassigned_voting_questions) == 2 + + assert Enum.map(unassigned_voting_questions, fn q -> q.question_id end) == [ + closed_question.id, + open_question.id + ] + Assessments.update_final_contest_entries() # only the closed_contest should have been updated @@ -433,6 +453,9 @@ defmodule Cadet.AssessmentsTest do |> where(question_id: ^compiled_question.id) |> Repo.all() |> length() == 4 * 3 + 6 * 4 + + assert SubmissionVotes |> where(question_id: ^invalid_question.id) |> Repo.all() |> length() == + 0 end test "create voting parameters with invalid contest number" do @@ -493,6 +516,65 @@ defmodule Cadet.AssessmentsTest do Assessments.delete_assessment(voting_assessment.id) refute Repo.exists?(SubmissionVotes, question_id: question.id) end + + test "does not delete contest assessment if referencing voting assessment is present" do + course = insert(:course) + config = insert(:assessment_config) + + contest_assessment = + insert(:assessment, + is_published: true, + open_at: Timex.shift(Timex.now(), days: -5), + close_at: Timex.shift(Timex.now(), hours: -1), + course: course, + config: config, + number: "test" + ) + + voting_assessment = insert(:assessment, %{course: course, config: config}) + + # insert voting question that references the contest assessment + _voting_question = + insert(:voting_question, %{ + assessment: voting_assessment, + question: build(:voting_question_content, contest_number: contest_assessment.number) + }) + + error_message = {:bad_request, "Contest voting for this contest is still up"} + + assert {:error, ^error_message} = Assessments.delete_assessment(contest_assessment.id) + # deletion should fail + assert Assessment |> where(id: ^contest_assessment.id) |> Repo.exists?() + end + + test "deletes contest assessment if voting assessment references same number but different course" do + course_1 = insert(:course) + course_2 = insert(:course) + config = insert(:assessment_config) + + contest_assessment = + insert(:assessment, + is_published: true, + open_at: Timex.shift(Timex.now(), days: -5), + close_at: Timex.shift(Timex.now(), hours: -1), + course: course_1, + config: config, + number: "test" + ) + + voting_assessment = insert(:assessment, %{course: course_2, config: config}) + + # insert voting question from a different course that references the same contest number + _voting_question = + insert(:voting_question, %{ + assessment: voting_assessment, + question: build(:voting_question_content, contest_number: contest_assessment.number) + }) + + assert {:ok, _} = Assessments.delete_assessment(contest_assessment.id) + # deletion should succeed + refute Assessment |> where(id: ^contest_assessment.id) |> Repo.exists?() + end end describe "contest voting leaderboard utility functions" do