E2619 - Student Quiz Backend#345
Conversation
…e quiz questionnaire
… team can have different quizzes for the same assignment
…to correctly score a quiz Co-authored-by: Copilot <copilot@github.com>
Added backend testing for quiz scoring logic
Questionnaire correct answers
📝 WalkthroughWalkthroughThis pull request introduces comprehensive quiz-response functionality and restructures questionnaire management. It adds new API controllers for responses, response maps, quiz responses, items, and sign-up topics; establishes new models for questionnaire types and item types; refactors response scoring to support quiz-specific logic; expands assignment and questionnaire associations with nested attributes; and migrates the schema to include quiz-related columns and table renames. Changes
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 |
Generated by 🚫 Danger |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
spec/rails_helper.rb (1)
34-48:⚠️ Potential issue | 🟡 MinorRemove redundant before/after hooks; use only the around(:each) wrapper.
The
before(:each),after(:each), andaround(:each)hooks are overlapping. In DatabaseCleaner 2.0.1, thecleaningmethod internally handles bothstartandclean, so combining it with explicitbefore/afterhooks causes:
DatabaseCleaner.startcalled twice (before hook + cleaning's internal start)DatabaseCleaner.cleancalled twice (cleaning's internal clean + after hook)Keep the
around(:each)block alone and move the strategy configuration inside it:Suggested fix
config.before(:suite) do FactoryBot.factories.clear FactoryBot.find_definitions DatabaseCleaner.allow_remote_database_url = true if ENV['GITHUB_ACTIONS'] DatabaseCleaner.clean_with(:deletion, except: %w[ar_internal_metadata]) else DatabaseCleaner.clean_with(:truncation) end end - config.before(:each) do - DatabaseCleaner.strategy = :transaction - DatabaseCleaner.start - end - - config.after(:each) do - DatabaseCleaner.clean - end - - # Ensures proper cleanup config.around(:each) do |example| + DatabaseCleaner.strategy = :transaction DatabaseCleaner.cleaning { example.run } end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/rails_helper.rb` around lines 34 - 48, Remove the redundant config.before(:each) and config.after(:each) hooks so DatabaseCleaner.start and DatabaseCleaner.clean are not invoked twice; instead keep only the config.around(:each) wrapper and set DatabaseCleaner.strategy inside that around block before calling DatabaseCleaner.cleaning { example.run } (referencing DatabaseCleaner.strategy, DatabaseCleaner.start, DatabaseCleaner.clean, config.before(:each), config.after(:each), config.around(:each), and example.run to locate the code).db/migrate/20250621180851_rename_item_foreign_key_index_in_answers.rb (1)
1-7:⚠️ Potential issue | 🟠 MajorUse
up/downwith defensive guards to ensure reversible migration.The
changemethod will hard-fail at runtime if the index "fk_score_questions" doesn't exist, and its automatic reversal won't work correctly. When rolling back, Rails will try to re-add the old index by name against a column that doesn't exist (sincequestion_idwas renamed toitem_idin the prior migration). Use explicitup/downmethods withindex_exists?guards:class RenameItemForeignKeyIndexInAnswers < ActiveRecord::Migration[8.0] - def change - remove_index :answers, name: "fk_score_questions" - add_index :answers, :item_id, name: "fk_score_items" - end + def up + remove_index :answers, name: "fk_score_questions" if index_exists?(:answers, name: "fk_score_questions") + add_index :answers, :item_id, name: "fk_score_items" unless index_exists?(:answers, :item_id, name: "fk_score_items") + end + + def down + remove_index :answers, name: "fk_score_items" if index_exists?(:answers, name: "fk_score_items") + add_index :answers, :item_id, name: "fk_score_questions" unless index_exists?(:answers, :item_id, name: "fk_score_questions") + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20250621180851_rename_item_foreign_key_index_in_answers.rb` around lines 1 - 7, Replace the current change method in RenameItemForeignKeyIndexInAnswers with explicit up and down methods that use index_exists? guards: in up, only remove_index(:answers, name: "fk_score_questions") if index_exists? with that name, and only add_index(:answers, :item_id, name: "fk_score_items") if that index doesn't already exist; in down, only remove_index(:answers, name: "fk_score_items") if it exists and re-create the old-named index ("fk_score_questions") but create it on :item_id (not :question_id) and only if an index with that name doesn't already exist—this ensures safe, reversible migrations without referencing a missing column.
🟠 Major comments (29)
db/seeds2.rb-44-53 (1)
44-53:⚠️ Potential issue | 🟠 MajorFix brittle/idempotency issues in seeds + remove redundant
beginRight now this seeds join records using
assignment_id: i+1andquestionnaire_id: i+1and then blindly callscreate!. That will:
- fail on a fresh DB if IDs
1..4don’t exist yet- create duplicate
AssignmentQuestionnairerows on re-runs (unless the DB already enforces uniqueness)Also, the surrounding
beginblock is redundant (norescue/ensure), and the block has multiple RuboCop-style indentation/spacing issues.🛠️ Proposed rewrite
-begin - count = 4 - count.times do |i| - AssignmentQuestionnaire.create!( - assignment_id: i+1, - questionnaire_id: i+1, - used_in_round: [1,2].sample - ) - end -end +count = 4 + +count.times do |i| + assignment = Assignment.find_by(id: i + 1) + questionnaire = Questionnaire.find_by(id: i + 1) + + raise "Missing Assignment with id=#{i + 1}" if assignment.nil? + raise "Missing Questionnaire with id=#{i + 1}" if questionnaire.nil? + + AssignmentQuestionnaire.find_or_create_by!( + assignment_id: assignment.id, + questionnaire_id: questionnaire.id + ) do |aq| + aq.used_in_round = [1, 2].sample + end +endIf you intend to rely on upstream seeds guaranteeing those IDs, consider still adding the explicit
raisechecks above so failures are actionable instead of “mysterious FK errors”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/seeds2.rb` around lines 44 - 53, Remove the redundant begin block and fix indentation; make seeding idempotent by replacing the blind AssignmentQuestionnaire.create! call with a lookup-or-create and explicit existence checks: verify the referenced Assignment and Questionnaire exist (raise a clear error if not) and then use AssignmentQuestionnaire.find_or_create_by(assignment_id: i+1, questionnaire_id: i+1) (or find_or_initialize_by + save!) to avoid duplicates, and ensure used_in_round is set/updated on the found or created record; keep the loop (count = 4 / count.times) but adjust spacing to match RuboCop style.app/controllers/student_teams_controller.rb-65-83 (1)
65-83:⚠️ Potential issue | 🟠 MajorFix duplicate-team-name check in
updateto exclude the current team.Right now
matching_teamsincludes the team you’re updating, so setting the team name to its current value would be treated as “already in use” (query isn’t excludingteam.id).🔧 Proposed fix
def update # Checks for duplicate team names within the same assignment (by parent_id). - matching_teams = AssignmentTeam.where(name: params[:team][:name], parent_id: team.parent_id) + desired_name = params.dig(:team, :name) + matching_teams = AssignmentTeam + .where(name: desired_name, parent_id: team.parent_id) + .where.not(id: team.id) # no team with that name found - goes ahead and saves the new name if matching_teams.empty? - if team.update(name: params[:team][:name]) + if team.update(name: desired_name) serialized_team = ActiveModelSerializers::SerializableResource.new(team, serializer: TeamSerializer).as_json render json: serialized_team.merge({ message: "Team updated successfully", success: true }), status: :ok else render json: { error: team.errors.full_messages }, status: :unprocessable_entity end else # Returns an error if another team with the same name already exists. - render json: { error: "#{params[:team][:name]} is already in use." }, status: :unprocessable_entity + render json: { error: "#{desired_name} is already in use." }, status: :unprocessable_entity end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/student_teams_controller.rb` around lines 65 - 83, The duplicate-name check in update is including the record being edited (matching_teams on AssignmentTeam), so updating to the same name triggers a false positive; change the query that builds matching_teams in the update action to exclude the current team (use a where.not(id: team.id) or equivalent) so the check only considers other teams in the same parent/assignment before deciding if the name is already in use.db/migrate/20251013123456_add_textarea_width_to_items.rb-3-5 (1)
3-5:⚠️ Potential issue | 🟠 MajorGuard these new columns with
unless column_exists?to prevent duplicate-column migration failures.The unconditional
add_columncalls on lines 3–5 will fail if the columns already exist in the database. This occurs during reruns, partial rollbacks, or when the migration is applied to environments that already have these columns (as indicated by their presence indb/schema.rb). Apply the proposed guards:Fix
class AddTextareaWidthToItems < ActiveRecord::Migration[8.0] def change - add_column :items, :textarea_width, :integer - add_column :items, :textarea_height, :integer - add_column :items, :textbox_width, :integer + add_column :items, :textarea_width, :integer unless column_exists?(:items, :textarea_width) + add_column :items, :textarea_height, :integer unless column_exists?(:items, :textarea_height) + add_column :items, :textbox_width, :integer unless column_exists?(:items, :textbox_width) end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20251013123456_add_textarea_width_to_items.rb` around lines 3 - 5, The migration currently unconditionally calls add_column for :textarea_width, :textarea_height, and :textbox_width on the :items table which will error if those columns already exist; update the migration to guard each add_column with a column_exists? check (e.g. only call add_column :items, :textarea_width, :integer unless column_exists?(:items, :textarea_width)) for all three columns so re-running or applying to schemas that already contain these fields is safe.db/migrate/20251021165336_add_participant_ref_to_invitations.rb-3-5 (1)
3-5:⚠️ Potential issue | 🟠 MajorFK constraint not created if
participant_idcolumn pre-exists.The guard on line 3 only checks for the column's existence. If
participant_idalready exists but lacks a foreign key constraint, this migration silently succeeds without creating the FK. The current schema confirms this scenario: the column exists but the FK is missing.The proposed fix has an issue:
add_referencewill fail with a duplicate column error if the column already exists, regardless of theforeign_keysetting. The correct approach is to separate concerns:Corrected fix
class AddParticipantRefToInvitations < ActiveRecord::Migration[8.0] def change unless column_exists?(:invitations, :participant_id) add_column :invitations, :participant_id, :bigint, null: false end + + unless foreign_key_exists?(:invitations, :participants, column: :participant_id) + add_foreign_key :invitations, :participants, column: :participant_id + end end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20251021165336_add_participant_ref_to_invitations.rb` around lines 3 - 5, The migration currently only checks column_exists?(:invitations, :participant_id) and skips creating a foreign key if the column exists; instead, change the logic to handle two cases: if the column does not exist, call add_reference :invitations, :participant, null: false (without attempting a duplicate foreign key), and if the column exists but the FK is missing, call add_foreign_key :invitations, :participants, column: :participant_id; use foreign_key_exists? (or equivalent) to detect if the FK already exists before calling add_foreign_key so the migration is idempotent.spec/factories/reponse.rb-3-6 (1)
3-6:⚠️ Potential issue | 🟠 MajorUse an association-backed response map instead of hardcoding the foreign key.
map_id { 1 }is non-portable and can fail depending on DB state. The Response model usesbelongs_to :response_map, so the association should beassociation :response_map, factory: :review_response_map. The timestamps can also be removed as FactoryBot handles these automatically.Proposed fix
FactoryBot.define do factory :response do - map_id { 1 } + association :response_map, factory: :review_response_map is_submitted { false } - created_at { Time.current } - updated_at { Time.current } end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/factories/reponse.rb` around lines 3 - 6, Replace the hardcoded foreign key and manual timestamps in the factory: remove map_id { 1 } and the created_at/updated_at attributes and instead declare the association using association :response_map, factory: :review_response_map so FactoryBot will build a linked ResponseMap; keep is_submitted { false } as-is and let FactoryBot handle timestamps automatically.db/migrate/20251126161701_remove_participant_ref_from_invitations.rb-3-5 (1)
3-5:⚠️ Potential issue | 🟠 MajorRemove the foreign key constraint explicitly before dropping the column.
The
add_referencein the prior migration creates a foreign key withforeign_key: true, butremove_referencewithoutforeign_key: trueonly removes the column, leaving the constraint behind. This causes the migration to fail in Rails 8.0.Proposed fix
def change if column_exists?(:invitations, :participant_id) - remove_reference :invitations, :participant + remove_foreign_key :invitations, :participants if foreign_key_exists?(:invitations, :participants) + remove_reference :invitations, :participant, foreign_key: false end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20251126161701_remove_participant_ref_from_invitations.rb` around lines 3 - 5, The migration currently checks column_exists?(:invitations, :participant_id) and calls remove_reference :invitations, :participant but does not drop the foreign key first; update the migration to first remove the FK using remove_foreign_key (for example remove_foreign_key :invitations, :participants or remove_foreign_key :invitations, column: :participant_id) when the column exists, then call remove_reference :invitations, :participant so the constraint is explicitly removed before the column is dropped.app/mailers/invitation_mailer.rb-31-33 (1)
31-33:⚠️ Potential issue | 🟠 MajorAvoid
find_by+ immediate dereference in subject construction.Line 31 can return
nil, and Line 33 then calls.name, which can raise and drop the email.Suggested fix
- `@from_team` = AssignmentTeam.find_by(id: `@from_participant.team_id`) + `@from_team` = AssignmentTeam.find(`@from_participant.team_id`) `@assignment` = Assignment.find(`@invitation.assignment_id`) - mail(to: `@to_participant.user.email`, subject: 'You have a new invitation from Expertiza for assignment ' + `@assignment.name` + ' by team ' + `@from_team.name`) + mail( + to: `@to_participant.user.email`, + subject: "You have a new invitation from Expertiza for assignment #{`@assignment.name`} by team #{`@from_team.name`}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/mailers/invitation_mailer.rb` around lines 31 - 33, The subject construction assumes `@from_team` is present and calls `@from_team.name` which can be nil; change the code that builds the mail subject in invitation_mailer.rb to safely handle a missing team by using `@from_team`&.name or a fallback (e.g., 'Unknown team') when composing the subject string (keep references to `@from_team`, `@from_participant.team_id`, `@assignment.name` and the mail(...) call).app/mailers/invitation_mailer.rb-27-28 (1)
27-28:⚠️ Potential issue | 🟠 MajorRemove invitation debug dump from the mailer path.
Line 27 logs
@invitation.inspectbefore Line 28 assigns@invitation, and it can leak invitation payloads into logs.Suggested fix
def send_invitation_email - puts `@invitation.inspect` `@invitation` = params[:invitation]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/mailers/invitation_mailer.rb` around lines 27 - 28, Remove the debug dump that potentially leaks sensitive invitation data: delete the stray puts `@invitation.inspect` in InvitationMailer (the mailer method where `@invitation` is set from params[:invitation]) and ensure any logging happens only after `@invitation` is assigned and uses safe/filtered fields if necessary; in short, remove the .inspect debug line or move and replace it with a safe log that references permitted attributes after `@invitation` = params[:invitation].db/migrate/20251022160053_change_invitation_from_id_foreign_key.rb-2-10 (1)
2-10:⚠️ Potential issue | 🟠 MajorUse
up/downor wrapchange_columnin areversibleblock.Line 9 uses
change_column :invitations, :from_id, :bigintinsidedef change, which Rails cannot automatically reverse. Per Rails migration documentation,change_columnis irreversible. Either switch to explicitup/downmethods or wrap this operation in areversibleblock to ensure rollbacks are predictable.Suggested fix with up/down
class ChangeInvitationFromIdForeignKey < ActiveRecord::Migration[7.0] - def change + def up # Remove old foreign key to participants if it exists if foreign_key_exists?(:invitations, column: :from_id) remove_foreign_key :invitations, column: :from_id end # Ensure from_id is bigint to match teams.id change_column :invitations, :from_id, :bigint # Add new foreign key to teams unless foreign_key_exists?(:invitations, :teams, column: :from_id) add_foreign_key :invitations, :teams, column: :from_id end end + + def down + raise ActiveRecord::IrreversibleMigration, "Reverting invitations.from_id foreign key requires explicit legacy schema mapping" + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20251022160053_change_invitation_from_id_foreign_key.rb` around lines 2 - 10, The migration uses change_column :invitations, :from_id, :bigint inside def change which Rails cannot reverse; update the migration to make the schema change reversible by either converting def change to explicit def up / def down methods (move the foreign_key_exists? check and remove_foreign_key + change_column into up, and restore the original column type and foreign key in down) or wrap the change_column call in a reversible do |dir| block and provide dir.up { change_column :invitations, :from_id, :bigint } and dir.down { change_column :invitations, :from_id, <original_type> } so rollbacks are deterministic; refer to the migration method names change, up, down and the change_column :invitations, :from_id, :bigint call to locate the code to modify.app/controllers/items_controller.rb-6-13 (1)
6-13:⚠️ Potential issue | 🟠 MajorAvoid logging full item payloads and attributes in production paths.
Line 8 and Line 12 can leak sensitive quiz data (e.g.,
correct_answer) to logs. Please remove these logs or strictly whitelist non-sensitive fields.Suggested hardening
- Rails.logger.info "Items for Questionnaire #{questionnaire.id}:" - questionnaire.items.each do |item| - Rails.logger.info item.attributes.inspect - end - - items_json = questionnaire.items.as_json - Rails.logger.info "JSON being rendered: #{items_json.inspect}" + Rails.logger.info "Fetching items for Questionnaire #{questionnaire.id}" + items_json = questionnaire.items.as_json(only: %i[id seq txt question_type break_before weight questionnaire_id])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/items_controller.rb` around lines 6 - 13, Remove the calls that log full item attributes and the full JSON payload (the Rails.logger.info lines that inspect questionnaire.items and items_json) to avoid leaking sensitive quiz data; instead, if logging is necessary use a whitelist of safe fields (e.g., call questionnaire.items.as_json(only: [:id, :question_text, :position]) or map/select on questionnaire.items to pick safe keys) and replace the existing Rails.logger.info invocations so only those non-sensitive fields are emitted (references: questionnaire.items, items_json).swagger/v1/swagger.yaml-1932-1954 (1)
1932-1954:⚠️ Potential issue | 🟠 MajorDuplicate
student_taskspaths with conflicting definitions will shadow earlier operations.
/student_tasks/listand/student_tasks/vieware already defined at lines 1474 and 1489 respectively, with complete parameter sets and error responses. The redeclarations at lines 1932 and 1940 have significant differences that conflict with the originals:
/student_tasks/list(line 1932): MissingAuthorizationheader parameter, fewer response codes/student_tasks/view(line 1940): Parameter type isstringinstead ofInteger, missingAuthorizationheader, missing 500/401 error responsesOpenAPI parsers use the last definition when encountering duplicate path keys, causing the newer incomplete definitions to override the original ones. This will break API documentation and client generation.
Remove the duplicate paths at lines 1932–1954 or consolidate all operations under the existing path definitions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swagger/v1/swagger.yaml` around lines 1932 - 1954, There are duplicate OpenAPI path definitions for /student_tasks/list and /student_tasks/view that are shadowing the earlier, complete definitions; remove the duplicate path objects or merge them so the original definitions remain authoritative: delete the later /student_tasks/list and /student_tasks/view entries (or consolidate by copying missing items into the original entries) and ensure the retained definitions include the Authorization header parameter, correct parameter types (Integer for view.id), and the full response set including 401/500 where applicable so parsers/clients use the complete spec.swagger/v1/swagger.yaml-2050-2058 (1)
2050-2058:⚠️ Potential issue | 🟠 MajorUse OpenAPI path-template syntax for
unsubmitroute.
"/responses/:id/unsubmit"is not valid OpenAPI syntax. Path parameters must use curly braces:"/responses/{id}/unsubmit". This is inconsistent with other response endpoints in the file (/responses/{id}and/responses/{id}/submit) and violates OpenAPI 3.0 specification.✅ Proposed fix
- "/responses/:id/unsubmit": + "/responses/{id}/unsubmit":🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swagger/v1/swagger.yaml` around lines 2050 - 2058, The OpenAPI path "/responses/:id/unsubmit" uses Express-style colon syntax instead of OpenAPI path-template syntax; update the path key to "/responses/{id}/unsubmit" so it matches the other endpoints (e.g., "/responses/{id}" and "/responses/{id}/submit") and keeps the existing parameters block (name: id, in: path, required: true) valid under OpenAPI 3.0.app/controllers/questions_controller.rb-46-47 (1)
46-47:⚠️ Potential issue | 🟠 MajorQuiz type whitelist is too narrow for current item-type naming.
QUIZ_ITEM_TYPESonly includes CamelCase variants, but quiz scoring/tests and seeded item types use spaced names too ("Text field","Multiple choice","Multiple choice checkbox"). Valid quiz inputs can be rejected with 422.Suggested fix (normalize aliases)
- QUIZ_ITEM_TYPES = %w[TextField MultipleChoiceRadio MultipleChoiceCheckbox Scale Checkbox].freeze + QUIZ_ITEM_TYPES = %w[TextField MultipleChoiceRadio MultipleChoiceCheckbox Scale Checkbox].freeze + QUIZ_ITEM_TYPE_ALIASES = { + 'Text field' => 'TextField', + 'Multiple choice' => 'MultipleChoiceRadio', + 'Multiple choice checkbox' => 'MultipleChoiceCheckbox' + }.freeze ... - if is_quiz && !QUIZ_ITEM_TYPES.include?(params[:question_type]) + normalized_type = QUIZ_ITEM_TYPE_ALIASES.fetch(params[:question_type], params[:question_type]) + if is_quiz && !QUIZ_ITEM_TYPES.include?(normalized_type) return render json: { error: "Invalid quiz item type. Allowed types: #{QUIZ_ITEM_TYPES.join(', ')}" }, status: :unprocessable_entity end ... - question_type: params[:question_type], + question_type: normalized_type,Also applies to: 69-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/questions_controller.rb` around lines 46 - 47, QUIZ_ITEM_TYPES currently lists only CamelCase names (constant QUIZ_ITEM_TYPES) causing valid inputs like "Text field" or "Multiple choice checkbox" to be rejected; update the code to normalize incoming item-type strings and the whitelist by mapping both the constant and inputs to a canonical form (e.g., downcase and strip spaces or replace spaces with no-space) before validation. Specifically, add a normalization step used wherever QUIZ_ITEM_TYPES is checked (referenced symbol: QUIZ_ITEM_TYPES) and in the related validation logic around the code block referenced at lines ~69-74 so that variants like "TextField", "Text field", "text field" and "textfield" all match the canonical whitelist entries. Ensure existing tests/seed data that use spaced names are accepted without changing seed data.db/seeds.rb-17-23 (1)
17-23:⚠️ Potential issue | 🟠 MajorDo not hard-code foreign keys in idempotent seeds.
This block still pins
institution_id/role_idto numeric IDs. If IDs differ in an existing DB, seeds can link to wrong records or fail.Suggested fix
User.find_or_create_by!(name: 'admin') do |user| user.email = 'admin2@example.com' user.password = 'password123' user.full_name = 'admin admin' - user.institution_id = 1 - user.role_id = 1 + user.institution_id = inst_id + user.role_id = Role.find_by!(name: 'Super Administrator').id end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/seeds.rb` around lines 17 - 23, The seed is hard-coding foreign keys (institution_id and role_id) on User.find_or_create_by! which breaks idempotency; update the seed to resolve the associated records by name (e.g., find_or_create_by on Institution and Role) and assign their ids or the associations to the User (use user.institution = found_institution or user.role = found_role or set user.institution_id = found_institution.id) instead of fixed numeric literals so the seed works regardless of existing ID values.app/models/sign_up_topic.rb-4-4 (1)
4-4:⚠️ Potential issue | 🟠 MajorUpdate foreign key in
has_many :signed_up_teamsassociation.The
signed_up_teamstable usesproject_topic_idas the foreign key column (as shown in schema line 359 and foreign key constraint on line 476), but the association specifiesforeign_key: 'topic_id'. This mismatch will break association queries and thedependent: :destroycallback. Change toforeign_key: 'project_topic_id'to match the actual schema column after the rename migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/sign_up_topic.rb` at line 4, The has_many association in SignUpTopic currently uses the wrong foreign_key; update the association declaration has_many :signed_up_teams, foreign_key: 'topic_id', dependent: :destroy to use foreign_key: 'project_topic_id' so it matches the signed_up_teams schema and preserves correct association queries and the dependent: :destroy behavior.app/controllers/teams_controller.rb-90-94 (1)
90-94:⚠️ Potential issue | 🟠 MajorValidate quiz type and assignment ownership before linking.
The action currently accepts any existing
Questionnaire. This allows linking non-quiz or cross-assignment questionnaires to a team, which breaks the quiz flow contract.Suggested fix
- unless Questionnaire.exists?(id: questionnaire_id) + questionnaire = Questionnaire.find_by(id: questionnaire_id) + unless questionnaire return render json: { error: 'Questionnaire not found' }, status: :not_found end - `@team.update`!(quiz_questionnaire_id: questionnaire_id) + unless %w[Quiz QuizQuestionnaire].include?(questionnaire.questionnaire_type.to_s) + return render json: { error: 'Questionnaire must be a quiz' }, status: :unprocessable_entity + end + unless AssignmentQuestionnaire.exists?(assignment_id: `@team.parent_id`, questionnaire_id: questionnaire.id) + return render json: { error: 'Questionnaire does not belong to this team assignment' }, status: :unprocessable_entity + end + + `@team.update`!(quiz_questionnaire_id: questionnaire.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/teams_controller.rb` around lines 90 - 94, The code currently links any existing Questionnaire to a team; restrict this by loading the Questionnaire (e.g., via Questionnaire.find_by(id: questionnaire_id)), verify it is a quiz (check the model discriminator or boolean like questionnaire.quiz? or questionnaire.questionnaire_type == 'quiz') and ensure it belongs to the same assignment as the team (compare questionnaire.assignment_id with `@team.assignment_id`) before calling `@team.update`!(quiz_questionnaire_id: questionnaire_id); if either check fails, return a 422 or 404 with an explanatory error message instead of updating.app/controllers/questionnaires_controller.rb-33-34 (1)
33-34:⚠️ Potential issue | 🟠 MajorDo not return backtraces in API responses.
Lines 33-34 expose raw exception messages and stack frames to clients. That leaks implementation details and can surface sensitive data from lower layers. Log the exception server-side and return a generic 500 payload instead.
Safer error handling
- rescue => e - render json: { error: e.message, backtrace: e.backtrace.take(5) }, status: :internal_server_error + rescue StandardError => e + Rails.logger.error("QuestionnairesController#create failed: #{e.class}: #{e.message}") + render json: { error: 'Failed to create questionnaire' }, status: :internal_server_error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/questionnaires_controller.rb` around lines 33 - 34, The rescue block currently renders the exception message and backtrace to clients; instead, log the full exception server-side (e.g., Rails.logger.error or logger.error with e.class, e.message and e.backtrace.join("\n") and/or send to your error tracker) and change the render call in the rescue to return a generic JSON 500 payload (e.g., render json: { error: "Internal server error" }, status: :internal_server_error) so the line that currently renders e.message and backtrace is not exposed to API clients.app/models/questionnaire.rb-18-19 (1)
18-19:⚠️ Potential issue | 🟠 MajorThe instructor exemption misses
QuizQuestionnaire.Line 19 only skips
instructor_idvalidation for the literal'Quiz', but this model still recognizes'QuizQuestionnaire'elsewhere. Student-authored quiz payloads using that existing type string will still fail validation.Safer predicate
- validates :instructor_id, presence: true, unless: -> { questionnaire_type == 'Quiz' } + validates :instructor_id, presence: true, + unless: -> { %w[Quiz QuizQuestionnaire].include?(questionnaire_type) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/questionnaire.rb` around lines 18 - 19, The current validation in the Questionnaire model requires instructor_id unless questionnaire_type == 'Quiz', but other parts of the app use 'QuizQuestionnaire' so student-authored quizzes still fail; update the validation predicate on the Questionnaire model (the validates :instructor_id, presence: true ...) to treat both 'Quiz' and 'QuizQuestionnaire' as quiz types (e.g., check questionnaire_type against a set or use a predicate like in? for %w[Quiz QuizQuestionnaire] or a method quiz_type?) so instructor_id is skipped for either quiz type.db/schema.rb-403-405 (1)
403-405:⚠️ Potential issue | 🟠 MajorAdd a foreign key for
teams.quiz_questionnaire_id.Line 403 introduces a new cross-table reference, but Lines 404-405 only index it. Without an FK, teams can keep dangling quiz questionnaire ids after deletes, which will show up later as hard-to-debug 404/500 paths in quiz creation and scoring.
Migration to add referential integrity
class AddQuizQuestionnaireForeignKeyToTeams < ActiveRecord::Migration[8.0] def change add_foreign_key :teams, :questionnaires, column: :quiz_questionnaire_id end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/schema.rb` around lines 403 - 405, Add a foreign key constraint for teams.quiz_questionnaire_id to enforce referential integrity: create a migration (e.g. AddQuizQuestionnaireForeignKeyToTeams) that calls add_foreign_key on the teams table referencing questionnaires with column: :quiz_questionnaire_id so deletes/updates on questionnaires cannot leave dangling ids in teams; run the migration and ensure the new FK name and index (existing index index_teams_on_quiz_questionnaire_id) remain consistent.config/routes.rb-234-236 (1)
234-236:⚠️ Potential issue | 🟠 MajorAvoid declaring
resources :assignmentstwice.Line 234 opens a second top-level
resources :assignments, which generates duplicate assignment CRUD routes and helper names. If you only need the nested duties route here, wrap this block withonly: []to prevent the duplicate routes while allowing the nested resource to function.Suggested fix
- resources :assignments do - resources :duties, controller: 'assignments_duties', only: [:index, :create, :destroy] + resources :assignments, only: [] do + resources :duties, controller: 'assignments_duties', only: %i[index create destroy] end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/routes.rb` around lines 234 - 236, The routes file declares a second top-level resources :assignments block which duplicates CRUD routes; change that block to only expose the nested resource by using resources :assignments, only: [] do ... end and keep the nested resources :duties (controller: 'assignments_duties', only: [:index, :create, :destroy]) inside it so you do not generate duplicate assignment routes or helper names.app/controllers/questionnaires_controller.rb-40-46 (1)
40-46:⚠️ Potential issue | 🟠 Major
destroy!now turns protected deletes into 500s.Line 43 will raise
ActiveRecord::DeleteRestrictionErrorwhen thecheck_for_question_associationscallback (defined inapp/models/questionnaire.rbline 78) blocks destruction, but this action only rescuesActiveRecord::RecordNotFound. The unhandled exception results in a 500 error instead of a controlled 409/422 response.Suggested controller handling
def destroy begin `@questionnaire` = Questionnaire.find(params[:id]) `@questionnaire.destroy`! # ensures dependent items are removed + head :no_content rescue ActiveRecord::RecordNotFound render json: $ERROR_INFO.to_s, status: :not_found and return + rescue ActiveRecord::DeleteRestrictionError, ActiveRecord::RecordNotDestroyed => e + render json: { error: e.message }, status: :conflict and return end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/questionnaires_controller.rb` around lines 40 - 46, The destroy action calls `@questionnaire.destroy`! which can raise ActiveRecord::DeleteRestrictionError from the model callback check_for_question_associations; add a rescue for ActiveRecord::DeleteRestrictionError (in addition to the existing ActiveRecord::RecordNotFound rescue) around the `@questionnaire.destroy`! call and render a controlled JSON error response (including the exception message) with an appropriate 409/422 status (e.g., status: :conflict) so protected deletes return a client error instead of a 500.app/models/response_map.rb-115-123 (1)
115-123:⚠️ Potential issue | 🟠 MajorTeam activity is never considered for review maps.
For
ReviewResponseMap,reviewee_idpoints to a team, but this helper starts by reading therevieweeassociation as aParticipant. That makesrevieweenil for normal peer reviews, soneeds_update_link?misses fresh team submissions and keeps returning the stale "Edit" state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/response_map.rb` around lines 115 - 123, latest_submission_at_for_reviewee assumes reviewee is a Participant and ignores team activity, causing ReviewResponseMap (where reviewee_id is a Team) to miss fresh submissions; update latest_submission_at_for_reviewee to handle team reviewees by checking if reviewee.respond_to?(:team) and, when reviewee is a Team or has a team, include timestamps from the Team (e.g., team.updated_at, team.submissions or team.participants' latest updated_at) in the candidates array so needs_update_link? sees recent team activity; ensure you reference the existing reviewee association and preserve legacy checks using respond_to?(:updated_at) and .present? when adding team-derived timestamps.app/models/student_task.rb-65-91 (1)
65-91:⚠️ Potential issue | 🟠 MajorThis collapses per-team quiz state into one arbitrary assignment-level quiz.
The PR now allows different reviewee teams on the same assignment to own different quizzes, but this code picks the first
ReviewResponseMapand exposes a singlequiz_questionnaire_id/quiz_takenonStudentTask. For reviewers with multiple maps, that state will be wrong for at least some rows. Prefer returning quiz state per response map only, or aggregate it explicitly instead of using.first.🤖 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 65 - 91, The code collapses team-specific quiz state by taking the first ReviewResponseMap; stop using .first on ReviewResponseMap and instead compute quiz state per response map (or explicitly aggregate) so reviewers with multiple maps get correct info. Replace the ReviewResponseMap... .first/quiz_questionnaire_id/quiz_taken logic: iterate over ReviewResponseMap.where(reviewer_id: participant.id) (or map each rm), for each rm find the Team via Team.find_by(id: rm.reviewee_id), read team&.quiz_questionnaire_id, and determine quiz_taken by querying QuizResponseMap.where(reviewer_id: participant.id, reviewed_object_id: quiz_questionnaire_id).joins(...).where(responses: { is_submitted: true }).exists?; then attach the per-map results (e.g., an array of {review_response_map_id/quiz_questionnaire_id/quiz_taken} or an explicit aggregated value) to StudentTask instead of a single quiz_questionnaire_id/quiz_taken derived from .first.app/controllers/assignments_controller.rb-256-256 (1)
256-256:⚠️ Potential issue | 🟠 Major
notification_limitis no longer writable.
Response#reportable_difference?still readsassignment_questionnaire.notification_limit, but that attribute is filtered out here. New or updated questionnaire links will silently drop the threshold instead of persisting it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/assignments_controller.rb` at line 256, The assignment_questionnaires_attributes strong params are missing :notification_limit which causes new/updated questionnaire links to drop that value while Response#reportable_difference? still reads assignment_questionnaire.notification_limit; update the permitted attributes list (assignment_questionnaires_attributes) to include :notification_limit so the controller permits and persists that field when creating/updating assignment_questionnaire records.app/controllers/quiz_response_maps_controller.rb-48-60 (1)
48-60:⚠️ Potential issue | 🟠 MajorThe fallback path can attach the wrong team's quiz.
This branch explicitly guesses from the reviewer's first
ReviewResponseMapwhenreviewee_team_idis missing. In the exact multi-team case this PR adds support for, that can create/reuse a quiz map for the wrong team and score the wrong questionnaire. If the team id is required for correctness, fail fast instead of guessing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/quiz_response_maps_controller.rb` around lines 48 - 60, The fallback branch that guesses a reviewee team when reviewee_team_id is missing can attach the wrong team's quiz; instead, if reviewee_team_id is blank or zero, fail fast rather than guessing: remove the code path that picks the first ReviewResponseMap (review_map = ReviewResponseMap.find_by(...)) and replace it with a guard that checks for reviewee_team_id presence and returns/raises an error (e.g., render status 400 or raise ArgumentError/ActiveRecord::RecordNotFound) when missing; if you must attempt lookup, use ReviewResponseMap.where(...).to_a and if more than one match exists, treat that as ambiguous and fail rather than selecting the first—refer to reviewee_team_id, reviewer_participant_lookup, ReviewResponseMap (review_map) and reviewee_team to locate the code to change.app/controllers/response_maps_controller.rb-45-50 (1)
45-50:⚠️ Potential issue | 🟠 MajorSkipping "quiz" maps by id equality will drop legitimate peer reviews.
Here
reviewer_idandreviewee_idcome from different tables for normal review maps, so equal integers are possible without the row being a quiz. That means some peer-review rows will disappear from the API by coincidence. Use the map's actual type/class discriminator instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/response_maps_controller.rb` around lines 45 - 50, The guard that skips quiz maps by comparing map.reviewer_id and map.reviewee_id can falsely drop legitimate peer reviews; replace that equality check with a type/class discriminator check. Locate the code using map.reviewer_id and map.reviewee_id and change the guard to detect quiz maps by their class/type (for example: next if map.type == 'QuizResponseMap' or next if map.is_a?(QuizResponseMap)) so you skip only actual quiz response maps rather than rows with coincidentally equal IDs.app/models/response.rb-122-125 (1)
122-125:⚠️ Potential issue | 🟠 MajorPerfect quiz submissions are under-scored when
max_question_score > 1.The quiz branch awards
1 * weightfor a correct answer, butmaximum_scorestill usesweight * questionnaire.max_question_score. That means a fully correct quiz tops out below 100% unlessmax_question_scorehappens to be 1.💡 Suggested fix
- sum += (student_answer == correct && correct.present? ? 1 : 0) * (s.item.weight || 1) + per_question_max = questionnaire.max_question_score + sum += (student_answer == correct && correct.present? ? per_question_max : 0) * (s.item.weight || 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/response.rb` around lines 122 - 125, The quiz scoring currently adds a flat 1 * weight for correct answers, causing under-scoring when questionnaire.max_question_score > 1; update the quiz branch (inside the is_quiz && comment_scored_types check) to award (weight * per-question max score) for a correct answer instead of 1 * weight — use s.item.max_question_score if present or fall back to questionnaire.max_question_score (and then weight via s.item.weight || 1) so sum and maximum_score use the same per-question scale.app/controllers/responses_controller.rb-26-26 (1)
26-26:⚠️ Potential issue | 🟠 MajorRemove the unconditional
truereturn or restructure to return boolean explicitly.The method calls
render json: { error: 'forbidden' }, status: :forbiddenin lines 15 and 21 but then unconditionally returnstrueat line 26. This contradicts the authorization contract—rendering a 403 response signals denial, but returningtruesignals approval. In the authorization flow,all_actions_allowed?returns the result ofaction_allowed?, so atruereturn will pass the authorization check in theauthorizebefore_action despite the forbidden response being sent.All other controllers in the codebase either return boolean expressions directly (without rendering) or use explicit patterns like
return falseandrender_forbidden. RestructureResponsesController#action_allowed?to either:
- Return
false(letauthorizefilter render the 403), or- Use
returnafterrenderto halt execution explicitly🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/responses_controller.rb` at line 26, The method ResponsesController#action_allowed? currently ends with an unconditional `true` which conflicts with earlier `render json: { error: 'forbidden' }, status: :forbidden` calls; remove that unconditional `true` and instead ensure the method returns a boolean consistent with the rendered response by either returning false after rendering (or calling an existing helper like render_forbidden and then `return false`) or adding an explicit `return` after each `render` so execution halts; update all_actions_allowed? usage if needed to rely on the boolean returned from action_allowed?.app/controllers/responses_controller.rb-125-127 (1)
125-127:⚠️ Potential issue | 🟠 MajorCheck return values of
updateanddestroyto prevent silent failures.The
unsubmitendpoint ignores the return value ofupdate, anddestroyonly rescues exceptions. In Rails, both operations can fail silently (returningfalse) without raising exceptions—for example, if validations fail or destroy callbacks abort. While the Response model currently has no validations or callbacks, the code is not defensively written and would silently report success if either issue occurs in the future.Suggested fix
if `@response.is_submitted`? - `@response.update`(is_submitted: false) - render json: { message: "#{response_map_label} submission reopened for edits. The reviewer can now make changes.", response: `@response` }, status: :ok + if `@response.update`(is_submitted: false) + render json: { message: "#{response_map_label} submission reopened for edits. The reviewer can now make changes.", response: `@response` }, status: :ok + else + render json: { error: `@response.errors.full_messages.to_sentence` }, status: :unprocessable_entity + end else render json: { error: "This #{response_map_label.downcase} submission is not locked, so it cannot be reopened" }, status: :unprocessable_entity end @@ - `@response.destroy` - head :no_content -rescue StandardError => e - render json: { error: e.message }, status: :unprocessable_entity + `@response.destroy`! + head :no_content +rescue ActiveRecord::RecordNotDestroyed => e + render json: { error: e.record.errors.full_messages.to_sentence }, status: :unprocessable_entityAlso applies to: 138-141
🤖 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 125 - 127, The unsubmit action currently calls `@response.update`(is_submitted: false) and always renders success; change it to check the boolean return of `@response.update` and render an error (e.g., 422) with `@response.errors.full_messages` when update returns false. Do the same for the destroy flow: call `@response.destroy` and if it returns false (or frozen?) render an error response with the model errors or a generic failure message instead of assuming success; ensure both branches use the existing rendering pattern in ResponsesController so callers get proper HTTP status codes and error details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4668e577-e2e6-4b30-b0ff-115ccfa58d69
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (92)
Gemfileapp/controllers/assignments_controller.rbapp/controllers/grades_controller.rbapp/controllers/invitations_controller.rbapp/controllers/item_types_controller.rbapp/controllers/items_controller.rbapp/controllers/questionnaire_types_controller.rbapp/controllers/questionnaires_controller.rbapp/controllers/questions_controller.rbapp/controllers/quiz_response_maps_controller.rbapp/controllers/response_maps_controller.rbapp/controllers/responses_controller.rbapp/controllers/sign_up_topics_controller.rbapp/controllers/student_teams_controller.rbapp/controllers/teams_controller.rbapp/helpers/security_helper.rbapp/mailers/invitation_mailer.rbapp/mailers/invitation_sent_mailer.rbapp/models/Item.rbapp/models/assignment.rbapp/models/concerns/response_map_subclass_titles.rbapp/models/concerns/review_aggregator.rbapp/models/item_type.rbapp/models/project_topic.rbapp/models/questionnaire.rbapp/models/questionnaire_type.rbapp/models/quiz_response_map.rbapp/models/response.rbapp/models/response_map.rbapp/models/sign_up_topic.rbapp/models/student_task.rbapp/models/team.rbapp/serializers/assignment_questionnaire_serializer.rbapp/serializers/assignment_serializer.rbapp/serializers/questionnaire_serializer.rbapp/validators/invitation_validator.rbapp/views/invitation_sent_mailer/send_invitation_email.html.erbconfig/database.ymlconfig/routes.rbdb/migrate/20250321222753_rename_sign_up_topic_to_project_topic_in_signed_up_teams.rbdb/migrate/20250418004442_change_to_polymorphic_association_in_teams.rbdb/migrate/20250621151644_add_round_to_responses.rbdb/migrate/20250621152946_add_version_number_to_responses.rbdb/migrate/20250621180527_change_question_to_item_in_answers.rbdb/migrate/20250621180851_rename_item_foreign_key_index_in_answers.rbdb/migrate/20250626161114_add_name_to_teams.rbdb/migrate/20250629185100_add_grade_to_participant.rbdb/migrate/20250629185439_add_grade_for_submission_to_team.rbdb/migrate/20250629190818_add_comment_for_submission_to_team.rbdb/migrate/20250727170825_add_questionnaire_weight_to_assignment_questionnaire.rbdb/migrate/20250805174104_rename_status_to_reply_status_in_in_join_team_requests.rbdb/migrate/20251013123456_add_textarea_width_to_items.rbdb/migrate/20251013123457_add_cols_rows_to_items.rbdb/migrate/20251021165336_add_participant_ref_to_invitations.rbdb/migrate/20251022160053_change_invitation_from_id_foreign_key.rbdb/migrate/20251029071649_add_advertisement_fields_to_signed_up_teams.rbdb/migrate/20251125012619_add_vary_by_round_to_assignments.rbdb/migrate/20251126161701_remove_participant_ref_from_invitations.rbdb/migrate/20251230123456_rename_question_types_to_item_types.rbdb/migrate/20260227203258_add_max_members_for_duty_to_duties.rbdb/migrate/20260313064334_add_submission_fields_to_teams.rbdb/migrate/20260423000001_add_correct_answer_to_items.rbdb/migrate/20260424000001_add_quiz_questionnaire_id_to_teams.rbdb/schema.rbdb/seeds.rbdb/seeds2.rbdocker-compose.ymlsetup.shspec/controllers/responses_controller_spec.rbspec/factories/project_topics.rbspec/factories/reponse.rbspec/factories/response_maps.rbspec/factories/review_response_maps.rbspec/mailers/previews/invitation_sent_preview.rbspec/models/item_spec.rbspec/models/project_topic_spec.rbspec/models/response_map_label_spec.rbspec/models/response_map_spec.rbspec/models/response_rubric_label_spec.rbspec/models/response_spec.rbspec/models/signed_up_team_spec.rbspec/models/student_task_spec.rbspec/rails_helper.rbspec/requests/api/v1/grades_controller_spec.rbspec/requests/api/v1/invitation_controller_spec.rbspec/requests/api/v1/questions_spec.rbspec/requests/api/v1/quiz_response_maps_controller_spec.rbspec/requests/api/v1/response_maps_controller_spec.rbspec/requests/api/v1/student_teams_controller_spec.rbspec/routing/sign_up_topics_routing_spec.rbspec/validators/invitation_validator_spec.rbswagger/v1/swagger.yaml
💤 Files with no reviewable changes (1)
- spec/factories/review_response_maps.rb
| # When the frontend sends assignment_questionnaires_attributes without ids, | ||
| # clear existing records first to avoid duplicates. | ||
| if params[:assignment]&.key?(:assignment_questionnaires_attributes) | ||
| incoming = params[:assignment][:assignment_questionnaires_attributes] | ||
| has_ids = incoming.is_a?(Array) && incoming.any? { |aq| aq[:id].present? } | ||
| assignment.assignment_questionnaires.destroy_all unless has_ids | ||
| end |
There was a problem hiding this comment.
This deletes existing questionnaire links before the update is known to be valid.
destroy_all commits immediately. If assignment.update(...) fails afterward, the assignment has already lost its current assignment_questionnaires. Wrap the replacement in a transaction or express removals through nested _destroy so the whole change rolls back atomically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/assignments_controller.rb` around lines 30 - 36, The code
currently calls assignment.assignment_questionnaires.destroy_all before
attempting assignment.update, which commits deletions immediately; change this
to perform removals atomically by wrapping the replacement in a transaction or
by converting removals to nested attributes with _destroy so they occur as part
of assignment.update. Specifically, remove the direct call to
assignment.assignment_questionnaires.destroy_all and instead either (A) wrap the
update flow in Assignment.transaction { assignment.update!(params[:assignment])
} where you build the incoming attributes so missing ids result in explicit
nested attributes deletions, or (B) transform the incoming
params[:assignment][:assignment_questionnaires_attributes] into nested
attributes that set _destroy: true for existing records to be removed and then
call assignment.update(params[:assignment]) so changes roll back if update
fails; locate the logic around
params[:assignment][:assignment_questionnaires_attributes],
assignment.assignment_questionnaires.destroy_all, and the subsequent
assignment.update(...) to implement one of these approaches.
| def action_allowed? | ||
| true | ||
| end |
There was a problem hiding this comment.
Do not let callers choose the reviewer identity.
This endpoint unconditionally authorizes and then trusts reviewer_user_id from the request. Any authenticated user can create quiz participants/maps for another student and learn the target team's quiz identifiers. Bind the reviewer to current_user unless the caller is staff acting on someone else's behalf.
Also applies to: 33-35, 67-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/quiz_response_maps_controller.rb` around lines 4 - 6, The
controller currently trusts params[:reviewer_user_id] from callers; change
create/update flows (the actions handling quiz response map creation referenced
around lines 33-35 and 67-75) so that reviewer_user_id is set to current_user.id
by default and only overridden when the caller is authorized staff (use your
app's staff-check like current_user.is_staff? or a role check) — update the code
paths that read params[:reviewer_user_id] to enforce this binding, and use
action_allowed? (or the app's existing permission helpers) to gate any branch
that permits specifying a different reviewer.
| def action_allowed? | ||
| true | ||
| end |
There was a problem hiding this comment.
These endpoints allow cross-user map reads and writes.
The controller unconditionally authorizes and then trusts reviewer_user_id/id from the request. That lets any authenticated user enumerate another student's review maps, create mappings on their behalf, or delete arbitrary maps. Derive the reviewer from current_user and gate deletes to staff/owners.
Also applies to: 23-29, 111-119, 155-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/response_maps_controller.rb` around lines 4 - 6, The
controller currently trusts incoming reviewer_user_id/id params and always
returns true in action_allowed?, allowing cross-user reads/writes; update
action_allowed? to require at least authenticated user and remove reliance on
reviewer_user_id by deriving the reviewer from current_user (use current_user.id
wherever reviewer_user_id or id from params is used in create/update/show
methods), and restrict destroy (and any delete paths referenced around methods
handling ids) so only staff or map owners can delete (check current_user.staff?
|| current_user.id == map.reviewer_id before deletion); similarly validate
ownership when returning or modifying maps in the methods referenced (ensure
methods that read or modify maps compare map.reviewer_id to current_user.id or
require staff role).
| when 'create', 'show', 'index' | ||
| true |
There was a problem hiding this comment.
create, show, and index are missing authorization.
These branches allow every caller through. Since create can return an existing draft and show/index serialize answer and comments, a user who knows or guesses IDs can read another reviewer’s quiz/review data. Gate these actions with the same owner/teaching-staff checks used for the write paths.
🧰 Tools
🪛 RuboCop (1.86.1)
[warning] 10-10: Literal true used in void context.
(Lint/Void)
🤖 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 9 - 10, The branches
allowing 'create', 'show', and 'index' in ResponsesController currently return
true and skip authorization; change them to invoke the same owner/teaching-staff
authorization used for write paths (i.e., replace the unconditional true for the
'create', 'show', 'index' branches with a call to the existing
owner/teaching-staff check method used elsewhere in this controller, or add a
before_action that enforces that check for create/show/index) so only the
response owner or teaching staff can read or create/return drafts.
| params.require(:response).permit( | ||
| :map_id, | ||
| :is_submitted, | ||
| :submitted_at, | ||
| scores_attributes: [:id, :item_id, :answer, :comments] |
There was a problem hiding this comment.
Don't mass-assign workflow-controlled fields.
update uses these params directly, so a reviewer can PATCH is_submitted, submitted_at, or even map_id. That bypasses the deadline/scoring flow in submit and lets a response be rebound to a different map. Only editable score content should be writable here.
Suggested fix
def response_params
params.require(:response).permit(
- :map_id,
- :is_submitted,
- :submitted_at,
scores_attributes: [:id, :item_id, :answer, :comments]
)
end🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 155-155: Use %i or %I for an array of symbols.
(Style/SymbolArray)
🤖 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 151 - 155, The
controller is permitting workflow-controlled fields (map_id, is_submitted,
submitted_at) in params.require(:response).permit used by update; remove map_id,
is_submitted and submitted_at from the permitted list so update cannot
mass-assign them, leaving only safe editable attributes (e.g. scores_attributes:
[:id, :item_id, :answer, :comments] and any other truly user-editable fields),
and ensure the submission/deadline flow remains handled only via the submit
method or dedicated controller action (do not add back those fields to the
update permit list).
| # Serialises the item to a JSON-safe hash for API responses. | ||
| # | ||
| # The +correct_answer+ field is included only when {#is_quiz_item?} is +true+ | ||
| # so that it is never accidentally exposed on peer-review rubric items. | ||
| # The nested +questionnaire+ object includes +questionnaire_type+ so the | ||
| # frontend can distinguish quiz items from review items without a second | ||
| # request. A synthetic +is_quiz_item+ boolean is appended to the hash. | ||
| # | ||
| # @param options [Hash] standard ActiveModel::Serializers::JSON options | ||
| # @return [Hash] the serialised item | ||
| def as_json(options = {}) | ||
| super(options.merge({ | ||
| only: %i[txt weight seq question_type size alternatives break_before min_label max_label created_at updated_at], | ||
| include: { | ||
| questionnaire: { only: %i[name id] } | ||
| } | ||
| })).tap do |hash| | ||
| end | ||
| only_fields = %i[ | ||
| id txt weight seq question_type size alternatives break_before | ||
| min_label max_label created_at updated_at textarea_width | ||
| textarea_height textbox_width col_names row_names | ||
| ] | ||
| only_fields << :correct_answer if is_quiz_item? | ||
|
|
||
| super(options.merge({ | ||
| only: only_fields, | ||
| include: { | ||
| questionnaire: { only: %i[name id questionnaire_type] } | ||
| } | ||
| })).tap do |hash| | ||
| hash['is_quiz_item'] = is_quiz_item? | ||
| end |
There was a problem hiding this comment.
Do not serialize correct_answer in reviewer-facing quiz payloads.
Line 76 appends the answer key to standard item JSON whenever the questionnaire is a quiz. Any client that can fetch quiz items can read the correct answers before submitting, which defeats the gating/scoring flow this PR adds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/Item.rb` around lines 60 - 85, The as_json method currently
appends :correct_answer whenever is_quiz_item? is true; change it so
correct_answer is only added when a caller explicitly requests it (e.g. via
options[:include_correct_answer] or similar) to avoid exposing answer keys to
reviewers. Update the line that modifies only_fields (replace the unconditional
check) to require both is_quiz_item? and options[:include_correct_answer], and
ensure any legitimate callers that need the key (exporters/instructors) pass
that option when calling Item#as_json; keep the rest of as_json (including the
questionnaire include and is_quiz_item? flag) unchanged.
| # E2619: quiz maps are identified by reviewer_id == reviewee_id (the student reviews | ||
| # themselves). The response_maps table has no STI type column so is_a?(QuizResponseMap) | ||
| # always returns false; this is the only reliable discriminator. | ||
| is_quiz = map.reviewer_id == map.reviewee_id | ||
| # E2619: The frontend stores question types with spaces ("Text field", "Multiple choice", | ||
| # "Multiple choice checkbox"). Include both the spaced and CamelCase variants so scoring | ||
| # works regardless of which convention was used when the quiz was created. | ||
| comment_scored_types = %w[ | ||
| TextField MultipleChoiceRadio MultipleChoiceCheckbox | ||
| Text\ field Multiple\ choice Multiple\ choice\ checkbox | ||
| ].freeze | ||
| scores.each do |s| | ||
| # For quiz responses, the weights will be 1 or 0, depending on if correct | ||
| sum += s.answer * s.item.weight unless s.answer.nil? #|| !s.item.scorable? | ||
| # E2619: TextField, MultipleChoiceRadio, and MultipleChoiceCheckbox quiz items put | ||
| # the student's selected/typed answer into the comments column (answer is null). | ||
| # Score them by case-insensitive equality against item.correct_answer. | ||
| if is_quiz && comment_scored_types.include?(s.item.question_type) |
There was a problem hiding this comment.
reviewer_id == reviewee_id is not a safe quiz discriminator.
Normal review maps compare a participant id to a team id, so equal integers can happen by coincidence. When that happens, this code will treat a peer review as a quiz in both aggregate_questionnaire_score and maximum_score. ResponseMap already exposes a real type signal elsewhere in the codebase, so key off that instead.
Also applies to: 151-160
🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 114-117: Use an array literal [...] for an array of words.
(Style/WordArray)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/response.rb` around lines 107 - 122, Replace the unsafe quiz
discriminator (map.reviewer_id == map.reviewee_id) with the ResponseMap's real
type signal: use the same type-check used elsewhere in the codebase (for example
checking map.type == 'QuizResponseMap' or calling the existing predicate method
used for ResponseMap type discrimination) to set is_quiz; update both the spot
around is_quiz in this block and the analogous check in lines 151-160 (affecting
aggregate_questionnaire_score and maximum_score) so they both rely on the
ResponseMap type signal (e.g., QuizResponseMap) rather than comparing
reviewer_id and reviewee_id.
| </head> | ||
|
|
||
| <body> | ||
| <h1>You have been invited to join the team by <%= @to_user.fullname %></h1> |
There was a problem hiding this comment.
Fix inviter identity in email body.
Line 7 says the invitation is by @to_user (the recipient), which is incorrect. It should display the inviter (@from_user).
Suggested fix
- <h1>You have been invited to join the team by <%= `@to_user.fullname` %></h1>
+ <h1>You have been invited to join the team by <%= `@from_user.fullname` %></h1>📝 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.
| <h1>You have been invited to join the team by <%= @to_user.fullname %></h1> | |
| <h1>You have been invited to join the team by <%= `@from_user.fullname` %></h1> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/invitation_sent_mailer/send_invitation_email.html.erb` at line 7,
The email body currently shows the recipient `@to_user` as the inviter; update the
template to use the inviter instance variable `@from_user` instead (replace
`@to_user.fullname` with `@from_user.fullname`). Also add a safe fallback in the
view (e.g., use `@from_user`&.fullname or a default string) to avoid nil errors
when `@from_user` is missing.
| url: <%= ENV.fetch('DATABASE_URL', 'mysql2://root:expertiza@127.0.0.1:3307/reimplementation').gsub('?', '_development?') %> | ||
|
|
||
| test: | ||
| <<: *default | ||
| url: <%= ENV['DATABASE_URL'].gsub('?', '_test?') %> | ||
| url: <%= ENV.fetch('DATABASE_URL', 'mysql2://root:expertiza@127.0.0.1:3307/reimplementation').gsub('?', '_test?') %> | ||
|
|
||
| production: | ||
| <<: *default | ||
| url: <%= ENV['DATABASE_URL'].gsub('?', '_production?') %> No newline at end of file | ||
| url: <%= ENV.fetch('DATABASE_URL', 'mysql2://root:expertiza@127.0.0.1:3307/reimplementation').gsub('?', '_production?') %> No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm same fallback base URL is reused across environments
rg -n "ENV\.fetch\('DATABASE_URL', 'mysql2://root:expertiza@127\.0\.0\.1:3307/reimplementation'\)\.gsub" config/database.ymlRepository: expertiza/reimplementation-back-end
Length of output: 458
🏁 Script executed:
# Search for DATABASE_URL references and deployment configuration
rg -i "DATABASE_URL" --type-list | head -20Repository: expertiza/reimplementation-back-end
Length of output: 469
🏁 Script executed:
# Check for environment configuration files and documentation
fd -type f \( -name ".env*" -o -name "*.md" -o -name "docker-compose*" -o -name "Dockerfile*" -o -name ".github" \) | head -20Repository: expertiza/reimplementation-back-end
Length of output: 250
🏁 Script executed:
# Search for DATABASE_URL in various configuration contexts
rg -n "DATABASE_URL" -A 2 -B 2Repository: expertiza/reimplementation-back-end
Length of output: 2558
🏁 Script executed:
# Look for README and setup documentation
fd -type f -iname "readme*" -o -iname "setup*" -o -iname "contributing*" | head -10Repository: expertiza/reimplementation-back-end
Length of output: 250
🏁 Script executed:
# Check GitHub Actions and CI configurations
fd -type f \( -path "*/.github/workflows/*" -o -name ".gitlab-ci.yml" -o -name ".travis.yml" \)Repository: expertiza/reimplementation-back-end
Length of output: 250
🏁 Script executed:
# Search for comments/documentation about database setup or DATABASE_URL expectations
rg -n "setup\|development\|environment" config/database.yml config/database.yml.old 2>/dev/null | head -20Repository: expertiza/reimplementation-back-end
Length of output: 61
🏁 Script executed:
# Check if there's a .env.example or similar that documents DATABASE_URL
fd -type f -iname ".env*"Repository: expertiza/reimplementation-back-end
Length of output: 250
🏁 Script executed:
# Find README files
find . -maxdepth 2 -iname "readme*" -o -iname "setup*"Repository: expertiza/reimplementation-back-end
Length of output: 114
🏁 Script executed:
# Look for GitHub Actions or CI config directories
find . -maxdepth 3 -type d -name ".github" -o -type f -name ".gitlab-ci.yml" -o -type f -name "*.yml" -path "*/.github/*"Repository: expertiza/reimplementation-back-end
Length of output: 281
🏁 Script executed:
# Read the full config/database.yml.old to understand the intended setup
cat config/database.yml.old | head -60Repository: expertiza/reimplementation-back-end
Length of output: 1982
🏁 Script executed:
# List root directory to see what's available
ls -la | head -20Repository: expertiza/reimplementation-back-end
Length of output: 1251
🏁 Script executed:
# Read README to understand setup requirements
cat README.mdRepository: expertiza/reimplementation-back-end
Length of output: 1081
🏁 Script executed:
# Check GitHub Actions test workflow to see how DATABASE_URL is handled
cat .github/workflows/TestPR.ymlRepository: expertiza/reimplementation-back-end
Length of output: 3542
🏁 Script executed:
# Check setup scripts
cat bin/setup 2>/dev/null || cat setup.sh 2>/dev/null || echo "No setup found"Repository: expertiza/reimplementation-back-end
Length of output: 1117
Environment DB isolation breaks in fallback scenarios when DATABASE_URL is unset.
The gsub('?', '_<env>?') pattern only works if a ? exists in the URL. The fallback 'mysql2://root:expertiza@127.0.0.1:3307/reimplementation' has no query string, so all three environments would use the same database name, breaking data isolation.
While the documented setup (docker-compose) sets DATABASE_URL explicitly, the fallback is reachable in local non-Docker environments or misconfigured deployments. This creates a critical data safety risk.
The proposed fix is sound: use environment-specific variable names (DATABASE_URL_DEVELOPMENT, DATABASE_URL_TEST, DATABASE_URL_PRODUCTION) with appropriate fallback databases to guarantee isolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/database.yml` around lines 10 - 18, The current url fallback uses
gsub('?', '_<env>?') on ENV.fetch('DATABASE_URL',
'mysql2://.../reimplementation'), which fails when DATABASE_URL is unset (no '?'
present) and causes all environments to share the same DB; change each
environment's url to prefer a dedicated env var (e.g. DATABASE_URL_DEVELOPMENT,
DATABASE_URL_TEST, DATABASE_URL_PRODUCTION) with a sensible fallback that
includes the env-specific database name instead of relying on gsub, and update
the three url lines (the ENV.fetch calls around DATABASE_URL and the gsub usage)
so each reads its own ENV key and falls back to a distinct database name to
guarantee isolation.
| # DELETE /invitations/:id | ||
| delete("Delete invitation") do | ||
| tags "Invitations" | ||
| parameter name: 'Authorization', in: :header, type: :string, required: true, description: 'Bearer token' | ||
|
|
||
| response(403, "Student cannot delete invitations") do | ||
| let(:id) { invitation2.id } | ||
| run_test! | ||
| end | ||
| end | ||
|
|
||
| delete("Delete invitation") do | ||
| tags "Invitations" | ||
| parameter name: 'Authorization', in: :header, type: :string, required: true | ||
| let(:Authorization) { "Bearer #{ta_token}" } | ||
|
|
||
| response(200, "Delete successful") do | ||
| let(:id) { invitation.id } | ||
| run_test! | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In rswag-specs, if the same pathblock defines two operations with the same HTTP verb (e.g., twodelete blocks), are both preserved or can one override the other in generated OpenAPI metadata?
💡 Result:
In rswag-specs, if the same path block defines two operations with the same HTTP verb (e.g., two delete blocks), the second operation overrides the first in the generated OpenAPI metadata. Both are not preserved. The DSL defines each verb method (delete, get, etc.) to create a single describe block that sets metadata[:operation] = { verb: verb, ... }. RSpec example groups nest hierarchically, so a second delete block inside the same path creates a nested group. The SwaggerFormatter merges metadata via deep_merge! on groups with :response metadata (from response blocks), but since both operations share the same path and verb keys in the OpenAPI paths object, the later one overwrites the earlier one due to hash key uniqueness in OpenAPI. No sources explicitly document allowing or handling duplicate verbs in one path block; the structure implies one operation per verb per path. OpenAPI itself prohibits duplicate verbs per path. Attempting duplicates would likely result in only the last operation appearing in the spec.
Citations:
- 1: https://github.com/domaindrivendev/rswag/blob/master/rswag-specs/lib/rswag/specs/example_group_helpers.rb
- 2: https://github.com/rswag/rswag/
- 3: https://www.rubydoc.info/gems/rswag-specs/1.0.0/Rswag/Specs/SwaggerFormatter
- 4: https://github.com/rswag/rswag/blob/master/README.md
The first delete operation is silently overridden by the second in the OpenAPI spec.
Defining two delete blocks for /invitations/{id} causes the second operation to overwrite the first in the generated OpenAPI metadata. This results in the 403 response case (student cannot delete) being completely lost from the spec. OpenAPI prohibits duplicate verbs per path, so only the last operation survives. Consolidate both response scenarios into a single delete block to preserve all test cases.
Refactor to one delete operation
- # DELETE /invitations/:id
- delete("Delete invitation") do
- tags "Invitations"
- parameter name: 'Authorization', in: :header, type: :string, required: true, description: 'Bearer token'
-
- response(403, "Student cannot delete invitations") do
- let(:id) { invitation2.id }
- run_test!
- end
- end
-
- delete("Delete invitation") do
+ delete("Delete invitation") do
tags "Invitations"
- parameter name: 'Authorization', in: :header, type: :string, required: true
- let(:Authorization) { "Bearer #{ta_token}" }
+ parameter name: 'Authorization', in: :header, type: :string, required: true, description: 'Bearer token'
+
+ response(403, "Student cannot delete invitations") do
+ let(:id) { invitation2.id }
+ run_test!
+ end
response(200, "Delete successful") do
+ let(:Authorization) { "Bearer #{ta_token}" }
let(:id) { invitation.id }
run_test!
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.
| # DELETE /invitations/:id | |
| delete("Delete invitation") do | |
| tags "Invitations" | |
| parameter name: 'Authorization', in: :header, type: :string, required: true, description: 'Bearer token' | |
| response(403, "Student cannot delete invitations") do | |
| let(:id) { invitation2.id } | |
| run_test! | |
| end | |
| end | |
| delete("Delete invitation") do | |
| tags "Invitations" | |
| parameter name: 'Authorization', in: :header, type: :string, required: true | |
| let(:Authorization) { "Bearer #{ta_token}" } | |
| response(200, "Delete successful") do | |
| let(:id) { invitation.id } | |
| run_test! | |
| end | |
| end | |
| delete("Delete invitation") do | |
| tags "Invitations" | |
| parameter name: 'Authorization', in: :header, type: :string, required: true, description: 'Bearer token' | |
| response(403, "Student cannot delete invitations") do | |
| let(:id) { invitation2.id } | |
| run_test! | |
| end | |
| response(200, "Delete successful") do | |
| let(:Authorization) { "Bearer #{ta_token}" } | |
| let(:id) { invitation.id } | |
| run_test! | |
| end | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/requests/api/v1/invitation_controller_spec.rb` around lines 255 - 275,
Two separate delete("Delete invitation") blocks for the same path overwrite each
other; consolidate into a single delete("Delete invitation") operation that
declares the Authorization header once and contains both response examples
(response(403, ...) using let(:id) { invitation2.id } and response(200, ...)
using let(:id) { invitation.id } with let(:Authorization) { "Bearer #{ta_token}"
} where appropriate) so both test cases are preserved in the OpenAPI metadata
and run_test! is called inside each response block.
1 participant
Implements the server-side quiz workflow for Expertiza. Submitting teams can now author quizzes with per-item correct answers. Reviewers must complete the team's quiz before starting their peer review. Quiz responses are automatically scored at submission time
Summary by CodeRabbit
Release Notes
New Features
Tests