From 431a0587161a9080c05201232ee3b6e87aab311a Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Mon, 6 Apr 2020 21:22:00 +0800 Subject: [PATCH 01/12] 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 59965c8a9..e39f66572 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 = @@ -163,11 +208,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) @@ -206,11 +258,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() @@ -240,7 +288,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{} -> @@ -259,6 +307,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] -> @@ -283,53 +339,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 1f37b5c3e..be2e2369c 100644 --- a/lib/cadet_web/router.ex +++ b/lib/cadet_web/router.ex @@ -37,7 +37,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 5a865d9c082d394540c52be36b1ce8498a934e5f Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Tue, 7 Apr 2020 21:47:10 +0800 Subject: [PATCH 02/12] 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 e39f66572..c314c6e69 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -258,6 +258,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 c0c73c97744381b2b886ee414d4433a615121542 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Wed, 15 Apr 2020 18:13:17 +0800 Subject: [PATCH 03/12] 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 c314c6e69..ac1082149 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 @@ -311,7 +311,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) @@ -362,16 +362,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 @@ -379,7 +379,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() @@ -399,10 +399,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 @@ -410,7 +410,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() @@ -435,7 +435,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 e3eddd690bf1c1d93c10bcd6ca86686218c219e9 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Wed, 15 Apr 2020 20:50:49 +0800 Subject: [PATCH 04/12] 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 ac1082149..6fc510994 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -354,7 +354,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 @@ -364,6 +364,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) @@ -379,12 +380,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) @@ -392,24 +397,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 @@ -439,6 +451,7 @@ defmodule Cadet.Assessments do params |> Map.delete(:open_at) |> Map.delete(:close_at) + |> Map.delete(:is_published) Assessment.changeset(assessment, new_params) From b39ad61974ff04229da5177c66c4df43b007557b Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Sun, 19 Apr 2020 22:15:26 +0800 Subject: [PATCH 05/12] 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 6fc510994..ea6488a95 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 be2e2369c..5de07ee45 100644 --- a/lib/cadet_web/router.ex +++ b/lib/cadet_web/router.ex @@ -40,7 +40,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 1a077575335a37ca702e2eb7275a5637c69afe97 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Sun, 19 Apr 2020 22:22:55 +0800 Subject: [PATCH 06/12] 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 ea6488a95..7ef40e078 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"}} @@ -209,6 +210,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 @@ -313,6 +315,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 @@ -346,7 +349,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(), @@ -363,7 +370,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 @@ -372,12 +382,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} = @@ -385,9 +395,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 @@ -402,9 +418,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 @@ -418,7 +434,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 @@ -433,6 +450,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 7663bf20453e69b470d2557f6d0d8250ba38a383 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Mon, 20 Apr 2020 02:26:25 +0800 Subject: [PATCH 07/12] 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 159a2d3512875140354f9f2b84fb725ac5a005b7 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Mon, 20 Apr 2020 13:39:55 +0800 Subject: [PATCH 08/12] 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 c7f01c20e9f907ded1b9643f7d719b8e1205c448 Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Mon, 20 Apr 2020 13:48:59 +0800 Subject: [PATCH 09/12] 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 7ef40e078..dbbfe7922 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 5d4e6d86becf73a90da22bc8ea9a441c6a00132f Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Mon, 20 Apr 2020 14:11:52 +0800 Subject: [PATCH 10/12] 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 dbbfe7922..b04b58b26 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -387,16 +387,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 @@ -427,23 +424,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 ae7beac0adc728d5f994b2d3ddc3bb221e73e81b Mon Sep 17 00:00:00 2001 From: ScrubWzz Date: Mon, 20 Apr 2020 18:33:39 +0800 Subject: [PATCH 11/12] 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 6597a3e4c25daae3d281c4a91b6e5cc2d8419c86 Mon Sep 17 00:00:00 2001 From: travisryte Date: Wed, 22 Apr 2020 05:21:32 +0800 Subject: [PATCH 12/12] Ran Mix Format --- lib/cadet/accounts/game_states.ex | 10 +++++----- lib/cadet_web/controllers/user_controller.ex | 14 +++++++++----- lib/cadet_web/router.ex | 2 -- lib/cadet_web/views/user_view.ex | 9 ++++++++- .../migrations/20200410074625_add_game_states.exs | 1 + test/cadet/updater/xml_parser_test.exs | 1 + 6 files changed, 24 insertions(+), 13 deletions(-) diff --git a/lib/cadet/accounts/game_states.ex b/lib/cadet/accounts/game_states.ex index 704444cd4..bec02ded3 100644 --- a/lib/cadet/accounts/game_states.ex +++ b/lib/cadet/accounts/game_states.ex @@ -18,9 +18,7 @@ defmodule Cadet.GameStates do def update(user, new_game_states) do if user.role == :student do - changeset = - Ecto.Changeset.cast(user, %{game_states: - new_game_states},[:game_states]) + changeset = Ecto.Changeset.cast(user, %{game_states: new_game_states}, [:game_states]) Cadet.Repo.update!(changeset) {:ok, nil} else @@ -31,8 +29,10 @@ defmodule Cadet.GameStates do def clear(user) do if user.role == :student do changeset = - Ecto.Changeset.cast(user, %{game_states: %{collectibles: %{}, - completed_quests: []}},[:game_states]) + Ecto.Changeset.cast(user, %{game_states: %{collectibles: %{}, completed_quests: []}}, [ + :game_states + ]) + Cadet.Repo.update!(changeset) {:ok, nil} else diff --git a/lib/cadet_web/controllers/user_controller.ex b/lib/cadet_web/controllers/user_controller.ex index 65760ba74..ad42f5de2 100644 --- a/lib/cadet_web/controllers/user_controller.ex +++ b/lib/cadet_web/controllers/user_controller.ex @@ -16,6 +16,7 @@ defmodule CadetWeb.UserController do story = user_current_story(user) xp = user_total_xp(user) game_states = user_game_states(user) + render( conn, "index.json", @@ -39,6 +40,7 @@ defmodule CadetWeb.UserController do def update_game_states(conn, %{"gameStates" => new_game_states}) do user = conn.assigns[:current_user] + case Cadet.GameStates.update(user, new_game_states) do {:ok, nil} -> text(conn, "OK") @@ -56,9 +58,11 @@ defmodule CadetWeb.UserController do security([%{JWT: []}]) consumes("application/json") produces("application/json") + parameters do new_game_states(:path, :map, "new game states", required: true) end + response(200, "OK", Schema.ref(:UserInfo)) response(201, "Created") response(204, "No Content") @@ -68,9 +72,11 @@ defmodule CadetWeb.UserController do def clear_up_game_states(conn, _) do user = conn.assigns[:current_user] + case Cadet.GameStates.clear(user) do {:ok, nil} -> text(conn, "OK") + {:error, {status, message}} -> conn |> put_status(status) @@ -90,7 +96,6 @@ defmodule CadetWeb.UserController do response(401, "Unauthorised") end - def swagger_definitions do %{ UserInfo: @@ -128,10 +133,9 @@ defmodule CadetWeb.UserController do game_states( :map, - "States for user's game, including users' collectibles and completed quests.\n" - <> "Collectibles is a map.\n" - <> "Completed quests is an array of strings" - + "States for user's game, including users' collectibles and completed quests.\n" <> + "Collectibles is a map.\n" <> + "Completed quests is an array of strings" ) end end, diff --git a/lib/cadet_web/router.ex b/lib/cadet_web/router.ex index 3d3d42cf0..e3698822f 100644 --- a/lib/cadet_web/router.ex +++ b/lib/cadet_web/router.ex @@ -53,12 +53,10 @@ defmodule CadetWeb.Router do get("/notification", NotificationController, :index) post("/notification/acknowledge", NotificationController, :acknowledge) - get("/user", UserController, :index) put("/user/game_states/clear", UserController, :clear_up_game_states) put("/user/game_states/save", UserController, :update_game_states) - post("/chat/token", ChatController, :index) post("/chat/notify", ChatController, :notify) end diff --git a/lib/cadet_web/views/user_view.ex b/lib/cadet_web/views/user_view.ex index 0c4ddfe61..e6839f7db 100644 --- a/lib/cadet_web/views/user_view.ex +++ b/lib/cadet_web/views/user_view.ex @@ -1,7 +1,14 @@ defmodule CadetWeb.UserView do use CadetWeb, :view - def render("index.json", %{user: user, grade: grade, max_grade: max_grade, xp: xp, story: story, game_states: game_states}) do + def render("index.json", %{ + user: user, + grade: grade, + max_grade: max_grade, + xp: xp, + story: story, + game_states: game_states + }) do %{ name: user.name, role: user.role, diff --git a/priv/repo/migrations/20200410074625_add_game_states.exs b/priv/repo/migrations/20200410074625_add_game_states.exs index 182be52ba..5d66a3f82 100644 --- a/priv/repo/migrations/20200410074625_add_game_states.exs +++ b/priv/repo/migrations/20200410074625_add_game_states.exs @@ -1,5 +1,6 @@ defmodule Cadet.Repo.Migrations.AddGameStates do use Ecto.Migration + def change do alter table(:users) do add(:game_states, :map, default: %{collectibles: %{}, completed_quests: []}) diff --git a/test/cadet/updater/xml_parser_test.exs b/test/cadet/updater/xml_parser_test.exs index 2725128ff..f348be93a 100644 --- a/test/cadet/updater/xml_parser_test.exs +++ b/test/cadet/updater/xml_parser_test.exs @@ -9,6 +9,7 @@ defmodule Cadet.Updater.XMLParserTest do import ExUnit.CaptureLog @local_name "test/fixtures/local_repo" + # @locations %{mission: "missions", sidequest: "quests", path: "paths", contest: "contests"} setup do