diff --git a/app/jobs/course/assessment/submission/publish_task_completion_job.rb b/app/jobs/course/assessment/submission/publish_task_completion_job.rb new file mode 100644 index 0000000000..dcff912354 --- /dev/null +++ b/app/jobs/course/assessment/submission/publish_task_completion_job.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true +# Pushes a submission's task status to Cikgo once the transaction that changed it has committed. +# +# The push is a synchronous HTTP call with a five second timeout, so it is kept off the request: a +# slow or unavailable Cikgo must not delay a response, hold locks on a submission that is already +# saved, or fail a request for work that has been committed. +# +# Unlike the inline +publish_task_completion+, which logs and swallows in production, a failure here +# is allowed to propagate. That is the point of running it as a job: Sidekiq retries it and Rollbar +# records it, rather than the error disappearing into the log. +class Course::Assessment::Submission::PublishTaskCompletionJob < ApplicationJob + rescue_from(ActiveJob::DeserializationError) do |_| + # The submission was deleted before the job ran; there is no status left to publish. + end + + def perform(submission) + instance = Course.unscoped { submission.assessment.course.instance } + + ActsAsTenant.with_tenant(instance) do + # Re-read rather than trusting the state at enqueue time. A later transition may already have + # been pushed, and the status Cikgo should end up with is the current one either way — which + # also makes two pushes racing each other harmless. + next unless submission.should_publish_task_completion? + + submission.publish_task_completion! + end + end +end diff --git a/app/jobs/course/lesson_plan/personalized_timeline_update_job.rb b/app/jobs/course/lesson_plan/personalized_timeline_update_job.rb new file mode 100644 index 0000000000..49cbda169e --- /dev/null +++ b/app/jobs/course/lesson_plan/personalized_timeline_update_job.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true +# Recomputes one course user's personalised timeline. +# +# Enqueued after a submission is finalised, rather than run inside the finalise transaction: the +# recomputation walks every lesson plan item in the course and writes a personal time per shiftable +# item, which is by far the largest unit of work in that transaction and the only part that contends +# with +CoursewidePersonalizedTimelineUpdateJob+ for the same rows. +# +# The timeline is derived state. If this job never succeeds the student's timeline is one submission +# stale but still valid, and the next recomputation for them — their next submission, an instructor +# pressing "Recompute all times", or a coursewide run — brings it forward. +class Course::LessonPlan::PersonalizedTimelineUpdateJob < ApplicationJob + include Course::LessonPlan::PersonalizationConcern + + queue_as :lowest + + rescue_from(ActiveJob::DeserializationError) do |_| + # The course user was removed from the course before the job ran; there is no timeline to update. + end + + def perform(course_user) + instance = Course.unscoped { course_user.course.instance } + + ActsAsTenant.with_tenant(instance) do + update_personalized_timeline_for_user(course_user) + end + end +end diff --git a/app/models/concerns/course/assessment/submission/cikgo_task_completion_concern.rb b/app/models/concerns/course/assessment/submission/cikgo_task_completion_concern.rb index 334356eed1..3b77547dd3 100644 --- a/app/models/concerns/course/assessment/submission/cikgo_task_completion_concern.rb +++ b/app/models/concerns/course/assessment/submission/cikgo_task_completion_concern.rb @@ -10,18 +10,27 @@ module Course::Assessment::Submission::CikgoTaskCompletionConcern extend ActiveSupport::Concern included do - after_save :publish_task_completion, if: -> { should_publish_task_completion? && saved_change_to_workflow_state? } + after_save :enqueue_publish_task_completion, + if: -> { should_publish_task_completion? && saved_change_to_workflow_state? } end + # Pushes the status, absorbing failures the way the controller's +edit+ hook needs: logged, and + # fatal only outside production so a broken Cikgo integration is noticed in development. def publish_task_completion + publish_task_completion! + rescue StandardError => e + Rails.logger.error("Cikgo: Cannot publish task completion for submission #{id}: #{e}") + raise e unless Rails.env.production? + end + + # Pushes the status and lets failures propagate, for callers that can act on them — + # +PublishTaskCompletionJob+, where Sidekiq retries and Rollbar records. + def publish_task_completion! Cikgo::ResourcesService.mark_task!(status, lesson_plan_item, { user_id: creator_id_on_cikgo, url: submission_url, score: grade&.to_i }) - rescue StandardError => e - Rails.logger.error("Cikgo: Cannot publish task completion for submission #{id}: #{e}") - raise e unless Rails.env.production? end def should_publish_task_completion? @@ -31,6 +40,14 @@ def should_publish_task_completion? private + # Hands the push to a job once the transaction that changed the status has committed, so the + # request neither waits on Cikgo nor fails because of it. See +PublishTaskCompletionJob+. + def enqueue_publish_task_completion + ActiveRecord.after_all_transactions_commit do + Course::Assessment::Submission::PublishTaskCompletionJob.perform_later(self) + end + end + delegate :edit_course_assessment_submission_url, to: 'Rails.application.routes.url_helpers' def status diff --git a/app/models/concerns/course/assessment/submission/notification_concern.rb b/app/models/concerns/course/assessment/submission/notification_concern.rb index f7fd4f00d6..c1975342b9 100644 --- a/app/models/concerns/course/assessment/submission/notification_concern.rb +++ b/app/models/concerns/course/assessment/submission/notification_concern.rb @@ -27,6 +27,14 @@ def send_submit_notification return if assessment.autograded? return unless course_user.student? - Course::AssessmentNotifier.assessment_submitted(creator, course_user, self) + # Building the activity walks the submitter's groups to find managers and subtracts the + # unsubscribed ones, then writes the activity and its notifications. None of that needs to be in + # the transaction that commits the student's work: an unsent notification is recoverable + # attention, a rolled back submission is lost work. It stays on the request rather than becoming + # a job of its own, since its real work is enqueuing mail delivery jobs — already deferred to + # after commit by Notifier::Base::ActivityWrapper. + ActiveRecord.after_all_transactions_commit do + Course::AssessmentNotifier.assessment_submitted(creator, course_user, self) + end end end diff --git a/app/models/concerns/course/assessment/submission/workflow_event_concern.rb b/app/models/concerns/course/assessment/submission/workflow_event_concern.rb index 1b66ac4974..09fb6db08a 100644 --- a/app/models/concerns/course/assessment/submission/workflow_event_concern.rb +++ b/app/models/concerns/course/assessment/submission/workflow_event_concern.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true module Course::Assessment::Submission::WorkflowEventConcern extend ActiveSupport::Concern - include Course::LessonPlan::PersonalizationConcern include Course::Assessment::Submission::CikgoTaskCompletionConcern included do @@ -30,7 +29,7 @@ def finalise(_ = nil) # NB: We are not recomputing on unsubmission because unsubmit is not done by the student # It will recompute again when resubmission occurs. This also prevents the timings for # the unsubmitted item from changing e.g. from other submissions that the student has done. - update_personalized_timeline_for_user(course_user) + enqueue_personalized_timeline_update end # Handles the marking of a submission. @@ -104,6 +103,21 @@ def resubmit_programming private + # Recomputes the submitter's personalised timeline out of band. + # + # The recomputation reads every lesson plan item in the course and writes a personal time per + # shiftable item, so it is kept out of the transaction that commits the student's work: a failure + # there used to roll the whole finalise back (see + # docs/personal_time_duplicate_incident_summary.md). Enqueued after commit so the job also reads a + # committed submission — +submitted_items+ is meant to include this one. + def enqueue_personalized_timeline_update + submitter = course_user + + ActiveRecord.after_all_transactions_commit do + Course::LessonPlan::PersonalizedTimelineUpdateJob.perform_later(submitter) + end + end + # finalise event (from attempting) - Assign 0 points as there are no questions. def assign_zero_experience_points return unless assessment.questions.empty? diff --git a/spec/controllers/concerns/course/lesson_plan/personalization_concern_spec.rb b/spec/controllers/concerns/course/lesson_plan/personalization_concern_spec.rb index 6f7e1166c9..4765cf95db 100644 --- a/spec/controllers/concerns/course/lesson_plan/personalization_concern_spec.rb +++ b/spec/controllers/concerns/course/lesson_plan/personalization_concern_spec.rb @@ -197,9 +197,15 @@ def submit_assessment(assessment) context 'when the course has many course users' do let!(:course_users) do - create_list(:course_user, 3, course: course, timeline_algorithm: 'fomo').each do |course_user| + users = create_list(:course_user, 3, course: course, timeline_algorithm: 'fomo') + users.each do |course_user| create(:course_assessment_submission, assessment: assessment, creator: course_user.user).tap(&:finalise!) end + # Each finalise enqueues a PersonalizedTimelineUpdateJob for its submitter. Settle them + # before the example runs, so the coursewide recomputation under test is not racing three + # per-user recomputations over the same rows. + wait_for_enqueued_jobs + users end it 'shifts the item for every course user' do diff --git a/spec/jobs/course/assessment/submission/publish_task_completion_job_spec.rb b/spec/jobs/course/assessment/submission/publish_task_completion_job_spec.rb new file mode 100644 index 0000000000..a34dd49104 --- /dev/null +++ b/spec/jobs/course/assessment/submission/publish_task_completion_job_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe Course::Assessment::Submission::PublishTaskCompletionJob do + let(:instance) { Instance.default } + with_tenant(:instance) do + let(:course) { create(:course) } + let(:assessment) { create(:assessment, course: course) } + let(:course_student) { create(:course_student, course: course) } + let(:submission) do + create(:submission, :attempting, assessment: assessment, + creator: course_student.user, course_user: course_student) + end + subject { Course::Assessment::Submission::PublishTaskCompletionJob } + + it 'can be queued' do + expect { subject.perform_later(submission) }.to have_enqueued_job(subject).exactly(:once) + end + + it 'pushes the current status' do + allow(submission).to receive(:should_publish_task_completion?).and_return(true) + expect(submission).to receive(:publish_task_completion!) + + subject.perform_now(submission) + end + + # Re-read at run time rather than trusted from enqueue time, so a superseded push is dropped. + it 'does not push when the submission no longer qualifies' do + allow(submission).to receive(:should_publish_task_completion?).and_return(false) + expect(submission).not_to receive(:publish_task_completion!) + + subject.perform_now(submission) + end + + it 'lets a failed push propagate, so Sidekiq retries and Rollbar records it' do + allow(submission).to receive(:should_publish_task_completion?).and_return(true) + allow(submission).to receive(:publish_task_completion!).and_raise(StandardError, 'cikgo down') + + expect { subject.perform_now(submission) }.to raise_error(StandardError, 'cikgo down') + end + end +end diff --git a/spec/jobs/course/lesson_plan/personalized_timeline_update_job_spec.rb b/spec/jobs/course/lesson_plan/personalized_timeline_update_job_spec.rb new file mode 100644 index 0000000000..a933499f71 --- /dev/null +++ b/spec/jobs/course/lesson_plan/personalized_timeline_update_job_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe Course::LessonPlan::PersonalizedTimelineUpdateJob do + let(:instance) { Instance.default } + with_tenant(:instance) do + let(:course) { create(:course) } + let!(:submitted_assessment) do + create(:assessment, course: course, start_at: 2.days.ago, end_at: 3.days.from_now, published: true) + end + let!(:upcoming_assessment) do + create(:assessment, course: course, start_at: 5.days.from_now, end_at: 10.days.from_now, published: true) + end + let(:timeline_algorithm) { 'fomo' } + let(:course_user) { create(:course_user, course: course, timeline_algorithm: timeline_algorithm) } + let(:submission) do + create(:course_assessment_submission, assessment: submitted_assessment, creator: course_user.user) + end + subject { Course::LessonPlan::PersonalizedTimelineUpdateJob } + + it 'can be queued' do + expect { subject.perform_later(course_user) }.to have_enqueued_job(subject).exactly(:once) + end + + it 'is enqueued when a submission is finalised' do + expect { submission.finalise! }.to have_enqueued_job(subject).exactly(:once) + end + + context 'when the course user is on a personalized timeline' do + it 'shifts the timeline for the course user', :sidekiq_same_thread do + submission.finalise! + submission.save! + expect(course_user.personal_times).to be_empty + + perform_sidekiq_jobs { subject.perform_later(course_user) } + + # The submitted item is never shifted, so only the upcoming one gets a personal time. + expect(course_user.personal_times.count).to eq(1) + expect(course_user.personal_times.first.lesson_plan_item_id). + to eq(upcoming_assessment.lesson_plan_item.id) + end + + it 'is safe to run more than once', :sidekiq_same_thread do + submission.finalise! + submission.save! + + perform_sidekiq_jobs { subject.perform_later(course_user) } + first_run = course_user.personal_times.pluck(:lesson_plan_item_id, :start_at, :end_at) + + perform_sidekiq_jobs { subject.perform_later(course_user) } + + # Idempotent, and — importantly for the offload — it never accumulates rows: the timeline is + # recomputed from current state rather than appended to. + expect(course_user.personal_times.reload.pluck(:lesson_plan_item_id, :start_at, :end_at)). + to match_array(first_run) + end + end + + context 'when the course user is on the fixed timeline' do + let(:timeline_algorithm) { 'fixed' } + + it 'creates no personal times', :sidekiq_same_thread do + submission.finalise! + submission.save! + + perform_sidekiq_jobs { subject.perform_later(course_user) } + + expect(course_user.personal_times).to be_empty + end + end + end +end diff --git a/spec/models/course/assessment/submission_spec.rb b/spec/models/course/assessment/submission_spec.rb index 9bc44a4277..44d4a0451c 100644 --- a/spec/models/course/assessment/submission_spec.rb +++ b/spec/models/course/assessment/submission_spec.rb @@ -344,6 +344,81 @@ end end + # The finalise transaction commits the student's work, so nothing derived from it — the + # personalised timeline, the notification, the Cikgo push — may be able to roll it back. The + # first two also leave the request entirely, as jobs, because they call out to a third party + # or walk the whole course. + describe 'work deferred past the finalise transaction' do + # Created up front so that only the finalise itself is observed: creating a submission + # notifies too, and would otherwise land inside the transaction under test. + before { submission } + + # The notification is only sent when the student submits for themselves, so the stamper has + # to be the creator — as it is on every path that reaches here in production. + def finalise + User.with_stamper(submission.creator) { submission.update!('finalise' => 'true') } + end + + def rolled_back_finalise + User.with_stamper(submission.creator) do + ActiveRecord::Base.transaction do + submission.update!('finalise' => 'true') + raise ActiveRecord::Rollback + end + end + end + + with_active_job_queue_adapter(:test) do + context 'when the course is pushed to Cikgo' do + before { allow(submission).to receive(:should_publish_task_completion?).and_return(true) } + + it 'enqueues the Cikgo push instead of calling out during the request' do + expect(submission).not_to receive(:publish_task_completion!) + + expect { finalise }. + to have_enqueued_job(Course::Assessment::Submission::PublishTaskCompletionJob). + exactly(:once) + end + + it 'does not enqueue the push when the finalise is rolled back' do + expect { rolled_back_finalise }. + not_to have_enqueued_job(Course::Assessment::Submission::PublishTaskCompletionJob) + end + end + + it 'enqueues the personalised timeline recomputation rather than running it inline' do + expect { finalise }. + to have_enqueued_job(Course::LessonPlan::PersonalizedTimelineUpdateJob).exactly(:once) + end + + it 'does not enqueue the recomputation when the finalise is rolled back' do + expect { rolled_back_finalise }. + not_to have_enqueued_job(Course::LessonPlan::PersonalizedTimelineUpdateJob) + end + end + + # Asserted through the activity record rather than the notifier, which is reached through + # Notifier::Base.method_missing and so cannot be a verified double. The notification stays on + # the request — its own work is enqueuing mail delivery — so this only checks that it is out + # of the transaction. + it 'writes the submission activity only once the transaction has committed' do + activities = Activity.count + + User.with_stamper(submission.creator) do + ActiveRecord::Base.transaction do + submission.update!('finalise' => 'true') + expect(Activity.count).to eq(activities) + end + end + + expect(Activity.count).to eq(activities + 1) + end + + it 'does not write the activity when the finalise is rolled back' do + expect { rolled_back_finalise }.not_to change(Activity, :count) + end + end + context 'when one of the answers is finalised' do before do answer = submission.answers.sample diff --git a/spec/support/active_job.rb b/spec/support/active_job.rb index b0de49e071..0b7c2f17f2 100644 --- a/spec/support/active_job.rb +++ b/spec/support/active_job.rb @@ -122,6 +122,17 @@ def clear_enqueued_jobs ActiveJob::Base.queue_adapter.clear_enqueued_jobs end + # Blocks until every job enqueued so far has finished. + # + # Unlike +wait_for_job+, this does not skip the example. Use it when setup enqueues work whose + # effects the example depends on — the background thread adapter otherwise runs those jobs + # concurrently with the example body, and only joins them once the example is over. + def wait_for_enqueued_jobs + return unless ActiveJob::Base.queue_adapter.is_a?(ActiveJob::QueueAdapters::BackgroundThreadAdapter) + + ActiveJob::Base.queue_adapter.wait_for_jobs + end + # Polls until at least +count+ emails have been delivered, or the Capybara wait time elapses. Under # the :sidekiq_separate_thread harness a mail job enqueued by the Capybara server thread is delivered # out-of-band by the worker thread, so feature specs must wait for it rather than assert immediately.