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

Avoid crash when calling a delayed job on a deleted question #4847

Merged
merged 3 commits into from
Jul 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 10 additions & 7 deletions app/models/question.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,15 @@ class Question < Annotation
# Used to authorize the transitions
attr_accessor :transition_to, :transition_from

def self.reset_in_progress(id)
return unless exists?(id)

question = find(id)
return unless question.question_state == 'in_progress'

question.update(question_state: 'unanswered')
end

def to_partial_path
'annotations/annotation'
end
Expand All @@ -58,15 +67,9 @@ def clear_transition
@transition_from = nil
end

def reset_in_progress
return unless question_state == 'in_progress'

update(question_state: 'unanswered')
end

def schedule_reset_in_progress
return unless question_state == 'in_progress'

delay(run_at: 1.hour.from_now).reset_in_progress
Question.delay(run_at: 1.hour.from_now).reset_in_progress(id)
end
end
24 changes: 14 additions & 10 deletions test/controllers/annotations_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -469,16 +469,20 @@ def setup
}
assert_response :forbidden

# In progress -> answered
question = create :question, submission: @submission, question_state: :in_progress
patch annotation_path(question), params: {
from: question.question_state,
question: {
question_state: :answered
},
format: :json
}
assert_response :ok
# without delayed jobs, in progress is automatically reset to unanswered
with_delayed_jobs do
# In progress -> answered
question = create :question, submission: @submission, question_state: :in_progress
patch annotation_path(question), params: {
from: question.question_state,
question: {
question_state: :answered
},
format: :json
}
assert_response :ok
end
run_delayed_jobs
end

test 'questions can transition from in_progress' do
Expand Down
15 changes: 15 additions & 0 deletions test/models/annotation_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -154,4 +154,19 @@ class AnnotationTest < ActiveSupport::TestCase
run_delayed_jobs
assert q.reload.answered?
end

test 'delayed job should not crash if question is deleted before execution' do
q = create :question, submission: @submission, user: @student
assert q.unanswered?

with_delayed_jobs do
q.update(question_state: :in_progress)
assert q.reload.in_progress?
q.destroy
end

assert_nil Question.find_by(id: q.id)

run_delayed_jobs
end
end
5 changes: 4 additions & 1 deletion test/testhelpers/delayed_job_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ def with_delayed_jobs
end

def run_delayed_jobs
Delayed::Job.find_each(batch_size: 100) { |d| Delayed::Worker.new.run(d) }
Delayed::Job.find_each(batch_size: 100) do |d|
Delayed::Worker.new.run(d)
assert_nil d.last_error
end
end
end