From 038cbc67eeb3e51250c0df8ce407a62aabb28dea Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Mon, 6 Apr 2020 21:22:00 +0800 Subject: [PATCH 01/22] Added GroundControl --- lib/cadet/assessments/assessments.ex | 185 ++++++++++++++---- lib/cadet/jobs/xml_parser.ex | 119 ++++++----- .../controllers/assessments_controller.ex | 72 ++++++- lib/cadet_web/router.ex | 4 + lib/cadet_web/views/assessments_view.ex | 3 +- 5 files changed, 292 insertions(+), 91 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 80413d551..4c5c9b6d0 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -16,11 +16,56 @@ defmodule Cadet.Assessments do @xp_early_submission_max_bonus 100 @xp_bonus_assessment_type ~w(mission sidequest)a @submit_answer_roles ~w(student)a + @change_dates_assessment_role ~w(staff admin)a + @delete_assessment_role ~w(staff admin)a + @publish_assessment_role ~w(staff admin)a @unsubmit_assessment_role ~w(staff admin)a @grading_roles ~w()a @see_all_submissions_roles ~w(staff admin)a @open_all_assessment_roles ~w(staff admin)a + def change_dates_assessment(_user = %User{role: role}, id, close_at, open_at) do + if role in @change_dates_assessment_role do + assessment = Repo.get(Assessment, id) + previous_open_time = assessment.open_at + cond do + Timex.before?(close_at, open_at) -> + {:error, {:bad_request, "New end date should occur after new opening date"}} + + Timex.before?(close_at, Timex.now()) -> + {:error, {:bad_request, "New end date should occur after current time"}} + + Timex.equal?(previous_open_time, open_at) or Timex.after?(previous_open_time, Timex.now()) -> + update_assessment(id, %{close_at: close_at, open_at: open_at}) + + Timex.before?(open_at, Timex.now()) -> + {:error, {:bad_request, "New Opening date should occur after current time"}} + + true -> + {:error, {:forbidden, "Assessment is already opened"}} + end + else + {:error, {:forbidden, "User is not permitted to edit"}} + end + end + + def toggle_publish_assessment(_publisher = %User{role: role}, id, bool) do + if role in @publish_assessment_role do + update_assessment(id, %{is_published: bool}) + else + {:error, {:forbidden, "User is not permitted to publish"}} + end + end + + def delete_assessment(_deleter = %User{role: role}, id) do + if role in @delete_assessment_role do + assessment = Repo.get(Assessment, id) + Repo.delete(assessment) + else + {:error, {:forbidden, "User is not permitted to delete"}} + end + end + @spec user_total_xp(%User{}) :: integer() def user_total_xp(%User{id: user_id}) when is_ecto_id(user_id) do total_xp_bonus = @@ -169,11 +214,18 @@ defmodule Cadet.Assessments do def assessment_with_questions_and_answers(id, user = %User{}, password) when is_ecto_id(id) do + role = user.role assessment = - Assessment - |> where(id: ^id) - |> where(is_published: true) - |> Repo.one() + if role in @open_all_assessment_roles do + Assessment + |> where(id: ^id) + |> Repo.one() + else + Assessment + |> where(id: ^id) + |> where(is_published: true) + |> Repo.one() + end if assessment do assessment_with_questions_and_answers(assessment, user, password) @@ -212,11 +264,7 @@ defmodule Cadet.Assessments do assessment_with_questions_and_answers(id, user, nil) end - @doc """ - Returns a list of assessments with all fields and an indicator showing whether it has been attempted - by the supplied user - """ - def all_published_assessments(user = %User{}) do + def all_assessments(user = %User{}) do assessments = Query.all_assessments_with_max_xp_and_grade() |> subquery() @@ -246,7 +294,7 @@ defmodule Cadet.Assessments do question_count: q_count.count, graded_count: a_count.count }) - |> where(is_published: true) + |> filter_published_assessments(user) |> order_by(:open_at) |> Repo.all() |> Enum.map(fn assessment = %Assessment{} -> @@ -265,6 +313,14 @@ defmodule Cadet.Assessments do {:ok, assessments} end + def filter_published_assessments(assessments, user) do + role = user.role + case role do + :student -> where(assessments, is_published: true) + _ -> assessments + end + end + defp build_grading_status(submission_status, a_type, q_count, g_count) do case a_type do type when type in [:mission, :sidequest, :practical] -> @@ -289,53 +345,108 @@ defmodule Cadet.Assessments do @doc """ The main function that inserts or updates assessments from the XML Parser """ - @spec insert_or_update_assessments_and_questions(map(), [map()]) :: + @spec insert_or_update_assessments_and_questions(map(), [map()], boolean()) :: {:ok, any()} | {:error, Ecto.Multi.name(), any(), %{optional(Ecto.Multi.name()) => any()}} - def insert_or_update_assessments_and_questions(assessment_params, questions_params) do + def insert_or_update_assessments_and_questions(assessment_params, questions_params, force_update) do assessment_multi = Multi.insert_or_update( Multi.new(), :assessment, - insert_or_update_assessment_changeset(assessment_params) + insert_or_update_assessment_changeset(assessment_params, force_update) ) - questions_params - |> Enum.with_index(1) - |> Enum.reduce(assessment_multi, fn {question_params, index}, multi -> - Multi.run(multi, String.to_atom("question#{index}"), fn _repo, - %{assessment: %Assessment{id: id}} -> - question_params - |> Map.put(:display_order, index) - |> build_question_changeset_for_assessment_id(id) - |> Repo.insert() + if force_update and check_question_count(assessment_multi, questions_params) do + {:error, "Question count is different"} + else + questions_params + |> Enum.with_index(1) + |> Enum.reduce(assessment_multi, fn {question_params, index}, multi -> + Multi.run(multi, String.to_atom("question#{index}"), fn _repo, + %{assessment: %Assessment{id: id}} -> + question_exists = + Repo.exists?(where(Question, [q], q.assessment_id == ^id and q.display_order == ^index)) + if !force_update or !question_exists do + question_params + |> Map.put(:display_order, index) + |> build_question_changeset_for_assessment_id(id) + |> Repo.insert() + else + params = + (if !question_params.max_xp do + question_params + |> Map.put(:max_xp, 0) + else + question_params + end) + |> Map.put(:display_order, index) + + %{id: question_id} = + where(Question, [q], q.display_order == ^index and q.assessment_id == ^id) + |> Repo.one() + + changeset = Question.changeset(%Question{assessment_id: id, id: question_id}, params) + Repo.update(changeset) + end + end) end) - end) - |> Repo.transaction() + |> Repo.transaction() + end end - @spec insert_or_update_assessment_changeset(map()) :: Ecto.Changeset.t() - defp insert_or_update_assessment_changeset(params = %{number: number}) do + defp check_question_count(assessment_multi, questions_params) do + assessment_id = + (assessment_multi.operations + |> List.first() + |> elem(1) + |> elem(1)).data.id + + if !assessment_id do + false + else + existing_questions_count = + where(Question, [q], q.assessment_id == ^assessment_id) + |> Repo.all() + |> Enum.count + + new_questions_count = Enum.count(questions_params) + + existing_questions_count != new_questions_count + end + end + + @spec insert_or_update_assessment_changeset(map(), boolean()) :: Ecto.Changeset.t() + defp insert_or_update_assessment_changeset(params = %{number: number}, force_update) do Assessment |> where(number: ^number) |> Repo.one() |> case do nil -> Assessment.changeset(%Assessment{}, params) - assessment -> - if Timex.after?(assessment.open_at, Timex.now()) do - # Delete all existing questions - %{id: assessment_id} = assessment + cond do + Timex.after?(assessment.open_at, Timex.now()) -> + # Delete all existing questions + %{id: assessment_id} = assessment - Question - |> where(assessment_id: ^assessment_id) - |> Repo.delete_all() + Question + |> where(assessment_id: ^assessment_id) + |> Repo.delete_all() - Assessment.changeset(assessment, params) - else - # if the assessment is already open, don't mess with it - create_invalid_changeset_with_error(:assessment, "is already open") + Assessment.changeset(assessment, params) + + force_update -> + # Maintain the same open/close date when force updating an assessment + new_params = + params + |> Map.delete(:open_at) + |> Map.delete(:close_at) + + Assessment.changeset(assessment, new_params) + + true -> + # if the assessment is already open, don't mess with it + create_invalid_changeset_with_error(:assessment, "is already open") end end end diff --git a/lib/cadet/jobs/xml_parser.ex b/lib/cadet/jobs/xml_parser.ex index c4da6a4d3..87e80aed7 100644 --- a/lib/cadet/jobs/xml_parser.ex +++ b/lib/cadet/jobs/xml_parser.ex @@ -77,14 +77,15 @@ defmodule Cadet.Updater.XMLParser do end end - @spec parse_xml(String.t()) :: :ok | :error - def parse_xml(xml) do + @spec parse_xml(String.t(), boolean()) :: :ok | {:ok, String.t} | {:error, {atom(), String.t}} + def parse_xml(xml, force_update \\ false) do with {:ok, assessment_params} <- process_assessment(xml), {:ok, questions_params} <- process_questions(xml), {:ok, %{assessment: assessment}} <- Assessments.insert_or_update_assessments_and_questions( assessment_params, - questions_params + questions_params, + force_update ) do Logger.info( "Created/updated assessment with id: #{assessment.id}, with #{length(questions_params)} questions." @@ -92,25 +93,49 @@ defmodule Cadet.Updater.XMLParser do :ok else - :error -> - :error - {:error, stage, %{errors: [assessment: {"is already open", []}]}, _} when is_atom(stage) -> Logger.warn("Assessment already open, ignoring...") - :ok + {:ok, "Assessment already open, ignoring..."} + + {:error, errmsg} -> + log_and_return_badrequest(errmsg) {:error, stage, changeset, _} when is_atom(stage) -> log_error_bad_changeset(changeset, stage) - :error + changeset_error = + changeset + |> Map.get(:errors) + |> extract_changeset_error_message + error_message = "Invalid #{stage} changeset " <> changeset_error + log_and_return_badrequest(error_message) end catch # the :erlsom library used by SweetXml will exit if XML is invalid - :exit, _ -> - :error + :exit, parse_error -> + error_message = + parse_error + |> nested_tuple_to_list() + |> List.flatten() + |> Enum.reduce("", fn x, acc -> acc <> to_string(x) <> " " end) + {:error, {:bad_request, "Invalid XML " <> error_message}} + end + + defp extract_changeset_error_message(errors_list) do + errors_list + |> Enum.map(fn {field, {error, _}} -> to_string(field) <> " " <> error end) + |> List.foldr("", fn x, acc -> acc <> x <> " " end) end - @spec process_assessment(String.t()) :: {:ok, map()} | :error + @spec process_assessment(String.t()) :: {:ok, map()} | {:error, String.t} defp process_assessment(xml) do + open_at = + Timex.now() + |> Timex.beginning_of_day() + |> Timex.shift(days: 3) + |> Timex.shift(hours: 4) + + close_at = Timex.shift(open_at, days: 7) + assessment_params = xml |> xpath( @@ -118,8 +143,6 @@ defmodule Cadet.Updater.XMLParser do access: ~x"./@access"s |> transform_by(&process_access/1), type: ~x"./@kind"s |> transform_by(&change_quest_to_sidequest/1), title: ~x"./@title"s, - open_at: ~x"./@startdate"s |> transform_by(&Timex.parse!(&1, "{ISO:Extended}")), - close_at: ~x"./@duedate"s |> transform_by(&Timex.parse!(&1, "{ISO:Extended}")), number: ~x"./@number"s, story: ~x"./@story"s, cover_picture: ~x"./@coverimage"s, @@ -128,7 +151,9 @@ defmodule Cadet.Updater.XMLParser do summary_long: ~x"./TEXT/text()" |> transform_by(&process_charlist/1), password: ~x"//PASSWORD/text()"so |> transform_by(&process_charlist/1) ) - |> Map.put(:is_published, true) + |> Map.put(:is_published, false) + |> Map.put(:open_at, open_at) + |> Map.put(:close_at, close_at) if assessment_params.access === "public" do Map.put(assessment_params, :password, nil) @@ -138,21 +163,12 @@ defmodule Cadet.Updater.XMLParser do Map.put(assessment_params, :password, "") end - if verify_has_time_offset(assessment_params) do - {:ok, assessment_params} - else - Logger.error("Time does not have offset specified.") - :error - end + {:ok, assessment_params} + rescue - e in Timex.Parse.ParseError -> - Logger.error("Time does not conform to ISO8601 DateTime: #{e.message}") - :error - # This error is raised by xpath/3 when TASK does not exist (hence is equal to nil) Protocol.UndefinedError -> - Logger.error("Missing TASK") - :error + {:error, "Missing TASK"} end def process_access("private") do @@ -172,17 +188,7 @@ defmodule Cadet.Updater.XMLParser do type end - @spec verify_has_time_offset(%{ - :open_at => DateTime.t() | NaiveDateTime.t(), - :close_at => DateTime.t() | NaiveDateTime.t(), - optional(atom()) => any() - }) :: boolean() - defp verify_has_time_offset(%{open_at: open_at, close_at: close_at}) do - # Timex.parse!/2 returns NaiveDateTime when offset is not specified, or DateTime otherwise. - open_at.__struct__ != NaiveDateTime and close_at.__struct__ != NaiveDateTime - end - - @spec process_questions(String.t()) :: {:ok, [map()]} | :error + @spec process_questions(String.t()) :: {:ok, [map()]} | {:error, String.t} defp process_questions(xml) do default_library = xpath(xml, ~x"//TASK/DEPLOYMENT"e) default_grading_library = xpath(xml, ~x"//TASK/GRADERDEPLOYMENT"e) @@ -206,16 +212,16 @@ defmodule Cadet.Updater.XMLParser do question else {:no_missing_attr?, false} -> - Logger.error("Missing attribute(s) on PROBLEM") - :error - - :error -> - :error + {:error, "Missing attribute(s) on PROBLEM"} + + {:error, errmsg} -> + {:error, errmsg} end end) - if Enum.any?(questions_params, &(&1 == :error)) do - :error + if Enum.any?(questions_params, &(!is_map(&1))) do + error = Enum.find(questions_params, &(!is_map(&1))) + error else {:ok, questions_params} end @@ -228,7 +234,7 @@ defmodule Cadet.Updater.XMLParser do Logger.error("Changeset: #{inspect(changeset, pretty: true)}") end - @spec process_question_by_question_type(map()) :: map() | :error + @spec process_question_by_question_type(map()) :: map() | {:error, String.t} defp process_question_by_question_type(question) do question[:entity] |> process_question_entity_by_type(question[:type]) @@ -236,8 +242,8 @@ defmodule Cadet.Updater.XMLParser do question_map when is_map(question_map) -> Map.put(question, :question, question_map) - :error -> - :error + {:error, errmsg} -> + {:error, errmsg} end end @@ -288,11 +294,10 @@ defmodule Cadet.Updater.XMLParser do end defp process_question_entity_by_type(_, _) do - Logger.error("Invalid question type.") - :error + {:error, "Invalid question type"} end - @spec process_question_library(map(), any(), any()) :: map() | :error + @spec process_question_library(map(), any(), any()) :: map() | {:error, String.t} defp process_question_library(question, default_library, default_grading_library) do library = xpath(question[:entity], ~x"./DEPLOYMENT"o) || default_library @@ -304,8 +309,7 @@ defmodule Cadet.Updater.XMLParser do |> Map.put(:library, process_question_library(library)) |> Map.put(:grading_library, process_question_library(grading_library)) else - Logger.error("Missing DEPLOYMENT") - :error + {:error, "Missing DEPLOYMENT"} end end @@ -350,4 +354,15 @@ defmodule Cadet.Updater.XMLParser do |> to_string() |> String.trim() end + + defp log_and_return_badrequest(error_message) do + Logger.error(error_message) + {:error, {:bad_request, error_message}} + end + + defp nested_tuple_to_list(tuple) when is_tuple(tuple) do + tuple |> Tuple.to_list |> Enum.map(&nested_tuple_to_list/1) + end + + defp nested_tuple_to_list(x), do: x end diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex index c6ba44fe9..fd23306b4 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -4,6 +4,7 @@ defmodule CadetWeb.AssessmentsController do use PhoenixSwagger alias Cadet.Assessments + import Cadet.Updater.XMLParser, only: [parse_xml: 2] def submit(conn, %{"assessmentid" => assessment_id}) when is_ecto_id(assessment_id) do case Assessments.finalise_submission(assessment_id, conn.assigns.current_user) do @@ -19,7 +20,7 @@ defmodule CadetWeb.AssessmentsController do def index(conn, _) do user = conn.assigns[:current_user] - {:ok, assessments} = Assessments.all_published_assessments(user) + {:ok, assessments} = Assessments.all_assessments(user) render(conn, "index.json", assessments: assessments) end @@ -34,6 +35,75 @@ defmodule CadetWeb.AssessmentsController do end end + def update(conn, %{"id" => id, "bool" => bool}) do + result = Assessments.toggle_publish_assessment(conn.assigns.current_user, id, bool) + + case result do + {:ok, _nil} -> + send_resp(conn, 200, "OK") + + {:error, {status, message}} -> + conn + |> put_status(status) + |> text(message) + end + end + + def update(conn, %{"id" => id, "closeAt" => close_at, "openAt" => open_at}) do + formatted_close_date = elem(DateTime.from_iso8601(close_at), 1) + formatted_open_date = elem(DateTime.from_iso8601(open_at), 1) + result = Assessments.change_dates_assessment(conn.assigns.current_user, id, formatted_close_date, formatted_open_date) + + case result do + {:ok, _nil} -> + send_resp(conn, 200, "OK") + + {:error, {status, message}} -> + conn + |> put_status(status) + |> text(message) + end + end + + def delete(conn, %{"id" => id}) do + result = Assessments.delete_assessment(conn.assigns.current_user, id) + + case result do + {:ok, _nil} -> + send_resp(conn, 200, "OK") + + {:error, {status, message}} -> + conn + |> put_status(status) + |> text(message) + end + end + + def create(conn, %{"assessment" => assessment, "forceUpdate" => force_update}) do + file = assessment["file"].path + |> File.read!() + result = + case force_update do + "true" -> parse_xml(file, true) + "false" -> parse_xml(file, false) + end + + case result do + :ok -> + if (force_update == "true") do + send_resp(conn, 200, "Force Update OK") + else + send_resp(conn, 200, "OK") + end + + {:ok, warning_message} -> + send_resp(conn, 200, warning_message) + + {:error, {status, message}} -> + send_resp(conn, status, message) + end + end + swagger_path :submit do post("/assessments/{assessmentId}/submit") summary("Finalise submission for an assessment") diff --git a/lib/cadet_web/router.ex b/lib/cadet_web/router.ex index c79ba7e46..0c91b8f0f 100644 --- a/lib/cadet_web/router.ex +++ b/lib/cadet_web/router.ex @@ -38,7 +38,11 @@ defmodule CadetWeb.Router do resources("/sourcecast", SourcecastController, only: [:create, :delete]) get("/assessments", AssessmentsController, :index) + post("/assessments", AssessmentsController, :create) + delete("/assessments/:id", AssessmentsController, :delete) post("/assessments/:id", AssessmentsController, :show) + post("/assessments/publish/:id", AssessmentsController, :update) + post("/assessments/update/:id", AssessmentsController, :update) post("/assessments/:assessmentid/submit", AssessmentsController, :submit) post("/assessments/question/:questionid/submit", AnswerController, :submit) diff --git a/lib/cadet_web/views/assessments_view.ex b/lib/cadet_web/views/assessments_view.ex index 4918e1693..e8b1dfea9 100644 --- a/lib/cadet_web/views/assessments_view.ex +++ b/lib/cadet_web/views/assessments_view.ex @@ -26,7 +26,8 @@ defmodule CadetWeb.AssessmentsView do xp: &(&1.xp || 0), grade: &(&1.grade || 0), coverImage: :cover_picture, - private: &password_protected?(&1.password) + private: &password_protected?(&1.password), + isPublished: :is_published }) end From 7e5866be7d129edffa582b6dbf7c67ed26b813b7 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Tue, 7 Apr 2020 21:47:10 +0800 Subject: [PATCH 02/22] Minor edit --- 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 4c5c9b6d0..94da860ac 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -264,6 +264,10 @@ defmodule Cadet.Assessments do assessment_with_questions_and_answers(id, user, nil) end + @doc """ + Returns a list of assessments with all fields and an indicator showing whether it has been attempted + by the supplied user + """ def all_assessments(user = %User{}) do assessments = Query.all_assessments_with_max_xp_and_grade() From 35445935dd85b7a94b5c282db80e2c7d5cde8e04 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Wed, 15 Apr 2020 18:13:17 +0800 Subject: [PATCH 03/22] Changed variable naming --- lib/cadet/assessments/assessments.ex | 30 +++++++++---------- .../controllers/assessments_controller.ex | 6 ++-- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 94da860ac..a05c4084b 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -29,7 +29,7 @@ defmodule Cadet.Assessments do assessment = Repo.get(Assessment, id) previous_open_time = assessment.open_at cond do - Timex.before?(close_at, open_at) -> + Timex.before?(close_at, open_at) -> {:error, {:bad_request, "New end date should occur after new opening date"}} Timex.before?(close_at, Timex.now()) -> @@ -40,7 +40,7 @@ defmodule Cadet.Assessments do Timex.before?(open_at, Timex.now()) -> {:error, {:bad_request, "New Opening date should occur after current time"}} - + true -> {:error, {:forbidden, "Assessment is already opened"}} end @@ -49,9 +49,9 @@ defmodule Cadet.Assessments do end end - def toggle_publish_assessment(_publisher = %User{role: role}, id, bool) do + def toggle_publish_assessment(_publisher = %User{role: role}, id, toggle_publish_to) do if role in @publish_assessment_role do - update_assessment(id, %{is_published: bool}) + update_assessment(id, %{is_published: toggle_publish_to}) else {:error, {:forbidden, "User is not permitted to publish"}} end @@ -317,7 +317,7 @@ defmodule Cadet.Assessments do {:ok, assessments} end - def filter_published_assessments(assessments, user) do + def filter_published_assessments(assessments, user) do role = user.role case role do :student -> where(assessments, is_published: true) @@ -368,16 +368,16 @@ defmodule Cadet.Assessments do |> Enum.reduce(assessment_multi, fn {question_params, index}, multi -> Multi.run(multi, String.to_atom("question#{index}"), fn _repo, %{assessment: %Assessment{id: id}} -> - question_exists = + question_exists = Repo.exists?(where(Question, [q], q.assessment_id == ^id and q.display_order == ^index)) - if !force_update or !question_exists do + if !force_update or !question_exists do question_params |> Map.put(:display_order, index) |> build_question_changeset_for_assessment_id(id) |> Repo.insert() else - params = - (if !question_params.max_xp do + params = + (if !question_params.max_xp do question_params |> Map.put(:max_xp, 0) else @@ -385,7 +385,7 @@ defmodule Cadet.Assessments do end) |> Map.put(:display_order, index) - %{id: question_id} = + %{id: question_id} = where(Question, [q], q.display_order == ^index and q.assessment_id == ^id) |> Repo.one() @@ -405,10 +405,10 @@ defmodule Cadet.Assessments do |> elem(1) |> elem(1)).data.id - if !assessment_id do - false + if !assessment_id do + false else - existing_questions_count = + existing_questions_count = where(Question, [q], q.assessment_id == ^assessment_id) |> Repo.all() |> Enum.count @@ -416,7 +416,7 @@ defmodule Cadet.Assessments do new_questions_count = Enum.count(questions_params) existing_questions_count != new_questions_count - end + end end @spec insert_or_update_assessment_changeset(map(), boolean()) :: Ecto.Changeset.t() @@ -441,7 +441,7 @@ defmodule Cadet.Assessments do force_update -> # Maintain the same open/close date when force updating an assessment - new_params = + new_params = params |> Map.delete(:open_at) |> Map.delete(:close_at) diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex index fd23306b4..b4a6fcaca 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -35,8 +35,8 @@ defmodule CadetWeb.AssessmentsController do end end - def update(conn, %{"id" => id, "bool" => bool}) do - result = Assessments.toggle_publish_assessment(conn.assigns.current_user, id, bool) + def update(conn, %{"id" => id, "togglePublishTo" => toggle_publish_to}) do + result = Assessments.toggle_publish_assessment(conn.assigns.current_user, id, toggle_publish_to) case result do {:ok, _nil} -> @@ -82,7 +82,7 @@ defmodule CadetWeb.AssessmentsController do def create(conn, %{"assessment" => assessment, "forceUpdate" => force_update}) do file = assessment["file"].path |> File.read!() - result = + result = case force_update do "true" -> parse_xml(file, true) "false" -> parse_xml(file, false) From 59d980c7d76888dee2ad3e22b1c1b30ebd516460 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Wed, 15 Apr 2020 20:50:49 +0800 Subject: [PATCH 04/22] Fixed bug when force updating assessments that are not opened. Added requirement that all question types remain the same when force updating --- lib/cadet/assessments/assessments.ex | 39 ++++++++++++++++++---------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index a05c4084b..8987ea576 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -360,7 +360,7 @@ defmodule Cadet.Assessments do insert_or_update_assessment_changeset(assessment_params, force_update) ) - if force_update and check_question_count(assessment_multi, questions_params) do + if force_update and invalid_force_update(assessment_multi, questions_params) do {:error, "Question count is different"} else questions_params @@ -370,6 +370,7 @@ defmodule Cadet.Assessments do %{assessment: %Assessment{id: id}} -> question_exists = Repo.exists?(where(Question, [q], q.assessment_id == ^id and q.display_order == ^index)) + # the !question_exists check allows for force updating of brand new assessments if !force_update or !question_exists do question_params |> Map.put(:display_order, index) @@ -385,12 +386,16 @@ defmodule Cadet.Assessments do end) |> Map.put(:display_order, index) - %{id: question_id} = + %{id: question_id, type: type} = where(Question, [q], q.display_order == ^index and q.assessment_id == ^id) |> Repo.one() - changeset = Question.changeset(%Question{assessment_id: id, id: question_id}, params) - Repo.update(changeset) + if question_params.type != Atom.to_string(type) do + {:error, create_invalid_changeset_with_error(:question, "Question types should remain the same")} + else + changeset = Question.changeset(%Question{assessment_id: id, id: question_id}, params) + Repo.update(changeset) + end end end) end) @@ -398,24 +403,31 @@ defmodule Cadet.Assessments do end end - defp check_question_count(assessment_multi, questions_params) do + # Function that checks if the force update is invalid. The force update is only invalid + # if the new question count is different from the old question count. + defp invalid_force_update(assessment_multi, questions_params) do assessment_id = (assessment_multi.operations |> List.first() |> elem(1) |> elem(1)).data.id + # check if assessment already exists if !assessment_id do false else - existing_questions_count = - where(Question, [q], q.assessment_id == ^assessment_id) - |> Repo.all() - |> Enum.count - - new_questions_count = Enum.count(questions_params) - - existing_questions_count != new_questions_count + open_date = Repo.get(Assessment, assessment_id).open_at + # check if assessment is already opened + if Timex.after?(open_date, Timex.now()) do + false + else + existing_questions_count = + where(Question, [q], q.assessment_id == ^assessment_id) + |> Repo.all() + |> Enum.count + new_questions_count = Enum.count(questions_params) + existing_questions_count != new_questions_count + end end end @@ -445,6 +457,7 @@ defmodule Cadet.Assessments do params |> Map.delete(:open_at) |> Map.delete(:close_at) + |> Map.delete(:is_published) Assessment.changeset(assessment, new_params) From 20db1c287e0892abf86be075b58e7660846c2218 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Sun, 19 Apr 2020 22:15:26 +0800 Subject: [PATCH 05/22] Added swagger paths --- lib/cadet/assessments/assessments.ex | 2 +- lib/cadet/jobs/xml_parser.ex | 20 ++-- .../controllers/assessments_controller.ex | 107 +++++++++++++++--- lib/cadet_web/router.ex | 2 +- 4 files changed, 101 insertions(+), 30 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 8987ea576..fc2a00282 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -42,7 +42,7 @@ defmodule Cadet.Assessments do {:error, {:bad_request, "New Opening date should occur after current time"}} true -> - {:error, {:forbidden, "Assessment is already opened"}} + {:error, {:unauthorized, "Assessment is already opened"}} end else {:error, {:forbidden, "User is not permitted to edit"}} diff --git a/lib/cadet/jobs/xml_parser.ex b/lib/cadet/jobs/xml_parser.ex index 87e80aed7..239d60ce5 100644 --- a/lib/cadet/jobs/xml_parser.ex +++ b/lib/cadet/jobs/xml_parser.ex @@ -102,33 +102,33 @@ defmodule Cadet.Updater.XMLParser do {:error, stage, changeset, _} when is_atom(stage) -> log_error_bad_changeset(changeset, stage) - changeset_error = + changeset_error = changeset |> Map.get(:errors) |> extract_changeset_error_message - error_message = "Invalid #{stage} changeset " <> changeset_error + error_message = "Invalid #{stage} changeset #{changeset_error}" log_and_return_badrequest(error_message) end catch # the :erlsom library used by SweetXml will exit if XML is invalid :exit, parse_error -> - error_message = + error_message = parse_error |> nested_tuple_to_list() |> List.flatten() - |> Enum.reduce("", fn x, acc -> acc <> to_string(x) <> " " end) - {:error, {:bad_request, "Invalid XML " <> error_message}} + |> Enum.reduce("", fn x, acc -> "#{acc <> to_string(x)} " end) + {:error, {:bad_request, "Invalid XML #{error_message}"}} end defp extract_changeset_error_message(errors_list) do errors_list - |> Enum.map(fn {field, {error, _}} -> to_string(field) <> " " <> error end) - |> List.foldr("", fn x, acc -> acc <> x <> " " end) + |> Enum.map(fn {field, {error, _}} -> "#{to_string(field)} #{error}" end) + |> List.foldr("", fn x, acc -> "#{acc <> x} " end) end @spec process_assessment(String.t()) :: {:ok, map()} | {:error, String.t} defp process_assessment(xml) do - open_at = + open_at = Timex.now() |> Timex.beginning_of_day() |> Timex.shift(days: 3) @@ -164,7 +164,7 @@ defmodule Cadet.Updater.XMLParser do end {:ok, assessment_params} - + rescue # This error is raised by xpath/3 when TASK does not exist (hence is equal to nil) Protocol.UndefinedError -> @@ -213,7 +213,7 @@ defmodule Cadet.Updater.XMLParser do else {:no_missing_attr?, false} -> {:error, "Missing attribute(s) on PROBLEM"} - + {:error, errmsg} -> {:error, errmsg} end diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex index b4a6fcaca..b521cf854 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -35,7 +35,7 @@ defmodule CadetWeb.AssessmentsController do end end - def update(conn, %{"id" => id, "togglePublishTo" => toggle_publish_to}) do + def publish(conn, %{"id" => id, "togglePublishTo" => toggle_publish_to}) do result = Assessments.toggle_publish_assessment(conn.assigns.current_user, id, toggle_publish_to) case result do @@ -80,27 +80,32 @@ defmodule CadetWeb.AssessmentsController do end def create(conn, %{"assessment" => assessment, "forceUpdate" => force_update}) do - file = assessment["file"].path + role = conn.assigns[:current_user].role + if role == :student do + send_resp(conn, :forbidden, "User not allowed to create") + else + file = assessment["file"].path |> File.read!() - result = - case force_update do - "true" -> parse_xml(file, true) - "false" -> parse_xml(file, false) - end - - case result do - :ok -> - if (force_update == "true") do - send_resp(conn, 200, "Force Update OK") - else - send_resp(conn, 200, "OK") + result = + case force_update do + "true" -> parse_xml(file, true) + "false" -> parse_xml(file, false) end - {:ok, warning_message} -> - send_resp(conn, 200, warning_message) + case result do + :ok -> + if (force_update == "true") do + send_resp(conn, 200, "Force Update OK") + else + send_resp(conn, 200, "OK") + end - {:error, {status, message}} -> - send_resp(conn, status, message) + {:ok, warning_message} -> + send_resp(conn, 200, warning_message) + + {:error, {status, message}} -> + send_resp(conn, status, message) + end end end @@ -153,6 +158,72 @@ defmodule CadetWeb.AssessmentsController do response(403, "Password incorrect") end + swagger_path :create do + post("/assessments") + + summary("Creates a new assessment or updates an existing assessment") + + security([%{JWT: []}]) + + parameters do + assessment(:body, :file, "assessment to create or update", required: true) + forceUpdate(:body, :boolean, "force update", required: true) + end + + response(200, "OK") + response(400, "XML parse error") + response(403, "User not allowed to create") + end + + swagger_path :delete do + PhoenixSwagger.Path.delete("/assessments/:id") + + summary("Deletes an assessment") + + security([%{JWT: []}]) + + parameters do + assessmentId(:path, :integer, "assessment id", required: true) + end + + response(200, "OK") + response(403, "User is not permitted to delete") + end + + swagger_path :publish do + post("/assessments/publish/:id") + + summary("Toggles an assessment between published and unpublished") + + security([%{JWT: []}]) + + parameters do + assessmentId(:path, :integer, "assessment id", required: true) + togglePublishTo(:body, :boolean, "toggles assessment publish state", required: true) + end + + response(200, "OK") + response(403, "User is not permitted to publish") + end + + swagger_path :update do + post("/assessments/update/:id") + + summary("Changes the open/close date of an assessment") + + security([%{JWT: []}]) + + parameters do + assessmentId(:path, :integer, "assessment id", required: true) + closeAt(:body, :string, "open date", required: true) + openAt(:body, :string, "close date", required: true) + end + + response(200, "OK") + response(401, "Assessment is already opened") + response(403, "User is not permitted to edit") + end + def swagger_definitions do %{ AssessmentsList: diff --git a/lib/cadet_web/router.ex b/lib/cadet_web/router.ex index 0c91b8f0f..f779252ed 100644 --- a/lib/cadet_web/router.ex +++ b/lib/cadet_web/router.ex @@ -41,7 +41,7 @@ defmodule CadetWeb.Router do post("/assessments", AssessmentsController, :create) delete("/assessments/:id", AssessmentsController, :delete) post("/assessments/:id", AssessmentsController, :show) - post("/assessments/publish/:id", AssessmentsController, :update) + post("/assessments/publish/:id", AssessmentsController, :publish) post("/assessments/update/:id", AssessmentsController, :update) post("/assessments/:assessmentid/submit", AssessmentsController, :submit) post("/assessments/question/:questionid/submit", AnswerController, :submit) From 228ac589cfe0fe476a5e1c4fc20e2a9895b9c54f Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Sun, 19 Apr 2020 22:22:55 +0800 Subject: [PATCH 06/22] Fixed format --- lib/cadet/assessments/assessments.ex | 38 ++++++++++++++----- lib/cadet/jobs/xml_parser.ex | 17 +++++---- .../controllers/assessments_controller.ex | 21 +++++++--- 3 files changed, 54 insertions(+), 22 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index fc2a00282..6019f1a0e 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -28,6 +28,7 @@ defmodule Cadet.Assessments do if role in @change_dates_assessment_role do assessment = Repo.get(Assessment, id) previous_open_time = assessment.open_at + cond do Timex.before?(close_at, open_at) -> {:error, {:bad_request, "New end date should occur after new opening date"}} @@ -215,6 +216,7 @@ defmodule Cadet.Assessments do def assessment_with_questions_and_answers(id, user = %User{}, password) when is_ecto_id(id) do role = user.role + assessment = if role in @open_all_assessment_roles do Assessment @@ -319,6 +321,7 @@ defmodule Cadet.Assessments do def filter_published_assessments(assessments, user) do role = user.role + case role do :student -> where(assessments, is_published: true) _ -> assessments @@ -352,7 +355,11 @@ defmodule Cadet.Assessments do @spec insert_or_update_assessments_and_questions(map(), [map()], boolean()) :: {:ok, any()} | {:error, Ecto.Multi.name(), any(), %{optional(Ecto.Multi.name()) => any()}} - def insert_or_update_assessments_and_questions(assessment_params, questions_params, force_update) do + def insert_or_update_assessments_and_questions( + assessment_params, + questions_params, + force_update + ) do assessment_multi = Multi.insert_or_update( Multi.new(), @@ -369,7 +376,10 @@ defmodule Cadet.Assessments do Multi.run(multi, String.to_atom("question#{index}"), fn _repo, %{assessment: %Assessment{id: id}} -> question_exists = - Repo.exists?(where(Question, [q], q.assessment_id == ^id and q.display_order == ^index)) + Repo.exists?( + where(Question, [q], q.assessment_id == ^id and q.display_order == ^index) + ) + # the !question_exists check allows for force updating of brand new assessments if !force_update or !question_exists do question_params @@ -378,12 +388,12 @@ defmodule Cadet.Assessments do |> Repo.insert() else params = - (if !question_params.max_xp do + if !question_params.max_xp do question_params |> Map.put(:max_xp, 0) else question_params - end) + end |> Map.put(:display_order, index) %{id: question_id, type: type} = @@ -391,9 +401,15 @@ defmodule Cadet.Assessments do |> Repo.one() if question_params.type != Atom.to_string(type) do - {:error, create_invalid_changeset_with_error(:question, "Question types should remain the same")} + {:error, + create_invalid_changeset_with_error( + :question, + "Question types should remain the same" + )} else - changeset = Question.changeset(%Question{assessment_id: id, id: question_id}, params) + changeset = + Question.changeset(%Question{assessment_id: id, id: question_id}, params) + Repo.update(changeset) end end @@ -408,9 +424,9 @@ defmodule Cadet.Assessments do defp invalid_force_update(assessment_multi, questions_params) do assessment_id = (assessment_multi.operations - |> List.first() - |> elem(1) - |> elem(1)).data.id + |> List.first() + |> elem(1) + |> elem(1)).data.id # check if assessment already exists if !assessment_id do @@ -424,7 +440,8 @@ defmodule Cadet.Assessments do existing_questions_count = where(Question, [q], q.assessment_id == ^assessment_id) |> Repo.all() - |> Enum.count + |> Enum.count() + new_questions_count = Enum.count(questions_params) existing_questions_count != new_questions_count end @@ -439,6 +456,7 @@ defmodule Cadet.Assessments do |> case do nil -> Assessment.changeset(%Assessment{}, params) + assessment -> cond do Timex.after?(assessment.open_at, Timex.now()) -> diff --git a/lib/cadet/jobs/xml_parser.ex b/lib/cadet/jobs/xml_parser.ex index 239d60ce5..849606f45 100644 --- a/lib/cadet/jobs/xml_parser.ex +++ b/lib/cadet/jobs/xml_parser.ex @@ -77,7 +77,8 @@ defmodule Cadet.Updater.XMLParser do end end - @spec parse_xml(String.t(), boolean()) :: :ok | {:ok, String.t} | {:error, {atom(), String.t}} + @spec parse_xml(String.t(), boolean()) :: + :ok | {:ok, String.t()} | {:error, {atom(), String.t()}} def parse_xml(xml, force_update \\ false) do with {:ok, assessment_params} <- process_assessment(xml), {:ok, questions_params} <- process_questions(xml), @@ -102,10 +103,12 @@ defmodule Cadet.Updater.XMLParser do {:error, stage, changeset, _} when is_atom(stage) -> log_error_bad_changeset(changeset, stage) + changeset_error = changeset |> Map.get(:errors) |> extract_changeset_error_message + error_message = "Invalid #{stage} changeset #{changeset_error}" log_and_return_badrequest(error_message) end @@ -117,6 +120,7 @@ defmodule Cadet.Updater.XMLParser do |> nested_tuple_to_list() |> List.flatten() |> Enum.reduce("", fn x, acc -> "#{acc <> to_string(x)} " end) + {:error, {:bad_request, "Invalid XML #{error_message}"}} end @@ -126,7 +130,7 @@ defmodule Cadet.Updater.XMLParser do |> List.foldr("", fn x, acc -> "#{acc <> x} " end) end - @spec process_assessment(String.t()) :: {:ok, map()} | {:error, String.t} + @spec process_assessment(String.t()) :: {:ok, map()} | {:error, String.t()} defp process_assessment(xml) do open_at = Timex.now() @@ -164,7 +168,6 @@ defmodule Cadet.Updater.XMLParser do end {:ok, assessment_params} - rescue # This error is raised by xpath/3 when TASK does not exist (hence is equal to nil) Protocol.UndefinedError -> @@ -188,7 +191,7 @@ defmodule Cadet.Updater.XMLParser do type end - @spec process_questions(String.t()) :: {:ok, [map()]} | {:error, String.t} + @spec process_questions(String.t()) :: {:ok, [map()]} | {:error, String.t()} defp process_questions(xml) do default_library = xpath(xml, ~x"//TASK/DEPLOYMENT"e) default_grading_library = xpath(xml, ~x"//TASK/GRADERDEPLOYMENT"e) @@ -234,7 +237,7 @@ defmodule Cadet.Updater.XMLParser do Logger.error("Changeset: #{inspect(changeset, pretty: true)}") end - @spec process_question_by_question_type(map()) :: map() | {:error, String.t} + @spec process_question_by_question_type(map()) :: map() | {:error, String.t()} defp process_question_by_question_type(question) do question[:entity] |> process_question_entity_by_type(question[:type]) @@ -297,7 +300,7 @@ defmodule Cadet.Updater.XMLParser do {:error, "Invalid question type"} end - @spec process_question_library(map(), any(), any()) :: map() | {:error, String.t} + @spec process_question_library(map(), any(), any()) :: map() | {:error, String.t()} defp process_question_library(question, default_library, default_grading_library) do library = xpath(question[:entity], ~x"./DEPLOYMENT"o) || default_library @@ -361,7 +364,7 @@ defmodule Cadet.Updater.XMLParser do end defp nested_tuple_to_list(tuple) when is_tuple(tuple) do - tuple |> Tuple.to_list |> Enum.map(&nested_tuple_to_list/1) + tuple |> Tuple.to_list() |> Enum.map(&nested_tuple_to_list/1) end defp nested_tuple_to_list(x), do: x diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex index b521cf854..b0b355afe 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -36,7 +36,8 @@ defmodule CadetWeb.AssessmentsController do end def publish(conn, %{"id" => id, "togglePublishTo" => toggle_publish_to}) do - result = Assessments.toggle_publish_assessment(conn.assigns.current_user, id, toggle_publish_to) + result = + Assessments.toggle_publish_assessment(conn.assigns.current_user, id, toggle_publish_to) case result do {:ok, _nil} -> @@ -52,7 +53,14 @@ defmodule CadetWeb.AssessmentsController do def update(conn, %{"id" => id, "closeAt" => close_at, "openAt" => open_at}) do formatted_close_date = elem(DateTime.from_iso8601(close_at), 1) formatted_open_date = elem(DateTime.from_iso8601(open_at), 1) - result = Assessments.change_dates_assessment(conn.assigns.current_user, id, formatted_close_date, formatted_open_date) + + result = + Assessments.change_dates_assessment( + conn.assigns.current_user, + id, + formatted_close_date, + formatted_open_date + ) case result do {:ok, _nil} -> @@ -81,11 +89,14 @@ defmodule CadetWeb.AssessmentsController do def create(conn, %{"assessment" => assessment, "forceUpdate" => force_update}) do role = conn.assigns[:current_user].role + if role == :student do send_resp(conn, :forbidden, "User not allowed to create") else - file = assessment["file"].path - |> File.read!() + file = + assessment["file"].path + |> File.read!() + result = case force_update do "true" -> parse_xml(file, true) @@ -94,7 +105,7 @@ defmodule CadetWeb.AssessmentsController do case result do :ok -> - if (force_update == "true") do + if force_update == "true" do send_resp(conn, 200, "Force Update OK") else send_resp(conn, 200, "OK") From 3c0606d19d1e47180671459ea25ae8f523e44a8c Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Mon, 20 Apr 2020 02:26:25 +0800 Subject: [PATCH 07/22] Modified AssessmentsController tests to fit with changes to how the is_published parameter is now handled --- .../assessments_controller_test.exs | 224 ++++++++++++------ 1 file changed, 151 insertions(+), 73 deletions(-) diff --git a/test/cadet_web/controllers/assessments_controller_test.exs b/test/cadet_web/controllers/assessments_controller_test.exs index a9cd2e7b1..35ddbe2f8 100644 --- a/test/cadet_web/controllers/assessments_controller_test.exs +++ b/test/cadet_web/controllers/assessments_controller_test.exs @@ -64,7 +64,8 @@ defmodule CadetWeb.AssessmentsControllerTest do "maxXp" => 4500, "status" => get_assessment_status(user, &1), "gradingStatus" => "excluded", - "private" => false + "private" => false, + "isPublished" => &1.is_published } ) @@ -80,7 +81,7 @@ defmodule CadetWeb.AssessmentsControllerTest do end end - test "does not render unpublished assessments", %{ + test "render password protected assessments properly", %{ conn: conn, users: users, assessments: assessments @@ -90,74 +91,73 @@ defmodule CadetWeb.AssessmentsControllerTest do {:ok, _} = mission.assessment - |> Assessment.changeset(%{is_published: false}) + |> Assessment.changeset(%{password: "mysupersecretpassword"}) |> Repo.update() - expected = - assessments - |> Map.delete(:mission) - |> Map.values() - |> Enum.map(fn a -> a.assessment end) - |> Enum.sort(&open_at_asc_comparator/2) - |> Enum.map( - &%{ - "id" => &1.id, - "title" => &1.title, - "shortSummary" => &1.summary_short, - "story" => &1.story, - "number" => &1.number, - "reading" => &1.reading, - "openAt" => format_datetime(&1.open_at), - "closeAt" => format_datetime(&1.close_at), - "type" => "#{&1.type}", - "coverImage" => &1.cover_picture, - "maxGrade" => 720, - "maxXp" => 4500, - "status" => get_assessment_status(user, &1), - "gradingStatus" => "excluded", - "private" => false - } - ) - resp = conn |> sign_in(user) |> get(build_url()) |> json_response(200) - |> Enum.map(&Map.delete(&1, "xp")) - |> Enum.map(&Map.delete(&1, "grade")) + |> Enum.find(&(&1["type"] == "mission")) + |> Map.get("private") - assert expected == resp + assert resp == true end end + end - test "render password protected assessments properly", %{ + describe "GET /, student only" do + test "does not render unpublished assessments", %{ conn: conn, - users: users, + users: %{student: student}, assessments: assessments } do - for {_role, user} <- users do - mission = assessments.mission + mission = assessments.mission - {:ok, _} = - mission.assessment - |> Assessment.changeset(%{password: "mysupersecretpassword"}) - |> Repo.update() + {:ok, _} = + mission.assessment + |> Assessment.changeset(%{is_published: false}) + |> Repo.update() - resp = - conn - |> sign_in(user) - |> get(build_url()) - |> json_response(200) - |> Enum.find(&(&1["type"] == "mission")) - |> Map.get("private") + expected = + assessments + |> Map.delete(:mission) + |> Map.values() + |> Enum.map(fn a -> a.assessment end) + |> Enum.sort(&open_at_asc_comparator/2) + |> Enum.map( + &%{ + "id" => &1.id, + "title" => &1.title, + "shortSummary" => &1.summary_short, + "story" => &1.story, + "number" => &1.number, + "reading" => &1.reading, + "openAt" => format_datetime(&1.open_at), + "closeAt" => format_datetime(&1.close_at), + "type" => "#{&1.type}", + "coverImage" => &1.cover_picture, + "maxGrade" => 720, + "maxXp" => 4500, + "status" => get_assessment_status(student, &1), + "gradingStatus" => "excluded", + "private" => false, + "isPublished" => &1.is_published + } + ) - assert resp == true - end + resp = + conn + |> sign_in(student) + |> get(build_url()) + |> json_response(200) + |> Enum.map(&Map.delete(&1, "xp")) + |> Enum.map(&Map.delete(&1, "grade")) + + assert expected == resp end - end - describe "GET /, student only" do test "renders student submission status in overview", %{ conn: conn, users: %{student: student}, @@ -220,6 +220,65 @@ defmodule CadetWeb.AssessmentsControllerTest do end end + describe "GET /, non-students" do + test "renders unpublished assessments", %{ + conn: conn, + users: users, + assessments: assessments + } do + for role <- ~w(staff admin)a do + user = Map.get(users, role) + mission = assessments.mission + + {:ok, _} = + mission.assessment + |> Assessment.changeset(%{is_published: false}) + |> Repo.update() + + resp = + conn + |> sign_in(user) + |> get(build_url()) + |> json_response(200) + |> Enum.map(&Map.delete(&1, "xp")) + |> Enum.map(&Map.delete(&1, "grade")) + + expected = + assessments + |> Map.values() + |> Enum.map(fn a -> a.assessment end) + |> Enum.sort(&open_at_asc_comparator/2) + |> Enum.map( + &%{ + "id" => &1.id, + "title" => &1.title, + "shortSummary" => &1.summary_short, + "story" => &1.story, + "number" => &1.number, + "reading" => &1.reading, + "openAt" => format_datetime(&1.open_at), + "closeAt" => format_datetime(&1.close_at), + "type" => "#{&1.type}", + "coverImage" => &1.cover_picture, + "maxGrade" => 720, + "maxXp" => 4500, + "status" => get_assessment_status(user, &1), + "gradingStatus" => "excluded", + "private" => false, + "isPublished" => + if &1.type == :mission do + false + else + &1.is_published + end + } + ) + + assert expected == resp + end + end + end + describe "POST /assessment_id, all roles" do test "it renders assessment details", %{ conn: conn, @@ -499,28 +558,6 @@ defmodule CadetWeb.AssessmentsControllerTest do end end end - - test "it does not permit access to unpublished assessments", %{ - conn: conn, - users: users, - assessments: %{mission: mission} - } do - for role <- Role.__enum_map__() do - user = Map.get(users, role) - - {:ok, _} = - mission.assessment - |> Assessment.changeset(%{is_published: false}) - |> Repo.update() - - conn = - conn - |> sign_in(user) - |> post(build_url(mission.assessment.id)) - - assert response(conn, 400) == "Assessment not found" - end - end end describe "POST /assessment_id, student" do @@ -601,6 +638,24 @@ defmodule CadetWeb.AssessmentsControllerTest do assert response(conn, 401) == "Assessment not open" end + + test "it does not permit access to unpublished assessments", %{ + conn: conn, + users: %{student: student}, + assessments: %{mission: mission} + } do + {:ok, _} = + mission.assessment + |> Assessment.changeset(%{is_published: false}) + |> Repo.update() + + conn = + conn + |> sign_in(student) + |> post(build_url(mission.assessment.id)) + + assert response(conn, 400) == "Assessment not found" + end end describe "POST /assessment_id, non-students" do @@ -650,6 +705,29 @@ defmodule CadetWeb.AssessmentsControllerTest do assert resp["id"] == mission.assessment.id end end + + test "it permits access to unpublished assessments", %{ + conn: conn, + users: users, + assessments: %{mission: mission} + } do + for role <- ~w(staff admin)a do + user = Map.get(users, role) + + {:ok, _} = + mission.assessment + |> Assessment.changeset(%{is_published: false}) + |> Repo.update() + + resp = + conn + |> sign_in(user) + |> post(build_url(mission.assessment.id)) + |> json_response(200) + + assert resp["id"] == mission.assessment.id + end + end end describe "POST /assessment_id/submit unauthenticated" do From 5209f7fba9864ef16ccb4833ae3ba43b5126a8d4 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Mon, 20 Apr 2020 13:39:55 +0800 Subject: [PATCH 08/22] Updated XML parsers tests --- lib/cadet/jobs/xml_parser.ex | 19 ++++-- test/cadet/updater/xml_parser_test.exs | 93 +++++++++++++------------- 2 files changed, 57 insertions(+), 55 deletions(-) diff --git a/lib/cadet/jobs/xml_parser.ex b/lib/cadet/jobs/xml_parser.ex index 849606f45..38e415b26 100644 --- a/lib/cadet/jobs/xml_parser.ex +++ b/lib/cadet/jobs/xml_parser.ex @@ -68,6 +68,10 @@ defmodule Cadet.Updater.XMLParser do Logger.error(error_message) Sentry.capture_message(error_message) :error + + {:error, {_status, error_message}} -> + Sentry.capture_message(error_message) + :error end end |> Enum.any?(&(&1 == :error)) @@ -98,8 +102,8 @@ defmodule Cadet.Updater.XMLParser do Logger.warn("Assessment already open, ignoring...") {:ok, "Assessment already open, ignoring..."} - {:error, errmsg} -> - log_and_return_badrequest(errmsg) + {:error, error_message} -> + log_and_return_badrequest(error_message) {:error, stage, changeset, _} when is_atom(stage) -> log_error_bad_changeset(changeset, stage) @@ -115,6 +119,7 @@ defmodule Cadet.Updater.XMLParser do catch # the :erlsom library used by SweetXml will exit if XML is invalid :exit, parse_error -> + # error info is stored in multiple nested tuples error_message = parse_error |> nested_tuple_to_list() @@ -217,8 +222,8 @@ defmodule Cadet.Updater.XMLParser do {:no_missing_attr?, false} -> {:error, "Missing attribute(s) on PROBLEM"} - {:error, errmsg} -> - {:error, errmsg} + {:error, error_message} -> + {:error, error_message} end end) @@ -245,8 +250,8 @@ defmodule Cadet.Updater.XMLParser do question_map when is_map(question_map) -> Map.put(question, :question, question_map) - {:error, errmsg} -> - {:error, errmsg} + {:error, error_message} -> + {:error, error_message} end end @@ -297,7 +302,7 @@ defmodule Cadet.Updater.XMLParser do end defp process_question_entity_by_type(_, _) do - {:error, "Invalid question type"} + {:error, "Invalid question type."} end @spec process_question_library(map(), any(), any()) :: map() | {:error, String.t()} diff --git a/test/cadet/updater/xml_parser_test.exs b/test/cadet/updater/xml_parser_test.exs index da175973a..2725128ff 100644 --- a/test/cadet/updater/xml_parser_test.exs +++ b/test/cadet/updater/xml_parser_test.exs @@ -10,7 +10,6 @@ defmodule Cadet.Updater.XMLParserTest do @local_name "test/fixtures/local_repo" # @locations %{mission: "missions", sidequest: "quests", path: "paths", contest: "contests"} - @time_fields ~w(open_at close_at)a setup do File.rm_rf!(@local_name) @@ -50,8 +49,22 @@ defmodule Cadet.Updater.XMLParserTest do |> where(number: ^number) |> Repo.one() + open_at = + Timex.now() + |> Timex.beginning_of_day() + |> Timex.shift(days: 3) + |> Timex.shift(hours: 4) + + close_at = Timex.shift(open_at, days: 7) + + expected_assesment = + assessment + |> Map.put(:open_at, open_at) + |> Map.put(:close_at, close_at) + |> Map.put(:is_published, false) + assert_map_keys( - Map.from_struct(assessment), + Map.from_struct(expected_assesment), Map.from_struct(assessment_db), ~w(title is_published type summary_short summary_long open_at close_at)a ++ ~w(number story reading password)a @@ -97,51 +110,17 @@ defmodule Cadet.Updater.XMLParserTest do end end - test "open and close dates not in ISO8601 DateTime", %{ - assessments: assessments, - questions: questions - } do - date_strings = - Enum.map( - ~w({ISO:Basic} {ISOdate} {RFC822} {RFC1123} {ANSIC} {UNIX}), - &{&1, Timex.format!(Timex.now(), &1)} - ) - - for assessment <- assessments, - {date_format_string, date_string} <- date_strings, - time_field <- @time_fields do - assessment_wrong_date_format = %{assessment | time_field => date_string} - - xml = XMLGenerator.generate_xml_for(assessment_wrong_date_format, questions) - - assert capture_log(fn -> - assert( - XMLParser.parse_xml(xml) == :error, - inspect({date_format_string, date_string}, pretty: true) - ) - end) =~ "Time does not conform to ISO8601 DateTime" - end - end - - test "open and close time without offset", %{assessments: assessments, questions: questions} do - datetime_string = Timex.format!(Timex.now(), "{YYYY}-{0M}-{0D}T{h24}:{m}:{s}") - - for assessment <- assessments, - time_field <- @time_fields do - assessment_time_without_offset = %{assessment | time_field => datetime_string} - xml = XMLGenerator.generate_xml_for(assessment_time_without_offset, questions) - - assert capture_log(fn -> assert XMLParser.parse_xml(xml) == :error end) =~ - "Time does not have offset specified." - end - end - test "PROBLEM with missing type", %{assessments: assessments, questions: questions} do for assessment <- assessments do xml = XMLGenerator.generate_xml_for(assessment, questions, problem_permit_keys: ~w(maxgrade)a) - assert capture_log(fn -> assert(XMLParser.parse_xml(xml) == :error) end) =~ + assert capture_log(fn -> + assert( + XMLParser.parse_xml(xml) == + {:error, {:bad_request, "Missing attribute(s) on PROBLEM"}} + ) + end) =~ "Missing attribute(s) on PROBLEM" end end @@ -150,7 +129,12 @@ defmodule Cadet.Updater.XMLParserTest do for assessment <- assessments do xml = XMLGenerator.generate_xml_for(assessment, questions, problem_permit_keys: ~w(type)a) - assert capture_log(fn -> assert(XMLParser.parse_xml(xml) == :error) end) =~ + assert capture_log(fn -> + assert( + XMLParser.parse_xml(xml) == + {:error, {:bad_request, "Missing attribute(s) on PROBLEM"}} + ) + end) =~ "Missing attribute(s) on PROBLEM" end end @@ -159,7 +143,11 @@ defmodule Cadet.Updater.XMLParserTest do for assessment <- assessments do xml = XMLGenerator.generate_xml_for(assessment, questions, override_type: "anu") - assert capture_log(fn -> assert(XMLParser.parse_xml(xml) == :error) end) =~ + assert capture_log(fn -> + assert( + XMLParser.parse_xml(xml) == {:error, {:bad_request, "Invalid question type."}} + ) + end) =~ "Invalid question type." end end @@ -171,7 +159,10 @@ defmodule Cadet.Updater.XMLParserTest do xml = XMLGenerator.generate_xml_for(assessment, questions_without_content) - assert capture_log(fn -> assert(XMLParser.parse_xml(xml) == :error) end) =~ + # the error message can be quite convoluted + assert capture_log(fn -> + assert({:error, {:bad_request, _error_message}} = XMLParser.parse_xml(xml)) + end) =~ ~r/Invalid \b.*\b changeset\./ end end @@ -180,7 +171,11 @@ defmodule Cadet.Updater.XMLParserTest do for assessment <- assessments do xml = XMLGenerator.generate_xml_for(assessment, questions, no_deployment: true) - assert capture_log(fn -> assert(XMLParser.parse_xml(xml) == :error) end) =~ + assert capture_log(fn -> + assert( + XMLParser.parse_xml(xml) == {:error, {:bad_request, "Missing DEPLOYMENT"}} + ) + end) =~ "Missing DEPLOYMENT" end end @@ -200,7 +195,9 @@ defmodule Cadet.Updater.XMLParserTest do xml = XMLGenerator.generate_xml_for(assessment, questions) - assert capture_log(fn -> assert XMLParser.parse_xml(xml) == :ok end) =~ + assert capture_log(fn -> + assert XMLParser.parse_xml(xml) == {:ok, "Assessment already open, ignoring..."} + end) =~ "Assessment already open, ignoring..." end end @@ -308,7 +305,7 @@ defmodule Cadet.Updater.XMLParserTest do """) assert capture_log(fn -> - XMLParser.parse_and_insert(path) == {:error, "Error processing XML files."} + XMLParser.parse_and_insert(path) == {:error, {:bad_request, "Missing TASK"}} end) =~ "Missing TASK" end end From bddadf8d56b0154917663b125412e5f40906987f Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Mon, 20 Apr 2020 13:48:59 +0800 Subject: [PATCH 09/22] Changed assessment deletion such that associated submissions are also deleted --- lib/cadet/assessments/assessments.ex | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 6019f1a0e..6a9d0969f 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -61,6 +61,11 @@ defmodule Cadet.Assessments do def delete_assessment(_deleter = %User{role: role}, id) do if role in @delete_assessment_role do assessment = Repo.get(Assessment, id) + + Submission + |> where(assessment_id: ^id) + |> Repo.delete_all() + Repo.delete(assessment) else {:error, {:forbidden, "User is not permitted to delete"}} From 9dd8c52b0c333470d56d9c27d0889baff8ebb746 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Mon, 20 Apr 2020 14:11:52 +0800 Subject: [PATCH 10/22] Minor formatting change --- lib/cadet/assessments/assessments.ex | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 6a9d0969f..6429755d8 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -393,16 +393,13 @@ defmodule Cadet.Assessments do |> Repo.insert() else params = - if !question_params.max_xp do - question_params - |> Map.put(:max_xp, 0) - else - question_params - end + question_params + |> Map.put_new(:max_xp, 0) |> Map.put(:display_order, index) %{id: question_id, type: type} = - where(Question, [q], q.display_order == ^index and q.assessment_id == ^id) + Question + |> where([q], q.display_order == ^index and q.assessment_id == ^id) |> Repo.one() if question_params.type != Atom.to_string(type) do @@ -433,23 +430,23 @@ defmodule Cadet.Assessments do |> elem(1) |> elem(1)).data.id - # check if assessment already exists - if !assessment_id do - false - else + if assessment_id do open_date = Repo.get(Assessment, assessment_id).open_at # check if assessment is already opened if Timex.after?(open_date, Timex.now()) do false else existing_questions_count = - where(Question, [q], q.assessment_id == ^assessment_id) + Question + |> where([q], q.assessment_id == ^assessment_id) |> Repo.all() |> Enum.count() new_questions_count = Enum.count(questions_params) existing_questions_count != new_questions_count end + else + false end end From f34d1df2fa0909352df029cb16cc07a96ea32111 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Mon, 20 Apr 2020 18:33:39 +0800 Subject: [PATCH 11/22] Updated AssessmentOverview schema --- lib/cadet_web/controllers/assessments_controller.ex | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex index b0b355afe..f0209760a 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -293,6 +293,8 @@ defmodule CadetWeb.AssessmentsController do coverImage(:string, "The URL to the cover picture", required: true) private(:boolean, "Is this an private assessment?", required: true) + + isPublished(:boolean, "Is the assessment published?", required: true) end end, Assessment: From 028043346384c8e79fde91bef165787007badae5 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Thu, 23 Apr 2020 13:16:55 +0800 Subject: [PATCH 12/22] Fixed a bug that occurs when deleting assessment --- lib/cadet/assessments/assessments.ex | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 6429755d8..498cad7cc 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -7,7 +7,7 @@ defmodule Cadet.Assessments do import Ecto.Query - alias Cadet.Accounts.{Notifications, User} + alias Cadet.Accounts.{Notification, Notifications, User} alias Cadet.Assessments.{Answer, Assessment, Query, Question, Submission} alias Cadet.Autograder.GradingJob alias Cadet.Chat.Room @@ -64,7 +64,7 @@ defmodule Cadet.Assessments do Submission |> where(assessment_id: ^id) - |> Repo.delete_all() + |> delete_submission_assocation(id) Repo.delete(assessment) else @@ -72,6 +72,23 @@ defmodule Cadet.Assessments do end end + defp delete_submission_assocation(submissions, assessment_id) do + submissions + |> Repo.all() + |> Enum.each(fn submission -> + Answer + |> where(submission_id: ^submission.id) + |> Repo.delete_all() + end) + + Notification + |> where(assessment_id: ^assessment_id) + |> Repo.delete_all() + + Repo.delete_all(submissions) + end + + @spec user_total_xp(%User{}) :: integer() def user_total_xp(%User{id: user_id}) when is_ecto_id(user_id) do total_xp_bonus = From 2e5f4c3e381f6f9103a6e3945bc7825ee7edd7a0 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Thu, 23 Apr 2020 13:18:00 +0800 Subject: [PATCH 13/22] Changed formatting --- lib/cadet/assessments/assessments.ex | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 498cad7cc..c4b44c4a5 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -88,7 +88,6 @@ defmodule Cadet.Assessments do Repo.delete_all(submissions) end - @spec user_total_xp(%User{}) :: integer() def user_total_xp(%User{id: user_id}) when is_ecto_id(user_id) do total_xp_bonus = From 905a9194b343272833c9044353153efeebd1cdd3 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Sat, 25 Apr 2020 13:08:23 +0800 Subject: [PATCH 14/22] Minor formatting change --- lib/cadet_web/controllers/assessments_controller.ex | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex index f0209760a..ed6629e54 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -6,6 +6,8 @@ defmodule CadetWeb.AssessmentsController do alias Cadet.Assessments import Cadet.Updater.XMLParser, only: [parse_xml: 2] + @create_assessment_roles ~w(staff admin)a + def submit(conn, %{"assessmentid" => assessment_id}) when is_ecto_id(assessment_id) do case Assessments.finalise_submission(assessment_id, conn.assigns.current_user) do {:ok, _nil} -> @@ -90,9 +92,7 @@ defmodule CadetWeb.AssessmentsController do def create(conn, %{"assessment" => assessment, "forceUpdate" => force_update}) do role = conn.assigns[:current_user].role - if role == :student do - send_resp(conn, :forbidden, "User not allowed to create") - else + if role in @create_assessment_roles do file = assessment["file"].path |> File.read!() @@ -117,6 +117,8 @@ defmodule CadetWeb.AssessmentsController do {:error, {status, message}} -> send_resp(conn, status, message) end + else + send_resp(conn, :forbidden, "User not allowed to create assessment") end end From 3c844996cae524f068d08b442e1185e1f4d064a8 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Sat, 25 Apr 2020 13:42:05 +0800 Subject: [PATCH 15/22] Changed variable names and changed formatting --- .../controllers/assessments_controller.ex | 108 ++++++++++-------- lib/cadet_web/router.ex | 6 +- 2 files changed, 62 insertions(+), 52 deletions(-) diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex index ed6629e54..702b13836 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -37,9 +37,41 @@ defmodule CadetWeb.AssessmentsController do end end - def publish(conn, %{"id" => id, "togglePublishTo" => toggle_publish_to}) do - result = - Assessments.toggle_publish_assessment(conn.assigns.current_user, id, toggle_publish_to) + def create(conn, %{"assessment" => assessment, "forceUpdate" => force_update}) do + role = conn.assigns[:current_user].role + + if role in @create_assessment_roles do + file = + assessment["file"].path + |> File.read!() + + result = + case force_update do + "true" -> parse_xml(file, true) + "false" -> parse_xml(file, false) + end + + case result do + :ok -> + if force_update == "true" do + send_resp(conn, 200, "Force Update OK") + else + send_resp(conn, 200, "OK") + end + + {:ok, warning_message} -> + send_resp(conn, 200, warning_message) + + {:error, {status, message}} -> + send_resp(conn, status, message) + end + else + send_resp(conn, :forbidden, "User not allowed to create assessment") + end + end + + def delete(conn, %{"assessmentid" => assessment_id}) do + result = Assessments.delete_assessment(conn.assigns.current_user, assessment_id) case result do {:ok, _nil} -> @@ -52,16 +84,12 @@ defmodule CadetWeb.AssessmentsController do end end - def update(conn, %{"id" => id, "closeAt" => close_at, "openAt" => open_at}) do - formatted_close_date = elem(DateTime.from_iso8601(close_at), 1) - formatted_open_date = elem(DateTime.from_iso8601(open_at), 1) - + def publish(conn, %{"assessmentid" => assessment_id, "togglePublishTo" => toggle_publish_to}) do result = - Assessments.change_dates_assessment( + Assessments.toggle_publish_assessment( conn.assigns.current_user, - id, - formatted_close_date, - formatted_open_date + assessment_id, + toggle_publish_to ) case result do @@ -75,8 +103,17 @@ defmodule CadetWeb.AssessmentsController do end end - def delete(conn, %{"id" => id}) do - result = Assessments.delete_assessment(conn.assigns.current_user, id) + def update(conn, %{"assessmentid" => assessment_id, "closeAt" => close_at, "openAt" => open_at}) do + formatted_close_date = elem(DateTime.from_iso8601(close_at), 1) + formatted_open_date = elem(DateTime.from_iso8601(open_at), 1) + + result = + Assessments.change_dates_assessment( + conn.assigns.current_user, + assessment_id, + formatted_close_date, + formatted_open_date + ) case result do {:ok, _nil} -> @@ -89,39 +126,6 @@ defmodule CadetWeb.AssessmentsController do end end - def create(conn, %{"assessment" => assessment, "forceUpdate" => force_update}) do - role = conn.assigns[:current_user].role - - if role in @create_assessment_roles do - file = - assessment["file"].path - |> File.read!() - - result = - case force_update do - "true" -> parse_xml(file, true) - "false" -> parse_xml(file, false) - end - - case result do - :ok -> - if force_update == "true" do - send_resp(conn, 200, "Force Update OK") - else - send_resp(conn, 200, "OK") - end - - {:ok, warning_message} -> - send_resp(conn, 200, warning_message) - - {:error, {status, message}} -> - send_resp(conn, status, message) - end - else - send_resp(conn, :forbidden, "User not allowed to create assessment") - end - end - swagger_path :submit do post("/assessments/{assessmentId}/submit") summary("Finalise submission for an assessment") @@ -178,6 +182,8 @@ defmodule CadetWeb.AssessmentsController do security([%{JWT: []}]) + consumes("application/json") + parameters do assessment(:body, :file, "assessment to create or update", required: true) forceUpdate(:body, :boolean, "force update", required: true) @@ -189,7 +195,7 @@ defmodule CadetWeb.AssessmentsController do end swagger_path :delete do - PhoenixSwagger.Path.delete("/assessments/:id") + PhoenixSwagger.Path.delete("/assessments/:assessmentid") summary("Deletes an assessment") @@ -204,12 +210,14 @@ defmodule CadetWeb.AssessmentsController do end swagger_path :publish do - post("/assessments/publish/:id") + post("/assessments/publish/:assessmentid") summary("Toggles an assessment between published and unpublished") security([%{JWT: []}]) + consumes("application/json") + parameters do assessmentId(:path, :integer, "assessment id", required: true) togglePublishTo(:body, :boolean, "toggles assessment publish state", required: true) @@ -220,12 +228,14 @@ defmodule CadetWeb.AssessmentsController do end swagger_path :update do - post("/assessments/update/:id") + post("/assessments/update/:assessmentid") summary("Changes the open/close date of an assessment") security([%{JWT: []}]) + consumes("application/json") + parameters do assessmentId(:path, :integer, "assessment id", required: true) closeAt(:body, :string, "open date", required: true) diff --git a/lib/cadet_web/router.ex b/lib/cadet_web/router.ex index f779252ed..18b5a75f1 100644 --- a/lib/cadet_web/router.ex +++ b/lib/cadet_web/router.ex @@ -39,10 +39,10 @@ defmodule CadetWeb.Router do get("/assessments", AssessmentsController, :index) post("/assessments", AssessmentsController, :create) - delete("/assessments/:id", AssessmentsController, :delete) + delete("/assessments/:assessmentid", AssessmentsController, :delete) post("/assessments/:id", AssessmentsController, :show) - post("/assessments/publish/:id", AssessmentsController, :publish) - post("/assessments/update/:id", AssessmentsController, :update) + post("/assessments/publish/:assessmentid", AssessmentsController, :publish) + post("/assessments/update/:assessmentid", AssessmentsController, :update) post("/assessments/:assessmentid/submit", AssessmentsController, :submit) post("/assessments/question/:questionid/submit", AnswerController, :submit) From 32ac32fa955b29d9109eed7c0519be4c414f978d Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Tue, 28 Apr 2020 11:16:15 +0800 Subject: [PATCH 16/22] Minor formatting change --- .../controllers/assessments_controller.ex | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex index 702b13836..7daa9d531 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -54,19 +54,23 @@ defmodule CadetWeb.AssessmentsController do case result do :ok -> if force_update == "true" do - send_resp(conn, 200, "Force Update OK") + text(conn, "Force Update OK") else - send_resp(conn, 200, "OK") + text(conn, "OK") end {:ok, warning_message} -> - send_resp(conn, 200, warning_message) + text(conn, warning_message) {:error, {status, message}} -> - send_resp(conn, status, message) + conn + |> put_status(status) + |> text(message) end else - send_resp(conn, :forbidden, "User not allowed to create assessment") + conn + |> put_status(:forbidden) + |> text("User not allowed to create assessment") end end @@ -75,7 +79,7 @@ defmodule CadetWeb.AssessmentsController do case result do {:ok, _nil} -> - send_resp(conn, 200, "OK") + text(conn, "OK") {:error, {status, message}} -> conn @@ -94,7 +98,7 @@ defmodule CadetWeb.AssessmentsController do case result do {:ok, _nil} -> - send_resp(conn, 200, "OK") + text(conn, "OK") {:error, {status, message}} -> conn @@ -117,7 +121,7 @@ defmodule CadetWeb.AssessmentsController do case result do {:ok, _nil} -> - send_resp(conn, 200, "OK") + text(conn, "OK") {:error, {status, message}} -> conn From 9ae818635f4feff394064eda415dc5ac760a1ddd Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Sat, 2 May 2020 15:06:24 +0800 Subject: [PATCH 17/22] Modified how publishing of assessment is done --- lib/cadet/assessments/assessments.ex | 5 +++-- lib/cadet_web/controllers/assessments_controller.ex | 5 ++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index c4b44c4a5..6e5bf79e4 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -50,9 +50,10 @@ defmodule Cadet.Assessments do end end - def toggle_publish_assessment(_publisher = %User{role: role}, id, toggle_publish_to) do + def toggle_publish_assessment(_publisher = %User{role: role}, id) do if role in @publish_assessment_role do - update_assessment(id, %{is_published: toggle_publish_to}) + assessment = Repo.get(Assessment, id) + update_assessment(id, %{is_published: !assessment.is_published}) else {:error, {:forbidden, "User is not permitted to publish"}} end diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex index 7daa9d531..b583c40a6 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -88,12 +88,11 @@ defmodule CadetWeb.AssessmentsController do end end - def publish(conn, %{"assessmentid" => assessment_id, "togglePublishTo" => toggle_publish_to}) do + def publish(conn, %{"assessmentid" => assessment_id}) do result = Assessments.toggle_publish_assessment( conn.assigns.current_user, - assessment_id, - toggle_publish_to + assessment_id ) case result do From 234b0ff7d9e837415d4d5c4341d47f3340943736 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Sat, 2 May 2020 15:40:58 +0800 Subject: [PATCH 18/22] Updated swagger_path --- lib/cadet_web/controllers/assessments_controller.ex | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex index b583c40a6..1c92b355b 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -219,11 +219,8 @@ defmodule CadetWeb.AssessmentsController do security([%{JWT: []}]) - consumes("application/json") - parameters do assessmentId(:path, :integer, "assessment id", required: true) - togglePublishTo(:body, :boolean, "toggles assessment publish state", required: true) end response(200, "OK") From 12797b54e54715d386aa9cf37cd85bf58d459ed6 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Sat, 2 May 2020 15:42:29 +0800 Subject: [PATCH 19/22] Fixed bugged that allowed setting open date of unopened assessments to before current time --- 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 6e5bf79e4..dea40f283 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -36,12 +36,12 @@ defmodule Cadet.Assessments do Timex.before?(close_at, Timex.now()) -> {:error, {:bad_request, "New end date should occur after current time"}} - Timex.equal?(previous_open_time, open_at) or Timex.after?(previous_open_time, Timex.now()) -> - update_assessment(id, %{close_at: close_at, open_at: open_at}) - Timex.before?(open_at, Timex.now()) -> {:error, {:bad_request, "New Opening date should occur after current time"}} + Timex.equal?(previous_open_time, open_at) or Timex.after?(previous_open_time, Timex.now()) -> + update_assessment(id, %{close_at: close_at, open_at: open_at}) + true -> {:error, {:unauthorized, "Assessment is already opened"}} end From 99ffb80d7b0440a6a177b93b10e3eaa2748f80da Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Sat, 2 May 2020 18:36:10 +0800 Subject: [PATCH 20/22] Added tests --- .../assessments_controller_test.exs | 373 ++++++++++++++++++ 1 file changed, 373 insertions(+) diff --git a/test/cadet_web/controllers/assessments_controller_test.exs b/test/cadet_web/controllers/assessments_controller_test.exs index 35ddbe2f8..852d8f3be 100644 --- a/test/cadet_web/controllers/assessments_controller_test.exs +++ b/test/cadet_web/controllers/assessments_controller_test.exs @@ -3,15 +3,25 @@ defmodule CadetWeb.AssessmentsControllerTest do use Timex import Ecto.Query + import ExUnit.CaptureLog import Mock alias Cadet.{Assessments, Repo} alias Cadet.Accounts.{Role, User} alias Cadet.Assessments.{Assessment, AssessmentType, Submission, SubmissionStatus} alias Cadet.Autograder.GradingJob + alias Cadet.Test.XMLGenerator alias CadetWeb.AssessmentsController + @local_name "test/fixtures/local_repo" + setup do + File.rm_rf!(@local_name) + + on_exit(fn -> + File.rm_rf!(@local_name) + end) + Cadet.Test.Seeds.assessments() end @@ -1199,9 +1209,372 @@ defmodule CadetWeb.AssessmentsControllerTest do end end + describe "POST /, unauthenticated" do + test "unauthorized", %{conn: conn} do + assessment = build(:assessment, type: :mission, is_published: true) + questions = build_list(5, :question, assessment: nil) + xml = XMLGenerator.generate_xml_for(assessment, questions) + file = File.write("test/fixtures/local_repo/test.xml", xml) + force_update = "false" + body = %{assessment: file, forceUpdate: force_update} + conn = post(conn, build_url(), body) + assert response(conn, 401) =~ "Unauthorised" + end + end + + describe "POST /, student only" do + @tag authenticate: :student + test "unauthorized", %{conn: conn} do + assessment = build(:assessment, type: :mission, is_published: true) + questions = build_list(5, :question, assessment: nil) + xml = XMLGenerator.generate_xml_for(assessment, questions) + force_update = "false" + body = %{assessment: xml, forceUpdate: force_update} + conn = post(conn, build_url(), body) + assert response(conn, 403) == "User not allowed to create assessment" + end + end + + describe "POST /, staff only" do + @tag authenticate: :staff + test "successful", %{conn: conn} do + assessment = build(:assessment, type: :mission, is_published: true) + questions = build_list(5, :question, assessment: nil) + + xml = XMLGenerator.generate_xml_for(assessment, questions) + force_update = "false" + path = Path.join(@local_name, "connTest") + file_name = "test.xml" + location = Path.join(path, file_name) + File.mkdir_p!(path) + File.write!(location, xml) + + formdata = %Plug.Upload{ + content_type: "text/xml", + filename: file_name, + path: location + } + + body = %{assessment: %{file: formdata}, forceUpdate: force_update} + conn = post(conn, build_url(), body) + number = assessment.number + + expected_assessment = + Assessment + |> where(number: ^number) + |> Repo.one() + + assert response(conn, 200) == "OK" + assert expected_assessment != nil + end + + @tag authenticate: :staff + test "upload empty xml", %{conn: conn} do + xml = "" + force_update = "true" + path = Path.join(@local_name, "connTest") + file_name = "test.xml" + location = Path.join(path, file_name) + File.mkdir_p!(path) + File.write!(location, xml) + + formdata = %Plug.Upload{ + content_type: "text/xml", + filename: file_name, + path: location + } + + body = %{assessment: %{file: formdata}, forceUpdate: force_update} + + err_msg = + "Invalid XML fatal expected_element_start_tag file file_name_unknown line 1 col 1 " + + assert capture_log(fn -> + conn = post(conn, build_url(), body) + assert(response(conn, 400) == err_msg) + end) =~ ~r/.*fatal: :expected_element_start_tag.*/ + end + end + + describe "DELETE /:assessment_id, unauthenticated" do + test "unauthorized", %{conn: conn} do + assessment = insert(:assessment) + conn = delete(conn, build_url(assessment.id)) + assert response(conn, 401) =~ "Unauthorised" + end + end + + describe "DELETE /:assessment_id, student only" do + @tag authenticate: :student + test "unauthorized", %{conn: conn} do + assessment = insert(:assessment) + conn = delete(conn, build_url(assessment.id)) + assert response(conn, 403) == "User is not permitted to delete" + end + end + + describe "DELETE /:assessment_id, staff only" do + @tag authenticate: :staff + test "successful", %{conn: conn} do + assessment = insert(:assessment) + conn = delete(conn, build_url(assessment.id)) + assert response(conn, 200) == "OK" + assert Repo.get(Assessment, assessment.id) == nil + end + end + + describe "POST /publish/:assessment_id, unauthenticated" do + test "unauthorized", %{conn: conn} do + assessment = insert(:assessment) + conn = post(conn, build_url_publish(assessment.id)) + assert response(conn, 401) =~ "Unauthorised" + end + end + + describe "POST /publish/:assessment_id, student only" do + @tag authenticate: :student + test "forbidden", %{conn: conn} do + assessment = insert(:assessment) + conn = post(conn, build_url_publish(assessment.id)) + assert response(conn, 403) == "User is not permitted to publish" + end + end + + describe "POST /publish/:assessment_id, staff only" do + @tag authenticate: :staff + test "successful toggle from published to unpublished", %{conn: conn} do + assessment = insert(:assessment, is_published: true) + conn = post(conn, build_url_publish(assessment.id)) + expected = Repo.get(Assessment, assessment.id).is_published + assert response(conn, 200) == "OK" + assert expected == false + end + + @tag authenticate: :staff + test "successful toggle from unpublished to published", %{conn: conn} do + assessment = insert(:assessment, is_published: false) + conn = post(conn, build_url_publish(assessment.id)) + expected = Repo.get(Assessment, assessment.id).is_published + assert response(conn, 200) == "OK" + assert expected == true + end + end + + describe "POST /update/:assessment_id, unauthenticated" do + test "unauthorized", %{conn: conn} do + assessment = insert(:assessment) + conn = post(conn, build_url_update(assessment.id)) + assert response(conn, 401) =~ "Unauthorised" + end + end + + describe "POST /update/:assessment_id, student only" do + @tag authenticate: :student + test "forbidden", %{conn: conn} do + new_open_at = + Timex.now() + |> Timex.beginning_of_day() + |> Timex.shift(days: 3) + |> Timex.shift(hours: 4) + + new_open_at_string = + new_open_at + |> Timex.format!("{ISO:Extended}") + + new_close_at = Timex.shift(new_open_at, days: 7) + + new_close_at_string = + new_close_at + |> Timex.format!("{ISO:Extended}") + + new_dates = %{openAt: new_open_at_string, closeAt: new_close_at_string} + assessment = insert(:assessment) + conn = post(conn, build_url_update(assessment.id), new_dates) + assert response(conn, 403) == "User is not permitted to edit" + end + end + + describe "POST /update/:assessment_id, staff only" do + @tag authenticate: :staff + test "successful", %{conn: conn} do + open_at = + Timex.now() + |> Timex.beginning_of_day() + |> Timex.shift(days: 3) + |> Timex.shift(hours: 4) + + close_at = Timex.shift(open_at, days: 7) + assessment = insert(:assessment, %{open_at: open_at, close_at: close_at}) + + new_open_at = + open_at + |> Timex.shift(days: 3) + + new_open_at_string = + new_open_at + |> Timex.format!("{ISO:Extended}") + + new_close_at = + close_at + |> Timex.shift(days: 5) + + new_close_at_string = + new_close_at + |> Timex.format!("{ISO:Extended}") + + new_dates = %{openAt: new_open_at_string, closeAt: new_close_at_string} + + conn = + conn + |> post(build_url_update(assessment.id), new_dates) + + assessment = Repo.get(Assessment, assessment.id) + assert response(conn, 200) == "OK" + assert [assessment.open_at, assessment.close_at] == [new_open_at, new_close_at] + end + + @tag authenticate: :staff + test "not allowed to change open time of opened assessments", %{conn: conn} do + open_at = + Timex.now() + |> Timex.beginning_of_day() + |> Timex.shift(days: -3) + |> Timex.shift(hours: 4) + + close_at = Timex.shift(open_at, days: 7) + assessment = insert(:assessment, %{open_at: open_at, close_at: close_at}) + + new_open_at = + open_at + |> Timex.shift(days: 6) + + new_open_at_string = + new_open_at + |> Timex.format!("{ISO:Extended}") + + close_at_string = + close_at + |> Timex.format!("{ISO:Extended}") + + new_dates = %{openAt: new_open_at_string, closeAt: close_at_string} + + conn = + conn + |> post(build_url_update(assessment.id), new_dates) + + assessment = Repo.get(Assessment, assessment.id) + assert response(conn, 401) == "Assessment is already opened" + assert [assessment.open_at, assessment.close_at] == [open_at, close_at] + end + + @tag authenticate: :staff + test "not allowed to set close time to before open time", %{conn: conn} do + open_at = + Timex.now() + |> Timex.beginning_of_day() + |> Timex.shift(days: 3) + |> Timex.shift(hours: 4) + + close_at = Timex.shift(open_at, days: 7) + assessment = insert(:assessment, %{open_at: open_at, close_at: close_at}) + + new_close_at = + open_at + |> Timex.shift(days: -1) + + new_close_at_string = + new_close_at + |> Timex.format!("{ISO:Extended}") + + open_at_string = + open_at + |> Timex.format!("{ISO:Extended}") + + new_dates = %{openAt: open_at_string, closeAt: new_close_at_string} + + conn = + conn + |> post(build_url_update(assessment.id), new_dates) + + assessment = Repo.get(Assessment, assessment.id) + assert response(conn, 400) == "New end date should occur after new opening date" + assert [assessment.open_at, assessment.close_at] == [open_at, close_at] + end + + @tag authenticate: :staff + test "not allowed to set close time to before current time", %{conn: conn} do + open_at = + Timex.now() + |> Timex.beginning_of_day() + |> Timex.shift(days: -3) + |> Timex.shift(hours: 4) + + close_at = Timex.shift(open_at, days: 7) + assessment = insert(:assessment, %{open_at: open_at, close_at: close_at}) + + new_close_at = + Timex.now() + |> Timex.shift(days: -1) + + new_close_at_string = + new_close_at + |> Timex.format!("{ISO:Extended}") + + open_at_string = + open_at + |> Timex.format!("{ISO:Extended}") + + new_dates = %{openAt: open_at_string, closeAt: new_close_at_string} + + conn = + conn + |> post(build_url_update(assessment.id), new_dates) + + assessment = Repo.get(Assessment, assessment.id) + assert response(conn, 400) == "New end date should occur after current time" + assert [assessment.open_at, assessment.close_at] == [open_at, close_at] + end + + @tag authenticate: :staff + test "not allowed to set open time to before current time", %{conn: conn} do + open_at = + Timex.now() + |> Timex.beginning_of_day() + |> Timex.shift(days: 3) + |> Timex.shift(hours: 4) + + close_at = Timex.shift(open_at, days: 7) + assessment = insert(:assessment, %{open_at: open_at, close_at: close_at}) + + new_open_at = + Timex.now() + |> Timex.shift(days: -1) + + new_open_at_string = + new_open_at + |> Timex.format!("{ISO:Extended}") + + close_at_string = + close_at + |> Timex.format!("{ISO:Extended}") + + new_dates = %{openAt: new_open_at_string, closeAt: close_at_string} + + conn = + conn + |> post(build_url_update(assessment.id), new_dates) + + assessment = Repo.get(Assessment, assessment.id) + assert response(conn, 400) == "New Opening date should occur after current time" + assert [assessment.open_at, assessment.close_at] == [open_at, close_at] + end + end + defp build_url, do: "/v1/assessments/" defp build_url(assessment_id), do: "/v1/assessments/#{assessment_id}" defp build_url_submit(assessment_id), do: "/v1/assessments/#{assessment_id}/submit" + defp build_url_publish(assessment_id), do: "/v1/assessments/publish/#{assessment_id}" + defp build_url_update(assessment_id), do: "/v1/assessments/update/#{assessment_id}" defp open_at_asc_comparator(x, y), do: Timex.before?(x.open_at, y.open_at) From bb77f2c1abf90f255dca98e76c366b3d47cbb72d Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Thu, 21 May 2020 16:51:51 +0800 Subject: [PATCH 21/22] Allow new open/close date of assessments to be before current time --- lib/cadet/assessments/assessments.ex | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index dea40f283..1ad5e2b68 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -33,12 +33,6 @@ defmodule Cadet.Assessments do Timex.before?(close_at, open_at) -> {:error, {:bad_request, "New end date should occur after new opening date"}} - Timex.before?(close_at, Timex.now()) -> - {:error, {:bad_request, "New end date should occur after current time"}} - - Timex.before?(open_at, Timex.now()) -> - {:error, {:bad_request, "New Opening date should occur after current time"}} - Timex.equal?(previous_open_time, open_at) or Timex.after?(previous_open_time, Timex.now()) -> update_assessment(id, %{close_at: close_at, open_at: open_at}) From 7ab9971df422253956e0afff64ba796daa84a143 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Thu, 21 May 2020 17:00:42 +0800 Subject: [PATCH 22/22] Updated tests when changing assessment opening/closing date --- .../controllers/assessments_controller_test.exs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/cadet_web/controllers/assessments_controller_test.exs b/test/cadet_web/controllers/assessments_controller_test.exs index 852d8f3be..52a53a379 100644 --- a/test/cadet_web/controllers/assessments_controller_test.exs +++ b/test/cadet_web/controllers/assessments_controller_test.exs @@ -1502,7 +1502,7 @@ defmodule CadetWeb.AssessmentsControllerTest do end @tag authenticate: :staff - test "not allowed to set close time to before current time", %{conn: conn} do + test "successful, set close time to before current time", %{conn: conn} do open_at = Timex.now() |> Timex.beginning_of_day() @@ -1531,12 +1531,12 @@ defmodule CadetWeb.AssessmentsControllerTest do |> post(build_url_update(assessment.id), new_dates) assessment = Repo.get(Assessment, assessment.id) - assert response(conn, 400) == "New end date should occur after current time" - assert [assessment.open_at, assessment.close_at] == [open_at, close_at] + assert response(conn, 200) == "OK" + assert [assessment.open_at, assessment.close_at] == [open_at, new_close_at] end @tag authenticate: :staff - test "not allowed to set open time to before current time", %{conn: conn} do + test "successful, set open time to before current time", %{conn: conn} do open_at = Timex.now() |> Timex.beginning_of_day() @@ -1565,8 +1565,8 @@ defmodule CadetWeb.AssessmentsControllerTest do |> post(build_url_update(assessment.id), new_dates) assessment = Repo.get(Assessment, assessment.id) - assert response(conn, 400) == "New Opening date should occur after current time" - assert [assessment.open_at, assessment.close_at] == [open_at, close_at] + assert response(conn, 200) == "OK" + assert [assessment.open_at, assessment.close_at] == [new_open_at, close_at] end end