Skip to content

Commit f406d0b

Browse files
committed
n_strand migrations and fix page error for old course copies
all migrations are put on an n_strand for the course's root account now. This is to prevent a school copying all its course at a semester boundary from starving other jobs. Also, some old content migration objects don't have the migration type set which causes a page error in the new UI Test Plan: * in the /jobs UI a migration should have a strand name like migrations:import_content:#{account_id} * Course copies from new migration UI and through course settings should work and not cause page errors when the new UI is refreshed * Also removed the question mark for the checkbox labeled "Select migration content" closes CNVS-6951 Change-Id: Ifbecbc49c40b35c3212e39e92b753c14e499ed6c Reviewed-on: https://gerrit.instructure.com/22427 QA-Review: August Thornton <august@instructure.com> Tested-by: Jenkins <jenkins@instructure.com> Reviewed-by: James Williams <jamesw@instructure.com> Product-Review: Bracken Mosbacker <bracken@instructure.com>
1 parent 268ec51 commit f406d0b

11 files changed

Lines changed: 40 additions & 19 deletions

File tree

app/controllers/content_imports_controller.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,8 +126,8 @@ def copy_course_content
126126
:source_course => @source_course,
127127
:copy_options => copy_params,
128128
:migration_type => 'course_copy_importer')
129+
cm.queue_migration
129130
cm.workflow_state = 'created'
130-
cm.copy_course
131131
render :json => copy_status_json(cm, @context, @current_user, session)
132132
end
133133
end

app/models/content_migration.rb

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,10 @@ def strand=(s)
124124
def strand
125125
migration_settings[:strand]
126126
end
127+
128+
def n_strand
129+
["migrations:import_content", self.root_account.try(:global_id) || "global"]
130+
end
127131

128132
def migration_ids_to_import=(val)
129133
migration_settings[:migration_ids_to_import] = val
@@ -323,17 +327,25 @@ def queue_migration
323327
check_quiz_id_prepender
324328
plugin = Canvas::Plugin.find(migration_type)
325329
if plugin
330+
queue_opts = {:priority => Delayed::LOW_PRIORITY, :max_attempts => 1}
331+
if self.strand
332+
queue_opts[:strand] = self.strand
333+
else
334+
queue_opts[:n_strand] = self.n_strand
335+
end
336+
326337
if self.workflow_state == 'exported' && !plugin.settings[:skip_conversion_step]
327338
# it's ready to be imported
328339
self.workflow_state = :importing
329340
self.save
330-
import_content
341+
self.send_later_enqueue_args(:import_content, queue_opts)
331342
else
332343
# find worker and queue for conversion
333344
begin
334345
if Canvas::Migration::Worker.const_defined?(plugin.settings['worker'])
335346
self.workflow_state = :exporting
336-
job = Canvas::Migration::Worker.const_get(plugin.settings['worker']).enqueue(self)
347+
worker_class = Canvas::Migration::Worker.const_get(plugin.settings['worker'])
348+
job = Delayed::Job.enqueue(worker_class.new(self.id), queue_opts)
337349
self.save
338350
job
339351
else
@@ -429,7 +441,7 @@ def import_content
429441
clear_migration_data
430442
end
431443
end
432-
handle_asynchronously :import_content, :priority => Delayed::LOW_PRIORITY, :max_attempts => 1
444+
alias_method :import_content_without_send_later, :import_content
433445

434446
def prepare_data(data)
435447
data = data.with_indifferent_access if data.is_a? Hash
@@ -461,12 +473,6 @@ def date_shift_options
461473
self.migration_settings[:date_shift_options]
462474
end
463475

464-
def copy_course
465-
worker = Canvas::Migration::Worker::CourseCopyWorker.new
466-
worker.perform(self)
467-
end
468-
handle_asynchronously :copy_course, :priority => Delayed::LOW_PRIORITY, :max_attempts => 1
469-
470476
scope :for_context, lambda { |context| where(:context_id => context, :context_type => context.class.to_s) }
471477

472478
scope :successful, where(:workflow_state => 'imported')

app/views/jst/content_migrations/ProgressingIssues.handlebars

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
<ul class="unstyled well collectionViewItems"></ul>
33
{{/if}}
44

5-
<div class="paginatedLoadingIndicator">Loading</div>
5+
<div class="paginatedLoadingIndicator">{{#t "loading"}}Loading{{/t}}</div>
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
<label class="checkbox" for="selectContentCheckbox">
2-
<input id="selectContentCheckbox" type="checkbox" />{{#t "select_migration_label"}}Select migration content?{{/t}}
2+
<input id="selectContentCheckbox" type="checkbox" />{{#t "select_migration_label"}}Select migration content{{/t}}
33
</label>

lib/api/v1/content_migration.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ def content_migration_json(migration, current_user, session, attachment_prefligh
4444
json[:migration_issues_count] = migration.migration_issues.count
4545
if attachment_preflight
4646
json[:pre_attachment] = attachment_preflight
47-
elsif migration.attachment && migration.migration_type != 'course_copy_importer'
47+
elsif migration.attachment && !migration.for_course_copy?
4848
json[:attachment] = attachment_json(migration.attachment, current_user, {}, {:can_manage_files => true})
4949
end
5050
if migration.job_progress

lib/canvas/migration/worker/course_copy_worker.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ def perform(cm=nil)
6262
cm.context.copy_attachments_from_course(source, :content_export => ce, :content_migration => cm)
6363
cm.update_import_progress(20)
6464

65-
cm.import_content_without_send_later
65+
cm.import_content
6666
cm.workflow_state = :imported
6767
cm.save
6868
cm.update_import_progress(100)

lib/cc/importer/cc_worker.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ def perform
5353
cm.update_conversion_progress(100)
5454

5555
if cm.import_immediately?
56-
cm.import_content_without_send_later
56+
cm.import_content
5757
cm.update_import_progress(100)
5858
saved = cm.save
5959
if converter.respond_to?(:post_process)

spec/apis/v1/content_migrations_api_spec.rb

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,20 @@
101101
course_with_student_logged_in(:course => @course, :active_all => true)
102102
api_call(:get, @migration_url, @params, {}, {}, :expected_status => 401)
103103
end
104+
105+
it "should not return attachment for course copies course copies" do
106+
@migration.migration_type = nil
107+
@migration.source_course_id = @course.id
108+
@attachment = Attachment.create!(:context => @migration, :filename => "test.zip", :uploaded_data => StringIO.new("test file"))
109+
@attachment.file_state = "deleted"
110+
@attachment.workflow_state = "unattached"
111+
@attachment.save
112+
@migration.attachment = @attachment
113+
@migration.save!
114+
115+
json = api_call(:get, @migration_url, @params)
116+
json["attachment"].should be_nil
117+
end
104118
end
105119

106120
describe 'create' do

spec/apis/v1/courses_api_spec.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1796,8 +1796,8 @@ def run_copy(to_id=nil, from_id=nil, options={})
17961796

17971797
api_call(:get, status_url, { :controller => 'content_imports', :action => 'copy_course_status', :course_id => @copy_to.to_param, :id => data['id'].to_param, :format => 'json' })
17981798
(JSON.parse(response.body)).tap do |res|
1799-
res['workflow_state'].should == 'created'
1800-
res['progress'].should be_nil
1799+
res['workflow_state'].should == 'started'
1800+
res['progress'].should == 0
18011801
end
18021802

18031803
run_jobs

spec/models/content_migration_spec.rb

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,8 @@
6363
end
6464

6565
def run_course_copy(warnings=[])
66-
@cm.copy_course_without_send_later
66+
worker = Canvas::Migration::Worker::CourseCopyWorker.new
67+
worker.perform(@cm)
6768
@cm.reload
6869
if @cm.migration_settings[:last_error]
6970
er = ErrorReport.last

0 commit comments

Comments
 (0)