From 2adcd51afd2668dd944235189059db47c6d2c23c Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Mon, 14 Oct 2024 17:17:21 +0800 Subject: [PATCH] fix: Auto submission cron job for team assessments bug (#1200) * Refactor helper function * Remove unused aliases * Refactor helper further * Account for team assessments in autoclose job * Add more extensive tests * Fix format * Fix credo * Fix compile error * Fix empty submission creation for team assessments * refactor: Negate if condition --- lib/cadet/accounts/teams.ex | 4 +- lib/cadet/assessments/assessments.ex | 40 ++++---- lib/cadet/jobs/autograder/grading_job.ex | 52 ++++++++-- lib/cadet/jobs/autograder/utilities.ex | 36 ++++++- test/cadet/jobs/autograder/utilities_test.exs | 96 +++++++++++++++++-- 5 files changed, 186 insertions(+), 42 deletions(-) diff --git a/lib/cadet/accounts/teams.ex b/lib/cadet/accounts/teams.ex index 9cfecbfef..131b9ca24 100644 --- a/lib/cadet/accounts/teams.ex +++ b/lib/cadet/accounts/teams.ex @@ -9,8 +9,8 @@ defmodule Cadet.Accounts.Teams do import Ecto.{Changeset, Query} alias Cadet.Repo - alias Cadet.Accounts.{Team, TeamMember, CourseRegistration, Notification} - alias Cadet.Assessments.{Answer, Assessment, Submission} + alias Cadet.Accounts.{Team, TeamMember, Notification} + alias Cadet.Assessments.{Answer, Submission} @doc """ Creates a new team and assigns an assessment and team members to it. diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index d6ca6b362..244ebc742 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -966,7 +966,17 @@ defmodule Cadet.Assessments do end end - defp find_teams(cr_id) do + def is_team_assessment?(assessment_id) when is_ecto_id(assessment_id) do + max_team_size = + Assessment + |> where(id: ^assessment_id) + |> select([a], a.max_team_size) + |> Repo.one() + + max_team_size > 1 + end + + defp find_teams(cr_id) when is_ecto_id(cr_id) do query = from(t in Team, join: tm in assoc(t, :team_members), @@ -986,28 +996,14 @@ defmodule Cadet.Assessments do limit: 1 ) - assessment_team_size = - Map.get( - Repo.one( - from(a in Assessment, - where: a.id == ^assessment_id, - select: %{max_team_size: a.max_team_size} - ) - ), - :max_team_size, - 0 - ) - - case assessment_team_size > 1 do - true -> - case Repo.one(query) do - nil -> {:error, :team_not_found} - team -> {:ok, team} - end - + if is_team_assessment?(assessment_id) do + case Repo.one(query) do + nil -> {:error, :team_not_found} + team -> {:ok, team} + end + else # team is nil for individual assessments - false -> - {:ok, nil} + {:ok, nil} end end diff --git a/lib/cadet/jobs/autograder/grading_job.ex b/lib/cadet/jobs/autograder/grading_job.ex index 4fa8b2924..618b4c885 100644 --- a/lib/cadet/jobs/autograder/grading_job.ex +++ b/lib/cadet/jobs/autograder/grading_job.ex @@ -9,6 +9,7 @@ defmodule Cadet.Autograder.GradingJob do require Logger + alias Cadet.Accounts.{Team, TeamMember} alias Cadet.Assessments alias Cadet.Assessments.{Answer, Assessment, Question, Submission, SubmissionVotes} alias Cadet.Autograder.Utilities @@ -102,13 +103,50 @@ defmodule Cadet.Autograder.GradingJob do end defp insert_empty_submission(%{student_id: student_id, assessment: assessment}) do - %Submission{} - |> Submission.changeset(%{ - student_id: student_id, - assessment: assessment, - status: :submitted - }) - |> Repo.insert!() + if Assessments.is_team_assessment?(assessment.id) do + # Get current team if any + team = + Team + |> where(assessment_id: ^assessment.id) + |> join(:inner, [t], tm in assoc(t, :team_members)) + |> where([_, tm], tm.student_id == ^student_id) + |> Repo.one() + + if !team do + # Student is not in any team + # Create new team just for the student + team = + %Team{} + |> Team.changeset(%{ + assessment_id: assessment.id + }) + |> Repo.insert!() + + %TeamMember{} + |> TeamMember.changeset(%{ + team_id: team.id, + student_id: student_id + }) + |> Repo.insert!() + end + + %Submission{} + |> Submission.changeset(%{ + team_id: team.id, + assessment: assessment, + status: :submitted + }) + |> Repo.insert!() + else + # Individual assessment + %Submission{} + |> Submission.changeset(%{ + student_id: student_id, + assessment: assessment, + status: :submitted + }) + |> Repo.insert!() + end end defp update_submission_status_to_submitted(submission = %Submission{status: status}) do diff --git a/lib/cadet/jobs/autograder/utilities.ex b/lib/cadet/jobs/autograder/utilities.ex index 32208e0cf..e2a1d35fe 100644 --- a/lib/cadet/jobs/autograder/utilities.ex +++ b/lib/cadet/jobs/autograder/utilities.ex @@ -8,7 +8,9 @@ defmodule Cadet.Autograder.Utilities do import Ecto.Query - alias Cadet.Accounts.CourseRegistration + alias Cadet.Accounts.{CourseRegistration, TeamMember} + + alias Cadet.Assessments alias Cadet.Assessments.{Answer, Assessment, Question, Submission} def dispatch_programming_answer(answer = %Answer{}, question = %Question{}, overwrite \\ false) do @@ -25,7 +27,37 @@ defmodule Cadet.Autograder.Utilities do }) end - def fetch_submissions(assessment_id, course_id) when is_ecto_id(assessment_id) do + def fetch_submissions(assessment_id, course_id) + when is_ecto_id(assessment_id) and is_ecto_id(course_id) do + if Assessments.is_team_assessment?(assessment_id) do + fetch_team_submissions(assessment_id, course_id) + else + fetch_student_submissions(assessment_id, course_id) + end + end + + defp fetch_team_submissions(assessment_id, course_id) + when is_ecto_id(assessment_id) and is_ecto_id(course_id) do + CourseRegistration + |> where(role: "student", course_id: ^course_id) + |> join( + :left, + [cr], + tm in TeamMember, + on: cr.id == tm.student_id + ) + |> join( + :left, + [cr, tm], + s in Submission, + on: tm.team_id == s.team_id and s.assessment_id == ^assessment_id + ) + |> select([cr, tm, s], %{student_id: cr.id, submission: s}) + |> Repo.all() + end + + defp fetch_student_submissions(assessment_id, course_id) + when is_ecto_id(assessment_id) and is_ecto_id(course_id) do CourseRegistration |> where(role: "student", course_id: ^course_id) |> join( diff --git a/test/cadet/jobs/autograder/utilities_test.exs b/test/cadet/jobs/autograder/utilities_test.exs index c4266caf7..aedc584d6 100644 --- a/test/cadet/jobs/autograder/utilities_test.exs +++ b/test/cadet/jobs/autograder/utilities_test.exs @@ -72,18 +72,58 @@ defmodule Cadet.Autograder.UtilitiesTest do setup do course = insert(:course) config = insert(:assessment_config, %{course: course}) - assessment = insert(:assessment, %{is_published: true, course: course, config: config}) + + # Individual assessment + assessment = + insert(:assessment, %{ + is_published: true, + course: course, + config: config, + max_team_size: 1 + }) + students = insert_list(5, :course_registration, %{role: :student, course: course}) insert(:course_registration, %{course: build(:course), role: :student}) - %{students: students, assessment: assessment} + + # Team assessment + team_assessment = + insert(:assessment, %{ + is_published: true, + course: course, + config: config, + max_team_size: 2 + }) + + team_member1 = insert(:team_member, %{student: Enum.at(students, 0)}) + team_member2 = insert(:team_member, %{student: Enum.at(students, 1)}) + + team1 = + insert(:team, %{assessment: team_assessment, team_members: [team_member1, team_member2]}) + + team_member3 = insert(:team_member, %{student: Enum.at(students, 2)}) + team_member4 = insert(:team_member, %{student: Enum.at(students, 3)}) + + team2 = + insert(:team, %{assessment: team_assessment, team_members: [team_member3, team_member4]}) + + %{ + individual: %{students: students, assessment: assessment}, + team: %{ + teams: [team1, team2], + assessment: team_assessment, + teamless: [Enum.at(students, 4)] + } + } end - test "it returns list of students with matching submissions", %{ - students: students, - assessment: assessment + test "it returns list of students with matching submissions for individual assessments", %{ + individual: %{students: students, assessment: assessment} } do submissions = - Enum.map(students, &insert(:submission, %{assessment: assessment, student: &1})) + Enum.map( + students, + &insert(:submission, %{assessment: assessment, student: &1, team: nil}) + ) expected = Enum.map(submissions, &%{student_id: &1.student_id, submission_id: &1.id}) @@ -95,9 +135,8 @@ defmodule Cadet.Autograder.UtilitiesTest do assert results == expected end - test "it returns list of students with without matching submissions", %{ - students: students, - assessment: assessment + test "it returns list of students without matching submissions for individual assessments", %{ + individual: %{students: students, assessment: assessment} } do expected_student_ids = Enum.map(students, & &1.id) @@ -108,6 +147,45 @@ defmodule Cadet.Autograder.UtilitiesTest do assert results |> Enum.map(& &1.submission) |> Enum.uniq() == [nil] end + + test "it returns list of students both with and without matching submissions for team assessments", + %{ + team: %{teams: teams, assessment: assessment, teamless: teamless} + } do + submissions = + Enum.map( + teams, + &%{ + team_id: &1.id, + submission: insert(:submission, %{assessment: assessment, team: &1, student: nil}) + } + ) + + expected = + teams + |> Enum.flat_map(& &1.team_members) + |> Enum.map( + &%{ + student_id: &1.student_id, + submission_id: + Enum.find(submissions, fn s -> s.team_id == &1.team_id end).submission.id + } + ) + + expected = expected ++ Enum.map(teamless, &%{student_id: &1.id, submission_id: nil}) + + results = + assessment.id + |> Utilities.fetch_submissions(assessment.course_id) + |> Enum.map( + &%{ + student_id: &1.student_id, + submission_id: if(&1.submission, do: &1.submission.id, else: nil) + } + ) + + assert results == expected + end end defp get_assessments_ids(assessments) do