handle repeated qti imports of the same filename better, closes #4411

Change-Id: I61d67d5005b9073e5c937611014f0929e45562d1
Reviewed-on: https://gerrit.instructure.com/3367
Tested-by: Hudson <hudson@instructure.com>
Reviewed-by: Zach Wily <zach@instructure.com>

app/models/attachment.rb

@@ -982,4 +982,28 @@ class Attachment < ActiveRecord::Base
       attachment.resubmit_to_scribd!
     end
   end
+
+  # returns filename, if it's already unique, or returns a modified version of
+  # filename that makes it unique. you can either pass existing_files as string
+  # filenames, in which case it'll test against those, or a block that'll be
+  # called repeatedly with a filename until it returns true.
+  def self.make_unique_filename(filename, existing_files = [], &block)
+    unless block
+      block = proc { |fname| !existing_files.include?(fname) }
+    end
+
+    return filename if block.call(filename)
+
+    new_name = filename
+    addition = 1
+    dir = File.dirname(filename)
+    dir = dir == "." ? "" : "#{dir}/"
+    extname = File.extname(filename)
+    basename = File.basename(filename, extname)
+
+    until block.call(new_name = "#{dir}#{basename}-#{addition}#{extname}")
+      addition += 1
+    end
+    new_name
+  end
 end

app/models/course.rb

@@ -1225,7 +1225,7 @@ 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'] }
+      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']
         if params[:copy][:files]
@@ -1248,7 +1248,20 @@ class Course < ActiveRecord::Base
           migration.fast_update_progress((current.to_f/total) * 20.0)
         end
       end
-      unzipper = UnzipAttachment.new(:course => migration.context, :filename => data['all_files_export']['file_path'], :valid_paths => valid_paths, :callback => callback, :logger => logger)
+      unzip_opts = {
+        :course => migration.context,
+        :filename => data['all_files_export']['file_path'],
+        :valid_paths => valid_paths,
+        :callback => callback,
+        :logger => logger,
+        :rename_files => migration.migration_settings[:files_import_allow_rename],
+        :migration_id_map => self.attachment_path_id_lookup,
+      }
+      if root_path = migration.migration_settings[:files_import_root_path]
+        unzip_opts[:root_directory] = Folder.assert_path(
+          root_path, migration.context)
+      end
+      unzipper = UnzipAttachment.new(unzip_opts)
       migration.fast_update_progress(1.0)
       unzipper.process
     end

lib/content_zipper.rb

@@ -304,17 +304,7 @@ class ContentZipper
     # we allow duplicate filenames in the same folder. it's a bit silly, but we
     # have to handle it here or people might not get all their files zipped up.
     @files_in_zip ||= Set.new
-    if @files_in_zip.include?(filename)
-      dir = File.dirname(filename)
-      dir = dir == "." ? "" : "#{dir}/"
-      extname = File.extname(filename)
-      basename = File.basename(filename, extname)
-      addition = 1
-      while @files_in_zip.include?(new_name = "#{dir}#{basename}-#{addition}#{extname}")
-        addition += 1
-      end
-      filename = new_name
-    end
+    filename = Attachment.make_unique_filename(filename, @files_in_zip)
     @files_in_zip << filename
 
     if Attachment.s3_storage?

lib/file_in_context.rb

@@ -39,18 +39,21 @@ class FileInContext
       end
     end
     
-    def attach(context, filename, display_name=nil, folder=nil, explicit_filename=nil)
+    def attach(context, filename, display_name=nil, folder=nil, explicit_filename=nil, allow_rename = false)
       display_name ||= File.split(filename).last
       uploaded_data = ActionController::TestUploadedFile.new(filename, Attachment.mimetype(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) } if folder && explicit_filename
+      if folder && allow_rename
+        # find a unique filename in the folder by appending numbers to the end
+        # total race condition here, by the way
+        atts = context.attachments.active.find_all_by_folder_id(folder.id)
+        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) }
+      end
 
-      # _and_display_name(
-        # folder.id,
-        # display_name
-      # ).try(:destroy) if folder && folder.id && display_name
-      
       @attachment = context.attachments.build(:uploaded_data => uploaded_data, :display_name => display_name, :folder => folder)
       @attachment.write_attribute(:filename, explicit_filename) if explicit_filename
       @attachment.context = context

lib/unzip_attachment.rb

@@ -48,6 +48,8 @@ class UnzipAttachment
     @tick_callback = opts[:callback]
     @valid_paths = opts[:valid_paths]
     @logger ||= opts[:logger]
+    @rename_files = !!opts[:rename_files]
+    @migration_id_map = opts[:migration_id_map] || {}
 
     raise ArgumentError, "Must provide a course." unless self.course and self.course.is_a?(Course)
     raise ArgumentError, "Must provide a filename." unless self.filename
@@ -136,11 +138,14 @@ class UnzipAttachment
         # This is where the attachment actually happens.  See file_in_context.rb
         attachment = nil
         begin
-          attachment = FileInContext.attach(self.course, path, display_name(entry.name), folder, File.split(entry.name).last)
+          attachment = FileInContext.attach(self.course, path, display_name(entry.name), folder, File.split(entry.name).last, @rename_files)
         rescue
-          attachment = FileInContext.attach(self.course, path, display_name(entry.name), folder, File.split(entry.name).last)
+          attachment = FileInContext.attach(self.course, path, display_name(entry.name), folder, File.split(entry.name).last, @rename_files)
         end
         id_positions[attachment.id] = path_positions[entry.name]
+        if migration_id = @migration_id_map[entry.name]
+          attachment.update_attribute(:migration_id, migration_id)
+        end
         @attachments << attachment if attachment
       rescue => e
         @logger.warn "Couldn't unzip archived file #{path}: #{e.message}" if @logger

spec/models/attachment_spec.rb

@@ -436,6 +436,19 @@ describe Attachment do
     end
   end
 
+  describe "make_unique_filename" do
+    it "should find a unique name for files" do
+      existing_files = %w(a.txt b.txt c.txt)
+      Attachment.make_unique_filename("d.txt", existing_files).should == "d.txt"
+      existing_files.should_not be_include(Attachment.make_unique_filename("b.txt", existing_files))
+
+      existing_files = %w(/a/b/a.txt /a/b/b.txt /a/b/c.txt)
+      Attachment.make_unique_filename("/a/b/d.txt", existing_files).should == "/a/b/d.txt"
+      new_name = Attachment.make_unique_filename("/a/b/b.txt", existing_files)
+      existing_files.should_not be_include(new_name)
+      new_name.should match(%r{^/a/b/b[^.]+\.txt})
+    end
+  end
 end
 
 def processing_model

vendor/plugins/respondus_soap_endpoint/lib/urn:RespondusAPIServant.rb

@@ -452,6 +452,8 @@ Implemented for: Canvas LMS}]
       :migration_type => 'qti_exporter',
       :apply_respondus_settings_file => (itemType != 'qdb'),
       :skip_import_notification => true,
+      :files_import_allow_rename => true,
+      :files_import_root_path => "imported qti files",
     }
 
     if item