diff --git a/lib/cadet/accounts/query.ex b/lib/cadet/accounts/query.ex index be2043fe8..040eda498 100644 --- a/lib/cadet/accounts/query.ex +++ b/lib/cadet/accounts/query.ex @@ -6,6 +6,7 @@ defmodule Cadet.Accounts.Query do alias Cadet.Accounts.{Authorization, User} alias Cadet.Course.Group + alias Cadet.Repo def user_nusnet_ids(user_id) do Authorization @@ -26,6 +27,17 @@ defmodule Cadet.Accounts.Query do |> where([_, g], g.leader_id == ^id) end + def avenger_of?(avenger, student_id) do + students = students_of(avenger) + + students + |> Repo.get(student_id) + |> case do + nil -> false + _ -> true + end + end + defp nusnet_ids(query) do query |> where([a], a.provider == "nusnet_id") end diff --git a/lib/cadet/assessments/answer.ex b/lib/cadet/assessments/answer.ex index 65f76a965..9d72b280b 100644 --- a/lib/cadet/assessments/answer.ex +++ b/lib/cadet/assessments/answer.ex @@ -47,7 +47,10 @@ defmodule Cadet.Assessments.Answer do @spec grading_changeset(%__MODULE__{} | Ecto.Changeset.t(), map()) :: Ecto.Changeset.t() def grading_changeset(answer, params) do answer - |> cast(params, ~w(grader_id xp_adjustment adjustment comment)a) + |> cast( + params, + ~w(grader_id xp xp_adjustment grade adjustment autograding_results autograding_status comment)a + ) |> add_belongs_to_id_from_model(:grader, params) |> foreign_key_constraint(:grader_id) |> validate_xp_grade_adjustment_total() diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index f76555966..4829ea32a 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -15,6 +15,7 @@ defmodule Cadet.Assessments do @xp_early_submission_max_bonus 100 @xp_bonus_assessment_type ~w(mission sidequest)a @submit_answer_roles ~w(student)a + @unsubmit_assessment_role ~w(staff)a @grading_roles ~w()a @see_all_submissions_roles ~w(staff admin)a @open_all_assessment_roles ~w(staff admin)a @@ -401,6 +402,90 @@ defmodule Cadet.Assessments do end end + def unsubmit_submission(submission_id, avenger = %User{id: staff_id, role: role}) + when is_ecto_id(submission_id) do + if role in @unsubmit_assessment_role 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)}, + {:is_open?, true} <- is_open?(submission.assessment), + {:status, :submitted} <- {:status, submission.status}, + {:avenger_of?, true} <- + {:avenger_of?, Cadet.Accounts.Query.avenger_of?(avenger, submission.student_id)} do + Multi.new() + |> Multi.run( + :rollback_submission, + fn _ -> + submission + |> Submission.changeset(%{ + status: :attempted, + xp_bonus: 0, + unsubmitted_by_id: staff_id, + unsubmitted_at: Timex.now() + }) + |> Repo.update() + end + ) + |> Multi.run(:rollback_answers, fn _ -> + Answer + |> join(:inner, [a], q in assoc(a, :question)) + |> join(:inner, [a, _], s in assoc(a, :submission)) + |> preload([_, q, s], question: q, submission: s) + |> where(submission_id: ^submission.id) + |> Repo.all() + |> Enum.reduce_while({:ok, nil}, fn answer, acc -> + case acc do + {:error, _} -> + {:halt, acc} + + {:ok, _} -> + {:cont, + answer + |> Answer.grading_changeset(%{ + grade: 0, + adjustment: 0, + xp: 0, + xp_adjustment: 0, + autograding_status: :none, + autograding_results: [], + comment: nil, + grader_id: nil + }) + |> Repo.update()} + end + end) + end) + |> Repo.transaction() + + {: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"}} + + {:avenger_of?, false} -> + {:error, {:forbidden, "Only Avenger of student is permitted to unsubmit"}} + + _ -> + {:error, {:internal_server_error, "Please try again later."}} + end + else + {:error, {:forbidden, "User is not permitted to unsubmit questions"}} + end + end + @spec update_submission_status_and_xp_bonus(%Submission{}) :: {:ok, %Submission{}} | {:error, Ecto.Changeset.t()} defp update_submission_status_and_xp_bonus(submission = %Submission{}) do @@ -471,15 +556,16 @@ defmodule Cadet.Assessments do x in subquery(Query.submissions_xp_and_grade()), s.id == x.submission_id ) - |> join(:inner, [s], st in assoc(s, :student)) + |> join(:inner, [s, _], st in assoc(s, :student)) |> join(:inner, [_, _, st], g in assoc(st, :group)) + |> join(:left, [s, _, _, g], u in assoc(s, :unsubmitted_by)) |> join( :inner, - [s], + [s, _, _, _, _], a in subquery(Query.all_assessments_with_max_xp_and_grade()), s.assessment_id == a.id ) - |> select([s, x, st, g, a], %Submission{ + |> select([s, x, st, g, u, a], %Submission{ s | grade: x.grade, adjustment: x.adjustment, @@ -487,7 +573,8 @@ defmodule Cadet.Assessments do xp_adjustment: x.xp_adjustment, student: st, assessment: a, - group_name: g.name + group_name: g.name, + unsubmitted_by: u }) cond do diff --git a/lib/cadet/assessments/submission.ex b/lib/cadet/assessments/submission.ex index 4827fd42d..7fe30f4dc 100644 --- a/lib/cadet/assessments/submission.ex +++ b/lib/cadet/assessments/submission.ex @@ -13,16 +13,18 @@ defmodule Cadet.Assessments.Submission do field(:xp_bonus, :integer, default: 0) field(:group_name, :string, virtual: true) field(:status, SubmissionStatus, default: :attempting) + field(:unsubmitted_at, Timex.Ecto.DateTime) belongs_to(:assessment, Assessment) belongs_to(:student, User) + belongs_to(:unsubmitted_by, User) has_many(:answers, Answer) timestamps() end @required_fields ~w(student_id assessment_id status)a - @optional_fields ~w(xp_bonus)a + @optional_fields ~w(xp_bonus unsubmitted_by_id unsubmitted_at)a @xp_early_submission_max_bonus 100 def changeset(submission, params) do @@ -33,9 +35,10 @@ defmodule Cadet.Assessments.Submission do greater_than_or_equal_to: 0, less_than_or_equal_to: @xp_early_submission_max_bonus ) - |> add_belongs_to_id_from_model([:student, :assessment], params) + |> add_belongs_to_id_from_model([:student, :assessment, :unsubmitted_by], params) |> validate_required(@required_fields) |> foreign_key_constraint(:student_id) |> foreign_key_constraint(:assessment_id) + |> foreign_key_constraint(:unsubmitted_by_id) end end diff --git a/lib/cadet_web/controllers/grading_controller.ex b/lib/cadet_web/controllers/grading_controller.ex index 6fe2efde1..43c073e86 100644 --- a/lib/cadet_web/controllers/grading_controller.ex +++ b/lib/cadet_web/controllers/grading_controller.ex @@ -77,6 +77,26 @@ defmodule CadetWeb.GradingController do |> text("Missing parameter") end + def unsubmit(conn, %{"submissionid" => submission_id}) when is_ecto_id(submission_id) do + user = conn.assigns[:current_user] + + case Assessments.unsubmit_submission(submission_id, user) do + {:ok, nil} -> + text(conn, "OK") + + {:error, {status, message}} -> + conn + |> put_status(status) + |> text(message) + end + end + + def unsubmit(conn, _params) do + conn + |> put_status(:bad_request) + |> text("Missing parameter") + end + swagger_path :index do get("/grading") @@ -99,6 +119,21 @@ defmodule CadetWeb.GradingController do response(401, "Unauthorised") end + swagger_path :unsubmit do + post("/grading/{submissionId}/unsubmit") + summary("Unsubmit submission. Can only be done by the Avenger of a student.") + security([%{JWT: []}]) + + parameters do + submissionId(:path, :integer, "submission id", required: true) + end + + response(200, "OK") + response(400, "Invalid parameters") + response(403, "User not permitted to unsubmit assessment or assessment not open") + response(404, "Submission not found") + end + swagger_path :show do get("/grading/{submissionId}") @@ -165,6 +200,9 @@ defmodule CadetWeb.GradingController do assessment(Schema.ref(:AssessmentInfo)) student(Schema.ref(:StudentInfo)) + + unsubmittedBy(Schema.ref(:GraderInfo)) + unsubmittedAt(:string, "Last unsubmitted at", format: "date-time", required: false) end end, AssessmentInfo: diff --git a/lib/cadet_web/router.ex b/lib/cadet_web/router.ex index 6046a3b3a..0b7ef1a0d 100644 --- a/lib/cadet_web/router.ex +++ b/lib/cadet_web/router.ex @@ -35,6 +35,7 @@ defmodule CadetWeb.Router do get("/grading", GradingController, :index) get("/grading/:submissionid", GradingController, :show) + post("/grading/:submissionid/unsubmit", GradingController, :unsubmit) post("/grading/:submissionid/:questionid", GradingController, :update) get("/user", UserController, :index) diff --git a/lib/cadet_web/views/grading_view.ex b/lib/cadet_web/views/grading_view.ex index ece58dc0f..d79fa976d 100644 --- a/lib/cadet_web/views/grading_view.ex +++ b/lib/cadet_web/views/grading_view.ex @@ -28,7 +28,9 @@ defmodule CadetWeb.GradingView do coverImage: :cover_picture }), groupName: :group_name, - status: :status + status: :status, + unsubmittedBy: &unsubmitted_by_builder(&1.unsubmitted_by), + unsubmittedAt: &format_datetime(&1.unsubmitted_at) }) end diff --git a/lib/cadet_web/views/view_helpers.ex b/lib/cadet_web/views/view_helpers.ex index 6cdfce543..957428b46 100644 --- a/lib/cadet_web/views/view_helpers.ex +++ b/lib/cadet_web/views/view_helpers.ex @@ -3,22 +3,30 @@ defmodule CadetWeb.ViewHelpers do Helper functions shared throughout views """ + defp build_staff(user) do + transform_map_for_view(user, [:name, :id]) + end + + def unsubmitted_by_builder(nil), do: nil + + def unsubmitted_by_builder(staff) do + build_staff(staff) + end + def grader_builder(nil), do: nil def grader_builder(_) do - fn %{grader: grader} -> - transform_map_for_view(grader, [:name, :id]) - end + fn %{grader: grader} -> build_staff(grader) end end def graded_at_builder(nil), do: nil def graded_at_builder(_) do - fn %{updated_at: updated_at} -> - format_datetime(updated_at) - end + fn %{updated_at: updated_at} -> format_datetime(updated_at) end end + def format_datetime(nil), do: nil + def format_datetime(datetime = %DateTime{}) do datetime |> DateTime.truncate(:millisecond) diff --git a/priv/repo/migrations/20190530065753_add_unsubmit_fields.exs b/priv/repo/migrations/20190530065753_add_unsubmit_fields.exs new file mode 100644 index 000000000..e026402b4 --- /dev/null +++ b/priv/repo/migrations/20190530065753_add_unsubmit_fields.exs @@ -0,0 +1,10 @@ +defmodule Cadet.Repo.Migrations.AddUnsubmitFields do + use Ecto.Migration + + def change do + alter table(:submissions) do + add(:unsubmitted_by_id, references(:users), null: true) + add(:unsubmitted_at, :timestamp, null: true) + end + end +end diff --git a/test/cadet_web/controllers/grading_controller_test.exs b/test/cadet_web/controllers/grading_controller_test.exs index 86abe4415..437731d38 100644 --- a/test/cadet_web/controllers/grading_controller_test.exs +++ b/test/cadet_web/controllers/grading_controller_test.exs @@ -1,7 +1,7 @@ defmodule CadetWeb.GradingControllerTest do use CadetWeb.ConnCase - alias Cadet.Assessments.Answer + alias Cadet.Assessments.{Answer, Submission} alias Cadet.Repo alias CadetWeb.GradingController @@ -33,6 +33,13 @@ defmodule CadetWeb.GradingControllerTest do end end + describe "GET /:submissionid/unsubmit, unauthenticated" do + test "unauthorized", %{conn: conn} do + conn = post(conn, build_url_unsubmit(1)) + assert response(conn, 401) =~ "Unauthorised" + end + end + describe "GET /, student" do @tag authenticate: :student test "unauthorized", %{conn: conn} do @@ -71,6 +78,14 @@ defmodule CadetWeb.GradingControllerTest do end end + describe "GET /:submissionid/unsubmit, student" do + @tag authenticate: :student + test "unauthorized", %{conn: conn} do + conn = post(conn, build_url_unsubmit(1)) + assert response(conn, 403) =~ "User is not permitted to unsubmit questions" + end + end + describe "GET /, staff" do @tag authenticate: :staff test "avenger gets to see all students submissions", %{conn: conn} do @@ -103,7 +118,9 @@ defmodule CadetWeb.GradingControllerTest do "coverImage" => mission.cover_picture }, "groupName" => submission.student.group.name, - "status" => Atom.to_string(submission.status) + "status" => Atom.to_string(submission.status), + "unsubmittedBy" => nil, + "unsubmittedAt" => nil } end) @@ -141,7 +158,9 @@ defmodule CadetWeb.GradingControllerTest do "coverImage" => mission.cover_picture }, "groupName" => submission.student.group.name, - "status" => Atom.to_string(submission.status) + "status" => Atom.to_string(submission.status), + "unsubmittedAt" => nil, + "unsubmittedBy" => nil } end) @@ -197,7 +216,9 @@ defmodule CadetWeb.GradingControllerTest do "coverImage" => mission.cover_picture }, "groupName" => submission.student.group.name, - "status" => Atom.to_string(submission.status) + "status" => Atom.to_string(submission.status), + "unsubmittedAt" => nil, + "unsubmittedBy" => nil } end) @@ -532,6 +553,164 @@ defmodule CadetWeb.GradingControllerTest do end end + describe "POST /:submissionid/unsubmit, staff" do + @tag authenticate: :staff + test "succeeds", %{conn: conn} do + %{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, + type: :mission + ) + + question = insert(:programming_question, assessment: assessment) + student = List.first(students) + + submission = + insert(:submission, assessment: assessment, student: student, status: :submitted) + + answer = + insert( + :answer, + submission: submission, + question: question, + answer: %{code: "f => f(f);"} + ) + + conn + |> sign_in(grader) + |> post(build_url_unsubmit(submission.id)) + |> response(200) + + submission_db = Repo.get(Submission, submission.id) + answer_db = Repo.get(Answer, answer.id) + + assert submission_db.status == :attempted + assert submission_db.unsubmitted_by_id === grader.id + assert submission_db.unsubmitted_at != nil + + assert answer_db.comment == nil + assert answer_db.autograding_status == :none + assert answer_db.autograding_results == [] + assert answer_db.grader_id == nil + assert answer_db.xp == 0 + assert answer_db.xp_adjustment == 0 + assert answer_db.grade == 0 + assert answer_db.adjustment == 0 + end + + @tag authenticate: :staff + test "assessments which have not been submitted should not be allowed to unsubmit", %{ + conn: conn + } do + %{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, + type: :mission + ) + + 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 + |> sign_in(grader) + |> post(build_url_unsubmit(submission.id)) + + assert response(conn, 400) =~ "Assessment has not been submitted" + end + + @tag authenticate: :staff + test "assessment that is not open anymore cannot be unsubmitted", %{conn: conn} do + %{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, + type: :mission + ) + + 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(grader) + |> post(build_url_unsubmit(submission.id)) + + assert response(conn, 403) =~ "Assessment not open" + end + + @tag authenticate: :staff + test "avenger should not be allowed to unsubmit for students outside of their group", %{ + conn: conn + } do + %{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, + type: :mission + ) + + other_grader = insert(:user, role: :staff) + 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) + |> post(build_url_unsubmit(submission.id)) + + assert response(conn, 403) =~ "Only Avenger of student is permitted to unsubmit" + end + end + describe "GET /, admin" do @tag authenticate: :staff test "can see all submissions", %{conn: conn} do @@ -569,7 +748,9 @@ defmodule CadetWeb.GradingControllerTest do "coverImage" => mission.cover_picture }, "groupName" => submission.student.group.name, - "status" => Atom.to_string(submission.status) + "status" => Atom.to_string(submission.status), + "unsubmittedAt" => nil, + "unsubmittedBy" => nil } end) @@ -609,7 +790,9 @@ defmodule CadetWeb.GradingControllerTest do "coverImage" => mission.cover_picture }, "groupName" => submission.student.group.name, - "status" => Atom.to_string(submission.status) + "status" => Atom.to_string(submission.status), + "unsubmittedAt" => nil, + "unsubmittedBy" => nil } end) @@ -760,6 +943,7 @@ defmodule CadetWeb.GradingControllerTest do defp build_url, do: "/v1/grading/" defp build_url(submission_id), do: "#{build_url()}#{submission_id}/" defp build_url(submission_id, question_id), do: "#{build_url(submission_id)}#{question_id}" + defp build_url_unsubmit(submission_id), do: "#{build_url(submission_id)}/unsubmit" defp seed_db(conn, override_grader \\ nil) do grader = override_grader || conn.assigns[:current_user]