E2601. Reimplement student quizzes#309
Open
MaybeNotArnav wants to merge 44 commits into
Open
Conversation
Generated by 🚫 Danger |
…f them pass! run 'rails test' and you can see all 30 test cases passing!
run 'rails test' to test all files.
…sponse_controller_test.rb
Feature/test cases has the Rspec testing done for the task_ordering/, student_tasks_controller, responses_controller files.
…e in which the guidelines did not specifically tell us to do.
Rspec tests for student_task_controller, responses_controller, task_ordering/... files done.
johnmweisz
reviewed
Apr 1, 2026
| include JwtToken | ||
|
|
||
| prepend_before_action :set_response, only: %i[show update] | ||
| before_action :find_and_authorize_map_for_create, only: %i[create] # changed from prepend_before_action |
There was a problem hiding this comment.
This is a breaking change. ApplicationController is a base controller used by many other controllers, why is this being added here? I would think this throws NoMethodError in various other controllers.
johnmweisz
reviewed
Apr 1, 2026
| production: | ||
| <<: *default | ||
| url: <%= ENV['DATABASE_URL'].gsub('?', '_production?') %> No newline at end of file | ||
| database: reimplementation_back_end_test |
There was a problem hiding this comment.
This is a breaking change, you should not commit to this file.
johnmweisz
reviewed
Apr 1, 2026
| @team_participant = team_participant | ||
| end | ||
|
|
||
| def tasks |
There was a problem hiding this comment.
If you put db calls behind logical methods, you tend to hit the db way more often than you need to. I would need to fully understand how all of this is working together to be confident in this and I dont have the time, but consider looking into how many times the db is hit during a request and if moving the TaskFactory further up the code flow could result in better performance. As is, I'm guessing the db is hit about 3x more than necessary.
For example, could map_ids take tasks as a param or reciever instead of calling the tasks function (which calls the db)?
johnmweisz
reviewed
Apr 1, 2026
| when "show", "update" | ||
| @response && @response.map.reviewer.user_id == current_user.id | ||
| else | ||
| true |
There was a problem hiding this comment.
Nit: Defaulting to true for unknown actions is less secure than defaulting to false (consider this a nitpick ie can dismiss without explanation)
johnmweisz
reviewed
Apr 1, 2026
| Max: 20 | ||
| Exclude: | ||
| - 'db/**/*.rb' | ||
| - "db/**/*.rb" |
There was a problem hiding this comment.
nit: unnecessary diffs in this file
johnmweisz
reviewed
Apr 1, 2026
| ## Development Environment | ||
|
|
||
| ### Prerequisites | ||
|
|
johnmweisz
reviewed
Apr 1, 2026
| end | ||
| resources :assignments do | ||
| resources :duties, controller: 'assignments_duties', only: [:index, :create, :destroy] | ||
| end |
There was a problem hiding this comment.
nit: I noticed the incorrect existing indent as well but resisted the temptation to fix it because there is a high chance this causes conflicts and could block the PR
johnmweisz
reviewed
Apr 1, 2026
| def action_allowed? | ||
| case action_name | ||
| when "create" | ||
| true # auth already handled by prepend_before_action above |
There was a problem hiding this comment.
This comment is misleading. Auth is handled by ApplicationController >> before_action :authorize
prepend_before_action is just calling set_response which is just calling Response.find
johnmweisz
reviewed
Apr 1, 2026
| @@ -0,0 +1,115 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| class ResponsesController < ApplicationController | |||
There was a problem hiding this comment.
Responses is a very generic name, consider something like task_responses_controller instead
There was a problem hiding this comment.
Responses controller is supposed to have other endpoints and functionalities being worked on in another PR that will be merged at some point. For consistency we kept the same file name for now.
|
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 server-side task ordering and response APIs, refactors StudentTask orchestration with quiz/review task items and duty-based permission checks, updates routing/config, and adds comprehensive model/request specs for the new workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as ResponsesController
participant Auth as Auth (current_user)
participant TaskLayer as StudentTask / TaskQueue
participant ResponseMap
participant DB
Client->>Controller: POST /responses { response_map_id, content, round }
Controller->>Auth: authenticate / authorize
Auth-->>Controller: current_user
Controller->>ResponseMap: find by id
ResponseMap-->>Controller: found / not found
Controller->>TaskLayer: enforce_task_order!(response_map, current_user)
TaskLayer-->>DB: check participant context and prior tasks
DB-->>TaskLayer: prior tasks complete? (true/false)
TaskLayer-->>Controller: allow / deny (403/412)
alt allowed
Controller->>DB: find_or_initialize Response by map_id & round
DB-->>Controller: response instance
Controller->>DB: save response (additional_comment, is_submitted)
DB-->>Controller: saved / validation errors
Controller-->>Client: 201 { response_id, map_id, round } or 422 { errors }
else denied
Controller-->>Client: 403/412 { error }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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: 12
♻️ Duplicate comments (1)
config/database.yml (1)
12-14:⚠️ Potential issue | 🟠 MajorKeep the test DB connection env-driven.
This change hard-codes only the database name, but
*defaultdoes not provide host, username, or password. The test environment now ignores theDATABASE_URLpattern still used elsewhere, sobundle exec rspecwill fall back to local MySQL socket defaults and can fail in Docker/CI or on any non-default local setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/database.yml` around lines 12 - 14, The test DB config in config/database.yml currently hardcodes only database: reimplementation_back_end_test under the test section and relies on <<: *default which doesn't supply host/username/password, causing the test env to ignore DATABASE_URL; update the test section to read connection info from environment like DATABASE_URL (or explicitly set adapter/host/username/password via ENV variables) instead of a bare database name so tests use the same env-driven connection (modify the test block and ensure it references *default plus ENV['DATABASE_URL'] or ENV vars for host/user/pass).
🧹 Nitpick comments (1)
spec/models/task_ordering/task_factory_spec.rb (1)
37-70: Add regression specs for.buildedge cases in current branching logic.Current tests don’t cover two high-risk paths:
- nil duty + quiz-only assignment behavior (Line 40 path),
- multiple review maps where only one reviewee has an existing quiz map.
💡 Suggested spec additions
describe '.build' do + context 'with nil duty and quiz questionnaire but no review maps' do + before do + teams_participant.update!(duty_id: nil) + allow(assignment).to receive(:quiz_questionnaire_for_review_flow).and_return(double(id: 123)) + end + + it 'returns a QuizTask' do + tasks = described_class.build(assignment: assignment, team_participant: teams_participant) + expect(tasks.map(&:task_type)).to include(:quiz) + end + end + + context 'with multiple review maps and quiz map for only one reviewee' do + # build two review maps, one matching existing quiz map, one not + # expect quiz task only for the matching reviewee when questionnaire is nil + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/task_ordering/task_factory_spec.rb` around lines 37 - 70, Add two regression specs for TaskOrdering::TaskFactory.build: (1) a context where the team_participant has no duty (ensure teams_participant.update!(duty_id: nil) or leave unset) and the assignment returns a quiz questionnaire (stub assignment.quiz_questionnaire_for_review_flow to a non-nil value) and assert the returned tasks reflect quiz-only behavior (e.g., contain quiz tasks and no review tasks); (2) a context with multiple ReviewResponseMap entries (create two ReviewResponseMap objects with different reviewee_id values) but only one of those reviewees has an associated quiz map (create a quiz map or stub lookup for that reviewee), then call TaskOrdering::TaskFactory.build and assert that tasks include review tasks only for the correct reviewee and that the quiz-aware branching handles the mixed case correctly. Ensure to reuse symbols from the spec (TaskOrdering::TaskFactory.build, teams_participant, assignment, ReviewResponseMap, quiz_questionnaire_for_review_flow) and set up necessary stubs/fixtures in the before blocks.
🤖 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/responses_controller.rb`:
- Around line 82-86: The response_update_params method currently permits :round
which allows clients to change an existing Response's round; remove :round from
the permitted list and ensure update uses only the allowed fields (e.g.,
:is_submitted, :additional_comment), keep the existing
content-to-additional_comment mapping (p[:additional_comment] = p[:content] if
p[:content].present?) and delete :content as before; update any reference in the
update action to call response_update_params unchanged so round cannot be
mass-assigned or rewritten via this endpoint.
In `@app/controllers/student_tasks_controller.rb`:
- Around line 38-41: The GET endpoints in student_tasks_controller are calling
queue.ensure_response_objects! (and next_task likewise), which causes DB writes
on read; remove these destructive calls from the read actions and replace them
with a non-mutating check or preload (e.g., a new method like
response_objects_present? or preload_responses_without_create) that only queries
for existing ResponseMap/Response rows, and move the responsibility to create
missing ResponseMap/Response rows into the task-assignment/create flow (or a
POST endpoint) so reads remain read-only; update references to
ensure_response_objects! in the queue and next_task code paths to use the new
non-writing predicate/preload method instead.
- Around line 75-79: start_task currently calls
TeamsParticipant.find_by(participant_id: participant.id) and passes the result
directly into TaskOrdering::TaskQueue.new, which raises a 500 for reviewers
without a team; guard this by checking the found team_participant before
building the queue and return the same controlled error/flow used elsewhere
(e.g., as in build_queue_for_user and ResponsesController#enforce_task_order!) —
if team_participant is nil, short-circuit with the appropriate error response or
redirect, otherwise construct TaskOrdering::TaskQueue.new(assignment,
team_participant) as before.
- Around line 105-110: The Participant lookup in build_queue_for_user is
ambiguous because parent_id can refer to courses or assignments; scope the query
to assignment participants by either querying through the Assignment model or
adding the parent_type constraint. For example, replace the current
Participant.find_by(user_id: current_user.id, parent_id: assignment_id) with
Assignment.find(assignment_id).participants.find_by(user_id: current_user.id) or
Participant.find_by(user_id: current_user.id, parent_id: assignment_id,
parent_type: 'Assignment') so only assignment participants are returned.
In `@app/models/assignment.rb`:
- Around line 23-27: The helper review_questionnaire_for_review_flow collapses
multi-round questionnaires by using questionnaires.find_by(...) and ignores
AssignmentQuestionnaire.used_in_round; update this method to select the
questionnaire whose join record AssignmentQuestionnaire.used_in_round matches
the current review round (or accept a round parameter), using the
AssignmentQuestionnaire association or join to filter by used_in_round when
varying_rubrics_by_round? is true and fall back to the original behavior when
not; reference the review_questionnaire_for_review_flow method, the
AssignmentQuestionnaire.used_in_round attribute, and the existing
varying_rubrics_by_round? / review_rounds helpers so the correct round-specific
questionnaire is returned.
In `@app/models/student_task.rb`:
- Around line 34-38: The method StudentTask.from_participant_id currently
returns nil for a missing Participant which causes controllers like
StudentTasksController#view to render 200 with a null body; change the behavior
to surface a not-found error instead: in from_participant_id, replace the
"return nil unless part" with raising ActiveRecord::RecordNotFound (including
the id in the message) so callers receive a 404, and leave
create_from_participant(part) unchanged for the success path.
- Around line 16-25: The factory create_from_participant is currently building
StudentTask records for non-assignment participants (course memberships) because
it uses participant.assignment&.name; fix by guarding early in
create_from_participant: return nil (or skip) unless
participant.assignment.present? before constructing the new StudentTask, keep
using send(:parse_stage_deadline, participant.stage_deadline) as-is, and apply
the same presence check to the other similar constructor/usage referenced at
lines 28-31 so callers can compact the results to drop nils.
In `@app/models/task_ordering/quiz_task.rb`:
- Around line 17-21: Lookup for an existing QuizResponseMap is missing the
reviewed_object_id filter and may return a map from another questionnaire;
update the find_by in the block that assigns existing (and the similar block
around lines 26-31) to include reviewed_object_id:
review_map&.reviewed_object_id || 0 so the query matches reviewer_id,
reviewee_id and the questionnaire context before returning or creating a new
QuizResponseMap.
In `@app/models/task_ordering/task_factory.rb`:
- Line 40: In TaskFactory (app/models/task_ordering/task_factory.rb) the quiz
branch uses allows_quiz?(duty) which rejects nil duties while another branch
treats duty.nil? as allowed; update the quiz branch condition to accept nil
duties as well (e.g., make the condition duty.nil? || allows_quiz?(duty)
combined appropriately with the existing quiz_questionnaire check) so quiz-only
tasks are not dropped when duty is unset.
- Around line 17-25: The code computes a reviewer-wide has_existing_quiz_maps
flag and uses it for every review_map, which allows creating QuizTask objects
for review_maps that have no per-reviewee quiz map (nil), causing blocked
progression; change the gating so that before instantiating a QuizTask for each
review_map you check for a per-review_map quiz existence (e.g., query
QuizResponseMap for reviewer_id: team_participant.participant_id and the
specific reviewee_id or inspect an existing map on the review_map) and only
create QuizTask when either quiz_questionnaire is present OR that specific
review_map has an existing quiz; ensure QuizTask is never created with a nil map
and keep existing allows_quiz?(duty) and duty checks.
In `@spec/requests/api/v1/student_tasks_controller_spec.rb`:
- Around line 124-129: The spec currently asserts a 500 for a missing
participant which masks a lookup failure; update the example to expect a 404 by
changing the response description from '500' to '404' and update the expectation
inside the block (expect(response.status).to eq(404)); also update the example
description string (e.g., 'participant not found returns 404') so the test
documents the correct client-facing behavior for the participant lookup failure.
- Around line 240-245: The current example mixes success and forbidden outcomes
by asserting expect([200, 403]) inside the response '200' block; split this into
two explicit examples: one response '200' example that sends let(:body) { {
response_map_id: review_map.id } } with the setup that guarantees the task
starts and asserts expect(response.status).to eq(200), and a separate response
'403' example that configures the test to trigger the queue-ordering guard
(adjust whatever preconditions are needed) and asserts
expect(response.status).to eq(403); keep using the same let(:body) and run_test!
invocation but separate the contexts so the 200 path and the forbidden path are
independently verified.
---
Duplicate comments:
In `@config/database.yml`:
- Around line 12-14: The test DB config in config/database.yml currently
hardcodes only database: reimplementation_back_end_test under the test section
and relies on <<: *default which doesn't supply host/username/password, causing
the test env to ignore DATABASE_URL; update the test section to read connection
info from environment like DATABASE_URL (or explicitly set
adapter/host/username/password via ENV variables) instead of a bare database
name so tests use the same env-driven connection (modify the test block and
ensure it references *default plus ENV['DATABASE_URL'] or ENV vars for
host/user/pass).
---
Nitpick comments:
In `@spec/models/task_ordering/task_factory_spec.rb`:
- Around line 37-70: Add two regression specs for
TaskOrdering::TaskFactory.build: (1) a context where the team_participant has no
duty (ensure teams_participant.update!(duty_id: nil) or leave unset) and the
assignment returns a quiz questionnaire (stub
assignment.quiz_questionnaire_for_review_flow to a non-nil value) and assert the
returned tasks reflect quiz-only behavior (e.g., contain quiz tasks and no
review tasks); (2) a context with multiple ReviewResponseMap entries (create two
ReviewResponseMap objects with different reviewee_id values) but only one of
those reviewees has an associated quiz map (create a quiz map or stub lookup for
that reviewee), then call TaskOrdering::TaskFactory.build and assert that tasks
include review tasks only for the correct reviewee and that the quiz-aware
branching handles the mixed case correctly. Ensure to reuse symbols from the
spec (TaskOrdering::TaskFactory.build, teams_participant, assignment,
ReviewResponseMap, quiz_questionnaire_for_review_flow) and set up necessary
stubs/fixtures in the before blocks.
🪄 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: 331cd541-3266-44aa-b74c-90b572bfe07b
📒 Files selected for processing (21)
.rubocop.ymlREADME.mdapp/controllers/application_controller.rbapp/controllers/responses_controller.rbapp/controllers/student_tasks_controller.rbapp/models/assignment.rbapp/models/student_task.rbapp/models/task_ordering/base_task.rbapp/models/task_ordering/quiz_task.rbapp/models/task_ordering/review_task.rbapp/models/task_ordering/task_factory.rbapp/models/task_ordering/task_queue.rbconfig/database.ymlconfig/routes.rbspec/models/task_ordering/base_task_spec.rbspec/models/task_ordering/quiz_task_spec.rbspec/models/task_ordering/review_task_spec.rbspec/models/task_ordering/task_factory_spec.rbspec/models/task_ordering/task_queue_spec.rbspec/requests/api/v1/responses_controller_spec.rbspec/requests/api/v1/student_tasks_controller_spec.rb
Comment on lines
+82
to
+86
| def response_update_params | ||
| p = params.permit(:is_submitted, :additional_comment, :content, :round) | ||
| p[:additional_comment] = p[:content] if p[:content].present? | ||
| p.delete(:content) | ||
| p |
There was a problem hiding this comment.
Do not let update rewrite the response round.
update is acting on an already-selected Response, but these strong params still permit :round. That lets a client move an existing submission into another round after the queue/order checks have passed.
Proposed fix
def response_update_params
- p = params.permit(:is_submitted, :additional_comment, :content, :round)
+ p = params.permit(:is_submitted, :additional_comment, :content)
p[:additional_comment] = p[:content] if p[:content].present?
p.delete(:content)
p
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 response_update_params | |
| p = params.permit(:is_submitted, :additional_comment, :content, :round) | |
| p[:additional_comment] = p[:content] if p[:content].present? | |
| p.delete(:content) | |
| p | |
| def response_update_params | |
| p = params.permit(:is_submitted, :additional_comment, :content) | |
| p[:additional_comment] = p[:content] if p[:content].present? | |
| p.delete(:content) | |
| p | |
| end |
🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 82-82: Expected 1 empty line between method definitions; found 2.
(Layout/EmptyLineBetweenDefs)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/responses_controller.rb` around lines 82 - 86, The
response_update_params method currently permits :round which allows clients to
change an existing Response's round; remove :round from the permitted list and
ensure update uses only the allowed fields (e.g., :is_submitted,
:additional_comment), keep the existing content-to-additional_comment mapping
(p[:additional_comment] = p[:content] if p[:content].present?) and delete
:content as before; update any reference in the update action to call
response_update_params unchanged so round cannot be mass-assigned or rewritten
via this endpoint.
Comment on lines
+38
to
+41
| # Ensure all ResponseMaps and Responses exist before returning tasks. | ||
| queue.ensure_response_objects! | ||
|
|
||
| render json: queue.tasks.map(&:to_task_hash), status: :ok |
There was a problem hiding this comment.
Keep the GET endpoints read-only.
queue and next_task both call ensure_response_objects!, so a read request now creates ResponseMap/Response rows. That makes these endpoints unsafe for retries, prefetchers, and caches.
Also applies to: 49-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/student_tasks_controller.rb` around lines 38 - 41, The GET
endpoints in student_tasks_controller are calling queue.ensure_response_objects!
(and next_task likewise), which causes DB writes on read; remove these
destructive calls from the read actions and replace them with a non-mutating
check or preload (e.g., a new method like response_objects_present? or
preload_responses_without_create) that only queries for existing
ResponseMap/Response rows, and move the responsibility to create missing
ResponseMap/Response rows into the task-assignment/create flow (or a POST
endpoint) so reads remain read-only; update references to
ensure_response_objects! in the queue and next_task code paths to use the new
non-writing predicate/preload method instead.
Comment on lines
+23
to
+27
| # Returns the assignment-linked review questionnaire record. | ||
| # The assignment can be linked to many questionnaires via AssignmentQuestionnaire. | ||
| def review_questionnaire_for_review_flow | ||
| questionnaires.find_by(questionnaire_type: 'ReviewQuestionnaire') | ||
| end |
There was a problem hiding this comment.
Don't collapse multi-round reviews to the first questionnaire.
This helper ignores AssignmentQuestionnaire.used_in_round, even though this model already supports round-specific rubrics later via varying_rubrics_by_round? and review_rounds. On assignments with more than one review questionnaire, find_by can bind a review task to the wrong rubric/questions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/assignment.rb` around lines 23 - 27, The helper
review_questionnaire_for_review_flow collapses multi-round questionnaires by
using questionnaires.find_by(...) and ignores
AssignmentQuestionnaire.used_in_round; update this method to select the
questionnaire whose join record AssignmentQuestionnaire.used_in_round matches
the current review round (or accept a round parameter), using the
AssignmentQuestionnaire association or join to filter by used_in_round when
varying_rubrics_by_round? is true and fall back to the original behavior when
not; reference the review_questionnaire_for_review_flow method, the
AssignmentQuestionnaire.used_in_round attribute, and the existing
varying_rubrics_by_round? / review_rounds helpers so the correct round-specific
questionnaire is returned.
Comment on lines
+17
to
+21
| existing = QuizResponseMap.find_by( | ||
| reviewer_id: team_participant.participant_id, | ||
| reviewee_id: review_map&.reviewee_id || 0 | ||
| ) | ||
| return @response_map = existing if existing |
There was a problem hiding this comment.
Scope existing quiz-map lookup to the current questionnaire context.
On Line 17, lookup ignores reviewed_object_id, but Line 29 sets it when creating new records. This can reuse an unrelated QuizResponseMap for the same reviewer/reviewee pair.
💡 Proposed fix
- existing = QuizResponseMap.find_by(
- reviewer_id: team_participant.participant_id,
- reviewee_id: review_map&.reviewee_id || 0
- )
+ scope = QuizResponseMap.where(
+ reviewer_id: team_participant.participant_id,
+ reviewee_id: review_map&.reviewee_id || 0
+ )
+ scope = scope.where(reviewed_object_id: questionnaire.id) if questionnaire.present?
+ existing = scope.first
return `@response_map` = existing if existingAlso applies to: 26-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/task_ordering/quiz_task.rb` around lines 17 - 21, Lookup for an
existing QuizResponseMap is missing the reviewed_object_id filter and may return
a map from another questionnaire; update the find_by in the block that assigns
existing (and the similar block around lines 26-31) to include
reviewed_object_id: review_map&.reviewed_object_id || 0 so the query matches
reviewer_id, reviewee_id and the questionnaire context before returning or
creating a new QuizResponseMap.
Comment on lines
+17
to
+25
| has_existing_quiz_maps = QuizResponseMap.where( | ||
| reviewer_id: team_participant.participant_id | ||
| ).exists? | ||
|
|
||
| if review_maps.any? | ||
| review_maps.each do |review_map| | ||
| # Add QuizTask if duty allows quizzes AND (questionnaire exists OR quiz maps already exist) | ||
| if (duty.nil? || allows_quiz?(duty)) && (quiz_questionnaire || has_existing_quiz_maps) | ||
| tasks << QuizTask.new( |
There was a problem hiding this comment.
Quiz-task gating is too broad and can enqueue non-actionable tasks.
Line 17 computes one reviewer-wide flag, then Line 24 applies it to every review_map. If only some reviewees have quiz maps (and no questionnaire exists), extra QuizTasks can be added with nil maps, which then block ordered progression.
💡 Proposed fix
- # Check if any QuizResponseMaps exist for this participant
- has_existing_quiz_maps = QuizResponseMap.where(
- reviewer_id: team_participant.participant_id
- ).exists?
-
if review_maps.any?
review_maps.each do |review_map|
+ has_quiz_map_for_reviewee = QuizResponseMap.where(
+ reviewer_id: team_participant.participant_id,
+ reviewee_id: review_map.reviewee_id
+ ).exists?
+
# Add QuizTask if duty allows quizzes AND (questionnaire exists OR quiz maps already exist)
- if (duty.nil? || allows_quiz?(duty)) && (quiz_questionnaire || has_existing_quiz_maps)
+ if (duty.nil? || allows_quiz?(duty)) && (quiz_questionnaire.present? || has_quiz_map_for_reviewee)
tasks << QuizTask.new(
assignment: assignment,
team_participant: team_participant,
review_map: review_map
)📝 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
| has_existing_quiz_maps = QuizResponseMap.where( | |
| reviewer_id: team_participant.participant_id | |
| ).exists? | |
| if review_maps.any? | |
| review_maps.each do |review_map| | |
| # Add QuizTask if duty allows quizzes AND (questionnaire exists OR quiz maps already exist) | |
| if (duty.nil? || allows_quiz?(duty)) && (quiz_questionnaire || has_existing_quiz_maps) | |
| tasks << QuizTask.new( | |
| if review_maps.any? | |
| review_maps.each do |review_map| | |
| has_quiz_map_for_reviewee = QuizResponseMap.where( | |
| reviewer_id: team_participant.participant_id, | |
| reviewee_id: review_map.reviewee_id | |
| ).exists? | |
| # Add QuizTask if duty allows quizzes AND (questionnaire exists OR quiz maps already exist) | |
| if (duty.nil? || allows_quiz?(duty)) && (quiz_questionnaire.present? || has_quiz_map_for_reviewee) | |
| tasks << QuizTask.new( |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/task_ordering/task_factory.rb` around lines 17 - 25, The code
computes a reviewer-wide has_existing_quiz_maps flag and uses it for every
review_map, which allows creating QuizTask objects for review_maps that have no
per-reviewee quiz map (nil), causing blocked progression; change the gating so
that before instantiating a QuizTask for each review_map you check for a
per-review_map quiz existence (e.g., query QuizResponseMap for reviewer_id:
team_participant.participant_id and the specific reviewee_id or inspect an
existing map on the review_map) and only create QuizTask when either
quiz_questionnaire is present OR that specific review_map has an existing quiz;
ensure QuizTask is never created with a nil map and keep existing
allows_quiz?(duty) and duty checks.
| ) | ||
| end | ||
| end | ||
| elsif allows_quiz?(duty) && quiz_questionnaire |
There was a problem hiding this comment.
Nil-duty handling is inconsistent between branches.
Line 24 treats duty.nil? as allowed, but Line 40 requires allows_quiz?(duty) (false for nil). This can drop valid quiz-only tasks when duty is unset.
💡 Proposed fix
- elsif allows_quiz?(duty) && quiz_questionnaire
+ elsif (duty.nil? || allows_quiz?(duty)) && quiz_questionnaire.present?📝 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
| elsif allows_quiz?(duty) && quiz_questionnaire | |
| elsif (duty.nil? || allows_quiz?(duty)) && quiz_questionnaire.present? |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/task_ordering/task_factory.rb` at line 40, In TaskFactory
(app/models/task_ordering/task_factory.rb) the quiz branch uses
allows_quiz?(duty) which rejects nil duties while another branch treats
duty.nil? as allowed; update the quiz branch condition to accept nil duties as
well (e.g., make the condition duty.nil? || allows_quiz?(duty) combined
appropriately with the existing quiz_questionnaire check) so quiz-only tasks are
not dropped when duty is unset.
Comment on lines
+124
to
+129
| response '500', 'participant not found returns error' do | ||
| let(:id) { -1 } | ||
| run_test! do |response| | ||
| expect(response.status).to eq(500) | ||
| end | ||
| end |
There was a problem hiding this comment.
Don't lock a missing participant into a 500 contract.
A nonexistent participant is a lookup failure, not a server error. Keeping this expectation means the spec passes only when the endpoint blows up instead of returning a proper client-facing error such as 404.
💡 Suggested expectation change
- response '500', 'participant not found returns error' do
+ response '404', 'participant not found returns error' do
let(:id) { -1 }
run_test! do |response|
- expect(response.status).to eq(500)
+ expect(response.status).to eq(404)
end
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
| response '500', 'participant not found returns error' do | |
| let(:id) { -1 } | |
| run_test! do |response| | |
| expect(response.status).to eq(500) | |
| end | |
| end | |
| response '404', 'participant not found returns error' do | |
| let(:id) { -1 } | |
| run_test! do |response| | |
| expect(response.status).to eq(404) | |
| end | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/requests/api/v1/student_tasks_controller_spec.rb` around lines 124 -
129, The spec currently asserts a 500 for a missing participant which masks a
lookup failure; update the example to expect a 404 by changing the response
description from '500' to '404' and update the expectation inside the block
(expect(response.status).to eq(404)); also update the example description string
(e.g., 'participant not found returns 404') so the test documents the correct
client-facing behavior for the participant lookup failure.
Comment on lines
+240
to
+245
| response '200', 'task started or blocked by queue ordering' do | ||
| let(:body) { { response_map_id: review_map.id } } | ||
| run_test! do |response| | ||
| expect([200, 403]).to include(response.status) | ||
| end | ||
| end |
There was a problem hiding this comment.
Split the forbidden path out of the success case.
This example passes whether the task starts correctly or the queue-ordering guard rejects it. That masks regressions in setup and makes the 200-path effectively untested.
💡 Suggested test split
- response '200', 'task started or blocked by queue ordering' do
+ response '200', 'task started' do
let(:body) { { response_map_id: review_map.id } }
run_test! do |response|
- expect([200, 403]).to include(response.status)
+ expect(response.status).to eq(200)
end
- end
+ end
+
+ response '403', 'task blocked by queue ordering' do
+ # create an earlier incomplete task in the queue here
+ end🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 244-244: Trailing whitespace detected.
(Layout/TrailingWhitespace)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/requests/api/v1/student_tasks_controller_spec.rb` around lines 240 -
245, The current example mixes success and forbidden outcomes by asserting
expect([200, 403]) inside the response '200' block; split this into two explicit
examples: one response '200' example that sends let(:body) { { response_map_id:
review_map.id } } with the setup that guarantees the task starts and asserts
expect(response.status).to eq(200), and a separate response '403' example that
configures the test to trigger the queue-ordering guard (adjust whatever
preconditions are needed) and asserts expect(response.status).to eq(403); keep
using the same let(:body) and run_test! invocation but separate the contexts so
the 200 path and the forbidden path are independently verified.
Refactor of the previous Re-implementation
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
app/controllers/responses_controller.rb (1)
82-86:⚠️ Potential issue | 🟠 MajorDo not let
updaterewrite the response round.
updatealready targets a concreteResponse. Permitting:roundlets a client move that row into another round after the queue/order checks have passed.Proposed fix
def response_update_params - p = params.permit(:is_submitted, :additional_comment, :content, :round) + p = params.permit(:is_submitted, :additional_comment, :content) p[:additional_comment] = p[:content] if p[:content].present? p.delete(:content) p end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/responses_controller.rb` around lines 82 - 86, The response_update_params method currently permits :round which allows changing a Response's round during update; remove :round from the permitted list in response_update_params (so only permit :is_submitted, :additional_comment, :content) and ensure no code in update or Response model assigns params[:round]; keep the existing logic that maps :content into :additional_comment and deletes :content so updates cannot alter the round via response_update_params.app/models/student_task.rb (3)
34-38:⚠️ Potential issue | 🟠 MajorMissing participants now become
200 null.Returning
nilhere makesStudentTasksController#viewrender a successful empty payload for an unknown participant. RaiseActiveRecord::RecordNotFoundor have the controller translate the miss into a 404 instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/student_task.rb` around lines 34 - 38, The method from_participant_id currently returns nil for missing participants causing controllers (e.g., StudentTasksController#view) to respond 200 with null; change it to raise ActiveRecord::RecordNotFound when the Participant is not found (either by replacing Participant.find_by(id: id) with Participant.find(id) or by raising ActiveRecord::RecordNotFound with a clear message) so callers get a 404 unless the controller explicitly rescues and handles the missing record; update or add tests for create_from_participant/from_participant_id behavior accordingly.
41-45:⚠️ Potential issue | 🟠 MajorScope this lookup to assignment participants.
Participant.find_by(user_id:, parent_id:)is ambiguous because course and assignment participants can shareparent_id. This can resolve the wrong participant and build the wrong queue. Constrain the query by STI type or query through the assignment’s participants.One safe option
def self.resolve_context_for_assignment(user, assignment_id) participant = Participant.find_by( user_id: user.id, - parent_id: assignment_id + parent_id: assignment_id, + type: 'AssignmentParticipant' ) return nil unless participant🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/student_task.rb` around lines 41 - 45, In resolve_context_for_assignment, the Participant.find_by(user_id: user.id, parent_id: assignment_id) lookup is ambiguous; restrict it to assignment participants by querying through the assignment or adding the STI type. Replace the current lookup with something like fetching the Assignment (e.g. Assignment.find(assignment_id)) and calling .participants.find_by(user_id: user.id) or include type: 'AssignmentParticipant' in the find_by criteria so the resolved participant is definitely the assignment participant.
16-25:⚠️ Potential issue | 🟠 MajorExclude non-assignment participants from task listings.
Participant.where(user_id: user.id)can include course participants here, andcreate_from_participantwill still serialize them withassignment: nilwhile exposingparticipant.parent_idasassignment_id. Filter to assignment participants, and skip anything withoutparticipant.assignment.Proposed fix
def self.create_from_participant(participant) + return nil unless participant.assignment.present? + new( assignment: participant.assignment&.name, assignment_id: participant.parent_id, @@ def self.from_user(user) - Participant.where(user_id: user.id) - .map { |p| create_from_participant(p) } + Participant.where(user_id: user.id, type: 'AssignmentParticipant') + .map { |p| create_from_participant(p) } + .compact .sort_by(&:stage_deadline) endAlso applies to: 28-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/student_task.rb` around lines 16 - 25, The factory method create_from_participant is serializing non-assignment participants (giving assignment: nil but assignment_id from parent_id); add a guard at the start of create_from_participant to return nil (or raise) unless participant.assignment is present, so only assignment participants are serialized, and apply the same guard to the duplicate block around the create_from_participant usage at lines 28-31; update callers (where they map participants) to compact/compact_map the results so nils are dropped if you choose to return nil.app/controllers/student_tasks_controller.rb (1)
18-25:⚠️ Potential issue | 🟠 MajorKeep
queueandnext_taskread-only.
StudentTask.ensure_response_objects!writesResponseMap/Responserows, so these GET endpoints are no longer safe for retries, prefetchers, or caches. Creation should happen in an explicit write path such asstart_taskorResponsesController#create.Also applies to: 28-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/student_tasks_controller.rb` around lines 18 - 25, The queue (and next_task) actions are performing writes via StudentTask.ensure_response_objects!, making GET/read endpoints unsafe; move the creation of ResponseMap/Response out of queue/next_task into an explicit write path: remove the call to StudentTask.ensure_response_objects! from the queue and next_task controllers and instead invoke that creation from a write action such as start_task or ResponsesController#create (or introduce a new start_task endpoint that calls StudentTask.ensure_response_objects! and returns the task payload), ensuring queue/next_task only build and return tasks (e.g., use StudentTask.build_tasks and map to_h) without persisting any response records.
🤖 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/models/student_task.rb`:
- Around line 74-80: The code currently uses has_existing_quiz_maps (computed
once per reviewer) to decide for every review_map whether to create a
QuizTaskItem, causing tasks to be created for reviewees that have no quiz
response_map; change the check to be per-review_map: inside review_maps.each,
query for a quiz response map specific to that pair (e.g. use
QuizResponseMap.find_by(reviewer_id: participant.id, reviewee_id:
review_map.reviewee_id) or .where(...).exists?) and only build a QuizTaskItem
when quiz_questionnaire is present or that per-review_map query returns true
(and prefer to fetch the actual response_map and ensure it’s non-nil before
using it in QuizTaskItem) so tasks are only created for reviewees that actually
have quiz maps.
In `@app/models/student_task/quiz_task_item.rb`:
- Around line 12-30: The response_map accessor currently creates and persists a
QuizResponseMap; change response_map to be read-only by returning an existing
QuizResponseMap lookup (using QuizResponseMap.find_by with
reviewer_id/team_participant.participant_id and
reviewee_id/review_map&.reviewee_id || 0) or nil, and remove any creation/saving
logic from it. Add a new method ensure_response_map! that builds and persists
the QuizResponseMap (using the same attributes: reviewer_id, reviewee_id,
reviewed_object_id: questionnaire.id, type: "QuizResponseMap"), assigns it to
`@response_map`, and saves it (validate: false) so callers that need creation can
call ensure_response_map! instead; keep callers like
StudentTask::BaseTaskItem#completed?, `#to_h` and StudentTask.find_task_for_map
using response_map for read-only access only.
---
Duplicate comments:
In `@app/controllers/responses_controller.rb`:
- Around line 82-86: The response_update_params method currently permits :round
which allows changing a Response's round during update; remove :round from the
permitted list in response_update_params (so only permit :is_submitted,
:additional_comment, :content) and ensure no code in update or Response model
assigns params[:round]; keep the existing logic that maps :content into
:additional_comment and deletes :content so updates cannot alter the round via
response_update_params.
In `@app/controllers/student_tasks_controller.rb`:
- Around line 18-25: The queue (and next_task) actions are performing writes via
StudentTask.ensure_response_objects!, making GET/read endpoints unsafe; move the
creation of ResponseMap/Response out of queue/next_task into an explicit write
path: remove the call to StudentTask.ensure_response_objects! from the queue and
next_task controllers and instead invoke that creation from a write action such
as start_task or ResponsesController#create (or introduce a new start_task
endpoint that calls StudentTask.ensure_response_objects! and returns the task
payload), ensuring queue/next_task only build and return tasks (e.g., use
StudentTask.build_tasks and map to_h) without persisting any response records.
In `@app/models/student_task.rb`:
- Around line 34-38: The method from_participant_id currently returns nil for
missing participants causing controllers (e.g., StudentTasksController#view) to
respond 200 with null; change it to raise ActiveRecord::RecordNotFound when the
Participant is not found (either by replacing Participant.find_by(id: id) with
Participant.find(id) or by raising ActiveRecord::RecordNotFound with a clear
message) so callers get a 404 unless the controller explicitly rescues and
handles the missing record; update or add tests for
create_from_participant/from_participant_id behavior accordingly.
- Around line 41-45: In resolve_context_for_assignment, the
Participant.find_by(user_id: user.id, parent_id: assignment_id) lookup is
ambiguous; restrict it to assignment participants by querying through the
assignment or adding the STI type. Replace the current lookup with something
like fetching the Assignment (e.g. Assignment.find(assignment_id)) and calling
.participants.find_by(user_id: user.id) or include type: 'AssignmentParticipant'
in the find_by criteria so the resolved participant is definitely the assignment
participant.
- Around line 16-25: The factory method create_from_participant is serializing
non-assignment participants (giving assignment: nil but assignment_id from
parent_id); add a guard at the start of create_from_participant to return nil
(or raise) unless participant.assignment is present, so only assignment
participants are serialized, and apply the same guard to the duplicate block
around the create_from_participant usage at lines 28-31; update callers (where
they map participants) to compact/compact_map the results so nils are dropped if
you choose to return nil.
🪄 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: e2860cc5-e691-465d-956b-5dc079f113dc
📒 Files selected for processing (7)
app/controllers/responses_controller.rbapp/controllers/student_tasks_controller.rbapp/models/assignment.rbapp/models/student_task.rbapp/models/student_task/quiz_task_item.rbapp/models/student_task/review_task_item.rbapp/models/teams_participant.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/assignment.rb
Comment on lines
+74
to
+80
| quiz_questionnaire = assignment.quiz_questionnaire_for_review_flow | ||
| has_existing_quiz_maps = QuizResponseMap.where(reviewer_id: participant.id).exists? | ||
|
|
||
| if review_maps.any? | ||
| review_maps.each do |review_map| | ||
| if (duty.nil? || team_participant.allows_quiz?) && (quiz_questionnaire || has_existing_quiz_maps) | ||
| tasks << QuizTaskItem.new( |
There was a problem hiding this comment.
Do not fan out quiz tasks from an unrelated existing quiz map.
has_existing_quiz_maps is computed once per reviewer, then every review_map gets a quiz task when it is true. If only some reviewees already have quiz maps and quiz_questionnaire is nil, this will enqueue quiz tasks whose response_map resolves to nil, so they show up in the queue but cannot be started.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/student_task.rb` around lines 74 - 80, The code currently uses
has_existing_quiz_maps (computed once per reviewer) to decide for every
review_map whether to create a QuizTaskItem, causing tasks to be created for
reviewees that have no quiz response_map; change the check to be per-review_map:
inside review_maps.each, query for a quiz response map specific to that pair
(e.g. use QuizResponseMap.find_by(reviewer_id: participant.id, reviewee_id:
review_map.reviewee_id) or .where(...).exists?) and only build a QuizTaskItem
when quiz_questionnaire is present or that per-review_map query returns true
(and prefer to fetch the actual response_map and ensure it’s non-nil before
using it in QuizTaskItem) so tasks are only created for reviewees that actually
have quiz maps.
Comment on lines
+12
to
+30
| def response_map | ||
| return @response_map if @response_map | ||
|
|
||
| existing_map = QuizResponseMap.find_by( | ||
| reviewer_id: team_participant.participant_id, | ||
| reviewee_id: review_map&.reviewee_id || 0 | ||
| ) | ||
| return @response_map = existing_map if existing_map | ||
|
|
||
| return nil if questionnaire.nil? | ||
|
|
||
| attributes = { | ||
| reviewer_id: team_participant.participant_id, | ||
| reviewee_id: review_map&.reviewee_id || 0, | ||
| reviewed_object_id: questionnaire.id, | ||
| type: "QuizResponseMap" | ||
| } | ||
|
|
||
| @response_map = QuizResponseMap.new(attributes).tap { |map| map.save!(validate: false) } |
There was a problem hiding this comment.
Keep response_map read-only.
StudentTask::BaseTaskItem#completed?, #to_h, and StudentTask.find_task_for_map all call response_map. Persisting a QuizResponseMap from this accessor means read paths can create rows just by checking completion or serializing tasks. Move the create/save path into ensure_response_map! and keep response_map query-only.
Suggested split
def response_map
- return `@response_map` if `@response_map`
-
- existing_map = QuizResponseMap.find_by(
- reviewer_id: team_participant.participant_id,
- reviewee_id: review_map&.reviewee_id || 0
- )
- return `@response_map` = existing_map if existing_map
-
- return nil if questionnaire.nil?
-
- attributes = {
- reviewer_id: team_participant.participant_id,
- reviewee_id: review_map&.reviewee_id || 0,
- reviewed_object_id: questionnaire.id,
- type: "QuizResponseMap"
- }
-
- `@response_map` = QuizResponseMap.new(attributes).tap { |map| map.save!(validate: false) }
+ return `@response_map` if defined?(`@response_map`)
+
+ `@response_map` = QuizResponseMap.find_by(
+ reviewer_id: team_participant.participant_id,
+ reviewee_id: review_map&.reviewee_id || 0
+ )
+end
+
+def ensure_response_map!
+ return response_map if response_map
+ return nil if questionnaire.nil?
+
+ attributes = {
+ reviewer_id: team_participant.participant_id,
+ reviewee_id: review_map&.reviewee_id || 0,
+ reviewed_object_id: questionnaire.id,
+ type: "QuizResponseMap"
+ }
+
+ `@response_map` = QuizResponseMap.new(attributes).tap { |map| map.save!(validate: false) }
end🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 12-31: Cyclomatic complexity for response_map is too high. [8/7]
(Metrics/CyclomaticComplexity)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/student_task/quiz_task_item.rb` around lines 12 - 30, The
response_map accessor currently creates and persists a QuizResponseMap; change
response_map to be read-only by returning an existing QuizResponseMap lookup
(using QuizResponseMap.find_by with reviewer_id/team_participant.participant_id
and reviewee_id/review_map&.reviewee_id || 0) or nil, and remove any
creation/saving logic from it. Add a new method ensure_response_map! that builds
and persists the QuizResponseMap (using the same attributes: reviewer_id,
reviewee_id, reviewed_object_id: questionnaire.id, type: "QuizResponseMap"),
assigns it to `@response_map`, and saves it (validate: false) so callers that need
creation can call ensure_response_map! instead; keep callers like
StudentTask::BaseTaskItem#completed?, `#to_h` and StudentTask.find_task_for_map
using response_map for read-only access only.
Add comments, untouch application_controller in open expertiza PR
Try to untouch application_controller again
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
app/controllers/responses_controller.rb (1)
88-92:⚠️ Potential issue | 🟠 MajorRemove
:roundfromresponse_update_params.
updatealready targets a concreteResponse, so permitting:roundlets clients move that submission into another round after the task-order checks have passed. Keep thecontent→additional_commentmapping, but do not mass-assign round here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/responses_controller.rb` around lines 88 - 92, Remove :round from the permitted attributes in response_update_params so callers cannot mass-assign or change a Response's round; keep the content→additional_comment mapping and p.delete(:content) logic intact. Specifically, edit the response_update_params method to stop permitting :round (only permit :is_submitted, :additional_comment, :content), preserve the existing lines that set p[:additional_comment] = p[:content] if present and p.delete(:content), and ensure the controller's update action continues to update the concrete Response without accepting a round parameter.app/models/student_task.rb (3)
82-91:⚠️ Potential issue | 🟠 MajorCheck existing quiz maps per reviewee, not once per reviewer.
has_existing_quiz_mapsis computed once for the reviewer, then reused for everyreview_map. If only one reviewee already has a quiz map andquiz_questionnaireis nil, this loop enqueues quiz tasks for other reviewees whoseQuizTaskItem#response_mapresolves to nil, so they show up in the queue but cannot be started.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/student_task.rb` around lines 82 - 91, The code computes has_existing_quiz_maps once per reviewer and reuses it for every review_map, causing quiz tasks to be enqueued for reviewees who actually have no quiz response; update the logic so the existence check is done per review_map (e.g., query QuizResponseMap.where(reviewer_id: participant.id, reviewee_id: review_map.reviewee_id).exists?) inside the review_maps.each loop before constructing ::QuizTaskItem.new, and use that per-reviewee boolean in the conditional instead of the single has_existing_quiz_maps variable.
38-42:⚠️ Potential issue | 🟠 MajorMissing participants should not return
200 null.This returns
nilfor an unknown id, andStudentTasksController#viewrenders that withstatus: :ok(app/controllers/student_tasks_controller.rb:20-25). RaiseActiveRecord::RecordNotFoundhere or handle the nil in the controller so callers get a 404 instead of a successful empty response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/student_task.rb` around lines 38 - 42, The from_participant_id class method currently returns nil for a missing Participant which leads StudentTasksController#view to render 200 null; update from_participant_id to raise ActiveRecord::RecordNotFound when Participant.find_by(id: id) returns nil (or alternatively handle the nil in StudentTasksController#view by returning head :not_found or raising RecordNotFound), so callers receive a 404; change the flow in self.from_participant_id (and keep create_from_participant usage) to raise when part is nil.
18-27:⚠️ Potential issue | 🟠 MajorRestrict these paths to assignment participants.
Participantincludes course-level rows too.from_userandresolve_context_for_assignmentboth query too broadly, andcreate_from_participantwill happily serialize a participant whoseassignmentisnil. That leaks course memberships into student-task payloads and can resolve the wrong queue when a course id collides with an assignment id.Also applies to: 31-34, 46-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/student_task.rb` around lines 18 - 27, create_from_participant is currently serializing course-level Participant rows; restrict it and the related lookup methods (from_user and resolve_context_for_assignment) to only handle assignment participants by checking the participant is tied to an Assignment (e.g. participant.assignment.present? or parent_type/parent_id indicating an Assignment) before building the StudentTask. Update create_from_participant to return nil or raise when participant.assignment is nil, and add the same guard/filter in from_user and resolve_context_for_assignment so they only query/return participants whose parent is an Assignment (avoid broad course-level rows) to prevent leaking course memberships and id collisions.app/controllers/student_tasks_controller.rb (1)
33-35:⚠️ Potential issue | 🟠 MajorKeep
queueandnext_taskread-only.
StudentTask.ensure_response_objects!writesResponseMap/Responserows (app/models/student_task.rb:114-119), so both GET endpoints mutate state just by being fetched. That makes retries, caches, and prefetchers unsafe. Move creation to the explicit start/create flow and keep these reads query-only.Also applies to: 44-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/student_tasks_controller.rb` around lines 33 - 35, The GET endpoints call StudentTask.ensure_response_objects!(tasks) which creates ResponseMap/Response rows and thus mutates state; make these endpoints read-only by removing or disabling calls to StudentTask.ensure_response_objects! (and any side-effecting logic in StudentTask.build_tasks) so that they only query and return task attributes (including queue and next_task) without creating DB records, and instead move the creation of ResponseMap/Response into the explicit start/create flow (e.g., the controller actions or service methods that begin a task) so that only those flows invoke StudentTask.ensure_response_objects! or a new create_response_objects_for(task) method.app/models/quiz_task_item.rb (1)
15-34:⚠️ Potential issue | 🟠 MajorKeep
response_mapread-only.
StudentTask::BaseTaskItem#completed?,#to_h, andStudentTask.find_task_for_mapall callresponse_map(app/models/student_task.rb:121-127,app/models/student_task.rb:186-203). Persisting aQuizResponseMaphere means queueing, serialization, or completion checks can create rows as a side effect. Split the write path intoensure_response_map!and keep this accessor query-only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/quiz_task_item.rb` around lines 15 - 34, response_map currently creates/persists a QuizResponseMap; change it to be read-only (only perform the find_by lookup and return nil if not found or questionnaire.nil?), and extract the write path into a new method ensure_response_map! that builds the same attributes (using team_participant.participant_id, review_map&.reviewee_id || 0, and questionnaire.id) and creates QuizResponseMap.new(...).save!(validate: false). Update any call sites that need creation (e.g. StudentTask::BaseTaskItem#completed?, `#to_h`, and StudentTask.find_task_for_map) to call ensure_response_map! where they expect a persisted map, while leaving response_map as a pure query accessor.
🤖 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/application_controller.rb`:
- Line 12: Remove the trailing whitespace on the blank line inside the
ApplicationController class definition; open the ApplicationController source,
locate the stray space on the empty line (within class ApplicationController)
and delete it so the line is truly empty, then run the formatter/linter and
amend the commit to avoid including unrelated whitespace-only changes in this
PR.
In `@app/controllers/responses_controller.rb`:
- Around line 34-37: Ignore params[:round] and derive the round on the server to
prevent clients from selecting off-queue rounds: stop using params[:round] (and
the .to_i coercion) in ResponsesController and instead compute the round from
existing responses for the map (e.g. inspect Response records for `@map` to get
the current/highest round or the next expected round) before doing
Response.where(map_id: `@map.id`, round: round)...last or building a new Response;
update the logic in ResponsesController that currently references
Response.where(map_id: `@map.id`, round: round) so it uses the server-derived
round, preventing creation of arbitrary rounds that StudentTask#completed? will
count.
---
Duplicate comments:
In `@app/controllers/responses_controller.rb`:
- Around line 88-92: Remove :round from the permitted attributes in
response_update_params so callers cannot mass-assign or change a Response's
round; keep the content→additional_comment mapping and p.delete(:content) logic
intact. Specifically, edit the response_update_params method to stop permitting
:round (only permit :is_submitted, :additional_comment, :content), preserve the
existing lines that set p[:additional_comment] = p[:content] if present and
p.delete(:content), and ensure the controller's update action continues to
update the concrete Response without accepting a round parameter.
In `@app/controllers/student_tasks_controller.rb`:
- Around line 33-35: The GET endpoints call
StudentTask.ensure_response_objects!(tasks) which creates ResponseMap/Response
rows and thus mutates state; make these endpoints read-only by removing or
disabling calls to StudentTask.ensure_response_objects! (and any side-effecting
logic in StudentTask.build_tasks) so that they only query and return task
attributes (including queue and next_task) without creating DB records, and
instead move the creation of ResponseMap/Response into the explicit start/create
flow (e.g., the controller actions or service methods that begin a task) so that
only those flows invoke StudentTask.ensure_response_objects! or a new
create_response_objects_for(task) method.
In `@app/models/quiz_task_item.rb`:
- Around line 15-34: response_map currently creates/persists a QuizResponseMap;
change it to be read-only (only perform the find_by lookup and return nil if not
found or questionnaire.nil?), and extract the write path into a new method
ensure_response_map! that builds the same attributes (using
team_participant.participant_id, review_map&.reviewee_id || 0, and
questionnaire.id) and creates QuizResponseMap.new(...).save!(validate: false).
Update any call sites that need creation (e.g.
StudentTask::BaseTaskItem#completed?, `#to_h`, and StudentTask.find_task_for_map)
to call ensure_response_map! where they expect a persisted map, while leaving
response_map as a pure query accessor.
In `@app/models/student_task.rb`:
- Around line 82-91: The code computes has_existing_quiz_maps once per reviewer
and reuses it for every review_map, causing quiz tasks to be enqueued for
reviewees who actually have no quiz response; update the logic so the existence
check is done per review_map (e.g., query QuizResponseMap.where(reviewer_id:
participant.id, reviewee_id: review_map.reviewee_id).exists?) inside the
review_maps.each loop before constructing ::QuizTaskItem.new, and use that
per-reviewee boolean in the conditional instead of the single
has_existing_quiz_maps variable.
- Around line 38-42: The from_participant_id class method currently returns nil
for a missing Participant which leads StudentTasksController#view to render 200
null; update from_participant_id to raise ActiveRecord::RecordNotFound when
Participant.find_by(id: id) returns nil (or alternatively handle the nil in
StudentTasksController#view by returning head :not_found or raising
RecordNotFound), so callers receive a 404; change the flow in
self.from_participant_id (and keep create_from_participant usage) to raise when
part is nil.
- Around line 18-27: create_from_participant is currently serializing
course-level Participant rows; restrict it and the related lookup methods
(from_user and resolve_context_for_assignment) to only handle assignment
participants by checking the participant is tied to an Assignment (e.g.
participant.assignment.present? or parent_type/parent_id indicating an
Assignment) before building the StudentTask. Update create_from_participant to
return nil or raise when participant.assignment is nil, and add the same
guard/filter in from_user and resolve_context_for_assignment so they only
query/return participants whose parent is an Assignment (avoid broad
course-level rows) to prevent leaking course memberships and id collisions.
🪄 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: 41cacaab-05ba-435c-bdfc-3be595a0578d
📒 Files selected for processing (6)
app/controllers/application_controller.rbapp/controllers/responses_controller.rbapp/controllers/student_tasks_controller.rbapp/models/quiz_task_item.rbapp/models/review_task_item.rbapp/models/student_task.rb
✅ Files skipped from review due to trivial changes (1)
- app/models/review_task_item.rb
| include Authorization | ||
| include JwtToken | ||
|
|
There was a problem hiding this comment.
Remove the trailing whitespace.
RuboCop detects trailing whitespace on this blank line. Additionally, this formatting change to ApplicationController appears unrelated to the PR's objectives of implementing student quizzes and task ordering. Consider whether this change is necessary or if it was included unintentionally.
🧹 Proposed fix to remove trailing whitespace
-�
+
before_action :authorize📝 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
🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 12-12: Trailing whitespace detected.
(Layout/TrailingWhitespace)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/application_controller.rb` at line 12, Remove the trailing
whitespace on the blank line inside the ApplicationController class definition;
open the ApplicationController source, locate the stray space on the empty line
(within class ApplicationController) and delete it so the line is truly empty,
then run the formatter/linter and amend the commit to avoid including unrelated
whitespace-only changes in this PR.
Comment on lines
+34
to
+37
| round = (params[:round].presence || 1).to_i | ||
| response = Response.where(map_id: @map.id, round: round) | ||
| .order(:created_at) | ||
| .last || Response.new(map_id: @map.id, round: round) |
There was a problem hiding this comment.
Do not let clients choose the task round.
The task-ordering layer only provisions round: 1 responses (app/models/student_task.rb:173-182), but this action trusts params[:round] and even coerces invalid input to 0 via to_i. That lets callers create off-queue responses, and completed? will still count any submitted response on the map as task completion (app/models/student_task.rb:186-190). Derive the round server-side instead of accepting it here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/responses_controller.rb` around lines 34 - 37, Ignore
params[:round] and derive the round on the server to prevent clients from
selecting off-queue rounds: stop using params[:round] (and the .to_i coercion)
in ResponsesController and instead compute the round from existing responses for
the map (e.g. inspect Response records for `@map` to get the current/highest round
or the next expected round) before doing Response.where(map_id: `@map.id`, round:
round)...last or building a new Response; update the logic in
ResponsesController that currently references Response.where(map_id: `@map.id`,
round: round) so it uses the server-derived round, preventing creation of
arbitrary rounds that StudentTask#completed? will count.
Fix student_task-Bot Comments
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/models/student_task.rb (2)
32-43:⚠️ Potential issue | 🟠 MajorHandle
nilresults fromcreate_from_participantin both entry points.
create_from_participantnow intentionally returnsnilfor orphaned assignment rows, butfrom_userstill sorts the raw mapped array andfrom_participant_idstill returns the raw result. That means one danglingAssignmentParticipantcan either explode atsort_by(&:stage_deadline)or regress the single-task path back to200 null.Proposed fix
def self.from_user(user) Participant.where(user_id: user.id, type: 'AssignmentParticipant') .map { |p| create_from_participant(p) } + .compact .sort_by(&:stage_deadline) end def self.from_participant_id(id) part = Participant.find_by(id: id, type: 'AssignmentParticipant') raise ActiveRecord::RecordNotFound, "AssignmentParticipant not found with id=#{id}" unless part - create_from_participant(part) + task = create_from_participant(part) + raise ActiveRecord::RecordNotFound, "AssignmentParticipant not found with id=#{id}" unless task + + task end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/student_task.rb` around lines 32 - 43, The mapped results from create_from_participant may be nil for orphaned rows, so in from_user call .compact after mapping (i.e., Participant.where(...).map { |p| create_from_participant(p) }.compact.sort_by(&:stage_deadline)) to remove nils before sorting; in from_participant_id, check the result of create_from_participant(part) and raise ActiveRecord::RecordNotFound (or handle consistently) when it returns nil instead of returning a nil payload so callers don't receive a 200 with null.
84-93:⚠️ Potential issue | 🟠 MajorStop using reviewer-wide quiz-map state to enqueue per-reviewee quiz tasks.
has_existing_quiz_mapsis still computed once per reviewer, then reused for everyreview_map. ButQuizTaskItem#response_mapinapp/models/quiz_task_item.rb:14-30resolves quiz maps perreviewer_id/reviewee_id, so this can still surface quiz tasks whoseresponse_mapstaysnilfor reviewees that never got a quiz map.Proposed fix
- has_existing_quiz_maps = QuizResponseMap.where(reviewer_id: participant.id).exists? - if review_maps.any? review_maps.each do |review_map| - if (duty.nil? || team_participant.allows_quiz?) && (quiz_questionnaire || has_existing_quiz_maps) + quiz_map_exists = QuizResponseMap.exists?( + reviewer_id: participant.id, + reviewee_id: review_map.reviewee_id + ) + + if (duty.nil? || team_participant.allows_quiz?) && (quiz_questionnaire || quiz_map_exists) tasks << ::QuizTaskItem.new( assignment: assignment, team_participant: team_participant, review_map: review_map )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/student_task.rb` around lines 84 - 93, The code uses a single has_existing_quiz_maps (QuizResponseMap.where(reviewer_id: participant.id).exists?) for all review_maps, causing per-reviewee quiz tasks to be enqueued incorrectly; fix by computing the quiz-map existence per review_map (i.e., include the reviewee_id) inside the loop where review_map is available (or otherwise derive per-reviewee existence) and use that result instead of has_existing_quiz_maps when deciding to build a ::QuizTaskItem; update the logic surrounding review_maps.each and the conditional that references has_existing_quiz_maps so it queries QuizResponseMap with both reviewer_id and the current review_map.reviewee_id (or equivalent lookup) to match QuizTaskItem#response_map behavior.
🤖 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/models/student_task.rb`:
- Around line 218-225: The parse_stage_deadline method can return nil because
Time.zone.parse(value.to_s) returns nil for unparsable strings; update
parse_stage_deadline (and any internal variable used there) to capture the
result of Time.zone.parse into a variable, and if that result is nil return the
fallback Time.current + 1.year (same as the existing nil/exception case);
preserve the current behavior for Time/ActiveSupport::TimeWithZone inputs and
keep the rescue as a final fallback so stage_deadline and callers like
from_user.sort_by(&:stage_deadline) never receive nil.
---
Duplicate comments:
In `@app/models/student_task.rb`:
- Around line 32-43: The mapped results from create_from_participant may be nil
for orphaned rows, so in from_user call .compact after mapping (i.e.,
Participant.where(...).map { |p| create_from_participant(p)
}.compact.sort_by(&:stage_deadline)) to remove nils before sorting; in
from_participant_id, check the result of create_from_participant(part) and raise
ActiveRecord::RecordNotFound (or handle consistently) when it returns nil
instead of returning a nil payload so callers don't receive a 200 with null.
- Around line 84-93: The code uses a single has_existing_quiz_maps
(QuizResponseMap.where(reviewer_id: participant.id).exists?) for all
review_maps, causing per-reviewee quiz tasks to be enqueued incorrectly; fix by
computing the quiz-map existence per review_map (i.e., include the reviewee_id)
inside the loop where review_map is available (or otherwise derive per-reviewee
existence) and use that result instead of has_existing_quiz_maps when deciding
to build a ::QuizTaskItem; update the logic surrounding review_maps.each and the
conditional that references has_existing_quiz_maps so it queries QuizResponseMap
with both reviewer_id and the current review_map.reviewee_id (or equivalent
lookup) to match QuizTaskItem#response_map behavior.
🪄 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: 9ad11e41-3fd7-4989-b307-ffe6c9e848eb
📒 Files selected for processing (1)
app/models/student_task.rb
Fix Bot & Review Comments - Add fallback for Time.zone.parse returning nil
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
app/controllers/responses_controller.rb (2)
88-92:⚠️ Potential issue | 🟠 MajorRemove
:roundfrom update params to prevent round rewriting.Permitting
:roundinresponse_update_paramsallows clients to move an existing response to a different round after task-order checks have passed, circumventing the sequencing logic.,
🐛 Proposed fix
def response_update_params - p = params.permit(:is_submitted, :additional_comment, :content, :round) + p = params.permit(:is_submitted, :additional_comment, :content) p[:additional_comment] = p[:content] if p[:content].present? p.delete(:content) p end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/responses_controller.rb` around lines 88 - 92, The response_update_params method currently permits :round which lets clients change a response's round; remove :round from the permitted list in response_update_params so updates cannot overwrite the round (leave the additional_comment-from-content logic and content deletion as-is), i.e., stop including :round in params.permit inside response_update_params to prevent round rewriting after task-order checks.
34-37:⚠️ Potential issue | 🟠 MajorClient-controlled round bypasses task ordering.
The code accepts
params[:round]allowing clients to create responses in arbitrary rounds. Sincecompleted?checksResponse.where(map_id: map.id, is_submitted: true).exists?without round filtering, andensure_response!only createsround: 1responses, a client could bypass ordering by specifying different rounds.Derive the round server-side based on the current task state rather than accepting it from the client.
,
🐛 Proposed fix
def create return unless enforce_task_order!(`@map`) - round = (params[:round].presence || 1).to_i - response = Response.where(map_id: `@map.id`, round: round) + round = 1 # Server-derived; extend if multi-round is needed + response = Response.where(map_id: `@map.id`, round: round) .order(:created_at) .last || Response.new(map_id: `@map.id`, round: round)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/responses_controller.rb` around lines 34 - 37, Ignore params[:round] and compute the round on the server by inspecting submitted responses for this map: derive round = (Response.where(map_id: `@map.id`, is_submitted: true).maximum(:round) || 0) + 1 so new/ensured responses are always the next sequential round; update the code that currently reads params[:round] and the logic in ensure_response! / where(...).order(...).last to use this server-derived round and prevent clients from creating arbitrary rounds.app/models/student_task.rb (2)
32-36:⚠️ Potential issue | 🟡 MinorAdd
.compactbefore sorting to handle orphaned participants.If an
AssignmentParticipanthas a danglingparent_id(deleted assignment),create_from_participantreturnsnil, andsort_by(&:stage_deadline)will raiseNoMethodError.🛡️ Proposed fix
def self.from_user(user) Participant.where(user_id: user.id, type: 'AssignmentParticipant') .map { |p| create_from_participant(p) } + .compact .sort_by(&:stage_deadline) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/student_task.rb` around lines 32 - 36, The from_user method in StudentTask collects AssignmentParticipant records and maps them via create_from_participant, but if create_from_participant returns nil for orphaned participants the subsequent sort_by(&:stage_deadline) will raise; update StudentTask.self.from_user to filter out nils (e.g., call .compact on the array returned by .map before .sort_by) so only non-nil StudentTask objects are sorted, referencing the from_user method and create_from_participant to locate the change.
84-94:⚠️ Potential issue | 🟠 MajorQuiz task eligibility check is not per-reviewee.
has_existing_quiz_maps(line 84) checks if the reviewer has any quiz map, not one specific to the currentreview_map's reviewee. Whenquiz_questionnaireisniland only some reviewees have quiz maps, this createsQuizTaskItementries for all reviewees. Those without actual quiz maps will haveresponse_mapreturnnil, creating tasks that appear in the queue but cannot be started.Consider checking for a quiz map per reviewee:
🐛 Proposed fix
review_maps.each do |review_map| - if (duty.nil? || team_participant.allows_quiz?) && (quiz_questionnaire || has_existing_quiz_maps) + has_quiz_for_reviewee = QuizResponseMap.exists?( + reviewer_id: participant.id, + reviewee_id: review_map.reviewee_id + ) + if (duty.nil? || team_participant.allows_quiz?) && (quiz_questionnaire || has_quiz_for_reviewee) tasks << ::QuizTaskItem.new( assignment: assignment, team_participant: team_participant, review_map: review_map ) endAlso consider removing the now-unused
has_existing_quiz_mapsvariable from line 84.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/student_task.rb` around lines 84 - 94, The current check uses has_existing_quiz_maps (QuizResponseMap.where(reviewer_id: participant.id).exists?) which is reviewer-wide and can create QuizTaskItem for reviewees who have no quiz map; instead, compute per-reviewee existence inside the review_maps loop (e.g. call QuizResponseMap.exists?(reviewer_id: participant.id, reviewee_id: review_map.reviewee_id) or similar) and replace the condition (duty.nil? || team_participant.allows_quiz?) && (quiz_questionnaire || has_existing_quiz_maps) with one that uses the per-reviewee boolean; remove the now-unused has_existing_quiz_maps variable. Ensure response_map usage (where QuizTaskItem expects a map) will now be valid because the per-reviewee existence check matches the review_map.reviewee.
🧹 Nitpick comments (1)
app/controllers/responses_controller.rb (1)
8-17: Default tofalsefor unknown actions inaction_allowed?.Returning
truefor unrecognized actions (line 15) is permissive by default. If a new action is added without updating this method, it will be automatically allowed. Defaulting tofalseis safer.♻️ Proposed fix
def action_allowed? case action_name when "create" - true # auth already handled by before_action above + true # auth already handled by before_action above when "show", "update" `@response` && `@response.map.reviewer.user_id` == current_user.id else - true + false end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/responses_controller.rb` around lines 8 - 17, The action_allowed? method is currently permissive for unknown actions; change its default to deny access by updating the else branch to return false so new actions aren't automatically allowed. Specifically, inside action_allowed? (using action_name), keep the existing cases for "create" (true) and "show"/"update" (the check `@response` && `@response.map.reviewer.user_id` == current_user.id), but replace the final else return true with false (or an explicit deny) so unknown action_names are rejected by default.
🤖 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/responses_controller.rb`:
- Around line 81-83: Fix the grammar in the unauthorized error message in
ResponsesController: update the render call that checks unless
`@map.reviewer.user_id` == current_user.id to return "You are not authorized to
create this response" (singular) instead of "this responses" so the message
reads correctly in the render json { error: ... }, status: :forbidden.
---
Duplicate comments:
In `@app/controllers/responses_controller.rb`:
- Around line 88-92: The response_update_params method currently permits :round
which lets clients change a response's round; remove :round from the permitted
list in response_update_params so updates cannot overwrite the round (leave the
additional_comment-from-content logic and content deletion as-is), i.e., stop
including :round in params.permit inside response_update_params to prevent round
rewriting after task-order checks.
- Around line 34-37: Ignore params[:round] and compute the round on the server
by inspecting submitted responses for this map: derive round =
(Response.where(map_id: `@map.id`, is_submitted: true).maximum(:round) || 0) + 1
so new/ensured responses are always the next sequential round; update the code
that currently reads params[:round] and the logic in ensure_response! /
where(...).order(...).last to use this server-derived round and prevent clients
from creating arbitrary rounds.
In `@app/models/student_task.rb`:
- Around line 32-36: The from_user method in StudentTask collects
AssignmentParticipant records and maps them via create_from_participant, but if
create_from_participant returns nil for orphaned participants the subsequent
sort_by(&:stage_deadline) will raise; update StudentTask.self.from_user to
filter out nils (e.g., call .compact on the array returned by .map before
.sort_by) so only non-nil StudentTask objects are sorted, referencing the
from_user method and create_from_participant to locate the change.
- Around line 84-94: The current check uses has_existing_quiz_maps
(QuizResponseMap.where(reviewer_id: participant.id).exists?) which is
reviewer-wide and can create QuizTaskItem for reviewees who have no quiz map;
instead, compute per-reviewee existence inside the review_maps loop (e.g. call
QuizResponseMap.exists?(reviewer_id: participant.id, reviewee_id:
review_map.reviewee_id) or similar) and replace the condition (duty.nil? ||
team_participant.allows_quiz?) && (quiz_questionnaire || has_existing_quiz_maps)
with one that uses the per-reviewee boolean; remove the now-unused
has_existing_quiz_maps variable. Ensure response_map usage (where QuizTaskItem
expects a map) will now be valid because the per-reviewee existence check
matches the review_map.reviewee.
---
Nitpick comments:
In `@app/controllers/responses_controller.rb`:
- Around line 8-17: The action_allowed? method is currently permissive for
unknown actions; change its default to deny access by updating the else branch
to return false so new actions aren't automatically allowed. Specifically,
inside action_allowed? (using action_name), keep the existing cases for "create"
(true) and "show"/"update" (the check `@response` &&
`@response.map.reviewer.user_id` == current_user.id), but replace the final else
return true with false (or an explicit deny) so unknown action_names are
rejected by default.
🪄 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: 1920a817-883d-4287-bc0c-2ca06d727beb
📒 Files selected for processing (2)
app/controllers/responses_controller.rbapp/models/student_task.rb
Fix Gramatical Error
… and student task controller based on items
Refactor test cases merged to Reimp-Quiz
Update student_tasks_controller_tasks_item_spec.rb
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
4 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.
Student Quiz Reimplementation
Implemented ordered quiz → review task sequencing using polymorphic task objects, while routing both quiz and review task work through
ResponsesController.The original
TaskOrderingnamespace has been removed from the app code. Task sequencing now lives in sharedStudentTaskclass methods, with quiz/review differences encapsulated by task item objects instead of controller conditionals.Implemented
Task sequencing
StudentTask:resolve_context_for_assignmentresolve_context_for_participantbuild_tasksensure_response_objects!find_task_for_mapprior_tasks_complete?StudentTask::BaseTaskItemQuizTaskItemReviewTaskItemTaskOrderingsource files.API/controller logic
StudentTasksControllerqueue, next task, and start task flows to useStudentTasktask sequencing.ResponsesControllerso create/update operations enforce task order through the same shared sequencing logic.ResponseMapandResponserecords are created when task queues are loaded or started.Tested
RSpec request coverage for key endpoints:
spec/requests/api/v1/responses_controller_spec.rbspec/requests/api/v1/student_tasks_controller_spec.rbExisting model/request support remains in place for student task behavior and response ordering.
How to run tests
bundle exec rspec