add daily syncing of LTI rosters

Changelog.md

@@ -7,6 +7,7 @@
 ### ✨ New features and improvements
 
 - Improve textviewer rendering speed (#7211)
+- Add periodic roster syncing via LTI (#7178)
 
 ### 🐛 Bug fixes
 

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 {
                   <input
                     type="checkbox"
                     name="include_instructors"
-                    key="1"
+                    key="3"
                     defaultChecked="true"
                     onChange={this.handleChange}
                   />
                   {I18n.t("lti.sync_instructors")}
                 </label>
               </p>
+              <p>
+                <label>
+                  <input
+                    type="checkbox"
+                    name="automatic_sync"
+                    key="4"
+                    defaultChecked="true"
+                    onChange={this.handleChange}
+                  />
+                  {I18n.t("lti.sync_automatically")}
+                </label>
+              </p>
 
               <section className={"modal-container dialog-actions"}>
                 <input type="submit" value={I18n.t("lti.roster_sync")} />

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

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)

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

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

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

config/initializers/resque.rb

@@ -19,4 +19,5 @@
   end
 
   Resque.schedule = Settings.resque_scheduler.to_h.deep_stringify_keys
+  Resque::Scheduler.dynamic = true
 end

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

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 * * *"

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 * * *"

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 * * *"

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

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
 

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'