diff --git a/app/api/activity_types_public_api.rb b/app/api/activity_types_public_api.rb index 5d08e11fbf..7a4004f20e 100644 --- a/app/api/activity_types_public_api.rb +++ b/app/api/activity_types_public_api.rb @@ -8,6 +8,14 @@ class ActivityTypesPublicApi < Grape::API desc 'Get all the activity types' get '/activity_types' do - present ActivityType.all, with: Entities::ActivityTypeEntity + if params.key?(:per_page) || params.key?(:page) + per_page = params[:per_page].to_i > 0 ? [params[:per_page].to_i, 500].min : 50 + page = params[:page].to_i > 0 ? params[:page].to_i : 1 + result = ActivityType.limit(per_page).offset((page - 1) * per_page) + else + result = ActivityType.all + end + + present result, with: Entities::ActivityTypeEntity end end diff --git a/app/api/api_root.rb b/app/api/api_root.rb index 3dbc682297..beba042e21 100644 --- a/app/api/api_root.rb +++ b/app/api/api_root.rb @@ -66,6 +66,7 @@ class ApiRoot < Grape::API mount GroupSetsApi mount LearningOutcomesApi mount ProjectsApi + mount PeerProgressApi mount SettingsApi mount StudentsApi mount Submission::PortfolioApi @@ -125,6 +126,7 @@ class ApiRoot < Grape::API AuthenticationHelpers.add_auth_to GroupSetsApi AuthenticationHelpers.add_auth_to LearningOutcomesApi AuthenticationHelpers.add_auth_to ProjectsApi + AuthenticationHelpers.add_auth_to PeerProgressApi AuthenticationHelpers.add_auth_to StudentsApi AuthenticationHelpers.add_auth_to Submission::PortfolioApi AuthenticationHelpers.add_auth_to Submission::PortfolioEvidenceApi diff --git a/app/api/campuses_public_api.rb b/app/api/campuses_public_api.rb index 9ec897edc0..34b59ded65 100644 --- a/app/api/campuses_public_api.rb +++ b/app/api/campuses_public_api.rb @@ -9,6 +9,14 @@ class CampusesPublicApi < Grape::API desc 'Get all the Campuses' get '/campuses' do - present Campus.all, with: Entities::CampusEntity + if params.key?(:per_page) || params.key?(:page) + per_page = params[:per_page].to_i > 0 ? [params[:per_page].to_i, 500].min : 50 + page = params[:page].to_i > 0 ? params[:page].to_i : 1 + result = Campus.limit(per_page).offset((page - 1) * per_page) + else + result = Campus.all + end + + present result, with: Entities::CampusEntity end end diff --git a/app/api/entities/unit_entity.rb b/app/api/entities/unit_entity.rb index 46f26976be..d23ee49f60 100644 --- a/app/api/entities/unit_entity.rb +++ b/app/api/entities/unit_entity.rb @@ -55,6 +55,11 @@ def can_read_unit_config?(my_role) expose :allow_student_change_tutorial, unless: :summary_only expose :allow_flexible_dates, unless: :summary_only expose :mark_late_submissions_as_assess_in_portfolio, unless: :summary_only + expose :peer_progress_enabled, + unless: :summary_only, + if: lambda { |_unit, options| + can_read_unit_config?(options[:my_role]) + } expose :learning_outcomes, using: LearningOutcomeEntity, as: :ilos, unless: :summary_only expose :tutorial_streams, using: TutorialStreamEntity, unless: :summary_only diff --git a/app/api/group_sets_api.rb b/app/api/group_sets_api.rb index eb78a85d4e..a687bb95c8 100644 --- a/app/api/group_sets_api.rb +++ b/app/api/group_sets_api.rb @@ -372,7 +372,15 @@ class GroupSetsApi < Grape::API error!({ error: 'Not authorised to get groups for this unit' }, 403) end - present grp.projects, with: Entities::ProjectEntity, only: [:student, :id, :target_grade], user: current_user + if params.key?(:per_page) || params.key?(:page) + per_page = params[:per_page].to_i > 0 ? [params[:per_page].to_i, 500].min : 50 + page = params[:page].to_i > 0 ? params[:page].to_i : 1 + result = grp.projects.limit(per_page).offset((page - 1) * per_page) + else + result = grp.projects + end + + present result, with: Entities::ProjectEntity, only: [:student, :id, :target_grade], user: current_user end desc 'Add a group member' diff --git a/app/api/peer_progress_api.rb b/app/api/peer_progress_api.rb new file mode 100644 index 0000000000..99567eaf2a --- /dev/null +++ b/app/api/peer_progress_api.rb @@ -0,0 +1,249 @@ +# frozen_string_literal: true + +require 'grape' + +class PeerProgressApi < Grape::API + helpers AuthenticationHelpers + + UNAVAILABLE_MESSAGE = 'Peer progress is currently unavailable.' + NOT_FOUND_MESSAGE = 'Peer progress is unavailable for this project or task.' + CONFIG_ERROR_MESSAGE = 'Peer progress is not configured.' + # These two constants are a pair and must not be changed independently. + # + # The zero and hundred edge buckets only hide the underlying submitted count + # while half a bucket is wider than one student's share of the cohort. At a + # cohort of 20, one student is exactly five percentage points and zero becomes + # a singleton bucket, revealing that nobody has submitted. A floor of 21 makes + # one student's share smaller than the five-point bucket boundary, so every + # returned bucket represents at least two possible submitted counts. + # + # 21 and 10.0 leave no cohort size at or above the floor from which the count + # can be recovered. peer_progress_api_test.rb asserts the relationship holds. + MINIMUM_SAFE_COHORT_SIZE = 21 + PERCENTAGE_BUCKET_SIZE = 10.0 + + before do + header 'Cache-Control', 'private, no-store' + authenticated? + end + + helpers do + def peer_progress_not_found! + error!({ error: PeerProgressApi::NOT_FOUND_MESSAGE }, 404) + end + + def effective_task(project:, task_definition:) + project.tasks.find_by( + task_definition_id: task_definition.id + ) || Task.new( + project: project, + task_definition: task_definition, + task_status: TaskStatus.not_started, + extensions: 0 + ) + end + + def released_for_project?(project:, task_definition:) + start_date = effective_task( + project: project, + task_definition: task_definition + ).local_start_date + + start_date.present? && start_date <= Time.zone.now + end + + def snapshot_predates_target_grade?(project, snapshot) + changed_at = project.target_grade_changed_at + + changed_at.present? && snapshot.calculated_at < changed_at + end + + def quantised_percentage(value) + bucket_size = PeerProgressApi::PERCENTAGE_BUCKET_SIZE + + ((value.to_f / bucket_size).round * bucket_size).to_f + end + + def safe_target_grade(project) + target_grade = project.target_grade + + return nil if target_grade.nil? + return nil unless project.unit.grade_value?(target_grade) + + target_grade + end + + def positive_integer_env!(name) + value = Integer(ENV.fetch(name), 10) + raise ArgumentError unless value.positive? + + value + rescue KeyError, ArgumentError + error!({ error: PeerProgressApi::CONFIG_ERROR_MESSAGE }, 503) + end + + def minimum_cohort_size! + value = positive_integer_env!( + 'DF_PPI_MINIMUM_COHORT_SIZE' + ) + + return value if value >= PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + + error!( + { error: PeerProgressApi::CONFIG_ERROR_MESSAGE }, + 503 + ) + end + + def peer_progress_payload( + project:, + task_definition:, + snapshot: nil, + submitted_percentage: nil, + is_suppressed: false, + is_stale: false, + is_feature_enabled: true, + unavailable_message: '' + ) + { + task_definition_id: task_definition.id, + unit_id: project.unit_id, + target_grade: safe_target_grade(project), + submitted_percentage: submitted_percentage, + is_suppressed: is_suppressed, + is_stale: is_stale, + is_feature_enabled: is_feature_enabled, + last_updated_at: snapshot&.calculated_at&.utc&.iso8601, + unavailable_message: unavailable_message + } + end + + def peer_progress_result(project:, task_definition:) + unit = project.unit + + unless unit.peer_progress_enabled? + return peer_progress_payload( + project: project, + task_definition: task_definition, + is_feature_enabled: false, + unavailable_message: PeerProgressApi::UNAVAILABLE_MESSAGE + ) + end + + target_grade = safe_target_grade(project) + unless target_grade + return peer_progress_payload( + project: project, + task_definition: task_definition, + unavailable_message: PeerProgressApi::UNAVAILABLE_MESSAGE + ) + end + + snapshot = unit.peer_progress_snapshots.find_by( + task_definition_id: task_definition.id, + target_grade: target_grade + ) + + if snapshot.nil? || + snapshot_predates_target_grade?(project, snapshot) + return peer_progress_payload( + project: project, + task_definition: task_definition, + unavailable_message: PeerProgressApi::UNAVAILABLE_MESSAGE + ) + end + + minimum_cohort_size = minimum_cohort_size! + stale_after_hours = positive_integer_env!( + 'DF_PPI_STALE_AFTER_HOURS' + ) + + is_stale = snapshot.calculated_at < stale_after_hours.hours.ago + + # Treat an empty cohort exactly like every other cohort below the + # privacy threshold. This prevents the response from revealing + # whether a target-grade group is empty or merely small. + if snapshot.cohort_size < minimum_cohort_size + return peer_progress_payload( + project: project, + task_definition: task_definition, + snapshot: snapshot, + is_suppressed: true, + is_stale: is_stale, + unavailable_message: PeerProgressApi::UNAVAILABLE_MESSAGE + ) + end + + if snapshot.submitted_percentage.nil? + return peer_progress_payload( + project: project, + task_definition: task_definition, + snapshot: snapshot, + is_stale: is_stale, + unavailable_message: PeerProgressApi::UNAVAILABLE_MESSAGE + ) + end + + if is_stale + return peer_progress_payload( + project: project, + task_definition: task_definition, + snapshot: snapshot, + is_stale: true, + unavailable_message: PeerProgressApi::UNAVAILABLE_MESSAGE + ) + end + + peer_progress_payload( + project: project, + task_definition: task_definition, + snapshot: snapshot, + submitted_percentage: quantised_percentage( + snapshot.submitted_percentage + ) + ) + end + end + + desc 'Get anonymous task-level peer progress for the authenticated student', + tags: ['peer_progress'], + summary: 'Get anonymous task-level peer progress' + params do + requires :id, + type: Integer, + desc: 'The authenticated student project ID' + requires :task_definition_id, + type: Integer, + desc: 'The task definition ID' + end + get '/projects/:id/task_def_id/:task_definition_id/peer_progress' do + peer_progress_not_found! if current_user.role.id != Role.student_id + + project = Project.for_user(current_user, false) + .includes(:unit) + .find_by(id: params[:id]) + peer_progress_not_found! if project.nil? + + unit = project.unit + task_definition = unit.task_definitions.find_by( + id: params[:task_definition_id] + ) + peer_progress_not_found! if task_definition.nil? + + peer_progress_not_found! unless released_for_project?( + project: project, + task_definition: task_definition + ) + + target_grade = project.target_grade + if target_grade.present? && unit.grade_value?(target_grade) && + task_definition.target_grade > target_grade + peer_progress_not_found! + end + + present peer_progress_result( + project: project, + task_definition: task_definition + ), with: Grape::Presenters::Presenter + end +end diff --git a/app/api/projects_api.rb b/app/api/projects_api.rb index a895007ff3..4d2c0d3f33 100644 --- a/app/api/projects_api.rb +++ b/app/api/projects_api.rb @@ -17,6 +17,11 @@ class ProjectsApi < Grape::API include_inactive = params[:include_inactive] || false projects = Project.eager_load(:unit, :user).for_user current_user, include_inactive + + per_page = params[:per_page].to_i > 0 ? [params[:per_page].to_i, 500].min : 50 + page = params[:page].to_i > 0 ? params[:page].to_i : 1 + projects = projects.limit(per_page).offset((page - 1) * per_page) + present projects, with: Entities::ProjectEntity, for_student: true, summary_only: true, user: current_user end diff --git a/app/api/units_api.rb b/app/api/units_api.rb index 411065f97e..d2ac2df87b 100644 --- a/app/api/units_api.rb +++ b/app/api/units_api.rb @@ -37,6 +37,10 @@ class UnitsApi < Grape::API units = units.where('active = true') unless params[:include_in_active] + per_page = params[:per_page].to_i > 0 ? [params[:per_page].to_i, 500].min : 50 + page = params[:page].to_i > 0 ? params[:page].to_i : 1 + units = units.limit(per_page).offset((page - 1) * per_page) + present units, with: Entities::UnitEntity, user: current_user, summary_only: true, in_unit: true end @@ -73,6 +77,7 @@ class UnitsApi < Grape::API optional :code, type: String optional :description, type: String optional :active, type: Boolean + optional :peer_progress_enabled, type: Boolean, desc: 'Enable anonymous peer progress for students in this unit' optional :teaching_period_id, type: Integer optional :start_date, type: Date optional :end_date, type: Date @@ -116,6 +121,7 @@ class UnitsApi < Grape::API :description, :start_date, :end_date, + :peer_progress_enabled, :teaching_period_id, :active, :main_convenor_id, diff --git a/app/api/users_api.rb b/app/api/users_api.rb index 2900bbfba4..291220e2b3 100644 --- a/app/api/users_api.rb +++ b/app/api/users_api.rb @@ -15,7 +15,10 @@ class UsersApi < Grape::API error!({ error: 'Cannot list users - not authorised' }, 403) end - present User.all.eager_load(:role), with: Entities::UserEntity + per_page = params[:per_page].to_i > 0 ? [params[:per_page].to_i, 500].min : 50 + page = params[:page].to_i > 0 ? params[:page].to_i : 1 + users = User.eager_load(:role).limit(per_page).offset((page - 1) * per_page) + present users, with: Entities::UserEntity end desc 'Get user' @@ -34,7 +37,10 @@ class UsersApi < Grape::API error!({ error: 'Cannot list convenors - not authorised' }, 403) end - present User.convenors, with: Entities::UserEntity + per_page = params[:per_page].to_i > 0 ? [params[:per_page].to_i, 500].min : 50 + page = params[:page].to_i > 0 ? params[:page].to_i : 1 + users = User.convenors.eager_load(:role).limit(per_page).offset((page - 1) * per_page) + present users, with: Entities::UserEntity end desc 'Get tutors' @@ -43,7 +49,10 @@ class UsersApi < Grape::API error!({ error: 'Cannot list tutors - not authorised' }, 403) end - present User.tutors.eager_load(:role), with: Entities::UserEntity + per_page = params[:per_page].to_i > 0 ? [params[:per_page].to_i, 500].min : 50 + page = params[:page].to_i > 0 ? params[:page].to_i : 1 + users = User.tutors.eager_load(:role).limit(per_page).offset((page - 1) * per_page) + present users, with: Entities::UserEntity end desc 'Update a user' diff --git a/app/models/peer_progress_snapshot.rb b/app/models/peer_progress_snapshot.rb new file mode 100644 index 0000000000..425a5de18b --- /dev/null +++ b/app/models/peer_progress_snapshot.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +class PeerProgressSnapshot < ApplicationRecord + belongs_to :unit, + inverse_of: :peer_progress_snapshots + + belongs_to :task_definition, + inverse_of: :peer_progress_snapshots + + validates :target_grade, + presence: true, + numericality: { + only_integer: true, + greater_than_or_equal_to: 0 + }, + uniqueness: { + scope: %i[unit_id task_definition_id] + } + + validates :submitted_percentage, + numericality: { + greater_than_or_equal_to: 0, + less_than_or_equal_to: 100 + }, + allow_nil: true + + validates :cohort_size, + presence: true, + numericality: { + only_integer: true, + greater_than_or_equal_to: 0 + } + + validates :calculated_at, + presence: true + + validate :task_definition_belongs_to_unit + validate :target_grade_enabled_for_unit + validate :target_grade_covers_task + validate :percentage_requires_non_empty_cohort + + private + + def task_definition_belongs_to_unit + return if unit.blank? || task_definition.blank? + return if task_definition.unit_id == unit_id + + errors.add( + :task_definition, + 'must belong to the same unit' + ) + end + + def target_grade_enabled_for_unit + return if unit.blank? || target_grade.nil? + return if unit.grade_value?(target_grade) + + errors.add( + :target_grade, + 'must be enabled for the unit' + ) + end + + def target_grade_covers_task + return if task_definition.blank? || target_grade.nil? + return if target_grade >= task_definition.target_grade + + errors.add( + :target_grade, + 'must be at least the task definition target grade' + ) + end + + def percentage_requires_non_empty_cohort + return if submitted_percentage.nil? + return if cohort_size.nil? + return if cohort_size.positive? + + errors.add( + :submitted_percentage, + 'must be blank when cohort size is zero' + ) + end +end diff --git a/app/models/project.rb b/app/models/project.rb index 64dc33ed4e..24de545911 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -35,6 +35,10 @@ class Project < ApplicationRecord has_many :staff_notes, dependent: :destroy has_many :engagements, dependent: :destroy, inverse_of: :project + before_create :record_target_grade_change + before_update :record_target_grade_change, + if: :will_save_change_to_target_grade? + # Callbacks - methods called are private before_destroy :can_destroy? @@ -718,6 +722,10 @@ def escalation_attempts_remaining private + def record_target_grade_change + self.target_grade_changed_at = Time.current + end + def can_destroy? return true if tutorial_enrolments.count == 0 diff --git a/app/models/task_definition.rb b/app/models/task_definition.rb index 7ef377811f..cbf7be1ce3 100644 --- a/app/models/task_definition.rb +++ b/app/models/task_definition.rb @@ -70,7 +70,8 @@ def self.permissions belongs_to :tutorial_stream, optional: true belongs_to :overseer_image, optional: true - has_many :tasks, dependent: :destroy # Destroying a task definition will also nuke any instances + has_many :tasks, dependent: :destroy # Destroying a task definition will also nuke any instances + has_many :peer_progress_snapshots, dependent: :destroy, inverse_of: :task_definition has_many :group_submissions, dependent: :destroy # Destroying a task definition will also nuke any group submissions has_many :learning_outcomes, as: :context, dependent: :destroy has_many :overseer_steps, -> { order(:sort_order) }, inverse_of: :task_definition, dependent: :destroy diff --git a/app/models/unit.rb b/app/models/unit.rb index 19e0098298..f4dead29eb 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -174,6 +174,7 @@ def role_for(user) has_many :learning_outcomes, as: :context, dependent: :destroy # inverse_of: :unit has_many :marking_sessions, dependent: :destroy has_many :task_completion_snapshots, dependent: :destroy, inverse_of: :unit + has_many :peer_progress_snapshots, dependent: :destroy, inverse_of: :unit has_many :communication_sets, class_name: 'CommunicationSet', dependent: :destroy has_many :communication_rules, through: :communication_sets, class_name: 'CommunicationRule' has_many :communication_set_schedules, through: :communication_sets, class_name: 'CommunicationSetSchedule' diff --git a/app/services/peer_progress_aggregation_service.rb b/app/services/peer_progress_aggregation_service.rb new file mode 100644 index 0000000000..d4c053add2 --- /dev/null +++ b/app/services/peer_progress_aggregation_service.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +# Calculates and stores task-level peer-progress snapshots for one unit. +# +# This service stores aggregate values only. It does not authorise students, +# apply the small-cohort display threshold, or expose API response data. +class PeerProgressAggregationService + def self.call(unit:, calculated_at: Time.zone.now) + new(unit: unit, calculated_at: calculated_at).call + end + + def initialize(unit:, calculated_at:) + unless unit.is_a?(Unit) && unit.persisted? + raise ArgumentError, 'unit must be a persisted Unit' + end + raise ArgumentError, 'calculated_at is required' if calculated_at.blank? + + @unit = unit + @calculated_at = calculated_at + end + + def call + snapshots = [] + + PeerProgressSnapshot.transaction do + existing_snapshots = PeerProgressSnapshot.where(unit: unit).index_by do |snapshot| + [snapshot.task_definition_id, snapshot.target_grade] + end + + unit.grade_values.map(&:to_i).uniq.sort.each do |target_grade| + cohort = unit.active_projects.where(target_grade: target_grade) + cohort_size = cohort.count + + task_definitions = unit.task_definitions + .where('target_grade <= ?', target_grade) + .order(:id) + + submitted_counts = submitted_counts_for( + cohort: cohort, + task_definitions: task_definitions + ) + + task_definitions.each do |task_definition| + key = [task_definition.id, target_grade] + + snapshot = existing_snapshots[key] || PeerProgressSnapshot.new( + unit: unit, + task_definition: task_definition, + target_grade: target_grade + ) + + snapshot.assign_attributes( + cohort_size: cohort_size, + submitted_percentage: percentage( + submitted_count: submitted_counts.fetch(task_definition.id, 0), + cohort_size: cohort_size + ), + calculated_at: calculated_at + ) + + snapshot.save! + snapshots << snapshot + end + end + end + + snapshots + end + + private + + attr_reader :unit, :calculated_at + + def submitted_counts_for(cohort:, task_definitions:) + Task + .where( + project_id: cohort.select(:id), + task_definition_id: task_definitions.select(:id) + ) + .where.not(file_uploaded_at: nil) + .group(:task_definition_id) + .distinct + .count(:project_id) + end + + def percentage(submitted_count:, cohort_size:) + return nil if cohort_size.zero? + + ((submitted_count * 100.0) / cohort_size).round(2) + end +end diff --git a/app/sidekiq/aggregate_peer_progress_job.rb b/app/sidekiq/aggregate_peer_progress_job.rb new file mode 100644 index 0000000000..c320e7e94a --- /dev/null +++ b/app/sidekiq/aggregate_peer_progress_job.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +class AggregatePeerProgressJob + class AggregationError < StandardError; end + + include Sidekiq::Job + include Sidekiq::Status::Worker + include LogHelper + include ApplicationHelper + + sidekiq_options lock: :until_executed, + lock_args_method: lambda { |args| + [args.first || 'all-active-units'] + }, + on_conflict: :reject, + retry: 3 + + def perform(unit_id = nil) + return enqueue_active_units if unit_id.blank? + + aggregate_unit(Unit.find(unit_id)) + rescue StandardError => e + log_unit_id = unit_id.presence || 'all-active-units' + failure_message = + "Peer progress aggregation failed for unit_id=#{log_unit_id}: " \ + "#{e.class.name}" + + logger.error(failure_message) + raise AggregationError, failure_message, cause: nil + end + + private + + def enqueue_active_units + logger.info( + 'Queueing peer progress aggregation for active units...' + ) + + # Only units whose convenor has opted in. Aggregating the rest would store + # derived cohort statistics for units that never enabled the feature, and + # the endpoint returns early on peer_progress_enabled? so those rows could + # never be served anyway. + Unit.active_units.where(peer_progress_enabled: true).find_each do |unit| + self.class.perform_async(unit.id) + end + + logger.info( + 'Queued peer progress aggregation jobs.' + ) + end + + def aggregate_unit(unit) + unless unit.active? + logger.info( + "Skipping peer progress aggregation for inactive unit_id=#{unit.id}" + ) + return + end + + unless unit.peer_progress_enabled? + logger.info( + "Skipping peer progress aggregation for unit_id=#{unit.id}, " \ + 'peer progress is not enabled' + ) + return + end + + logger.info( + "Starting peer progress aggregation for unit_id=#{unit.id}..." + ) + + at(0) + total(1) + + PeerProgressAggregationService.call( + unit: unit, + calculated_at: Time.zone.now + ) + + at(1) + + logger.info( + "Completed peer progress aggregation for unit_id=#{unit.id}." + ) + end +end diff --git a/app/sidekiq/execute_communication_set_job.rb b/app/sidekiq/execute_communication_set_job.rb index fa279e1124..15667ed10c 100644 --- a/app/sidekiq/execute_communication_set_job.rb +++ b/app/sidekiq/execute_communication_set_job.rb @@ -294,7 +294,7 @@ def send_action_log_to_convenors(projects, unit, rule, prior_action_results) }] end - recipients = unit.convenors.includes(:user).map(&:user).select { |user| user&.email.present? }.uniq(&:id) + recipients = User.joins(:unit_roles).where(unit_roles: { unit_id: unit.id }).distinct.where.not(email: [nil, '']).to_a if recipients.empty? return [{ @@ -349,7 +349,7 @@ def staff_recipients_for(project, unit, action) end if action.email_convenors - recipients.concat(unit.convenors.includes(:user).map(&:user)) + recipients.concat(User.joins(:unit_roles).where(unit_roles: { unit_id: unit.id }).distinct.to_a) end recipients.select { |recipient| recipient&.email.present? }.uniq(&:id) diff --git a/config/schedule.yml b/config/schedule.yml index 62fd893daf..f6aee20e66 100644 --- a/config/schedule.yml +++ b/config/schedule.yml @@ -16,6 +16,10 @@ refresh_moderation_feedback_timestamps: cron: "every 60 minutes" class: "RefreshModerationFeedbackTimestampsJob" +aggregate_peer_progress: + cron: "every day at 11:45pm" + class: "AggregatePeerProgressJob" + aggregate_task_completion_stats: cron: "every day at 11:55pm" class: "AggregateTaskCompletionStatsJob" diff --git a/db/migrate/20260809153000_create_peer_progress_snapshots.rb b/db/migrate/20260809153000_create_peer_progress_snapshots.rb new file mode 100644 index 0000000000..a96373436f --- /dev/null +++ b/db/migrate/20260809153000_create_peer_progress_snapshots.rb @@ -0,0 +1,32 @@ +class CreatePeerProgressSnapshots < ActiveRecord::Migration[8.0] + def change + create_table :peer_progress_snapshots, + options: 'ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 ' \ + 'COLLATE=utf8mb4_general_ci' do |t| + t.references :unit, null: false + t.references :task_definition, null: false + + t.integer :target_grade, null: false + + # nil represents suppressed or unavailable data. + # A genuine zero result is stored as 0.00. + t.decimal :submitted_percentage, + precision: 5, + scale: 2 + + # Internal only. Never expose this raw value through the student API. + t.integer :cohort_size, null: false + + # The time the aggregate was calculated, rather than when this row + # happened to be inserted or updated. + t.datetime :calculated_at, null: false + + t.timestamps + end + + add_index :peer_progress_snapshots, + [:unit_id, :task_definition_id, :target_grade], + unique: true, + name: 'idx_peer_progress_unit_task_grade' + end +end diff --git a/db/migrate/20260810033824_add_peer_progress_enabled_to_units.rb b/db/migrate/20260810033824_add_peer_progress_enabled_to_units.rb new file mode 100644 index 0000000000..2d17007d0b --- /dev/null +++ b/db/migrate/20260810033824_add_peer_progress_enabled_to_units.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class AddPeerProgressEnabledToUnits < ActiveRecord::Migration[8.0] + def change + add_column :units, + :peer_progress_enabled, + :boolean, + default: false, + null: false + end +end diff --git a/db/migrate/20260818160804_add_target_grade_changed_at_to_projects.rb b/db/migrate/20260818160804_add_target_grade_changed_at_to_projects.rb new file mode 100644 index 0000000000..4e167bce59 --- /dev/null +++ b/db/migrate/20260818160804_add_target_grade_changed_at_to_projects.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class AddTargetGradeChangedAtToProjects < ActiveRecord::Migration[8.0] + def up + add_column :projects, :target_grade_changed_at, :datetime + + # Existing projects have no trustworthy record of when their current + # target grade was selected. Backfill to now so existing snapshots fail + # closed until the next successful aggregation run. + execute <<~SQL + UPDATE projects + SET target_grade_changed_at = UTC_TIMESTAMP() + WHERE target_grade_changed_at IS NULL + SQL + + change_column_null :projects, :target_grade_changed_at, false + end + + def down + remove_column :projects, :target_grade_changed_at + end +end diff --git a/db/schema.rb b/db/schema.rb index b8ec5659b3..69c01f6628 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_09_014859) do +ActiveRecord::Schema[8.0].define(version: 2026_08_18_160804) 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 @@ -443,6 +443,20 @@ t.index ["task_definition_id"], name: "index_overseer_steps_on_task_definition_id" end + create_table "peer_progress_snapshots", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| + t.bigint "unit_id", null: false + t.bigint "task_definition_id", null: false + t.integer "target_grade", null: false + t.decimal "submitted_percentage", precision: 5, scale: 2 + t.integer "cohort_size", null: false + t.datetime "calculated_at", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["task_definition_id"], name: "index_peer_progress_snapshots_on_task_definition_id" + t.index ["unit_id", "task_definition_id", "target_grade"], name: "idx_peer_progress_unit_task_grade", unique: true + t.index ["unit_id"], name: "index_peer_progress_snapshots_on_unit_id" + end + create_table "projects", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| t.bigint "unit_id" t.string "project_role" @@ -467,6 +481,7 @@ t.integer "spec_con_days", default: 0, null: false t.bigint "assessor_id" t.datetime "portfolio_submission_date" + t.datetime "target_grade_changed_at", null: false t.index ["assessor_id"], name: "index_projects_on_assessor_id" t.index ["campus_id"], name: "index_projects_on_campus_id" t.index ["enrolled"], name: "index_projects_on_enrolled" @@ -901,6 +916,7 @@ t.integer "feedback_overflow_threshold_days", default: 7 t.boolean "enforce_feedback_before_discussed_in_class", default: false, null: false t.text "grade_values", size: :long, collation: "utf8mb4_bin" + t.boolean "peer_progress_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/docs/peer-progress-api.md b/docs/peer-progress-api.md new file mode 100644 index 0000000000..b1040115f9 --- /dev/null +++ b/docs/peer-progress-api.md @@ -0,0 +1,226 @@ +# Student Peer Progress API + +## Route + +`GET /api/projects/:id/task_def_id/:task_definition_id/peer_progress` + +The route is restricted to the authenticated student who owns the enrolled project. +The unit and target grade are derived from that project. The route does not accept a +student ID, unit ID, trimester, cohort, or target grade from the browser. + +## Successful response contract + +All authorised business states return HTTP 200 with exactly the following +fields. The API uses snake_case. PPI-F01 maps these fields to the frontend +camelCase interface. + +| Field | Type | Nullable | Meaning | +| --- | --- | --- | --- | +| `task_definition_id` | Integer | No | Requested task definition | +| `unit_id` | Integer | No | Unit derived from the authenticated student's project | +| `target_grade` | Integer | Yes | Valid server-side target grade, or `null` when none is valid | +| `submitted_percentage` | Number | Yes | Value from 0.0 to 100.0, or `null` when the value must not be displayed | +| `is_suppressed` | Boolean | No | True when the cohort is below the privacy threshold | +| `is_stale` | Boolean | No | True when the stored snapshot is older than the approved freshness window | +| `is_feature_enabled` | Boolean | No | Whether the unit has enabled PPI | +| `last_updated_at` | String | Yes | UTC ISO 8601 snapshot time, or `null` when no snapshot was used | +| `unavailable_message` | String | No | Empty on success; otherwise a neutral and privacy-safe message | + +Student-facing percentages are quantised to the nearest ten percentage points. +The precise stored aggregate is never returned by this API. + +The bucket size and the minimum cohort size are a matched pair. At the `0.0` +and `100.0` edges, quantising only hides the submitted count while half a bucket +is strictly wider than one student's share of the cohort, which is +`100.0 / cohort_size`. With a floor of 21 and a bucket of 10, every returned +bucket represents at least two possible counts. Changing either number can +break that guarantee, so the relationship is asserted across cohort sizes in +`test/api/peer_progress_api_test.rb`. + +Quantisation applies to `0.0` and `100.0` as well. A cohort where nobody has +submitted and a cohort where fewer than one bucket's worth have submitted both +return `0.0`, so `0.0` means "at most a rounding bucket", not "exactly none". +The same holds at the top of the range. + +`submitted_percentage` must be `null` for suppressed, stale, disabled and +unavailable states. The response never includes raw cohort size or submitted +count. + +## State behaviour + +| State | Percentage | Suppressed | Stale | Enabled | Last updated | +| --- | --- | --- | --- | --- | --- | +| Normal | Number | False | False | True | Timestamp | +| Rounds to zero | `0.0` | False | False | True | Timestamp | +| Empty or small cohort, fresh | `null` | True | False | True | Timestamp | +| Empty or small cohort, stale | `null` | True | True | True | Timestamp | +| Stale snapshot | `null` | False | True | True | Timestamp | +| No snapshot | `null` | False | False | True | `null` | +| No valid target grade | `null` | False | False | True | `null` | +| Feature disabled | `null` | False | False | False | `null` | + +### Normal +``` json +{ + "task_definition_id": 12, + "unit_id": 5, + "target_grade": 2, + "submitted_percentage": 60.0, + "is_suppressed": false, + "is_stale": false, + "is_feature_enabled": true, + "last_updated_at": "2026-08-10T03:15:00Z", + "unavailable_message": "" +} +``` + +### Rounds to zero +``` json +{ + "task_definition_id": 12, + "unit_id": 5, + "target_grade": 2, + "submitted_percentage": 0.0, + "is_suppressed": false, + "is_stale": false, + "is_feature_enabled": true, + "last_updated_at": "2026-08-10T03:15:00Z", + "unavailable_message": "" +} +``` + +### Small-cohort suppression +``` json +{ + "task_definition_id": 12, + "unit_id": 5, + "target_grade": 2, + "submitted_percentage": null, + "is_suppressed": true, + "is_stale": false, + "is_feature_enabled": true, + "last_updated_at": "2026-08-10T03:15:00Z", + "unavailable_message": "Peer progress is currently unavailable." +} +``` + +### Stale data +``` json +{ + "task_definition_id": 12, + "unit_id": 5, + "target_grade": 2, + "submitted_percentage": null, + "is_suppressed": false, + "is_stale": true, + "is_feature_enabled": true, + "last_updated_at": "2026-08-07T03:15:00Z", + "unavailable_message": "Peer progress is currently unavailable." +} +``` + +### No valid target grade +``` json +{ + "task_definition_id": 12, + "unit_id": 5, + "target_grade": null, + "submitted_percentage": null, + "is_suppressed": false, + "is_stale": false, + "is_feature_enabled": true, + "last_updated_at": null, + "unavailable_message": "Peer progress is currently unavailable." +} +``` + + +### No snapshot for a valid target grade +``` json +{ + "task_definition_id": 12, + "unit_id": 5, + "target_grade": 2, + "submitted_percentage": null, + "is_suppressed": false, + "is_stale": false, + "is_feature_enabled": true, + "last_updated_at": null, + "unavailable_message": "Peer progress is currently unavailable." +} +``` + +### Disabled +``` json +{ + "task_definition_id": 12, + "unit_id": 5, + "target_grade": 2, + "submitted_percentage": null, + "is_suppressed": false, + "is_stale": false, + "is_feature_enabled": false, + "last_updated_at": null, + "unavailable_message": "Peer progress is currently unavailable." +} +``` + +## Error responses + +Business states such as suppressed, stale, disabled and missing data return the +normal nine-field HTTP 200 response. + +Access-control and technical failures use a separate error response: + +```json +{ + "error": "Safe error message" +} +``` +- `404`: the student cannot safely access the requested project or task. +- `419`: authentication failed through the existing OnTrack authentication flow. +- `503`: required PPI configuration is missing or invalid. + +## Privacy boundary + +The response must not include names, usernames, student IDs, peer project IDs, +marks, feedback, individual task statuses, submitted counts, or raw cohort sizes. +The endpoint reads `cohort_size` only to apply suppression. + +An empty cohort and any cohort below the configured privacy threshold return the same suppressed state. A suppressed snapshot can also be stale, so `is_suppressed` and `is_stale` may both be `true`. + + +## Target-grade change protection + +Each project records `target_grade_changed_at`. The API does not return a +snapshot calculated before the current target-grade selection. After a target +grade change, peer progress remains unavailable until a newer aggregation run +creates a snapshot for that grade. + +## Configuration + +- `DF_PPI_MINIMUM_COHORT_SIZE`: approved minimum cohort size. +- `DF_PPI_STALE_AFTER_HOURS`: approved maximum snapshot age. + +`DF_PPI_STALE_AFTER_HOURS` must be a positive integer. `DF_PPI_MINIMUM_COHORT_SIZE` must be an integer of at least `PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE`, which is `21`. A lower value is rejected rather than honoured, so configuration alone cannot defeat suppression. No production defaults are included. An enabled unit +with a valid snapshot fails closed with HTTP 503 when either value is missing or invalid. + +## Feature enablement + +`units.peer_progress_enabled` defaults to `false`. Enable it for a test unit only after +privacy thresholds and the endpoint have been reviewed. + +## Status behaviour + +Malformed integer path parameters may return HTTP 400 from Grape before the project or task lookup. + +- `200`: authorised request, including normal, zero, suppressed, stale, disabled, or unavailable state. +- `404`: wrong user, project, unit, task, target-grade applicability, inactive unit, or unreleased task. The same message is used to reduce object enumeration. +- `419`: OnTrack authentication failed. +- `503`: required PPI configuration is missing or invalid. + +## Handover + +The background job creates the snapshots. This endpoint only authorises the student, +selects the correct stored snapshot, applies display suppression and freshness rules, +and returns an allowlisted response. Frontend HTTP mapping remains in PPI-F01. diff --git a/docs/peer-progress/data-source-map.md b/docs/peer-progress/data-source-map.md new file mode 100644 index 0000000000..926106f7b5 --- /dev/null +++ b/docs/peer-progress/data-source-map.md @@ -0,0 +1,238 @@ +# Peer Progress Indicator — Backend Data-Source Map + +**Ticket:** PPI-D02 — Publish the peer-progress backend data-source and field-ownership map +**Status:** Documentation only. No production code is implemented or modified by this ticket. +**Builds on:** [PPI API discovery](./task-completion-data-discovery.md) — the earlier starter task that +located existing task-completion data (`Task`, `TaskStatus`, `Project#task_stats`, +`Unit#student_task_completion_stats`) and found it was not reachable by students. This document goes +one level deeper: it maps the current backend implementation against the agreed PPI response contract, +field by field, and records what is still open. + +## Implementation status + +PPI-B01 was developed on `ppi/student-progress-endpoint` and merged into the shared +`feature/peer-progress-indicator` branch through API PR #16 (merge commit `1e011b12`). The source branch +has since been deleted. Everything marked "available" below is therefore available on the shared +objective branch; the PR #16 head (`91d4db95`) remains the useful review snapshot for the implementation. + +This document does not implement or modify that backend code. It records what the merged implementation +contains so the rest of the team can build against it without re-discovering it. + +--- + +## 1. Backend data-source table + +| File | Class / method | Branch | Role | +|---|---|---|---| +| `app/api/peer_progress_api.rb` | `PeerProgressApi` (Grape API), `get '/projects/:id/task_def_id/:task_definition_id/peer_progress'` | `feature/peer-progress-indicator` (PPI-B01, merged via #16) | Student-facing endpoint. Authorises the request, looks up the stored snapshot, applies suppression/staleness rules, returns the allowlisted response. | +| `app/models/peer_progress_snapshot.rb` | `PeerProgressSnapshot` | `feature/peer-progress-indicator` (PPI-B01, merged via #16) | One row per `(unit, task_definition, target_grade)`. Stores `cohort_size`, `submitted_percentage`, `calculated_at`. Validates target grade is enabled for the unit and covers the task. | +| `app/services/peer_progress_aggregation_service.rb` | `PeerProgressAggregationService.call(unit:, calculated_at:)` | `feature/peer-progress-indicator` (PPI-B01, merged via #16) | Batch job logic. For each grade value in the unit, selects the eligible cohort and counts submissions per task, then upserts `PeerProgressSnapshot` rows. | +| `app/sidekiq/aggregate_peer_progress_job.rb` | `AggregatePeerProgressJob#perform(unit_id = nil)` | `feature/peer-progress-indicator` (PPI-B01, merged via #16) | Scheduled dispatcher selects active, PPI-enabled units and enqueues one job per unit. Each per-unit job rechecks active/enabled state before calling the aggregation service. Scheduled via `config/schedule.yml` — `"every day at 11:45pm"`. | +| `db/migrate/20260809153000_create_peer_progress_snapshots.rb` | — | `feature/peer-progress-indicator` (PPI-B01, merged via #16) | Creates `peer_progress_snapshots` table. Comment in the migration explicitly flags `cohort_size` as "Internal only. Never expose this raw value through the student API." | +| `db/migrate/20260810033824_add_peer_progress_enabled_to_units.rb` | — | `feature/peer-progress-indicator` (PPI-B01, merged via #16) | Adds `units.peer_progress_enabled` boolean, `default: false, null: false`. | +| `app/models/unit.rb` | `Unit#active_projects` | `feature/peer-progress-indicator` (pre-existing) | Reused as the base scope for cohort selection (`unit.active_projects.where(target_grade: …)`). | +| `app/models/unit.rb` | `Unit#grade_value?` | `feature/peer-progress-indicator` (pre-existing) | Reused to validate a project's `target_grade` is actually a value the unit has enabled, both when aggregating and when deriving the safe target grade for a request. | +| `app/models/unit.rb` | `Unit.active_units` | `feature/peer-progress-indicator` (pre-existing) | Reused so the nightly dispatcher skips inactive units. The job further scopes this relation to `peer_progress_enabled: true`. | +| `app/models/project.rb` | `Project.for_user(user, include_inactive)` | `feature/peer-progress-indicator` (pre-existing) | Reused to authorise that the requested project actually belongs to the authenticated student. | +| `app/models/task.rb` | `Task#file_uploaded_at` | `feature/peer-progress-indicator` (pre-existing column) | The signal used to decide whether a task counts as "submitted" for aggregation — see note below, this is **not** the same signal the original discovery task found. | +| `app/models/project.rb` | `Project#target_grade_changed_at`, `#record_target_grade_change` (`before_create`/`before_update` callback) | `feature/peer-progress-indicator` (PPI-B01, merged via #16) | New column + callback. Records when a student's target grade last changed, so a snapshot calculated *before* a grade change is never shown as if it applied to the new grade. Backfill migration sets it to "now" for all existing projects — see §5. | +| `db/migrate/20260818160804_add_target_grade_changed_at_to_projects.rb` | — | `feature/peer-progress-indicator` (PPI-B01, merged via #16) | Adds `projects.target_grade_changed_at`, backfilled to the migration run time for existing rows, then `NOT NULL`. | +| `app/api/units_api.rb`, `app/api/entities/unit_entity.rb` | `PUT /units/:id` accepts `peer_progress_enabled`; `UnitEntity` exposes it gated by `can_read_unit_config?` | `feature/peer-progress-indicator` (PPI-B01, merged via #16) | Convenors can toggle PPI on/off through the normal unit-update endpoint. Visibility remains staff-only, matching the "students never see raw config" pattern. | + +### Divergence from the original discovery task + +The [earlier discovery](./task-completion-data-discovery.md) found `Unit#student_task_completion_stats` +and `Project#task_stats` as existing, reusable aggregation infrastructure, built on `TaskStatus.complete`. +**PPI-B01 does not reuse either of them.** It introduces a parallel, PPI-specific path instead: + +- Completion signal: `Task.where(...).where.not(file_uploaded_at: nil)` (a file has been uploaded), not + `task_status_id == TaskStatus.complete.id`. Note this changed mid-development from an earlier + `submission_date`-based check to `file_uploaded_at` — if you're comparing against an older read of + this branch, that's the difference. +- Storage: a new `PeerProgressSnapshot` table, calculated nightly, not the ad-hoc per-request + `Unit#student_task_completion_stats` calculation. + +This looks like a deliberate design choice (a stored nightly snapshot makes the suppression/staleness +checks in the student-facing endpoint cheap and simple), not an oversight. It's recorded here so nobody +assumes the two paths are the same thing, and so **PPI-T01** (calculation rules) has an accurate +starting point if "submitted" vs "complete" needs revisiting. + +--- + +## 2. PPI field-ownership table + +Response contract as implemented in `PeerProgressApi#peer_progress_payload` on +`feature/peer-progress-indicator`. All 9 fields are implemented and merged; **none are conceptually +missing from the task-level response design**. + +| Field | Purpose | Current backend source | Available / Calculated / Missing | Transformation | Owning ticket | +|---|---|---|---|---|---| +| `task_definition_id` | Task context | Request param, validated via `unit.task_definitions.find_by(id:)` | Available | Passthrough of the validated ID | PPI-B01 | +| `unit_id` | Unit context | `project.unit_id` | Available | Passthrough | PPI-B01 | +| `target_grade` | Authorised-project target-grade lookup | `Project#target_grade`, validated through `Unit#grade_value?` inside `safe_target_grade` | Available (validated, not a raw column read) | Returns `nil` if the project has no target grade or it isn't enabled for the unit. This route accepts only `:id` and `:task_definition_id`, so a grade cannot be supplied directly to this request. However, `Project#target_grade` is student-writable through the existing project-update API: it is server-stored, not server-controlled. The timestamp guard withholds older snapshots until the next aggregation, but does not permanently bind a student to one grade band. See §5. | PPI-B01 / PPI-S01 | +| `submitted_percentage` | Anonymous submitted percentage | `PeerProgressSnapshot#submitted_percentage`, computed nightly by `PeerProgressAggregationService#percentage` from `file_uploaded_at` presence counts | Calculated (batch, not live) | Stored rounded to 2 dp; **quantised to the nearest 10 percentage points** at request time (`quantised_percentage`, `PeerProgressApi`) before being returned. The 10-point bucket is paired with a hard cohort floor of 21 and the relationship is pinned by API tests that reject singleton buckets. Forced to `nil` (never `0` used as a sentinel) whenever suppressed, stale, disabled, unavailable, or the snapshot predates the student's last target-grade change. A stored genuine zero remains distinct from `nil`, but a client-facing `0.0` can also mean a small non-zero percentage rounded into the zero bucket. | PPI-B01 (endpoint) / PPI-T01 (whether submission-based is the right definition, and whether 10-point buckets are the agreed granularity) | +| `is_suppressed` | Small-cohort suppression | Computed per-request: `snapshot.cohort_size < minimum_cohort_size!` (env-configured, but hard-floored at `MINIMUM_SAFE_COHORT_SIZE = 21` regardless of config) | Calculated | `cohort_size` itself is read internally but **never included** in the response. An empty cohort (0 students) is deliberately treated identically to "below threshold" — the response can't distinguish "nobody's in this grade band" from "too few to show," by design. **Can be `true` at the same time as `is_stale`** — suppression and staleness are not mutually exclusive branches. The count includes the requesting student's project, so a cohort of 21 means 20 peers plus the reader. | PPI-S01 (approve the threshold) / PPI-B01 (implementation) | +| `is_stale` | Data freshness | Computed per-request: `snapshot.calculated_at < ENV['DF_PPI_STALE_AFTER_HOURS'].hours.ago` | Calculated | Computed once and threaded through every branch, so it can appear alongside `is_suppressed: true` in the same response — see above. | PPI-T01 (approve the freshness window) / PPI-B01 (implementation) | +| `is_feature_enabled` | Whether PPI is on for this unit | `units.peer_progress_enabled` column, `default: false`; settable via `PUT /units/:id` | Available | None | Unit-level config, convenor-controlled. The `db:ppi_sample_data` task does not enable PPI: newly created `PPI1001`/`PPI1002` units keep the default, and existing sample units keep their current setting. See §5. | +| `last_updated_at` | Snapshot freshness display | `snapshot.calculated_at.utc.iso8601` | Available when a snapshot exists, else `nil` | ISO 8601 UTC string | PPI-B01 / PPI-F01 (display formatting) | +| `unavailable_message` | Safe unavailable message | Hardcoded Ruby constants in `PeerProgressApi` (`UNAVAILABLE_MESSAGE`, etc.) | Available, but **placeholder wording** | None | PPI-D01 — user-facing wording is explicitly out of scope for PPI-B01; the current strings are implementation placeholders, not approved copy. | + +### Fields the response must never include (confirmed by code review) + +`peer_progress_payload` is an allowlist — it only ever builds the 9 fields above. Confirmed absent: +peer names, usernames, student IDs, peer project IDs, marks, feedback, individual task statuses, raw +cohort records, and raw `cohort_size` / submitted counts. The migration comment on `cohort_size` +explicitly flags it as internal-only. This satisfies acceptance criterion 6 based on the code merged +through API PR #16. That PR received a privacy-focused independent review and corrective commit; the +dedicated PPI-S01 ticket should still decide the explicitly retained risks listed in §5 against the +merged code and deployment settings. + +--- + +## 3. Proposed / actual data-flow diagram + +```mermaid +flowchart TD + A["Authenticated student user
GET /api/projects/:id/task_def_id/:task_definition_id/peer_progress"] --> B["PeerProgressApi
authenticated? + role == student"] + B -->|"not a student / project not found"| X1["404 Not Found
(same message for all cases - avoids object enumeration)"] + B -->|ok| C["Project.for_user current_user
= authorised project/unit"] + C --> D["Task validation:
unit.task_definitions.find_by id
+ effective_task local_start_date released? (honours extensions)"] + D -->|"not found / not released"| X1 + D -->|ok| E["safe_target_grade project
= authorised-project target-grade lookup
(server-stored and student-writable elsewhere;
validated via Unit#grade_value?)"] + E -->|"nil / not applicable"| F1a["200 OK, unavailable
target_grade: null
= no valid target grade"] + E -->|valid| F["PeerProgressSnapshot lookup
by unit_id + task_definition_id + target_grade"] + + subgraph nightly ["Nightly dispatcher - AggregatePeerProgressJob (11:45pm)"] + G["Unit.active_units.where
peer_progress_enabled: true"] --> G1["enqueue one AggregatePeerProgressJob
per enabled active unit"] + G1 --> H["PeerProgressAggregationService.call"] + H --> I["Unit#active_projects.where target_grade: ...
= eligible cohort selection"] + I --> J["Task.where project in cohort,
file_uploaded_at not null
= aggregate calculation"] + J --> K[("PeerProgressSnapshot row
cohort_size, submitted_percentage, calculated_at")] + end + + K -.snapshot read at request time.-> F + F -->|"no snapshot yet"| F1b["200 OK, unavailable
target_grade: present
= no snapshot for a valid target grade"] + F -->|found| R{"snapshot.calculated_at older than
project.target_grade_changed_at ?"} + R -->|yes| F1b + R -->|no| L{"cohort_size below hard floor of 21,
or below DF_PPI_MINIMUM_COHORT_SIZE ?"} + L -->|yes| M1["200 OK
is_suppressed: true
(is_stale may ALSO be true)
= small-cohort suppression"] + L -->|no| N{"calculated_at older than
DF_PPI_STALE_AFTER_HOURS ?"} + N -->|yes| M2["200 OK
is_stale: true, percentage: null"] + N -->|no| M3["quantised_percentage
round to nearest 10 points"] + M3 --> M4["200 OK
submitted_percentage, last_updated_at
= safe API response"] + + F1a --> O + F1b --> O + M1 --> O + M2 --> O + M4 --> O["PeerProgressIndicatorService.getIndicator
frontend adapter (PPI-F01)
currently returns MOCK data only"] + O --> P["resolvePeerProgressState
PPI-F03 - UI state mapping"] + P --> Q["PpiWidgetComponent (f-ppi-widget)
existing PPI component
rendered inside task-description-card"] +``` + +--- + +## 4. Safe example responses + +Reproduced from the merged `docs/peer-progress-api.md` (PPI-B01), which documents these in more state +variations than required here. Shown in the backend's snake_case; the task-widget frontend model +(`PeerProgressIndicator`) uses the corresponding camelCase names (`submittedPercentage`, +`isSuppressed`, etc.). Its current `targetGrade` and `lastUpdatedAt` types are incorrectly non-nullable +for this response contract and must be widened before the live adapter lands — see §5. + +### Normal aggregate result + +Note `submitted_percentage` is quantised to the nearest 10 — the raw stored aggregate (e.g. 62.5) is +never returned; this example shows the quantised value the client actually receives. + +```json +{ + "task_definition_id": 12, + "unit_id": 5, + "target_grade": 2, + "submitted_percentage": 60.0, + "is_suppressed": false, + "is_stale": false, + "is_feature_enabled": true, + "last_updated_at": "2026-08-10T03:15:00Z", + "unavailable_message": "" +} +``` + +### Small-cohort-suppressed result + +```json +{ + "task_definition_id": 12, + "unit_id": 5, + "target_grade": 2, + "submitted_percentage": null, + "is_suppressed": true, + "is_stale": false, + "is_feature_enabled": true, + "last_updated_at": "2026-08-10T03:15:00Z", + "unavailable_message": "Peer progress is currently unavailable." +} +``` + +### Unavailable result — no valid target grade + +```json +{ + "task_definition_id": 12, + "unit_id": 5, + "target_grade": null, + "submitted_percentage": null, + "is_suppressed": false, + "is_stale": false, + "is_feature_enabled": true, + "last_updated_at": null, + "unavailable_message": "Peer progress is currently unavailable." +} +``` + +### Unavailable result — valid target grade, no usable snapshot yet + +This is also what a student sees after changing their target grade, until the next successful +aggregation creates a snapshot newer than that change. Environments that already contain PPI snapshots +when the target-grade timestamp migration runs see the same state until aggregation is rerun — see §5. + +```json +{ + "task_definition_id": 12, + "unit_id": 5, + "target_grade": 2, + "submitted_percentage": null, + "is_suppressed": false, + "is_stale": false, + "is_feature_enabled": true, + "last_updated_at": null, + "unavailable_message": "Peer progress is currently unavailable." +} +``` + +--- + +## 5. Confirmed status, gaps and unresolved decisions + +| # | Gap / decision | Detail | Owner | +|---|---|---|---| +| 1 | **Backend merged** | PPI-B01 merged through API PR #16 at `1e011b12`; the implementation is present on `feature/peer-progress-indicator` and the source branch was deleted. | PPI-B01 (complete) | +| 2 | **Frontend live-adapter mismatch** | The current mock widget calls `getIndicator(taskDefId, unitId, targetGrade, mockState)`. The real route expects an authorised project ID (`:id`) plus `:task_definition_id`; it derives unit and grade from that project. PPI-F01 should replace the mock signature with a project/task request, not forward `unitId`, `targetGrade`, or `mockState`. It must also widen `PeerProgressIndicator.targetGrade` and `.lastUpdatedAt` to accept `null`, as the backend contract does. | PPI-F01 | +| 3 | **Two distinct frontend PPI contracts** | Both contracts are now merged into the web objective branch. `PeerProgressIndicator` / `PeerProgressIndicatorService` represents the task-level percentage widget. `PeerProgressResponse` / `PeerProgressService` represents a weekly burndown median with different fields. This is not a rename conflict and the types are not interchangeable; both services remain mock-backed pending their respective live API work. | PPI-F01 / burndown API owner | +| 4 | **Production config still needs approval** | `doubtfire-deploy` 11.0.x supplies local-development values in `development/api.env` and both Compose files: `DF_PPI_MINIMUM_COHORT_SIZE=21` and `DF_PPI_STALE_AFTER_HOURS=48`. Production must supply separately reviewed values. The API rejects a cohort setting below the hard floor of 21, and the floor is coupled to the 10-point percentage bucket by tests. | PPI-T01 / PPI-S01 (approve production values) | +| 5 | **Sample units are disabled and too small by default** | `units.peer_progress_enabled` defaults `false`. The merged `db:ppi_sample_data` task does not set it: newly created `PPI1001` / `PPI1002` units stay disabled, while existing units retain their current value. The task creates 2 classes × 4 students per grade, so each target-grade cohort has 8 students and remains suppressed under the hard floor of 21 even if a convenor enables the unit through `PUT /units/:id`. | PPI test-data / integration owner | +| 6 | **Placeholder wording** | `unavailable_message` strings are hardcoded in Ruby, written by whoever built PPI-B01, not reviewed for tone/wording. | PPI-D01 | +| 7 | **Privacy follow-ups remain** | API PR #16 received an independent privacy/authorisation review and the blocking count-recovery issue was fixed before merge. Two accepted follow-ups remain: students can change `Project#target_grade` and read the new band after the next aggregation, so the timestamp guard rate-limits band enumeration rather than closing it; and `cohort_size` includes the requesting student, so the floor of 21 can mean 20 peers plus the reader. | PPI-S01 | +| 8 | **Backfill invalidates snapshots in already-running PPI environments** | `add_target_grade_changed_at_to_projects` backfills existing projects to migration time, so any snapshot calculated before that time is withheld until aggregation runs again. On the first deployment of the complete PPI migration series the snapshot table is created empty, so there is nothing to invalidate. This matters to development or staging environments that ran the earlier snapshot migration and aggregation before applying the later timestamp migration. | PPI-B01 (deploy sequencing) | +| 9 | **Suppression and staleness are not mutually exclusive** | `is_suppressed` and `is_stale` can both be `true`. The current frontend `resolvePeerProgressState` checks `isSuppressed` before `isStale`, so a suppressed-and-stale response resolves to the "hidden" UI state. PPI-F01/PPI-F03 should confirm that priority is intentional. | PPI-F01 / PPI-F03 | + +--- + +## 6. Explicitly out of scope for this document + +This document does not implement the backend endpoint (PPI-B01), the frontend adapter (PPI-F01), the +unit-level component (PPI-F02), percentage calculation rules (PPI-T01), loading/error states (PPI-F03), +the dedicated security follow-up (PPI-S01), or user-facing wording (PPI-D01). It does not create another +mock-data service or another minimal test-data task. Where this document identifies a security-relevant +boundary (§2, §5), that observation does not replace PPI-S01 sign-off on the retained risks. diff --git a/docs/peer-progress/task-completion-data-discovery.md b/docs/peer-progress/task-completion-data-discovery.md new file mode 100644 index 0000000000..4c9304dcf9 --- /dev/null +++ b/docs/peer-progress/task-completion-data-discovery.md @@ -0,0 +1,62 @@ +# PPI — Locate existing task-completion data in the API + +**Original ticket:** PPI - Locate existing task-completion data in the API (Discovery, starter task) +**Author:** Gaurav Manohar Myana +**Repo checked at the time:** `doubtfire-api`, branch `feature/peer-progress-indicator` + +> Preserved here, unedited from the original ticket deliverable, per PPI-D02's requirement to keep a +> link to the prior discovery work. See [data-source-map.md](./data-source-map.md) for how this +> compares against the actual PPI-B01 implementation found on `ppi/student-progress-endpoint`. + +## Purpose + +Find what task-completion data already exists in the API, so the Peer Progress Indicator isn't +designed around information that isn't actually available. + +## Relevant Rails models + +| Model | File | Relevant fields/notes | +|---|---|---| +| `Task` | `app/models/task.rb` | `task_status_id`, `completion_date`, `target_start_date`, `submission_date` | +| `TaskStatus` | `app/models/task_status.rb` | 15 fixed statuses (complete, working_on_it, fail, etc.) | +| `Project` (student's enrolment in a unit) | `app/models/project.rb` | `task_stats` (JSON): `{ red_pct, orange_pct, green_pct, blue_pct, grey_pct, order_scale }` — one student's own task-status mix | +| `Unit` | `app/models/unit.rb` | `#student_task_completion_stats` — cohort-wide median/min/max/quartile of completed tasks, broken down by tutorial and grade | + +## Relevant API endpoints + +| Endpoint | Access | Returns | +|---|---|---| +| `GET /projects/:id` | Authenticated user | Individual `task_stats` — **but hidden from the student themselves** (`unless: :for_student` in `ProjectEntity`) | +| `GET /units/:id/stats/task_completion_stats` | Staff only (`:download_stats`) | Cohort-wide completed-task stats (median/min/max/quartiles) by unit/tutorial/grade | +| `GET /units/:id/stats/task_completion_snapshots` | Staff only (`:download_stats`) | Historical point-in-time snapshots of status counts | + +## Data gap + +**No student-facing endpoint exposes any peer/cohort completion data**, and a student can't even see +their own `task_stats`. Confirmed in two places: + +1. `Unit.permissions` grants students only `[:get_unit]` — `:download_stats` is staff-only. +2. `ProjectEntity` explicitly excludes `task_stats` when the viewer is the student themselves. + +## Key finding + +The aggregation the PPI needs — anonymized cohort completed-task stats (median/quartiles by +tutorial/grade) — **already exists** in `Unit#student_task_completion_stats`. It does not need to be +built. It's just not reachable by students. + +## Recommended next step + +Add a new, student-authorised endpoint (e.g. `GET /units/:id/my_progress`) that returns the calling +student's own `task_stats` plus the cohort aggregate for their tutorial/grade, by reusing +`Unit#student_task_completion_stats` — without granting students the broader `:download_stats` +permission. + +## Blockers + +None. Scope was read-only exploration of the existing codebase; no production code changed. + +## What actually happened next (added retrospectively for PPI-D02) + +The recommendation above (reuse `Unit#student_task_completion_stats`) was **not** what PPI-B01 built. +See [data-source-map.md](./data-source-map.md) §1 "Divergence from the original discovery task" for +what was actually implemented instead, and why. diff --git a/docs/pull_request_template.md b/docs/pull_request_template.md index b70119000e..92d5dc9709 100644 --- a/docs/pull_request_template.md +++ b/docs/pull_request_template.md @@ -1,35 +1,41 @@ -# Description +## Jira ticket -Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. +Ticket number or link: -Fixes # (issue) +## Summary -## Type of change +Briefly explain what you changed and why. -Please delete options that are not relevant. +## Target branch -- [ ] Bug fix (non-breaking change which fixes an issue) -- [ ] New feature (non-breaking change which adds functionality) -- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) -- [ ] This change requires a documentation update +Which shared branch should this be merged into? -# How Has This Been Tested? +Example: `feature/email-notifications` -Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration +## Testing -- [ ] Test A -- [ ] Test B +Explain how you tested the change. -# Checklist: +Include any useful commands, screenshots, logs, or test results. -- [ ] My code follows the style guidelines of this project -- [ ] I have performed a self-review of my own code -- [ ] I have commented my code, particularly in hard-to-understand areas -- [ ] I have made corresponding changes to the documentation if appropriate -- [ ] My changes generate no new warnings -- [ ] I have added tests that prove my fix is effective or that my feature works -- [ ] I have created or extended unit tests to address my new additions -- [ ] New and existing unit tests pass locally with my changes -- [ ] Any dependent changes have been merged and published in downstream modules +## Security and privacy -If you have any questions, please contact @macite or @jakerenzella. +Does this change affect authentication, permissions, notifications, student data, +secrets, personal information, or privacy? + +If there is no known impact, write: `No known security or privacy impact.` + +## Evidence + +Add any screenshots, test output, diagrams, or other evidence that will help the reviewer. + +## Checklist + +- [ ] I selected the correct base branch. +- [ ] My changes match the assigned Jira ticket. +- [ ] I kept the change within the agreed scope. +- [ ] I tested my changes. +- [ ] I did not include passwords, tokens, API keys, secrets, or real student data. +- [ ] I updated relevant documentation, or no documentation change was needed. +- [ ] I reviewed my own changes before requesting review. +- [ ] This pull request is ready for review. diff --git a/lib/tasks/ppi_sample_data.rake b/lib/tasks/ppi_sample_data.rake new file mode 100644 index 0000000000..06657164f8 --- /dev/null +++ b/lib/tasks/ppi_sample_data.rake @@ -0,0 +1,135 @@ +require_all 'lib/helpers' + +namespace :db do + desc 'Create a small, deterministic sample dataset for testing the Peer Progress Indicator dashboard' + task ppi_sample_data: [:skip_prod, :environment] do + Rails.logger.level = :info + + # ---- configuration ------------------------------------------------- + num_units = 2 + classes_per_unit = 2 + students_per_grade = 4 + grade_labels = { 0 => 'Pass', 1 => 'Credit', 2 => 'Distinction', 3 => 'HighDistinction' }.freeze + grades = grade_labels.keys.freeze # [0, 1, 2, 3] + num_tasks = 7 # within the requested 5-10 range + weekdays = %w[Monday Tuesday Wednesday Thursday Friday].freeze + + # ---- helpers --------------------------------------------------------- + + # Finds or creates a user with a fixed, deterministic username - safe to re-run. + def ppi_find_or_create_user(username, first_name, last_name, role_id) + existing = User.find_by(username: username) + return existing if existing + + profile = { + first_name: first_name, + last_name: last_name, + nickname: username, + role_id: role_id, + email: "#{username}@doubtfire.com", + username: username + } + unless AuthenticationHelpers.aaf_auth? + profile[:password] = 'password' + profile[:password_confirmation] = 'password' + end + User.create!(profile) + end + + campus = Campus.first || Campus.create!(name: 'Online', mode: 'timetable', abbreviation: 'C', active: true) + convenor = ppi_find_or_create_user('ppi_convenor', 'Peer', 'Convenor', Role.convenor_id) + + (1..num_units).each do |unit_num| + code = "PPI100#{unit_num}" + unit = Unit.find_by(code: code) || Unit.create!( + code: code, + name: "PPI Sample Unit #{unit_num}", + description: 'Deterministic sample data for testing the Peer Progress Indicator dashboard. Not a real unit.', + start_date: Time.zone.now - 6.weeks, + end_date: Time.zone.now + 7.weeks + ) + + unit.employ_staff(convenor, Role.convenor) + + # All tasks are assigned regardless of a student's target grade (target_grade: 0 = Pass), + # so every student in the unit has the same task list - needed to compare % completion + # meaningfully across target-grade bands. + task_defs = (1..num_tasks).map do |t| + unit.task_definitions.find_by(abbreviation: "T#{t}") || TaskDefinition.create!( + unit_id: unit.id, + name: "Task #{t}", + abbreviation: "T#{t}", + description: "Sample task #{t} for PPI dashboard testing.", + weighting: BigDecimal('1'), + target_grade: 0, + start_date: unit.start_date, + target_date: unit.start_date + t.weeks, + upload_requirements: [{ key: 'file0', name: 'Document', type: 'document' }] + ) + end + + (1..classes_per_unit).each do |class_num| + tutor_username = "ppi_tutor_u#{unit_num}c#{class_num}" + tutor = ppi_find_or_create_user(tutor_username, "Tutor#{unit_num}#{class_num}", 'PPI', Role.tutor_id) + unit.employ_staff(tutor, Role.tutor) + + tutorial_abbrev = "PPI-U#{unit_num}-C#{class_num}" + tutorial = unit.tutorials.find_by(abbreviation: tutorial_abbrev) || unit.add_tutorial( + weekdays[class_num - 1], + '10:00', + "EN1-0#{class_num}", + tutor, + campus, + students_per_grade * grades.length, + tutorial_abbrev + ) + + student_index = 0 + + grades.each do |target_grade| + students_per_grade.times do |i| + student_index += 1 + username = "ppi_u#{unit_num}c#{class_num}s#{student_index.to_s.rjust(2, '0')}" + student = ppi_find_or_create_user(username, "Student#{student_index}", grade_labels[target_grade], Role.student_id) + + project = unit.enrol_student(student, campus) + project.update!(target_grade: target_grade) + project.enrol_in(tutorial) + + # Vary completion so percentages differ meaningfully both between tasks + # and between target-grade bands: + # - higher target grade -> higher base completion rate + # - later tasks -> lower completion rate (fewer students have reached them) + # - a small per-student jitter spreads the 4 students within a grade band + task_defs.each_with_index do |td, td_idx| + task = project.task_for_task_definition(td) + next unless task.task_status_id == TaskStatus.not_started.id # skip on re-run + + base_completion = (target_grade + 1) / grades.length.to_f # 0.25, 0.5, 0.75, 1.0 + task_decay = 1.0 - ((td_idx.to_f / task_defs.length) * 0.4) + student_jitter = (i - ((students_per_grade - 1) / 2.0)) * 0.05 + completion_chance = ((base_completion * task_decay) + student_jitter).clamp(0.05, 0.98) + + seed = (student_index * 13) + (td_idx * 7) + (unit_num * 31) + (class_num * 17) + roll = (seed % 100) / 100.0 + + if roll < completion_chance + complete_date = [unit.start_date + (td_idx + 1).weeks + rand(0..3).days, Time.zone.now].min + DatabasePopulator.assess_task(project, task, tutor, TaskStatus.complete, complete_date) + elsif roll < completion_chance + 0.15 + DatabasePopulator.assess_task(project, task, tutor, TaskStatus.working_on_it, Time.zone.now) + end + # otherwise left as not_started (the Task.create! default) + end + + project.update_task_stats + end + end + end + + puts "-> #{unit.code}: #{unit.tutorials.count} classes, #{unit.projects.count} students, #{task_defs.count} tasks" + end + + puts 'PPI sample dashboard data ready.' + end +end diff --git a/test/api/peer_progress_api_test.rb b/test/api/peer_progress_api_test.rb new file mode 100644 index 0000000000..bd37554b08 --- /dev/null +++ b/test/api/peer_progress_api_test.rb @@ -0,0 +1,882 @@ +# frozen_string_literal: true + +require 'test_helper' +require 'time' + +class PeerProgressApiTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + RESPONSE_KEYS = %w[ + task_definition_id + unit_id + target_grade + submitted_percentage + is_suppressed + is_stale + is_feature_enabled + last_updated_at + unavailable_message + ].freeze + + FORBIDDEN_KEYS = %w[ + cohort_size + submitted_count + user_id + student_id + username + first_name + last_name + project_id + task_status + marks + feedback + ].freeze + + setup do + clear_auth_header + + @original_minimum_cohort_size = + ENV.fetch('DF_PPI_MINIMUM_COHORT_SIZE', nil) + + @original_stale_after_hours = + ENV.fetch('DF_PPI_STALE_AFTER_HOURS', nil) + ENV['DF_PPI_MINIMUM_COHORT_SIZE'] = + PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE.to_s + ENV['DF_PPI_STALE_AFTER_HOURS'] = '48' + + @unit = create( + :unit, + with_students: false, + task_count: 0, + stream_count: 0, + tutorials: 1, + staff_count: 0, + outcome_count: 0 + ) + @unit.update!(peer_progress_enabled: true) + + @student = create(:user, :student) + @project = @unit.enrol_student( + @student, + @unit.tutorials.first.campus + ) + @project.update!(target_grade: 1) + @project.update!(target_grade_changed_at: 1.year.ago) + + @task_definition = create( + :task_definition, + unit: @unit, + target_grade: 0, + start_date: Time.zone.parse('2026-01-01 00:00:00 UTC'), + outcome_count: 0 + ) + end + + teardown do + restore_env( + 'DF_PPI_MINIMUM_COHORT_SIZE', + @original_minimum_cohort_size + ) + restore_env( + 'DF_PPI_STALE_AFTER_HOURS', + @original_stale_after_hours + ) + clear_auth_header + end + + test 'requires authentication' do + get endpoint + + assert_equal 419, last_response.status + assert_private_no_store + end + + test 'returns a privacy-safe normal response for the owning student' do + create_snapshot( + submitted_percentage: 62.5, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + ) + + request_as(@student) + + assert_equal 200, last_response.status, last_response.body + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_equal @task_definition.id, body['task_definition_id'] + assert_equal @unit.id, body['unit_id'] + assert_equal @project.target_grade, body['target_grade'] + assert_equal 60.0, body['submitted_percentage'] + assert_equal false, body['is_suppressed'] + assert_equal false, body['is_stale'] + assert_equal true, body['is_feature_enabled'] + assert body['last_updated_at'].present? + assert_equal '', body['unavailable_message'] + end + + test 'returns a genuine zero as zero rather than unavailable' do + create_snapshot( + submitted_percentage: 0, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + ) + + request_as(@student) + + assert_equal 200, last_response.status + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_equal 0.0, body['submitted_percentage'] + assert_equal false, body['is_suppressed'] + assert_equal false, body['is_stale'] + assert_equal '', body['unavailable_message'] + end + + test 'does not allow access before a student specific flexible start date' do + @unit.update!(allow_flexible_dates: true) + + create( + :task, + project: @project, + task_definition: @task_definition, + task_status: TaskStatus.not_started, + target_start_date: 1.day.from_now + ) + + request_as(@student) + + assert_peer_progress_not_found + end + + test 'does not allow access before a target grade specific start date' do + @unit.update!(allow_flexible_dates: true) + + TaskDefinitionGradeDueDate.create!( + task_definition: @task_definition, + target_grade: @project.target_grade, + start_date: 1.day.from_now, + target_due_date: @task_definition.target_date + ) + + request_as(@student) + + assert_peer_progress_not_found + end + + test 'allows access after the target grade specific start date' do + @unit.update!(allow_flexible_dates: true) + + TaskDefinitionGradeDueDate.create!( + task_definition: @task_definition, + target_grade: @project.target_grade, + start_date: 1.day.ago, + target_due_date: @task_definition.target_date + ) + + create_snapshot( + submitted_percentage: 50, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + ) + + request_as(@student) + + assert_equal 200, last_response.status + assert_equal 50.0, last_response_body['submitted_percentage'] + end + + test 'does not create a task row while checking the release date' do + create_snapshot( + submitted_percentage: 50, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + ) + + assert_no_difference('Task.count') do + request_as(@student) + end + + assert_equal 200, last_response.status + end + + test 'quantises the student percentage to ten point buckets' do + create_snapshot( + submitted_percentage: 61, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + ) + + request_as(@student) + + assert_equal 200, last_response.status + assert_equal 60.0, last_response_body['submitted_percentage'] + end + + test 'fails closed when the cohort configuration is below the privacy floor' do + create_snapshot( + submitted_percentage: 50, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + ) + + ENV['DF_PPI_MINIMUM_COHORT_SIZE'] = '20' + + request_as(@student) + + assert_equal 503, last_response.status + assert_equal( + PeerProgressApi::CONFIG_ERROR_MESSAGE, + last_response_body['error'] + ) + assert_private_no_store + end + + test 'accepts a configured threshold above the privacy floor' do + configured_threshold = PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + 1 + ENV['DF_PPI_MINIMUM_COHORT_SIZE'] = configured_threshold.to_s + + create_snapshot( + submitted_percentage: 50, + cohort_size: configured_threshold + ) + + request_as(@student) + + assert_equal 200, last_response.status + assert_equal 50.0, last_response_body['submitted_percentage'] + assert_equal false, last_response_body['is_suppressed'] + end + + test 'does not allow a student to read another students project' do + other_student = create(:user, :student) + other_project = @unit.enrol_student( + other_student, + @unit.tutorials.first.campus + ) + other_project.update!(target_grade: 1) + + request_as( + @student, + endpoint(project: other_project) + ) + + assert_peer_progress_not_found + end + + test 'does not allow a tutor to use the student endpoint' do + tutor = create(:user, :tutor) + @unit.employ_staff(tutor, Role.tutor) + + request_as(tutor) + + assert_peer_progress_not_found + end + + test 'does not allow an unenrolled project' do + @project.update!(enrolled: false) + + request_as(@student) + + assert_peer_progress_not_found + end + + test 'does not allow an inactive unit in the first release' do + @unit.update!(active: false) + + request_as(@student) + + assert_peer_progress_not_found + end + + test 'does not allow a task from another unit' do + other_unit = create( + :unit, + with_students: false, + task_count: 0, + stream_count: 0, + tutorials: 0, + staff_count: 0, + outcome_count: 0 + ) + other_task = create( + :task_definition, + unit: other_unit, + target_grade: 0, + start_date: 1.day.ago, + outcome_count: 0 + ) + + request_as( + @student, + endpoint(task_definition: other_task) + ) + + assert_peer_progress_not_found + end + + test 'does not allow a task above the students target grade' do + higher_grade_task = create( + :task_definition, + unit: @unit, + target_grade: 2, + start_date: 1.day.ago, + outcome_count: 0 + ) + + request_as( + @student, + endpoint(task_definition: higher_grade_task) + ) + + assert_peer_progress_not_found + end + + test 'does not allow an unreleased task' do + future_task = create( + :task_definition, + unit: @unit, + target_grade: 0, + start_date: 1.day.from_now, + outcome_count: 0 + ) + + request_as( + @student, + endpoint(task_definition: future_task) + ) + + assert_peer_progress_not_found + end + + test 'returns a neutral unavailable state when no snapshot exists' do + request_as(@student) + + assert_equal 200, last_response.status + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_nil body['submitted_percentage'] + assert_equal false, body['is_suppressed'] + assert_equal false, body['is_stale'] + assert_equal true, body['is_feature_enabled'] + assert_nil body['last_updated_at'] + assert body['unavailable_message'].present? + end + + test 'suppresses the formerly unsafe cohort of twenty' do + create_snapshot( + submitted_percentage: 50, + cohort_size: 20 + ) + + request_as(@student) + + assert_equal 200, last_response.status + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_nil body['submitted_percentage'] + assert_equal true, body['is_suppressed'] + assert_equal false, body['is_stale'] + assert body['unavailable_message'].present? + assert_not body.key?('cohort_size') + end + + test 'shows a cohort at the exact configured threshold' do + create_snapshot( + submitted_percentage: 40, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + ) + + request_as(@student) + + assert_equal 200, last_response.status + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_equal 40.0, body['submitted_percentage'] + assert_equal false, body['is_suppressed'] + end + + test 'keeps half a bucket wider than one students share of the smallest cohort' do + # The zero and hundred edge buckets are only non-singletons while one + # student's share is smaller than half the bucket width. + assert_operator( + PeerProgressApi::PERCENTAGE_BUCKET_SIZE / 2.0, + :>, + 100.0 / PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE, + 'Half of PERCENTAGE_BUCKET_SIZE must exceed one student share, or an ' \ + 'edge bucket reveals the exact submitted count' + ) + end + + test 'does not let the quantised percentage reveal the submitted count' do + minimum = PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + + (minimum..1_000).each do |cohort_size| + singleton_buckets = quantised_count_groups(cohort_size).select do |_bucket, counts| + counts.one? + end + + assert_empty( + singleton_buckets, + "cohort #{cohort_size} exposes exact submitted counts" + ) + end + + floor_groups = quantised_count_groups(minimum) + assert_equal [0, 1], floor_groups.fetch(0.0) + assert_equal [minimum - 1, minimum], floor_groups.fetch(100.0) + end + + test 'hides the percentage when an active unit snapshot is stale' do + create_snapshot( + submitted_percentage: 50, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE, + calculated_at: 49.hours.ago + ) + + request_as(@student) + + assert_equal 200, last_response.status + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_nil body['submitted_percentage'] + assert_equal false, body['is_suppressed'] + assert_equal true, body['is_stale'] + assert body['last_updated_at'].present? + assert body['unavailable_message'].present? + end + + test 'returns a disabled state when the unit has disabled PPI' do + @unit.update!(peer_progress_enabled: false) + + request_as(@student) + + assert_equal 200, last_response.status + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_nil body['submitted_percentage'] + assert_equal false, body['is_suppressed'] + assert_equal false, body['is_stale'] + assert_equal false, body['is_feature_enabled'] + assert_nil body['last_updated_at'] + assert body['unavailable_message'].present? + end + + test 'ignores a browser supplied target grade' do + create_snapshot( + submitted_percentage: 60, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + ) + + request_as( + @student, + "#{endpoint}?target_grade=3" + ) + + assert_equal 200, last_response.status + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_equal @project.target_grade, body['target_grade'] + assert_equal 60.0, body['submitted_percentage'] + end + + test 'returns a neutral unavailable state when no target grade is selected' do + # Intentionally bypass validations and callbacks to verify that the API + # safely handles a project with no stored target grade. + # rubocop:disable Rails/SkipsModelValidations + @project.update_column(:target_grade, nil) + # rubocop:enable Rails/SkipsModelValidations + + request_as(@student) + + assert_equal 200, last_response.status + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_nil body['target_grade'] + assert_nil body['submitted_percentage'] + assert_equal false, body['is_suppressed'] + assert_equal false, body['is_stale'] + assert_equal true, body['is_feature_enabled'] + assert_nil body['last_updated_at'] + assert_equal( + PeerProgressApi::UNAVAILABLE_MESSAGE, + body['unavailable_message'] + ) + end + + test 'does not expose an invalid stored target grade' do + # Intentionally bypass validations and callbacks to verify that the API + # safely handles an invalid legacy target-grade value. + # rubocop:disable Rails/SkipsModelValidations + @project.update_column(:target_grade, 999) + # rubocop:enable Rails/SkipsModelValidations + + request_as(@student) + + assert_equal 200, last_response.status + + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_nil body['target_grade'] + assert_nil body['submitted_percentage'] + assert_equal false, body['is_suppressed'] + assert_equal false, body['is_stale'] + assert_equal true, body['is_feature_enabled'] + assert_nil body['last_updated_at'] + assert_equal( + PeerProgressApi::UNAVAILABLE_MESSAGE, + body['unavailable_message'] + ) + end + + test 'returns unavailable rather than zero for an empty stored cohort' do + create_snapshot( + submitted_percentage: nil, + cohort_size: 0 + ) + + request_as(@student) + + assert_equal 200, last_response.status + + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_nil body['submitted_percentage'] + assert_equal true, body['is_suppressed'] + assert_equal false, body['is_stale'] + assert_equal true, body['is_feature_enabled'] + assert body['last_updated_at'].present? + assert body['unavailable_message'].present? + end + + test 'returns the snapshot timestamp in UTC ISO 8601 format' do + calculated_at = Time.zone.parse('2026-08-10 03:15:00 UTC') + + create_snapshot( + submitted_percentage: 62.5, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE, + calculated_at: calculated_at + ) + + request_as(@student) + + assert_equal 200, last_response.status + + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_equal( + calculated_at.utc.iso8601, + body['last_updated_at'] + ) + end + + test 'fails closed when the stale window configuration is missing' do + create_snapshot( + submitted_percentage: 50, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + ) + + ENV.delete('DF_PPI_STALE_AFTER_HOURS') + + request_as(@student) + + assert_equal 503, last_response.status + assert_equal( + PeerProgressApi::CONFIG_ERROR_MESSAGE, + last_response_body['error'] + ) + assert_private_no_store + end + + test 'returns the same generic response for unknown project and task ids' do + unknown_project_id = Project.maximum(:id).to_i + 10_000 + + request_as( + @student, + "/api/projects/#{unknown_project_id}/task_def_id/" \ + "#{@task_definition.id}/peer_progress" + ) + + assert_peer_progress_not_found + + unknown_task_id = TaskDefinition.maximum(:id).to_i + 10_000 + + request_as( + @student, + "/api/projects/#{@project.id}/task_def_id/" \ + "#{unknown_task_id}/peer_progress" + ) + + assert_peer_progress_not_found + end + + test 'fails closed for invalid positive integer configuration' do + create_snapshot( + submitted_percentage: 50, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + ) + + [ + ['DF_PPI_MINIMUM_COHORT_SIZE', '0'], + ['DF_PPI_MINIMUM_COHORT_SIZE', 'not-a-number'], + ['DF_PPI_STALE_AFTER_HOURS', '-1'], + ['DF_PPI_STALE_AFTER_HOURS', '1.5'] + ].each do |name, value| + original = ENV.fetch(name, nil) + + begin + ENV[name] = value + request_as(@student) + + assert_equal 503, last_response.status + assert_equal( + PeerProgressApi::CONFIG_ERROR_MESSAGE, + last_response_body['error'] + ) + assert_private_no_store + ensure + restore_env(name, original) + end + end + end + + test 'keeps a snapshot available at the exact stale boundary' do + travel_to Time.zone.parse('2026-08-10 12:00:00 UTC') do + create_snapshot( + submitted_percentage: 50, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE, + calculated_at: 48.hours.ago + ) + + request_as(@student) + + assert_equal 200, last_response.status + + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_equal 50.0, body['submitted_percentage'] + assert_equal false, body['is_stale'] + end + end + + test 'does not serve a snapshot created before the target grade changed' do + travel_to Time.zone.parse('2026-08-10 12:00:00 UTC') do + create_snapshot( + target_grade: 2, + submitted_percentage: 60, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE, + calculated_at: 1.hour.ago + ) + + @project.update!(target_grade: 2) + + request_as(@student) + + assert_equal 200, last_response.status + + body = last_response_body + assert_peer_progress_response_contract(body) + + assert_equal 2, body['target_grade'] + assert_nil body['submitted_percentage'] + assert_equal false, body['is_suppressed'] + assert_nil body['last_updated_at'] + assert body['unavailable_message'].present? + end + end + + test 'records when a project target grade changes' do + project = create(:project) + original_timestamp = project.target_grade_changed_at + + travel 1.minute + project.update!(target_grade: project.target_grade + 1) + + assert_operator( + project.reload.target_grade_changed_at, + :>, + original_timestamp + ) + end + + test 'does not change the grade timestamp for an unrelated update' do + project = create(:project) + original_timestamp = project.target_grade_changed_at + + travel 1.minute + project.update!(started: !project.started) + + assert_equal( + original_timestamp, + project.reload.target_grade_changed_at + ) + end + + test 'fails closed when required PPI configuration is missing' do + create_snapshot( + submitted_percentage: 50, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE + ) + ENV.delete('DF_PPI_MINIMUM_COHORT_SIZE') + + request_as(@student) + + assert_equal 503, last_response.status + assert_equal( + PeerProgressApi::CONFIG_ERROR_MESSAGE, + last_response_body['error'] + ) + assert_private_no_store + end + + test 'serves a fresh snapshot calculated after the target grade changed' do + travel_to Time.zone.parse('2026-08-10 12:00:00 UTC') do + @project.update!(target_grade: 2) + + travel 1.minute + + create_snapshot( + target_grade: 2, + submitted_percentage: 61, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE, + calculated_at: Time.zone.now + ) + + request_as(@student) + + assert_equal 200, last_response.status + assert_equal 60.0, last_response_body['submitted_percentage'] + end + end + + private + + def endpoint(project: @project, task_definition: @task_definition) + "/api/projects/#{project.id}/task_def_id/" \ + "#{task_definition.id}/peer_progress" + end + + def request_as(user, path = endpoint) + clear_auth_header + add_auth_header_for(user: user) + get path + end + + def create_snapshot( + submitted_percentage:, + cohort_size:, + calculated_at: Time.zone.now, + target_grade: @project.target_grade + ) + create( + :peer_progress_snapshot, + unit: @unit, + task_definition: @task_definition, + target_grade: target_grade, + submitted_percentage: submitted_percentage, + cohort_size: cohort_size, + calculated_at: calculated_at + ) + end + + def quantised_count_groups(cohort_size) + bucket_size = PeerProgressApi::PERCENTAGE_BUCKET_SIZE + + (0..cohort_size).group_by do |submitted_count| + exact_percentage = ((submitted_count * 100.0) / cohort_size).round(2) + ((exact_percentage / bucket_size).round * bucket_size).to_f + end + end + + def assert_peer_progress_not_found + assert_equal 404, last_response.status + + body = last_response_body + + assert_json_limit_keys_to_exactly %w[error], body + + assert_equal( + PeerProgressApi::NOT_FOUND_MESSAGE, + body['error'] + ) + + assert_private_no_store + end + + def restore_env(name, value) + if value.nil? + ENV.delete(name) + else + ENV[name] = value + end + end + + def assert_private_no_store + cache_control = last_response.headers.fetch('Cache-Control', '') + + assert_includes cache_control, 'private' + assert_includes cache_control, 'no-store' + end + + def assert_peer_progress_response_contract(body) + assert_json_limit_keys_to_exactly RESPONSE_KEYS, body + + assert_kind_of Integer, body['task_definition_id'] + assert_kind_of Integer, body['unit_id'] + + assert( + body['target_grade'].nil? || + body['target_grade'].is_a?(Integer), + 'target_grade must be an integer or null' + ) + + assert( + body['submitted_percentage'].nil? || + body['submitted_percentage'].is_a?(Numeric), + 'submitted_percentage must be numeric or null' + ) + + unless body['submitted_percentage'].nil? + assert_operator body['submitted_percentage'], :>=, 0.0 + assert_operator body['submitted_percentage'], :<=, 100.0 + end + + %w[ + is_suppressed + is_stale + is_feature_enabled + ].each do |key| + assert_includes( + [true, false], + body.fetch(key), + "#{key} must be a boolean" + ) + end + + unless body['last_updated_at'].nil? + parsed_timestamp = nil + + assert_nothing_raised do + parsed_timestamp = Time.iso8601(body['last_updated_at']) + end + + assert_equal( + 0, + parsed_timestamp.utc_offset, + 'last_updated_at must use UTC' + ) + end + + assert_kind_of String, body['unavailable_message'] + assert_empty FORBIDDEN_KEYS & body.keys + assert_private_no_store + end +end diff --git a/test/api/units_api_test.rb b/test/api/units_api_test.rb index 23add5b13e..5a22933930 100644 --- a/test/api/units_api_test.rb +++ b/test/api/units_api_test.rb @@ -488,6 +488,74 @@ def test_put_update_unit_invalid_id assert_equal 404, last_response.status end + def test_main_convenor_can_enable_peer_progress + unit = FactoryBot.create( + :unit, + with_students: false, + task_count: 0 + ) + + add_auth_header_for(user: unit.main_convenor_user) + + put_json( + "/api/units/#{unit.id}", + { + unit: { + peer_progress_enabled: true + } + } + ) + + assert_equal 200, last_response.status, last_response_body + assert unit.reload.peer_progress_enabled? + assert_equal true, last_response_body['peer_progress_enabled'] + end + + def test_student_cannot_enable_peer_progress + unit = FactoryBot.create( + :unit, + with_students: false, + task_count: 0, + tutorials: 1 + ) + + student = FactoryBot.create(:user, :student) + unit.enrol_student( + student, + unit.tutorials.first.campus + ) + + add_auth_header_for(user: student) + + put_json( + "/api/units/#{unit.id}", + { + unit: { + peer_progress_enabled: true + } + } + ) + + assert_equal 403, last_response.status + assert_not unit.reload.peer_progress_enabled? + end + + def test_unit_details_expose_peer_progress_setting_to_the_convenor + unit = FactoryBot.create( + :unit, + with_students: false, + task_count: 0, + peer_progress_enabled: true + ) + + add_auth_header_for(user: unit.main_convenor_user) + + get "/api/units/#{unit.id}" + + assert_equal 200, last_response.status + assert_equal true, last_response_body['peer_progress_enabled'] + end + # Test can update unit start and end dates def test_put_update_unit_dates # Add username and auth_token to Header diff --git a/test/factories/peer_progress_snapshot_factory.rb b/test/factories/peer_progress_snapshot_factory.rb new file mode 100644 index 0000000000..809fd565de --- /dev/null +++ b/test/factories/peer_progress_snapshot_factory.rb @@ -0,0 +1,26 @@ +FactoryBot.define do + factory :peer_progress_snapshot do + unit do + create( + :unit, + with_students: false, + task_count: 0, + stream_count: 0 + ) + end + + task_definition do + create( + :task_definition, + unit: unit, + target_grade: 0, + outcome_count: 0 + ) + end + + target_grade { task_definition.target_grade } + submitted_percentage { 50.0 } + cohort_size { 10 } + calculated_at { Time.current } + end +end diff --git a/test/models/peer_progress_snapshot_test.rb b/test/models/peer_progress_snapshot_test.rb new file mode 100644 index 0000000000..85e29b0e2f --- /dev/null +++ b/test/models/peer_progress_snapshot_test.rb @@ -0,0 +1,226 @@ +require 'test_helper' + +class PeerProgressSnapshotTest < ActiveSupport::TestCase + setup do + @unit = create( + :unit, + with_students: false, + task_count: 0, + stream_count: 0 + ) + + @task_definition = create( + :task_definition, + unit: @unit, + target_grade: 0, + outcome_count: 0 + ) + end + + test 'is valid with the required aggregate fields' do + assert build_snapshot.valid? + end + + test 'belongs to its unit and task definition' do + snapshot = create( + :peer_progress_snapshot, + unit: @unit, + task_definition: @task_definition + ) + + assert_equal @unit, snapshot.unit + assert_equal @task_definition, snapshot.task_definition + end + + test 'requires a calculation timestamp' do + snapshot = build_snapshot(calculated_at: nil) + + assert_not snapshot.valid? + + assert_includes( + snapshot.errors[:calculated_at], + "can't be blank" + ) + end + + test 'accepts a genuine zero percentage for a non-empty cohort' do + snapshot = build_snapshot( + submitted_percentage: 0, + cohort_size: 10 + ) + + assert snapshot.valid? + end + + test 'accepts a nil percentage for unavailable or suppressed data' do + suppressed = build_snapshot( + submitted_percentage: nil, + cohort_size: 3 + ) + + unavailable = build_snapshot( + submitted_percentage: nil, + cohort_size: 0 + ) + + assert suppressed.valid? + assert unavailable.valid? + end + + test 'rejects percentages outside zero to one hundred' do + below_zero = build_snapshot( + submitted_percentage: -0.01 + ) + + above_one_hundred = build_snapshot( + submitted_percentage: 100.01 + ) + + assert_not below_zero.valid? + assert_not above_one_hundred.valid? + end + + test 'requires a non-negative integer cohort size' do + negative = build_snapshot(cohort_size: -1) + decimal = build_snapshot(cohort_size: 2.5) + + assert_not negative.valid? + assert_not decimal.valid? + end + + test 'does not allow a percentage when cohort size is zero' do + snapshot = build_snapshot( + submitted_percentage: 0, + cohort_size: 0 + ) + + assert_not snapshot.valid? + + assert_includes( + snapshot.errors[:submitted_percentage], + 'must be blank when cohort size is zero' + ) + end + + test 'requires the task definition to belong to the same unit' do + other_unit = create( + :unit, + with_students: false, + task_count: 0, + stream_count: 0 + ) + + snapshot = build_snapshot(unit: other_unit) + + assert_not snapshot.valid? + + assert_includes( + snapshot.errors[:task_definition], + 'must belong to the same unit' + ) + end + + test 'requires a target grade enabled for the unit' do + snapshot = build_snapshot(target_grade: 99) + + assert_not snapshot.valid? + + assert_includes( + snapshot.errors[:target_grade], + 'must be enabled for the unit' + ) + end + + test 'requires the cohort grade to cover the task target grade' do + higher_grade_task = create( + :task_definition, + unit: @unit, + target_grade: 2, + outcome_count: 0 + ) + + snapshot = build_snapshot( + task_definition: higher_grade_task, + target_grade: 1 + ) + + assert_not snapshot.valid? + + assert_includes( + snapshot.errors[:target_grade], + 'must be at least the task definition target grade' + ) + end + + test 'enforces one snapshot per unit task and target grade' do + create( + :peer_progress_snapshot, + unit: @unit, + task_definition: @task_definition, + target_grade: 0 + ) + + duplicate = build_snapshot(target_grade: 0) + + assert_not duplicate.valid? + + assert_includes( + duplicate.errors[:target_grade], + 'has already been taken' + ) + end + + test 'allows another target grade for the same unit and task' do + create( + :peer_progress_snapshot, + unit: @unit, + task_definition: @task_definition, + target_grade: 0 + ) + + second_grade = build_snapshot(target_grade: 1) + + assert second_grade.valid?, + second_grade.errors.full_messages.to_sentence + end + + test 'database index rejects duplicate aggregate keys' do + snapshot = create( + :peer_progress_snapshot, + unit: @unit, + task_definition: @task_definition, + target_grade: 0 + ) + + duplicate = snapshot.dup + + assert_raises ActiveRecord::RecordNotUnique do + duplicate.save!(validate: false) + end + end + + test 'destroying a task definition destroys its snapshots' do + snapshot = create( + :peer_progress_snapshot, + unit: @unit, + task_definition: @task_definition + ) + + snapshot_id = snapshot.id + + @task_definition.destroy! + + assert_not PeerProgressSnapshot.exists?(snapshot_id) + end + + private + + def build_snapshot(**overrides) + build( + :peer_progress_snapshot, + unit: @unit, + task_definition: @task_definition, + **overrides + ) + end +end diff --git a/test/services/peer_progress_aggregation_service_test.rb b/test/services/peer_progress_aggregation_service_test.rb new file mode 100644 index 0000000000..83e7364c25 --- /dev/null +++ b/test/services/peer_progress_aggregation_service_test.rb @@ -0,0 +1,452 @@ +# frozen_string_literal: true + +require 'test_helper' + +class PeerProgressAggregationServiceTest < ActiveSupport::TestCase + def setup + @unit = create( + :unit, + with_students: false, + task_count: 0, + stream_count: 0, + tutorials: 0, + staff_count: 0, + outcome_count: 0 + ) + + @pass_task = create( + :task_definition, + unit: @unit, + target_grade: 0, + outcome_count: 0 + ) + + @credit_task = create( + :task_definition, + unit: @unit, + target_grade: 1, + outcome_count: 0 + ) + + @calculated_at = Time.zone.parse('2026-08-10 10:00:00') + end + + def test_calculates_percentage_for_enrolled_projects_in_the_same_target_grade + projects = create_list( + :project, + 4, + unit: @unit, + target_grade: 0, + enrolled: true + ) + + projects.first(3).each do |project| + create_submitted_task( + project: project, + task_definition: @pass_task + ) + end + + other_grade = create( + :project, + unit: @unit, + target_grade: 1, + enrolled: true + ) + + create_submitted_task( + project: other_grade, + task_definition: @pass_task + ) + + withdrawn = create( + :project, + unit: @unit, + target_grade: 0, + enrolled: false + ) + + create_submitted_task( + project: withdrawn, + task_definition: @pass_task + ) + + run_service + + snapshot = find_snapshot( + task_definition: @pass_task, + target_grade: 0 + ) + + assert_equal 4, snapshot.cohort_size + assert_equal 75.0, snapshot.submitted_percentage.to_f + assert_equal @calculated_at, snapshot.calculated_at + end + + def test_returns_a_genuine_zero_when_the_cohort_exists_but_nobody_has_submitted + create_list( + :project, + 4, + unit: @unit, + target_grade: 0, + enrolled: true + ) + + run_service + + snapshot = find_snapshot( + task_definition: @pass_task, + target_grade: 0 + ) + + assert_equal 4, snapshot.cohort_size + assert_equal 0.0, snapshot.submitted_percentage.to_f + end + + def test_returns_nil_percentage_when_the_cohort_is_empty + run_service + + snapshot = find_snapshot( + task_definition: @pass_task, + target_grade: 3 + ) + + assert_equal 0, snapshot.cohort_size + assert_nil snapshot.submitted_percentage + end + + def test_only_creates_snapshots_for_tasks_applicable_to_the_target_grade + create( + :project, + unit: @unit, + target_grade: 0, + enrolled: true + ) + + create( + :project, + unit: @unit, + target_grade: 1, + enrolled: true + ) + + run_service + + assert PeerProgressSnapshot.exists?( + unit: @unit, + task_definition: @pass_task, + target_grade: 0 + ) + + assert_not PeerProgressSnapshot.exists?( + unit: @unit, + task_definition: @credit_task, + target_grade: 0 + ) + + assert PeerProgressSnapshot.exists?( + unit: @unit, + task_definition: @pass_task, + target_grade: 1 + ) + + assert PeerProgressSnapshot.exists?( + unit: @unit, + task_definition: @credit_task, + target_grade: 1 + ) + end + + def test_counts_uploads_regardless_of_the_current_task_status + statuses = [ + TaskStatus.ready_for_feedback, + TaskStatus.complete, + TaskStatus.redo, + TaskStatus.fix_and_resubmit + ] + + projects = create_list( + :project, + statuses.length, + unit: @unit, + target_grade: 0, + enrolled: true + ) + + projects.zip(statuses).each do |project, status| + create_submitted_task( + project: project, + task_definition: @pass_task, + task_status: status + ) + end + + run_service + + snapshot = find_snapshot( + task_definition: @pass_task, + target_grade: 0 + ) + + assert_equal statuses.length, snapshot.cohort_size + assert_equal 100.0, snapshot.submitted_percentage.to_f + end + + def test_does_not_mix_projects_or_submissions_from_another_unit + local_projects = create_list( + :project, + 2, + unit: @unit, + target_grade: 0, + enrolled: true + ) + + create_submitted_task( + project: local_projects.first, + task_definition: @pass_task + ) + + other_unit = create( + :unit, + with_students: false, + task_count: 0, + stream_count: 0, + tutorials: 0, + staff_count: 0, + outcome_count: 0 + ) + + other_task = create( + :task_definition, + unit: other_unit, + target_grade: 0, + outcome_count: 0 + ) + + other_projects = create_list( + :project, + 4, + unit: other_unit, + target_grade: 0, + enrolled: true + ) + + other_projects.each do |project| + create_submitted_task( + project: project, + task_definition: other_task + ) + end + + run_service + + snapshot = find_snapshot( + task_definition: @pass_task, + target_grade: 0 + ) + + assert_equal 2, snapshot.cohort_size + assert_equal 50.0, snapshot.submitted_percentage.to_f + assert_not PeerProgressSnapshot.exists?(unit: other_unit) + end + + def test_does_not_create_missing_task_rows + create_list( + :project, + 2, + unit: @unit, + target_grade: 0, + enrolled: true + ) + + assert_no_difference('Task.count') do + run_service + end + end + + def test_updates_existing_snapshots_without_creating_duplicates + projects = create_list( + :project, + 2, + unit: @unit, + target_grade: 0, + enrolled: true + ) + + run_service + snapshot_count = PeerProgressSnapshot.count + + create_submitted_task( + project: projects.first, + task_definition: @pass_task + ) + + PeerProgressAggregationService.call( + unit: @unit, + calculated_at: @calculated_at + 1.hour + ) + + snapshot = find_snapshot( + task_definition: @pass_task, + target_grade: 0 + ) + + assert_equal snapshot_count, PeerProgressSnapshot.count + assert_equal 50.0, snapshot.submitted_percentage.to_f + assert_equal @calculated_at + 1.hour, snapshot.calculated_at + end + + def test_rounds_percentages_to_two_decimal_places + projects = create_list( + :project, + 3, + unit: @unit, + target_grade: 0, + enrolled: true + ) + + create_submitted_task( + project: projects.first, + task_definition: @pass_task + ) + + run_service + + snapshot = find_snapshot( + task_definition: @pass_task, + target_grade: 0 + ) + + assert_equal 33.33, snapshot.submitted_percentage.to_f + end + + def test_does_not_count_staff_assessment_without_a_student_upload + project = create( + :project, + unit: @unit, + target_grade: 0, + enrolled: true + ) + + create( + :task, + project: project, + task_definition: @pass_task, + task_status: TaskStatus.complete, + file_uploaded_at: nil, + submission_date: @calculated_at - 1.hour, + assessment_date: @calculated_at - 1.hour + ) + + run_service + + snapshot = find_snapshot( + task_definition: @pass_task, + target_grade: 0 + ) + + assert_equal 1, snapshot.cohort_size + assert_equal 0.0, snapshot.submitted_percentage.to_f + end + + def test_counts_a_group_upload_for_each_participating_project + group_unit = create( + :unit, + with_students: true, + student_count: 2, + unenrolled_student_count: 0, + part_enrolled_student_count: 0, + inactive_student_count: 0, + task_count: 0, + tutorials: 1, + group_sets: 1, + groups: [{ gs: 0, students: 2 }], + outcome_count: 0 + ) + + group_task = create( + :task_definition, + unit: group_unit, + group_set: group_unit.group_sets.first, + target_grade: 0, + upload_requirements: [], + start_date: 1.day.ago, + outcome_count: 0 + ) + + projects = group_unit.groups.first.projects.to_a + projects.each { |project| project.update!(target_grade: 0) } + + submitting_task = + projects.first.task_for_task_definition(group_task) + + contributions = projects.map do |project| + { + project_id: project.id, + pct: 100 / projects.length, + pts: 3 + } + end + + submitting_task.create_submission_and_trigger_state_change( + submitting_task.student, + true, + contributions, + 'ready_for_feedback' + ) + + PeerProgressAggregationService.call( + unit: group_unit, + calculated_at: @calculated_at + ) + + snapshot = PeerProgressSnapshot.find_by!( + unit: group_unit, + task_definition: group_task, + target_grade: 0 + ) + + assert_equal 2, snapshot.cohort_size + assert_equal 100.0, snapshot.submitted_percentage.to_f + + projects.each do |project| + task = project.tasks.find_by!( + task_definition: group_task + ) + + assert task.file_uploaded_at.present? + end + end + + def run_service + PeerProgressAggregationService.call( + unit: @unit, + calculated_at: @calculated_at + ) + end + + def find_snapshot(task_definition:, target_grade:) + PeerProgressSnapshot.find_by!( + unit: @unit, + task_definition: task_definition, + target_grade: target_grade + ) + end + + def create_submitted_task( + project:, + task_definition:, + task_status: TaskStatus.ready_for_feedback + ) + uploaded_at = @calculated_at - 1.hour + + create( + :task, + project: project, + task_definition: task_definition, + task_status: task_status, + file_uploaded_at: uploaded_at, + submission_date: uploaded_at + ) + end +end diff --git a/test/sidekiq/aggregate_peer_progress_job_test.rb b/test/sidekiq/aggregate_peer_progress_job_test.rb new file mode 100644 index 0000000000..07dac1945e --- /dev/null +++ b/test/sidekiq/aggregate_peer_progress_job_test.rb @@ -0,0 +1,240 @@ +# frozen_string_literal: true + +require 'test_helper' +require 'minitest/mock' + +class AggregatePeerProgressJobTest < ActiveSupport::TestCase + def setup + @active_unit = create_minimal_unit(active: true) + @inactive_unit = create_minimal_unit(active: false) + @disabled_unit = create_minimal_unit( + active: true, + peer_progress_enabled: false + ) + @calculated_at = Time.zone.parse('2026-08-10 23:45:00') + end + + def test_aggregates_the_requested_active_unit + calls = [] + + travel_to @calculated_at do + PeerProgressAggregationService.stub( + :call, + lambda do |unit:, calculated_at:| + calls << { + unit: unit, + calculated_at: calculated_at + } + [] + end + ) do + AggregatePeerProgressJob.new.perform(@active_unit.id) + end + end + + assert_equal 1, calls.length + assert_equal @active_unit, calls.first[:unit] + assert_equal @calculated_at, calls.first[:calculated_at] + end + + def test_enqueues_one_job_for_each_enabled_active_unit_when_no_unit_id_is_given + Sidekiq::Job.clear_all + + expected_unit_ids = + Unit.active_units + .where(peer_progress_enabled: true) + .order(:id) + .pluck(:id) + + assert_difference( + -> { AggregatePeerProgressJob.jobs.size }, + expected_unit_ids.length + ) do + AggregatePeerProgressJob.new.perform + end + + actual_unit_ids = + AggregatePeerProgressJob.jobs + .last(expected_unit_ids.length) + .map { |job| job['args'].first } + .sort + + assert_equal expected_unit_ids, actual_unit_ids + assert_not_includes actual_unit_ids, @inactive_unit.id + assert_not_includes actual_unit_ids, @disabled_unit.id + end + + def test_failure_for_one_unit_does_not_prevent_another_unit_job + other_unit = create_minimal_unit(active: true) + successful_unit_ids = [] + + PeerProgressAggregationService.stub( + :call, + lambda do |unit:, **_kwargs| + if unit.id == @active_unit.id + raise StandardError, 'first unit failed' + end + + successful_unit_ids << unit.id + [] + end + ) do + assert_raises(StandardError) do + AggregatePeerProgressJob.new.perform(@active_unit.id) + end + + AggregatePeerProgressJob.new.perform(other_unit.id) + end + + assert_equal [other_unit.id], successful_unit_ids + end + + def test_skips_a_requested_inactive_unit + calls = [] + + PeerProgressAggregationService.stub( + :call, + lambda do |unit:, calculated_at:| + calls << [unit, calculated_at] + [] + end + ) do + AggregatePeerProgressJob.new.perform(@inactive_unit.id) + end + + assert_empty calls + end + + def test_skips_a_requested_unit_with_peer_progress_disabled + calls = [] + + PeerProgressAggregationService.stub( + :call, + lambda do |unit:, calculated_at:| + calls << [unit, calculated_at] + [] + end + ) do + AggregatePeerProgressJob.new.perform(@disabled_unit.id) + end + + assert_empty calls + end + + def test_sanitizes_the_error_when_requested_unit_does_not_exist + missing_unit_id = Unit.maximum(:id).to_i + 10_000 + + error = assert_raises(AggregatePeerProgressJob::AggregationError) do + AggregatePeerProgressJob.new.perform(missing_unit_id) + end + + assert_equal( + "Peer progress aggregation failed for unit_id=#{missing_unit_id}: " \ + 'ActiveRecord::RecordNotFound', + error.message + ) + assert_nil error.cause + end + + def test_sanitizes_aggregation_errors_before_sidekiq_handles_them + sensitive_message = + 'peer_username=private-peer name=Private Student ' \ + 'email=private-peer@example.invalid student_id=987654321' + start_log = + "Starting peer progress aggregation for unit_id=#{@active_unit.id}..." + failure_message = + "Peer progress aggregation failed for unit_id=#{@active_unit.id}: " \ + 'StandardError' + logger = Minitest::Mock.new + logger.expect(:info, nil, [start_log]) + logger.expect(:error, nil, [failure_message]) + job = AggregatePeerProgressJob.new + + PeerProgressAggregationService.stub( + :call, + lambda do |**_kwargs| + raise StandardError, sensitive_message + end + ) do + error = assert_raises(AggregatePeerProgressJob::AggregationError) do + job.stub(:logger, logger) do + job.perform(@active_unit.id) + end + end + + assert_equal failure_message, error.message + assert_nil error.cause + assert_not_includes error.message, sensitive_message + assert_not_includes error.full_message, sensitive_message + end + + assert_mock logger + assert_equal 3, AggregatePeerProgressJob.get_sidekiq_options['retry'] + end + + def test_enqueues_only_the_unit_id + assert_difference -> { AggregatePeerProgressJob.jobs.size }, 1 do + AggregatePeerProgressJob.perform_async(@active_unit.id) + end + + queued_job = AggregatePeerProgressJob.jobs.last + + assert_equal [@active_unit.id], queued_job['args'] + end + + def test_creates_a_snapshot_through_the_real_aggregation_service + task_definition = create( + :task_definition, + unit: @active_unit, + target_grade: 0, + outcome_count: 0 + ) + + projects = create_list( + :project, + 2, + unit: @active_unit, + target_grade: 0, + enrolled: true + ) + + create( + :task, + project: projects.first, + task_definition: task_definition, + task_status: TaskStatus.ready_for_feedback, + file_uploaded_at: @calculated_at - 1.hour, + submission_date: @calculated_at - 1.hour + ) + + travel_to @calculated_at do + AggregatePeerProgressJob.new.perform(@active_unit.id) + end + + snapshot = PeerProgressSnapshot.find_by!( + unit: @active_unit, + task_definition: task_definition, + target_grade: 0 + ) + + assert_equal 2, snapshot.cohort_size + assert_equal 50.0, snapshot.submitted_percentage.to_f + assert_equal @calculated_at, snapshot.calculated_at + end + + private + + def create_minimal_unit(active:, peer_progress_enabled: true) + create( + :unit, + active: active, + peer_progress_enabled: peer_progress_enabled, + with_students: false, + task_count: 0, + stream_count: 0, + tutorials: 0, + staff_count: 0, + outcome_count: 0 + ) + end +end diff --git a/test/sidekiq/scheduled_job_test.rb b/test/sidekiq/scheduled_job_test.rb index e21285fdf3..f921bff57b 100644 --- a/test/sidekiq/scheduled_job_test.rb +++ b/test/sidekiq/scheduled_job_test.rb @@ -1,21 +1,36 @@ # frozen_string_literal: true require 'test_helper' -class TiiCheckProgressJobTest < ActiveSupport::TestCase +require 'sidekiq_unique_jobs/testing' +class TiiCheckProgressJobTest < ActiveSupport::TestCase def test_jobs_are_scheduled + # Clear fake jobs and any unique-job locks left by an earlier test run. + Sidekiq::Job.clear_all Sidekiq::Cron::Job.destroy_all! - Sidekiq::Cron::Job.load_from_hash!(YAML.load_file(Rails.root.join('config/schedule.yml'))) - assert_equal 6, Sidekiq::Cron::Job.all.count, Sidekiq::Cron::Job.all.map(&:name) - Sidekiq::Cron::Job.all.each(&:enqueue!) + Sidekiq::Cron::Job.load_from_hash!( + YAML.load_file(Rails.root.join('config/schedule.yml')) + ) + + jobs = Sidekiq::Cron::Job.all + peer_progress_job = + jobs.find { |job| job.name == 'aggregate_peer_progress' } + + assert_equal 7, jobs.count, jobs.map(&:name) + assert_not_nil peer_progress_job + assert_equal 'AggregatePeerProgressJob', peer_progress_job.klass + + # Sidekiq::Cron::Job.all returns an Array, not an ActiveRecord relation. + jobs.each(&:enqueue!) + assert_equal 1, TiiRegisterWebHookJob.jobs.count assert_equal 1, TiiCheckProgressJob.jobs.count assert_equal 1, ClearAccessTokensJob.jobs.count assert_equal 1, RefreshModerationFeedbackTimestampsJob.jobs.count + assert_equal 1, AggregatePeerProgressJob.jobs.count assert_equal 1, AggregateTaskCompletionStatsJob.jobs.count assert_equal 1, PollCommunicationSetSchedulesJob.jobs.count # assert_equal 1, ArchiveOldUnitsJob.jobs.count end - end