Leaderboard #1238
Leaderboard #1238
Conversation
…rd sql query for all users in course
…ny to display in leaderboard)
… into leaderboard
…tests. Updated leaderboard fetching and exporting for assessment workspace leaderboard. Added Leaderboard Dropdown contests fetching.
…voting XML and added dispatch endpoint for XP customisation
…values. Refactor Score calculation logic to reset to 0 before calculating.
…bility. Uncommented leaderboard portions after finalising testing
…ew helpers to include rank
There was a problem hiding this comment.
Thanks for this! Made quite a few comments, do look through them. One issue I have is the lack of pagination, sending over 1000 students in a json for each request seems expensive to me. Also would it be possible to implement some tests? Some errors I caught would've been caught if there were tests.
| alias Cadet.Assessments | ||
| alias Cadet.Assessments.{Question, Assessment} | ||
| alias Cadet.{Assessments, Repo} |
There was a problem hiding this comment.
Not sure if you need to alias Assessments again
| @@ -67,6 +71,44 @@ defmodule CadetWeb.AssessmentsController do | |||
| end | |||
| end | |||
|
|
|||
| def combined_total_xp_for_all_users(conn, %{"course_id" => course_id}) do | |||
There was a problem hiding this comment.
Is there a reason why we require all users? Could we possible paginate this. It feels like it would be an expensive query.
| add(:enable_overall_leaderboard, :boolean, null: false, default: true) | ||
| add(:enable_contest_leaderboard, :boolean, null: false, default: true) | ||
| add(:top_leaderboard_display, :integer, default: 100) | ||
| add(:top_contest_leaderboard_display, :integer, default: 10) |
There was a problem hiding this comment.
You should be creating a new migration file with today's date
| @@ -25,7 +26,8 @@ defmodule Cadet.Assessments do | |||
| alias Cadet.Jobs.Log | |||
| alias Cadet.ProgramAnalysis.Lexer | |||
| alias Ecto.Multi | |||
| alias Cadet.Incentives.Achievements | |||
| alias Cadet.Incentives.{Achievements, AchievementToGoal} | |||
| alias Ecto.Changeset | |||
There was a problem hiding this comment.
Could you combine the aliases
| having: | ||
| fragment( | ||
| "bool_and(?)", | ||
| p.completed and p.count == g.target_count and not is_nil(p.course_reg_id) |
There was a problem hiding this comment.
Correct me if I'm wrong, course_reg_id has a not null constraint so is_nil is redundent.
The rest of them aren't aggregated columns, I think you can put it into the where clause
| @@ -1591,7 +1724,8 @@ defmodule Cadet.Assessments do | |||
| defp load_contest_voting_entries( | |||
| questions, | |||
| %CourseRegistration{role: role, course_id: course_id, id: voter_id}, | |||
| assessment | |||
| assessment, | |||
| visibleEntries | |||
There was a problem hiding this comment.
This should be snake cased
lib/cadet/assessments/assessments.ex
Outdated
| # |> where( | ||
| # [a], | ||
| # fragment( | ||
| # "?->>'code' like ?", | ||
| # a.answer, | ||
| # "%return%" | ||
| # ) | ||
| # ) |
There was a problem hiding this comment.
Remove uncommented lines
lib/cadet/assessments/assessments.ex
Outdated
| |> join(:left, [a], s in assoc(a, :submission)) | ||
| |> join(:left, [a, s], student in assoc(s, :student)) | ||
| |> join(:inner, [a, s, student], student_user in assoc(student, :user)) | ||
| # |> where([a, s, student], student.role == "student") |
There was a problem hiding this comment.
Remove uncommented lines
| contest_id = | ||
| Question | ||
| |> where(assessment_id: ^assessment_id) | ||
| |> Repo.one() |
There was a problem hiding this comment.
I know contests are supposed to have one question, but can we handle the error raised in case for some reason there was more than one question? i.e. returning a proper error message.
| |> select([a, s, student, student_user], %{ | ||
| submission_id: a.submission_id, | ||
| code: a.answer["code"], | ||
| score: a.relative_score, | ||
| name: student_user.name, | ||
| username: student_user.username | ||
| }) | ||
|
|
||
| query = | ||
| from(sub in subquery(subquery), | ||
| select_merge: %{ | ||
| rank: fragment("RANK() OVER (ORDER BY ? DESC)", sub.score) | ||
| } | ||
| ) | ||
|
|
||
| Repo.all(query) |
There was a problem hiding this comment.
Is it not possible to avoid the subquery by doing the following? Please do check the correctness of my suggestion :)
select([a, s, student, user], %{
submission_id: a.submission_id,
code: a.answer["code"],
score: a.relative_score,
name: user.name,
username: user.username,
rank: fragment("RANK() OVER (ORDER BY ? DESC)", a.relative_score)
})
Description
Type of change
How to test
Checklist