diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 80413d551..1ad5e2b68 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 @@ -16,11 +16,73 @@ 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.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 + else + {:error, {:forbidden, "User is not permitted to edit"}} + end + end + + def toggle_publish_assessment(_publisher = %User{role: role}, id) do + if role in @publish_assessment_role do + assessment = Repo.get(Assessment, id) + update_assessment(id, %{is_published: !assessment.is_published}) + 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) + + Submission + |> where(assessment_id: ^id) + |> delete_submission_assocation(id) + + Repo.delete(assessment) + else + {:error, {:forbidden, "User is not permitted to delete"}} + 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 = @@ -169,11 +231,19 @@ 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) @@ -216,7 +286,7 @@ defmodule Cadet.Assessments do 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 +316,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 +335,15 @@ 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,33 +368,101 @@ 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 invalid_force_update(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) + ) + + # 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) + |> build_question_changeset_for_assessment_id(id) + |> Repo.insert() + else + params = + question_params + |> Map.put_new(:max_xp, 0) + |> Map.put(:display_order, index) + + %{id: question_id, type: type} = + Question + |> where([q], q.display_order == ^index and q.assessment_id == ^id) + |> Repo.one() + + 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) - 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 + # 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 + + 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 = + 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 + + @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() @@ -324,18 +471,30 @@ defmodule Cadet.Assessments do 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) + |> Map.delete(:is_published) + + 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..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)) @@ -77,14 +81,16 @@ 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 +98,53 @@ 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, error_message} -> + log_and_return_badrequest(error_message) {: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 info is stored in multiple nested tuples + 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 - @spec process_assessment(String.t()) :: {:ok, map()} | :error + 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, 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 +152,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 +160,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 +172,11 @@ 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 +196,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 +220,16 @@ defmodule Cadet.Updater.XMLParser do question else {:no_missing_attr?, false} -> - Logger.error("Missing attribute(s) on PROBLEM") - :error + {:error, "Missing attribute(s) on PROBLEM"} - :error -> - :error + {:error, error_message} -> + {:error, error_message} 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 +242,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 +250,8 @@ defmodule Cadet.Updater.XMLParser do question_map when is_map(question_map) -> Map.put(question, :question, question_map) - :error -> - :error + {:error, error_message} -> + {:error, error_message} end end @@ -288,11 +302,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 +317,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 +362,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..1c92b355b 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -4,6 +4,9 @@ defmodule CadetWeb.AssessmentsController do use PhoenixSwagger 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 @@ -19,7 +22,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 +37,98 @@ 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 + text(conn, "Force Update OK") + else + text(conn, "OK") + end + + {:ok, warning_message} -> + text(conn, warning_message) + + {:error, {status, message}} -> + conn + |> put_status(status) + |> text(message) + end + else + conn + |> put_status(:forbidden) + |> text("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} -> + text(conn, "OK") + + {:error, {status, message}} -> + conn + |> put_status(status) + |> text(message) + end + end + + def publish(conn, %{"assessmentid" => assessment_id}) do + result = + Assessments.toggle_publish_assessment( + conn.assigns.current_user, + assessment_id + ) + + case result do + {:ok, _nil} -> + text(conn, "OK") + + {:error, {status, message}} -> + conn + |> put_status(status) + |> text(message) + end + end + + 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} -> + text(conn, "OK") + + {:error, {status, message}} -> + conn + |> put_status(status) + |> text(message) + end + end + swagger_path :submit do post("/assessments/{assessmentId}/submit") summary("Finalise submission for an assessment") @@ -83,6 +178,75 @@ 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: []}]) + + consumes("application/json") + + 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/:assessmentid") + + 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/:assessmentid") + + summary("Toggles an assessment between published and unpublished") + + security([%{JWT: []}]) + + parameters do + assessmentId(:path, :integer, "assessment id", required: true) + end + + response(200, "OK") + response(403, "User is not permitted to publish") + end + + swagger_path :update do + 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) + 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: @@ -141,6 +305,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: diff --git a/lib/cadet_web/router.ex b/lib/cadet_web/router.ex index c79ba7e46..18b5a75f1 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/:assessmentid", AssessmentsController, :delete) post("/assessments/:id", AssessmentsController, :show) + 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) 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 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 diff --git a/test/cadet_web/controllers/assessments_controller_test.exs b/test/cadet_web/controllers/assessments_controller_test.exs index a9cd2e7b1..52a53a379 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 @@ -64,7 +74,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 +91,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 +101,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 +230,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 +568,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 +648,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 +715,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 @@ -1121,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 "successful, 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, 200) == "OK" + assert [assessment.open_at, assessment.close_at] == [open_at, new_close_at] + end + + @tag authenticate: :staff + test "successful, 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, 200) == "OK" + assert [assessment.open_at, assessment.close_at] == [new_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)