From fc1d6940cbb9a1e1a743fb444c9cbadf00b3458c Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Mon, 14 Oct 2024 01:38:06 +0800 Subject: [PATCH 01/10] Refactor helper function --- lib/cadet/assessments/assessments.ex | 37 ++++++++++++---------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index d6ca6b362..7084d9642 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -966,6 +966,15 @@ defmodule Cadet.Assessments do end end + def get_team_size(assessment_id) when is_ecto_id(assessment_id) do + from(a in Assessment, + where: a.id == ^assessment_id, + select: %{max_team_size: a.max_team_size} + ) + |> Repo.one() + |> Map.get(:max_team_size, 0) + end + defp find_teams(cr_id) do query = from(t in Team, @@ -986,28 +995,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 get_team_size(assessment_id) > 1 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 From c06b4002480b5cd08c4fecc86e2649207b05f086 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Mon, 14 Oct 2024 01:41:49 +0800 Subject: [PATCH 02/10] Remove unused aliases --- lib/cadet/accounts/teams.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 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. From e19dcec6042ffc29763c0d6c1db3c6972bfdd65e Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Mon, 14 Oct 2024 01:47:04 +0800 Subject: [PATCH 03/10] Refactor helper further --- lib/cadet/assessments/assessments.ex | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 7084d9642..327075744 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -966,16 +966,18 @@ defmodule Cadet.Assessments do end end - def get_team_size(assessment_id) when is_ecto_id(assessment_id) do - from(a in Assessment, - where: a.id == ^assessment_id, - select: %{max_team_size: a.max_team_size} - ) - |> Repo.one() - |> Map.get(:max_team_size, 0) + def is_team_assessment?(assessment_id) when is_ecto_id(assessment_id) do + max_team_size = + from(a in Assessment, + where: a.id == ^assessment_id, + select: a.max_team_size + ) + |> Repo.one() + + max_team_size > 1 end - defp find_teams(cr_id) do + defp find_teams(cr_id) when is_ecto_id(cr_id) do query = from(t in Team, join: tm in assoc(t, :team_members), @@ -995,7 +997,7 @@ defmodule Cadet.Assessments do limit: 1 ) - if get_team_size(assessment_id) > 1 do + if is_team_assessment?(assessment_id) do case Repo.one(query) do nil -> {:error, :team_not_found} team -> {:ok, team} From 5bdd7c54ed02c52391712a90579c56505d87f2af Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Mon, 14 Oct 2024 01:56:32 +0800 Subject: [PATCH 04/10] Account for team assessments in autoclose job --- lib/cadet/jobs/autograder/utilities.ex | 36 ++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) 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( From ca07021db3f509fac2f925b1587806cb99a1b404 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Mon, 14 Oct 2024 02:44:43 +0800 Subject: [PATCH 05/10] Add more extensive tests --- test/cadet/jobs/autograder/utilities_test.exs | 76 ++++++++++++++++--- 1 file changed, 67 insertions(+), 9 deletions(-) diff --git a/test/cadet/jobs/autograder/utilities_test.exs b/test/cadet/jobs/autograder/utilities_test.exs index c4266caf7..7533523c3 100644 --- a/test/cadet/jobs/autograder/utilities_test.exs +++ b/test/cadet/jobs/autograder/utilities_test.exs @@ -72,18 +72,54 @@ 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 +131,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 +143,29 @@ 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 = Enum.flat_map(teams, & &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 From f4f6ee5265c13731815c2d5bbec8b39eb6255a37 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Mon, 14 Oct 2024 03:01:27 +0800 Subject: [PATCH 06/10] Fix format --- test/cadet/jobs/autograder/utilities_test.exs | 45 +++++++++++++------ 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/test/cadet/jobs/autograder/utilities_test.exs b/test/cadet/jobs/autograder/utilities_test.exs index 7533523c3..7e9ac47f4 100644 --- a/test/cadet/jobs/autograder/utilities_test.exs +++ b/test/cadet/jobs/autograder/utilities_test.exs @@ -108,7 +108,11 @@ defmodule Cadet.Autograder.UtilitiesTest do %{ individual: %{students: students, assessment: assessment}, - team: %{teams: [team1, team2], assessment: team_assessment, teamless: [Enum.at(students, 4)]} + team: %{ + teams: [team1, team2], + assessment: team_assessment, + teamless: [Enum.at(students, 4)] + } } end @@ -144,25 +148,40 @@ 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 + 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}) - }) + Enum.map( + teams, + &%{ + team_id: &1.id, + submission: insert(:submission, %{assessment: assessment, team: &1, student: nil}) + } + ) + + expected = + Enum.flat_map(teams, & &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 = Enum.flat_map(teams, & &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)}) + |> Enum.map( + &%{ + student_id: &1.student_id, + submission_id: if(&1.submission, do: &1.submission.id, else: nil) + } + ) assert results == expected end From 64d24c22188996c420c7965f30660e57d7687542 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Mon, 14 Oct 2024 03:08:27 +0800 Subject: [PATCH 07/10] Fix credo --- lib/cadet/assessments/assessments.ex | 7 +++---- test/cadet/jobs/autograder/utilities_test.exs | 3 ++- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 327075744..f7d9a636b 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -968,10 +968,9 @@ defmodule Cadet.Assessments do def is_team_assessment?(assessment_id) when is_ecto_id(assessment_id) do max_team_size = - from(a in Assessment, - where: a.id == ^assessment_id, - select: a.max_team_size - ) + Assessment + |> where(id: ^assessment_id) + |> select(:max_team_size) |> Repo.one() max_team_size > 1 diff --git a/test/cadet/jobs/autograder/utilities_test.exs b/test/cadet/jobs/autograder/utilities_test.exs index 7e9ac47f4..aedc584d6 100644 --- a/test/cadet/jobs/autograder/utilities_test.exs +++ b/test/cadet/jobs/autograder/utilities_test.exs @@ -162,7 +162,8 @@ defmodule Cadet.Autograder.UtilitiesTest do ) expected = - Enum.flat_map(teams, & &1.team_members) + teams + |> Enum.flat_map(& &1.team_members) |> Enum.map( &%{ student_id: &1.student_id, From 01b92b6a5efe384ea1c85371808802034e3cd087 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Mon, 14 Oct 2024 03:09:11 +0800 Subject: [PATCH 08/10] Fix compile error --- 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 f7d9a636b..244ebc742 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -970,7 +970,7 @@ defmodule Cadet.Assessments do max_team_size = Assessment |> where(id: ^assessment_id) - |> select(:max_team_size) + |> select([a], a.max_team_size) |> Repo.one() max_team_size > 1 From fc64d3fd75cde19f15f706bc07a4ca3f756fc8d9 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Mon, 14 Oct 2024 16:36:15 +0800 Subject: [PATCH 09/10] Fix empty submission creation for team assessments --- lib/cadet/jobs/autograder/grading_job.ex | 52 ++++++++++++++++++++---- 1 file changed, 45 insertions(+), 7 deletions(-) diff --git a/lib/cadet/jobs/autograder/grading_job.ex b/lib/cadet/jobs/autograder/grading_job.ex index 4fa8b2924..13cdf8d88 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 + # Individual assessment + %Submission{} + |> Submission.changeset(%{ + student_id: student_id, + assessment: assessment, + status: :submitted + }) + |> Repo.insert!() + else + # Team assessment, 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!() + end end defp update_submission_status_to_submitted(submission = %Submission{status: status}) do From cecba451539c8a76623d51c5257cf900c3acd6c0 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Mon, 14 Oct 2024 16:38:01 +0800 Subject: [PATCH 10/10] refactor: Negate if condition --- lib/cadet/jobs/autograder/grading_job.ex | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/cadet/jobs/autograder/grading_job.ex b/lib/cadet/jobs/autograder/grading_job.ex index 13cdf8d88..618b4c885 100644 --- a/lib/cadet/jobs/autograder/grading_job.ex +++ b/lib/cadet/jobs/autograder/grading_job.ex @@ -103,17 +103,8 @@ defmodule Cadet.Autograder.GradingJob do end defp insert_empty_submission(%{student_id: student_id, assessment: assessment}) do - if !Assessments.is_team_assessment?(assessment.id) do - # Individual assessment - %Submission{} - |> Submission.changeset(%{ - student_id: student_id, - assessment: assessment, - status: :submitted - }) - |> Repo.insert!() - else - # Team assessment, get current team if any + if Assessments.is_team_assessment?(assessment.id) do + # Get current team if any team = Team |> where(assessment_id: ^assessment.id) @@ -146,6 +137,15 @@ defmodule Cadet.Autograder.GradingJob do status: :submitted }) |> Repo.insert!() + else + # Individual assessment + %Submission{} + |> Submission.changeset(%{ + student_id: student_id, + assessment: assessment, + status: :submitted + }) + |> Repo.insert!() end end