fix: Auto submission cron job for team assessments bug (#1200)

RichDom2185 · web-flow · commit 2adcd51afd26 · 2024-10-14T17:17:21.000+08:00
* 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
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
