Add grading controller #101
Conversation
Pull Request Test Coverage Report for Build 844
💛 - Coveralls |
indocomsoft
force-pushed
the
add-grading-controller
branch
2 times, most recently
from
6c29b6e to
56d3e90
Compare
indocomsoft
force-pushed
the
add-grading-controller
branch
2 times, most recently
from
4ef1062 to
9c12d80
Compare
lib/cadet/accounts/query.ex
Outdated
| @spec students_of(User.t()) :: User.t() | ||
| def students_of(%User{id: id, role: :staff}) do | ||
| User | ||
| |> join(:inner, [u], g in Group, u.group_id == g.id and g.leader_id == ^id) |
There was a problem hiding this comment.
I think it's clearer to move the leader_id check into a separate line. Cursory googling suggest both will be optimised to the same query.
User
|> join(:inner, [u], g in assoc(u, :group)
|> where([_, g], g.leader_id == ^id)
There was a problem hiding this comment.
Yeah they both are optimised to the same query
lib/cadet/assessments/assessments.ex
Outdated
| s.assessment_id == a.id | ||
| ) | ||
| |> select([s, x, st, a], %Submission{s | xp: x.xp, student: st, assessment: a}) | ||
| |> join(:inner, [s], t in subquery(students), s.student_id == t.id) |
There was a problem hiding this comment.
Can replace join(:inner, [s], st in assoc(s, :student)) with this directly.
lib/cadet/assessments/assessments.ex
Outdated
| |> join(:inner, [a], s in Submission, a.submission_id == s.id) | ||
| |> join(:inner, [a, s], t in subquery(students), t.id == s.student_id) | ||
| |> join(:inner, [a], q in assoc(a, :question)) | ||
| |> preload([a, _, _, q], question: q) |
There was a problem hiding this comment.
Use preload([a, ..., q], question: q) instead
There was a problem hiding this comment.
Ooh, I didn't know about this syntax. Cool!
lib/cadet/assessments/assessments.ex
Outdated
| answer = | ||
| Answer | ||
| |> where([a], a.submission_id == ^submission_id and a.question_id == ^question_id) | ||
| |> join(:inner, [a], s in Submission, a.submission_id == s.id) |
There was a problem hiding this comment.
I prefer assoc joins to be written with the asssoc syntax: (join(:inner, [a], s in assoc(a, :submission))) but I get it also causes some inconsistency in syntax because it doesn't work with subqueries. What are your thoughts?
There was a problem hiding this comment.
Yeah I think I prefer the assoc syntax too. It's an oversight on my part.
lib/cadet/assessments/assessments.ex
Outdated
| |> join(:inner, [a, s], t in subquery(students), t.id == s.student_id) | ||
| |> Repo.one() | ||
|
|
||
| if answer do |
There was a problem hiding this comment.
Could consider replacing this block with:
with {:answer_found?, answer = %Answer{}} <- {:answer_found?, answer},
# or with {:answer_found?, true} <- {:answer_found?, is_map(answer)},
{:valid, changeset = %Ecto.Changeset{valid?: true}} <-
{:valid, Answer.grading_changeset(answer, attrs)},
{:ok, _} <- Repo.update(changeset) do
{:ok, nil}
else
{:answer_found?, _} ->
{:error, {:bad_request, "Answer not found or user not permitted to grade."}}
{:valid, changeset} ->
{:error, {:bad_request, full_error_messages(changeset.errors)}}
{:error, _} ->
{:error, {:internal_server_error, "Please try again later."}}
end
| %{ | ||
| content: sequence("ProgrammingQuestion"), | ||
| solution_template: "f => f(f);", | ||
| solution: "(f => f(f))(f => f(f));" |
| end | ||
| end | ||
|
|
||
| defmacro is_ecto_id(id) do |
lib/cadet/helpers/display_helper.ex
Outdated
|
|
||
| @spec full_error_messages(keyword({String.t(), term()})) :: String.t() | ||
| def full_error_messages(errors) do | ||
| for {key, {message, _}} <- errors do |
There was a problem hiding this comment.
I personally prefer:
to_error_string = fn {key, {message, _}} -> "#{key} #{message}" end
errors
|> Enum.map(to_error_string)
|> Enum.join(", ")
There was a problem hiding this comment.
I think in this case Enum makes more sense than list comprehension.
|
@tuesmiddt Fixed my code according to your code review. I also re-wrote seeds.exs and |
* Add factories and seeds * Update api spec according to changes from Vignesh * Implement index * Hotfix tests * Implement show sans test * Add tests * Update context and model in preparation of implementing update * Fix duplicate factory * Implement update sans test * Add typespec and is_ecto_id macro guard clause * Add and refactor tests * Fix according to code review * Change to list comprehension for readability
Fixes #67