Skip to content

Update /assessments endpoint #729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 36 commits into from
Apr 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
d7da86f
`/assessments`: split out separate `/admin` endpoint
bnjmnt4n Feb 23, 2021
01de819
Consistently use `/assessments/:assessmentid`
bnjmnt4n Feb 23, 2021
2b247c9
Merge `/admin/assessments/:assessmentid/{update,publish}` endpoints
bnjmnt4n Feb 23, 2021
956abc2
Quick swagger docs fix for `AdminAssessmentsController`
bnjmnt4n Feb 23, 2021
0f48423
Format code via `mix format`
bnjmnt4n Feb 23, 2021
3e33845
Formatting fix
bnjmnt4n Feb 24, 2021
abd2fa2
`/assessments/{assessmentid}/submit`: shift role validation to contro…
bnjmnt4n Feb 24, 2021
41cd47d
Add more guards
bnjmnt4n Feb 24, 2021
10d5ec1
`/assessments/question/{questionid}/submit`: shift role validation to…
bnjmnt4n Feb 24, 2021
af263bc
clean up some docs
qreoct Feb 25, 2021
6370c36
Modify `AnswerController#submit` path
bnjmnt4n Mar 25, 2021
646c514
Assessments: remove unused definitions
bnjmnt4n Mar 25, 2021
2706c44
Remove leftover unused route
bnjmnt4n Mar 25, 2021
1d2428d
Split `AssesmentsController#unlock` out of `AssessmentsController#show`
bnjmnt4n Mar 26, 2021
18e7d19
Merge remote-tracking branch 'origin/master' into v2
bnjmnt4n Mar 29, 2021
27658cb
Fix formatting issues
bnjmnt4n Mar 29, 2021
21b114e
`/assessments`: split out separate `/admin` endpoint
bnjmnt4n Feb 23, 2021
5428e03
Consistently use `/assessments/:assessmentid`
bnjmnt4n Feb 23, 2021
a99e907
Merge `/admin/assessments/:assessmentid/{update,publish}` endpoints
bnjmnt4n Feb 23, 2021
666df2f
Quick swagger docs fix for `AdminAssessmentsController`
bnjmnt4n Feb 23, 2021
af1bfda
Format code via `mix format`
bnjmnt4n Feb 23, 2021
0802b86
`/assessments/{assessmentid}/submit`: shift role validation to contro…
bnjmnt4n Feb 24, 2021
638e8ed
Add more guards
bnjmnt4n Feb 24, 2021
961df0f
`/assessments/question/{questionid}/submit`: shift role validation to…
bnjmnt4n Feb 24, 2021
0fb4505
clean up some docs
qreoct Feb 25, 2021
09b9255
Modify `AnswerController#submit` path
bnjmnt4n Mar 25, 2021
7057cf2
Assessments: remove unused definitions
bnjmnt4n Mar 25, 2021
7c909a3
Remove leftover unused route
bnjmnt4n Mar 25, 2021
5bf6a0c
Split `AssesmentsController#unlock` out of `AssessmentsController#show`
bnjmnt4n Mar 26, 2021
d9aaeba
Fix formatting issues
bnjmnt4n Mar 29, 2021
991b025
pull master
qreoct Apr 1, 2021
3911707
formatting
qreoct Apr 1, 2021
a6e47b0
Merge branch 'master' of https://github.com/source-academy/cadet into…
qreoct Apr 1, 2021
c18d9e3
Update per PR feedback
bnjmnt4n Apr 8, 2021
748bb57
Merge remote-tracking branch 'origin/v2' into bnjmnt4n/assessments
bnjmnt4n Apr 8, 2021
13a7da6
Remove unused variable
bnjmnt4n Apr 8, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 25 additions & 68 deletions lib/cadet/assessments/assessments.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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

Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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"}}

Expand All @@ -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})
Expand Down Expand Up @@ -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

Expand All @@ -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)
Expand Down
147 changes: 147 additions & 0 deletions lib/cadet_web/admin_controllers/admin_assessments_controller.ex
Original file line number Diff line number Diff line change
@@ -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
Comment on lines +9 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking, forceUpdate is a string because this is submitted as multipart/form-data, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is the case.


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
32 changes: 26 additions & 6 deletions lib/cadet_web/controllers/answer_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.")

Expand All @@ -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
Expand Down
Loading