From c9fc4c3afed05874b651ab4d3f28d20d6167d7cd Mon Sep 17 00:00:00 2001 From: YaleChen299 Date: Mon, 20 Sep 2021 21:54:14 +0800 Subject: [PATCH 1/2] Add contest leaderboard hiding --- lib/cadet/assessments/assessments.ex | 31 +++- .../question_types/voting_question.ex | 3 +- lib/cadet/jobs/xml_parser.ex | 4 +- .../20210915125021_add_reveal_hours.exs | 7 + test/cadet/assessments/assessments_test.exs | 3 +- test/cadet/assessments/question_test.exs | 3 +- .../question_types/voting_question_test.exs | 3 +- .../assessments_controller_test.exs | 156 ++++++++++++++++-- .../factories/assessments/question_factory.ex | 6 +- test/support/xml_generator.ex | 8 +- 10 files changed, 195 insertions(+), 29 deletions(-) create mode 100644 priv/repo/migrations/20210915125021_add_reveal_hours.exs diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 7a83d80f7..f95cdfec2 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -244,7 +244,7 @@ defmodule Cadet.Assessments do {q, a, nil, _} -> %{q | answer: %Answer{a | grader: nil}} {q, a, g, u} -> %{q | answer: %Answer{a | grader: %CourseRegistration{g | user: u}}} end) - |> load_contest_voting_entries(course_reg.course_id, course_reg.id) + |> load_contest_voting_entries(course_reg, assessment) assessment = assessment |> Map.put(:questions, questions) {:ok, assessment} @@ -734,6 +734,7 @@ defmodule Cadet.Assessments do |> preload([_, a], assessment: a) |> Repo.get(submission_id) + # allows staff to unsubmit own assessment bypass = role in @bypass_closed_roles and submission.student_id == course_reg_id with {:submission_found?, true} <- {:submission_found?, is_map(submission)}, @@ -884,7 +885,11 @@ defmodule Cadet.Assessments do end end - defp load_contest_voting_entries(questions, course_id, voter_id) do + defp load_contest_voting_entries( + questions, + %CourseRegistration{role: role, course_id: course_id, id: voter_id}, + assessment + ) do Enum.map( questions, fn q -> @@ -893,11 +898,16 @@ defmodule Cadet.Assessments do # fetch top 10 contest voting entries with the contest question id question_id = fetch_associated_contest_question_id(course_id, q) - leaderboard_results = [] - # temporary fix to hide the leaderboard - # if is_nil(question_id), - # do: [], - # else: fetch_top_relative_score_answers(question_id, 10) + leaderboard_results = + if is_nil(question_id) do + [] + else + if leaderboard_open?(assessment, q) or role in @open_all_assessment_roles do + fetch_top_relative_score_answers(question_id, 10) + else + [] + end + end # populate entries to vote for and leaderboard data into the question voting_question = @@ -941,6 +951,13 @@ defmodule Cadet.Assessments do end end + defp leaderboard_open?(assessment, voting_question) do + Timex.before?( + Timex.now(), + Timex.shift(assessment.close_at, hours: voting_question.question["reveal_hours"]) + ) + end + @doc """ Fetches top answers for the given question, based on the contest relative_score diff --git a/lib/cadet/assessments/question_types/voting_question.ex b/lib/cadet/assessments/question_types/voting_question.ex index 2d60a99f0..d76d65912 100644 --- a/lib/cadet/assessments/question_types/voting_question.ex +++ b/lib/cadet/assessments/question_types/voting_question.ex @@ -10,9 +10,10 @@ defmodule Cadet.Assessments.QuestionTypes.VotingQuestion do field(:prepend, :string, default: "") field(:template, :string) field(:contest_number, :string) + field(:reveal_hours, :integer) end - @required_fields ~w(content contest_number)a + @required_fields ~w(content contest_number reveal_hours)a @optional_fields ~w(prepend template)a def changeset(question, params \\ %{}) do diff --git a/lib/cadet/jobs/xml_parser.ex b/lib/cadet/jobs/xml_parser.ex index 847a38d09..1b67fa56b 100644 --- a/lib/cadet/jobs/xml_parser.ex +++ b/lib/cadet/jobs/xml_parser.ex @@ -85,7 +85,6 @@ defmodule Cadet.Updater.XMLParser do |> xpath( ~x"//TASK"e, access: ~x"./@access"s |> transform_by(&process_access/1), - # type: ~x"./@kind"s |> transform_by(&change_quest_to_sidequest/1), title: ~x"./@title"s, number: ~x"./@number"s, story: ~x"./@story"s, @@ -259,7 +258,8 @@ defmodule Cadet.Updater.XMLParser do entity |> xpath( ~x"./VOTING"e, - contest_number: ~x"./@assessment_number"s + contest_number: ~x"./@assessment_number"s, + reveal_hours: ~x"./@reveal_hours"i ) ) end diff --git a/priv/repo/migrations/20210915125021_add_reveal_hours.exs b/priv/repo/migrations/20210915125021_add_reveal_hours.exs new file mode 100644 index 000000000..1650d4087 --- /dev/null +++ b/priv/repo/migrations/20210915125021_add_reveal_hours.exs @@ -0,0 +1,7 @@ +defmodule Cadet.Repo.Migrations.AddRevealHours do + use Ecto.Migration + + def change do + execute("update questions set question = question || jsonb_build_object('reveal_hours', 48)") + end +end diff --git a/test/cadet/assessments/assessments_test.exs b/test/cadet/assessments/assessments_test.exs index acfe5e738..67865f2e8 100644 --- a/test/cadet/assessments/assessments_test.exs +++ b/test/cadet/assessments/assessments_test.exs @@ -74,7 +74,8 @@ defmodule Cadet.AssessmentsTest do library: build(:library), question: %{ content: Faker.Pokemon.name(), - contest_number: assessment.number + contest_number: assessment.number, + reveal_hours: 48 } }, assessment.id diff --git a/test/cadet/assessments/question_test.exs b/test/cadet/assessments/question_test.exs index a44aaaa5e..7a77f38fb 100644 --- a/test/cadet/assessments/question_test.exs +++ b/test/cadet/assessments/question_test.exs @@ -38,7 +38,8 @@ defmodule Cadet.Assessments.QuestionTest do library: build(:library), question: %{ content: Faker.Pokemon.name(), - contest_number: assessment.number + contest_number: assessment.number, + reveal_hours: 48 } } diff --git a/test/cadet/assessments/question_types/voting_question_test.exs b/test/cadet/assessments/question_types/voting_question_test.exs index 70e61678a..b3fd8949e 100644 --- a/test/cadet/assessments/question_types/voting_question_test.exs +++ b/test/cadet/assessments/question_types/voting_question_test.exs @@ -8,7 +8,8 @@ defmodule Cadet.Assessments.QuestionTypes.VotingQuestionTest do assert_changeset( %{ content: "content", - contest_number: "C4" + contest_number: "C4", + reveal_hours: 48 }, :valid ) diff --git a/test/cadet_web/controllers/assessments_controller_test.exs b/test/cadet_web/controllers/assessments_controller_test.exs index 403e88ebd..1a27ec05c 100644 --- a/test/cadet_web/controllers/assessments_controller_test.exs +++ b/test/cadet_web/controllers/assessments_controller_test.exs @@ -473,7 +473,7 @@ defmodule CadetWeb.AssessmentsControllerTest do end end - test "it renders contest leaderboards", %{ + test "renders open leaderboard for all roles", %{ conn: conn, course_regs: course_regs, courses: %{course1: course1}, @@ -510,17 +510,16 @@ defmodule CadetWeb.AssessmentsControllerTest do }) end - expected_leaderboard = [] - # temporary fix to hide the leaderboard - # for answer <- contest_answers do - # %{ - # "answer" => %{"code" => answer.answer.code}, - # "score" => answer.relative_score, - # "student_name" => answer.submission.student.user.name, - # "submission_id" => answer.submission.id - # } - # end - # |> Enum.sort_by(& &1["score"], &>=/2) + expected_leaderboard = + for answer <- contest_answers do + %{ + "answer" => %{"code" => answer.answer.code}, + "final_score" => answer.relative_score, + "student_name" => answer.submission.student.user.name, + "submission_id" => answer.submission.id + } + end + |> Enum.sort_by(& &1["final_score"], &>=/2) for role <- Role.__enum_map__() do course_reg = Map.get(role_crs, role) @@ -538,6 +537,139 @@ defmodule CadetWeb.AssessmentsControllerTest do end end + test "renders close leaderboard for staff and admin", %{ + conn: conn, + course_regs: course_regs, + courses: %{course1: course1}, + role_crs: role_crs, + assessments: assessments + } do + voting_assessment = assessments["practical"].assessment + + voting_assessment + |> Assessment.changeset(%{ + open_at: Timex.shift(Timex.now(), days: -30), + close_at: Timex.shift(Timex.now(), days: -20) + }) + |> Repo.update() + + voting_question = assessments["practical"].voting_questions |> List.first() + contest_assessment_number = voting_question.question.contest_number + + contest_assessment = Repo.get_by(Assessment, number: contest_assessment_number) + + # insert contest question + contest_question = + insert(:programming_question, %{ + display_order: 1, + assessment: contest_assessment, + max_xp: 1000 + }) + + # insert contest submissions and answers + contest_submissions = + for student <- Enum.take(course_regs.students, 5) do + insert(:submission, %{assessment: contest_assessment, student: student}) + end + + contest_answers = + for {submission, score} <- Enum.with_index(contest_submissions, 1) do + insert(:answer, %{ + xp: 1000, + question: contest_question, + submission: submission, + answer: build(:programming_answer), + relative_score: score / 1 + }) + end + + expected_leaderboard = + for answer <- contest_answers do + %{ + "answer" => %{"code" => answer.answer.code}, + "final_score" => answer.relative_score, + "student_name" => answer.submission.student.user.name, + "submission_id" => answer.submission.id + } + end + |> Enum.sort_by(& &1["final_score"], &>=/2) + + for role <- [:admin, :staff] do + course_reg = Map.get(role_crs, role) + + resp_leaderboard = + conn + |> sign_in(course_reg.user) + |> get(build_url(course1.id, voting_question.assessment.id)) + |> json_response(200) + |> Map.get("questions", []) + |> Enum.find(&(&1["id"] == voting_question.id)) + |> Map.get("contestLeaderboard") + + assert resp_leaderboard == expected_leaderboard + end + end + + test "does not render close leaderboard for students", %{ + conn: conn, + course_regs: course_regs, + courses: %{course1: course1}, + role_crs: %{student: course_reg}, + assessments: assessments + } do + voting_assessment = assessments["practical"].assessment + + voting_assessment + |> Assessment.changeset(%{ + open_at: Timex.shift(Timex.now(), days: -30), + close_at: Timex.shift(Timex.now(), days: -20) + }) + |> Repo.update() + + voting_question = assessments["practical"].voting_questions |> List.first() + contest_assessment_number = voting_question.question.contest_number + + contest_assessment = Repo.get_by(Assessment, number: contest_assessment_number) + + # insert contest question + contest_question = + insert(:programming_question, %{ + display_order: 1, + assessment: contest_assessment, + max_xp: 1000 + }) + + # insert contest submissions and answers + contest_submissions = + for student <- Enum.take(course_regs.students, 5) do + insert(:submission, %{assessment: contest_assessment, student: student}) + end + + _contest_answers = + for {submission, score} <- Enum.with_index(contest_submissions, 1) do + insert(:answer, %{ + xp: 1000, + question: contest_question, + submission: submission, + answer: build(:programming_answer), + relative_score: score / 1 + }) + end + + expected_leaderboard = [] + + resp_leaderboard = + conn + |> sign_in(course_reg.user) + |> get(build_url(course1.id, voting_question.assessment.id)) + |> json_response(200) + |> Map.get("questions", []) + |> Enum.find(&(&1["id"] == voting_question.id)) + |> Map.get("contestLeaderboard") + + assert resp_leaderboard == expected_leaderboard + end + test "it renders assessment question libraries", %{ conn: conn, courses: %{course1: course1}, diff --git a/test/factories/assessments/question_factory.ex b/test/factories/assessments/question_factory.ex index 63dd17c51..9836d1047 100644 --- a/test/factories/assessments/question_factory.ex +++ b/test/factories/assessments/question_factory.ex @@ -92,7 +92,8 @@ defmodule Cadet.Assessments.QuestionFactory do content: Faker.Pokemon.name(), prepend: Faker.Pokemon.location(), template: Faker.Lorem.Shakespeare.as_you_like_it(), - contest_number: contest_assessment.number + contest_number: contest_assessment.number, + reveal_hours: 48 } } end @@ -104,7 +105,8 @@ defmodule Cadet.Assessments.QuestionFactory do content: Faker.Pokemon.name(), prepend: Faker.Pokemon.location(), template: Faker.Lorem.Shakespeare.as_you_like_it(), - contest_number: contest_assessment.number + contest_number: contest_assessment.number, + reveal_hours: 48 } end end diff --git a/test/support/xml_generator.ex b/test/support/xml_generator.ex index b5df37648..e3d5e1a13 100644 --- a/test/support/xml_generator.ex +++ b/test/support/xml_generator.ex @@ -154,7 +154,11 @@ defmodule Cadet.Test.XMLGenerator do template_field = [template(question.question.template)] - voting_field = voting(%{assessment_number: question.question.contest_number}) + voting_field = + voting(%{ + reveal_hours: question.question.reveal_hours, + assessment_number: question.question.contest_number + }) [ snippet(prepend_field ++ template_field) @@ -163,7 +167,7 @@ defmodule Cadet.Test.XMLGenerator do end defp voting(raw_attr) do - {"VOTING", map_permit_keys(raw_attr, ~w(assessment_number)a)} + {"VOTING", map_permit_keys(raw_attr, ~w(assessment_number reveal_hours)a)} end defp deployment(raw_attrs, children) do From faf78c2139d3d370478f43b886b453bc9983a077 Mon Sep 17 00:00:00 2001 From: YaleChen299 Date: Fri, 24 Sep 2021 14:12:22 +0800 Subject: [PATCH 2/2] fix open leaderboard wrong condition --- lib/cadet/assessments/assessments.ex | 4 ++-- .../controllers/assessments_controller_test.exs | 15 +++++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index f95cdfec2..d37b0c45c 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -953,8 +953,8 @@ defmodule Cadet.Assessments do defp leaderboard_open?(assessment, voting_question) do Timex.before?( - Timex.now(), - Timex.shift(assessment.close_at, hours: voting_question.question["reveal_hours"]) + Timex.shift(assessment.close_at, hours: voting_question.question["reveal_hours"]), + Timex.now() ) end diff --git a/test/cadet_web/controllers/assessments_controller_test.exs b/test/cadet_web/controllers/assessments_controller_test.exs index 1a27ec05c..709fa960e 100644 --- a/test/cadet_web/controllers/assessments_controller_test.exs +++ b/test/cadet_web/controllers/assessments_controller_test.exs @@ -480,6 +480,15 @@ defmodule CadetWeb.AssessmentsControllerTest do role_crs: role_crs, assessments: assessments } do + voting_assessment = assessments["practical"].assessment + + voting_assessment + |> Assessment.changeset(%{ + open_at: Timex.shift(Timex.now(), days: -30), + close_at: Timex.shift(Timex.now(), days: -20) + }) + |> Repo.update() + voting_question = assessments["practical"].voting_questions |> List.first() contest_assessment_number = voting_question.question.contest_number @@ -548,8 +557,7 @@ defmodule CadetWeb.AssessmentsControllerTest do voting_assessment |> Assessment.changeset(%{ - open_at: Timex.shift(Timex.now(), days: -30), - close_at: Timex.shift(Timex.now(), days: -20) + close_at: Timex.shift(Timex.now(), days: 20) }) |> Repo.update() @@ -621,8 +629,7 @@ defmodule CadetWeb.AssessmentsControllerTest do voting_assessment |> Assessment.changeset(%{ - open_at: Timex.shift(Timex.now(), days: -30), - close_at: Timex.shift(Timex.now(), days: -20) + close_at: Timex.shift(Timex.now(), days: 20) }) |> Repo.update()