From 0f3ea6e3df40b2cc1f018a3e1aa9feb51a27f3b3 Mon Sep 17 00:00:00 2001 From: Ben Anderson Date: Fri, 11 Jul 2025 22:00:02 +1200 Subject: [PATCH 1/2] Add tests for (and fix) the after_save behaviour for contest This after_save hook is supposed to be handling the cases where the contest is changed, but there are all sorts of things going on that means it's being overwritten or ignored or just not called at all. I'd like to write out the weird stuff around contest_relation#finish_at, so this is step 1 of that. before_save is changed to after_save, as that's really what it should have been in the first place. The only reason it works now is that the calculation of finish_at is done inside `Contest` instead of `ContestRelation` which is 'a bit odd', but fine. --- app/models/contest.rb | 4 +- spec/models/contest_spec.rb | 135 ++++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 2 deletions(-) create mode 100644 spec/models/contest_spec.rb diff --git a/app/models/contest.rb b/app/models/contest.rb index 024ca908..2848677f 100644 --- a/app/models/contest.rb +++ b/app/models/contest.rb @@ -27,11 +27,11 @@ class Contest < ApplicationRecord scope :not_ended, -> { where("end_time > ?", Time.current) } scope :publicly_observable, -> { where(observation: OBSERVATION[:public]) } - before_save do # update the end time that was cached + after_save do if duration_changed? || end_time_changed? contest_relations.find_each do |relation| relation.finish_at = [end_time, relation.started_at.advance(hours: duration.to_f)].min.advance(seconds: relation.extra_time) unless relation.started_at.nil? - relation.save + relation.save! end end diff --git a/spec/models/contest_spec.rb b/spec/models/contest_spec.rb new file mode 100644 index 00000000..dbde725d --- /dev/null +++ b/spec/models/contest_spec.rb @@ -0,0 +1,135 @@ +require "spec_helper" + +describe Contest do + include ActiveSupport::Testing::TimeHelpers + + let(:user) { FactoryBot.create(:user) } + let(:contest) { FactoryBot.create(:contest, duration: 3) } + let(:relation) { ContestRelation.create!(contest: contest, user: user) } + + context "when the contest end_time is updated" do + context "when the contest can be completed within the window" do + before do + travel_to contest.start_time + 5.minutes + + relation.start! + end + + it "doesn't update contest_relation#finish_at" do + expect { + contest.update!(end_time: contest.end_time + 1.hour) + }.not_to change { + relation.reload.finish_at + } + end + end + + context "when the competitor started late and has a shortened window" do + before do + travel_to contest.end_time - 1.hour + + relation.start! + end + + context "when the contest is extended by a small amount" do + it "updates contest_relation#finish_at" do + expect { + contest.update!(end_time: contest.end_time + 1.hour) + }.to change { + relation.reload.finish_at + }.by(1.hour) + end + end + + context "when the contest is extended by a huge amount" do + it "updates contest_relation#finish_at to the maximum duration" do + expect { + contest.update!(end_time: contest.end_time + 10.weeks) + }.to change { + relation.reload.finish_at + }.by(2.hours) + end + end + end + end + + context "when the contest can be completed within the window" do + before do + travel_to contest.start_time + 5.minutes + end + + context "when the contest duration is increased" do + before do + relation.start! + end + + it "gives the competitor more time" do + expect { + contest.update!(duration: 7) + }.to change { + relation.reload.finish_at + }.by(4.hours) + end + end + + context "when the contest duration is decreased" do + before do + relation.start! + end + + it "gives the competitor less time" do + expect { + contest.update!(duration: 2) + }.to change { + relation.reload.finish_at + }.by(-1.hours) + end + end + end + + context "when the competitor started late and has a shortened window" do + before do + travel_to contest.end_time - 1.hour + + relation.start! + end + + context "when the contest duration is increased" do + it "gives the competitor no additional time" do + expect { + contest.update!(duration: 10.hours) + }.not_to change { + relation.reload.finish_at + } + end + end + + context "when the contest duration is decreased" do + it "gives the competitor no additional time" do + expect { + contest.update!(duration: 3.hours) + }.not_to change { + relation.reload.finish_at + } + end + end + end + + context "when the competitor started late but has full time" do + before do + travel_to contest.end_time - 4.hour + + relation.start! + end + + context "when the contest duration is increased" do + it "gives the competitor more time, but not the full duration" do + expect { + contest.update!(duration: 10.hours) + }.to change { + relation.reload.finish_at + }.by(1.hour) + end + end + end +end From 2dcf8fdad26a722affcd1241674fa023f75f5ca8 Mon Sep 17 00:00:00 2001 From: Ben Anderson Date: Fri, 11 Jul 2025 22:16:02 +1200 Subject: [PATCH 2/2] Refactor contest#after_save The prior version of this code is a bit hard to follow as also has one hook doing two responsibilities. This is just a minor factor to hopefully clear it all up a bit and move some of the responsibilities around. The long term goal is to let ContestRelation calculate it's own finish_at from its parent Contest, then changes to Contest just ask ContestRelation to recalculate, like every other way a contest_relation might need to be updated. We _could_ use touch: true on the has_many so that any saved changed to contest causes an update of contest_relation, then have the set_finish_at be a before_validation callback on ContestRelation, but I always feel a little weird about the implictness of that. --- app/models/contest.rb | 15 ++++++++------- app/models/contest_relation.rb | 5 +++++ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/app/models/contest.rb b/app/models/contest.rb index 2848677f..ad3f9bc7 100644 --- a/app/models/contest.rb +++ b/app/models/contest.rb @@ -27,14 +27,9 @@ class Contest < ApplicationRecord scope :not_ended, -> { where("end_time > ?", Time.current) } scope :publicly_observable, -> { where(observation: OBSERVATION[:public]) } - after_save do - if duration_changed? || end_time_changed? - contest_relations.find_each do |relation| - relation.finish_at = [end_time, relation.started_at.advance(hours: duration.to_f)].min.advance(seconds: relation.extra_time) unless relation.started_at.nil? - relation.save! - end - end + after_save :update_contest_relations + after_save do update_contest_scores if finalized_at_was && finalized_at.nil? true end @@ -172,4 +167,10 @@ def startcode=(code) def max_extra_time (duration * 3600).to_i end + + def update_contest_relations + return unless duration_changed? || end_time_changed? + + contest_relations.where.not(started_at: nil).find_each(&:set_finish_at!) + end end diff --git a/app/models/contest_relation.rb b/app/models/contest_relation.rb index 3a6658f0..986d5533 100644 --- a/app/models/contest_relation.rb +++ b/app/models/contest_relation.rb @@ -104,4 +104,9 @@ def recalculate_contest_scores_and_save after_save do recalculate_contest_scores_and_save if contest.finalized_at.nil? && extra_time_changed? end + + def set_finish_at! + new_finish_at = [contest.end_time, started_at + contest.duration.hours].min + update!(finish_at: new_finish_at + extra_time.seconds) + end end