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 <svc.cloudjenkins@instructure.com>
QA-Review: Ahmad Amireh <ahmad@instructure.com>
Product-Review: Ahmad Amireh <ahmad@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>
This commit is contained in:
August Thornton 2020-06-28 17:34:35 -06:00
parent 4c341a1b89
commit 8f00d2cdc8
7 changed files with 106 additions and 13 deletions

View File

@ -736,6 +736,14 @@ Ignored for any role other than observer</td>
<td>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. </td>
</tr>
<tr>
<td>notify</td>
<td>boolean</td>
<td></td>
<td></td>
<td>If true, a notification will be sent to the enrolled user. Notifications are
not sent by default. </td>
</tr>
</table>
&#42; course_id or section_id is required, role or role_id is required, and

View File

@ -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
)

View File

@ -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

View File

@ -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

View File

@ -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",
",,,,,"
)

View File

@ -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')

View File

@ -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