From 4774b136f8f1d16aff1dc5409b993f194f70781e Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Tue, 4 Aug 2026 13:21:00 +1000 Subject: [PATCH 1/6] feat: add external sync to moodle via api --- README.md | 1 + app/api/api_root.rb | 2 + app/api/entities/unit_entity.rb | 1 + app/api/moodle_integration_api.rb | 123 +++++++++++++++++ app/api/units_api.rb | 2 + app/models/moodle_integration.rb | 11 ++ app/models/unit.rb | 3 + app/services/moodle_api.rb | 128 +++++++++++++++++ app/sidekiq/import_moodle_extensions_job.rb | 85 ++++++++++++ app/sidekiq/import_moodle_students_job.rb | 76 +++++++++++ app/sidekiq/test_moodle_connection_job.rb | 27 ++++ config/application.rb | 6 + ...4005203_add_moodle_integration_to_units.rb | 16 +++ db/schema.rb | 15 +- test/api/moodle_integration_api_test.rb | 129 ++++++++++++++++++ test/services/moodle_integration_test.rb | 79 +++++++++++ test/sidekiq/import_moodle_jobs_test.rb | 106 ++++++++++++++ 17 files changed, 809 insertions(+), 1 deletion(-) create mode 100644 app/api/moodle_integration_api.rb create mode 100644 app/models/moodle_integration.rb create mode 100644 app/services/moodle_api.rb create mode 100644 app/sidekiq/import_moodle_extensions_job.rb create mode 100644 app/sidekiq/import_moodle_students_job.rb create mode 100644 app/sidekiq/test_moodle_connection_job.rb create mode 100644 db/migrate/20260804005203_add_moodle_integration_to_units.rb create mode 100644 test/api/moodle_integration_api_test.rb create mode 100644 test/services/moodle_integration_test.rb create mode 100644 test/sidekiq/import_moodle_jobs_test.rb diff --git a/README.md b/README.md index c81d68e9cf..12cf73daa9 100644 --- a/README.md +++ b/README.md @@ -170,6 +170,7 @@ for `DF_SECRET_KEY_BASE`, `DF_SECRET_KEY_ATTR`, `DF_SECRET_KEY_DEVISE`, | `TII_REGISTER_WEBHOOK` | Register the Turnitin webhook. | `false` | | `TCA_API_KEY` | Turnitin Core API key. | Unset | | `TCA_HOST` | Turnitin institution host. | Unset | +| `DF_MOODLE_API_URL` | Moodle base URL used by unit integrations. | Unset | | `DF_JPLAG_MIN_TOKENS` | Minimum matching-token threshold used by JPlag. | `-1` | | `DF_JPLAG_SKIP_CLUSTER_CHECK` | Skip JPlag cluster calculation. | `false` | | `DF_JPLAG_MAX_SHOWN_COMPARISONS` | Maximum comparisons retained in a JPlag report; `-1` means all. | `2500` | diff --git a/app/api/api_root.rb b/app/api/api_root.rb index cb583d7a2c..1a3b6ee8ef 100644 --- a/app/api/api_root.rb +++ b/app/api/api_root.rb @@ -62,6 +62,7 @@ class ApiRoot < Grape::API mount DiscussionCommentApi mount EngagementsApi mount ExtensionCommentsApi + mount MoodleIntegrationApi mount ScormExtensionCommentsApi mount GroupSetsApi mount LearningOutcomesApi @@ -122,6 +123,7 @@ class ApiRoot < Grape::API AuthenticationHelpers.add_auth_to DiscussionCommentApi AuthenticationHelpers.add_auth_to EngagementsApi AuthenticationHelpers.add_auth_to ExtensionCommentsApi + AuthenticationHelpers.add_auth_to MoodleIntegrationApi AuthenticationHelpers.add_auth_to ScormExtensionCommentsApi AuthenticationHelpers.add_auth_to GroupSetsApi AuthenticationHelpers.add_auth_to LearningOutcomesApi diff --git a/app/api/entities/unit_entity.rb b/app/api/entities/unit_entity.rb index c820f2e362..8a4418ded9 100644 --- a/app/api/entities/unit_entity.rb +++ b/app/api/entities/unit_entity.rb @@ -50,6 +50,7 @@ def can_read_unit_config?(my_role) unless: :summary_only expose :overseer_image_id, unless: :summary_only, if: lambda { |unit, options| can_read_unit_config?(options[:my_role]) } + expose :moodle_enabled, unless: :summary_only, if: lambda { |unit, options| can_read_unit_config?(options[:my_role]) } expose :assessment_enabled, unless: :summary_only expose :auto_apply_extension_before_deadline, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } diff --git a/app/api/moodle_integration_api.rb b/app/api/moodle_integration_api.rb new file mode 100644 index 0000000000..bb29c060c2 --- /dev/null +++ b/app/api/moodle_integration_api.rb @@ -0,0 +1,123 @@ +# frozen_string_literal: true + +require 'grape' +require 'entities/sidekiq_job_entity' + +class MoodleIntegrationApi < Grape::API + helpers AuthenticationHelpers + helpers AuthorisationHelpers + helpers SidekiqHelper + + before do + authenticated? + end + + desc 'Get Moodle settings for a unit' + get '/units/:unit_id/moodle' do + unit = Unit.find(params[:unit_id]) + error!({ error: 'Moodle integration is not enabled for this unit' }, 404) unless unit.moodle_enabled? + unless authorise?(current_user, unit, :update) + error!({ error: 'Not authorised to manage Moodle for this unit' }, 403) + end + + integration = unit.moodle_integration + present( + { + id: integration&.id, + course_id: integration&.course_id, + assignment_id: integration&.assignment_id, + assignment_name: integration&.assignment_name, + fetch_extensions: integration&.fetch_extensions || false, + api_key_configured: integration&.api_key.present? + }, + with: Grape::Presenters::Presenter + ) + end + + desc 'Update Moodle settings for a unit' + params do + requires :course_id, type: Integer + optional :api_key, type: String + optional :assignment_id, type: Integer + optional :assignment_name, type: String + optional :fetch_extensions, type: Boolean, default: false + end + put '/units/:unit_id/moodle' do + unit = Unit.find(params[:unit_id]) + error!({ error: 'Moodle integration is not enabled for this unit' }, 404) unless unit.moodle_enabled? + unless authorise?(current_user, unit, :update) + error!({ error: 'Not authorised to manage Moodle for this unit' }, 403) + end + + integration = unit.moodle_integration || unit.build_moodle_integration + integration.course_id = params[:course_id] + integration.api_key = params[:api_key] if params[:api_key].present? + integration.fetch_extensions = params[:fetch_extensions] + integration.assignment_id = params[:fetch_extensions] ? params[:assignment_id] : nil + integration.assignment_name = params[:fetch_extensions] ? params[:assignment_name] : nil + integration.save! + + present( + { + id: integration.id, + course_id: integration.course_id, + assignment_id: integration.assignment_id, + assignment_name: integration.assignment_name, + fetch_extensions: integration.fetch_extensions, + api_key_configured: integration.api_key.present? + }, + with: Grape::Presenters::Presenter + ) + end + + desc 'Test Moodle API permissions for a unit' + post '/units/:unit_id/moodle/test' do + unit = Unit.find(params[:unit_id]) + error!({ error: 'Moodle integration is not enabled for this unit' }, 404) unless unit.moodle_enabled? + unless authorise?(current_user, unit, :update) + error!({ error: 'Not authorised to manage Moodle for this unit' }, 403) + end + error!({ error: 'Configure Moodle for this unit first' }, 422) if unit.moodle_integration.blank? + + job_id = TestMoodleConnectionJob.perform_async(unit.id) + job = setup_job(job_id) + present job, with: Entities::SidekiqJobEntity + end + + desc 'Import active Moodle students into a unit' + params do + requires :preview_only, type: Boolean, default: false + end + post '/units/:unit_id/moodle/import_students' do + unit = Unit.find(params[:unit_id]) + error!({ error: 'Moodle integration is not enabled for this unit' }, 404) unless unit.moodle_enabled? + unless authorise?(current_user, unit, :upload_csv) + error!({ error: 'Not authorised to manage Moodle for this unit' }, 403) + end + error!({ error: 'Configure Moodle for this unit first' }, 422) if unit.moodle_integration.blank? + + job_id = ImportMoodleStudentsJob.perform_async(unit.id, params[:preview_only]) + present setup_job(job_id), with: Entities::SidekiqJobEntity + end + + desc 'Import Moodle assignment extensions into a unit' + params do + requires :preview_only, type: Boolean, default: false + end + post '/units/:unit_id/moodle/import_extensions' do + unit = Unit.find(params[:unit_id]) + error!({ error: 'Moodle integration is not enabled for this unit' }, 404) unless unit.moodle_enabled? + unless authorise?(current_user, unit, :update) + error!({ error: 'Not authorised to manage Moodle for this unit' }, 403) + end + + integration = unit.moodle_integration + error!({ error: 'Configure Moodle for this unit first' }, 422) if integration.blank? + unless integration.fetch_extensions && integration.assignment_id.present? + error!({ error: 'Enable extension imports and select a Moodle assignment first' }, 422) + end + + job_id = ImportMoodleExtensionsJob.perform_async(unit.id, params[:preview_only]) + present setup_job(job_id), with: Entities::SidekiqJobEntity + end +end diff --git a/app/api/units_api.rb b/app/api/units_api.rb index 89e0105eee..3d444a2c04 100644 --- a/app/api/units_api.rb +++ b/app/api/units_api.rb @@ -83,6 +83,7 @@ class UnitsApi < Grape::API optional :send_notifications, type: Boolean, desc: 'Indicates if emails should be sent on updates each week' optional :enable_sync_timetable, type: Boolean, desc: 'Sync to timetable automatically if supported by deployment' optional :enable_sync_enrolments, type: Boolean, desc: 'Sync student enrolments automatically if supported by deployment' + optional :moodle_enabled, type: Boolean, desc: 'Enable the Moodle integration for this unit' optional :draft_task_definition_id, type: Integer, desc: 'Indicates the ID of the task definition used as the "draft learning summary task"' optional :portfolio_auto_generation_date, type: Date, desc: 'Indicates a date where student portfolio will automatically compile' optional :allow_flexible_dates, type: Boolean, desc: 'Can turn on/off flexible dates for tasks in this unit' @@ -128,6 +129,7 @@ class UnitsApi < Grape::API :send_notifications, :enable_sync_timetable, :enable_sync_enrolments, + :moodle_enabled, :draft_task_definition_id, :portfolio_auto_generation_date, :allow_flexible_dates, diff --git a/app/models/moodle_integration.rb b/app/models/moodle_integration.rb new file mode 100644 index 0000000000..25854894a2 --- /dev/null +++ b/app/models/moodle_integration.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class MoodleIntegration < ApplicationRecord + belongs_to :unit + + encrypts :api_key + + validates :course_id, numericality: { only_integer: true, greater_than: 0 } + validates :assignment_id, numericality: { only_integer: true, greater_than: 0 }, allow_nil: true + validates :unit_id, uniqueness: true +end diff --git a/app/models/unit.rb b/app/models/unit.rb index 2ebe9c58de..4829f95384 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -22,6 +22,9 @@ class Unit < ApplicationRecord include MimeCheckHelpers include CsvHelper + + has_one :moodle_integration, dependent: :destroy + # # Permissions around unit data # diff --git a/app/services/moodle_api.rb b/app/services/moodle_api.rb new file mode 100644 index 0000000000..a4aa3cc7e2 --- /dev/null +++ b/app/services/moodle_api.rb @@ -0,0 +1,128 @@ +# frozen_string_literal: true + +require 'json' +require 'net/http' +require 'uri' + +class MoodleApi + class Error < StandardError + attr_reader :code + + def initialize(message, code: nil) + @code = code + super(message) + end + end + + def initialize(integration) + @integration = integration + end + + def assignments + request('mod_assign_get_assignments', 'courseids[0]' => @integration.course_id) + end + + def students + request( + 'core_enrol_get_enrolled_users', + 'courseid' => @integration.course_id, + 'options[0][name]' => 'onlyactive', + 'options[0][value]' => 1 + ) + end + + def user_flags(assignment_id = @integration.assignment_id) + request('mod_assign_get_user_flags', 'assignmentids[0]' => assignment_id) + end + + def participant(assignment_id, user_id) + request('mod_assign_get_participant', 'assignid' => assignment_id, 'userid' => user_id, 'embeduser' => 0) + end + + def test_connection(progress_callback: nil) + results = [] + progress_callback&.call(1, 'Fetching course assignments') + assignment_response = test_function(results, 'mod_assign_get_assignments') { assignments } + progress_callback&.call(2, 'Fetching enrolled users') + enrolled_users = test_function(results, 'core_enrol_get_enrolled_users') { students } + + course = Array(assignment_response&.fetch('courses', nil)).find do |item| + item['id'].to_i == @integration.course_id + end + available_assignments = Array(course&.fetch('assignments', nil)) + assignment_id = @integration.assignment_id || available_assignments.first&.fetch('id', nil) + participant_user = Array(enrolled_users).find do |user| + Array(user['roles']).any? { |role| role['shortname'] == 'student' } + end + + progress_callback&.call(3, 'Testing assignment flag access') + + test_function(results, 'mod_assign_get_user_flags') do + raise Error, 'No assignment is available to test this permission' if assignment_id.blank? + + user_flags(assignment_id) + end + progress_callback&.call(4, 'Tested get participant access') + test_function(results, 'mod_assign_get_participant', successful_error_codes: ['userisfilteredout']) do + if assignment_id.blank? || participant_user.blank? + raise Error, 'An assignment and enrolled student are required to test this permission' + end + + participant(assignment_id, participant_user['id']) + end + + { + course: course&.slice('id', 'fullname', 'shortname'), + assignments: available_assignments.map { |item| item.slice('id', 'name', 'duedate') }, + permissions: results + } + end + + private + + def test_function(results, function, successful_error_codes: []) + response = yield + results << { function: function, success: true } + response + rescue Error => e + result = if successful_error_codes.include?(e.code) + { function: function, success: true, message: e.message } + else + { function: function, success: false, error: e.message } + end + results << result + nil + end + + def request(function, params) + raise Error, 'DF_MOODLE_API_URL is not configured' if Doubtfire::Application.config.moodle_api_url.blank? + + base_url = Doubtfire::Application.config.moodle_api_url.sub(%r{/+\z}, '') + endpoint = base_url.end_with?('/webservice/rest/server.php') ? base_url : "#{base_url}/webservice/rest/server.php" + response = Net::HTTP.post_form( + URI.parse(endpoint), + { + 'wstoken' => @integration.api_key, + 'wsfunction' => function, + 'moodlewsrestformat' => 'json' + }.merge(params.transform_values(&:to_s)) + ) + payload = JSON.parse(response.body) + + if payload.is_a?(Hash) && payload['exception'] + raise Error.new( + payload['message'] || payload['errorcode'] || 'Moodle rejected the request', + code: payload['errorcode'] + ) + end + raise Error, "Moodle returned HTTP #{response.code}" unless response.is_a?(Net::HTTPSuccess) + + payload + rescue JSON::ParserError + raise Error, 'Moodle returned an invalid response' + rescue URI::InvalidURIError + raise Error, 'DF_MOODLE_API_URL is invalid' + rescue Timeout::Error, SocketError, SystemCallError, OpenSSL::SSL::SSLError => e + raise Error, "Unable to connect to Moodle: #{e.message}" + end +end diff --git a/app/sidekiq/import_moodle_extensions_job.rb b/app/sidekiq/import_moodle_extensions_job.rb new file mode 100644 index 0000000000..d501614cc3 --- /dev/null +++ b/app/sidekiq/import_moodle_extensions_job.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +class ImportMoodleExtensionsJob + include Sidekiq::Job + include Sidekiq::Status::Worker + + sidekiq_options lock: :until_executed, + lock_args_method: ->(args) { [args.first] }, + on_conflict: :reject, + retry: false + + def perform(unit_id, preview_only) + total(3) + at(0, 'Fetching Moodle assignment') + + unit = Unit.find(unit_id) + raise MoodleApi::Error, 'Moodle integration is not enabled for this unit' unless unit.moodle_enabled? + + integration = unit.moodle_integration + raise MoodleApi::Error, 'Configure Moodle for this unit first' if integration.blank? + unless integration.fetch_extensions && integration.assignment_id.present? + raise MoodleApi::Error, 'Enable extension imports and select a Moodle assignment first' + end + + moodle = MoodleApi.new(integration) + assignment_response = moodle.assignments + course = Array(assignment_response['courses']).find { |item| item['id'].to_i == integration.course_id } + assignment = Array(course&.fetch('assignments', nil)).find do |item| + item['id'].to_i == integration.assignment_id + end + raise MoodleApi::Error, 'The selected assignment was not found in this course' if assignment.blank? + + at(1, 'Fetching enrolled Moodle students') + students = moodle.students.index_by { |student| student['id'].to_i } + + at(2, 'Fetching Moodle extensions') + flags = moodle.user_flags + assignment_flags = Array(flags['assignments']).find do |item| + item['assignmentid'].to_i == integration.assignment_id + end + user_flags = Array(assignment_flags&.fetch('userflags', nil)).select do |flag| + flag['extensionduedate'].to_i.positive? + end + total(3 + user_flags.length) + + result = { success: [], ignored: [], errors: [] } + user_flags.each_with_index do |flag, index| + student = students[flag['userid'].to_i] + extension_date = Time.zone.at(flag['extensionduedate'].to_i).to_date + row = { + username: student&.fetch('username', nil), + extension_date: extension_date.iso8601, + spec_con_days: nil + } + + begin + days = [ + (extension_date - Time.zone.at(assignment['duedate'].to_i).to_date).to_i, + 0 + ].max + row[:spec_con_days] = days + + project = unit.projects.joins(:user).find_by(users: { username: row[:username] }) + if project.blank? + result[:ignored] << { row: row, message: 'Student is not enrolled in OnTrack' } + next + end + + if project.spec_con_days == days + result[:ignored] << { row: row, message: 'Special consideration days are unchanged' } + else + project.update!(spec_con_days: days) unless preview_only + message = preview_only ? "Would update special consideration to #{days} days" : "Special consideration updated to #{days} days" + result[:success] << { row: row, message: message } + end + rescue StandardError => e + result[:errors] << { row: row, message: e.message } + ensure + at(3 + index + 1, "Processing extension for #{row[:username] || 'unknown student'}") + end + end + + store(result: result.to_json) + end +end diff --git a/app/sidekiq/import_moodle_students_job.rb b/app/sidekiq/import_moodle_students_job.rb new file mode 100644 index 0000000000..1a45d75d70 --- /dev/null +++ b/app/sidekiq/import_moodle_students_job.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +class ImportMoodleStudentsJob + include Sidekiq::Job + include Sidekiq::Status::Worker + + sidekiq_options lock: :until_executed, + lock_args_method: ->(args) { [args.first] }, + on_conflict: :reject, + retry: false + + def perform(unit_id, preview_only) + at(0, 'Fetching enrolled Moodle students') + total(0) + + unit = Unit.find(unit_id) + raise MoodleApi::Error, 'Moodle integration is not enabled for this unit' unless unit.moodle_enabled? + + integration = unit.moodle_integration + raise MoodleApi::Error, 'Configure Moodle for this unit first' if integration.blank? + + students = MoodleApi.new(integration).students.select do |student| + Array(student['roles']).any? { |role| role['shortname'] == 'student' } + end + total(students.length) + + rows = students.map do |student| + display_row = { + unit_code: unit.code, + username: student['username'], + student_id: student['idnumber'], + first_name: student['firstname'], + last_name: student['lastname'], + nickname: student['firstname'], + email: student['email'], + enrolled: true + } + display_row.merge(row: display_row, tutorials: [], campus: nil) + end + + result = if preview_only + { + success: rows.map.with_index(1) do |row, index| + at(index, "Reading Moodle student #{row[:username]}") + { row: row[:row], message: 'Student data fetched from Moodle' } + end, + ignored: [], + errors: [] + } + else + import_students(unit, rows) + end + + store(result: result.to_json) + end + + private + + def import_students(unit, rows) + result = { success: [], ignored: [], errors: [] } + unit.sync_enrolment_with( + rows, + { + replace_existing_tutorial: false, + replace_existing_campus: false, + merge_duplicate_students: false + }, + result, + progress_callback: lambda { |message: nil, total_rows: nil, rows_processed: nil| + total(total_rows) if total_rows + at(rows_processed, message) if rows_processed + } + ) + result + end +end diff --git a/app/sidekiq/test_moodle_connection_job.rb b/app/sidekiq/test_moodle_connection_job.rb new file mode 100644 index 0000000000..fe04db3cc5 --- /dev/null +++ b/app/sidekiq/test_moodle_connection_job.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class TestMoodleConnectionJob + include Sidekiq::Job + include Sidekiq::Status::Worker + + sidekiq_options lock: :until_executed, + lock_args_method: ->(args) { [args.first] }, + on_conflict: :reject, + retry: false + + def perform(unit_id) + total(4) + at(0, 'Starting Moodle connection test') + + unit = Unit.find(unit_id) + raise MoodleApi::Error, 'Moodle integration is not enabled for this unit' unless unit.moodle_enabled? + + integration = unit.moodle_integration + raise MoodleApi::Error, 'Configure Moodle for this unit first' if integration.blank? + result = MoodleApi.new(integration).test_connection( + progress_callback: ->(completed, message) { at(completed, message) } + ) + + store(result: result.to_json) + end +end diff --git a/config/application.rb b/config/application.rb index ee62d44efe..e09837d8ae 100644 --- a/config/application.rb +++ b/config/application.rb @@ -146,6 +146,9 @@ def self.fetch_credential_or_env(*credential_path, env_key:, default: nil) # LTI.js will send signed JWT tokens using this secret config.lti_api_secret = Application.fetch_credential_or_env(:lti, :shared_api_secret, env_key: 'LTI_SHARED_API_SECRET') + # ==> Moodle settings + config.moodle_api_url = ENV.fetch('DF_MOODLE_API_URL', nil) + # ==> Moderation settings config.moderation_score_factor = Float(ENV.fetch('MODERATION_SCORE_FACTOR', 1.0)) @@ -270,8 +273,11 @@ def self.fetch_credential_or_env(*credential_path, env_key:, default: nil) # Ensure that auth tokens do not appear in log files config.filter_parameters += %i( auth_token + api_key + moodle_api_key password password_confirmation + wstoken ) # Grape Serialization diff --git a/db/migrate/20260804005203_add_moodle_integration_to_units.rb b/db/migrate/20260804005203_add_moodle_integration_to_units.rb new file mode 100644 index 0000000000..0e2d75700e --- /dev/null +++ b/db/migrate/20260804005203_add_moodle_integration_to_units.rb @@ -0,0 +1,16 @@ +class AddMoodleIntegrationToUnits < ActiveRecord::Migration[8.0] + def change + add_column :units, :moodle_enabled, :boolean, null: false, default: false + + create_table :moodle_integrations do |t| + t.references :unit, null: false, index: { unique: true } + t.bigint :course_id, null: false + t.text :api_key, null: false + t.bigint :assignment_id + t.string :assignment_name + t.boolean :fetch_extensions, null: false, default: false + + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 2a0252e4ce..8d040b5728 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_07_28_033837) do +ActiveRecord::Schema[8.0].define(version: 2026_08_04_005203) do create_table "activity_types", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| t.string "name", null: false t.string "abbreviation", null: false @@ -352,6 +352,18 @@ t.index ["task_id"], name: "index_moderated_tasks_on_task_id" end + create_table "moodle_integrations", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| + t.bigint "unit_id", null: false + t.bigint "course_id", null: false + t.text "api_key", null: false + t.bigint "assignment_id" + t.string "assignment_name" + t.boolean "fetch_extensions", default: false, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["unit_id"], name: "index_moodle_integrations_on_unit_id", unique: true + end + create_table "overflow_task_claim_logs", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| t.bigint "unit_id", null: false t.bigint "task_id", null: false @@ -949,6 +961,7 @@ t.boolean "discuss_timeout_enabled", default: false, null: false t.integer "discuss_timeout_warning_days", default: 7, null: false t.integer "discuss_timeout_expire_days", default: 14, null: false + t.boolean "moodle_enabled", default: false, null: false t.index ["draft_task_definition_id"], name: "index_units_on_draft_task_definition_id" t.index ["main_convenor_id"], name: "index_units_on_main_convenor_id" t.index ["overseer_image_id"], name: "index_units_on_overseer_image_id" diff --git a/test/api/moodle_integration_api_test.rb b/test/api/moodle_integration_api_test.rb new file mode 100644 index 0000000000..922fd639bb --- /dev/null +++ b/test/api/moodle_integration_api_test.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true + +require 'test_helper' + +class MoodleIntegrationApiTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + test 'convenor can save and read Moodle settings without exposing the API key' do + unit = FactoryBot.create(:unit, with_students: false, moodle_enabled: true) + add_auth_header_for(user: unit.main_convenor_user) + + put "/api/units/#{unit.id}/moodle", { + course_id: 42, + api_key: 'secret-token', + assignment_id: 7, + assignment_name: 'Portfolio', + fetch_extensions: true + } + + assert_equal 200, last_response.status, last_response.inspect + integration = unit.reload.moodle_integration + assert_equal 42, integration.course_id + assert_equal 'secret-token', integration.api_key + assert_equal 'Portfolio', integration.assignment_name + assert integration.fetch_extensions + assert_equal integration.id, last_response_body['id'] + assert_equal true, last_response_body['api_key_configured'] + assert_not last_response.body.include?('secret-token') + + get "/api/units/#{unit.id}/moodle" + assert_equal 42, last_response_body['course_id'] + assert_equal 7, last_response_body['assignment_id'] + assert_equal 'Portfolio', last_response_body['assignment_name'] + assert_equal true, last_response_body['fetch_extensions'] + assert_nil last_response_body['api_key'] + end + + test 'student cannot manage Moodle settings' do + unit = FactoryBot.create(:unit, with_students: false, moodle_enabled: true) + add_auth_header_for(user: FactoryBot.create(:user, :student)) + + get "/api/units/#{unit.id}/moodle" + + assert_equal 403, last_response.status + end + + test 'Moodle settings are unavailable when the integration is disabled' do + unit = FactoryBot.create(:unit, with_students: false, moodle_enabled: false) + add_auth_header_for(user: unit.main_convenor_user) + + get "/api/units/#{unit.id}/moodle" + + assert_equal 404, last_response.status + assert_equal 'Moodle integration is not enabled for this unit', last_response_body['error'] + end + + test 'connection endpoint returns the permission report' do + unit = FactoryBot.create( + :unit, + with_students: false, + moodle_enabled: true + ) + unit.create_moodle_integration!(course_id: 42, api_key: 'secret-token') + add_auth_header_for(user: unit.main_convenor_user) + job = { + 'jid' => 'moodle-job-id', + 'status' => 'queued', + 'at' => 0, + 'total' => 4 + } + + TestMoodleConnectionJob.stub(:perform_async, 'moodle-job-id') do + Sidekiq::Status.stub(:get_all, job) do + Sidekiq::Status.stub(:store_for_id, true) do + post "/api/units/#{unit.id}/moodle/test" + end + end + end + + assert_equal 200, last_response.status, last_response.inspect + assert_equal 'moodle-job-id', last_response_body['id'] + assert_equal 4, last_response_body['total_count'].to_i + end + + test 'Moodle imports enqueue preview and import jobs' do + unit = FactoryBot.create(:unit, with_students: false, moodle_enabled: true) + unit.create_moodle_integration!( + course_id: 42, + api_key: 'secret-token', + assignment_id: 7, + assignment_name: 'Portfolio', + fetch_extensions: true + ) + add_auth_header_for(user: unit.main_convenor_user) + queued = [] + job = { 'jid' => 'moodle-import-job', 'status' => 'queued', 'at' => 0, 'total' => 0 } + + students_enqueue = lambda do |unit_id, preview_only| + queued << [:students, unit_id, preview_only] + 'moodle-import-job' + end + extensions_enqueue = lambda do |unit_id, preview_only| + queued << [:extensions, unit_id, preview_only] + 'moodle-import-job' + end + + ImportMoodleStudentsJob.stub(:perform_async, students_enqueue) do + ImportMoodleExtensionsJob.stub(:perform_async, extensions_enqueue) do + Sidekiq::Status.stub(:get_all, job) do + Sidekiq::Status.stub(:store_for_id, true) do + post "/api/units/#{unit.id}/moodle/import_students", preview_only: true + assert_equal 200, last_response.status, last_response.inspect + + post "/api/units/#{unit.id}/moodle/import_extensions", preview_only: false + assert_equal 200, last_response.status, last_response.inspect + end + end + end + end + + assert_equal [[:students, unit.id, true], [:extensions, unit.id, false]], queued + end +end diff --git a/test/services/moodle_integration_test.rb b/test/services/moodle_integration_test.rb new file mode 100644 index 0000000000..0c6b385854 --- /dev/null +++ b/test/services/moodle_integration_test.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'test_helper' + +class MoodleApiTest < ActiveSupport::TestCase + setup do + settings = Struct.new(:course_id, :api_key, :assignment_id).new(42, 'token', nil) + @integration = MoodleApi.new(settings) + end + + test 'connection test reports each required permission and course assignments' do + assignments = { + 'courses' => [{ + 'id' => 42, + 'fullname' => 'Programming 1', + 'shortname' => 'COS10001', + 'assignments' => [{ 'id' => 7, 'name' => 'Portfolio', 'duedate' => 100 }] + }] + } + students = [{ + 'id' => 12, + 'username' => 'student', + 'roles' => [{ 'shortname' => 'student' }] + }] + + @integration.stub(:assignments, assignments) do + @integration.stub(:students, students) do + @integration.stub(:user_flags, {}) do + @integration.stub(:participant, {}) do + result = @integration.test_connection + + assert_equal 'Programming 1', result[:course]['fullname'] + assert_equal 'Portfolio', result[:assignments].first['name'] + assert_equal %w[mod_assign_get_assignments core_enrol_get_enrolled_users mod_assign_get_user_flags mod_assign_get_participant], result[:permissions].pluck(:function) + assert result[:permissions].all? { |permission| permission[:success] } + end + end + end + end + end + + test 'connection test reports an individual failed permission' do + @integration.stub(:assignments, { 'courses' => [] }) do + @integration.stub(:students, []) do + result = @integration.test_connection + + flags = result[:permissions].find { |permission| permission[:function] == 'mod_assign_get_user_flags' } + participant = result[:permissions].find { |permission| permission[:function] == 'mod_assign_get_participant' } + assert_not flags[:success] + assert_not participant[:success] + end + end + end + + test 'participant table filters still confirm participant permission' do + assignments = { + 'courses' => [{ + 'id' => 42, + 'assignments' => [{ 'id' => 7, 'name' => 'Portfolio' }] + }] + } + students = [{ 'id' => 12, 'roles' => [{ 'shortname' => 'student' }] }] + filtered = MoodleApi::Error.new('User is filtered out', code: 'userisfilteredout') + + @integration.stub(:assignments, assignments) do + @integration.stub(:students, students) do + @integration.stub(:user_flags, {}) do + @integration.stub(:participant, ->(*) { raise filtered }) do + result = @integration.test_connection + permission = result[:permissions].last + + assert permission[:success] + assert_equal 'User is filtered out', permission[:message] + end + end + end + end + end +end diff --git a/test/sidekiq/import_moodle_jobs_test.rb b/test/sidekiq/import_moodle_jobs_test.rb new file mode 100644 index 0000000000..1afd2c01d4 --- /dev/null +++ b/test/sidekiq/import_moodle_jobs_test.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +require 'test_helper' + +class ImportMoodleJobsTest < ActiveSupport::TestCase + test 'student preview reports Moodle data without syncing enrolments' do + unit = FactoryBot.create(:unit, with_students: false, moodle_enabled: true) + integration = unit.create_moodle_integration!(course_id: 42, api_key: 'secret-token') + moodle = Minitest::Mock.new + moodle.expect( + :students, + [{ + 'username' => 'preview.student', + 'idnumber' => '123456', + 'firstname' => 'Preview', + 'lastname' => 'Student', + 'email' => 'preview.student@example.com', + 'roles' => [{ 'shortname' => 'student' }] + }] + ) + stored = nil + job = ImportMoodleStudentsJob.new + + Unit.stub(:find, unit) do + MoodleApi.stub(:new, moodle) do + job.stub(:at, nil) do + job.stub(:total, nil) do + job.stub(:store, ->(**data) { stored = data }) do + unit.stub(:sync_enrolment_with, ->(*) { flunk 'Preview must not sync enrolments' }) do + assert_no_difference -> { unit.projects.count } do + job.perform(unit.id, true) + end + end + end + end + end + end + end + + result = JSON.parse(stored[:result]) + assert_equal 1, result['success'].length + assert_equal 'preview.student', result['success'].first.dig('row', 'username') + assert_equal '123456', result['success'].first.dig('row', 'student_id') + moodle.verify + end + + test 'extension preview reports calculated days without updating the project' do + unit = FactoryBot.create(:unit, with_students: false, moodle_enabled: true) + user = FactoryBot.create(:user, :student, username: 'extension.student') + project = FactoryBot.create(:project, unit: unit, user: user, spec_con_days: 0) + integration = unit.create_moodle_integration!( + course_id: 42, + api_key: 'secret-token', + assignment_id: 7, + assignment_name: 'Portfolio', + fetch_extensions: true + ) + due_date = Time.zone.parse('2026-08-01 09:00:00').to_i + moodle = Minitest::Mock.new + moodle.expect( + :assignments, + { 'courses' => [{ 'id' => 42, 'assignments' => [{ 'id' => 7, 'duedate' => due_date }] }] } + ) + moodle.expect( + :students, + [ + { 'id' => 12, 'username' => user.username }, + { 'id' => 13, 'username' => 'not.enrolled' } + ] + ) + moodle.expect( + :user_flags, + { + 'assignments' => [{ + 'assignmentid' => 7, + 'userflags' => [ + { 'userid' => 12, 'extensionduedate' => due_date + 2.days.to_i }, + { 'userid' => 13, 'extensionduedate' => due_date + 3.days.to_i } + ] + }] + } + ) + stored = nil + job = ImportMoodleExtensionsJob.new + + MoodleApi.stub(:new, moodle) do + job.stub(:at, nil) do + job.stub(:total, nil) do + job.stub(:store, ->(**data) { stored = data }) do + job.perform(unit.id, true) + end + end + end + end + + result = JSON.parse(stored[:result]) + assert_equal 0, project.reload.spec_con_days + assert_equal 2, result['success'].first.dig('row', 'spec_con_days') + assert_equal '2026-08-03', result['success'].first.dig('row', 'extension_date') + assert_equal user.username, result['success'].first.dig('row', 'username') + assert_equal 3, result['ignored'].first.dig('row', 'spec_con_days') + assert_equal '2026-08-04', result['ignored'].first.dig('row', 'extension_date') + assert_equal 'not.enrolled', result['ignored'].first.dig('row', 'username') + moodle.verify + end +end From ef214634dcbc9d0682f32036ce757cd0ed6304df Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Tue, 4 Aug 2026 14:55:08 +1000 Subject: [PATCH 2/6] feat: enable group mappings --- .../entities/moodle_group_mapping_entity.rb | 14 ++ app/api/entities/moodle_integration_entity.rb | 18 ++ app/api/moodle_integration_api.rb | 74 ++++---- app/models/moodle_group_mapping.rb | 49 ++++++ app/models/moodle_integration.rb | 1 + app/services/moodle_api.rb | 9 + app/sidekiq/import_moodle_students_job.rb | 162 ++++++++++++++++-- app/sidekiq/test_moodle_connection_job.rb | 2 +- ...4005203_add_moodle_integration_to_units.rb | 21 +++ db/schema.rb | 23 +++ test/api/moodle_integration_api_test.rb | 18 +- test/services/moodle_integration_test.rb | 38 ++-- test/sidekiq/import_moodle_jobs_test.rb | 74 ++++++++ 13 files changed, 444 insertions(+), 59 deletions(-) create mode 100644 app/api/entities/moodle_group_mapping_entity.rb create mode 100644 app/api/entities/moodle_integration_entity.rb create mode 100644 app/models/moodle_group_mapping.rb diff --git a/app/api/entities/moodle_group_mapping_entity.rb b/app/api/entities/moodle_group_mapping_entity.rb new file mode 100644 index 0000000000..99aae69b74 --- /dev/null +++ b/app/api/entities/moodle_group_mapping_entity.rb @@ -0,0 +1,14 @@ +module Entities + class MoodleGroupMappingEntity < Grape::Entity + expose :id + expose :moodle_group_id + expose :moodle_group_name + expose :target_type + expose :group_set_id + expose :group_id + expose :campus_id + expose :tutorial_stream_id + expose :tutorial_id + expose :create_if_missing + end +end diff --git a/app/api/entities/moodle_integration_entity.rb b/app/api/entities/moodle_integration_entity.rb new file mode 100644 index 0000000000..6b0ea01f79 --- /dev/null +++ b/app/api/entities/moodle_integration_entity.rb @@ -0,0 +1,18 @@ +require 'entities/moodle_group_mapping_entity' + +module Entities + class MoodleIntegrationEntity < Grape::Entity + expose :id + expose :course_id + expose :assignment_id + expose :assignment_name + expose :fetch_extensions + expose :group_mapping_enabled + expose :moodle_group_mappings, + as: :group_mappings, + using: Entities::MoodleGroupMappingEntity + expose :api_key_configured do |integration| + integration.api_key.present? + end + end +end diff --git a/app/api/moodle_integration_api.rb b/app/api/moodle_integration_api.rb index bb29c060c2..6640564758 100644 --- a/app/api/moodle_integration_api.rb +++ b/app/api/moodle_integration_api.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'grape' +require 'entities/moodle_integration_entity' require 'entities/sidekiq_job_entity' class MoodleIntegrationApi < Grape::API @@ -20,18 +21,8 @@ class MoodleIntegrationApi < Grape::API error!({ error: 'Not authorised to manage Moodle for this unit' }, 403) end - integration = unit.moodle_integration - present( - { - id: integration&.id, - course_id: integration&.course_id, - assignment_id: integration&.assignment_id, - assignment_name: integration&.assignment_name, - fetch_extensions: integration&.fetch_extensions || false, - api_key_configured: integration&.api_key.present? - }, - with: Grape::Presenters::Presenter - ) + integration = unit.moodle_integration || unit.build_moodle_integration + present integration, with: Entities::MoodleIntegrationEntity end desc 'Update Moodle settings for a unit' @@ -41,6 +32,18 @@ class MoodleIntegrationApi < Grape::API optional :assignment_id, type: Integer optional :assignment_name, type: String optional :fetch_extensions, type: Boolean, default: false + optional :group_mapping_enabled, type: Boolean, default: false + optional :group_mappings, type: Array do + requires :moodle_group_id, type: Integer + requires :moodle_group_name, type: String + requires :target_type, type: String, values: MoodleGroupMapping::TARGET_TYPES + optional :group_set_id, type: Integer + optional :group_id, type: Integer + optional :campus_id, type: Integer + optional :tutorial_stream_id, type: Integer + optional :tutorial_id, type: Integer + optional :create_if_missing, type: Boolean, default: false + end end put '/units/:unit_id/moodle' do unit = Unit.find(params[:unit_id]) @@ -50,24 +53,35 @@ class MoodleIntegrationApi < Grape::API end integration = unit.moodle_integration || unit.build_moodle_integration - integration.course_id = params[:course_id] - integration.api_key = params[:api_key] if params[:api_key].present? - integration.fetch_extensions = params[:fetch_extensions] - integration.assignment_id = params[:fetch_extensions] ? params[:assignment_id] : nil - integration.assignment_name = params[:fetch_extensions] ? params[:assignment_name] : nil - integration.save! - - present( - { - id: integration.id, - course_id: integration.course_id, - assignment_id: integration.assignment_id, - assignment_name: integration.assignment_name, - fetch_extensions: integration.fetch_extensions, - api_key_configured: integration.api_key.present? - }, - with: Grape::Presenters::Presenter - ) + MoodleIntegration.transaction do + integration.course_id = params[:course_id] + integration.api_key = params[:api_key] if params[:api_key].present? + integration.fetch_extensions = params[:fetch_extensions] + integration.assignment_id = params[:fetch_extensions] ? params[:assignment_id] : nil + integration.assignment_name = params[:fetch_extensions] ? params[:assignment_name] : nil + integration.group_mapping_enabled = params[:group_mapping_enabled] + integration.save! + + if integration.group_mapping_enabled? + integration.moodle_group_mappings.delete_all + Array(params[:group_mappings]).each do |mapping| + integration.moodle_group_mappings.create!( + moodle_group_id: mapping[:moodle_group_id], + moodle_group_name: mapping[:moodle_group_name], + target_type: mapping[:target_type], + group_set_id: mapping[:group_set_id], + group_id: mapping[:group_id], + campus_id: mapping[:campus_id], + tutorial_stream_id: mapping[:tutorial_stream_id], + tutorial_id: mapping[:tutorial_id], + create_if_missing: mapping[:create_if_missing] + ) + end + end + end + + integration.moodle_group_mappings.reload + present integration, with: Entities::MoodleIntegrationEntity end desc 'Test Moodle API permissions for a unit' diff --git a/app/models/moodle_group_mapping.rb b/app/models/moodle_group_mapping.rb new file mode 100644 index 0000000000..c9df862da9 --- /dev/null +++ b/app/models/moodle_group_mapping.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +class MoodleGroupMapping < ApplicationRecord + TARGET_TYPES = %w[group campus tutorial].freeze + + belongs_to :moodle_integration + belongs_to :group_set, optional: true + belongs_to :group, optional: true + belongs_to :campus, optional: true + belongs_to :tutorial_stream, optional: true + belongs_to :tutorial, optional: true + + validates :moodle_group_id, numericality: { only_integer: true, greater_than: 0 } + validates :moodle_group_id, uniqueness: { scope: :moodle_integration_id } + validates :moodle_group_name, presence: true + validates :target_type, inclusion: { in: TARGET_TYPES } + validate :valid_target + + private + + def valid_target + unit = moodle_integration&.unit + + case target_type + when 'group' + errors.add(:group_set, 'must be selected') if group_set.blank? + errors.add(:group_set, 'must belong to this unit') if group_set.present? && group_set.unit != unit + unless create_if_missing? + errors.add(:group, 'must be selected') if group.blank? + if group.present? && (group.group_set != group_set || group.unit != unit) + errors.add(:group, 'must belong to the selected group set') + end + end + when 'campus' + errors.add(:campus, 'must be selected') if campus.blank? + when 'tutorial' + errors.add(:tutorial_stream, 'must be selected') if tutorial_stream.blank? + if tutorial_stream.present? && tutorial_stream.unit != unit + errors.add(:tutorial_stream, 'must belong to this unit') + end + unless create_if_missing? + errors.add(:tutorial, 'must be selected') if tutorial.blank? + if tutorial.present? && (tutorial.tutorial_stream != tutorial_stream || tutorial.unit != unit) + errors.add(:tutorial, 'must belong to the selected tutorial stream') + end + end + end + end +end diff --git a/app/models/moodle_integration.rb b/app/models/moodle_integration.rb index 25854894a2..8ce1539613 100644 --- a/app/models/moodle_integration.rb +++ b/app/models/moodle_integration.rb @@ -2,6 +2,7 @@ class MoodleIntegration < ApplicationRecord belongs_to :unit + has_many :moodle_group_mappings, dependent: :destroy encrypts :api_key diff --git a/app/services/moodle_api.rb b/app/services/moodle_api.rb index a4aa3cc7e2..8bc37bc3eb 100644 --- a/app/services/moodle_api.rb +++ b/app/services/moodle_api.rb @@ -39,6 +39,10 @@ def participant(assignment_id, user_id) request('mod_assign_get_participant', 'assignid' => assignment_id, 'userid' => user_id, 'embeduser' => 0) end + def course_groups + request('core_group_get_course_groups', 'courseid' => @integration.course_id) + end + def test_connection(progress_callback: nil) results = [] progress_callback&.call(1, 'Fetching course assignments') @@ -71,9 +75,14 @@ def test_connection(progress_callback: nil) participant(assignment_id, participant_user['id']) end + progress_callback&.call(5, 'Fetching course groups') + groups = test_function(results, 'core_group_get_course_groups') { course_groups } + available_groups = Array(groups) + { course: course&.slice('id', 'fullname', 'shortname'), assignments: available_assignments.map { |item| item.slice('id', 'name', 'duedate') }, + groups: available_groups.map { |item| item.slice('id', 'name', 'idnumber') }, permissions: results } end diff --git a/app/sidekiq/import_moodle_students_job.rb b/app/sidekiq/import_moodle_students_job.rb index 1a45d75d70..3632eb7941 100644 --- a/app/sidekiq/import_moodle_students_job.rb +++ b/app/sidekiq/import_moodle_students_job.rb @@ -19,12 +19,18 @@ def perform(unit_id, preview_only) integration = unit.moodle_integration raise MoodleApi::Error, 'Configure Moodle for this unit first' if integration.blank? - students = MoodleApi.new(integration).students.select do |student| + moodle = MoodleApi.new(integration) + students = moodle.students.select do |student| Array(student['roles']).any? { |role| role['shortname'] == 'student' } end + mappings = integration.group_mapping_enabled? ? integration.moodle_group_mappings.includes(:group_set, :group, :campus, :tutorial_stream, :tutorial) : [] + mappings_by_group_id = mappings.index_by(&:moodle_group_id) total(students.length) rows = students.map do |student| + student_mappings = Array(student['groups']).filter_map do |group| + mappings_by_group_id[group['id'].to_i] + end display_row = { unit_code: unit.code, username: student['username'], @@ -33,20 +39,17 @@ def perform(unit_id, preview_only) last_name: student['lastname'], nickname: student['firstname'], email: student['email'], + moodle_groups: student_mappings.map(&:moodle_group_name).join(', '), + mapped_campus: student_mappings.filter_map { |mapping| mapping.campus&.name }.uniq.join(', '), + mapped_tutorial: student_mappings.select { |mapping| mapping.target_type == 'tutorial' }.map { |mapping| mapping.tutorial&.abbreviation || mapping.moodle_group_name }.uniq.join(', '), + mapped_group: student_mappings.select { |mapping| mapping.target_type == 'group' }.map { |mapping| mapping.group&.name || mapping.moodle_group_name }.uniq.join(', '), enrolled: true } - display_row.merge(row: display_row, tutorials: [], campus: nil) + display_row.merge(row: display_row, tutorials: [], campus: nil, moodle_mappings: student_mappings) end result = if preview_only - { - success: rows.map.with_index(1) do |row, index| - at(index, "Reading Moodle student #{row[:username]}") - { row: row[:row], message: 'Student data fetched from Moodle' } - end, - ignored: [], - errors: [] - } + preview_students(rows) else import_students(unit, rows) end @@ -56,10 +59,23 @@ def perform(unit_id, preview_only) private + def preview_students(rows) + result = { success: [], ignored: [], errors: [] } + rows.each_with_index do |row, index| + at(index + 1, "Reading Moodle student #{row[:username]}") + errors = mapping_errors(row[:moodle_mappings]) + result[errors.empty? ? :success : :errors] << { + row: row[:row], + message: errors.empty? ? 'Student data and group mappings fetched from Moodle' : errors.join('; ') + } + end + result + end + def import_students(unit, rows) result = { success: [], ignored: [], errors: [] } unit.sync_enrolment_with( - rows, + rows.map { |row| row.except(:moodle_mappings) }, { replace_existing_tutorial: false, replace_existing_campus: false, @@ -71,6 +87,130 @@ def import_students(unit, rows) at(rows_processed, message) if rows_processed } ) + + rows.each do |row| + next if row[:moodle_mappings].empty? + + project = unit.projects.joins(:user).find_by(users: { username: row[:username] }) + errors = mapping_errors(row[:moodle_mappings]) + errors << 'Student could not be found after import' unless project + mappings_changed = false + if errors.empty? + begin + mappings_changed = apply_mappings(project, row[:moodle_mappings]) + rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved, ActiveRecord::RecordNotUnique => e + errors << "Group mapping failed: #{e.message}" + end + end + + existing_entry = nil + existing_type = nil + [:success, :ignored].each do |type| + entry = result[type].find { |item| item[:row][:username] == row[:username] } + next unless entry + + existing_entry = entry + existing_type = type + result[type].delete(entry) if errors.present? || (mappings_changed && type == :ignored) + end + + if errors.present? + result[:errors] << { row: row[:row], message: errors.join('; ') } + elsif mappings_changed && existing_entry + existing_entry[:message] = "#{existing_entry[:message].to_s.delete_suffix('.')}; Moodle group mappings updated" + result[:success] << existing_entry unless result[:success].include?(existing_entry) + elsif mappings_changed + result[:success] << { row: row[:row], message: 'Moodle group mappings updated' } + elsif existing_entry + existing_entry[:message] = "#{existing_entry[:message].to_s.delete_suffix('.')}; Moodle group mappings unchanged" + elsif existing_type.nil? + result[:ignored] << { row: row[:row], message: 'No change; Moodle group mappings unchanged' } + end + end result end + + def mapping_errors(mappings) + errors = [] + campus_mappings = mappings.select { |mapping| mapping.target_type == 'campus' } + errors << 'Student belongs to multiple mapped campuses' if campus_mappings.map(&:campus_id).uniq.length > 1 + + mappings.select { |mapping| mapping.target_type == 'tutorial' } + .group_by(&:tutorial_stream_id) + .each_value do |stream_mappings| + errors << 'Student belongs to multiple Moodle groups mapped to the same tutorial stream' if stream_mappings.length > 1 + end + mappings.select { |mapping| mapping.target_type == 'group' } + .group_by(&:group_set_id) + .each_value do |group_mappings| + errors << 'Student belongs to multiple Moodle groups mapped to the same group set' if group_mappings.length > 1 + end + + mappings.each do |mapping| + case mapping.target_type + when 'group' + errors << "Select an existing group in #{mapping.group_set.name}" if !mapping.create_if_missing? && mapping.group.blank? + when 'tutorial' + if !mapping.create_if_missing? && mapping.tutorial.blank? + errors << "Select an existing tutorial in #{mapping.tutorial_stream.name}" + end + end + end + errors + end + + def apply_mappings(project, mappings) + ActiveRecord::Base.transaction do + changed = false + campus = mappings.find { |mapping| mapping.target_type == 'campus' }&.campus + if campus && project.campus_id != campus.id + project.update!(campus: campus) + changed = true + end + + mappings.select { |mapping| mapping.target_type == 'tutorial' }.each do |mapping| + tutorial = mapping.tutorial + if mapping.create_if_missing? + tutorial = mapping.tutorial_stream.tutorials.where( + unit: project.unit + ).where('LOWER(abbreviation) = ?', mapping.moodle_group_name.downcase).first + tutorial ||= Tutorial.create!( + unit: project.unit, + tutorial_stream: mapping.tutorial_stream, + abbreviation: mapping.moodle_group_name, + meeting_day: 'Moodle', + meeting_time: '', + meeting_location: mapping.moodle_group_name + ) + end + if project.tutorial_for_stream(mapping.tutorial_stream)&.id != tutorial.id + project.enrol_in(tutorial) + changed = true + end + end + + mappings.select { |mapping| mapping.target_type == 'group' }.each do |mapping| + group = mapping.group + if mapping.create_if_missing? + group = mapping.group_set.groups.where('LOWER(name) = ?', mapping.moodle_group_name.downcase).first + tutorial = project.tutorial_enrolments.includes(:tutorial).first&.tutorial || + mapping.group_set.groups.first&.tutorial || + project.unit.tutorials.first + if group.blank? && tutorial.blank? + raise ActiveRecord::RecordNotSaved, 'A tutorial is required before a group can be created' + end + group ||= Group.create!( + group_set: mapping.group_set, + tutorial: tutorial, + name: mapping.moodle_group_name + ) + end + if project.group_for_groupset(mapping.group_set)&.id != group.id + group.add_member(project) + changed = true + end + end + changed + end + end end diff --git a/app/sidekiq/test_moodle_connection_job.rb b/app/sidekiq/test_moodle_connection_job.rb index fe04db3cc5..f3e900f91f 100644 --- a/app/sidekiq/test_moodle_connection_job.rb +++ b/app/sidekiq/test_moodle_connection_job.rb @@ -10,7 +10,7 @@ class TestMoodleConnectionJob retry: false def perform(unit_id) - total(4) + total(5) at(0, 'Starting Moodle connection test') unit = Unit.find(unit_id) diff --git a/db/migrate/20260804005203_add_moodle_integration_to_units.rb b/db/migrate/20260804005203_add_moodle_integration_to_units.rb index 0e2d75700e..720bba5655 100644 --- a/db/migrate/20260804005203_add_moodle_integration_to_units.rb +++ b/db/migrate/20260804005203_add_moodle_integration_to_units.rb @@ -9,8 +9,29 @@ def change t.bigint :assignment_id t.string :assignment_name t.boolean :fetch_extensions, null: false, default: false + t.boolean :group_mapping_enabled, null: false, default: false t.timestamps end + + create_table :moodle_group_mappings do |t| + t.references :moodle_integration, null: false + t.bigint :moodle_group_id, null: false + t.string :moodle_group_name, null: false + t.string :target_type, null: false + t.references :group_set + t.references :group + t.references :campus + t.references :tutorial_stream + t.references :tutorial + t.boolean :create_if_missing, null: false, default: false + + t.timestamps + end + + add_index :moodle_group_mappings, + [:moodle_integration_id, :moodle_group_id], + unique: true, + name: 'index_moodle_group_mappings_on_integration_and_group' end end diff --git a/db/schema.rb b/db/schema.rb index 8d040b5728..cfb2709751 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -352,6 +352,28 @@ t.index ["task_id"], name: "index_moderated_tasks_on_task_id" end + create_table "moodle_group_mappings", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| + t.bigint "moodle_integration_id", null: false + t.bigint "moodle_group_id", null: false + t.string "moodle_group_name", null: false + t.string "target_type", null: false + t.bigint "group_set_id" + t.bigint "group_id" + t.bigint "campus_id" + t.bigint "tutorial_stream_id" + t.bigint "tutorial_id" + t.boolean "create_if_missing", default: false, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["campus_id"], name: "index_moodle_group_mappings_on_campus_id" + t.index ["group_id"], name: "index_moodle_group_mappings_on_group_id" + t.index ["group_set_id"], name: "index_moodle_group_mappings_on_group_set_id" + t.index ["moodle_integration_id", "moodle_group_id"], name: "index_moodle_group_mappings_on_integration_and_group", unique: true + t.index ["moodle_integration_id"], name: "index_moodle_group_mappings_on_moodle_integration_id" + t.index ["tutorial_id"], name: "index_moodle_group_mappings_on_tutorial_id" + t.index ["tutorial_stream_id"], name: "index_moodle_group_mappings_on_tutorial_stream_id" + end + create_table "moodle_integrations", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| t.bigint "unit_id", null: false t.bigint "course_id", null: false @@ -359,6 +381,7 @@ t.bigint "assignment_id" t.string "assignment_name" t.boolean "fetch_extensions", default: false, null: false + t.boolean "group_mapping_enabled", default: false, null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false t.index ["unit_id"], name: "index_moodle_integrations_on_unit_id", unique: true diff --git a/test/api/moodle_integration_api_test.rb b/test/api/moodle_integration_api_test.rb index 922fd639bb..510fc02878 100644 --- a/test/api/moodle_integration_api_test.rb +++ b/test/api/moodle_integration_api_test.rb @@ -20,7 +20,15 @@ def app api_key: 'secret-token', assignment_id: 7, assignment_name: 'Portfolio', - fetch_extensions: true + fetch_extensions: true, + group_mapping_enabled: true, + group_mappings: [{ + moodle_group_id: 31, + moodle_group_name: 'Hawthorn', + target_type: 'campus', + campus_id: FactoryBot.create(:campus).id, + create_if_missing: false + }] } assert_equal 200, last_response.status, last_response.inspect @@ -29,6 +37,8 @@ def app assert_equal 'secret-token', integration.api_key assert_equal 'Portfolio', integration.assignment_name assert integration.fetch_extensions + assert integration.group_mapping_enabled + assert_equal 'Hawthorn', integration.moodle_group_mappings.first.moodle_group_name assert_equal integration.id, last_response_body['id'] assert_equal true, last_response_body['api_key_configured'] assert_not last_response.body.include?('secret-token') @@ -38,6 +48,8 @@ def app assert_equal 7, last_response_body['assignment_id'] assert_equal 'Portfolio', last_response_body['assignment_name'] assert_equal true, last_response_body['fetch_extensions'] + assert_equal true, last_response_body['group_mapping_enabled'] + assert_equal 31, last_response_body['group_mappings'].first['moodle_group_id'] assert_nil last_response_body['api_key'] end @@ -72,7 +84,7 @@ def app 'jid' => 'moodle-job-id', 'status' => 'queued', 'at' => 0, - 'total' => 4 + 'total' => 5 } TestMoodleConnectionJob.stub(:perform_async, 'moodle-job-id') do @@ -85,7 +97,7 @@ def app assert_equal 200, last_response.status, last_response.inspect assert_equal 'moodle-job-id', last_response_body['id'] - assert_equal 4, last_response_body['total_count'].to_i + assert_equal 5, last_response_body['total_count'].to_i end test 'Moodle imports enqueue preview and import jobs' do diff --git a/test/services/moodle_integration_test.rb b/test/services/moodle_integration_test.rb index 0c6b385854..a23ab84fb5 100644 --- a/test/services/moodle_integration_test.rb +++ b/test/services/moodle_integration_test.rb @@ -22,17 +22,21 @@ class MoodleApiTest < ActiveSupport::TestCase 'username' => 'student', 'roles' => [{ 'shortname' => 'student' }] }] + groups = [{ 'id' => 31, 'name' => 'Tutorial A', 'idnumber' => 'T-A' }] @integration.stub(:assignments, assignments) do @integration.stub(:students, students) do @integration.stub(:user_flags, {}) do @integration.stub(:participant, {}) do - result = @integration.test_connection + @integration.stub(:course_groups, groups) do + result = @integration.test_connection - assert_equal 'Programming 1', result[:course]['fullname'] - assert_equal 'Portfolio', result[:assignments].first['name'] - assert_equal %w[mod_assign_get_assignments core_enrol_get_enrolled_users mod_assign_get_user_flags mod_assign_get_participant], result[:permissions].pluck(:function) - assert result[:permissions].all? { |permission| permission[:success] } + assert_equal 'Programming 1', result[:course]['fullname'] + assert_equal 'Portfolio', result[:assignments].first['name'] + assert_equal 'Tutorial A', result[:groups].first['name'] + assert_equal %w[mod_assign_get_assignments core_enrol_get_enrolled_users mod_assign_get_user_flags mod_assign_get_participant core_group_get_course_groups], result[:permissions].pluck(:function) + assert(result[:permissions].all? { |permission| permission[:success] }) + end end end end @@ -42,12 +46,14 @@ class MoodleApiTest < ActiveSupport::TestCase test 'connection test reports an individual failed permission' do @integration.stub(:assignments, { 'courses' => [] }) do @integration.stub(:students, []) do - result = @integration.test_connection + @integration.stub(:course_groups, []) do + result = @integration.test_connection - flags = result[:permissions].find { |permission| permission[:function] == 'mod_assign_get_user_flags' } - participant = result[:permissions].find { |permission| permission[:function] == 'mod_assign_get_participant' } - assert_not flags[:success] - assert_not participant[:success] + flags = result[:permissions].find { |permission| permission[:function] == 'mod_assign_get_user_flags' } + participant = result[:permissions].find { |permission| permission[:function] == 'mod_assign_get_participant' } + assert_not flags[:success] + assert_not participant[:success] + end end end end @@ -66,11 +72,15 @@ class MoodleApiTest < ActiveSupport::TestCase @integration.stub(:students, students) do @integration.stub(:user_flags, {}) do @integration.stub(:participant, ->(*) { raise filtered }) do - result = @integration.test_connection - permission = result[:permissions].last + @integration.stub(:course_groups, []) do + result = @integration.test_connection + permission = result[:permissions].find do |item| + item[:function] == 'mod_assign_get_participant' + end - assert permission[:success] - assert_equal 'User is filtered out', permission[:message] + assert permission[:success] + assert_equal 'User is filtered out', permission[:message] + end end end end diff --git a/test/sidekiq/import_moodle_jobs_test.rb b/test/sidekiq/import_moodle_jobs_test.rb index 1afd2c01d4..d46c753caf 100644 --- a/test/sidekiq/import_moodle_jobs_test.rb +++ b/test/sidekiq/import_moodle_jobs_test.rb @@ -44,6 +44,80 @@ class ImportMoodleJobsTest < ActiveSupport::TestCase moodle.verify end + test 'student preview reports configured Moodle group mappings without changing students' do + unit = FactoryBot.create(:unit, with_students: false, moodle_enabled: true) + campus = FactoryBot.create(:campus) + integration = unit.create_moodle_integration!( + course_id: 42, + api_key: 'secret-token', + group_mapping_enabled: true + ) + integration.moodle_group_mappings.create!( + moodle_group_id: 31, + moodle_group_name: 'City students', + target_type: 'campus', + campus: campus + ) + moodle = Minitest::Mock.new + moodle.expect( + :students, + [{ + 'id' => 12, + 'username' => 'group.student', + 'idnumber' => '654321', + 'firstname' => 'Group', + 'lastname' => 'Student', + 'email' => 'group.student@example.com', + 'groups' => [{ 'id' => 31, 'name' => 'City students' }], + 'roles' => [{ 'shortname' => 'student' }] + }] + ) + stored = nil + job = ImportMoodleStudentsJob.new + + MoodleApi.stub(:new, moodle) do + job.stub(:at, nil) do + job.stub(:total, nil) do + job.stub(:store, ->(**data) { stored = data }) do + assert_no_difference -> { unit.projects.count } do + job.perform(unit.id, true) + end + end + end + end + end + + result = JSON.parse(stored[:result]) + assert_equal campus.name, result['success'].first.dig('row', 'mapped_campus') + assert_equal 'City students', result['success'].first.dig('row', 'moodle_groups') + moodle.verify + end + + test 'student group mapping creates a missing group using a unit tutorial' do + unit = FactoryBot.create(:unit, with_students: false, moodle_enabled: true) + tutorial = FactoryBot.create(:tutorial, unit: unit) + group_set = FactoryBot.create(:group_set, unit: unit) + project = FactoryBot.create(:project, unit: unit) + integration = unit.create_moodle_integration!(course_id: 42, api_key: 'secret-token') + mapping = integration.moodle_group_mappings.create!( + moodle_group_id: 31, + moodle_group_name: 'Moodle Group 1', + target_type: 'group', + group_set: group_set, + create_if_missing: true + ) + + job = ImportMoodleStudentsJob.new + assert_difference -> { group_set.groups.count }, 1 do + assert job.send(:apply_mappings, project, [mapping]) + end + + group = group_set.groups.find_by!(name: 'Moodle Group 1') + assert_equal tutorial, group.tutorial + assert_equal group, project.reload.group_for_groupset(group_set) + assert_not job.send(:apply_mappings, project.reload, [mapping]) + end + test 'extension preview reports calculated days without updating the project' do unit = FactoryBot.create(:unit, with_students: false, moodle_enabled: true) user = FactoryBot.create(:user, :student, username: 'extension.student') From e2aca49a6296d0a5428e42991df44316e06ec836 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:03:31 +1000 Subject: [PATCH 3/6] fix: tests --- test/api/moodle_integration_api_test.rb | 7 ++++--- test/services/moodle_integration_test.rb | 1 + test/sidekiq/import_moodle_jobs_test.rb | 6 ++++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/test/api/moodle_integration_api_test.rb b/test/api/moodle_integration_api_test.rb index 510fc02878..38dcc9fb03 100644 --- a/test/api/moodle_integration_api_test.rb +++ b/test/api/moodle_integration_api_test.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'test_helper' +require 'minitest/mock' class MoodleIntegrationApiTest < ActiveSupport::TestCase include Rack::Test::Methods @@ -95,7 +96,7 @@ def app end end - assert_equal 200, last_response.status, last_response.inspect + assert_equal 201, last_response.status, last_response.inspect assert_equal 'moodle-job-id', last_response_body['id'] assert_equal 5, last_response_body['total_count'].to_i end @@ -127,10 +128,10 @@ def app Sidekiq::Status.stub(:get_all, job) do Sidekiq::Status.stub(:store_for_id, true) do post "/api/units/#{unit.id}/moodle/import_students", preview_only: true - assert_equal 200, last_response.status, last_response.inspect + assert_equal 201, last_response.status, last_response.inspect post "/api/units/#{unit.id}/moodle/import_extensions", preview_only: false - assert_equal 200, last_response.status, last_response.inspect + assert_equal 201, last_response.status, last_response.inspect end end end diff --git a/test/services/moodle_integration_test.rb b/test/services/moodle_integration_test.rb index a23ab84fb5..8494518019 100644 --- a/test/services/moodle_integration_test.rb +++ b/test/services/moodle_integration_test.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'test_helper' +require 'minitest/mock' class MoodleApiTest < ActiveSupport::TestCase setup do diff --git a/test/sidekiq/import_moodle_jobs_test.rb b/test/sidekiq/import_moodle_jobs_test.rb index d46c753caf..b37b31e06e 100644 --- a/test/sidekiq/import_moodle_jobs_test.rb +++ b/test/sidekiq/import_moodle_jobs_test.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'test_helper' +require 'minitest/mock' class ImportMoodleJobsTest < ActiveSupport::TestCase test 'student preview reports Moodle data without syncing enrolments' do @@ -95,9 +96,10 @@ class ImportMoodleJobsTest < ActiveSupport::TestCase test 'student group mapping creates a missing group using a unit tutorial' do unit = FactoryBot.create(:unit, with_students: false, moodle_enabled: true) - tutorial = FactoryBot.create(:tutorial, unit: unit) - group_set = FactoryBot.create(:group_set, unit: unit) project = FactoryBot.create(:project, unit: unit) + tutorial = FactoryBot.create(:tutorial, unit: unit, campus: project.campus) + group_set = FactoryBot.create(:group_set, unit: unit) + TutorialEnrolment.create!(project: project, tutorial: tutorial) integration = unit.create_moodle_integration!(course_id: 42, api_key: 'secret-token') mapping = integration.moodle_group_mappings.create!( moodle_group_id: 31, From beda385421c4c5fe81ad667ff122166761ac2f7f Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:16:56 +1000 Subject: [PATCH 4/6] refactor: use existing or create new tutorial --- app/models/moodle_group_mapping.rb | 10 ++++++- app/sidekiq/import_moodle_students_job.rb | 30 ++++++++++++++++----- test/sidekiq/import_moodle_jobs_test.rb | 33 ++++++++++++++++++++++- 3 files changed, 65 insertions(+), 8 deletions(-) diff --git a/app/models/moodle_group_mapping.rb b/app/models/moodle_group_mapping.rb index c9df862da9..9a0e0fa5bd 100644 --- a/app/models/moodle_group_mapping.rb +++ b/app/models/moodle_group_mapping.rb @@ -25,7 +25,15 @@ def valid_target when 'group' errors.add(:group_set, 'must be selected') if group_set.blank? errors.add(:group_set, 'must belong to this unit') if group_set.present? && group_set.unit != unit - unless create_if_missing? + if create_if_missing? + if tutorial.blank? == tutorial_stream.blank? + errors.add(:base, 'select an existing tutorial or a tutorial stream for the new group') + end + errors.add(:tutorial, 'must belong to this unit') if tutorial.present? && tutorial.unit != unit + if tutorial_stream.present? && tutorial_stream.unit != unit + errors.add(:tutorial_stream, 'must belong to this unit') + end + else errors.add(:group, 'must be selected') if group.blank? if group.present? && (group.group_set != group_set || group.unit != unit) errors.add(:group, 'must belong to the selected group set') diff --git a/app/sidekiq/import_moodle_students_job.rb b/app/sidekiq/import_moodle_students_job.rb index 3632eb7941..90192b757e 100644 --- a/app/sidekiq/import_moodle_students_job.rb +++ b/app/sidekiq/import_moodle_students_job.rb @@ -149,7 +149,13 @@ def mapping_errors(mappings) mappings.each do |mapping| case mapping.target_type when 'group' - errors << "Select an existing group in #{mapping.group_set.name}" if !mapping.create_if_missing? && mapping.group.blank? + if mapping.create_if_missing? + if mapping.tutorial.blank? == mapping.tutorial_stream.blank? + errors << 'Select an existing tutorial or a tutorial stream for the new group' + end + elsif mapping.group.blank? + errors << "Select an existing group in #{mapping.group_set.name}" + end when 'tutorial' if !mapping.create_if_missing? && mapping.tutorial.blank? errors << "Select an existing tutorial in #{mapping.tutorial_stream.name}" @@ -193,17 +199,29 @@ def apply_mappings(project, mappings) group = mapping.group if mapping.create_if_missing? group = mapping.group_set.groups.where('LOWER(name) = ?', mapping.moodle_group_name.downcase).first - tutorial = project.tutorial_enrolments.includes(:tutorial).first&.tutorial || - mapping.group_set.groups.first&.tutorial || - project.unit.tutorials.first - if group.blank? && tutorial.blank? - raise ActiveRecord::RecordNotSaved, 'A tutorial is required before a group can be created' + tutorial = group&.tutorial || mapping.tutorial + if tutorial.blank? + tutorial = mapping.tutorial_stream.tutorials.where( + unit: project.unit + ).where('LOWER(abbreviation) = ?', mapping.moodle_group_name.downcase).first + tutorial ||= Tutorial.create!( + unit: project.unit, + tutorial_stream: mapping.tutorial_stream, + abbreviation: mapping.moodle_group_name, + meeting_day: 'Moodle', + meeting_time: '', + meeting_location: mapping.moodle_group_name + ) end group ||= Group.create!( group_set: mapping.group_set, tutorial: tutorial, name: mapping.moodle_group_name ) + if project.tutorial_for_stream(tutorial.tutorial_stream)&.id != tutorial.id + project.enrol_in(tutorial) + changed = true + end end if project.group_for_groupset(mapping.group_set)&.id != group.id group.add_member(project) diff --git a/test/sidekiq/import_moodle_jobs_test.rb b/test/sidekiq/import_moodle_jobs_test.rb index b37b31e06e..6ca4f52807 100644 --- a/test/sidekiq/import_moodle_jobs_test.rb +++ b/test/sidekiq/import_moodle_jobs_test.rb @@ -99,13 +99,13 @@ class ImportMoodleJobsTest < ActiveSupport::TestCase project = FactoryBot.create(:project, unit: unit) tutorial = FactoryBot.create(:tutorial, unit: unit, campus: project.campus) group_set = FactoryBot.create(:group_set, unit: unit) - TutorialEnrolment.create!(project: project, tutorial: tutorial) integration = unit.create_moodle_integration!(course_id: 42, api_key: 'secret-token') mapping = integration.moodle_group_mappings.create!( moodle_group_id: 31, moodle_group_name: 'Moodle Group 1', target_type: 'group', group_set: group_set, + tutorial: tutorial, create_if_missing: true ) @@ -116,10 +116,41 @@ class ImportMoodleJobsTest < ActiveSupport::TestCase group = group_set.groups.find_by!(name: 'Moodle Group 1') assert_equal tutorial, group.tutorial + assert_equal tutorial, project.reload.tutorial_for_stream(tutorial.tutorial_stream) assert_equal group, project.reload.group_for_groupset(group_set) assert_not job.send(:apply_mappings, project.reload, [mapping]) end + test 'student group mapping creates a matching tutorial and group' do + unit = FactoryBot.create(:unit, with_students: false, moodle_enabled: true) + project = FactoryBot.create(:project, unit: unit) + tutorial_stream = FactoryBot.create(:tutorial_stream, unit: unit) + group_set = FactoryBot.create(:group_set, unit: unit) + integration = unit.create_moodle_integration!(course_id: 42, api_key: 'secret-token') + mapping = integration.moodle_group_mappings.create!( + moodle_group_id: 32, + moodle_group_name: 'Moodle Group 2', + target_type: 'group', + group_set: group_set, + tutorial_stream: tutorial_stream, + create_if_missing: true + ) + + job = ImportMoodleStudentsJob.new + assert_difference -> { group_set.groups.count }, 1 do + assert_difference -> { tutorial_stream.tutorials.count }, 1 do + assert job.send(:apply_mappings, project, [mapping]) + end + end + + tutorial = tutorial_stream.tutorials.find_by!(abbreviation: 'Moodle Group 2') + group = group_set.groups.find_by!(name: 'Moodle Group 2') + assert_equal tutorial, group.tutorial + assert_equal tutorial, project.reload.tutorial_for_stream(tutorial_stream) + assert_equal group, project.group_for_groupset(group_set) + assert_not job.send(:apply_mappings, project.reload, [mapping]) + end + test 'extension preview reports calculated days without updating the project' do unit = FactoryBot.create(:unit, with_students: false, moodle_enabled: true) user = FactoryBot.create(:user, :student, username: 'extension.student') From 4b7df21440af979ef35b71827e4f2420a8704837 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:16:06 +1000 Subject: [PATCH 5/6] feat: expose assignments due date --- app/services/moodle_api.rb | 23 ++++--- app/sidekiq/test_moodle_connection_job.rb | 2 +- test/api/moodle_integration_api_test.rb | 4 +- test/services/moodle_integration_test.rb | 77 ++++++++++++++--------- 4 files changed, 66 insertions(+), 40 deletions(-) diff --git a/app/services/moodle_api.rb b/app/services/moodle_api.rb index 8bc37bc3eb..9ec076670e 100644 --- a/app/services/moodle_api.rb +++ b/app/services/moodle_api.rb @@ -22,6 +22,10 @@ def assignments request('mod_assign_get_assignments', 'courseids[0]' => @integration.course_id) end + def course_details + request('core_course_get_courses_by_field', 'field' => 'id', 'value' => @integration.course_id) + end + def students request( 'core_enrol_get_enrolled_users', @@ -47,26 +51,31 @@ def test_connection(progress_callback: nil) results = [] progress_callback&.call(1, 'Fetching course assignments') assignment_response = test_function(results, 'mod_assign_get_assignments') { assignments } - progress_callback&.call(2, 'Fetching enrolled users') + progress_callback&.call(2, 'Fetching course details') + course_response = test_function(results, 'core_course_get_courses_by_field') { course_details } + progress_callback&.call(3, 'Fetching enrolled users') enrolled_users = test_function(results, 'core_enrol_get_enrolled_users') { students } - course = Array(assignment_response&.fetch('courses', nil)).find do |item| + assignment_course = Array(assignment_response&.fetch('courses', nil)).find do |item| item['id'].to_i == @integration.course_id end - available_assignments = Array(course&.fetch('assignments', nil)) + course = Array(course_response&.fetch('courses', nil)).find do |item| + item['id'].to_i == @integration.course_id + end || assignment_course + available_assignments = Array(assignment_course&.fetch('assignments', nil)) assignment_id = @integration.assignment_id || available_assignments.first&.fetch('id', nil) participant_user = Array(enrolled_users).find do |user| Array(user['roles']).any? { |role| role['shortname'] == 'student' } end - progress_callback&.call(3, 'Testing assignment flag access') + progress_callback&.call(4, 'Testing assignment flag access') test_function(results, 'mod_assign_get_user_flags') do raise Error, 'No assignment is available to test this permission' if assignment_id.blank? user_flags(assignment_id) end - progress_callback&.call(4, 'Tested get participant access') + progress_callback&.call(5, 'Tested get participant access') test_function(results, 'mod_assign_get_participant', successful_error_codes: ['userisfilteredout']) do if assignment_id.blank? || participant_user.blank? raise Error, 'An assignment and enrolled student are required to test this permission' @@ -75,12 +84,12 @@ def test_connection(progress_callback: nil) participant(assignment_id, participant_user['id']) end - progress_callback&.call(5, 'Fetching course groups') + progress_callback&.call(6, 'Fetching course groups') groups = test_function(results, 'core_group_get_course_groups') { course_groups } available_groups = Array(groups) { - course: course&.slice('id', 'fullname', 'shortname'), + course: course&.slice('id', 'fullname', 'shortname', 'startdate', 'enddate'), assignments: available_assignments.map { |item| item.slice('id', 'name', 'duedate') }, groups: available_groups.map { |item| item.slice('id', 'name', 'idnumber') }, permissions: results diff --git a/app/sidekiq/test_moodle_connection_job.rb b/app/sidekiq/test_moodle_connection_job.rb index f3e900f91f..ff1a0c1da8 100644 --- a/app/sidekiq/test_moodle_connection_job.rb +++ b/app/sidekiq/test_moodle_connection_job.rb @@ -10,7 +10,7 @@ class TestMoodleConnectionJob retry: false def perform(unit_id) - total(5) + total(6) at(0, 'Starting Moodle connection test') unit = Unit.find(unit_id) diff --git a/test/api/moodle_integration_api_test.rb b/test/api/moodle_integration_api_test.rb index 38dcc9fb03..e1f6415c02 100644 --- a/test/api/moodle_integration_api_test.rb +++ b/test/api/moodle_integration_api_test.rb @@ -85,7 +85,7 @@ def app 'jid' => 'moodle-job-id', 'status' => 'queued', 'at' => 0, - 'total' => 5 + 'total' => 6 } TestMoodleConnectionJob.stub(:perform_async, 'moodle-job-id') do @@ -98,7 +98,7 @@ def app assert_equal 201, last_response.status, last_response.inspect assert_equal 'moodle-job-id', last_response_body['id'] - assert_equal 5, last_response_body['total_count'].to_i + assert_equal 6, last_response_body['total_count'].to_i end test 'Moodle imports enqueue preview and import jobs' do diff --git a/test/services/moodle_integration_test.rb b/test/services/moodle_integration_test.rb index 8494518019..b7659dfbe0 100644 --- a/test/services/moodle_integration_test.rb +++ b/test/services/moodle_integration_test.rb @@ -24,19 +24,32 @@ class MoodleApiTest < ActiveSupport::TestCase 'roles' => [{ 'shortname' => 'student' }] }] groups = [{ 'id' => 31, 'name' => 'Tutorial A', 'idnumber' => 'T-A' }] + course_details = { + 'courses' => [{ + 'id' => 42, + 'fullname' => 'Programming 1', + 'shortname' => 'COS10001', + 'startdate' => 1_775_347_200, + 'enddate' => 1_786_838_400 + }] + } - @integration.stub(:assignments, assignments) do - @integration.stub(:students, students) do - @integration.stub(:user_flags, {}) do - @integration.stub(:participant, {}) do - @integration.stub(:course_groups, groups) do - result = @integration.test_connection + @integration.stub(:course_details, course_details) do + @integration.stub(:assignments, assignments) do + @integration.stub(:students, students) do + @integration.stub(:user_flags, {}) do + @integration.stub(:participant, {}) do + @integration.stub(:course_groups, groups) do + result = @integration.test_connection - assert_equal 'Programming 1', result[:course]['fullname'] - assert_equal 'Portfolio', result[:assignments].first['name'] - assert_equal 'Tutorial A', result[:groups].first['name'] - assert_equal %w[mod_assign_get_assignments core_enrol_get_enrolled_users mod_assign_get_user_flags mod_assign_get_participant core_group_get_course_groups], result[:permissions].pluck(:function) - assert(result[:permissions].all? { |permission| permission[:success] }) + assert_equal 'Programming 1', result[:course]['fullname'] + assert_equal 1_775_347_200, result[:course]['startdate'] + assert_equal 1_786_838_400, result[:course]['enddate'] + assert_equal 'Portfolio', result[:assignments].first['name'] + assert_equal 'Tutorial A', result[:groups].first['name'] + assert_equal %w[mod_assign_get_assignments core_course_get_courses_by_field core_enrol_get_enrolled_users mod_assign_get_user_flags mod_assign_get_participant core_group_get_course_groups], result[:permissions].pluck(:function) + assert(result[:permissions].all? { |permission| permission[:success] }) + end end end end @@ -45,15 +58,17 @@ class MoodleApiTest < ActiveSupport::TestCase end test 'connection test reports an individual failed permission' do - @integration.stub(:assignments, { 'courses' => [] }) do - @integration.stub(:students, []) do - @integration.stub(:course_groups, []) do - result = @integration.test_connection + @integration.stub(:course_details, { 'courses' => [] }) do + @integration.stub(:assignments, { 'courses' => [] }) do + @integration.stub(:students, []) do + @integration.stub(:course_groups, []) do + result = @integration.test_connection - flags = result[:permissions].find { |permission| permission[:function] == 'mod_assign_get_user_flags' } - participant = result[:permissions].find { |permission| permission[:function] == 'mod_assign_get_participant' } - assert_not flags[:success] - assert_not participant[:success] + flags = result[:permissions].find { |permission| permission[:function] == 'mod_assign_get_user_flags' } + participant = result[:permissions].find { |permission| permission[:function] == 'mod_assign_get_participant' } + assert_not flags[:success] + assert_not participant[:success] + end end end end @@ -69,18 +84,20 @@ class MoodleApiTest < ActiveSupport::TestCase students = [{ 'id' => 12, 'roles' => [{ 'shortname' => 'student' }] }] filtered = MoodleApi::Error.new('User is filtered out', code: 'userisfilteredout') - @integration.stub(:assignments, assignments) do - @integration.stub(:students, students) do - @integration.stub(:user_flags, {}) do - @integration.stub(:participant, ->(*) { raise filtered }) do - @integration.stub(:course_groups, []) do - result = @integration.test_connection - permission = result[:permissions].find do |item| - item[:function] == 'mod_assign_get_participant' - end + @integration.stub(:course_details, { 'courses' => [] }) do + @integration.stub(:assignments, assignments) do + @integration.stub(:students, students) do + @integration.stub(:user_flags, {}) do + @integration.stub(:participant, ->(*) { raise filtered }) do + @integration.stub(:course_groups, []) do + result = @integration.test_connection + permission = result[:permissions].find do |item| + item[:function] == 'mod_assign_get_participant' + end - assert permission[:success] - assert_equal 'User is filtered out', permission[:message] + assert permission[:success] + assert_equal 'User is filtered out', permission[:message] + end end end end From 1611584ae9c13e7a3b0901353e39ee274a8cdec6 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 5 Aug 2026 14:12:13 +1000 Subject: [PATCH 6/6] feat: add auto sync --- app/api/entities/moodle_integration_entity.rb | 2 + app/api/moodle_integration_api.rb | 4 + app/sidekiq/sync_moodle_integrations_job.rb | 29 +++++ config/schedule.yml | 4 + ...4005203_add_moodle_integration_to_units.rb | 2 + db/schema.rb | 2 + test/api/moodle_integration_api_test.rb | 6 ++ .../sync_moodle_integrations_job_test.rb | 102 ++++++++++++++++++ 8 files changed, 151 insertions(+) create mode 100644 app/sidekiq/sync_moodle_integrations_job.rb create mode 100644 test/sidekiq/sync_moodle_integrations_job_test.rb diff --git a/app/api/entities/moodle_integration_entity.rb b/app/api/entities/moodle_integration_entity.rb index 6b0ea01f79..0b6fa9a9b1 100644 --- a/app/api/entities/moodle_integration_entity.rb +++ b/app/api/entities/moodle_integration_entity.rb @@ -7,6 +7,8 @@ class MoodleIntegrationEntity < Grape::Entity expose :assignment_id expose :assignment_name expose :fetch_extensions + expose :auto_sync_students + expose :auto_sync_extensions expose :group_mapping_enabled expose :moodle_group_mappings, as: :group_mappings, diff --git a/app/api/moodle_integration_api.rb b/app/api/moodle_integration_api.rb index 6640564758..df744f1639 100644 --- a/app/api/moodle_integration_api.rb +++ b/app/api/moodle_integration_api.rb @@ -32,6 +32,8 @@ class MoodleIntegrationApi < Grape::API optional :assignment_id, type: Integer optional :assignment_name, type: String optional :fetch_extensions, type: Boolean, default: false + optional :auto_sync_students, type: Boolean, default: false + optional :auto_sync_extensions, type: Boolean, default: false optional :group_mapping_enabled, type: Boolean, default: false optional :group_mappings, type: Array do requires :moodle_group_id, type: Integer @@ -59,6 +61,8 @@ class MoodleIntegrationApi < Grape::API integration.fetch_extensions = params[:fetch_extensions] integration.assignment_id = params[:fetch_extensions] ? params[:assignment_id] : nil integration.assignment_name = params[:fetch_extensions] ? params[:assignment_name] : nil + integration.auto_sync_students = params[:auto_sync_students] + integration.auto_sync_extensions = params[:fetch_extensions] && params[:auto_sync_extensions] integration.group_mapping_enabled = params[:group_mapping_enabled] integration.save! diff --git a/app/sidekiq/sync_moodle_integrations_job.rb b/app/sidekiq/sync_moodle_integrations_job.rb new file mode 100644 index 0000000000..80a4b59228 --- /dev/null +++ b/app/sidekiq/sync_moodle_integrations_job.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +class SyncMoodleIntegrationsJob + include Sidekiq::Job + + sidekiq_options lock: :until_executed, + lock_args_method: ->(_args) { ['sync-moodle-integrations'] }, + on_conflict: :reject, + retry: 1 + + def perform + today = Time.zone.today + + MoodleIntegration.includes(:unit).find_each do |integration| + unit = integration.unit + next unless unit.moodle_enabled? && unit.active? + + if integration.auto_sync_students? && today.between?(unit.start_date.to_date, unit.end_date.to_date) + ImportMoodleStudentsJob.perform_async(unit.id, false) + end + + next unless integration.auto_sync_extensions? + next unless integration.fetch_extensions? && integration.assignment_id.present? + next unless today.between?(unit.start_date.to_date, unit.end_date.to_date + 14.days) + + ImportMoodleExtensionsJob.perform_async(unit.id, false) + end + end +end diff --git a/config/schedule.yml b/config/schedule.yml index adb2a1f282..60befad13f 100644 --- a/config/schedule.yml +++ b/config/schedule.yml @@ -28,6 +28,10 @@ notify_discuss_timeout: cron: "every day at 8am" class: "NotifyDiscussTimeoutJob" +sync_moodle_integrations: + cron: "every day at 3am" + class: "SyncMoodleIntegrationsJob" + # archive_old_units: # cron: "every 6 months" # class: "ArchiveOldUnitsJob" diff --git a/db/migrate/20260804005203_add_moodle_integration_to_units.rb b/db/migrate/20260804005203_add_moodle_integration_to_units.rb index 720bba5655..3d9c96cc49 100644 --- a/db/migrate/20260804005203_add_moodle_integration_to_units.rb +++ b/db/migrate/20260804005203_add_moodle_integration_to_units.rb @@ -9,6 +9,8 @@ def change t.bigint :assignment_id t.string :assignment_name t.boolean :fetch_extensions, null: false, default: false + t.boolean :auto_sync_students, null: false, default: false + t.boolean :auto_sync_extensions, null: false, default: false t.boolean :group_mapping_enabled, null: false, default: false t.timestamps diff --git a/db/schema.rb b/db/schema.rb index cfb2709751..3bc15ddd76 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -381,6 +381,8 @@ t.bigint "assignment_id" t.string "assignment_name" t.boolean "fetch_extensions", default: false, null: false + t.boolean "auto_sync_students", default: false, null: false + t.boolean "auto_sync_extensions", default: false, null: false t.boolean "group_mapping_enabled", default: false, null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false diff --git a/test/api/moodle_integration_api_test.rb b/test/api/moodle_integration_api_test.rb index e1f6415c02..e14e8d85ee 100644 --- a/test/api/moodle_integration_api_test.rb +++ b/test/api/moodle_integration_api_test.rb @@ -22,6 +22,8 @@ def app assignment_id: 7, assignment_name: 'Portfolio', fetch_extensions: true, + auto_sync_students: true, + auto_sync_extensions: true, group_mapping_enabled: true, group_mappings: [{ moodle_group_id: 31, @@ -38,6 +40,8 @@ def app assert_equal 'secret-token', integration.api_key assert_equal 'Portfolio', integration.assignment_name assert integration.fetch_extensions + assert integration.auto_sync_students + assert integration.auto_sync_extensions assert integration.group_mapping_enabled assert_equal 'Hawthorn', integration.moodle_group_mappings.first.moodle_group_name assert_equal integration.id, last_response_body['id'] @@ -49,6 +53,8 @@ def app assert_equal 7, last_response_body['assignment_id'] assert_equal 'Portfolio', last_response_body['assignment_name'] assert_equal true, last_response_body['fetch_extensions'] + assert_equal true, last_response_body['auto_sync_students'] + assert_equal true, last_response_body['auto_sync_extensions'] assert_equal true, last_response_body['group_mapping_enabled'] assert_equal 31, last_response_body['group_mappings'].first['moodle_group_id'] assert_nil last_response_body['api_key'] diff --git a/test/sidekiq/sync_moodle_integrations_job_test.rb b/test/sidekiq/sync_moodle_integrations_job_test.rb new file mode 100644 index 0000000000..d1c8f547af --- /dev/null +++ b/test/sidekiq/sync_moodle_integrations_job_test.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +require 'test_helper' +require 'minitest/mock' + +class SyncMoodleIntegrationsJobTest < ActiveSupport::TestCase + test 'queues both imports for a current active unit' do + travel_to Time.zone.local(2026, 8, 4, 3, 0, 0) do + unit = create_unit(start_date: 1.week.ago, end_date: 1.week.from_now) + create_integration(unit: unit, auto_sync_students: true, auto_sync_extensions: true) + + assert_queued_imports students: [[unit.id, false]], extensions: [[unit.id, false]] + end + end + + test 'only queues extensions during the fourteen day grace period' do + travel_to Time.zone.local(2026, 8, 4, 3, 0, 0) do + unit = create_unit(start_date: 15.weeks.ago, end_date: 1.week.ago) + create_integration(unit: unit, auto_sync_students: true, auto_sync_extensions: true) + + assert_queued_imports students: [], extensions: [[unit.id, false]] + end + end + + test 'does not queue imports outside their date windows' do + travel_to Time.zone.local(2026, 8, 4, 3, 0, 0) do + future_unit = create_unit(start_date: 1.week.from_now, end_date: 15.weeks.from_now) + ended_unit = create_unit(start_date: 20.weeks.ago, end_date: 3.weeks.ago) + create_integration(unit: future_unit, auto_sync_students: true, auto_sync_extensions: true) + create_integration(unit: ended_unit, auto_sync_students: true, auto_sync_extensions: true) + + assert_queued_imports students: [], extensions: [] + end + end + + test 'does not queue imports for a disabled or inactive unit' do + travel_to Time.zone.local(2026, 8, 4, 3, 0, 0) do + disabled_unit = create_unit(start_date: 1.week.ago, end_date: 1.week.from_now, moodle_enabled: false) + inactive_unit = create_unit(start_date: 1.week.ago, end_date: 1.week.from_now, active: false) + create_integration(unit: disabled_unit, auto_sync_students: true, auto_sync_extensions: true) + create_integration(unit: inactive_unit, auto_sync_students: true, auto_sync_extensions: true) + + assert_queued_imports students: [], extensions: [] + end + end + + test 'requires extension imports and an assignment before scheduling extensions' do + travel_to Time.zone.local(2026, 8, 4, 3, 0, 0) do + unit = create_unit(start_date: 1.week.ago, end_date: 1.week.from_now) + create_integration( + unit: unit, + auto_sync_extensions: true, + fetch_extensions: false, + assignment_id: nil + ) + + assert_queued_imports students: [], extensions: [] + end + end + + private + + def create_unit(start_date:, end_date:, active: true, moodle_enabled: true) + FactoryBot.create( + :unit, + with_students: false, + start_date: start_date, + end_date: end_date, + active: active, + moodle_enabled: moodle_enabled + ) + end + + def create_integration(unit:, auto_sync_students: false, auto_sync_extensions: false, + fetch_extensions: true, assignment_id: 7) + unit.create_moodle_integration!( + course_id: unit.id, + api_key: 'secret-token', + assignment_id: assignment_id, + fetch_extensions: fetch_extensions, + auto_sync_students: auto_sync_students, + auto_sync_extensions: auto_sync_extensions + ) + end + + def assert_queued_imports(students:, extensions:) + queued_students = [] + queued_extensions = [] + + student_enqueue = ->(unit_id, preview_only) { queued_students << [unit_id, preview_only] } + extension_enqueue = ->(unit_id, preview_only) { queued_extensions << [unit_id, preview_only] } + + ImportMoodleStudentsJob.stub(:perform_async, student_enqueue) do + ImportMoodleExtensionsJob.stub(:perform_async, extension_enqueue) do + SyncMoodleIntegrationsJob.new.perform + end + end + + assert_equal students, queued_students + assert_equal extensions, queued_extensions + end +end