Add cache to Outcomes::LearningOutcomeGroupChildren

Updates Outcomes::LearningOutcomeGroupChildren by adding cache
at the queries for getting the total subgroups and outcomes.

closes OUT-4148
flag=improved_outcomes_management

Test plan:
- Create nested learning outcome groups
- For each nested learning outcome group create learning outcomes
> With FF improved_outcomes_management: OFF
- On Rails console: calls to Outcomes::LearningOutcomeGroupChildren
methods should return a default value
- On web: when generating actions over ContentTag, LearningOutcome
and LearningOutcomeGroup it should not lead to clear any cache
> With FF improved_outcomes_management: ON
- On Rails console: call Outcomes::LearningOutcomeGroupChildren
methods for getting total subgroups and outcomes, queries to the
DB should be made (it will need the root context)
- Call again the same methods, it should return the values from
cache
- Create a new instance of the class and call the same methods,
it should return the values from cache
- Clear the cache with `Rails.cache.clear`
- On web (or through GraphiQL) get the total subgroups and total
outcomes multiple times; it should cache the data and should not
run additional queries
- On web: clear the cache by executing the following actions:
  - Add a Learning Outcome Group
  - Adopt a Learning Outcome Group
  - Copy a Learning Outcome Group from global
  - Remove a Learning Outcome Group
  - Add an Outcome
  - Remove an Outcome
- Get the total subgroups and total outcomes, it should run new
queries
- Run the same tests for global context, it should generate and
clear the cache in the same way

Change-Id: I9b0bfc68b84b3e36869d69a926ef84d9989ea96d
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/257257
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Augusto Callejas <acallejas@instructure.com>
QA-Review: Chrystal Langston <chrystal.langston@instructure.com>
Product-Review: Michael Brewer-Davis <mbd@instructure.com>
This commit is contained in:
Pablo Marti-Gomez 2021-01-22 19:03:53 -06:00 committed by Pablo Gomez
parent eca632b70c
commit 4c65f3f583
7 changed files with 319 additions and 41 deletions

View File

@ -63,6 +63,7 @@ class ContentTag < ActiveRecord::Base
after_save :run_due_date_cacher_for_quizzes_next
after_save :clear_discussion_stream_items
after_save :send_items_to_stream
after_save :clear_total_outcomes_cache
after_create :update_outcome_contexts
include CustomValidations
@ -679,4 +680,10 @@ class ContentTag < ActiveRecord::Base
super({:methods => :quiz_lti}.merge(options))
end
def clear_total_outcomes_cache
return unless tag_type == 'learning_outcome_association' && associated_asset_type == 'LearningOutcomeGroup'
clear_context = context_type == 'LearningOutcomeGroup' ? nil : context
Outcomes::LearningOutcomeGroupChildren.new(clear_context).clear_total_outcomes_cache
end
end

View File

@ -295,6 +295,9 @@ class LearningOutcome < ActiveRecord::Base
alias_method :destroy_permanently!, :destroy
def destroy
# delete total_outcomes cache for each active learning_outcome_links contexts
clear_total_outcomes_cache
# delete any remaining links to the outcome. in case of UI, this was
# triggered by ContentTag#destroy and the checks have already run, we don't
# need to do it again. in case of console, we don't care to force the
@ -448,4 +451,27 @@ class LearningOutcome < ActiveRecord::Base
end
new_mastery_type
end
def clear_total_outcomes_cache
return unless improved_outcomes_management?
ContentTag.learning_outcome_links.
active.
distinct.
where(content_id: id).
select(<<-SQL).
root_account_id,
(CASE WHEN context_type='LearningOutcomeGroup' THEN NULL ELSE context_type END) context_type,
(CASE WHEN context_type='LearningOutcomeGroup' THEN NULL ELSE context_id END) context_id
SQL
map do |ct|
Outcomes::LearningOutcomeGroupChildren.new(ct.context).clear_total_outcomes_cache
end
end
def improved_outcomes_management?
return context.root_account.feature_enabled?(:improved_outcomes_management) if context
LoadAccount.default_domain_root_account.feature_enabled?(:improved_outcomes_management)
end
end

View File

@ -32,6 +32,8 @@ class LearningOutcomeGroup < ActiveRecord::Base
belongs_to :context, polymorphic: [:account, :course]
before_save :infer_defaults
after_create :clear_descendants_cache
after_update :clear_descendants_cache, if: -> { clear_descendants_cache? }
resolves_root_account through: -> (group) { group.context_id ? group.context.resolved_root_account_id : 0 }
validates :vendor_guid, length: { maximum: maximum_string_length, allow_nil: true }
validates_length_of :description, :maximum => maximum_text_length, :allow_nil => true, :allow_blank => true
@ -247,6 +249,14 @@ class LearningOutcomeGroup < ActiveRecord::Base
ancestor_ids.member?(id)
end
def clear_descendants_cache
Outcomes::LearningOutcomeGroupChildren.new(context).clear_descendants_cache
end
def clear_descendants_cache?
(previous_changes.keys & %w[learning_outcome_group_id workflow_state]).any?
end
private_class_method def self.title_order_by_clause(table = nil)
col = table ? "#{table}.title" : "title"
best_unicode_collation_key(col)

View File

@ -19,7 +19,7 @@
#
module Outcomes
class LearningOutcomeGroupChildren
class LearningOutcomeGroupChildren
attr_reader :context
def initialize(context = nil)
@ -27,32 +27,50 @@ module Outcomes
end
def total_subgroups(learning_outcome_group_id)
return 0 unless improved_outcomes_management?
children_ids(learning_outcome_group_id).length
end
def total_outcomes(learning_outcome_group_id)
ids = children_ids(learning_outcome_group_id) << learning_outcome_group_id
return 0 unless improved_outcomes_management?
ContentTag.active.learning_outcome_links.
where(associated_asset_id: ids).
joins(:learning_outcome_content).
select(:content_id).
distinct.
count
cache_key = total_outcomes_cache_key(learning_outcome_group_id)
Rails.cache.fetch(cache_key) do
learning_outcome_groups_ids = children_ids(learning_outcome_group_id) << learning_outcome_group_id
ContentTag.active.learning_outcome_links.
where(associated_asset_id: learning_outcome_groups_ids).
joins(:learning_outcome_content).
select(:content_id).
distinct.
count
end
end
def suboutcomes_by_group_id(learning_outcome_group_id)
ids = children_ids(learning_outcome_group_id) << learning_outcome_group_id
return unless improved_outcomes_management?
learning_outcome_groups_ids = children_ids(learning_outcome_group_id) << learning_outcome_group_id
ContentTag.active.learning_outcome_links.
where(associated_asset_id: ids).
where(associated_asset_id: learning_outcome_groups_ids).
joins(:learning_outcome_content).
joins("INNER JOIN #{LearningOutcomeGroup.quoted_table_name} AS logs
on logs.id = content_tags.associated_asset_id").
joins("INNER JOIN #{LearningOutcomeGroup.quoted_table_name} AS logs
ON logs.id = content_tags.associated_asset_id").
order(LearningOutcomeGroup.best_unicode_collation_key('logs.title'),
LearningOutcome.best_unicode_collation_key('short_description'))
end
def clear_descendants_cache
return unless improved_outcomes_management?
Rails.cache.delete(descendants_cache_key)
clear_total_outcomes_cache
end
def clear_total_outcomes_cache
Rails.cache.delete(context_timestamp_cache_key) if improved_outcomes_management?
end
private
def children_ids(learning_outcome_group_id)
@ -61,19 +79,56 @@ module Outcomes
end
def data
@data ||= begin
LearningOutcomeGroup.connection.execute(<<-SQL).as_json
WITH RECURSIVE levels AS (
SELECT id, learning_outcome_group_id AS parent_id
FROM (#{LearningOutcomeGroup.active.where(context: @context).to_sql}) AS data
UNION ALL
SELECT child.id AS id, parent.parent_id AS parent_id
FROM #{LearningOutcomeGroup.quoted_table_name} child
INNER JOIN levels parent ON parent.id = child.learning_outcome_group_id
WHERE child.workflow_state <> 'deleted'
)
SELECT parent_id, array_agg(id) AS descendant_ids FROM levels WHERE parent_id IS NOT NULL GROUP BY parent_id
SQL
Rails.cache.fetch(descendants_cache_key) do
LearningOutcomeGroup.connection.execute(learning_outcome_group_descendants_query).as_json
end
end
def learning_outcome_group_descendants_query
<<-SQL
WITH RECURSIVE levels AS (
SELECT id, learning_outcome_group_id AS parent_id
FROM (#{LearningOutcomeGroup.active.where(context: context).to_sql}) AS data
UNION ALL
SELECT child.id AS id, parent.parent_id AS parent_id
FROM #{LearningOutcomeGroup.quoted_table_name} child
INNER JOIN levels parent ON parent.id = child.learning_outcome_group_id
WHERE child.workflow_state <> 'deleted'
)
SELECT parent_id, array_agg(id) AS descendant_ids FROM levels WHERE parent_id IS NOT NULL GROUP BY parent_id
SQL
end
def context_timestamp_cache
Rails.cache.fetch(context_timestamp_cache_key) do
(Time.zone.now.to_f * 1000).to_i
end
end
def descendants_cache_key
['learning_outcome_group_descendants', context_asset_string].cache_key
end
def total_outcomes_cache_key(learning_outcome_group_id = nil)
['learning_outcome_group_total_outcomes',
context_asset_string,
context_timestamp_cache,
learning_outcome_group_id].cache_key
end
def context_timestamp_cache_key
['learning_outcome_group_context_timestamp', context_asset_string].cache_key
end
def context_asset_string
(context || LearningOutcomeGroup.global_root_outcome_group).global_asset_string
end
def improved_outcomes_management?
@improved_outcomes_management ||= begin
return context.root_account.feature_enabled?(:improved_outcomes_management) if context
LoadAccount.default_domain_root_account.feature_enabled?(:improved_outcomes_management)
end
end
end

View File

@ -42,6 +42,7 @@ describe Types::LearningOutcomeGroupType do
@user = @admin
@outcome1 = outcome_model(context: Account.default, outcome_group: @outcome_group, short_description: "BBBB")
@outcome2 = outcome_model(context: Account.default, outcome_group: @outcome_group, short_description: "AAAA")
Account.default.enable_feature! :improved_outcomes_management
end
let(:outcome_group_type) { GraphQLTypeTester.new(@outcome_group, current_user: @user) }

View File

@ -39,13 +39,14 @@ RSpec.describe Outcomes::Import do
attr_reader :context
end
let_once(:context) { account_model }
let_once(:course) { course_model(account: context) }
let_once(:root_account) { account_model }
let_once(:course) { course_model(account: root_account) }
let_once(:other_context) { account_model }
let_once(:parent1) { outcome_group_model(context: context, vendor_guid: 'parent1') }
let_once(:parent2) { outcome_group_model(context: context, vendor_guid: 'parent2') }
let_once(:outcome_vendor_guid) { 'imanoutcome' }
let_once(:group_vendor_guid) { 'imagroup' }
let(:context) { root_account }
let(:parent1) { outcome_group_model(context: context, vendor_guid: 'parent1') }
let(:parent2) { outcome_group_model(context: context, vendor_guid: 'parent2') }
let(:group_attributes) do
{
title: "i'm a group",
@ -289,7 +290,7 @@ RSpec.describe Outcomes::Import do
expect do
importer.import_outcome(
**existing_outcome.slice(:title, :description, :display_name,
:workflow_state, :calculation_method, :calculation_int).symbolize_keys,
:workflow_state, :calculation_method, :calculation_int).symbolize_keys,
vendor_guid: magic_guid
)
existing_outcome.reload
@ -428,12 +429,12 @@ RSpec.describe Outcomes::Import do
end
context 'with outcomes from other contexts' do
let(:parent_context) { account_model }
let(:subaccount) { root_account.sub_accounts.create! }
let(:context) { subaccount }
before do
context.update! parent_account: parent_context
LearningOutcomeGroup.where(root_account: context).update_all(root_account_id: parent_context.id) if parent_context
existing_outcome.update! context: parent_context
parent1.update! context: subaccount
parent2.update! context: subaccount
end
it 'does not assign parents when attributes are changed' do
@ -457,7 +458,9 @@ RSpec.describe Outcomes::Import do
end
context 'with global context' do
let(:parent_context) { nil }
before do
existing_outcome.update! context: nil
end
it 'does not assign parents when attributes are changed' do
expect do

View File

@ -26,6 +26,7 @@ describe Outcomes::LearningOutcomeGroupChildren do
# rubocop:disable RSpec/LetSetup
let!(:context) { Account.default }
let!(:global_group) { LearningOutcomeGroup.create(title: 'global') }
let!(:global_group_subgroup) { global_group.child_outcome_groups.build(title: 'global subgroup') }
let!(:global_outcome1) { outcome_model(outcome_group: global_group, title: 'G Outcome 1') }
let!(:global_outcome2) { outcome_model(outcome_group: global_group, title: 'G Outcome 2') }
let!(:g0) { context.root_outcome_group }
@ -75,9 +76,9 @@ describe Outcomes::LearningOutcomeGroupChildren do
# g5: Group 3
# o8: Outcome 6
before do
Rails.cache.clear
context.root_account.enable_feature! :improved_outcomes_management
end
describe '#total_subgroups' do
@ -106,10 +107,19 @@ describe Outcomes::LearningOutcomeGroupChildren do
end
context 'when context is nil' do
subject { described_class.new(nil) }
subject { described_class.new }
it 'returns global outcome groups' do
expect(subject.total_subgroups(global_group.id)).to eq 0
expect(subject.total_subgroups(global_group.id)).to eq 1
end
end
it 'caches the total subgroups' do
enable_cache do
expect(LearningOutcomeGroup.connection).to receive(:execute).and_call_original.once
expect(subject.total_subgroups(g0.id)).to eq 6
expect(subject.total_subgroups(g0.id)).to eq 6
expect(described_class.new(context).total_subgroups(g0.id)).to eq 6
end
end
end
@ -125,10 +135,19 @@ describe Outcomes::LearningOutcomeGroupChildren do
expect(subject.total_outcomes(g6.id)).to eq 3
end
it 'caches the total outcomes' do
enable_cache do
expect(ContentTag).to receive(:active).and_call_original.once
expect(subject.total_outcomes(g0.id)).to eq 12
expect(subject.total_outcomes(g0.id)).to eq 12
expect(described_class.new(context).total_outcomes(g0.id)).to eq 12
end
end
context 'when outcome is deleted' do
before { o4.destroy }
it 'returns the total sugroups for a learning outcome group without the deleted groups' do
it 'returns the total outcomes for a learning outcome group without the deleted outcomes' do
expect(subject.total_outcomes(g0.id)).to eq 11
expect(subject.total_outcomes(g1.id)).to eq 8
expect(subject.total_outcomes(g2.id)).to eq 2
@ -140,7 +159,7 @@ describe Outcomes::LearningOutcomeGroupChildren do
end
context 'when context is nil' do
subject { described_class.new(nil) }
subject { described_class.new }
it 'returns global outcomes' do
expect(subject.total_outcomes(global_group.id)).to eq 2
@ -211,7 +230,7 @@ describe Outcomes::LearningOutcomeGroupChildren do
end
context 'when context is nil' do
subject { described_class.new(nil) }
subject { described_class.new }
it 'returns global outcomes' do
outcomes = subject.suboutcomes_by_group_id(global_group.id).map(&:learning_outcome_content).map(&:short_description)
@ -219,4 +238,161 @@ describe Outcomes::LearningOutcomeGroupChildren do
end
end
end
describe '#clear_descendants_cache' do
it 'clears the cache' do
enable_cache do
expect(LearningOutcomeGroup.connection).to receive(:execute).and_call_original.twice
expect(ContentTag).to receive(:active).and_call_original.exactly(4).times
expect(subject.total_subgroups(g0.id)).to eq 6
expect(subject.total_outcomes(g0.id)).to eq 12
expect(subject.total_outcomes(g1.id)).to eq 9
subject.clear_descendants_cache
instance = described_class.new(context)
expect(instance.total_subgroups(g0.id)).to eq 6
expect(instance.total_outcomes(g0.id)).to eq 12
expect(instance.total_outcomes(g1.id)).to eq 9
end
end
end
describe '#clear_total_outcomes_cache' do
it 'clears the cache' do
enable_cache do
expect(ContentTag).to receive(:active).and_call_original.twice
expect(subject.total_outcomes(g0.id)).to eq 12
subject.clear_total_outcomes_cache
instance = described_class.new(context)
expect(instance.total_outcomes(g0.id)).to eq 12
end
end
end
context 'learning outcome groups and learning outcomes events' do
context 'when a group is destroyed' do
it 'clears the cache' do
enable_cache do
expect(subject.total_subgroups(g0.id)).to eq 6
g6.destroy
expect(described_class.new(context).total_subgroups(g0.id)).to eq 5
end
end
end
context 'when a group is added' do
it 'clears the cache' do
enable_cache do
expect(subject.total_subgroups(g0.id)).to eq 6
outcome_group_model(context: context, outcome_group_id: g0)
expect(described_class.new(context).total_subgroups(g0.id)).to eq 7
end
end
context 'when a global group is added' do
it 'clears the cache for total_subgroups and total_outcomes' do
enable_cache do
expect(subject.total_subgroups(g0.id)).to eq 6
expect(subject.total_outcomes(g0.id)).to eq 12
g0.add_outcome_group(global_group)
expect(described_class.new(context).total_subgroups(g0.id)).to eq 8
expect(described_class.new(context).total_outcomes(g0.id)).to eq 14
end
end
end
end
context 'when a group is adopted' do
it 'clears the cache' do
enable_cache do
expect(subject.total_subgroups(g0.id)).to eq 6
outcome_group = outcome_group_model(context: context)
g1.adopt_outcome_group(outcome_group)
expect(described_class.new(context).total_subgroups(g0.id)).to eq 7
end
end
end
context 'when a group is edited' do
it 'does not clear the cache' do
enable_cache do
# rubocop:disable RSpec/AnyInstance
expect_any_instance_of(Outcomes::LearningOutcomeGroupChildren).not_to receive(:clear_descendants_cache)
# rubocop:enable RSpec/AnyInstance
expect(subject.total_subgroups(g0.id)).to eq 6
g1.update(title: 'title edited')
expect(described_class.new(context).total_subgroups(g0.id)).to eq 6
end
end
end
context 'when an outcome is added' do
it 'clears the cache' do
enable_cache do
expect(subject.total_outcomes(g1.id)).to eq 9
outcome = LearningOutcome.create!(title: 'test outcome', context: context)
g1.add_outcome(outcome)
expect(described_class.new(context).total_outcomes(g1.id)).to eq 10
end
end
end
context 'when an outcome is destroyed' do
it 'clears the cache' do
enable_cache do
outcome = LearningOutcome.create!(title: 'test outcome', context: context)
g1.add_outcome(outcome)
expect(described_class.new(context).total_outcomes(g1.id)).to eq 10
outcome.destroy
expect(described_class.new(context).total_outcomes(g1.id)).to eq 9
end
end
context 'when the outcome belongs to a global group' do
it 'clears the cache' do
enable_cache do
expect(described_class.new.total_outcomes(global_group.id)).to eq 2
global_outcome1.destroy
expect(described_class.new.total_outcomes(global_group.id)).to eq 1
end
end
end
context 'when the outcome belongs to different contexts' do
it 'clears the cache on each context' do
enable_cache do
g1.add_outcome(global_outcome1)
expect(described_class.new(context).total_outcomes(g1.id)).to eq 10
expect(described_class.new.total_outcomes(global_group.id)).to eq 2
global_outcome1.destroy
expect(described_class.new(context).total_outcomes(g1.id)).to eq 9
expect(described_class.new.total_outcomes(global_group.id)).to eq 1
end
end
end
end
context 'when a child_outcome_link is destroyed' do
it 'clears the cache' do
enable_cache do
outcome = LearningOutcome.create!(title: 'test outcome', context: context)
child_outcome_link = g1.add_outcome(outcome)
expect(described_class.new(context).total_outcomes(g1.id)).to eq 10
child_outcome_link.destroy
expect(described_class.new(context).total_outcomes(g1.id)).to eq 9
end
end
context 'when the child_outcome_link belongs to global learning outcome group' do
it 'clears the cache' do
enable_cache do
outcome = LearningOutcome.create!(title: 'test outcome')
child_outcome_link = global_group.add_outcome(outcome)
expect(described_class.new.total_outcomes(global_group.id)).to eq 3
child_outcome_link.destroy
expect(described_class.new.total_outcomes(global_group.id)).to eq 2
end
end
end
end
end
end