diff --git a/app/controllers/api/v1/task_plans_controller.rb b/app/controllers/api/v1/task_plans_controller.rb index 6ea5e9bd58..c9ec04aa1f 100644 --- a/app/controllers/api/v1/task_plans_controller.rb +++ b/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? diff --git a/app/subsystems/tasks/get_performance_report.rb b/app/subsystems/tasks/get_performance_report.rb index 64d5f77bf8..6e09679a5e 100644 --- a/app/subsystems/tasks/get_performance_report.rb +++ b/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) diff --git a/app/subsystems/tasks/models/task.rb b/app/subsystems/tasks/models/task.rb index 918f5470fe..9b3423da3c 100644 --- a/app/subsystems/tasks/models/task.rb +++ b/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,7 +587,7 @@ 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? @@ -592,7 +595,7 @@ def started?(use_cache: false) 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? diff --git a/app/subsystems/tasks/models/task_plan.rb b/app/subsystems/tasks/models/task_plan.rb index 1b53d5bbb6..9d0caee2b2 100644 --- a/app/subsystems/tasks/models/task_plan.rb +++ b/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 diff --git a/db/migrate/20210622152149_add_updated_by_instructor_at_to_tasks_task_plans.rb b/db/migrate/20210622152149_add_updated_by_instructor_at_to_tasks_task_plans.rb new file mode 100644 index 0000000000..03969da776 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index d9096554a8..9da58d1e91 100644 --- a/db/schema.rb +++ b/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" diff --git a/spec/routines/distribute_tasks_spec.rb b/spec/routines/distribute_tasks_spec.rb index 2e31c847c5..48b8748b63 100644 --- a/spec/routines/distribute_tasks_spec.rb +++ b/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