[go: nahoru, domu]
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix DB crash due to timeout #1031

Merged
merged 9 commits into from
Nov 7, 2023
Merged

Fix DB crash due to timeout #1031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ea43351
Create index on assessment ID in submissions table
RichDom2185 Nov 6, 2023
a4aed5a
Create separate view layer for grading summary
RichDom2185 Nov 6, 2023
ab9ac01
Move views under AdminGradingView
RichDom2185 Nov 6, 2023
97890a6
Migrate to more optimised query
RichDom2185 Nov 6, 2023
26c74ed
Fix typing
RichDom2185 Nov 6, 2023
69040ac
Restore group filtering functionality
RichDom2185 Nov 6, 2023
af5c9e1
Update Avenger backlog notification workers
RichDom2185 Nov 6, 2023
7646767
Remove old query
RichDom2185 Nov 6, 2023
b51a598
Remove comment
RichDom2185 Nov 6, 2023
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
180 changes: 86 additions & 94 deletions lib/cadet/assessments/assessments.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1238,114 +1238,106 @@ defmodule Cadet.Assessments do
{:forbidden, "Forbidden."}}
"""
@spec all_submissions_by_grader_for_index(CourseRegistration.t()) ::
{:ok, String.t()}
{:ok, %{:assessments => [any()], :submissions => [any()], :users => [any()]}}
def all_submissions_by_grader_for_index(
grader = %CourseRegistration{course_id: course_id},
group_only \\ false,
ungraded_only \\ false
_ungraded_only \\ false
) do
show_all = not group_only

group_where =
group_filter =
if show_all,
do: "",
else:
"where s.student_id in (select cr.id from course_registrations cr inner join groups g on cr.group_id = g.id where g.leader_id = $2) or s.student_id = $2"
"AND s.student_id IN (SELECT cr.id FROM course_registrations AS cr INNER JOIN groups AS g ON cr.group_id = g.id WHERE g.leader_id = #{grader.id}) OR s.student_id = #{grader.id}"

ungraded_where =
if ungraded_only,
do: "where s.\"gradedCount\" < assts.\"questionCount\"",
else: ""

params = if show_all, do: [course_id], else: [course_id, grader.id]
# TODO: Restore ungraded filtering
# ... or more likely, decouple email logic from this function
# ungraded_where =
# if ungraded_only,
# do: "where s.\"gradedCount\" < assts.\"questionCount\"",
# else: ""

# We bypass Ecto here and use a raw query to generate JSON directly from
# PostgreSQL, because doing it in Elixir/Erlang is too inefficient.

case Repo.query(
"""
select json_agg(q)::TEXT from
(
select
s.id,
s.status,
s."unsubmittedAt",
s.xp,
s."xpAdjustment",
s."xpBonus",
s."gradedCount",
assts.jsn as assessment,
students.jsn as student,
unsubmitters.jsn as "unsubmittedBy"
from
(select
s.id,
s.student_id,
s.assessment_id,
s.status,
s.unsubmitted_at as "unsubmittedAt",
s.unsubmitted_by_id,
sum(ans.xp) as xp,
sum(ans.xp_adjustment) as "xpAdjustment",
s.xp_bonus as "xpBonus",
count(ans.id) filter (where ans.grader_id is not null) as "gradedCount"
from submissions s
left join
answers ans on s.id = ans.submission_id
#{group_where}
group by s.id) s
inner join
(select
a.id, a."questionCount", to_json(a) as jsn
from
(select
a.id,
a.title,
a.number as "assessmentNumber",
bool_or(ac.is_manually_graded) as "isManuallyGraded",
max(ac.type) as "type",
sum(q.max_xp) as "maxXp",
count(q.id) as "questionCount"
from assessments a
left join
questions q on a.id = q.assessment_id
inner join
assessment_configs ac on ac.id = a.config_id
where a.course_id = $1
group by a.id) a) assts on assts.id = s.assessment_id
inner join
(select
cr.id, to_json(cr) as jsn
from
(select
cr.id,
u.name as "name",
u.username as "username",
g.name as "groupName",
g.leader_id as "groupLeaderId"
from course_registrations cr
left join
groups g on g.id = cr.group_id
inner join
users u on u.id = cr.user_id) cr) students on students.id = s.student_id
left join
(select
cr.id, to_json(cr) as jsn
from
(select
cr.id,
u.name
from course_registrations cr
inner join
users u on u.id = cr.user_id) cr) unsubmitters on s.unsubmitted_by_id = unsubmitters.id
#{ungraded_where}
) q
""",
params
) do
{:ok, %{rows: [[nil]]}} -> {:ok, "[]"}
{:ok, %{rows: [[json]]}} -> {:ok, json}
end
submissions =
case Repo.query("""
SELECT
s.id,
s.status,
s.unsubmitted_at,
s.unsubmitted_by_id,
s_ans.xp,
s_ans.xp_adjustment,
s.xp_bonus,
s_ans.graded_count,
s.student_id,
s.assessment_id
FROM
submissions AS s
LEFT JOIN (
SELECT
ans.submission_id,
SUM(ans.xp) AS xp,
SUM(ans.xp_adjustment) AS xp_adjustment,
COUNT(ans.id) FILTER (
WHERE
ans.grader_id IS NOT NULL
) AS graded_count
FROM
answers AS ans
GROUP BY
ans.submission_id
) AS s_ans ON s_ans.submission_id = s.id
WHERE
s.assessment_id IN (
SELECT
id
FROM
assessments
WHERE
assessments.course_id = #{course_id}
) #{group_filter};
""") do
{:ok, %{columns: columns, rows: result}} ->
result
|> Enum.map(
&(columns
|> Enum.map(fn c -> String.to_atom(c) end)
|> Enum.zip(&1)
|> Enum.into(%{}))
)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we put a default case here? in case of error

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't think it was necessary as I don't think the SQL will ever error (unless the DB server itself is down, in which case it would give the same error messages as the ones we have been having recently).

What do you think?

end

{:ok, generate_grading_summary_view_model(submissions, course_id)}
end

defp generate_grading_summary_view_model(submissions, course_id) do
users =
CourseRegistration
|> where([cr], cr.course_id == ^course_id)
|> join(:inner, [cr], u in assoc(cr, :user))
|> join(:left, [cr, u], g in assoc(cr, :group))
|> preload([cr, u, g], user: u, group: g)
|> Repo.all()

assessment_ids = submissions |> Enum.map(& &1.assessment_id) |> Enum.uniq()

assessments =
Assessment
|> where([a], a.id in ^assessment_ids)
|> join(:left, [a], q in assoc(a, :questions))
|> join(:inner, [a], ac in assoc(a, :config))
|> preload([a, q, ac], questions: q, config: ac)
|> Repo.all()

%{
users: users,
assessments: assessments,
submissions: submissions
}
end

@spec get_answers_in_submission(integer() | String.t()) ::
Expand Down
11 changes: 3 additions & 8 deletions lib/cadet/workers/NotificationWorker.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,10 @@ defmodule Cadet.Workers.NotificationWorker do
for avenger_cr <- avengers_crs do
avenger = Cadet.Accounts.get_user(avenger_cr.user_id)

ungraded_submissions =
Jason.decode!(
elem(
Cadet.Assessments.all_submissions_by_grader_for_index(avenger_cr, true, true),
1
)
)
{:ok, %{submissions: ungraded_submissions}} =
Cadet.Assessments.all_submissions_by_grader_for_index(avenger_cr, true, true)

if length(ungraded_submissions) < ungraded_threshold do
if Enum.count(ungraded_submissions) < ungraded_threshold do
IO.puts("[AVENGER_BACKLOG] below threshold!")
else
IO.puts("[AVENGER_BACKLOG] SENDING_OUT")
Expand Down
4 changes: 2 additions & 2 deletions lib/cadet_web/admin_controllers/admin_grading_controller.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ defmodule CadetWeb.AdminGradingController do
group = String.to_atom(group)

case Assessments.all_submissions_by_grader_for_index(course_reg, group) do
{:ok, submissions} ->
{:ok, view_model} ->
conn
|> put_status(:ok)
|> put_resp_content_type("application/json")
|> text(submissions)
|> render("gradingsummaries.json", view_model)
end
end

Expand Down
88 changes: 88 additions & 0 deletions lib/cadet_web/admin_views/admin_grading_view.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,94 @@ defmodule CadetWeb.AdminGradingView do
render_many(answers, CadetWeb.AdminGradingView, "grading_info.json", as: :answer)
end

def render("gradingsummaries.json", %{
users: users,
assessments: assessments,
submissions: submissions
}) do
for submission <- submissions do
user = users |> Enum.find(&(&1.id == submission.student_id))
assessment = assessments |> Enum.find(&(&1.id == submission.assessment_id))

render(
CadetWeb.AdminGradingView,
"gradingsummary.json",
%{
user: user,
assessment: assessment,
submission: submission,
unsubmitter:
case submission.unsubmitted_by_id do
nil -> nil
_ -> users |> Enum.find(&(&1.id == submission.unsubmitted_by_id))
end
}
)
end
end

def render("gradingsummary.json", %{
user: user,
assessment: a,
submission: s,
unsubmitter: unsubmitter
}) do
s
|> transform_map_for_view(%{
id: :id,
status: :status,
unsubmittedAt: :unsubmitted_at,
xp: :xp,
xpAdjustment: :xp_adjustment,
xpBonus: :xp_bonus,
gradedCount:
&case &1.graded_count do
nil -> 0
x -> x
end
})
|> Map.merge(%{
assessment:
render_one(a, CadetWeb.AdminGradingView, "gradingsummaryassessment.json", as: :assessment),
student: render_one(user, CadetWeb.AdminGradingView, "gradingsummaryuser.json", as: :cr),
unsubmittedBy:
case unsubmitter do
nil -> nil
cr -> transform_map_for_view(cr, %{id: :id, name: & &1.user.name})
end
})
end

def render("gradingsummaryassessment.json", %{assessment: a}) do
%{
id: a.id,
title: a.title,
assessmentNumber: a.number,
isManuallyGraded: a.config.is_manually_graded,
type: a.config.type,
maxXp: a.questions |> Enum.map(& &1.max_xp) |> Enum.sum(),
questionCount: a.questions |> Enum.count()
}
end

def render("gradingsummaryuser.json", %{cr: cr}) do
%{
id: cr.id,
name: cr.user.name,
username: cr.user.username,
groupName:
case cr.group do
nil -> nil
_ -> cr.group.name
end,
groupLeaderId:
case cr.group do
nil -> nil
_ -> cr.group.leader_id
end
}
end

def render("grading_info.json", %{answer: answer}) do
transform_map_for_view(answer, %{
student:
Expand Down
11 changes: 11 additions & 0 deletions priv/repo/migrations/20231105164101_create_submissions_assessment_index.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
defmodule Cadet.Repo.Migrations.CreateSubmissionsAssessmentIndex do
use Ecto.Migration

def up do
create(index(:submissions, [:assessment_id]))
end

def down do
drop(index(:submissions, [:assessment_id]))
end
end
6 changes: 2 additions & 4 deletions test/cadet/email_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,8 @@ defmodule Cadet.EmailTest do
avenger_user = insert(:user, %{email: "test@gmail.com"})
avenger = insert(:course_registration, %{user: avenger_user, role: :staff})

ungraded_submissions =
Jason.decode!(
elem(Cadet.Assessments.all_submissions_by_grader_for_index(avenger, true, true), 1)
)
{:ok, %{submissions: ungraded_submissions}} =
Cadet.Assessments.all_submissions_by_grader_for_index(avenger, true, true)

email = Email.avenger_backlog_email("avenger_backlog", avenger_user, ungraded_submissions)

Expand Down
6 changes: 2 additions & 4 deletions test/cadet/jobs/notification_worker/notification_worker_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,13 @@
avenger_cr = assessments.course_regs.avenger1_cr

# setup for assessment submission
asssub_ntype = Cadet.Notifications.get_notification_type_by_name!("ASSESSMENT SUBMISSION")

Check warning on line 16 in test/cadet/jobs/notification_worker/notification_worker_test.exs

View workflow job for this annotation

GitHub Actions / Run CI 

variable "asssub_ntype" is unused (if the variable is not meant to be used, prefix it with an underscore)
{_name, data} = Enum.at(assessments.assessments, 0)
submission = List.first(List.first(data.mcq_answers)).submission

# setup for avenger backlog
ungraded_submissions =
Jason.decode!(
elem(Cadet.Assessments.all_submissions_by_grader_for_index(avenger_cr, true, true), 1)
)
{:ok, %{submissions: ungraded_submissions}} =
Cadet.Assessments.all_submissions_by_grader_for_index(avenger_cr, true, true)

Repo.update_all(NotificationType, set: [is_enabled: true])
Repo.update_all(NotificationConfig, set: [is_enabled: true])
Expand Down
Loading