Merge pull request #2331 from openstax/fix_score_performance

Fixed logic for determining when the task cache should be valid and added specs

app/controllers/api/v1/task_plans_controller.rb

@@ -294,7 +294,9 @@ def restore
   def distribute_tasks(task_plan)
     preview_only = !task_plan.is_publish_requested && !task_plan.is_published?
 
-    task_plan.publish_last_requested_at = Time.current \
+    current_time = Time.current
+    task_plan.updated_by_instructor_at = current_time
+    task_plan.publish_last_requested_at = current_time \
       unless preview_only || task_plan.out_to_students?
     task_plan.save
     return if task_plan.errors.any?

app/subsystems/tasks/get_performance_report.rb

@@ -219,7 +219,8 @@ def get_past_due_tasks_by_role_id(roles, current_time)
       task_types = Tasks::Models::Task.task_types.values_at(:reading, :homework, :external)
       tt = Tasks::Models::Task.arel_table
       er = Entity::Role.arel_table
-      Tasks::Models::Task
+
+      tasks = Tasks::Models::Task
         .select([ tt[Arel.star], er[:id].as('"role_id"') ])
         .joins(:course, task_plan: :tasking_plans, taskings: { role: [ profile: :account ] })
         .where(task_type: task_types, task_plan: { withdrawn_at: nil }, taskings: { role: roles })
@@ -233,7 +234,12 @@ def get_past_due_tasks_by_role_id(roles, current_time)
         .preload(:taskings, :course, task_plan: [ :tasking_plans, :course, :extensions ])
         .reorder(nil)
         .distinct
-        .group_by(&:role_id)
+
+      # Preload uncached tasks to avoid N + 1 queries
+      uncached_tasks = tasks.reject(&:cache_valid?)
+      ActiveRecord::Associations::Preloader.new.preload uncached_tasks, task_steps: :tasked
+
+      tasks.group_by(&:role_id)
     end
 
     def filter_and_sort_tasking_plans(tasks, course, period)

app/subsystems/tasks/models/task.rb

@@ -231,8 +231,11 @@ def late_work_penalty_per_period
     grading_template&.late_work_penalty || 0.0
   end
 
-  def cached?
-    updated_at.nil? || task_plan.nil? || updated_at >= task_plan.updated_at
+  def cache_valid?
+    updated_at.nil? ||
+    task_plan.nil? ||
+    task_plan.updated_by_instructor_at.nil? ||
+    updated_at >= task_plan.updated_by_instructor_at
   end
 
   def completion
@@ -269,7 +272,7 @@ def available_points_without_dropping
   end
 
   def available_points(use_cache: true)
-    if use_cache && cached?
+    if use_cache && cache_valid?
       cache = super()
       return cache unless cache.nil?
     end
@@ -333,7 +336,7 @@ def points
   def published_points(past_due: nil, use_cache: true)
     past_due = past_due? if past_due.nil?
 
-    if use_cache && cached?
+    if use_cache && cache_valid?
       if past_due
         return if published_points_after_due&.nan?
         return published_points_after_due \
@@ -544,7 +547,7 @@ def feedback_available?(past_due: nil, current_time: Time.current, current_time_
   def provisional_score?(past_due: nil, use_cache: true)
     past_due = past_due? if past_due.nil?
 
-    if use_cache && cached?
+    if use_cache && cache_valid?
       if past_due
         return is_provisional_score_after_due \
           unless is_provisional_score_after_due.nil?
@@ -584,15 +587,15 @@ def hidden?
   end
 
   def started?(use_cache: false)
-    if use_cache && cached?
+    if use_cache && cache_valid?
       completed_steps_count > 0
     else
       task_steps.loaded? ? task_steps.any?(&:completed?) : task_steps.complete.exists?
     end
   end
 
   def completed?(use_cache: false)
-    if use_cache && cached?
+    if use_cache && cache_valid?
       completed_steps_count == steps_count
     else
       task_steps.loaded? ? task_steps.all?(&:completed?) : !task_steps.incomplete.exists?

app/subsystems/tasks/models/task_plan.rb

@@ -1,6 +1,9 @@
 require 'json-schema'
 
 class Tasks::Models::TaskPlan < ApplicationRecord
+  # Allow use of 'type' column without STI
+  self.inheritance_column = nil
+
   acts_as_paranoid column: :withdrawn_at, without_default_scope: true
 
   UPDATEABLE_ATTRIBUTES_AFTER_OPEN = [
@@ -9,7 +12,8 @@ class Tasks::Models::TaskPlan < ApplicationRecord
     'last_published_at',
     'tasks_grading_template_id',
     'gradable_step_count',
-    'ungraded_step_count'
+    'ungraded_step_count',
+    'updated_by_instructor_at'
   ]
 
   CACHE_COLUMNS = [
@@ -19,9 +23,6 @@ class Tasks::Models::TaskPlan < ApplicationRecord
 
   attr_accessor :is_publish_requested
 
-  # Allow use of 'type' column without STI
-  self.inheritance_column = nil
-
   belongs_to :cloned_from, foreign_key: 'cloned_from_id',
                            class_name: 'Tasks::Models::TaskPlan',
                            optional: true
@@ -78,6 +79,13 @@ def reload(*args)
     super
   end
 
+  def updated_by_instructor_at
+    val = super
+    return val unless val.nil?
+
+    publish_last_requested_at
+  end
+
   def withdrawn?
     deleted?
   end

db/migrate/20210622152149_add_updated_by_instructor_at_to_tasks_task_plans.rb

@@ -0,0 +1,5 @@
+class AddUpdatedByInstructorAtToTasksTaskPlans < ActiveRecord::Migration[5.2]
+  def change
+    add_column :tasks_task_plans, :updated_by_instructor_at, :datetime
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2021_04_19_211921) do
+ActiveRecord::Schema.define(version: 2021_06_22_152149) do
 
   create_sequence "active_storage_attachments_id_seq"
   create_sequence "active_storage_blobs_id_seq"
@@ -1001,6 +1001,7 @@
     t.integer "ungraded_step_count", default: 0, null: false
     t.integer "wrq_count", default: 0, null: false
     t.integer "gradable_step_count", default: 0, null: false
+    t.datetime "updated_by_instructor_at"
     t.index ["cloned_from_id"], name: "index_tasks_task_plans_on_cloned_from_id"
     t.index ["content_ecosystem_id"], name: "index_tasks_task_plans_on_content_ecosystem_id"
     t.index ["course_profile_course_id"], name: "index_tasks_task_plans_on_course_profile_course_id"

spec/routines/distribute_tasks_spec.rb

@@ -75,6 +75,7 @@
 
         homework_plan.tasks.each do | task |
           expect(task.task_steps.map(&:group_type)).to eq(expected_step_types)
+          expect(task).to be_cache_valid
         end
       end
     end
@@ -113,6 +114,7 @@
 
         homework_plan.tasks.each do | task |
           expect(task.task_steps.map(&:group_type)).to eq(expected_step_types)
+          expect(task).to be_cache_valid
         end
       end
 
@@ -181,6 +183,7 @@
           expect(task.points).to eq 9.0
           expect(task.score_without_lateness).to eq 1.0
           expect(task.score).to eq 1.0
+          expect(task).to be_cache_valid
         end
       end
     end
@@ -220,6 +223,7 @@
             task = reading_plan.tasks.first
             expect(task.taskings.first.role).to eq teacher_student_role
             expect(task.opens_at).to be_within(1e-6).of(reading_tasking_plan.opens_at)
+            expect(task).to be_cache_valid
           end
 
           it 'does not save plan if it is new' do
@@ -241,6 +245,7 @@
           expect(reading_plan).not_to be_out_to_students
           reading_plan.tasks.each do |task|
             expect(task.opens_at).to be_within(1e-6).of(reading_tasking_plan.opens_at)
+            expect(task).to be_cache_valid
           end
         end
 
@@ -289,6 +294,7 @@
           expect(reading_plan).to be_out_to_students
           reading_plan.tasks.each do |task|
             expect(task.opens_at).to be_within(1e-6).of(reading_tasking_plan.opens_at)
+            expect(task).to be_cache_valid
           end
 
           reading_tasking_plan.update_attribute :due_at, reading_tasking_plan.time_zone.now + 1.hour
@@ -299,6 +305,7 @@
           expect(reading_plan).to be_out_to_students
           reading_plan.tasks.each do |task|
             expect(task.due_at).to be_within(1e-6).of(reading_tasking_plan.due_at)
+            expect(task).to be_cache_valid
           end
         end
 
@@ -331,6 +338,8 @@
         described_class.call(task_plan: reading_plan)
         new_user.roles.each { |role| role.taskings.each { |tasking| tasking.task.really_destroy! } }
         expect(reading_plan.reload).to be_out_to_students
+        reading_plan.update_attribute :updated_by_instructor_at, Time.current
+        reading_plan.tasks.each { |task| expect(task).not_to be_cache_valid }
       end
 
       context 'before the open date' do
@@ -351,6 +360,7 @@
           expect(reading_plan).not_to be_out_to_students
           reading_plan.tasks.each do |task|
             expect(task.opens_at).to be_within(1e-6).of(reading_tasking_plan.opens_at)
+            expect(task).to be_cache_valid
           end
         end
 
@@ -411,6 +421,7 @@
               expect(task.closes_at).to                be_within(1e-6).of(new_closes_at)
               expect(task.auto_grading_feedback_on).to eq gt.auto_grading_feedback_on
               expect(task.manual_grading_feedback_on).to eq gt.manual_grading_feedback_on
+              expect(task).to be_cache_valid
             end
           end
         end
@@ -446,6 +457,7 @@
               expect(task.closes_at).to   be_within(1e-6).of(new_closes_at)
               expect(task.auto_grading_feedback_on).to eq gt.auto_grading_feedback_on
               expect(task.manual_grading_feedback_on).to eq gt.manual_grading_feedback_on
+              expect(task).to be_cache_valid
             end
           end
         end