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.

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

Remove bonus xp #1089

Merged
merged 27 commits into from
Aug 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
bab5d39
bonus exp removed and awarded through grader
thortol Feb 28, 2024
c7e13ed
implemented give bonus exp after tutor is done marking
thortol Mar 12, 2024
c65fa09
moved update_xp_bonus to assessments.ex
thortol Mar 12, 2024
998b73e
fixed formatting
thortol Mar 17, 2024
27c3a6f
updated to master
thortol Mar 17, 2024
527619f
removed unnecessary code from grading job
thortol Mar 17, 2024
1f407cc
called decimal_to_integer rather than Decimal.to_integer
thortol Mar 23, 2024
ee9845d
updated documentation
thortol Mar 23, 2024
4bb69dc
fixed tests
thortol Mar 23, 2024
e3e4be0
bonus xp is awarded even if marker is the same as user
thortol Mar 23, 2024
48ac193
added new tests
thortol Mar 23, 2024
126d18f
fixed formatting
thortol Mar 23, 2024
ef76f3d
db migration added submitted_at column to submissions
thortol Mar 24, 2024
dd8b7b8
bonus xp is now calculated from the new submitted_at column
thortol Mar 24, 2024
da4e11f
Merge branch 'master' of https://github.com/source-academy/backend in…
RichDom2185 Apr 6, 2024
6bf4626
fixed merge conflicts
thortol Apr 14, 2024
e561ab7
Merge branch 'master' into remove-bonus-xp
thortol Apr 15, 2024
848e538
Merge branch 'master' into remove-bonus-xp
RichDom2185 May 5, 2024
88324a1
Merge branch 'master' into remove-bonus-xp
RichDom2185 May 5, 2024
189f4dd
Merge branch 'master' into remove-bonus-xp
thortol May 9, 2024
c9549c2
Merge branch 'master' into remove-bonus-xp
RichDom2185 May 19, 2024
f3c2347
Merge branch 'master' into remove-bonus-xp
thortol May 21, 2024
246d3ac
Merge branch 'master' into remove-bonus-xp
RichDom2185 Aug 4, 2024
44670b5
Merge branch 'master' into remove-bonus-xp
RichDom2185 Aug 11, 2024
fbe2e34
Redate migration file to maintain total ordering
RichDom2185 Aug 11, 2024
7e2b154
Remove extra newline
RichDom2185 Aug 11, 2024
3930e4e
Merge branch 'master' into remove-bonus-xp
RichDom2185 Aug 11, 2024
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
97 changes: 75 additions & 22 deletions lib/cadet/assessments/assessments.ex
Original file line number Diff line number Diff line change
Expand Up @@ -916,7 +916,7 @@
raw_answer,
force_submit
) do
with {:ok, team} <- find_team(question.assessment.id, cr_id),

Check warning on line 919 in lib/cadet/assessments/assessments.ex

View workflow job for this annotation

GitHub Actions / Run CI

variable "team" is unused (if the variable is not meant to be used, prefix it with an underscore)
{:ok, submission} <- find_or_create_submission(cr, question.assessment),
{:status, true} <- {:status, force_submit or submission.status != :submitted},
{:ok, _answer} <- insert_or_update_answer(submission, question, raw_answer, cr_id) do
Expand Down Expand Up @@ -1019,8 +1019,8 @@

def finalise_submission(submission = %Submission{}) do
with {:status, :attempted} <- {:status, submission.status},
{:ok, updated_submission} <- update_submission_status_and_xp_bonus(submission) do
# Couple with update_submission_status_and_xp_bonus to ensure notification is sent
{:ok, updated_submission} <- update_submission_status(submission) do
# Couple with update_submission_status and update_xp_bonus to ensure notification is sent
Copy link
Contributor

Choose a reason for hiding this comment

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

is update_xp_bonus still coupled?

Notifications.write_notification_when_student_submits(submission)
# Send email notification to avenger
%{notification_type: "assessment_submission", submission_id: updated_submission.id}
Expand All @@ -1029,6 +1029,7 @@

# Begin autograding job
GradingJob.force_grade_individual_submission(updated_submission)
update_xp_bonus(submission)
Copy link
Contributor

Choose a reason for hiding this comment

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

submitted_at will always be nil, since this submission is the one in line 1020 and Elixir is immutable.

Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be called here. A better place to trigger this is after the grading job response.

Also remember to remove irrelevant test in the cadet web and add relevant test in cadet and grading job.


{:ok, nil}
else
Expand Down Expand Up @@ -1413,30 +1414,75 @@
end
end

@spec update_submission_status_and_xp_bonus(Submission.t()) ::
@spec update_submission_status(Submission.t()) ::
{:ok, Submission.t()} | {:error, Ecto.Changeset.t()}
defp update_submission_status_and_xp_bonus(submission = %Submission{}) do
assessment = submission.assessment
assessment_conifg = Repo.get_by(AssessmentConfig, id: assessment.config_id)
defp update_submission_status(submission = %Submission{}) do
submission
|> Submission.changeset(%{status: :submitted, submitted_at: Timex.now()})
|> Repo.update()
end

max_bonus_xp = assessment_conifg.early_submission_xp
early_hours = assessment_conifg.hours_before_early_xp_decay
@spec update_xp_bonus(Submission.t()) ::
{:ok, Submission.t()} | {:error, Ecto.Changeset.t()}
defp update_xp_bonus(submission = %Submission{id: submission_id}) do
# to ensure backwards compatibility
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you mean here? what happen if xp_bonus is not 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the reason this was here was because I was worried that if a grader went to grade an old submission where submitted_at is null, the xp_bonus will be set to 0 and decrease. Therefore if xp_bonus is not 0 it does nothing and keeps it at whatever value it was previously set to.

Copy link
Contributor

Choose a reason for hiding this comment

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

but, if you have set the submitted_at, time, this should not be an worry.

The way to reason this is that think about this as a function, the bonus_xp should be a pure mathematical function of (submitted_at, xp, xp_adjustment).

Otherwise, if the autograder thinks that it is not low effort and bonus_xp is awarded, then the our staff grader will not be able to revoke this low effort 'bonus_xp'.
In the example that I gave here, bonus_xp depends onthe order of operation. Explicitly, if the TA did the adjustment before the bonus xp is awarded, then the bonus xp can be 0.
If the TA did the adjustment after the bonus xp is awarded, then the bonus xp cannot be 0.

If we decided that the TA should not affect the bonus xp behaviour, then we should not trigger update in update grading info.

if submission.xp_bonus == 0 do
assessment = submission.assessment
assessment_conifg = Repo.get_by(AssessmentConfig, id: assessment.config_id)

xp_bonus =
if Timex.before?(Timex.now(), Timex.shift(assessment.open_at, hours: early_hours)) do
max_bonus_xp
else
# This logic interpolates from max bonus at early hour to 0 bonus at close time
decaying_hours = Timex.diff(assessment.close_at, assessment.open_at, :hours) - early_hours
remaining_hours = Enum.max([0, Timex.diff(assessment.close_at, Timex.now(), :hours)])
proportion = if(decaying_hours > 0, do: remaining_hours / decaying_hours, else: 1)
bonus_xp = round(max_bonus_xp * proportion)
Enum.max([0, bonus_xp])
end
max_bonus_xp = assessment_conifg.early_submission_xp
early_hours = assessment_conifg.hours_before_early_xp_decay

submission
|> Submission.changeset(%{status: :submitted, xp_bonus: xp_bonus})
|> Repo.update()
ans_xp =
Answer
|> where(submission_id: ^submission_id)
|> order_by(:question_id)
|> group_by([a], a.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

You will only have answers here, there is no need to group

|> select([a], %{
# grouping by submission, so s.xp_bonus will be the same, but we need an
# aggregate function
total_xp: sum(a.xp) + sum(a.xp_adjustment)
Copy link
Contributor

Choose a reason for hiding this comment

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

rethink about what you are querying before editing the code. As mentioned, there is only one answer and there is not need to sum.

})

total =
ans_xp
|> subquery
|> select([a], %{
total_xp: sum(a.total_xp)
})
|> Repo.one()
Copy link
Contributor

Choose a reason for hiding this comment

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

this sum does nothing, since you did not group by anything.


xp = decimal_to_integer(total.total_xp)
Copy link
Contributor

Choose a reason for hiding this comment

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

if you just want to get the current xp, there should be shorter and cleaner way to do this.


cur_time =
if submission.submitted_at == nil do
Copy link
Contributor

Choose a reason for hiding this comment

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

as mentioned, it should not be nil, and this should be in the guard.

Timex.now()
else
submission.submitted_at
end

xp_bonus =
if xp <= 0 do
0
else
if Timex.before?(cur_time, Timex.shift(assessment.open_at, hours: early_hours)) do
max_bonus_xp
else
# This logic interpolates from max bonus at early hour to 0 bonus at close time
decaying_hours =
Timex.diff(assessment.close_at, assessment.open_at, :hours) - early_hours

remaining_hours = Enum.max([0, Timex.diff(assessment.close_at, cur_time, :hours)])
proportion = if(decaying_hours > 0, do: remaining_hours / decaying_hours, else: 1)
bonus_xp = round(max_bonus_xp * proportion)
Enum.max([0, bonus_xp])
end
end

submission
|> Submission.changeset(%{xp_bonus: xp_bonus})
|> Repo.update()
end
end

defp update_submission_status_router(submission = %Submission{}, question = %Question{}) do
Expand Down Expand Up @@ -2231,12 +2277,19 @@

is_own_submission = grader_id == answer.submission.student_id

submission =
Submission
|> join(:inner, [s], a in assoc(s, :assessment))
|> preload([_, a], assessment: a)
|> Repo.get(submission_id)

Copy link
Contributor

Choose a reason for hiding this comment

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

getting the submission and updating the bonus, both are not guarded here. Should write error handling here.

Copy link
Contributor

Choose a reason for hiding this comment

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

like how it was done with the with clause.

with {:answer_found?, true} <- {:answer_found?, is_map(answer)},
{:status, true} <-
{:status, answer.submission.status == :submitted or is_own_submission},
{:valid, changeset = %Ecto.Changeset{valid?: true}} <-
{:valid, Answer.grading_changeset(answer, attrs)},
{:ok, _} <- Repo.update(changeset) do
update_xp_bonus(submission)
{:ok, nil}
else
{:answer_found?, false} ->
Expand Down Expand Up @@ -2501,7 +2554,7 @@

def has_last_modified_answer?(
question = %Question{},
cr = %CourseRegistration{id: cr_id},

Check warning on line 2557 in lib/cadet/assessments/assessments.ex

View workflow job for this annotation

GitHub Actions / Run CI

variable "cr_id" is unused (if the variable is not meant to be used, prefix it with an underscore)
last_modified_at,
force_submit
) do
Expand Down
3 changes: 2 additions & 1 deletion lib/cadet/assessments/submission.ex
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ defmodule Cadet.Assessments.Submission do
field(:graded_count, :integer, virtual: true, default: 0)
field(:grading_status, :string, virtual: true)
field(:unsubmitted_at, :utc_datetime_usec)
field(:submitted_at, :utc_datetime_usec)
field(:is_grading_published, :boolean, default: false)

belongs_to(:assessment, Assessment)
Expand All @@ -29,7 +30,7 @@ defmodule Cadet.Assessments.Submission do
end

@required_fields ~w(assessment_id status is_grading_published)a
@optional_fields ~w(xp_bonus unsubmitted_by_id unsubmitted_at student_id team_id)a
@optional_fields ~w(xp_bonus unsubmitted_by_id unsubmitted_at submitted_at student_id team_id)a

def changeset(submission, params) do
submission
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
defmodule Cadet.Repo.Migrations.SubmissionsAddSubmittedAtColumn do
use Ecto.Migration

def change do
alter table(:submissions) do
add(:submitted_at, :timestamp, null: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

try to enforce a constraint like:
submission status is ‘submitted', if and only if submitted_at is not null.
so this means if we unsubmit we should also remove the submitted at, so that we will not accidentally 'compute' or keep the old xp bonus.
Also this is a semantically logical thing to do.

Please also update the auto-submission job to insert this value appropriately.

Copy link
Contributor

Choose a reason for hiding this comment

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

remember to write data migration, to make existing data conform to this constraint.

end
end
end
196 changes: 193 additions & 3 deletions test/cadet_web/controllers/assessments_controller_test.exs
Copy link
Contributor

Choose a reason for hiding this comment

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

Please additionally do unit test. since now update status and update bonus are separated, they should be tested individually.

Copy link
Contributor

Choose a reason for hiding this comment

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

please note that in this writing, the mock is only use to test if certain function is called in the process. it does not test what your algorithm will react to different grading job response.
To do that, you will probably need to use the cassettes.
Additionally, this is not the most appropriate place to test the logic/behaviour, do this in the unit test.

Original file line number Diff line number Diff line change
Expand Up @@ -1069,7 +1069,7 @@ defmodule CadetWeb.AssessmentsControllerTest do
end
end

test "submission of answer within early hours(seeded 48) of opening grants full XP bonus", %{
test "submission of answer with no effort grants 0 XP bonus", %{
conn: conn,
courses: %{course1: course1},
role_crs: role_crs
Expand Down Expand Up @@ -1117,6 +1117,194 @@ defmodule CadetWeb.AssessmentsControllerTest do

submission_db = Repo.get(Submission, submission.id)

assert submission_db.status == :submitted
assert submission_db.xp_bonus == 0
end
end

test "submission of answer grants XP bonus only after being marked by an avenger", %{
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the behaviour that we want? what if it is autograded?

conn: conn,
courses: %{course1: course1},
role_crs: role_crs
} do
with_mock GradingJob, force_grade_individual_submission: fn _ -> nil end do
assessment_config =
insert(
:assessment_config,
early_submission_xp: 100,
hours_before_early_xp_decay: 48,
course: course1
)

assessment =
insert(
:assessment,
open_at: Timex.shift(Timex.now(), hours: -40),
close_at: Timex.shift(Timex.now(), days: 7),
is_published: true,
config: assessment_config,
course: course1
)

question = insert(:programming_question, assessment: assessment)

group = insert(:group, leader: role_crs.staff)

course_reg =
insert(:course_registration, %{role: :student, group: group, course: course1})

submission =
insert(:submission, assessment: assessment, student: course_reg, status: :attempted)

insert(
:answer,
submission: submission,
question: question,
answer: %{code: "f => f(f);"},
xp_adjustment: 0
)

conn
|> sign_in(course_reg.user)
|> post(build_url_submit(course1.id, assessment.id))
|> response(200)

submission_db = Repo.get(Submission, submission.id)

assert submission_db.status == :submitted
assert submission_db.xp_bonus == 0

grade_question = fn question ->
Assessments.update_grading_info(
%{submission_id: submission.id, question_id: question.id},
%{"xp_adjustment" => 10},
role_crs.staff
)
end

grade_question.(question)

submission_db = Repo.get(Submission, submission.id)
assert submission_db.xp_bonus == 100
end
end

test "submission of answer grants 0 XP bonus if an avenger gives 0 as well", %{
conn: conn,
courses: %{course1: course1},
role_crs: role_crs
} do
with_mock GradingJob, force_grade_individual_submission: fn _ -> nil end do
assessment_config =
insert(
:assessment_config,
early_submission_xp: 100,
hours_before_early_xp_decay: 48,
course: course1
)

assessment =
insert(
:assessment,
open_at: Timex.shift(Timex.now(), hours: -40),
close_at: Timex.shift(Timex.now(), days: 7),
is_published: true,
config: assessment_config,
course: course1
)

question = insert(:programming_question, assessment: assessment)

group = insert(:group, leader: role_crs.staff)

course_reg =
insert(:course_registration, %{role: :student, group: group, course: course1})

submission =
insert(:submission, assessment: assessment, student: course_reg, status: :attempted)

insert(
:answer,
submission: submission,
question: question,
answer: %{code: "f => f(f);"},
xp_adjustment: 0
)

conn
|> sign_in(course_reg.user)
|> post(build_url_submit(course1.id, assessment.id))
|> response(200)

submission_db = Repo.get(Submission, submission.id)

assert submission_db.status == :submitted
assert submission_db.xp_bonus == 0

grade_question = fn question ->
Assessments.update_grading_info(
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should not be here, if you want to run this function, you should test it in cadet. cadet web is for testing end points.

Additionally, the admin grading controller endpoint which is affected by the update_grading_info should be tested.

%{submission_id: submission.id, question_id: question.id},
%{"xp_adjustment" => 0},
role_crs.staff
)
end

grade_question.(question)

submission_db = Repo.get(Submission, submission.id)
assert submission_db.xp_bonus == 0
end
end

test "submission of answer within early hours(seeded 48) of opening grants full XP bonus", %{
conn: conn,
courses: %{course1: course1},
role_crs: role_crs
} do
with_mock GradingJob, force_grade_individual_submission: fn _ -> nil end do
Copy link
Contributor

Choose a reason for hiding this comment

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

some of these seems to be old issue since with_mock is actually not used here. since we did not test if force_grade_individual_submission is called like the first test in this suit.

assessment_config =
insert(
:assessment_config,
early_submission_xp: 100,
hours_before_early_xp_decay: 48,
course: course1
)

assessment =
insert(
:assessment,
open_at: Timex.shift(Timex.now(), hours: -40),
close_at: Timex.shift(Timex.now(), days: 7),
is_published: true,
config: assessment_config,
course: course1
)

question = insert(:programming_question, assessment: assessment)

group = insert(:group, leader: role_crs.staff)

course_reg =
insert(:course_registration, %{role: :student, group: group, course: course1})

submission =
insert(:submission, assessment: assessment, student: course_reg, status: :attempted)

insert(
:answer,
submission: submission,
question: question,
answer: %{code: "f => f(f);"},
xp: 10
)

conn
|> sign_in(course_reg.user)
|> post(build_url_submit(course1.id, assessment.id))
|> response(200)

submission_db = Repo.get(Submission, submission.id)

assert submission_db.status == :submitted
assert submission_db.xp_bonus == 100
end
Expand Down Expand Up @@ -1161,7 +1349,8 @@ defmodule CadetWeb.AssessmentsControllerTest do
:answer,
submission: submission,
question: question,
answer: %{code: "f => f(f);"}
answer: %{code: "f => f(f);"},
xp: 10
)

conn
Expand Down Expand Up @@ -1219,7 +1408,8 @@ defmodule CadetWeb.AssessmentsControllerTest do
:answer,
submission: submission,
question: question,
answer: %{code: "f => f(f);"}
answer: %{code: "f => f(f);"},
xp: 10
)

conn
Expand Down
Loading