From 8f00d2cdc8510f581bc0545c1745e733b41a598d Mon Sep 17 00:00:00 2001 From: August Thornton Date: Sun, 28 Jun 2020 17:34:35 -0600 Subject: [PATCH] add ability to send notifications for SIS batch enrollment This change allows us to trigger course enrollment notifications via the SIS enrollment batch importer. The default will be to not broadcast the notifications unless the optional param is included and set to *true*. We only send out one notification as to avoid spam or duplicates in case of re-upload or a failed import. closes FOO-652 flag = none test plan: * create a new course and have an existing user to work with * ensure the communication channel for that user is verified * create an enrollments.csv with the required fields and omit the optional field `notify` * navigate to `/accounts/self/sis_import` * import the .csv from above * verify no notification is sent via MailCatcher or `/users/:id/messages` for the SIS user * add the `notify` header field to the same .csv with a value of `true` for the particular user enrollment * import the .csv and verify an email *was* sent * delete the newly added course enrollment for that user * import the same .csv and verify we don't send another enrollment notification for that user * ensure no documentation errors found for /doc/api/file.sis_csv.html with the newly added optional field name _notify_ Change-Id: Ibddb11bce765b3830370bc07219e34e5ec982f5d Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/241370 Tested-by: Service Cloud Jenkins QA-Review: Ahmad Amireh Product-Review: Ahmad Amireh Reviewed-by: Cody Cutrer --- doc/api/sis_csv.md | 8 ++++ lib/sis/csv/enrollment_importer.rb | 1 + lib/sis/enrollment_importer.rb | 9 ++-- lib/sis/models/enrollment.rb | 6 ++- spec/lib/sis/csv/enrollment_importer_spec.rb | 12 ++--- spec/lib/sis/enrollment_importer_spec.rb | 49 ++++++++++++++++++++ spec/lib/sis/models/enrollment_spec.rb | 34 +++++++++++++- 7 files changed, 106 insertions(+), 13 deletions(-) diff --git a/doc/api/sis_csv.md b/doc/api/sis_csv.md index 36bfb1d190d..b41d8f9fea0 100644 --- a/doc/api/sis_csv.md +++ b/doc/api/sis_csv.md @@ -736,6 +736,14 @@ Ignored for any role other than observer Defaults to false. When true, the enrollment will only allow the user to see and interact with users enrolled in the section given by course_section_id. + +notify +boolean + + +If true, a notification will be sent to the enrolled user. Notifications are + not sent by default. + * course_id or section_id is required, role or role_id is required, and diff --git a/lib/sis/csv/enrollment_importer.rb b/lib/sis/csv/enrollment_importer.rb index 3b46c2d9bcf..6d23b80c13f 100644 --- a/lib/sis/csv/enrollment_importer.rb +++ b/lib/sis/csv/enrollment_importer.rb @@ -59,6 +59,7 @@ module SIS root_account_id: row['root_account'], role_id: row['role_id'], limit_section_privileges: row['limit_section_privileges'], + notify: row['notify'], lineno: row['lineno'], csv: csv ) diff --git a/lib/sis/enrollment_importer.rb b/lib/sis/enrollment_importer.rb index d5cc003e4f5..cb5b0c91502 100644 --- a/lib/sis/enrollment_importer.rb +++ b/lib/sis/enrollment_importer.rb @@ -175,7 +175,6 @@ module SIS message = "Neither course nor section existed for user enrollment " message << "(Course ID: #{enrollment_info.course_id}, Section ID: #{enrollment_info.section_id}, User ID: #{enrollment_info.user_id})" @messages << SisBatch.build_error(enrollment_info.csv, message, sis_batch: @batch, row: enrollment_info.lineno, row_info: enrollment_info.row_info) - next end @@ -258,7 +257,6 @@ module SIS next end end - enrollment = @section.all_enrollments.where(user_id: user, type: type, associated_user_id: associated_user_id, @@ -309,7 +307,11 @@ module SIS enrollment.sis_batch_id = @batch.id enrollment.skip_touch_user = true begin - enrollment.save_without_broadcasting! + if Canvas::Plugin.value_to_boolean(enrollment_info.notify) + enrollment.save! + else + enrollment.save_without_broadcasting! + end rescue ActiveRecord::RecordInvalid msg = "An enrollment did not pass validation " msg += "(" + "course: #{enrollment_info.course_id}, section: #{enrollment_info.section_id}, " @@ -336,7 +338,6 @@ module SIS else @enrollments_to_update_sis_batch_ids << enrollment.id end - @success_count += 1 end end diff --git a/lib/sis/models/enrollment.rb b/lib/sis/models/enrollment.rb index 3ab319bd3c6..e3dcee268c8 100644 --- a/lib/sis/models/enrollment.rb +++ b/lib/sis/models/enrollment.rb @@ -21,14 +21,14 @@ module SIS attr_accessor :course_id, :section_id, :user_id, :user_integration_id, :role, :status, :associated_user_id, :root_account_id, :role_id, :start_date, :end_date, :sis_batch_id, - :limit_section_privileges, :lineno, :csv + :limit_section_privileges, :notify, :lineno, :csv def initialize(course_id: nil, section_id: nil, user_id: nil, user_integration_id: nil, role: nil, status: nil, associated_user_id: nil, root_account_id: nil, role_id: nil, start_date: nil, end_date: nil, sis_batch_id: nil, limit_section_privileges: nil, - lineno: nil, csv: nil) + notify: nil, lineno: nil, csv: nil) self.course_id = course_id self.section_id = section_id self.user_id = user_id @@ -39,6 +39,7 @@ module SIS self.root_account_id = root_account_id self.role_id = role_id self.limit_section_privileges = limit_section_privileges + self.notify = notify self.start_date = start_date self.end_date = end_date self.lineno = lineno @@ -71,6 +72,7 @@ module SIS root_account_id: root_account_id, role_id: role_id, limit_section_privileges: limit_section_privileges, + notify: notify, start_date: start_date, end_date: end_date].to_s end diff --git a/spec/lib/sis/csv/enrollment_importer_spec.rb b/spec/lib/sis/csv/enrollment_importer_spec.rb index 7ae73b1e190..e4488264a98 100644 --- a/spec/lib/sis/csv/enrollment_importer_spec.rb +++ b/spec/lib/sis/csv/enrollment_importer_spec.rb @@ -96,18 +96,18 @@ describe SIS::CSV::EnrollmentImporter do end it "should enroll users" do - #create course, users, and sections + # create course, users, and sections process_csv_data_cleanly( "course_id,short_name,long_name,account_id,term_id,status", "test_1,TC 101,Test Course 101,,,active" ) importer = process_csv_data( - "user_id,login_id,first_name,last_name,email,status", - "user_1,user1,User,Uno,user@example.com,active", - "user_2,user2,User,Dos,user2@example.com,active", + "user_id,login_id,first_name,last_name,email,status,notify", + "user_1,user1,User,Uno,user@example.com,active,true", + "user_2,user2,User,Dos,user2@example.com,active,true", "user_3,user4,User,Tres,user3@example.com,active", - "user_5,user5,User,Quatro,user5@example.com,active", - "user_6,user6,User,Cinco,user6@example.com,active", + "user_5,user5,User,Quatro,user5@example.com,active,false", + "user_6,user6,User,Cinco,user6@example.com,active,false", "user_7,user7,User,Siete,user7@example.com,active", ",,,,," ) diff --git a/spec/lib/sis/enrollment_importer_spec.rb b/spec/lib/sis/enrollment_importer_spec.rb index bc0155ae4bd..d5247f6eade 100644 --- a/spec/lib/sis/enrollment_importer_spec.rb +++ b/spec/lib/sis/enrollment_importer_spec.rb @@ -91,6 +91,55 @@ module SIS end end + context 'notifications' do + let(:messages) { [] } + let(:enrollment) { StudentEnrollment.new } + + before(:once) do + @course = course_model(sis_source_id: 'C001') + @section = @course.course_sections.create!(sis_source_id: 'S001') + @user = user_with_managed_pseudonym(sis_user_id: 'U001') + Account.default.pseudonyms << @user.pseudonym + end + + before(:each) do + allow(StudentEnrollment).to receive(:new).and_return(enrollment) + allow(SisBatchRollBackData).to receive(:build_data).and_return(nil) + allow(Setting).to receive(:get).and_return(1) + end + + it "should save without broadcasting if notify is blank" do + expect(enrollment).to receive(:save_without_broadcasting!).once + + EnrollmentImporter.new(Account.default, {batch: Account.default.sis_batches.create!}).process(messages) do |importer| + sis_enrollment = SIS::Models::Enrollment.new( + course_id: @course.sis_source_id, + section_id: @section.sis_source_id, + user_id: @user.pseudonym.sis_user_id, + role: 'student', + status: 'active' + ) + importer.add_enrollment(sis_enrollment) + end + end + + it "should save with broadcasting if notify is set" do + expect(enrollment).to receive(:save_without_broadcasting!).never + + EnrollmentImporter.new(Account.default, {batch: Account.default.sis_batches.create!}).process(messages) do |importer| + sis_enrollment = SIS::Models::Enrollment.new( + course_id: @course.sis_source_id, + section_id: @section.sis_source_id, + user_id: @user.pseudonym.sis_user_id, + role: 'student', + status: 'active', + notify: 'true' + ) + importer.add_enrollment(sis_enrollment) + end + end + end + it 'should skip touching courses' do Timecop.freeze(2.days.ago) do @c = course_model(sis_source_id: 'C001') diff --git a/spec/lib/sis/models/enrollment_spec.rb b/spec/lib/sis/models/enrollment_spec.rb index 5b23f3cad04..bd3242e7be4 100644 --- a/spec/lib/sis/models/enrollment_spec.rb +++ b/spec/lib/sis/models/enrollment_spec.rb @@ -19,6 +19,32 @@ require File.expand_path(File.dirname(__FILE__) + '/../../../spec_helper.rb') describe SIS::Models::Enrollment do + describe '#initialize' do + let(:params) do + { + course_id: '1', + section_id: '2', + role: 'StudentEnrollment', + notify: 'true' + } + end + let(:subject) { described_class.new(params) } + + it 'sets nil defaults to all params' do + expect(subject.user_integration_id).to be_nil + expect(subject.associated_user_id).to be_nil + expect(subject.status).to be_nil + expect(subject.limit_section_privileges).to be_nil + end + + it 'assigns args if provided' do + expect(subject.notify).to eq 'true' + expect(subject.course_id).to eq '1' + expect(subject.section_id).to eq '2' + expect(subject.role).to eq 'StudentEnrollment' + end + end + describe '#valid_context?' do it 'detects an invalid context' do expect(subject).to_not be_valid_context @@ -35,7 +61,6 @@ describe SIS::Models::Enrollment do end end - describe '#valid_user?' do it 'detects an invalid user' do expect(subject).to_not be_valid_user @@ -82,4 +107,11 @@ describe SIS::Models::Enrollment do expect(subject).to be_valid_status end end + + describe '#row_info' do + it 'provides row info for available attributes' do + subject.notify = 'true' + expect(subject.row_info).to include(':notify=>"true"') + end + end end