Update /assessments endpoint (#729)

* `/assessments`: split out separate `/admin` endpoint

* Consistently use `/assessments/:assessmentid`

* Merge `/admin/assessments/:assessmentid/{update,publish}` endpoints

* Quick swagger docs fix for `AdminAssessmentsController`

* Format code via `mix format`

* Formatting fix

* `/assessments/{assessmentid}/submit`: shift role validation to controller

* Add more guards

* `/assessments/question/{questionid}/submit`: shift role validation to AnswerController

* clean up some docs

* Modify `AnswerController#submit` path

* Assessments: remove unused definitions

* Remove leftover unused route

* Split `AssesmentsController#unlock` out of `AssessmentsController#show`

`AssesmentsController#unlock` unlocks password-protected assessments and
creates a Submission for them, following which the assessments do not
require requests with the password.

* Fix formatting issues

* `/assessments`: split out separate `/admin` endpoint

* Consistently use `/assessments/:assessmentid`

* Merge `/admin/assessments/:assessmentid/{update,publish}` endpoints

* Quick swagger docs fix for `AdminAssessmentsController`

* Format code via `mix format`

* `/assessments/{assessmentid}/submit`: shift role validation to controller

* Add more guards

* `/assessments/question/{questionid}/submit`: shift role validation to AnswerController

* clean up some docs

* Modify `AnswerController#submit` path

* Assessments: remove unused definitions

* Remove leftover unused route

* Split `AssesmentsController#unlock` out of `AssessmentsController#show`

`AssesmentsController#unlock` unlocks password-protected assessments and
creates a Submission for them, following which the assessments do not
require requests with the password.

* Fix formatting issues

* formatting

* Update per PR feedback

* Remove unused variable

Co-authored-by: qreoct <9080974+qreoct@users.noreply.github.com>

Author: bnjmnt4n

lib/cadet/assessments/assessments.ex

@@ -17,32 +17,12 @@ defmodule Cadet.Assessments do
 
   @xp_early_submission_max_bonus 100
   @xp_bonus_assessment_type ~w(mission sidequest)
-  # @submit_answer_roles ~w(student staff admin)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
-  # @see_all_submissions_roles ~w(staff admin)a
-  # @group_grading_summary_roles @see_all_submissions_roles
   @open_all_assessment_roles ~w(staff admin)a
 
   # These roles can save and finalise answers for closed assessments and
   # submitted answers
   @bypass_closed_roles ~w(staff admin)a
 
-  def change_dates_assessment(id, close_at, open_at) do
-    if Timex.before?(close_at, open_at) do
-      {:error, {:bad_request, "New end date should occur after new opening date"}}
-    else
-      update_assessment(id, %{close_at: close_at, open_at: open_at})
-    end
-  end
-
-  def toggle_publish_assessment(id) do
-    assessment = Repo.get(Assessment, id)
-    update_assessment(id, %{is_published: !assessment.is_published})
-  end
-
   def delete_assessment(id) do
     assessment = Repo.get(Assessment, id)
 
@@ -487,7 +467,7 @@ defmodule Cadet.Assessments do
     )
   end
 
-  def publish_assessment(id) do
+  def publish_assessment(id) when is_ecto_id(id) do
     update_assessment(id, %{is_published: true})
   end
 
@@ -511,6 +491,14 @@ defmodule Cadet.Assessments do
     end
   end
 
+  def get_question(id) when is_ecto_id(id) do
+    Question
+    |> where(id: ^id)
+    |> join(:inner, [q], assessment in assoc(q, :assessment))
+    |> preload([_, a], assessment: a)
+    |> Repo.one()
+  end
+
   def delete_question(id) when is_ecto_id(id) do
     question = Repo.get(Question, id)
     Repo.delete(question)
@@ -525,53 +513,32 @@ defmodule Cadet.Assessments do
    `{:bad_request, "Missing or invalid parameter(s)"}`
 
   """
-  def answer_question(id, user = %User{role: role}, raw_answer) when is_ecto_id(id) do
-    # if role in @submit_answer_roles do
-    question =
-      Question
-      |> where(id: ^id)
-      |> join(:inner, [q], assessment in assoc(q, :assessment))
-      |> preload([_, a], assessment: a)
-      |> Repo.one()
-
-    bypass = role in @bypass_closed_roles
-
-    with {:question_found?, true} <- {:question_found?, is_map(question)},
-         {:is_open?, true} <- {:is_open?, bypass or is_open?(question.assessment)},
-         {:ok, submission} <- find_or_create_submission(user, question.assessment),
-         {:status, true} <- {:status, bypass or submission.status != :submitted},
+  def answer_question(question = %Question{}, user = %User{}, raw_answer, force_submit) do
+    with {:ok, submission} <- find_or_create_submission(user, question.assessment),
+         {:status, true} <- {:status, force_submit or submission.status != :submitted},
          {:ok, _answer} <- insert_or_update_answer(submission, question, raw_answer) do
       update_submission_status(submission, question.assessment)
 
       {:ok, nil}
     else
-      {:question_found?, false} -> {:error, {:not_found, "Question not found"}}
-      {:is_open?, false} -> {:error, {:forbidden, "Assessment not open"}}
       {:status, _} -> {:error, {:forbidden, "Assessment submission already finalised"}}
       {:error, :race_condition} -> {:error, {:internal_server_error, "Please try again later."}}
       _ -> {:error, {:bad_request, "Missing or invalid parameter(s)"}}
     end
-
-    # else
-    #  {:error, {:forbidden, "User is not permitted to answer questions"}}
-    # end
   end
 
-  def finalise_submission(assessment_id, %User{id: user_id, role: role})
+  def get_submission(assessment_id, %User{id: user_id})
       when is_ecto_id(assessment_id) do
-    # if role in @submit_answer_roles do
-    submission =
-      Submission
-      |> where(assessment_id: ^assessment_id)
-      |> where(student_id: ^user_id)
-      |> join(:inner, [s], a in assoc(s, :assessment))
-      |> preload([_, a], assessment: a)
-      |> Repo.one()
+    Submission
+    |> where(assessment_id: ^assessment_id)
+    |> where(student_id: ^user_id)
+    |> join(:inner, [s], a in assoc(s, :assessment))
+    |> preload([_, a], assessment: a)
+    |> Repo.one()
+  end
 
-    with {:submission_found?, true} <- {:submission_found?, is_map(submission)},
-         {:is_open?, true} <-
-           {:is_open?, role in @bypass_closed_roles or is_open?(submission.assessment)},
-         {:status, :attempted} <- {:status, submission.status},
+  def finalise_submission(submission = %Submission{}) do
+    with {:status, :attempted} <- {:status, submission.status},
          {:ok, updated_submission} <- update_submission_status_and_xp_bonus(submission) do
       # TODO: Couple with update_submission_status_and_xp_bonus to ensure notification is sent
       Notifications.write_notification_when_student_submits(submission)
@@ -580,12 +547,6 @@ defmodule Cadet.Assessments do
 
       {:ok, nil}
     else
-      {:submission_found?, false} ->
-        {:error, {:not_found, "Submission not found"}}
-
-      {:is_open?, false} ->
-        {:error, {:forbidden, "Assessment not open"}}
-
       {:status, :attempting} ->
         {:error, {:bad_request, "Some questions have not been attempted"}}
 
@@ -595,10 +556,6 @@ defmodule Cadet.Assessments do
       _ ->
         {:error, {:internal_server_error, "Please try again later."}}
     end
-
-    # else
-    #   {:error, {:forbidden, "User is not permitted to answer questions"}}
-    # end
   end
 
   def unsubmit_submission(submission_id, user = %User{id: user_id, role: role})
@@ -1003,7 +960,7 @@ defmodule Cadet.Assessments do
 
   # Checks if an assessment is open and published.
   @spec is_open?(%Assessment{}) :: boolean()
-  defp is_open?(%Assessment{open_at: open_at, close_at: close_at, is_published: is_published}) do
+  def is_open?(%Assessment{open_at: open_at, close_at: close_at, is_published: is_published}) do
     Timex.between?(Timex.now(), open_at, close_at) and is_published
   end
 
@@ -1016,9 +973,9 @@ defmodule Cadet.Assessments do
           submitted_sidequests: number()
         }
 
-  @spec get_group_grading_summary() ::
+  @spec get_group_grading_summary ::
           {:ok, [group_summary_entry()]}
-  def get_group_grading_summary() do
+  def get_group_grading_summary do
     subs =
       Answer
       |> join(:left, [ans], s in Submission, on: s.id == ans.submission_id)

lib/cadet_web/admin_controllers/admin_assessments_controller.ex

@@ -0,0 +1,147 @@
+defmodule CadetWeb.AdminAssessmentsController do
+  use CadetWeb, :controller
+
+  use PhoenixSwagger
+
+  alias Cadet.Assessments
+  import Cadet.Updater.XMLParser, only: [parse_xml: 2]
+
+  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
+          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
+  end
+
+  def delete(conn, %{"assessmentid" => assessment_id}) do
+    result = Assessments.delete_assessment(assessment_id)
+
+    case result do
+      {:ok, _nil} ->
+        text(conn, "OK")
+
+      {:error, {status, message}} ->
+        conn
+        |> put_status(status)
+        |> text(message)
+    end
+  end
+
+  def update(conn, params = %{"assessmentid" => assessment_id}) when is_ecto_id(assessment_id) do
+    open_at = params |> Map.get("openAt")
+    close_at = params |> Map.get("closeAt")
+    is_published = params |> Map.get("isPublished")
+
+    updated_assessment =
+      if is_published == nil do
+        %{}
+      else
+        %{:is_published => is_published}
+      end
+
+    with {:ok, assessment} <- check_dates(open_at, close_at, updated_assessment),
+         {:ok, _nil} <- Assessments.update_assessment(assessment_id, assessment) do
+      text(conn, "OK")
+    else
+      {:error, {status, message}} ->
+        conn
+        |> put_status(status)
+        |> text(message)
+    end
+  end
+
+  defp check_dates(open_at, close_at, assessment) do
+    if open_at == nil and close_at == nil do
+      {:ok, assessment}
+    else
+      formatted_open_date = elem(DateTime.from_iso8601(open_at), 1)
+      formatted_close_date = elem(DateTime.from_iso8601(close_at), 1)
+
+      if Timex.before?(formatted_close_date, formatted_open_date) do
+        {:error, {:bad_request, "New end date should occur after new opening date"}}
+      else
+        assessment = Map.put(assessment, :open_at, formatted_open_date)
+        assessment = Map.put(assessment, :close_at, formatted_close_date)
+
+        {:ok, assessment}
+      end
+    end
+  end
+
+  swagger_path :create do
+    post("/admin/assessments")
+
+    summary("Creates a new assessment or updates an existing assessment.")
+
+    security([%{JWT: []}])
+
+    consumes("multipart/form-data")
+
+    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, "Forbidden")
+  end
+
+  swagger_path :delete do
+    PhoenixSwagger.Path.delete("/admin/assessments/{assessmentid}")
+
+    summary("Deletes an assessment.")
+
+    security([%{JWT: []}])
+
+    parameters do
+      assessmentId(:path, :integer, "Assessment ID", required: true)
+    end
+
+    response(200, "OK")
+    response(403, "Forbidden")
+  end
+
+  swagger_path :update do
+    post("/admin/assessments/{assessmentid}")
+
+    summary("Updates an assessment.")
+
+    security([%{JWT: []}])
+
+    consumes("application/json")
+
+    parameters do
+      assessmentId(:path, :integer, "Assessment ID", required: true)
+      closeAt(:body, :string, "Open date", required: false)
+      openAt(:body, :string, "Close date", required: false)
+      isPublished(:body, :boolean, "Whether the assessment is published", required: false)
+    end
+
+    response(200, "OK")
+    response(401, "Assessment is already opened")
+    response(403, "Forbidden")
+  end
+end

lib/cadet_web/controllers/answer_controller.ex

@@ -5,11 +5,31 @@ defmodule CadetWeb.AnswerController do
 
   alias Cadet.Assessments
 
+  # These roles can save and finalise answers for closed assessments and
+  # submitted answers
+  @bypass_closed_roles ~w(staff admin)a
+
   def submit(conn, %{"questionid" => question_id, "answer" => answer})
       when is_ecto_id(question_id) do
-    case Assessments.answer_question(question_id, conn.assigns.current_user, answer) do
-      {:ok, _nil} ->
-        text(conn, "OK")
+    user = conn.assigns[:current_user]
+    can_bypass? = user.role in @bypass_closed_roles
+
+    with {:question, question} when not is_nil(question) <-
+           {:question, Assessments.get_question(question_id)},
+         {:is_open?, true} <-
+           {:is_open?, can_bypass? or Assessments.is_open?(question.assessment)},
+         {:ok, _nil} <- Assessments.answer_question(question, user, answer, can_bypass?) do
+      text(conn, "OK")
+    else
+      {:question, nil} ->
+        conn
+        |> put_status(:not_found)
+        |> text("Question not found")
+
+      {:is_open?, false} ->
+        conn
+        |> put_status(:forbidden)
+        |> text("Assessment not open")
 
       {:error, {status, message}} ->
         conn
@@ -18,12 +38,12 @@ defmodule CadetWeb.AnswerController do
     end
   end
 
-  def submit(conn, _parms) do
+  def submit(conn, _params) do
     send_resp(conn, :bad_request, "Missing or invalid parameter(s)")
   end
 
   swagger_path :submit do
-    post("/assessments/question/{questionId}/submit")
+    post("/assessments/question/{questionId}/answer")
 
     summary("Submit an answer to a question.")
 
@@ -43,7 +63,7 @@ defmodule CadetWeb.AnswerController do
     response(200, "OK")
     response(400, "Invalid parameters")
     response(403, "User not permitted to answer questions or assessment not open")
-    response(404, "Assessment not found")
+    response(404, "Question not found")
   end
 
   def swagger_definitions do