correctly replace modules and files on content migrations

Files weren't being replaced if they had a filename different
from their display_name.

The content_tags for modules are cleared before they are
imported so that only the items aren't duplicated.

Test Plan:
  * Import a package twice
	* The modules should be correct and not have 2 items for each item
	* The files should also not be duplicated

closes #10405

Change-Id: I3cb15c2530734185675a2b8bb9017bdf6ac35202
Reviewed-on: https://gerrit.instructure.com/13695
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>

app/models/context_module.rb

@@ -673,6 +673,8 @@ class ContextModule < ActiveRecord::Base
       item.prerequisites = preqs if preqs.length > 0
     end
     
+    # Clear the old tags to be replaced by new ones
+    item.content_tags.destroy_all
     item.save!
     
     item_map = {}
@@ -732,7 +734,7 @@ class ContextModule < ActiveRecord::Base
       end
     elsif hash[:linked_resource_type] =~ /page_type|file_type|attachment/i
       # this is a file of some kind
-      file = self.context.attachments.find_by_migration_id(hash[:linked_resource_id]) if hash[:linked_resource_id]
+      file = self.context.attachments.active.find_by_migration_id(hash[:linked_resource_id]) if hash[:linked_resource_id]
       if file
         title = hash[:title] || hash[:linked_resource_title]
         item = self.add_item({

app/models/course.rb

@@ -1700,15 +1700,13 @@ class Course < ActiveRecord::Base
     params = migration.migration_settings[:migration_ids_to_import]
     valid_paths = []
     (data['file_map'] || {}).each do |id, file|
-      if !migration.context.attachments.detect { |f| f.migration_id == file['migration_id'] } || migration.migration_settings[:files_import_allow_rename]
-        path = file['path_name'].starts_with?('/') ? file['path_name'][1..-1] : file['path_name']
-        self.attachment_path_id_lookup[path] = file['migration_id']
-        self.attachment_path_id_lookup_lower[path.downcase] = file['migration_id']
-        if params[:copy][:files]
-          valid_paths << path if (bool_res(params[:copy][:files][file['migration_id'].to_sym]) rescue false)
-        else
-          valid_paths << path
-        end
+      path = file['path_name'].starts_with?('/') ? file['path_name'][1..-1] : file['path_name']
+      self.attachment_path_id_lookup[path] = file['migration_id']
+      self.attachment_path_id_lookup_lower[path.downcase] = file['migration_id']
+      if params[:copy][:files]
+        valid_paths << path if (bool_res(params[:copy][:files][file['migration_id'].to_sym]) rescue false)
+      else
+        valid_paths << path
       end
     end
     valid_paths = [0] if valid_paths.empty? && params[:copy] && params[:copy][:files]

lib/file_in_context.rb

@@ -50,8 +50,8 @@ class FileInContext
         explicit_filename = Attachment.make_unique_filename(explicit_filename) { |fname| !atts.any? { |a| a.filename == fname } }
         display_name = Attachment.make_unique_filename(display_name) { |fname| !atts.any? { |a| a.display_name == fname } }
       elsif folder && explicit_filename
-        # This code will delete any file in the folder that had the same name...
-        context.attachments.active.find_all_by_folder_id(folder.id).select{|a| a.filename == explicit_filename }.each{|a| destroy_file(a) }
+        # Delete any file in the folder that had the same display name so that it is "replaced"
+        context.attachments.active.find_all_by_folder_id(folder.id).select{|a| a.display_name == explicit_filename }.each{|a| destroy_file(a) }
       end
 
       @attachment = context.attachments.build(:uploaded_data => uploaded_data, :display_name => display_name, :folder => folder)

spec/lib/cc/importer/common_cartridge_converter_spec.rb

@@ -180,6 +180,39 @@ describe "Standard Common Cartridge importing" do
       pending("Can't import assessment data with python QTI tool.")
     end
   end
+  
+  context "re-importing the cartridge" do
+    
+    append_before do
+      @migration2 = ContentMigration.create(:context => @course)
+      @migration2.migration_settings[:migration_ids_to_import] = {:copy=>{}}
+      @course.import_from_migration(@course_data, nil, @migration2)
+    end
+    
+    it "should import webcontent" do
+      @course.attachments.count.should == 20
+      @course.attachments.active.count.should == 10
+      mig_ids = %w{I_00001_R I_00006_Media I_media_R f3 f4 f5 8612e3db71e452d5d2952ff64647c0d8 I_00003_R_IMAGERESOURCE 7acb90d1653008e73753aa2cafb16298 6a35b0974f59819404dc86d48fe39fc3}
+      mig_ids.each do |mig_id|
+        atts = @course.attachments.find_all_by_migration_id(mig_id)
+        atts.length.should == 2
+        atts.any?{|a|a.file_state = 'deleted'}.should == true
+        atts.any?{|a|a.file_state = 'available'}.should == true
+      end
+    end
+    
+    it "should point to new attachment from module" do
+      @course.context_modules.count.should == 3
+      
+      mod1 = @course.context_modules.find_by_migration_id("I_00000")
+      mod1.content_tags.active.count.should == (Qti.qti_enabled? ? 5 : 4)
+      mod1.name.should == "Your Mom, Research, & You"
+      tag = mod1.content_tags.active[0]
+      tag.content_type.should == 'Attachment'
+      tag.content_id.should == @course.attachments.active.find_by_migration_id("I_00001_R").id
+      puts mod1.content_tags.active.count
+    end
+  end
 
 end