E2609: Review Calibration - participant management & report pipeline#341
Open
ruju4a wants to merge 34 commits into
Open
E2609: Review Calibration - participant management & report pipeline#341ruju4a wants to merge 34 commits into
ruju4a wants to merge 34 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a calibration reporting feature: new controllers and routes for managing calibration participants and serving assignment-scoped calibration reports, model/API additions for marking and serializing calibration maps/responses, service classes to build per-item summaries, a migration adding Changes
sequenceDiagram
participant Client
participant ReportsController
participant ReviewResponseMap as Maps
participant ReportsCalibration as CalibrationReport
participant Response
participant AssignmentTeam as Team
Client->>ReportsController: GET /assignments/:id/calibration/:map_id
ReportsController->>Maps: find map by id scoped to assignment & for_calibration
alt map missing
Maps-->>ReportsController: nil
ReportsController-->>Client: 404 { error }
else map found
Maps-->>ReportsController: instructor_map
ReportsController->>CalibrationReport: initialize(instructor_map)
CalibrationReport->>Response: load latest submitted instructor response
alt instructor response or rubric missing
CalibrationReport-->>ReportsController: raise InstructorResponseMissing/RubricMissing
ReportsController-->>Client: 422 { error }
else valid
CalibrationReport->>Maps: peer_calibration_responses_each(instructor_map)
Maps-->>CalibrationReport: yields latest student responses
CalibrationReport->>CalibrationReport: accumulate per-item buckets
CalibrationReport->>Team: fetch submitted_content (if present)
CalibrationReport-->>ReportsController: payload JSON
ReportsController-->>Client: 200 JSON payload
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Generated by 🚫 Danger |
ruju4a
changed the title
Apply feedback
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/models/Item.rb (1)
74-108:⚠️ Potential issue | 🟡 MinorDuplicate method definitions detected.
The methods
max_score(lines 74-76 and 92-94) andself.for(lines 78-90 and 96-108) are defined twice in this file. While these are not part of this PR's changes, this will cause the second definition to silently override the first.🧹 Proposed fix: remove duplicates
- def max_score - weight - end - - def self.for(record) - klass = case record.question_type - when 'Criterion' - Criterion - when 'Scale' - Scale - else - Item - end - - # Cast the existing record to the desired subclass - klass.new(record.attributes) - end endRemove the duplicate definitions at lines 92-108.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/Item.rb` around lines 74 - 108, The file contains duplicate definitions of the instance method max_score and the class method self.for which causes the latter definitions to silently override the earlier ones; remove the duplicate set (the second max_score and the second self.for block) so each of max_score and self.for is defined only once, keeping the intended implementation that uses weight for max_score and the self.for logic that selects klass (Criterion, Scale, or Item) and instantiates it with record.attributes.app/models/assignment_team.rb (1)
126-239:⚠️ Potential issue | 🟠 MajorRemove duplicate method definitions in app/models/assignment_team.rb.
Six methods are defined twice, with the second definitions silently overriding the first:
review_map_type(lines 127–129 and 188–190) — identicalassign_reviewer(lines 136–141 and 223–228) — identicalhas_submissions?(lines 144–146 and 231–233) — identicalaggregate_review_grade(lines 150–152 and 237–239) — identicaladd_participant(lines 157–166 and 195–204) — identicalremove_participant(lines 172–185 and 210–220) — different behavior: the first includesdestroy if participants.empty?to delete empty teams; the second omits this, causing teams to persist when they should be destroyed.Keep only one definition of each method, using the first
remove_participantimplementation (lines 172–185) which includes the team cleanup logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/assignment_team.rb` around lines 126 - 239, The file contains duplicate method definitions — remove the repeated copies of review_map_type, assign_reviewer, has_submissions?, aggregate_review_grade, add_participant, and remove_participant so each method is defined only once; keep the original implementations and ensure the remove_participant implementation that includes the cleanup step (calls destroy if participants.empty?) is retained (delete the second, shorter remove_participant), and remove the duplicate blocks for add_participant, review_map_type, assign_reviewer, has_submissions?, and aggregate_review_grade so there is a single canonical definition for each.
🧹 Nitpick comments (2)
app/models/team.rb (1)
25-70: Fix error message propagation in exception handling.The syntax
raise ActiveRecord::RecordInvalid.new(team), errorpasses the error string as the backtrace, not the exception message. The custom error won't appear in the exception message as intended.♻️ Proposed fix
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 + team.errors.add(:base, error) + raise ActiveRecord::RecordInvalid.new(team)This ensures the error message is included in the exception's message via the team's errors collection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/team.rb` around lines 25 - 70, The exception currently does not include the custom error because `raise ActiveRecord::RecordInvalid.new(team), error` misuses the second arg; update create_team_for_participant to add the error into the team's errors (e.g. team.errors.add(:base, error)) before raising and then raise ActiveRecord::RecordInvalid.new(team) so the custom message is part of the RecordInvalid exception; modify the block that handles a failed add_member (the result variable, team.destroy, and the raise) to add the failure message to team.errors and then raise ActiveRecord::RecordInvalid.new(team).app/controllers/review_mappings_controller.rb (1)
250-262: UseResponseMap#review_statusinstead of duplicating the state machine here.This controller now has a second copy of the review-status rules, so it can drift from the model on the next behavior change.
serialize_calibration_rowshould read the status directly from the map and keep the controller focused on shaping JSON.Small cleanup
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), + instructor_review_status: instructor_map&.review_status || :not_started, submissions: team.respond_to?(:submitted_content_detail) ? team.submitted_content_detail : { hyperlinks: [], files: [] } } end - - 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 - endAlso applies to: 270-278
🤖 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 250 - 262, serialize_calibration_row currently computes the instructor review state via the controller helper instructor_review_status_for, duplicating state-machine logic; instead read the status directly from the ResponseMap model by calling instructor_map.review_status (or instructor_map&.review_status to handle nil) and return that in instructor_review_status, and apply the same replacement for the other block noted (lines 270-278) so the controller only shapes JSON and the model owns review-status logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/reports_controller.rb`:
- Around line 25-37: The code currently treats any calibration map as the
"instructor" baseline; after fetching instructor_map via
ReviewResponseMap.find_by!(id: params[:map_id], reviewed_object_id:
`@assignment.id`, for_calibration: true) add a guard that verifies the map is
actually the instructor's calibration map (e.g. check a discriminator or
reviewer role on instructor_map such as instructor_map.reviewer.role ==
'Instructor' or instructor_map.type == 'InstructorReviewResponseMap'), and if it
is a student calibration map return render_error('Map must be instructor
calibration map', :unprocessable_entity) before calling
instructor_map.latest_submitted_response and
ReviewResponseMap.peer_calibration_responses_for(instructor_map).
In `@app/controllers/review_mappings_controller.rb`:
- Around line 98-109: Replace the fallback that uses participant.team or
Team.create_team_for_participant when choosing the reviewee team; instead call
the assignment-level calibration helper to obtain or create a dedicated
single-member calibration team (do not use participant.team or
Team.create_team_for_participant). Concretely, where the code uses
"participant.team || Team.create_team_for_participant(participant)" before
creating the ReviewResponseMap, call the assignment helper (e.g.,
`@assignment.calibration_team_for`(participant) or
Assignment#calibration_team_for(participant)); if that helper does not exist,
add one that returns/creates a single-member team scoped to the assignment and
use its id for reviewee_id in ReviewResponseMap creation. Ensure all references
to participant.team are removed from this calibration flow so the real student
teams are never reused.
In `@app/models/response.rb`:
- Around line 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.
In `@app/services/calibration_per_item_summary.rb`:
- Line 43: The selection using max_by(&:updated_at) in
calibration_per_item_summary (the mapping that picks the latest response) is
non-deterministic when two responses share the same updated_at; replace that
call with a deterministic comparison that uses a composite key of updated_at
then id (i.e., use updated_at as the primary sort key and id as a secondary
tie-breaker) so the latest-response selection is stable on timestamp ties.
In `@lib/tasks/calibration_demo.rake`:
- Around line 90-103: Replace hard-coded primary key lookups
(Assignment.find_by(id: 8), ReviewResponseMap.find(8),
ReviewResponseMap.exists?(id: 8, ...)) with lookups that use stable/demo
attributes (e.g., name and for_calibration) and safe creation; specifically,
change the Assignment resolution to find_by(name: 'Calibration Demo Assignment')
and fall back to find_or_create_by!(name: ..., instructor: ..., has_teams: true,
private: false) (update attributes afterwards as needed), and change the
ReviewResponseMap logic to search by reviewed_object_id/reviewed_object:
assignment and for_calibration: true (e.g.,
ReviewResponseMap.find_by(reviewed_object: assignment, for_calibration: true))
and create a new map if missing instead of referencing a hard-coded id; apply
the same pattern to the similar block around the later occurrence (lines
136-143) to avoid reusing arbitrary PKs.
---
Outside diff comments:
In `@app/models/assignment_team.rb`:
- Around line 126-239: The file contains duplicate method definitions — remove
the repeated copies of review_map_type, assign_reviewer, has_submissions?,
aggregate_review_grade, add_participant, and remove_participant so each method
is defined only once; keep the original implementations and ensure the
remove_participant implementation that includes the cleanup step (calls destroy
if participants.empty?) is retained (delete the second, shorter
remove_participant), and remove the duplicate blocks for add_participant,
review_map_type, assign_reviewer, has_submissions?, and aggregate_review_grade
so there is a single canonical definition for each.
In `@app/models/Item.rb`:
- Around line 74-108: The file contains duplicate definitions of the instance
method max_score and the class method self.for which causes the latter
definitions to silently override the earlier ones; remove the duplicate set (the
second max_score and the second self.for block) so each of max_score and
self.for is defined only once, keeping the intended implementation that uses
weight for max_score and the self.for logic that selects klass (Criterion,
Scale, or Item) and instantiates it with record.attributes.
---
Nitpick comments:
In `@app/controllers/review_mappings_controller.rb`:
- Around line 250-262: serialize_calibration_row currently computes the
instructor review state via the controller helper instructor_review_status_for,
duplicating state-machine logic; instead read the status directly from the
ResponseMap model by calling instructor_map.review_status (or
instructor_map&.review_status to handle nil) and return that in
instructor_review_status, and apply the same replacement for the other block
noted (lines 270-278) so the controller only shapes JSON and the model owns
review-status logic.
In `@app/models/team.rb`:
- Around line 25-70: The exception currently does not include the custom error
because `raise ActiveRecord::RecordInvalid.new(team), error` misuses the second
arg; update create_team_for_participant to add the error into the team's errors
(e.g. team.errors.add(:base, error)) before raising and then raise
ActiveRecord::RecordInvalid.new(team) so the custom message is part of the
RecordInvalid exception; modify the block that handles a failed add_member (the
result variable, team.destroy, and the raise) to add the failure message to
team.errors and then raise ActiveRecord::RecordInvalid.new(team).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e82275c-014e-40cc-a2dc-07c14b8d3260
📒 Files selected for processing (21)
app/controllers/reports_controller.rbapp/controllers/review_mappings_controller.rbapp/models/Item.rbapp/models/assignment_team.rbapp/models/response.rbapp/models/response_map.rbapp/models/review_mapping_handler.rbapp/models/review_response_map.rbapp/models/team.rbapp/services/calibration_per_item_summary.rbconfig/routes.rbdb/migrate/20260423010050_add_for_calibration_to_response_maps.rbdb/schema.rblib/tasks/calibration_demo.rakespec/factories/review_response_maps.rbspec/factories/teams.rbspec/models/review_mapping_handler_spec.rbspec/models/team_factory_spec.rbspec/requests/api/v1/calibration_participants_spec.rbspec/requests/api/v1/reports_calibration_spec.rbspec/services/calibration_per_item_summary_spec.rb
Comment on lines
+25
to
+37
| 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) |
There was a problem hiding this comment.
Reject student calibration map IDs here.
This action accepts any calibration map on the assignment. If params[:map_id] points to a student map, the student’s submission becomes the “instructor” baseline and the real instructor response is folded into student_responses, so the report is wrong while still returning 200.
Suggested guard
instructor_map = ReviewResponseMap.find_by!(
id: params[:map_id],
reviewed_object_id: `@assignment.id`,
for_calibration: true
)
+ return render_error('Calibration review map not found', :not_found) unless instructor_map.reviewer&.user_id == `@assignment.instructor_id`
instructor_response = instructor_map.latest_submitted_response📝 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
| 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) | |
| instructor_map = ReviewResponseMap.find_by!( | |
| id: params[:map_id], | |
| reviewed_object_id: `@assignment.id`, | |
| for_calibration: true | |
| ) | |
| return render_error('Calibration review map not found', :not_found) unless instructor_map.reviewer&.user_id == `@assignment.instructor_id` | |
| 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) |
🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 32-32: Modifier form of unless makes the line too long.
(Style/IfUnlessModifier)
[convention] 32-32: Line is too long. [128/120]
(Layout/LineLength)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/reports_controller.rb` around lines 25 - 37, The code
currently treats any calibration map as the "instructor" baseline; after
fetching instructor_map via ReviewResponseMap.find_by!(id: params[:map_id],
reviewed_object_id: `@assignment.id`, for_calibration: true) add a guard that
verifies the map is actually the instructor's calibration map (e.g. check a
discriminator or reviewer role on instructor_map such as
instructor_map.reviewer.role == 'Instructor' or instructor_map.type ==
'InstructorReviewResponseMap'), and if it is a student calibration map return
render_error('Map must be instructor calibration map', :unprocessable_entity)
before calling instructor_map.latest_submitted_response and
ReviewResponseMap.peer_calibration_responses_for(instructor_map).
Comment on lines
+69
to
+73
| def rubric_items | ||
| questionnaire.items.order(:seq) | ||
| rescue NoMethodError | ||
| [] | ||
| end |
There was a problem hiding this comment.
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.
| .select(&:is_submitted) | ||
| .group_by(&:map_id) | ||
| .values | ||
| .map { |responses| responses.max_by(&:updated_at) } |
There was a problem hiding this comment.
Make latest-response selection deterministic on timestamp ties.
On Line 43, max_by(&:updated_at) is ambiguous when two submitted responses share the same updated_at. Add a secondary key (e.g., id) to ensure stable behavior.
Suggested fix
- .map { |responses| responses.max_by(&:updated_at) }
+ .map { |responses| responses.max_by { |response| [response.updated_at, response.id] } }📝 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
| .map { |responses| responses.max_by(&:updated_at) } | |
| .map { |responses| responses.max_by { |response| [response.updated_at, response.id] } } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/calibration_per_item_summary.rb` at line 43, The selection using
max_by(&:updated_at) in calibration_per_item_summary (the mapping that picks the
latest response) is non-deterministic when two responses share the same
updated_at; replace that call with a deterministic comparison that uses a
composite key of updated_at then id (i.e., use updated_at as the primary sort
key and id as a secondary tie-breaker) so the latest-response selection is
stable on timestamp ties.
Comment on lines
+90
to
+103
| 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') |
There was a problem hiding this comment.
Remove the hard-coded PK reuse from the demo seed.
Looking up Assignment.find_by(id: 8) and ReviewResponseMap.find_by(id: 8, ...) will repurpose any unrelated records that happen to have those IDs, then overwrite them as demo data. This is a bad footgun for shared dev/staging databases.
Safer lookup pattern
- assignment = Assignment.find_by(id: 8) || Assignment.find_by(name: 'Calibration Demo Assignment')
+ assignment = 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
+ reviewee_team = assignment.teams.find_or_create_by!(name: 'Calibration Demo Reviewee Team')
@@
- 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
- )
+ instructor_map = ReviewResponseMap.find_or_create_by!(
+ reviewed_object_id: assignment.id,
+ reviewer_id: instructor_participant.id,
+ reviewee_id: reviewee_team.id,
+ for_calibration: true
+ )Also applies to: 136-143
🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 100-100: Line is too long. [121/120]
(Layout/LineLength)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/tasks/calibration_demo.rake` around lines 90 - 103, Replace hard-coded
primary key lookups (Assignment.find_by(id: 8), ReviewResponseMap.find(8),
ReviewResponseMap.exists?(id: 8, ...)) with lookups that use stable/demo
attributes (e.g., name and for_calibration) and safe creation; specifically,
change the Assignment resolution to find_by(name: 'Calibration Demo Assignment')
and fall back to find_or_create_by!(name: ..., instructor: ..., has_teams: true,
private: false) (update attributes afterwards as needed), and change the
ReviewResponseMap logic to search by reviewed_object_id/reviewed_object:
assignment and for_calibration: true (e.g.,
ReviewResponseMap.find_by(reviewed_object: assignment, for_calibration: true))
and create a new map if missing instead of referencing a hard-coded id; apply
the same pattern to the similar block around the later occurrence (lines
136-143) to avoid reusing arbitrary PKs.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
app/models/assignment.rb (1)
100-100:⚠️ Potential issue | 🟠 MajorDo not reuse a participant’s regular team for calibration reviewees.
On Line 100,
AssignmentTeam.team(submitter)can attach calibration mappings to a real assignment team/submission. Calibration should use a dedicated calibration-scoped team path only.Suggested direction
- team = AssignmentTeam.team(submitter) || Team.create_team_for_participant(submitter) + team = calibration_team_for!(submitter)# Add in Assignment (example) def calibration_team_for!(participant) # resolve/create a single-member calibration-designated AssignmentTeam # and never fall back to participant’s regular submission team end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/assignment.rb` at line 100, Replace the use of AssignmentTeam.team(submitter) (and the fallback Team.create_team_for_participant(submitter)) with a dedicated calibration-team resolver: add a method like calibration_team_for!(participant) on Assignment that always resolves or creates a single-member, calibration-scoped AssignmentTeam (never falling back to the participant’s regular submission team), and then call that method in place of AssignmentTeam.team(submitter) on the offending line; ensure the new method uses the calibration team path/flags so calibration mappings attach only to the calibration-specific team.
🧹 Nitpick comments (4)
app/services/calibration_per_item_summary.rb (1)
94-99: Consider usingQuestionnaire#score_rangeinstead of duplicating logic.The
score_range_formethod duplicates the logic inQuestionnaire#score_range(lines 93-97 inapp/models/questionnaire.rb). Using the questionnaire method avoids duplication and ensures consistency.♻️ Suggested change
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 + item.questionnaire&.score_range || (0..5) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/calibration_per_item_summary.rb` around lines 94 - 99, Replace the duplicated range logic in score_range_for by delegating to the Questionnaire model's score_range method: when item.questionnaire is present call item.questionnaire.score_range and otherwise fall back to the same default range (0..5); update the score_range_for method to use safe navigation (item.questionnaire&.score_range) with a default to preserve current behavior. This removes duplication between score_range_for and Questionnaire#score_range and keeps semantics consistent.app/models/review_mapping_handler.rb (2)
95-97: Consider using thecalibration_forscope here as well.Similar to above, this query could leverage the centralized scope.
♻️ Suggested change
def calibration_reviews_for(reviewer) - ReviewResponseMap.where(reviewer: reviewer, reviewed_object_id: `@assignment.id`, for_calibration: true) + ReviewResponseMap.calibration_for(`@assignment`).where(reviewer: reviewer) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/review_mapping_handler.rb` around lines 95 - 97, Replace the ad-hoc query in calibration_reviews_for with the centralized ReviewResponseMap scope by calling the existing calibration_for scope instead of directly using where; update calibration_reviews_for (the method currently calling ReviewResponseMap.where(reviewer: reviewer, reviewed_object_id: `@assignment.id`, for_calibration: true)) to delegate to ReviewResponseMap.calibration_for(...) passing the reviewer and the assignment identifier/object expected by that scope so the centralized filtering logic is reused.
72-77: Consider using thecalibration_forscope for consistency.The
ReviewResponseMap.calibration_forscope (seeapp/models/review_response_map.rb:8-10) centralizes thefor_calibration: truecondition. Using it here avoids repeating the condition and ensures consistency if the criteria change.♻️ Suggested change
- calibration_team_ids = ReviewResponseMap - .where(reviewed_object_id: `@assignment.id`, for_calibration: true) - .distinct - .pluck(:reviewee_id) + calibration_team_ids = ReviewResponseMap + .calibration_for(`@assignment`) + .distinct + .pluck(:reviewee_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/review_mapping_handler.rb` around lines 72 - 77, Replace the inline where(..., for_calibration: true) query with the existing ReviewResponseMap.calibration_for scope to avoid duplicating the calibration condition; specifically, update the code that builds calibration_team_ids (currently using ReviewResponseMap.where(reviewed_object_id: `@assignment.id`, for_calibration: true).distinct.pluck(:reviewee_id)) to call ReviewResponseMap.calibration_for(`@assignment.id`).distinct.pluck(:reviewee_id) (or the correct arity for calibration_for) and keep the subsequent AssignmentTeam.where(id: calibration_team_ids) logic unchanged.app/models/response_map.rb (1)
35-39: Consider usingexists?instead ofempty?for efficiency.
responses.empty?loads all response records into memory to check if the collection is empty. Usingresponses.exists?performs a more efficientSELECT 1 ... LIMIT 1query instead.♻️ Suggested improvement
def review_status - return :not_started if responses.empty? + return :not_started unless responses.exists? + return :submitted if responses.where(is_submitted: true).exists? + :in_progress end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/response_map.rb` around lines 35 - 39, The review_status method uses responses.empty? which loads records; change it to use responses.exists? (or invert the check: return :not_started unless responses.exists?) to perform an efficient presence query, while keeping the subsequent submitted check (responses.where(is_submitted: true).exists?) and the final :in_progress return intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/review_mappings_controller.rb`:
- Around line 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.
In `@app/models/review_response_map.rb`:
- Around line 56-73: Remove the unused instructor_user_id parameter from the
calibration_participant_json method signature in ReviewResponseMap (delete
"instructor_user_id:" from def calibration_participant_json) and update any
callers to stop passing that argument—specifically remove the extra argument
where calibration_participant_json is invoked from the Assignment model (the
calls that currently pass instructor_user_id) so callers simply call
calibration_participant_json with no params; ensure tests/uses that relied on
that parameter are adjusted accordingly.
In `@app/services/reports/calibration_report.rb`:
- Around line 90-97: The student_response_count is currently using the global
`@student_responses.size` which overcounts; change it to use the per-item bucket
total from `@bucket_counts` for this item (replace `@student_responses.size` with
the sum of the bucket values for `@bucket_counts`[item.id], e.g.
`@bucket_counts`[item.id]&.values&.sum || 0) so student_response_count reflects
only responses for that item (refer to student_response_count,
`@student_responses`, `@bucket_counts` and item.id).
In `@spec/services/calibration_per_item_summary_spec.rb`:
- Line 29: The test assumes instructor_participant.user_id maps to an Instructor
record but the default user factory doesn't create an instructor; update the
spec to create the participant with an instructor user so
Instructor.find(instructor_participant.user_id) succeeds — e.g. when building
instructor_participant use create(:assignment_participant, assignment:
assignment, user: create(:user, :instructor)) or change the
:assignment_participant factory to support an :instructor trait; ensure the
change is applied where instructor_participant is defined so
Instructor.find(instructor_participant.user_id) no longer raises RecordNotFound.
---
Duplicate comments:
In `@app/models/assignment.rb`:
- Line 100: Replace the use of AssignmentTeam.team(submitter) (and the fallback
Team.create_team_for_participant(submitter)) with a dedicated calibration-team
resolver: add a method like calibration_team_for!(participant) on Assignment
that always resolves or creates a single-member, calibration-scoped
AssignmentTeam (never falling back to the participant’s regular submission
team), and then call that method in place of AssignmentTeam.team(submitter) on
the offending line; ensure the new method uses the calibration team path/flags
so calibration mappings attach only to the calibration-specific team.
---
Nitpick comments:
In `@app/models/response_map.rb`:
- Around line 35-39: The review_status method uses responses.empty? which loads
records; change it to use responses.exists? (or invert the check: return
:not_started unless responses.exists?) to perform an efficient presence query,
while keeping the subsequent submitted check (responses.where(is_submitted:
true).exists?) and the final :in_progress return intact.
In `@app/models/review_mapping_handler.rb`:
- Around line 95-97: Replace the ad-hoc query in calibration_reviews_for with
the centralized ReviewResponseMap scope by calling the existing calibration_for
scope instead of directly using where; update calibration_reviews_for (the
method currently calling ReviewResponseMap.where(reviewer: reviewer,
reviewed_object_id: `@assignment.id`, for_calibration: true)) to delegate to
ReviewResponseMap.calibration_for(...) passing the reviewer and the assignment
identifier/object expected by that scope so the centralized filtering logic is
reused.
- Around line 72-77: Replace the inline where(..., for_calibration: true) query
with the existing ReviewResponseMap.calibration_for scope to avoid duplicating
the calibration condition; specifically, update the code that builds
calibration_team_ids (currently using
ReviewResponseMap.where(reviewed_object_id: `@assignment.id`, for_calibration:
true).distinct.pluck(:reviewee_id)) to call
ReviewResponseMap.calibration_for(`@assignment.id`).distinct.pluck(:reviewee_id)
(or the correct arity for calibration_for) and keep the subsequent
AssignmentTeam.where(id: calibration_team_ids) logic unchanged.
In `@app/services/calibration_per_item_summary.rb`:
- Around line 94-99: Replace the duplicated range logic in score_range_for by
delegating to the Questionnaire model's score_range method: when
item.questionnaire is present call item.questionnaire.score_range and otherwise
fall back to the same default range (0..5); update the score_range_for method to
use safe navigation (item.questionnaire&.score_range) with a default to preserve
current behavior. This removes duplication between score_range_for and
Questionnaire#score_range and keeps semantics consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 42e85f4a-d08b-4eef-a1f2-3067cc4bdbd0
📒 Files selected for processing (15)
.gitignoreapp/controllers/reports_controller.rbapp/controllers/review_mappings_controller.rbapp/models/assignment.rbapp/models/questionnaire.rbapp/models/response.rbapp/models/response_map.rbapp/models/review_mapping_handler.rbapp/models/review_response_map.rbapp/services/calibration_per_item_summary.rbapp/services/reports/calibration_report.rbapp/services/reports/report_template.rbconfig/routes.rbspec/models/response_map_spec.rbspec/services/calibration_per_item_summary_spec.rb
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- spec/models/response_map_spec.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- config/routes.rb
- app/models/response.rb
Comment on lines
+93
to
+96
| rescue ArgumentError, ActiveRecord::RecordInvalid => e | ||
| render json: { error: e.message }, status: :unprocessable_entity | ||
| rescue StandardError => e | ||
| render json: { error: e.message }, status: :unprocessable_entity |
There was a problem hiding this comment.
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.
Comment on lines
+56
to
+73
| 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 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how calibration_participant_json is called to see if instructor_user_id is passed
rg -n 'calibration_participant_json'Repository: expertiza/reimplementation-back-end
Length of output: 391
🏁 Script executed:
# View the call sites in assignment.rb to understand the context
sed -n '100,130p' app/models/assignment.rbRepository: expertiza/reimplementation-back-end
Length of output: 1450
🏁 Script executed:
# View the exact method implementation in review_response_map.rb
sed -n '56,73p' app/models/review_response_map.rbRepository: expertiza/reimplementation-back-end
Length of output: 777
Remove unused parameter instructor_user_id.
The instructor_user_id parameter is declared but never used in the method body. If removed, update the call sites in assignment.rb (lines 108 and 122) to no longer pass this parameter.
🔧 Changes required
In app/models/review_response_map.rb:
-def calibration_participant_json(instructor_user_id:)
+def calibration_participant_json
team = reviewee
submitter = team.participants.where(type: 'AssignmentParticipant').firstIn app/models/assignment.rb (line 108):
-map.calibration_participant_json(instructor_user_id: instructor_id)
+map.calibration_participant_jsonIn app/models/assignment.rb (line 122):
-instructor_map.calibration_participant_json(instructor_user_id: instructor_id)
+instructor_map.calibration_participant_json📝 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 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 | |
| def calibration_participant_json | |
| 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 |
🧰 Tools
🪛 RuboCop (1.86.1)
[warning] 56-56: Unused method argument - instructor_user_id. You can also write as calibration_participant_json(*) if you want the method to accept any arguments but don't care about them.
(Lint/UnusedMethodArgument)
[convention] 62-62: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 63-63: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 64-64: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 65-65: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 66-66: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 67-67: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 68-68: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 71-71: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/review_response_map.rb` around lines 56 - 73, Remove the unused
instructor_user_id parameter from the calibration_participant_json method
signature in ReviewResponseMap (delete "instructor_user_id:" from def
calibration_participant_json) and update any callers to stop passing that
argument—specifically remove the extra argument where
calibration_participant_json is invoked from the Assignment model (the calls
that currently pass instructor_user_id) so callers simply call
calibration_participant_json with no params; ensure tests/uses that relied on
that parameter are adjusted accordingly.
Comment on lines
+90
to
+97
| 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 | ||
| } |
There was a problem hiding this comment.
student_response_count should be item-specific, not global.
On Line 96, using @student_responses.size overstates counts when answers are missing for a given item. Use that item’s bucket total instead.
Suggested fix
- student_response_count: `@student_responses.size`
+ student_response_count: `@bucket_counts`[item.id].values.sum🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 90-90: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 91-91: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 92-92: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 93-93: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 94-94: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 95-95: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/reports/calibration_report.rb` around lines 90 - 97, The
student_response_count is currently using the global `@student_responses.size`
which overcounts; change it to use the per-item bucket total from `@bucket_counts`
for this item (replace `@student_responses.size` with the sum of the bucket values
for `@bucket_counts`[item.id], e.g. `@bucket_counts`[item.id]&.values&.sum || 0) so
student_response_count reflects only responses for that item (refer to
student_response_count, `@student_responses`, `@bucket_counts` and item.id).
| private: false, | ||
| min_question_score: 0, | ||
| max_question_score: 5, | ||
| instructor: Instructor.find(instructor_participant.user_id) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what user type the assignment_participant factory creates
rg -n -A 10 'factory :assignment_participant' spec/factories/Repository: expertiza/reimplementation-back-end
Length of output: 810
🏁 Script executed:
#!/bin/bash
# Read the test file around line 29 to see the context
head -40 spec/services/calibration_per_item_summary_spec.rb | tail -20Repository: expertiza/reimplementation-back-end
Length of output: 693
🏁 Script executed:
#!/bin/bash
# Read the full spec file to understand the test setup, particularly where instructor_participant is defined
cat -n spec/services/calibration_per_item_summary_spec.rb | head -50Repository: expertiza/reimplementation-back-end
Length of output: 2358
🏁 Script executed:
#!/bin/bash
# Check the factories for User and Instructor to understand the relationship
rg -n 'factory :user' spec/factories/ -A 5Repository: expertiza/reimplementation-back-end
Length of output: 61
🏁 Script executed:
#!/bin/bash
# List all factory files
ls -la spec/factories/Repository: expertiza/reimplementation-back-end
Length of output: 841
🏁 Script executed:
#!/bin/bash
# Check the main factories file
cat spec/factories/factories.rbRepository: expertiza/reimplementation-back-end
Length of output: 141
🏁 Script executed:
#!/bin/bash
# Check the roles factory which likely defines the User factory
cat spec/factories/roles.rbRepository: expertiza/reimplementation-back-end
Length of output: 1259
🏁 Script executed:
#!/bin/bash
# Search for User factory definition across the repository
rg 'factory :user' spec/ -A 8Repository: expertiza/reimplementation-back-end
Length of output: 595
🏁 Script executed:
#!/bin/bash
# Get more context on the User factory and its traits
rg 'factory :user' spec/factories.rb -A 25Repository: expertiza/reimplementation-back-end
Length of output: 697
🏁 Script executed:
#!/bin/bash
# Check the Instructor model to understand its relationship to User
rg 'class Instructor' app/models/ -A 3Repository: expertiza/reimplementation-back-end
Length of output: 273
🏁 Script executed:
#!/bin/bash
# Check if User uses STI (Single Table Inheritance) with a type column
rg 'type.*Instructor\|Instructor.*type' app/models/user.rb -B 3 -A 3Repository: expertiza/reimplementation-back-end
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Check the User model structure
head -50 app/models/user.rbRepository: expertiza/reimplementation-back-end
Length of output: 1806
🏁 Script executed:
#!/bin/bash
# Check if there's an :instructor_participant factory variant
rg 'factory.*instructor' spec/factories/ -iRepository: expertiza/reimplementation-back-end
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Check the Questionnaire model to see how it validates the instructor field
rg 'class Questionnaire' app/models/ -A 20Repository: expertiza/reimplementation-back-end
Length of output: 1309
🏁 Script executed:
#!/bin/bash
# Check what role is assigned by default in the :user factory by looking at the role factory
cat spec/factories.rb | grep -A 30 'factory :role'Repository: expertiza/reimplementation-back-end
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Check the spec/factories/roles.rb for the default role factory
rg 'factory :role' spec/factories/roles.rb -A 3Repository: expertiza/reimplementation-back-end
Length of output: 161
🏁 Script executed:
#!/bin/bash
# Now let's verify by checking what the default role is when creating a plain user
rg 'role { create' spec/factories.rb -B 2 -A 2Repository: expertiza/reimplementation-back-end
Length of output: 182
Test fragility: assumes participant's user is an Instructor when factory creates default User role.
Line 29 calls Instructor.find(instructor_participant.user_id), but instructor_participant is created with the default :user factory which assigns a generic role, not an instructor role. This will raise RecordNotFound since only users with the :instructor trait (or role) can be retrieved via Instructor.find().
Use create(:assignment_participant, assignment: assignment, user: create(:user, :instructor)) instead, or add an :instructor trait to the :assignment_participant factory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/services/calibration_per_item_summary_spec.rb` at line 29, The test
assumes instructor_participant.user_id maps to an Instructor record but the
default user factory doesn't create an instructor; update the spec to create the
participant with an instructor user so
Instructor.find(instructor_participant.user_id) succeeds — e.g. when building
instructor_participant use create(:assignment_participant, assignment:
assignment, user: create(:user, :instructor)) or change the
:assignment_participant factory to support an :instructor trait; ensure the
change is applied where instructor_participant is defined so
Instructor.find(instructor_participant.user_id) no longer raises RecordNotFound.
Updated specs for reports controller
There was a problem hiding this comment.
🧹 Nitpick comments (4)
spec/requests/api/v1/reports_calibration_spec.rb (4)
310-315: Minor: Line length exceeds 120 characters.Line 313 exceeds the configured line length limit. Consider breaking it for readability.
♻️ Suggested fix
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]) } + scores.each do |item, score| + Answer.create!(response: r, item: item, answer: score[:answer], comments: score[:comments]) + end r end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/api/v1/reports_calibration_spec.rb` around lines 310 - 315, The long single-line call to Response.create! in make_response exceeds 120 chars; split the argument hash across multiple lines for readability (e.g., place each attribute like response_map:, round:, version_num:, is_submitted:, created_at:, updated_at: on its own line) and similarly keep the scores.each { |item, score| Answer.create!(...) } line short by either breaking the block into a multi-line do...end or formatting the Answer.create! call with each keyword argument on its own line; update make_response, Response.create!, and Answer.create! invocations accordingly to stay under the line-length limit while preserving behavior.
64-68: Minor: Preferdo...endfor multi-line blocks.RuboCop flagged the use of
{...}for a multi-line block. Consider usingdo...endfor consistency with Ruby style conventions in tests.♻️ Suggested fix
- 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 do + post participants_path, params: { username: 'unctlt1' }, headers: instructor_headers + end.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)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/api/v1/reports_calibration_spec.rb` around lines 64 - 68, Change the multi-line expect block from brace syntax to do...end: replace the outer expect { ... }.to change { ... }.and change { ... }.and change { ... } with expect do ... end so the entire multi-line block uses do/end; update the inner change blocks to keep their existing lambdas (e.g., AssignmentParticipant.where(...).count, AssignmentTeam.where(...).count, ReviewResponseMap.where(...).count) unchanged but ensure the outer block uses do/end around the post participants_path call with params: { username: 'unctlt1' }, headers: instructor_headers.
259-261: Redundantupdate!call.The
seqattribute is already set duringItem.create!on line 255. The subsequentitem.update!(seq: seq)appears unnecessary unless there's a specific callback or validation that requires a two-step save.♻️ Suggested simplification
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🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/api/v1/reports_calibration_spec.rb` around lines 259 - 261, The second save is redundant: after creating the record with Item.create!(...) that already sets seq, remove the subsequent item.update!(seq: seq) call; keep the originally created item (variable item) returned from the creation and rely on Item.create! to persist seq unless there is an explicit reason (callback/validation) to re-run update — if such special logic exists, add a comment explaining it, otherwise delete the item.update!(seq: seq) line.
246-248: Use the User instance directly instead of re-fetching as Instructor.The
instructorvariable is already a User with the instructor role, so there's no need to callInstructor.find(instructor.id). SinceInstructorinherits fromUserwith a shared table, this technically works but is unnecessarily indirect and makes the code harder to follow. Passinstructordirectly:max_question_score: 5, instructor: instructor )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/api/v1/reports_calibration_spec.rb` around lines 246 - 248, The test is re-fetching the instructor with Instructor.find(instructor.id) even though the local variable instructor already holds the User instance with the instructor role; replace the Instructor.find(...) usage with the existing instructor variable (i.e., pass instructor directly to the object creation where max_question_score and instructor are set) to simplify and clarify the spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/requests/api/v1/reports_calibration_spec.rb`:
- Around line 310-315: The long single-line call to Response.create! in
make_response exceeds 120 chars; split the argument hash across multiple lines
for readability (e.g., place each attribute like response_map:, round:,
version_num:, is_submitted:, created_at:, updated_at: on its own line) and
similarly keep the scores.each { |item, score| Answer.create!(...) } line short
by either breaking the block into a multi-line do...end or formatting the
Answer.create! call with each keyword argument on its own line; update
make_response, Response.create!, and Answer.create! invocations accordingly to
stay under the line-length limit while preserving behavior.
- Around line 64-68: Change the multi-line expect block from brace syntax to
do...end: replace the outer expect { ... }.to change { ... }.and change { ...
}.and change { ... } with expect do ... end so the entire multi-line block uses
do/end; update the inner change blocks to keep their existing lambdas (e.g.,
AssignmentParticipant.where(...).count, AssignmentTeam.where(...).count,
ReviewResponseMap.where(...).count) unchanged but ensure the outer block uses
do/end around the post participants_path call with params: { username: 'unctlt1'
}, headers: instructor_headers.
- Around line 259-261: The second save is redundant: after creating the record
with Item.create!(...) that already sets seq, remove the subsequent
item.update!(seq: seq) call; keep the originally created item (variable item)
returned from the creation and rely on Item.create! to persist seq unless there
is an explicit reason (callback/validation) to re-run update — if such special
logic exists, add a comment explaining it, otherwise delete the
item.update!(seq: seq) line.
- Around line 246-248: The test is re-fetching the instructor with
Instructor.find(instructor.id) even though the local variable instructor already
holds the User instance with the instructor role; replace the
Instructor.find(...) usage with the existing instructor variable (i.e., pass
instructor directly to the object creation where max_question_score and
instructor are set) to simplify and clarify the spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aebbe478-e717-4cc2-a60e-754d8d6dc48f
📒 Files selected for processing (1)
spec/requests/api/v1/reports_calibration_spec.rb
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 matched /(temp)/i in the Dangerfile check; renamed to base_report.rb and updated CalibrationReport to inherit from Reports::Base.
Rename ReportTemplate to Reports::Base to fix Danger temp-file warning
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/services/reports/calibration_report.rb (1)
87-97:⚠️ Potential issue | 🟠 MajorUse item-local response totals in
per_item_summary.At Line 96,
@student_responses.sizeis global and overcounts for items with missing answers. Use this item’s bucket total instead.Suggested fix
def per_item_summary(item) instructor_answer = `@instructor_response.answer_for`(item) { 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` + student_response_count: `@bucket_counts`[item.id]&.values&.sum || 0 } end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/reports/calibration_report.rb` around lines 87 - 97, per_item_summary currently uses the global `@student_responses.size` which overcounts items with missing answers; change student_response_count to use the per-item total from `@bucket_counts` for that item (e.g. compute item_bucket_total = (`@bucket_counts`[item.id] || {}).values.sum or similar) and return that value instead of `@student_responses.size` inside the per_item_summary method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/services/reports/calibration_report.rb`:
- Around line 87-97: per_item_summary currently uses the global
`@student_responses.size` which overcounts items with missing answers; change
student_response_count to use the per-item total from `@bucket_counts` for that
item (e.g. compute item_bucket_total = (`@bucket_counts`[item.id] ||
{}).values.sum or similar) and return that value instead of
`@student_responses.size` inside the per_item_summary method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c190177c-2ca2-4adc-b077-ceb3594eb89e
📒 Files selected for processing (2)
app/services/reports/base_report.rbapp/services/reports/calibration_report.rb
efg
reviewed
Apr 28, 2026
| raise ArgumentError, "Unsupported participant type: #{participant.class}" | ||
| end | ||
|
|
||
| raise ArgumentError, 'participant is not associated with an assignment or course' if parent.nil? |
Member
There was a problem hiding this comment.
Please remove this statement. It's checking something that will always be false.
efg
reviewed
Apr 28, 2026
| # or {} when the response is nil (e.g. instructor has no response yet). | ||
| def answers_by_item(response) | ||
| return {} unless response | ||
|
|
Member
There was a problem hiding this comment.
Is this loop looping over all the calibration reviewers who reviewed a particular calibration artifact? (Please add a comment.)
There was a problem hiding this comment.
Yes, it loops over the calibration reviewers who reviewed that artifact, but only through their latest submitted response for that map
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.
ruju4a
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Participant management
GET/POST/DELETEcalibration participant endpoints onReviewMappingsController(staff-only, 403 otherwise)Assignment#add_calibration_submitter!atomically createsAssignmentParticipant,AssignmentTeam, and instructorReviewResponseMap(for_calibration: true) in one transactionReport pipeline
ReportsController#calibration— read-only report endpointReports::CalibrationReport— iterator-based service (find_each); computes per-item agree/near/disagree bucket counts without bulk-loading responsesRefactoring
ReviewResponseMap— addedcalibration_forscope,peer_calibration_responses_eachiterator,calibration_participant_jsonserialiserResponseMap#review_status/#latest_submitted_response— status logic moved from controller to modelResponse#answer_for(item)— answer lookup moved to modelQuestionnaire#score_range— score range defaults centralised on the owning modelTests
ResponseMap#review_statustransitionsSummary by CodeRabbit
New Features
Database