From 8022419eb72ee236dd48079500ebe1d0b9e9a8ea Mon Sep 17 00:00:00 2001 From: emptygx Date: Mon, 3 Jul 2023 11:01:40 +0800 Subject: [PATCH 01/74] Add migrations to create is_grading_published column for submissions Current submissions will have the is_grading_published field populated as `true` if all answers have been graded i.e. have a grader_id, while the rest will default to `false`. Future submissions will have the field with default value `false` upon creation. --- ...0230702122108_add_is_grading_published.exs | 9 +++++++ ...0702184853_update_is_grading_published.exs | 24 +++++++++++++++++++ 2 files changed, 33 insertions(+) create mode 100644 priv/repo/migrations/20230702122108_add_is_grading_published.exs create mode 100644 priv/repo/migrations/20230702184853_update_is_grading_published.exs diff --git a/priv/repo/migrations/20230702122108_add_is_grading_published.exs b/priv/repo/migrations/20230702122108_add_is_grading_published.exs new file mode 100644 index 000000000..473a71f37 --- /dev/null +++ b/priv/repo/migrations/20230702122108_add_is_grading_published.exs @@ -0,0 +1,9 @@ +defmodule Cadet.Repo.Migrations.AddIsGradingPublished do + use Ecto.Migration + + def change do + alter table(:submissions) do + add(:is_grading_published, :boolean, null: false, default: false) + end + end +end diff --git a/priv/repo/migrations/20230702184853_update_is_grading_published.exs b/priv/repo/migrations/20230702184853_update_is_grading_published.exs new file mode 100644 index 000000000..28c43392a --- /dev/null +++ b/priv/repo/migrations/20230702184853_update_is_grading_published.exs @@ -0,0 +1,24 @@ +defmodule MyApp.Repo.Migrations.UpdateIsGradingPublished do + use Ecto.Migration + + def up do + execute(""" + UPDATE submissions + SET is_grading_published = true + WHERE id IN ( + SELECT s.id + FROM submissions AS s + JOIN answers AS a ON a.submission_id = s.id + GROUP BY s.id + HAVING COUNT(a.id) = COUNT(a.grader_id) + ) + """) + end + + def down do + execute(""" + UPDATE submissions + SET is_grading_published = false + """) + end +end From e28690262e5a773463388d5fd762359cf304eda2 Mon Sep 17 00:00:00 2001 From: emptygx Date: Sat, 8 Jul 2023 03:12:48 +0800 Subject: [PATCH 02/74] Add and update endpoints for new publish grading feature - Update existing endpoints and queries related to assessment retrieval - Add new routes and endpoints to handle new publish grading feature - Add notification for new feature --- lib/cadet/accounts/notification_type.ex | 4 +- lib/cadet/accounts/notifications.ex | 26 +++- lib/cadet/assessments/assessment.ex | 1 + lib/cadet/assessments/assessments.ex | 132 +++++++++++++++++- lib/cadet/assessments/submission.ex | 3 +- .../admin_grading_controller.ex | 30 ++++ lib/cadet_web/router.ex | 2 + lib/cadet_web/views/assessments_view.ex | 3 +- 8 files changed, 192 insertions(+), 9 deletions(-) diff --git a/lib/cadet/accounts/notification_type.ex b/lib/cadet/accounts/notification_type.ex index 3ce69f709..990e78efb 100644 --- a/lib/cadet/accounts/notification_type.ex +++ b/lib/cadet/accounts/notification_type.ex @@ -10,5 +10,7 @@ defenum(Cadet.Accounts.NotificationType, :notification_type, [ # Notifications for unsubmitted submissions :unsubmitted, # Notifications for submitted assessments - :submitted + :submitted, + # Notifications for unpublished grades + :unpublishedGrading ]) diff --git a/lib/cadet/accounts/notifications.ex b/lib/cadet/accounts/notifications.ex index 8ee558eb3..553f52feb 100644 --- a/lib/cadet/accounts/notifications.ex +++ b/lib/cadet/accounts/notifications.ex @@ -151,7 +151,7 @@ defmodule Cadet.Accounts.Notifications do Notification |> where(course_reg_id: ^student.id) |> where(assessment_id: ^assessment_id) - |> where([n], n.type in ^[:autograded, :graded]) + |> where([n], n.type in ^[:autograded, :graded, :unpublishedGrading]) |> Repo.delete_all() write(%{ @@ -162,6 +162,30 @@ defmodule Cadet.Accounts.Notifications do }) end + @doc """ + Function that handles notifications when a submission grade is unpublished. + """ + @spec handle_unpublish_grades_notifications(integer(), CourseRegistration.t()) :: + {:ok, Ecto.Schema.t()} | {:error, Ecto.Changeset.t()} + def handle_unpublish_grades_notifications(assessment_id, student = %CourseRegistration{}) + when is_ecto_id(assessment_id) do + # Fetch and delete all notifications of :graded + # Add new notification :unpublished + + Notification + |> where(course_reg_id: ^student.id) + |> where(assessment_id: ^assessment_id) + |> where([n], n.type in ^[:autograded, :graded, :unsubmitted]) + |> Repo.delete_all() + + write(%{ + type: :unpublishedGrading, + role: student.role, + course_reg_id: student.id, + assessment_id: assessment_id + }) + end + @doc """ Writes a notification that a student's submission has been graded successfully. (for the student) diff --git a/lib/cadet/assessments/assessment.ex b/lib/cadet/assessments/assessment.ex index 77d0ef513..59a7db6ef 100644 --- a/lib/cadet/assessments/assessment.ex +++ b/lib/cadet/assessments/assessment.ex @@ -20,6 +20,7 @@ defmodule Cadet.Assessments.Assessment do field(:grading_status, :string, virtual: true) field(:question_count, :integer, virtual: true) field(:graded_count, :integer, virtual: true) + field(:is_grading_published, :boolean, virtual: true, default: false) field(:title, :string) field(:is_published, :boolean, default: false) field(:summary_short, :string) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 40c9a4869..44809b937 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -91,6 +91,7 @@ defmodule Cadet.Assessments do submission_xp = Submission |> where(student_id: ^cr_id) + |> where(is_grading_published: true) |> join(:inner, [s], a in Answer, on: s.id == a.submission_id) |> group_by([s], s.id) |> select([s, a], %{ @@ -248,6 +249,11 @@ defmodule Cadet.Assessments do Answer |> join(:inner, [a], s in assoc(a, :submission)) |> where([_, s], s.student_id == ^course_reg.id) + # change a.xp and a.xp_adjustment to 0 if a.is_grading_published is false and return all answers + |> select([a, s], %{ + a + | xp: fragment("CASE WHEN ? THEN ? ELSE 0 END", s.is_grading_published, a.xp), + xp_adjustment: fragment("CASE WHEN ? THEN ? ELSE 0 END", s.is_grading_published, a.xp_adjustment)}) questions = Question @@ -298,6 +304,11 @@ defmodule Cadet.Assessments do |> where([s], s.student_id == ^cr.id) |> select([s], [:assessment_id, :status]) + grading_published_status = + Submission + |> where([s], s.student_id == ^cr.id) + |> select([s], [:assessment_id, :is_grading_published]) + assessments = cr.course_id |> Query.all_assessments_with_aggregates() @@ -309,11 +320,13 @@ defmodule Cadet.Assessments do on: a.id == sa.assessment_id ) |> join(:left, [a, _], s in subquery(submission_status), on: a.id == s.assessment_id) - |> select([a, sa, s], %{ + |> join(:left, [a, _, _], gp in subquery(grading_published_status), on: a.id == gp.assessment_id) + |> select([a, sa, s, gp], %{ a - | xp: sa.xp, + | xp: fragment("CASE WHEN ? THEN ? ELSE 0 END", gp.is_grading_published, sa.xp), graded_count: sa.graded_count, - user_status: s.status + user_status: s.status, + is_grading_published: gp.is_grading_published }) |> filter_published_assessments(cr) |> order_by(:open_at) @@ -844,6 +857,113 @@ defmodule Cadet.Assessments do end end + # This function changes the is_grading_published field of the submission to false + # and sends a notification to the student + def unpublish_grading(submission_id, cr = %CourseRegistration{id: course_reg_id, role: role}) + when is_ecto_id(submission_id) do +submission = + Submission + |> join(:inner, [s], a in assoc(s, :assessment)) + |> preload([_, a], assessment: a) + |> Repo.get(submission_id) + +# allows staff to unpublish own assessment +bypass = role in @bypass_closed_roles and submission.student_id == course_reg_id + +with {:submission_found?, true} <- {:submission_found?, is_map(submission)}, + {:allowed_to_unpublish?, true} <- + {:allowed_to_unpublish?, + role == :admin or bypass or + Cadet.Accounts.Query.avenger_of?(cr, submission.student_id)} do + Multi.new() + |> Multi.run( + :unpublish_grading, + fn _repo, _ -> + submission + |> Submission.changeset(%{is_grading_published: false}) + |> Repo.update() + end + ) + |> Repo.transaction() + + Cadet.Accounts.Notifications.handle_unpublish_grades_notifications( + submission.assessment.id, + Repo.get(CourseRegistration, submission.student_id) + ) + + {:ok, nil} +else + {:submission_found?, false} -> + {:error, {:not_found, "Submission not found"}} + + {:is_open?, false} -> + {:error, {:forbidden, "Assessment not open"}} + + {:allowed_to_unpublish?, false} -> + {:error, {:forbidden, "Only Avenger of student is permitted to unpublish grading"}} + + _ -> + {:error, {:internal_server_error, "Please try again later."}} +end +end + + # This function changes the is_grading_published field of the submission to true only if all the answers are graded + # and sends a notification to the student + def publish_grading(submission_id, cr = %CourseRegistration{id: course_reg_id, role: role}) + when is_ecto_id(submission_id) do + submission = + Submission + |> join(:inner, [s], a in assoc(s, :assessment)) + |> preload([_, a], assessment: a) + |> Repo.get(submission_id) + + # allows staff to publish own assessment + bypass = role in @bypass_closed_roles and submission.student_id == course_reg_id + + with {:submission_found?, true} <- {:submission_found?, is_map(submission)}, + {:status, :submitted} <- {:status, submission.status}, + {:allowed_to_publish?, true} <- + {:allowed_to_publish?, + role == :admin or bypass or + Cadet.Accounts.Query.avenger_of?(cr, submission.student_id)} do + Multi.new() + |> Multi.run( + :publish_grading, + fn _repo, _ -> + submission + |> Submission.changeset(%{is_grading_published: true}) + |> Repo.update() + end + ) + |> Repo.transaction() + + Cadet.Accounts.Notifications.write_notification_when_graded( + submission.assessment.id, + :graded + ) + + {:ok, nil} + else + {:submission_found?, false} -> + {:error, {:not_found, "Submission not found"}} + + {:is_open?, false} -> + {:error, {:forbidden, "Assessment not open"}} + + {:status, :attempting} -> + {:error, {:bad_request, "Some questions have not been attempted"}} + + {:status, :attempted} -> + {:error, {:bad_request, "Assessment has not been submitted"}} + + {:allowed_to_publish?, false} -> + {:error, {:forbidden, "Only Avenger of student is permitted to publish grading"}} + + _ -> + {:error, {:internal_server_error, "Please try again later."}} + end + end + @spec update_submission_status_and_xp_bonus(Submission.t()) :: {:ok, Submission.t()} | {:error, Ecto.Changeset.t()} defp update_submission_status_and_xp_bonus(submission = %Submission{}) do @@ -1197,6 +1317,7 @@ defmodule Cadet.Assessments do s."xpAdjustment", s."xpBonus", s."gradedCount", + s."isGradingPublished", assts.jsn as assessment, students.jsn as student, unsubmitters.jsn as "unsubmittedBy" @@ -1211,7 +1332,8 @@ defmodule Cadet.Assessments do sum(ans.xp) as xp, sum(ans.xp_adjustment) as "xpAdjustment", s.xp_bonus as "xpBonus", - count(ans.id) filter (where ans.grader_id is not null) as "gradedCount" + count(ans.id) filter (where ans.grader_id is not null) as "gradedCount", + s.is_grading_published as "isGradingPublished" from submissions s left join answers ans on s.id = ans.submission_id @@ -1369,7 +1491,7 @@ defmodule Cadet.Assessments do {:ok, _} <- Repo.update(changeset) do if is_fully_graded?(answer) and not is_own_submission do # Every answer in this submission has been graded manually - Notifications.write_notification_when_graded(submission_id, :graded) + # Notifications.write_notification_when_graded(submission_id, :graded) else {:ok, nil} end diff --git a/lib/cadet/assessments/submission.ex b/lib/cadet/assessments/submission.ex index eb8164065..5170dae1d 100644 --- a/lib/cadet/assessments/submission.ex +++ b/lib/cadet/assessments/submission.ex @@ -17,6 +17,7 @@ defmodule Cadet.Assessments.Submission do field(:graded_count, :integer, virtual: true, default: 0) field(:grading_status, :string, virtual: true) field(:unsubmitted_at, :utc_datetime_usec) + field(:is_grading_published, :boolean, default: false) belongs_to(:assessment, Assessment) belongs_to(:student, CourseRegistration) @@ -26,7 +27,7 @@ defmodule Cadet.Assessments.Submission do timestamps() end - @required_fields ~w(student_id assessment_id status)a + @required_fields ~w(student_id assessment_id status is_grading_published)a @optional_fields ~w(xp_bonus unsubmitted_by_id unsubmitted_at)a def changeset(submission, params) do diff --git a/lib/cadet_web/admin_controllers/admin_grading_controller.ex b/lib/cadet_web/admin_controllers/admin_grading_controller.ex index 2bf3c0360..7cc2fef87 100644 --- a/lib/cadet_web/admin_controllers/admin_grading_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_grading_controller.ex @@ -82,6 +82,34 @@ defmodule CadetWeb.AdminGradingController do end end + def unpublish_grades(conn, %{"submissionid" => submission_id}) when is_ecto_id(submission_id) do + course_reg = conn.assigns[:course_reg] + + case Assessments.unpublish_grading(submission_id, course_reg) do + {:ok, nil} -> + text(conn, "OK") + + {:error, {status, message}} -> + conn + |> put_status(status) + |> text(message) + end + end + + def publish_grades(conn, %{"submissionid" => submission_id}) when is_ecto_id(submission_id) do + course_reg = conn.assigns[:course_reg] + + case Assessments.publish_grading(submission_id, course_reg) do + {:ok, nil} -> + text(conn, "OK") + + {:error, {status, message}} -> + conn + |> put_status(status) + |> text(message) + end + end + def autograde_submission(conn, %{"submissionid" => submission_id}) do course_reg = conn.assigns[:course_reg] @@ -277,6 +305,8 @@ defmodule CadetWeb.AdminGradingController do unsubmittedBy(Schema.ref(:GraderInfo)) unsubmittedAt(:string, "Last unsubmitted at", format: "date-time", required: false) + + isGradingPublished(:boolean, "Whether the grading is published", required: true) end end, AssessmentInfo: diff --git a/lib/cadet_web/router.ex b/lib/cadet_web/router.ex index cbd3fb755..14f0e04d3 100644 --- a/lib/cadet_web/router.ex +++ b/lib/cadet_web/router.ex @@ -118,6 +118,8 @@ defmodule CadetWeb.Router do get("/grading/summary", AdminGradingController, :grading_summary) get("/grading/:submissionid", AdminGradingController, :show) post("/grading/:submissionid/unsubmit", AdminGradingController, :unsubmit) + post("/grading/:submissionid/unpublish_grades", AdminGradingController, :unpublish_grades) + post("/grading/:submissionid/publish_grades", AdminGradingController, :publish_grades) post("/grading/:submissionid/autograde", AdminGradingController, :autograde_submission) post("/grading/:submissionid/:questionid", AdminGradingController, :update) diff --git a/lib/cadet_web/views/assessments_view.ex b/lib/cadet_web/views/assessments_view.ex index 20aad951f..282372ee3 100644 --- a/lib/cadet_web/views/assessments_view.ex +++ b/lib/cadet_web/views/assessments_view.ex @@ -28,7 +28,8 @@ defmodule CadetWeb.AssessmentsView do private: &password_protected?(&1.password), isPublished: :is_published, questionCount: :question_count, - gradedCount: &(&1.graded_count || 0) + gradedCount: &(&1.graded_count || 0), + isGradingPublished: :is_grading_published }) end From d174fbdbd77c3ee772c8f553268716e5accfc7c2 Mon Sep 17 00:00:00 2001 From: emptygx Date: Sun, 9 Jul 2023 23:52:46 +0800 Subject: [PATCH 03/74] Fix missing return tuple values in assessments.ex --- lib/cadet/assessments/assessments.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 44809b937..bcc6610ef 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1492,6 +1492,7 @@ end if is_fully_graded?(answer) and not is_own_submission do # Every answer in this submission has been graded manually # Notifications.write_notification_when_graded(submission_id, :graded) + {:ok, nil} else {:ok, nil} end From bc36b60d16d7ce077bef96b68262f9e064179a05 Mon Sep 17 00:00:00 2001 From: emptygx Date: Wed, 12 Jul 2023 17:40:15 +0800 Subject: [PATCH 04/74] Fix notification and formatting --- lib/cadet/assessments/assessments.ex | 90 +++++++++---------- .../controllers/notifications_controller.ex | 2 +- 2 files changed, 44 insertions(+), 48 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index bcc6610ef..90e9e8e01 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -860,52 +860,52 @@ defmodule Cadet.Assessments do # This function changes the is_grading_published field of the submission to false # and sends a notification to the student def unpublish_grading(submission_id, cr = %CourseRegistration{id: course_reg_id, role: role}) - when is_ecto_id(submission_id) do -submission = - Submission - |> join(:inner, [s], a in assoc(s, :assessment)) - |> preload([_, a], assessment: a) - |> Repo.get(submission_id) - -# allows staff to unpublish own assessment -bypass = role in @bypass_closed_roles and submission.student_id == course_reg_id - -with {:submission_found?, true} <- {:submission_found?, is_map(submission)}, - {:allowed_to_unpublish?, true} <- - {:allowed_to_unpublish?, - role == :admin or bypass or - Cadet.Accounts.Query.avenger_of?(cr, submission.student_id)} do - Multi.new() - |> Multi.run( - :unpublish_grading, - fn _repo, _ -> - submission - |> Submission.changeset(%{is_grading_published: false}) - |> Repo.update() - end - ) - |> Repo.transaction() + when is_ecto_id(submission_id) do + submission = + Submission + |> join(:inner, [s], a in assoc(s, :assessment)) + |> preload([_, a], assessment: a) + |> Repo.get(submission_id) + + # allows staff to unpublish own assessment + bypass = role in @bypass_closed_roles and submission.student_id == course_reg_id + + with {:submission_found?, true} <- {:submission_found?, is_map(submission)}, + {:allowed_to_unpublish?, true} <- + {:allowed_to_unpublish?, + role == :admin or bypass or + Cadet.Accounts.Query.avenger_of?(cr, submission.student_id)} do + Multi.new() + |> Multi.run( + :unpublish_grading, + fn _repo, _ -> + submission + |> Submission.changeset(%{is_grading_published: false}) + |> Repo.update() + end + ) + |> Repo.transaction() - Cadet.Accounts.Notifications.handle_unpublish_grades_notifications( - submission.assessment.id, - Repo.get(CourseRegistration, submission.student_id) - ) + Cadet.Accounts.Notifications.handle_unpublish_grades_notifications( + submission.assessment.id, + Repo.get(CourseRegistration, submission.student_id) + ) - {:ok, nil} -else - {:submission_found?, false} -> - {:error, {:not_found, "Submission not found"}} + {:ok, nil} + else + {:submission_found?, false} -> + {:error, {:not_found, "Submission not found"}} - {:is_open?, false} -> - {:error, {:forbidden, "Assessment not open"}} + {:is_open?, false} -> + {:error, {:forbidden, "Assessment not open"}} - {:allowed_to_unpublish?, false} -> - {:error, {:forbidden, "Only Avenger of student is permitted to unpublish grading"}} + {:allowed_to_unpublish?, false} -> + {:error, {:forbidden, "Only Avenger of student is permitted to unpublish grading"}} - _ -> - {:error, {:internal_server_error, "Please try again later."}} -end -end + _ -> + {:error, {:internal_server_error, "Please try again later."}} + end + end # This function changes the is_grading_published field of the submission to true only if all the answers are graded # and sends a notification to the student @@ -938,7 +938,7 @@ end |> Repo.transaction() Cadet.Accounts.Notifications.write_notification_when_graded( - submission.assessment.id, + submission.id, :graded ) @@ -1489,11 +1489,7 @@ end {:valid, changeset = %Ecto.Changeset{valid?: true}} <- {:valid, Answer.grading_changeset(answer, attrs)}, {:ok, _} <- Repo.update(changeset) do - if is_fully_graded?(answer) and not is_own_submission do - # Every answer in this submission has been graded manually - # Notifications.write_notification_when_graded(submission_id, :graded) - {:ok, nil} - else + {:ok, nil} end else diff --git a/lib/cadet_web/controllers/notifications_controller.ex b/lib/cadet_web/controllers/notifications_controller.ex index 5989f3336..33c538f9f 100644 --- a/lib/cadet_web/controllers/notifications_controller.ex +++ b/lib/cadet_web/controllers/notifications_controller.ex @@ -100,7 +100,7 @@ defmodule CadetWeb.NotificationsController do NotificationType: swagger_schema do type(:string) - enum([:new, :deadline, :autograded, :graded, :submitted, :unsubmitted, :new_message]) + enum([:new, :deadline, :autograded, :graded, :submitted, :unsubmitted, :unpublishedGrading, :new_message]) end } end From 215328037dcc929e70ac73ff3f1d39f8d2da9b30 Mon Sep 17 00:00:00 2001 From: emptygx Date: Mon, 17 Jul 2023 13:19:59 +0800 Subject: [PATCH 05/74] Fix indentation and minor error --- lib/cadet/assessments/assessments.ex | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 90e9e8e01..49a913a48 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1490,8 +1490,7 @@ defmodule Cadet.Assessments do {:valid, Answer.grading_changeset(answer, attrs)}, {:ok, _} <- Repo.update(changeset) do - {:ok, nil} - end + {:ok, nil} else {:answer_found?, false} -> {:error, {:bad_request, "Answer not found or user not permitted to grade."}} From 1891d7230d3a0bfec2401aae8566f994caa12277 Mon Sep 17 00:00:00 2001 From: emptygx Date: Mon, 17 Jul 2023 16:49:11 +0800 Subject: [PATCH 06/74] Fix formatting --- lib/cadet/accounts/notifications.ex | 2 +- lib/cadet/assessments/assessments.ex | 13 ++++++++----- lib/cadet/devices/devices.ex | 2 +- .../admin_controllers/admin_assets_controller.ex | 10 +++++++--- .../admin_controllers/admin_courses_controller.ex | 6 +++++- .../admin_controllers/admin_grading_controller.ex | 4 +++- .../admin_controllers/admin_stories_controller.ex | 6 +++--- .../controllers/notifications_controller.ex | 12 +++++++++++- 8 files changed, 39 insertions(+), 16 deletions(-) diff --git a/lib/cadet/accounts/notifications.ex b/lib/cadet/accounts/notifications.ex index 553f52feb..bab127a97 100644 --- a/lib/cadet/accounts/notifications.ex +++ b/lib/cadet/accounts/notifications.ex @@ -166,7 +166,7 @@ defmodule Cadet.Accounts.Notifications do Function that handles notifications when a submission grade is unpublished. """ @spec handle_unpublish_grades_notifications(integer(), CourseRegistration.t()) :: - {:ok, Ecto.Schema.t()} | {:error, Ecto.Changeset.t()} + {:ok, Ecto.Schema.t()} | {:error, Ecto.Changeset.t()} def handle_unpublish_grades_notifications(assessment_id, student = %CourseRegistration{}) when is_ecto_id(assessment_id) do # Fetch and delete all notifications of :graded diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 49a913a48..881982022 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -253,7 +253,9 @@ defmodule Cadet.Assessments do |> select([a, s], %{ a | xp: fragment("CASE WHEN ? THEN ? ELSE 0 END", s.is_grading_published, a.xp), - xp_adjustment: fragment("CASE WHEN ? THEN ? ELSE 0 END", s.is_grading_published, a.xp_adjustment)}) + xp_adjustment: + fragment("CASE WHEN ? THEN ? ELSE 0 END", s.is_grading_published, a.xp_adjustment) + }) questions = Question @@ -320,7 +322,9 @@ defmodule Cadet.Assessments do on: a.id == sa.assessment_id ) |> join(:left, [a, _], s in subquery(submission_status), on: a.id == s.assessment_id) - |> join(:left, [a, _, _], gp in subquery(grading_published_status), on: a.id == gp.assessment_id) + |> join(:left, [a, _, _], gp in subquery(grading_published_status), + on: a.id == gp.assessment_id + ) |> select([a, sa, s, gp], %{ a | xp: fragment("CASE WHEN ? THEN ? ELSE 0 END", gp.is_grading_published, sa.xp), @@ -871,8 +875,8 @@ defmodule Cadet.Assessments do bypass = role in @bypass_closed_roles and submission.student_id == course_reg_id with {:submission_found?, true} <- {:submission_found?, is_map(submission)}, - {:allowed_to_unpublish?, true} <- - {:allowed_to_unpublish?, + {:allowed_to_unpublish?, true} <- + {:allowed_to_unpublish?, role == :admin or bypass or Cadet.Accounts.Query.avenger_of?(cr, submission.student_id)} do Multi.new() @@ -1489,7 +1493,6 @@ defmodule Cadet.Assessments do {:valid, changeset = %Ecto.Changeset{valid?: true}} <- {:valid, Answer.grading_changeset(answer, attrs)}, {:ok, _} <- Repo.update(changeset) do - {:ok, nil} else {:answer_found?, false} -> diff --git a/lib/cadet/devices/devices.ex b/lib/cadet/devices/devices.ex index 7862b8792..d036151d3 100644 --- a/lib/cadet/devices/devices.ex +++ b/lib/cadet/devices/devices.ex @@ -208,7 +208,7 @@ defmodule Cadet.Devices do }, 300, [], - '' + ~c"" ) # ExAws includes the session token in the signed payload and doesn't allow diff --git a/lib/cadet_web/admin_controllers/admin_assets_controller.ex b/lib/cadet_web/admin_controllers/admin_assets_controller.ex index 8fd2f7003..b02f1b364 100644 --- a/lib/cadet_web/admin_controllers/admin_assets_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_assets_controller.ex @@ -22,7 +22,7 @@ defmodule CadetWeb.AdminAssetsController do case Assets.delete_object(Courses.assets_prefix(course_reg.course), foldername, filename) do {:error, {status, message}} -> conn |> put_status(status) |> text(message) - _ -> conn |> put_status(204) |> text('') + _ -> conn |> put_status(204) |> text(~c"") end end @@ -96,7 +96,9 @@ defmodule CadetWeb.AdminAssetsController do parameters do folderName(:path, :string, "Folder name", required: true) - fileName(:path, :string, "File path in folder, which may contain subfolders", required: true) + fileName(:path, :string, "File path in folder, which may contain subfolders", + required: true + ) end security([%{JWT: []}]) @@ -115,7 +117,9 @@ defmodule CadetWeb.AdminAssetsController do parameters do folderName(:path, :string, "Folder name", required: true) - fileName(:path, :string, "File path in folder, which may contain subfolders", required: true) + fileName(:path, :string, "File path in folder, which may contain subfolders", + required: true + ) end security([%{JWT: []}]) diff --git a/lib/cadet_web/admin_controllers/admin_courses_controller.ex b/lib/cadet_web/admin_controllers/admin_courses_controller.ex index c932c67c9..b4912aa65 100644 --- a/lib/cadet_web/admin_controllers/admin_courses_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_courses_controller.ex @@ -142,7 +142,11 @@ defmodule CadetWeb.AdminCoursesController do title("AdminSublanguage") properties do - chapter(:integer, "Chapter number from 1 to 4", required: true, minimum: 1, maximum: 4) + chapter(:integer, "Chapter number from 1 to 4", + required: true, + minimum: 1, + maximum: 4 + ) variant(Schema.ref(:SourceVariant), "Variant name", required: true) end diff --git a/lib/cadet_web/admin_controllers/admin_grading_controller.ex b/lib/cadet_web/admin_controllers/admin_grading_controller.ex index 7cc2fef87..efcd01755 100644 --- a/lib/cadet_web/admin_controllers/admin_grading_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_grading_controller.ex @@ -301,7 +301,9 @@ defmodule CadetWeb.AdminGradingController do required: true ) - student(Schema.ref(:StudentInfo), "Student who created the submission", required: true) + student(Schema.ref(:StudentInfo), "Student who created the submission", + required: true + ) unsubmittedBy(Schema.ref(:GraderInfo)) unsubmittedAt(:string, "Last unsubmitted at", format: "date-time", required: false) diff --git a/lib/cadet_web/admin_controllers/admin_stories_controller.ex b/lib/cadet_web/admin_controllers/admin_stories_controller.ex index 08d869245..92596cf2b 100644 --- a/lib/cadet_web/admin_controllers/admin_stories_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_stories_controller.ex @@ -12,7 +12,7 @@ defmodule CadetWeb.AdminStoriesController do case result do {:ok, _story} -> - conn |> put_status(200) |> text('') + conn |> put_status(200) |> text(~c"") {:error, {status, message}} -> conn @@ -29,7 +29,7 @@ defmodule CadetWeb.AdminStoriesController do case result do {:ok, _story} -> - conn |> put_status(200) |> text('') + conn |> put_status(200) |> text(~c"") {:error, {status, message}} -> conn @@ -43,7 +43,7 @@ defmodule CadetWeb.AdminStoriesController do case result do {:ok, _nil} -> - conn |> put_status(204) |> text('') + conn |> put_status(204) |> text(~c"") {:error, {status, message}} -> conn diff --git a/lib/cadet_web/controllers/notifications_controller.ex b/lib/cadet_web/controllers/notifications_controller.ex index 33c538f9f..4b3167035 100644 --- a/lib/cadet_web/controllers/notifications_controller.ex +++ b/lib/cadet_web/controllers/notifications_controller.ex @@ -100,7 +100,17 @@ defmodule CadetWeb.NotificationsController do NotificationType: swagger_schema do type(:string) - enum([:new, :deadline, :autograded, :graded, :submitted, :unsubmitted, :unpublishedGrading, :new_message]) + + enum([ + :new, + :deadline, + :autograded, + :graded, + :submitted, + :unsubmitted, + :unpublishedGrading, + :new_message + ]) end } end From 55c5a3634289f6023b4c90d8e1b11f7018d27146 Mon Sep 17 00:00:00 2001 From: YaleChen299 Date: Sun, 23 Jul 2023 17:33:58 +0800 Subject: [PATCH 07/74] fix formatting --- .../admin_controllers/admin_assets_controller.ex | 8 ++------ .../admin_controllers/admin_grading_controller.ex | 4 +--- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/cadet_web/admin_controllers/admin_assets_controller.ex b/lib/cadet_web/admin_controllers/admin_assets_controller.ex index b02f1b364..2fc938571 100644 --- a/lib/cadet_web/admin_controllers/admin_assets_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_assets_controller.ex @@ -96,9 +96,7 @@ defmodule CadetWeb.AdminAssetsController do parameters do folderName(:path, :string, "Folder name", required: true) - fileName(:path, :string, "File path in folder, which may contain subfolders", - required: true - ) + fileName(:path, :string, "File path in folder, which may contain subfolders", required: true) end security([%{JWT: []}]) @@ -117,9 +115,7 @@ defmodule CadetWeb.AdminAssetsController do parameters do folderName(:path, :string, "Folder name", required: true) - fileName(:path, :string, "File path in folder, which may contain subfolders", - required: true - ) + fileName(:path, :string, "File path in folder, which may contain subfolders", required: true) end security([%{JWT: []}]) diff --git a/lib/cadet_web/admin_controllers/admin_grading_controller.ex b/lib/cadet_web/admin_controllers/admin_grading_controller.ex index efcd01755..7cc2fef87 100644 --- a/lib/cadet_web/admin_controllers/admin_grading_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_grading_controller.ex @@ -301,9 +301,7 @@ defmodule CadetWeb.AdminGradingController do required: true ) - student(Schema.ref(:StudentInfo), "Student who created the submission", - required: true - ) + student(Schema.ref(:StudentInfo), "Student who created the submission", required: true) unsubmittedBy(Schema.ref(:GraderInfo)) unsubmittedAt(:string, "Last unsubmitted at", format: "date-time", required: false) From af683fd28b78e50a3d89d27805342edea57e3ecc Mon Sep 17 00:00:00 2001 From: YaleChen299 Date: Mon, 24 Jul 2023 14:20:17 +0800 Subject: [PATCH 08/74] fix credo long line --- lib/cadet/assessments/assessments.ex | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 881982022..d6aae9966 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -249,7 +249,8 @@ defmodule Cadet.Assessments do Answer |> join(:inner, [a], s in assoc(a, :submission)) |> where([_, s], s.student_id == ^course_reg.id) - # change a.xp and a.xp_adjustment to 0 if a.is_grading_published is false and return all answers + # change a.xp and a.xp_adjustment to 0 + # if a.is_grading_published is false and return all answers |> select([a, s], %{ a | xp: fragment("CASE WHEN ? THEN ? ELSE 0 END", s.is_grading_published, a.xp), @@ -911,7 +912,8 @@ defmodule Cadet.Assessments do end end - # This function changes the is_grading_published field of the submission to true only if all the answers are graded + # This function changes the is_grading_published field of the submission to true + # only if all the answers are graded # and sends a notification to the student def publish_grading(submission_id, cr = %CourseRegistration{id: course_reg_id, role: role}) when is_ecto_id(submission_id) do From cf98cffeb8f3d711c0c55261cd30827f92c6a68f Mon Sep 17 00:00:00 2001 From: YaleChen299 Date: Mon, 24 Jul 2023 14:22:08 +0800 Subject: [PATCH 09/74] fix credo nesting alias --- lib/cadet/assessments/assessments.ex | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index d6aae9966..c0254e5cf 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -835,7 +835,7 @@ defmodule Cadet.Assessments do end) |> Repo.transaction() - Cadet.Accounts.Notifications.handle_unsubmit_notifications( + Notifications.handle_unsubmit_notifications( submission.assessment.id, Repo.get(CourseRegistration, submission.student_id) ) @@ -891,7 +891,7 @@ defmodule Cadet.Assessments do ) |> Repo.transaction() - Cadet.Accounts.Notifications.handle_unpublish_grades_notifications( + Notifications.handle_unpublish_grades_notifications( submission.assessment.id, Repo.get(CourseRegistration, submission.student_id) ) @@ -943,7 +943,7 @@ defmodule Cadet.Assessments do ) |> Repo.transaction() - Cadet.Accounts.Notifications.write_notification_when_graded( + Notifications.write_notification_when_graded( submission.id, :graded ) From 568cc6ff38953106e39b590028b3151e653214fe Mon Sep 17 00:00:00 2001 From: emptygx Date: Tue, 25 Jul 2023 16:03:02 +0800 Subject: [PATCH 10/74] Fix formatting and code quality --- lib/cadet/accounts/notification_type.ex | 2 +- lib/cadet/accounts/notifications.ex | 2 +- lib/cadet/assessments/assessments.ex | 42 ++++++------------------- 3 files changed, 12 insertions(+), 34 deletions(-) diff --git a/lib/cadet/accounts/notification_type.ex b/lib/cadet/accounts/notification_type.ex index 990e78efb..6e80266be 100644 --- a/lib/cadet/accounts/notification_type.ex +++ b/lib/cadet/accounts/notification_type.ex @@ -12,5 +12,5 @@ defenum(Cadet.Accounts.NotificationType, :notification_type, [ # Notifications for submitted assessments :submitted, # Notifications for unpublished grades - :unpublishedGrading + :unpublished_grading ]) diff --git a/lib/cadet/accounts/notifications.ex b/lib/cadet/accounts/notifications.ex index bab127a97..72ebc1687 100644 --- a/lib/cadet/accounts/notifications.ex +++ b/lib/cadet/accounts/notifications.ex @@ -179,7 +179,7 @@ defmodule Cadet.Accounts.Notifications do |> Repo.delete_all() write(%{ - type: :unpublishedGrading, + type: :unpublished_grading, role: student.role, course_reg_id: student.id, assessment_id: assessment_id diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index c0254e5cf..e2bbd9541 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -305,12 +305,7 @@ defmodule Cadet.Assessments do submission_status = Submission |> where([s], s.student_id == ^cr.id) - |> select([s], [:assessment_id, :status]) - - grading_published_status = - Submission - |> where([s], s.student_id == ^cr.id) - |> select([s], [:assessment_id, :is_grading_published]) + |> select([s], [:assessment_id, :status, :is_grading_published]) assessments = cr.course_id @@ -323,15 +318,12 @@ defmodule Cadet.Assessments do on: a.id == sa.assessment_id ) |> join(:left, [a, _], s in subquery(submission_status), on: a.id == s.assessment_id) - |> join(:left, [a, _, _], gp in subquery(grading_published_status), - on: a.id == gp.assessment_id - ) - |> select([a, sa, s, gp], %{ + |> select([a, sa, s], %{ a - | xp: fragment("CASE WHEN ? THEN ? ELSE 0 END", gp.is_grading_published, sa.xp), + | xp: fragment("CASE WHEN ? THEN ? ELSE 0 END", s.is_grading_published, sa.xp), graded_count: sa.graded_count, user_status: s.status, - is_grading_published: gp.is_grading_published + is_grading_published: s.is_grading_published }) |> filter_published_assessments(cr) |> order_by(:open_at) @@ -880,16 +872,9 @@ defmodule Cadet.Assessments do {:allowed_to_unpublish?, role == :admin or bypass or Cadet.Accounts.Query.avenger_of?(cr, submission.student_id)} do - Multi.new() - |> Multi.run( - :unpublish_grading, - fn _repo, _ -> - submission - |> Submission.changeset(%{is_grading_published: false}) - |> Repo.update() - end - ) - |> Repo.transaction() + submission + |> Submission.changeset(%{is_grading_published: false}) + |> Repo.update() Notifications.handle_unpublish_grades_notifications( submission.assessment.id, @@ -932,16 +917,9 @@ defmodule Cadet.Assessments do {:allowed_to_publish?, role == :admin or bypass or Cadet.Accounts.Query.avenger_of?(cr, submission.student_id)} do - Multi.new() - |> Multi.run( - :publish_grading, - fn _repo, _ -> - submission - |> Submission.changeset(%{is_grading_published: true}) - |> Repo.update() - end - ) - |> Repo.transaction() + submission + |> Submission.changeset(%{is_grading_published: true}) + |> Repo.update() Notifications.write_notification_when_graded( submission.id, From 4700a49f86f639aec287cbf87b8e2302368f84d9 Mon Sep 17 00:00:00 2001 From: emptygx Date: Fri, 28 Jul 2023 18:30:05 +0800 Subject: [PATCH 11/74] Fix tests --- lib/cadet/accounts/notifications.ex | 2 +- priv/repo/seeds.exs | 3 ++- .../admin_grading_controller_test.exs | 12 ++++++++---- .../controllers/assessments_controller_test.exs | 5 ++++- test/factories/assessments/submission_factory.ex | 3 ++- test/support/seeds.ex | 4 +++- 6 files changed, 20 insertions(+), 9 deletions(-) diff --git a/lib/cadet/accounts/notifications.ex b/lib/cadet/accounts/notifications.ex index 72ebc1687..c9849c862 100644 --- a/lib/cadet/accounts/notifications.ex +++ b/lib/cadet/accounts/notifications.ex @@ -151,7 +151,7 @@ defmodule Cadet.Accounts.Notifications do Notification |> where(course_reg_id: ^student.id) |> where(assessment_id: ^assessment_id) - |> where([n], n.type in ^[:autograded, :graded, :unpublishedGrading]) + |> where([n], n.type in ^[:autograded, :graded, :unpublished_grading]) |> Repo.delete_all() write(%{ diff --git a/priv/repo/seeds.exs b/priv/repo/seeds.exs index dee7be580..b4f3469e1 100644 --- a/priv/repo/seeds.exs +++ b/priv/repo/seeds.exs @@ -79,7 +79,8 @@ if Cadet.Env.env() == :dev do &insert(:submission, %{ assessment: assessment, student: &1, - status: Enum.random(SubmissionStatus.__enum_map__()) + status: Enum.random(SubmissionStatus.__enum_map__()), + is_grading_published: false }) ) 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 dfe691e7b..9a1d838b9 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -137,7 +137,8 @@ defmodule CadetWeb.AdminGradingControllerTest do "status" => Atom.to_string(submission.status), "gradedCount" => 5, "unsubmittedBy" => nil, - "unsubmittedAt" => nil + "unsubmittedAt" => nil, + "isGradingPublished" => submission.is_grading_published } end) @@ -200,7 +201,8 @@ defmodule CadetWeb.AdminGradingControllerTest do "status" => Atom.to_string(submission.status), "gradedCount" => 5, "unsubmittedBy" => nil, - "unsubmittedAt" => nil + "unsubmittedAt" => nil, + "isGradingPublished" => submission.is_grading_published } end) @@ -797,7 +799,8 @@ defmodule CadetWeb.AdminGradingControllerTest do "status" => Atom.to_string(submission.status), "gradedCount" => 5, "unsubmittedBy" => nil, - "unsubmittedAt" => nil + "unsubmittedAt" => nil, + "isGradingPublished" => submission.is_grading_published } end) @@ -840,7 +843,8 @@ defmodule CadetWeb.AdminGradingControllerTest do "status" => Atom.to_string(submission.status), "gradedCount" => 5, "unsubmittedBy" => nil, - "unsubmittedAt" => nil + "unsubmittedAt" => nil, + "isGradingPublished" => submission.is_grading_published } end) diff --git a/test/cadet_web/controllers/assessments_controller_test.exs b/test/cadet_web/controllers/assessments_controller_test.exs index 2cd89434b..d91ecb8a8 100644 --- a/test/cadet_web/controllers/assessments_controller_test.exs +++ b/test/cadet_web/controllers/assessments_controller_test.exs @@ -88,6 +88,7 @@ defmodule CadetWeb.AssessmentsControllerTest do |> get(build_url(course1.id)) |> json_response(200) |> Enum.map(&Map.delete(&1, "xp")) + |> Enum.map(&Map.delete(&1, "isGradingPublished")) assert expected == resp end @@ -161,7 +162,8 @@ defmodule CadetWeb.AssessmentsControllerTest do "private" => false, "isPublished" => &1.is_published, "gradedCount" => 0, - "questionCount" => 9 + "questionCount" => 9, + "isGradingPublished" => true } ) @@ -271,6 +273,7 @@ defmodule CadetWeb.AssessmentsControllerTest do "private" => false, "gradedCount" => 0, "questionCount" => 9, + "isGradingPublished" => nil, "isPublished" => if &1.config.type == hd(configs).type do false diff --git a/test/factories/assessments/submission_factory.ex b/test/factories/assessments/submission_factory.ex index 971345a93..db2c58499 100644 --- a/test/factories/assessments/submission_factory.ex +++ b/test/factories/assessments/submission_factory.ex @@ -10,7 +10,8 @@ defmodule Cadet.Assessments.SubmissionFactory do def submission_factory do %Submission{ student: build(:course_registration, %{role: :student}), - assessment: build(:assessment) + assessment: build(:assessment), + is_grading_published: true } end end diff --git a/test/support/seeds.ex b/test/support/seeds.ex index e88ec7f21..8d9c6fbad 100644 --- a/test/support/seeds.ex +++ b/test/support/seeds.ex @@ -180,7 +180,9 @@ defmodule Cadet.Test.Seeds do submissions = students |> Enum.take(2) - |> Enum.map(&insert(:submission, %{assessment: assessment, student: &1})) + |> Enum.map( + &insert(:submission, %{assessment: assessment, student: &1, is_grading_published: true}) + ) # Programming Answers programming_answers = From 750ba954237456718bd64cdb4bfe85354fd3fb15 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 1 Mar 2024 17:10:39 +0800 Subject: [PATCH 12/74] feat: Implement format helper function Changing xp in the assessment_with_questions_and_answers is the wrong direction, I feel that the function should not be tied to a specific format. Instead, I will call a helper function in the controller to format the xp. --- lib/cadet/assessments/assessments.ex | 40 ++++++++++++++----- .../controllers/assessments_controller.ex | 8 +++- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index e9a639490..7f5c2fafd 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -265,14 +265,6 @@ defmodule Cadet.Assessments do Answer |> join(:inner, [a], s in assoc(a, :submission)) |> where([_, s], s.student_id == ^course_reg.id) - # change a.xp and a.xp_adjustment to 0 - # if a.is_grading_published is false and return all answers - |> select([a, s], %{ - a - | xp: fragment("CASE WHEN ? THEN ? ELSE 0 END", s.is_grading_published, a.xp), - xp_adjustment: - fragment("CASE WHEN ? THEN ? ELSE 0 END", s.is_grading_published, a.xp_adjustment) - }) questions = Question @@ -336,7 +328,7 @@ defmodule Cadet.Assessments do |> join(:left, [a, _], s in subquery(submission_status), on: a.id == s.assessment_id) |> select([a, sa, s], %{ a - | xp: fragment("CASE WHEN ? THEN ? ELSE 0 END", s.is_grading_published, sa.xp), + | xp: sa.xp, graded_count: sa.graded_count, user_status: s.status, is_grading_published: s.is_grading_published @@ -349,6 +341,36 @@ defmodule Cadet.Assessments do {:ok, assessments} end + @doc """ + A helper function for which removes grading information from the assessment + if it's grading is not published. + """ + def format_assessment_with_questions_and_answers(assessment) do + if assessment.is_grading_published do + assessment + else + %{ + assessment + | questions: + Enum.map(assessment.questions, fn q -> + %{ + q + | answer: %{ + q.answer + | xp: 0, + xp_adjustment: 0, + autograding_status: :none, + autograding_results: [], + grader: nil, + grader_id: nil, + comments: nil + } + } + end) + } + end + end + def filter_published_assessments(assessments, cr) do role = cr.role diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex index 5fb0659ec..4a6aa65c8 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -48,8 +48,12 @@ defmodule CadetWeb.AssessmentsController do cr = conn.assigns.course_reg case Assessments.assessment_with_questions_and_answers(assessment_id, cr) do - {:ok, assessment} -> render(conn, "show.json", assessment: assessment) - {:error, {status, message}} -> send_resp(conn, status, message) + {:ok, assessment} -> + assessment = Assessments.format_assessment_with_questions_and_answers(assessment) + render(conn, "show.json", assessment: assessment) + + {:error, {status, message}} -> + send_resp(conn, status, message) end end From f82e43fce131582d1987ea1616c6286a3a19c9c4 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 1 Mar 2024 20:02:16 +0800 Subject: [PATCH 13/74] feat: Add formatting function for getting all assessments --- lib/cadet/assessments/assessments.ex | 17 ++++++++++++++++- .../controllers/assessments_controller.ex | 2 +- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 7f5c2fafd..944ac0262 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -342,7 +342,22 @@ defmodule Cadet.Assessments do end @doc """ - A helper function for which removes grading information from the assessment + A helper function which removes grading information from all assessments + if it's grading is not published. + """ + def format_all_assessments(assessments) do + Enum.map(assessments, fn a -> + if a.is_grading_published do + a + else + a + |> Map.put(:xp, 0) + |> Map.put(:graded_count, 0) + end + end) + end + @doc """ + A helper function which removes grading information from the assessment if it's grading is not published. """ def format_assessment_with_questions_and_answers(assessment) do diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex index 4a6aa65c8..fa7faa939 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -40,7 +40,7 @@ defmodule CadetWeb.AssessmentsController do def index(conn, _) do cr = conn.assigns.course_reg {:ok, assessments} = Assessments.all_assessments(cr) - + assessments = Assessments.format_all_assessments(assessments) render(conn, "index.json", assessments: assessments) end From e7090f85481a5987cefd48e1f94eced690d8ba65 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 1 Mar 2024 20:02:35 +0800 Subject: [PATCH 14/74] fix: Fix bug of using virtual variable instead --- lib/cadet/assessments/assessments.ex | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 944ac0262..936756562 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -282,7 +282,13 @@ defmodule Cadet.Assessments do end) |> load_contest_voting_entries(course_reg, assessment) - assessment = assessment |> Map.put(:questions, questions) + is_grading_published = + Submission + |> where(assessment_id: ^id) + |> where(student_id: ^course_reg.id) + |> select([s], s.is_grading_published) + |> Repo.one() + assessment = assessment |> Map.put(:questions, questions) |> Map.put(:is_grading_published, is_grading_published) {:ok, assessment} else {:error, {:forbidden, "Assessment not open"}} From 5c1b15503480605bd417f7897844caa014544a38 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 1 Mar 2024 21:39:46 +0800 Subject: [PATCH 15/74] refactor: Remove default value for virtual variable is_grading_published --- lib/cadet/assessments/assessment.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cadet/assessments/assessment.ex b/lib/cadet/assessments/assessment.ex index 59a7db6ef..e49042178 100644 --- a/lib/cadet/assessments/assessment.ex +++ b/lib/cadet/assessments/assessment.ex @@ -20,7 +20,7 @@ defmodule Cadet.Assessments.Assessment do field(:grading_status, :string, virtual: true) field(:question_count, :integer, virtual: true) field(:graded_count, :integer, virtual: true) - field(:is_grading_published, :boolean, virtual: true, default: false) + field(:is_grading_published, :boolean, virtual: true) field(:title, :string) field(:is_published, :boolean, default: false) field(:summary_short, :string) From 788e31e0c430391ddc92aa444979fcbd3a1997b2 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sat, 2 Mar 2024 11:19:48 +0800 Subject: [PATCH 16/74] feat: Implement tests for helper functions --- test/cadet/assessments/assessments_test.exs | 101 ++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index 3575768ff..37b098bb0 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -1700,6 +1700,107 @@ defmodule Cadet.AssessmentsTest do end end + describe "grading published feature" do + + setup do + course = insert(:course) + config = insert(:assessment_config, %{type: "Test", course: course}) + student = insert(:course_registration, course: course, role: :student) + student_2 = insert(:course_registration, course: course, role: :student) + avenger = insert(:course_registration, course: course, role: :staff) + assessment = + insert( + :assessment, + open_at: Timex.shift(Timex.now(), hours: -1), + close_at: Timex.shift(Timex.now(), hours: 500), + is_published: true, + config: config, + course: course + ) + + question = insert(:mcq_question, assessment: assessment) + + submission = + insert(:submission, assessment: assessment, student: student, status: :attempted, is_grading_published: false) + + answer = insert( + :answer, + submission: submission, + question: question, + answer: %{:choice_id => 1}, + xp: 400, + xp_adjustment: 300, + comments: "Dummy Comment", + autograding_status: :failed, + autograding_results: [ + %{ + errors: [ + %{ + error_message: "DummyError", + error_type: "systemError" + } + ], + result_type: "error" + } + ], + grader: avenger + ) + + published_submission = insert(:submission, assessment: assessment, student: student_2, status: :submitted, is_grading_published: true) + published_answer = insert( + :answer, + submission: published_submission, + question: question, + answer: %{:choice_id => 1}, + xp: 400, + xp_adjustment: 300, + comments: "Dummy Comment", + autograding_status: :failed, + autograding_results: [ + %{ + errors: [ + %{ + error_message: "DummyError", + error_type: "systemError" + } + ], + result_type: "error" + } + ], + grader: avenger + ) + %{assessment: assessment, student: student, student_2: student_2} + end + + test "unpublished grades are hidden", %{assessment: assessment, student: student} do + {_, assessment_with_q_and_a} = Assessments.assessment_with_questions_and_answers(assessment, student) + formatted_assessment = Assessments.format_assessment_with_questions_and_answers(assessment_with_q_and_a) + formatted_answer = (hd(formatted_assessment.questions).answer) + + assert formatted_answer.xp == 0 + assert formatted_answer.xp_adjustment == 0 + assert formatted_answer.autograding_status == :none + assert formatted_answer.autograding_results == [] + assert formatted_answer.grader == nil + assert formatted_answer.grader_id == nil + assert formatted_answer.comments == nil + end + + test "published grades are shown", %{assessment: assessment, student_2: student} do + + {_, assessment_with_q_and_a} = Assessments.assessment_with_questions_and_answers(assessment, student) + formatted_assessment = Assessments.format_assessment_with_questions_and_answers(assessment_with_q_and_a) + formatted_answer = (hd(formatted_assessment.questions).answer) + + assert formatted_answer.xp != 0 + assert formatted_answer.xp_adjustment != 0 + assert formatted_answer.autograding_status != :none + assert formatted_answer.autograding_results != [] + assert formatted_answer.grader != nil + assert formatted_answer.grader_id != nil + assert formatted_answer.comments != nil + end + end defp get_answer_relative_scores(answers) do answers |> Enum.map(fn ans -> ans.relative_score end) end From 70ebac5e38200d8dfa9f9dfca9c7c97a570471ca Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sat, 2 Mar 2024 11:20:47 +0800 Subject: [PATCH 17/74] feat: Add isGradingPublished to response for fetching all assessments --- lib/cadet_web/admin_views/admin_grading_view.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/cadet_web/admin_views/admin_grading_view.ex b/lib/cadet_web/admin_views/admin_grading_view.ex index e6a9089af..2685f8ec4 100644 --- a/lib/cadet_web/admin_views/admin_grading_view.ex +++ b/lib/cadet_web/admin_views/admin_grading_view.ex @@ -71,6 +71,7 @@ defmodule CadetWeb.AdminGradingView do xp: :xp, xpAdjustment: :xp_adjustment, xpBonus: :xp_bonus, + isGradingPublished: :is_grading_published, gradedCount: &case &1.graded_count do nil -> 0 From 54cbcfc042a064e7772afd28c0e755e39c4fc092 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sat, 2 Mar 2024 11:23:08 +0800 Subject: [PATCH 18/74] refactor: Format --- lib/cadet/assessments/assessments.ex | 8 +- test/cadet/assessments/assessments_test.exs | 83 +++++++++++++-------- 2 files changed, 60 insertions(+), 31 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 936756562..f280e2f99 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -288,7 +288,12 @@ defmodule Cadet.Assessments do |> where(student_id: ^course_reg.id) |> select([s], s.is_grading_published) |> Repo.one() - assessment = assessment |> Map.put(:questions, questions) |> Map.put(:is_grading_published, is_grading_published) + + assessment = + assessment + |> Map.put(:questions, questions) + |> Map.put(:is_grading_published, is_grading_published) + {:ok, assessment} else {:error, {:forbidden, "Assessment not open"}} @@ -362,6 +367,7 @@ defmodule Cadet.Assessments do end end) end + @doc """ A helper function which removes grading information from the assessment if it's grading is not published. diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index 37b098bb0..5bd26f9d5 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -1701,29 +1701,35 @@ defmodule Cadet.AssessmentsTest do end describe "grading published feature" do - setup do - course = insert(:course) - config = insert(:assessment_config, %{type: "Test", course: course}) - student = insert(:course_registration, course: course, role: :student) - student_2 = insert(:course_registration, course: course, role: :student) - avenger = insert(:course_registration, course: course, role: :staff) - assessment = - insert( - :assessment, - open_at: Timex.shift(Timex.now(), hours: -1), - close_at: Timex.shift(Timex.now(), hours: 500), - is_published: true, - config: config, - course: course - ) + course = insert(:course) + config = insert(:assessment_config, %{type: "Test", course: course}) + student = insert(:course_registration, course: course, role: :student) + student_2 = insert(:course_registration, course: course, role: :student) + avenger = insert(:course_registration, course: course, role: :staff) - question = insert(:mcq_question, assessment: assessment) + assessment = + insert( + :assessment, + open_at: Timex.shift(Timex.now(), hours: -1), + close_at: Timex.shift(Timex.now(), hours: 500), + is_published: true, + config: config, + course: course + ) - submission = - insert(:submission, assessment: assessment, student: student, status: :attempted, is_grading_published: false) + question = insert(:mcq_question, assessment: assessment) - answer = insert( + submission = + insert(:submission, + assessment: assessment, + student: student, + status: :attempted, + is_grading_published: false + ) + + answer = + insert( :answer, submission: submission, question: question, @@ -1743,11 +1749,19 @@ defmodule Cadet.AssessmentsTest do result_type: "error" } ], - grader: avenger + grader: avenger + ) + + published_submission = + insert(:submission, + assessment: assessment, + student: student_2, + status: :submitted, + is_grading_published: true ) - published_submission = insert(:submission, assessment: assessment, student: student_2, status: :submitted, is_grading_published: true) - published_answer = insert( + published_answer = + insert( :answer, submission: published_submission, question: question, @@ -1767,15 +1781,20 @@ defmodule Cadet.AssessmentsTest do result_type: "error" } ], - grader: avenger + grader: avenger ) - %{assessment: assessment, student: student, student_2: student_2} + + %{assessment: assessment, student: student, student_2: student_2} end test "unpublished grades are hidden", %{assessment: assessment, student: student} do - {_, assessment_with_q_and_a} = Assessments.assessment_with_questions_and_answers(assessment, student) - formatted_assessment = Assessments.format_assessment_with_questions_and_answers(assessment_with_q_and_a) - formatted_answer = (hd(formatted_assessment.questions).answer) + {_, assessment_with_q_and_a} = + Assessments.assessment_with_questions_and_answers(assessment, student) + + formatted_assessment = + Assessments.format_assessment_with_questions_and_answers(assessment_with_q_and_a) + + formatted_answer = hd(formatted_assessment.questions).answer assert formatted_answer.xp == 0 assert formatted_answer.xp_adjustment == 0 @@ -1787,10 +1806,13 @@ defmodule Cadet.AssessmentsTest do end test "published grades are shown", %{assessment: assessment, student_2: student} do + {_, assessment_with_q_and_a} = + Assessments.assessment_with_questions_and_answers(assessment, student) + + formatted_assessment = + Assessments.format_assessment_with_questions_and_answers(assessment_with_q_and_a) - {_, assessment_with_q_and_a} = Assessments.assessment_with_questions_and_answers(assessment, student) - formatted_assessment = Assessments.format_assessment_with_questions_and_answers(assessment_with_q_and_a) - formatted_answer = (hd(formatted_assessment.questions).answer) + formatted_answer = hd(formatted_assessment.questions).answer assert formatted_answer.xp != 0 assert formatted_answer.xp_adjustment != 0 @@ -1801,6 +1823,7 @@ defmodule Cadet.AssessmentsTest do assert formatted_answer.comments != nil end end + defp get_answer_relative_scores(answers) do answers |> Enum.map(fn ans -> ans.relative_score end) end From cf62185a71094ad98c659c9b3545ccf54a842483 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Mon, 4 Mar 2024 16:59:21 +0800 Subject: [PATCH 19/74] chore: Remove unused match --- lib/cadet/assessments/assessments.ex | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index f280e2f99..81ba1d575 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1011,11 +1011,8 @@ defmodule Cadet.Assessments do {:submission_found?, false} -> {:error, {:not_found, "Submission not found"}} - {:is_open?, false} -> - {:error, {:forbidden, "Assessment not open"}} - {:allowed_to_unpublish?, false} -> - {:error, {:forbidden, "Only Avenger of student is permitted to unpublish grading"}} + {:error, {:forbidden, "Only Avenger of student or Admin is permitted to unpublish grading"}} _ -> {:error, {:internal_server_error, "Please try again later."}} @@ -1056,9 +1053,6 @@ defmodule Cadet.Assessments do {:submission_found?, false} -> {:error, {:not_found, "Submission not found"}} - {:is_open?, false} -> - {:error, {:forbidden, "Assessment not open"}} - {:status, :attempting} -> {:error, {:bad_request, "Some questions have not been attempted"}} From 6b5f0e83c73c9286d3cb68338846b0be78732899 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Mon, 4 Mar 2024 17:08:51 +0800 Subject: [PATCH 20/74] feat: Implement tests for unpublish route --- .../admin_grading_controller_test.exs | 120 ++++++++++++++++++ 1 file changed, 120 insertions(+) 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 1b6f3ebac..8a9f75c8e 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -796,6 +796,123 @@ defmodule CadetWeb.AdminGradingControllerTest do end end + describe "POST /:submissionid/unpublish_grades" do + @tag authenticate: :staff + test "succeeds", %{conn: conn} do + %{course: course, config: config, grader: grader, students: students} = seed_db(conn) + + assessment = + insert( + :assessment, + open_at: Timex.shift(Timex.now(), hours: -1), + close_at: Timex.shift(Timex.now(), hours: 500), + is_published: true, + config: config, + course: course + ) + + question = insert(:programming_question, assessment: assessment) + student = List.first(students) + + submission = + insert(:submission, assessment: assessment, student: student, status: :submitted, is_grading_published: true) + answer = + insert( + :answer, + submission: submission, + question: question, + answer: %{code: "f => f(f);"}, + grader_id: grader.id + ) + + conn + |> post(build_url_unpublish(course.id, submission.id)) + |> response(200) + + submission_db = Repo.get(Submission, submission.id) + + assert submission_db.is_grading_published == false + end + + @tag authenticate: :staff + test "avenger should not be allowed to unpublish for students outside of their group", %{ + conn: conn + } do + %{course: course, config: config, students: students} = seed_db(conn) + + assessment = + insert( + :assessment, + open_at: Timex.shift(Timex.now(), hours: -1), + close_at: Timex.shift(Timex.now(), hours: 500), + is_published: true, + course: course, + config: config + ) + + other_grader = insert(:course_registration, %{role: :staff, course: course}) + question = insert(:programming_question, assessment: assessment) + student = List.first(students) + + submission = + insert(:submission, assessment: assessment, student: student, status: :submitted) + + insert( + :answer, + submission: submission, + question: question, + answer: %{code: "f => f(f);"} + ) + + conn = + conn + |> sign_in(other_grader.user) + |> post(build_url_unpublish(course.id, submission.id)) + + assert response(conn, 403) =~ "Only Avenger of student or Admin is permitted to unpublish grading" + end + + @tag authenticate: :admin + test "admin should be allowed to unpublish", %{ + conn: conn + } do + %{course: course, config: config, students: students} = seed_db(conn) + + admin = insert(:course_registration, %{role: :admin, course: course}) + + assessment = + insert( + :assessment, + open_at: Timex.shift(Timex.now(), hours: -1), + close_at: Timex.shift(Timex.now(), hours: 500), + is_published: true, + course: course, + config: config + ) + + question = insert(:programming_question, assessment: assessment) + student = List.first(students) + + submission = + insert(:submission, assessment: assessment, student: student, status: :submitted, is_grading_published: true) + + answer = + insert( + :answer, + submission: submission, + question: question, + answer: %{code: "f => f(f);"} + ) + + conn + |> sign_in(admin.user) + |> post(build_url_unpublish(course.id, submission.id)) + + submission_db = Repo.get(Submission, submission.id) + assert submission_db.is_grading_published == false + end + end + describe "GET /, admin" do @tag authenticate: :staff test "can see all submissions", %{conn: conn} do @@ -1314,6 +1431,9 @@ defmodule CadetWeb.AdminGradingControllerTest do defp build_url_unsubmit(course_id, submission_id), do: "#{build_url(course_id, submission_id)}/unsubmit" + defp build_url_unpublish(course_id, submission_id), + do: "#{build_url(course_id, submission_id)}/unpublish_grades" + defp build_url_autograde(course_id, submission_id), do: "#{build_url(course_id, submission_id)}/autograde" From 2a055974a9415ceeb23000f7425dcad319d50839 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Mon, 4 Mar 2024 17:40:26 +0800 Subject: [PATCH 21/74] feat: Implement tests for publish route --- .../admin_grading_controller_test.exs | 186 +++++++++++++++++- 1 file changed, 183 insertions(+), 3 deletions(-) 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 8a9f75c8e..74a5057d6 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -815,7 +815,13 @@ defmodule CadetWeb.AdminGradingControllerTest do student = List.first(students) submission = - insert(:submission, assessment: assessment, student: student, status: :submitted, is_grading_published: true) + insert(:submission, + assessment: assessment, + student: student, + status: :submitted, + is_grading_published: true + ) + answer = insert( :answer, @@ -869,7 +875,8 @@ defmodule CadetWeb.AdminGradingControllerTest do |> sign_in(other_grader.user) |> post(build_url_unpublish(course.id, submission.id)) - assert response(conn, 403) =~ "Only Avenger of student or Admin is permitted to unpublish grading" + assert response(conn, 403) =~ + "Only Avenger of student or Admin is permitted to unpublish grading" end @tag authenticate: :admin @@ -894,7 +901,12 @@ defmodule CadetWeb.AdminGradingControllerTest do student = List.first(students) submission = - insert(:submission, assessment: assessment, student: student, status: :submitted, is_grading_published: true) + insert(:submission, + assessment: assessment, + student: student, + status: :submitted, + is_grading_published: true + ) answer = insert( @@ -913,6 +925,171 @@ defmodule CadetWeb.AdminGradingControllerTest do end end + describe "POST /:submissionid/publish_grades" do + @tag authenticate: :staff + test "succeeds", %{conn: conn} do + %{course: course, config: config, grader: grader, students: students} = seed_db(conn) + + assessment = + insert( + :assessment, + open_at: Timex.shift(Timex.now(), hours: -1), + close_at: Timex.shift(Timex.now(), hours: 500), + is_published: true, + config: config, + course: course + ) + + question = insert(:programming_question, assessment: assessment) + student = List.first(students) + + submission = + insert(:submission, + assessment: assessment, + student: student, + status: :submitted, + is_grading_published: false + ) + + answer = + insert( + :answer, + submission: submission, + question: question, + answer: %{code: "f => f(f);"}, + grader_id: grader.id + ) + + conn + |> post(build_url_publish(course.id, submission.id)) + |> response(200) + + submission_db = Repo.get(Submission, submission.id) + + assert submission_db.is_grading_published == true + end + + @tag authenticate: :staff + test "assessments which have not been submitted should not be allowed to publish", %{ + conn: conn + } do + %{course: course, config: config, students: students} = seed_db(conn) + + assessment = + insert( + :assessment, + open_at: Timex.shift(Timex.now(), hours: -1), + close_at: Timex.shift(Timex.now(), hours: 500), + is_published: true, + config: config, + course: course + ) + + question = insert(:programming_question, assessment: assessment) + student = List.first(students) + + submission = + insert(:submission, assessment: assessment, student: student, status: :attempted) + + insert( + :answer, + submission: submission, + question: question, + answer: %{code: "f => f(f);"} + ) + + conn = + conn + |> post(build_url_publish(course.id, submission.id)) + + assert response(conn, 400) =~ "Assessment has not been submitted" + end + + @tag authenticate: :staff + test "avenger should not be allowed to publish for students outside of their group", %{ + conn: conn + } do + %{course: course, config: config, students: students} = seed_db(conn) + + assessment = + insert( + :assessment, + open_at: Timex.shift(Timex.now(), hours: -1), + close_at: Timex.shift(Timex.now(), hours: 500), + is_published: true, + course: course, + config: config + ) + + other_grader = insert(:course_registration, %{role: :staff, course: course}) + question = insert(:programming_question, assessment: assessment) + student = List.first(students) + + submission = + insert(:submission, assessment: assessment, student: student, status: :submitted) + + insert( + :answer, + submission: submission, + question: question, + answer: %{code: "f => f(f);"} + ) + + conn = + conn + |> sign_in(other_grader.user) + |> post(build_url_publish(course.id, submission.id)) + + assert response(conn, 403) =~ + "Only Avenger of student or Admin is permitted to publish grading" + end + + @tag authenticate: :admin + test "admin should be allowed to publish", %{ + conn: conn + } do + %{course: course, config: config, students: students} = seed_db(conn) + + admin = insert(:course_registration, %{role: :admin, course: course}) + + assessment = + insert( + :assessment, + open_at: Timex.shift(Timex.now(), hours: -1), + close_at: Timex.shift(Timex.now(), hours: 500), + is_published: true, + course: course, + config: config + ) + + question = insert(:programming_question, assessment: assessment) + student = List.first(students) + + submission = + insert(:submission, + assessment: assessment, + student: student, + status: :submitted, + is_grading_published: false + ) + + answer = + insert( + :answer, + submission: submission, + question: question, + answer: %{code: "f => f(f);"} + ) + + conn + |> sign_in(admin.user) + |> post(build_url_publish(course.id, submission.id)) + + submission_db = Repo.get(Submission, submission.id) + assert submission_db.is_grading_published == true + end + end + describe "GET /, admin" do @tag authenticate: :staff test "can see all submissions", %{conn: conn} do @@ -1434,6 +1611,9 @@ defmodule CadetWeb.AdminGradingControllerTest do defp build_url_unpublish(course_id, submission_id), do: "#{build_url(course_id, submission_id)}/unpublish_grades" + defp build_url_publish(course_id, submission_id), + do: "#{build_url(course_id, submission_id)}/publish_grades" + defp build_url_autograde(course_id, submission_id), do: "#{build_url(course_id, submission_id)}/autograde" From 9934af43185172f093bade52863a32c68c789690 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Mon, 4 Mar 2024 17:57:41 +0800 Subject: [PATCH 22/74] refactor: Move repeated code into setup for publish test --- .../admin_grading_controller_test.exs | 123 +++++------------- 1 file changed, 29 insertions(+), 94 deletions(-) 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 74a5057d6..2d602ab86 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -926,8 +926,7 @@ defmodule CadetWeb.AdminGradingControllerTest do end describe "POST /:submissionid/publish_grades" do - @tag authenticate: :staff - test "succeeds", %{conn: conn} do + setup %{conn: conn} do %{course: course, config: config, grader: grader, students: students} = seed_db(conn) assessment = @@ -940,25 +939,32 @@ defmodule CadetWeb.AdminGradingControllerTest do course: course ) - question = insert(:programming_question, assessment: assessment) - student = List.first(students) + question = insert(:programming_question, assessment: assessment) - submission = - insert(:submission, - assessment: assessment, - student: student, - status: :submitted, - is_grading_published: false - ) + student = List.first(students) - answer = - insert( - :answer, - submission: submission, - question: question, - answer: %{code: "f => f(f);"}, - grader_id: grader.id - ) + submission = + insert(:submission, + assessment: assessment, + student: student, + status: :submitted, + is_grading_published: false + ) + + answer = + insert( + :answer, + submission: submission, + question: question, + answer: %{code: "f => f(f);"}, + grader_id: grader.id + ) + + %{course: course, assessment: assessment, submission: submission, question: question, students: students} + end + + @tag authenticate: :staff + test "succeeds", %{conn: conn, course: course, submission: submission} do conn |> post(build_url_publish(course.id, submission.id)) @@ -970,23 +976,9 @@ defmodule CadetWeb.AdminGradingControllerTest do end @tag authenticate: :staff - test "assessments which have not been submitted should not be allowed to publish", %{ - conn: conn - } do - %{course: course, config: config, students: students} = seed_db(conn) - - assessment = - insert( - :assessment, - open_at: Timex.shift(Timex.now(), hours: -1), - close_at: Timex.shift(Timex.now(), hours: 500), - is_published: true, - config: config, - course: course - ) + test "assessments which have not been submitted should not be allowed to publish", %{conn: conn, course: course, students: students, assessment: assessment, question: question} do - question = insert(:programming_question, assessment: assessment) - student = List.first(students) + student = List.last(students) submission = insert(:submission, assessment: assessment, student: student, status: :attempted) @@ -1006,34 +998,9 @@ defmodule CadetWeb.AdminGradingControllerTest do end @tag authenticate: :staff - test "avenger should not be allowed to publish for students outside of their group", %{ - conn: conn - } do - %{course: course, config: config, students: students} = seed_db(conn) - - assessment = - insert( - :assessment, - open_at: Timex.shift(Timex.now(), hours: -1), - close_at: Timex.shift(Timex.now(), hours: 500), - is_published: true, - course: course, - config: config - ) + test "avenger should not be allowed to publish for students outside of their group", %{conn: conn, course: course, submission: submission} do other_grader = insert(:course_registration, %{role: :staff, course: course}) - question = insert(:programming_question, assessment: assessment) - student = List.first(students) - - submission = - insert(:submission, assessment: assessment, student: student, status: :submitted) - - insert( - :answer, - submission: submission, - question: question, - answer: %{code: "f => f(f);"} - ) conn = conn @@ -1045,42 +1012,10 @@ defmodule CadetWeb.AdminGradingControllerTest do end @tag authenticate: :admin - test "admin should be allowed to publish", %{ - conn: conn - } do - %{course: course, config: config, students: students} = seed_db(conn) + test "admin should be allowed to publish", %{conn: conn, course: course, submission: submission} do admin = insert(:course_registration, %{role: :admin, course: course}) - assessment = - insert( - :assessment, - open_at: Timex.shift(Timex.now(), hours: -1), - close_at: Timex.shift(Timex.now(), hours: 500), - is_published: true, - course: course, - config: config - ) - - question = insert(:programming_question, assessment: assessment) - student = List.first(students) - - submission = - insert(:submission, - assessment: assessment, - student: student, - status: :submitted, - is_grading_published: false - ) - - answer = - insert( - :answer, - submission: submission, - question: question, - answer: %{code: "f => f(f);"} - ) - conn |> sign_in(admin.user) |> post(build_url_publish(course.id, submission.id)) From 1097e23c0aa5813495f08b18234b4c04cc60e9ed Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Mon, 4 Mar 2024 18:00:13 +0800 Subject: [PATCH 23/74] refactor: Move repeated code into setup for unpublish test --- .../admin_grading_controller_test.exs | 104 +++++------------- 1 file changed, 26 insertions(+), 78 deletions(-) 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 2d602ab86..921a9b080 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -797,8 +797,7 @@ defmodule CadetWeb.AdminGradingControllerTest do end describe "POST /:submissionid/unpublish_grades" do - @tag authenticate: :staff - test "succeeds", %{conn: conn} do + setup %{conn: conn} do %{course: course, config: config, grader: grader, students: students} = seed_db(conn) assessment = @@ -811,25 +810,31 @@ defmodule CadetWeb.AdminGradingControllerTest do course: course ) - question = insert(:programming_question, assessment: assessment) - student = List.first(students) + question = insert(:programming_question, assessment: assessment) - submission = - insert(:submission, - assessment: assessment, - student: student, - status: :submitted, - is_grading_published: true - ) + student = List.first(students) - answer = - insert( - :answer, - submission: submission, - question: question, - answer: %{code: "f => f(f);"}, - grader_id: grader.id - ) + submission = + insert(:submission, + assessment: assessment, + student: student, + status: :submitted, + is_grading_published: true + ) + + answer = + insert( + :answer, + submission: submission, + question: question, + answer: %{code: "f => f(f);"}, + grader_id: grader.id + ) + + %{course: course, assessment: assessment, submission: submission, question: question, students: students} + end + @tag authenticate: :staff + test "succeeds", %{conn: conn, course: course, submission: submission} do conn |> post(build_url_unpublish(course.id, submission.id)) @@ -841,34 +846,9 @@ defmodule CadetWeb.AdminGradingControllerTest do end @tag authenticate: :staff - test "avenger should not be allowed to unpublish for students outside of their group", %{ - conn: conn - } do - %{course: course, config: config, students: students} = seed_db(conn) - - assessment = - insert( - :assessment, - open_at: Timex.shift(Timex.now(), hours: -1), - close_at: Timex.shift(Timex.now(), hours: 500), - is_published: true, - course: course, - config: config - ) + test "avenger should not be allowed to unpublish for students outside of their group", %{conn: conn, course: course, submission: submission} do other_grader = insert(:course_registration, %{role: :staff, course: course}) - question = insert(:programming_question, assessment: assessment) - student = List.first(students) - - submission = - insert(:submission, assessment: assessment, student: student, status: :submitted) - - insert( - :answer, - submission: submission, - question: question, - answer: %{code: "f => f(f);"} - ) conn = conn @@ -880,42 +860,10 @@ defmodule CadetWeb.AdminGradingControllerTest do end @tag authenticate: :admin - test "admin should be allowed to unpublish", %{ - conn: conn - } do - %{course: course, config: config, students: students} = seed_db(conn) + test "admin should be allowed to unpublish", %{conn: conn, course: course, submission: submission} do admin = insert(:course_registration, %{role: :admin, course: course}) - assessment = - insert( - :assessment, - open_at: Timex.shift(Timex.now(), hours: -1), - close_at: Timex.shift(Timex.now(), hours: 500), - is_published: true, - course: course, - config: config - ) - - question = insert(:programming_question, assessment: assessment) - student = List.first(students) - - submission = - insert(:submission, - assessment: assessment, - student: student, - status: :submitted, - is_grading_published: true - ) - - answer = - insert( - :answer, - submission: submission, - question: question, - answer: %{code: "f => f(f);"} - ) - conn |> sign_in(admin.user) |> post(build_url_unpublish(course.id, submission.id)) From 4ce2d6e2e5e5b6cd17831be255b749b8f3778de3 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Mon, 4 Mar 2024 18:01:11 +0800 Subject: [PATCH 24/74] refactor: Format code --- lib/cadet/assessments/assessments.ex | 5 +- .../admin_grading_controller_test.exs | 124 +++++++++++------- 2 files changed, 79 insertions(+), 50 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 81ba1d575..f0f1969e3 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1012,7 +1012,8 @@ defmodule Cadet.Assessments do {:error, {:not_found, "Submission not found"}} {:allowed_to_unpublish?, false} -> - {:error, {:forbidden, "Only Avenger of student or Admin is permitted to unpublish grading"}} + {:error, + {:forbidden, "Only Avenger of student or Admin is permitted to unpublish grading"}} _ -> {:error, {:internal_server_error, "Please try again later."}} @@ -1060,7 +1061,7 @@ defmodule Cadet.Assessments do {:error, {:bad_request, "Assessment has not been submitted"}} {:allowed_to_publish?, false} -> - {:error, {:forbidden, "Only Avenger of student is permitted to publish grading"}} + {:error, {:forbidden, "Only Avenger of student or Admin is permitted to publish grading"}} _ -> {:error, {:internal_server_error, "Please try again later."}} 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 921a9b080..79540fe38 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -810,32 +810,38 @@ defmodule CadetWeb.AdminGradingControllerTest do course: course ) - question = insert(:programming_question, assessment: assessment) + question = insert(:programming_question, assessment: assessment) - student = List.first(students) + student = List.first(students) - submission = - insert(:submission, - assessment: assessment, - student: student, - status: :submitted, - is_grading_published: true - ) + submission = + insert(:submission, + assessment: assessment, + student: student, + status: :submitted, + is_grading_published: true + ) - answer = - insert( - :answer, - submission: submission, - question: question, - answer: %{code: "f => f(f);"}, - grader_id: grader.id - ) + answer = + insert( + :answer, + submission: submission, + question: question, + answer: %{code: "f => f(f);"}, + grader_id: grader.id + ) - %{course: course, assessment: assessment, submission: submission, question: question, students: students} + %{ + course: course, + assessment: assessment, + submission: submission, + question: question, + students: students + } end + @tag authenticate: :staff test "succeeds", %{conn: conn, course: course, submission: submission} do - conn |> post(build_url_unpublish(course.id, submission.id)) |> response(200) @@ -846,8 +852,11 @@ defmodule CadetWeb.AdminGradingControllerTest do end @tag authenticate: :staff - test "avenger should not be allowed to unpublish for students outside of their group", %{conn: conn, course: course, submission: submission} do - + test "avenger should not be allowed to unpublish for students outside of their group", %{ + conn: conn, + course: course, + submission: submission + } do other_grader = insert(:course_registration, %{role: :staff, course: course}) conn = @@ -860,8 +869,11 @@ defmodule CadetWeb.AdminGradingControllerTest do end @tag authenticate: :admin - test "admin should be allowed to unpublish", %{conn: conn, course: course, submission: submission} do - + test "admin should be allowed to unpublish", %{ + conn: conn, + course: course, + submission: submission + } do admin = insert(:course_registration, %{role: :admin, course: course}) conn @@ -887,33 +899,38 @@ defmodule CadetWeb.AdminGradingControllerTest do course: course ) - question = insert(:programming_question, assessment: assessment) + question = insert(:programming_question, assessment: assessment) - student = List.first(students) + student = List.first(students) - submission = - insert(:submission, - assessment: assessment, - student: student, - status: :submitted, - is_grading_published: false - ) + submission = + insert(:submission, + assessment: assessment, + student: student, + status: :submitted, + is_grading_published: false + ) - answer = - insert( - :answer, - submission: submission, - question: question, - answer: %{code: "f => f(f);"}, - grader_id: grader.id - ) + answer = + insert( + :answer, + submission: submission, + question: question, + answer: %{code: "f => f(f);"}, + grader_id: grader.id + ) - %{course: course, assessment: assessment, submission: submission, question: question, students: students} + %{ + course: course, + assessment: assessment, + submission: submission, + question: question, + students: students + } end @tag authenticate: :staff test "succeeds", %{conn: conn, course: course, submission: submission} do - conn |> post(build_url_publish(course.id, submission.id)) |> response(200) @@ -924,8 +941,13 @@ defmodule CadetWeb.AdminGradingControllerTest do end @tag authenticate: :staff - test "assessments which have not been submitted should not be allowed to publish", %{conn: conn, course: course, students: students, assessment: assessment, question: question} do - + test "assessments which have not been submitted should not be allowed to publish", %{ + conn: conn, + course: course, + students: students, + assessment: assessment, + question: question + } do student = List.last(students) submission = @@ -946,8 +968,11 @@ defmodule CadetWeb.AdminGradingControllerTest do end @tag authenticate: :staff - test "avenger should not be allowed to publish for students outside of their group", %{conn: conn, course: course, submission: submission} do - + test "avenger should not be allowed to publish for students outside of their group", %{ + conn: conn, + course: course, + submission: submission + } do other_grader = insert(:course_registration, %{role: :staff, course: course}) conn = @@ -960,8 +985,11 @@ defmodule CadetWeb.AdminGradingControllerTest do end @tag authenticate: :admin - test "admin should be allowed to publish", %{conn: conn, course: course, submission: submission} do - + test "admin should be allowed to publish", %{ + conn: conn, + course: course, + submission: submission + } do admin = insert(:course_registration, %{role: :admin, course: course}) conn From d046b3b4c181f9ffa347fc416a8f8c0153ef43c3 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Thu, 14 Mar 2024 12:20:40 +0800 Subject: [PATCH 25/74] feat: Add a guard to prevent unsubmit when grade is still published --- lib/cadet/assessments/assessments.ex | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 9c79110c3..5530f4a17 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -910,7 +910,8 @@ defmodule Cadet.Assessments do {:allowed_to_unsubmit?, true} <- {:allowed_to_unsubmit?, role == :admin or bypass or - Cadet.Accounts.Query.avenger_of?(cr, submission.student_id)} do + Cadet.Accounts.Query.avenger_of?(cr, submission.student_id)}, + {:is_unpublished?, true} <- {:is_unpublished?, submission.is_grading_published} do Multi.new() |> Multi.run( :rollback_submission, @@ -974,6 +975,9 @@ defmodule Cadet.Assessments do {:allowed_to_unsubmit?, false} -> {:error, {:forbidden, "Only Avenger of student or Admin is permitted to unsubmit"}} + {:is_unpublished?, false} -> + {:error, {:forbidden, "Grading has not been unpublished"}} + _ -> {:error, {:internal_server_error, "Please try again later."}} end From 07f25484588da672c154b890ad2b05c6d31c7d2d Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Thu, 14 Mar 2024 12:24:21 +0800 Subject: [PATCH 26/74] feat: Implement filter by notPublished --- lib/cadet/assessments/assessments.ex | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 5530f4a17..c5e42052b 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1609,6 +1609,9 @@ defmodule Cadet.Assessments do {"notFullyGraded", "true"}, dynamic -> dynamic([ans: ans, asst: asst], ^dynamic and asst.question_count > ans.graded_count) + {"notPublished", "true"}, dynamic -> + dynamic([submission], ^dynamic and not(submission.is_grading_published)) + {_, _}, dynamic -> dynamic end) From 9b33670da69dc74b8126cce9f45de3fdbe2ecb93 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Thu, 14 Mar 2024 12:24:59 +0800 Subject: [PATCH 27/74] refactor: Format --- 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 c5e42052b..f17b0e390 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1610,7 +1610,7 @@ defmodule Cadet.Assessments do dynamic([ans: ans, asst: asst], ^dynamic and asst.question_count > ans.graded_count) {"notPublished", "true"}, dynamic -> - dynamic([submission], ^dynamic and not(submission.is_grading_published)) + dynamic([submission], ^dynamic and not submission.is_grading_published) {_, _}, dynamic -> dynamic From e658166fe813994fc494a0856d2c00a87a859fb3 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 15 Mar 2024 18:54:15 +0800 Subject: [PATCH 28/74] fix: Fix incorrect guard check for is_grading_published --- lib/cadet/assessments/assessments.ex | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index f17b0e390..a2a2831b9 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -911,7 +911,8 @@ defmodule Cadet.Assessments do {:allowed_to_unsubmit?, role == :admin or bypass or Cadet.Accounts.Query.avenger_of?(cr, submission.student_id)}, - {:is_unpublished?, true} <- {:is_unpublished?, submission.is_grading_published} do + {:is_grading_published?, false} <- + {:is_grading_published?, submission.is_grading_published} do Multi.new() |> Multi.run( :rollback_submission, @@ -975,7 +976,7 @@ defmodule Cadet.Assessments do {:allowed_to_unsubmit?, false} -> {:error, {:forbidden, "Only Avenger of student or Admin is permitted to unsubmit"}} - {:is_unpublished?, false} -> + {:is_grading_published?, true} -> {:error, {:forbidden, "Grading has not been unpublished"}} _ -> From 054d89a4a81fb2af35ce441b079700801ba898dd Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 15 Mar 2024 18:55:05 +0800 Subject: [PATCH 29/74] refactor: Edit tests to accommodate new guard --- .../admin_grading_controller_test.exs | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) 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 79540fe38..476496409 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -537,7 +537,12 @@ defmodule CadetWeb.AdminGradingControllerTest do student = List.first(students) submission = - insert(:submission, assessment: assessment, student: student, status: :submitted) + insert(:submission, + assessment: assessment, + student: student, + status: :submitted, + is_grading_published: false + ) answer = insert( @@ -694,7 +699,12 @@ defmodule CadetWeb.AdminGradingControllerTest do question = insert(:programming_question, assessment: assessment) submission = - insert(:submission, assessment: assessment, student: grader, status: :submitted) + insert(:submission, + assessment: assessment, + student: grader, + status: :submitted, + is_grading_published: false + ) insert( :answer, @@ -729,7 +739,12 @@ defmodule CadetWeb.AdminGradingControllerTest do question = insert(:programming_question, assessment: assessment) submission = - insert(:submission, assessment: assessment, student: grader, status: :submitted) + insert(:submission, + assessment: assessment, + student: grader, + status: :submitted, + is_grading_published: false + ) insert( :answer, @@ -767,7 +782,12 @@ defmodule CadetWeb.AdminGradingControllerTest do student = List.first(students) submission = - insert(:submission, assessment: assessment, student: student, status: :submitted) + insert(:submission, + assessment: assessment, + student: student, + status: :submitted, + is_grading_published: false + ) answer = insert( From 2caecd71fe26c54185069dc1096a09ffe03d5784 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 15 Mar 2024 18:55:35 +0800 Subject: [PATCH 30/74] feat: Implement test for new guard (check is_grading_published) --- .../admin_grading_controller_test.exs | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) 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 476496409..82ded4b87 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -608,6 +608,47 @@ defmodule CadetWeb.AdminGradingControllerTest do assert response(conn, 400) =~ "Assessment has not been submitted" end + @tag authenticate: :staff + test "assessments that have not been unpublished should not be allowed to unsubmit", %{ + conn: conn + } do + %{course: course, config: config, students: students} = seed_db(conn) + + assessment = + insert( + :assessment, + open_at: Timex.shift(Timex.now(), hours: -1), + close_at: Timex.shift(Timex.now(), hours: 500), + is_published: true, + config: config, + course: course + ) + + question = insert(:programming_question, assessment: assessment) + student = List.first(students) + + submission = + insert(:submission, + assessment: assessment, + student: student, + status: :submitted, + is_grading_published: true + ) + + insert( + :answer, + submission: submission, + question: question, + answer: %{code: "f => f(f);"} + ) + + conn = + conn + |> post(build_url_unsubmit(course.id, submission.id)) + + assert response(conn, 403) =~ "Grading has not been unpublished" + end + @tag authenticate: :staff test "assessment that is not open anymore cannot be unsubmitted", %{conn: conn} do %{course: course, config: config, students: students} = seed_db(conn) From 1f41b11edbf1772483228baf891f8edf6d7b7f2f Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Thu, 21 Mar 2024 21:43:47 +0800 Subject: [PATCH 31/74] feat: Update seed to include is_grading_published --- test/support/seeds.ex | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/test/support/seeds.ex b/test/support/seeds.ex index ce9f55205..20ab5061b 100644 --- a/test/support/seeds.ex +++ b/test/support/seeds.ex @@ -65,6 +65,7 @@ defmodule Cadet.Test.Seeds do student_submitted = insert(:user, %{latest_viewed_course: course1}) student_graded = insert(:user, %{latest_viewed_course: course1}) student_different_group = insert(:user, %{latest_viewed_course: course1}) + student_grading_published = insert(:user, %{latest_viewed_course: course1}) # CourseRegistration and Group avenger1_cr = insert(:course_registration, %{user: avenger1, course: course1, role: :staff}) @@ -129,6 +130,14 @@ defmodule Cadet.Test.Seeds do group: group2 }) + student_grading_published_cr = + insert(:course_registration, %{ + user: student_grading_published, + course: course1, + role: :student, + group: group + }) + students = [ student1a_cr, student1b_cr, @@ -136,18 +145,20 @@ defmodule Cadet.Test.Seeds do student_attempted_cr, student_submitted_cr, student_graded_cr, - student_different_group_cr + student_different_group_cr, + student_grading_published_cr ] - # {student_cr, submission_status, is_graded, avenger} + # {student_cr, submission_status, is_graded, is_published, avenger} students_with_assessment_info = [ - {student1a_cr, :attempting, false, avenger1_cr}, - {student1b_cr, :attempting, false, avenger1_cr}, - {student1c_cr, :attempting, false, avenger1_cr}, - {student_attempted_cr, :attempted, false, avenger1_cr}, - {student_submitted_cr, :submitted, false, avenger1_cr}, - {student_graded_cr, :submitted, true, avenger1_cr}, - {student_different_group_cr, :attempting, false, avenger2_cr} + {student1a_cr, :attempting, false, false, avenger1_cr}, + {student1b_cr, :attempting, false, false, avenger1_cr}, + {student1c_cr, :attempting, false, false, avenger1_cr}, + {student_attempted_cr, :attempted, false, false, avenger1_cr}, + {student_submitted_cr, :submitted, false, false, avenger1_cr}, + {student_graded_cr, :submitted, true, false, avenger1_cr}, + {student_different_group_cr, :attempting, false, false, avenger2_cr}, + {student_grading_published_cr, :submitted, true, true, avenger1_cr} ] _admin2cr = @@ -205,7 +216,9 @@ defmodule Cadet.Test.Seeds do admin: admin1_cr }, assessment_configs: assessment_configs, - assessments: assessments + assessments: assessments, + students_with_assessment_info: students_with_assessment_info, + student_grading_published: student_grading_published_cr } end end @@ -250,14 +263,15 @@ defmodule Cadet.Test.Seeds do submissions_with_grader = students - |> Enum.map(fn {student, submission_status, is_graded, avenger} -> + |> Enum.map(fn {student, submission_status, is_graded, is_published, avenger} -> grader = if is_graded, do: avenger, else: nil {grader, insert(:submission, %{ assessment: assessment, student: student, - status: submission_status + status: submission_status, + is_grading_published: is_published })} end) From 8c1387f5932777fd8ccf831be1502873a9f99f4b Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Thu, 21 Mar 2024 21:44:16 +0800 Subject: [PATCH 32/74] refactor: Update old tests to accommodate for is_grading_published --- .../controllers/assessments_controller_test.exs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/test/cadet_web/controllers/assessments_controller_test.exs b/test/cadet_web/controllers/assessments_controller_test.exs index ac7315e47..1f484aee0 100644 --- a/test/cadet_web/controllers/assessments_controller_test.exs +++ b/test/cadet_web/controllers/assessments_controller_test.exs @@ -163,7 +163,7 @@ defmodule CadetWeb.AssessmentsControllerTest do "isPublished" => &1.is_published, "gradedCount" => 0, "questionCount" => 9, - "isGradingPublished" => true + "isGradingPublished" => false } ) @@ -207,15 +207,15 @@ defmodule CadetWeb.AssessmentsControllerTest do test "renders xp for students", %{ conn: conn, courses: %{course1: course1}, - role_crs: %{student: student}, assessment_configs: configs, - assessments: assessments + assessments: assessments, + student_grading_published: student_grading_published } do assessment = assessments[hd(configs).type].assessment resp = conn - |> sign_in(student.user) + |> sign_in(student_grading_published.user) |> get(build_url(course1.id)) |> json_response(200) |> Enum.find(&(&1["id"] == assessment.id)) @@ -776,8 +776,11 @@ defmodule CadetWeb.AssessmentsControllerTest do conn: conn, courses: %{course1: course1}, role_crs: role_crs, - assessments: assessments + assessments: assessments, + student_grading_published: student_grading_published } do + role_crs = Map.put(role_crs, :student, student_grading_published) + for role <- Role.__enum_map__() do course_reg = Map.get(role_crs, role) From 7cc1448e10ef319f64187037bc2bae0cd0a48b4f Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Thu, 21 Mar 2024 21:45:23 +0800 Subject: [PATCH 33/74] refactor: Improve filter tests to be more flexible Remove hard coded numbers and count expected number with students_with_assessment_info --- test/cadet/assessments/assessments_test.exs | 43 ++++++++++++++------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index 26bb51385..17a5988c5 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -1821,7 +1821,9 @@ defmodule Cadet.AssessmentsTest do assert formatted_answer.grader != nil assert formatted_answer.grader_id != nil assert formatted_answer.comments != nil - + end + end + # Tests assume each config has only 1 assessment describe "get submission function" do setup do @@ -1936,10 +1938,12 @@ defmodule Cadet.AssessmentsTest do test "filter by submission not fully graded", %{ course_regs: %{avenger1_cr: avenger, students: students}, assessments: assessments, - total_submissions: total_submissions + total_submissions: total_submissions, + students_with_assessment_info: students_with_assessment_info } do - # All but one is fully graded - expected_length = length(Map.keys(assessments)) * (length(students) - 1) + expected_length = + length(Map.keys(assessments)) * + Enum.count(students_with_assessment_info, fn {_, _, is_graded, _, _} -> !is_graded end) {_, res} = Assessments.submissions_by_grader_for_index(avenger, %{ @@ -1959,10 +1963,14 @@ defmodule Cadet.AssessmentsTest do test "filter by group avenger", %{ course_regs: %{avenger1_cr: avenger, group: group, students: students}, assessments: assessments, - total_submissions: total_submissions + total_submissions: total_submissions, + students_with_assessment_info: students_with_assessment_info } do - # All but one is in the same group - expected_length = length(Map.keys(assessments)) * (length(students) - 1) + expected_length = + length(Map.keys(assessments)) * + Enum.count(students_with_assessment_info, fn {_, _, _, _, avenger_cr} -> + avenger_cr.id == avenger.id + end) {_, res} = Assessments.submissions_by_grader_for_index(avenger, %{ @@ -1983,10 +1991,14 @@ defmodule Cadet.AssessmentsTest do test "filter by group avenger2", %{ course_regs: %{avenger2_cr: avenger2, group2: group2, students: students}, assessments: assessments, - total_submissions: total_submissions + total_submissions: total_submissions, + students_with_assessment_info: students_with_assessment_info } do - # One in the same group - expected_length = length(Map.keys(assessments)) + expected_length = + length(Map.keys(assessments)) * + Enum.count(students_with_assessment_info, fn {_, _, _, _, avenger_cr} -> + avenger_cr.id == avenger2.id + end) {_, res} = Assessments.submissions_by_grader_for_index(avenger2, %{ @@ -2008,10 +2020,10 @@ defmodule Cadet.AssessmentsTest do test "filter by group name group", %{ course_regs: %{avenger2_cr: avenger2, group: group, students: students}, assessments: assessments, - total_submissions: total_submissions + total_submissions: total_submissions, + students_with_assessment_info: students_with_assessment_info } do - # All but one is in group - expected_length = length(Map.keys(assessments)) * (length(students) - 1) + expected_length = length(Map.keys(assessments)) * Enum.count(students_with_assessment_info, fn {student, _, _, _, _} -> student.group.id == group.id end) group_name = group.name {_, res} = @@ -2034,10 +2046,11 @@ defmodule Cadet.AssessmentsTest do test "filter by group name group2", %{ course_regs: %{avenger1_cr: avenger, group2: group2, students: students}, assessments: assessments, - total_submissions: total_submissions + total_submissions: total_submissions, + students_with_assessment_info: students_with_assessment_info } do # One in the group - expected_length = length(Map.keys(assessments)) + expected_length = length(Map.keys(assessments)) * Enum.count(students_with_assessment_info, fn {student, _, _, _, _} -> student.group.id == group2.id end) group_name = group2.name {_, res} = From 44cbab86b89088dc57bf35ead383c945e723787f Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Thu, 21 Mar 2024 21:46:20 +0800 Subject: [PATCH 34/74] feat: Implement test for filter by not published --- test/cadet/assessments/assessments_test.exs | 27 +++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index 17a5988c5..6ad4f07c1 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -1960,6 +1960,33 @@ defmodule Cadet.AssessmentsTest do end) end + test "filter by submission not published", %{ + course_regs: %{avenger1_cr: avenger, students: students}, + assessments: assessments, + total_submissions: total_submissions, + students_with_assessment_info: students_with_assessment_info + } do + expected_length = + length(Map.keys(assessments)) * + Enum.count(students_with_assessment_info, fn {_, _, _, is_published, _} -> + !is_published + end) + + {_, res} = + Assessments.submissions_by_grader_for_index(avenger, %{ + "notPublished" => "true", + "pageSize" => total_submissions + }) + + submissions_from_res = res[:data][:submissions] + + assert length(submissions_from_res) == expected_length + + Enum.each(submissions_from_res, fn s -> + assert s.is_grading_published == false + end) + end + test "filter by group avenger", %{ course_regs: %{avenger1_cr: avenger, group: group, students: students}, assessments: assessments, From 89411adab0e2e16ea4416b1e0c38ec6a5af308e1 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Thu, 21 Mar 2024 21:46:54 +0800 Subject: [PATCH 35/74] chore: Format --- test/cadet/assessments/assessments_test.exs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index 6ad4f07c1..4d281caea 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -2050,7 +2050,12 @@ defmodule Cadet.AssessmentsTest do total_submissions: total_submissions, students_with_assessment_info: students_with_assessment_info } do - expected_length = length(Map.keys(assessments)) * Enum.count(students_with_assessment_info, fn {student, _, _, _, _} -> student.group.id == group.id end) + expected_length = + length(Map.keys(assessments)) * + Enum.count(students_with_assessment_info, fn {student, _, _, _, _} -> + student.group.id == group.id + end) + group_name = group.name {_, res} = @@ -2077,7 +2082,12 @@ defmodule Cadet.AssessmentsTest do students_with_assessment_info: students_with_assessment_info } do # One in the group - expected_length = length(Map.keys(assessments)) * Enum.count(students_with_assessment_info, fn {student, _, _, _, _} -> student.group.id == group2.id end) + expected_length = + length(Map.keys(assessments)) * + Enum.count(students_with_assessment_info, fn {student, _, _, _, _} -> + student.group.id == group2.id + end) + group_name = group2.name {_, res} = From 888a4f70e390ecff981ec844f22fff9a0a6aca24 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Thu, 21 Mar 2024 22:08:33 +0800 Subject: [PATCH 36/74] refactor: Make is_grading_published default to false in factory function --- test/cadet/assessments/assessments_test.exs | 33 ++++++++++++------- .../admin_user_controller_test.exs | 3 +- .../assessments_controller_test.exs | 7 +++- .../controllers/user_controller_test.exs | 9 +++-- .../assessments/submission_factory.ex | 2 +- 5 files changed, 37 insertions(+), 17 deletions(-) diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index 4d281caea..3be9d692e 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -944,7 +944,8 @@ defmodule Cadet.AssessmentsTest do assessment: assessment, student: test_cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ @@ -1012,7 +1013,8 @@ defmodule Cadet.AssessmentsTest do assessment: assessment, student: test_cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ @@ -1081,7 +1083,8 @@ defmodule Cadet.AssessmentsTest do assessment: assessment, student: test_cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ @@ -1168,7 +1171,8 @@ defmodule Cadet.AssessmentsTest do assessment: assessment, student: test_cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ @@ -1279,7 +1283,8 @@ defmodule Cadet.AssessmentsTest do assessment: assessment, student: test_cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ @@ -1315,7 +1320,8 @@ defmodule Cadet.AssessmentsTest do assessment: assessment, student: test_cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ @@ -1363,7 +1369,8 @@ defmodule Cadet.AssessmentsTest do assessment: assessment, student: test_cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ @@ -1424,7 +1431,8 @@ defmodule Cadet.AssessmentsTest do assessment: assessment, student: test_cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ @@ -1492,7 +1500,8 @@ defmodule Cadet.AssessmentsTest do assessment: assessment, student: test_cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ @@ -1560,7 +1569,8 @@ defmodule Cadet.AssessmentsTest do assessment: assessment, student: test_cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ @@ -1647,7 +1657,8 @@ defmodule Cadet.AssessmentsTest do assessment: assessment, student: test_cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ diff --git a/test/cadet_web/admin_controllers/admin_user_controller_test.exs b/test/cadet_web/admin_controllers/admin_user_controller_test.exs index 786210dec..bd134876c 100644 --- a/test/cadet_web/admin_controllers/admin_user_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_user_controller_test.exs @@ -582,7 +582,8 @@ defmodule CadetWeb.AdminUserControllerTest do assessment: assessment, student: test_cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ diff --git a/test/cadet_web/controllers/assessments_controller_test.exs b/test/cadet_web/controllers/assessments_controller_test.exs index 1f484aee0..1b47aef66 100644 --- a/test/cadet_web/controllers/assessments_controller_test.exs +++ b/test/cadet_web/controllers/assessments_controller_test.exs @@ -1402,7 +1402,12 @@ defmodule CadetWeb.AssessmentsControllerTest do course_reg = insert(:course_registration, role: :student, course: course1) submission = - insert(:submission, assessment: assessment, student: course_reg, status: :submitted) + insert(:submission, + assessment: assessment, + student: course_reg, + status: :submitted, + is_grading_published: true + ) Enum.each( [question_one, question_two], diff --git a/test/cadet_web/controllers/user_controller_test.exs b/test/cadet_web/controllers/user_controller_test.exs index c16a9b1fc..939f0cf9e 100644 --- a/test/cadet_web/controllers/user_controller_test.exs +++ b/test/cadet_web/controllers/user_controller_test.exs @@ -42,7 +42,8 @@ defmodule CadetWeb.UserControllerTest do assessment: assessment, student: cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ @@ -194,7 +195,8 @@ defmodule CadetWeb.UserControllerTest do assessment: assessment, student: test_cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ @@ -265,7 +267,8 @@ defmodule CadetWeb.UserControllerTest do assessment: assessment, student: cr, status: :submitted, - xp_bonus: 100 + xp_bonus: 100, + is_grading_published: true }) insert(:answer, %{ diff --git a/test/factories/assessments/submission_factory.ex b/test/factories/assessments/submission_factory.ex index db2c58499..41a25c3bd 100644 --- a/test/factories/assessments/submission_factory.ex +++ b/test/factories/assessments/submission_factory.ex @@ -11,7 +11,7 @@ defmodule Cadet.Assessments.SubmissionFactory do %Submission{ student: build(:course_registration, %{role: :student}), assessment: build(:assessment), - is_grading_published: true + is_grading_published: false } end end From 4298970dce0b91b61d14da560b6dba0c0ed333d0 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 22 Mar 2024 09:47:15 +0800 Subject: [PATCH 37/74] feat: Add is_grading_auto_published column to assessment config --- lib/cadet/courses/assessment_config.ex | 3 ++- ...240321222300_add_is_grading_auto_published.exs | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 priv/repo/migrations/20240321222300_add_is_grading_auto_published.exs diff --git a/lib/cadet/courses/assessment_config.ex b/lib/cadet/courses/assessment_config.ex index ec2455b69..9180d5a7c 100644 --- a/lib/cadet/courses/assessment_config.ex +++ b/lib/cadet/courses/assessment_config.ex @@ -16,6 +16,7 @@ defmodule Cadet.Courses.AssessmentConfig do # used by frontend to determine display styles field(:early_submission_xp, :integer, default: 0) field(:hours_before_early_xp_decay, :integer, default: 0) + field(:is_grading_auto_published, :boolean, default: false) belongs_to(:course, Course) @@ -24,7 +25,7 @@ defmodule Cadet.Courses.AssessmentConfig do @required_fields ~w(course_id)a @optional_fields ~w(order type early_submission_xp - hours_before_early_xp_decay show_grading_summary is_manually_graded has_token_counter)a + hours_before_early_xp_decay show_grading_summary is_manually_graded has_token_counter is_grading_auto_published)a def changeset(assessment_config, params) do assessment_config diff --git a/priv/repo/migrations/20240321222300_add_is_grading_auto_published.exs b/priv/repo/migrations/20240321222300_add_is_grading_auto_published.exs new file mode 100644 index 000000000..4c44c2101 --- /dev/null +++ b/priv/repo/migrations/20240321222300_add_is_grading_auto_published.exs @@ -0,0 +1,15 @@ +defmodule Cadet.Repo.Migrations.AddIsGradingAutoPublished do + use Ecto.Migration + + def up do + alter table(:assessment_configs) do + add(:is_grading_auto_published, :boolean, null: false, default: false) + end + end + + def down do + alter table(:assessment_configs) do + remove(:is_grading_auto_published) + end + end +end From 147cf4d671fba0ff6a78f53e97bcfd6ece73d950 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 22 Mar 2024 16:47:44 +0800 Subject: [PATCH 38/74] feat: Add guard clause to ensure submission is fully graded before publishing --- lib/cadet/assessments/assessments.ex | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 265006305..0f79f1329 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1041,6 +1041,7 @@ defmodule Cadet.Assessments do with {:submission_found?, true} <- {:submission_found?, is_map(submission)}, {:status, :submitted} <- {:status, submission.status}, + {:fully_graded?, true} <- {:fully_graded?, is_fully_graded?(submission_id)}, {:allowed_to_publish?, true} <- {:allowed_to_publish?, role == :admin or bypass or @@ -1068,6 +1069,9 @@ defmodule Cadet.Assessments do {:allowed_to_publish?, false} -> {:error, {:forbidden, "Only Avenger of student or Admin is permitted to publish grading"}} + {:fully_graded?, false} -> + {:error, {:bad_request, "Some answers are not graded"}} + _ -> {:error, {:internal_server_error, "Please try again later."}} end From a41a894058d4c672059b57231809bbea1ee98012 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 22 Mar 2024 16:51:53 +0800 Subject: [PATCH 39/74] feat: Implement publish_grading and is_fully_autograded? - Adds another publish_grading function which bypasses all checks - Adds a new function is_fully_autograded? which checks if all answers are successfully auto graded --- lib/cadet/assessments/assessments.ex | 58 +++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 0f79f1329..dfca11344 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1077,6 +1077,41 @@ defmodule Cadet.Assessments do end end + def publish_grading(submission_id) + when is_ecto_id(submission_id) do + submission = + Submission + |> join(:inner, [s], a in assoc(s, :assessment)) + |> preload([_, a], assessment: a) + |> Repo.get(submission_id) + + with {:submission_found?, true} <- {:submission_found?, is_map(submission)}, + {:status, :submitted} <- {:status, submission.status} do + submission + |> Submission.changeset(%{is_grading_published: true}) + |> Repo.update() + + Notifications.write_notification_when_graded( + submission.id, + :autograded + ) + + {:ok, nil} + else + {:submission_found?, false} -> + {:error, {:not_found, "Submission not found"}} + + {:status, :attempting} -> + {:error, {:bad_request, "Some questions have not been attempted"}} + + {:status, :attempted} -> + {:error, {:bad_request, "Assessment has not been submitted"}} + + _ -> + {:error, {:internal_server_error, "Please try again later."}} + end + end + @spec update_submission_status_and_xp_bonus(Submission.t()) :: {:ok, Submission.t()} | {:error, Ecto.Changeset.t()} defp update_submission_status_and_xp_bonus(submission = %Submission{}) do @@ -1784,7 +1819,7 @@ defmodule Cadet.Assessments do end end - defp is_fully_graded?(%Answer{submission_id: submission_id}) do + defp is_fully_graded?(submission_id) do submission = Submission |> Repo.get_by(id: submission_id) @@ -1805,6 +1840,27 @@ defmodule Cadet.Assessments do question_count == graded_count end + def is_fully_autograded?(submission_id) do + submission = + Submission + |> Repo.get_by(id: submission_id) + + question_count = + Question + |> where(assessment_id: ^submission.assessment_id) + |> select([q], count(q.id)) + |> Repo.one() + + graded_count = + Answer + |> where([a], submission_id: ^submission_id) + |> where([a], a.autograding_status == :success) + |> select([a], count(a.id)) + |> Repo.one() + + question_count == graded_count + end + @spec update_grading_info( %{submission_id: integer() | String.t(), question_id: integer() | String.t()}, %{}, From d2b173bb184767120396f82553da120ed81eb73b Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 22 Mar 2024 21:50:21 +0800 Subject: [PATCH 40/74] chore: Clean up code --- test/cadet/assessments/assessments_test.exs | 2 +- test/support/seeds.ex | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index 3be9d692e..19ee427a0 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -2092,7 +2092,7 @@ defmodule Cadet.AssessmentsTest do total_submissions: total_submissions, students_with_assessment_info: students_with_assessment_info } do - # One in the group + expected_length = length(Map.keys(assessments)) * Enum.count(students_with_assessment_info, fn {student, _, _, _, _} -> diff --git a/test/support/seeds.ex b/test/support/seeds.ex index 20ab5061b..e263860e1 100644 --- a/test/support/seeds.ex +++ b/test/support/seeds.ex @@ -149,7 +149,7 @@ defmodule Cadet.Test.Seeds do student_grading_published_cr ] - # {student_cr, submission_status, is_graded, is_published, avenger} + # {student_cr, submission_status, is_graded, is_grading_published, avenger} students_with_assessment_info = [ {student1a_cr, :attempting, false, false, avenger1_cr}, {student1b_cr, :attempting, false, false, avenger1_cr}, @@ -263,7 +263,7 @@ defmodule Cadet.Test.Seeds do submissions_with_grader = students - |> Enum.map(fn {student, submission_status, is_graded, is_published, avenger} -> + |> Enum.map(fn {student, submission_status, is_graded, is_grading_published, avenger} -> grader = if is_graded, do: avenger, else: nil {grader, @@ -271,7 +271,7 @@ defmodule Cadet.Test.Seeds do assessment: assessment, student: student, status: submission_status, - is_grading_published: is_published + is_grading_published: is_grading_published })} end) From 626b61cf534d18d8c1cb3e84993cfa04c3d3ab3e Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 22 Mar 2024 21:51:08 +0800 Subject: [PATCH 41/74] feat: Add is_grading_auto_published and is_manually_graded to seed --- priv/repo/seeds.exs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/priv/repo/seeds.exs b/priv/repo/seeds.exs index c5661ee65..731054234 100644 --- a/priv/repo/seeds.exs +++ b/priv/repo/seeds.exs @@ -99,11 +99,22 @@ if Cadet.Env.env() == :dev do end # Assessments and Submissions - valid_assessment_types = [{1, "Missions"}, {2, "Paths"}, {3, "Quests"}] + valid_assessment_types = [ + {1, "Missions", false, true}, + {2, "Paths", true, false}, + {3, "Quests", false, true} + ] assessment_configs = - Enum.map(valid_assessment_types, fn {order, type} -> - insert(:assessment_config, %{type: type, order: order, course: course}) + Enum.map(valid_assessment_types, fn {order, type, is_grading_auto_published, + is_manually_graded} -> + insert(:assessment_config, %{ + type: type, + order: order, + course: course, + is_grading_auto_published: is_grading_auto_published, + is_manually_graded: is_manually_graded + }) end) for i <- 1..number_of_assessments do From 07a63e68d5c4280926a00ea0a5c6e74e541c0325 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 22 Mar 2024 21:51:41 +0800 Subject: [PATCH 42/74] feat: Implement tests for is_fully_autograded?/1 --- test/cadet/assessments/assessments_test.exs | 47 +++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index 19ee427a0..1ed3702c6 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -2411,6 +2411,53 @@ defmodule Cadet.AssessmentsTest do end end + describe "is_fully_autograded? helper function" do + setup do + assessment = insert(:assessment) + student = insert(:course_registration, role: :student) + question = insert(:mcq_question, assessment: assessment) + question2 = insert(:mcq_question, assessment: assessment) + + submission = + insert(:submission, assessment: assessment, student: student, status: :submitted) + + %{question: question, question2: question2, submission: submission} + end + + test "returns true when all answers are autograded successfully", %{ + question: question, + question2: question2, + submission: submission + } do + insert(:answer, submission: submission, question: question, autograding_status: :success) + insert(:answer, submission: submission, question: question2, autograding_status: :success) + + assert Assessments.is_fully_autograded?(submission.id) == true + end + + test "returns false when not all answers are autograded successfully", %{ + question: question, + question2: question2, + submission: submission + } do + insert(:answer, submission: submission, question: question, autograding_status: :success) + insert(:answer, submission: submission, question: question2, autograding_status: :failed) + + assert Assessments.is_fully_autograded?(submission.id) == false + end + + test "returns false when not all answers are autograded successfully 2", %{ + question: question, + question2: question2, + submission: submission + } do + insert(:answer, submission: submission, question: question, autograding_status: :success) + insert(:answer, submission: submission, question: question2, autograding_status: :none) + + assert Assessments.is_fully_autograded?(submission.id) == false + end + end + defp get_answer_relative_scores(answers) do answers |> Enum.map(fn ans -> ans.relative_score end) end From 38b3303e99f21e74cf52fc5b0a91e78e7ddc18a2 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 09:30:06 +0800 Subject: [PATCH 43/74] feat: Implement publish_all_grades route --- lib/cadet_web/router.ex | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/cadet_web/router.ex b/lib/cadet_web/router.ex index 4f468b2c5..0f60c6bcb 100644 --- a/lib/cadet_web/router.ex +++ b/lib/cadet_web/router.ex @@ -112,6 +112,9 @@ defmodule CadetWeb.Router do get("/grading", AdminGradingController, :index) get("/grading/summary", AdminGradingController, :grading_summary) + + post("/grading/:assessmentid/publish_all_grades", AdminGradingController, :publish_all_grades) + get("/grading/:submissionid", AdminGradingController, :show) post("/grading/:submissionid/unsubmit", AdminGradingController, :unsubmit) post("/grading/:submissionid/unpublish_grades", AdminGradingController, :unpublish_grades) From ae9b42fa4ffa66426056cf946ee804819f8195d4 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 12:31:56 +0800 Subject: [PATCH 44/74] feat: Add publish_all_grades in controller --- .../admin_controllers/admin_grading_controller.ex | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/lib/cadet_web/admin_controllers/admin_grading_controller.ex b/lib/cadet_web/admin_controllers/admin_grading_controller.ex index 9f8c9e733..46a2d80ce 100644 --- a/lib/cadet_web/admin_controllers/admin_grading_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_grading_controller.ex @@ -109,6 +109,21 @@ defmodule CadetWeb.AdminGradingController do end end + def publish_all_grades(conn, %{"assessmentid" => assessment_id}) + when is_ecto_id(assessment_id) do + course_reg = conn.assigns[:course_reg] + + case Assessments.publish_all_graded(course_reg, assessment_id) do + {:ok, count} -> + text(conn, "Published #{count} grades") + + {:error, {status, message}} -> + conn + |> put_status(status) + |> text(message) + end + end + def autograde_submission(conn, %{"submissionid" => submission_id}) do course_reg = conn.assigns[:course_reg] From b6069053ad98334b8f624148740af661e96048f3 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 12:32:43 +0800 Subject: [PATCH 45/74] feat: Implement publish_all_graded function --- lib/cadet/assessments/assessments.ex | 50 ++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index dfca11344..9e1361ed4 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1112,6 +1112,56 @@ defmodule Cadet.Assessments do end end + def publish_all_graded(publisher = %CourseRegistration{}, assessment_id) do + if publisher.role == :admin do + answers_query = + Answer + |> group_by([ans], ans.submission_id) + |> select([ans], %{ + submission_id: ans.submission_id, + graded_count: filter(count(ans.id), not is_nil(ans.grader_id)), + autograded_count: filter(count(ans.id), ans.autograding_status == :success) + }) + + question_query = + Question + |> group_by([q], q.assessment_id) + |> join(:inner, [q], a in Assessment, on: q.assessment_id == a.id) + |> select([q, a], %{ + assessment_id: q.assessment_id, + question_count: count(q.id) + }) + + submissions = + Submission + |> join(:left, [s], ans in subquery(answers_query), on: ans.submission_id == s.id) + |> join(:left, [s, ans], asst in subquery(question_query), + on: s.assessment_id == asst.assessment_id + ) + |> join(:inner, [s, ans, asst], cr in CourseRegistration, on: s.student_id == cr.id) + |> where([s, ans, asst, cr], cr.course_id == ^publisher.course_id) + |> where( + [s, ans, asst, cr], + asst.question_count == ans.graded_count or asst.question_count == ans.autograded_count + ) + |> where([s, ans, asst, cr], s.is_grading_published == false) + |> where([s, ans, asst, cr], s.assessment_id == ^assessment_id) + |> select([s, ans, asst, cr], %{ + id: s.id + }) + |> Repo.all() + + submissions + |> Enum.map(fn s -> + publish_grading(s.id, publisher) + end) + + {:ok, length(submissions)} + else + {:error, {:forbidden, "Only Admin is permitted to publish all grades"}} + end + end + @spec update_submission_status_and_xp_bonus(Submission.t()) :: {:ok, Submission.t()} | {:error, Ecto.Changeset.t()} defp update_submission_status_and_xp_bonus(submission = %Submission{}) do From 582a394fa0cda52385c549d53e1a067910c6a734 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 12:33:06 +0800 Subject: [PATCH 46/74] feat: Implement tests for publish_all_graded/2 --- test/cadet/assessments/assessments_test.exs | 31 +++++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index 1ed3702c6..b1968fb89 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -4,7 +4,7 @@ defmodule Cadet.AssessmentsTest do import Cadet.{Factory, TestEntityHelper} alias Cadet.Assessments - alias Cadet.Assessments.{Assessment, Question, SubmissionVotes} + alias Cadet.Assessments.{Assessment, Question, Submission, SubmissionVotes} test "create assessments of all types" do course = insert(:course) @@ -1835,7 +1835,6 @@ defmodule Cadet.AssessmentsTest do end end - # Tests assume each config has only 1 assessment describe "get submission function" do setup do seed = Cadet.Test.Seeds.assessments() @@ -2092,7 +2091,6 @@ defmodule Cadet.AssessmentsTest do total_submissions: total_submissions, students_with_assessment_info: students_with_assessment_info } do - expected_length = length(Map.keys(assessments)) * Enum.count(students_with_assessment_info, fn {student, _, _, _, _} -> @@ -2458,6 +2456,33 @@ defmodule Cadet.AssessmentsTest do end end + test "publish all graded submissions" do + %{ + role_crs: %{admin: admin}, + assessments: assessments, + students_with_assessment_info: students + } = Cadet.Test.Seeds.assessments() + + assessment_id = assessments["mission"][:assessment].id + + # 1 student has all assessments published + expected_length = + Enum.count(students, fn {_, _, is_graded, is_grading_published, _} -> + is_graded and not is_grading_published + end) + length(Map.keys(assessments)) + + Assessments.publish_all_graded(admin, assessment_id) + + published_submissions = + Submission + |> where([s], s.is_grading_published == true) + |> select([s], %{count: s.id |> count()}) + |> Repo.one() + + number_of_published_submissions = published_submissions.count + assert number_of_published_submissions == expected_length + end + defp get_answer_relative_scores(answers) do answers |> Enum.map(fn ans -> ans.relative_score end) end From ade0e24974c7a770a8dc4c5ab15eaf1159f2fbfb Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 12:45:33 +0800 Subject: [PATCH 47/74] refactor: Rename param names and allow filter by true or false --- lib/cadet/assessments/assessments.ex | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 9e1361ed4..8c2d56ebb 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1694,11 +1694,11 @@ defmodule Cadet.Assessments do {"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) + {"isFullyGraded", value}, dynamic -> + dynamic([ans: ans, asst: asst], ^dynamic and (asst.question_count == ans.graded_count) == ^value) - {"notPublished", "true"}, dynamic -> - dynamic([submission], ^dynamic and not submission.is_grading_published) + {"isGradingPublished", value}, dynamic -> + dynamic([submission], ^dynamic and submission.is_grading_published == ^value) {_, _}, dynamic -> dynamic From f3a0eebab0e2fa359402958bb73b4f70907fdefc Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 12:51:46 +0800 Subject: [PATCH 48/74] refactor: Add tests for change in param and refactor code --- lib/cadet/assessments/assessments.ex | 7 ++- lib/cadet/workers/NotificationWorker.ex | 2 +- test/cadet/assessments/assessments_test.exs | 56 ++++++++++++++++++- .../notification_worker_test.exs | 2 +- 4 files changed, 61 insertions(+), 6 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 8c2d56ebb..e66d3064f 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1590,7 +1590,7 @@ defmodule Cadet.Assessments do grader = %CourseRegistration{course_id: course_id}, params \\ %{ "group" => "false", - "notFullyGraded" => "true", + "isFullyGraded" => "false", "pageSize" => "10", "offset" => "0" } @@ -1695,7 +1695,10 @@ defmodule Cadet.Assessments do dynamic([submission], ^dynamic and submission.status == ^value) {"isFullyGraded", value}, dynamic -> - dynamic([ans: ans, asst: asst], ^dynamic and (asst.question_count == ans.graded_count) == ^value) + dynamic( + [ans: ans, asst: asst], + ^dynamic and asst.question_count == ans.graded_count == ^value + ) {"isGradingPublished", value}, dynamic -> dynamic([submission], ^dynamic and submission.is_grading_published == ^value) diff --git a/lib/cadet/workers/NotificationWorker.ex b/lib/cadet/workers/NotificationWorker.ex index 67913a033..ad402a8fb 100644 --- a/lib/cadet/workers/NotificationWorker.ex +++ b/lib/cadet/workers/NotificationWorker.ex @@ -76,7 +76,7 @@ defmodule Cadet.Workers.NotificationWorker do {:ok, %{data: %{submissions: ungraded_submissions}}} = Cadet.Assessments.submissions_by_grader_for_index(avenger_cr, %{ "group" => "true", - "notFullyGraded" => "true" + "isFullyGraded" => "false" }) if Enum.count(ungraded_submissions) < ungraded_threshold do diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index b1968fb89..47e558768 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -1945,6 +1945,31 @@ defmodule Cadet.AssessmentsTest do end) end + test "filter by submission fully graded", %{ + course_regs: %{avenger1_cr: avenger, students: students}, + assessments: assessments, + total_submissions: total_submissions, + students_with_assessment_info: students_with_assessment_info + } do + expected_length = + length(Map.keys(assessments)) * + Enum.count(students_with_assessment_info, fn {_, _, is_graded, _, _} -> is_graded end) + + {_, res} = + Assessments.submissions_by_grader_for_index(avenger, %{ + "isFullyGraded" => "true", + "pageSize" => total_submissions + }) + + submissions_from_res = res[:data][:submissions] + + assert length(submissions_from_res) == expected_length + + Enum.each(submissions_from_res, fn s -> + assert s.question_count == s.graded_count + end) + end + test "filter by submission not fully graded", %{ course_regs: %{avenger1_cr: avenger, students: students}, assessments: assessments, @@ -1957,7 +1982,7 @@ defmodule Cadet.AssessmentsTest do {_, res} = Assessments.submissions_by_grader_for_index(avenger, %{ - "notFullyGraded" => "true", + "isFullyGraded" => "false", "pageSize" => total_submissions }) @@ -1970,6 +1995,33 @@ defmodule Cadet.AssessmentsTest do end) end + test "filter by submission published", %{ + course_regs: %{avenger1_cr: avenger, students: students}, + assessments: assessments, + total_submissions: total_submissions, + students_with_assessment_info: students_with_assessment_info + } do + expected_length = + length(Map.keys(assessments)) * + Enum.count(students_with_assessment_info, fn {_, _, _, is_published, _} -> + is_published + end) + + {_, res} = + Assessments.submissions_by_grader_for_index(avenger, %{ + "isGradingPublished" => "true", + "pageSize" => total_submissions + }) + + submissions_from_res = res[:data][:submissions] + + assert length(submissions_from_res) == expected_length + + Enum.each(submissions_from_res, fn s -> + assert s.is_grading_published == true + end) + end + test "filter by submission not published", %{ course_regs: %{avenger1_cr: avenger, students: students}, assessments: assessments, @@ -1984,7 +2036,7 @@ defmodule Cadet.AssessmentsTest do {_, res} = Assessments.submissions_by_grader_for_index(avenger, %{ - "notPublished" => "true", + "isGradingPublished" => "false", "pageSize" => total_submissions }) diff --git a/test/cadet/jobs/notification_worker/notification_worker_test.exs b/test/cadet/jobs/notification_worker/notification_worker_test.exs index 9368a033c..62b3f4a39 100644 --- a/test/cadet/jobs/notification_worker/notification_worker_test.exs +++ b/test/cadet/jobs/notification_worker/notification_worker_test.exs @@ -21,7 +21,7 @@ defmodule Cadet.NotificationWorker.NotificationWorkerTest do {:ok, %{data: %{submissions: ungraded_submissions}}} = Cadet.Assessments.submissions_by_grader_for_index(avenger_cr, %{ "group" => "true", - "notFullyGraded" => "true" + "isFullyGraded" => "false" }) Repo.update_all(NotificationType, set: [is_enabled: true]) From 1abf60b8a26741a1afefe0249fde018ae4a69098 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 13:44:27 +0800 Subject: [PATCH 49/74] refactor: Change response for publish all grades --- lib/cadet_web/admin_controllers/admin_grading_controller.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cadet_web/admin_controllers/admin_grading_controller.ex b/lib/cadet_web/admin_controllers/admin_grading_controller.ex index 46a2d80ce..2fb354199 100644 --- a/lib/cadet_web/admin_controllers/admin_grading_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_grading_controller.ex @@ -114,8 +114,8 @@ defmodule CadetWeb.AdminGradingController do course_reg = conn.assigns[:course_reg] case Assessments.publish_all_graded(course_reg, assessment_id) do - {:ok, count} -> - text(conn, "Published #{count} grades") + {:ok, nil} -> + text(conn, "OK") {:error, {status, message}} -> conn From 1ae8d876bde8f013282227e6f4a37908a080dd08 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 13:44:49 +0800 Subject: [PATCH 50/74] refactor: Use update_all instead of recursively updating individual submissions --- lib/cadet/assessments/assessments.ex | 64 +++++++++++++++++----------- 1 file changed, 40 insertions(+), 24 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index e66d3064f..b771bc9f5 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1077,6 +1077,7 @@ defmodule Cadet.Assessments do end end + # Used by the auto-grading system to publish grading (Bypasses Course Reg checks) def publish_grading(submission_id) when is_ecto_id(submission_id) do submission = @@ -1112,6 +1113,8 @@ defmodule Cadet.Assessments do end end + @spec publish_all_graded(Cadet.Accounts.CourseRegistration.t(), any()) :: + {:ok, nil} | {:error, {:forbidden, String.t()}} def publish_all_graded(publisher = %CourseRegistration{}, assessment_id) do if publisher.role == :admin do answers_query = @@ -1132,36 +1135,49 @@ defmodule Cadet.Assessments do question_count: count(q.id) }) - submissions = - Submission - |> join(:left, [s], ans in subquery(answers_query), on: ans.submission_id == s.id) - |> join(:left, [s, ans], asst in subquery(question_query), - on: s.assessment_id == asst.assessment_id - ) - |> join(:inner, [s, ans, asst], cr in CourseRegistration, on: s.student_id == cr.id) - |> where([s, ans, asst, cr], cr.course_id == ^publisher.course_id) - |> where( - [s, ans, asst, cr], - asst.question_count == ans.graded_count or asst.question_count == ans.autograded_count - ) - |> where([s, ans, asst, cr], s.is_grading_published == false) - |> where([s, ans, asst, cr], s.assessment_id == ^assessment_id) - |> select([s, ans, asst, cr], %{ - id: s.id - }) - |> Repo.all() - - submissions - |> Enum.map(fn s -> - publish_grading(s.id, publisher) - end) + Submission + |> join(:inner, [s], ans in subquery(answers_query), on: ans.submission_id == s.id) + |> join(:inner, [s, ans], asst in subquery(question_query), + on: s.assessment_id == asst.assessment_id + ) + |> join(:inner, [s, ans, asst], cr in CourseRegistration, on: s.student_id == cr.id) + |> where([s, ans, asst, cr], cr.course_id == ^publisher.course_id) + |> where( + [s, ans, asst, cr], + asst.question_count == ans.graded_count or asst.question_count == ans.autograded_count + ) + |> where([s, ans, asst, cr], s.is_grading_published == false) + |> where([s, ans, asst, cr], s.assessment_id == ^assessment_id) + |> select([s, ans, asst, cr], %{ + id: s.id + }) + |> Repo.update_all(set: [is_grading_published: true]) - {:ok, length(submissions)} + {:ok, nil} else {:error, {:forbidden, "Only Admin is permitted to publish all grades"}} end end + # ! TODO + # def unpublish_all(publisher = %CourseRegistration{}, assessment_id) do + # if publisher.role == :admin do + # submissions = + # Submission + # |> join(:inner, [s, ans, asst], cr in CourseRegistration, on: s.student_id == cr.id) + # |> where([s, ans, asst, cr], cr.course_id == ^publisher.course_id) + # |> where([s, ans, asst, cr], s.is_grading_published == true) + # |> where([s, ans, asst, cr], s.assessment_id == ^assessment_id) + # |> select([s, ans, asst, cr], %{ + # id: s.id + # }) + # |> Repo.update_all(set: [is_grading_published: false]) + + # {:ok, nil} + # else + # {:error, {:forbidden, "Only Admin is permitted to publish all grades"}} + # end + # end @spec update_submission_status_and_xp_bonus(Submission.t()) :: {:ok, Submission.t()} | {:error, Ecto.Changeset.t()} defp update_submission_status_and_xp_bonus(submission = %Submission{}) do From 5077d70abca5ab6d3bfd9e06e9b9783146e066d7 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 13:52:10 +0800 Subject: [PATCH 51/74] feat: Implement unpublish all grades route --- .../admin_controllers/admin_grading_controller.ex | 15 +++++++++++++++ lib/cadet_web/router.ex | 1 + 2 files changed, 16 insertions(+) diff --git a/lib/cadet_web/admin_controllers/admin_grading_controller.ex b/lib/cadet_web/admin_controllers/admin_grading_controller.ex index 2fb354199..c83678d42 100644 --- a/lib/cadet_web/admin_controllers/admin_grading_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_grading_controller.ex @@ -124,6 +124,21 @@ defmodule CadetWeb.AdminGradingController do end end + def unpublish_all_grades(conn, %{"assessmentid" => assessment_id}) + when is_ecto_id(assessment_id) do +course_reg = conn.assigns[:course_reg] + +case Assessments.unpublish_all(course_reg, assessment_id) do + {:ok, nil} -> + text(conn, "OK") + + {:error, {status, message}} -> + conn + |> put_status(status) + |> text(message) +end +end + def autograde_submission(conn, %{"submissionid" => submission_id}) do course_reg = conn.assigns[:course_reg] diff --git a/lib/cadet_web/router.ex b/lib/cadet_web/router.ex index 0f60c6bcb..f7d71ad9f 100644 --- a/lib/cadet_web/router.ex +++ b/lib/cadet_web/router.ex @@ -114,6 +114,7 @@ defmodule CadetWeb.Router do get("/grading/summary", AdminGradingController, :grading_summary) post("/grading/:assessmentid/publish_all_grades", AdminGradingController, :publish_all_grades) + post("/grading/:assessmentid/unpublish_all_grades", AdminGradingController, :unpublish_all_grades) get("/grading/:submissionid", AdminGradingController, :show) post("/grading/:submissionid/unsubmit", AdminGradingController, :unsubmit) From 50a85a9d2f9b97cd06e6570d96e8f66d21a02463 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 14:04:20 +0800 Subject: [PATCH 52/74] feat: Implement unpublish_all --- lib/cadet/assessments/assessments.ex | 39 +++++++++++++--------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index b771bc9f5..63e7298eb 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1113,8 +1113,6 @@ defmodule Cadet.Assessments do end end - @spec publish_all_graded(Cadet.Accounts.CourseRegistration.t(), any()) :: - {:ok, nil} | {:error, {:forbidden, String.t()}} def publish_all_graded(publisher = %CourseRegistration{}, assessment_id) do if publisher.role == :admin do answers_query = @@ -1159,25 +1157,24 @@ defmodule Cadet.Assessments do end end - # ! TODO - # def unpublish_all(publisher = %CourseRegistration{}, assessment_id) do - # if publisher.role == :admin do - # submissions = - # Submission - # |> join(:inner, [s, ans, asst], cr in CourseRegistration, on: s.student_id == cr.id) - # |> where([s, ans, asst, cr], cr.course_id == ^publisher.course_id) - # |> where([s, ans, asst, cr], s.is_grading_published == true) - # |> where([s, ans, asst, cr], s.assessment_id == ^assessment_id) - # |> select([s, ans, asst, cr], %{ - # id: s.id - # }) - # |> Repo.update_all(set: [is_grading_published: false]) - - # {:ok, nil} - # else - # {:error, {:forbidden, "Only Admin is permitted to publish all grades"}} - # end - # end + def unpublish_all(publisher = %CourseRegistration{}, assessment_id) do + if publisher.role == :admin do + Submission + |> join(:inner, [s], cr in CourseRegistration, on: s.student_id == cr.id) + |> where([s, cr], cr.course_id == ^publisher.course_id) + |> where([s, cr], s.is_grading_published == true) + |> where([s, cr], s.assessment_id == ^assessment_id) + |> select([s, cr], %{ + id: s.id + }) + |> Repo.update_all(set: [is_grading_published: false]) + + {:ok, nil} + else + {:error, {:forbidden, "Only Admin is permitted to publish all grades"}} + end + end + @spec update_submission_status_and_xp_bonus(Submission.t()) :: {:ok, Submission.t()} | {:error, Ecto.Changeset.t()} defp update_submission_status_and_xp_bonus(submission = %Submission{}) do From ddd522d8a6f6ffb697c7d03407c9473db93d9fb3 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 14:04:43 +0800 Subject: [PATCH 53/74] feat: Implement unpublish_all tests --- test/cadet/assessments/assessments_test.exs | 76 +++++++++++++++------ 1 file changed, 56 insertions(+), 20 deletions(-) diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index 47e558768..f2b3307a0 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -2508,31 +2508,67 @@ defmodule Cadet.AssessmentsTest do end end - test "publish all graded submissions" do - %{ - role_crs: %{admin: admin}, - assessments: assessments, - students_with_assessment_info: students - } = Cadet.Test.Seeds.assessments() + describe "publish and unpublish all submissions for an assessment" do + setup do + Cadet.Test.Seeds.assessments() + end - assessment_id = assessments["mission"][:assessment].id + test "publish all graded submissions for an assessment", + %{ + role_crs: %{admin: admin}, + assessments: assessments, + students_with_assessment_info: students + } do + assessment_id = assessments["mission"][:assessment].id - # 1 student has all assessments published - expected_length = - Enum.count(students, fn {_, _, is_graded, is_grading_published, _} -> - is_graded and not is_grading_published - end) + length(Map.keys(assessments)) + # 1 student has all assessments published + expected_length = + Enum.count(students, fn {_, _, is_graded, is_grading_published, _} -> + is_graded and not is_grading_published + end) + length(Map.keys(assessments)) + + Assessments.publish_all_graded(admin, assessment_id) - Assessments.publish_all_graded(admin, assessment_id) + published_submissions = + Submission + |> where([s], s.is_grading_published == true) + |> select([s], %{count: s.id |> count()}) + |> Repo.one() - published_submissions = - Submission - |> where([s], s.is_grading_published == true) - |> select([s], %{count: s.id |> count()}) - |> Repo.one() + number_of_published_submissions = published_submissions.count + assert number_of_published_submissions == expected_length + end - number_of_published_submissions = published_submissions.count - assert number_of_published_submissions == expected_length + test "unpublish all submissions for an assessment", + %{ + role_crs: %{admin: admin}, + assessments: assessments, + students_with_assessment_info: students + } do + assessment_id = assessments["mission"][:assessment].id + + published_submissions_before = + Submission + |> where([s], s.is_grading_published == true) + |> select([s], %{count: s.id |> count()}) + |> Repo.one() + + expected_unpublished_length = + Enum.count(students, fn {_, _, _, is_grading_published, _} -> + is_grading_published + end) + + Assessments.unpublish_all(admin, assessment_id) + + published_submissions_after = + Submission + |> where([s], s.is_grading_published == true) + |> select([s], %{count: s.id |> count()}) + |> Repo.one() + + assert published_submissions_after.count + expected_unpublished_length == + published_submissions_before.count + end end defp get_answer_relative_scores(answers) do From 5360e3a36b3f2e4e04e957fb8c25366556740ec4 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 14:04:54 +0800 Subject: [PATCH 54/74] chore: Format --- .../admin_grading_controller.ex | 22 +++++++++---------- lib/cadet_web/router.ex | 7 +++++- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/lib/cadet_web/admin_controllers/admin_grading_controller.ex b/lib/cadet_web/admin_controllers/admin_grading_controller.ex index c83678d42..d95431362 100644 --- a/lib/cadet_web/admin_controllers/admin_grading_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_grading_controller.ex @@ -125,19 +125,19 @@ defmodule CadetWeb.AdminGradingController do end def unpublish_all_grades(conn, %{"assessmentid" => assessment_id}) - when is_ecto_id(assessment_id) do -course_reg = conn.assigns[:course_reg] + when is_ecto_id(assessment_id) do + course_reg = conn.assigns[:course_reg] -case Assessments.unpublish_all(course_reg, assessment_id) do - {:ok, nil} -> - text(conn, "OK") + case Assessments.unpublish_all(course_reg, assessment_id) do + {:ok, nil} -> + text(conn, "OK") - {:error, {status, message}} -> - conn - |> put_status(status) - |> text(message) -end -end + {:error, {status, message}} -> + conn + |> put_status(status) + |> text(message) + end + end def autograde_submission(conn, %{"submissionid" => submission_id}) do course_reg = conn.assigns[:course_reg] diff --git a/lib/cadet_web/router.ex b/lib/cadet_web/router.ex index f7d71ad9f..962a38f52 100644 --- a/lib/cadet_web/router.ex +++ b/lib/cadet_web/router.ex @@ -114,7 +114,12 @@ defmodule CadetWeb.Router do get("/grading/summary", AdminGradingController, :grading_summary) post("/grading/:assessmentid/publish_all_grades", AdminGradingController, :publish_all_grades) - post("/grading/:assessmentid/unpublish_all_grades", AdminGradingController, :unpublish_all_grades) + + post( + "/grading/:assessmentid/unpublish_all_grades", + AdminGradingController, + :unpublish_all_grades + ) get("/grading/:submissionid", AdminGradingController, :show) post("/grading/:submissionid/unsubmit", AdminGradingController, :unsubmit) From a33ff69a2e14b91a9815bd2be50eec5eafcb4006 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 15:52:29 +0800 Subject: [PATCH 55/74] feat: Implement auto publish for mcq/voting questions --- lib/cadet/jobs/autograder/grading_job.ex | 12 +- .../jobs/autograder/grading_job_test.exs | 293 +++++++++++++++++- 2 files changed, 290 insertions(+), 15 deletions(-) diff --git a/lib/cadet/jobs/autograder/grading_job.ex b/lib/cadet/jobs/autograder/grading_job.ex index 89afb69ba..78e41bf2f 100644 --- a/lib/cadet/jobs/autograder/grading_job.ex +++ b/lib/cadet/jobs/autograder/grading_job.ex @@ -9,8 +9,10 @@ defmodule Cadet.Autograder.GradingJob do require Logger + alias Cadet.Assessments alias Cadet.Assessments.{Answer, Assessment, Question, Submission, SubmissionVotes} alias Cadet.Autograder.Utilities + alias Cadet.Courses.AssessmentConfig alias Cadet.Jobs.Log def close_and_make_empty_submission(assessment = %Assessment{id: id}) do @@ -258,6 +260,14 @@ defmodule Cadet.Autograder.GradingJob do ) end - defp grade_submission_question_answer_lists(_, [], [], _, _) do + defp grade_submission_question_answer_lists(submission_id, [], [], _, _) do + submission = Repo.get(Submission, submission_id) + assessment = Repo.get(Assessment, submission.assessment_id) + assessment_config = Repo.get_by(AssessmentConfig, id: assessment.config_id) + is_grading_auto_published = assessment_config.is_grading_auto_published + + if Assessments.is_fully_autograded?(submission_id) and is_grading_auto_published do + Assessments.publish_grading(submission_id) + end end end diff --git a/test/cadet/jobs/autograder/grading_job_test.exs b/test/cadet/jobs/autograder/grading_job_test.exs index eaac9903e..44b333553 100644 --- a/test/cadet/jobs/autograder/grading_job_test.exs +++ b/test/cadet/jobs/autograder/grading_job_test.exs @@ -411,10 +411,12 @@ defmodule Cadet.Autograder.GradingJobTest do end end - describe "#grade_all_due_yesterday, all mcq questions" do + describe "#grade_all_due_yesterday, all mcq questions, grading set to auto publish" do setup do course = insert(:course) - assessment_config = insert(:assessment_config, %{course: course}) + + assessment_config = + insert(:assessment_config, %{course: course, is_grading_auto_published: true}) assessments = insert_list(3, :assessment, %{ @@ -441,9 +443,10 @@ defmodule Cadet.Autograder.GradingJobTest do } do student = insert(:course_registration, %{course: course, role: :student}) - for {assessment, _} <- assessments do - insert(:submission, %{student: student, assessment: assessment, status: :attempting}) - end + submissions = + Enum.map(assessments, fn {assessment, _} -> + insert(:submission, %{student: student, assessment: assessment, status: :attempting}) + end) GradingJob.grade_all_due_yesterday() @@ -456,6 +459,12 @@ defmodule Cadet.Autograder.GradingJobTest do assert Enum.count(answers) == 9 + Enum.each(submissions, fn submission -> + submission = Repo.get(Submission, submission.id) + assert submission.status == :submitted + assert submission.is_grading_published == true + end) + for answer <- answers do assert answer.xp == 0 assert answer.autograding_status == :success @@ -495,10 +504,128 @@ defmodule Cadet.Autograder.GradingJobTest do questions = Enum.flat_map(assessments, fn {_, questions} -> questions end) answers = Enum.flat_map(submissions_answers, fn {_, answers} -> answers end) - for submission <- submissions do - assert Repo.get(Submission, submission.id).status == :submitted + Enum.each(submissions, fn submission -> + submission = Repo.get(Submission, submission.id) + assert submission.status == :submitted + assert submission.is_grading_published == true + end) + + for {answer, question} <- Enum.zip(answers, questions) do + answer_db = Repo.get(Answer, answer.id) + + # seeded questions have correct choice as 0 + if answer_db.answer["choice_id"] == 0 do + assert answer_db.xp == question.max_xp + else + assert answer_db.xp == 0 + end + + assert answer_db.autograding_status == :success + end + + assert Enum.empty?(JobsQueue.all()) + end + end + + describe "#grade_all_due_yesterday, all mcq questions, grading set to not auto publish" do + setup do + course = insert(:course) + + assessment_config = + insert(:assessment_config, %{course: course, is_grading_auto_published: false}) + + assessments = + insert_list(3, :assessment, %{ + is_published: true, + open_at: Timex.shift(Timex.now(), days: -5), + close_at: Timex.shift(Timex.now(), hours: -4), + config: assessment_config, + course: course + }) + + questions = + for assessment <- assessments do + insert_list(3, :mcq_question, %{max_xp: 200, assessment: assessment}) + end + + %{course: course, assessments: Enum.zip(assessments, questions)} + end + + test "all assessments attempted, all questions unanswered, " <> + "should insert empty answers, should not enqueue any", + %{ + course: course, + assessments: assessments + } do + student = insert(:course_registration, %{course: course, role: :student}) + + submissions = + Enum.map(assessments, fn {assessment, _} -> + insert(:submission, %{student: student, assessment: assessment, status: :attempting}) + end) + + GradingJob.grade_all_due_yesterday() + + answers = + Submission + |> where(student_id: ^student.id) + |> join(:inner, [s], a in assoc(s, :answers)) + |> select([_, a], a) + |> Repo.all() + + assert Enum.count(answers) == 9 + + Enum.each(submissions, fn submission -> + submission = Repo.get(Submission, submission.id) + assert submission.status == :submitted + assert submission.is_grading_published == false + end) + + for answer <- answers do + assert answer.xp == 0 + assert answer.autograding_status == :success + assert answer.answer == %{"choice_id" => 0} end + assert Enum.empty?(JobsQueue.all()) + end + + test "all assessments attempted, all questions answered, " <> + "should grade all questions, should not enqueue any", + %{ + course: course, + assessments: assessments + } do + student = insert(:course_registration, %{course: course, role: :student}) + + submissions_answers = + Enum.map(assessments, fn {assessment, questions} -> + submission = + insert(:submission, %{student: student, assessment: assessment, status: :attempted}) + + answers = + Enum.map(questions, fn question -> + insert(:answer, %{ + question: question, + submission: submission, + answer: build(:mcq_answer) + }) + end) + + {submission, answers} + end) + + GradingJob.grade_all_due_yesterday() + submissions = Enum.map(submissions_answers, fn {submission, _} -> submission end) + questions = Enum.flat_map(assessments, fn {_, questions} -> questions end) + answers = Enum.flat_map(submissions_answers, fn {_, answers} -> answers end) + + Enum.each(submissions, fn submission -> + submission = Repo.get(Submission, submission.id) + assert submission.status == :submitted + assert submission.is_grading_published == false + end) + for {answer, question} <- Enum.zip(answers, questions) do answer_db = Repo.get(Answer, answer.id) @@ -516,10 +643,12 @@ defmodule Cadet.Autograder.GradingJobTest do end end - describe "#grade_all_due_yesterday, all voting questions" do + describe "#grade_all_due_yesterday, all voting questions, grading set to auto publish" do setup do course = insert(:course) - assessment_config = insert(:assessment_config, %{course: course}) + + assessment_config = + insert(:assessment_config, %{course: course, is_grading_auto_published: true}) assessments = insert_list(3, :assessment, %{ @@ -546,9 +675,10 @@ defmodule Cadet.Autograder.GradingJobTest do } do student = insert(:course_registration, %{course: course, role: :student}) - for {assessment, _} <- assessments do - insert(:submission, %{student: student, assessment: assessment, status: :attempting}) - end + submissions = + Enum.map(assessments, fn {assessment, _} -> + insert(:submission, %{student: student, assessment: assessment, status: :attempting}) + end) GradingJob.grade_all_due_yesterday() @@ -561,6 +691,12 @@ defmodule Cadet.Autograder.GradingJobTest do assert Enum.count(answers) == 9 + Enum.each(submissions, fn submission -> + submission = Repo.get(Submission, submission.id) + assert submission.status == :submitted + assert submission.is_grading_published == true + end) + for answer <- answers do assert answer.xp == 0 assert answer.autograding_status == :success @@ -605,10 +741,139 @@ defmodule Cadet.Autograder.GradingJobTest do submissions = Enum.map(submissions_answers, fn {submission, _} -> submission end) answers = Enum.flat_map(submissions_answers, fn {_, answers} -> answers end) - for submission <- submissions do - assert Repo.get(Submission, submission.id).status == :submitted + Enum.each(submissions, fn submission -> + submission = Repo.get(Submission, submission.id) + assert submission.status == :submitted + assert submission.is_grading_published == true + end) + + for {question, answer} <- Enum.zip(questions, answers) do + is_nil_entries = + SubmissionVotes + |> where(voter_id: ^student.id) + |> where(question_id: ^question.id) + |> where([sv], is_nil(sv.score)) + |> Repo.exists?() + + answer_db = Repo.get(Answer, answer.id) + + if is_nil_entries do + assert answer_db.xp == 0 + else + assert answer_db.xp == question.max_xp + end + + assert answer_db.autograding_status == :success end + assert Enum.empty?(JobsQueue.all()) + end + end + + describe "#grade_all_due_yesterday, all voting questions, grading set to not auto publish" do + setup do + course = insert(:course) + + assessment_config = + insert(:assessment_config, %{course: course, is_grading_auto_published: false}) + + assessments = + insert_list(3, :assessment, %{ + is_published: true, + open_at: Timex.shift(Timex.now(), days: -5), + close_at: Timex.shift(Timex.now(), hours: -4), + config: assessment_config, + course: course + }) + + questions = + for assessment <- assessments do + insert_list(3, :voting_question, %{max_xp: 20, assessment: assessment}) + end + + %{course: course, assessments: Enum.zip(assessments, questions)} + end + + test "all assessments attempted, all questions unanswered, " <> + "should insert empty answers, should not enqueue any", + %{ + course: course, + assessments: assessments + } do + student = insert(:course_registration, %{course: course, role: :student}) + + submissions = + Enum.map(assessments, fn {assessment, _} -> + insert(:submission, %{student: student, assessment: assessment, status: :attempting}) + end) + + GradingJob.grade_all_due_yesterday() + + answers = + Submission + |> where(student_id: ^student.id) + |> join(:inner, [s], a in assoc(s, :answers)) + |> select([_, a], a) + |> Repo.all() + + assert Enum.count(answers) == 9 + + Enum.each(submissions, fn submission -> + submission = Repo.get(Submission, submission.id) + assert submission.status == :submitted + assert submission.is_grading_published == false + end) + + for answer <- answers do + assert answer.xp == 0 + assert answer.autograding_status == :success + assert answer.answer == %{"completed" => false} + end + + assert Enum.empty?(JobsQueue.all()) + end + + test "all assessments attempted, all questions aswered, " <> + "should grade all questions, should not enqueue any", + %{ + course: course, + assessments: assessments + } do + student = insert(:course_registration, %{course: course, role: :student}) + + submissions_answers = + for {assessment, questions} <- assessments do + submission = + insert(:submission, %{student: student, assessment: assessment, status: :attempted}) + + answers = + for question <- questions do + case Enum.random(0..1) do + 0 -> insert(:submission_vote, %{voter: student, question: question, score: 1}) + 1 -> insert(:submission_vote, %{voter: student, question: question}) + end + + insert(:answer, %{ + question: question, + submission: submission, + answer: build(:voting_answer) + }) + end + + {submission, answers} + end + + GradingJob.grade_all_due_yesterday() + questions = Enum.flat_map(assessments, fn {_, questions} -> questions end) + submissions = Enum.map(submissions_answers, fn {submission, _} -> submission end) + answers = Enum.flat_map(submissions_answers, fn {_, answers} -> answers end) + + Enum.each(submissions, fn submission -> + submission = Repo.get(Submission, submission.id) + assert submission.status == :submitted + assert submission.is_grading_published == false + end) + for {question, answer} <- Enum.zip(questions, answers) do is_nil_entries = SubmissionVotes From 858c74047b5811af11a825d4810fad47ab6dbd2f Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 15:56:16 +0800 Subject: [PATCH 56/74] feat: Implement auto publish for auto graded programming questions --- .../jobs/autograder/result_store_worker.ex | 28 +++++++++++++++---- .../autograder/result_store_worker_test.exs | 27 ++++++++++++++---- 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/lib/cadet/jobs/autograder/result_store_worker.ex b/lib/cadet/jobs/autograder/result_store_worker.ex index 0851de8de..059aed8d8 100644 --- a/lib/cadet/jobs/autograder/result_store_worker.ex +++ b/lib/cadet/jobs/autograder/result_store_worker.ex @@ -15,7 +15,9 @@ defmodule Cadet.Autograder.ResultStoreWorker do alias Ecto.Multi alias Cadet.Repo - alias Cadet.Assessments.Answer + alias Cadet.Assessments + alias Cadet.Assessments.{Answer, Assessment, Submission} + alias Cadet.Courses.AssessmentConfig def perform(params = %{answer_id: answer_id, result: result}) when is_ecto_id(answer_id) do @@ -53,7 +55,11 @@ defmodule Cadet.Autograder.ResultStoreWorker do end end - defp update_answer(answer = %Answer{}, result = %{status: status}, overwrite) do + defp update_answer( + answer = %Answer{submission_id: submission_id}, + result = %{status: status}, + overwrite + ) do xp = cond do result.max_score == 0 and length(result.result) > 0 -> @@ -79,8 +85,20 @@ defmodule Cadet.Autograder.ResultStoreWorker do changes = if(overwrite, do: Map.put(changes, :xp_adjustment, 0), else: changes) - answer - |> Answer.autograding_changeset(changes) - |> Repo.update() + res = + answer + |> Answer.autograding_changeset(changes) + |> Repo.update() + + submission = Repo.get(Submission, submission_id) + assessment = Repo.get(Assessment, submission.assessment_id) + assessment_config = Repo.get_by(AssessmentConfig, id: assessment.config_id) + is_grading_auto_published = assessment_config.is_grading_auto_published + + if Assessments.is_fully_autograded?(submission_id) and is_grading_auto_published do + Assessments.publish_grading(submission_id) + end + + res end end diff --git a/test/cadet/jobs/autograder/result_store_worker_test.exs b/test/cadet/jobs/autograder/result_store_worker_test.exs index a907b809d..57961ebef 100644 --- a/test/cadet/jobs/autograder/result_store_worker_test.exs +++ b/test/cadet/jobs/autograder/result_store_worker_test.exs @@ -3,11 +3,15 @@ defmodule Cadet.Autograder.ResultStoreWorkerTest do import ExUnit.CaptureLog - alias Cadet.Assessments.Answer + alias Cadet.Assessments.{Answer, Submission} alias Cadet.Autograder.ResultStoreWorker setup do - answer = insert(:answer, %{question: insert(:question), submission: insert(:submission)}) + assessment_config = insert(:assessment_config, %{is_grading_auto_published: true}) + assessment = insert(:assessment, %{config: assessment_config}) + question = insert(:question, %{assessment: assessment}) + submission = insert(:submission, %{status: :submitted, assessment: assessment}) + answer = insert(:answer, %{question: question, submission: submission}) success_no_errors = %{status: :success, score: 10, max_score: 10, result: []} success_with_errors = %{ @@ -68,7 +72,7 @@ defmodule Cadet.Autograder.ResultStoreWorkerTest do end describe "#perform, valid answer_id" do - test "before manual grading", %{answer: answer, results: results} do + test "before manual grading and grading auto published", %{answer: answer, results: results} do for result <- results do ResultStoreWorker.perform(%{answer_id: answer.id, result: result}) @@ -95,18 +99,27 @@ defmodule Cadet.Autograder.ResultStoreWorkerTest do ) end + submission = Repo.get(Submission, answer.submission_id) + + if result.status == :success do + assert submission.is_grading_published == true + else + assert submission.is_grading_published == false + end + assert answer.autograding_status == result.status assert answer.autograding_results == errors_string_keys end end - test "after manual grading", %{results: results} do + test "after manual grading and grading not auto published", %{results: results} do grader = insert(:course_registration, %{role: :staff}) + # Question uses default assessment config (is_grading_auto_published: false) answer = insert(:answer, %{ question: insert(:question), - submission: insert(:submission), + submission: insert(:submission, %{status: :submitted}), grader_id: grader.id }) @@ -136,6 +149,10 @@ defmodule Cadet.Autograder.ResultStoreWorkerTest do ) end + submission = Repo.get(Submission, answer.submission_id) + + assert submission.is_grading_published == false + assert answer.autograding_status == result.status assert answer.autograding_results == errors_string_keys end From f376f1e2a229666ce37d0f8b22c2646493279f61 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 24 Mar 2024 16:25:56 +0800 Subject: [PATCH 57/74] chore: Fix consistency issue --- lib/cadet/jobs/autograder/result_store_worker.ex | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/cadet/jobs/autograder/result_store_worker.ex b/lib/cadet/jobs/autograder/result_store_worker.ex index 059aed8d8..852253488 100644 --- a/lib/cadet/jobs/autograder/result_store_worker.ex +++ b/lib/cadet/jobs/autograder/result_store_worker.ex @@ -14,8 +14,7 @@ defmodule Cadet.Autograder.ResultStoreWorker do alias Ecto.Multi - alias Cadet.Repo - alias Cadet.Assessments + alias Cadet.{Assessments, Repo} alias Cadet.Assessments.{Answer, Assessment, Submission} alias Cadet.Courses.AssessmentConfig From 5b91c1232a2114775923471bf4d33bfe3d4f2280 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Thu, 28 Mar 2024 01:46:26 +0800 Subject: [PATCH 58/74] feat: Implement published and unpublished notifications and remove deprecated ones --- lib/cadet/accounts/notification_type.ex | 6 ++-- lib/cadet/accounts/notifications.ex | 20 ++++++------- lib/cadet/assessments/assessments.ex | 8 +++--- .../controllers/notifications_controller.ex | 4 +-- test/cadet/accounts/notification_test.exs | 28 +++++++++---------- .../accounts/notification_factory.ex | 2 +- 6 files changed, 32 insertions(+), 36 deletions(-) diff --git a/lib/cadet/accounts/notification_type.ex b/lib/cadet/accounts/notification_type.ex index 6e80266be..a5c62397e 100644 --- a/lib/cadet/accounts/notification_type.ex +++ b/lib/cadet/accounts/notification_type.ex @@ -3,14 +3,12 @@ import EctoEnum defenum(Cadet.Accounts.NotificationType, :notification_type, [ # Notifications for new assessments :new, - # Notifications for autograded assessments - :autograded, - # Notifications for manually graded assessments - :graded, # Notifications for unsubmitted submissions :unsubmitted, # Notifications for submitted assessments :submitted, + # Notifications for published grades + :published_grading, # Notifications for unpublished grades :unpublished_grading ]) diff --git a/lib/cadet/accounts/notifications.ex b/lib/cadet/accounts/notifications.ex index bbbc08635..920d2337c 100644 --- a/lib/cadet/accounts/notifications.ex +++ b/lib/cadet/accounts/notifications.ex @@ -145,18 +145,18 @@ defmodule Cadet.Accounts.Notifications do {:ok, Ecto.Schema.t()} | {:error, Ecto.Changeset.t()} def handle_unsubmit_notifications(assessment_id, student = %CourseRegistration{}) when is_ecto_id(assessment_id) do - # Fetch and delete all notifications of :autograded and :graded + # Fetch and delete all notifications of :unpublished_grading # Add new notification :unsubmitted Notification |> where(course_reg_id: ^student.id) |> where(assessment_id: ^assessment_id) - |> where([n], n.type in ^[:autograded, :graded, :unpublished_grading]) + |> where([n], n.type in ^[:unpublished_grading]) |> Repo.delete_all() write(%{ type: :unsubmitted, - role: student.role, + role: :student, course_reg_id: student.id, assessment_id: assessment_id }) @@ -169,18 +169,18 @@ defmodule Cadet.Accounts.Notifications do {:ok, Ecto.Schema.t()} | {:error, Ecto.Changeset.t()} def handle_unpublish_grades_notifications(assessment_id, student = %CourseRegistration{}) when is_ecto_id(assessment_id) do - # Fetch and delete all notifications of :graded - # Add new notification :unpublished + # Fetch and delete all notifications of :published + # Add new notification :unpublished_grading Notification |> where(course_reg_id: ^student.id) |> where(assessment_id: ^assessment_id) - |> where([n], n.type in ^[:autograded, :graded, :unsubmitted]) + |> where([n], n.type in ^[:published_grading]) |> Repo.delete_all() - write(%{ type: :unpublished_grading, - role: student.role, + read: false, + role: :student, course_reg_id: student.id, assessment_id: assessment_id }) @@ -190,9 +190,9 @@ defmodule Cadet.Accounts.Notifications do Writes a notification that a student's submission has been graded successfully. (for the student) """ - @spec write_notification_when_graded(integer(), any()) :: + @spec write_notification_when_published(integer(), any()) :: {:ok, Ecto.Schema.t()} | {:error, Ecto.Changeset.t()} - def write_notification_when_graded(submission_id, type) when type in [:graded, :autograded] do + def write_notification_when_published(submission_id, type) when type in [:published_grading] do case Repo.get(Submission, submission_id) do nil -> {:error, %Ecto.Changeset{}} diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 21330b634..a572f8c7e 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1177,9 +1177,9 @@ defmodule Cadet.Assessments do |> Submission.changeset(%{is_grading_published: true}) |> Repo.update() - Notifications.write_notification_when_graded( + Notifications.write_notification_when_published( submission.id, - :graded + :published_grading ) {:ok, nil} @@ -1219,9 +1219,9 @@ defmodule Cadet.Assessments do |> Submission.changeset(%{is_grading_published: true}) |> Repo.update() - Notifications.write_notification_when_graded( + Notifications.write_notification_when_published( submission.id, - :autograded + :published_grading ) {:ok, nil} diff --git a/lib/cadet_web/controllers/notifications_controller.ex b/lib/cadet_web/controllers/notifications_controller.ex index 544b85344..efe9427b2 100644 --- a/lib/cadet_web/controllers/notifications_controller.ex +++ b/lib/cadet_web/controllers/notifications_controller.ex @@ -103,12 +103,10 @@ defmodule CadetWeb.NotificationsController do enum([ :new, - :deadline, - :autograded, - :graded, :submitted, :unsubmitted, :unpublishedGrading, + :publishedGrading, :new_message ]) end diff --git a/test/cadet/accounts/notification_test.exs b/test/cadet/accounts/notification_test.exs index 7d0ece257..638ca229d 100644 --- a/test/cadet/accounts/notification_test.exs +++ b/test/cadet/accounts/notification_test.exs @@ -320,28 +320,28 @@ defmodule Cadet.Accounts.NotificationTest do student: student, individual_submission: individual_submission } do - Notifications.write_notification_when_graded(individual_submission.id, :autograded) + Notifications.write_notification_when_published(individual_submission.id, :published_grading) notification = Repo.get_by(Notification, course_reg_id: student.id, - type: :autograded, + type: :published_grading, assessment_id: assessment.id ) - assert %{type: :autograded} = notification + assert %{type: :published_grading} = notification end test "no notification when no submission", %{ assessment: assessment, student: student } do - Notifications.write_notification_when_graded(-1, :autograded) + Notifications.write_notification_when_published(-1, :published_grading) notification = Repo.get_by(Notification, course_reg_id: student.id, - type: :autograded, + type: :published_grading, assessment_id: assessment.id ) @@ -353,7 +353,7 @@ defmodule Cadet.Accounts.NotificationTest do team: team, team_submission: team_submission } do - Notifications.write_notification_when_graded(team_submission.id, :autograded) + Notifications.write_notification_when_published(team_submission.id, :published_grading) team_members = Repo.all(from(tm in TeamMember, where: tm.team_id == ^team.id, preload: :student)) @@ -364,11 +364,11 @@ defmodule Cadet.Accounts.NotificationTest do notification = Repo.get_by(Notification, course_reg_id: student.id, - type: :autograded, + type: :published_grading, assessment_id: assessment.id ) - assert %{type: :autograded} = notification + assert %{type: :published_grading} = notification end) end @@ -377,16 +377,16 @@ defmodule Cadet.Accounts.NotificationTest do student: student, individual_submission: individual_submission } do - Notifications.write_notification_when_graded(individual_submission.id, :graded) + Notifications.write_notification_when_published(individual_submission.id, :published_grading) notification = Repo.get_by(Notification, course_reg_id: student.id, - type: :graded, + type: :published_grading, assessment_id: assessment.id ) - assert %{type: :graded} = notification + assert %{type: :published_grading} = notification end test "receives notification when maunally graded [team submission]", %{ @@ -394,7 +394,7 @@ defmodule Cadet.Accounts.NotificationTest do team: team, team_submission: team_submission } do - Notifications.write_notification_when_graded(team_submission.id, :graded) + Notifications.write_notification_when_published(team_submission.id, :published_grading) team_members = Repo.all(from(tm in TeamMember, where: tm.team_id == ^team.id, preload: :student)) @@ -405,11 +405,11 @@ defmodule Cadet.Accounts.NotificationTest do notification = Repo.get_by(Notification, course_reg_id: student.id, - type: :graded, + type: :published_grading, assessment_id: assessment.id ) - assert %{type: :graded} = notification + assert %{type: :published_grading} = notification end) end diff --git a/test/factories/accounts/notification_factory.ex b/test/factories/accounts/notification_factory.ex index 0dc933180..847d0d454 100644 --- a/test/factories/accounts/notification_factory.ex +++ b/test/factories/accounts/notification_factory.ex @@ -8,7 +8,7 @@ defmodule Cadet.Accounts.NotificationFactory do alias Cadet.Accounts.Notification def notification_factory do - valid_types = [:new, :autograded, :graded] + valid_types = [:new] %Notification{ type: Enum.random(valid_types), From 64b5305429d94d497b6263569fe895e9cf2007e7 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Thu, 28 Mar 2024 02:11:14 +0800 Subject: [PATCH 59/74] feat: Include isGradingAutoPublished in response for assessment configs --- lib/cadet/accounts/notifications.ex | 1 + lib/cadet_web/admin_views/admin_courses_view.ex | 3 ++- test/cadet/accounts/notification_test.exs | 10 ++++++++-- .../admin_courses_controller_test.exs | 9 ++++++--- 4 files changed, 17 insertions(+), 6 deletions(-) diff --git a/lib/cadet/accounts/notifications.ex b/lib/cadet/accounts/notifications.ex index 920d2337c..7d35112ae 100644 --- a/lib/cadet/accounts/notifications.ex +++ b/lib/cadet/accounts/notifications.ex @@ -177,6 +177,7 @@ defmodule Cadet.Accounts.Notifications do |> where(assessment_id: ^assessment_id) |> where([n], n.type in ^[:published_grading]) |> Repo.delete_all() + write(%{ type: :unpublished_grading, read: false, diff --git a/lib/cadet_web/admin_views/admin_courses_view.ex b/lib/cadet_web/admin_views/admin_courses_view.ex index 2804eb664..ba5ec6820 100644 --- a/lib/cadet_web/admin_views/admin_courses_view.ex +++ b/lib/cadet_web/admin_views/admin_courses_view.ex @@ -14,7 +14,8 @@ defmodule CadetWeb.AdminCoursesView do earlySubmissionXp: :early_submission_xp, hasVotingFeatures: :has_voting_features, hasTokenCounter: :has_token_counter, - hoursBeforeEarlyXpDecay: :hours_before_early_xp_decay + hoursBeforeEarlyXpDecay: :hours_before_early_xp_decay, + isGradingAutoPublished: :is_grading_auto_published }) end end diff --git a/test/cadet/accounts/notification_test.exs b/test/cadet/accounts/notification_test.exs index 638ca229d..ec206e0e1 100644 --- a/test/cadet/accounts/notification_test.exs +++ b/test/cadet/accounts/notification_test.exs @@ -320,7 +320,10 @@ defmodule Cadet.Accounts.NotificationTest do student: student, individual_submission: individual_submission } do - Notifications.write_notification_when_published(individual_submission.id, :published_grading) + Notifications.write_notification_when_published( + individual_submission.id, + :published_grading + ) notification = Repo.get_by(Notification, @@ -377,7 +380,10 @@ defmodule Cadet.Accounts.NotificationTest do student: student, individual_submission: individual_submission } do - Notifications.write_notification_when_published(individual_submission.id, :published_grading) + Notifications.write_notification_when_published( + individual_submission.id, + :published_grading + ) notification = Repo.get_by(Notification, diff --git a/test/cadet_web/admin_controllers/admin_courses_controller_test.exs b/test/cadet_web/admin_controllers/admin_courses_controller_test.exs index c9a3096c2..cb4c1e30f 100644 --- a/test/cadet_web/admin_controllers/admin_courses_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_courses_controller_test.exs @@ -176,7 +176,8 @@ defmodule CadetWeb.AdminCoursesControllerTest do "type" => "Mission1", "assessmentConfigId" => config1.id, "hasVotingFeatures" => false, - "hasTokenCounter" => false + "hasTokenCounter" => false, + "isGradingAutoPublished" => false }, %{ "earlySubmissionXp" => 200, @@ -186,7 +187,8 @@ defmodule CadetWeb.AdminCoursesControllerTest do "type" => "Mission2", "assessmentConfigId" => config2.id, "hasVotingFeatures" => true, - "hasTokenCounter" => true + "hasTokenCounter" => true, + "isGradingAutoPublished" => false }, %{ "earlySubmissionXp" => 200, @@ -196,7 +198,8 @@ defmodule CadetWeb.AdminCoursesControllerTest do "type" => "Mission3", "assessmentConfigId" => config3.id, "hasVotingFeatures" => false, - "hasTokenCounter" => false + "hasTokenCounter" => false, + "isGradingAutoPublished" => false } ] From 21c65f6638e162fc2bae181c20d477c3b1ef7707 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Mon, 1 Apr 2024 16:59:44 +0800 Subject: [PATCH 60/74] fix: Include sending of notifications when publishing/unpublishing all --- lib/cadet/assessments/assessments.ex | 75 ++++++++++++++++++---------- 1 file changed, 49 insertions(+), 26 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index a572f8c7e..fca68363f 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1260,23 +1260,34 @@ defmodule Cadet.Assessments do question_count: count(q.id) }) - Submission - |> join(:inner, [s], ans in subquery(answers_query), on: ans.submission_id == s.id) - |> join(:inner, [s, ans], asst in subquery(question_query), - on: s.assessment_id == asst.assessment_id - ) - |> join(:inner, [s, ans, asst], cr in CourseRegistration, on: s.student_id == cr.id) - |> where([s, ans, asst, cr], cr.course_id == ^publisher.course_id) - |> where( - [s, ans, asst, cr], - asst.question_count == ans.graded_count or asst.question_count == ans.autograded_count - ) - |> where([s, ans, asst, cr], s.is_grading_published == false) - |> where([s, ans, asst, cr], s.assessment_id == ^assessment_id) - |> select([s, ans, asst, cr], %{ - id: s.id - }) - |> Repo.update_all(set: [is_grading_published: true]) + submission_query = + Submission + |> join(:inner, [s], ans in subquery(answers_query), on: ans.submission_id == s.id) + |> join(:inner, [s, ans], asst in subquery(question_query), + on: s.assessment_id == asst.assessment_id + ) + |> join(:inner, [s, ans, asst], cr in CourseRegistration, on: s.student_id == cr.id) + |> where([s, ans, asst, cr], cr.course_id == ^publisher.course_id) + |> where( + [s, ans, asst, cr], + asst.question_count == ans.graded_count or asst.question_count == ans.autograded_count + ) + |> where([s, ans, asst, cr], s.is_grading_published == false) + |> where([s, ans, asst, cr], s.assessment_id == ^assessment_id) + |> select([s, ans, asst, cr], %{ + id: s.id + }) + + submissions = Repo.all(submission_query) + + Repo.update_all(submission_query, set: [is_grading_published: true]) + + Enum.each(submissions, fn submission -> + Notifications.write_notification_when_published( + submission.id, + :published_grading + ) + end) {:ok, nil} else @@ -1286,15 +1297,27 @@ defmodule Cadet.Assessments do def unpublish_all(publisher = %CourseRegistration{}, assessment_id) do if publisher.role == :admin do - Submission - |> join(:inner, [s], cr in CourseRegistration, on: s.student_id == cr.id) - |> where([s, cr], cr.course_id == ^publisher.course_id) - |> where([s, cr], s.is_grading_published == true) - |> where([s, cr], s.assessment_id == ^assessment_id) - |> select([s, cr], %{ - id: s.id - }) - |> Repo.update_all(set: [is_grading_published: false]) + submission_query = + Submission + |> join(:inner, [s], cr in CourseRegistration, on: s.student_id == cr.id) + |> where([s, cr], cr.course_id == ^publisher.course_id) + |> where([s, cr], s.is_grading_published == true) + |> where([s, cr], s.assessment_id == ^assessment_id) + |> select([s, cr], %{ + id: s.id, + student_id: cr.id + }) + + submissions = Repo.all(submission_query) + + Repo.update_all(submission_query, set: [is_grading_published: false]) + + Enum.each(submissions, fn submission -> + Notifications.handle_unpublish_grades_notifications( + assessment_id, + Repo.get(CourseRegistration, submission.student_id) + ) + end) {:ok, nil} else From 596d3c3f1fda0e8f80e55641967e3f80b12257c5 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Tue, 2 Apr 2024 19:55:27 +0800 Subject: [PATCH 61/74] docs: Add docs for functions implemented --- lib/cadet/accounts/notifications.ex | 3 +- lib/cadet/assessments/assessments.ex | 62 ++++++++++++++++++++++------ 2 files changed, 50 insertions(+), 15 deletions(-) diff --git a/lib/cadet/accounts/notifications.ex b/lib/cadet/accounts/notifications.ex index 7d35112ae..175928089 100644 --- a/lib/cadet/accounts/notifications.ex +++ b/lib/cadet/accounts/notifications.ex @@ -164,13 +164,12 @@ defmodule Cadet.Accounts.Notifications do @doc """ Function that handles notifications when a submission grade is unpublished. + Deletes all :published notifications and adds a new :unpublished_grading notification. """ @spec handle_unpublish_grades_notifications(integer(), CourseRegistration.t()) :: {:ok, Ecto.Schema.t()} | {:error, Ecto.Changeset.t()} def handle_unpublish_grades_notifications(assessment_id, student = %CourseRegistration{}) when is_ecto_id(assessment_id) do - # Fetch and delete all notifications of :published - # Add new notification :unpublished_grading Notification |> where(course_reg_id: ^student.id) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index fca68363f..772e95cbb 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1111,8 +1111,12 @@ defmodule Cadet.Assessments do end end - # This function changes the is_grading_published field of the submission to false - # and sends a notification to the student + @doc """ + Unpublishes grading for a submission and send notification to student. + Requires admin or staff who is group leader of student. + + Returns `{:ok, nil}` on success, otherwise `{:error, {status, message}}`. + """ def unpublish_grading(submission_id, cr = %CourseRegistration{id: course_reg_id, role: role}) when is_ecto_id(submission_id) do submission = @@ -1152,9 +1156,12 @@ defmodule Cadet.Assessments do end end - # This function changes the is_grading_published field of the submission to true - # only if all the answers are graded - # and sends a notification to the student + @doc """ + Publishes grading for a submission and send notification to student. + Requires admin or staff who is group leader of student and all answers to be graded. + + Returns `{:ok, nil}` on success, otherwise `{:error, {status, message}}`. + """ def publish_grading(submission_id, cr = %CourseRegistration{id: course_reg_id, role: role}) when is_ecto_id(submission_id) do submission = @@ -1204,7 +1211,12 @@ defmodule Cadet.Assessments do end end - # Used by the auto-grading system to publish grading (Bypasses Course Reg checks) + @doc """ + Publishes grading for a submission and send notification to student. + This function is used by the auto-grading system to publish grading. Bypasses Course Reg checks. + + Returns `{:ok, nil}` on success, otherwise `{:error, {status, message}}`. + """ def publish_grading(submission_id) when is_ecto_id(submission_id) do submission = @@ -1240,6 +1252,12 @@ defmodule Cadet.Assessments do end end + @doc """ + Publishes grading for all graded submissions for an assessment and sends notifications to students. + Requires admin. + + Returns `{:ok, nil}` on success, otherwise `{:error, {status, message}}`. + """ def publish_all_graded(publisher = %CourseRegistration{}, assessment_id) do if publisher.role == :admin do answers_query = @@ -1295,6 +1313,13 @@ defmodule Cadet.Assessments do end end + @doc """ + Unpublishes grading for all submissions with grades published for an assessment and sends notifications to students. + Requires admin role. + + Returns `{:ok, nil}` on success, otherwise `{:error, {status, message}}`. + """ + def unpublish_all(publisher = %CourseRegistration{}, assessment_id) do if publisher.role == :admin do submission_query = @@ -1734,13 +1759,24 @@ defmodule Cadet.Assessments do The input parameters are the user and query parameters. Query parameters are used to filter the submissions. - 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."}} + The return value is `{:ok, %{"count": count, "data": submissions}}` + + # Parameters + - `pageSize`: Integer. The number of submissions to return. Default is 10. + - `offset`: Integer. The number of submissions to skip. Default is 0. + - `title`: String. Assessment title. + - `status`: String. Submission status. + - `isFullyGraded`: Boolean. Whether the submission is fully graded. + - `isGradingPublished`: Boolean. Whether the grading is published. + - `group`: Boolean. Only the groups under the grader should be returned. + - `groupName`: String. Group name. + - `name`: String. User name. + - `username`: String. User username. + - `type`: String. Assessment Config type. + - `isManuallyGraded`: Boolean. Whether the assessment is manually graded. + + # Implementation + Uses helper functions to build the filter query. Helper functions are separated by tables in the database. """ @spec submissions_by_grader_for_index(CourseRegistration.t()) :: From 340de3a1e7cc16da0a3f51edd7e9b7ad18fe29c4 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Tue, 2 Apr 2024 20:04:40 +0800 Subject: [PATCH 62/74] chore: Update wording for tests --- test/cadet/assessments/assessments_test.exs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index 36ce7b0a4..f74f5fe28 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -2125,7 +2125,7 @@ defmodule Cadet.AssessmentsTest do end end - describe "get submission function" do + describe "submissions_by_grader_for_index function" do setup do seed = Cadet.Test.Seeds.assessments() @@ -2751,7 +2751,7 @@ defmodule Cadet.AssessmentsTest do end end - describe "is_fully_autograded? helper function" do + describe "is_fully_autograded? function" do setup do assessment = insert(:assessment) student = insert(:course_registration, role: :student) @@ -2798,7 +2798,7 @@ defmodule Cadet.AssessmentsTest do end end - describe "publish and unpublish all submissions for an assessment" do + describe "publish and unpublish all grading" do setup do Cadet.Test.Seeds.assessments() end From b2ab4b0f90eeb3e514315bc18a6dec42b3d5be65 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Tue, 2 Apr 2024 20:08:45 +0800 Subject: [PATCH 63/74] chore: Remove unused variables --- lib/cadet/accounts/notifications.ex | 1 - test/cadet/assessments/assessments_test.exs | 17 ++++++----------- .../admin_grading_controller_test.exs | 4 ++-- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/lib/cadet/accounts/notifications.ex b/lib/cadet/accounts/notifications.ex index 175928089..0dd5d8cc5 100644 --- a/lib/cadet/accounts/notifications.ex +++ b/lib/cadet/accounts/notifications.ex @@ -170,7 +170,6 @@ defmodule Cadet.Accounts.Notifications do {:ok, Ecto.Schema.t()} | {:error, Ecto.Changeset.t()} def handle_unpublish_grades_notifications(assessment_id, student = %CourseRegistration{}) when is_ecto_id(assessment_id) do - Notification |> where(course_reg_id: ^student.id) |> where(assessment_id: ^assessment_id) diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index f74f5fe28..f0cd41ea4 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -2029,7 +2029,7 @@ defmodule Cadet.AssessmentsTest do is_grading_published: false ) - answer = + _answer = insert( :answer, submission: submission, @@ -2061,7 +2061,7 @@ defmodule Cadet.AssessmentsTest do is_grading_published: true ) - published_answer = + _published_answer = insert( :answer, submission: published_submission, @@ -2236,7 +2236,7 @@ defmodule Cadet.AssessmentsTest do end test "filter by submission fully graded", %{ - course_regs: %{avenger1_cr: avenger, students: students}, + course_regs: %{avenger1_cr: avenger}, assessments: assessments, total_submissions: total_submissions, students_with_assessment_info: students_with_assessment_info @@ -2261,7 +2261,7 @@ defmodule Cadet.AssessmentsTest do end test "filter by submission not fully graded", %{ - course_regs: %{avenger1_cr: avenger, students: students}, + course_regs: %{avenger1_cr: avenger}, assessments: assessments, total_submissions: total_submissions, students_with_assessment_info: students_with_assessment_info @@ -2286,7 +2286,7 @@ defmodule Cadet.AssessmentsTest do end test "filter by submission published", %{ - course_regs: %{avenger1_cr: avenger, students: students}, + course_regs: %{avenger1_cr: avenger}, assessments: assessments, total_submissions: total_submissions, students_with_assessment_info: students_with_assessment_info @@ -2313,7 +2313,7 @@ defmodule Cadet.AssessmentsTest do end test "filter by submission not published", %{ - course_regs: %{avenger1_cr: avenger, students: students}, + course_regs: %{avenger1_cr: avenger}, assessments: assessments, total_submissions: total_submissions, students_with_assessment_info: students_with_assessment_info @@ -2603,7 +2603,6 @@ defmodule Cadet.AssessmentsTest do test "filter by assessment config 1", %{ course_regs: %{avenger1_cr: avenger, students: students}, - assessments: assessments, assessment_configs: assessment_configs, total_submissions: total_submissions } do @@ -2632,7 +2631,6 @@ defmodule Cadet.AssessmentsTest do test "filter by assessment config 2", %{ course_regs: %{avenger1_cr: avenger, students: students}, - assessments: assessments, assessment_configs: assessment_configs, total_submissions: total_submissions } do @@ -2662,7 +2660,6 @@ defmodule Cadet.AssessmentsTest do test "filter by assessment config 3", %{ course_regs: %{avenger1_cr: avenger, students: students}, - assessments: assessments, assessment_configs: assessment_configs, total_submissions: total_submissions } do @@ -2692,7 +2689,6 @@ defmodule Cadet.AssessmentsTest do test "filter by assessment config manually graded", %{ course_regs: %{avenger1_cr: avenger, students: students}, - assessments: assessments, assessment_configs: assessment_configs, total_submissions: total_submissions } do @@ -2722,7 +2718,6 @@ defmodule Cadet.AssessmentsTest do test "filter by assessment config not manually graded", %{ course_regs: %{avenger1_cr: avenger, students: students}, - assessments: assessments, assessment_configs: assessment_configs, total_submissions: total_submissions } do 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 f0a0f9329..b2f7a2c4f 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -888,7 +888,7 @@ defmodule CadetWeb.AdminGradingControllerTest do is_grading_published: true ) - answer = + _answer = insert( :answer, submission: submission, @@ -977,7 +977,7 @@ defmodule CadetWeb.AdminGradingControllerTest do is_grading_published: false ) - answer = + _answer = insert( :answer, submission: submission, From a25f9f7fe45794f86316991de0c4f644b70c5ef0 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sat, 6 Apr 2024 15:03:14 +0800 Subject: [PATCH 64/74] feat: Implement test for unpublish and publish all routes --- .../admin_grading_controller_test.exs | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) 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 b2f7a2c4f..2c0e3503a 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -1067,6 +1067,84 @@ defmodule CadetWeb.AdminGradingControllerTest do end end + describe "POST /:assessmentid/unpublish_all_grades" do + setup %{conn: conn} do + seed = Cadet.Test.Seeds.assessments() + assessment_id = seed[:assessments]["mission"][:assessment].id + %{conn: conn, assessment_id: assessment_id, course: seed[:courses][:course1]} + end + + @tag authenticate: :admin + test "successful", %{conn: conn, assessment_id: assessment_id, course: course} do + admin = insert(:course_registration, %{role: :admin, course: course}) + + conn = + conn + |> sign_in(admin.user) + |> post(build_url_unpublish_all(course.id, assessment_id)) + + assert response(conn, 200) == "OK" + end + + @tag authenticate: :staff + test "staff not allowed to unpublish all grades", %{ + conn: conn, + assessment_id: assessment_id, + course: course + } do + staff = insert(:course_registration, %{role: :staff, course: course}) + + conn = + conn + |> sign_in(staff.user) + + conn = + conn + |> post(build_url_unpublish_all(course.id, assessment_id)) + + assert response(conn, 403) == "Only Admin is permitted to publish all grades" + end + end + + describe "POST /:assessmentid/publish_all_grades" do + setup %{conn: conn} do + seed = Cadet.Test.Seeds.assessments() + assessment_id = seed[:assessments]["mission"][:assessment].id + %{conn: conn, assessment_id: assessment_id, course: seed[:courses][:course1]} + end + + @tag authenticate: :admin + test "successful", %{conn: conn, assessment_id: assessment_id, course: course} do + admin = insert(:course_registration, %{role: :admin, course: course}) + + conn = + conn + |> sign_in(admin.user) + |> post(build_url_publish_all(course.id, assessment_id)) + + assert response(conn, 200) == "OK" + end + + @tag authenticate: :staff + test "staff not allowed to publish all grades", %{ + conn: conn, + assessment_id: assessment_id, + course: course + } do + staff = insert(:course_registration, %{role: :staff, course: course}) + + conn = + conn + |> sign_in(staff.user) + + conn = + conn + |> post(build_url_publish_all(course.id, assessment_id)) + + assert response(conn, 403) == "Only Admin is permitted to publish all grades" + end + end + describe "GET /, admin" do @tag authenticate: :staff test "can see all submissions", %{conn: conn} do @@ -1596,6 +1674,12 @@ defmodule CadetWeb.AdminGradingControllerTest do defp build_url_publish(course_id, submission_id), do: "#{build_url(course_id, submission_id)}/publish_grades" + defp build_url_unpublish_all(course_id, assessment_id), + do: "#{build_url(course_id)}#{assessment_id}/unpublish_all_grades" + + defp build_url_publish_all(course_id, assessment_id), + do: "#{build_url(course_id)}#{assessment_id}/publish_all_grades" + defp build_url_autograde(course_id, submission_id), do: "#{build_url(course_id, submission_id)}/autograde" From fd1508b095ace803bc6b9ecbd142b7d7db60b1f7 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 7 Apr 2024 18:00:03 +0800 Subject: [PATCH 65/74] feat: Implement guard for publish/unpublish grades This commit prevents admin/staff from individually publishing/unpublishing grades for students. --- lib/cadet/assessments/assessments.ex | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 772e95cbb..e0b737018 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1115,6 +1115,9 @@ defmodule Cadet.Assessments do Unpublishes grading for a submission and send notification to student. Requires admin or staff who is group leader of student. + Only manually graded assessments can be individually unpublished. We can only + unpublish all submissions for auto-graded assessments. + Returns `{:ok, nil}` on success, otherwise `{:error, {status, message}}`. """ def unpublish_grading(submission_id, cr = %CourseRegistration{id: course_reg_id, role: role}) @@ -1122,13 +1125,16 @@ defmodule Cadet.Assessments do submission = Submission |> join(:inner, [s], a in assoc(s, :assessment)) - |> preload([_, a], assessment: a) + |> join(:inner, [_, a], c in assoc(a, :config)) + |> preload([_, a, c], assessment: {a, config: c}) |> Repo.get(submission_id) # allows staff to unpublish own assessment bypass = role in @bypass_closed_roles and submission.student_id == course_reg_id with {:submission_found?, true} <- {:submission_found?, is_map(submission)}, + {:is_manually_graded?, true} <- + {:is_manually_graded?, submission.assessment.config.is_manually_graded}, {:allowed_to_unpublish?, true} <- {:allowed_to_unpublish?, role == :admin or bypass or @@ -1151,6 +1157,10 @@ defmodule Cadet.Assessments do {:error, {:forbidden, "Only Avenger of student or Admin is permitted to unpublish grading"}} + {:is_manually_graded?, false} -> + {:error, + {:bad_request, "Only manually graded assessments can be individually unpublished"}} + _ -> {:error, {:internal_server_error, "Please try again later."}} end @@ -1160,6 +1170,9 @@ defmodule Cadet.Assessments do Publishes grading for a submission and send notification to student. Requires admin or staff who is group leader of student and all answers to be graded. + Only manually graded assessments can be individually published. We can only + publish all submissions for auto-graded assessments. + Returns `{:ok, nil}` on success, otherwise `{:error, {status, message}}`. """ def publish_grading(submission_id, cr = %CourseRegistration{id: course_reg_id, role: role}) @@ -1167,7 +1180,8 @@ defmodule Cadet.Assessments do submission = Submission |> join(:inner, [s], a in assoc(s, :assessment)) - |> preload([_, a], assessment: a) + |> join(:inner, [_, a], c in assoc(a, :config)) + |> preload([_, a, c], assessment: {a, config: c}) |> Repo.get(submission_id) # allows staff to publish own assessment @@ -1175,6 +1189,8 @@ defmodule Cadet.Assessments do with {:submission_found?, true} <- {:submission_found?, is_map(submission)}, {:status, :submitted} <- {:status, submission.status}, + {:is_manually_graded?, true} <- + {:is_manually_graded?, submission.assessment.config.is_manually_graded}, {:fully_graded?, true} <- {:fully_graded?, is_fully_graded?(submission_id)}, {:allowed_to_publish?, true} <- {:allowed_to_publish?, @@ -1203,6 +1219,9 @@ defmodule Cadet.Assessments do {:allowed_to_publish?, false} -> {:error, {:forbidden, "Only Avenger of student or Admin is permitted to publish grading"}} + {:is_manually_graded?, false} -> + {:error, {:bad_request, "Only manually graded assessments can be individually published"}} + {:fully_graded?, false} -> {:error, {:bad_request, "Some answers are not graded"}} From 4ac87afa0d6a3e204b80524f39132e3a6f252384 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 7 Apr 2024 18:20:46 +0800 Subject: [PATCH 66/74] refactor: Change notification types in swagger_schema --- lib/cadet_web/controllers/notifications_controller.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cadet_web/controllers/notifications_controller.ex b/lib/cadet_web/controllers/notifications_controller.ex index efe9427b2..19c436cd9 100644 --- a/lib/cadet_web/controllers/notifications_controller.ex +++ b/lib/cadet_web/controllers/notifications_controller.ex @@ -105,8 +105,8 @@ defmodule CadetWeb.NotificationsController do :new, :submitted, :unsubmitted, - :unpublishedGrading, - :publishedGrading, + :unpublished_grading, + :published_grading, :new_message ]) end From 93d6bd2c3c3ab4c12e07ed12b37cab1c7fef5faa Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Sun, 7 Apr 2024 18:23:53 +0800 Subject: [PATCH 67/74] chore: Add comment in seed --- priv/repo/seeds.exs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/priv/repo/seeds.exs b/priv/repo/seeds.exs index 731054234..37be60527 100644 --- a/priv/repo/seeds.exs +++ b/priv/repo/seeds.exs @@ -80,8 +80,8 @@ if Cadet.Env.env() == :dev do avenger_group = insert(:group, %{name: "MockAvengerGroup", leader: avenger_cr}) groups_and_courses = [{admin_group, admin_course}, {avenger_group, avenger_course}] - # Users + # Users Enum.each(groups_and_courses, fn {group, course} -> students = for i <- 1..number_of_students do @@ -99,6 +99,7 @@ if Cadet.Env.env() == :dev do end # Assessments and Submissions + # {order, type, is_grading_auto_published, is_manually_graded} valid_assessment_types = [ {1, "Missions", false, true}, {2, "Paths", true, false}, From bd9abf9fe91de101df740efa0b0dfb7e6e62f0ec Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Mon, 8 Apr 2024 21:30:09 +0800 Subject: [PATCH 68/74] refactor: remove redundant lines --- .../admin_controllers/admin_grading_controller_test.exs | 6 ------ 1 file changed, 6 deletions(-) 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 2c0e3503a..21ff5302a 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -1097,9 +1097,6 @@ defmodule CadetWeb.AdminGradingControllerTest do conn = conn |> sign_in(staff.user) - - conn = - conn |> post(build_url_unpublish_all(course.id, assessment_id)) assert response(conn, 403) == "Only Admin is permitted to publish all grades" @@ -1136,9 +1133,6 @@ defmodule CadetWeb.AdminGradingControllerTest do conn = conn |> sign_in(staff.user) - - conn = - conn |> post(build_url_publish_all(course.id, assessment_id)) assert response(conn, 403) == "Only Admin is permitted to publish all grades" From ce6ec53f5ac8c5f4a02e77f96861db094ddfd4f5 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Fri, 12 Apr 2024 02:18:34 +0800 Subject: [PATCH 69/74] Redate migrations Ensures total ordering of migration files are preserved. --- ..._published.exs => 20240322122108_add_is_grading_published.exs} | 0 ...blished.exs => 20240322184853_update_is_grading_published.exs} | 0 ...ished.exs => 20240331222300_add_is_grading_auto_published.exs} | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename priv/repo/migrations/{20230702122108_add_is_grading_published.exs => 20240322122108_add_is_grading_published.exs} (100%) rename priv/repo/migrations/{20230702184853_update_is_grading_published.exs => 20240322184853_update_is_grading_published.exs} (100%) rename priv/repo/migrations/{20240321222300_add_is_grading_auto_published.exs => 20240331222300_add_is_grading_auto_published.exs} (100%) diff --git a/priv/repo/migrations/20230702122108_add_is_grading_published.exs b/priv/repo/migrations/20240322122108_add_is_grading_published.exs similarity index 100% rename from priv/repo/migrations/20230702122108_add_is_grading_published.exs rename to priv/repo/migrations/20240322122108_add_is_grading_published.exs diff --git a/priv/repo/migrations/20230702184853_update_is_grading_published.exs b/priv/repo/migrations/20240322184853_update_is_grading_published.exs similarity index 100% rename from priv/repo/migrations/20230702184853_update_is_grading_published.exs rename to priv/repo/migrations/20240322184853_update_is_grading_published.exs diff --git a/priv/repo/migrations/20240321222300_add_is_grading_auto_published.exs b/priv/repo/migrations/20240331222300_add_is_grading_auto_published.exs similarity index 100% rename from priv/repo/migrations/20240321222300_add_is_grading_auto_published.exs rename to priv/repo/migrations/20240331222300_add_is_grading_auto_published.exs From 710213ca8c1521629e94c64aaa4e4a6c4297fd6d Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Fri, 12 Apr 2024 02:22:39 +0800 Subject: [PATCH 70/74] Revert unnecessary changes --- lib/cadet/devices/devices.ex | 2 +- lib/cadet_web/admin_controllers/admin_assets_controller.ex | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cadet/devices/devices.ex b/lib/cadet/devices/devices.ex index d036151d3..7862b8792 100644 --- a/lib/cadet/devices/devices.ex +++ b/lib/cadet/devices/devices.ex @@ -208,7 +208,7 @@ defmodule Cadet.Devices do }, 300, [], - ~c"" + '' ) # ExAws includes the session token in the signed payload and doesn't allow diff --git a/lib/cadet_web/admin_controllers/admin_assets_controller.ex b/lib/cadet_web/admin_controllers/admin_assets_controller.ex index 3bb832af5..3316cfffa 100644 --- a/lib/cadet_web/admin_controllers/admin_assets_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_assets_controller.ex @@ -22,7 +22,7 @@ defmodule CadetWeb.AdminAssetsController do case Assets.delete_object(Courses.assets_prefix(course_reg.course), foldername, filename) do {:error, {status, message}} -> conn |> put_status(status) |> text(message) - _ -> conn |> put_status(204) |> text(~c"") + _ -> conn |> put_status(204) |> text('') end end From 593708e491d70d56b759e3b6081a9dd2bccfe6d1 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Fri, 12 Apr 2024 02:25:59 +0800 Subject: [PATCH 71/74] Revert more unnecessary changes --- lib/cadet_web/admin_controllers/admin_courses_controller.ex | 6 +----- lib/cadet_web/admin_controllers/admin_stories_controller.ex | 6 +++--- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/cadet_web/admin_controllers/admin_courses_controller.ex b/lib/cadet_web/admin_controllers/admin_courses_controller.ex index bdda2c868..7220a4d80 100644 --- a/lib/cadet_web/admin_controllers/admin_courses_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_courses_controller.ex @@ -143,11 +143,7 @@ defmodule CadetWeb.AdminCoursesController do title("AdminSublanguage") properties do - chapter(:integer, "Chapter number from 1 to 4", - required: true, - minimum: 1, - maximum: 4 - ) + chapter(:integer, "Chapter number from 1 to 4", required: true, minimum: 1, maximum: 4) variant(Schema.ref(:SourceVariant), "Variant name", required: true) end diff --git a/lib/cadet_web/admin_controllers/admin_stories_controller.ex b/lib/cadet_web/admin_controllers/admin_stories_controller.ex index 16c08f1ad..a6cdd46c0 100644 --- a/lib/cadet_web/admin_controllers/admin_stories_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_stories_controller.ex @@ -12,7 +12,7 @@ defmodule CadetWeb.AdminStoriesController do case result do {:ok, _story} -> - conn |> put_status(200) |> text(~c"") + conn |> put_status(200) |> text('') {:error, {status, message}} -> conn @@ -29,7 +29,7 @@ defmodule CadetWeb.AdminStoriesController do case result do {:ok, _story} -> - conn |> put_status(200) |> text(~c"") + conn |> put_status(200) |> text('') {:error, {status, message}} -> conn @@ -43,7 +43,7 @@ defmodule CadetWeb.AdminStoriesController do case result do {:ok, _nil} -> - conn |> put_status(204) |> text(~c"") + conn |> put_status(204) |> text('') {:error, {status, message}} -> conn From d00b686e8eb0c804d889962adb99890cc62e371d Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 12 Apr 2024 11:55:57 +0800 Subject: [PATCH 72/74] refactor: Move duplicate code into helper function --- lib/cadet/assessments/assessments.ex | 58 ++++++++++++---------------- 1 file changed, 24 insertions(+), 34 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 22ba00f4b..62601ca5a 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1163,17 +1163,7 @@ defmodule Cadet.Assessments do end end - @doc """ - Unpublishes grading for a submission and send notification to student. - Requires admin or staff who is group leader of student. - - Only manually graded assessments can be individually unpublished. We can only - unpublish all submissions for auto-graded assessments. - - Returns `{:ok, nil}` on success, otherwise `{:error, {status, message}}`. - """ - def unpublish_grading(submission_id, cr = %CourseRegistration{id: course_reg_id, role: role}) - when is_ecto_id(submission_id) do + defp can_publish?(submission_id, cr = %CourseRegistration{id: course_reg_id, role: role}) do submission = Submission |> join(:inner, [s], a in assoc(s, :assessment)) @@ -1185,12 +1175,30 @@ defmodule Cadet.Assessments do bypass = role in @bypass_closed_roles and submission.student_id == course_reg_id with {:submission_found?, true} <- {:submission_found?, is_map(submission)}, + {:status, :submitted} <- {:status, submission.status}, {:is_manually_graded?, true} <- {:is_manually_graded?, submission.assessment.config.is_manually_graded}, - {:allowed_to_unpublish?, true} <- - {:allowed_to_unpublish?, + {:fully_graded?, true} <- {:fully_graded?, is_fully_graded?(submission_id)}, + {:allowed_to_publish?, true} <- + {:allowed_to_publish?, role == :admin or bypass or Cadet.Accounts.Query.avenger_of?(cr, submission.student_id)} do + {:ok, submission} + end + end + + @doc """ + Unpublishes grading for a submission and send notification to student. + Requires admin or staff who is group leader of student. + + Only manually graded assessments can be individually unpublished. We can only + unpublish all submissions for auto-graded assessments. + + Returns `{:ok, nil}` on success, otherwise `{:error, {status, message}}`. + """ + def unpublish_grading(submission_id, cr = %CourseRegistration{}) + when is_ecto_id(submission_id) do + with {:ok, submission} <- can_publish?(submission_id, cr) do submission |> Submission.changeset(%{is_grading_published: false}) |> Repo.update() @@ -1205,7 +1213,7 @@ defmodule Cadet.Assessments do {:submission_found?, false} -> {:error, {:not_found, "Submission not found"}} - {:allowed_to_unpublish?, false} -> + {:allowed_to_publish?, false} -> {:error, {:forbidden, "Only Avenger of student or Admin is permitted to unpublish grading"}} @@ -1227,27 +1235,9 @@ defmodule Cadet.Assessments do Returns `{:ok, nil}` on success, otherwise `{:error, {status, message}}`. """ - def publish_grading(submission_id, cr = %CourseRegistration{id: course_reg_id, role: role}) + def publish_grading(submission_id, cr = %CourseRegistration{}) when is_ecto_id(submission_id) do - submission = - Submission - |> join(:inner, [s], a in assoc(s, :assessment)) - |> join(:inner, [_, a], c in assoc(a, :config)) - |> preload([_, a, c], assessment: {a, config: c}) - |> Repo.get(submission_id) - - # allows staff to publish own assessment - bypass = role in @bypass_closed_roles and submission.student_id == course_reg_id - - with {:submission_found?, true} <- {:submission_found?, is_map(submission)}, - {:status, :submitted} <- {:status, submission.status}, - {:is_manually_graded?, true} <- - {:is_manually_graded?, submission.assessment.config.is_manually_graded}, - {:fully_graded?, true} <- {:fully_graded?, is_fully_graded?(submission_id)}, - {:allowed_to_publish?, true} <- - {:allowed_to_publish?, - role == :admin or bypass or - Cadet.Accounts.Query.avenger_of?(cr, submission.student_id)} do + with {:ok, submission} <- can_publish?(submission_id, cr) do submission |> Submission.changeset(%{is_grading_published: true}) |> Repo.update() From e355d54e31f5d8d223788289ed4b95ae69b9a7f2 Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 12 Apr 2024 11:56:28 +0800 Subject: [PATCH 73/74] chore: Fix typo --- lib/cadet/assessments/assessments.ex | 2 +- .../admin_controllers/admin_grading_controller_test.exs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 62601ca5a..c94d08e30 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1407,7 +1407,7 @@ defmodule Cadet.Assessments do {:ok, nil} else - {:error, {:forbidden, "Only Admin is permitted to publish all grades"}} + {:error, {:forbidden, "Only Admin is permitted to unpublish all grades"}} end end 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 21ff5302a..11d8df0f0 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -1099,7 +1099,7 @@ defmodule CadetWeb.AdminGradingControllerTest do |> sign_in(staff.user) |> post(build_url_unpublish_all(course.id, assessment_id)) - assert response(conn, 403) == "Only Admin is permitted to publish all grades" + assert response(conn, 403) == "Only Admin is permitted to unpublish all grades" end end From 541fac9bc25a0ad1ce25b8208f25cd867b06102f Mon Sep 17 00:00:00 2001 From: GabrielCWT <77312579+GabrielCWT@users.noreply.github.com> Date: Fri, 12 Apr 2024 12:34:11 +0800 Subject: [PATCH 74/74] refactor: Change control flow structure --- lib/cadet/assessments/assessments.ex | 42 +++++++++++++++------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index c94d08e30..acffe823d 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1198,18 +1198,19 @@ defmodule Cadet.Assessments do """ def unpublish_grading(submission_id, cr = %CourseRegistration{}) when is_ecto_id(submission_id) do - with {:ok, submission} <- can_publish?(submission_id, cr) do - submission - |> Submission.changeset(%{is_grading_published: false}) - |> Repo.update() + case can_publish?(submission_id, cr) do + {:ok, submission} -> + submission + |> Submission.changeset(%{is_grading_published: false}) + |> Repo.update() - Notifications.handle_unpublish_grades_notifications( - submission.assessment.id, - Repo.get(CourseRegistration, submission.student_id) - ) + Notifications.handle_unpublish_grades_notifications( + submission.assessment.id, + Repo.get(CourseRegistration, submission.student_id) + ) + + {:ok, nil} - {:ok, nil} - else {:submission_found?, false} -> {:error, {:not_found, "Submission not found"}} @@ -1237,18 +1238,19 @@ defmodule Cadet.Assessments do """ def publish_grading(submission_id, cr = %CourseRegistration{}) when is_ecto_id(submission_id) do - with {:ok, submission} <- can_publish?(submission_id, cr) do - submission - |> Submission.changeset(%{is_grading_published: true}) - |> Repo.update() + case can_publish?(submission_id, cr) do + {:ok, submission} -> + submission + |> Submission.changeset(%{is_grading_published: true}) + |> Repo.update() - Notifications.write_notification_when_published( - submission.id, - :published_grading - ) + Notifications.write_notification_when_published( + submission.id, + :published_grading + ) + + {:ok, nil} - {:ok, nil} - else {:submission_found?, false} -> {:error, {:not_found, "Submission not found"}}