diff --git a/Changelog.md b/Changelog.md index e827001a89..306a892526 100644 --- a/Changelog.md +++ b/Changelog.md @@ -7,6 +7,7 @@ ### ✨ New features and improvements - Improve textviewer rendering speed (#7211) +- Add periodic roster syncing via LTI (#7178) ### 🐛 Bug fixes diff --git a/app/assets/javascripts/Components/Modals/roster_sync_modal.jsx b/app/assets/javascripts/Components/Modals/roster_sync_modal.jsx index 2b7743c07f..b59194196f 100644 --- a/app/assets/javascripts/Components/Modals/roster_sync_modal.jsx +++ b/app/assets/javascripts/Components/Modals/roster_sync_modal.jsx @@ -10,6 +10,7 @@ class LtiRosterModal extends React.Component { include_tas: true, include_students: true, include_instructors: true, + automatic_sync: true, }; } @@ -36,6 +37,7 @@ class LtiRosterModal extends React.Component { include_students: this.state.include_students, include_tas: this.state.include_tas, include_instructors: this.state.include_instructors, + automatic_sync: this.state.automatic_sync, lti_deployment_id: this.props.roster_deployment_id, }, }); @@ -82,13 +84,25 @@ class LtiRosterModal extends React.Component { {I18n.t("lti.sync_instructors")}

+

+ +

diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index 716469a8e3..ba6a4dcafd 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -171,10 +171,24 @@ def sync_roster if params[:include_instructors] == 'true' roles.append(LtiDeployment::LTI_ROLES[:instructor]) end - @current_job = LtiRosterSyncJob.perform_later(deployment, @current_course, - roles, - can_create_users: allowed_to?(:lti_manage?, with: UserPolicy), - can_create_roles: allowed_to?(:manage?, with: RolePolicy)) + job_args = {} + job_args[:deployment_id] = deployment.id + job_args[:role_types] = roles + job_args[:can_create_users] = allowed_to?(:lti_manage?, with: UserPolicy) + job_args[:can_create_roles] = allowed_to?(:manage?, with: RolePolicy) + name = "LtiRosterSync_#{deployment.id}_#{root_path.tr!('/', '')}" + if params[:automatic_sync] == 'true' + config = {} + config[:class] = 'ActiveJob::QueueAdapters::ResqueAdapter::JobWrapper' + config[:args] = { job_class: 'LtiRosterSyncJob', arguments: [job_args] } + config[:cron] = Settings.lti.sync_schedule + config[:persist] = true + config[:queue] = LtiRosterSyncJob.queue_name + Resque.set_schedule(name, config) + else + Resque.remove_schedule(name) + end + @current_job = LtiRosterSyncJob.perform_later(job_args) session[:job_id] = @current_job.job_id redirect_to edit_course_path(@current_course) end diff --git a/app/helpers/lti_helper.rb b/app/helpers/lti_helper.rb index 7059d94401..324b4add2b 100644 --- a/app/helpers/lti_helper.rb +++ b/app/helpers/lti_helper.rb @@ -4,8 +4,9 @@ module LtiHelper # if role is not nil, attempt to create users # based on the values of can_create_users and # can_create_roles. - def roster_sync(lti_deployment, course, role_types, can_create_users: false, can_create_roles: false) + def roster_sync(lti_deployment, role_types, can_create_users: false, can_create_roles: false) error = false + course = lti_deployment.course auth_data = lti_deployment.lti_client.get_oauth_token([LtiDeployment::LTI_SCOPES[:names_role]]) names_service = lti_deployment.lti_services.find_by!(service_type: 'namesrole') membership_uri = URI(names_service.url) diff --git a/app/jobs/lti_roster_sync_job.rb b/app/jobs/lti_roster_sync_job.rb index f6e88cb10c..24d20b845a 100644 --- a/app/jobs/lti_roster_sync_job.rb +++ b/app/jobs/lti_roster_sync_job.rb @@ -9,9 +9,12 @@ def self.completed_message(_status) I18n.t('lti.roster_sync_complete') end - def perform(lti_deployment, course, role_types, can_create_users: false, can_create_roles: false) - roster_error = roster_sync(lti_deployment, course, role_types, can_create_users: can_create_users, - can_create_roles: can_create_roles) + def perform(args) + args = args.deep_symbolize_keys + lti_deployment = LtiDeployment.find(args[:deployment_id]) + + roster_error = roster_sync(lti_deployment, args[:role_types], can_create_users: args[:can_create_users], + can_create_roles: args[:can_create_roles]) if roster_error status.update(warning_message: [status[:warning_message], I18n.t('lti.roster_sync_errors')].compact.join("\n")) end diff --git a/config.ru b/config.ru index 2b6f81c649..992918243f 100644 --- a/config.ru +++ b/config.ru @@ -1,6 +1,8 @@ # This file is used by Rack-based servers to start the application. require File.expand_path('config/environment', __dir__) +# Required for managing scheduled jobs via web interface +use Rack::MethodOverride map ENV['RAILS_RELATIVE_URL_ROOT'] || '/' do run Markus::Application end diff --git a/config/initializers/config.rb b/config/initializers/config.rb index 4dab29bf2c..cf9746d527 100644 --- a/config/initializers/config.rb +++ b/config/initializers/config.rb @@ -193,6 +193,7 @@ required(:domains).array(:str?) required(:token_endpoint).filled(:string) optional(:unpermitted_new_course_message).filled(:string) + required(:sync_schedule).filled(:string) end end end diff --git a/config/initializers/resque.rb b/config/initializers/resque.rb index 4d3e207e34..56cab8e3c5 100644 --- a/config/initializers/resque.rb +++ b/config/initializers/resque.rb @@ -19,4 +19,5 @@ end Resque.schedule = Settings.resque_scheduler.to_h.deep_stringify_keys + Resque::Scheduler.dynamic = true end diff --git a/config/locales/common/en.yml b/config/locales/common/en.yml index d004b4afec..b426716c05 100644 --- a/config/locales/common/en.yml +++ b/config/locales/common/en.yml @@ -48,6 +48,7 @@ en: select_course: Select the course that matches %{course_name} start_grade_sync: Syncing Grades start_roster_sync: Syncing Roster + sync_automatically: Enable automatic syncing sync_grades: Sync Grades sync_grades_lms: Sync grades with LMS sync_instructors: Sync Instructor roster diff --git a/config/settings/development.yml b/config/settings/development.yml index 7c2186f3b3..c00eab1d19 100644 --- a/config/settings/development.yml +++ b/config/settings/development.yml @@ -43,3 +43,4 @@ lti: domains: <%= %w[host.docker.internal] %> token_endpoint: "http://host.docker.internal:3100/login/oauth2/token" unpermitted_new_course_message: 'You are not permitted to create a new MarkUs course for %{course_name}. Please contact your system administrator.' + sync_schedule: "0 3 * * *" diff --git a/config/settings/production.yml b/config/settings/production.yml index 3675e7e8db..e6414c88f7 100644 --- a/config/settings/production.yml +++ b/config/settings/production.yml @@ -37,3 +37,4 @@ autotest: lti: domains: <%= %w[canvas.instructure.com] %> token_endpoint: "https://canvas.instructure.com/login/oauth2/token" + sync_schedule: "0 3 * * *" diff --git a/config/settings/test.yml b/config/settings/test.yml index c2fb4f99f5..88f0a218cf 100644 --- a/config/settings/test.yml +++ b/config/settings/test.yml @@ -36,3 +36,4 @@ lti: domains: <%= %w[test.host] %> token_endpoint: "http://test.host.com/login/oauth2/token" unpermitted_new_course_message: 'You are not permitted to create a new MarkUs course for %{course_name}. Please contact your system administrator.' + sync_schedule: "0 3 * * *" diff --git a/spec/controllers/courses_controller_spec.rb b/spec/controllers/courses_controller_spec.rb index bdc7d44958..d161d8fbfb 100644 --- a/spec/controllers/courses_controller_spec.rb +++ b/spec/controllers/courses_controller_spec.rb @@ -619,4 +619,47 @@ expect(response.parsed_body[0]).to have_key('lti_client') end end + + describe 'sync_roster' do + let!(:lti_deployment) { create(:lti_deployment, course: course) } + + before do + create(:lti_service_namesrole, lti_deployment: lti_deployment) + create(:lti_service_lineitem, lti_deployment: lti_deployment) + end + + after do + Resque.remove_schedule("LtiRosterSync_#{lti_deployment.id}_#{root_path.tr!('/', '')}") + clear_enqueued_jobs + clear_performed_jobs + end + + it 'enqueues a job' do + post_as instructor, :sync_roster, + params: { id: course.id, lti_deployment_id: lti_deployment.id, include_students: 'true' } + expect { LtiRosterSyncJob.perform_later }.to have_enqueued_job + end + + it 'creates a schedule' do + post_as instructor, :sync_roster, + params: { id: course.id, lti_deployment_id: lti_deployment.id, + include_students: 'true', automatic_sync: 'true' } + expect(Resque.fetch_schedule("LtiRosterSync_#{lti_deployment.id}_#{root_path.tr!('/', '')}")).not_to be_nil + end + + it 'unsets a schedule' do + post_as instructor, :sync_roster, + params: { id: course.id, lti_deployment_id: lti_deployment.id, + include_students: 'true', automatic_sync: 'true' } + post_as instructor, :sync_roster, + params: { id: course.id, lti_deployment_id: lti_deployment.id, include_students: 'true' } + expect(Resque.fetch_schedule("LtiRosterSync_#{lti_deployment.id}_#{root_path.tr!('/', '')}")).to be_nil + end + + it 'does not raise an error when no schedule can be unset' do + post_as instructor, :sync_roster, + params: { id: course.id, lti_deployment_id: lti_deployment.id, include_students: 'true' } + expect(response).to have_http_status :redirect + end + end end diff --git a/spec/helpers/lti_helper_spec.rb b/spec/helpers/lti_helper_spec.rb index 47b1d7517f..9069cb1370 100644 --- a/spec/helpers/lti_helper_spec.rb +++ b/spec/helpers/lti_helper_spec.rb @@ -95,8 +95,8 @@ context 'when run by an admin user' do subject do - roster_sync lti_deployment, course, [LtiDeployment::LTI_ROLES[:learner]], can_create_users: true, - can_create_roles: true + roster_sync lti_deployment, [LtiDeployment::LTI_ROLES[:learner]], can_create_users: true, + can_create_roles: true end it 'creates a new user' do @@ -127,8 +127,8 @@ context 'when run by an instructor' do subject do - roster_sync lti_deployment, course, [LtiDeployment::LTI_ROLES[:learner]], can_create_users: true, - can_create_roles: true + roster_sync lti_deployment, [LtiDeployment::LTI_ROLES[:learner]], can_create_users: true, + can_create_roles: true end it 'does create users' do @@ -171,7 +171,7 @@ context 'when run by an admin user' do subject do - roster_sync lti_deployment, course, [LtiDeployment::LTI_ROLES[:learner], LtiDeployment::LTI_ROLES[:ta]], + roster_sync lti_deployment, [LtiDeployment::LTI_ROLES[:learner], LtiDeployment::LTI_ROLES[:ta]], can_create_users: true, can_create_roles: true end @@ -203,7 +203,7 @@ context 'when run by an instructor' do subject do - roster_sync lti_deployment, course, [LtiDeployment::LTI_ROLES[:learner], LtiDeployment::LTI_ROLES[:ta]], + roster_sync lti_deployment, [LtiDeployment::LTI_ROLES[:learner], LtiDeployment::LTI_ROLES[:ta]], can_create_users: true, can_create_roles: true end @@ -247,8 +247,8 @@ context 'when run by an admin user' do subject do - roster_sync lti_deployment, course, [LtiDeployment::LTI_ROLES[:learner], LtiDeployment::LTI_ROLES[:ta], - LtiDeployment::LTI_ROLES[:instructor]], + roster_sync lti_deployment, [LtiDeployment::LTI_ROLES[:learner], LtiDeployment::LTI_ROLES[:ta], + LtiDeployment::LTI_ROLES[:instructor]], can_create_users: true, can_create_roles: true end @@ -280,8 +280,8 @@ context 'when run by an instructor' do subject do - roster_sync lti_deployment, course, [LtiDeployment::LTI_ROLES[:learner], LtiDeployment::LTI_ROLES[:ta], - LtiDeployment::LTI_ROLES[:instructor]], + roster_sync lti_deployment, [LtiDeployment::LTI_ROLES[:learner], LtiDeployment::LTI_ROLES[:ta], + LtiDeployment::LTI_ROLES[:instructor]], can_create_users: true, can_create_roles: true end @@ -318,8 +318,8 @@ context 'with paginated results' do subject do - roster_sync lti_deployment, course, [LtiDeployment::LTI_ROLES[:learner], LtiDeployment::LTI_ROLES[:ta], - LtiDeployment::LTI_ROLES[:instructor]], + roster_sync lti_deployment, [LtiDeployment::LTI_ROLES[:learner], LtiDeployment::LTI_ROLES[:ta], + LtiDeployment::LTI_ROLES[:instructor]], can_create_users: true, can_create_roles: true end diff --git a/spec/jobs/lti_roster_sync_job_spec.rb b/spec/jobs/lti_roster_sync_job_spec.rb index d1ca323d0a..1f8517fa96 100644 --- a/spec/jobs/lti_roster_sync_job_spec.rb +++ b/spec/jobs/lti_roster_sync_job_spec.rb @@ -30,7 +30,7 @@ context 'when running as a background job' do let(:job_args) do - [lti_deployment.id, course, [LtiDeployment::LTI_ROLES[:learner], LtiDeployment::LTI_ROLES[:ta]]] + [lti_deployment.id, [LtiDeployment::LTI_ROLES[:learner], LtiDeployment::LTI_ROLES[:ta]]] end include_examples 'background job'