diff --git a/app/controllers/sis_imports_api_controller.rb b/app/controllers/sis_imports_api_controller.rb index d01d4541003..2f003ab4d10 100644 --- a/app/controllers/sis_imports_api_controller.rb +++ b/app/controllers/sis_imports_api_controller.rb @@ -420,22 +420,28 @@ class SisImportsApiController < ApplicationController batch_mode_term = api_find(@account.enrollment_terms.active, params[:batch_mode_term_id]) end - unless batch_mode_term + unless batch_mode_term || params[:multi_term_batch_mode] return render :json => {:message => "Batch mode specified, but the given batch_mode_term_id cannot be found."}, :status => :bad_request end end batch = SisBatch.create_with_attachment(@account, params[:import_type], file_obj, @current_user) do |batch| batch.change_threshold = params[:change_threshold] + batch.options ||= {} if batch_mode_term batch.batch_mode = true batch.batch_mode_term = batch_mode_term + elsif params[:multi_term_batch_mode] + batch.batch_mode=true + batch.options[:multi_term_batch_mode] = value_to_boolean(params[:multi_term_batch_mode]) + unless batch.change_threshold + return render json: {message: "change_threshold is required to use multi term_batch mode."}, status: :bad_request + end elsif params[:diffing_data_set_identifier].present? batch.enable_diffing(params[:diffing_data_set_identifier], remaster: value_to_boolean(params[:diffing_remaster_data_set])) end - batch.options ||= {} if value_to_boolean(params[:override_sis_stickiness]) batch.options[:override_sis_stickiness] = true [:add_sis_stickiness, :clear_sis_stickiness].each do |option| diff --git a/app/models/sis_batch.rb b/app/models/sis_batch.rb index 8fcc850f275..ffc960522a3 100644 --- a/app/models/sis_batch.rb +++ b/app/models/sis_batch.rb @@ -191,7 +191,7 @@ class SisBatch < ActiveRecord::Base end def batch_aborted(message) - add_errors([message]) + add_errors([[message]]) self.save! raise SisBatch::Aborted end @@ -308,10 +308,23 @@ class SisBatch < ActiveRecord::Base end end + def batch_mode_terms + if self.options[:multi_term_batch_mode] + @terms ||= EnrollmentTerm.where(sis_batch_id: self) + unless @terms.exists? + abort_batch + batch_aborted(t('Terms not found. Terms must be included with multi_term_batch_mode')) + end + @terms + else + self.batch_mode_term + end + end + def term_course_scope if data[:supplied_batches].include?(:course) scope = account.all_courses.active.where.not(sis_batch_id: nil, sis_source_id: nil) - scope.where(enrollment_term_id: self.batch_mode_term) + scope.where(enrollment_term_id: batch_mode_terms) end end @@ -340,7 +353,7 @@ class SisBatch < ActiveRecord::Base def term_sections_scope if data[:supplied_batches].include?(:section) - scope = self.account.course_sections.active.where(courses: {enrollment_term_id: self.batch_mode_term}) + scope = self.account.course_sections.active.where(courses: {enrollment_term_id: batch_mode_terms}) scope = scope.where.not(sis_batch_id: nil, sis_source_id: nil) scope.joins("INNER JOIN #{Course.quoted_table_name} ON courses.id=COALESCE(nonxlist_course_id, course_id)").readonly(false) end @@ -369,7 +382,7 @@ class SisBatch < ActiveRecord::Base def term_enrollments_scope if data[:supplied_batches].include?(:enrollment) scope = self.account.enrollments.active.joins(:course).readonly(false).where.not(sis_batch_id: nil) - scope.where(courses: {enrollment_term_id: self.batch_mode_term}) + scope.where(courses: {enrollment_term_id: batch_mode_terms}) end end @@ -394,17 +407,18 @@ class SisBatch < ActiveRecord::Base def remove_previous_imports # we shouldn't be able to get here without a term, but if we do, skip - return unless self.batch_mode_term + return unless self.batch_mode_term || options[:multi_term_batch_mode] supplied_batches = data[:supplied_batches].dup.keep_if { |i| [:course, :section, :enrollment].include? i } return unless supplied_batches.present? - SisBatch.where(id: self).update_all(workflow_state: 'cleanup_batch') - - count = 0 - courses = non_batch_courses_scope - sections = non_batch_sections_scope - enrollments = non_batch_enrollments_scope - begin + batch_mode_terms if options[:multi_term_batch_mode] + SisBatch.where(id: self).update_all(workflow_state: 'cleanup_batch') + + count = 0 + courses = non_batch_courses_scope + sections = non_batch_sections_scope + enrollments = non_batch_enrollments_scope + count = detect_changes(count, courses, enrollments, sections) row = remove_non_batch_courses(courses, count) if courses row = remove_non_batch_sections(sections, count, row) if sections @@ -449,8 +463,8 @@ class SisBatch < ActiveRecord::Base end def change_detected_message(count, type) - [t("%{count} %{type} would be deleted and exceeds the set threshold of %{change_threshold}%", - count: count, type: type, change_threshold: change_threshold)] + t("%{count} %{type} would be deleted and exceeds the set threshold of %{change_threshold}%", + count: count, type: type, change_threshold: change_threshold) end def as_json(options={}) diff --git a/doc/api/sis_csv.md b/doc/api/sis_csv.md index 65dd1fd4721..6b3a2a518e8 100644 --- a/doc/api/sis_csv.md +++ b/doc/api/sis_csv.md @@ -48,6 +48,15 @@ The change_threshold can be set to any integer between 1 and 100. change_threshold also impacts diffing mode. +Multi Term Batch Mode +--------------------- + +Multi term batch mode is just like batch mode except against multiple terms. +Multi term batch mode is run against all terms included in the same import for +the batch. To use multi term batch mode you must also set a change_threshold. If +you intend to remove all items with multi term batch mode, you can set the +change_threshold to 100. + Diffing Mode ------------ diff --git a/spec/apis/v1/sis_imports_api_spec.rb b/spec/apis/v1/sis_imports_api_spec.rb index bf95d9867d1..779fb64cdfa 100644 --- a/spec/apis/v1/sis_imports_api_spec.rb +++ b/spec/apis/v1/sis_imports_api_spec.rb @@ -248,6 +248,31 @@ describe SisImportsApiController, type: :request do expect(batch.change_threshold).to eq 7 end + it "should requre change threshold for multi_term_batch_mode" do + json = api_call(:post, + "/api/v1/accounts/#{@account.id}/sis_imports.json", + { controller: 'sis_imports_api', action: 'create', + format: 'json', account_id: @account.id.to_s }, + { import_type: 'instructure_csv', + attachment: fixture_file_upload("files/sis/test_user_1.csv", 'text/csv'), + multi_term_batch_mode: '1'}) + expect(json['message']).to eq 'change_threshold is required to use multi term_batch mode.' + end + + it "should use multi_term_batch_mode" do + json = api_call(:post, + "/api/v1/accounts/#{@account.id}/sis_imports.json", + { controller: 'sis_imports_api', action: 'create', + format: 'json', account_id: @account.id.to_s }, + { import_type: 'instructure_csv', + attachment: fixture_file_upload("files/sis/test_user_1.csv", 'text/csv'), + batch_mode: '1', + multi_term_batch_mode: '1', + change_threshold: 7,}) + batch = SisBatch.find(json["id"]) + expect(batch.options[:multi_term_batch_mode]).to be_truthy + end + it "should enable batch with sis stickyness" do json = api_call(:post, "/api/v1/accounts/#{@account.id}/sis_imports.json", diff --git a/spec/models/sis_batch_spec.rb b/spec/models/sis_batch_spec.rb index 5d10ab2e66c..99c17656ea3 100644 --- a/spec/models/sis_batch_spec.rb +++ b/spec/models/sis_batch_spec.rb @@ -736,6 +736,55 @@ test_1,u1,student,active} expect(b1.processing_errors.size).to eq 0 end + describe 'multi_term_batch_mode' do + before :once do + @term2 = @account.enrollment_terms.first + @term2.update_attribute(:sis_source_id, 'term2') + + @c2 = factory_with_protected_attributes(@account.courses, name: "delete me", enrollment_term: @term2, + sis_source_id: 'test_2', sis_batch_id: @old_batch.id) + end + + it 'should use multi_term_batch_mode' do + batch = create_csv_data([ + %{term_id,name,status + term1,term1,active + term2,term2,active}, + %{course_id,short_name,long_name,account_id,term_id,status}, + %{course_id,user_id,role,status}, + ]) do |batch| + batch.options = {} + batch.batch_mode = true + batch.options[:multi_term_batch_mode] = true + batch.save! + batch.process_without_send_later + end + expect(@e1.reload).to be_deleted + expect(@e2.reload).to be_deleted + expect(@c1.reload).to be_deleted + expect(@c2.reload).to be_deleted + expect(batch.workflow_state).to eq 'imported' + end + + it 'should not use multi_term_batch_mode if no terms are passed' do + batch = create_csv_data([ + %{course_id,short_name,long_name,account_id,term_id,status}, + %{course_id,user_id,role,status}, + ]) do |batch| + batch.options = {} + batch.batch_mode = true + batch.options[:multi_term_batch_mode] = true + batch.save! + batch.process_without_send_later + end + expect(@e1.reload).to be_active + expect(@e2.reload).to be_active + expect(@c1.reload.workflow_state).to eq 'created' + expect(@c2.reload.workflow_state).to eq 'created' + expect(batch.workflow_state).to eq 'aborted' + end + end + end end end