Refactor leaderboard (#1281)

sayomaki · web-flow · commit 8e5aa2e994c3 · 2025-08-08T09:21:25.000+08:00
* Move leaderboard routes into own controller file

* Fix casing for calculation of contest score

* Add swagger documentation for leaderboard controller

* Standardise naming of routes

* Refactor contest leaderboard controller routes
1. database methods should be within cadet instead of cadet_web
2. simplify processing to only show leaderboard for voting assessments instead of both submission and voting
3. add error message if assessment is not a contest voting one

* Remove imports no longer required

* Standardise naming for contest-related assessment endpoints

* Fix checking if voting_question is nil

* Use case instead of with

* Update tests for changes in endpoint
diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex
@@ -1053,6 +1053,13 @@ defmodule Cadet.Assessments do
     Repo.delete(question)
   end
 
+  def get_contest_voting_question(assessment_id) do
+    Question
+    |> where(type: :voting)
+    |> where(assessment_id: ^assessment_id)
+    |> Repo.one()
+  end
+
   @doc """
   Public internal api to submit new answers for a question. Possible return values are:
   `{:ok, nil}` -> success
diff --git a/lib/cadet_web/admin_controllers/admin_assessments_controller.ex b/lib/cadet_web/admin_controllers/admin_assessments_controller.ex
@@ -144,7 +144,7 @@ defmodule CadetWeb.AdminAssessmentsController do
 
     if voting_questions do
       Assessments.compute_relative_score(voting_questions.id)
-      text(conn, "CONTEST SCORE CALCULATED")
+      text(conn, "Contest scores calculated")
     else
       text(conn, "No voting questions found for the given assessment")
     end
diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex
@@ -3,10 +3,7 @@ defmodule CadetWeb.AssessmentsController do
 
   use PhoenixSwagger
 
-  import Ecto.Query, only: [where: 2]
-
   alias Cadet.{Assessments, Repo}
-  alias Cadet.Assessments.Question
   alias CadetWeb.AssessmentsHelpers
 
   # These roles can save and finalise answers for closed assessments and
@@ -71,96 +68,66 @@ defmodule CadetWeb.AssessmentsController do
     end
   end
 
-  def combined_total_xp_for_all_users(conn, %{"course_id" => course_id}) do
-    users_with_xp = Assessments.all_user_total_xp(course_id)
-    json(conn, %{users: users_with_xp.users})
-  end
-
-  def paginated_total_xp_for_leaderboard_display(conn, %{"course_id" => course_id}) do
-    offset = String.to_integer(conn.params["offset"] || "0")
-    page_size = String.to_integer(conn.params["page_size"] || "25")
-    paginated_display = Assessments.all_user_total_xp(course_id, offset, page_size)
-    json(conn, paginated_display)
-  end
-
-  def get_score_leaderboard(conn, %{
+  def contest_score_leaderboard(conn, %{
         "assessmentid" => assessment_id,
         "course_id" => course_id
       }) do
-    visible_entries = String.to_integer(conn.params["visible_entries"] || "10")
-    voting_id = Assessments.fetch_contest_voting_assesment_id(assessment_id)
-
-    voting_questions =
-      Question
-      |> where(type: :voting)
-      |> where(assessment_id: ^assessment_id)
-      |> Repo.one()
-      |> case do
-        nil ->
-          Question
-          |> where(type: :voting)
-          |> where(assessment_id: ^voting_id)
-          |> Repo.one()
-
-        question ->
-          question
-      end
-
-    contest_id = Assessments.fetch_associated_contest_question_id(course_id, voting_questions)
-
-    result =
-      contest_id
-      |> Assessments.fetch_top_relative_score_answers(visible_entries)
-      |> Enum.map(fn entry ->
-        updated_entry = %{
-          entry
-          | answer: entry.answer["code"]
-        }
-
-        AssessmentsHelpers.build_contest_leaderboard_entry(updated_entry)
-      end)
-
-    json(conn, %{leaderboard: result, voting_id: voting_id})
+    count = String.to_integer(conn.params["count"] || "10")
+
+    case {:voting_question, Assessments.get_contest_voting_question(assessment_id)} do
+      {:voting_question, voting_question} when not is_nil(voting_question) ->
+        question_id = Assessments.fetch_associated_contest_question_id(course_id, voting_question)
+
+        result =
+          question_id
+          |> Assessments.fetch_top_relative_score_answers(count)
+          |> Enum.map(fn entry ->
+            updated_entry = %{
+              entry
+              | answer: entry.answer["code"]
+            }
+
+            AssessmentsHelpers.build_contest_leaderboard_entry(updated_entry)
+          end)
+
+        json(conn, %{leaderboard: result})
+
+      {:voting_question, nil} ->
+        conn
+        |> put_status(:not_found)
+        |> text("Not a contest voting assessment")
+    end
   end
 
-  def get_popular_leaderboard(conn, %{
+  def contest_popular_leaderboard(conn, %{
         "assessmentid" => assessment_id,
         "course_id" => course_id
       }) do
-    visible_entries = String.to_integer(conn.params["visible_entries"] || "10")
-    voting_id = Assessments.fetch_contest_voting_assesment_id(assessment_id)
-
-    voting_questions =
-      Question
-      |> where(type: :voting)
-      |> where(assessment_id: ^assessment_id)
-      |> Repo.one()
-      |> case do
-        nil ->
-          Question
-          |> where(type: :voting)
-          |> where(assessment_id: ^voting_id)
-          |> Repo.one()
-
-        question ->
-          question
-      end
-
-    contest_id = Assessments.fetch_associated_contest_question_id(course_id, voting_questions)
-
-    result =
-      contest_id
-      |> Assessments.fetch_top_popular_score_answers(visible_entries)
-      |> Enum.map(fn entry ->
-        updated_entry = %{
-          entry
-          | answer: entry.answer["code"]
-        }
-
-        AssessmentsHelpers.build_popular_leaderboard_entry(updated_entry)
-      end)
-
-    json(conn, %{leaderboard: result, voting_id: voting_id})
+    count = String.to_integer(conn.params["count"] || "10")
+
+    case {:voting_question, Assessments.get_contest_voting_question(assessment_id)} do
+      {:voting_question, voting_question} when not is_nil(voting_question) ->
+        question_id = Assessments.fetch_associated_contest_question_id(course_id, voting_question)
+
+        result =
+          question_id
+          |> Assessments.fetch_top_popular_score_answers(count)
+          |> Enum.map(fn entry ->
+            updated_entry = %{
+              entry
+              | answer: entry.answer["code"]
+            }
+
+            AssessmentsHelpers.build_popular_leaderboard_entry(updated_entry)
+          end)
+
+        json(conn, %{leaderboard: result})
+
+      {:voting_question, nil} ->
+        conn
+        |> put_status(:not_found)
+        |> text("Not a contest voting assessment")
+    end
   end
 
   def get_all_contests(conn, %{"course_id" => course_id}) do
diff --git a/lib/cadet_web/controllers/leaderboard_controller.ex b/lib/cadet_web/controllers/leaderboard_controller.ex
@@ -0,0 +1,76 @@
+defmodule CadetWeb.LeaderboardController do
+  use CadetWeb, :controller
+
+  use PhoenixSwagger
+
+  alias Cadet.Assessments
+
+  def xp_all(conn, %{"course_id" => course_id}) do
+    users_with_xp = Assessments.all_user_total_xp(course_id)
+    json(conn, %{users: users_with_xp.users})
+  end
+
+  def xp_paginated(conn, %{"course_id" => course_id}) do
+    offset = String.to_integer(conn.params["offset"] || "0")
+    page_size = String.to_integer(conn.params["page_size"] || "25")
+    paginated_display = Assessments.all_user_total_xp(course_id, offset, page_size)
+    json(conn, paginated_display)
+  end
+
+  swagger_path :xp_all do
+    get("/courses/{course_id}/leaderboards/xp_all")
+
+    summary("Get all users XP in course")
+
+    security([%{JWT: []}])
+
+    produces("application/json")
+
+    response(200, "OK", Schema.ref(:XPLeaderboardUsers))
+    response(401, "Unauthorised")
+  end
+
+  swagger_path :xp_paginated do
+    get("/courses/{course_id}/leaderboards/xp")
+
+    summary("Get all users XP in course (paginated)")
+
+    security([%{JWT: []}])
+
+    produces("application/json")
+
+    parameters do
+      offset(:query, :integer, "Pagination offset", required: false, default: 0)
+      page_size(:query, :integer, "Number of users per page", required: false, default: 25)
+    end
+
+    response(200, "OK", Schema.ref(:XPLeaderboardUsers))
+    response(401, "Unauthorised")
+  end
+
+  def swagger_definitions do
+    %{
+      XPLeaderboardUsers:
+        swagger_schema do
+          description("XP Leaderboard Response")
+
+          properties do
+            users(:array, "List of users in the leaderboard",
+              items: %{
+                type: :object,
+                properties: %{
+                  name: %{type: :string, description: "User's full name"},
+                  username: %{type: :string, description: "User's login name"},
+                  rank: %{type: :integer, description: "User's rank"},
+                  user_id: %{type: :integer, description: "User ID"},
+                  total_xp: %{type: :integer, description: "User's total XP"}
+                }
+              }
+            )
+
+            total_count(:integer, "Total number of users in the leaderboard (for paginated)")
+          end
+        end
+    }
+  end
+end
diff --git a/lib/cadet_web/router.ex b/lib/cadet_web/router.ex
@@ -120,24 +120,19 @@ defmodule CadetWeb.Router do
     put("/user/game_states", UserController, :update_game_states)
     put("/user/research_agreement", UserController, :update_research_agreement)
 
-    get("/all_users_xp", AssessmentsController, :combined_total_xp_for_all_users)
+    get("/leaderboards/xp_all", LeaderboardController, :xp_all)
+    get("/leaderboards/xp", LeaderboardController, :xp_paginated)
 
     get(
-      "/get_paginated_display",
+      "/assessments/:assessmentid/contest_popular_leaderboard",
       AssessmentsController,
-      :paginated_total_xp_for_leaderboard_display
+      :contest_popular_leaderboard
     )
 
     get(
-      "/assessments/:assessmentid/popularVoteLeaderboard",
+      "/assessments/:assessmentid/contest_score_leaderboard",
       AssessmentsController,
-      :get_popular_leaderboard
-    )
-
-    get(
-      "/assessments/:assessmentid/scoreLeaderboard",
-      AssessmentsController,
-      :get_score_leaderboard
+      :contest_score_leaderboard
     )
 
     get("/all_contests", AssessmentsController, :get_all_contests)
@@ -204,13 +199,13 @@ defmodule CadetWeb.Router do
     resources("/sourcecast", AdminSourcecastController, only: [:create, :delete])
 
     post(
-      "/assessments/:assessmentid/calculateContestScore",
+      "/assessments/:assessmentid/contest_calculate_score",
       AdminAssessmentsController,
       :calculate_contest_score
     )
 
     post(
-      "/assessments/:assessmentid/dispatchContestXp",
+      "/assessments/:assessmentid/contest_dispatch_xp",
       AdminAssessmentsController,
       :dispatch_contest_xp
     )
diff --git a/test/cadet_web/controllers/assessments_controller_test.exs b/test/cadet_web/controllers/assessments_controller_test.exs
@@ -1800,13 +1800,13 @@ defmodule CadetWeb.AssessmentsControllerTest do
     end
   end
 
-  describe "GET /:assessment_id/popularVoteLeaderboard, unauthenticated" do
+  describe "GET /:assessment_id/contest_popular_leaderboard, unauthenticated" do
     test "unauthorized", %{conn: conn, courses: %{course1: course1}} do
       config = insert(:assessment_config, %{course: course1})
       assessment = insert(:assessment, %{course: course1, config: config})
 
       params = %{
-        "visible_entries" => 9
+        "count" => 9
       }
 
       conn
@@ -1815,13 +1815,13 @@ defmodule CadetWeb.AssessmentsControllerTest do
     end
   end
 
-  describe "GET /:assessment_id/scoreLeaderboard, unauthenticated" do
+  describe "GET /:assessment_id/contest_score_leaderboard, unauthenticated" do
     test "unauthorized", %{conn: conn, courses: %{course1: course1}} do
       config = insert(:assessment_config, %{course: course1})
       assessment = insert(:assessment, %{course: course1, config: config})
 
       params = %{
-        "visible_entries" => 9
+        "count" => 9
       }
 
       conn
@@ -1830,7 +1830,7 @@ defmodule CadetWeb.AssessmentsControllerTest do
     end
   end
 
-  describe "GET /:assessment_id/popularVoteLeaderboard" do
+  describe "GET /:assessment_id/contest_popular_leaderboard" do
     @tag authenticate: :student
     test "successful", %{conn: conn, courses: %{course1: course1}} do
       user = conn.assigns[:current_user]
@@ -1882,7 +1882,7 @@ defmodule CadetWeb.AssessmentsControllerTest do
         )
 
       params = %{
-        "visible_entries" => 1
+        "count" => 1
       }
 
       resp =
@@ -1894,7 +1894,7 @@ defmodule CadetWeb.AssessmentsControllerTest do
     end
   end
 
-  describe "GET /:assessment_id/scoreLeaderboard" do
+  describe "GET /:assessment_id/contest_score_leaderboard" do
     @tag authenticate: :student
     test "successful", %{conn: conn, courses: %{course1: course1}} do
       user = conn.assigns[:current_user]
@@ -1946,7 +1946,7 @@ defmodule CadetWeb.AssessmentsControllerTest do
         )
 
       params = %{
-        "visible_entries" => 1
+        "count" => 1
       }
 
       resp =
@@ -1970,7 +1970,7 @@ defmodule CadetWeb.AssessmentsControllerTest do
     do: "/v2/courses/#{course_id}/assessments/#{assessment_id}/unlock"
 
   defp build_popular_leaderboard_url(course_id, assessment_id, params \\ %{}) do
-    base_url = "#{build_url(course_id, assessment_id)}/popularVoteLeaderboard"
+    base_url = "#{build_url(course_id, assessment_id)}/contest_popular_leaderboard"
 
     if params != %{} do
       query_string = URI.encode_query(params)
@@ -1981,7 +1981,7 @@ defmodule CadetWeb.AssessmentsControllerTest do
   end
 
   defp build_score_leaderboard_url(course_id, assessment_id, params \\ %{}) do
-    base_url = "#{build_url(course_id, assessment_id)}/scoreLeaderboard"
+    base_url = "#{build_url(course_id, assessment_id)}/contest_score_leaderboard"
 
     if params != %{} do
       query_string = URI.encode_query(params)
