diff --git a/app/models/content_tag.rb b/app/models/content_tag.rb index d071c976ebd..6b4471809a3 100644 --- a/app/models/content_tag.rb +++ b/app/models/content_tag.rb @@ -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 diff --git a/app/models/learning_outcome.rb b/app/models/learning_outcome.rb index 88616e7c477..10f2eb6c27c 100644 --- a/app/models/learning_outcome.rb +++ b/app/models/learning_outcome.rb @@ -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 diff --git a/app/models/learning_outcome_group.rb b/app/models/learning_outcome_group.rb index 21141c454fe..7c8c3f3498c 100644 --- a/app/models/learning_outcome_group.rb +++ b/app/models/learning_outcome_group.rb @@ -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) diff --git a/lib/outcomes/learning_outcome_group_children.rb b/lib/outcomes/learning_outcome_group_children.rb index a54c7e2379f..bb1cdb4d941 100644 --- a/lib/outcomes/learning_outcome_group_children.rb +++ b/lib/outcomes/learning_outcome_group_children.rb @@ -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 diff --git a/spec/graphql/types/learning_outcome_group_type_spec.rb b/spec/graphql/types/learning_outcome_group_type_spec.rb index 44b2a20a8e8..8d3f41fd619 100644 --- a/spec/graphql/types/learning_outcome_group_type_spec.rb +++ b/spec/graphql/types/learning_outcome_group_type_spec.rb @@ -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) } diff --git a/spec/lib/outcomes/import_spec.rb b/spec/lib/outcomes/import_spec.rb index bf3f5f45067..1d14cc7a697 100644 --- a/spec/lib/outcomes/import_spec.rb +++ b/spec/lib/outcomes/import_spec.rb @@ -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 diff --git a/spec/lib/outcomes/learning_outcome_group_children_spec.rb b/spec/lib/outcomes/learning_outcome_group_children_spec.rb index 7e6f311976d..16b071eaf14 100644 --- a/spec/lib/outcomes/learning_outcome_group_children_spec.rb +++ b/spec/lib/outcomes/learning_outcome_group_children_spec.rb @@ -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