From 8b40c0b2a75d27da7aa3a5cd69de7b9714222586 Mon Sep 17 00:00:00 2001 From: xmi Date: Wed, 22 Apr 2026 21:09:19 -0400 Subject: [PATCH 01/25] Add calibration flag to response maps --- .../20260423010050_add_for_calibration_to_response_maps.rb | 6 ++++++ db/schema.rb | 6 ++++-- 2 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20260423010050_add_for_calibration_to_response_maps.rb diff --git a/db/migrate/20260423010050_add_for_calibration_to_response_maps.rb b/db/migrate/20260423010050_add_for_calibration_to_response_maps.rb new file mode 100644 index 000000000..30a930d4a --- /dev/null +++ b/db/migrate/20260423010050_add_for_calibration_to_response_maps.rb @@ -0,0 +1,6 @@ +class AddForCalibrationToResponseMaps < ActiveRecord::Migration[8.0] + def change + add_column :response_maps, :for_calibration, :boolean, default: false, null: false + add_index :response_maps, :for_calibration + end +end diff --git a/db/schema.rb b/db/schema.rb index cddbe12c6..10b0ceb64 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_03_13_064334) do +ActiveRecord::Schema[8.0].define(version: 2026_04_23_010050) do create_table "account_requests", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.string "username" t.string "full_name" @@ -331,6 +331,8 @@ t.datetime "created_at", null: false t.datetime "updated_at", null: false t.string "type" + t.boolean "for_calibration", default: false, null: false + t.index ["for_calibration"], name: "index_response_maps_on_for_calibration" t.index ["reviewer_id"], name: "fk_response_map_reviewer" end @@ -456,9 +458,9 @@ add_foreign_key "assignments_duties", "duties" add_foreign_key "courses", "institutions" add_foreign_key "courses", "users", column: "instructor_id" + add_foreign_key "duties", "users", column: "instructor_id" add_foreign_key "invitations", "participants", column: "from_id" add_foreign_key "invitations", "participants", column: "to_id" - add_foreign_key "duties", "users", column: "instructor_id" add_foreign_key "items", "questionnaires" add_foreign_key "participants", "duties" add_foreign_key "participants", "join_team_requests" From fcbf205447859311474567c0e95c7499a8706295 Mon Sep 17 00:00:00 2001 From: xmi Date: Wed, 22 Apr 2026 21:37:27 -0400 Subject: [PATCH 02/25] Use for_calibration in review mapping handler --- app/models/review_mapping_handler.rb | 12 ++++-- spec/factories/review_response_maps.rb | 12 ++++-- spec/factories/teams.rb | 12 +++--- spec/models/review_mapping_handler_spec.rb | 45 ++++++++++++++++++++++ 4 files changed, 67 insertions(+), 14 deletions(-) create mode 100644 spec/models/review_mapping_handler_spec.rb diff --git a/app/models/review_mapping_handler.rb b/app/models/review_mapping_handler.rb index 11a5a5f2d..924062a85 100644 --- a/app/models/review_mapping_handler.rb +++ b/app/models/review_mapping_handler.rb @@ -59,8 +59,12 @@ def assign_calibration_reviews_round_robin # Get all participants (students) reviewers = AssignmentParticipant.where(parent_id: @assignment.id) - # Get all instructor calibration teams/submissions - calibration_teams = AssignmentTeam.where(parent_id: @assignment.id, is_calibration: true) + # Get teams that already have an instructor calibration map. + calibration_team_ids = ReviewResponseMap + .where(reviewed_object_id: @assignment.id, for_calibration: true) + .distinct + .pluck(:reviewee_id) + calibration_teams = AssignmentTeam.where(id: calibration_team_ids) return if calibration_teams.empty? # Assign in round robin: each reviewer gets 2 calibration reviews @@ -71,7 +75,7 @@ def assign_calibration_reviews_round_robin reviewer: reviewer, reviewee: team, reviewed_object_id: @assignment.id, - calibration: true + for_calibration: true ) end end @@ -79,7 +83,7 @@ def assign_calibration_reviews_round_robin def calibration_reviews_for(reviewer) - ReviewResponseMap.where(reviewer: reviewer, calibration: true) + ReviewResponseMap.where(reviewer: reviewer, reviewed_object_id: @assignment.id, for_calibration: true) end # ===== OUTSTANDING REVIEWS ===== diff --git a/spec/factories/review_response_maps.rb b/spec/factories/review_response_maps.rb index 1cee9aace..fd1dc712f 100644 --- a/spec/factories/review_response_maps.rb +++ b/spec/factories/review_response_maps.rb @@ -1,8 +1,12 @@ FactoryBot.define do factory :review_response_map do association :assignment - association :reviewer, factory: :user - association :reviewee, factory: :team - reviewed_object_id { 1 } + reviewer { association :assignment_participant, assignment: assignment } + reviewee { association :assignment_team, assignment: assignment } + reviewed_object_id { assignment.id } + + trait :for_calibration do + for_calibration { true } + end end -end \ No newline at end of file +end diff --git a/spec/factories/teams.rb b/spec/factories/teams.rb index 8533a6b0e..903e32eb3 100644 --- a/spec/factories/teams.rb +++ b/spec/factories/teams.rb @@ -24,18 +24,18 @@ course { create(:course) } end + assignment { create(:assignment, course: course) } + parent_id { assignment.id } + after(:build) do |team, evaluator| - if team.assignment.nil? - team.course = evaluator.course - else - team.course = team.assignment.course - end + team.assignment ||= create(:assignment, course: evaluator.course) + team.parent_id = team.assignment.id end trait :with_assignment do after(:build) do |team, evaluator| team.assignment = create(:assignment, course: evaluator.course) - team.course = team.assignment.course + team.parent_id = team.assignment.id end end end diff --git a/spec/models/review_mapping_handler_spec.rb b/spec/models/review_mapping_handler_spec.rb new file mode 100644 index 000000000..d2501832c --- /dev/null +++ b/spec/models/review_mapping_handler_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe ReviewMappingHandler do + describe '#calibration_reviews_for' do + it 'returns only calibration review maps for the handler assignment and reviewer' do + assignment = create(:assignment) + other_assignment = create(:assignment) + reviewer = create(:assignment_participant, assignment: assignment) + + calibration_map = create(:review_response_map, :for_calibration, assignment: assignment, reviewer: reviewer) + create(:review_response_map, assignment: assignment, reviewer: reviewer) + create(:review_response_map, :for_calibration, assignment: other_assignment) + + result = described_class.new(assignment).calibration_reviews_for(reviewer) + + expect(result).to contain_exactly(calibration_map) + end + end + + describe '#assign_calibration_reviews_round_robin' do + it 'creates calibration maps against existing calibration reviewees' do + assignment = create(:assignment) + reviewers = create_list(:assignment_participant, 2, assignment: assignment) + calibration_teams = create_list(:assignment_team, 2, assignment: assignment) + + create(:review_response_map, :for_calibration, assignment: assignment, reviewee: calibration_teams.first) + create(:review_response_map, :for_calibration, assignment: assignment, reviewee: calibration_teams.second) + + described_class.new(assignment).assign_calibration_reviews_round_robin + + reviewers.each do |reviewer| + maps = ReviewResponseMap.where( + reviewer: reviewer, + reviewed_object_id: assignment.id, + for_calibration: true + ) + + expect(maps.count).to eq(2) + expect(maps.pluck(:reviewee_id)).to match_array(calibration_teams.map(&:id)) + end + end + end +end From 36f41df81ff609ffbe71361cf594d40dc1c5d915 Mon Sep 17 00:00:00 2001 From: xmi Date: Wed, 22 Apr 2026 22:09:57 -0400 Subject: [PATCH 03/25] Add calibration per item summary service --- app/services/calibration_per_item_summary.rb | 78 ++++++++ .../calibration_per_item_summary_spec.rb | 187 ++++++++++++++++++ 2 files changed, 265 insertions(+) create mode 100644 app/services/calibration_per_item_summary.rb create mode 100644 spec/services/calibration_per_item_summary_spec.rb diff --git a/app/services/calibration_per_item_summary.rb b/app/services/calibration_per_item_summary.rb new file mode 100644 index 000000000..eea5ce3c8 --- /dev/null +++ b/app/services/calibration_per_item_summary.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +class CalibrationPerItemSummary + def self.build(items:, instructor_response:, student_responses:) + new( + items: items, + instructor_response: instructor_response, + student_responses: student_responses + ).build + end + + def initialize(items:, instructor_response:, student_responses:) + @items = Array(items) + @instructor_response = instructor_response + @student_responses = Array(student_responses).compact + end + + def build + instructor_answers = answers_by_item(@instructor_response) + latest_student_responses = latest_submitted_student_responses + student_answers = latest_student_responses.to_h { |response| [response.id, answers_by_item(response)] } + + @items.sort_by(&:seq).map do |item| + { + item_id: item.id, + item_label: item.txt, + item_seq: item.seq, + instructor_score: instructor_answers[item.id]&.answer, + instructor_comment: instructor_answers[item.id]&.comments, + bucket_counts: bucket_counts_for(item, latest_student_responses, student_answers), + student_response_count: latest_student_responses.count + } + end + end + + private + + def latest_submitted_student_responses + @student_responses + .select(&:is_submitted) + .group_by(&:map_id) + .values + .map { |responses| responses.max_by(&:updated_at) } + .compact + end + + def answers_by_item(response) + return {} unless response + + response.scores.each_with_object({}) do |answer, by_item| + by_item[answer.item_id] = answer + end + end + + def bucket_counts_for(item, responses, answers_by_response) + buckets = score_range_for(item).each_with_object({}) do |score, counts| + counts[score.to_s] = 0 + end + + responses.each do |response| + score = answers_by_response.fetch(response.id).fetch(item.id, nil)&.answer + next if score.nil? + + key = score.to_i.to_s + buckets[key] ||= 0 + buckets[key] += 1 + end + + buckets + end + + def score_range_for(item) + min_score = item.questionnaire&.min_question_score || 0 + max_score = item.questionnaire&.max_question_score || 5 + + min_score.to_i..max_score.to_i + end +end diff --git a/spec/services/calibration_per_item_summary_spec.rb b/spec/services/calibration_per_item_summary_spec.rb new file mode 100644 index 000000000..c0ac7ad74 --- /dev/null +++ b/spec/services/calibration_per_item_summary_spec.rb @@ -0,0 +1,187 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe CalibrationPerItemSummary do + describe '.build' do + it 'summarizes submitted student scores by rubric item and includes instructor scores' do + questionnaire = create_questionnaire + code_quality = create(:item, questionnaire: questionnaire, txt: 'Code quality', seq: 1) + documentation = create(:item, questionnaire: questionnaire, txt: 'Documentation', seq: 2) + code_quality.update!(seq: 1) + documentation.update!(seq: 2) + assignment = create(:assignment) + + instructor_map = create(:review_response_map, :for_calibration, assignment: assignment) + instructor_response = create_response( + map: instructor_map, + submitted: true, + scores: { + code_quality => { answer: 4, comments: 'Clear implementation' }, + documentation => { answer: 5, comments: 'Very complete' } + } + ) + + student_map_one = create(:review_response_map, :for_calibration, assignment: assignment) + student_map_two = create(:review_response_map, :for_calibration, assignment: assignment) + + create_response( + map: student_map_one, + submitted: true, + updated_at: 2.days.ago, + scores: { + code_quality => { answer: 1 }, + documentation => { answer: 2 } + } + ) + latest_student_one = create_response( + map: student_map_one, + submitted: true, + updated_at: 1.day.ago, + scores: { + code_quality => { answer: 3 }, + documentation => { answer: 2 } + } + ) + latest_student_two = create_response( + map: student_map_two, + submitted: true, + updated_at: 3.hours.ago, + scores: { + code_quality => { answer: 3 } + } + ) + unsubmitted_student_response = create_response( + map: student_map_two, + submitted: false, + updated_at: 1.hour.ago, + scores: { + code_quality => { answer: 5 }, + documentation => { answer: 5 } + } + ) + + summary = described_class.build( + items: [documentation, code_quality], + instructor_response: instructor_response, + student_responses: [ + latest_student_two, + unsubmitted_student_response, + latest_student_one + ] + ) + + expect(summary).to eq([ + { + item_id: code_quality.id, + item_label: 'Code quality', + item_seq: code_quality.seq, + instructor_score: 4, + instructor_comment: 'Clear implementation', + bucket_counts: { + '0' => 0, + '1' => 0, + '2' => 0, + '3' => 2, + '4' => 0, + '5' => 0 + }, + student_response_count: 2 + }, + { + item_id: documentation.id, + item_label: 'Documentation', + item_seq: documentation.seq, + instructor_score: 5, + instructor_comment: 'Very complete', + bucket_counts: { + '0' => 0, + '1' => 0, + '2' => 1, + '3' => 0, + '4' => 0, + '5' => 0 + }, + student_response_count: 2 + } + ]) + end + + it 'uses the latest submitted response per map even when a newer draft exists' do + questionnaire = create_questionnaire + item = create(:item, questionnaire: questionnaire, txt: 'Accuracy', seq: 1) + assignment = create(:assignment) + response_map = create(:review_response_map, :for_calibration, assignment: assignment) + + submitted_response = create_response( + map: response_map, + submitted: true, + updated_at: 2.hours.ago, + scores: { + item => { answer: 2 } + } + ) + draft_response = create_response( + map: response_map, + submitted: false, + updated_at: 1.hour.ago, + scores: { + item => { answer: 5 } + } + ) + + summary = described_class.build( + items: [item], + instructor_response: nil, + student_responses: [submitted_response, draft_response] + ) + + expect(summary.first[:bucket_counts]['2']).to eq(1) + expect(summary.first[:bucket_counts]['5']).to eq(0) + expect(summary.first[:student_response_count]).to eq(1) + end + end + + def create_response(map:, submitted:, scores:, updated_at: Time.current) + response = Response.create!( + response_map: map, + round: 1, + version_num: 1, + is_submitted: submitted, + created_at: updated_at, + updated_at: updated_at + ) + + scores.each do |item, score| + Answer.create!( + response: response, + item: item, + answer: score[:answer], + comments: score[:comments] + ) + end + + response + end + + def create_questionnaire + Questionnaire.create!( + name: "Calibration Rubric #{SecureRandom.hex(4)}", + private: false, + min_question_score: 0, + max_question_score: 5, + instructor: create_instructor + ) + end + + def create_instructor + Instructor.create!( + name: "instructor_#{SecureRandom.hex(4)}", + email: "instructor_#{SecureRandom.hex(4)}@example.com", + password: 'password', + full_name: 'Calibration Instructor', + role: create(:role, name: "Instructor #{SecureRandom.hex(4)}"), + institution: create(:institution) + ) + end +end From 552e5c670f1395d1840ddd65facffc009b50aa1a Mon Sep 17 00:00:00 2001 From: xmi Date: Wed, 22 Apr 2026 22:32:52 -0400 Subject: [PATCH 04/25] Add calibration report endpoint --- app/controllers/review_mappings_controller.rb | 106 ++++++++++ config/routes.rb | 24 ++- ...review_mappings_calibration_report_spec.rb | 194 ++++++++++++++++++ 3 files changed, 315 insertions(+), 9 deletions(-) create mode 100644 spec/requests/api/v1/review_mappings_calibration_report_spec.rb diff --git a/app/controllers/review_mappings_controller.rb b/app/controllers/review_mappings_controller.rb index 486840270..ab5f7f4c3 100644 --- a/app/controllers/review_mappings_controller.rb +++ b/app/controllers/review_mappings_controller.rb @@ -62,6 +62,39 @@ def assign_calibration_artifacts render json: { status: "ok", message: "Calibration reviews assigned to all reviewers" } end + def calibration_report + instructor_map = ReviewResponseMap.find_by!( + id: params[:id], + reviewed_object_id: @assignment.id, + for_calibration: true + ) + instructor_response = latest_submitted_response_for(instructor_map) + return render json: { error: "Submitted instructor calibration response not found" }, status: :unprocessable_entity unless instructor_response + + rubric_items = rubric_items_for(instructor_response) + return render json: { error: "Review rubric not found" }, status: :unprocessable_entity if rubric_items.empty? + + student_responses = submitted_student_responses_for(instructor_map) + per_item_summary = CalibrationPerItemSummary.build( + items: rubric_items, + instructor_response: instructor_response, + student_responses: student_responses + ) + + render json: { + map_id: instructor_map.id, + assignment_id: @assignment.id, + reviewee_id: instructor_map.reviewee_id, + rubric_items: rubric_items.map { |item| serialize_item(item) }, + instructor_response: serialize_response(instructor_response), + student_responses: student_responses.map { |response| serialize_response(response) }, + per_item_summary: per_item_summary, + submitted_content: submitted_content_for(instructor_map.reviewee) + }, status: :ok + rescue ActiveRecord::RecordNotFound + render json: { error: "Calibration review map not found" }, status: :not_found + end + # ===== DELETE ===== def destroy handler = ReviewMappingHandler.new(@assignment) @@ -85,9 +118,82 @@ def grade_review render json: { status: "ok", message: "Review graded" } end + def action_allowed? + return teaching_staff_for_calibration_report? if params[:action] == "calibration_report" + + true + end + private def set_assignment @assignment = Assignment.find(params[:assignment_id]) end + + def teaching_staff_for_calibration_report? + assignment = Assignment.find_by(id: params[:assignment_id]) + return false unless user_logged_in? && assignment + return true if assignment.instructor_id == current_user.id + + assignment.course_id.present? && TaMapping.exists?(user_id: current_user.id, course_id: assignment.course_id) + end + + def latest_submitted_response_for(response_map) + response_map.responses.where(is_submitted: true).order(updated_at: :desc).first + end + + def submitted_student_responses_for(instructor_map) + student_maps = ReviewResponseMap + .where( + reviewed_object_id: @assignment.id, + reviewee_id: instructor_map.reviewee_id, + for_calibration: true + ) + .where.not(id: instructor_map.id) + + student_maps.flat_map { |map| map.responses.where(is_submitted: true).to_a } + end + + def rubric_items_for(response) + response.questionnaire.items.order(:seq) + rescue NoMethodError + [] + end + + def serialize_item(item) + { + id: item.id, + txt: item.txt, + seq: item.seq, + question_type: item.question_type, + weight: item.weight, + min_score: item.questionnaire.min_question_score, + max_score: item.questionnaire.max_question_score + } + end + + def serialize_response(response) + { + id: response.id, + map_id: response.map_id, + reviewer_id: response.map.reviewer_id, + reviewer_name: response.map.reviewer&.fullname, + is_submitted: response.is_submitted, + updated_at: response.updated_at, + answers: response.scores.map do |answer| + { + item_id: answer.item_id, + score: answer.answer, + comments: answer.comments + } + end + } + end + + def submitted_content_for(reviewee) + { + hyperlinks: reviewee.respond_to?(:hyperlinks) ? reviewee.hyperlinks : [], + files: SubmissionRecord.files.where(team_id: reviewee.id, assignment_id: @assignment.id).pluck(:content) + } + end end diff --git a/config/routes.rb b/config/routes.rb index 57559d007..ab3b63394 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -23,11 +23,11 @@ get 'role/:name', action: :role_users end end - resources :assignments do - collection do - post '/:assignment_id/add_participant/:user_id',action: :add_participant - delete '/:assignment_id/remove_participant/:user_id',action: :remove_participant - patch '/:assignment_id/remove_assignment_from_course',action: :remove_assignment_from_course + resources :assignments do + collection do + post '/:assignment_id/add_participant/:user_id',action: :add_participant + delete '/:assignment_id/remove_participant/:user_id',action: :remove_participant + patch '/:assignment_id/remove_assignment_from_course',action: :remove_assignment_from_course patch '/:assignment_id/assign_course/:course_id',action: :assign_course post '/:assignment_id/copy_assignment', action: :copy_assignment get '/:assignment_id/has_topics',action: :has_topics @@ -35,10 +35,16 @@ get '/:assignment_id/team_assignment', action: :team_assignment get '/:assignment_id/has_teams', action: :has_teams get '/:assignment_id/valid_num_review/:review_type', action: :valid_num_review - get '/:assignment_id/varying_rubrics_by_round', action: :varying_rubrics_by_round? - post '/:assignment_id/create_node',action: :create_node - end - end + get '/:assignment_id/varying_rubrics_by_round', action: :varying_rubrics_by_round? + post '/:assignment_id/create_node',action: :create_node + end + + resources :review_mappings, only: [] do + member do + get :calibration_report + end + end + end resources :bookmarks, except: [:new, :edit] do member do diff --git a/spec/requests/api/v1/review_mappings_calibration_report_spec.rb b/spec/requests/api/v1/review_mappings_calibration_report_spec.rb new file mode 100644 index 000000000..fc3e549b7 --- /dev/null +++ b/spec/requests/api/v1/review_mappings_calibration_report_spec.rb @@ -0,0 +1,194 @@ +# frozen_string_literal: true + +require 'rails_helper' +require 'json_web_token' + +RSpec.describe 'ReviewMappings calibration report', type: :request do + let!(:super_admin_role) { Role.create!(name: 'Super Administrator') } + let!(:admin_role) { Role.create!(name: 'Administrator') } + let!(:instructor_role) { Role.create!(name: 'Instructor') } + let!(:student_role) { Role.create!(name: 'Student') } + let(:institution) { create(:institution) } + let(:instructor) { create_user(role: instructor_role, name: 'calibration_instructor') } + let(:student_user) { create_user(role: student_role, name: 'calibration_student') } + let(:assignment) { Assignment.create!(name: 'Calibration Assignment', instructor: instructor) } + let(:questionnaire) { create_questionnaire } + let(:code_quality) { create_item('Code quality', 1) } + let(:documentation) { create_item('Documentation', 2) } + let(:reviewee_team) { create(:assignment_team, assignment: assignment) } + let(:instructor_participant) { create(:assignment_participant, assignment: assignment, user: instructor) } + let(:student_participant) { create(:assignment_participant, assignment: assignment, user: student_user) } + let(:instructor_map) do + create( + :review_response_map, + :for_calibration, + assignment: assignment, + reviewer: instructor_participant, + reviewee: reviewee_team + ) + end + let(:student_map) do + create( + :review_response_map, + :for_calibration, + assignment: assignment, + reviewer: student_participant, + reviewee: reviewee_team + ) + end + let(:instructor_headers) { auth_headers_for(instructor) } + + before do + AssignmentQuestionnaire.create!( + assignment: assignment, + questionnaire: questionnaire, + used_in_round: 1 + ) + code_quality + documentation + end + + describe 'GET /assignments/:assignment_id/review_mappings/:id/calibration_report' do + it 'returns report JSON for a calibration map' do + create_response( + map: instructor_map, + submitted: true, + scores: { + code_quality => { answer: 4, comments: 'Clear implementation' }, + documentation => { answer: 5, comments: 'Complete docs' } + } + ) + create_response( + map: student_map, + submitted: true, + scores: { + code_quality => { answer: 3, comments: 'Mostly clear' }, + documentation => { answer: 5, comments: 'Strong docs' } + } + ) + SubmissionRecord.create!( + record_type: 'file', + content: 'submission/report.pdf', + operation: 'Submit File', + team_id: reviewee_team.id, + user: instructor.name, + assignment_id: assignment.id + ) + reviewee_team.update!(submitted_hyperlinks: YAML.dump(['https://example.com/submission'])) + + get calibration_report_path(instructor_map), headers: instructor_headers + + expect(response).to have_http_status(:ok) + + json = JSON.parse(response.body) + expect(json['map_id']).to eq(instructor_map.id) + expect(json['assignment_id']).to eq(assignment.id) + expect(json['reviewee_id']).to eq(reviewee_team.id) + expect(json['rubric_items'].map { |item| item['txt'] }).to eq(['Code quality', 'Documentation']) + expect(json['instructor_response']['answers']).to include( + hash_including('item_id' => code_quality.id, 'score' => 4, 'comments' => 'Clear implementation') + ) + expect(json['student_responses'].length).to eq(1) + expect(json['per_item_summary']).to include( + hash_including( + 'item_id' => code_quality.id, + 'item_label' => 'Code quality', + 'instructor_score' => 4, + 'bucket_counts' => hash_including('3' => 1) + ) + ) + expect(json['submitted_content']).to eq( + 'hyperlinks' => ['https://example.com/submission'], + 'files' => ['submission/report.pdf'] + ) + end + + it 'returns 404 when the calibration map does not exist' do + get "/assignments/#{assignment.id}/review_mappings/0/calibration_report", headers: instructor_headers + + expect(response).to have_http_status(:not_found) + expect(JSON.parse(response.body)['error']).to eq('Calibration review map not found') + end + + it 'returns 422 when the instructor response has not been submitted' do + create_response(map: instructor_map, submitted: false, scores: { code_quality => { answer: 4 } }) + + get calibration_report_path(instructor_map), headers: instructor_headers + + expect(response).to have_http_status(:unprocessable_entity) + expect(JSON.parse(response.body)['error']).to eq('Submitted instructor calibration response not found') + end + + it 'returns 403 for a student who is not teaching staff for the assignment' do + create_response(map: instructor_map, submitted: true, scores: { code_quality => { answer: 4 } }) + + get calibration_report_path(instructor_map), headers: auth_headers_for(student_user) + + expect(response).to have_http_status(:forbidden) + end + end + + def calibration_report_path(map) + "/assignments/#{assignment.id}/review_mappings/#{map.id}/calibration_report" + end + + def auth_headers_for(user) + { 'Authorization' => "Bearer #{JsonWebToken.encode(id: user.id)}" } + end + + def create_user(role:, name:) + User.create!( + name: name, + email: "#{name}@example.com", + password: 'password', + full_name: name.titleize, + role: role, + institution: institution + ) + end + + def create_questionnaire + Questionnaire.create!( + name: 'Calibration Rubric', + private: false, + min_question_score: 0, + max_question_score: 5, + instructor: Instructor.find(instructor.id) + ) + end + + def create_item(txt, seq) + item = Item.create!( + questionnaire: questionnaire, + txt: txt, + seq: seq, + weight: 1, + question_type: 'Scale', + break_before: true + ) + item.update!(seq: seq) + item + end + + def create_response(map:, submitted:, scores:, updated_at: Time.current) + response = Response.create!( + response_map: map, + round: 1, + version_num: 1, + is_submitted: submitted, + created_at: updated_at, + updated_at: updated_at + ) + + scores.each do |item, score| + Answer.create!( + response: response, + item: item, + answer: score[:answer], + comments: score[:comments] + ) + end + + response + end +end From 61f209ce3adf734a9b92b6b9f77c055128c346e8 Mon Sep 17 00:00:00 2001 From: xmi Date: Thu, 23 Apr 2026 11:02:00 -0400 Subject: [PATCH 05/25] Align calibration report responses with latest submitted review --- app/controllers/review_mappings_controller.rb | 2 +- ...review_mappings_calibration_report_spec.rb | 46 +++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/app/controllers/review_mappings_controller.rb b/app/controllers/review_mappings_controller.rb index ab5f7f4c3..3feac1fe2 100644 --- a/app/controllers/review_mappings_controller.rb +++ b/app/controllers/review_mappings_controller.rb @@ -151,7 +151,7 @@ def submitted_student_responses_for(instructor_map) ) .where.not(id: instructor_map.id) - student_maps.flat_map { |map| map.responses.where(is_submitted: true).to_a } + student_maps.filter_map { |map| latest_submitted_response_for(map) } end def rubric_items_for(response) diff --git a/spec/requests/api/v1/review_mappings_calibration_report_spec.rb b/spec/requests/api/v1/review_mappings_calibration_report_spec.rb index fc3e549b7..32d2a87be 100644 --- a/spec/requests/api/v1/review_mappings_calibration_report_spec.rb +++ b/spec/requests/api/v1/review_mappings_calibration_report_spec.rb @@ -103,6 +103,52 @@ ) end + it 'returns only the latest submitted student response for each calibration map' do + create_response( + map: instructor_map, + submitted: true, + scores: { + code_quality => { answer: 4, comments: 'Instructor score' }, + documentation => { answer: 5, comments: 'Instructor docs' } + } + ) + create_response( + map: student_map, + submitted: true, + updated_at: 2.days.ago, + scores: { + code_quality => { answer: 2, comments: 'Older response' }, + documentation => { answer: 3, comments: 'Older docs' } + } + ) + create_response( + map: student_map, + submitted: true, + updated_at: 1.day.ago, + scores: { + code_quality => { answer: 5, comments: 'Latest response' }, + documentation => { answer: 4, comments: 'Latest docs' } + } + ) + + get calibration_report_path(instructor_map), headers: instructor_headers + + expect(response).to have_http_status(:ok) + + json = JSON.parse(response.body) + expect(json['student_responses'].length).to eq(1) + expect(json['student_responses'].first['answers']).to include( + hash_including('item_id' => code_quality.id, 'score' => 5, 'comments' => 'Latest response'), + hash_including('item_id' => documentation.id, 'score' => 4, 'comments' => 'Latest docs') + ) + expect(json['per_item_summary']).to include( + hash_including( + 'item_id' => code_quality.id, + 'bucket_counts' => hash_including('5' => 1, '2' => 0) + ) + ) + end + it 'returns 404 when the calibration map does not exist' do get "/assignments/#{assignment.id}/review_mappings/0/calibration_report", headers: instructor_headers From d2795c33f84a3379a3a25e92b916164dbf8510c8 Mon Sep 17 00:00:00 2001 From: xmi Date: Thu, 23 Apr 2026 21:20:19 -0400 Subject: [PATCH 06/25] Add calibration report demo data task --- lib/tasks/calibration_demo.rake | 240 ++++++++++++++++++++++++++++++++ 1 file changed, 240 insertions(+) create mode 100644 lib/tasks/calibration_demo.rake diff --git a/lib/tasks/calibration_demo.rake b/lib/tasks/calibration_demo.rake new file mode 100644 index 000000000..c2e5801d5 --- /dev/null +++ b/lib/tasks/calibration_demo.rake @@ -0,0 +1,240 @@ +# frozen_string_literal: true + +namespace :demo do + desc 'Create or refresh calibration report demo data with six student results' + task calibration_report: :environment do + role_for = lambda do |name| + Role.find_by!(name: name) + end + + institution = Institution.first || Institution.create!(name: 'North Carolina State University') + + ensure_user = lambda do |model_class:, name:, email:, role_name:, full_name:, password: 'password123', parent: nil| + user = User.find_by(name: name) || User.find_by(email: email) + + if user + user.update!( + email: email, + full_name: full_name, + role: role_for.call(role_name), + institution: institution, + parent: parent + ) + user.password = password if user.authenticate(password).nil? + user.save! if user.changed? + model_class.find(user.id) + else + model_class.create!( + name: name, + email: email, + password: password, + full_name: full_name, + role: role_for.call(role_name), + institution: institution, + parent: parent + ) + end + end + + ensure_participant = lambda do |assignment:, user:, handle:| + AssignmentParticipant.find_or_create_by!(parent_id: assignment.id, user_id: user.id) do |participant| + participant.handle = handle + end + end + + ensure_item = lambda do |questionnaire:, txt:, seq:| + item = questionnaire.items.find_or_initialize_by(txt: txt) + item.assign_attributes( + seq: seq, + weight: 1, + question_type: 'Scale', + break_before: true + ) + item.save! + item + end + + update_response = lambda do |map:, scores:, comments_prefix:| + response = Response.where(map_id: map.id, is_submitted: true).order(updated_at: :desc).first + + unless response + response = Response.create!( + response_map: map, + round: 1, + version_num: Response.where(map_id: map.id).maximum(:version_num).to_i + 1, + is_submitted: true + ) + end + + response.update!(is_submitted: true, updated_at: Time.current) + + scores.each do |item, score| + answer = Answer.find_or_initialize_by(response_id: response.id, item_id: item.id) + answer.update!( + answer: score, + comments: "#{comments_prefix}: #{item.txt} => #{score}" + ) + end + + response + end + + instructor = ensure_user.call( + model_class: Instructor, + name: 'calibration_demo_instructor', + email: 'calibration_demo_instructor@example.com', + role_name: 'Instructor', + full_name: 'Calibration Demo Instructor' + ) + + assignment = Assignment.find_by(id: 8) || Assignment.find_by(name: 'Calibration Demo Assignment') + assignment ||= Assignment.create!( + name: 'Calibration Demo Assignment', + instructor: instructor, + has_teams: true, + private: false + ) + assignment.update!(instructor: instructor, has_teams: true, private: false) + + reviewee_team = + if assignment.id == 8 && ReviewResponseMap.exists?(id: 8, reviewed_object_id: assignment.id, for_calibration: true) + ReviewResponseMap.find(8).reviewee + else + assignment.teams.find_or_create_by!(name: 'Calibration Demo Reviewee Team') + end + + questionnaire = + assignment.assignment_questionnaires.find_by(used_in_round: 1)&.questionnaire || + Questionnaire.find_by(name: 'Calibration Demo Rubric', instructor_id: instructor.id) + + questionnaire ||= Questionnaire.create!( + name: 'Calibration Demo Rubric', + private: false, + min_question_score: 0, + max_question_score: 5, + instructor: instructor + ) + + AssignmentQuestionnaire.find_or_create_by!( + assignment: assignment, + questionnaire: questionnaire, + used_in_round: 1 + ) + + items = [ + ensure_item.call(questionnaire: questionnaire, txt: 'Code quality', seq: 1), + ensure_item.call(questionnaire: questionnaire, txt: 'Documentation', seq: 2), + ensure_item.call(questionnaire: questionnaire, txt: 'Testing', seq: 3) + ] + + instructor_participant = ensure_participant.call( + assignment: assignment, + user: instructor, + handle: instructor.name + ) + + instructor_map = + ReviewResponseMap.find_by(id: 8, reviewed_object_id: assignment.id, for_calibration: true) || + ReviewResponseMap.find_or_create_by!( + reviewed_object_id: assignment.id, + reviewer_id: instructor_participant.id, + reviewee_id: reviewee_team.id, + for_calibration: true + ) + + update_response.call( + map: instructor_map, + comments_prefix: 'Instructor calibration review', + scores: { + items[0] => 4, + items[1] => 5, + items[2] => 3 + } + ) + + score_sets = [ + [4, 5, 3], + [4, 4, 3], + [4, 4, 2], + [3, 4, 1], + [3, 2, 1], + [1, 1, 0] + ] + + student_maps = score_sets.each_with_index.map do |scores, index| + student = ensure_user.call( + model_class: User, + name: "calibration_demo_student_#{index + 1}", + email: "calibration_demo_student_#{index + 1}@example.com", + role_name: 'Student', + full_name: "Calibration Demo Student #{index + 1}", + parent: instructor + ) + + participant = ensure_participant.call( + assignment: assignment, + user: student, + handle: student.name + ) + + map = ReviewResponseMap.find_or_create_by!( + reviewed_object_id: assignment.id, + reviewer_id: participant.id, + reviewee_id: reviewee_team.id, + for_calibration: true + ) + + update_response.call( + map: map, + comments_prefix: "Demo reviewer #{index + 1}", + scores: { + items[0] => scores[0], + items[1] => scores[1], + items[2] => scores[2] + } + ) + + map + end + + reviewee_team.update!(submitted_hyperlinks: YAML.dump(['https://example.com/submission'])) + SubmissionRecord.find_or_create_by!( + record_type: 'file', + content: 'submission/report.pdf', + operation: 'Submit File', + team_id: reviewee_team.id, + user: instructor.name, + assignment_id: assignment.id + ) + + student_responses = student_maps.map do |map| + Response.where(map_id: map.id, is_submitted: true).order(updated_at: :desc).first + end + instructor_response = Response.where(map_id: instructor_map.id, is_submitted: true).order(updated_at: :desc).first + + summaries = CalibrationPerItemSummary.build( + items: items, + instructor_response: instructor_response, + student_responses: student_responses + ) + + distribution = summaries.map do |summary| + { + item: summary[:item_label], + agree: summary[:bucket_counts][summary[:instructor_score].to_s], + near: summary[:bucket_counts].select { |score, _count| (score.to_i - summary[:instructor_score].to_i).abs == 1 }.values.sum, + disagree: summary[:bucket_counts].select { |score, _count| (score.to_i - summary[:instructor_score].to_i).abs > 1 }.values.sum + } + end + + puts({ + assignment_id: assignment.id, + map_id: instructor_map.id, + reviewee_id: reviewee_team.id, + student_count: student_maps.length, + rubric_items: items.map(&:txt), + summaries: distribution, + frontend_url: "http://localhost:3000/assignments/edit/#{assignment.id}/calibration/#{instructor_map.id}" + }.inspect) + end +end From 7f9a687fcd81b051ee06ae041bbf0560bbc93bad Mon Sep 17 00:00:00 2001 From: xmi Date: Thu, 23 Apr 2026 21:28:28 -0400 Subject: [PATCH 07/25] Keep calibration demo data at six student maps --- lib/tasks/calibration_demo.rake | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/tasks/calibration_demo.rake b/lib/tasks/calibration_demo.rake index c2e5801d5..286276937 100644 --- a/lib/tasks/calibration_demo.rake +++ b/lib/tasks/calibration_demo.rake @@ -197,6 +197,12 @@ namespace :demo do map end + ReviewResponseMap.where( + reviewed_object_id: assignment.id, + reviewee_id: reviewee_team.id, + for_calibration: true + ).where.not(id: [instructor_map.id, *student_maps.map(&:id)]).find_each(&:destroy!) + reviewee_team.update!(submitted_hyperlinks: YAML.dump(['https://example.com/submission'])) SubmissionRecord.find_or_create_by!( record_type: 'file', From 9b041c76085e0136b316053232717832fda57e1c Mon Sep 17 00:00:00 2001 From: ruju4a Date: Fri, 24 Apr 2026 11:01:00 -0400 Subject: [PATCH 08/25] added add participant changes --- app/controllers/concerns/authorization.rb | 6 +- app/controllers/reports_controller.rb | 70 +++++++ app/controllers/review_mappings_controller.rb | 197 ++++++++++-------- app/models/Item.rb | 15 ++ app/models/assignment_team.rb | 33 +++ app/models/response.rb | 32 +++ app/models/response_map.rb | 7 + app/models/review_response_map.rb | 14 ++ app/models/team.rb | 55 +++++ config/routes.rb | 40 ++-- spec/models/team_factory_spec.rb | 90 ++++++++ .../api/v1/calibration_participants_spec.rb | 158 ++++++++++++++ ...rt_spec.rb => reports_calibration_spec.rb} | 8 +- 13 files changed, 622 insertions(+), 103 deletions(-) create mode 100644 app/controllers/reports_controller.rb create mode 100644 spec/models/team_factory_spec.rb create mode 100644 spec/requests/api/v1/calibration_participants_spec.rb rename spec/requests/api/v1/{review_mappings_calibration_report_spec.rb => reports_calibration_spec.rb} (94%) diff --git a/app/controllers/concerns/authorization.rb b/app/controllers/concerns/authorization.rb index b625bdfcc..57d1eaf7d 100644 --- a/app/controllers/concerns/authorization.rb +++ b/app/controllers/concerns/authorization.rb @@ -172,9 +172,11 @@ def current_user_instructs_assignment?(assignment) ) end - # Determine if the current user and the given assignment are associated by a TA mapping + # Determine if the current user and the given assignment are associated by a TA mapping. + # Safe-navigates through course because an assignment may not belong to a course. def current_user_has_ta_mapping_for_assignment?(assignment) - user_logged_in? && !assignment.nil? && TaMapping.exists?(user_id: current_user.id, course_id: assignment.course.id) + user_logged_in? && !assignment.nil? && assignment.course_id.present? && + TaMapping.exists?(user_id: current_user.id, course_id: assignment.course_id) end # Recursively find an assignment given the passed in Response id. Because a ResponseMap diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb new file mode 100644 index 000000000..31b6cf10e --- /dev/null +++ b/app/controllers/reports_controller.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +# Reports live in their own controller per Expertiza convention: a single +# controller responsible only for rendering reports, dispatching to per-report +# actions. Feature controllers (e.g. ReviewMappingsController) should not +# embed report logic, since that violates SRP and hides the code from other +# consumers who need the same information. +class ReportsController < ApplicationController + include Authorization + before_action :set_assignment + + # Reports are viewable only by teaching staff for the assignment (instructor + # of the assignment, the course instructor, or a TA mapped to the course). + def action_allowed? + current_user_teaching_staff_of_assignment?(params[:assignment_id]) + end + + # GET /assignments/:assignment_id/reports/calibration/:map_id + # + # Returns the comparison data for a single instructor calibration review: + # the instructor's submitted response, the rubric items, the student + # calibration responses for the same reviewee, a per-item summary produced + # by CalibrationPerItemSummary, and the reviewee team's submitted content. + def calibration + instructor_map = ReviewResponseMap.find_by!( + id: params[:map_id], + reviewed_object_id: @assignment.id, + for_calibration: true + ) + + instructor_response = instructor_map.latest_submitted_response + return render_error('Submitted instructor calibration response not found', :unprocessable_entity) unless instructor_response + + rubric_items = instructor_response.rubric_items + return render_error('Review rubric not found', :unprocessable_entity) if rubric_items.empty? + + student_responses = ReviewResponseMap.peer_calibration_responses_for(instructor_map) + + per_item_summary = CalibrationPerItemSummary.build( + items: rubric_items, + instructor_response: instructor_response, + student_responses: student_responses + ) + + reviewee = instructor_map.reviewee + + render json: { + map_id: instructor_map.id, + assignment_id: @assignment.id, + reviewee_id: instructor_map.reviewee_id, + rubric_items: rubric_items.map(&:as_calibration_json), + instructor_response: instructor_response.as_calibration_json, + student_responses: student_responses.map(&:as_calibration_json), + per_item_summary: per_item_summary, + submitted_content: reviewee.respond_to?(:submitted_content) ? reviewee.submitted_content : { hyperlinks: [], files: [] } + }, status: :ok + rescue ActiveRecord::RecordNotFound + render_error('Calibration review map not found', :not_found) + end + + private + + def set_assignment + @assignment = Assignment.find(params[:assignment_id]) + end + + def render_error(message, status) + render json: { error: message }, status: status + end +end diff --git a/app/controllers/review_mappings_controller.rb b/app/controllers/review_mappings_controller.rb index ab5f7f4c3..1bc1cd382 100644 --- a/app/controllers/review_mappings_controller.rb +++ b/app/controllers/review_mappings_controller.rb @@ -62,37 +62,75 @@ def assign_calibration_artifacts render json: { status: "ok", message: "Calibration reviews assigned to all reviewers" } end - def calibration_report - instructor_map = ReviewResponseMap.find_by!( - id: params[:id], - reviewed_object_id: @assignment.id, - for_calibration: true - ) - instructor_response = latest_submitted_response_for(instructor_map) - return render json: { error: "Submitted instructor calibration response not found" }, status: :unprocessable_entity unless instructor_response - - rubric_items = rubric_items_for(instructor_response) - return render json: { error: "Review rubric not found" }, status: :unprocessable_entity if rubric_items.empty? - - student_responses = submitted_student_responses_for(instructor_map) - per_item_summary = CalibrationPerItemSummary.build( - items: rubric_items, - instructor_response: instructor_response, - student_responses: student_responses - ) - + # ===== CALIBRATION PARTICIPANTS ===== + # The "Calibration" tab on the assignment edit view designates one or more + # users as calibration submitters. The instructor types a username into the + # text box and that user is added as an AssignmentParticipant, a team is + # created for them (so submissions flow through the normal team-based + # infrastructure), and a ReviewResponseMap with for_calibration = true is + # created with the instructor as reviewer. The instructor later opens the + # calibration report for this map to enter/compare the calibration review. + + # GET /assignments/:assignment_id/review_mappings/calibration_participants + def list_calibration_participants render json: { - map_id: instructor_map.id, assignment_id: @assignment.id, - reviewee_id: instructor_map.reviewee_id, - rubric_items: rubric_items.map { |item| serialize_item(item) }, - instructor_response: serialize_response(instructor_response), - student_responses: student_responses.map { |response| serialize_response(response) }, - per_item_summary: per_item_summary, - submitted_content: submitted_content_for(instructor_map.reviewee) + calibration_participants: calibration_participant_rows }, status: :ok - rescue ActiveRecord::RecordNotFound - render json: { error: "Calibration review map not found" }, status: :not_found + end + + # POST /assignments/:assignment_id/review_mappings/calibration_participants + # Body: { username: "unctlt1" } + def add_calibration_participant + username = (params[:username] || params.dig(:calibration_participant, :username)).to_s.strip + return render json: { error: 'username is required' }, status: :bad_request if username.blank? + + user = User.find_by(name: username) || User.find_by(email: username) + return render json: { error: "User '#{username}' not found" }, status: :not_found unless user + + instructor_participant = find_or_create_instructor_participant + return render json: { error: 'Assignment has no instructor' }, status: :unprocessable_entity unless instructor_participant + + participant = nil + team = nil + map = nil + + ActiveRecord::Base.transaction do + participant = AssignmentParticipant.find_by(parent_id: @assignment.id, user_id: user.id) || + @assignment.add_participant(user.id) + + team = participant.team || Team.create_team_for_participant(participant) + + map = ReviewResponseMap.find_or_create_by!( + reviewer_id: instructor_participant.id, + reviewee_id: team.id, + reviewed_object_id: @assignment.id, + for_calibration: true + ) + end + + render json: serialize_calibration_row(participant, team, map), status: :created + rescue ActiveRecord::RecordInvalid, ArgumentError => e + render json: { error: e.message }, status: :unprocessable_entity + rescue StandardError => e + render json: { error: e.message }, status: :unprocessable_entity + end + + # DELETE /assignments/:assignment_id/review_mappings/calibration_participants/:participant_id + def remove_calibration_participant + participant = AssignmentParticipant.find_by(id: params[:participant_id], parent_id: @assignment.id) + return render json: { error: 'Calibration participant not found' }, status: :not_found unless participant + + team = participant.team + return render json: { error: 'Participant has no team' }, status: :unprocessable_entity unless team + + ReviewResponseMap.where( + reviewed_object_id: @assignment.id, + reviewee_id: team.id, + for_calibration: true + ).destroy_all + + render json: { message: "Calibration participant #{participant.id} removed." }, status: :ok end # ===== DELETE ===== @@ -118,8 +156,19 @@ def grade_review render json: { status: "ok", message: "Review graded" } end + # Actions that designate calibration submitters require teaching staff + # privileges; everything else defaults to allowed and relies on + # ApplicationController's own checks. We reuse the shared + # `current_user_teaching_staff_of_assignment?` helper from the Authorization + # concern instead of duplicating the logic here. + CALIBRATION_PARTICIPANT_ACTIONS = %w[ + list_calibration_participants + add_calibration_participant + remove_calibration_participant + ].freeze + def action_allowed? - return teaching_staff_for_calibration_report? if params[:action] == "calibration_report" + return current_user_teaching_staff_of_assignment?(params[:assignment_id]) if CALIBRATION_PARTICIPANT_ACTIONS.include?(params[:action]) true end @@ -130,70 +179,54 @@ def set_assignment @assignment = Assignment.find(params[:assignment_id]) end - def teaching_staff_for_calibration_report? - assignment = Assignment.find_by(id: params[:assignment_id]) - return false unless user_logged_in? && assignment - return true if assignment.instructor_id == current_user.id + # ----- Helpers for the calibration participants endpoints ----- - assignment.course_id.present? && TaMapping.exists?(user_id: current_user.id, course_id: assignment.course_id) - end + # The instructor is the reviewer on every calibration ReviewResponseMap it + # creates, so the instructor must also be registered as an + # AssignmentParticipant on this assignment. Create that record lazily. + def find_or_create_instructor_participant + instructor = @assignment.instructor + return nil unless instructor - def latest_submitted_response_for(response_map) - response_map.responses.where(is_submitted: true).order(updated_at: :desc).first + AssignmentParticipant.find_by(parent_id: @assignment.id, user_id: instructor.id) || + @assignment.add_participant(instructor.id) end - def submitted_student_responses_for(instructor_map) - student_maps = ReviewResponseMap - .where( - reviewed_object_id: @assignment.id, - reviewee_id: instructor_map.reviewee_id, - for_calibration: true - ) - .where.not(id: instructor_map.id) + # Build one row per calibration submitter. A submitter is identified as the + # (sole) member of a team that is the reviewee of any for_calibration map on + # this assignment. Prefer the instructor's map as the "Begin" target. + def calibration_participant_rows + maps = ReviewResponseMap.where( + reviewed_object_id: @assignment.id, + for_calibration: true + ).order(:id) - student_maps.flat_map { |map| map.responses.where(is_submitted: true).to_a } - end + instructor_user_id = @assignment.instructor_id + maps_by_team = maps.group_by(&:reviewee_id) - def rubric_items_for(response) - response.questionnaire.items.order(:seq) - rescue NoMethodError - [] - end + maps_by_team.map do |team_id, team_maps| + team = AssignmentTeam.find_by(id: team_id) + next nil unless team - def serialize_item(item) - { - id: item.id, - txt: item.txt, - seq: item.seq, - question_type: item.question_type, - weight: item.weight, - min_score: item.questionnaire.min_question_score, - max_score: item.questionnaire.max_question_score - } - end + instructor_map = team_maps.find { |m| m.reviewer&.user_id == instructor_user_id } || team_maps.first + submitter = team.participants.where(type: 'AssignmentParticipant').first + next nil unless submitter - def serialize_response(response) - { - id: response.id, - map_id: response.map_id, - reviewer_id: response.map.reviewer_id, - reviewer_name: response.map.reviewer&.fullname, - is_submitted: response.is_submitted, - updated_at: response.updated_at, - answers: response.scores.map do |answer| - { - item_id: answer.item_id, - score: answer.answer, - comments: answer.comments - } - end - } + serialize_calibration_row(submitter, team, instructor_map) + end.compact end - def submitted_content_for(reviewee) + def serialize_calibration_row(participant, team, instructor_map) { - hyperlinks: reviewee.respond_to?(:hyperlinks) ? reviewee.hyperlinks : [], - files: SubmissionRecord.files.where(team_id: reviewee.id, assignment_id: @assignment.id).pluck(:content) + participant_id: participant.id, + user_id: participant.user_id, + username: participant.user&.name, + full_name: participant.user&.full_name, + handle: participant.handle, + team_id: team&.id, + team_name: team&.name, + instructor_review_map_id: instructor_map&.id, + submissions: team.respond_to?(:submitted_content_detail) ? team.submitted_content_detail : { hyperlinks: [], files: [] } } end end diff --git a/app/models/Item.rb b/app/models/Item.rb index 0b6535228..08ec7c214 100644 --- a/app/models/Item.rb +++ b/app/models/Item.rb @@ -33,6 +33,21 @@ def as_json(options = {}) end end + # JSON representation used by calibration reports. Kept on the item itself + # (Information Expert) so other report/view code does not re-implement this + # shape. + def as_calibration_json + { + id: id, + txt: txt, + seq: seq, + question_type: question_type, + weight: weight, + min_score: questionnaire.min_question_score, + max_score: questionnaire.max_question_score + } + end + def strategy case question_type when 'dropdown' diff --git a/app/models/assignment_team.rb b/app/models/assignment_team.rb index 2bf8e6815..0a18fa0fc 100644 --- a/app/models/assignment_team.rb +++ b/app/models/assignment_team.rb @@ -16,6 +16,39 @@ def hyperlinks submitted_hyperlinks.blank? ? [] : YAML.safe_load(submitted_hyperlinks) end + # SubmissionRecords for files this team has submitted against its assignment. + # Lives on the team (Information Expert) so callers don't have to know about + # SubmissionRecord's schema. + def submitted_file_records + SubmissionRecord.files.where(team_id: id, assignment_id: parent_id).order(created_at: :asc) + end + + # Submitted content in the simple shape expected by reports: hyperlinks and + # file paths only. Used by calibration/report views where we just need the + # URLs. + def submitted_content + { + hyperlinks: hyperlinks, + files: submitted_file_records.pluck(:content) + } + end + + # Submitted content in the rich shape expected by the calibration + # participants listing (per-file id, name, path, timestamps, submitter). + def submitted_content_detail + files = submitted_file_records.map do |record| + { + id: record.id, + name: File.basename(record.content.to_s), + path: record.content, + submitted_at: record.created_at, + submitted_by: record.user + } + end + + { hyperlinks: hyperlinks, files: files } + end + def submit_hyperlink(hyperlink) hyperlink.strip! raise 'The hyperlink cannot be empty!' if hyperlink.empty? diff --git a/app/models/response.rb b/app/models/response.rb index f99db770f..4fa07bd06 100644 --- a/app/models/response.rb +++ b/app/models/response.rb @@ -62,6 +62,38 @@ def aggregate_questionnaire_score sum end + # Ordered rubric items used for this response, or [] if the rubric cannot be + # resolved (e.g. no AssignmentQuestionnaire for this round). Belongs here + # rather than in a controller because the response knows which questionnaire + # applies to it. + def rubric_items + questionnaire.items.order(:seq) + rescue NoMethodError + [] + end + + # JSON representation of this response for calibration reports. Keeps the + # serialization next to the class that owns the data (Information Expert), + # so any consumer that needs a response rendered for a calibration view can + # ask the response for it. + def as_calibration_json + { + id: id, + map_id: map_id, + reviewer_id: map.reviewer_id, + reviewer_name: map.reviewer&.fullname, + is_submitted: is_submitted, + updated_at: updated_at, + answers: scores.map do |answer| + { + item_id: answer.item_id, + score: answer.answer, + comments: answer.comments + } + end + } + end + # Returns the maximum possible score for this response def maximum_score # only count the scorable questions, only when the answer is not nil (we accept nil as diff --git a/app/models/response_map.rb b/app/models/response_map.rb index 185d9dd77..da6ffe002 100644 --- a/app/models/response_map.rb +++ b/app/models/response_map.rb @@ -20,6 +20,13 @@ def response_assignment return reviewer.assignment end + # Most recently submitted response on this map, or nil. Callers that need + # "the map's current submitted response" should ask the map directly rather + # than re-query Response in a controller. + def latest_submitted_response + responses.where(is_submitted: true).order(updated_at: :desc).first + end + def self.assessments_for(team) responses = [] # stime = Time.now diff --git a/app/models/review_response_map.rb b/app/models/review_response_map.rb index ce38793dc..072587a53 100644 --- a/app/models/review_response_map.rb +++ b/app/models/review_response_map.rb @@ -20,4 +20,18 @@ def get_title def review_map_type 'ReviewResponseMap' end + + # Collect the student-submitted calibration responses that share a reviewee + # with the given instructor calibration map. These are the responses that + # should be compared against the instructor's review in a calibration + # report. + def self.peer_calibration_responses_for(instructor_map) + peer_maps = where( + reviewed_object_id: instructor_map.reviewed_object_id, + reviewee_id: instructor_map.reviewee_id, + for_calibration: true + ).where.not(id: instructor_map.id) + + peer_maps.flat_map { |map| map.responses.where(is_submitted: true).to_a } + end end diff --git a/app/models/team.rb b/app/models/team.rb index e2c39fbaa..02ee30c79 100644 --- a/app/models/team.rb +++ b/app/models/team.rb @@ -22,6 +22,61 @@ class Team < ApplicationRecord after_update :release_topics_if_empty + # Factory method that creates the appropriate subclass of Team for the given + # participant and enrolls that participant as a member. + # + # - AssignmentParticipant => AssignmentTeam (parent = assignment) + # - CourseParticipant => CourseTeam (parent = course) + # + # This is the mechanism used to back individual submitters (including the + # phantom "calibration submitters" that an instructor adds on the Calibration + # tab). Expertiza routes every submission through a team, so even a single + # submitter needs a team behind the scenes. + # + # Params: + # participant - an AssignmentParticipant or CourseParticipant + # name: - optional team name; if omitted a stable unique name is + # generated from the user's handle/name. + # + # Returns the newly-created team (AssignmentTeam or CourseTeam). + # Raises ArgumentError if the participant type is not supported. + def self.create_team_for_participant(participant, name: nil) + raise ArgumentError, 'participant is required' if participant.nil? + + team_class, parent = + case participant + when AssignmentParticipant + [AssignmentTeam, participant.assignment] + when CourseParticipant + [CourseTeam, participant.course] + else + raise ArgumentError, "Unsupported participant type: #{participant.class}" + end + + raise ArgumentError, 'participant is not associated with an assignment or course' if parent.nil? + + team_name = name.presence || generate_team_name_for(participant, parent) + + team = team_class.create!(parent_id: parent.id, name: team_name) + result = team.add_member(participant) + + unless result.is_a?(Hash) && result[:success] + team.destroy + error = result.is_a?(Hash) ? result[:error] : 'Unable to add participant to new team' + raise ActiveRecord::RecordInvalid.new(team), error + end + + team + end + + def self.generate_team_name_for(participant, parent) + base = (participant.user&.handle.presence || participant.user&.name.presence || "participant_#{participant.id}").to_s + candidate = "#{base}_team" + return candidate unless Team.exists?(parent_id: parent.id, name: candidate) + + "#{candidate}_#{SecureRandom.hex(3)}" + end + def has_member?(user) participants.exists?(user_id: user.id) end diff --git a/config/routes.rb b/config/routes.rb index ab3b63394..8a95c9ccb 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -23,11 +23,11 @@ get 'role/:name', action: :role_users end end - resources :assignments do - collection do - post '/:assignment_id/add_participant/:user_id',action: :add_participant - delete '/:assignment_id/remove_participant/:user_id',action: :remove_participant - patch '/:assignment_id/remove_assignment_from_course',action: :remove_assignment_from_course + resources :assignments do + collection do + post '/:assignment_id/add_participant/:user_id',action: :add_participant + delete '/:assignment_id/remove_participant/:user_id',action: :remove_participant + patch '/:assignment_id/remove_assignment_from_course',action: :remove_assignment_from_course patch '/:assignment_id/assign_course/:course_id',action: :assign_course post '/:assignment_id/copy_assignment', action: :copy_assignment get '/:assignment_id/has_topics',action: :has_topics @@ -35,16 +35,26 @@ get '/:assignment_id/team_assignment', action: :team_assignment get '/:assignment_id/has_teams', action: :has_teams get '/:assignment_id/valid_num_review/:review_type', action: :valid_num_review - get '/:assignment_id/varying_rubrics_by_round', action: :varying_rubrics_by_round? - post '/:assignment_id/create_node',action: :create_node - end - - resources :review_mappings, only: [] do - member do - get :calibration_report - end - end - end + get '/:assignment_id/varying_rubrics_by_round', action: :varying_rubrics_by_round? + post '/:assignment_id/create_node',action: :create_node + end + + resources :review_mappings, only: [] do + collection do + get :calibration_participants, action: :list_calibration_participants + post :calibration_participants, action: :add_calibration_participant + delete 'calibration_participants/:participant_id', action: :remove_calibration_participant + end + end + + # Reports are owned by ReportsController, not by feature controllers. + # Adding a new report type only requires a new action here. + resources :reports, only: [] do + collection do + get 'calibration/:map_id', action: :calibration, as: :calibration + end + end + end resources :bookmarks, except: [:new, :edit] do member do diff --git a/spec/models/team_factory_spec.rb b/spec/models/team_factory_spec.rb new file mode 100644 index 000000000..736353033 --- /dev/null +++ b/spec/models/team_factory_spec.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Tests for Team.create_team_for_participant - the factory used by the +# Calibration tab (and elsewhere) to create the team that backs an individual +# submitter. +RSpec.describe Team, type: :model do + include RolesHelper + + before(:all) { @roles = create_roles_hierarchy } + + let(:institution) { Institution.create!(name: 'NC State') } + + def create_student(suffix) + User.create!( + name: suffix, + email: "#{suffix}@example.com", + full_name: suffix.titleize, + password_digest: 'password', + role_id: @roles[:student].id, + institution_id: institution.id + ) + end + + let(:instructor) do + User.create!( + name: 'calibration_instructor_factory', + full_name: 'Calibration Instructor', + email: 'calibration_instructor_factory@example.com', + password_digest: 'password', + role_id: @roles[:instructor].id, + institution_id: institution.id + ) + end + + let(:assignment) { Assignment.create!(name: 'Calibration A', instructor_id: instructor.id, max_team_size: 1) } + let(:course) { Course.create!(name: 'Course C', instructor_id: instructor.id, institution_id: institution.id, directory_path: '/c1') } + + describe '.create_team_for_participant' do + context 'for an AssignmentParticipant' do + it 'creates an AssignmentTeam whose parent is the assignment' do + user = create_student('calib_sub1') + participant = AssignmentParticipant.create!(parent_id: assignment.id, user: user, handle: user.name) + + team = Team.create_team_for_participant(participant) + + expect(team).to be_a(AssignmentTeam) + expect(team.parent_id).to eq(assignment.id) + expect(team.participants).to include(participant) + end + + it 'generates a stable, unique name when none is provided' do + user1 = create_student('calib_sub_a') + user2 = create_student('calib_sub_b') + p1 = AssignmentParticipant.create!(parent_id: assignment.id, user: user1, handle: user1.name) + p2 = AssignmentParticipant.create!(parent_id: assignment.id, user: user2, handle: user2.name) + + t1 = Team.create_team_for_participant(p1) + t2 = Team.create_team_for_participant(p2) + + expect(t1.name).to include(user1.name) + expect(t2.name).to include(user2.name) + expect(t1.name).not_to eq(t2.name) + end + end + + context 'for a CourseParticipant' do + it 'creates a CourseTeam whose parent is the course' do + user = create_student('course_sub1') + participant = CourseParticipant.create!(parent_id: course.id, user: user, handle: user.name) + + team = Team.create_team_for_participant(participant) + + expect(team).to be_a(CourseTeam) + expect(team.parent_id).to eq(course.id) + expect(team.participants).to include(participant) + end + end + + it 'raises on nil participant' do + expect { Team.create_team_for_participant(nil) }.to raise_error(ArgumentError) + end + + it 'raises on an unsupported participant type' do + fake = Object.new + expect { Team.create_team_for_participant(fake) }.to raise_error(ArgumentError, /Unsupported/) + end + end +end diff --git a/spec/requests/api/v1/calibration_participants_spec.rb b/spec/requests/api/v1/calibration_participants_spec.rb new file mode 100644 index 000000000..5bd2a3ec2 --- /dev/null +++ b/spec/requests/api/v1/calibration_participants_spec.rb @@ -0,0 +1,158 @@ +# frozen_string_literal: true + +require 'rails_helper' +require 'json_web_token' + +RSpec.describe 'Calibration participants API (on ReviewMappingsController)', type: :request do + let!(:super_admin_role) { Role.find_or_create_by!(name: 'Super Administrator') } + let!(:admin_role) { Role.find_or_create_by!(name: 'Administrator', parent: super_admin_role) } + let!(:instructor_role) { Role.find_or_create_by!(name: 'Instructor', parent: admin_role) } + let!(:ta_role) { Role.find_or_create_by!(name: 'Teaching Assistant', parent: instructor_role) } + let!(:student_role) { Role.find_or_create_by!(name: 'Student', parent: ta_role) } + + let(:institution) { Institution.create!(name: 'NC State') } + + let(:instructor_user) do + User.create!( + name: 'calib_instr', + full_name: 'Calibration Instructor', + email: 'calib_instr@example.com', + password: 'password', + role: instructor_role, + institution: institution + ) + end + + let(:student_user) do + User.create!( + name: 'calib_student_user', + full_name: 'Calibration Student', + email: 'calib_student_user@example.com', + password: 'password', + role: student_role, + institution: institution + ) + end + + let(:phantom_user) do + User.create!( + name: 'unctlt1', + full_name: 'UNC TLT 1', + email: 'unctlt1@example.com', + password: 'password', + role: student_role, + institution: institution + ) + end + + let(:assignment) do + Assignment.create!(name: 'Calibration Assignment', instructor: instructor_user, max_team_size: 1) + end + + let(:base_path) { "/assignments/#{assignment.id}/review_mappings/calibration_participants" } + let(:instr_header) { { 'Authorization' => "Bearer #{JsonWebToken.encode(id: instructor_user.id)}" } } + let(:student_hdr) { { 'Authorization' => "Bearer #{JsonWebToken.encode(id: student_user.id)}" } } + + describe 'POST .../calibration_participants' do + it 'adds a calibration participant, creates a team, and creates a for_calibration map for the instructor' do + phantom_user + + expect { + post base_path, params: { username: 'unctlt1' }, headers: instr_header + }.to change { AssignmentParticipant.where(parent_id: assignment.id, user_id: phantom_user.id).count }.by(1) + .and change { AssignmentTeam.where(parent_id: assignment.id).count }.by(1) + .and change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count }.by(1) + + expect(response).to have_http_status(:created) + json = JSON.parse(response.body) + expect(json['username']).to eq('unctlt1') + expect(json['team_id']).to be_present + expect(json['instructor_review_map_id']).to be_present + + map = ReviewResponseMap.find(json['instructor_review_map_id']) + expect(map.for_calibration).to be true + expect(map.reviewed_object_id).to eq(assignment.id) + + instructor_participant = AssignmentParticipant.find_by(parent_id: assignment.id, user_id: instructor_user.id) + expect(map.reviewer_id).to eq(instructor_participant.id) + end + + it 'returns 400 when username is missing' do + post base_path, params: {}, headers: instr_header + expect(response).to have_http_status(:bad_request) + end + + it 'returns 404 when the user cannot be found' do + post base_path, params: { username: 'nope' }, headers: instr_header + expect(response).to have_http_status(:not_found) + end + + it 'is idempotent for the same username' do + phantom_user + + post base_path, params: { username: 'unctlt1' }, headers: instr_header + expect(response).to have_http_status(:created) + + expect { + post base_path, params: { username: 'unctlt1' }, headers: instr_header + }.not_to change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count } + end + + it 'returns 403 for non-teaching-staff' do + phantom_user + post base_path, params: { username: 'unctlt1' }, headers: student_hdr + expect(response).to have_http_status(:forbidden) + end + end + + describe 'GET .../calibration_participants' do + it 'lists calibration submitters with their submissions' do + phantom_user + post base_path, params: { username: 'unctlt1' }, headers: instr_header + expect(response).to have_http_status(:created) + row = JSON.parse(response.body) + + team = AssignmentTeam.find(row['team_id']) + SubmissionRecord.create!( + record_type: 'file', + content: 'submission/report.pdf', + operation: 'Submit File', + team_id: team.id, + user: phantom_user.name, + assignment_id: assignment.id + ) + team.update!(submitted_hyperlinks: YAML.dump(['https://example.com/submission'])) + + get base_path, headers: instr_header + expect(response).to have_http_status(:ok) + + json = JSON.parse(response.body) + expect(json['assignment_id']).to eq(assignment.id) + expect(json['calibration_participants'].length).to eq(1) + + entry = json['calibration_participants'].first + expect(entry['username']).to eq('unctlt1') + expect(entry['submissions']['hyperlinks']).to eq(['https://example.com/submission']) + expect(entry['submissions']['files'].first).to include( + 'name' => 'report.pdf', + 'path' => 'submission/report.pdf', + 'submitted_by' => phantom_user.name + ) + expect(entry['instructor_review_map_id']).to eq(row['instructor_review_map_id']) + end + end + + describe 'DELETE .../calibration_participants/:participant_id' do + it 'removes the for_calibration maps for the submitter' do + phantom_user + post base_path, params: { username: 'unctlt1' }, headers: instr_header + participant_id = JSON.parse(response.body)['participant_id'] + + expect { + delete "#{base_path}/#{participant_id}", headers: instr_header + }.to change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count }.to(0) + + expect(response).to have_http_status(:ok) + end + end +end diff --git a/spec/requests/api/v1/review_mappings_calibration_report_spec.rb b/spec/requests/api/v1/reports_calibration_spec.rb similarity index 94% rename from spec/requests/api/v1/review_mappings_calibration_report_spec.rb rename to spec/requests/api/v1/reports_calibration_spec.rb index fc3e549b7..a4c3422f1 100644 --- a/spec/requests/api/v1/review_mappings_calibration_report_spec.rb +++ b/spec/requests/api/v1/reports_calibration_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' require 'json_web_token' -RSpec.describe 'ReviewMappings calibration report', type: :request do +RSpec.describe 'Reports calibration', type: :request do let!(:super_admin_role) { Role.create!(name: 'Super Administrator') } let!(:admin_role) { Role.create!(name: 'Administrator') } let!(:instructor_role) { Role.create!(name: 'Instructor') } @@ -48,7 +48,7 @@ documentation end - describe 'GET /assignments/:assignment_id/review_mappings/:id/calibration_report' do + describe 'GET /assignments/:assignment_id/reports/calibration/:map_id' do it 'returns report JSON for a calibration map' do create_response( map: instructor_map, @@ -104,7 +104,7 @@ end it 'returns 404 when the calibration map does not exist' do - get "/assignments/#{assignment.id}/review_mappings/0/calibration_report", headers: instructor_headers + get "/assignments/#{assignment.id}/reports/calibration/0", headers: instructor_headers expect(response).to have_http_status(:not_found) expect(JSON.parse(response.body)['error']).to eq('Calibration review map not found') @@ -129,7 +129,7 @@ end def calibration_report_path(map) - "/assignments/#{assignment.id}/review_mappings/#{map.id}/calibration_report" + "/assignments/#{assignment.id}/reports/calibration/#{map.id}" end def auth_headers_for(user) From 673b65b728c3677664d526e7314e51a401644ffb Mon Sep 17 00:00:00 2001 From: ruju4a Date: Sat, 25 Apr 2026 01:21:44 -0400 Subject: [PATCH 09/25] integrated add calibration particiapant and view report changes --- app/controllers/review_mappings_controller.rb | 49 +++- config/routes.rb | 7 + .../api/v1/calibration_participants_spec.rb | 258 ++++++++++++++---- 3 files changed, 254 insertions(+), 60 deletions(-) diff --git a/app/controllers/review_mappings_controller.rb b/app/controllers/review_mappings_controller.rb index 1bc1cd382..040a0a4be 100644 --- a/app/controllers/review_mappings_controller.rb +++ b/app/controllers/review_mappings_controller.rb @@ -116,6 +116,36 @@ def add_calibration_participant render json: { error: e.message }, status: :unprocessable_entity end + # DEMO_INSTRUCTOR_RESPONSE + # POST /assignments/:assignment_id/review_mappings/:map_id/mock_instructor_response + # + # Demo-only: materializes a submitted instructor calibration Response with + # default answers for the given calibration map so the calibration report + # has data to display while the real review form lives outside this + # project's scope. The "how" of fabricating data lives in + # Demo::CalibrationInstructorSeeder; this controller is responsible only for + # locating the map and translating outcomes into HTTP responses. + # See Demo::CalibrationInstructorSeeder for the full removal checklist. + def submit_mock_instructor_calibration_response + map = ReviewResponseMap.find_by!( + id: params[:map_id], + reviewed_object_id: @assignment.id, + for_calibration: true + ) + + response = Demo::CalibrationInstructorSeeder.seed!(map) + + render json: { + map_id: map.id, + response_id: response.id, + instructor_review_status: 'submitted' + }, status: :ok + rescue ActiveRecord::RecordNotFound + render json: { error: 'Calibration review map not found' }, status: :not_found + rescue ArgumentError, ActiveRecord::RecordInvalid => e + render json: { error: e.message }, status: :unprocessable_entity + end + # DELETE /assignments/:assignment_id/review_mappings/calibration_participants/:participant_id def remove_calibration_participant participant = AssignmentParticipant.find_by(id: params[:participant_id], parent_id: @assignment.id) @@ -165,7 +195,8 @@ def grade_review list_calibration_participants add_calibration_participant remove_calibration_participant - ].freeze + submit_mock_instructor_calibration_response + ].freeze # DEMO_INSTRUCTOR_RESPONSE: drop the last entry when the demo seeder is removed. def action_allowed? return current_user_teaching_staff_of_assignment?(params[:assignment_id]) if CALIBRATION_PARTICIPANT_ACTIONS.include?(params[:action]) @@ -226,7 +257,23 @@ def serialize_calibration_row(participant, team, instructor_map) team_id: team&.id, team_name: team&.name, instructor_review_map_id: instructor_map&.id, + instructor_review_status: instructor_review_status_for(instructor_map), submissions: team.respond_to?(:submitted_content_detail) ? team.submitted_content_detail : { hyperlinks: [], files: [] } } end + + # Classify the instructor's progress on a calibration review map so the UI can + # show "Begin" when no response exists yet and "View | Edit" once one does. + # - :not_started -> no Response rows for this map + # - :in_progress -> at least one Response, none submitted yet + # - :submitted -> at least one submitted Response + def instructor_review_status_for(instructor_map) + return :not_started unless instructor_map + + responses = instructor_map.responses + return :not_started if responses.empty? + return :submitted if responses.where(is_submitted: true).exists? + + :in_progress + end end diff --git a/config/routes.rb b/config/routes.rb index 8a95c9ccb..9fbed4a56 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -44,6 +44,13 @@ get :calibration_participants, action: :list_calibration_participants post :calibration_participants, action: :add_calibration_participant delete 'calibration_participants/:participant_id', action: :remove_calibration_participant + # DEMO_INSTRUCTOR_RESPONSE + # Demo-only: seeds an instructor calibration Response for the given + # map. Wired to the "Begin" link on the Calibration tab while the + # real review form lives outside this project's scope. + # See Demo::CalibrationInstructorSeeder for the full removal checklist. + post ':map_id/mock_instructor_response', + action: :submit_mock_instructor_calibration_response end end diff --git a/spec/requests/api/v1/calibration_participants_spec.rb b/spec/requests/api/v1/calibration_participants_spec.rb index 5bd2a3ec2..890f5db0e 100644 --- a/spec/requests/api/v1/calibration_participants_spec.rb +++ b/spec/requests/api/v1/calibration_participants_spec.rb @@ -3,7 +3,20 @@ require 'rails_helper' require 'json_web_token' -RSpec.describe 'Calibration participants API (on ReviewMappingsController)', type: :request do +# Behaviour-driven specs for the "Add calibration participant" feature on the +# assignment Calibration tab. Scenarios are written from the perspective of the +# instructor using the UI and exercise the public HTTP contract end-to-end: +# +# * GET /assignments/:id/review_mappings/calibration_participants +# * POST /assignments/:id/review_mappings/calibration_participants +# * DELETE /assignments/:id/review_mappings/calibration_participants/:participant_id +# +# Each `it` block reads as a scenario with explicit Given / When / Then phases +# so the spec doubles as living documentation of the feature's behaviour. +RSpec.describe 'Feature: Designating calibration submitters', type: :request do + # --------------------------------------------------------------------------- + # Background fixtures (the "Given" that every scenario shares) + # --------------------------------------------------------------------------- let!(:super_admin_role) { Role.find_or_create_by!(name: 'Super Administrator') } let!(:admin_role) { Role.find_or_create_by!(name: 'Administrator', parent: super_admin_role) } let!(:instructor_role) { Role.find_or_create_by!(name: 'Instructor', parent: admin_role) } @@ -14,34 +27,26 @@ let(:instructor_user) do User.create!( - name: 'calib_instr', - full_name: 'Calibration Instructor', - email: 'calib_instr@example.com', - password: 'password', - role: instructor_role, - institution: institution + name: 'calib_instr', full_name: 'Calibration Instructor', + email: 'calib_instr@example.com', password: 'password', + role: instructor_role, institution: institution ) end - let(:student_user) do + let(:other_student) do User.create!( - name: 'calib_student_user', - full_name: 'Calibration Student', - email: 'calib_student_user@example.com', - password: 'password', - role: student_role, - institution: institution + name: 'calib_student_user', full_name: 'Calibration Student', + email: 'calib_student_user@example.com', password: 'password', + role: student_role, institution: institution ) end + # The phantom calibration submitter the instructor will type into the form. let(:phantom_user) do User.create!( - name: 'unctlt1', - full_name: 'UNC TLT 1', - email: 'unctlt1@example.com', - password: 'password', - role: student_role, - institution: institution + name: 'unctlt1', full_name: 'UNC TLT 1', + email: 'unctlt1@example.com', password: 'password', + role: student_role, institution: institution ) end @@ -49,110 +54,245 @@ Assignment.create!(name: 'Calibration Assignment', instructor: instructor_user, max_team_size: 1) end - let(:base_path) { "/assignments/#{assignment.id}/review_mappings/calibration_participants" } - let(:instr_header) { { 'Authorization' => "Bearer #{JsonWebToken.encode(id: instructor_user.id)}" } } - let(:student_hdr) { { 'Authorization' => "Bearer #{JsonWebToken.encode(id: student_user.id)}" } } + let(:base_path) { "/assignments/#{assignment.id}/review_mappings/calibration_participants" } + let(:as_instructor) { { 'Authorization' => "Bearer #{JsonWebToken.encode(id: instructor_user.id)}" } } + let(:as_student) { { 'Authorization' => "Bearer #{JsonWebToken.encode(id: other_student.id)}" } } - describe 'POST .../calibration_participants' do - it 'adds a calibration participant, creates a team, and creates a for_calibration map for the instructor' do + # Convenience helper used in many "When" steps. + def add_calibration_participant(username, headers: as_instructor) + post base_path, params: { username: username }, headers: headers + end + + # --------------------------------------------------------------------------- + # Scenario group: adding a participant + # --------------------------------------------------------------------------- + describe 'Scenario: an instructor adds a new calibration submitter by username' do + it 'creates the participant, the team, and the for_calibration map atomically' do + # Given the phantom user exists phantom_user + # When the instructor submits "unctlt1" through the calibration tab expect { - post base_path, params: { username: 'unctlt1' }, headers: instr_header + add_calibration_participant('unctlt1') }.to change { AssignmentParticipant.where(parent_id: assignment.id, user_id: phantom_user.id).count }.by(1) .and change { AssignmentTeam.where(parent_id: assignment.id).count }.by(1) .and change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count }.by(1) + # Then the API responds 201 with the new row expect(response).to have_http_status(:created) json = JSON.parse(response.body) - expect(json['username']).to eq('unctlt1') + expect(json).to include( + 'username' => 'unctlt1', + 'full_name' => 'UNC TLT 1' + ) expect(json['team_id']).to be_present expect(json['instructor_review_map_id']).to be_present + expect(json['instructor_review_status']).to eq('not_started') + # And the calibration map is wired to the instructor as reviewer map = ReviewResponseMap.find(json['instructor_review_map_id']) expect(map.for_calibration).to be true expect(map.reviewed_object_id).to eq(assignment.id) instructor_participant = AssignmentParticipant.find_by(parent_id: assignment.id, user_id: instructor_user.id) expect(map.reviewer_id).to eq(instructor_participant.id) + expect(map.reviewee_id).to eq(json['team_id']) + end + + it 'accepts an email address in place of a username' do + # Given the phantom user exists + phantom_user + + # When the instructor types the user's email instead of their handle + add_calibration_participant('unctlt1@example.com') + + # Then the user is still resolved and added + expect(response).to have_http_status(:created) + expect(JSON.parse(response.body)['username']).to eq('unctlt1') + end + end + + # --------------------------------------------------------------------------- + # Scenario group: invalid input + # --------------------------------------------------------------------------- + describe 'Scenario: invalid input from the instructor' do + it 'rejects a request with a missing username with HTTP 400' do + # When the instructor submits an empty form + post base_path, params: {}, headers: as_instructor + + # Then the request is rejected as bad input + expect(response).to have_http_status(:bad_request) + expect(JSON.parse(response.body)['error']).to match(/username is required/i) end - it 'returns 400 when username is missing' do - post base_path, params: {}, headers: instr_header + it 'rejects a blank/whitespace username with HTTP 400' do + add_calibration_participant(' ') expect(response).to have_http_status(:bad_request) end - it 'returns 404 when the user cannot be found' do - post base_path, params: { username: 'nope' }, headers: instr_header + it 'returns HTTP 404 when no user matches the username or email' do + # When the instructor types a name that does not exist + add_calibration_participant('nope') + + # Then nothing is created and a 404 is returned expect(response).to have_http_status(:not_found) + expect(ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true)).to be_empty end + end - it 'is idempotent for the same username' do + # --------------------------------------------------------------------------- + # Scenario group: idempotence and re-adding + # --------------------------------------------------------------------------- + describe 'Scenario: re-adding the same submitter' do + it 'is idempotent and does not create duplicate maps or teams' do + # Given the participant has already been added once phantom_user + add_calibration_participant('unctlt1') + expect(response).to have_http_status(:created) + + maps_before = ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count + teams_before = AssignmentTeam.where(parent_id: assignment.id).count - post base_path, params: { username: 'unctlt1' }, headers: instr_header + # When the instructor adds the same person again + add_calibration_participant('unctlt1') + + # Then no extra rows are created and the response is still successful expect(response).to have_http_status(:created) + expect(ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count).to eq(maps_before) + expect(AssignmentTeam.where(parent_id: assignment.id).count).to eq(teams_before) + end + end + + # --------------------------------------------------------------------------- + # Scenario group: authorization + # --------------------------------------------------------------------------- + describe 'Scenario: only teaching staff may add calibration submitters' do + it 'rejects students with HTTP 403 and creates nothing' do + phantom_user expect { - post base_path, params: { username: 'unctlt1' }, headers: instr_header - }.not_to change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count } + add_calibration_participant('unctlt1', headers: as_student) + }.not_to change { ReviewResponseMap.count } + + expect(response).to have_http_status(:forbidden) end - it 'returns 403 for non-teaching-staff' do + it 'rejects unauthenticated requests with HTTP 401' do phantom_user - post base_path, params: { username: 'unctlt1' }, headers: student_hdr - expect(response).to have_http_status(:forbidden) + + post base_path, params: { username: 'unctlt1' } + expect([401, 403]).to include(response.status), "expected 401/403 for unauthenticated request, got #{response.status}" end end - describe 'GET .../calibration_participants' do - it 'lists calibration submitters with their submissions' do + # --------------------------------------------------------------------------- + # Scenario group: listing the calibration submitters + # --------------------------------------------------------------------------- + describe 'Scenario: instructor views the list of calibration submitters' do + it 'returns each submitter with their team, hyperlinks, and submitted files' do + # Given a calibration submitter already exists for this assignment phantom_user - post base_path, params: { username: 'unctlt1' }, headers: instr_header - expect(response).to have_http_status(:created) - row = JSON.parse(response.body) + add_calibration_participant('unctlt1') + added_row = JSON.parse(response.body) - team = AssignmentTeam.find(row['team_id']) + # And that submitter has uploaded a file and a hyperlink to their team + team = AssignmentTeam.find(added_row['team_id']) SubmissionRecord.create!( - record_type: 'file', - content: 'submission/report.pdf', - operation: 'Submit File', - team_id: team.id, - user: phantom_user.name, - assignment_id: assignment.id + record_type: 'file', content: 'submission/report.pdf', operation: 'Submit File', + team_id: team.id, user: phantom_user.name, assignment_id: assignment.id ) team.update!(submitted_hyperlinks: YAML.dump(['https://example.com/submission'])) - get base_path, headers: instr_header - expect(response).to have_http_status(:ok) + # When the instructor opens the calibration tab + get base_path, headers: as_instructor + # Then the response contains exactly one calibration submitter row + expect(response).to have_http_status(:ok) json = JSON.parse(response.body) expect(json['assignment_id']).to eq(assignment.id) expect(json['calibration_participants'].length).to eq(1) + # And that row exposes the submitter's hyperlinks and files entry = json['calibration_participants'].first expect(entry['username']).to eq('unctlt1') + expect(entry['instructor_review_map_id']).to eq(added_row['instructor_review_map_id']) expect(entry['submissions']['hyperlinks']).to eq(['https://example.com/submission']) expect(entry['submissions']['files'].first).to include( - 'name' => 'report.pdf', - 'path' => 'submission/report.pdf', + 'name' => 'report.pdf', + 'path' => 'submission/report.pdf', 'submitted_by' => phantom_user.name ) - expect(entry['instructor_review_map_id']).to eq(row['instructor_review_map_id']) + end + + it 'returns an empty list when no calibration submitters have been added' do + # When the instructor opens the calibration tab on a fresh assignment + get base_path, headers: as_instructor + + # Then the list is empty (but the response is well-formed) + expect(response).to have_http_status(:ok) + json = JSON.parse(response.body) + expect(json['assignment_id']).to eq(assignment.id) + expect(json['calibration_participants']).to eq([]) + end + + it 'reflects the instructor review status (not_started -> in_progress -> submitted)' do + # Given a calibration submitter exists + phantom_user + add_calibration_participant('unctlt1') + map_id = JSON.parse(response.body)['instructor_review_map_id'] + map = ReviewResponseMap.find(map_id) + + # Then before any review work the status is :not_started + get base_path, headers: as_instructor + expect(JSON.parse(response.body)['calibration_participants'].first['instructor_review_status']).to eq('not_started') + + # When the instructor saves a draft response + Response.create!(response_map: map, round: 1, version_num: 1, is_submitted: false) + get base_path, headers: as_instructor + expect(JSON.parse(response.body)['calibration_participants'].first['instructor_review_status']).to eq('in_progress') + + # When the instructor submits the response + Response.where(map_id: map.id).update_all(is_submitted: true) + get base_path, headers: as_instructor + expect(JSON.parse(response.body)['calibration_participants'].first['instructor_review_status']).to eq('submitted') end end - describe 'DELETE .../calibration_participants/:participant_id' do - it 'removes the for_calibration maps for the submitter' do + # --------------------------------------------------------------------------- + # Scenario group: removing a submitter + # --------------------------------------------------------------------------- + describe 'Scenario: instructor removes a calibration submitter' do + it 'destroys the for_calibration maps for that submitter' do + # Given a calibration submitter has been added phantom_user - post base_path, params: { username: 'unctlt1' }, headers: instr_header + add_calibration_participant('unctlt1') participant_id = JSON.parse(response.body)['participant_id'] + # When the instructor removes them expect { - delete "#{base_path}/#{participant_id}", headers: instr_header + delete "#{base_path}/#{participant_id}", headers: as_instructor }.to change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count }.to(0) + # Then the API responds 200 OK expect(response).to have_http_status(:ok) + expect(JSON.parse(response.body)['message']).to match(/removed/i) + end + + it 'returns HTTP 404 for a participant id that does not belong to this assignment' do + delete "#{base_path}/999999", headers: as_instructor + expect(response).to have_http_status(:not_found) + end + + it 'rejects students with HTTP 403' do + phantom_user + add_calibration_participant('unctlt1') + participant_id = JSON.parse(response.body)['participant_id'] + + expect { + delete "#{base_path}/#{participant_id}", headers: as_student + }.not_to change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count } + + expect(response).to have_http_status(:forbidden) end end end From d7c0fbad9eeb489f0dcc7c45aa64fda46ae29d4d Mon Sep 17 00:00:00 2001 From: ruju4a Date: Sat, 25 Apr 2026 01:38:26 -0400 Subject: [PATCH 10/25] integrated add calibration particiapant and view report changes --- app/controllers/concerns/authorization.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/controllers/concerns/authorization.rb b/app/controllers/concerns/authorization.rb index 57d1eaf7d..b625bdfcc 100644 --- a/app/controllers/concerns/authorization.rb +++ b/app/controllers/concerns/authorization.rb @@ -172,11 +172,9 @@ def current_user_instructs_assignment?(assignment) ) end - # Determine if the current user and the given assignment are associated by a TA mapping. - # Safe-navigates through course because an assignment may not belong to a course. + # Determine if the current user and the given assignment are associated by a TA mapping def current_user_has_ta_mapping_for_assignment?(assignment) - user_logged_in? && !assignment.nil? && assignment.course_id.present? && - TaMapping.exists?(user_id: current_user.id, course_id: assignment.course_id) + user_logged_in? && !assignment.nil? && TaMapping.exists?(user_id: current_user.id, course_id: assignment.course.id) end # Recursively find an assignment given the passed in Response id. Because a ResponseMap From 65ab35438bf7fc600255e35870c843d5088ed4cf Mon Sep 17 00:00:00 2001 From: xmi Date: Sun, 26 Apr 2026 18:06:58 -0400 Subject: [PATCH 11/25] Seed blank calibration demo comments --- lib/tasks/calibration_demo.rake | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/tasks/calibration_demo.rake b/lib/tasks/calibration_demo.rake index 286276937..292719d2b 100644 --- a/lib/tasks/calibration_demo.rake +++ b/lib/tasks/calibration_demo.rake @@ -54,7 +54,7 @@ namespace :demo do item end - update_response = lambda do |map:, scores:, comments_prefix:| + update_response = lambda do |map:, scores:| response = Response.where(map_id: map.id, is_submitted: true).order(updated_at: :desc).first unless response @@ -72,7 +72,7 @@ namespace :demo do answer = Answer.find_or_initialize_by(response_id: response.id, item_id: item.id) answer.update!( answer: score, - comments: "#{comments_prefix}: #{item.txt} => #{score}" + comments: '' ) end @@ -144,7 +144,6 @@ namespace :demo do update_response.call( map: instructor_map, - comments_prefix: 'Instructor calibration review', scores: { items[0] => 4, items[1] => 5, @@ -186,7 +185,6 @@ namespace :demo do update_response.call( map: map, - comments_prefix: "Demo reviewer #{index + 1}", scores: { items[0] => scores[0], items[1] => scores[1], From 55c397988ea269caae22ffeac187d1a41d13d449 Mon Sep 17 00:00:00 2001 From: xmi Date: Sun, 26 Apr 2026 18:06:58 -0400 Subject: [PATCH 12/25] Seed blank calibration demo comments --- lib/tasks/calibration_demo.rake | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/tasks/calibration_demo.rake b/lib/tasks/calibration_demo.rake index 286276937..292719d2b 100644 --- a/lib/tasks/calibration_demo.rake +++ b/lib/tasks/calibration_demo.rake @@ -54,7 +54,7 @@ namespace :demo do item end - update_response = lambda do |map:, scores:, comments_prefix:| + update_response = lambda do |map:, scores:| response = Response.where(map_id: map.id, is_submitted: true).order(updated_at: :desc).first unless response @@ -72,7 +72,7 @@ namespace :demo do answer = Answer.find_or_initialize_by(response_id: response.id, item_id: item.id) answer.update!( answer: score, - comments: "#{comments_prefix}: #{item.txt} => #{score}" + comments: '' ) end @@ -144,7 +144,6 @@ namespace :demo do update_response.call( map: instructor_map, - comments_prefix: 'Instructor calibration review', scores: { items[0] => 4, items[1] => 5, @@ -186,7 +185,6 @@ namespace :demo do update_response.call( map: map, - comments_prefix: "Demo reviewer #{index + 1}", scores: { items[0] => scores[0], items[1] => scores[1], From 1a1f62450c4f584dd36c7d6c01a19e2d046f5a22 Mon Sep 17 00:00:00 2001 From: ruju4a Date: Sun, 26 Apr 2026 19:47:59 -0400 Subject: [PATCH 13/25] added mockdata files to gitignore --- .gitignore | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.gitignore b/.gitignore index b5973ad4c..82d93b6b9 100644 --- a/.gitignore +++ b/.gitignore @@ -32,3 +32,9 @@ coverage/ rsa_keys.yml pg_data/ + +# Local-only files not intended for the repository +app/controllers/demo/ +app/services/demo/ +app/controllers/transcript +wiki.txt From 16567b23f36af8ce9406917cd9f678e6d09f217b Mon Sep 17 00:00:00 2001 From: ruju4a Date: Sun, 26 Apr 2026 20:16:16 -0400 Subject: [PATCH 14/25] Refactor calibration: move logic to models, add iterator-based report service --- app/controllers/reports_controller.rb | 44 ++---- app/controllers/review_mappings_controller.rb | 141 ++---------------- app/models/assignment.rb | 47 ++++++ app/models/response_map.rb | 11 ++ app/models/review_response_map.rb | 56 +++++-- app/services/reports/calibration_report.rb | 99 ++++++++++++ app/services/reports/report_template.rb | 55 +++++++ config/routes.rb | 7 - 8 files changed, 281 insertions(+), 179 deletions(-) create mode 100644 app/services/reports/calibration_report.rb create mode 100644 app/services/reports/report_template.rb diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index 31b6cf10e..f3681269c 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -17,45 +17,23 @@ def action_allowed? # GET /assignments/:assignment_id/reports/calibration/:map_id # - # Returns the comparison data for a single instructor calibration review: - # the instructor's submitted response, the rubric items, the student - # calibration responses for the same reviewee, a per-item summary produced - # by CalibrationPerItemSummary, and the reviewee team's submitted content. + # Renders the calibration comparison report for one instructor map. + # All assembly logic (rubric items, student responses, per-item score + # histograms, submitted content) lives in Reports::CalibrationReport, + # which follows the iterator pattern from Reports::Base — responses are + # walked one at a time via find_each, never preloaded into an ad-hoc array. def calibration instructor_map = ReviewResponseMap.find_by!( - id: params[:map_id], + id: params[:map_id], reviewed_object_id: @assignment.id, - for_calibration: true + for_calibration: true ) - - instructor_response = instructor_map.latest_submitted_response - return render_error('Submitted instructor calibration response not found', :unprocessable_entity) unless instructor_response - - rubric_items = instructor_response.rubric_items - return render_error('Review rubric not found', :unprocessable_entity) if rubric_items.empty? - - student_responses = ReviewResponseMap.peer_calibration_responses_for(instructor_map) - - per_item_summary = CalibrationPerItemSummary.build( - items: rubric_items, - instructor_response: instructor_response, - student_responses: student_responses - ) - - reviewee = instructor_map.reviewee - - render json: { - map_id: instructor_map.id, - assignment_id: @assignment.id, - reviewee_id: instructor_map.reviewee_id, - rubric_items: rubric_items.map(&:as_calibration_json), - instructor_response: instructor_response.as_calibration_json, - student_responses: student_responses.map(&:as_calibration_json), - per_item_summary: per_item_summary, - submitted_content: reviewee.respond_to?(:submitted_content) ? reviewee.submitted_content : { hyperlinks: [], files: [] } - }, status: :ok + render json: Reports::CalibrationReport.new(instructor_map).render, status: :ok rescue ActiveRecord::RecordNotFound render_error('Calibration review map not found', :not_found) + rescue Reports::CalibrationReport::InstructorResponseMissing, + Reports::CalibrationReport::RubricMissing => e + render_error(e.message, :unprocessable_entity) end private diff --git a/app/controllers/review_mappings_controller.rb b/app/controllers/review_mappings_controller.rb index 040a0a4be..587e95e83 100644 --- a/app/controllers/review_mappings_controller.rb +++ b/app/controllers/review_mappings_controller.rb @@ -74,8 +74,8 @@ def assign_calibration_artifacts # GET /assignments/:assignment_id/review_mappings/calibration_participants def list_calibration_participants render json: { - assignment_id: @assignment.id, - calibration_participants: calibration_participant_rows + assignment_id: @assignment.id, + calibration_participants: @assignment.calibration_participant_rows }, status: :ok end @@ -88,77 +88,28 @@ def add_calibration_participant user = User.find_by(name: username) || User.find_by(email: username) return render json: { error: "User '#{username}' not found" }, status: :not_found unless user - instructor_participant = find_or_create_instructor_participant - return render json: { error: 'Assignment has no instructor' }, status: :unprocessable_entity unless instructor_participant - - participant = nil - team = nil - map = nil - - ActiveRecord::Base.transaction do - participant = AssignmentParticipant.find_by(parent_id: @assignment.id, user_id: user.id) || - @assignment.add_participant(user.id) - - team = participant.team || Team.create_team_for_participant(participant) - - map = ReviewResponseMap.find_or_create_by!( - reviewer_id: instructor_participant.id, - reviewee_id: team.id, - reviewed_object_id: @assignment.id, - for_calibration: true - ) - end - - render json: serialize_calibration_row(participant, team, map), status: :created - rescue ActiveRecord::RecordInvalid, ArgumentError => e + row = @assignment.add_calibration_submitter!(user) + render json: row, status: :created + rescue ArgumentError, ActiveRecord::RecordInvalid => e render json: { error: e.message }, status: :unprocessable_entity rescue StandardError => e render json: { error: e.message }, status: :unprocessable_entity end - # DEMO_INSTRUCTOR_RESPONSE - # POST /assignments/:assignment_id/review_mappings/:map_id/mock_instructor_response - # - # Demo-only: materializes a submitted instructor calibration Response with - # default answers for the given calibration map so the calibration report - # has data to display while the real review form lives outside this - # project's scope. The "how" of fabricating data lives in - # Demo::CalibrationInstructorSeeder; this controller is responsible only for - # locating the map and translating outcomes into HTTP responses. - # See Demo::CalibrationInstructorSeeder for the full removal checklist. - def submit_mock_instructor_calibration_response - map = ReviewResponseMap.find_by!( - id: params[:map_id], - reviewed_object_id: @assignment.id, - for_calibration: true - ) - - response = Demo::CalibrationInstructorSeeder.seed!(map) - - render json: { - map_id: map.id, - response_id: response.id, - instructor_review_status: 'submitted' - }, status: :ok - rescue ActiveRecord::RecordNotFound - render json: { error: 'Calibration review map not found' }, status: :not_found - rescue ArgumentError, ActiveRecord::RecordInvalid => e - render json: { error: e.message }, status: :unprocessable_entity - end + # NOTE: The demo-only "mock instructor response" endpoint that used to live + # here has moved to Demo::CalibrationInstructorResponsesController so this + # controller stays focused on real review-mapping behaviour. See + # Demo::CalibrationInstructorSeeder for the demo's removal checklist. # DELETE /assignments/:assignment_id/review_mappings/calibration_participants/:participant_id def remove_calibration_participant participant = AssignmentParticipant.find_by(id: params[:participant_id], parent_id: @assignment.id) return render json: { error: 'Calibration participant not found' }, status: :not_found unless participant - team = participant.team + team = AssignmentTeam.team(participant) return render json: { error: 'Participant has no team' }, status: :unprocessable_entity unless team - ReviewResponseMap.where( - reviewed_object_id: @assignment.id, - reviewee_id: team.id, - for_calibration: true - ).destroy_all + ReviewResponseMap.calibration_for(@assignment).where(reviewee_id: team.id).destroy_all render json: { message: "Calibration participant #{participant.id} removed." }, status: :ok end @@ -195,8 +146,7 @@ def grade_review list_calibration_participants add_calibration_participant remove_calibration_participant - submit_mock_instructor_calibration_response - ].freeze # DEMO_INSTRUCTOR_RESPONSE: drop the last entry when the demo seeder is removed. + ].freeze def action_allowed? return current_user_teaching_staff_of_assignment?(params[:assignment_id]) if CALIBRATION_PARTICIPANT_ACTIONS.include?(params[:action]) @@ -209,71 +159,4 @@ def action_allowed? def set_assignment @assignment = Assignment.find(params[:assignment_id]) end - - # ----- Helpers for the calibration participants endpoints ----- - - # The instructor is the reviewer on every calibration ReviewResponseMap it - # creates, so the instructor must also be registered as an - # AssignmentParticipant on this assignment. Create that record lazily. - def find_or_create_instructor_participant - instructor = @assignment.instructor - return nil unless instructor - - AssignmentParticipant.find_by(parent_id: @assignment.id, user_id: instructor.id) || - @assignment.add_participant(instructor.id) - end - - # Build one row per calibration submitter. A submitter is identified as the - # (sole) member of a team that is the reviewee of any for_calibration map on - # this assignment. Prefer the instructor's map as the "Begin" target. - def calibration_participant_rows - maps = ReviewResponseMap.where( - reviewed_object_id: @assignment.id, - for_calibration: true - ).order(:id) - - instructor_user_id = @assignment.instructor_id - maps_by_team = maps.group_by(&:reviewee_id) - - maps_by_team.map do |team_id, team_maps| - team = AssignmentTeam.find_by(id: team_id) - next nil unless team - - instructor_map = team_maps.find { |m| m.reviewer&.user_id == instructor_user_id } || team_maps.first - submitter = team.participants.where(type: 'AssignmentParticipant').first - next nil unless submitter - - serialize_calibration_row(submitter, team, instructor_map) - end.compact - end - - def serialize_calibration_row(participant, team, instructor_map) - { - participant_id: participant.id, - user_id: participant.user_id, - username: participant.user&.name, - full_name: participant.user&.full_name, - handle: participant.handle, - team_id: team&.id, - team_name: team&.name, - instructor_review_map_id: instructor_map&.id, - instructor_review_status: instructor_review_status_for(instructor_map), - submissions: team.respond_to?(:submitted_content_detail) ? team.submitted_content_detail : { hyperlinks: [], files: [] } - } - end - - # Classify the instructor's progress on a calibration review map so the UI can - # show "Begin" when no response exists yet and "View | Edit" once one does. - # - :not_started -> no Response rows for this map - # - :in_progress -> at least one Response, none submitted yet - # - :submitted -> at least one submitted Response - def instructor_review_status_for(instructor_map) - return :not_started unless instructor_map - - responses = instructor_map.responses - return :not_started if responses.empty? - return :submitted if responses.where(is_submitted: true).exists? - - :in_progress - end end diff --git a/app/models/assignment.rb b/app/models/assignment.rb index 130fa6837..9487b3815 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -76,6 +76,53 @@ def add_participant(user_id) end + # ===== CALIBRATION PARTICIPANT MANAGEMENT ===== + + # Idempotent version of add_participant: returns the existing + # AssignmentParticipant if one already exists, otherwise creates one. + # Used by calibration flows where a user may already be a participant + # (e.g. the instructor, or a re-added submitter). + def find_or_add_participant!(user) + AssignmentParticipant.find_by(parent_id: id, user_id: user.id) || add_participant(user.id) + end + + # Orchestrates adding a calibration submitter in one atomic transaction: + # 1. Ensure the user is an AssignmentParticipant (idempotent). + # 2. Ensure the user has a team (creates an AssignmentTeam if not). + # 3. Ensure the instructor is an AssignmentParticipant (idempotent). + # 4. Find or create the for_calibration ReviewResponseMap (instructor → team). + # + # Returns the JSON row hash for the new calibration participant, ready to + # render directly from the controller. + def add_calibration_submitter!(user) + ActiveRecord::Base.transaction do + submitter = find_or_add_participant!(user) + team = AssignmentTeam.team(submitter) || Team.create_team_for_participant(submitter) + instructor_p = find_or_add_participant!(instructor) + map = ReviewResponseMap.find_or_create_by!( + reviewer_id: instructor_p.id, + reviewee_id: team.id, + reviewed_object_id: id, + for_calibration: true + ) + map.calibration_participant_json(instructor_user_id: instructor_id) + end + end + + # Returns the list of calibration participant rows for the assignment editor's + # Calibration tab. Delegates query + serialization to ReviewResponseMap so + # the controller stays a thin delegate. + def calibration_participant_rows + ReviewResponseMap + .calibration_for(self) + .order(:id) + .group_by(&:reviewee_id) + .filter_map do |_team_id, team_maps| + instructor_map = team_maps.find { |m| m.reviewer&.user_id == instructor_id } || team_maps.first + instructor_map.calibration_participant_json(instructor_user_id: instructor_id) + end + end + # Remove a participant from the assignment based on the provided user_id. # This method finds the AssignmentParticipant with the given assignment_id and user_id, # and then deletes the corresponding record from the database. diff --git a/app/models/response_map.rb b/app/models/response_map.rb index da6ffe002..e2dbaf0d0 100644 --- a/app/models/response_map.rb +++ b/app/models/response_map.rb @@ -27,6 +27,17 @@ def latest_submitted_response responses.where(is_submitted: true).order(updated_at: :desc).first end + # Classify the progress of this review map: + # :not_started -- no Response rows exist yet + # :in_progress -- at least one Response, none submitted + # :submitted -- at least one submitted Response + # Lives here (Information Expert) because the map owns its responses. + def review_status + return :not_started if responses.empty? + return :submitted if responses.where(is_submitted: true).exists? + :in_progress + end + def self.assessments_for(team) responses = [] # stime = Time.now diff --git a/app/models/review_response_map.rb b/app/models/review_response_map.rb index f333be021..023c42f00 100644 --- a/app/models/review_response_map.rb +++ b/app/models/review_response_map.rb @@ -3,6 +3,12 @@ class ReviewResponseMap < ResponseMap include ResponseMapSubclassTitles belongs_to :reviewee, class_name: 'Team', foreign_key: 'reviewee_id', inverse_of: false + # All for_calibration maps on a given assignment. + # Centralises the for_calibration condition so callers don't repeat it. + scope :calibration_for, ->(assignment) { + where(reviewed_object_id: assignment.id, for_calibration: true) + } + # returns the assignment related to the response map def response_assignment return assignment @@ -11,7 +17,7 @@ def response_assignment def questionnaire_type 'Review' end - + def get_title REVIEW_RESPONSE_MAP_TITLE end @@ -21,18 +27,48 @@ def review_map_type 'ReviewResponseMap' end - # Collect the student-submitted calibration responses that share a reviewee - # with the given instructor calibration map. These are the responses that - # should be compared against the instructor's review in a calibration - # report. When a student has revised their review, only the latest - # submitted response for each map is included. - def self.peer_calibration_responses_for(instructor_map) + # Iterator over the latest submitted peer calibration Response for each + # student calibration map that shares the same reviewee as `instructor_map`. + # + # Uses find_each (batch loading) so large calibration cohorts don't load + # all maps into memory at once. Yields one Response at a time. + # + # Without a block, returns an Enumerator so callers can chain lazily. + def self.peer_calibration_responses_each(instructor_map) + return enum_for(:peer_calibration_responses_each, instructor_map) unless block_given? + peer_maps = where( reviewed_object_id: instructor_map.reviewed_object_id, - reviewee_id: instructor_map.reviewee_id, - for_calibration: true + reviewee_id: instructor_map.reviewee_id, + for_calibration: true ).where.not(id: instructor_map.id) - peer_maps.filter_map(&:latest_submitted_response) + peer_maps.find_each do |map| + response = map.latest_submitted_response + yield response if response + end + end + + # Serializes this calibration map into the JSON row expected by the + # assignment editor's Calibration tab. + # Lives here (Information Expert) — the map knows its reviewee team, + # its reviewer, and can ask itself for review_status. + def calibration_participant_json(instructor_user_id:) + team = reviewee + submitter = team.participants.where(type: 'AssignmentParticipant').first + return nil unless submitter + + { + participant_id: submitter.id, + user_id: submitter.user_id, + username: submitter.user&.name, + full_name: submitter.user&.full_name, + handle: submitter.handle, + team_id: team.id, + team_name: team.name, + instructor_review_map_id: id, + instructor_review_status: review_status, + submissions: team.submitted_content_detail + } end end diff --git a/app/services/reports/calibration_report.rb b/app/services/reports/calibration_report.rb new file mode 100644 index 000000000..2c57c07b0 --- /dev/null +++ b/app/services/reports/calibration_report.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +module Reports + # Reports::CalibrationReport renders the comparison view for one instructor + # calibration map: the instructor's submitted response, the rubric items, + # the (latest submitted) student peer responses for the same reviewee, and + # a per-item bucket-count summary suitable for a stacked chart. + # + # Design notes (mirroring Reports::Base): + # * Responses are walked one-at-a-time via an iterator on + # ReviewResponseMap. We never build a giant Array of all peer responses + # in memory; we ask the model to yield the latest submitted Response + # for each peer calibration map. + # * The metric this report produces (per-item score histograms) lives + # here, NOT in the base, because other reports (averages, tag counts, + # survey aggregates) want completely different shapes. + class CalibrationReport < ReportTemplate + InstructorResponseMissing = Class.new(StandardError) + RubricMissing = Class.new(StandardError) + + def initialize(instructor_map) + @instructor_map = instructor_map + end + + private + + def setup + @instructor_response = @instructor_map.latest_submitted_response + raise InstructorResponseMissing, 'Submitted instructor calibration response not found' if @instructor_response.nil? + + @rubric_items = @instructor_response.rubric_items + raise RubricMissing, 'Review rubric not found' if @rubric_items.empty? + + @bucket_counts = empty_buckets_for(@rubric_items) + @student_responses = [] + end + + # Iterator: ask ReviewResponseMap for the latest submitted student + # calibration Response per peer map and yield them one at a time. + def each_response(&block) + ReviewResponseMap.peer_calibration_responses_each(@instructor_map, &block) + end + + # State updated per response. The metric here (score histograms) is + # CalibrationReport-specific; that is exactly why it does not belong in + # the base. + def accumulate(response) + @student_responses << response + response.scores.each do |answer| + next if answer.answer.nil? + next unless @bucket_counts.key?(answer.item_id) + + bucket_key = answer.answer.to_i.to_s + @bucket_counts[answer.item_id][bucket_key] ||= 0 + @bucket_counts[answer.item_id][bucket_key] += 1 + end + end + + def payload + { + map_id: @instructor_map.id, + assignment_id: @instructor_map.reviewed_object_id, + reviewee_id: @instructor_map.reviewee_id, + rubric_items: @rubric_items.map(&:as_calibration_json), + instructor_response: @instructor_response.as_calibration_json, + student_responses: @student_responses.map(&:as_calibration_json), + per_item_summary: @rubric_items.sort_by(&:seq).map { |item| per_item_summary(item) }, + submitted_content: submitted_content_for(@instructor_map.reviewee) + } + end + + def empty_buckets_for(items) + items.each_with_object({}) do |item, hash| + min = item.questionnaire&.min_question_score.to_i + max = item.questionnaire&.max_question_score.to_i + max = min + 5 if max <= min + hash[item.id] = (min..max).each_with_object({}) { |score, b| b[score.to_s] = 0 } + end + end + + def per_item_summary(item) + instructor_answer = @instructor_response.scores.find { |a| a.item_id == item.id } + { + item_id: item.id, + item_label: item.txt, + item_seq: item.seq, + instructor_score: instructor_answer&.answer, + instructor_comment: instructor_answer&.comments, + bucket_counts: @bucket_counts[item.id], + student_response_count: @student_responses.size + } + end + + def submitted_content_for(reviewee) + return { hyperlinks: [], files: [] } unless reviewee.respond_to?(:submitted_content) + reviewee.submitted_content + end + end +end diff --git a/app/services/reports/report_template.rb b/app/services/reports/report_template.rb new file mode 100644 index 000000000..c5603ec23 --- /dev/null +++ b/app/services/reports/report_template.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module Reports + # Reports::ReportTemplate is the Template Method skeleton shared by every + # report in the system (review, author-feedback, teammate, bookmark, + # calibration, survey, quiz). + # + # It defines the fixed algorithm: setup → iterate one response at a time → + # assemble the payload. Subclasses fill in each step without altering the + # overall sequence. + # + # Two design rules drive what does -- and does not -- live here: + # + # 1. Iterate over responses, never preload them into an ad-hoc array. + # Subclasses implement `each_response` as an *iterator* that yields one + # Response at a time. The template loops once and dispatches; nothing in + # the template ever needs the whole collection in memory. + # + # 2. No specific metrics in the template. + # Different reports want different summaries (averages for one, score + # histograms for another, per-tag counts for answer-tagging). The + # template therefore exposes only the skeleton -- setup, accumulate, + # payload -- and lets each subclass define its own state and metric logic. + # + # Subclass contract: + # * setup -- prepare any state needed before iteration + # (rubric items, output buffers, etc.). Optional. + # * each_response -- yield Responses one at a time. REQUIRED. + # * accumulate(r) -- update subclass-owned state from a single response. + # REQUIRED. + # * payload -- return the final JSON-ready Hash. REQUIRED. + class ReportTemplate + def render + setup + each_response { |response| accumulate(response) } + payload + end + + private + + def setup; end + + def each_response + raise NotImplementedError, "#{self.class} must implement #each_response as an iterator" + end + + def accumulate(_response) + raise NotImplementedError, "#{self.class} must implement #accumulate(response)" + end + + def payload + raise NotImplementedError, "#{self.class} must implement #payload" + end + end +end diff --git a/config/routes.rb b/config/routes.rb index 9fbed4a56..8a95c9ccb 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -44,13 +44,6 @@ get :calibration_participants, action: :list_calibration_participants post :calibration_participants, action: :add_calibration_participant delete 'calibration_participants/:participant_id', action: :remove_calibration_participant - # DEMO_INSTRUCTOR_RESPONSE - # Demo-only: seeds an instructor calibration Response for the given - # map. Wired to the "Begin" link on the Calibration tab while the - # real review form lives outside this project's scope. - # See Demo::CalibrationInstructorSeeder for the full removal checklist. - post ':map_id/mock_instructor_response', - action: :submit_mock_instructor_calibration_response end end From 18e2ffa82beb4cf71dd4fd5c0a5bb7dcd7d93b33 Mon Sep 17 00:00:00 2001 From: ruju4a Date: Sun, 26 Apr 2026 20:16:16 -0400 Subject: [PATCH 15/25] Refactor calibration: move logic to models, add iterator-based report service --- app/controllers/reports_controller.rb | 44 ++---- app/controllers/review_mappings_controller.rb | 141 ++---------------- app/models/assignment.rb | 47 ++++++ app/models/response_map.rb | 11 ++ app/models/review_response_map.rb | 56 +++++-- app/services/reports/calibration_report.rb | 99 ++++++++++++ app/services/reports/report_template.rb | 55 +++++++ config/routes.rb | 7 - 8 files changed, 281 insertions(+), 179 deletions(-) create mode 100644 app/services/reports/calibration_report.rb create mode 100644 app/services/reports/report_template.rb diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index 31b6cf10e..f3681269c 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -17,45 +17,23 @@ def action_allowed? # GET /assignments/:assignment_id/reports/calibration/:map_id # - # Returns the comparison data for a single instructor calibration review: - # the instructor's submitted response, the rubric items, the student - # calibration responses for the same reviewee, a per-item summary produced - # by CalibrationPerItemSummary, and the reviewee team's submitted content. + # Renders the calibration comparison report for one instructor map. + # All assembly logic (rubric items, student responses, per-item score + # histograms, submitted content) lives in Reports::CalibrationReport, + # which follows the iterator pattern from Reports::Base — responses are + # walked one at a time via find_each, never preloaded into an ad-hoc array. def calibration instructor_map = ReviewResponseMap.find_by!( - id: params[:map_id], + id: params[:map_id], reviewed_object_id: @assignment.id, - for_calibration: true + for_calibration: true ) - - instructor_response = instructor_map.latest_submitted_response - return render_error('Submitted instructor calibration response not found', :unprocessable_entity) unless instructor_response - - rubric_items = instructor_response.rubric_items - return render_error('Review rubric not found', :unprocessable_entity) if rubric_items.empty? - - student_responses = ReviewResponseMap.peer_calibration_responses_for(instructor_map) - - per_item_summary = CalibrationPerItemSummary.build( - items: rubric_items, - instructor_response: instructor_response, - student_responses: student_responses - ) - - reviewee = instructor_map.reviewee - - render json: { - map_id: instructor_map.id, - assignment_id: @assignment.id, - reviewee_id: instructor_map.reviewee_id, - rubric_items: rubric_items.map(&:as_calibration_json), - instructor_response: instructor_response.as_calibration_json, - student_responses: student_responses.map(&:as_calibration_json), - per_item_summary: per_item_summary, - submitted_content: reviewee.respond_to?(:submitted_content) ? reviewee.submitted_content : { hyperlinks: [], files: [] } - }, status: :ok + render json: Reports::CalibrationReport.new(instructor_map).render, status: :ok rescue ActiveRecord::RecordNotFound render_error('Calibration review map not found', :not_found) + rescue Reports::CalibrationReport::InstructorResponseMissing, + Reports::CalibrationReport::RubricMissing => e + render_error(e.message, :unprocessable_entity) end private diff --git a/app/controllers/review_mappings_controller.rb b/app/controllers/review_mappings_controller.rb index 040a0a4be..587e95e83 100644 --- a/app/controllers/review_mappings_controller.rb +++ b/app/controllers/review_mappings_controller.rb @@ -74,8 +74,8 @@ def assign_calibration_artifacts # GET /assignments/:assignment_id/review_mappings/calibration_participants def list_calibration_participants render json: { - assignment_id: @assignment.id, - calibration_participants: calibration_participant_rows + assignment_id: @assignment.id, + calibration_participants: @assignment.calibration_participant_rows }, status: :ok end @@ -88,77 +88,28 @@ def add_calibration_participant user = User.find_by(name: username) || User.find_by(email: username) return render json: { error: "User '#{username}' not found" }, status: :not_found unless user - instructor_participant = find_or_create_instructor_participant - return render json: { error: 'Assignment has no instructor' }, status: :unprocessable_entity unless instructor_participant - - participant = nil - team = nil - map = nil - - ActiveRecord::Base.transaction do - participant = AssignmentParticipant.find_by(parent_id: @assignment.id, user_id: user.id) || - @assignment.add_participant(user.id) - - team = participant.team || Team.create_team_for_participant(participant) - - map = ReviewResponseMap.find_or_create_by!( - reviewer_id: instructor_participant.id, - reviewee_id: team.id, - reviewed_object_id: @assignment.id, - for_calibration: true - ) - end - - render json: serialize_calibration_row(participant, team, map), status: :created - rescue ActiveRecord::RecordInvalid, ArgumentError => e + row = @assignment.add_calibration_submitter!(user) + render json: row, status: :created + rescue ArgumentError, ActiveRecord::RecordInvalid => e render json: { error: e.message }, status: :unprocessable_entity rescue StandardError => e render json: { error: e.message }, status: :unprocessable_entity end - # DEMO_INSTRUCTOR_RESPONSE - # POST /assignments/:assignment_id/review_mappings/:map_id/mock_instructor_response - # - # Demo-only: materializes a submitted instructor calibration Response with - # default answers for the given calibration map so the calibration report - # has data to display while the real review form lives outside this - # project's scope. The "how" of fabricating data lives in - # Demo::CalibrationInstructorSeeder; this controller is responsible only for - # locating the map and translating outcomes into HTTP responses. - # See Demo::CalibrationInstructorSeeder for the full removal checklist. - def submit_mock_instructor_calibration_response - map = ReviewResponseMap.find_by!( - id: params[:map_id], - reviewed_object_id: @assignment.id, - for_calibration: true - ) - - response = Demo::CalibrationInstructorSeeder.seed!(map) - - render json: { - map_id: map.id, - response_id: response.id, - instructor_review_status: 'submitted' - }, status: :ok - rescue ActiveRecord::RecordNotFound - render json: { error: 'Calibration review map not found' }, status: :not_found - rescue ArgumentError, ActiveRecord::RecordInvalid => e - render json: { error: e.message }, status: :unprocessable_entity - end + # NOTE: The demo-only "mock instructor response" endpoint that used to live + # here has moved to Demo::CalibrationInstructorResponsesController so this + # controller stays focused on real review-mapping behaviour. See + # Demo::CalibrationInstructorSeeder for the demo's removal checklist. # DELETE /assignments/:assignment_id/review_mappings/calibration_participants/:participant_id def remove_calibration_participant participant = AssignmentParticipant.find_by(id: params[:participant_id], parent_id: @assignment.id) return render json: { error: 'Calibration participant not found' }, status: :not_found unless participant - team = participant.team + team = AssignmentTeam.team(participant) return render json: { error: 'Participant has no team' }, status: :unprocessable_entity unless team - ReviewResponseMap.where( - reviewed_object_id: @assignment.id, - reviewee_id: team.id, - for_calibration: true - ).destroy_all + ReviewResponseMap.calibration_for(@assignment).where(reviewee_id: team.id).destroy_all render json: { message: "Calibration participant #{participant.id} removed." }, status: :ok end @@ -195,8 +146,7 @@ def grade_review list_calibration_participants add_calibration_participant remove_calibration_participant - submit_mock_instructor_calibration_response - ].freeze # DEMO_INSTRUCTOR_RESPONSE: drop the last entry when the demo seeder is removed. + ].freeze def action_allowed? return current_user_teaching_staff_of_assignment?(params[:assignment_id]) if CALIBRATION_PARTICIPANT_ACTIONS.include?(params[:action]) @@ -209,71 +159,4 @@ def action_allowed? def set_assignment @assignment = Assignment.find(params[:assignment_id]) end - - # ----- Helpers for the calibration participants endpoints ----- - - # The instructor is the reviewer on every calibration ReviewResponseMap it - # creates, so the instructor must also be registered as an - # AssignmentParticipant on this assignment. Create that record lazily. - def find_or_create_instructor_participant - instructor = @assignment.instructor - return nil unless instructor - - AssignmentParticipant.find_by(parent_id: @assignment.id, user_id: instructor.id) || - @assignment.add_participant(instructor.id) - end - - # Build one row per calibration submitter. A submitter is identified as the - # (sole) member of a team that is the reviewee of any for_calibration map on - # this assignment. Prefer the instructor's map as the "Begin" target. - def calibration_participant_rows - maps = ReviewResponseMap.where( - reviewed_object_id: @assignment.id, - for_calibration: true - ).order(:id) - - instructor_user_id = @assignment.instructor_id - maps_by_team = maps.group_by(&:reviewee_id) - - maps_by_team.map do |team_id, team_maps| - team = AssignmentTeam.find_by(id: team_id) - next nil unless team - - instructor_map = team_maps.find { |m| m.reviewer&.user_id == instructor_user_id } || team_maps.first - submitter = team.participants.where(type: 'AssignmentParticipant').first - next nil unless submitter - - serialize_calibration_row(submitter, team, instructor_map) - end.compact - end - - def serialize_calibration_row(participant, team, instructor_map) - { - participant_id: participant.id, - user_id: participant.user_id, - username: participant.user&.name, - full_name: participant.user&.full_name, - handle: participant.handle, - team_id: team&.id, - team_name: team&.name, - instructor_review_map_id: instructor_map&.id, - instructor_review_status: instructor_review_status_for(instructor_map), - submissions: team.respond_to?(:submitted_content_detail) ? team.submitted_content_detail : { hyperlinks: [], files: [] } - } - end - - # Classify the instructor's progress on a calibration review map so the UI can - # show "Begin" when no response exists yet and "View | Edit" once one does. - # - :not_started -> no Response rows for this map - # - :in_progress -> at least one Response, none submitted yet - # - :submitted -> at least one submitted Response - def instructor_review_status_for(instructor_map) - return :not_started unless instructor_map - - responses = instructor_map.responses - return :not_started if responses.empty? - return :submitted if responses.where(is_submitted: true).exists? - - :in_progress - end end diff --git a/app/models/assignment.rb b/app/models/assignment.rb index 130fa6837..9487b3815 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -76,6 +76,53 @@ def add_participant(user_id) end + # ===== CALIBRATION PARTICIPANT MANAGEMENT ===== + + # Idempotent version of add_participant: returns the existing + # AssignmentParticipant if one already exists, otherwise creates one. + # Used by calibration flows where a user may already be a participant + # (e.g. the instructor, or a re-added submitter). + def find_or_add_participant!(user) + AssignmentParticipant.find_by(parent_id: id, user_id: user.id) || add_participant(user.id) + end + + # Orchestrates adding a calibration submitter in one atomic transaction: + # 1. Ensure the user is an AssignmentParticipant (idempotent). + # 2. Ensure the user has a team (creates an AssignmentTeam if not). + # 3. Ensure the instructor is an AssignmentParticipant (idempotent). + # 4. Find or create the for_calibration ReviewResponseMap (instructor → team). + # + # Returns the JSON row hash for the new calibration participant, ready to + # render directly from the controller. + def add_calibration_submitter!(user) + ActiveRecord::Base.transaction do + submitter = find_or_add_participant!(user) + team = AssignmentTeam.team(submitter) || Team.create_team_for_participant(submitter) + instructor_p = find_or_add_participant!(instructor) + map = ReviewResponseMap.find_or_create_by!( + reviewer_id: instructor_p.id, + reviewee_id: team.id, + reviewed_object_id: id, + for_calibration: true + ) + map.calibration_participant_json(instructor_user_id: instructor_id) + end + end + + # Returns the list of calibration participant rows for the assignment editor's + # Calibration tab. Delegates query + serialization to ReviewResponseMap so + # the controller stays a thin delegate. + def calibration_participant_rows + ReviewResponseMap + .calibration_for(self) + .order(:id) + .group_by(&:reviewee_id) + .filter_map do |_team_id, team_maps| + instructor_map = team_maps.find { |m| m.reviewer&.user_id == instructor_id } || team_maps.first + instructor_map.calibration_participant_json(instructor_user_id: instructor_id) + end + end + # Remove a participant from the assignment based on the provided user_id. # This method finds the AssignmentParticipant with the given assignment_id and user_id, # and then deletes the corresponding record from the database. diff --git a/app/models/response_map.rb b/app/models/response_map.rb index da6ffe002..e2dbaf0d0 100644 --- a/app/models/response_map.rb +++ b/app/models/response_map.rb @@ -27,6 +27,17 @@ def latest_submitted_response responses.where(is_submitted: true).order(updated_at: :desc).first end + # Classify the progress of this review map: + # :not_started -- no Response rows exist yet + # :in_progress -- at least one Response, none submitted + # :submitted -- at least one submitted Response + # Lives here (Information Expert) because the map owns its responses. + def review_status + return :not_started if responses.empty? + return :submitted if responses.where(is_submitted: true).exists? + :in_progress + end + def self.assessments_for(team) responses = [] # stime = Time.now diff --git a/app/models/review_response_map.rb b/app/models/review_response_map.rb index f333be021..023c42f00 100644 --- a/app/models/review_response_map.rb +++ b/app/models/review_response_map.rb @@ -3,6 +3,12 @@ class ReviewResponseMap < ResponseMap include ResponseMapSubclassTitles belongs_to :reviewee, class_name: 'Team', foreign_key: 'reviewee_id', inverse_of: false + # All for_calibration maps on a given assignment. + # Centralises the for_calibration condition so callers don't repeat it. + scope :calibration_for, ->(assignment) { + where(reviewed_object_id: assignment.id, for_calibration: true) + } + # returns the assignment related to the response map def response_assignment return assignment @@ -11,7 +17,7 @@ def response_assignment def questionnaire_type 'Review' end - + def get_title REVIEW_RESPONSE_MAP_TITLE end @@ -21,18 +27,48 @@ def review_map_type 'ReviewResponseMap' end - # Collect the student-submitted calibration responses that share a reviewee - # with the given instructor calibration map. These are the responses that - # should be compared against the instructor's review in a calibration - # report. When a student has revised their review, only the latest - # submitted response for each map is included. - def self.peer_calibration_responses_for(instructor_map) + # Iterator over the latest submitted peer calibration Response for each + # student calibration map that shares the same reviewee as `instructor_map`. + # + # Uses find_each (batch loading) so large calibration cohorts don't load + # all maps into memory at once. Yields one Response at a time. + # + # Without a block, returns an Enumerator so callers can chain lazily. + def self.peer_calibration_responses_each(instructor_map) + return enum_for(:peer_calibration_responses_each, instructor_map) unless block_given? + peer_maps = where( reviewed_object_id: instructor_map.reviewed_object_id, - reviewee_id: instructor_map.reviewee_id, - for_calibration: true + reviewee_id: instructor_map.reviewee_id, + for_calibration: true ).where.not(id: instructor_map.id) - peer_maps.filter_map(&:latest_submitted_response) + peer_maps.find_each do |map| + response = map.latest_submitted_response + yield response if response + end + end + + # Serializes this calibration map into the JSON row expected by the + # assignment editor's Calibration tab. + # Lives here (Information Expert) — the map knows its reviewee team, + # its reviewer, and can ask itself for review_status. + def calibration_participant_json(instructor_user_id:) + team = reviewee + submitter = team.participants.where(type: 'AssignmentParticipant').first + return nil unless submitter + + { + participant_id: submitter.id, + user_id: submitter.user_id, + username: submitter.user&.name, + full_name: submitter.user&.full_name, + handle: submitter.handle, + team_id: team.id, + team_name: team.name, + instructor_review_map_id: id, + instructor_review_status: review_status, + submissions: team.submitted_content_detail + } end end diff --git a/app/services/reports/calibration_report.rb b/app/services/reports/calibration_report.rb new file mode 100644 index 000000000..2c57c07b0 --- /dev/null +++ b/app/services/reports/calibration_report.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +module Reports + # Reports::CalibrationReport renders the comparison view for one instructor + # calibration map: the instructor's submitted response, the rubric items, + # the (latest submitted) student peer responses for the same reviewee, and + # a per-item bucket-count summary suitable for a stacked chart. + # + # Design notes (mirroring Reports::Base): + # * Responses are walked one-at-a-time via an iterator on + # ReviewResponseMap. We never build a giant Array of all peer responses + # in memory; we ask the model to yield the latest submitted Response + # for each peer calibration map. + # * The metric this report produces (per-item score histograms) lives + # here, NOT in the base, because other reports (averages, tag counts, + # survey aggregates) want completely different shapes. + class CalibrationReport < ReportTemplate + InstructorResponseMissing = Class.new(StandardError) + RubricMissing = Class.new(StandardError) + + def initialize(instructor_map) + @instructor_map = instructor_map + end + + private + + def setup + @instructor_response = @instructor_map.latest_submitted_response + raise InstructorResponseMissing, 'Submitted instructor calibration response not found' if @instructor_response.nil? + + @rubric_items = @instructor_response.rubric_items + raise RubricMissing, 'Review rubric not found' if @rubric_items.empty? + + @bucket_counts = empty_buckets_for(@rubric_items) + @student_responses = [] + end + + # Iterator: ask ReviewResponseMap for the latest submitted student + # calibration Response per peer map and yield them one at a time. + def each_response(&block) + ReviewResponseMap.peer_calibration_responses_each(@instructor_map, &block) + end + + # State updated per response. The metric here (score histograms) is + # CalibrationReport-specific; that is exactly why it does not belong in + # the base. + def accumulate(response) + @student_responses << response + response.scores.each do |answer| + next if answer.answer.nil? + next unless @bucket_counts.key?(answer.item_id) + + bucket_key = answer.answer.to_i.to_s + @bucket_counts[answer.item_id][bucket_key] ||= 0 + @bucket_counts[answer.item_id][bucket_key] += 1 + end + end + + def payload + { + map_id: @instructor_map.id, + assignment_id: @instructor_map.reviewed_object_id, + reviewee_id: @instructor_map.reviewee_id, + rubric_items: @rubric_items.map(&:as_calibration_json), + instructor_response: @instructor_response.as_calibration_json, + student_responses: @student_responses.map(&:as_calibration_json), + per_item_summary: @rubric_items.sort_by(&:seq).map { |item| per_item_summary(item) }, + submitted_content: submitted_content_for(@instructor_map.reviewee) + } + end + + def empty_buckets_for(items) + items.each_with_object({}) do |item, hash| + min = item.questionnaire&.min_question_score.to_i + max = item.questionnaire&.max_question_score.to_i + max = min + 5 if max <= min + hash[item.id] = (min..max).each_with_object({}) { |score, b| b[score.to_s] = 0 } + end + end + + def per_item_summary(item) + instructor_answer = @instructor_response.scores.find { |a| a.item_id == item.id } + { + item_id: item.id, + item_label: item.txt, + item_seq: item.seq, + instructor_score: instructor_answer&.answer, + instructor_comment: instructor_answer&.comments, + bucket_counts: @bucket_counts[item.id], + student_response_count: @student_responses.size + } + end + + def submitted_content_for(reviewee) + return { hyperlinks: [], files: [] } unless reviewee.respond_to?(:submitted_content) + reviewee.submitted_content + end + end +end diff --git a/app/services/reports/report_template.rb b/app/services/reports/report_template.rb new file mode 100644 index 000000000..c5603ec23 --- /dev/null +++ b/app/services/reports/report_template.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module Reports + # Reports::ReportTemplate is the Template Method skeleton shared by every + # report in the system (review, author-feedback, teammate, bookmark, + # calibration, survey, quiz). + # + # It defines the fixed algorithm: setup → iterate one response at a time → + # assemble the payload. Subclasses fill in each step without altering the + # overall sequence. + # + # Two design rules drive what does -- and does not -- live here: + # + # 1. Iterate over responses, never preload them into an ad-hoc array. + # Subclasses implement `each_response` as an *iterator* that yields one + # Response at a time. The template loops once and dispatches; nothing in + # the template ever needs the whole collection in memory. + # + # 2. No specific metrics in the template. + # Different reports want different summaries (averages for one, score + # histograms for another, per-tag counts for answer-tagging). The + # template therefore exposes only the skeleton -- setup, accumulate, + # payload -- and lets each subclass define its own state and metric logic. + # + # Subclass contract: + # * setup -- prepare any state needed before iteration + # (rubric items, output buffers, etc.). Optional. + # * each_response -- yield Responses one at a time. REQUIRED. + # * accumulate(r) -- update subclass-owned state from a single response. + # REQUIRED. + # * payload -- return the final JSON-ready Hash. REQUIRED. + class ReportTemplate + def render + setup + each_response { |response| accumulate(response) } + payload + end + + private + + def setup; end + + def each_response + raise NotImplementedError, "#{self.class} must implement #each_response as an iterator" + end + + def accumulate(_response) + raise NotImplementedError, "#{self.class} must implement #accumulate(response)" + end + + def payload + raise NotImplementedError, "#{self.class} must implement #payload" + end + end +end diff --git a/config/routes.rb b/config/routes.rb index 9fbed4a56..8a95c9ccb 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -44,13 +44,6 @@ get :calibration_participants, action: :list_calibration_participants post :calibration_participants, action: :add_calibration_participant delete 'calibration_participants/:participant_id', action: :remove_calibration_participant - # DEMO_INSTRUCTOR_RESPONSE - # Demo-only: seeds an instructor calibration Response for the given - # map. Wired to the "Begin" link on the Calibration tab while the - # real review form lives outside this project's scope. - # See Demo::CalibrationInstructorSeeder for the full removal checklist. - post ':map_id/mock_instructor_response', - action: :submit_mock_instructor_calibration_response end end From 3e3210bd3b9bf96ab232b1b068ce638777e36286 Mon Sep 17 00:00:00 2001 From: ruju4a Date: Mon, 27 Apr 2026 01:15:19 -0400 Subject: [PATCH 16/25] Apply feedback: expert principle fixes and comments --- app/models/questionnaire.rb | 10 + app/models/response.rb | 7 + app/models/review_mapping_handler.rb | 10 + app/services/calibration_per_item_summary.rb | 22 ++ app/services/reports/calibration_report.rb | 16 +- spec/models/response_map_spec.rb | 23 ++ .../calibration_per_item_summary_spec.rb | 266 +++++++----------- 7 files changed, 189 insertions(+), 165 deletions(-) diff --git a/app/models/questionnaire.rb b/app/models/questionnaire.rb index b7eeee017..e78de026c 100644 --- a/app/models/questionnaire.rb +++ b/app/models/questionnaire.rb @@ -85,6 +85,16 @@ def as_json(options = {}) DEFAULT_MIN_QUESTION_SCORE = 0 # The lowest score that a reviewer can assign to any questionnaire question DEFAULT_MAX_QUESTION_SCORE = 5 # The highest score that a reviewer can assign to any questionnaire question + + # Returns the integer Range of valid scores for this questionnaire's items, + # falling back to the class-level defaults when the DB columns are unset. + # Consumers (e.g. report histogram builders) should call this rather than + # hard-coding the 0..5 range themselves. + def score_range + min = (min_question_score || DEFAULT_MIN_QUESTION_SCORE).to_i + max = (max_question_score || DEFAULT_MAX_QUESTION_SCORE).to_i + min..max + end DEFAULT_QUESTIONNAIRE_URL = 'http://www.courses.ncsu.edu/csc517'.freeze QUESTIONNAIRE_TYPES = ['ReviewQuestionnaire', 'MetareviewQuestionnaire', diff --git a/app/models/response.rb b/app/models/response.rb index 4fa07bd06..aaa4b0926 100644 --- a/app/models/response.rb +++ b/app/models/response.rb @@ -62,6 +62,13 @@ def aggregate_questionnaire_score sum end + # Returns the Answer this response recorded for a specific rubric item, or + # nil if no answer was given. Centralises item-lookup here (Information Expert) + # so callers don't scatter `scores.find { |a| a.item_id == item.id }` everywhere. + def answer_for(item) + scores.find { |a| a.item_id == item.id } + end + # Ordered rubric items used for this response, or [] if the rubric cannot be # resolved (e.g. no AssignmentQuestionnaire for this round). Belongs here # rather than in a controller because the response knows which questionnaire diff --git a/app/models/review_mapping_handler.rb b/app/models/review_mapping_handler.rb index 924062a85..6b0951577 100644 --- a/app/models/review_mapping_handler.rb +++ b/app/models/review_mapping_handler.rb @@ -1,3 +1,13 @@ +# ReviewMappingHandler contains the **business logic** for creating, deleting, +# and managing ReviewResponseMap records on an assignment. It is deliberately +# separate from ReviewMappingsController (the HTTP layer): the controller +# parses params, authorises the request, and renders JSON; the handler +# performs the actual domain work using pluggable Strategy objects. +# +# Strategy classes live in ReviewMappingStrategies/ and encapsulate the +# pairing algorithm (round-robin, random, CSV, topic-fair, etc.). The handler +# is not tied to any particular strategy — it just calls +# `strategy.each_review_pair` and creates the resulting maps. class ReviewMappingHandler DEFAULT_OUTSTANDING_LIMIT = 2 diff --git a/app/services/calibration_per_item_summary.rb b/app/services/calibration_per_item_summary.rb index eea5ce3c8..49b4b20d9 100644 --- a/app/services/calibration_per_item_summary.rb +++ b/app/services/calibration_per_item_summary.rb @@ -1,6 +1,17 @@ # frozen_string_literal: true +# CalibrationPerItemSummary builds a per-rubric-item summary (score buckets + +# instructor score) from a pre-fetched collection of Response objects. +# +# NOTE: This class is superseded for the HTTP report endpoint by +# Reports::CalibrationReport, which uses an iterator-based pipeline +# (Template Method pattern) so responses are never bulk-loaded into memory. +# CalibrationPerItemSummary is retained for use by the calibration demo rake +# task (lib/tasks/calibration_demo.rake), which works with small in-memory +# arrays and does not need the streaming approach. class CalibrationPerItemSummary + # Convenience factory: constructs an instance and immediately calls build. + # Equivalent to `new(...).build`. def self.build(items:, instructor_response:, student_responses:) new( items: items, @@ -15,11 +26,15 @@ def initialize(items:, instructor_response:, student_responses:) @student_responses = Array(student_responses).compact end + # Returns an Array of per-item summary hashes sorted by item sequence number. + # Each hash contains the instructor score, instructor comment, per-score + # bucket counts, and the number of student responses that contributed. def build instructor_answers = answers_by_item(@instructor_response) latest_student_responses = latest_submitted_student_responses student_answers = latest_student_responses.to_h { |response| [response.id, answers_by_item(response)] } + # Sort by seq so the caller receives items in rubric display order. @items.sort_by(&:seq).map do |item| { item_id: item.id, @@ -35,6 +50,8 @@ def build private + # For each calibration map, keep only the most recently updated submitted + # response. Older submitted versions and any drafts are discarded. def latest_submitted_student_responses @student_responses .select(&:is_submitted) @@ -44,6 +61,8 @@ def latest_submitted_student_responses .compact end + # Returns a Hash mapping item_id → Answer for all answers in the response, + # or {} when the response is nil (e.g. instructor has no response yet). def answers_by_item(response) return {} unless response @@ -52,6 +71,9 @@ def answers_by_item(response) end end + # Tallies how many student responses scored each possible value for `item`. + # Returns a Hash of { score_string => count } covering every integer in the + # questionnaire's min..max range so the caller always gets a full histogram. def bucket_counts_for(item, responses, answers_by_response) buckets = score_range_for(item).each_with_object({}) do |score, counts| counts[score.to_s] = 0 diff --git a/app/services/reports/calibration_report.rb b/app/services/reports/calibration_report.rb index 2c57c07b0..87bfc3e2e 100644 --- a/app/services/reports/calibration_report.rb +++ b/app/services/reports/calibration_report.rb @@ -31,6 +31,11 @@ def setup @rubric_items = @instructor_response.rubric_items raise RubricMissing, 'Review rubric not found' if @rubric_items.empty? + # @bucket_counts is a two-level Hash: + # { item_id => { "0" => count, "1" => count, ..., "N" => count } } + # The inner Hash covers every integer in the questionnaire's score range so + # the presenter always receives a complete histogram (zero-filled buckets + # included) without needing to know the min/max score. @bucket_counts = empty_buckets_for(@rubric_items) @student_responses = [] end @@ -71,15 +76,16 @@ def payload def empty_buckets_for(items) items.each_with_object({}) do |item, hash| - min = item.questionnaire&.min_question_score.to_i - max = item.questionnaire&.max_question_score.to_i - max = min + 5 if max <= min - hash[item.id] = (min..max).each_with_object({}) { |score, b| b[score.to_s] = 0 } + # Delegate the valid score range to the Questionnaire (Information Expert). + # The fallback to class-level defaults lives in Questionnaire#score_range, + # not here, so calibration code does not carry knowledge of score bounds. + range = item.questionnaire&.score_range || (0..5) + hash[item.id] = range.each_with_object({}) { |score, b| b[score.to_s] = 0 } end end def per_item_summary(item) - instructor_answer = @instructor_response.scores.find { |a| a.item_id == item.id } + instructor_answer = @instructor_response.answer_for(item) { item_id: item.id, item_label: item.txt, diff --git a/spec/models/response_map_spec.rb b/spec/models/response_map_spec.rb index 0a84ef6de..58b44fee9 100644 --- a/spec/models/response_map_spec.rb +++ b/spec/models/response_map_spec.rb @@ -3,4 +3,27 @@ require 'rails_helper' RSpec.describe ResponseMap, type: :model do + describe '#review_status' do + let(:map) { create(:review_response_map) } + + it 'returns :not_started when no responses have been created' do + expect(map.review_status).to eq(:not_started) + end + + it 'returns :in_progress when a draft (unsubmitted) response exists' do + Response.create!(response_map: map, round: 1, version_num: 1, is_submitted: false) + expect(map.review_status).to eq(:in_progress) + end + + it 'returns :submitted when at least one submitted response exists' do + Response.create!(response_map: map, round: 1, version_num: 1, is_submitted: true) + expect(map.review_status).to eq(:submitted) + end + + it 'returns :submitted even when a later draft also exists' do + Response.create!(response_map: map, round: 1, version_num: 1, is_submitted: true) + Response.create!(response_map: map, round: 1, version_num: 2, is_submitted: false) + expect(map.review_status).to eq(:submitted) + end + end end diff --git a/spec/services/calibration_per_item_summary_spec.rb b/spec/services/calibration_per_item_summary_spec.rb index c0ac7ad74..a5c88c71d 100644 --- a/spec/services/calibration_per_item_summary_spec.rb +++ b/spec/services/calibration_per_item_summary_spec.rb @@ -2,186 +2,132 @@ require 'rails_helper' -RSpec.describe CalibrationPerItemSummary do - describe '.build' do - it 'summarizes submitted student scores by rubric item and includes instructor scores' do - questionnaire = create_questionnaire - code_quality = create(:item, questionnaire: questionnaire, txt: 'Code quality', seq: 1) - documentation = create(:item, questionnaire: questionnaire, txt: 'Documentation', seq: 2) - code_quality.update!(seq: 1) - documentation.update!(seq: 2) - assignment = create(:assignment) - - instructor_map = create(:review_response_map, :for_calibration, assignment: assignment) - instructor_response = create_response( - map: instructor_map, - submitted: true, - scores: { - code_quality => { answer: 4, comments: 'Clear implementation' }, - documentation => { answer: 5, comments: 'Very complete' } - } - ) +# Unit tests for Reports::CalibrationReport – the service that replaced the +# former CalibrationPerItemSummary class. These specs exercise the accumulate / +# per-item-summary logic directly (without going through the HTTP layer) so that +# bucket counting and latest-response selection are validated in isolation. +RSpec.describe Reports::CalibrationReport do + # ------------------------------------------------------------------------- + # Shared setup + # ------------------------------------------------------------------------- + let(:assignment) { create(:assignment) } + let(:reviewee_team) { create(:assignment_team, assignment: assignment) } + let(:instructor_participant) { create(:assignment_participant, assignment: assignment) } + let(:instructor_map) do + create(:review_response_map, :for_calibration, + assignment: assignment, + reviewer: instructor_participant, + reviewee: reviewee_team) + end - student_map_one = create(:review_response_map, :for_calibration, assignment: assignment) - student_map_two = create(:review_response_map, :for_calibration, assignment: assignment) + let(:questionnaire) do + Questionnaire.create!( + name: "Calibration Rubric #{SecureRandom.hex(4)}", + private: false, + min_question_score: 0, + max_question_score: 5, + instructor: Instructor.find(instructor_participant.user_id) + ) + end - create_response( - map: student_map_one, - submitted: true, - updated_at: 2.days.ago, - scores: { - code_quality => { answer: 1 }, - documentation => { answer: 2 } - } - ) - latest_student_one = create_response( - map: student_map_one, - submitted: true, - updated_at: 1.day.ago, - scores: { - code_quality => { answer: 3 }, - documentation => { answer: 2 } - } - ) - latest_student_two = create_response( - map: student_map_two, - submitted: true, - updated_at: 3.hours.ago, - scores: { - code_quality => { answer: 3 } - } - ) - unsubmitted_student_response = create_response( - map: student_map_two, - submitted: false, - updated_at: 1.hour.ago, - scores: { - code_quality => { answer: 5 }, - documentation => { answer: 5 } - } - ) + let!(:code_quality) { create(:item, questionnaire: questionnaire, txt: 'Code quality', seq: 1) } + let!(:documentation) { create(:item, questionnaire: questionnaire, txt: 'Documentation', seq: 2) } - summary = described_class.build( - items: [documentation, code_quality], - instructor_response: instructor_response, - student_responses: [ - latest_student_two, - unsubmitted_student_response, - latest_student_one - ] - ) + before do + AssignmentQuestionnaire.create!( + assignment: assignment, + questionnaire: questionnaire, + used_in_round: 1 + ) + end - expect(summary).to eq([ - { - item_id: code_quality.id, - item_label: 'Code quality', - item_seq: code_quality.seq, - instructor_score: 4, - instructor_comment: 'Clear implementation', - bucket_counts: { - '0' => 0, - '1' => 0, - '2' => 0, - '3' => 2, - '4' => 0, - '5' => 0 - }, - student_response_count: 2 - }, - { - item_id: documentation.id, - item_label: 'Documentation', - item_seq: documentation.seq, - instructor_score: 5, - instructor_comment: 'Very complete', - bucket_counts: { - '0' => 0, - '1' => 0, - '2' => 1, - '3' => 0, - '4' => 0, - '5' => 0 - }, - student_response_count: 2 - } - ]) + def make_response(map:, submitted:, scores:, updated_at: Time.current) + r = Response.create!( + response_map: map, round: 1, version_num: 1, + is_submitted: submitted, created_at: updated_at, updated_at: updated_at + ) + scores.each do |item, score| + Answer.create!(response: r, item: item, + answer: score[:answer], comments: score[:comments]) end + r + end - it 'uses the latest submitted response per map even when a newer draft exists' do - questionnaire = create_questionnaire - item = create(:item, questionnaire: questionnaire, txt: 'Accuracy', seq: 1) - assignment = create(:assignment) - response_map = create(:review_response_map, :for_calibration, assignment: assignment) - - submitted_response = create_response( - map: response_map, - submitted: true, - updated_at: 2.hours.ago, + # ------------------------------------------------------------------------- + describe '#render' do + it 'accumulates bucket counts from submitted student responses' do + make_response( + map: instructor_map, submitted: true, scores: { - item => { answer: 2 } + code_quality => { answer: 4, comments: 'Clear implementation' }, + documentation => { answer: 5, comments: 'Complete docs' } } ) - draft_response = create_response( - map: response_map, - submitted: false, - updated_at: 1.hour.ago, + + student_participant = create(:assignment_participant, assignment: assignment) + student_map = create(:review_response_map, :for_calibration, + assignment: assignment, + reviewer: student_participant, + reviewee: reviewee_team) + make_response( + map: student_map, submitted: true, scores: { - item => { answer: 5 } + code_quality => { answer: 3, comments: 'Mostly clear' }, + documentation => { answer: 5, comments: 'Strong docs' } } ) - summary = described_class.build( - items: [item], - instructor_response: nil, - student_responses: [submitted_response, draft_response] - ) + result = described_class.new(instructor_map).render - expect(summary.first[:bucket_counts]['2']).to eq(1) - expect(summary.first[:bucket_counts]['5']).to eq(0) - expect(summary.first[:student_response_count]).to eq(1) - end - end + cq_summary = result[:per_item_summary].find { |s| s[:item_id] == code_quality.id } + expect(cq_summary[:instructor_score]).to eq(4) + expect(cq_summary[:bucket_counts]['3']).to eq(1) + expect(cq_summary[:bucket_counts]['4']).to eq(0) + expect(cq_summary[:student_response_count]).to eq(1) - def create_response(map:, submitted:, scores:, updated_at: Time.current) - response = Response.create!( - response_map: map, - round: 1, - version_num: 1, - is_submitted: submitted, - created_at: updated_at, - updated_at: updated_at - ) + doc_summary = result[:per_item_summary].find { |s| s[:item_id] == documentation.id } + expect(doc_summary[:instructor_score]).to eq(5) + expect(doc_summary[:bucket_counts]['5']).to eq(1) + end - scores.each do |item, score| - Answer.create!( - response: response, - item: item, - answer: score[:answer], - comments: score[:comments] + it 'uses only the latest submitted response per student map, ignoring older or draft responses' do + make_response( + map: instructor_map, submitted: true, + scores: { code_quality => { answer: 4 }, documentation => { answer: 5 } } ) + + student_participant = create(:assignment_participant, assignment: assignment) + student_map = create(:review_response_map, :for_calibration, + assignment: assignment, + reviewer: student_participant, + reviewee: reviewee_team) + + make_response(map: student_map, submitted: true, updated_at: 2.days.ago, + scores: { code_quality => { answer: 1 }, documentation => { answer: 2 } }) + make_response(map: student_map, submitted: true, updated_at: 1.day.ago, + scores: { code_quality => { answer: 3 }, documentation => { answer: 2 } }) + make_response(map: student_map, submitted: false, updated_at: 1.hour.ago, + scores: { code_quality => { answer: 5 }, documentation => { answer: 5 } }) + + result = described_class.new(instructor_map).render + + cq = result[:per_item_summary].find { |s| s[:item_id] == code_quality.id } + expect(cq[:bucket_counts]['3']).to eq(1) + expect(cq[:bucket_counts]['1']).to eq(0) + expect(cq[:bucket_counts]['5']).to eq(0) end - response - end + it 'raises InstructorResponseMissing when the instructor has not submitted' do + expect { described_class.new(instructor_map).render } + .to raise_error(Reports::CalibrationReport::InstructorResponseMissing) + end - def create_questionnaire - Questionnaire.create!( - name: "Calibration Rubric #{SecureRandom.hex(4)}", - private: false, - min_question_score: 0, - max_question_score: 5, - instructor: create_instructor - ) - end + it 'raises InstructorResponseMissing when the instructor response is a draft' do + make_response(map: instructor_map, submitted: false, + scores: { code_quality => { answer: 4 } }) - def create_instructor - Instructor.create!( - name: "instructor_#{SecureRandom.hex(4)}", - email: "instructor_#{SecureRandom.hex(4)}@example.com", - password: 'password', - full_name: 'Calibration Instructor', - role: create(:role, name: "Instructor #{SecureRandom.hex(4)}"), - institution: create(:institution) - ) + expect { described_class.new(instructor_map).render } + .to raise_error(Reports::CalibrationReport::InstructorResponseMissing) + end end end From a5aae2d227e46978a0e4d0b1a41475921b66216a Mon Sep 17 00:00:00 2001 From: ruju4a Date: Mon, 27 Apr 2026 01:15:19 -0400 Subject: [PATCH 17/25] Apply feedback: expert principle fixes and comments --- app/models/questionnaire.rb | 10 + app/models/response.rb | 7 + app/models/review_mapping_handler.rb | 10 + app/services/calibration_per_item_summary.rb | 22 ++ app/services/reports/calibration_report.rb | 16 +- spec/models/response_map_spec.rb | 23 ++ .../calibration_per_item_summary_spec.rb | 266 +++++++----------- 7 files changed, 189 insertions(+), 165 deletions(-) diff --git a/app/models/questionnaire.rb b/app/models/questionnaire.rb index b7eeee017..e78de026c 100644 --- a/app/models/questionnaire.rb +++ b/app/models/questionnaire.rb @@ -85,6 +85,16 @@ def as_json(options = {}) DEFAULT_MIN_QUESTION_SCORE = 0 # The lowest score that a reviewer can assign to any questionnaire question DEFAULT_MAX_QUESTION_SCORE = 5 # The highest score that a reviewer can assign to any questionnaire question + + # Returns the integer Range of valid scores for this questionnaire's items, + # falling back to the class-level defaults when the DB columns are unset. + # Consumers (e.g. report histogram builders) should call this rather than + # hard-coding the 0..5 range themselves. + def score_range + min = (min_question_score || DEFAULT_MIN_QUESTION_SCORE).to_i + max = (max_question_score || DEFAULT_MAX_QUESTION_SCORE).to_i + min..max + end DEFAULT_QUESTIONNAIRE_URL = 'http://www.courses.ncsu.edu/csc517'.freeze QUESTIONNAIRE_TYPES = ['ReviewQuestionnaire', 'MetareviewQuestionnaire', diff --git a/app/models/response.rb b/app/models/response.rb index 4fa07bd06..aaa4b0926 100644 --- a/app/models/response.rb +++ b/app/models/response.rb @@ -62,6 +62,13 @@ def aggregate_questionnaire_score sum end + # Returns the Answer this response recorded for a specific rubric item, or + # nil if no answer was given. Centralises item-lookup here (Information Expert) + # so callers don't scatter `scores.find { |a| a.item_id == item.id }` everywhere. + def answer_for(item) + scores.find { |a| a.item_id == item.id } + end + # Ordered rubric items used for this response, or [] if the rubric cannot be # resolved (e.g. no AssignmentQuestionnaire for this round). Belongs here # rather than in a controller because the response knows which questionnaire diff --git a/app/models/review_mapping_handler.rb b/app/models/review_mapping_handler.rb index 924062a85..6b0951577 100644 --- a/app/models/review_mapping_handler.rb +++ b/app/models/review_mapping_handler.rb @@ -1,3 +1,13 @@ +# ReviewMappingHandler contains the **business logic** for creating, deleting, +# and managing ReviewResponseMap records on an assignment. It is deliberately +# separate from ReviewMappingsController (the HTTP layer): the controller +# parses params, authorises the request, and renders JSON; the handler +# performs the actual domain work using pluggable Strategy objects. +# +# Strategy classes live in ReviewMappingStrategies/ and encapsulate the +# pairing algorithm (round-robin, random, CSV, topic-fair, etc.). The handler +# is not tied to any particular strategy — it just calls +# `strategy.each_review_pair` and creates the resulting maps. class ReviewMappingHandler DEFAULT_OUTSTANDING_LIMIT = 2 diff --git a/app/services/calibration_per_item_summary.rb b/app/services/calibration_per_item_summary.rb index eea5ce3c8..49b4b20d9 100644 --- a/app/services/calibration_per_item_summary.rb +++ b/app/services/calibration_per_item_summary.rb @@ -1,6 +1,17 @@ # frozen_string_literal: true +# CalibrationPerItemSummary builds a per-rubric-item summary (score buckets + +# instructor score) from a pre-fetched collection of Response objects. +# +# NOTE: This class is superseded for the HTTP report endpoint by +# Reports::CalibrationReport, which uses an iterator-based pipeline +# (Template Method pattern) so responses are never bulk-loaded into memory. +# CalibrationPerItemSummary is retained for use by the calibration demo rake +# task (lib/tasks/calibration_demo.rake), which works with small in-memory +# arrays and does not need the streaming approach. class CalibrationPerItemSummary + # Convenience factory: constructs an instance and immediately calls build. + # Equivalent to `new(...).build`. def self.build(items:, instructor_response:, student_responses:) new( items: items, @@ -15,11 +26,15 @@ def initialize(items:, instructor_response:, student_responses:) @student_responses = Array(student_responses).compact end + # Returns an Array of per-item summary hashes sorted by item sequence number. + # Each hash contains the instructor score, instructor comment, per-score + # bucket counts, and the number of student responses that contributed. def build instructor_answers = answers_by_item(@instructor_response) latest_student_responses = latest_submitted_student_responses student_answers = latest_student_responses.to_h { |response| [response.id, answers_by_item(response)] } + # Sort by seq so the caller receives items in rubric display order. @items.sort_by(&:seq).map do |item| { item_id: item.id, @@ -35,6 +50,8 @@ def build private + # For each calibration map, keep only the most recently updated submitted + # response. Older submitted versions and any drafts are discarded. def latest_submitted_student_responses @student_responses .select(&:is_submitted) @@ -44,6 +61,8 @@ def latest_submitted_student_responses .compact end + # Returns a Hash mapping item_id → Answer for all answers in the response, + # or {} when the response is nil (e.g. instructor has no response yet). def answers_by_item(response) return {} unless response @@ -52,6 +71,9 @@ def answers_by_item(response) end end + # Tallies how many student responses scored each possible value for `item`. + # Returns a Hash of { score_string => count } covering every integer in the + # questionnaire's min..max range so the caller always gets a full histogram. def bucket_counts_for(item, responses, answers_by_response) buckets = score_range_for(item).each_with_object({}) do |score, counts| counts[score.to_s] = 0 diff --git a/app/services/reports/calibration_report.rb b/app/services/reports/calibration_report.rb index 2c57c07b0..87bfc3e2e 100644 --- a/app/services/reports/calibration_report.rb +++ b/app/services/reports/calibration_report.rb @@ -31,6 +31,11 @@ def setup @rubric_items = @instructor_response.rubric_items raise RubricMissing, 'Review rubric not found' if @rubric_items.empty? + # @bucket_counts is a two-level Hash: + # { item_id => { "0" => count, "1" => count, ..., "N" => count } } + # The inner Hash covers every integer in the questionnaire's score range so + # the presenter always receives a complete histogram (zero-filled buckets + # included) without needing to know the min/max score. @bucket_counts = empty_buckets_for(@rubric_items) @student_responses = [] end @@ -71,15 +76,16 @@ def payload def empty_buckets_for(items) items.each_with_object({}) do |item, hash| - min = item.questionnaire&.min_question_score.to_i - max = item.questionnaire&.max_question_score.to_i - max = min + 5 if max <= min - hash[item.id] = (min..max).each_with_object({}) { |score, b| b[score.to_s] = 0 } + # Delegate the valid score range to the Questionnaire (Information Expert). + # The fallback to class-level defaults lives in Questionnaire#score_range, + # not here, so calibration code does not carry knowledge of score bounds. + range = item.questionnaire&.score_range || (0..5) + hash[item.id] = range.each_with_object({}) { |score, b| b[score.to_s] = 0 } end end def per_item_summary(item) - instructor_answer = @instructor_response.scores.find { |a| a.item_id == item.id } + instructor_answer = @instructor_response.answer_for(item) { item_id: item.id, item_label: item.txt, diff --git a/spec/models/response_map_spec.rb b/spec/models/response_map_spec.rb index 0a84ef6de..58b44fee9 100644 --- a/spec/models/response_map_spec.rb +++ b/spec/models/response_map_spec.rb @@ -3,4 +3,27 @@ require 'rails_helper' RSpec.describe ResponseMap, type: :model do + describe '#review_status' do + let(:map) { create(:review_response_map) } + + it 'returns :not_started when no responses have been created' do + expect(map.review_status).to eq(:not_started) + end + + it 'returns :in_progress when a draft (unsubmitted) response exists' do + Response.create!(response_map: map, round: 1, version_num: 1, is_submitted: false) + expect(map.review_status).to eq(:in_progress) + end + + it 'returns :submitted when at least one submitted response exists' do + Response.create!(response_map: map, round: 1, version_num: 1, is_submitted: true) + expect(map.review_status).to eq(:submitted) + end + + it 'returns :submitted even when a later draft also exists' do + Response.create!(response_map: map, round: 1, version_num: 1, is_submitted: true) + Response.create!(response_map: map, round: 1, version_num: 2, is_submitted: false) + expect(map.review_status).to eq(:submitted) + end + end end diff --git a/spec/services/calibration_per_item_summary_spec.rb b/spec/services/calibration_per_item_summary_spec.rb index c0ac7ad74..a5c88c71d 100644 --- a/spec/services/calibration_per_item_summary_spec.rb +++ b/spec/services/calibration_per_item_summary_spec.rb @@ -2,186 +2,132 @@ require 'rails_helper' -RSpec.describe CalibrationPerItemSummary do - describe '.build' do - it 'summarizes submitted student scores by rubric item and includes instructor scores' do - questionnaire = create_questionnaire - code_quality = create(:item, questionnaire: questionnaire, txt: 'Code quality', seq: 1) - documentation = create(:item, questionnaire: questionnaire, txt: 'Documentation', seq: 2) - code_quality.update!(seq: 1) - documentation.update!(seq: 2) - assignment = create(:assignment) - - instructor_map = create(:review_response_map, :for_calibration, assignment: assignment) - instructor_response = create_response( - map: instructor_map, - submitted: true, - scores: { - code_quality => { answer: 4, comments: 'Clear implementation' }, - documentation => { answer: 5, comments: 'Very complete' } - } - ) +# Unit tests for Reports::CalibrationReport – the service that replaced the +# former CalibrationPerItemSummary class. These specs exercise the accumulate / +# per-item-summary logic directly (without going through the HTTP layer) so that +# bucket counting and latest-response selection are validated in isolation. +RSpec.describe Reports::CalibrationReport do + # ------------------------------------------------------------------------- + # Shared setup + # ------------------------------------------------------------------------- + let(:assignment) { create(:assignment) } + let(:reviewee_team) { create(:assignment_team, assignment: assignment) } + let(:instructor_participant) { create(:assignment_participant, assignment: assignment) } + let(:instructor_map) do + create(:review_response_map, :for_calibration, + assignment: assignment, + reviewer: instructor_participant, + reviewee: reviewee_team) + end - student_map_one = create(:review_response_map, :for_calibration, assignment: assignment) - student_map_two = create(:review_response_map, :for_calibration, assignment: assignment) + let(:questionnaire) do + Questionnaire.create!( + name: "Calibration Rubric #{SecureRandom.hex(4)}", + private: false, + min_question_score: 0, + max_question_score: 5, + instructor: Instructor.find(instructor_participant.user_id) + ) + end - create_response( - map: student_map_one, - submitted: true, - updated_at: 2.days.ago, - scores: { - code_quality => { answer: 1 }, - documentation => { answer: 2 } - } - ) - latest_student_one = create_response( - map: student_map_one, - submitted: true, - updated_at: 1.day.ago, - scores: { - code_quality => { answer: 3 }, - documentation => { answer: 2 } - } - ) - latest_student_two = create_response( - map: student_map_two, - submitted: true, - updated_at: 3.hours.ago, - scores: { - code_quality => { answer: 3 } - } - ) - unsubmitted_student_response = create_response( - map: student_map_two, - submitted: false, - updated_at: 1.hour.ago, - scores: { - code_quality => { answer: 5 }, - documentation => { answer: 5 } - } - ) + let!(:code_quality) { create(:item, questionnaire: questionnaire, txt: 'Code quality', seq: 1) } + let!(:documentation) { create(:item, questionnaire: questionnaire, txt: 'Documentation', seq: 2) } - summary = described_class.build( - items: [documentation, code_quality], - instructor_response: instructor_response, - student_responses: [ - latest_student_two, - unsubmitted_student_response, - latest_student_one - ] - ) + before do + AssignmentQuestionnaire.create!( + assignment: assignment, + questionnaire: questionnaire, + used_in_round: 1 + ) + end - expect(summary).to eq([ - { - item_id: code_quality.id, - item_label: 'Code quality', - item_seq: code_quality.seq, - instructor_score: 4, - instructor_comment: 'Clear implementation', - bucket_counts: { - '0' => 0, - '1' => 0, - '2' => 0, - '3' => 2, - '4' => 0, - '5' => 0 - }, - student_response_count: 2 - }, - { - item_id: documentation.id, - item_label: 'Documentation', - item_seq: documentation.seq, - instructor_score: 5, - instructor_comment: 'Very complete', - bucket_counts: { - '0' => 0, - '1' => 0, - '2' => 1, - '3' => 0, - '4' => 0, - '5' => 0 - }, - student_response_count: 2 - } - ]) + def make_response(map:, submitted:, scores:, updated_at: Time.current) + r = Response.create!( + response_map: map, round: 1, version_num: 1, + is_submitted: submitted, created_at: updated_at, updated_at: updated_at + ) + scores.each do |item, score| + Answer.create!(response: r, item: item, + answer: score[:answer], comments: score[:comments]) end + r + end - it 'uses the latest submitted response per map even when a newer draft exists' do - questionnaire = create_questionnaire - item = create(:item, questionnaire: questionnaire, txt: 'Accuracy', seq: 1) - assignment = create(:assignment) - response_map = create(:review_response_map, :for_calibration, assignment: assignment) - - submitted_response = create_response( - map: response_map, - submitted: true, - updated_at: 2.hours.ago, + # ------------------------------------------------------------------------- + describe '#render' do + it 'accumulates bucket counts from submitted student responses' do + make_response( + map: instructor_map, submitted: true, scores: { - item => { answer: 2 } + code_quality => { answer: 4, comments: 'Clear implementation' }, + documentation => { answer: 5, comments: 'Complete docs' } } ) - draft_response = create_response( - map: response_map, - submitted: false, - updated_at: 1.hour.ago, + + student_participant = create(:assignment_participant, assignment: assignment) + student_map = create(:review_response_map, :for_calibration, + assignment: assignment, + reviewer: student_participant, + reviewee: reviewee_team) + make_response( + map: student_map, submitted: true, scores: { - item => { answer: 5 } + code_quality => { answer: 3, comments: 'Mostly clear' }, + documentation => { answer: 5, comments: 'Strong docs' } } ) - summary = described_class.build( - items: [item], - instructor_response: nil, - student_responses: [submitted_response, draft_response] - ) + result = described_class.new(instructor_map).render - expect(summary.first[:bucket_counts]['2']).to eq(1) - expect(summary.first[:bucket_counts]['5']).to eq(0) - expect(summary.first[:student_response_count]).to eq(1) - end - end + cq_summary = result[:per_item_summary].find { |s| s[:item_id] == code_quality.id } + expect(cq_summary[:instructor_score]).to eq(4) + expect(cq_summary[:bucket_counts]['3']).to eq(1) + expect(cq_summary[:bucket_counts]['4']).to eq(0) + expect(cq_summary[:student_response_count]).to eq(1) - def create_response(map:, submitted:, scores:, updated_at: Time.current) - response = Response.create!( - response_map: map, - round: 1, - version_num: 1, - is_submitted: submitted, - created_at: updated_at, - updated_at: updated_at - ) + doc_summary = result[:per_item_summary].find { |s| s[:item_id] == documentation.id } + expect(doc_summary[:instructor_score]).to eq(5) + expect(doc_summary[:bucket_counts]['5']).to eq(1) + end - scores.each do |item, score| - Answer.create!( - response: response, - item: item, - answer: score[:answer], - comments: score[:comments] + it 'uses only the latest submitted response per student map, ignoring older or draft responses' do + make_response( + map: instructor_map, submitted: true, + scores: { code_quality => { answer: 4 }, documentation => { answer: 5 } } ) + + student_participant = create(:assignment_participant, assignment: assignment) + student_map = create(:review_response_map, :for_calibration, + assignment: assignment, + reviewer: student_participant, + reviewee: reviewee_team) + + make_response(map: student_map, submitted: true, updated_at: 2.days.ago, + scores: { code_quality => { answer: 1 }, documentation => { answer: 2 } }) + make_response(map: student_map, submitted: true, updated_at: 1.day.ago, + scores: { code_quality => { answer: 3 }, documentation => { answer: 2 } }) + make_response(map: student_map, submitted: false, updated_at: 1.hour.ago, + scores: { code_quality => { answer: 5 }, documentation => { answer: 5 } }) + + result = described_class.new(instructor_map).render + + cq = result[:per_item_summary].find { |s| s[:item_id] == code_quality.id } + expect(cq[:bucket_counts]['3']).to eq(1) + expect(cq[:bucket_counts]['1']).to eq(0) + expect(cq[:bucket_counts]['5']).to eq(0) end - response - end + it 'raises InstructorResponseMissing when the instructor has not submitted' do + expect { described_class.new(instructor_map).render } + .to raise_error(Reports::CalibrationReport::InstructorResponseMissing) + end - def create_questionnaire - Questionnaire.create!( - name: "Calibration Rubric #{SecureRandom.hex(4)}", - private: false, - min_question_score: 0, - max_question_score: 5, - instructor: create_instructor - ) - end + it 'raises InstructorResponseMissing when the instructor response is a draft' do + make_response(map: instructor_map, submitted: false, + scores: { code_quality => { answer: 4 } }) - def create_instructor - Instructor.create!( - name: "instructor_#{SecureRandom.hex(4)}", - email: "instructor_#{SecureRandom.hex(4)}@example.com", - password: 'password', - full_name: 'Calibration Instructor', - role: create(:role, name: "Instructor #{SecureRandom.hex(4)}"), - institution: create(:institution) - ) + expect { described_class.new(instructor_map).render } + .to raise_error(Reports::CalibrationReport::InstructorResponseMissing) + end end end From 32549ed58888ca59bc67d6916772a6d5178108a7 Mon Sep 17 00:00:00 2001 From: ruju4a Date: Mon, 27 Apr 2026 15:44:38 -0400 Subject: [PATCH 18/25] Updated specs for reports controller --- spec/models/team_factory_spec.rb | 90 ------ .../api/v1/calibration_participants_spec.rb | 298 ------------------ .../api/v1/reports_calibration_spec.rb | 115 +++++++ .../calibration_per_item_summary_spec.rb | 133 -------- 4 files changed, 115 insertions(+), 521 deletions(-) delete mode 100644 spec/models/team_factory_spec.rb delete mode 100644 spec/requests/api/v1/calibration_participants_spec.rb delete mode 100644 spec/services/calibration_per_item_summary_spec.rb diff --git a/spec/models/team_factory_spec.rb b/spec/models/team_factory_spec.rb deleted file mode 100644 index 736353033..000000000 --- a/spec/models/team_factory_spec.rb +++ /dev/null @@ -1,90 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -# Tests for Team.create_team_for_participant - the factory used by the -# Calibration tab (and elsewhere) to create the team that backs an individual -# submitter. -RSpec.describe Team, type: :model do - include RolesHelper - - before(:all) { @roles = create_roles_hierarchy } - - let(:institution) { Institution.create!(name: 'NC State') } - - def create_student(suffix) - User.create!( - name: suffix, - email: "#{suffix}@example.com", - full_name: suffix.titleize, - password_digest: 'password', - role_id: @roles[:student].id, - institution_id: institution.id - ) - end - - let(:instructor) do - User.create!( - name: 'calibration_instructor_factory', - full_name: 'Calibration Instructor', - email: 'calibration_instructor_factory@example.com', - password_digest: 'password', - role_id: @roles[:instructor].id, - institution_id: institution.id - ) - end - - let(:assignment) { Assignment.create!(name: 'Calibration A', instructor_id: instructor.id, max_team_size: 1) } - let(:course) { Course.create!(name: 'Course C', instructor_id: instructor.id, institution_id: institution.id, directory_path: '/c1') } - - describe '.create_team_for_participant' do - context 'for an AssignmentParticipant' do - it 'creates an AssignmentTeam whose parent is the assignment' do - user = create_student('calib_sub1') - participant = AssignmentParticipant.create!(parent_id: assignment.id, user: user, handle: user.name) - - team = Team.create_team_for_participant(participant) - - expect(team).to be_a(AssignmentTeam) - expect(team.parent_id).to eq(assignment.id) - expect(team.participants).to include(participant) - end - - it 'generates a stable, unique name when none is provided' do - user1 = create_student('calib_sub_a') - user2 = create_student('calib_sub_b') - p1 = AssignmentParticipant.create!(parent_id: assignment.id, user: user1, handle: user1.name) - p2 = AssignmentParticipant.create!(parent_id: assignment.id, user: user2, handle: user2.name) - - t1 = Team.create_team_for_participant(p1) - t2 = Team.create_team_for_participant(p2) - - expect(t1.name).to include(user1.name) - expect(t2.name).to include(user2.name) - expect(t1.name).not_to eq(t2.name) - end - end - - context 'for a CourseParticipant' do - it 'creates a CourseTeam whose parent is the course' do - user = create_student('course_sub1') - participant = CourseParticipant.create!(parent_id: course.id, user: user, handle: user.name) - - team = Team.create_team_for_participant(participant) - - expect(team).to be_a(CourseTeam) - expect(team.parent_id).to eq(course.id) - expect(team.participants).to include(participant) - end - end - - it 'raises on nil participant' do - expect { Team.create_team_for_participant(nil) }.to raise_error(ArgumentError) - end - - it 'raises on an unsupported participant type' do - fake = Object.new - expect { Team.create_team_for_participant(fake) }.to raise_error(ArgumentError, /Unsupported/) - end - end -end diff --git a/spec/requests/api/v1/calibration_participants_spec.rb b/spec/requests/api/v1/calibration_participants_spec.rb deleted file mode 100644 index 890f5db0e..000000000 --- a/spec/requests/api/v1/calibration_participants_spec.rb +++ /dev/null @@ -1,298 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' -require 'json_web_token' - -# Behaviour-driven specs for the "Add calibration participant" feature on the -# assignment Calibration tab. Scenarios are written from the perspective of the -# instructor using the UI and exercise the public HTTP contract end-to-end: -# -# * GET /assignments/:id/review_mappings/calibration_participants -# * POST /assignments/:id/review_mappings/calibration_participants -# * DELETE /assignments/:id/review_mappings/calibration_participants/:participant_id -# -# Each `it` block reads as a scenario with explicit Given / When / Then phases -# so the spec doubles as living documentation of the feature's behaviour. -RSpec.describe 'Feature: Designating calibration submitters', type: :request do - # --------------------------------------------------------------------------- - # Background fixtures (the "Given" that every scenario shares) - # --------------------------------------------------------------------------- - let!(:super_admin_role) { Role.find_or_create_by!(name: 'Super Administrator') } - let!(:admin_role) { Role.find_or_create_by!(name: 'Administrator', parent: super_admin_role) } - let!(:instructor_role) { Role.find_or_create_by!(name: 'Instructor', parent: admin_role) } - let!(:ta_role) { Role.find_or_create_by!(name: 'Teaching Assistant', parent: instructor_role) } - let!(:student_role) { Role.find_or_create_by!(name: 'Student', parent: ta_role) } - - let(:institution) { Institution.create!(name: 'NC State') } - - let(:instructor_user) do - User.create!( - name: 'calib_instr', full_name: 'Calibration Instructor', - email: 'calib_instr@example.com', password: 'password', - role: instructor_role, institution: institution - ) - end - - let(:other_student) do - User.create!( - name: 'calib_student_user', full_name: 'Calibration Student', - email: 'calib_student_user@example.com', password: 'password', - role: student_role, institution: institution - ) - end - - # The phantom calibration submitter the instructor will type into the form. - let(:phantom_user) do - User.create!( - name: 'unctlt1', full_name: 'UNC TLT 1', - email: 'unctlt1@example.com', password: 'password', - role: student_role, institution: institution - ) - end - - let(:assignment) do - Assignment.create!(name: 'Calibration Assignment', instructor: instructor_user, max_team_size: 1) - end - - let(:base_path) { "/assignments/#{assignment.id}/review_mappings/calibration_participants" } - let(:as_instructor) { { 'Authorization' => "Bearer #{JsonWebToken.encode(id: instructor_user.id)}" } } - let(:as_student) { { 'Authorization' => "Bearer #{JsonWebToken.encode(id: other_student.id)}" } } - - # Convenience helper used in many "When" steps. - def add_calibration_participant(username, headers: as_instructor) - post base_path, params: { username: username }, headers: headers - end - - # --------------------------------------------------------------------------- - # Scenario group: adding a participant - # --------------------------------------------------------------------------- - describe 'Scenario: an instructor adds a new calibration submitter by username' do - it 'creates the participant, the team, and the for_calibration map atomically' do - # Given the phantom user exists - phantom_user - - # When the instructor submits "unctlt1" through the calibration tab - expect { - add_calibration_participant('unctlt1') - }.to change { AssignmentParticipant.where(parent_id: assignment.id, user_id: phantom_user.id).count }.by(1) - .and change { AssignmentTeam.where(parent_id: assignment.id).count }.by(1) - .and change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count }.by(1) - - # Then the API responds 201 with the new row - expect(response).to have_http_status(:created) - json = JSON.parse(response.body) - expect(json).to include( - 'username' => 'unctlt1', - 'full_name' => 'UNC TLT 1' - ) - expect(json['team_id']).to be_present - expect(json['instructor_review_map_id']).to be_present - expect(json['instructor_review_status']).to eq('not_started') - - # And the calibration map is wired to the instructor as reviewer - map = ReviewResponseMap.find(json['instructor_review_map_id']) - expect(map.for_calibration).to be true - expect(map.reviewed_object_id).to eq(assignment.id) - - instructor_participant = AssignmentParticipant.find_by(parent_id: assignment.id, user_id: instructor_user.id) - expect(map.reviewer_id).to eq(instructor_participant.id) - expect(map.reviewee_id).to eq(json['team_id']) - end - - it 'accepts an email address in place of a username' do - # Given the phantom user exists - phantom_user - - # When the instructor types the user's email instead of their handle - add_calibration_participant('unctlt1@example.com') - - # Then the user is still resolved and added - expect(response).to have_http_status(:created) - expect(JSON.parse(response.body)['username']).to eq('unctlt1') - end - end - - # --------------------------------------------------------------------------- - # Scenario group: invalid input - # --------------------------------------------------------------------------- - describe 'Scenario: invalid input from the instructor' do - it 'rejects a request with a missing username with HTTP 400' do - # When the instructor submits an empty form - post base_path, params: {}, headers: as_instructor - - # Then the request is rejected as bad input - expect(response).to have_http_status(:bad_request) - expect(JSON.parse(response.body)['error']).to match(/username is required/i) - end - - it 'rejects a blank/whitespace username with HTTP 400' do - add_calibration_participant(' ') - expect(response).to have_http_status(:bad_request) - end - - it 'returns HTTP 404 when no user matches the username or email' do - # When the instructor types a name that does not exist - add_calibration_participant('nope') - - # Then nothing is created and a 404 is returned - expect(response).to have_http_status(:not_found) - expect(ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true)).to be_empty - end - end - - # --------------------------------------------------------------------------- - # Scenario group: idempotence and re-adding - # --------------------------------------------------------------------------- - describe 'Scenario: re-adding the same submitter' do - it 'is idempotent and does not create duplicate maps or teams' do - # Given the participant has already been added once - phantom_user - add_calibration_participant('unctlt1') - expect(response).to have_http_status(:created) - - maps_before = ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count - teams_before = AssignmentTeam.where(parent_id: assignment.id).count - - # When the instructor adds the same person again - add_calibration_participant('unctlt1') - - # Then no extra rows are created and the response is still successful - expect(response).to have_http_status(:created) - expect(ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count).to eq(maps_before) - expect(AssignmentTeam.where(parent_id: assignment.id).count).to eq(teams_before) - end - end - - # --------------------------------------------------------------------------- - # Scenario group: authorization - # --------------------------------------------------------------------------- - describe 'Scenario: only teaching staff may add calibration submitters' do - it 'rejects students with HTTP 403 and creates nothing' do - phantom_user - - expect { - add_calibration_participant('unctlt1', headers: as_student) - }.not_to change { ReviewResponseMap.count } - - expect(response).to have_http_status(:forbidden) - end - - it 'rejects unauthenticated requests with HTTP 401' do - phantom_user - - post base_path, params: { username: 'unctlt1' } - expect([401, 403]).to include(response.status), "expected 401/403 for unauthenticated request, got #{response.status}" - end - end - - # --------------------------------------------------------------------------- - # Scenario group: listing the calibration submitters - # --------------------------------------------------------------------------- - describe 'Scenario: instructor views the list of calibration submitters' do - it 'returns each submitter with their team, hyperlinks, and submitted files' do - # Given a calibration submitter already exists for this assignment - phantom_user - add_calibration_participant('unctlt1') - added_row = JSON.parse(response.body) - - # And that submitter has uploaded a file and a hyperlink to their team - team = AssignmentTeam.find(added_row['team_id']) - SubmissionRecord.create!( - record_type: 'file', content: 'submission/report.pdf', operation: 'Submit File', - team_id: team.id, user: phantom_user.name, assignment_id: assignment.id - ) - team.update!(submitted_hyperlinks: YAML.dump(['https://example.com/submission'])) - - # When the instructor opens the calibration tab - get base_path, headers: as_instructor - - # Then the response contains exactly one calibration submitter row - expect(response).to have_http_status(:ok) - json = JSON.parse(response.body) - expect(json['assignment_id']).to eq(assignment.id) - expect(json['calibration_participants'].length).to eq(1) - - # And that row exposes the submitter's hyperlinks and files - entry = json['calibration_participants'].first - expect(entry['username']).to eq('unctlt1') - expect(entry['instructor_review_map_id']).to eq(added_row['instructor_review_map_id']) - expect(entry['submissions']['hyperlinks']).to eq(['https://example.com/submission']) - expect(entry['submissions']['files'].first).to include( - 'name' => 'report.pdf', - 'path' => 'submission/report.pdf', - 'submitted_by' => phantom_user.name - ) - end - - it 'returns an empty list when no calibration submitters have been added' do - # When the instructor opens the calibration tab on a fresh assignment - get base_path, headers: as_instructor - - # Then the list is empty (but the response is well-formed) - expect(response).to have_http_status(:ok) - json = JSON.parse(response.body) - expect(json['assignment_id']).to eq(assignment.id) - expect(json['calibration_participants']).to eq([]) - end - - it 'reflects the instructor review status (not_started -> in_progress -> submitted)' do - # Given a calibration submitter exists - phantom_user - add_calibration_participant('unctlt1') - map_id = JSON.parse(response.body)['instructor_review_map_id'] - map = ReviewResponseMap.find(map_id) - - # Then before any review work the status is :not_started - get base_path, headers: as_instructor - expect(JSON.parse(response.body)['calibration_participants'].first['instructor_review_status']).to eq('not_started') - - # When the instructor saves a draft response - Response.create!(response_map: map, round: 1, version_num: 1, is_submitted: false) - get base_path, headers: as_instructor - expect(JSON.parse(response.body)['calibration_participants'].first['instructor_review_status']).to eq('in_progress') - - # When the instructor submits the response - Response.where(map_id: map.id).update_all(is_submitted: true) - get base_path, headers: as_instructor - expect(JSON.parse(response.body)['calibration_participants'].first['instructor_review_status']).to eq('submitted') - end - end - - # --------------------------------------------------------------------------- - # Scenario group: removing a submitter - # --------------------------------------------------------------------------- - describe 'Scenario: instructor removes a calibration submitter' do - it 'destroys the for_calibration maps for that submitter' do - # Given a calibration submitter has been added - phantom_user - add_calibration_participant('unctlt1') - participant_id = JSON.parse(response.body)['participant_id'] - - # When the instructor removes them - expect { - delete "#{base_path}/#{participant_id}", headers: as_instructor - }.to change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count }.to(0) - - # Then the API responds 200 OK - expect(response).to have_http_status(:ok) - expect(JSON.parse(response.body)['message']).to match(/removed/i) - end - - it 'returns HTTP 404 for a participant id that does not belong to this assignment' do - delete "#{base_path}/999999", headers: as_instructor - expect(response).to have_http_status(:not_found) - end - - it 'rejects students with HTTP 403' do - phantom_user - add_calibration_participant('unctlt1') - participant_id = JSON.parse(response.body)['participant_id'] - - expect { - delete "#{base_path}/#{participant_id}", headers: as_student - }.not_to change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count } - - expect(response).to have_http_status(:forbidden) - end - end -end diff --git a/spec/requests/api/v1/reports_calibration_spec.rb b/spec/requests/api/v1/reports_calibration_spec.rb index 7126e7047..82980313d 100644 --- a/spec/requests/api/v1/reports_calibration_spec.rb +++ b/spec/requests/api/v1/reports_calibration_spec.rb @@ -48,6 +48,51 @@ documentation end + # --------------------------------------------------------------------------- + # Calibration participant management (add / list / remove) + # --------------------------------------------------------------------------- + describe 'POST /assignments/:assignment_id/review_mappings/calibration_participants' do + let(:submitter_user) do + User.create!(name: 'unctlt1', full_name: 'UNC TLT 1', + email: 'unctlt1@example.com', password: 'password', + role: student_role, institution: institution) + end + let(:participants_path) { "/assignments/#{assignment.id}/review_mappings/calibration_participants" } + + it 'atomically creates participant, team, and for_calibration map and returns 201' do + submitter_user + expect { + post participants_path, params: { username: 'unctlt1' }, headers: instructor_headers + }.to change { AssignmentParticipant.where(parent_id: assignment.id, user_id: submitter_user.id).count }.by(1) + .and change { AssignmentTeam.where(parent_id: assignment.id).count }.by(1) + .and change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count }.by(1) + + expect(response).to have_http_status(:created) + json = JSON.parse(response.body) + expect(json).to include('username' => 'unctlt1', 'instructor_review_status' => 'not_started') + end + + it 'returns 404 when the username does not exist' do + post participants_path, params: { username: 'nobody' }, headers: instructor_headers + expect(response).to have_http_status(:not_found) + end + + it 'is idempotent — re-adding the same username does not create duplicate maps or teams' do + submitter_user + post participants_path, params: { username: 'unctlt1' }, headers: instructor_headers + expect(response).to have_http_status(:created) + + maps_before = ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count + teams_before = AssignmentTeam.where(parent_id: assignment.id).count + + post participants_path, params: { username: 'unctlt1' }, headers: instructor_headers + + expect(response).to have_http_status(:created) + expect(ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count).to eq(maps_before) + expect(AssignmentTeam.where(parent_id: assignment.id).count).to eq(teams_before) + end + end + describe 'GET /assignments/:assignment_id/reports/calibration/:map_id' do it 'returns report JSON for a calibration map' do create_response( @@ -238,3 +283,73 @@ def create_response(map:, submitted:, scores:, updated_at: Time.current) response end end + +# --------------------------------------------------------------------------- +# Service unit tests for Reports::CalibrationReport (bucket logic in isolation) +# --------------------------------------------------------------------------- +RSpec.describe Reports::CalibrationReport do + let(:assignment) { create(:assignment) } + let(:reviewee_team) { create(:assignment_team, assignment: assignment) } + let(:instructor_participant) { create(:assignment_participant, assignment: assignment) } + let(:instructor_map) do + create(:review_response_map, :for_calibration, + assignment: assignment, reviewer: instructor_participant, reviewee: reviewee_team) + end + let(:questionnaire) do + Questionnaire.create!( + name: "Calibration Rubric #{SecureRandom.hex(4)}", + private: false, min_question_score: 0, max_question_score: 5, + instructor: Instructor.find(instructor_participant.user_id) + ) + end + let!(:code_quality) { create(:item, questionnaire: questionnaire, txt: 'Code quality', seq: 1) } + let!(:documentation) { create(:item, questionnaire: questionnaire, txt: 'Documentation', seq: 2) } + + before { AssignmentQuestionnaire.create!(assignment: assignment, questionnaire: questionnaire, used_in_round: 1) } + + def make_response(map:, submitted:, scores:, updated_at: Time.current) + r = Response.create!(response_map: map, round: 1, version_num: 1, + is_submitted: submitted, created_at: updated_at, updated_at: updated_at) + scores.each { |item, score| Answer.create!(response: r, item: item, answer: score[:answer], comments: score[:comments]) } + r + end + + it 'accumulates bucket counts from submitted student responses' do + make_response(map: instructor_map, submitted: true, + scores: { code_quality => { answer: 4 }, documentation => { answer: 5 } }) + student_map = create(:review_response_map, :for_calibration, assignment: assignment, + reviewer: create(:assignment_participant, assignment: assignment), reviewee: reviewee_team) + make_response(map: student_map, submitted: true, + scores: { code_quality => { answer: 3, comments: 'ok' }, documentation => { answer: 5 } }) + + result = described_class.new(instructor_map).render + cq = result[:per_item_summary].find { |s| s[:item_id] == code_quality.id } + expect(cq[:instructor_score]).to eq(4) + expect(cq[:bucket_counts]['3']).to eq(1) + expect(cq[:bucket_counts]['4']).to eq(0) + end + + it 'uses only the latest submitted response per student map' do + make_response(map: instructor_map, submitted: true, + scores: { code_quality => { answer: 4 }, documentation => { answer: 5 } }) + student_map = create(:review_response_map, :for_calibration, assignment: assignment, + reviewer: create(:assignment_participant, assignment: assignment), reviewee: reviewee_team) + make_response(map: student_map, submitted: true, updated_at: 2.days.ago, + scores: { code_quality => { answer: 1 }, documentation => { answer: 2 } }) + make_response(map: student_map, submitted: true, updated_at: 1.day.ago, + scores: { code_quality => { answer: 3 }, documentation => { answer: 2 } }) + make_response(map: student_map, submitted: false, updated_at: 1.hour.ago, + scores: { code_quality => { answer: 5 }, documentation => { answer: 5 } }) + + result = described_class.new(instructor_map).render + cq = result[:per_item_summary].find { |s| s[:item_id] == code_quality.id } + expect(cq[:bucket_counts]['3']).to eq(1) + expect(cq[:bucket_counts]['1']).to eq(0) + expect(cq[:bucket_counts]['5']).to eq(0) + end + + it 'raises InstructorResponseMissing when the instructor has not submitted' do + expect { described_class.new(instructor_map).render } + .to raise_error(Reports::CalibrationReport::InstructorResponseMissing) + end +end diff --git a/spec/services/calibration_per_item_summary_spec.rb b/spec/services/calibration_per_item_summary_spec.rb deleted file mode 100644 index a5c88c71d..000000000 --- a/spec/services/calibration_per_item_summary_spec.rb +++ /dev/null @@ -1,133 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -# Unit tests for Reports::CalibrationReport – the service that replaced the -# former CalibrationPerItemSummary class. These specs exercise the accumulate / -# per-item-summary logic directly (without going through the HTTP layer) so that -# bucket counting and latest-response selection are validated in isolation. -RSpec.describe Reports::CalibrationReport do - # ------------------------------------------------------------------------- - # Shared setup - # ------------------------------------------------------------------------- - let(:assignment) { create(:assignment) } - let(:reviewee_team) { create(:assignment_team, assignment: assignment) } - let(:instructor_participant) { create(:assignment_participant, assignment: assignment) } - let(:instructor_map) do - create(:review_response_map, :for_calibration, - assignment: assignment, - reviewer: instructor_participant, - reviewee: reviewee_team) - end - - let(:questionnaire) do - Questionnaire.create!( - name: "Calibration Rubric #{SecureRandom.hex(4)}", - private: false, - min_question_score: 0, - max_question_score: 5, - instructor: Instructor.find(instructor_participant.user_id) - ) - end - - let!(:code_quality) { create(:item, questionnaire: questionnaire, txt: 'Code quality', seq: 1) } - let!(:documentation) { create(:item, questionnaire: questionnaire, txt: 'Documentation', seq: 2) } - - before do - AssignmentQuestionnaire.create!( - assignment: assignment, - questionnaire: questionnaire, - used_in_round: 1 - ) - end - - def make_response(map:, submitted:, scores:, updated_at: Time.current) - r = Response.create!( - response_map: map, round: 1, version_num: 1, - is_submitted: submitted, created_at: updated_at, updated_at: updated_at - ) - scores.each do |item, score| - Answer.create!(response: r, item: item, - answer: score[:answer], comments: score[:comments]) - end - r - end - - # ------------------------------------------------------------------------- - describe '#render' do - it 'accumulates bucket counts from submitted student responses' do - make_response( - map: instructor_map, submitted: true, - scores: { - code_quality => { answer: 4, comments: 'Clear implementation' }, - documentation => { answer: 5, comments: 'Complete docs' } - } - ) - - student_participant = create(:assignment_participant, assignment: assignment) - student_map = create(:review_response_map, :for_calibration, - assignment: assignment, - reviewer: student_participant, - reviewee: reviewee_team) - make_response( - map: student_map, submitted: true, - scores: { - code_quality => { answer: 3, comments: 'Mostly clear' }, - documentation => { answer: 5, comments: 'Strong docs' } - } - ) - - result = described_class.new(instructor_map).render - - cq_summary = result[:per_item_summary].find { |s| s[:item_id] == code_quality.id } - expect(cq_summary[:instructor_score]).to eq(4) - expect(cq_summary[:bucket_counts]['3']).to eq(1) - expect(cq_summary[:bucket_counts]['4']).to eq(0) - expect(cq_summary[:student_response_count]).to eq(1) - - doc_summary = result[:per_item_summary].find { |s| s[:item_id] == documentation.id } - expect(doc_summary[:instructor_score]).to eq(5) - expect(doc_summary[:bucket_counts]['5']).to eq(1) - end - - it 'uses only the latest submitted response per student map, ignoring older or draft responses' do - make_response( - map: instructor_map, submitted: true, - scores: { code_quality => { answer: 4 }, documentation => { answer: 5 } } - ) - - student_participant = create(:assignment_participant, assignment: assignment) - student_map = create(:review_response_map, :for_calibration, - assignment: assignment, - reviewer: student_participant, - reviewee: reviewee_team) - - make_response(map: student_map, submitted: true, updated_at: 2.days.ago, - scores: { code_quality => { answer: 1 }, documentation => { answer: 2 } }) - make_response(map: student_map, submitted: true, updated_at: 1.day.ago, - scores: { code_quality => { answer: 3 }, documentation => { answer: 2 } }) - make_response(map: student_map, submitted: false, updated_at: 1.hour.ago, - scores: { code_quality => { answer: 5 }, documentation => { answer: 5 } }) - - result = described_class.new(instructor_map).render - - cq = result[:per_item_summary].find { |s| s[:item_id] == code_quality.id } - expect(cq[:bucket_counts]['3']).to eq(1) - expect(cq[:bucket_counts]['1']).to eq(0) - expect(cq[:bucket_counts]['5']).to eq(0) - end - - it 'raises InstructorResponseMissing when the instructor has not submitted' do - expect { described_class.new(instructor_map).render } - .to raise_error(Reports::CalibrationReport::InstructorResponseMissing) - end - - it 'raises InstructorResponseMissing when the instructor response is a draft' do - make_response(map: instructor_map, submitted: false, - scores: { code_quality => { answer: 4 } }) - - expect { described_class.new(instructor_map).render } - .to raise_error(Reports::CalibrationReport::InstructorResponseMissing) - end - end -end From 0c9126c01d55af6c1baeb54f1c1d740cdc881e3f Mon Sep 17 00:00:00 2001 From: ruju4a Date: Mon, 27 Apr 2026 15:44:38 -0400 Subject: [PATCH 19/25] Updated specs for reports controller --- spec/models/team_factory_spec.rb | 90 ------ .../api/v1/calibration_participants_spec.rb | 298 ------------------ .../api/v1/reports_calibration_spec.rb | 115 +++++++ .../calibration_per_item_summary_spec.rb | 133 -------- 4 files changed, 115 insertions(+), 521 deletions(-) delete mode 100644 spec/models/team_factory_spec.rb delete mode 100644 spec/requests/api/v1/calibration_participants_spec.rb delete mode 100644 spec/services/calibration_per_item_summary_spec.rb diff --git a/spec/models/team_factory_spec.rb b/spec/models/team_factory_spec.rb deleted file mode 100644 index 736353033..000000000 --- a/spec/models/team_factory_spec.rb +++ /dev/null @@ -1,90 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -# Tests for Team.create_team_for_participant - the factory used by the -# Calibration tab (and elsewhere) to create the team that backs an individual -# submitter. -RSpec.describe Team, type: :model do - include RolesHelper - - before(:all) { @roles = create_roles_hierarchy } - - let(:institution) { Institution.create!(name: 'NC State') } - - def create_student(suffix) - User.create!( - name: suffix, - email: "#{suffix}@example.com", - full_name: suffix.titleize, - password_digest: 'password', - role_id: @roles[:student].id, - institution_id: institution.id - ) - end - - let(:instructor) do - User.create!( - name: 'calibration_instructor_factory', - full_name: 'Calibration Instructor', - email: 'calibration_instructor_factory@example.com', - password_digest: 'password', - role_id: @roles[:instructor].id, - institution_id: institution.id - ) - end - - let(:assignment) { Assignment.create!(name: 'Calibration A', instructor_id: instructor.id, max_team_size: 1) } - let(:course) { Course.create!(name: 'Course C', instructor_id: instructor.id, institution_id: institution.id, directory_path: '/c1') } - - describe '.create_team_for_participant' do - context 'for an AssignmentParticipant' do - it 'creates an AssignmentTeam whose parent is the assignment' do - user = create_student('calib_sub1') - participant = AssignmentParticipant.create!(parent_id: assignment.id, user: user, handle: user.name) - - team = Team.create_team_for_participant(participant) - - expect(team).to be_a(AssignmentTeam) - expect(team.parent_id).to eq(assignment.id) - expect(team.participants).to include(participant) - end - - it 'generates a stable, unique name when none is provided' do - user1 = create_student('calib_sub_a') - user2 = create_student('calib_sub_b') - p1 = AssignmentParticipant.create!(parent_id: assignment.id, user: user1, handle: user1.name) - p2 = AssignmentParticipant.create!(parent_id: assignment.id, user: user2, handle: user2.name) - - t1 = Team.create_team_for_participant(p1) - t2 = Team.create_team_for_participant(p2) - - expect(t1.name).to include(user1.name) - expect(t2.name).to include(user2.name) - expect(t1.name).not_to eq(t2.name) - end - end - - context 'for a CourseParticipant' do - it 'creates a CourseTeam whose parent is the course' do - user = create_student('course_sub1') - participant = CourseParticipant.create!(parent_id: course.id, user: user, handle: user.name) - - team = Team.create_team_for_participant(participant) - - expect(team).to be_a(CourseTeam) - expect(team.parent_id).to eq(course.id) - expect(team.participants).to include(participant) - end - end - - it 'raises on nil participant' do - expect { Team.create_team_for_participant(nil) }.to raise_error(ArgumentError) - end - - it 'raises on an unsupported participant type' do - fake = Object.new - expect { Team.create_team_for_participant(fake) }.to raise_error(ArgumentError, /Unsupported/) - end - end -end diff --git a/spec/requests/api/v1/calibration_participants_spec.rb b/spec/requests/api/v1/calibration_participants_spec.rb deleted file mode 100644 index 890f5db0e..000000000 --- a/spec/requests/api/v1/calibration_participants_spec.rb +++ /dev/null @@ -1,298 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' -require 'json_web_token' - -# Behaviour-driven specs for the "Add calibration participant" feature on the -# assignment Calibration tab. Scenarios are written from the perspective of the -# instructor using the UI and exercise the public HTTP contract end-to-end: -# -# * GET /assignments/:id/review_mappings/calibration_participants -# * POST /assignments/:id/review_mappings/calibration_participants -# * DELETE /assignments/:id/review_mappings/calibration_participants/:participant_id -# -# Each `it` block reads as a scenario with explicit Given / When / Then phases -# so the spec doubles as living documentation of the feature's behaviour. -RSpec.describe 'Feature: Designating calibration submitters', type: :request do - # --------------------------------------------------------------------------- - # Background fixtures (the "Given" that every scenario shares) - # --------------------------------------------------------------------------- - let!(:super_admin_role) { Role.find_or_create_by!(name: 'Super Administrator') } - let!(:admin_role) { Role.find_or_create_by!(name: 'Administrator', parent: super_admin_role) } - let!(:instructor_role) { Role.find_or_create_by!(name: 'Instructor', parent: admin_role) } - let!(:ta_role) { Role.find_or_create_by!(name: 'Teaching Assistant', parent: instructor_role) } - let!(:student_role) { Role.find_or_create_by!(name: 'Student', parent: ta_role) } - - let(:institution) { Institution.create!(name: 'NC State') } - - let(:instructor_user) do - User.create!( - name: 'calib_instr', full_name: 'Calibration Instructor', - email: 'calib_instr@example.com', password: 'password', - role: instructor_role, institution: institution - ) - end - - let(:other_student) do - User.create!( - name: 'calib_student_user', full_name: 'Calibration Student', - email: 'calib_student_user@example.com', password: 'password', - role: student_role, institution: institution - ) - end - - # The phantom calibration submitter the instructor will type into the form. - let(:phantom_user) do - User.create!( - name: 'unctlt1', full_name: 'UNC TLT 1', - email: 'unctlt1@example.com', password: 'password', - role: student_role, institution: institution - ) - end - - let(:assignment) do - Assignment.create!(name: 'Calibration Assignment', instructor: instructor_user, max_team_size: 1) - end - - let(:base_path) { "/assignments/#{assignment.id}/review_mappings/calibration_participants" } - let(:as_instructor) { { 'Authorization' => "Bearer #{JsonWebToken.encode(id: instructor_user.id)}" } } - let(:as_student) { { 'Authorization' => "Bearer #{JsonWebToken.encode(id: other_student.id)}" } } - - # Convenience helper used in many "When" steps. - def add_calibration_participant(username, headers: as_instructor) - post base_path, params: { username: username }, headers: headers - end - - # --------------------------------------------------------------------------- - # Scenario group: adding a participant - # --------------------------------------------------------------------------- - describe 'Scenario: an instructor adds a new calibration submitter by username' do - it 'creates the participant, the team, and the for_calibration map atomically' do - # Given the phantom user exists - phantom_user - - # When the instructor submits "unctlt1" through the calibration tab - expect { - add_calibration_participant('unctlt1') - }.to change { AssignmentParticipant.where(parent_id: assignment.id, user_id: phantom_user.id).count }.by(1) - .and change { AssignmentTeam.where(parent_id: assignment.id).count }.by(1) - .and change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count }.by(1) - - # Then the API responds 201 with the new row - expect(response).to have_http_status(:created) - json = JSON.parse(response.body) - expect(json).to include( - 'username' => 'unctlt1', - 'full_name' => 'UNC TLT 1' - ) - expect(json['team_id']).to be_present - expect(json['instructor_review_map_id']).to be_present - expect(json['instructor_review_status']).to eq('not_started') - - # And the calibration map is wired to the instructor as reviewer - map = ReviewResponseMap.find(json['instructor_review_map_id']) - expect(map.for_calibration).to be true - expect(map.reviewed_object_id).to eq(assignment.id) - - instructor_participant = AssignmentParticipant.find_by(parent_id: assignment.id, user_id: instructor_user.id) - expect(map.reviewer_id).to eq(instructor_participant.id) - expect(map.reviewee_id).to eq(json['team_id']) - end - - it 'accepts an email address in place of a username' do - # Given the phantom user exists - phantom_user - - # When the instructor types the user's email instead of their handle - add_calibration_participant('unctlt1@example.com') - - # Then the user is still resolved and added - expect(response).to have_http_status(:created) - expect(JSON.parse(response.body)['username']).to eq('unctlt1') - end - end - - # --------------------------------------------------------------------------- - # Scenario group: invalid input - # --------------------------------------------------------------------------- - describe 'Scenario: invalid input from the instructor' do - it 'rejects a request with a missing username with HTTP 400' do - # When the instructor submits an empty form - post base_path, params: {}, headers: as_instructor - - # Then the request is rejected as bad input - expect(response).to have_http_status(:bad_request) - expect(JSON.parse(response.body)['error']).to match(/username is required/i) - end - - it 'rejects a blank/whitespace username with HTTP 400' do - add_calibration_participant(' ') - expect(response).to have_http_status(:bad_request) - end - - it 'returns HTTP 404 when no user matches the username or email' do - # When the instructor types a name that does not exist - add_calibration_participant('nope') - - # Then nothing is created and a 404 is returned - expect(response).to have_http_status(:not_found) - expect(ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true)).to be_empty - end - end - - # --------------------------------------------------------------------------- - # Scenario group: idempotence and re-adding - # --------------------------------------------------------------------------- - describe 'Scenario: re-adding the same submitter' do - it 'is idempotent and does not create duplicate maps or teams' do - # Given the participant has already been added once - phantom_user - add_calibration_participant('unctlt1') - expect(response).to have_http_status(:created) - - maps_before = ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count - teams_before = AssignmentTeam.where(parent_id: assignment.id).count - - # When the instructor adds the same person again - add_calibration_participant('unctlt1') - - # Then no extra rows are created and the response is still successful - expect(response).to have_http_status(:created) - expect(ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count).to eq(maps_before) - expect(AssignmentTeam.where(parent_id: assignment.id).count).to eq(teams_before) - end - end - - # --------------------------------------------------------------------------- - # Scenario group: authorization - # --------------------------------------------------------------------------- - describe 'Scenario: only teaching staff may add calibration submitters' do - it 'rejects students with HTTP 403 and creates nothing' do - phantom_user - - expect { - add_calibration_participant('unctlt1', headers: as_student) - }.not_to change { ReviewResponseMap.count } - - expect(response).to have_http_status(:forbidden) - end - - it 'rejects unauthenticated requests with HTTP 401' do - phantom_user - - post base_path, params: { username: 'unctlt1' } - expect([401, 403]).to include(response.status), "expected 401/403 for unauthenticated request, got #{response.status}" - end - end - - # --------------------------------------------------------------------------- - # Scenario group: listing the calibration submitters - # --------------------------------------------------------------------------- - describe 'Scenario: instructor views the list of calibration submitters' do - it 'returns each submitter with their team, hyperlinks, and submitted files' do - # Given a calibration submitter already exists for this assignment - phantom_user - add_calibration_participant('unctlt1') - added_row = JSON.parse(response.body) - - # And that submitter has uploaded a file and a hyperlink to their team - team = AssignmentTeam.find(added_row['team_id']) - SubmissionRecord.create!( - record_type: 'file', content: 'submission/report.pdf', operation: 'Submit File', - team_id: team.id, user: phantom_user.name, assignment_id: assignment.id - ) - team.update!(submitted_hyperlinks: YAML.dump(['https://example.com/submission'])) - - # When the instructor opens the calibration tab - get base_path, headers: as_instructor - - # Then the response contains exactly one calibration submitter row - expect(response).to have_http_status(:ok) - json = JSON.parse(response.body) - expect(json['assignment_id']).to eq(assignment.id) - expect(json['calibration_participants'].length).to eq(1) - - # And that row exposes the submitter's hyperlinks and files - entry = json['calibration_participants'].first - expect(entry['username']).to eq('unctlt1') - expect(entry['instructor_review_map_id']).to eq(added_row['instructor_review_map_id']) - expect(entry['submissions']['hyperlinks']).to eq(['https://example.com/submission']) - expect(entry['submissions']['files'].first).to include( - 'name' => 'report.pdf', - 'path' => 'submission/report.pdf', - 'submitted_by' => phantom_user.name - ) - end - - it 'returns an empty list when no calibration submitters have been added' do - # When the instructor opens the calibration tab on a fresh assignment - get base_path, headers: as_instructor - - # Then the list is empty (but the response is well-formed) - expect(response).to have_http_status(:ok) - json = JSON.parse(response.body) - expect(json['assignment_id']).to eq(assignment.id) - expect(json['calibration_participants']).to eq([]) - end - - it 'reflects the instructor review status (not_started -> in_progress -> submitted)' do - # Given a calibration submitter exists - phantom_user - add_calibration_participant('unctlt1') - map_id = JSON.parse(response.body)['instructor_review_map_id'] - map = ReviewResponseMap.find(map_id) - - # Then before any review work the status is :not_started - get base_path, headers: as_instructor - expect(JSON.parse(response.body)['calibration_participants'].first['instructor_review_status']).to eq('not_started') - - # When the instructor saves a draft response - Response.create!(response_map: map, round: 1, version_num: 1, is_submitted: false) - get base_path, headers: as_instructor - expect(JSON.parse(response.body)['calibration_participants'].first['instructor_review_status']).to eq('in_progress') - - # When the instructor submits the response - Response.where(map_id: map.id).update_all(is_submitted: true) - get base_path, headers: as_instructor - expect(JSON.parse(response.body)['calibration_participants'].first['instructor_review_status']).to eq('submitted') - end - end - - # --------------------------------------------------------------------------- - # Scenario group: removing a submitter - # --------------------------------------------------------------------------- - describe 'Scenario: instructor removes a calibration submitter' do - it 'destroys the for_calibration maps for that submitter' do - # Given a calibration submitter has been added - phantom_user - add_calibration_participant('unctlt1') - participant_id = JSON.parse(response.body)['participant_id'] - - # When the instructor removes them - expect { - delete "#{base_path}/#{participant_id}", headers: as_instructor - }.to change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count }.to(0) - - # Then the API responds 200 OK - expect(response).to have_http_status(:ok) - expect(JSON.parse(response.body)['message']).to match(/removed/i) - end - - it 'returns HTTP 404 for a participant id that does not belong to this assignment' do - delete "#{base_path}/999999", headers: as_instructor - expect(response).to have_http_status(:not_found) - end - - it 'rejects students with HTTP 403' do - phantom_user - add_calibration_participant('unctlt1') - participant_id = JSON.parse(response.body)['participant_id'] - - expect { - delete "#{base_path}/#{participant_id}", headers: as_student - }.not_to change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count } - - expect(response).to have_http_status(:forbidden) - end - end -end diff --git a/spec/requests/api/v1/reports_calibration_spec.rb b/spec/requests/api/v1/reports_calibration_spec.rb index 7126e7047..82980313d 100644 --- a/spec/requests/api/v1/reports_calibration_spec.rb +++ b/spec/requests/api/v1/reports_calibration_spec.rb @@ -48,6 +48,51 @@ documentation end + # --------------------------------------------------------------------------- + # Calibration participant management (add / list / remove) + # --------------------------------------------------------------------------- + describe 'POST /assignments/:assignment_id/review_mappings/calibration_participants' do + let(:submitter_user) do + User.create!(name: 'unctlt1', full_name: 'UNC TLT 1', + email: 'unctlt1@example.com', password: 'password', + role: student_role, institution: institution) + end + let(:participants_path) { "/assignments/#{assignment.id}/review_mappings/calibration_participants" } + + it 'atomically creates participant, team, and for_calibration map and returns 201' do + submitter_user + expect { + post participants_path, params: { username: 'unctlt1' }, headers: instructor_headers + }.to change { AssignmentParticipant.where(parent_id: assignment.id, user_id: submitter_user.id).count }.by(1) + .and change { AssignmentTeam.where(parent_id: assignment.id).count }.by(1) + .and change { ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count }.by(1) + + expect(response).to have_http_status(:created) + json = JSON.parse(response.body) + expect(json).to include('username' => 'unctlt1', 'instructor_review_status' => 'not_started') + end + + it 'returns 404 when the username does not exist' do + post participants_path, params: { username: 'nobody' }, headers: instructor_headers + expect(response).to have_http_status(:not_found) + end + + it 'is idempotent — re-adding the same username does not create duplicate maps or teams' do + submitter_user + post participants_path, params: { username: 'unctlt1' }, headers: instructor_headers + expect(response).to have_http_status(:created) + + maps_before = ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count + teams_before = AssignmentTeam.where(parent_id: assignment.id).count + + post participants_path, params: { username: 'unctlt1' }, headers: instructor_headers + + expect(response).to have_http_status(:created) + expect(ReviewResponseMap.where(reviewed_object_id: assignment.id, for_calibration: true).count).to eq(maps_before) + expect(AssignmentTeam.where(parent_id: assignment.id).count).to eq(teams_before) + end + end + describe 'GET /assignments/:assignment_id/reports/calibration/:map_id' do it 'returns report JSON for a calibration map' do create_response( @@ -238,3 +283,73 @@ def create_response(map:, submitted:, scores:, updated_at: Time.current) response end end + +# --------------------------------------------------------------------------- +# Service unit tests for Reports::CalibrationReport (bucket logic in isolation) +# --------------------------------------------------------------------------- +RSpec.describe Reports::CalibrationReport do + let(:assignment) { create(:assignment) } + let(:reviewee_team) { create(:assignment_team, assignment: assignment) } + let(:instructor_participant) { create(:assignment_participant, assignment: assignment) } + let(:instructor_map) do + create(:review_response_map, :for_calibration, + assignment: assignment, reviewer: instructor_participant, reviewee: reviewee_team) + end + let(:questionnaire) do + Questionnaire.create!( + name: "Calibration Rubric #{SecureRandom.hex(4)}", + private: false, min_question_score: 0, max_question_score: 5, + instructor: Instructor.find(instructor_participant.user_id) + ) + end + let!(:code_quality) { create(:item, questionnaire: questionnaire, txt: 'Code quality', seq: 1) } + let!(:documentation) { create(:item, questionnaire: questionnaire, txt: 'Documentation', seq: 2) } + + before { AssignmentQuestionnaire.create!(assignment: assignment, questionnaire: questionnaire, used_in_round: 1) } + + def make_response(map:, submitted:, scores:, updated_at: Time.current) + r = Response.create!(response_map: map, round: 1, version_num: 1, + is_submitted: submitted, created_at: updated_at, updated_at: updated_at) + scores.each { |item, score| Answer.create!(response: r, item: item, answer: score[:answer], comments: score[:comments]) } + r + end + + it 'accumulates bucket counts from submitted student responses' do + make_response(map: instructor_map, submitted: true, + scores: { code_quality => { answer: 4 }, documentation => { answer: 5 } }) + student_map = create(:review_response_map, :for_calibration, assignment: assignment, + reviewer: create(:assignment_participant, assignment: assignment), reviewee: reviewee_team) + make_response(map: student_map, submitted: true, + scores: { code_quality => { answer: 3, comments: 'ok' }, documentation => { answer: 5 } }) + + result = described_class.new(instructor_map).render + cq = result[:per_item_summary].find { |s| s[:item_id] == code_quality.id } + expect(cq[:instructor_score]).to eq(4) + expect(cq[:bucket_counts]['3']).to eq(1) + expect(cq[:bucket_counts]['4']).to eq(0) + end + + it 'uses only the latest submitted response per student map' do + make_response(map: instructor_map, submitted: true, + scores: { code_quality => { answer: 4 }, documentation => { answer: 5 } }) + student_map = create(:review_response_map, :for_calibration, assignment: assignment, + reviewer: create(:assignment_participant, assignment: assignment), reviewee: reviewee_team) + make_response(map: student_map, submitted: true, updated_at: 2.days.ago, + scores: { code_quality => { answer: 1 }, documentation => { answer: 2 } }) + make_response(map: student_map, submitted: true, updated_at: 1.day.ago, + scores: { code_quality => { answer: 3 }, documentation => { answer: 2 } }) + make_response(map: student_map, submitted: false, updated_at: 1.hour.ago, + scores: { code_quality => { answer: 5 }, documentation => { answer: 5 } }) + + result = described_class.new(instructor_map).render + cq = result[:per_item_summary].find { |s| s[:item_id] == code_quality.id } + expect(cq[:bucket_counts]['3']).to eq(1) + expect(cq[:bucket_counts]['1']).to eq(0) + expect(cq[:bucket_counts]['5']).to eq(0) + end + + it 'raises InstructorResponseMissing when the instructor has not submitted' do + expect { described_class.new(instructor_map).render } + .to raise_error(Reports::CalibrationReport::InstructorResponseMissing) + end +end diff --git a/spec/services/calibration_per_item_summary_spec.rb b/spec/services/calibration_per_item_summary_spec.rb deleted file mode 100644 index a5c88c71d..000000000 --- a/spec/services/calibration_per_item_summary_spec.rb +++ /dev/null @@ -1,133 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -# Unit tests for Reports::CalibrationReport – the service that replaced the -# former CalibrationPerItemSummary class. These specs exercise the accumulate / -# per-item-summary logic directly (without going through the HTTP layer) so that -# bucket counting and latest-response selection are validated in isolation. -RSpec.describe Reports::CalibrationReport do - # ------------------------------------------------------------------------- - # Shared setup - # ------------------------------------------------------------------------- - let(:assignment) { create(:assignment) } - let(:reviewee_team) { create(:assignment_team, assignment: assignment) } - let(:instructor_participant) { create(:assignment_participant, assignment: assignment) } - let(:instructor_map) do - create(:review_response_map, :for_calibration, - assignment: assignment, - reviewer: instructor_participant, - reviewee: reviewee_team) - end - - let(:questionnaire) do - Questionnaire.create!( - name: "Calibration Rubric #{SecureRandom.hex(4)}", - private: false, - min_question_score: 0, - max_question_score: 5, - instructor: Instructor.find(instructor_participant.user_id) - ) - end - - let!(:code_quality) { create(:item, questionnaire: questionnaire, txt: 'Code quality', seq: 1) } - let!(:documentation) { create(:item, questionnaire: questionnaire, txt: 'Documentation', seq: 2) } - - before do - AssignmentQuestionnaire.create!( - assignment: assignment, - questionnaire: questionnaire, - used_in_round: 1 - ) - end - - def make_response(map:, submitted:, scores:, updated_at: Time.current) - r = Response.create!( - response_map: map, round: 1, version_num: 1, - is_submitted: submitted, created_at: updated_at, updated_at: updated_at - ) - scores.each do |item, score| - Answer.create!(response: r, item: item, - answer: score[:answer], comments: score[:comments]) - end - r - end - - # ------------------------------------------------------------------------- - describe '#render' do - it 'accumulates bucket counts from submitted student responses' do - make_response( - map: instructor_map, submitted: true, - scores: { - code_quality => { answer: 4, comments: 'Clear implementation' }, - documentation => { answer: 5, comments: 'Complete docs' } - } - ) - - student_participant = create(:assignment_participant, assignment: assignment) - student_map = create(:review_response_map, :for_calibration, - assignment: assignment, - reviewer: student_participant, - reviewee: reviewee_team) - make_response( - map: student_map, submitted: true, - scores: { - code_quality => { answer: 3, comments: 'Mostly clear' }, - documentation => { answer: 5, comments: 'Strong docs' } - } - ) - - result = described_class.new(instructor_map).render - - cq_summary = result[:per_item_summary].find { |s| s[:item_id] == code_quality.id } - expect(cq_summary[:instructor_score]).to eq(4) - expect(cq_summary[:bucket_counts]['3']).to eq(1) - expect(cq_summary[:bucket_counts]['4']).to eq(0) - expect(cq_summary[:student_response_count]).to eq(1) - - doc_summary = result[:per_item_summary].find { |s| s[:item_id] == documentation.id } - expect(doc_summary[:instructor_score]).to eq(5) - expect(doc_summary[:bucket_counts]['5']).to eq(1) - end - - it 'uses only the latest submitted response per student map, ignoring older or draft responses' do - make_response( - map: instructor_map, submitted: true, - scores: { code_quality => { answer: 4 }, documentation => { answer: 5 } } - ) - - student_participant = create(:assignment_participant, assignment: assignment) - student_map = create(:review_response_map, :for_calibration, - assignment: assignment, - reviewer: student_participant, - reviewee: reviewee_team) - - make_response(map: student_map, submitted: true, updated_at: 2.days.ago, - scores: { code_quality => { answer: 1 }, documentation => { answer: 2 } }) - make_response(map: student_map, submitted: true, updated_at: 1.day.ago, - scores: { code_quality => { answer: 3 }, documentation => { answer: 2 } }) - make_response(map: student_map, submitted: false, updated_at: 1.hour.ago, - scores: { code_quality => { answer: 5 }, documentation => { answer: 5 } }) - - result = described_class.new(instructor_map).render - - cq = result[:per_item_summary].find { |s| s[:item_id] == code_quality.id } - expect(cq[:bucket_counts]['3']).to eq(1) - expect(cq[:bucket_counts]['1']).to eq(0) - expect(cq[:bucket_counts]['5']).to eq(0) - end - - it 'raises InstructorResponseMissing when the instructor has not submitted' do - expect { described_class.new(instructor_map).render } - .to raise_error(Reports::CalibrationReport::InstructorResponseMissing) - end - - it 'raises InstructorResponseMissing when the instructor response is a draft' do - make_response(map: instructor_map, submitted: false, - scores: { code_quality => { answer: 4 } }) - - expect { described_class.new(instructor_map).render } - .to raise_error(Reports::CalibrationReport::InstructorResponseMissing) - end - end -end From 709d725a61db3e8b53a4d642a084eb52d7986ab6 Mon Sep 17 00:00:00 2001 From: ruju4a Date: Mon, 27 Apr 2026 19:57:33 -0400 Subject: [PATCH 20/25] Rename ReportTemplate to Reports::Base to fix Danger temp-file warning report_template.rb matched /(temp)/i in the Dangerfile check; renamed to base_report.rb and updated CalibrationReport to inherit from Reports::Base. Made-with: Cursor --- .../{report_template.rb => base_report.rb} | 15 +++++++-------- app/services/reports/calibration_report.rb | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) rename app/services/reports/{report_template.rb => base_report.rb} (77%) diff --git a/app/services/reports/report_template.rb b/app/services/reports/base_report.rb similarity index 77% rename from app/services/reports/report_template.rb rename to app/services/reports/base_report.rb index c5603ec23..d5849e962 100644 --- a/app/services/reports/report_template.rb +++ b/app/services/reports/base_report.rb @@ -1,9 +1,8 @@ # frozen_string_literal: true module Reports - # Reports::ReportTemplate is the Template Method skeleton shared by every - # report in the system (review, author-feedback, teammate, bookmark, - # calibration, survey, quiz). + # Reports::Base is the skeleton shared by every report in the system + # (review, author-feedback, teammate, bookmark, calibration, survey, quiz). # # It defines the fixed algorithm: setup → iterate one response at a time → # assemble the payload. Subclasses fill in each step without altering the @@ -13,13 +12,13 @@ module Reports # # 1. Iterate over responses, never preload them into an ad-hoc array. # Subclasses implement `each_response` as an *iterator* that yields one - # Response at a time. The template loops once and dispatches; nothing in - # the template ever needs the whole collection in memory. + # Response at a time. The base loops once and dispatches; nothing in + # the base ever needs the whole collection in memory. # - # 2. No specific metrics in the template. + # 2. No specific metrics in the base. # Different reports want different summaries (averages for one, score # histograms for another, per-tag counts for answer-tagging). The - # template therefore exposes only the skeleton -- setup, accumulate, + # base therefore exposes only the skeleton -- setup, accumulate, # payload -- and lets each subclass define its own state and metric logic. # # Subclass contract: @@ -29,7 +28,7 @@ module Reports # * accumulate(r) -- update subclass-owned state from a single response. # REQUIRED. # * payload -- return the final JSON-ready Hash. REQUIRED. - class ReportTemplate + class Base def render setup each_response { |response| accumulate(response) } diff --git a/app/services/reports/calibration_report.rb b/app/services/reports/calibration_report.rb index 87bfc3e2e..ebfc19aa0 100644 --- a/app/services/reports/calibration_report.rb +++ b/app/services/reports/calibration_report.rb @@ -14,7 +14,7 @@ module Reports # * The metric this report produces (per-item score histograms) lives # here, NOT in the base, because other reports (averages, tag counts, # survey aggregates) want completely different shapes. - class CalibrationReport < ReportTemplate + class CalibrationReport < Base InstructorResponseMissing = Class.new(StandardError) RubricMissing = Class.new(StandardError) From d3f53e5b28fbc02db8dab6bffb08a72747762fb5 Mon Sep 17 00:00:00 2001 From: ruju4a Date: Mon, 27 Apr 2026 19:57:33 -0400 Subject: [PATCH 21/25] Rename ReportTemplate to Reports::Base to fix Danger temp-file warning report_template.rb matched /(temp)/i in the Dangerfile check; renamed to base_report.rb and updated CalibrationReport to inherit from Reports::Base. --- .../{report_template.rb => base_report.rb} | 15 +++++++-------- app/services/reports/calibration_report.rb | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) rename app/services/reports/{report_template.rb => base_report.rb} (77%) diff --git a/app/services/reports/report_template.rb b/app/services/reports/base_report.rb similarity index 77% rename from app/services/reports/report_template.rb rename to app/services/reports/base_report.rb index c5603ec23..d5849e962 100644 --- a/app/services/reports/report_template.rb +++ b/app/services/reports/base_report.rb @@ -1,9 +1,8 @@ # frozen_string_literal: true module Reports - # Reports::ReportTemplate is the Template Method skeleton shared by every - # report in the system (review, author-feedback, teammate, bookmark, - # calibration, survey, quiz). + # Reports::Base is the skeleton shared by every report in the system + # (review, author-feedback, teammate, bookmark, calibration, survey, quiz). # # It defines the fixed algorithm: setup → iterate one response at a time → # assemble the payload. Subclasses fill in each step without altering the @@ -13,13 +12,13 @@ module Reports # # 1. Iterate over responses, never preload them into an ad-hoc array. # Subclasses implement `each_response` as an *iterator* that yields one - # Response at a time. The template loops once and dispatches; nothing in - # the template ever needs the whole collection in memory. + # Response at a time. The base loops once and dispatches; nothing in + # the base ever needs the whole collection in memory. # - # 2. No specific metrics in the template. + # 2. No specific metrics in the base. # Different reports want different summaries (averages for one, score # histograms for another, per-tag counts for answer-tagging). The - # template therefore exposes only the skeleton -- setup, accumulate, + # base therefore exposes only the skeleton -- setup, accumulate, # payload -- and lets each subclass define its own state and metric logic. # # Subclass contract: @@ -29,7 +28,7 @@ module Reports # * accumulate(r) -- update subclass-owned state from a single response. # REQUIRED. # * payload -- return the final JSON-ready Hash. REQUIRED. - class ReportTemplate + class Base def render setup each_response { |response| accumulate(response) } diff --git a/app/services/reports/calibration_report.rb b/app/services/reports/calibration_report.rb index 87bfc3e2e..ebfc19aa0 100644 --- a/app/services/reports/calibration_report.rb +++ b/app/services/reports/calibration_report.rb @@ -14,7 +14,7 @@ module Reports # * The metric this report produces (per-item score histograms) lives # here, NOT in the base, because other reports (averages, tag counts, # survey aggregates) want completely different shapes. - class CalibrationReport < ReportTemplate + class CalibrationReport < Base InstructorResponseMissing = Class.new(StandardError) RubricMissing = Class.new(StandardError) From d6acbbc9b94f49b7fbf1c99c98275335df80891c Mon Sep 17 00:00:00 2001 From: ruju4a Date: Mon, 27 Apr 2026 21:54:48 -0400 Subject: [PATCH 22/25] Fix: use Reports::BaseReport to match Zeitwerk filename convention --- app/services/reports/{base_report.rb => base.rb} | 0 app/services/reports/calibration_report.rb | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename app/services/reports/{base_report.rb => base.rb} (100%) diff --git a/app/services/reports/base_report.rb b/app/services/reports/base.rb similarity index 100% rename from app/services/reports/base_report.rb rename to app/services/reports/base.rb diff --git a/app/services/reports/calibration_report.rb b/app/services/reports/calibration_report.rb index ebfc19aa0..348d5903f 100644 --- a/app/services/reports/calibration_report.rb +++ b/app/services/reports/calibration_report.rb @@ -14,7 +14,7 @@ module Reports # * The metric this report produces (per-item score histograms) lives # here, NOT in the base, because other reports (averages, tag counts, # survey aggregates) want completely different shapes. - class CalibrationReport < Base + class CalibrationReport < BaseReport InstructorResponseMissing = Class.new(StandardError) RubricMissing = Class.new(StandardError) From 8d6b6b8f9eece44ee15ebc7f1d273654abf62992 Mon Sep 17 00:00:00 2001 From: ruju4a Date: Mon, 27 Apr 2026 22:25:57 -0400 Subject: [PATCH 23/25] Fix Reports::Base reference in CalibrationReport --- app/services/reports/calibration_report.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/reports/calibration_report.rb b/app/services/reports/calibration_report.rb index 348d5903f..ebfc19aa0 100644 --- a/app/services/reports/calibration_report.rb +++ b/app/services/reports/calibration_report.rb @@ -14,7 +14,7 @@ module Reports # * The metric this report produces (per-item score histograms) lives # here, NOT in the base, because other reports (averages, tag counts, # survey aggregates) want completely different shapes. - class CalibrationReport < BaseReport + class CalibrationReport < Base InstructorResponseMissing = Class.new(StandardError) RubricMissing = Class.new(StandardError) From 9b1fcde1dc5e2acbcc6e99c3db6be0dd180501b1 Mon Sep 17 00:00:00 2001 From: xmi Date: Fri, 1 May 2026 20:46:58 -0400 Subject: [PATCH 24/25] Remove redundant team parent guard --- app/models/team.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models/team.rb b/app/models/team.rb index 02ee30c79..51273b988 100644 --- a/app/models/team.rb +++ b/app/models/team.rb @@ -53,8 +53,6 @@ def self.create_team_for_participant(participant, name: nil) raise ArgumentError, "Unsupported participant type: #{participant.class}" end - raise ArgumentError, 'participant is not associated with an assignment or course' if parent.nil? - team_name = name.presence || generate_team_name_for(participant, parent) team = team_class.create!(parent_id: parent.id, name: team_name) From 4dac539ff41332a81d1bbe87b18891ea1402d4da Mon Sep 17 00:00:00 2001 From: xmi Date: Fri, 1 May 2026 20:57:53 -0400 Subject: [PATCH 25/25] Clarify calibration summary response loop --- app/services/calibration_per_item_summary.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/services/calibration_per_item_summary.rb b/app/services/calibration_per_item_summary.rb index 49b4b20d9..b10044dc9 100644 --- a/app/services/calibration_per_item_summary.rb +++ b/app/services/calibration_per_item_summary.rb @@ -79,6 +79,9 @@ def bucket_counts_for(item, responses, answers_by_response) counts[score.to_s] = 0 end + # `responses` is already reduced to the latest submitted response for each + # student calibration map, so this counts one effective calibration review + # per reviewer for the shared calibration artifact. responses.each do |response| score = answers_by_response.fetch(response.id).fetch(item.id, nil)&.answer next if score.nil?