From 955e3a273940f354b82951c2b726b4940e2ed642 Mon Sep 17 00:00:00 2001 From: Samuel Maldonado Date: Wed, 18 Sep 2024 15:13:03 -0400 Subject: [PATCH 1/9] Enable cron-based automatic syncing of LTI rosters (#7178) --- Changelog.md | 22 ++++++++++ .../Components/Modals/roster_sync_modal.jsx | 16 ++++++- app/controllers/courses_controller.rb | 22 ++++++++-- app/helpers/lti_helper.rb | 3 +- app/jobs/lti_roster_sync_job.rb | 9 ++-- config.ru | 2 + config/initializers/config.rb | 1 + config/initializers/resque.rb | 1 + config/locales/common/en.yml | 1 + config/settings/development.yml | 1 + config/settings/production.yml | 1 + config/settings/test.yml | 1 + spec/controllers/courses_controller_spec.rb | 43 +++++++++++++++++++ spec/helpers/lti_helper_spec.rb | 24 +++++------ spec/jobs/lti_roster_sync_job_spec.rb | 2 +- 15 files changed, 127 insertions(+), 22 deletions(-) diff --git a/Changelog.md b/Changelog.md index 6f11bd52ca..129f1588ae 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,5 +1,27 @@ # Changelog +## [unreleased] + +### 🚨 Breaking changes + +### ✨ New features and improvements + +- Improve textviewer rendering speed (#7211) +- Add periodic roster syncing via LTI (#7178) + +### 🐛 Bug fixes +- Fix incorrect calculation of token penalties when submissions are on time (#7216) + +### 🔧 Internal changes + +- Fix test coverage for ExamTemplate.create_with_file method (#7213) +- Upgrade Docker environment to use Ruby v3.3 (#7185) +- Upgrade to Rails v7.2 (#7185) +- Manually specify chromedriver port number in Github actions (#7209) +- Move Exception message in student model to a localization file (#7218) +- Add test cases for the student model to cover Group or Grouping save method failure (#7218) +- Create tests for overtime messages of the submission rule classes (#7216) + ## [v2.5.1] ### 🐛 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' From 5afd41bc405beb70aa034498088212d58e1cc38a Mon Sep 17 00:00:00 2001 From: Aina Merchant <129339474+AinaMerch@users.noreply.github.com> Date: Wed, 18 Sep 2024 15:58:22 -0400 Subject: [PATCH 2/9] Enable assigning graders by section (#7179) --- Changelog.md | 1 + .../Modals/section_distribution_modal.js | 80 +++++++++++++++ .../Components/graders_manager.jsx | 41 ++++++++ app/assets/javascripts/fontawesome_config.js | 2 + app/controllers/graders_controller.rb | 69 +++++++++++-- app/models/grouping.rb | 9 ++ config/locales/views/graders/en.yml | 3 + spec/controllers/graders_controller_spec.rb | 98 +++++++++++++++++++ 8 files changed, 296 insertions(+), 7 deletions(-) create mode 100644 app/assets/javascripts/Components/Modals/section_distribution_modal.js diff --git a/Changelog.md b/Changelog.md index 129f1588ae..f5620ad70f 100644 --- a/Changelog.md +++ b/Changelog.md @@ -8,6 +8,7 @@ - Improve textviewer rendering speed (#7211) - Add periodic roster syncing via LTI (#7178) +- Allow instructors to assign graders by section (#7179) ### 🐛 Bug fixes - Fix incorrect calculation of token penalties when submissions are on time (#7216) diff --git a/app/assets/javascripts/Components/Modals/section_distribution_modal.js b/app/assets/javascripts/Components/Modals/section_distribution_modal.js new file mode 100644 index 0000000000..3ea46e9ec0 --- /dev/null +++ b/app/assets/javascripts/Components/Modals/section_distribution_modal.js @@ -0,0 +1,80 @@ +import React from "react"; +import Modal from "react-modal"; +import PropTypes from "prop-types"; + +export class SectionDistributionModal extends React.Component { + static defaultProps = { + override: false, + }; + + constructor(props) { + super(props); + this.input = React.createRef(); + this.sectionsArray = Object.values(this.props.sections).sort(); + this.graderMap = this.props.graders.reduce((map, grader) => { + map[grader.user_name] = grader._id; + return map; + }, {}); + } + + componentDidMount() { + Modal.setAppElement("body"); + } + + onSubmit = event => { + event.preventDefault(); + const form = new FormData(this.input.current); + const assignments = {}; + form.forEach((value, key) => { + assignments[key] = this.graderMap[value]; + }); + this.props.onSubmit(assignments); + }; + + renderSectionRow = section => { + const {graders} = this.props; + return ( +
+ + +
+ ); + }; + + render() { + return ( + +
+
+

{I18n.t("graders.assign_by_section_modal_title")}

+

{I18n.t("graders.assign_by_section_instruction")}

+ {this.sectionsArray.map(section => this.renderSectionRow(section))} +
+
+ +
+
+ + ); + } +} + +SectionDistributionModal.propTypes = { + graders: PropTypes.arrayOf(PropTypes.object).isRequired, + isOpen: PropTypes.bool.isRequired, + onSubmit: PropTypes.func.isRequired, + sections: PropTypes.objectOf(PropTypes.string).isRequired, +}; diff --git a/app/assets/javascripts/Components/graders_manager.jsx b/app/assets/javascripts/Components/graders_manager.jsx index cdc70c2f58..8ad927a2ac 100644 --- a/app/assets/javascripts/Components/graders_manager.jsx +++ b/app/assets/javascripts/Components/graders_manager.jsx @@ -6,6 +6,7 @@ import {FontAwesomeIcon} from "@fortawesome/react-fontawesome"; import {withSelection, CheckboxTable} from "./markus_with_selection_hoc"; import {selectFilter} from "./Helpers/table_helpers"; import {GraderDistributionModal} from "./Modals/graders_distribution_modal"; +import {SectionDistributionModal} from "./Modals/section_distribution_modal"; class GradersManager extends React.Component { constructor(props) { @@ -22,6 +23,7 @@ class GradersManager extends React.Component { hide_unassigned_criteria: false, sections: {}, isGraderDistributionModalOpen: false, + isSectionDistributionModalOpen: false, show_hidden: false, show_hidden_groups: false, hidden_graders_count: 0, @@ -51,6 +53,11 @@ class GradersManager extends React.Component { isGraderDistributionModalOpen: true, }); }; + openSectionDistributionModal = () => { + this.setState({ + isSectionDistributionModalOpen: true, + }); + }; fetchData = () => { fetch(Routes.course_assignment_graders_path(this.props.course_id, this.props.assignment_id), { @@ -87,6 +94,7 @@ class GradersManager extends React.Component { anonymize_groups: res.anonymize_groups, hide_unassigned_criteria: res.hide_unassigned_criteria, isGraderDistributionModalOpen: false, + isSectionDistributionModalOpen: false, hidden_graders_count: res.graders.filter(grader => grader.hidden).length, inactive_groups_count: inactive_groups_count, }); @@ -124,6 +132,25 @@ class GradersManager extends React.Component { }).then(this.fetchData); }; + assignSections = assignments => { + let sections = Object.keys(assignments); + let graders = Object.values(assignments); + $.post({ + url: Routes.global_actions_course_assignment_graders_path( + this.props.course_id, + this.props.assignment_id + ), + data: { + global_actions: "assign_sections", + current_table: this.state.tableName, + skip_empty_submissions: this.state.skip_empty_submissions, + assignments: assignments, + sections: sections, + graders: graders, + }, + }).then(this.fetchData); + }; + assignRandomly = weightings => { let groups = this.groupsTable ? this.groupsTable.state.selection : []; let criteria = this.criteriaTable ? this.criteriaTable.state.selection : []; @@ -305,6 +332,7 @@ class GradersManager extends React.Component { )} + {this.state.isSectionDistributionModalOpen && ( + this.setState({isSectionDistributionModalOpen: false})} + onSubmit={this.assignSections} + graders={this.state.graders} + sections={this.state.sections} + /> + )} ); } @@ -778,6 +815,10 @@ class GradersActionBox extends React.Component { {I18n.t("graders.actions.randomly_assign_graders")} +