From 49dd6e6e4633bf5a920c06813368dd606ac9cd84 Mon Sep 17 00:00:00 2001 From: Frank Murphy Date: Tue, 17 Jul 2018 18:40:15 -0400 Subject: [PATCH] Handle concurrent destroy / import of outcome groups Fixes OUT-2336 Test Plan: - Create or import a group with lots of outcomes and groups. - Attempt to import that group into another context. - Immediately open another tab and navigate to the parent group of the import. - Verify that the imported group does not appear currently. - Reload / wait for the import to finish and group to appear. Change-Id: Ic97f86201932ff7751914e4cc2f95290b6e1c8b8 Reviewed-on: https://gerrit.instructure.com/157757 Tested-by: Jenkins Reviewed-by: Michael Brewer-Davis Reviewed-by: Augusto Callejas QA-Review: Matt Berns Product-Review: Sidharth Oberoi --- app/models/learning_outcome_group.rb | 40 ++++++++++++---------- spec/models/learning_outcome_group_spec.rb | 20 ++++++----- 2 files changed, 32 insertions(+), 28 deletions(-) diff --git a/app/models/learning_outcome_group.rb b/app/models/learning_outcome_group.rb index c42bb02f58d..03ca2dcc470 100644 --- a/app/models/learning_outcome_group.rb +++ b/app/models/learning_outcome_group.rb @@ -79,27 +79,29 @@ class LearningOutcomeGroup < ActiveRecord::Base # commit! def add_outcome_group(original, opts={}) # copy group into this group - copy = child_outcome_groups.build - copy.title = original.title - copy.description = original.description - copy.vendor_guid = original.vendor_guid - copy.context = self.context - copy.save! + transaction do + copy = child_outcome_groups.build + copy.title = original.title + copy.description = original.description + copy.vendor_guid = original.vendor_guid + copy.context = self.context + copy.save! - # copy the group contents - original.child_outcome_groups.active.each do |group| - next if opts[:only] && opts[:only][group.asset_string] != "1" - copy.add_outcome_group(group, opts) + # copy the group contents + original.child_outcome_groups.active.each do |group| + next if opts[:only] && opts[:only][group.asset_string] != "1" + copy.add_outcome_group(group, opts) + end + + original.child_outcome_links.active.each do |link| + next if opts[:only] && opts[:only][link.asset_string] != "1" + copy.add_outcome(link.content) + end + + touch_parent_group + # done + copy end - - original.child_outcome_links.active.each do |link| - next if opts[:only] && opts[:only][link.asset_string] != "1" - copy.add_outcome(link.content) - end - - touch_parent_group - # done - copy end # moves an existing outcome link from the same context to be under this diff --git a/spec/models/learning_outcome_group_spec.rb b/spec/models/learning_outcome_group_spec.rb index 9588e35522d..ca65ef92202 100644 --- a/spec/models/learning_outcome_group_spec.rb +++ b/spec/models/learning_outcome_group_spec.rb @@ -138,19 +138,21 @@ describe LearningOutcomeGroup do end describe '#add_outcome_group' do + before :each do + @group1 = @course.learning_outcome_groups.create!(:title => 'group1') + @group2 = @course.learning_outcome_groups.create!(:title => 'group2') + @outcome1 = @course.created_learning_outcomes.create!(:title => 'o1') + @group2.add_outcome(@outcome1) + end + it 'adds a child outcome group and copies all contents' do - group1 = @course.learning_outcome_groups.create!(:title => 'group1') - group2 = @course.learning_outcome_groups.create!(:title => 'group2') - outcome1 = @course.created_learning_outcomes.create!(:title => 'o1') - group2.add_outcome(outcome1) + expect(@group1.child_outcome_groups).to be_empty - expect(group1.child_outcome_groups).to be_empty + child_outcome_group = @group1.add_outcome_group(@group2) - child_outcome_group = group1.add_outcome_group(group2) - - expect(child_outcome_group.title).to eq(group2.title) + expect(child_outcome_group.title).to eq(@group2.title) expect(child_outcome_group.child_outcome_links.map(&:content_id)).to eq( - group2.child_outcome_links.map(&:content_id) + @group2.child_outcome_links.map(&:content_id) ) end end