ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

prevent new migrations from rewriting attachment migration ids 

test plan:
* have a blueprint course with a file
* sync to an associated course
* export the blueprint course
* import the export file directly
  into the associated course
* the associated course should have two files
* make a page in the blueprint with a link
 to the file
* syncing to the associated course should
create a page that still has a valid file link

* create a new course with a file
* copy the course into the blueprint
* sync the copied file to the associated course
* now copy the new course again into the
 associated course
* the associated course should have two files
* create another page in the blueprint referencing
 the copied file
* syncing to the associated course should
create a page that still has a valid file link

closes #LS-1496

Change-Id: I0549dd4b36c3235654b98f6cc825597b91878c2c
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/248481
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
QA-Review: Robin Kuss <rkuss@instructure.com>
Product-Review: James Williams <jamesw@instructure.com>
This commit is contained in:
James Williams 2020-09-24 13:26:13 -06:00
parent 4c7ab8e6e8
commit 296717e708
8 changed files with 95 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/models/assignment.rb
View File

@ -2363,7 +2363,7 @@ class Assignment < ActiveRecord::Base
files_for_user.each do |user, files|
attachments = files.map do |g|
FileInContext.attach(self, g[:filename], g[:display_name])
FileInContext.attach(self, g[:filename], display_name: g[:display_name])
end
comment_attr = {

5
app/models/attachment.rb
View File

@ -300,6 +300,7 @@ class Attachment < ActiveRecord::Base
existing = context.attachments.active.find_by_id(self)
options[:cloned_item_id] ||= self.cloned_item_id
options[:migration_id] ||= CC::CCHelper.create_key(self)
existing ||= Attachment.find_existing_attachment_for_clone(context, options.merge(:active_only => true))
return existing if existing && !options[:overwrite] && !options[:force_copy]
existing ||= Attachment.find_existing_attachment_for_clone(context, options)
@ -331,7 +332,7 @@ class Attachment < ActiveRecord::Base
end
end
dup.write_attribute(:filename, self.filename) unless dup.read_attribute(:filename) || dup.root_attachment_id?
dup.migration_id = options[:migration_id] || CC::CCHelper.create_key(self)
dup.migration_id = options[:migration_id]
dup.mark_as_importing!(options[:migration]) if options[:migration]
if context.respond_to?(:log_merge_result)
context.log_merge_result("File \"#{dup.folder && dup.folder.full_name}/#{dup.display_name}\" created")
@ -355,7 +356,7 @@ class Attachment < ActiveRecord::Base
if options[:migration_id] && options[:match_on_migration_id]
scope.where(migration_id: options[:migration_id]).first
elsif options[:cloned_item_id]
scope.where(cloned_item_id: options[:cloned_item_id]).first
scope.where(cloned_item_id: options[:cloned_item_id]).where(migration_id: [nil, options[:migration_id]]).first
end
end

2
app/models/importers/link_parser.rb
View File

@ -171,7 +171,7 @@ module Importers
file.binmode
file.write(image_data)
file.close
attachment = FileInContext.attach(context, file.path, filename, @folder, filename, false, md5)
attachment = FileInContext.attach(context, file.path, display_name: filename, folder: @folder, explicit_filename: filename, md5: md5)
resolved("#{context_path}/files/#{attachment.id}/preview")
rescue
unresolved(:file, rel_path: "#{folder_name}/#{filename}")

19
lib/file_in_context.rb
View File

@ -40,11 +40,23 @@ class FileInContext
end
end
def attach(context, filename, display_name=nil, folder=nil, explicit_filename=nil, allow_rename = false, md5=nil)
def attach(context, filename, display_name: nil, folder: nil, explicit_filename: nil, allow_rename: false, md5: nil, migration_id: nil)
display_name ||= File.split(filename).last
if md5 && folder && !allow_rename
existing_att = context.attachments.where(:display_name => display_name, :folder => folder, :md5 => md5).not_deleted.first
return existing_att if existing_att
scope = context.attachments.where(:display_name => display_name, :folder => folder, :md5 => md5).not_deleted
if migration_id
scope = scope.where(:migration_id => [migration_id, nil]) # either find a previous copy or an unassociated match
end
existing_att = scope.take
if existing_att
if migration_id && existing_att.migration_id.nil? # can set an existing unassociated attachment to the new migration_id
existing_att.update_attribute(:migration_id, migration_id)
end
return existing_att
elsif migration_id
allow_rename = true # prevent overwriting if there's an existing matching filename that has a different migration_id
end
end
uploaded_data = Rack::Test::UploadedFile.new(filename, Attachment.mimetype(explicit_filename || filename))
@ -52,6 +64,7 @@ class FileInContext
@attachment = Attachment.new(:context => context, :display_name => display_name, :folder => folder)
Attachments::Storage.store_for_attachment(@attachment, uploaded_data)
@attachment.filename = explicit_filename if explicit_filename
@attachment.migration_id = migration_id
@attachment.set_publish_state_for_usage_rights
@attachment.save!

14
lib/unzip_attachment.rb
View File

@ -118,11 +118,9 @@ class UnzipAttachment
end
zip_stats.charge_quota(file_size)
# This is where the attachment actually happens. See file_in_context.rb
attachment = attach(f.path, entry, folder, md5)
migration_id = @migration_id_map[entry.name]
attachment = attach(f.path, entry, folder, md5, migration_id: migration_id)
id_positions[attachment.id] = path_positions[entry.name]
if migration_id = @migration_id_map[entry.name]
attachment.update_attribute(:migration_id, migration_id)
end
rescue Attachment::OverQuotaError
f.unlink
raise
@ -153,11 +151,13 @@ class UnzipAttachment
end
end
def attach(path, entry, folder, md5)
def attach(path, entry, folder, md5, migration_id: nil)
begin
FileInContext.attach(self.context, path, display_name(entry.name), folder, File.split(entry.name).last, @rename_files, md5)
FileInContext.attach(self.context, path, display_name: display_name(entry.name), folder: folder,
explicit_filename: File.split(entry.name).last, allow_rename: @rename_files, md5: md5, migration_id: migration_id)
rescue
FileInContext.attach(self.context, path, display_name(entry.name), folder, File.split(entry.name).last, @rename_files, md5)
FileInContext.attach(self.context, path, display_name: display_name(entry.name), folder: folder,
explicit_filename: File.split(entry.name).last, allow_rename: @rename_files, md5: md5, migration_id: migration_id)
end
end

BIN
spec/fixtures/migration/canvas_attachment.zip vendored Normal file
View File

Binary file not shown.

2
spec/lib/file_in_context_spec.rb
View File

@ -39,7 +39,7 @@ describe FileInContext do
unbound_method = Attachment.instance_method(:filename=)
class Attachment; def filename=(new_name); write_attribute :filename, sanitize_filename(new_name); end; end
filename = File.expand_path(File.join(File.dirname(__FILE__), %w(.. fixtures files escaping_test[0].txt)))
attachment = FileInContext.attach(@course, filename, nil, @folder)
attachment = FileInContext.attach(@course, filename, folder: @folder)
expect(attachment.filename).to eq 'escaping_test%5B0%5D.txt'
expect(attachment).to be_published
Attachment.send(:define_method, :filename=, unbound_method)

66
spec/models/master_courses/master_migration_spec.rb
View File

@ -2490,6 +2490,72 @@ describe MasterCourses::MasterMigration do
expect(sub.reload.cached_due_date.to_i).to eq due_at.to_i
end
context "attachment migration id preservation" do
def run_course_copy(copy_from, copy_to)
@cm = ContentMigration.new(:context => copy_to, :user => @user, :source_course => copy_from,
:migration_type => 'course_copy_importer', :copy_options => {:everything => "1"})
@cm.migration_settings[:import_immediately] = true
@cm.set_default_settings
@cm.save!
worker = Canvas::Migration::Worker::CourseCopyWorker.new
worker.perform(@cm)
end
it "should not overwrite blueprint attachment migration ids from other course copies" do
att = Attachment.create!(:filename => 'first.txt', :uploaded_data => StringIO.new('ohai'), :folder => Folder.unfiled_folder(@copy_from), :context => @copy_from)
@copy_to = course_factory(:active_all => true)
@sub = @template.add_child_course!(@copy_to)
run_master_migration
att_to = @copy_to.attachments.where(:migration_id => mig_id(att)).take
@other_copy_from = course_factory(:active_all => true)
run_course_copy(@copy_from, @other_copy_from)
impostor_att = @other_copy_from.attachments.last
run_course_copy(@other_copy_from, @copy_to)
expect(att_to.reload.migration_id).to eq mig_id(att) # should not have changed
impostor_att_to = @copy_to.attachments.where(:migration_id => CC::CCHelper.create_key(impostor_att, global: true)).first
expect(impostor_att_to.id).to_not eq att_to.id # should make a copy
end
def import_package(course)
cm = ContentMigration.create!(context: course, user: @user)
cm.migration_type = 'canvas_cartridge_importer'
cm.migration_settings['import_immediately'] = true
cm.save!
package_path = File.join(File.dirname(__FILE__) + "/../../fixtures/migration/canvas_attachment.zip")
attachment = Attachment.create!(:context => cm, :uploaded_data => File.open(package_path, 'rb'), :filename => 'file.zip')
cm.attachment = attachment
cm.save!
cm.queue_migration
run_jobs
end
it "should not overwrite blueprint attachment migration ids from other canvas package imports" do
import_package(@copy_from)
att = @copy_from.attachments.first
course_factory(:active_all => true)
@copy_to = @course
@sub = @template.add_child_course!(@copy_to)
run_master_migration
att_to = @copy_to.attachments.where(:migration_id => mig_id(att)).take
import_package(@copy_to)
expect(att_to.reload.migration_id).to eq mig_id(att) # should not have changed
impostor_att_to = @copy_to.attachments.where(:migration_id => att.migration_id).first # package should make a copy
expect(impostor_att_to.id).to_not eq att_to.id
end
end
context "caching" do
specs_require_cache(:redis_cache_store)