diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 55b413fe5..3e88fac0a 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1230,92 +1230,229 @@ defmodule Cadet.Assessments do @doc """ Function returning submissions under a grader. This function returns only the - fields that are exposed in the /grading endpoint. The reason we select only - those fields is to reduce the memory usage especially when the number of - submissions is large i.e. > 25000 submissions. + fields that are exposed in the /grading endpoint. - The input parameters are the user and group_only. group_only is used to check - whether only the groups under the grader should be returned. The parameter is - a boolean which is false by default. + The input parameters are the user and query parameters. Query parameters are + used to filter the submissions. - The return value is {:ok, submissions} if no errors, else it is {:error, - {:forbidden, "Forbidden."}} + The group parameter is used to check whether only the groups under the grader + should be returned. If pageSize and offset are not provided, the default + values are 10 and 0 respectively. + + The return value is + {:ok, %{"count": count, "data": submissions}} if no errors, + else it is {:error, {:forbidden, "Forbidden."}} """ - @spec all_submissions_by_grader_for_index(CourseRegistration.t()) :: - {:ok, %{:assessments => [any()], :submissions => [any()], :users => [any()]}} - def all_submissions_by_grader_for_index( + + # We bypass Ecto here and use a raw query to generate JSON directly from + # PostgreSQL, because doing it in Elixir/Erlang is too inefficient. + @spec submissions_by_grader_for_index(CourseRegistration.t()) :: + {:ok, + %{ + :count => integer, + :data => %{:assessments => [any()], :submissions => [any()], :users => [any()]} + }} + def submissions_by_grader_for_index( grader = %CourseRegistration{course_id: course_id}, - group_only \\ false, - _ungraded_only \\ false + params \\ %{ + "group" => "false", + "notFullyGraded" => "true", + "pageSize" => "10", + "offset" => "0" + } ) do - show_all = not group_only - - group_filter = - if show_all, - do: "", - else: - "AND s.student_id IN (SELECT cr.id FROM course_registrations AS cr INNER JOIN groups AS g ON cr.group_id = g.id WHERE g.leader_id = #{grader.id}) OR s.student_id = #{grader.id}" - - # TODO: Restore ungraded filtering - # ... or more likely, decouple email logic from this function - # ungraded_where = - # if ungraded_only, - # do: "where s.\"gradedCount\" < assts.\"questionCount\"", - # else: "" - - # We bypass Ecto here and use a raw query to generate JSON directly from - # PostgreSQL, because doing it in Elixir/Erlang is too inefficient. - - submissions = - case Repo.query(""" - SELECT - s.id, - s.status, - s.unsubmitted_at, - s.unsubmitted_by_id, - s_ans.xp, - s_ans.xp_adjustment, - s.xp_bonus, - s_ans.graded_count, - s.student_id, - s.assessment_id - FROM - submissions AS s - LEFT JOIN ( - SELECT - ans.submission_id, - SUM(ans.xp) AS xp, - SUM(ans.xp_adjustment) AS xp_adjustment, - COUNT(ans.id) FILTER ( - WHERE - ans.grader_id IS NOT NULL - ) AS graded_count - FROM - answers AS ans - GROUP BY - ans.submission_id - ) AS s_ans ON s_ans.submission_id = s.id - WHERE - s.assessment_id IN ( - SELECT - id - FROM - assessments - WHERE - assessments.course_id = #{course_id} - ) #{group_filter}; - """) do - {:ok, %{columns: columns, rows: result}} -> - result - |> Enum.map( - &(columns - |> Enum.map(fn c -> String.to_atom(c) end) - |> Enum.zip(&1) - |> Enum.into(%{})) - ) - end + submission_answers_query = + from(ans in Answer, + group_by: ans.submission_id, + select: %{ + submission_id: ans.submission_id, + xp: sum(ans.xp), + xp_adjustment: sum(ans.xp_adjustment), + graded_count: filter(count(ans.id), not is_nil(ans.grader_id)) + } + ) + + question_answers_query = + from(q in Question, + group_by: q.assessment_id, + join: a in Assessment, + on: q.assessment_id == a.id, + select: %{ + assessment_id: q.assessment_id, + question_count: count(q.id) + } + ) + + query = + from(s in Submission, + left_join: ans in subquery(submission_answers_query), + on: ans.submission_id == s.id, + as: :ans, + left_join: asst in subquery(question_answers_query), + on: asst.assessment_id == s.assessment_id, + as: :asst, + where: ^build_user_filter(params), + where: s.assessment_id in subquery(build_assessment_filter(params, course_id)), + where: s.assessment_id in subquery(build_assessment_config_filter(params)), + where: ^build_submission_filter(params), + where: ^build_course_registration_filter(params, grader), + order_by: [desc: s.inserted_at], + limit: ^elem(Integer.parse(Map.get(params, "pageSize", "10")), 0), + offset: ^elem(Integer.parse(Map.get(params, "offset", "0")), 0), + select: %{ + id: s.id, + status: s.status, + xp_bonus: s.xp_bonus, + unsubmitted_at: s.unsubmitted_at, + unsubmitted_by_id: s.unsubmitted_by_id, + student_id: s.student_id, + assessment_id: s.assessment_id, + xp: ans.xp, + xp_adjustment: ans.xp_adjustment, + graded_count: ans.graded_count, + question_count: asst.question_count + } + ) + + submissions = Repo.all(query) + + count_query = + from(s in Submission, + left_join: ans in subquery(submission_answers_query), + on: ans.submission_id == s.id, + as: :ans, + left_join: asst in subquery(question_answers_query), + on: asst.assessment_id == s.assessment_id, + as: :asst, + where: s.assessment_id in subquery(build_assessment_filter(params, course_id)), + where: s.assessment_id in subquery(build_assessment_config_filter(params)), + where: ^build_user_filter(params), + where: ^build_submission_filter(params), + where: ^build_course_registration_filter(params, grader), + select: count(s.id) + ) + + count = Repo.one(count_query) + + {:ok, %{count: count, data: generate_grading_summary_view_model(submissions, course_id)}} + end - {:ok, generate_grading_summary_view_model(submissions, course_id)} + defp build_assessment_filter(params, course_id) do + assessments_filters = + Enum.reduce(params, dynamic(true), fn + {"title", value}, dynamic -> + dynamic([assessment], ^dynamic and ilike(assessment.title, ^"%#{value}%")) + + {_, _}, dynamic -> + dynamic + end) + + from(a in Assessment, + where: a.course_id == ^course_id, + where: ^assessments_filters, + select: a.id + ) + end + + defp build_submission_filter(params) do + Enum.reduce(params, dynamic(true), fn + {"status", value}, dynamic -> + dynamic([submission], ^dynamic and submission.status == ^value) + + {"notFullyGraded", "true"}, dynamic -> + dynamic([ans: ans, asst: asst], ^dynamic and asst.question_count > ans.graded_count) + + {_, _}, dynamic -> + dynamic + end) + end + + defp build_course_registration_filter(params, grader) do + Enum.reduce(params, dynamic(true), fn + {"group", "true"}, dynamic -> + dynamic( + [submission], + (^dynamic and + submission.student_id in subquery( + from(cr in CourseRegistration, + join: g in Group, + on: cr.group_id == g.id, + where: g.leader_id == ^grader.id, + select: cr.id + ) + )) or submission.student_id == ^grader.id + ) + + {"groupName", value}, dynamic -> + dynamic( + [submission], + ^dynamic and + submission.student_id in subquery( + from(cr in CourseRegistration, + join: g in Group, + on: cr.group_id == g.id, + where: g.name == ^value, + select: cr.id + ) + ) + ) + + {_, _}, dynamic -> + dynamic + end) + end + + defp build_user_filter(params) do + Enum.reduce(params, dynamic(true), fn + {"name", value}, dynamic -> + dynamic( + [submission], + ^dynamic and + submission.student_id in subquery( + from(user in User, + where: ilike(user.name, ^"%#{value}%"), + select: user.id + ) + ) + ) + + {"username", value}, dynamic -> + dynamic( + [submission], + ^dynamic and + submission.student_id in subquery( + from(user in User, + where: ilike(user.username, ^"%#{value}%"), + select: user.id + ) + ) + ) + + {_, _}, dynamic -> + dynamic + end) + end + + defp build_assessment_config_filter(params) do + assessment_config_filters = + Enum.reduce(params, dynamic(true), fn + {"type", value}, dynamic -> + dynamic([assessment_config: config], ^dynamic and config.type == ^value) + + {"isManuallyGraded", value}, dynamic -> + dynamic([assessment_config: config], ^dynamic and config.is_manually_graded == ^value) + + {_, _}, dynamic -> + dynamic + end) + + from(a in Assessment, + inner_join: config in AssessmentConfig, + on: a.config_id == config.id, + as: :assessment_config, + where: ^assessment_config_filters, + select: a.id + ) end defp generate_grading_summary_view_model(submissions, course_id) do diff --git a/lib/cadet/workers/NotificationWorker.ex b/lib/cadet/workers/NotificationWorker.ex index 609aab13c..67913a033 100644 --- a/lib/cadet/workers/NotificationWorker.ex +++ b/lib/cadet/workers/NotificationWorker.ex @@ -73,8 +73,11 @@ defmodule Cadet.Workers.NotificationWorker do for avenger_cr <- avengers_crs do avenger = Cadet.Accounts.get_user(avenger_cr.user_id) - {:ok, %{submissions: ungraded_submissions}} = - Cadet.Assessments.all_submissions_by_grader_for_index(avenger_cr, true, true) + {:ok, %{data: %{submissions: ungraded_submissions}}} = + Cadet.Assessments.submissions_by_grader_for_index(avenger_cr, %{ + "group" => "true", + "notFullyGraded" => "true" + }) if Enum.count(ungraded_submissions) < ungraded_threshold do IO.puts("[AVENGER_BACKLOG] below threshold!") diff --git a/lib/cadet_web/admin_controllers/admin_grading_controller.ex b/lib/cadet_web/admin_controllers/admin_grading_controller.ex index 24b915371..b69c743f9 100644 --- a/lib/cadet_web/admin_controllers/admin_grading_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_grading_controller.ex @@ -4,12 +4,11 @@ defmodule CadetWeb.AdminGradingController do alias Cadet.Assessments - def index(conn, %{"group" => group}) when group in ["true", "false"] do + def index(conn, %{"group" => group} = params) + when group in ["true", "false"] do course_reg = conn.assigns[:course_reg] - group = String.to_atom(group) - - case Assessments.all_submissions_by_grader_for_index(course_reg, group) do + case Assessments.submissions_by_grader_for_index(course_reg, params) do {:ok, view_model} -> conn |> put_status(:ok) diff --git a/lib/cadet_web/admin_views/admin_grading_view.ex b/lib/cadet_web/admin_views/admin_grading_view.ex index c9418ef9a..e6a9089af 100644 --- a/lib/cadet_web/admin_views/admin_grading_view.ex +++ b/lib/cadet_web/admin_views/admin_grading_view.ex @@ -25,29 +25,36 @@ defmodule CadetWeb.AdminGradingView do end def render("gradingsummaries.json", %{ - users: users, - assessments: assessments, - submissions: submissions + count: count, + data: %{ + users: users, + assessments: assessments, + submissions: submissions + } }) do - for submission <- submissions do - user = users |> Enum.find(&(&1.id == submission.student_id)) - assessment = assessments |> Enum.find(&(&1.id == submission.assessment_id)) + %{ + count: count, + data: + for submission <- submissions do + user = users |> Enum.find(&(&1.id == submission.student_id)) + assessment = assessments |> Enum.find(&(&1.id == submission.assessment_id)) - render( - CadetWeb.AdminGradingView, - "gradingsummary.json", - %{ - user: user, - assessment: assessment, - submission: submission, - unsubmitter: - case submission.unsubmitted_by_id do - nil -> nil - _ -> users |> Enum.find(&(&1.id == submission.unsubmitted_by_id)) - end - } - ) - end + render( + CadetWeb.AdminGradingView, + "gradingsummary.json", + %{ + user: user, + assessment: assessment, + submission: submission, + unsubmitter: + case submission.unsubmitted_by_id do + nil -> nil + _ -> users |> Enum.find(&(&1.id == submission.unsubmitted_by_id)) + end + } + ) + end + } end def render("gradingsummary.json", %{ diff --git a/test/cadet/email_test.exs b/test/cadet/email_test.exs index 2ee826979..e7c1ec3e6 100644 --- a/test/cadet/email_test.exs +++ b/test/cadet/email_test.exs @@ -24,8 +24,11 @@ defmodule Cadet.EmailTest do avenger_user = insert(:user, %{email: "test@gmail.com"}) avenger = insert(:course_registration, %{user: avenger_user, role: :staff}) - {:ok, %{submissions: ungraded_submissions}} = - Cadet.Assessments.all_submissions_by_grader_for_index(avenger, true, true) + {:ok, %{data: %{submissions: ungraded_submissions}}} = + Cadet.Assessments.submissions_by_grader_for_index(avenger, %{ + "group" => "true", + "ungradedOnly" => "true" + }) email = Email.avenger_backlog_email("avenger_backlog", avenger_user, ungraded_submissions) diff --git a/test/cadet/jobs/notification_worker/notification_worker_test.exs b/test/cadet/jobs/notification_worker/notification_worker_test.exs index 626c46cac..9368a033c 100644 --- a/test/cadet/jobs/notification_worker/notification_worker_test.exs +++ b/test/cadet/jobs/notification_worker/notification_worker_test.exs @@ -18,8 +18,11 @@ defmodule Cadet.NotificationWorker.NotificationWorkerTest do submission = List.first(List.first(data.mcq_answers)).submission # setup for avenger backlog - {:ok, %{submissions: ungraded_submissions}} = - Cadet.Assessments.all_submissions_by_grader_for_index(avenger_cr, true, true) + {:ok, %{data: %{submissions: ungraded_submissions}}} = + Cadet.Assessments.submissions_by_grader_for_index(avenger_cr, %{ + "group" => "true", + "notFullyGraded" => "true" + }) Repo.update_all(NotificationType, set: [is_enabled: true]) Repo.update_all(NotificationConfig, set: [is_enabled: true]) diff --git a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs index a42131c19..05d926fb3 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -113,37 +113,45 @@ defmodule CadetWeb.AdminGradingControllerTest do conn = get(conn, build_url(course.id)) - expected = - Enum.map(submissions, fn submission -> - %{ - "xp" => 5000, - "xpAdjustment" => -2500, - "xpBonus" => 100, - "id" => submission.id, - "student" => %{ - "name" => submission.student.user.name, - "username" => submission.student.user.username, - "id" => submission.student.id, - "groupName" => submission.student.group.name, - "groupLeaderId" => submission.student.group.leader_id - }, - "assessment" => %{ - "type" => mission.config.type, - "isManuallyGraded" => mission.config.is_manually_graded, - "maxXp" => 5000, - "id" => mission.id, - "title" => mission.title, - "questionCount" => 5, - "assessmentNumber" => mission.number - }, - "status" => Atom.to_string(submission.status), - "gradedCount" => 5, - "unsubmittedBy" => nil, - "unsubmittedAt" => nil - } - end) + expected = %{ + "count" => length(submissions), + "data" => + Enum.map(submissions, fn submission -> + %{ + "xp" => 5000, + "xpAdjustment" => -2500, + "xpBonus" => 100, + "id" => submission.id, + "student" => %{ + "name" => submission.student.user.name, + "username" => submission.student.user.username, + "id" => submission.student.id, + "groupName" => submission.student.group.name, + "groupLeaderId" => submission.student.group.leader_id + }, + "assessment" => %{ + "type" => mission.config.type, + "isManuallyGraded" => mission.config.is_manually_graded, + "maxXp" => 5000, + "id" => mission.id, + "title" => mission.title, + "questionCount" => 5, + "assessmentNumber" => mission.number + }, + "status" => Atom.to_string(submission.status), + "gradedCount" => 5, + "unsubmittedBy" => nil, + "unsubmittedAt" => nil + } + end) + } + + res = json_response(conn, 200) - assert expected == Enum.sort_by(json_response(conn, 200), & &1["id"]) + assert expected == %{ + "count" => res["count"], + "data" => Enum.sort_by(res["data"], & &1["id"]) + } end end @@ -161,7 +169,7 @@ defmodule CadetWeb.AdminGradingControllerTest do |> get(build_url(test_cr.course_id), %{"group" => "true"}) |> json_response(200) - assert resp == [] + assert resp == %{"count" => 0, "data" => []} end @tag authenticate: :staff @@ -178,37 +186,45 @@ defmodule CadetWeb.AdminGradingControllerTest do conn = get(conn, build_url(course.id), %{"group" => "true"}) - expected = - Enum.map(submissions, fn submission -> - %{ - "xp" => 5000, - "xpAdjustment" => -2500, - "xpBonus" => 100, - "id" => submission.id, - "student" => %{ - "name" => submission.student.user.name, - "username" => submission.student.user.username, - "id" => submission.student.id, - "groupName" => submission.student.group.name, - "groupLeaderId" => submission.student.group.leader_id - }, - "assessment" => %{ - "type" => mission.config.type, - "isManuallyGraded" => mission.config.is_manually_graded, - "maxXp" => 5000, - "id" => mission.id, - "title" => mission.title, - "questionCount" => 5, - "assessmentNumber" => mission.number - }, - "status" => Atom.to_string(submission.status), - "gradedCount" => 5, - "unsubmittedBy" => nil, - "unsubmittedAt" => nil - } - end) + expected = %{ + "count" => length(submissions), + "data" => + Enum.map(submissions, fn submission -> + %{ + "xp" => 5000, + "xpAdjustment" => -2500, + "xpBonus" => 100, + "id" => submission.id, + "student" => %{ + "name" => submission.student.user.name, + "username" => submission.student.user.username, + "id" => submission.student.id, + "groupName" => submission.student.group.name, + "groupLeaderId" => submission.student.group.leader_id + }, + "assessment" => %{ + "type" => mission.config.type, + "isManuallyGraded" => mission.config.is_manually_graded, + "maxXp" => 5000, + "id" => mission.id, + "title" => mission.title, + "questionCount" => 5, + "assessmentNumber" => mission.number + }, + "status" => Atom.to_string(submission.status), + "gradedCount" => 5, + "unsubmittedBy" => nil, + "unsubmittedAt" => nil + } + end) + } + + res = json_response(conn, 200) - assert expected == Enum.sort_by(json_response(conn, 200), & &1["id"]) + assert expected == %{ + "count" => res["count"], + "data" => Enum.sort_by(res["data"], & &1["id"]) + } end end @@ -793,37 +809,45 @@ defmodule CadetWeb.AdminGradingControllerTest do |> sign_in(admin.user) |> get(build_url(course.id)) - expected = - Enum.map(submissions, fn submission -> - %{ - "xp" => 5000, - "xpAdjustment" => -2500, - "xpBonus" => 100, - "id" => submission.id, - "student" => %{ - "name" => submission.student.user.name, - "username" => submission.student.user.username, - "id" => submission.student.id, - "groupName" => submission.student.group.name, - "groupLeaderId" => submission.student.group.leader_id - }, - "assessment" => %{ - "type" => mission.config.type, - "isManuallyGraded" => mission.config.is_manually_graded, - "maxXp" => 5000, - "id" => mission.id, - "title" => mission.title, - "questionCount" => 5, - "assessmentNumber" => mission.number - }, - "status" => Atom.to_string(submission.status), - "gradedCount" => 5, - "unsubmittedBy" => nil, - "unsubmittedAt" => nil - } - end) + expected = %{ + "count" => length(submissions), + "data" => + Enum.map(submissions, fn submission -> + %{ + "xp" => 5000, + "xpAdjustment" => -2500, + "xpBonus" => 100, + "id" => submission.id, + "student" => %{ + "name" => submission.student.user.name, + "username" => submission.student.user.username, + "id" => submission.student.id, + "groupName" => submission.student.group.name, + "groupLeaderId" => submission.student.group.leader_id + }, + "assessment" => %{ + "type" => mission.config.type, + "isManuallyGraded" => mission.config.is_manually_graded, + "maxXp" => 5000, + "id" => mission.id, + "title" => mission.title, + "questionCount" => 5, + "assessmentNumber" => mission.number + }, + "status" => Atom.to_string(submission.status), + "gradedCount" => 5, + "unsubmittedBy" => nil, + "unsubmittedAt" => nil + } + end) + } + + res = json_response(conn, 200) - assert expected == Enum.sort_by(json_response(conn, 200), & &1["id"]) + assert expected == %{ + "count" => res["count"], + "data" => Enum.sort_by(res["data"], & &1["id"]) + } end end @@ -838,37 +862,45 @@ defmodule CadetWeb.AdminGradingControllerTest do conn = get(conn, build_url(course.id), %{"group" => "true"}) - expected = - Enum.map(submissions, fn submission -> - %{ - "xp" => 5000, - "xpAdjustment" => -2500, - "xpBonus" => 100, - "id" => submission.id, - "student" => %{ - "name" => submission.student.user.name, - "username" => submission.student.user.username, - "id" => submission.student.id, - "groupName" => submission.student.group.name, - "groupLeaderId" => submission.student.group.leader_id - }, - "assessment" => %{ - "type" => mission.config.type, - "isManuallyGraded" => mission.config.is_manually_graded, - "maxXp" => 5000, - "id" => mission.id, - "title" => mission.title, - "questionCount" => 5, - "assessmentNumber" => mission.number - }, - "status" => Atom.to_string(submission.status), - "gradedCount" => 5, - "unsubmittedBy" => nil, - "unsubmittedAt" => nil - } - end) + expected = %{ + "count" => length(submissions), + "data" => + Enum.map(submissions, fn submission -> + %{ + "xp" => 5000, + "xpAdjustment" => -2500, + "xpBonus" => 100, + "id" => submission.id, + "student" => %{ + "name" => submission.student.user.name, + "username" => submission.student.user.username, + "id" => submission.student.id, + "groupName" => submission.student.group.name, + "groupLeaderId" => submission.student.group.leader_id + }, + "assessment" => %{ + "type" => mission.config.type, + "isManuallyGraded" => mission.config.is_manually_graded, + "maxXp" => 5000, + "id" => mission.id, + "title" => mission.title, + "questionCount" => 5, + "assessmentNumber" => mission.number + }, + "status" => Atom.to_string(submission.status), + "gradedCount" => 5, + "unsubmittedBy" => nil, + "unsubmittedAt" => nil + } + end) + } + + res = json_response(conn, 200) - assert expected == Enum.sort_by(json_response(conn, 200), & &1["id"]) + assert expected == %{ + "count" => res["count"], + "data" => Enum.sort_by(res["data"], & &1["id"]) + } end end