Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
8b40c0b
Add calibration flag to response maps
Apr 23, 2026
fcbf205
Use for_calibration in review mapping handler
Apr 23, 2026
36f41df
Add calibration per item summary service
Apr 23, 2026
552e5c6
Add calibration report endpoint
Apr 23, 2026
61f209c
Align calibration report responses with latest submitted review
Apr 23, 2026
d2795c3
Add calibration report demo data task
Apr 24, 2026
7f9a687
Keep calibration demo data at six student maps
Apr 24, 2026
9b041c7
added add participant changes
ruju4a Apr 24, 2026
79c5d35
Merge branch 'main' of https://github.com/ruju4a/reimplementation-bac…
ruju4a Apr 24, 2026
673b65b
integrated add calibration particiapant and view report changes
ruju4a Apr 25, 2026
d7c0fba
integrated add calibration particiapant and view report changes
ruju4a Apr 25, 2026
a154eed
Merge pull request #10 from ruju4a/rujuta
ruju4a Apr 25, 2026
a9f02bf
Merge pull request #10 from ruju4a/rujuta
ruju4a Apr 25, 2026
65ab354
Seed blank calibration demo comments
Apr 26, 2026
55c3979
Seed blank calibration demo comments
Apr 26, 2026
1a1f624
added mockdata files to gitignore
ruju4a Apr 26, 2026
7adee79
Merge branch 'main2' of https://github.com/ruju4a/reimplementation-ba…
ruju4a Apr 26, 2026
d83ca22
Merge branch 'main2' of https://github.com/ruju4a/reimplementation-ba…
ruju4a Apr 26, 2026
16567b2
Refactor calibration: move logic to models, add iterator-based report…
ruju4a Apr 27, 2026
18e2ffa
Refactor calibration: move logic to models, add iterator-based report…
ruju4a Apr 27, 2026
3e3210b
Apply feedback: expert principle fixes and comments
ruju4a Apr 27, 2026
a5aae2d
Apply feedback: expert principle fixes and comments
ruju4a Apr 27, 2026
4cbbdf9
Merge pull request #11 from ruju4a/rujuta
ruju4a Apr 27, 2026
32549ed
Updated specs for reports controller
ruju4a Apr 27, 2026
0c9126c
Updated specs for reports controller
ruju4a Apr 27, 2026
d131bf7
Merge pull request #12 from ruju4a/rujuta
ruju4a Apr 27, 2026
709d725
Rename ReportTemplate to Reports::Base to fix Danger temp-file warning
ruju4a Apr 27, 2026
d3f53e5
Rename ReportTemplate to Reports::Base to fix Danger temp-file warning
ruju4a Apr 27, 2026
537712e
Merge pull request #13 from ruju4a/rujuta
ruju4a Apr 27, 2026
d6acbbc
Fix: use Reports::BaseReport to match Zeitwerk filename convention
ruju4a Apr 28, 2026
8d6b6b8
Fix Reports::Base reference in CalibrationReport
ruju4a Apr 28, 2026
f5e418a
Merge pull request #14 from ruju4a/rujuta
ruju4a Apr 28, 2026
9b1fcde
Remove redundant team parent guard
May 2, 2026
4dac539
Clarify calibration summary response loop
May 2, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -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
48 changes: 48 additions & 0 deletions app/controllers/reports_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# 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
#
# 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],
reviewed_object_id: @assignment.id,
for_calibration: true
)
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

def set_assignment
@assignment = Assignment.find(params[:assignment_id])
end

def render_error(message, status)
render json: { error: message }, status: status
end
end
69 changes: 69 additions & 0 deletions app/controllers/review_mappings_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,58 @@ def assign_calibration_artifacts
render json: { status: "ok", message: "Calibration reviews assigned to all reviewers" }
end

# ===== 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: {
assignment_id: @assignment.id,
calibration_participants: @assignment.calibration_participant_rows
}, status: :ok
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

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
Comment on lines +93 to +96

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid rescuing StandardError into a 422 with raw exception text.

On Line 95, this turns unexpected server failures into client-validation errors and exposes internals via e.message. Keep 422 for known domain errors, and return a generic 500 for unknown exceptions.

Suggested hardening
-  rescue ArgumentError, ActiveRecord::RecordInvalid => e
+  rescue ArgumentError, ActiveRecord::RecordInvalid, RuntimeError => e
     render json: { error: e.message }, status: :unprocessable_entity
-  rescue StandardError => e
-    render json: { error: e.message }, status: :unprocessable_entity
+  rescue StandardError => e
+    Rails.logger.error("add_calibration_participant failed: #{e.class}: #{e.message}")
+    render json: { error: 'Unable to add calibration participant' }, status: :internal_server_error
   end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
rescue ArgumentError, ActiveRecord::RecordInvalid => e
render json: { error: e.message }, status: :unprocessable_entity
rescue StandardError => e
render json: { error: e.message }, status: :unprocessable_entity
rescue ArgumentError, ActiveRecord::RecordInvalid, RuntimeError => e
render json: { error: e.message }, status: :unprocessable_entity
rescue StandardError => e
Rails.logger.error("add_calibration_participant failed: #{e.class}: #{e.message}")
render json: { error: 'Unable to add calibration participant' }, status: :internal_server_error
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/controllers/review_mappings_controller.rb` around lines 93 - 96, The
rescue block in ReviewMappingsController that currently rescues StandardError
and returns a 422 with e.message should be changed to avoid exposing internals
and misclassifying server errors: keep rescuing ArgumentError and
ActiveRecord::RecordInvalid (returning status :unprocessable_entity with the
specific message), but replace the generic rescue StandardError => e with
logging (e.g., Rails.logger.error or a configured logger) of the exception and a
render json: { error: "Internal server error" }, status: :internal_server_error;
ensure you reference the rescue block in review_mappings_controller.rb so only
known domain errors return 422 and unexpected exceptions return 500 with no raw
exception text.

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 = AssignmentTeam.team(participant)
return render json: { error: 'Participant has no team' }, status: :unprocessable_entity unless team

ReviewResponseMap.calibration_for(@assignment).where(reviewee_id: team.id).destroy_all

render json: { message: "Calibration participant #{participant.id} removed." }, status: :ok
end

# ===== DELETE =====
def destroy
handler = ReviewMappingHandler.new(@assignment)
Expand All @@ -85,6 +137,23 @@ 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 current_user_teaching_staff_of_assignment?(params[:assignment_id]) if CALIBRATION_PARTICIPANT_ACTIONS.include?(params[:action])

true
end

private

def set_assignment
Expand Down
15 changes: 15 additions & 0 deletions app/models/Item.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
47 changes: 47 additions & 0 deletions app/models/assignment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
33 changes: 33 additions & 0 deletions app/models/assignment_team.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
10 changes: 10 additions & 0 deletions app/models/questionnaire.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
39 changes: 39 additions & 0 deletions app/models/response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,45 @@ 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
# applies to it.
def rubric_items
questionnaire.items.order(:seq)
rescue NoMethodError
[]
end
Comment on lines +76 to +80

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid broad NoMethodError rescue in rubric resolution.

Line 71 can silently mask unrelated bugs and return [], making failures hard to detect. Prefer explicit nil-safe traversal instead of exception-driven control flow.

Suggested fix
 def rubric_items
-  questionnaire.items.order(:seq)
-rescue NoMethodError
-  []
+  assignment_questionnaire = response_assignment.assignment_questionnaires.find_by(used_in_round: round)
+  assignment_questionnaire&.questionnaire&.items&.order(:seq) || []
 end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def rubric_items
questionnaire.items.order(:seq)
rescue NoMethodError
[]
end
def rubric_items
questionnaire&.items&.order(:seq) || []
end
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/models/response.rb` around lines 69 - 73, The current rubric_items method
rescues NoMethodError broadly which can hide real bugs; change it to use
nil-safe traversal so only a missing questionnaire returns an empty array—e.g.,
replace the rescue with a safe navigation call that checks questionnaire before
calling items and order (use questionnaire&.items&.order(:seq) || []) so only
the nil case yields [] and other errors bubble up.


# 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
Expand Down
18 changes: 18 additions & 0 deletions app/models/response_map.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,24 @@ 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

# 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
Expand Down
22 changes: 18 additions & 4 deletions app/models/review_mapping_handler.rb
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -59,8 +69,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
Expand All @@ -71,15 +85,15 @@ def assign_calibration_reviews_round_robin
reviewer: reviewer,
reviewee: team,
reviewed_object_id: @assignment.id,
calibration: true
for_calibration: true
)
end
end
end


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 =====
Expand Down
Loading
Loading