From 4c7489b17611e5f598c3d9e1ee27bc2c1cc41ef5 Mon Sep 17 00:00:00 2001 From: Clupai8o0 Date: Sun, 27 Sep 2026 19:58:34 +1000 Subject: [PATCH 1/2] feat(ppi): peer progress and cross-unit dashboard Brings the T2 2026 ppi-cpd work from ontrack-features-t2-2026 11.0.x (reviewed and merged work) onto thoth-tech 11.0.x. Co-authored-by: maplefoxgit Co-authored-by: Tan Tai Co-authored-by: JOSHUA ERICKSON Co-authored-by: Gaurav Myana --- .../entities/minimal/minimal_unit_entity.rb | 5 + app/api/entities/unit_entity.rb | 5 + app/api/peer_progress_api.rb | 295 ++++ app/api/projects_api.rb | 79 +- app/api/task_prioritization_api.rb | 39 + app/helpers/collection_pagination_helpers.rb | 23 + app/models/peer_progress_snapshot.rb | 138 ++ .../peer_progress_aggregation_service.rb | 147 ++ .../peer_progress_distribution_policy.rb | 151 +++ app/services/peer_progress_viewer_policy.rb | 90 ++ app/services/task_prioritization_service.rb | 202 +++ app/sidekiq/aggregate_peer_progress_job.rb | 86 ++ ...09153000_create_peer_progress_snapshots.rb | 32 + ...3824_add_peer_progress_enabled_to_units.rb | 11 + ...add_target_grade_changed_at_to_projects.rb | 28 + ..._ensure_target_grade_changed_at_default.rb | 30 + ...260824000003_add_detailed_peer_progress.rb | 19 + docs/collection-pagination.md | 37 + docs/dashboard-feedback-state.md | 59 + docs/peer-progress-api.md | 217 +++ docs/peer-progress/data-source-map.md | 178 +++ .../task-completion-data-discovery.md | 62 + test/api/collection_pagination_test.rb | 157 +++ test/api/peer_progress_api_test.rb | 1185 +++++++++++++++++ test/api/projects_api_test.rb | 278 +++- test/api/task_prioritization_api_test.rb | 484 +++++++ test/api/units_api_test.rb | 68 + .../peer_progress_snapshot_factory.rb | 26 + test/models/peer_progress_snapshot_test.rb | 300 +++++ .../project_target_grade_changed_at_test.rb | 61 + .../peer_progress_aggregation_service_test.rb | 538 ++++++++ .../peer_progress_distribution_policy_test.rb | 93 ++ .../peer_progress_viewer_policy_test.rb | 187 +++ .../aggregate_peer_progress_job_test.rb | 240 ++++ 34 files changed, 5539 insertions(+), 11 deletions(-) create mode 100644 app/api/peer_progress_api.rb create mode 100644 app/api/task_prioritization_api.rb create mode 100644 app/helpers/collection_pagination_helpers.rb create mode 100644 app/models/peer_progress_snapshot.rb create mode 100644 app/services/peer_progress_aggregation_service.rb create mode 100644 app/services/peer_progress_distribution_policy.rb create mode 100644 app/services/peer_progress_viewer_policy.rb create mode 100644 app/services/task_prioritization_service.rb create mode 100644 app/sidekiq/aggregate_peer_progress_job.rb create mode 100644 db/migrate/20260809153000_create_peer_progress_snapshots.rb create mode 100644 db/migrate/20260810033824_add_peer_progress_enabled_to_units.rb create mode 100644 db/migrate/20260818160804_add_target_grade_changed_at_to_projects.rb create mode 100644 db/migrate/20260824000002_ensure_target_grade_changed_at_default.rb create mode 100644 db/migrate/20260824000003_add_detailed_peer_progress.rb create mode 100644 docs/collection-pagination.md create mode 100644 docs/dashboard-feedback-state.md create mode 100644 docs/peer-progress-api.md create mode 100644 docs/peer-progress/data-source-map.md create mode 100644 docs/peer-progress/task-completion-data-discovery.md create mode 100644 test/api/collection_pagination_test.rb create mode 100644 test/api/peer_progress_api_test.rb create mode 100644 test/api/task_prioritization_api_test.rb create mode 100644 test/factories/peer_progress_snapshot_factory.rb create mode 100644 test/models/peer_progress_snapshot_test.rb create mode 100644 test/models/project_target_grade_changed_at_test.rb create mode 100644 test/services/peer_progress_aggregation_service_test.rb create mode 100644 test/services/peer_progress_distribution_policy_test.rb create mode 100644 test/services/peer_progress_viewer_policy_test.rb create mode 100644 test/sidekiq/aggregate_peer_progress_job_test.rb diff --git a/app/api/entities/minimal/minimal_unit_entity.rb b/app/api/entities/minimal/minimal_unit_entity.rb index 06f9659f2a..44cc958c92 100644 --- a/app/api/entities/minimal/minimal_unit_entity.rb +++ b/app/api/entities/minimal/minimal_unit_entity.rb @@ -20,6 +20,11 @@ class MinimalUnitEntity < Grape::Entity end expose :active + expose :allow_flexible_dates + expose :ordered_task_definitions, + as: :task_definitions, + using: Entities::TaskDefinitionEntity, + if: :include_task_definitions expose :grade_values expose :grade_definitions 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/peer_progress_api.rb b/app/api/peer_progress_api.rb new file mode 100644 index 0000000000..c3787e7423 --- /dev/null +++ b/app/api/peer_progress_api.rb @@ -0,0 +1,295 @@ +# 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 peer-only + # cohort. At 20 remaining peers, one peer is exactly five percentage points + # and zero becomes a singleton bucket. A floor of 21 remaining peers makes + # one peer's share smaller than the boundary, so every returned bucket + # represents at least two possible peer 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 = + PeerProgressDistributionPolicy::PERCENTAGE_BUCKET_SIZE + + 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 safe_target_grade(project) + target_grade = project.target_grade + + target_grade if target_grade.present? && + project.unit.grade_value?(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:, **overrides) + state = { + snapshot: nil, + submitted_percentage: nil, + completed_percentage: nil, + status_distribution: nil, + distribution_unavailable_reason: nil, + is_suppressed: false, + is_stale: false, + is_feature_enabled: true, + is_user_enabled: current_user.display_peer_progress?, + unavailable_reason: nil, + unavailable_message: '' + } + overrides.assert_valid_keys(*state.keys) + state.merge!(overrides) + + distribution_available = state[:status_distribution].present? + if !distribution_available && + state[:distribution_unavailable_reason].nil? + state[:distribution_unavailable_reason] = state[:unavailable_reason] + end + + { + task_definition_id: task_definition.id, + unit_id: project.unit_id, + target_grade: safe_target_grade(project), + submitted_percentage: state[:submitted_percentage], + completed_percentage: state[:completed_percentage], + status_distribution: state[:status_distribution], + distribution_available: distribution_available, + distribution_unavailable_reason: + state[:distribution_unavailable_reason], + is_suppressed: state[:is_suppressed], + is_stale: state[:is_stale], + is_feature_enabled: state[:is_feature_enabled], + is_user_enabled: state[:is_user_enabled], + last_updated_at: state[:snapshot]&.calculated_at&.utc&.iso8601, + unavailable_reason: state[:unavailable_reason], + unavailable_message: state[:unavailable_message] + } + end + + def peer_progress_result(project:, task_definition:) + unit = project.unit + + unless current_user.display_peer_progress? + return peer_progress_payload( + project: project, + task_definition: task_definition, + is_user_enabled: false, + unavailable_reason: 'user_disabled', + unavailable_message: PeerProgressApi::UNAVAILABLE_MESSAGE + ) + end + + unless unit.peer_progress_enabled? + return peer_progress_payload( + project: project, + task_definition: task_definition, + is_feature_enabled: false, + unavailable_reason: 'feature_disabled', + 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_reason: 'target_grade_unavailable', + 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? || + (project.target_grade_changed_at.present? && + snapshot.calculated_at < project.target_grade_changed_at) + return peer_progress_payload( + project: project, + task_definition: task_definition, + unavailable_reason: 'snapshot_unavailable', + unavailable_message: PeerProgressApi::UNAVAILABLE_MESSAGE + ) + end + + viewer_task = effective_task( + project: project, + task_definition: task_definition + ) + unless PeerProgressViewerPolicy.viewer_context_current?( + snapshot: snapshot, + viewer_project: project, + viewer_task: viewer_task + ) + return peer_progress_payload( + project: project, + task_definition: task_definition, + snapshot: snapshot, + unavailable_reason: 'snapshot_unavailable', + 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. + peer_cohort_size = [snapshot.cohort_size - 1, 0].max + if peer_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_reason: 'insufficient_cohort', + unavailable_message: PeerProgressApi::UNAVAILABLE_MESSAGE + ) + end + + peer_progress = PeerProgressViewerPolicy.build( + snapshot: snapshot, + viewer_project: project, + viewer_task: viewer_task + ) + if peer_progress.nil? + return peer_progress_payload( + project: project, + task_definition: task_definition, + snapshot: snapshot, + is_stale: is_stale, + unavailable_reason: 'aggregation_incomplete', + 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_reason: 'stale', + unavailable_message: PeerProgressApi::UNAVAILABLE_MESSAGE + ) + end + + peer_progress_payload( + project: project, + task_definition: task_definition, + snapshot: snapshot, + **PeerProgressViewerPolicy.public_metrics(peer_progress) + ) + 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..a309fb7d99 100644 --- a/app/api/projects_api.rb +++ b/app/api/projects_api.rb @@ -1,23 +1,75 @@ require 'grape' class ProjectsApi < Grape::API + helpers CollectionPaginationHelpers + TASK_DEFINITION_PRELOADS = [ + :discussion_prompts, + :grade_due_dates, + { learning_outcomes: :linked_outcomes }, + :overseer_steps, + :tutorial_stream + ].freeze + helpers AuthenticationHelpers helpers AuthorisationHelpers helpers DbHelpers + helpers do + def notify_portfolio_received(project) + timezone = project.campus&.timezone.presence || Time.zone.name + received_at = project.portfolio_submission_date.in_time_zone(timezone) + submitted_at = received_at.strftime('%-d %B %Y at %-I:%M %p %Z (UTC%:z)') + product_name = Doubtfire::Application.config.institution[:product_name] + + NotificationService.notify( + user: project.student, + type: 'portfolio', + event: 'portfolio_received', + message: "#{product_name} received your portfolio submission at #{submitted_at}.", + link: "/projects/#{project.id}/dashboard" + ) + rescue StandardError => e + Rails.logger.error( + "Failed to raise portfolio_received notification for project #{project.id}: #{e.message}" + ) + end + end + before do authenticated? end desc "Fetches all of the current user's projects" params do + optional :page, type: Integer, values: 1..CollectionPaginationHelpers::MAX_PAGE, allow_blank: false + optional :per_page, type: Integer, values: 1..CollectionPaginationHelpers::MAX_PER_PAGE, allow_blank: false optional :include_inactive, type: Boolean, desc: 'Include projects for units that are no longer active?' + optional :include_task_definitions, type: Boolean, desc: 'Include all task definitions with tasks for each project?' end get '/projects' do include_inactive = params[:include_inactive] || false + include_task_definitions = params[:include_task_definitions] || false projects = Project.eager_load(:unit, :user).for_user current_user, include_inactive - present projects, with: Entities::ProjectEntity, for_student: true, summary_only: true, user: current_user + if include_task_definitions + projects = projects.preload(unit: { task_definitions: TASK_DEFINITION_PRELOADS }) + end + + projects = paginate_collection(projects) + + present projects, with: Entities::ProjectEntity, for_student: true, summary_only: true, include_task_definitions: include_task_definitions, user: current_user + Rails.logger.info({ event: 'projects.index', user_id: current_user.id, + include_task_definitions: include_task_definitions, include_inactive: include_inactive, + project_count: projects.size, + task_definition_count: include_task_definitions ? projects.sum { |project| project.unit.task_definitions.size } : 0 }.to_json) + end + + desc "Reports whether the current user has any project history, including withdrawn enrolments" + get '/projects/history' do + # Project.for_user deliberately hides withdrawn enrolments. Onboarding must + # count those as prior history without exposing project details or accepting + # a client-supplied owner. Keep the ordinary project listing unchanged. + present({ hasProjects: Project.where(user_id: current_user.id).exists? }) end desc 'Get project' @@ -147,11 +199,26 @@ class ProjectsApi < Grape::API error!({ error: "You do not have permissions to change this student" }, 403) end - # if someone changes this setting manually, clear the autogenerated status - project.portfolio_auto_generated = false - project.compile_portfolio = params[:compile_portfolio] - project.portfolio_submission_date = Time.zone.now - project.save + new_portfolio_submission = false + submission_saved = false + + # Lock the project while deciding whether this is new so concurrent + # retries cannot both observe the old state and send two receipts. + project.with_lock do + # A true request starts a new manual submission when no manual portfolio + # is already queued. Converting a queued auto-generated portfolio into + # a manual submission is also new; retrying the same request is not. + new_portfolio_submission = params[:compile_portfolio] && + (!project.compile_portfolio? || project.portfolio_auto_generated?) + + # if someone changes this setting manually, clear the autogenerated status + project.portfolio_auto_generated = false + project.compile_portfolio = params[:compile_portfolio] + project.portfolio_submission_date = Time.zone.now if new_portfolio_submission + submission_saved = project.save + end + + notify_portfolio_received(project) if submission_saved && new_portfolio_submission end Entities::ProjectEntity.represent(project, only: [:campus_id, :enrolled, :target_grade, :submitted_grade, :compile_portfolio, :portfolio_available, :uses_draft_learning_summary, :stats], for_student: for_student) diff --git a/app/api/task_prioritization_api.rb b/app/api/task_prioritization_api.rb new file mode 100644 index 0000000000..cd1f8b036c --- /dev/null +++ b/app/api/task_prioritization_api.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'grape' + +class TaskPrioritizationApi < Grape::API + helpers AuthenticationHelpers + helpers AuthorisationHelpers + helpers DbHelpers + + DEFAULT_PER_PAGE = 50 + MAX_PER_PAGE = 50 + + before do + authenticated? + end + + desc 'Get prioritized task recommendations for a student', + detail: 'Returns the authenticated student\'s actionable tasks ranked by effective deadline, relative task size, and deadline workload.' + + params do + optional :page, type: Integer, default: 1, values: ->(value) { value.positive? } + optional :per_page, type: Integer, default: DEFAULT_PER_PAGE, values: 1..MAX_PER_PAGE + end + + get '/tasks/recommended' do + recommendations = TaskPrioritizationService.new(current_user).call + offset = (params[:page] - 1) * params[:per_page] + + { + data: recommendations.slice(offset, params[:per_page]) || [], + meta: { + page: params[:page], + per_page: params[:per_page], + total_count: recommendations.length, + total_pages: (recommendations.length / params[:per_page].to_f).ceil + } + } + end +end diff --git a/app/helpers/collection_pagination_helpers.rb b/app/helpers/collection_pagination_helpers.rb new file mode 100644 index 0000000000..7968067f67 --- /dev/null +++ b/app/helpers/collection_pagination_helpers.rb @@ -0,0 +1,23 @@ +module CollectionPaginationHelpers + MAX_PAGE = 2_147_483_647 + MAX_PER_PAGE = 500 + DEFAULT_PER_PAGE = 50 + + # Existing web/mobile callers consume a complete array. Only callers that + # explicitly request pagination receive a bounded page, still as an array. + # Count the already-authorised relation, never the unrestricted model. + def paginate_collection(relation) + return relation if params[:page].nil? && params[:per_page].nil? + + page = params[:page] || 1 + per_page = params[:per_page] || DEFAULT_PER_PAGE + total = relation.count + + header 'X-Total-Count', total.to_s + header 'X-Page', page.to_s + header 'X-Per-Page', per_page.to_s + header 'X-Total-Pages', ((total + per_page - 1) / per_page).to_s + + relation.reorder(id: :asc).limit(per_page).offset((page - 1) * per_page) + end +end diff --git a/app/models/peer_progress_snapshot.rb b/app/models/peer_progress_snapshot.rb new file mode 100644 index 0000000000..fe5dab6aef --- /dev/null +++ b/app/models/peer_progress_snapshot.rb @@ -0,0 +1,138 @@ +# frozen_string_literal: true + +class PeerProgressSnapshot < ApplicationRecord + # MariaDB exposes its JSON-compatible LONGTEXT column as text to the mysql2 + # adapter. Declaring the logical type explicitly keeps Hash casting identical + # on MariaDB and native-JSON MySQL deployments. + attribute :status_counts, :json + + 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 :submitted_count, + numericality: { + only_integer: true, + greater_than_or_equal_to: 0 + }, + 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 + validate :submitted_count_fits_cohort + validate :status_counts_cover_the_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 + + def submitted_count_fits_cohort + return if submitted_count.nil? || cohort_size.nil? + return if submitted_count <= cohort_size + + errors.add( + :submitted_count, + 'must not exceed the cohort size' + ) + end + + def status_counts_cover_the_cohort + return if status_counts.nil? + + keys_valid = status_counts.is_a?(Hash) && + status_counts.keys.map(&:to_s).sort == + PeerProgressDistributionPolicy::STATUS_KEYS.sort + values_valid = status_counts.is_a?(Hash) && + status_counts.values.all? do |value| + value.is_a?(Integer) && value >= 0 + end + + unless keys_valid && values_valid + errors.add( + :status_counts, + 'must contain every supported task status with non-negative integer counts' + ) + return + end + + return if PeerProgressDistributionPolicy.valid_status_counts?( + status_counts, + cohort_size: cohort_size + ) + + errors.add( + :status_counts, + 'must sum to the cohort size' + ) + end +end diff --git a/app/services/peer_progress_aggregation_service.rb b/app/services/peer_progress_aggregation_service.rb new file mode 100644 index 0000000000..6ad693f5c7 --- /dev/null +++ b/app/services/peer_progress_aggregation_service.rb @@ -0,0 +1,147 @@ +# 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 + class UnsupportedTaskStatusError < StandardError; end + + 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 + ) + status_counts = status_counts_for( + cohort: cohort, + task_definitions: task_definitions, + cohort_size: cohort_size + ) + + task_definitions.each do |task_definition| + key = [task_definition.id, target_grade] + submitted_count = submitted_counts.fetch(task_definition.id, 0) + + snapshot = existing_snapshots[key] || PeerProgressSnapshot.new( + unit: unit, + task_definition: task_definition, + target_grade: target_grade + ) + + snapshot.assign_attributes( + cohort_size: cohort_size, + submitted_count: submitted_count, + submitted_percentage: percentage( + submitted_count: submitted_count, + cohort_size: cohort_size + ), + status_counts: status_counts.fetch(task_definition.id), + 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 status_counts_for(cohort:, task_definitions:, cohort_size:) + materialized_counts = Task + .where( + project_id: cohort.select(:id), + task_definition_id: task_definitions.select(:id) + ) + .group(:task_definition_id, :task_status_id) + .distinct + .count(:project_id) + + task_definitions.to_h do |task_definition| + counts = PeerProgressDistributionPolicy::STATUS_KEYS.index_with { 0 } + + materialized_counts.each do |(task_definition_id, status_id), count| + next unless task_definition_id == task_definition.id + + status = canonical_status_for(status_id) + counts[status] += count + end + + missing_task_count = cohort_size - counts.values.sum + if missing_task_count.negative? + raise ArgumentError, + 'task status counts exceed the peer-progress cohort size' + end + + counts['not_started'] += missing_task_count + [task_definition.id, counts] + end + end + + def canonical_status_for(status_id) + id = status_id.to_i + expected_status = PeerProgressDistributionPolicy::STATUS_KEYS[id - 1] + mapped_status = TaskStatus.id_to_key(id).to_s if expected_status.present? + + return mapped_status if expected_status.present? && + mapped_status == expected_status + + # TaskStatus.id_to_key deliberately falls back to not_started for unknown + # IDs. That is useful elsewhere, but would silently corrupt an aggregate + # if a new lifecycle state were introduced without extending this policy. + raise UnsupportedTaskStatusError, + 'peer-progress aggregation encountered an unsupported task status' + 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/services/peer_progress_distribution_policy.rb b/app/services/peer_progress_distribution_policy.rb new file mode 100644 index 0000000000..75786e2df1 --- /dev/null +++ b/app/services/peer_progress_distribution_policy.rb @@ -0,0 +1,151 @@ +# frozen_string_literal: true + +# Builds the public, privacy-preserving task-status distribution from an +# internal peer-progress snapshot. +# +# Quantising every status independently is not sufficient on its own. When the +# buckets are considered together, their sum constraint can occasionally make +# a raw count unique (for example, a cohort of 24 split into 6 and 18). Before +# releasing a vector, this policy assumes an observer knows the cohort size and +# verifies that every status still has at least two feasible raw counts. +class PeerProgressDistributionPolicy + PERCENTAGE_BUCKET_SIZE = 10.0 + + STATUS_KEYS = %w[ + not_started + complete + need_help + working_on_it + fix_and_resubmit + feedback_exceeded + redo + discuss + ready_for_feedback + demonstrate + fail + time_exceeded + assess_in_portfolio + attention_required + rediscuss + ].freeze + + def self.quantised_percentage(value) + ((value.to_f / PERCENTAGE_BUCKET_SIZE).round * + PERCENTAGE_BUCKET_SIZE).to_f + end + + def self.percentage(count:, cohort_size:) + return nil unless cohort_size.to_i.positive? + + ((count.to_i * 100.0) / cohort_size).round(2) + end + + def self.quantised_count_percentage(count:, cohort_size:) + quantised_percentage( + percentage(count: count, cohort_size: cohort_size) + ) + end + + def self.build(status_counts:, cohort_size:) + counts = normalized_counts(status_counts) + return nil if counts.nil? || cohort_size.to_i <= 0 + return nil unless counts.values.sum == cohort_size + + distribution = STATUS_KEYS.map do |status| + { + status: status, + percentage: quantised_count_percentage( + count: counts.fetch(status), + cohort_size: cohort_size + ) + } + end + + return nil unless preserves_count_ambiguity?( + distribution: distribution, + cohort_size: cohort_size + ) + + distribution + end + + def self.valid_status_counts?(status_counts, cohort_size:) + counts = normalized_counts(status_counts) + + counts.present? && counts.values.sum == cohort_size + end + + def self.normalized_counts(status_counts) + return nil unless status_counts.is_a?(Hash) + + counts = status_counts.transform_keys(&:to_s) + return nil unless counts.keys.sort == STATUS_KEYS.sort + return nil unless counts.values.all? do |value| + value.is_a?(Integer) && value >= 0 + end + + counts + end + private_class_method :normalized_counts + + def self.preserves_count_ambiguity?(distribution:, cohort_size:) + ranges = distribution.map do |entry| + count_range_for_bucket( + entry.fetch(:percentage), + cohort_size + ) + end + + minimum_sum = ranges.sum(&:begin) + maximum_sum = ranges.sum(&:end) + + ranges.all? do |range| + other_minimum = minimum_sum - range.begin + other_maximum = maximum_sum - range.end + feasible_minimum = [range.begin, cohort_size - other_maximum].max + feasible_maximum = [range.end, cohort_size - other_minimum].min + + feasible_maximum - feasible_minimum >= 1 + end + end + private_class_method :preserves_count_ambiguity? + + # The quantised value is monotonic as count increases. Binary-searching both + # edges avoids rebuilding every possible count bucket on every student GET: + # detailed policy evaluation is O(statuses * log(cohort_size)), with no + # unbounded cohort-size cache. + def self.count_range_for_bucket(bucket, cohort_size) + first = binary_search_count(cohort_size) do |count| + quantised_count_percentage( + count: count, + cohort_size: cohort_size + ) >= bucket + end + last = binary_search_count(cohort_size, upper: true) do |count| + quantised_count_percentage( + count: count, + cohort_size: cohort_size + ) <= bucket + end + + first..last + end + private_class_method :count_range_for_bucket + + def self.binary_search_count(cohort_size, upper: false) + low = 0 + high = cohort_size + + while low < high + midpoint = (low + high + (upper ? 1 : 0)) / 2 + if yield(midpoint) + upper ? low = midpoint : high = midpoint + else + upper ? high = midpoint - 1 : low = midpoint + 1 + end + end + + low + end + private_class_method :binary_search_count +end diff --git a/app/services/peer_progress_viewer_policy.rb b/app/services/peer_progress_viewer_policy.rb new file mode 100644 index 0000000000..40b0f011f2 --- /dev/null +++ b/app/services/peer_progress_viewer_policy.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +# Converts an internal whole-cohort snapshot into peer-only exact aggregates +# for one authenticated viewer. Public quantisation and vector ambiguity checks +# are applied afterwards; raw values from this policy never cross the API. +class PeerProgressViewerPolicy + def self.viewer_context_current?(snapshot:, viewer_project:, viewer_task:) + project_current = viewer_project.persisted? && + viewer_project.updated_at.present? && + viewer_project.updated_at <= snapshot.calculated_at + task_current = !viewer_task.persisted? || + (viewer_task.updated_at.present? && + viewer_task.updated_at <= snapshot.calculated_at) + + project_current && task_current + end + + def self.build(snapshot:, viewer_project:, viewer_task:) + return nil unless viewer_context_current?( + snapshot: snapshot, + viewer_project: viewer_project, + viewer_task: viewer_task + ) + return nil unless snapshot.submitted_count.is_a?(Integer) + return nil unless snapshot.submitted_count.between?( + 0, + snapshot.cohort_size + ) + return nil unless PeerProgressDistributionPolicy.valid_status_counts?( + snapshot.status_counts, + cohort_size: snapshot.cohort_size + ) + + peer_cohort_size = snapshot.cohort_size - 1 + return nil if peer_cohort_size.negative? + + counts = snapshot.status_counts.to_h.transform_keys(&:to_s).dup + viewer_status = canonical_status(viewer_task.task_status_id) + return nil if viewer_status.nil? || counts.fetch(viewer_status).zero? + + counts[viewer_status] -= 1 + submitted_count = snapshot.submitted_count + submitted_count -= 1 if viewer_task.file_uploaded_at.present? + return nil unless submitted_count.between?(0, peer_cohort_size) + return nil unless PeerProgressDistributionPolicy.valid_status_counts?( + counts, + cohort_size: peer_cohort_size + ) + + { + cohort_size: peer_cohort_size, + submitted_count: submitted_count, + status_counts: counts + } + end + + def self.public_metrics(peer_progress) + counts = peer_progress.fetch(:status_counts) + cohort_size = peer_progress.fetch(:cohort_size) + distribution = PeerProgressDistributionPolicy.build( + status_counts: counts, + cohort_size: cohort_size + ) + + { + submitted_percentage: + PeerProgressDistributionPolicy.quantised_count_percentage( + count: peer_progress.fetch(:submitted_count), + cohort_size: cohort_size + ), + completed_percentage: + PeerProgressDistributionPolicy.quantised_count_percentage( + count: counts.fetch('complete'), + cohort_size: cohort_size + ), + status_distribution: distribution, + distribution_unavailable_reason: + distribution.nil? ? 'privacy_protection' : nil + } + end + + def self.canonical_status(status_id) + id = status_id.to_i + expected_status = PeerProgressDistributionPolicy::STATUS_KEYS[id - 1] + mapped_status = TaskStatus.id_to_key(id).to_s if expected_status.present? + + mapped_status if mapped_status == expected_status + end + private_class_method :canonical_status +end diff --git a/app/services/task_prioritization_service.rb b/app/services/task_prioritization_service.rb new file mode 100644 index 0000000000..b4b7a7fe08 --- /dev/null +++ b/app/services/task_prioritization_service.rb @@ -0,0 +1,202 @@ +# frozen_string_literal: true + +class TaskPrioritizationService + Candidate = Data.define(:project, :task_definition, :task, :due_date, :blocked) + + DEADLINE_HORIZON_DAYS = 28 + DEADLINE_WEIGHT = 0.60 + WORKLOAD_WEIGHT = 0.25 + TASK_SIZE_WEIGHT = 0.15 + WORKLOAD_MIDPOINT = 5.0 + PREREQUISITE_STATUS_LEVELS = { + attention_required: 0, + ready_for_feedback: 1, + assess_in_portfolio: 1, + discuss: 2, + rediscuss: 2, + demonstrate: 2, + complete: 3 + }.freeze + + def initialize(user, today: Time.zone.today) + @user = user + @today = today + end + + def call + candidates = remaining_candidates + recommendation_candidates = candidates.reject(&:blocked) + task_size_scores = calculate_task_size_scores(candidates) + workload_scores = calculate_workload_scores(candidates, task_size_scores) + + recommendations = recommendation_candidates.map do |candidate| + [candidate, build_recommendation(candidate, task_size_scores, workload_scores)] + end + sorted_recommendations = recommendations.sort_by do |candidate, recommendation| + [ + -recommendation[:priority_score], + candidate.due_date || Date.new(9999, 12, 31), + recommendation[:project_id], + recommendation[:task_definition_id] + ] + end + + sorted_recommendations.map(&:last) + end + + private + + attr_reader :today, :user + + def remaining_candidates + projects.flat_map do |project| + tasks_by_definition = project.tasks.index_by(&:task_definition_id) + + assigned_task_definitions(project).filter_map do |task_definition| + task = tasks_by_definition[task_definition.id] + next if task && final_status_ids.include?(task.task_status_id) + + Candidate.new( + project: project, + task_definition: task_definition, + task: task, + due_date: effective_due_date(project, task_definition, task)&.to_date, + blocked: blocked_by_prerequisite?(task_definition, tasks_by_definition) + ) + end + end + end + + def projects + Project + .for_user(user, false) + .includes( + { tasks: [:task_status, { task_definition: :grade_due_dates }] }, + { unit: { task_definitions: [:grade_due_dates, :task_prerequisites] } } + ) + end + + def assigned_task_definitions(project) + @assigned_task_definitions ||= {} + @assigned_task_definitions[project.id] ||= project.unit.task_definitions.select do |task_definition| + task_definition.target_grade <= project.target_grade.to_i + end + end + + def final_status_ids + @final_status_ids ||= [ + TaskStatus.complete.id, + TaskStatus.fail.id, + TaskStatus.feedback_exceeded.id, + TaskStatus.time_exceeded.id, + TaskStatus.assess_in_portfolio.id, + TaskStatus.ready_for_feedback.id + ] + end + + def effective_due_date(project, task_definition, task) + return task.local_due_date if task + + if project.unit.allow_flexible_dates + grade_target_date = task_definition.grade_target_date(project.target_grade.to_i) + return grade_target_date if grade_target_date + end + + task_definition.target_date + end + + def blocked_by_prerequisite?(task_definition, tasks_by_definition) + task_definition.task_prerequisites.any? do |link| + prerequisite_task = tasks_by_definition[link.prerequisite_id] + next true unless prerequisite_task&.ready_or_complete? + + current_level = PREREQUISITE_STATUS_LEVELS[prerequisite_task.status] + required_level = PREREQUISITE_STATUS_LEVELS[TaskStatus.id_to_key(link.task_status_id)] + + current_level.nil? || required_level.nil? || current_level < required_level + end + end + + # Weighting is comparable within a unit, not across units. The denominator + # includes all work assigned at the student's target grade, so completing a + # task does not inflate the relative size of every task that remains. + def calculate_task_size_scores(candidates) + project_totals = candidates.map(&:project).uniq.to_h do |project| + assigned_definitions = assigned_task_definitions(project) + total_weight = assigned_definitions.sum { |task_definition| definition_weight(task_definition) } + + [project.id, { weight: total_weight, count: assigned_definitions.length }] + end + + candidates.to_h do |candidate| + totals = project_totals.fetch(candidate.project.id) + score = if totals[:weight].positive? + (task_weight(candidate) / totals[:weight]) * 100 + elsif totals[:count].positive? + 100.0 / totals[:count] + else + 0 + end + [candidate, score] + end + end + + # Workload pressure is full-project percentage points due by this task's date + # per available day. A fixed saturating curve maps five percentage points per + # day to 50 without rescaling recommendations against one another. + # Grouping equal dates before accumulating preserves the inclusive + # "work due by this date" semantics without rescanning every candidate. + def calculate_workload_scores(candidates, task_size_scores) + workload_scores = candidates.index_with { 0 } + candidates_with_due_dates = candidates.select(&:due_date).group_by(&:due_date) + cumulative_work = 0.0 + + candidates_with_due_dates.sort_by { |due_date, _| due_date }.each do |due_date, due_candidates| + cumulative_work += due_candidates.sum { |candidate| task_size_scores.fetch(candidate) } + available_days = [(due_date - today).to_i, 1].max + raw_pressure = cumulative_work / available_days + pressure = (raw_pressure * 100) / (raw_pressure + WORKLOAD_MIDPOINT) + + due_candidates.each do |candidate| + workload_scores[candidate] = pressure + end + end + + workload_scores + end + + def task_weight(candidate) + definition_weight(candidate.task_definition) + end + + def definition_weight(task_definition) + [task_definition.weighting.to_f, 0].max + end + + def deadline_score(candidate) + return 0 unless candidate.due_date + + days_left = (candidate.due_date - today).to_i + return 100 if days_left <= 0 + return 0 if days_left >= DEADLINE_HORIZON_DAYS + + ((DEADLINE_HORIZON_DAYS - days_left) / DEADLINE_HORIZON_DAYS.to_f) * 100 + end + + def build_recommendation(candidate, task_size_scores, workload_scores) + priority_score = + (DEADLINE_WEIGHT * deadline_score(candidate)) + + (WORKLOAD_WEIGHT * workload_scores.fetch(candidate)) + + (TASK_SIZE_WEIGHT * task_size_scores.fetch(candidate)) + priority_score = priority_score.clamp(0, 100) + + { + task_id: candidate.task&.id, + task_definition_id: candidate.task_definition.id, + task_name: candidate.task_definition.name, + project_id: candidate.project.id, + unit_id: candidate.project.unit_id, + priority_score: priority_score.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/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..cecc6b0620 --- /dev/null +++ b/db/migrate/20260818160804_add_target_grade_changed_at_to_projects.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +class AddTargetGradeChangedAtToProjects < ActiveRecord::Migration[8.0] + def up + # Keep the database default after the migration. During a rolling deploy an + # older application instance does not know about this column, so its INSERT + # must still produce a valid row once the column becomes NOT NULL. + add_column :projects, + :target_grade_changed_at, + :datetime, + default: -> { 'CURRENT_TIMESTAMP(6)' } + + # 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/migrate/20260824000002_ensure_target_grade_changed_at_default.rb b/db/migrate/20260824000002_ensure_target_grade_changed_at_default.rb new file mode 100644 index 0000000000..ae825c27ad --- /dev/null +++ b/db/migrate/20260824000002_ensure_target_grade_changed_at_default.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +class EnsureTargetGradeChangedAtDefault < ActiveRecord::Migration[8.0] + CURRENT_TIMESTAMP_DEFAULT = /\Acurrent_timestamp\(6\)\z/i + + def up + column = connection.columns(:projects).find do |candidate| + candidate.name == 'target_grade_changed_at' + end + raise 'projects.target_grade_changed_at must exist before its default is repaired' unless column + + return if current_timestamp_default?(column) + + change_column_default :projects, + :target_grade_changed_at, + -> { 'CURRENT_TIMESTAMP(6)' } + end + + def down + # The default is an ongoing rolling-deploy invariant, not temporary data + # needed only while this migration runs. Deliberately retain it on rollback. + end + + private + + def current_timestamp_default?(column) + value = column.default_function || column.default + value.to_s.delete(' ').match?(CURRENT_TIMESTAMP_DEFAULT) + end +end diff --git a/db/migrate/20260824000003_add_detailed_peer_progress.rb b/db/migrate/20260824000003_add_detailed_peer_progress.rb new file mode 100644 index 0000000000..1df15bbf58 --- /dev/null +++ b/db/migrate/20260824000003_add_detailed_peer_progress.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddDetailedPeerProgress < ActiveRecord::Migration[8.0] + def change + # Internal aggregate counts only. Exact upload counts are required so the + # student API can subtract the authenticated viewer before quantisation; + # reconstructing a count from the legacy rounded percentage is unsafe. + add_column :peer_progress_snapshots, :submitted_count, :integer + add_column :peer_progress_snapshots, :status_counts, :json + + # Existing and future users start opted in, while the profile endpoint can + # persist an explicit false value. + add_column :users, + :display_peer_progress, + :boolean, + default: true, + null: false + end +end diff --git a/docs/collection-pagination.md b/docs/collection-pagination.md new file mode 100644 index 0000000000..898edd1f03 --- /dev/null +++ b/docs/collection-pagination.md @@ -0,0 +1,37 @@ +# Optional collection pagination + +Existing callers still receive a complete JSON array when neither `page` nor +`per_page` is provided. No web or mobile client update is required to retain +the existing behaviour. + +The following GET endpoints support opt-in pagination: + +- `/api/activity_types` +- `/api/campuses` +- `/api/users`, `/api/users/convenors`, `/api/users/tutors` +- `/api/units` +- `/api/projects` +- `/api/units/:unit_id/group_sets/:group_set_id/groups/:group_id/members` + +Provide either `page` or `per_page` to opt in. The missing parameter defaults +to page 1 or 50 records per page. Both parameters must be positive scalar +integers; the maximum page is 2,147,483,647 and maximum page size is 500. +Malformed, blank, zero, negative and out-of-range values return HTTP 400. + +Paged responses remain JSON arrays, ordered by record ID. Existing filters, +preloads and access permissions apply before pagination. Pages past the end +return an empty array. These response headers describe the authorised result: + +- `X-Total-Count`: total matching records before pagination +- `X-Page`: requested page +- `X-Per-Page`: effective page size +- `X-Total-Pages`: number of pages (zero for an empty collection) + +These headers are exposed through CORS for browser clients. For example, +`GET /api/projects?page=2&per_page=25&include_inactive=true` returns the second +page of the signed-in user's enrolled projects, including inactive units. + +ID ordering makes pages deterministic for an unchanged collection. This is +offset pagination, not a snapshot: concurrent additions or deletions may +change page boundaries. Clients needing the full current list can retain the +existing unpaged request, or refresh their pages. diff --git a/docs/dashboard-feedback-state.md b/docs/dashboard-feedback-state.md new file mode 100644 index 0000000000..5160607381 --- /dev/null +++ b/docs/dashboard-feedback-state.md @@ -0,0 +1,59 @@ +# Cross-Project Dashboard Feedback State + +## Purpose + +The Cross-Project Dashboard needs to distinguish genuine staff feedback from the existing general unread comment count without exposing feedback content. + +## Response contract + +When task data is included in the authenticated student's `/api/projects` response, each task may include: + +| Field | Type | Meaning | +| --- | --- | --- | +| `has_feedback` | Boolean | Whether the task has qualifying manual staff feedback according to the existing `Task#has_manual_feedback_since_first_ready_for_feedback?` rule. | + +Example: + +```json +{ + "id": 123, + "task_definition_id": 45, + "status": "complete", + "num_new_comments": 1, + "has_feedback": true +} +``` + +## Exact meaning + +`has_feedback` is `true` when the existing task feedback rule finds at least one qualifying comment: + +- the comment type is `text`, `audio`, `image`, `pdf`, or `discussion`; +- the comment was authored by unit teaching staff; +- when the task has entered Ready for Feedback, the comment was created on or after the first Ready for Feedback event; +- the comment is not an automated message beginning with `**Automated Message:**`. + +The field reuses the existing backend feedback definition rather than introducing a dashboard-specific definition. + +## Privacy and access control + +Only the boolean feedback state is exposed. + +The dashboard response does not expose: + +- feedback text; +- marker notes; +- feedback author details; +- feedback timestamps; +- unread-feedback state; +- another student's feedback state. + +`GET /api/projects` derives projects from the authenticated `current_user`. Direct project access continues to use the existing project authorisation checks. + +## Compatibility + +Frontend consumers must treat `has_feedback` as optional. Missing feedback metadata must not prevent the Cross-Project Dashboard from loading and is treated as no available feedback state. + +## Scope + +This ticket does not add feedback text, feedback timestamps, author information, or unread-feedback tracking. Any future expansion requires a separate privacy and contract review. diff --git a/docs/peer-progress-api.md b/docs/peer-progress-api.md new file mode 100644 index 0000000000..7d37d64eac --- /dev/null +++ b/docs/peer-progress-api.md @@ -0,0 +1,217 @@ +# Student Peer Progress API + +## Route and authorisation + +`GET /api/projects/:id/task_def_id/:task_definition_id/peer_progress` + +The route is restricted to the authenticated student who owns the active, +enrolled project. The unit and target grade are derived on the server. The task +must belong to that unit, be applicable to the student's target grade, and be +released for that project. The browser cannot select a peer cohort. + +Every response has `Cache-Control: private, no-store`. + +## HTTP 200 response contract + +Authorised business states use the same allowlisted response shape: + +| Field | Type | Nullable | Meaning | +| --- | --- | --- | --- | +| `task_definition_id` | Integer | No | Requested task definition. | +| `unit_id` | Integer | No | Unit derived from the authenticated project. | +| `target_grade` | Integer | Yes | Valid server-derived grade, or `null`. | +| `submitted_percentage` | Number | Yes | Compatibility metric: other students with a task file upload, quantised to 10-point buckets. | +| `completed_percentage` | Number | Yes | Compact-display metric: other students whose snapshot status is exactly `complete`, quantised to 10-point buckets. | +| `status_distribution` | Array | Yes | Ordered, quantised full-lifecycle distribution, or `null` when it cannot safely be released. | +| `distribution_available` | Boolean | No | Whether `status_distribution` is present. | +| `distribution_unavailable_reason` | String | Yes | Safe machine reason when detailed data is absent. | +| `is_suppressed` | Boolean | No | The entire aggregate is hidden because the cohort is below the configured floor. | +| `is_stale` | Boolean | No | The stored snapshot is older than the configured window. | +| `is_feature_enabled` | Boolean | No | Whether the unit has enabled peer progress. | +| `is_user_enabled` | Boolean | No | The authenticated user's saved `display_peer_progress` preference. | +| `last_updated_at` | String | Yes | Snapshot time as UTC ISO 8601, or `null`. | +| `unavailable_reason` | String | Yes | Safe machine reason when compact data is absent. | +| `unavailable_message` | String | No | Empty on compact success; otherwise neutral user-facing copy. | + +Normal response example: + +```json +{ + "task_definition_id": 12, + "unit_id": 5, + "target_grade": 2, + "submitted_percentage": 60.0, + "completed_percentage": 10.0, + "status_distribution": [ + { "status": "not_started", "percentage": 20.0 }, + { "status": "complete", "percentage": 10.0 }, + { "status": "need_help", "percentage": 0.0 }, + { "status": "working_on_it", "percentage": 20.0 }, + { "status": "fix_and_resubmit", "percentage": 10.0 }, + { "status": "feedback_exceeded", "percentage": 0.0 }, + { "status": "redo", "percentage": 10.0 }, + { "status": "discuss", "percentage": 0.0 }, + { "status": "ready_for_feedback", "percentage": 20.0 }, + { "status": "demonstrate", "percentage": 0.0 }, + { "status": "fail", "percentage": 10.0 }, + { "status": "time_exceeded", "percentage": 0.0 }, + { "status": "assess_in_portfolio", "percentage": 0.0 }, + { "status": "attention_required", "percentage": 0.0 }, + { "status": "rediscuss", "percentage": 0.0 } + ], + "distribution_available": true, + "distribution_unavailable_reason": null, + "is_suppressed": false, + "is_stale": false, + "is_feature_enabled": true, + "is_user_enabled": true, + "last_updated_at": "2026-08-24T03:15:00Z", + "unavailable_reason": null, + "unavailable_message": "" +} +``` + +The 15 status entries are always ordered by canonical `TaskStatus` ID: + +1. `not_started` +2. `complete` +3. `need_help` +4. `working_on_it` +5. `fix_and_resubmit` +6. `feedback_exceeded` +7. `redo` +8. `discuss` +9. `ready_for_feedback` +10. `demonstrate` +11. `fail` +12. `time_exceeded` +13. `assess_in_portfolio` +14. `attention_required` +15. `rediscuss` + +A missing task row counts as `not_started`. Each enrolled project contributes +to exactly one stored status for a task. Before any public calculation, the API +subtracts the authenticated student's project, status, and upload contribution. +All percentages therefore describe other students, never a cohort containing +the viewer. + +## Availability reasons + +`unavailable_reason` is one of: + +- `user_disabled` +- `feature_disabled` +- `target_grade_unavailable` +- `snapshot_unavailable` +- `insufficient_cohort` +- `aggregation_incomplete` +- `stale` + +`distribution_unavailable_reason` repeats the applicable compact reason, or is: + +- `detailed_data_unavailable` when a pre-migration/incomplete snapshot has no + valid lifecycle aggregate; +- `privacy_protection` when compact metrics are safe but the combined detailed + vector is not safe to release. + +The API never states which status caused detailed privacy suppression. + +## Privacy and quantisation + +Raw whole-cohort size, exact uploaded count, completed count, and per-status +counts are internal-only. They are never included in the student response. +`PeerProgressViewerPolicy` first subtracts the authenticated viewer from all +three exact aggregates. The privacy floor and every quantisation/policy check +then run over the remaining peers. + +`DF_PPI_MINIMUM_COHORT_SIZE` must be at least +`PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE` (`21`). Cohorts below the configured +value return `is_suppressed: true` and no percentages or distribution. The +configured minimum is a **remaining-peer** floor: with the default of 21, a +stored cohort needs at least 22 active projects including the viewer. Empty and +small peer cohorts use the same response state. + +Public percentages are independently rounded to the nearest 10 percentage +points. With 21 remaining peers, a single compact bucket maps to at least two +possible peer counts. Because the viewer is absent from those counts, their +knowledge of their own status or upload cannot collapse that ambiguity. +Therefore `0.0` does not prove no peer is in a state, and `100.0` does not prove +every peer is. + +Independent buckets are not sufficient for a multi-status histogram because +the buckets can constrain one another. Before returning `status_distribution`, +`PeerProgressDistributionPolicy` assumes the observer already knows the exact +cohort size and computes the feasible raw-count range for every status given all +15 buckets and the requirement that counts sum to the cohort. The whole vector +is returned only if every status retains at least two feasible raw counts. +Otherwise the vector is withheld with `privacy_protection`; compact metrics can +remain available. + +Because each status is independently quantised, a public distribution is a +visual estimate and its percentages are not generally guaranteed to sum to 100. + +## User preference + +`users.display_peer_progress` is `true` by default and non-null for new and +existing users. It is exposed by `Entities::UserEntity`, including authentication +responses, and can be saved through the normal profile endpoint: + +```http +PUT /api/users/:id +Content-Type: application/json + +{ + "user": { + "display_peer_progress": false + } +} +``` + +When false, the peer-progress endpoint returns `is_user_enabled: false`, reason +`user_disabled`, and no peer metrics. Saving `true` re-enables it. + +## Freshness and grade changes + +`DF_PPI_STALE_AFTER_HOURS` must be a positive integer. A stale snapshot returns +no metrics. Each project records `target_grade_changed_at`; a snapshot calculated +before the current target-grade selection is treated as unavailable until the +next aggregation. + +Missing or invalid configuration fails closed with HTTP 503 once an enabled +user, unit, valid grade, and snapshot require the configuration. + +If the viewer's persisted task changed after `snapshot.calculated_at`, the API +returns `snapshot_unavailable` rather than subtracting a current status/upload +from an older aggregate. Snapshots created before exact `submitted_count` and +15-status data were introduced return `aggregation_incomplete` until the next +aggregation. + +## Error responses + +- `200`: authorised normal, preference-off, disabled, suppressed, stale, or + otherwise unavailable business state. +- `404`: the project/task cannot safely be exposed to this caller. Unknown IDs, + wrong ownership, wrong role, inactive enrolment/unit, unreleased tasks, and + inapplicable tasks share the same message. +- `419`: authentication failed through the existing OnTrack flow. +- `503`: required peer-progress configuration is missing or invalid. + +## Demo data + +The all-features demo is triple-guarded: Rails development, database exactly +`doubtfire-all-features-demo`, and `DF_DEMO_DATA_PROFILE=all-features`. + +`db:all_features_demo` creates a 25-student total cohort (24 remaining peers for +the demo viewer) with visible `not_started`, +`working_on_it`, `ready_for_feedback`, `fix_and_resubmit`, `redo`, `complete`, +and `fail` states. It uses the production aggregation service. + +`db:all_features_demo_verify` is read-only and fails unless the preference and +unit feature are enabled, the task is released, the snapshot is fresh and leaves +enough peers after viewer subtraction, the true completed metric is available, +and the same production viewer/public policies release all 15 status keys with +the seven showcase states visible. + +`db:ppi_sample_data` creates the larger two-unit dashboard dataset under the +same guards and validates every generated snapshot with the same distribution +policy. diff --git a/docs/peer-progress/data-source-map.md b/docs/peer-progress/data-source-map.md new file mode 100644 index 0000000000..00ac4657a3 --- /dev/null +++ b/docs/peer-progress/data-source-map.md @@ -0,0 +1,178 @@ +# Peer Progress Indicator — Backend Data-Source Map + +**Ticket:** PPI-D02 — Publish the peer-progress backend data-source and field-ownership map +**Status:** Updated for the production API PR #60 contract. +**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 records that baseline plus the additive detailed lifecycle, +completion, privacy, and profile-preference work in API PR #60. + +--- + +## 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` | API PR #60 | One row per `(unit, task_definition, target_grade)`. Stores internal whole-cohort `cohort_size`, exact `submitted_count`, all 15 raw `status_counts`, compatibility `submitted_percentage`, and `calculated_at`. Validates exact counts fit/cover the cohort. Raw counts never cross the student API boundary. | +| `app/services/peer_progress_aggregation_service.rb` | `PeerProgressAggregationService.call(unit:, calculated_at:)` | API PR #60 | Batch job logic. For each grade value, counts uploads and every canonical current task status. A missing task row contributes to `not_started`, so each cohort member contributes exactly once per task. | +| `app/services/peer_progress_viewer_policy.rb` | `PeerProgressViewerPolicy.build`, `.public_metrics` | API PR #60 | Subtracts the authenticated viewer's project, exact upload contribution, and status before applying the remaining-peer floor, compact quantisation, or detailed-vector policy. Fails closed if the viewer project/task changed after the snapshot or an exact aggregate is incomplete. | +| `app/services/peer_progress_distribution_policy.rb` | `PeerProgressDistributionPolicy` | API PR #60 | Defines the 15-key canonical order, 10-point quantisation, and the vector-wide feasible-count ambiguity check. A detailed vector is released only when every status retains at least two possible raw counts even if the observer knows the cohort size. | +| `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` with a retained database `CURRENT_TIMESTAMP` default for rolling-deploy compatibility, backfills existing rows to the migration run time, then applies `NOT NULL`. | +| `db/migrate/20260824000002_ensure_target_grade_changed_at_default.rb` | — | Release readiness | Idempotently restores the retained `CURRENT_TIMESTAMP` default for development or staging databases that recorded the earlier migration before its rolling-deploy fix was added. Fresh databases already satisfy the invariant, so this migration performs no schema change there. | +| `db/migrate/20260824000003_add_detailed_peer_progress.rb` | — | API PR #60 | Adds internal exact `peer_progress_snapshots.submitted_count`, `status_counts` JSON, and `users.display_peer_progress` with `default: true, null: false`. Existing snapshot rows retain null exact aggregates and fail closed until re-aggregated. | +| `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. | +| `app/api/users_api.rb`, `app/api/entities/user_entity.rb` | `PUT /users/:id`, `Entities::UserEntity` | API PR #60 | Persists and exposes the user's `display_peer_progress` opt-out. It defaults on; when false, the PPI endpoint returns no metrics. | + +### 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: + +- Compatibility submission signal: `Task.where(...).where.not(file_uploaded_at: nil)`. +- Compact completion signal: the current status is exactly `TaskStatus.complete`. +- Advanced signal: one mutually exclusive count for each of all 15 canonical statuses. +- 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. API PR #60 now exposes both meanings explicitly rather than +labelling upload presence as task completion. + +--- + +## 2. PPI field-ownership table + +Response contract as implemented in `PeerProgressApi#peer_progress_payload` on +API PR #60. It is an additive 15-field allowlist; the canonical, deployment +contract is maintained in [`docs/peer-progress-api.md`](../peer-progress-api.md). + +| 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 peer submitted percentage | Exact internal `submitted_count`, minus the viewer's upload contribution | Calculated (batch plus request-time viewer subtraction) | Quantised to the nearest 10 points over remaining peers. The stored compatibility percentage is not used to reconstruct an exact count. Null for suppressed/stale/disabled/unavailable or legacy snapshots. | API PR #60 | +| `completed_percentage` | Truthful compact peer completion percentage | Internal `status_counts['complete']`, minus the viewer if complete | Calculated | Independently quantised to 10 points over remaining peers; `nil` for suppressed/stale/disabled/unavailable states or incomplete exact snapshots. | API PR #60 | +| `status_distribution` | Advanced full lifecycle bar | Internal exact 15-key `status_counts` | Calculated | Ordered array of `{status, percentage}`. Entire vector is `null` unless above the cohort floor and every status retains at least two feasible counts after considering all buckets together. | API PR #60 | +| `distribution_available`, `distribution_unavailable_reason` | Detailed-mode availability | Distribution privacy policy and overall state | Calculated | Reasons are neutral (`privacy_protection`, `detailed_data_unavailable`, or the applicable overall reason) and never identify a sensitive category. | API PR #60 | +| `is_suppressed` | Small-cohort suppression | Computed per-request after subtracting the viewer: `peer_cohort_size < minimum_cohort_size!` (hard floor 21) | Calculated | Raw whole/peer cohort sizes are never returned. The default requires at least 22 stored active projects so 21 other students remain. Empty and small peer cohorts share the same response. Can be true with `is_stale`. | API PR #60 / PPI-S01 | +| `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 remains convenor-controlled for normal units. The demo-only `db:ppi_sample_data` task opts its synthetic `PPI1001` / `PPI1002` units in on both first run and rerun. See §5. | +| `is_user_enabled` | Whether this user wants PPI displayed | `users.display_peer_progress`, default true/non-null; settable via `PUT /users/:id` | Available | False gates all peer metrics even when the unit feature is enabled. | API PR #60 | +| `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. | +| `unavailable_reason` | Safe machine-readable compact state | `PeerProgressApi#peer_progress_result` | Calculated | One of `user_disabled`, `feature_disabled`, `target_grade_unavailable`, `snapshot_unavailable`, `insufficient_cohort`, `aggregation_incomplete`, or `stale`; `null` on compact success. | API PR #60 | + +### Fields the response must never include (confirmed by code review) + +`peer_progress_payload` is an allowlist — it only ever builds the 15 public fields. Confirmed absent: +peer names, usernames, student IDs, peer project IDs, marks, feedback, individual peer task records, +raw `status_counts`, raw `cohort_size`, and submitted/completed 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
= upload count + all 15 current statuses;
missing task = not_started"] + J --> K[("PeerProgressSnapshot row
whole cohort_size + exact submitted_count,
15-key status_counts, 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| V["PeerProgressViewerPolicy
verify viewer project/task snapshot age;
subtract viewer cohort/upload/status"] + V --> L{"remaining peers 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, all metrics null"] + N -->|no| M3["10-point compact quantisation
+ vector-wide lifecycle privacy policy"] + M3 --> M4["200 OK
submitted_percentage, completed_percentage,
optional 15-status distribution, availability metadata"] + + F1a --> O + F1b --> O + M1 --> O + M2 --> O + M4 --> O["PeerProgressIndicatorService.getIndicator
frontend adapter (PPI-F01)"] + O --> P["resolvePeerProgressState
PPI-F03 - UI state mapping"] + P --> Q["PpiWidgetComponent (f-ppi-widget)
rendered by task-dashboard
after task-submission-card"] +``` + +--- + +## 4. Safe example responses + +The canonical 15-field normal response, lifecycle order, nullability, state +reasons, preference semantics, and privacy explanation are maintained in +[`docs/peer-progress-api.md`](../peer-progress-api.md). Keeping a second JSON copy +here previously allowed the handover map to drift behind the production +contract, so this document now links to the tested source of truth. + +--- + +## 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 task adapter is live** | `PeerProgressIndicatorService.getIndicator(projectId, taskDefinitionId)` calls the authorised project/task route and maps the additive 15-field response. Unit, grade, mock state, and raw cohort values are not client-supplied. | PPI-F01 (implemented) | +| 3 | **Two distinct frontend PPI contracts** | `PeerProgressIndicator` / `PeerProgressIndicatorService` is the live task-level API adapter. `PeerProgressResponse` / `PeerProgressService` is the separate weekly burndown contract. They are intentionally not interchangeable; weekly demo fixtures remain separate from the live task request. | 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 | **Demo sample units are privacy-floor and advanced-mode ready** | Both demo tasks remain triple-guarded. `db:all_features_demo` creates 25 total students, leaving 24 peers for the demo viewer, with seven visible lifecycle states. Read-only verify uses the production viewer and public-metrics policies. `db:ppi_sample_data` provisions at least configured peer floor + 1 total and validates public metrics for every viewer/snapshot. | API PR #60 / deploy PR #12 | +| 6 | **Placeholder wording** | `unavailable_message` strings are hardcoded in Ruby, written by whoever built PPI-B01, not reviewed for tone/wording. | PPI-D01 | +| 7 | **Detailed distribution is vector-checked** | Independent 10-point status buckets can jointly reveal exact counts even though each bucket alone is ambiguous (for example, cohort 24 split 6/18). API PR #60 therefore withholds the entire vector unless every status retains at least two feasible raw counts when all buckets and a known cohort size are considered. Compact values remain independently protected. Target-grade switching remains timestamp-gated as described below. | API PR #60 / 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/test/api/collection_pagination_test.rb b/test/api/collection_pagination_test.rb new file mode 100644 index 0000000000..d3f69e5bdd --- /dev/null +++ b/test/api/collection_pagination_test.rb @@ -0,0 +1,157 @@ +require 'test_helper' + +class CollectionPaginationTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + def assert_page(path, relation, extra = {}) + expected_ids = relation.reorder(id: :asc).pluck(:id) + get path, extra.merge(page: 2, per_page: 2) + assert_equal 200, last_response.status, last_response.body + assert_kind_of Array, last_response_body + assert_equal expected_ids.drop(2).first(2), last_response_body.map { |row| row['id'] } + assert_equal expected_ids.length.to_s, last_response.headers['X-Total-Count'] + assert_equal '2', last_response.headers['X-Page'] + assert_equal '2', last_response.headers['X-Per-Page'] + assert_equal ((expected_ids.length + 1) / 2).to_s, last_response.headers['X-Total-Pages'] + end + + def test_existing_staff_lists_are_not_truncated_without_pagination + FactoryBot.create_list(:user, 51, :convenor) + add_auth_header_for(user: User.first) + + {'/api/users' => User.all, '/api/users/convenors' => User.convenors, + '/api/users/tutors' => User.tutors}.each do |path, scope| + get path + assert_equal 200, last_response.status + assert_operator scope.count, :>, 50 + assert_equal scope.pluck(:id).sort, last_response_body.map { |row| row['id'] }.sort + assert_nil last_response.headers['X-Total-Count'] + assert_page(path, scope) + end + end + + def test_public_collections_have_stable_opt_in_pages + FactoryBot.create_list(:campus, 5) + FactoryBot.create_list(:activity_type, 5) + {'/api/campuses' => Campus.all, '/api/activity_types' => ActivityType.all}.each do |path, scope| + get path + assert_equal scope.count, last_response_body.length + assert_page(path, scope) + + get path, page: 2 + assert_equal '50', last_response.headers['X-Per-Page'] + assert_equal scope.reorder(id: :asc).offset(50).pluck(:id), last_response_body.map { |row| row['id'] } + + get path, per_page: 1 + assert_equal [scope.minimum(:id)], last_response_body.map { |row| row['id'] } + assert_equal '1', last_response.headers['X-Page'] + + get path, page: 2_147_483_647, per_page: 500 + assert_equal 200, last_response.status + assert_empty last_response_body + end + end + + def test_bad_public_page_parameters_return_400_instead_of_500 + ['/api/campuses', '/api/activity_types'].each do |path| + [{page: [1]}, {per_page: [1]}, {page: {number: 1}}, {page: 'abc'}, + {page: 0}, {per_page: -1}, {per_page: 501}, {page: ''}, + {page: 2_147_483_648}].each do |parameters| + get path, parameters + assert_equal 400, last_response.status, "#{path} #{parameters.inspect}: #{last_response.body}" + end + end + end + + def test_pagination_does_not_bypass_staff_list_permissions + add_auth_header_for(user: FactoryBot.create(:user, :student)) + ['/api/users', '/api/users/convenors', '/api/users/tutors', '/api/units'].each do |path| + get path, page: 1, per_page: 2 + assert_equal 403, last_response.status, path + end + end + + def minimal_unit(**attributes) + FactoryBot.create(:unit, **{ + with_students: false, task_count: 0, tutorials: 0, + stream_count: 0, outcome_count: 0, staff_count: 0 + }.merge(attributes)) + end + + def test_large_unit_and_project_lists_keep_their_existing_filters + student = FactoryBot.create(:user, :student) + 51.times { minimal_unit.enrol_student(student, Campus.first) } + minimal_unit(active: false).enrol_student(student, Campus.first) + + add_auth_header_for(user: User.first) + get '/api/units' + assert_equal 200, last_response.status + assert_equal Unit.where(active: true).pluck(:id).sort, last_response_body.map { |row| row['id'] }.sort + assert_operator last_response_body.length, :>, 50 + assert_page('/api/units', Unit.all, include_in_active: true) + + add_auth_header_for(user: student) + get '/api/projects' + assert_equal 200, last_response.status + assert_equal 51, last_response_body.length + assert_equal Project.for_user(student, false).pluck(:id).sort, last_response_body.map { |row| row['id'] }.sort + get '/api/projects', include_inactive: true + assert_equal 52, last_response_body.length + assert_page('/api/projects', Project.for_user(student, true), include_inactive: true, include_task_definitions: true) + end + + def test_group_pages_remain_scoped_to_the_authorised_group + unit = minimal_unit(tutorials: 1) + group_set = FactoryBot.create(:group_set, unit: unit) + group = FactoryBot.create(:group, group_set: group_set) + 5.times do + project = unit.enrol_student(FactoryBot.create(:user, :student), Campus.first) + group.add_member(project) + end + path = "/api/units/#{unit.id}/group_sets/#{group_set.id}/groups/#{group.id}/members" + add_auth_header_for(user: unit.main_convenor_user) + get path + assert_equal 200, last_response.status + assert_equal group.projects.pluck(:id).sort, last_response_body.map { |row| row['id'] }.sort + assert_page(path, group.projects) + get path, per_page: [2] + assert_equal 400, last_response.status + + add_auth_header_for(user: FactoryBot.create(:user, :student)) + get path, page: 1, per_page: 2 + assert_equal 403, last_response.status + assert_nil last_response.headers['X-Total-Count'] + end + + def test_authenticated_collections_validate_page_parameters + add_auth_header_for(user: User.first) + %w[/api/users /api/users/convenors /api/users/tutors /api/units /api/projects].each do |path| + get path, per_page: [2] + assert_equal 400, last_response.status, path + get path, page: -1 + assert_equal 400, last_response.status, path + end + end + + def test_empty_project_pages_report_zero_totals + add_auth_header_for(user: FactoryBot.create(:user, :student)) + get '/api/projects', page: 1, per_page: 2 + assert_equal 200, last_response.status + assert_empty last_response_body + assert_equal '0', last_response.headers['X-Total-Count'] + assert_equal '0', last_response.headers['X-Total-Pages'] + end + + def test_browser_clients_can_read_pagination_headers + get '/api/campuses', {page: 1, per_page: 2}, {'HTTP_ORIGIN' => 'https://client.example'} + assert_equal 200, last_response.status + exposed = last_response.headers['Access-Control-Expose-Headers'].to_s.downcase + %w[x-total-count x-page x-per-page x-total-pages].each { |name| assert_includes exposed, name } + 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..7d115d0ec5 --- /dev/null +++ b/test/api/peer_progress_api_test.rb @@ -0,0 +1,1185 @@ +# 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 + completed_percentage + status_distribution + distribution_available + distribution_unavailable_reason + is_suppressed + is_stale + is_feature_enabled + is_user_enabled + last_updated_at + unavailable_reason + unavailable_message + ].freeze + + FORBIDDEN_KEYS = %w[ + cohort_size + submitted_count + status_counts + 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: 60, + cohort_size: 25, + status_counts: safe_status_counts + ) + + 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 10.0, body['completed_percentage'] + assert_equal true, body['distribution_available'] + assert_nil body['distribution_unavailable_reason'] + assert_equal PeerProgressDistributionPolicy::STATUS_KEYS, + body['status_distribution'].pluck('status') + assert_equal false, body['is_suppressed'] + assert_equal false, body['is_stale'] + assert_equal true, body['is_feature_enabled'] + assert_equal true, body['is_user_enabled'] + assert body['last_updated_at'].present? + assert_nil body['unavailable_reason'] + assert_equal '', body['unavailable_message'] + end + + test 'excludes a submitted complete viewer before compact and detailed output' do + viewer_task = create( + :task, + project: @project, + task_definition: @task_definition, + task_status: TaskStatus.complete, + file_uploaded_at: 2.hours.ago, + submission_date: 2.hours.ago + ) + calculated_at = viewer_task.updated_at + 1.minute + create( + :peer_progress_snapshot, + unit: @unit, + task_definition: @task_definition, + target_grade: @project.target_grade, + cohort_size: 22, + submitted_count: 1, + submitted_percentage: 4.55, + status_counts: empty_status_counts.merge( + 'not_started' => 21, + 'complete' => 1 + ), + calculated_at: calculated_at + ) + + request_as(@student) + + body = last_response_body + assert_equal 200, last_response.status + assert_equal 0.0, body['submitted_percentage'] + assert_equal 0.0, body['completed_percentage'] + assert_equal true, body['distribution_available'] + assert_equal 100.0, + distribution_percentage(body, 'not_started') + assert_equal 0.0, + distribution_percentage(body, 'complete') + end + + test 'excludes an unsubmitted viewer from a fully complete peer cohort' do + create( + :peer_progress_snapshot, + unit: @unit, + task_definition: @task_definition, + target_grade: @project.target_grade, + cohort_size: 22, + submitted_count: 21, + submitted_percentage: 95.45, + status_counts: empty_status_counts.merge( + 'not_started' => 1, + 'complete' => 21 + ), + calculated_at: Time.zone.now + ) + + request_as(@student) + + body = last_response_body + assert_equal 200, last_response.status + assert_equal 100.0, body['submitted_percentage'] + assert_equal 100.0, body['completed_percentage'] + assert_equal true, body['distribution_available'] + assert_equal 0.0, + distribution_percentage(body, 'not_started') + assert_equal 100.0, + distribution_percentage(body, 'complete') + end + + test 'requires twenty one remaining peers rather than counting the viewer' do + create_snapshot( + submitted_percentage: 50, + cohort_size: 20 + ) + + request_as(@student) + + assert_equal 200, last_response.status + assert_equal true, last_response_body['is_suppressed'] + assert_nil last_response_body['submitted_percentage'] + end + + test 'fails closed when the viewer task changed after aggregation' do + calculated_at = 1.hour.ago + create_snapshot( + submitted_percentage: 50, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE, + calculated_at: calculated_at + ) + create( + :task, + project: @project, + task_definition: @task_definition, + task_status: TaskStatus.complete, + updated_at: calculated_at + 1.minute + ) + + request_as(@student) + + body = last_response_body + assert_equal 200, last_response.status + assert_nil body['submitted_percentage'] + assert_nil body['completed_percentage'] + assert_nil body['status_distribution'] + assert_equal 'snapshot_unavailable', body['unavailable_reason'] + end + + test 'fails closed when the viewer re-enrolled after aggregation' do + calculated_at = 1.hour.ago + create_snapshot( + submitted_percentage: 50, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE, + calculated_at: calculated_at + ) + @project.update!(enrolled: false) + @project.update!(enrolled: true) + + request_as(@student) + + body = last_response_body + assert_equal 200, last_response.status + assert_nil body['submitted_percentage'] + assert_nil body['completed_percentage'] + assert_equal 'snapshot_unavailable', body['unavailable_reason'] + end + + test 'fails closed when a legacy snapshot has no exact submitted count' do + create_snapshot( + submitted_percentage: 50, + cohort_size: PeerProgressApi::MINIMUM_SAFE_COHORT_SIZE, + submitted_count: nil + ) + + request_as(@student) + + body = last_response_body + assert_equal 200, last_response.status + assert_nil body['submitted_percentage'] + assert_nil body['completed_percentage'] + assert_nil body['status_distribution'] + assert_equal 'aggregation_incomplete', body['unavailable_reason'] + end + + test 'suppresses a detailed vector that jointly reveals exact counts' do + status_counts = empty_status_counts.merge( + 'not_started' => 6, + 'complete' => 18 + ) + create_snapshot( + submitted_percentage: 75, + cohort_size: 24, + status_counts: status_counts + ) + + request_as(@student) + + body = last_response_body + assert_equal 200, last_response.status + assert_equal 80.0, body['submitted_percentage'] + assert_equal 80.0, body['completed_percentage'] + assert_nil body['status_distribution'] + assert_equal false, body['distribution_available'] + assert_equal 'privacy_protection', + body['distribution_unavailable_reason'] + assert_nil body['unavailable_reason'] + end + + test 'honours a students disabled peer progress preference' do + @student.update!(display_peer_progress: false) + create_snapshot( + submitted_percentage: 60, + cohort_size: 25, + status_counts: safe_status_counts + ) + + request_as(@student) + + body = last_response_body + assert_equal 200, last_response.status + assert_nil body['submitted_percentage'] + assert_nil body['completed_percentage'] + assert_nil body['status_distribution'] + assert_equal false, body['distribution_available'] + assert_equal false, body['is_user_enabled'] + assert_equal 'user_disabled', body['unavailable_reason'] + assert_equal 'user_disabled', + body['distribution_unavailable_reason'] + 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, + status_counts: :default, + submitted_count: :default + ) + @project.update!(updated_at: calculated_at - 1.second) if + @project.updated_at > calculated_at + + peer_status_counts = if status_counts == :default + empty_status_counts.merge( + 'not_started' => cohort_size + ) + else + status_counts + end + stored_status_counts = peer_status_counts&.dup + if stored_status_counts + stored_status_counts['not_started'] += 1 + end + + peer_submitted_count = if submitted_count == :default + if submitted_percentage.nil? + nil + else + ((submitted_percentage * cohort_size) / 100.0).round + end + else + submitted_count + end + stored_submitted_count = peer_submitted_count + stored_percentage = submitted_percentage + unless stored_submitted_count.nil? + stored_percentage = ((stored_submitted_count * 100.0) / + (cohort_size + 1)).round(2) + end + + create( + :peer_progress_snapshot, + unit: @unit, + task_definition: @task_definition, + target_grade: target_grade, + submitted_percentage: stored_percentage, + submitted_count: stored_submitted_count, + cohort_size: cohort_size + 1, + status_counts: stored_status_counts, + calculated_at: calculated_at + ) + end + + def empty_status_counts + PeerProgressDistributionPolicy::STATUS_KEYS.index_with { 0 } + end + + def safe_status_counts + empty_status_counts.merge( + 'not_started' => 5, + 'working_on_it' => 5, + 'ready_for_feedback' => 4, + 'fix_and_resubmit' => 3, + 'redo' => 3, + 'complete' => 3, + 'fail' => 2 + ) + 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 distribution_percentage(body, status) + body.fetch('status_distribution').find do |entry| + entry.fetch('status') == status + end.fetch('percentage') + 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 + + assert( + body['completed_percentage'].nil? || + body['completed_percentage'].is_a?(Numeric), + 'completed_percentage must be numeric or null' + ) + + unless body['completed_percentage'].nil? + assert_operator body['completed_percentage'], :>=, 0.0 + assert_operator body['completed_percentage'], :<=, 100.0 + end + + if body['status_distribution'].nil? + assert_equal false, body['distribution_available'] + else + assert_equal true, body['distribution_available'] + assert_equal PeerProgressDistributionPolicy::STATUS_KEYS, + body['status_distribution'].pluck('status') + + body['status_distribution'].each do |entry| + assert_json_limit_keys_to_exactly %w[status percentage], entry + assert_kind_of String, entry['status'] + assert_kind_of Numeric, entry['percentage'] + assert_operator entry['percentage'], :>=, 0.0 + assert_operator entry['percentage'], :<=, 100.0 + assert_equal 0.0, + entry['percentage'] % + PeerProgressApi::PERCENTAGE_BUCKET_SIZE + end + end + assert( + body['distribution_unavailable_reason'].nil? || + body['distribution_unavailable_reason'].is_a?(String), + 'distribution_unavailable_reason must be a string or null' + ) + + %w[ + is_suppressed + is_stale + is_feature_enabled + is_user_enabled + distribution_available + ].each do |key| + assert_includes( + [true, false], + body.fetch(key), + "#{key} must be a boolean" + ) + end + + assert( + body['unavailable_reason'].nil? || + body['unavailable_reason'].is_a?(String), + 'unavailable_reason must be a string or null' + ) + + 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/projects_api_test.rb b/test/api/projects_api_test.rb index 30407838e6..330dc81576 100644 --- a/test/api/projects_api_test.rb +++ b/test/api/projects_api_test.rb @@ -62,13 +62,281 @@ def test_projects_returns_correct_data assert_json_limit_keys_to_exactly keys, data - assert_json_matches_model(project, data, %w(campus_id target_grade campus_id)) - assert_json_matches_model(project.unit, data['unit'], %w(id code name active)) + assert_json_matches_model(project, data, %w[campus_id target_grade campus_id]) + assert_json_matches_model(project.unit, data['unit'], %w[id code name active]) assert_json_matches_model project, data, key_test end end + def test_projects_with_task_definitions_uses_student_safe_serialization + unit = FactoryBot.create( + :unit, + with_students: false, + task_count: 0, + allow_flexible_dates: true + ) + common_start_date = unit.start_date + 1.week + later_task = FactoryBot.create( + :task_definition, + unit: unit, + abbreviation: 'CROSS-Z', + start_date: common_start_date, + plagiarism_report_url: 'https://staff.invalid/report', + plagiarism_warn_pct: 99, + tii_group_id: 'staff-only-group', + similarity_language: 'staff-only-language', + use_resources_for_jplag_base_code: true, + lock_assessments_to_tutorial_stream: true, + upload_requirements: [ + { + 'key' => 'file0', + 'name' => 'Student report', + 'type' => 'document', + 'tii_check' => true, + 'tii_pct' => 35 + } + ] + ) + earlier_task = FactoryBot.create( + :task_definition, + unit: unit, + abbreviation: 'CROSS-A', + start_date: common_start_date + ) + grade_due_date = FactoryBot.create( + :task_definition_grade_due_date, + task_definition: later_task, + target_grade: 1, + target_due_date: later_task.target_date + 2.days, + start_date: later_task.start_date + 1.day + ) + student = FactoryBot.create(:user, :student) + unit.enrol_student(student, unit.tutorials.first.campus) + + add_auth_header_for(user: student) + + get '/api/projects' + assert_equal 200, last_response.status, last_response_body + assert_not last_response_body.first.key?('tasks') + assert_not last_response_body.first.fetch('unit').key?('task_definitions') + + get '/api/projects?include_task_definitions=true' + assert_equal 200, last_response.status, last_response_body + + project_data = last_response_body.first + assert project_data.key?('tasks') + unit_data = project_data.fetch('unit') + assert_equal true, unit_data.fetch('allow_flexible_dates') + + task_definitions = unit_data.fetch('task_definitions') + assert_equal [earlier_task.id, later_task.id], task_definitions.pluck('id') + + task_definitions.each do |task_definition| + %w[id abbreviation name description weighting target_grade upload_requirements].each do |key| + assert task_definition.key?(key), "Expected student-safe task definition to include #{key}" + end + + %w[ + plagiarism_report_url plagiarism_warn_pct tii_group_id similarity_language + overseer_image_id use_resources_for_jplag_base_code + lock_assessments_to_tutorial_stream restrict_status_updates created_at updated_at + ].each do |key| + assert_not task_definition.key?(key), "Student response exposed staff-only field #{key}" + end + end + + student_requirements = task_definitions.find do |task_definition| + task_definition['id'] == later_task.id + end.fetch('upload_requirements') + assert_equal( + [{ 'key' => 'file0', 'name' => 'Student report', 'type' => 'document' }], + student_requirements + ) + + student_later_task = task_definitions.find do |task_definition| + task_definition['id'] == later_task.id + end + grade_due_dates = student_later_task.fetch('grade_due_dates') + assert_equal 1, grade_due_dates.length + assert_equal grade_due_date.target_grade, grade_due_dates.first.fetch('target_grade') + assert_equal grade_due_date.target_due_date.to_date, + Date.parse(grade_due_dates.first.fetch('target_due_date')) + assert_equal grade_due_date.start_date.to_date, + Date.parse(grade_due_dates.first.fetch('start_date')) + end + + def test_projects_payload_audit_distinguishes_summary_and_dashboard_without_content + project = FactoryBot.create(:project) + add_auth_header_for(user: project.student) + lines = [] + Rails.logger.stub(:info, ->(line = nil, *) { lines << line }) do + get '/api/projects' + assert_equal 200, last_response.status + get '/api/projects?include_task_definitions=true&include_inactive=true' + assert_equal 200, last_response.status + end + events = lines.filter_map do |line| + JSON.parse(line) if line.is_a?(String) && line.start_with?('{') + end + events.select! { |line| line['event'] == 'projects.index' } + assert_equal 2, events.size + assert_equal false, events.first['include_task_definitions'] + assert_equal 0, events.first['task_definition_count'] + assert_equal true, events.last['include_task_definitions'] + assert_equal true, events.last['include_inactive'] + assert_equal project.student.id, events.last['user_id'] + assert events.last['task_definition_count'].positive? + assert_not_includes events.to_json, project.student.email + end + + def test_unauthenticated_projects_request_has_no_payload_success_audit + lines = [] + Rails.logger.stub(:info, ->(line = nil, *) { lines << line }) do + get '/api/projects?include_task_definitions=true' + assert_equal 419, last_response.status + end + assert_not(lines.any? { |line| line.to_s.include?('projects.index') }) + end + + def test_projects_with_task_definitions_exposes_privacy_safe_feedback_state + project = FactoryBot.create(:project) + unit = project.unit + task_definition = unit.task_definitions.first + task = project.task_for_task_definition(task_definition) + student = project.student + tutor = unit.main_convenor_user + + task.update!(task_status: TaskStatus.ready_for_feedback) + task.add_status_comment(student, TaskStatus.ready_for_feedback) + + add_auth_header_for(user: student) + + get '/api/projects?include_task_definitions=true' + assert_equal 200, last_response.status, last_response_body + + task_data = lambda do + last_response_body + .find { |data| data['id'] == project.id } + .fetch('tasks') + .find { |data| data['id'] == task.id } + end + + assert_equal false, task_data.call.fetch('has_feedback') + + task.add_text_comment(student, 'Student follow-up') + task.add_text_comment(tutor, '**Automated Message:** Automated feedback') + + get '/api/projects?include_task_definitions=true' + assert_equal 200, last_response.status, last_response_body + assert_equal false, task_data.call.fetch('has_feedback') + + task.add_text_comment(tutor, 'Manual tutor feedback') + + get '/api/projects?include_task_definitions=true' + assert_equal 200, last_response.status, last_response_body + + response_task = task_data.call + assert_equal true, response_task.fetch('has_feedback') + + %w[ + feedback feedback_text marker_notes feedback_author + last_feedback_at has_unread_feedback + ].each do |key| + assert_not response_task.key?(key), "Student response exposed #{key}" + end + + assert_not_includes last_response.body, 'Manual tutor feedback' + assert_not_includes last_response.body, '**Automated Message:** Automated feedback' + end + + def test_projects_feedback_state_is_scoped_to_authenticated_student + unit = FactoryBot.create( + :unit, + with_students: false, + task_count: 1, + tutorials: 1 + ) + + student = FactoryBot.create(:user, :student) + other_student = FactoryBot.create(:user, :student) + + project = unit.enrol_student(student, unit.tutorials.first.campus) + other_project = unit.enrol_student(other_student, unit.tutorials.first.campus) + + task_definition = unit.task_definitions.first + project.task_for_task_definition(task_definition) + other_task = other_project.task_for_task_definition(task_definition) + + other_task.update!(task_status: TaskStatus.ready_for_feedback) + other_task.add_status_comment(other_student, TaskStatus.ready_for_feedback) + other_task.add_text_comment(unit.main_convenor_user, 'Private feedback for other student') + + add_auth_header_for(user: student) + + get '/api/projects?include_task_definitions=true' + assert_equal 200, last_response.status, last_response_body + + returned_project_ids = last_response_body.pluck('id') + + assert_includes returned_project_ids, project.id + assert_not_includes returned_project_ids, other_project.id + assert_not_includes last_response.body, 'Private feedback for other student' + + get "/api/projects/#{other_project.id}" + + assert_equal 403, last_response.status + assert_not_includes last_response.body, 'Private feedback for other student' + end + + def test_projects_with_inactive_task_definitions_avoids_per_record_queries + student = FactoryBot.create(:user, :student) + units = 2.times.map do + unit = FactoryBot.create( + :unit, + with_students: false, + task_count: 4, + tutorials: 1, + outcome_count: 0, + active: true + ) + project = unit.enrol_student(student, unit.tutorials.first.campus) + unit.task_definitions.each do |task_definition| + project.task_for_task_definition(task_definition) + end + unit + end + units.last.update!(active: false) + add_auth_header_for(user: student) + + query_count = 0 + count_query = lambda do |_name, _started, _finished, _unique_id, payload| + next if payload[:cached] || %w[SCHEMA TRANSACTION].include?(payload[:name]) + + query_count += 1 + end + + [false, true].each do |flexible_dates| + units.each { |unit| unit.update!(allow_flexible_dates: flexible_dates) } + query_count = 0 + ActiveSupport::Notifications.subscribed(count_query, 'sql.active_record') do + get '/api/projects?include_inactive=true&include_task_definitions=true' + end + + assert_equal 200, last_response.status, last_response_body + assert_equal 2, last_response_body.length + active_states = last_response_body.pluck('unit').pluck('active') + assert_equal [false, true], (active_states.sort_by { |active| active ? 1 : 0 }) + assert_equal 8, (last_response_body.sum { |project| project.fetch('tasks').length }) + task_definition_count = last_response_body.sum do |project| + project.fetch('unit').fetch('task_definitions').length + end + assert_equal 8, task_definition_count + assert_operator query_count, :<=, 45, + "Expected a bounded project query graph, got #{query_count} SQL queries (flexible: #{flexible_dates})" + end + end + def test_get_project_response_is_correct user = FactoryBot.create(:user, :student, enrol_in: 1) project = user.projects.first @@ -107,8 +375,8 @@ def test_projects_works_with_inactive_units project = user.projects.find(data['id']) assert project.present?, data.inspect - assert_json_matches_model(project, data, %w(campus_id target_grade campus_id)) - assert_json_matches_model(project.unit, data['unit'], %w(code id name active)) + assert_json_matches_model(project, data, %w[campus_id target_grade campus_id]) + assert_json_matches_model(project.unit, data['unit'], %w[code id name active]) end end @@ -129,7 +397,7 @@ def test_submitted_grade_cant_change_after_submission assert_equal 200, last_response.status, last_response_body assert_equal user.projects.find(project.id).submitted_grade, 2 - keys = %w(campus_id target_grade submitted_grade compile_portfolio portfolio_available uses_draft_learning_summary) + keys = %w[campus_id target_grade submitted_grade compile_portfolio portfolio_available uses_draft_learning_summary] assert_json_limit_keys_to_exactly keys, last_response_body assert_json_matches_model project, last_response_body, keys diff --git a/test/api/task_prioritization_api_test.rb b/test/api/task_prioritization_api_test.rb new file mode 100644 index 0000000000..52bcaede71 --- /dev/null +++ b/test/api/task_prioritization_api_test.rb @@ -0,0 +1,484 @@ +# frozen_string_literal: true + +require 'test_helper' + +class TaskPrioritizationApiTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + setup do + clear_auth_header + @today = Time.zone.parse('2026-08-24 10:00:00 UTC') + end + + teardown do + clear_auth_header + end + + test 'requires authentication' do + get endpoint + + assert_equal 419, last_response.status + end + + test 'recommends assigned definitions even before task rows exist' do + travel_to @today do + unit = create_unit + later_definition = create_task_definition(unit, name: 'Later task', target_date: 12.days.from_now) + urgent_definition = create_task_definition(unit, name: 'Urgent task', target_date: 2.days.from_now) + student = create(:user, :student) + project = enrol_student(unit, student, target_grade: 0) + + assert_empty project.tasks + + assert_no_difference 'Task.count' do + request_as(student) + end + + assert_equal 200, last_response.status, last_response.body + body = last_response_body + assert_equal [urgent_definition.id, later_definition.id], body['data'].pluck('task_definition_id') + assert(body['data'].all? { |recommendation| recommendation['task_id'].nil? }) + assert_equal %w[ + task_id + task_definition_id + task_name + project_id + unit_id + priority_score + ], body['data'].first.keys + assert_equal( + { + 'page' => 1, + 'per_page' => TaskPrioritizationApi::DEFAULT_PER_PAGE, + 'total_count' => 2, + 'total_pages' => 1 + }, + body['meta'] + ) + end + end + + test 'uses flexible grade dates for assigned definitions without task rows' do + travel_to @today do + unit = create_unit(allow_flexible_dates: true) + base_earlier_definition = create_task_definition( + unit, + name: 'Base earlier task', + target_date: 2.days.from_now + ) + base_later_definition = create_task_definition( + unit, + name: 'Base later task', + target_date: 12.days.from_now + ) + create_grade_due_date(base_earlier_definition, target_grade: 1, target_due_date: 20.days.from_now) + create_grade_due_date(base_later_definition, target_grade: 1, target_due_date: 1.day.from_now) + student = create(:user, :student) + project = enrol_student(unit, student, target_grade: 1) + + assert_empty project.tasks + + request_as(student) + + assert_equal [base_later_definition.id, base_earlier_definition.id], + last_response_body['data'].pluck('task_definition_id') + assert_empty project.tasks.reload + end + end + + test 'uses personalized local due dates for materialized tasks' do + travel_to @today do + unit = create_unit(allow_flexible_dates: true) + base_earlier_definition = create_task_definition( + unit, + name: 'Base earlier task', + target_date: 1.day.from_now + ) + base_later_definition = create_task_definition( + unit, + name: 'Base later task', + target_date: 20.days.from_now + ) + student = create(:user, :student) + project = enrol_student(unit, student, target_grade: 0) + base_earlier_task = project.task_for_task_definition(base_earlier_definition) + base_later_task = project.task_for_task_definition(base_later_definition) + + base_earlier_task.update!(target_due_date: 20.days.from_now) + base_later_task.update!(target_due_date: 1.day.from_now) + + request_as(student) + + assert_equal [base_later_definition.id, base_earlier_definition.id], + last_response_body['data'].pluck('task_definition_id') + assert_operator last_response_body['data'].first['priority_score'], + :>, + last_response_body['data'].last['priority_score'] + end + end + + test 'uses extension-adjusted due dates for materialized tasks' do + travel_to @today do + unit = create_unit + extended_definition = create_task_definition( + unit, + name: 'Extended task', + target_date: 1.day.from_now + ) + nearer_definition = create_task_definition( + unit, + name: 'Nearer task', + target_date: 7.days.from_now + ) + student = create(:user, :student) + project = enrol_student(unit, student, target_grade: 0) + extended_task = project.task_for_task_definition(extended_definition) + project.task_for_task_definition(nearer_definition) + + extended_task.update!(extensions: 2) + + request_as(student) + + assert_equal [nearer_definition.id, extended_definition.id], + last_response_body['data'].pluck('task_definition_id') + end + end + + test 'uses task-specific deadline workload and relative size in the ranking' do + travel_to @today do + unit = create_unit + early_definition = create_task_definition( + unit, + name: 'Small early task', + target_date: 5.days.from_now, + weighting: 1 + ) + clustered_small_definition = create_task_definition( + unit, + name: 'Small clustered task', + target_date: 6.days.from_now, + weighting: 1 + ) + clustered_large_definition = create_task_definition( + unit, + name: 'Large clustered task', + target_date: 6.days.from_now, + weighting: 8 + ) + student = create(:user, :student) + enrol_student(unit, student, target_grade: 0) + + request_as(student) + + returned_ids = last_response_body['data'].pluck('task_definition_id') + assert_equal clustered_large_definition.id, returned_ids.first + assert_operator returned_ids.index(clustered_small_definition.id), :<, returned_ids.index(early_definition.id) + end + end + + test 'completed work lowers workload without inflating the remaining task size' do + travel_to @today do + unit = create_unit + remaining_definition = create_task_definition( + unit, + name: 'Remaining task', + target_date: 7.days.from_now, + weighting: 1 + ) + completed_definition = create_task_definition( + unit, + name: 'Task to complete', + target_date: 7.days.from_now, + weighting: 1 + ) + student = create(:user, :student) + project = enrol_student(unit, student, target_grade: 0) + + request_as(student) + score_before_completion = score_for(last_response_body['data'], remaining_definition) + + project.task_for_task_definition(completed_definition).update!(task_status: TaskStatus.complete) + request_as(student) + score_after_completion = score_for(last_response_body['data'], remaining_definition) + + assert_operator score_after_completion, :<, score_before_completion + end + end + + test 'does not recommend a dependent until its prerequisite reaches the required status' do + travel_to @today do + unit = create_unit + prerequisite_definition = create_task_definition(unit, name: 'Prerequisite') + dependent_definition = create_task_definition(unit, name: 'Dependent') + TaskPrerequisite.create!( + task_definition: dependent_definition, + prerequisite: prerequisite_definition, + task_status_id: TaskStatus.complete.id + ) + student = create(:user, :student) + project = enrol_student(unit, student, target_grade: 0) + + request_as(student) + assert_equal [prerequisite_definition.id], last_response_body['data'].pluck('task_definition_id') + + prerequisite_task = project.task_for_task_definition(prerequisite_definition) + prerequisite_task.update!(task_status: TaskStatus.ready_for_feedback) + request_as(student) + assert_empty last_response_body['data'] + + prerequisite_task.update!(task_status: TaskStatus.complete) + request_as(student) + assert_equal [dependent_definition.id], last_response_body['data'].pluck('task_definition_id') + end + end + + test 'keeps attention required blocked to match submission authorization' do + travel_to @today do + unit = create_unit + prerequisite_definition = create_task_definition(unit, name: 'Attention prerequisite') + dependent_definition = create_task_definition(unit, name: 'Attention dependent') + create_prerequisite( + dependent_definition, + prerequisite_definition, + required_status: TaskStatus.attention_required + ) + student = create(:user, :student) + project = enrol_student(unit, student, target_grade: 0) + project + .task_for_task_definition(prerequisite_definition) + .update!(task_status: TaskStatus.attention_required) + + request_as(student) + + assert_not_includes last_response_body['data'].pluck('task_definition_id'), dependent_definition.id + end + end + + test 'accepts rediscuss for a discussion-level prerequisite' do + travel_to @today do + unit = create_unit + prerequisite_definition = create_task_definition(unit, name: 'Discussion prerequisite') + dependent_definition = create_task_definition(unit, name: 'Discussion dependent') + create_prerequisite( + dependent_definition, + prerequisite_definition, + required_status: TaskStatus.discuss + ) + student = create(:user, :student) + project = enrol_student(unit, student, target_grade: 0) + project + .task_for_task_definition(prerequisite_definition) + .update!(task_status: TaskStatus.rediscuss) + + request_as(student) + + assert_includes last_response_body['data'].pluck('task_definition_id'), dependent_definition.id + end + end + + test 'keeps overdue and future priority scores within the zero to one hundred contract' do + travel_to @today do + unit = create_unit + overdue_definition = create_task_definition(unit, name: 'Overdue task', target_date: 40.days.ago) + future_definition = create_task_definition(unit, name: 'Future task', target_date: 7.days.from_now) + student = create(:user, :student) + enrol_student(unit, student, target_grade: 0) + + request_as(student) + + recommendations = last_response_body['data'] + scores = recommendations.pluck('priority_score') + assert(scores.all? { |score| score.between?(0, 100) }) + assert_operator score_for(recommendations, overdue_definition), + :>, + score_for(recommendations, future_definition) + end + end + + test 'only returns eligible unfinished work owned by the authenticated student' do + travel_to @today do + active_unit = create_unit + open_definition = create_task_definition(active_unit, name: 'Open task', target_grade: 0) + higher_grade_definition = create_task_definition(active_unit, name: 'Higher grade task', target_grade: 3) + excluded_definitions = non_actionable_statuses.each_with_index.to_h do |status, index| + definition = create_task_definition(active_unit, name: "Non-actionable task #{index}", target_grade: 0) + [definition, status] + end + student = create(:user, :student) + project = enrol_student(active_unit, student, target_grade: 0) + excluded_definitions.each do |definition, status| + project.task_for_task_definition(definition).update!(task_status: status) + end + + other_student = create(:user, :student) + enrol_student(active_unit, other_student, target_grade: 3) + + inactive_unit = create_unit(active: false) + inactive_definition = create_task_definition(inactive_unit, name: 'Inactive task') + enrol_student(inactive_unit, student, target_grade: 0) + + withdrawn_unit = create_unit + withdrawn_definition = create_task_definition(withdrawn_unit, name: 'Withdrawn task') + withdrawn_project = enrol_student(withdrawn_unit, student, target_grade: 0) + withdrawn_project.update!(enrolled: false) + + request_as(student) + + returned_ids = last_response_body['data'].pluck('task_definition_id') + assert_equal [open_definition.id], returned_ids + assert_not_includes returned_ids, higher_grade_definition.id + assert_not_includes returned_ids, inactive_definition.id + assert_not_includes returned_ids, withdrawn_definition.id + excluded_definitions.each_key do |definition| + assert_not_includes returned_ids, definition.id + end + end + end + + test 'paginates every recommendation without overlap' do + travel_to @today do + unit = create_unit + definitions = 3.times.map do |index| + create_task_definition( + unit, + name: "Task #{index}", + target_date: (index + 1).days.from_now + ) + end + student = create(:user, :student) + enrol_student(unit, student, target_grade: 0) + + add_auth_header_for(user: student) + get endpoint, page: 1, per_page: 2 + first_page = last_response_body + + get endpoint, page: 2, per_page: 2 + second_page = last_response_body + + returned_ids = first_page['data'].pluck('task_definition_id') + + second_page['data'].pluck('task_definition_id') + assert_equal definitions.map(&:id).sort, returned_ids.sort + assert_equal 2, first_page['data'].length + assert_equal 1, second_page['data'].length + assert_equal( + { + 'page' => 1, + 'per_page' => 2, + 'total_count' => 3, + 'total_pages' => 2 + }, + first_page['meta'] + ) + assert_empty first_page['data'].pluck('task_definition_id') & + second_page['data'].pluck('task_definition_id') + end + end + + test 'uses project and task definition ids as deterministic tie breakers' do + travel_to @today do + unit = create_unit + definitions = 2.times.map do |index| + create_task_definition( + unit, + name: "Equal task #{index}", + target_date: 5.days.from_now, + weighting: 1 + ) + end + student = create(:user, :student) + enrol_student(unit, student, target_grade: 0) + + request_as(student) + + assert_equal definitions.map(&:id).sort, last_response_body['data'].pluck('task_definition_id') + end + end + + private + + def endpoint + '/api/tasks/recommended' + end + + def request_as(user) + add_auth_header_for(user: user) + get endpoint + end + + def non_actionable_statuses + [ + TaskStatus.complete, + TaskStatus.fail, + TaskStatus.feedback_exceeded, + TaskStatus.time_exceeded, + TaskStatus.assess_in_portfolio, + TaskStatus.ready_for_feedback + ] + end + + def create_unit(active: true, allow_flexible_dates: false) + create( + :unit, + with_students: false, + task_count: 0, + staff_count: 0, + outcome_count: 0, + active: active, + allow_flexible_dates: allow_flexible_dates, + start_date: @today - 30.days, + end_date: @today + 90.days + ) + end + + def create_task_definition( + unit, + name:, + target_date: @today + 7.days, + target_grade: 0, + weighting: 1 + ) + create( + :task_definition, + unit: unit, + name: name, + start_date: @today - 7.days, + target_date: target_date, + due_date: @today + 60.days, + target_grade: target_grade, + weighting: weighting, + outcome_count: 0 + ) + end + + def create_grade_due_date(task_definition, target_grade:, target_due_date:) + create( + :task_definition_grade_due_date, + task_definition: task_definition, + target_grade: target_grade, + target_due_date: target_due_date, + start_date: task_definition.start_date + ) + end + + def create_prerequisite(task_definition, prerequisite, required_status:) + TaskPrerequisite.create!( + task_definition: task_definition, + prerequisite: prerequisite, + task_status_id: required_status.id + ) + end + + def score_for(recommendations, task_definition) + recommendations.find do |recommendation| + recommendation['task_definition_id'] == task_definition.id + end.fetch('priority_score') + end + + def enrol_student(unit, student, target_grade:) + project = unit.enrol_student(student, unit.tutorials.first&.campus) + project.update!(target_grade: target_grade) + project + 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..9f25db3d9f --- /dev/null +++ b/test/models/peer_progress_snapshot_test.rb @@ -0,0 +1,300 @@ +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 'accepts an exact submitted count within the cohort' do + snapshot = build_snapshot( + submitted_count: 4, + cohort_size: 10 + ) + + assert snapshot.valid? + end + + test 'rejects an invalid exact submitted count' do + negative = build_snapshot(submitted_count: -1) + decimal = build_snapshot(submitted_count: 1.5) + above_cohort = build_snapshot( + submitted_count: 11, + cohort_size: 10 + ) + + assert_not negative.valid? + assert_not decimal.valid? + assert_not above_cohort.valid? + end + + test 'accepts complete internal status counts that sum to the cohort' do + snapshot = build_snapshot( + cohort_size: 10, + status_counts: empty_status_counts.merge( + 'not_started' => 6, + 'complete' => 4 + ) + ) + + assert snapshot.valid?, snapshot.errors.full_messages.to_sentence + end + + test 'persists lifecycle JSON as a hash on MariaDB compatible text columns' do + counts = empty_status_counts.merge('not_started' => 10) + snapshot = create( + :peer_progress_snapshot, + unit: @unit, + task_definition: @task_definition, + cohort_size: 10, + submitted_count: 0, + status_counts: counts + ) + snapshot.reload + + assert_instance_of Hash, snapshot.status_counts + assert_equal counts, snapshot.status_counts + assert_equal 0, snapshot.submitted_count + end + + test 'rejects incomplete invalid or inconsistent internal status counts' do + missing = build_snapshot( + status_counts: empty_status_counts.except('redo') + ) + negative = build_snapshot( + status_counts: empty_status_counts.merge( + 'not_started' => 11, + 'redo' => -1 + ) + ) + wrong_total = build_snapshot( + status_counts: empty_status_counts.merge('not_started' => 9) + ) + + assert_not missing.valid? + assert_not negative.valid? + assert_not wrong_total.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 + + def empty_status_counts + PeerProgressDistributionPolicy::STATUS_KEYS.index_with { 0 } + end +end diff --git a/test/models/project_target_grade_changed_at_test.rb b/test/models/project_target_grade_changed_at_test.rb new file mode 100644 index 0000000000..3b20568da0 --- /dev/null +++ b/test/models/project_target_grade_changed_at_test.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'test_helper' +require Rails.root.join('db/migrate/20260824000002_ensure_target_grade_changed_at_default') + +class ProjectTargetGradeChangedAtTest < Minitest::Test + def teardown + Project.where(status: @insert_marker).delete_all if @insert_marker + EnsureTargetGradeChangedAtDefault.new.up + Project.reset_column_information + end + + def test_database_default_supports_old_writers + inserted_at = Time.current + @insert_marker = "target-grade-default-regression-#{object_id}" + + # Bypass Project's before_create callback on purpose. This matches an older + # application instance that does not know about target_grade_changed_at. + # rubocop:disable Rails/SkipsModelValidations + Project.insert_all!( + [ + { + status: @insert_marker, + created_at: inserted_at, + updated_at: inserted_at + } + ] + ) + # rubocop:enable Rails/SkipsModelValidations + + project = Project.find_by!(status: @insert_marker) + assert project.target_grade_changed_at + assert_operator project.target_grade_changed_at, :>=, inserted_at - 1.second + end + + def test_follow_up_migration_repairs_a_missing_default_and_is_idempotent + migration = EnsureTargetGradeChangedAtDefault.new + migration.change_column_default :projects, :target_grade_changed_at, nil + + assert_nil target_grade_changed_at_column.default_function + + migration.up + assert_current_timestamp_default + + migration.up + assert_current_timestamp_default + end + + private + + def assert_current_timestamp_default + value = target_grade_changed_at_column.default_function.to_s.delete(' ') + assert_match(/\Acurrent_timestamp(?:\(\d*\))?\z/i, value) + end + + def target_grade_changed_at_column + ActiveRecord::Base.connection.columns(:projects).find do |column| + column.name == 'target_grade_changed_at' + end + 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..fc3a009266 --- /dev/null +++ b/test/services/peer_progress_aggregation_service_test.rb @@ -0,0 +1,538 @@ +# 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 3, snapshot.submitted_count + assert_equal 75.0, snapshot.submitted_percentage.to_f + assert_equal 1, snapshot.status_counts.fetch('not_started') + assert_equal 3, + snapshot.status_counts.fetch('ready_for_feedback') + assert_equal PeerProgressDistributionPolicy::STATUS_KEYS.sort, + snapshot.status_counts.keys.sort + assert_equal 4, snapshot.status_counts.values.sum + assert_equal @calculated_at, snapshot.calculated_at + end + + def test_aggregates_every_canonical_task_status + projects = create_list( + :project, + PeerProgressDistributionPolicy::STATUS_KEYS.length, + unit: @unit, + target_grade: 0, + enrolled: true + ) + + projects.each_with_index do |project, index| + create( + :task, + project: project, + task_definition: @pass_task, + task_status: TaskStatus.find(index + 1) + ) + end + + run_service + + snapshot = find_snapshot( + task_definition: @pass_task, + target_grade: 0 + ) + + assert_equal( + PeerProgressDistributionPolicy::STATUS_KEYS.index_with { 1 }, + snapshot.status_counts + ) + end + + def test_rejects_an_unknown_task_status_instead_of_counting_it_as_not_started + unsupported_status = TaskStatus.create!( + id: PeerProgressDistributionPolicy::STATUS_KEYS.length + 1, + name: 'Future lifecycle state', + description: 'Not yet included in the public peer-progress contract' + ) + project = create( + :project, + unit: @unit, + target_grade: 0, + enrolled: true + ) + create( + :task, + project: project, + task_definition: @pass_task, + task_status: unsupported_status + ) + + assert_raises( + PeerProgressAggregationService::UnsupportedTaskStatusError + ) { run_service } + end + + def test_indexes_status_counts_by_task_definition_id_for_snapshot_upsert + create( + :project, + unit: @unit, + target_grade: 0, + enrolled: true + ) + + assert_nothing_raised { run_service } + + snapshot = find_snapshot( + task_definition: @pass_task, + target_grade: 0 + ) + assert_equal 1, snapshot.status_counts.fetch('not_started') + 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, snapshot.submitted_count + 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_equal 0, snapshot.submitted_count + assert_nil snapshot.submitted_percentage + assert_equal( + PeerProgressDistributionPolicy::STATUS_KEYS.index_with { 0 }, + snapshot.status_counts + ) + 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 statuses.length, snapshot.submitted_count + 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/services/peer_progress_distribution_policy_test.rb b/test/services/peer_progress_distribution_policy_test.rb new file mode 100644 index 0000000000..acc51b4510 --- /dev/null +++ b/test/services/peer_progress_distribution_policy_test.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +require 'test_helper' + +class PeerProgressDistributionPolicyTest < ActiveSupport::TestCase + test 'returns every lifecycle status in canonical order' do + distribution = PeerProgressDistributionPolicy.build( + status_counts: safe_status_counts, + cohort_size: 25 + ) + + assert_equal PeerProgressDistributionPolicy::STATUS_KEYS, + distribution.pluck(:status) + redo_entry = distribution.find do |entry| + entry.fetch(:status) == 'redo' + end + resubmit_entry = distribution.find do |entry| + entry.fetch(:status) == 'fix_and_resubmit' + end + + assert_equal 10.0, redo_entry.fetch(:percentage) + assert_equal 10.0, resubmit_entry.fetch(:percentage) + end + + test 'suppresses a jointly identifying vector even though each bucket is independently ambiguous' do + counts = empty_status_counts.merge( + 'not_started' => 6, + 'complete' => 18 + ) + + assert_nil PeerProgressDistributionPolicy.build( + status_counts: counts, + cohort_size: 24 + ) + end + + test 'rejects missing extra negative and inconsistent counts' do + missing = safe_status_counts.except('redo') + extra = safe_status_counts.merge('unknown' => 0) + negative = safe_status_counts.merge('redo' => -1, 'fail' => 4) + + [missing, extra, negative].each do |counts| + assert_nil PeerProgressDistributionPolicy.build( + status_counts: counts, + cohort_size: 25 + ) + end + + assert_nil PeerProgressDistributionPolicy.build( + status_counts: safe_status_counts, + cohort_size: 26 + ) + end + + test 'binary count ranges match exhaustive quantisation without a cohort cache' do + (21..200).each do |cohort_size| + exhaustive = (0..cohort_size).group_by do |count| + PeerProgressDistributionPolicy.quantised_count_percentage( + count: count, + cohort_size: cohort_size + ) + end + + exhaustive.each do |bucket, counts| + actual = PeerProgressDistributionPolicy.send( + :count_range_for_bucket, + bucket, + cohort_size + ) + + assert_equal counts.min..counts.max, actual + end + end + end + + private + + def empty_status_counts + PeerProgressDistributionPolicy::STATUS_KEYS.index_with { 0 } + end + + def safe_status_counts + empty_status_counts.merge( + 'not_started' => 5, + 'working_on_it' => 5, + 'ready_for_feedback' => 4, + 'fix_and_resubmit' => 3, + 'redo' => 3, + 'complete' => 3, + 'fail' => 2 + ) + end +end diff --git a/test/services/peer_progress_viewer_policy_test.rb b/test/services/peer_progress_viewer_policy_test.rb new file mode 100644 index 0000000000..fcaf3b158d --- /dev/null +++ b/test/services/peer_progress_viewer_policy_test.rb @@ -0,0 +1,187 @@ +# frozen_string_literal: true + +require 'test_helper' + +class PeerProgressViewerPolicyTest < ActiveSupport::TestCase + Snapshot = Struct.new( + :submitted_count, + :cohort_size, + :status_counts, + :calculated_at, + keyword_init: true + ) + ViewerTask = Struct.new( + :task_status_id, + :file_uploaded_at, + :updated_at, + :is_persisted, + keyword_init: true + ) do + def persisted? + is_persisted + end + end + ViewerProject = Struct.new( + :updated_at, + :is_persisted, + keyword_init: true + ) do + def persisted? + is_persisted + end + end + + test 'subtracts the viewers known status upload and cohort membership' do + calculated_at = Time.zone.now + snapshot = Snapshot.new( + cohort_size: 22, + submitted_count: 1, + status_counts: empty_status_counts.merge( + 'not_started' => 21, + 'complete' => 1 + ), + calculated_at: calculated_at + ) + viewer_task = ViewerTask.new( + task_status_id: TaskStatus.complete.id, + file_uploaded_at: 1.hour.ago, + updated_at: calculated_at - 1.minute, + is_persisted: true + ) + + result = PeerProgressViewerPolicy.build( + snapshot: snapshot, + viewer_project: viewer_project_for(snapshot), + viewer_task: viewer_task + ) + + assert_equal 21, result.fetch(:cohort_size) + assert_equal 0, result.fetch(:submitted_count) + assert_equal 21, result.fetch(:status_counts).fetch('not_started') + assert_equal 0, result.fetch(:status_counts).fetch('complete') + end + + test 'treats a missing viewer task as not started and unsubmitted' do + snapshot = Snapshot.new( + cohort_size: 22, + submitted_count: 21, + status_counts: empty_status_counts.merge( + 'not_started' => 1, + 'complete' => 21 + ), + calculated_at: Time.zone.now + ) + viewer_task = ViewerTask.new( + task_status_id: TaskStatus.not_started.id, + file_uploaded_at: nil, + updated_at: nil, + is_persisted: false + ) + + result = PeerProgressViewerPolicy.build( + snapshot: snapshot, + viewer_project: viewer_project_for(snapshot), + viewer_task: viewer_task + ) + + assert_equal 21, result.fetch(:cohort_size) + assert_equal 21, result.fetch(:submitted_count) + assert_equal 0, result.fetch(:status_counts).fetch('not_started') + assert_equal 21, result.fetch(:status_counts).fetch('complete') + end + + test 'fails closed when the viewer changed after the snapshot' do + calculated_at = 1.hour.ago + viewer_task = ViewerTask.new( + task_status_id: TaskStatus.not_started.id, + file_uploaded_at: nil, + updated_at: calculated_at + 1.minute, + is_persisted: true + ) + + snapshot = valid_snapshot(calculated_at: calculated_at) + assert_not PeerProgressViewerPolicy.viewer_context_current?( + snapshot: snapshot, + viewer_project: viewer_project_for(snapshot), + viewer_task: viewer_task + ) + assert_nil PeerProgressViewerPolicy.build( + snapshot: snapshot, + viewer_project: viewer_project_for(snapshot), + viewer_task: viewer_task + ) + end + + test 'fails closed when project membership may have changed after snapshot' do + snapshot = valid_snapshot(calculated_at: 1.hour.ago) + viewer_project = ViewerProject.new( + updated_at: snapshot.calculated_at + 1.minute, + is_persisted: true + ) + + assert_not PeerProgressViewerPolicy.viewer_context_current?( + snapshot: snapshot, + viewer_project: viewer_project, + viewer_task: missing_viewer_task + ) + assert_nil PeerProgressViewerPolicy.build( + snapshot: snapshot, + viewer_project: viewer_project, + viewer_task: missing_viewer_task + ) + end + + test 'fails closed for an incomplete exact upload aggregate' do + snapshot = valid_snapshot + snapshot.submitted_count = nil + + assert_nil PeerProgressViewerPolicy.build( + snapshot: snapshot, + viewer_project: viewer_project_for(snapshot), + viewer_task: missing_viewer_task + ) + end + + test 'fails closed for a lifecycle status outside the canonical contract' do + viewer_task = missing_viewer_task + viewer_task.task_status_id = 16 + + snapshot = valid_snapshot + assert_nil PeerProgressViewerPolicy.build( + snapshot: snapshot, + viewer_project: viewer_project_for(snapshot), + viewer_task: viewer_task + ) + end + + private + + def valid_snapshot(calculated_at: Time.zone.now) + Snapshot.new( + cohort_size: 22, + submitted_count: 0, + status_counts: empty_status_counts.merge('not_started' => 22), + calculated_at: calculated_at + ) + end + + def missing_viewer_task + ViewerTask.new( + task_status_id: TaskStatus.not_started.id, + file_uploaded_at: nil, + updated_at: nil, + is_persisted: false + ) + end + + def viewer_project_for(snapshot) + ViewerProject.new( + updated_at: snapshot.calculated_at - 1.minute, + is_persisted: true + ) + end + + def empty_status_counts + PeerProgressDistributionPolicy::STATUS_KEYS.index_with { 0 } + 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 From ca62d8d8885480681f35b2270f954acb7830d491 Mon Sep 17 00:00:00 2001 From: Clupai8o0 Date: Mon, 28 Sep 2026 00:29:20 +1000 Subject: [PATCH 2/2] feat(ppi-cpd): bring in org api PR 178 for the ppi-cpd files Brings ontrack-features-t2-2026/doubtfire-api#178 to the files this PR already carries, so every file stays in exactly one PR. It carries the portfolio submitted notification for tutors. --- app/api/projects_api.rb | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/app/api/projects_api.rb b/app/api/projects_api.rb index a309fb7d99..b83f46eea6 100644 --- a/app/api/projects_api.rb +++ b/app/api/projects_api.rb @@ -33,6 +33,41 @@ def notify_portfolio_received(project) "Failed to raise portfolio_received notification for project #{project.id}: #{e.message}" ) end + + # Tell the staff who will mark it that a portfolio arrived. + # + # Every tutor the student is enrolled with, because a student in a lab and a + # workshop stream has two and either may be the one marking. With no tutor + # at all it goes to the main convenor, the same fallback tutor_for uses. + # + # 'portfolio' so the tutor's own portfolio preference switches it off. The + # dedupe key is the submission time, so a retried request cannot notify + # twice but a later resubmission still does. + def notify_portfolio_submitted(project, actor) + student = project.student + recipients = project.tutorial_enrolments.includes(tutorial: { unit_role: :user }) + .filter_map { |enrolment| enrolment.tutorial&.tutor } + .uniq + recipients = [project.main_convenor_user].compact if recipients.empty? + + recipients.each do |recipient| + next if recipient == student || recipient == actor + + NotificationService.notify( + user: recipient, + type: 'portfolio', + event: 'portfolio_submitted', + message: "#{student.name} submitted a portfolio in #{project.unit.code}.", + link: "/projects/#{project.id}/dashboard", + notifiable: project, + dedupe_key: "portfolio_submitted:project:#{project.id}:#{project.portfolio_submission_date.to_i}" + ) + end + rescue StandardError => e + Rails.logger.error( + "Failed to raise portfolio_submitted notification for project #{project.id}: #{e.message}" + ) + end end before do @@ -218,7 +253,10 @@ def notify_portfolio_received(project) submission_saved = project.save end - notify_portfolio_received(project) if submission_saved && new_portfolio_submission + if submission_saved && new_portfolio_submission + notify_portfolio_received(project) + notify_portfolio_submitted(project, current_user) + end end Entities::ProjectEntity.represent(project, only: [:campus_id, :enrolled, :target_grade, :submitted_grade, :compile_portfolio, :portfolio_available, :uses_draft_learning_summary, :stats], for_student: for_student)