E2609: Add calibration participant and review comparison APIs#323
Open
isaacmartin7777-maker wants to merge 12 commits into
Open
E2609: Add calibration participant and review comparison APIs#323isaacmartin7777-maker wants to merge 12 commits into
isaacmartin7777-maker wants to merge 12 commits into
Conversation
Generated by 🚫 Danger |
isaacmartin7777-maker
changed the title
johnmweisz
reviewed
Apr 2, 2026
| case record.role | ||
| when Role::TEACHING_ASSISTANT | ||
| case record.role_id | ||
| when Role::TEACHING_ASSISTANT_ID |
There was a problem hiding this comment.
this is a huge change to include here, how is this related to the requirements?
and I dont see updates to user tests
johnmweisz
reviewed
Apr 2, 2026
| end | ||
| end | ||
| resources :assignments do | ||
| resources :duties, controller: 'assignments_duties', only: %i[index create destroy] |
There was a problem hiding this comment.
The entire file has changed - I have seen this done in many other PRs, while its tempting to fix stuff as you go this is guaranteed to cause merge conflicts that someone else will likely have to deal with.
|
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 non-null boolean Changes
Sequence Diagram(s)sequenceDiagram
participant TeachingStaff as TeachingStaff
participant Controller as ReviewMappingsController
participant Auth as Authorization
participant DB as DB/Models
participant Service as CalibrationPerItemSummary
TeachingStaff->>Controller: GET /assignments/:id/review_mappings/:map_id/calibration_report
Controller->>Auth: authorize teaching_staff_for_calibration_report?(current_user, assignment)
Auth-->>Controller: authorized
Controller->>DB: find ReviewResponseMap(map_id) where for_calibration: true
DB-->>Controller: map record
Controller->>DB: fetch instructor latest submitted response for map
DB-->>Controller: instructor_response
Controller->>DB: fetch latest submitted student responses for same assignment/reviewee (exclude instructor map)
DB-->>Controller: student_responses
Controller->>Service: CalibrationPerItemSummary.build(items, instructor_response, student_responses)
Service-->>Controller: per-item summary
Controller-->>TeachingStaff: 200 OK with calibration_report payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
db/schema.rb (1)
13-13: Excludedb/schema.rbfromStyle/NumericLiteralsin RuboCop config.The schema file is auto-generated; manual numeric-literal formatting will be overwritten on the next migration. Following the existing pattern in
.rubocop.yml, adddb/**/*.rbto the exclusions forStyle/NumericLiterals(or enable the cop and configure it consistently with other metrics already excluded for this directory).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/schema.rb` at line 13, Update the RuboCop configuration to exclude generated schema files from the Style/NumericLiterals cop: add the pattern db/**/*.rb (or specifically db/schema.rb) to the exclusion list for Style/NumericLiterals in .rubocop.yml (matching the existing exclusions style used for other metrics), so the auto-generated ActiveRecord::Schema[8.0].define(...) file isn't flagged or reformatted by RuboCop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db/migrate/20260423010050_add_for_calibration_to_response_maps.rb`:
- Line 3: The code in app/models/review_mapping_handler.rb still references the
removed calibration attribute; update the create/query calls to use the new
for_calibration flag: replace any occurrences like calibration: true with
for_calibration: true (specifically the create call at the spot previously using
calibration: true and the query where(reviewer: reviewer, calibration: true) ->
where(reviewer: reviewer, for_calibration: true)); after changing those
references, add a short data migration to backfill existing rows that should be
calibration-enabled (set for_calibration = true based on the old logic/criteria)
so historical records behave correctly.
---
Nitpick comments:
In `@db/schema.rb`:
- Line 13: Update the RuboCop configuration to exclude generated schema files
from the Style/NumericLiterals cop: add the pattern db/**/*.rb (or specifically
db/schema.rb) to the exclusion list for Style/NumericLiterals in .rubocop.yml
(matching the existing exclusions style used for other metrics), so the
auto-generated ActiveRecord::Schema[8.0].define(...) file isn't flagged or
reformatted by RuboCop.
🪄 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: 32b60095-f0a9-46e1-aa1f-0ec677dabbbe
📒 Files selected for processing (2)
db/migrate/20260423010050_add_for_calibration_to_response_maps.rbdb/schema.rb
| @@ -0,0 +1,6 @@ | |||
| class AddForCalibrationToResponseMaps < ActiveRecord::Migration[8.0] | |||
| def change | |||
| add_column :response_maps, :for_calibration, :boolean, default: false, null: false | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find calibration-related writes/queries on response maps"
rg -n -C2 '\bfor_calibration\b|\bcalibration:\s*true\b|ReviewResponseMap\.(find_or_create_by!?|create!?|new|where)\b|ResponseMap\.(where|find_by|find_or_create_by!?|create!?)\b'
echo
echo "2) Check whether any migration/backfill logic maps old calibration state to for_calibration"
rg -n -C2 'for_calibration|calibration.*for_calibration|UPDATE response_maps|backfill' db/migrate
echo
echo "Expected: calibration creation/query paths consistently use for_calibration, and a backfill exists if legacy rows matter."Repository: expertiza/reimplementation-back-end
Length of output: 16279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Search all migrations for 'calibration' field ==="
find db/migrate -name "*.rb" -exec grep -l "calibration" {} \;
echo
echo "=== 2) Check schema for calibration column ==="
grep -n "calibration" db/schema.rb || echo "No 'calibration' found in schema"
echo
echo "=== 3) Inspect ReviewResponseMap model for attribute aliases/setters ==="
cat app/models/review_response_map.rb 2>/dev/null || echo "ReviewResponseMap file not found at expected location"
echo
echo "=== 4) Check ResponseMap base model ==="
cat app/models/response_map.rb 2>/dev/null || echo "ResponseMap file not found at expected location"
echo
echo "=== 5) Locate and inspect all response_map related migrations ==="
ls -la db/migrate/*response_map* 2>/dev/null | head -20 || echo "No response_map migrations found by pattern"Repository: expertiza/reimplementation-back-end
Length of output: 4761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check initial create_response_maps migration ==="
cat db/migrate/20230424172612_create_response_maps.rb
echo
echo "=== Search codebase for any 'calibration' column references ==="
rg -n "\.calibration|:calibration|\bcalibration\b" app/models/response_map.rb app/models/review_response_map.rb || echo "No calibration references in models"
echo
echo "=== Check if there are any setters/getters for calibration in ApplicationRecord or parent classes ==="
rg -n "calibration" app/models/application_record.rb 2>/dev/null || echo "No application_record.rb or no calibration there"Repository: expertiza/reimplementation-back-end
Length of output: 797
Update all calibration writes and queries to use the new for_calibration flag.
The migration adds for_calibration column but code at app/models/review_mapping_handler.rb lines 74 and 82 still uses the now-nonexistent calibration attribute. This will cause all newly-created calibration maps to silently default to for_calibration = false, breaking downstream visibility and filtering. No backfill exists for any historical records.
Update:
- Line 74: change
calibration: truetofor_calibration: true - Line 82: change
where(reviewer: reviewer, calibration: true)towhere(reviewer: reviewer, for_calibration: true)
If any rows created before this fix need calibration enabled, add a data migration to set for_calibration = true where applicable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrate/20260423010050_add_for_calibration_to_response_maps.rb` at line 3,
The code in app/models/review_mapping_handler.rb still references the removed
calibration attribute; update the create/query calls to use the new
for_calibration flag: replace any occurrences like calibration: true with
for_calibration: true (specifically the create call at the spot previously using
calibration: true and the query where(reviewer: reviewer, calibration: true) ->
where(reviewer: reviewer, for_calibration: true)); after changing those
references, add a short data migration to backfill existing rows that should be
calibration-enabled (set for_calibration = true based on the old logic/criteria)
so historical records behave correctly.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
app/models/review_mapping_handler.rb (1)
84-86: Remove the extra blank line.Static analysis flagged an extra blank line between method definitions.
🧹 Proposed fix
end - - + def calibration_reviews_for(reviewer)🤖 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 84 - 86, Remove the extra blank line between method definitions by collapsing the blank line after the calibration_reviews_for(reviewer) method; locate the calibration_reviews_for method in review_mapping_handler.rb and ensure it directly precedes the next method definition with no stray empty line so that method definitions are consecutive.spec/factories/teams.rb (2)
27-33: Redundant assignment creation between attribute declarations andafter(:build)hook.The factory declares
assignmenton line 27 andparent_idon line 28. Theafter(:build)hook on lines 31-32 repeats the same logic with||=. Since FactoryBot evaluates attributes before callbacks,team.assignmentwill already be set, making the||=branch dead code. However, line 32 unconditionally overwritesparent_id, which is already correctly set on line 28.Consider removing the redundant
after(:build)hook or simplifying it to only handle edge cases where assignment is explicitly passed as nil.♻️ Proposed simplification
assignment { create(:assignment, course: course) } parent_id { assignment.id } - after(:build) do |team, evaluator| - team.assignment ||= create(:assignment, course: evaluator.course) - team.parent_id = team.assignment.id - end + after(:build) do |team, _evaluator| + # Ensure parent_id stays in sync if assignment was overridden + team.parent_id = team.assignment.id if team.assignment + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/factories/teams.rb` around lines 27 - 33, The after(:build) hook is redundant because the attributes assignment { create(:assignment, course: course) } and parent_id { assignment.id } are evaluated before callbacks, so team.assignment will already be set and the ||= branch is dead while parent_id is being overwritten; remove the after(:build) block or change it to only handle explicit nil assignment (e.g., check if evaluator.assignment is nil) so that parent_id is not unconditionally reassigned — update the factory by deleting or simplifying the after(:build) hook that references team.assignment and team.parent_id.
35-40: The:with_assignmenttrait creates a new assignment, overwriting the default.The trait creates a fresh assignment on line 37, which overwrites the assignment already created by line 27. This may be intentional for test isolation, but consider whether you want the trait to be additive or replacement behavior. If the base factory already creates an assignment, this trait may be redundant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/factories/teams.rb` around lines 35 - 40, The :with_assignment trait currently always creates a new Assignment and assigns it to team.assignment (in the after(:build) block), which overwrites any assignment the base factory may have created; update the trait to be additive by only creating/assigning when team.assignment is nil (e.g., wrap create(:assignment, course: evaluator.course) and team.parent_id assignment in an if team.assignment.nil? check), or if replacement behavior is intended, rename the trait to :with_new_assignment and document that it replaces the base assignment; ensure you reference the trait name :with_assignment, the after(:build) hook, team.assignment and team.parent_id when making the change.spec/models/review_mapping_handler_spec.rb (1)
22-43: Consider adding edge case tests.The current test covers the happy path, but you may want to add tests for:
- Empty calibration teams (early return on line 68 of handler)
- Single calibration team with multiple reviewers (wrap-around behavior)
- Idempotency when called multiple times
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/review_mapping_handler_spec.rb` around lines 22 - 43, Add three new spec examples for assign_calibration_reviews_round_robin to cover the edge cases: (1) when there are no calibration teams ensure the method returns early and creates no ReviewResponseMap records (use described_class.new(assignment).assign_calibration_reviews_round_robin and assert ReviewResponseMap.where(...).count remains 0), (2) when there is a single calibration team with multiple reviewers assert wrap-around behavior by creating multiple reviewers and one calibration team and expecting each reviewer to have maps pointing to that single team's id, and (3) idempotency by invoking assign_calibration_reviews_round_robin twice on the same assignment and asserting no duplicate ReviewResponseMap records are created (compare counts before and after or ensure uniqueness of reviewer+reviewee+for_calibration).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/models/review_mapping_handler.rb`:
- Around line 84-86: Remove the extra blank line between method definitions by
collapsing the blank line after the calibration_reviews_for(reviewer) method;
locate the calibration_reviews_for method in review_mapping_handler.rb and
ensure it directly precedes the next method definition with no stray empty line
so that method definitions are consecutive.
In `@spec/factories/teams.rb`:
- Around line 27-33: The after(:build) hook is redundant because the attributes
assignment { create(:assignment, course: course) } and parent_id { assignment.id
} are evaluated before callbacks, so team.assignment will already be set and the
||= branch is dead while parent_id is being overwritten; remove the
after(:build) block or change it to only handle explicit nil assignment (e.g.,
check if evaluator.assignment is nil) so that parent_id is not unconditionally
reassigned — update the factory by deleting or simplifying the after(:build)
hook that references team.assignment and team.parent_id.
- Around line 35-40: The :with_assignment trait currently always creates a new
Assignment and assigns it to team.assignment (in the after(:build) block), which
overwrites any assignment the base factory may have created; update the trait to
be additive by only creating/assigning when team.assignment is nil (e.g., wrap
create(:assignment, course: evaluator.course) and team.parent_id assignment in
an if team.assignment.nil? check), or if replacement behavior is intended,
rename the trait to :with_new_assignment and document that it replaces the base
assignment; ensure you reference the trait name :with_assignment, the
after(:build) hook, team.assignment and team.parent_id when making the change.
In `@spec/models/review_mapping_handler_spec.rb`:
- Around line 22-43: Add three new spec examples for
assign_calibration_reviews_round_robin to cover the edge cases: (1) when there
are no calibration teams ensure the method returns early and creates no
ReviewResponseMap records (use
described_class.new(assignment).assign_calibration_reviews_round_robin and
assert ReviewResponseMap.where(...).count remains 0), (2) when there is a single
calibration team with multiple reviewers assert wrap-around behavior by creating
multiple reviewers and one calibration team and expecting each reviewer to have
maps pointing to that single team's id, and (3) idempotency by invoking
assign_calibration_reviews_round_robin twice on the same assignment and
asserting no duplicate ReviewResponseMap records are created (compare counts
before and after or ensure uniqueness of reviewer+reviewee+for_calibration).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7af20c9e-e6e1-4f0a-89e6-7c25ec8d38ed
📒 Files selected for processing (4)
app/models/review_mapping_handler.rbspec/factories/review_response_maps.rbspec/factories/teams.rbspec/models/review_mapping_handler_spec.rb
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spec/services/calibration_per_item_summary_spec.rb (1)
5-187: Split this spec into smaller contexts/helpers for maintainability.Line 5 through Line 187 exceeds the configured block-length threshold and is getting harder to scan. Consider extracting setup into
let/shared_contextand moving helper builders into support helpers.🤖 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` around lines 5 - 187, The spec file is too large and should be split: extract repeated setup (questionnaire, items, assignment, maps, common responses) into let blocks or a shared_context and move helper factory methods create_response, create_questionnaire, and create_instructor into spec/support helper modules; then create smaller describe/context blocks for the two examples (e.g., "summarizes submitted scores" and "latest submitted response wins") that call those lets/shared_contexts to keep each example focused and reduce the top-level block length while preserving usage of CalibrationPerItemSummary.build, create_response, create_questionnaire, and create_instructor.
🤖 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/services/calibration_per_item_summary.rb`:
- Around line 38-44: The latest_submitted_student_responses method currently
picks the newest response with max_by(&:updated_at) which is non-deterministic
on timestamp ties; update the selection to use a deterministic composite
tiebreaker by comparing updated_at, then version_num, then id (e.g. use max_by
with a tuple/key [updated_at, version_num, id] or equivalent) so tied timestamps
resolve by version_num then id and the result is stable.
- Around line 30-31: The student_response_count is currently set to
latest_student_responses.count which counts responses even when they have no
answer for the current item; change it to count only those
latest_student_responses that actually have an answer for the given item (use
the existing student_answers lookup or derive from bucket_counts_for(item,
latest_student_responses, student_answers) instead of
latest_student_responses.count) so per-item denominators match the item-specific
bucket totals in CalibrationPerItemSummary.
---
Nitpick comments:
In `@spec/services/calibration_per_item_summary_spec.rb`:
- Around line 5-187: The spec file is too large and should be split: extract
repeated setup (questionnaire, items, assignment, maps, common responses) into
let blocks or a shared_context and move helper factory methods create_response,
create_questionnaire, and create_instructor into spec/support helper modules;
then create smaller describe/context blocks for the two examples (e.g.,
"summarizes submitted scores" and "latest submitted response wins") that call
those lets/shared_contexts to keep each example focused and reduce the top-level
block length while preserving usage of CalibrationPerItemSummary.build,
create_response, create_questionnaire, and create_instructor.
🪄 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: 0070deca-1ffc-418f-8e27-2e1de3280de5
📒 Files selected for processing (2)
app/services/calibration_per_item_summary.rbspec/services/calibration_per_item_summary_spec.rb
Comment on lines
+30
to
+31
| bucket_counts: bucket_counts_for(item, latest_student_responses, student_answers), | ||
| student_response_count: latest_student_responses.count |
There was a problem hiding this comment.
Make student_response_count item-specific to avoid skewed per-item stats.
Line 31 counts all latest submitted responses, including those with no answer for the current item. That can produce a denominator larger than the item’s bucket total and mislead agreement/distribution reporting.
Proposed fix
@@
- bucket_counts: bucket_counts_for(item, latest_student_responses, student_answers),
- student_response_count: latest_student_responses.count
+ bucket_counts: bucket_counts_for(item, latest_student_responses, student_answers),
+ student_response_count: student_response_count_for(item, latest_student_responses, student_answers)
}
end
end
@@
+ def student_response_count_for(item, responses, answers_by_response)
+ responses.count do |response|
+ !answers_by_response.fetch(response.id, {}).fetch(item.id, nil)&.answer.nil?
+ end
+ end
+
def bucket_counts_for(item, responses, answers_by_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
| bucket_counts: bucket_counts_for(item, latest_student_responses, student_answers), | |
| student_response_count: latest_student_responses.count | |
| bucket_counts: bucket_counts_for(item, latest_student_responses, student_answers), | |
| student_response_count: student_response_count_for(item, latest_student_responses, student_answers) | |
| } | |
| end | |
| end | |
| def student_response_count_for(item, responses, answers_by_response) | |
| responses.count do |response| | |
| !answers_by_response.fetch(response.id, {}).fetch(item.id, nil)&.answer.nil? | |
| end | |
| end | |
| def bucket_counts_for(item, responses, answers_by_response) | |
| # existing implementation... | |
| 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 30 - 31, The
student_response_count is currently set to latest_student_responses.count which
counts responses even when they have no answer for the current item; change it
to count only those latest_student_responses that actually have an answer for
the given item (use the existing student_answers lookup or derive from
bucket_counts_for(item, latest_student_responses, student_answers) instead of
latest_student_responses.count) so per-item denominators match the item-specific
bucket totals in CalibrationPerItemSummary.
Comment on lines
+38
to
+44
| def latest_submitted_student_responses | ||
| @student_responses | ||
| .select(&:is_submitted) | ||
| .group_by(&:map_id) | ||
| .values | ||
| .map { |responses| responses.max_by(&:updated_at) } | ||
| .compact |
There was a problem hiding this comment.
Stabilize “latest submitted” selection when timestamps tie.
Line 43 uses max_by(&:updated_at) only. If two submitted versions share the same timestamp, selection becomes non-deterministic. Add a deterministic tiebreaker (version_num, then id).
Proposed fix
- .map { |responses| responses.max_by(&:updated_at) }
+ .map do |responses|
+ responses.max_by { |r| [r.updated_at || Time.at(0), r.version_num.to_i, r.id.to_i] }
+ end
.compact🤖 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 38 - 44, The
latest_submitted_student_responses method currently picks the newest response
with max_by(&:updated_at) which is non-deterministic on timestamp ties; update
the selection to use a deterministic composite tiebreaker by comparing
updated_at, then version_num, then id (e.g. use max_by with a tuple/key
[updated_at, version_num, id] or equivalent) so tied timestamps resolve by
version_num then id and the result is stable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spec/requests/api/v1/review_mappings_calibration_report_spec.rb (1)
122-128: Add a TA happy-path authorization example.The controller now has a separate
TaMappingbranch forcalibration_report, but this spec only proves instructor access and student rejection. A passing TA case would lock down the new authorization contract and catch regressions in that path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/api/v1/review_mappings_calibration_report_spec.rb` around lines 122 - 128, Add a happy-path spec that asserts a TA mapping can access calibration_report: create a TaMapping (or reuse a mapping with class/method name TaMapping) tied to a TA user, call get calibration_report_path with headers: auth_headers_for(ta_user) after creating the response (use create_response(map: ta_map, submitted: true, scores: ...)), and expect(response).to have_http_status(:ok); this will validate the controller branch that handles TaMapping and lock down the TA authorization contract.
🤖 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 66-71: Replace the permissive lookup that uses params[:id] with an
explicit lookup for the assignment's instructor calibration map: fetch
ReviewResponseMap where reviewed_object_id == `@assignment.id` and for_calibration
== true and reviewer_id matches the assignment's instructor (e.g.,
`@assignment.instructor_id` or equivalent), then pass that instructor_map into
latest_submitted_response_for; optionally validate that params[:id] matches the
resolved instructor_map.id and fail if it does not so a student map cannot be
used.
- Around line 145-155: submitted_student_responses_for currently returns all
submitted responses for each ReviewResponseMap, causing duplicates; change it to
return only the latest submitted response per student map by, for each map
returned by ReviewResponseMap (identified in this method), selecting the single
most-recent submitted response (e.g., responses.where(is_submitted:
true).order(updated_at: :desc).first) and return a compacted array of those
latest responses so callers like CalibrationPerItemSummary see the same
latest-response data.
---
Nitpick comments:
In `@spec/requests/api/v1/review_mappings_calibration_report_spec.rb`:
- Around line 122-128: Add a happy-path spec that asserts a TA mapping can
access calibration_report: create a TaMapping (or reuse a mapping with
class/method name TaMapping) tied to a TA user, call get calibration_report_path
with headers: auth_headers_for(ta_user) after creating the response (use
create_response(map: ta_map, submitted: true, scores: ...)), and
expect(response).to have_http_status(:ok); this will validate the controller
branch that handles TaMapping and lock down the TA authorization contract.
🪄 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: 0b83559c-393d-4510-a82b-629c1db05f33
📒 Files selected for processing (3)
app/controllers/review_mappings_controller.rbconfig/routes.rbspec/requests/api/v1/review_mappings_calibration_report_spec.rb
Comment on lines
+66
to
+71
| instructor_map = ReviewResponseMap.find_by!( | ||
| id: params[:id], | ||
| reviewed_object_id: @assignment.id, | ||
| for_calibration: true | ||
| ) | ||
| instructor_response = latest_submitted_response_for(instructor_map) |
There was a problem hiding this comment.
Restrict this lookup to the instructor’s calibration map.
This query accepts any calibration ReviewResponseMap for the assignment. If a student map ID is passed here, that student's submission becomes the instructor_response, which produces the wrong comparison report. Resolve the instructor map explicitly from the assignment/reviewee context instead of trusting the requested map ID alone.
🤖 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 66 - 71, Replace
the permissive lookup that uses params[:id] with an explicit lookup for the
assignment's instructor calibration map: fetch ReviewResponseMap where
reviewed_object_id == `@assignment.id` and for_calibration == true and reviewer_id
matches the assignment's instructor (e.g., `@assignment.instructor_id` or
equivalent), then pass that instructor_map into latest_submitted_response_for;
optionally validate that params[:id] matches the resolved instructor_map.id and
fail if it does not so a student map cannot be used.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
spec/requests/api/v1/review_mappings_calibration_report_spec.rb (1)
206-217: Remove redundantupdate!call.The
seqattribute is already set duringItem.create!at line 210. The subsequentupdate!at line 215 is unnecessary.♻️ Suggested fix
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/review_mappings_calibration_report_spec.rb` around lines 206 - 217, Remove the redundant update call in the create_item helper: the Item.create! call in create_item already sets seq, so delete the subsequent item.update!(seq: seq) line (refer to the create_item method and the Item.create! / update! usage) to avoid an unnecessary database update and simplify the helper.app/controllers/review_mappings_controller.rb (3)
163-191: Potential N+1 queries in serialization helpers.
serialize_itemaccessesitem.questionnairefor each item, andserialize_responseaccessesresponse.map.reviewerfor each response. If these associations aren't eager-loaded, this could cause N+1 queries. Consider preloading or caching the questionnaire reference since all items share the same questionnaire.🤖 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 163 - 191, serialize_item and serialize_response can trigger N+1 queries because serialize_item calls item.questionnaire and serialize_response calls response.map.reviewer; fix by ensuring those associations are eager-loaded or passed in: update calling code to preload items with :questionnaire (or pass questionnaire once to serialize_item) and preload responses with map: :reviewer (or pass map.reviewer into serialize_response) so the methods use already-loaded associations instead of triggering queries for each record.
133-139: Use@assignmentinstead of re-querying.
@assignmentis already set bybefore_action :set_assignment. Thefind_byat line 134 duplicates that query.♻️ Suggested fix
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 + 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) + `@assignment.course_id.present`? && TaMapping.exists?(user_id: current_user.id, course_id: `@assignment.course_id`) end🤖 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 133 - 139, The method teaching_staff_for_calibration_report? should use the already-loaded `@assignment` instead of re-querying Assignment.find_by(id: params[:assignment_id]); remove the local assignment = Assignment.find_by(...) line, replace subsequent uses of assignment with `@assignment` (e.g., return false unless user_logged_in? && `@assignment`, return true if `@assignment.instructor_id` == current_user.id, and use `@assignment.course_id` for the TaMapping existence check), and ensure the before_action :set_assignment remains present so `@assignment` is available.
71-72: Split long line for readability.Line 72 exceeds 120 characters due to the modifier
unless. Consider extracting to a guard clause block.♻️ Suggested refactor
instructor_response = latest_submitted_response_for(instructor_map) - return render json: { error: "Submitted instructor calibration response not found" }, status: :unprocessable_entity unless instructor_response + unless instructor_response + return render json: { error: "Submitted instructor calibration response not found" }, status: :unprocessable_entity + end🤖 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 71 - 72, Split the long conditional onto multiple lines by turning the inline modifier into an explicit guard clause: keep the assignment to instructor_response from latest_submitted_response_for(instructor_map), then on the next lines check unless instructor_response and call render json: { error: "Submitted instructor calibration response not found" }, status: :unprocessable_entity followed by an explicit return (or use return render ... on its own line). This preserves the same logic but removes the >120-char modifier usage and improves readability around instructor_response and latest_submitted_response_for.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/controllers/review_mappings_controller.rb`:
- Around line 163-191: serialize_item and serialize_response can trigger N+1
queries because serialize_item calls item.questionnaire and serialize_response
calls response.map.reviewer; fix by ensuring those associations are eager-loaded
or passed in: update calling code to preload items with :questionnaire (or pass
questionnaire once to serialize_item) and preload responses with map: :reviewer
(or pass map.reviewer into serialize_response) so the methods use already-loaded
associations instead of triggering queries for each record.
- Around line 133-139: The method teaching_staff_for_calibration_report? should
use the already-loaded `@assignment` instead of re-querying Assignment.find_by(id:
params[:assignment_id]); remove the local assignment = Assignment.find_by(...)
line, replace subsequent uses of assignment with `@assignment` (e.g., return false
unless user_logged_in? && `@assignment`, return true if `@assignment.instructor_id`
== current_user.id, and use `@assignment.course_id` for the TaMapping existence
check), and ensure the before_action :set_assignment remains present so
`@assignment` is available.
- Around line 71-72: Split the long conditional onto multiple lines by turning
the inline modifier into an explicit guard clause: keep the assignment to
instructor_response from latest_submitted_response_for(instructor_map), then on
the next lines check unless instructor_response and call render json: { error:
"Submitted instructor calibration response not found" }, status:
:unprocessable_entity followed by an explicit return (or use return render ...
on its own line). This preserves the same logic but removes the >120-char
modifier usage and improves readability around instructor_response and
latest_submitted_response_for.
In `@spec/requests/api/v1/review_mappings_calibration_report_spec.rb`:
- Around line 206-217: Remove the redundant update call in the create_item
helper: the Item.create! call in create_item already sets seq, so delete the
subsequent item.update!(seq: seq) line (refer to the create_item method and the
Item.create! / update! usage) to avoid an unnecessary database update and
simplify the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f4d2c71d-c870-4a55-a215-0121afe9b1c3
📒 Files selected for processing (2)
app/controllers/review_mappings_controller.rbspec/requests/api/v1/review_mappings_calibration_report_spec.rb
…k-end into rujuta # Conflicts: # app/controllers/review_mappings_controller.rb
Merging add participant changes
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.
Summary
Why
This PR implements the backend support for calibration assignments. Instructors can add a calibration participant by username, create the gold-standard review for that submission, and provide the frontend with the data needed to display submission content and compare student reviews against the instructor review.
Test Plan
Summary by CodeRabbit