E2619 - Student Quizzes Backend / (diff solved)#349
Conversation
📝 WalkthroughWalkthroughThis PR introduces quiz response functionality and refactors question/item terminology across the application. It adds new API controllers for managing responses, response maps, quiz response maps, and sign-up topics. It extends models to support quiz-specific fields like Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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: 12
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 (1)
app/models/Item.rb (1)
5-27:⚠️ Potential issue | 🟠 MajorRemove
optional: trueand simplifyset_seqto match the database schema.The schema enforces
questionnaire_id NOT NULL, but line 5 setsoptional: trueon the association. This allows Items to pass model validation and execute callbacks (including theset_seqbefore_create hook) without a questionnaire, then fail at the database layer. The fallbackself.seq ||= 1on lines 25-26 masks the invalid state rather than preventing it. Since all actual usage requires a questionnaire and the column isNOT NULL,optional: trueshould be removed andset_seqsimplified to assume the questionnaire is always present.Suggested fix
- belongs_to :questionnaire, optional: true, inverse_of: :items + belongs_to :questionnaire, inverse_of: :itemsdef set_seq - if questionnaire - self.seq ||= questionnaire.items.size + 1 - else - self.seq ||= 1 - end + self.seq ||= questionnaire.items.size + 1 endAlso update or remove the test at spec/models/item_spec.rb:59-63 that expects Items to be valid without a questionnaire.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/Item.rb` around lines 5 - 27, Remove the permissive association and the nil-fallback in set_seq: delete the optional: true option on the belongs_to :questionnaire declaration so Item requires a questionnaire at the model level, and simplify the set_seq method (used by the before_create callback) to assume questionnaire is always present (remove the else branch that sets self.seq ||= 1 and just set self.seq ||= questionnaire.items.size + 1). Also update or remove the spec/models/item_spec.rb test that asserts an Item can be valid without a questionnaire.
🟠 Major comments (30)
app/mailers/invitation_sent_mailer.rb-3-10 (1)
3-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire this mailer into the invitation flow (or remove duplication).
Invitation#send_invite_emailcurrently callsInvitationMailer(app/models/invitation.rb, Lines 8-12), so this newInvitationSentMailerpath appears unused.Potential wiring change (outside this file)
- InvitationMailer.with(invitation: self) + InvitationSentMailer.with(invitation: self) .send_invitation_email .deliver_later🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/mailers/invitation_sent_mailer.rb` around lines 3 - 10, You introduced an unused duplicate mailer InvitationSentMailer while Invitation#send_invite_email currently calls InvitationMailer; either update Invitation#send_invite_email to call InvitationSentMailer (pass params[:invitation] and any required locals) and ensure tests/uses expect that class, or remove InvitationSentMailer and consolidate its logic into InvitationMailer to eliminate duplication; reference InvitationSentMailer, InvitationMailer and the Invitation#send_invite_email method when making the change.spec/factories/reponse.rb-3-3 (1)
3-3:⚠️ Potential issue | 🟠 MajorUse a FactoryBot association for
map_idinstead of hardcodingHardcoded
map_id { 1 }makes factory validity depend on a ResponseMap with id=1 existing in the test database. Useassociation :response_map, factory: :response_mapto ensure each factory instance creates its own related ResponseMap, making the factory self-contained and test-safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/factories/reponse.rb` at line 3, Replace the hardcoded map_id { 1 } in the Response factory with a FactoryBot association so each Response builds its own ResponseMap; specifically, remove map_id { 1 } and add association :response_map, factory: :response_map (or association :response_map if factory name matches) so the factory sets the ResponseMap/response_map_id automatically and no longer depends on an existing DB record.spec/factories/response_maps.rb-3-5 (1)
3-5:⚠️ Potential issue | 🟠 MajorUse associations instead of hardcoded IDs in the ResponseMap factory
The factory hardcodes
reviewed_object_id,reviewer_id, andreviewee_idto1, which makes it brittle. The model definesbelongs_toassociations for these foreign keys, and while the current migration doesn't enforce explicit FK constraints, this approach will fail if records with ID 1 don't exist or if FK constraints are added later. Use FactoryBot associations (matching the pattern inparticipants.rbandassignments.rb) to create linked records instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/factories/response_maps.rb` around lines 3 - 5, The ResponseMap factory currently hardcodes reviewed_object_id, reviewer_id, and reviewee_id to 1; replace those hardcoded foreign-key attributes with FactoryBot associations so the factory builds/creates related records instead (mirror the pattern used in participants.rb and assignments.rb). Update the factory to declare associations for :reviewed_object (or the model name used by the belongs_to), :reviewer, and :reviewee so FactoryBot will generate the linked objects automatically, removing the brittle reliance on ID 1.db/migrate/20250321222753_rename_sign_up_topic_to_project_topic_in_signed_up_teams.rb-3-8 (1)
3-8:⚠️ Potential issue | 🟠 MajorUse explicit
up/downmethods to restore migration reversibilityThis migration is irreversible. Rails'
CommandRecordercannot reverse conditional logic likeif column_exists?(...)insidedef change. On rollback, the condition fails silently (the renamed column no longer matches the old name check), making the rollback a no-op and leaving the schema inconsistent. Additionally,rename_indexshould be guarded to handle schema drift gracefully.Replace
def changewith explicitupanddownmethods:Migration-safe structure
class RenameSignUpTopicToProjectTopicInSignedUpTeams < ActiveRecord::Migration[8.0] - def change - if column_exists?(:signed_up_teams, :sign_up_topic_id) - rename_column :signed_up_teams, :sign_up_topic_id, :project_topic_id - rename_index :signed_up_teams, - :index_signed_up_teams_on_sign_up_topic_id, - :index_signed_up_teams_on_project_topic_id - end + def up + return unless column_exists?(:signed_up_teams, :sign_up_topic_id) + + rename_column :signed_up_teams, :sign_up_topic_id, :project_topic_id + if index_exists?(:signed_up_teams, :index_signed_up_teams_on_sign_up_topic_id) + rename_index :signed_up_teams, + :index_signed_up_teams_on_sign_up_topic_id, + :index_signed_up_teams_on_project_topic_id + end + end + + def down + return unless column_exists?(:signed_up_teams, :project_topic_id) + + rename_column :signed_up_teams, :project_topic_id, :sign_up_topic_id + if index_exists?(:signed_up_teams, :index_signed_up_teams_on_project_topic_id) + rename_index :signed_up_teams, + :index_signed_up_teams_on_project_topic_id, + :index_signed_up_teams_on_sign_up_topic_id + end end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20250321222753_rename_sign_up_topic_to_project_topic_in_signed_up_teams.rb` around lines 3 - 8, The migration uses conditional logic inside def change which makes it irreversible; replace def change with explicit def up and def down methods in the migration class, move the existing conditional rename logic into def up (guarding both rename_column :signed_up_teams, :sign_up_topic_id, :project_topic_id and the associated rename_index call with column_exists? and index_exists? checks) and implement def down to reverse the operations (guarded by column_exists? and index_exists?) to safely rename project_topic_id back to sign_up_topic_id and rename the index back; ensure you reference the exact symbols rename_column, rename_index, column_exists?, and index_exists? when locating the code to change.db/migrate/20260424000001_add_quiz_questionnaire_id_to_teams.rb-7-8 (1)
7-8:⚠️ Potential issue | 🟠 MajorAdd foreign key constraint for
quiz_questionnaire_idThe migration adds the column and index but omits the foreign key constraint. Without it, orphan questionnaire IDs can persist if a questionnaire is deleted. This codebase already enforces FK constraints in similar scenarios (e.g., assignments_duties, invitations, question_advices). Add the constraint to match the established pattern and prevent data integrity issues.
Suggested fix
def change add_column :teams, :quiz_questionnaire_id, :integer, null: true, default: nil add_index :teams, :quiz_questionnaire_id + add_foreign_key :teams, :questionnaires, column: :quiz_questionnaire_id end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20260424000001_add_quiz_questionnaire_id_to_teams.rb` around lines 7 - 8, Add a foreign key constraint for teams.quiz_questionnaire_id to enforce referential integrity: in the migration (where add_column :teams, :quiz_questionnaire_id and add_index :teams, :quiz_questionnaire_id are defined) call add_foreign_key :teams, :quiz_questionnaires, column: :quiz_questionnaire_id, on_delete: :nullify (to clear the FK when the referenced questionnaire is deleted) so orphan IDs cannot persist; place this call after the index declaration to match existing patterns like assignments_duties/invitations/question_advices.config/database.yml-10-18 (1)
10-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnvironment DB isolation is broken when
DATABASE_URLis missing.With the current fallback, all three environments resolve to the same
reimplementationdatabase (thegsubnever applies because fallback has no?). This can cause destructive cross-environment behavior, especially for test and production fallback paths.Suggested fix
development: <<: *default - url: <%= ENV.fetch('DATABASE_URL', 'mysql2://root:expertiza@127.0.0.1:3307/reimplementation').gsub('?', '_development?') %> + url: <%= ENV.fetch('DATABASE_URL_DEVELOPMENT', 'mysql2://root:expertiza@127.0.0.1:3307/reimplementation_development') %> test: <<: *default - url: <%= ENV.fetch('DATABASE_URL', 'mysql2://root:expertiza@127.0.0.1:3307/reimplementation').gsub('?', '_test?') %> + url: <%= ENV.fetch('DATABASE_URL_TEST', 'mysql2://root:expertiza@127.0.0.1:3307/reimplementation_test') %> production: <<: *default - url: <%= ENV.fetch('DATABASE_URL', 'mysql2://root:expertiza@127.0.0.1:3307/reimplementation').gsub('?', '_production?') %> + url: <%= ENV.fetch('DATABASE_URL') %>🤖 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 fallback DATABASE_URLs currently lack a '?' so the .gsub('?', '_development?')/.gsub('?', '_test?')/.gsub('?', '_production?') never runs and all environments point to the same database; update the three url: fallback strings (the ENV.fetch calls that are passed to .gsub in the url entries) to include a query separator before the environment tag (or use distinct fallback URLs per environment) so the gsub can append the proper _development/_test/_production suffix and ensure environment DB isolation.app/models/sign_up_topic.rb-4-5 (1)
4-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire
signed_up_teamstoproject_topic_id.
SignedUpTeambelongs to:project_topicinapp/models/signed_up_team.rb:10-11, so this association is pointing at the wrong foreign key. As written,sign_up_topic.signed_up_teams/teamswill not resolve the actual join rows, anddependent: :destroywill miss them too.Suggested fix
class SignUpTopic < ApplicationRecord - has_many :signed_up_teams, foreign_key: 'topic_id', dependent: :destroy + has_many :signed_up_teams, foreign_key: 'project_topic_id', dependent: :destroy has_many :teams, through: :signed_up_teams # list all teams choose this topic, no matter in waitlist or not has_many :assignment_questionnaires, class_name: 'AssignmentQuestionnaire', foreign_key: 'topic_id', dependent: :destroy has_many :due_dates, as: :parent,class_name: 'DueDate', dependent: :destroy belongs_to :assignment end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/sign_up_topic.rb` around lines 4 - 5, The has_many association in SignUpTopic is using the wrong foreign key; change the association on SignUpTopic (the has_many :signed_up_teams declaration) to use foreign_key: 'project_topic_id' so it matches SignedUpTeam's belongs_to :project_topic and ensures sign_up_topic.signed_up_teams, sign_up_topic.teams and dependent: :destroy operate on the correct join rows.app/controllers/sign_up_topics_controller.rb-66-69 (1)
66-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
destroyhere;each(&:delete)always looks successful and skips cleanup.
eachreturns the collection, so this condition is truthy even if the deletes do the wrong thing, anddeletebypasses callbacks plus thedependent: :destroyassociations onSignUpTopic. That can leave relatedsigned_up_teams,assignment_questionnaires, anddue_datesbehind.Suggested fix
- if `@sign_up_topics.each`(&:delete) - render json: { message: "The topic has been deleted successfully. " }, status: :no_content - else - render json: `@sign_up_topic.errors`, status: :unprocessable_entity - end + SignUpTopic.transaction do + `@sign_up_topics.find_each`(&:destroy!) + end + head :no_content🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/sign_up_topics_controller.rb` around lines 66 - 69, The code uses `@sign_up_topics.each`(&:delete) which is always truthy and bypasses callbacks/dependent destroys; replace this with invoking destroy on each topic and surface failures: iterate `@sign_up_topics` calling destroy (e.g., `@sign_up_topics.each` { |t| t.destroy }), collect any records where t.persisted? or t.errors.present? (use sign_up_topic.errors) and if none failed render no_content with success message, otherwise render the collected errors with status :unprocessable_entity; ensure callbacks and dependent: :destroy run by using destroy, not delete.db/seeds.rb-158-176 (1)
158-176:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSeed canonical item-type values, not only display labels.
These rows use labels like
"Dropdown"and"Multiple choice", but the rest of the app still operates on machine values likedropdown,multiple_choice, andscaleforItem.question_type(spec/models/item_spec.rbLines 79-109). If the frontend round-trips/item_typesdata back into item creation/update, the selected name will not map cleanly to the existing strategy dispatch. Please persist/expose a canonical value alongside the label, or normalize at the API boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/seeds.rb` around lines 158 - 176, The seed currently creates ItemType records using only display labels from item_type_names; update the seeding to persist a canonical machine value for each type so API/DB expose both label and machine-friendly value (e.g., "Dropdown" → "dropdown") instead of relying only on ItemType.find_or_create_by!(name: type_name). Modify the seeding logic that builds item_types (the item_type_names array and the block that calls ItemType.find_or_create_by!) to set a second attribute (e.g., value or key) with normalized snake_case values matching Item.question_type (see spec/models/item_spec.rb usages like dropdown, multiple_choice, scale), ensuring new records include both the human-readable name and the canonical value used by the app or normalize incoming API params to map names to these canonical values at the Item creation/update boundary.db/seeds.rb-139-177 (1)
139-177:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis lookup seeding is still blocked by the non-idempotent inserts above it.
The new
find_or_create_by!blocks are rerunnable, but the earlierInstitution.create!,Role.create!, and other plaincreate!/createcalls will still raise on an already-seeded database before Lines 139-177 are reached. That means a seconddb:seedor a partial seed recovery still won't populate these new lookup tables. Please either make the earlier seed data idempotent too, or move these lookup seeds into their own rerunnable section/task.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/seeds.rb` around lines 139 - 177, The seeding for QuestionnaireType and ItemType is idempotent (using QuestionnaireType.find_or_create_by! and ItemType.find_or_create_by!) but earlier non-idempotent inserts like Institution.create!, Role.create!, and other plain create/create calls will cause a second db:seed to fail before these lookups run; either make those earlier inserts idempotent (replace Institution.create!, Role.create!, etc. with find_or_create_by!/find_or_initialize_by! plus save! or use upsert/upsert_all) or move the questionnaire and item lookup blocks into a separate rerunnable seed/task (e.g., separate seed file or rake task) so they can be run independently—locate the offending calls to Institution.create!, Role.create!, etc., and update them accordingly or extract the QuestionnaireType/ItemType block into its own rerunnable seed.app/models/team.rb-16-18 (1)
16-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce that
quiz_questionnaireactually references a quiz.
TeamsController#set_quiz_questionnairecurrently accepts any existingQuestionnaire, and the quiz-response flow later treatsreviewee_team.quiz_questionnaireas a quiz unconditionally. That means a review/survey questionnaire can be attached here and break the reviewer gate/scoring flow. Please add a model validation that rejects associated questionnaires whosequestionnaire_type != 'Quiz'.Suggested model-level guard
class Team < ApplicationRecord # E2619: each submitting team owns their quiz questionnaire (set via "Create Quiz" on AssignReviewer page) belongs_to :quiz_questionnaire, class_name: 'Questionnaire', foreign_key: 'quiz_questionnaire_id', optional: true, inverse_of: false + validate :quiz_questionnaire_must_be_quiz + + def quiz_questionnaire_must_be_quiz + return if quiz_questionnaire.blank? || quiz_questionnaire.questionnaire_type == 'Quiz' + + errors.add(:quiz_questionnaire, 'must reference a Quiz questionnaire') + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/team.rb` around lines 16 - 18, Add a model-level validation on Team to ensure the associated quiz_questionnaire is actually a quiz: implement a custom validator (e.g., validate :quiz_questionnaire_must_be_quiz) that checks if quiz_questionnaire is present and then rejects the record unless quiz_questionnaire.questionnaire_type == 'Quiz'; update TeamsController#set_quiz_questionnaire handling as needed to surface validation errors rather than silently accepting non-Quiz Questionnaires.app/controllers/teams_controller.rb-90-94 (1)
90-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-quiz questionnaires in
set_quiz_questionnaire.Existence-only validation allows linking arbitrary questionnaire types, which can break quiz-response flow and scoring assumptions.
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) + return render json: { error: 'Questionnaire must be a quiz questionnaire' }, 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 current code only checks existence and may link non-quiz questionnaires; change the validation in set_quiz_questionnaire to ensure the targeted Questionnaire is of the quiz type (e.g., use a scope or attribute check such as Questionnaire.exists?(id: questionnaire_id, kind: 'quiz') or load the record and assert questionnaire.quiz? before updating). Locate the block that currently uses Questionnaire.exists?(id: questionnaire_id), replace it with a check that validates the questionnaire’s quiz type (or load via Questionnaire.find_by(id: questionnaire_id) and return the same not_found error if nil or not a quiz), then only call `@team.update`!(quiz_questionnaire_id: questionnaire_id) and render on success. Ensure the error response remains { error: 'Questionnaire not found' }/status :not_found or adjust message to indicate invalid quiz type as needed.app/controllers/items_controller.rb-6-13 (1)
6-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove raw item/JSON logging from this request path.
Logging full attributes/payloads can leak quiz answers and creates noisy request-time logs.
Suggested fix
- 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}"🤖 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 request-time raw payload logging in ItemsController where Rails.logger is used to print questionnaire.items, item.attributes.inspect, and items_json.inspect; delete those Rails.logger.info lines (or replace them with a single non-sensitive debug log that does not include item attributes or answers) so the controller no longer emits full item hashes/JSON for the questionnaire variable or the items_json local; ensure any remaining logs reference only non-sensitive identifiers (e.g., questionnaire.id) and not item attributes or payload content.db/schema.rb-107-126 (1)
107-126:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThese new assignment columns duplicate existing settings.
This migration leaves pairs like
show_teammate_reviews/show_teammate_review,enable_pair_programming/is_pair_programming,max_reviews_per_submission/maximum_number_of_reviews_per_submission,review_assignment_strategy/review_strategy, androunds_of_reviews/number_of_review_roundsin the same table. That creates two sources of truth, andAssignmentsController#assignment_paramsis already writing the new names. Unless the rest of the model/serializer layer is fully migrated in lockstep, reads and writes will diverge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/schema.rb` around lines 107 - 126, The migration introduced duplicate columns (e.g., show_teammate_review vs show_teammate_reviews, is_pair_programming vs enable_pair_programming, maximum_number_of_reviews_per_submission vs max_reviews_per_submission, review_strategy vs review_assignment_strategy, number_of_review_rounds vs rounds_of_reviews) causing two sources of truth while AssignmentsController#assignment_params already writes the new names; remove or consolidate the duplicated columns so only the canonical column names used by AssignmentsController#assignment_params remain, update any model attributes/serializers referencing the old names to the canonical names, and create a migration that backfills values from the old column to the chosen canonical column (or drops the old columns after backfill) to ensure reads and writes stay consistent.app/controllers/questionnaires_controller.rb-33-34 (1)
33-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't expose backtraces in API responses.
These lines return raw exception messages and stack frames to the client. That leaks internal implementation details and file paths on every unexpected failure. Log the exception server-side and return a generic 500 payload instead.
Suggested fix
- rescue => e - render json: { error: e.message, backtrace: e.backtrace.take(5) }, status: :internal_server_error + rescue StandardError => e + Rails.logger.error(e.full_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, Replace the current rescue block that renders exception message and backtrace (the lines with "rescue => e" and "render json: { error: e.message, backtrace: e.backtrace.take(5) }, status: :internal_server_error") so that the exception is logged server-side (e.g., Rails.logger.error or logger.error including e.message and e.backtrace) and the controller returns a generic JSON 500 response such as render json: { error: "Internal server error" }, status: :internal_server_error; keep the logging detailed but do not expose e.message or backtrace to the client.db/schema.rb-422-422 (1)
422-422:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an index and foreign key for
teams.quiz_questionnaire_id.This is a new relationship-looking column, but the schema leaves it unchecked and unindexed. That allows dangling questionnaire IDs and makes team→quiz lookups slower as
teamsgrows.Suggested migration follow-up
+ add_index :teams, :quiz_questionnaire_id + add_foreign_key :teams, :questionnaires, column: :quiz_questionnaire_id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/schema.rb` at line 422, Add an index and a foreign key constraint for the teams.quiz_questionnaire_id column: create a migration that runs add_index :teams, :quiz_questionnaire_id (optionally name the index) and add_foreign_key :teams, :quiz_questionnaires, column: :quiz_questionnaire_id, on_delete: :restrict (or :nullify if you prefer nulling on delete); before adding the foreign key, scan/backfill or remove any dangling quiz_questionnaire_id values in teams so the constraint will apply cleanly.app/serializers/questionnaire_serializer.rb-6-11 (1)
6-11:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the
instructorpayload shape stable.This serializer changes
instructorfrom an object-with-idfallback to eithernilor{ name, email }. That can break existing clients that dereferencequestionnaire.instructor.idor assume the key is always an object.Suggested fix
def instructor inst = object.instructor - return nil unless inst - { name: inst.name, email: inst.email } + return { id: nil, name: nil, email: nil } unless inst + + { id: inst.id, name: inst.name, email: inst.email } end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/serializers/questionnaire_serializer.rb` around lines 6 - 11, The instructor serializer currently returns nil or a {name, email} hash which breaks clients expecting an object with an id; update the instructor method (in questionnaire_serializer.rb) to preserve the original payload shape by always returning an object-like hash for instructor: include id, name and email keys (use object.instructor.id, .name, .email) and when no instructor exists return the same object-shaped structure with nil values (e.g. id: nil, name: nil, email: nil) so existing consumers can safely dereference questionnaire.instructor.id.app/controllers/questionnaires_controller.rb-41-44 (1)
41-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
destroy!failures explicitly.Switching to
destroy!is good for honoring dependencies, but this action only rescuesActiveRecord::RecordNotFound. If a callback or dependent destroy blocks deletion, Rails will now raiseActiveRecord::RecordNotDestroyedand return a 500 instead of a 422-style API error.Suggested fix
def destroy begin `@questionnaire` = Questionnaire.find(params[:id]) `@questionnaire.destroy`! # ensures dependent items are removed rescue ActiveRecord::RecordNotFound render json: $ERROR_INFO.to_s, status: :not_found and return + rescue ActiveRecord::RecordNotDestroyed => e + render json: { errors: e.record.errors.full_messages }, status: :unprocessable_entity 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 41 - 44, The destroy action in QuestionnairesController currently rescues only ActiveRecord::RecordNotFound but not ActiveRecord::RecordNotDestroyed, so any callback/dependent failure will raise and return 500; update the begin/rescue around Questionnaire.find(params[:id]) and `@questionnaire.destroy`! to also rescue ActiveRecord::RecordNotDestroyed (or capture the exception), and render a 422 Unprocessable Entity response with the failure details (use exception.record.errors.full_messages or `@questionnaire.errors.full_messages`) instead of letting it bubble to a 500; keep the RecordNotFound rescue as-is for 404 handling.app/models/questionnaire.rb-18-19 (1)
18-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat
QuizQuestionnaireas quiz-owned too.This exemption only skips
instructor_idvalidation forquestionnaire_type == 'Quiz', but the rest of the codebase already treatsQuizQuestionnaireas a valid quiz type as well. Those records will still fail validation here, so one of the supported quiz spellings remains broken.🤖 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 validation currently exempts only questionnaire_type == 'Quiz', causing QuizQuestionnaire records to fail; update the presence validation for instructor_id in app/models/questionnaire.rb to skip when the questionnaire is a quiz by checking both 'Quiz' and 'QuizQuestionnaire' (e.g., change the unless lambda to test questionnaire_type.in?(['Quiz','QuizQuestionnaire']) or equivalent) so QuizQuestionnaire is treated as a quiz-owned type like Quiz.swagger/v1/swagger.yaml-1932-1954 (1)
1932-1954:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the duplicate
student_taskspath entries.
/student_tasks/listand/student_tasks/vieware already declared earlier in this file. Re-declaring them here with different shapes makes the spec ambiguous, and most parsers will keep only one copy, so part of the API contract silently disappears.🤖 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, Remove the duplicate OpenAPI path entries for /student_tasks/list and /student_tasks/view from this swagger.yaml fragment: locate the second occurrences of the paths named "/student_tasks/list" and "/student_tasks/view" (the blocks with summary "List all Student Tasks" and "View a student task") and delete those duplicate path objects so only the original definitions remain; ensure no other references rely on these duplicated shapes and run the OpenAPI validator to confirm the spec is unambiguous.app/models/response.rb-38-60 (1)
38-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis changes the peer baseline from “average of percentages” to “percentage of summed points.”
When peers answer different subsets,
total_numerator / total_denominatorweights responses with largermaximum_scoremore heavily. The discrepancy check below is comparing against the mean peer percentage, so this can suppress or trigger notifications incorrectly. Compute each peer’s normalized score first, then average those values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/response.rb` around lines 38 - 60, The current aggregation (using total_numerator/total_denominator) biases peers with larger maximum_score; instead, for each response in existing_responses (skipping self via id == response.id) compute that peer's normalized score by dividing response.aggregate_questionnaire_score by response.maximum_score (guarding against zero max), accumulate those per-peer normalized scores and increment count, then set average_score = (sum_of_normalized_scores / count). Update the block that builds total_numerator/total_denominator and the average_score calculation to use this per-peer normalization approach (referencing existing_responses, response.aggregate_questionnaire_score, response.maximum_score, count, and average_score).app/serializers/assignment_serializer.rb-19-21 (1)
19-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreload these associations before using this serializer by default.
Adding
assignment_questionnaires,questionnaires, anddue_dateshere means the default assignment render path now needs those associations eagerly loaded.AssignmentsController#indexstill doesAssignment.all, andshowstill doesAssignment.find(...), so this turns assignment listing into an N+1 query path once there is real data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/serializers/assignment_serializer.rb` around lines 19 - 21, The serializer declares has_many :assignment_questionnaires, :questionnaires, and :due_dates which forces eager loading; fix the N+1 by preloading these associations where assignments are fetched: update AssignmentsController#index to use Assignment.includes(:assignment_questionnaires, :questionnaires, :due_dates).all (or the equivalent scope) and update AssignmentsController#show to use Assignment.includes(...).find(id) (or use a scope that includes them) so the AssignmentSerializer can safely rely on those associations without causing N+1 queries.swagger/v1/swagger.yaml-1955-2078 (1)
1955-2078:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe
responsessection no longer matchesResponsesController.
ResponsesController#createlooks upmap_id/response_map_id, notuser_id,questionnaire_id, orcontent, and it can return200when reusing an existing draft.updateexpects a nestedresponsepayload withscores_attributes, whilesubmit/unsubmitdo not consume thecontentbody shown here. The unsubmit path should also mirror the real route placeholder style, not a literal:idsegment. As written, generated docs/clients will call the wrong contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swagger/v1/swagger.yaml` around lines 1955 - 2078, The OpenAPI spec for Responses endpoints is out of sync with ResponsesController: update the POST /responses requestBody to require map_id/response_map_id (not user_id/questionnaire_id/content) and reflect that create (ResponsesController#create) may return 200 when reusing an existing draft in addition to 201; change the PATCH /responses/{id} requestBody to accept a nested "response" object with "scores_attributes" (matching update), remove the unused "content" body from the PATCH /responses/{id}/submit and PATCH /responses/{id}/unsubmit operations (they do not consume content), and fix the unsubmit path to use the {id} placeholder (not ":id"); ensure response status codes mirror the controller (e.g., create 200/201, update 200/403/404/422, submit/unsubmit codes as implemented).app/controllers/responses_controller.rb-225-248 (1)
225-248:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPick the due date for this response type/round, not just the first upcoming date.
Assignments can have multiple deadlines. This method ignores the map type and round and gates submission on whichever due date happens to be
upcoming.firstorfirst, so a review can be incorrectly opened or closed based on an unrelated submission/metareview phase.🤖 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 225 - 248, The method submission_window_open? is currently checking only the first/upcoming due date; instead pick the due date that matches this response's response_map (round and map type) before comparing times. Locate response.response_map (and its round and map_type/type attributes) and assignment.due_dates, then find the specific due_date where due_date.round == response.response_map.round (and matches the map_type/deadline_type if your model uses that field) — if none is found treat as open (return true), otherwise compare that due_date.due_at > Time.current; apply the same lookup when using due_dates.upcoming or legacy future? collections so you check the matching round/type entry not just the first element.app/controllers/quiz_response_maps_controller.rb-81-94 (1)
81-94:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis idempotent create is still racy.
find_byfollowed bynew/saveallows two concurrent requests to create duplicateQuizResponseMaps for the same reviewer/questionnaire pair. If this endpoint can be retried from the UI, protect it with a unique index and switch tofind_or_create_by!/create_or_find_by!on that key.🤖 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 81 - 94, The current find_by + new + save(validate: false) flow for QuizResponseMap is racy and can produce duplicates under concurrent requests; add a unique DB constraint on the tuple (reviewed_object_id, reviewer_id, reviewee_id) and replace the find/new/save sequence with an atomic finder/creator call such as QuizResponseMap.create_or_find_by!(reviewed_object_id: quiz_questionnaire.id, reviewer_id: reviewer_participant.id, reviewee_id: reviewer_participant.id) (or find_or_create_by! depending on Rails version) to rely on the DB uniqueness to prevent duplicates and surface errors if creation fails.app/models/student_task.rb-65-78 (1)
65-78:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not collapse quiz state to the first review map.
This code picks one arbitrary
ReviewResponseMapand treats that team's quiz as the assignment's quiz. That conflicts with the per-map design already documented inapp/controllers/response_maps_controller.rbLines 58-60 ("quiz_taken must be checked per map, not per assignment"). A reviewer with multiple assigned teams can therefore get the wrongquiz_questionnaire_idor appear quiz-complete after finishing only one team's quiz.🤖 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 - 78, The code incorrectly collapses quiz state by picking the first ReviewResponseMap; instead iterate over all ReviewResponseMap records for the reviewer (ReviewResponseMap.where(reviewer_id: participant.id)) and for each map resolve its team (Team.find_by(id: rm.reviewee_id)) to get that map's quiz_questionnaire_id, then check quiz_taken per map by querying QuizResponseMap.where(reviewer_id: participant.id, reviewed_object_id: quiz_questionnaire_id).joins("INNER JOIN responses ON responses.map_id = response_maps.id").where(responses: { is_submitted: true }).exists?; replace the existing single quiz_questionnaire_id/has_quiz/quiz_taken logic with per-map checks (e.g., produce a hash or list of maps -> { has_quiz:, quiz_taken: } or otherwise propagate per-map results) so you no longer infer quiz state from the first map.app/controllers/response_maps_controller.rb-40-53 (1)
40-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope this query to
ReviewResponseMap.
ResponseMap.where(reviewer_id: ...)is broader than the endpoint contract here. Filtering outreviewer_id == reviewee_idonly removes quiz/self maps; other response-map subclasses can still leak into this payload and be serialized as peer reviews if theirreviewed_object_idhappens to resolve to an assignment. That will confuse the row-level quiz gating state.Suggested change
- map_scope = ResponseMap.where(reviewer_id: participants.map(&:id)) + map_scope = ReviewResponseMap.where(reviewer_id: participants.map(&:id)) map_scope = map_scope.where(reviewed_object_id: assignment_id) if assignment_id maps = map_scope.to_a🤖 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 40 - 53, The query currently uses ResponseMap.where(reviewer_id: participants.map(&:id)) which is too broad and may return non-review maps; change it to explicitly query ReviewResponseMap (e.g., ReviewResponseMap.where(reviewer_id: ...)) so only peer review maps are returned, keeping the subsequent guards (map.reviewer_id == map.reviewee_id and Assignment.find_by(id: map.reviewed_object_id)) intact; update any related variable names or comments around map_scope and maps if needed to reflect that the scope is now specifically ReviewResponseMap.app/models/response_map.rb-144-160 (1)
144-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter to review deadlines when inferring the current round.
assignment.due_datesincludes more than review deadlines, but this method treats any dated row with a round as eligible.Assignment#num_review_roundsinapp/models/assignment.rbLines 31-34 already filters onDueDate::REVIEW_DEADLINE_TYPE_ID; this code should do the same orneeds_update_link?can advance to a later round off a submission/team deadline and showUpdatetoo early.Suggested change
due_dates = Array(assignment.due_dates).select do |d| - d.respond_to?(:round) && d.round.present? && + d.respond_to?(:deadline_type_id) && + d.deadline_type_id == DueDate::REVIEW_DEADLINE_TYPE_ID && + d.respond_to?(:round) && d.round.present? && d.respond_to?(:due_at) && d.due_at.present? end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/response_map.rb` around lines 144 - 160, Filter the due_dates to only review deadlines before computing the reference round: when building due_dates from assignment.due_dates, apply the same filter used in Assignment#num_review_rounds (e.g. select entries where deadline_type_id == DueDate::REVIEW_DEADLINE_TYPE_ID) in addition to the existing round/due_at checks so that the logic in needs_update_link? and the final reference.round.to_i only considers review deadlines.app/controllers/response_maps_controller.rb-121-137 (1)
121-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the assignment/team pair before creating the map.
This action only checks that the ids are non-zero. As written, it can create an
AssignmentParticipantandReviewResponseMapfor a missing assignment, or link a reviewer to a team from a different assignment. That leaves orphaned mappings and bypasses assignment scoping.Suggested change
+ assignment = Assignment.find_by(id: assignment_id) + return render json: { error: 'Assignment not found' }, status: :not_found unless assignment + + reviewee_team = Team.find_by(id: reviewee_team_id, parent_id: assignment_id) + return render json: { error: 'Reviewee team not found for assignment' }, + status: :unprocessable_entity unless reviewee_team + # Find or create the reviewer's participant record for this assignment reviewer_participant = AssignmentParticipant.find_by(user_id: reviewer_user_id, parent_id: assignment_id) @@ map = ReviewResponseMap.find_or_create_by!( reviewed_object_id: assignment_id, reviewer_id: reviewer_participant.id, - reviewee_id: reviewee_team_id + reviewee_id: reviewee_team.id )🤖 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 121 - 137, Before creating AssignmentParticipant or ReviewResponseMap, verify the assignment exists and that the reviewee team belongs to that assignment: fetch Assignment.find_by(id: assignment_id) and fetch the team via Team.find_by(id: reviewee_team_id, parent_id: assignment_id) (or equivalent) and return/error if either is nil; only then proceed to create or find AssignmentParticipant (reviewer_participant) and ReviewResponseMap, ensuring ReviewResponseMap.lookup uses the validated reviewer_participant.id and the validated reviewee_team_id so you never create a participant or map for a missing assignment or a team from another assignment.app/models/response_map.rb-123-132 (1)
123-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
ReviewResponseMap#revieweewhen it is already aTeam.For peer reviews,
revieweeis aTeam(app/models/review_response_map.rbLines 3-4), so thereviewee.respond_to?(:team)branch never runs. That meansteams_participants.updated_atis ignored for the normal review path, and teammate changes will not flipneeds_update_link?even though this method says they should.Suggested change
- # Check team-related timestamps if the reviewee has a team - if reviewee.respond_to?(:team) && reviewee.team - team = reviewee.team + team = + if reviewee.is_a?(Team) + reviewee + elsif reviewee.respond_to?(:team) + reviewee.team + end + + # Check team-related timestamps if the reviewee has a team + if team candidates << team.updated_at if team.respond_to?(:updated_at) && team.updated_at.present? - # Also gather timestamps from join records (teams_participants) so collaborator edits count as activity + # Also gather timestamps from join records (teams_participants) so collaborator edits count as activity if team.respond_to?(:teams_participants) team.teams_participants.each do |tp| candidates << tp.updated_at if tp.respond_to?(:updated_at) && tp.updated_at.present?🤖 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 123 - 132, The code only checks reviewee.respond_to?(:team) so when reviewee is already a Team (peer reviews) its teams_participants timestamps are skipped; update the logic in ReviewResponseMap (where reviewee is accessed) to treat a reviewee that is a Team the same as reviewee.team—e.g. resolve a local team variable with team = (reviewee.respond_to?(:team) && reviewee.team) || (reviewee.respond_to?(:teams_participants) ? reviewee : nil) and then collect team.updated_at and team.teams_participants.each { |tp| tp.updated_at } if present; ensure this change is used by needs_update_link? so teammate edits are considered for peer reviews.
🟡 Minor comments (1)
app/models/student_task.rb-55-57 (1)
55-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
participantbefore dereferencing it.
from_participant_idpassesParticipant.find_by(id: id)into this method, so an unknown id currently raises on Line 56 instead of returningnilas the method contract says.Suggested change
def self.create_from_participant(participant) + return nil if participant.nil? + asgn = participant.assignment return nil if asgn.nil?🤖 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 55 - 57, The method create_from_participant should guard against a nil participant before dereferencing it: add an early return (e.g., return nil) at the top of create_from_participant if participant.nil? so that when from_participant_id passes Participant.find_by(id: id) the method honors its contract and returns nil instead of raising; update the create_from_participant method (and any callers like from_participant_id) to check participant.nil? before using participant.assignment.
🧹 Nitpick comments (7)
spec/factories/project_topics.rb (1)
1-9: 💤 Low valueLGTM — factory logic unchanged; minor EOF newline nit.
The
:project_topicfactory definition looks correct (sequences/attributes/association intact). If this PR only normalized CRLF→LF and removed the trailing newline at EOF, consider re-adding the final newline to keep POSIX/tooling/lint expectations consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/factories/project_topics.rb` around lines 1 - 9, The file currently lacks a trailing newline at EOF; open the spec/factories file containing the factory :project_topic (the block that defines factory :project_topic and sequences like sequence(:topic_name) and sequence(:topic_identifier)) and add a single final newline character at the end of the file so the file ends with a newline to satisfy POSIX/tooling/lint expectations.app/mailers/invitation_sent_mailer.rb (1)
4-4: ⚡ Quick winUse environment-configured sender instead of a hardcoded placeholder.
Line 4 hardcodes
from@example.com; prefer app credentials/config so non-dev environments don’t send with a placeholder address.Proposed refactor
- default from: 'from@example.com' + default from: Rails.application.credentials.dig(:mail, :from_address) || ENV.fetch('MAIL_FROM_ADDRESS')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/mailers/invitation_sent_mailer.rb` at line 4, The mailer currently hardcodes the sender in the InvitationSentMailer via "default from: 'from@example.com'"; replace that with an environment/credentials-backed value (for example use Rails.application.credentials.dig(:email, :default_from) or ENV['DEFAULT_FROM_EMAIL']) and update the default declaration in InvitationSentMailer to reference that config value so non-dev environments use the proper sender address.app/models/questionnaire_type.rb (1)
2-3: Add a unique database index onquestionnaire_types.name.The model validation
validates :name, presence: true, uniqueness: trueis not race-safe. Concurrent inserts can still create duplicate lookup rows that will be exposed by the/questionnaire_typesendpoint. Addadd_index :questionnaire_types, :name, unique: trueto the migration to enforce this at the database level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/questionnaire_type.rb` around lines 2 - 3, The uniqueness validation on QuestionnaireType (validates :name, presence: true, uniqueness: true) is not race-safe; add a unique DB constraint by creating a migration that runs add_index :questionnaire_types, :name, unique: true (or alter the existing migration if it hasn't been run) so the database enforces uniqueness; name the migration clearly (e.g., add_unique_index_to_questionnaire_types_name), run/rollback and migrate to apply, and ensure schema.rb is updated so the database and the model validation stay consistent.app/controllers/questions_controller.rb (1)
41-47: ⚡ Quick winUse a single source of truth for quiz item types.
This allowlist is duplicated from
Item::QUIZ_ITEM_TYPES; drift here can create hard-to-debug mismatches between create validation and model behavior.Suggested refactor
- QUIZ_ITEM_TYPES = %w[TextField MultipleChoiceRadio MultipleChoiceCheckbox Scale Checkbox].freeze + QUIZ_ITEM_TYPES = Item::QUIZ_ITEM_TYPES🤖 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 41 - 47, The QUIZ_ITEM_TYPES constant in QuestionsController duplicates Item::QUIZ_ITEM_TYPES; remove the duplicated %w[...] constant and replace all uses of QUIZ_ITEM_TYPES in this controller with the canonical Item::QUIZ_ITEM_TYPES (or delegate a private method like quiz_item_types that returns Item::QUIZ_ITEM_TYPES) so the controller uses a single source of truth (reference symbols: QUIZ_ITEM_TYPES, Item::QUIZ_ITEM_TYPES, QuestionsController).spec/controllers/responses_controller_spec.rb (1)
192-212: ⚡ Quick winAssert the auto-scoring side effect in
submit.This example stubs
aggregate_questionnaire_scoreto42, but never checks that the firstPATCHactually persisted or returned that score. Since automatic scoring is one of this PR’s headline behaviors, I'd add an expectation on the saved attribute or response payload here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/controllers/responses_controller_spec.rb` around lines 192 - 212, The test stubs aggregate_questionnaire_score to return 42 but never asserts that the score was applied on the first PATCH; update the spec to verify the auto-scoring side effect after the first patch by either parsing the first response payload or confirming the double was saved with the score: call patch :submit, params: { id: 1 } then parse JSON.parse(response.body) and expect the returned payload to include the aggregate score (e.g. ['aggregate_questionnaire_score'] == 42), or change the response_double.as_json stub to include the score and assert that as_json contains 42; you can also add an expectation that response_double.aggregate_questionnaire_score was invoked and that response_double.save was called after setting is_submitted via is_submitted=.spec/requests/api/v1/response_maps_controller_spec.rb (2)
103-112: ⚡ Quick winActually assert that quiz maps are excluded.
This example proves
review_mapis returned, but it never checks thatquiz_map.idis absent. It would still pass if the endpoint returned both entries, which is the regression this case is supposed to catch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/api/v1/response_maps_controller_spec.rb` around lines 103 - 112, The test currently asserts that review_map.id is present but does not check quiz_map is excluded; update the example in response_maps_controller_spec.rb (the response(200, 'returns peer-review maps excluding quiz maps') block) to parse response.body into maps (already stored in maps) and add an assertion like expect(maps.map { |m| m['id'] }).not_to include(quiz_map.id) so the spec fails if quiz maps are returned; keep the existing checks for review_map.id and the Array type.
211-235: ⚡ Quick winTighten the idempotency check to prove reuse.
Asserting that the response
idis an integer does not prove the existing map was reused. This should also verify that the returned id matches the pre-created map and that the(reviewed_object_id, reviewer_id, reviewee_id)tuple still exists only once after the request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/api/v1/response_maps_controller_spec.rb` around lines 211 - 235, The test currently only asserts the response id is an Integer; update the before block to capture the pre-created ReviewResponseMap (use the ReviewResponseMap.find_or_create_by! call result into a local variable or let, e.g. precreated_map) and then in the run_test! block assert that JSON.parse(response.body)['id'] equals precreated_map.id and that ReviewResponseMap.where(reviewed_object_id: assignment.id, reviewer_id: precreated_map.reviewer_id, reviewee_id: precreated_map.reviewee_id).count == 1 to ensure the tuple was reused and not duplicated.
| # 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.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "assignments_controller.rb" | head -5Repository: expertiza/reimplementation-back-end
Length of output: 123
🏁 Script executed:
git ls-files | grep -E "(assignments_controller|assignment.*spec)" | head -20Repository: expertiza/reimplementation-back-end
Length of output: 338
🏁 Script executed:
wc -l app/controllers/assignments_controller.rbRepository: expertiza/reimplementation-back-end
Length of output: 125
🏁 Script executed:
head -60 app/controllers/assignments_controller.rb | cat -nRepository: expertiza/reimplementation-back-end
Length of output: 2426
🏁 Script executed:
grep -n "assignment_questionnaires_attributes" test/controllers/assignments_controller_test.rb spec/requests/api/v1/assignment_controller_spec.rb 2>/dev/null | head -20Repository: expertiza/reimplementation-back-end
Length of output: 61
🏁 Script executed:
grep -r "assignment_questionnaires_attributes" spec/ test/ --include="*.rb" | head -15Repository: expertiza/reimplementation-back-end
Length of output: 61
🏁 Script executed:
sed -n '26,43p' app/controllers/assignments_controller.rb | cat -nRepository: expertiza/reimplementation-back-end
Length of output: 918
🏁 Script executed:
grep -n "accepts_nested_attributes_for" app/models/assignment.rbRepository: expertiza/reimplementation-back-end
Length of output: 162
🏁 Script executed:
cat app/models/assignment.rb | grep -A 10 "accepts_nested_attributes_for"Repository: expertiza/reimplementation-back-end
Length of output: 892
🏁 Script executed:
grep -n "def assignment_params" app/controllers/assignments_controller.rb -A 15Repository: expertiza/reimplementation-back-end
Length of output: 501
🏁 Script executed:
cat test/controllers/assignments_controller_test.rb | head -100Repository: expertiza/reimplementation-back-end
Length of output: 253
🏁 Script executed:
wc -l spec/requests/api/v1/assignment_controller_spec.rbRepository: expertiza/reimplementation-back-end
Length of output: 134
🏁 Script executed:
head -100 spec/requests/api/v1/assignment_controller_spec.rb | cat -nRepository: expertiza/reimplementation-back-end
Length of output: 4129
🏁 Script executed:
grep -n "assignment_questionnaires" spec/requests/api/v1/assignment_controller_spec.rb | head -20Repository: expertiza/reimplementation-back-end
Length of output: 61
🏁 Script executed:
sed -n '200,300p' spec/requests/api/v1/assignment_controller_spec.rb | cat -nRepository: expertiza/reimplementation-back-end
Length of output: 4223
🏁 Script executed:
grep -n "patch\|put" spec/requests/api/v1/assignment_controller_spec.rb -i | head -20Repository: expertiza/reimplementation-back-end
Length of output: 324
🏁 Script executed:
sed -n '130,180p' spec/requests/api/v1/assignment_controller_spec.rb | cat -nRepository: expertiza/reimplementation-back-end
Length of output: 2190
🏁 Script executed:
grep -rn "assignment_questionnaires_attributes" . --include="*.rb" --include="*.json"Repository: expertiza/reimplementation-back-end
Length of output: 615
🏁 Script executed:
sed -n '224,250p' app/controllers/assignments_controller.rb | cat -nRepository: expertiza/reimplementation-back-end
Length of output: 990
🏁 Script executed:
web_search
How do Rails nested attributes arrive in params - Array or Hash?Repository: expertiza/reimplementation-back-end
Length of output: 172
🏁 Script executed:
grep -rn "assignment_questionnaires" . --include="*.js" --include="*.jsx" --include="*.ts" | head -20Repository: expertiza/reimplementation-back-end
Length of output: 61
🌐 Web query:
Rails nested attributes params format Array vs Hash ActionController::Parameters
💡 Result:
In Ruby on Rails, nested attributes for models using accepts_nested_attributes_for (typically for has_many associations) expect parameters in a specific format within ActionController::Parameters. From HTML forms using fields_for, Rails generates params as a hash with string integer keys (e.g., {"0" => {...}, "1" => {...}}), not a true array. This represents an indexed collection suitable for HTTP form submissions. ActiveRecord accepts both formats for nested attributes: - Hash of hashes: posts_attributes: {"0" => {title: "A"}, "1" => {title: "B"}} - Array of hashes: posts_attributes: [{title: "A"}, {title: "B"}] The keys in the hash form are ignored (except 'id' keys trigger special array wrapping), converting to an array internally. For strong parameters with permit (traditional): - Use array syntax: params.require(:post).permit(:title, posts_attributes: [:body, :id, :_destroy]) - This permits both hash (with numeric string keys from forms) and array formats. The rule of thumb: "To permit a hash, pass an array" applies here—the permit arg [:body] permits each nested hash's permitted keys regardless of outer structure. For newer expect (Rails 7+ preferred, stricter): - For arrays of objects: params.expect(post: [:title, posts_attributes: [[:body]]]) (double brackets) - Single bracket [:body] may strip nested hash from fields_for; double [[:body]] explicitly expects array of hashes. Example form-generated params: { post: { title: "Test", comments_attributes: { "0" => {body: "First"}, "1" => {body: "Second"} } } } Both permit and expect with correct syntax pass this to the model successfully. APIs may send JSON arrays directly. Sources confirm this dual acceptance (Rails API docs) and form generation (Guides). As of Rails 8 (2026), behavior unchanged.
Citations:
- 1:
params.expectdoesn't handle nested attributes hash format fromfields_forrails/rails#56702 - 2: https://api.rubyonrails.org/v8.0.2/classes/ActiveRecord/NestedAttributes/ClassMethods.html
- 3: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/nested_attributes.rb
- 4: https://api.rubyonrails.org/v8.0.0.1/classes/ActionController/Parameters.html
- 5: https://api.rubyonrails.org/v7.1.2/classes/ActiveRecord/NestedAttributes/ClassMethods.html
- 6: https://api.rubyonrails.org/classes/ActionController/StrongParameters.html
- 7: https://guides.rubyonrails.org/v7.2/form_helpers.html
- 8: https://guides.rubyonrails.org/action_controller_overview.html
- 9: https://guides.rubyonrails.org/v7.0.4.2/form_helpers.html
🏁 Script executed:
grep -rn "assignment_questionnaires" app/models/ --include="*.rb" -B 2 -A 2Repository: expertiza/reimplementation-back-end
Length of output: 2750
🏁 Script executed:
python3 << 'EOF'
# Let's trace what types params can be when nested attributes are sent
import json
# When form-generated params arrive in Rails:
form_params = {
"assignment": {
"assignment_questionnaires_attributes": {
"0": {"id": 1, "questionnaire_id": 5},
"1": {"id": 2, "questionnaire_id": 6}
}
}
}
# When JSON API sends it:
api_params = {
"assignment": {
"assignment_questionnaires_attributes": [
{"id": 1, "questionnaire_id": 5},
{"id": 2, "questionnaire_id": 6}
]
}
}
print("Form-generated (Hash):")
incoming_form = form_params["assignment"]["assignment_questionnaires_attributes"]
print(f" Type: {type(incoming_form)}")
print(f" is_a?(Array): {isinstance(incoming_form, list)}")
print(f" Values exist with ids: {any(v.get('id') for v in incoming_form.values() if isinstance(v, dict))}")
print("\nJSON API (Array):")
incoming_api = api_params["assignment"]["assignment_questionnaires_attributes"]
print(f" Type: {type(incoming_api)}")
print(f" is_a?(Array): {isinstance(incoming_api, list)}")
print(f" Values exist with ids: {any(v.get('id') for v in incoming_api if isinstance(v, dict))}")
EOFRepository: expertiza/reimplementation-back-end
Length of output: 270
🏁 Script executed:
grep -rn "assignment_questionnaires_attributes" . --include="*.ts" --include="*.tsx" --include="*.js" --include="*.jsx" 2>/dev/null | head -20Repository: expertiza/reimplementation-back-end
Length of output: 61
🏁 Script executed:
find . -type f -name "*.ts" -o -name "*.tsx" -o -name "*.js" | xargs grep -l "assignment" | grep -i "form\|edit" | head -5Repository: expertiza/reimplementation-back-end
Length of output: 61
🏁 Script executed:
sed -n '26,43p' app/controllers/assignments_controller.rb | cat -nRepository: expertiza/reimplementation-back-end
Length of output: 918
Fix destructive preprocessing logic to handle both Hash and Array nested attribute formats.
The type check on line 34 only recognizes Array input, but Rails commonly sends nested attributes as an indexed Hash (e.g., from HTML forms: {"0" => {...}, "1" => {...}}). In that case, has_ids is false even when IDs are present, triggering unwanted deletion of all existing questionnaires. Additionally, because deletion occurs before the update call on line 38, any subsequent validation failure leaves the assignment permanently without its questionnaires.
Suggested fix
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
+ incoming_values =
+ case incoming
+ when Array
+ incoming
+ when ActionController::Parameters, Hash
+ incoming.values
+ else
+ []
+ end
+
+ has_ids = incoming_values.any? { |aq| aq[:id].present? || aq['id'].present? }
+
+ Assignment.transaction do
+ assignment.assignment_questionnaires.destroy_all unless has_ids
+ raise ActiveRecord::Rollback unless assignment.update(assignment_params)
+ end
end🤖 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 current
preprocessing treats incoming nested attributes as Array only, causing false
negatives when Rails sends an indexed Hash and leading to premature destroy_all
on assignment.assignment_questionnaires; update the logic to normalize incoming
= params[:assignment][:assignment_questionnaires_attributes] into a values array
(incoming.is_a?(Hash) ? incoming.values : incoming) and compute has_ids by
checking both string and symbol keys (e.g., any? { |aq| aq[:id].present? ||
aq['id'].present? }), and move the destructive
assignment.assignment_questionnaires.destroy_all so it runs only after a
successful assignment.update(...) (or wrap both in a transaction) to avoid
losing questionnaires on validation failure.
| items_json = questionnaire.items.as_json | ||
| Rails.logger.info "JSON being rendered: #{items_json.inspect}" | ||
|
|
||
| render json: items_json |
There was a problem hiding this comment.
Do not expose quiz correct_answer in the index response.
This endpoint currently serializes items directly, which can reveal answer keys for quiz items and undermine quiz gating.
Suggested fix
- items_json = questionnaire.items.as_json
+ items_json = questionnaire.items.map do |item|
+ item.as_json.except('correct_answer')
+ 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.
| items_json = questionnaire.items.as_json | |
| Rails.logger.info "JSON being rendered: #{items_json.inspect}" | |
| render json: items_json | |
| items_json = questionnaire.items.map do |item| | |
| item.as_json.except('correct_answer') | |
| end | |
| Rails.logger.info "JSON being rendered: #{items_json.inspect}" | |
| render json: items_json |
🤖 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 11 - 14, The index action
is serializing questionnaire.items (items_json) and exposing sensitive quiz
fields like correct_answer; update the serialization to exclude or redact that
attribute before logging and rendering by either mapping questionnaire.items to
a safe hash (e.g., items = questionnaire.items.map { |i|
i.as_json.except('correct_answer') } or using a serializer that omits
:correct_answer) and change the Rails.logger.info to log the redacted items
variable, then render json: items instead of items_json; ensure the attribute
name correct_answer is referenced so the filter covers it consistently.
| if reviewee_team_id.present? && !reviewee_team_id.zero? | ||
| reviewee_team = Team.find_by(id: reviewee_team_id) | ||
| else | ||
| # Fall back: find the review map for this reviewer on this assignment. find_by returns | ||
| # only one record — if a student reviews multiple teams this is ambiguous, which is why | ||
| # callers should always supply reviewee_team_id. | ||
| reviewer_participant_lookup = AssignmentParticipant.find_by(user_id: reviewer_user_id, parent_id: assignment_id) | ||
| review_map = ReviewResponseMap.find_by(reviewer_id: reviewer_user_id, reviewed_object_id: assignment_id) | ||
| unless review_map | ||
| review_map = ReviewResponseMap.find_by(reviewer_id: reviewer_participant_lookup&.id, reviewed_object_id: assignment_id) | ||
| end | ||
| reviewee_team = review_map ? Team.find_by(id: review_map.reviewee_id) : nil | ||
| end | ||
| quiz_questionnaire = reviewee_team&.quiz_questionnaire |
There was a problem hiding this comment.
Scope reviewee_team_id to the assignment before using its quiz.
Team.find_by(id: reviewee_team_id) accepts any team in the system. A caller can pair an assignment_id from one assignment with a reviewee_team_id from another and get a quiz map for the wrong team’s questionnaire. Resolve the team through the assignment before reading quiz_questionnaire.
🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 56-58: Use the double pipe equals operator ||= instead.
(Style/OrAssignment)
[convention] 57-57: Line is too long. [127/120]
(Layout/LineLength)
🤖 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 - 61, When
resolving reviewee_team (used later to read quiz_questionnaire), scope the Team
lookup to the assignment by including the assignment identifier: replace
Team.find_by(id: reviewee_team_id) with a lookup that also matches the
assignment (e.g., Team.find_by(id: reviewee_team_id, parent_id: assignment_id))
and likewise when deriving reviewee_team from review_map use Team.find_by(id:
review_map.reviewee_id, parent_id: assignment_id); update the branch that falls
back to review_map so both code paths only return a team that belongs to the
given assignment before accessing quiz_questionnaire.
| case action_name | ||
| when 'create', 'show', 'index' | ||
| true |
There was a problem hiding this comment.
create, show, and index are currently readable by any caller who can hit the endpoint.
These branches return true without checking response/map ownership or teaching-staff privileges. That means a user who can guess a map_id or response id can open someone else’s draft or read their scores/comments through show and index.
🧰 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 8 - 10, The
create/show/index branches on action_name currently return true and bypass
ownership/privilege checks; update ResponsesController so these actions enforce
authorization by verifying that the current_user either owns the target
Map/Response or has teaching-staff privileges before allowing access. Locate the
logic that switches on action_name (the when 'create', 'show', 'index' branch)
and replace the unconditional true with calls to your existing authorization
helpers (or a new before_action) that: for create check current_user owns the
Map with the provided map_id, and for show/index check current_user owns the
Response(s) or is teaching staff; ensure you reference the same identifiers
(action_name, map_id, Response model, and any teaching_staff role/method) so the
permission checks are applied consistently.
| def response_params | ||
| 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 let update mutate workflow-owned fields.
Permitting map_id, is_submitted, and submitted_at lets a reviewer who owns any draft rebind it to a different map or mark it submitted via PATCH /responses/:id, bypassing the deadline and scoring logic in submit. This payload should be limited to answer content only.
🧰 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 150 - 156, The strong
params in response_params currently permit workflow-owned fields (map_id,
is_submitted, submitted_at) which allows PATCH /responses/:id to rebind or
submit a response; remove those keys so response_params only permits answer
content (e.g. scores_attributes: [:id, :item_id, :answer, :comments]) and ensure
any map_id or submission state changes are handled only in the create/submit
flow or dedicated controller methods (not in update) so that update (and any use
of response_params in the update action) cannot mutate workflow-owned fields.
| @to_user = User.find(@invitation.to_id) | ||
| @from_user = User.find(@invitation.from_id) |
There was a problem hiding this comment.
Resolve participant/user ID mismatch in recipient lookup.
Lines 7-8 treat Invitation.to_id/from_id as User IDs, but they are AssignmentParticipant IDs (app/models/invitation.rb, Lines 6-7). This can resolve the wrong user or fail.
Proposed fix
- `@to_user` = User.find(`@invitation.to_id`)
- `@from_user` = User.find(`@invitation.from_id`)
+ `@to_user` = `@invitation.to_participant.user`
+ `@from_user` = `@invitation.from_participant.user`📝 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.
| @to_user = User.find(@invitation.to_id) | |
| @from_user = User.find(@invitation.from_id) | |
| `@to_user` = `@invitation.to_participant.user` | |
| `@from_user` = `@invitation.from_participant.user` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/mailers/invitation_sent_mailer.rb` around lines 7 - 8, The mailer is
looking up Users using Invitation.to_id/from_id but those IDs are
AssignmentParticipant IDs; change the lookup in invitation_sent_mailer (where
`@to_user` and `@from_user` are set) to first fetch the AssignmentParticipant
records via AssignmentParticipant.find(`@invitation.to_id`) and
AssignmentParticipant.find(`@invitation.from_id`) and then get the associated User
(e.g., via .user or .user_id + User.find) to assign `@to_user` and `@from_user` so
the mailer sends to the correct users.
| # 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) | ||
| correct = s.item.correct_answer.to_s.strip.downcase |
There was a problem hiding this comment.
Don't infer quiz mode from reviewer_id == reviewee_id.
On normal review maps those ids come from different tables (AssignmentParticipant vs Team), so equal numeric values are possible. When that happens, an ordinary peer review will be scored as a quiz. While iterating scores, the reliable signal is the item/questionnaire itself (s.item.is_quiz_item? or s.item.questionnaire.questionnaire_type), not cross-table id equality.
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 - 123, Don't infer quiz mode from
map.reviewer_id == map.reviewee_id; instead detect quiz items from the
item/questionnaire itself. Remove or stop using the is_quiz boolean derived from
map.reviewer_id/map.reviewee_id and change the scoring condition to use
s.item.is_quiz_item? (or check s.item.questionnaire.questionnaire_type == 'Quiz'
if is_quiz_item? is unavailable). Update the check that currently reads
comment_scored_types.include?(s.item.question_type) && is_quiz to use
s.item.is_quiz_item? && comment_scored_types.include?(s.item.question_type) (or
the questionnaire_type-based equivalent), and apply the same replacement to the
other similar block mentioned (the second occurrence around the later scoring
logic).
| </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 interpolation and attribute name in the email body.
Line 7 uses @to_user.fullname; User exposes full_name, and this sentence should show the inviter (@from_user). Current code can fail at render time and shows the wrong person.
Proposed 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.full_name` %></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.full_name` %></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 template uses the wrong variable and attribute — change the interpolation
from `@to_user.fullname` to use the inviter `@from_user` and the correct attribute
name full_name (i.e. replace `@to_user.fullname` with `@from_user.full_name`) in
send_invitation_email.html.erb so the email shows the inviter and doesn't error
at render time.
| get 'show_all/questionnaire/:id', to:'questions#show_all#questionnaire', as: 'show_all' | ||
| delete 'delete_all/questionnaire/:id', to:'questions#delete_all#questionnaire', as: 'delete_all' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Ruby on Rails routing, is to: 'questions#show_all#questionnaire'valid, or must the route target be exactlycontroller#actionwith a single#?
💡 Result:
In Rails routing, to: 'questions#show_all#questionnaire' is not valid for mapping to a controller action. The to: string form expects exactly controller#action (a single # separating controller and action).
Citations:
- 1: https://guides.rubyonrails.org/routing.html
- 2: https://edgeguides.rubyonrails.org/routing.html
- 3: https://guides.rubyonrails.org/v8.0.0/routing.html
- 4: https://github.com/rails/rails/blob/main/actionpack/lib/action_dispatch/routing/mapper.rb
These route targets use invalid Rails action paths.
Rails routes expect controller#action with a single #. The routes on lines 84-85 include a second #:
to:'questions#show_all#questionnaire'should likely beto:'questions#show_all'orto:'questions#questionnaire'to:'questions#delete_all#questionnaire'should likely beto:'questions#delete_all'orto:'questions#questionnaire'
These will fail to dispatch to any controller action.
🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 84-84: Space missing after colon.
(Layout/SpaceAfterColon)
[convention] 85-85: Space missing after colon.
(Layout/SpaceAfterColon)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/routes.rb` around lines 84 - 85, The two routes use invalid action
paths with two '#' symbols ('questions#show_all#questionnaire' and
'questions#delete_all#questionnaire'); update the to: values to a single
controller#action string (e.g., use 'questions#show_all' or
'questions#questionnaire' for the GET route and 'questions#delete_all' or
'questions#questionnaire' for the DELETE route) so they dispatch to a valid
action (adjust the mapped action names to match the actual controller methods
show_all, delete_all, or questionnaire).
| if column_exists?(:teams, :assignment_id) | ||
| if foreign_key_exists?(:teams, :assignments) | ||
| remove_reference :teams, :assignment, foreign_key: true | ||
| else | ||
| remove_reference :teams, :assignment, foreign_key: false | ||
| end | ||
| end | ||
|
|
||
| # Add polymorphic association fields (type column already exists) | ||
| add_column :teams, :parent_id, :integer, null: false | ||
| add_column :teams, :parent_id, :integer, null: false unless column_exists?(:teams, :parent_id) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether a separate migration already backfills teams.assignment_id -> teams.parent_id
rg -n -C3 "assignment_id.*parent_id|parent_id.*assignment_id|UPDATE teams|change_column_null\\s*:teams,\\s*:parent_id" db/migrateRepository: expertiza/reimplementation-back-end
Length of output: 61
🏁 Script executed:
# Locate and read the actual migration file
find db/migrate -name "*polymorphic*association*teams*" -type fRepository: expertiza/reimplementation-back-end
Length of output: 151
🏁 Script executed:
# Also check for any data seeding or backfill in db/seeds or lib tasks
find db -name "*.rb" -type f | head -20Repository: expertiza/reimplementation-back-end
Length of output: 1056
🏁 Script executed:
# Search for any backfill-related code outside migrations (seeds, lib, etc.)
rg -l "assignment_id|parent_id" db/ --type rb | grep -v migrateRepository: expertiza/reimplementation-back-end
Length of output: 106
🏁 Script executed:
cat -n db/migrate/20250418004442_change_to_polymorphic_association_in_teams.rbRepository: expertiza/reimplementation-back-end
Length of output: 809
🏁 Script executed:
# Check db/seeds.rb for backfill logic
if [ -f db/seeds.rb ]; then
rg -n "assignment_id|parent_id" db/seeds.rb -A3 -B3
else
echo "db/seeds.rb not found"
fiRepository: expertiza/reimplementation-back-end
Length of output: 1018
🏁 Script executed:
# Search for any Rake tasks related to data migration
find . -name "Rakefile" -o -name "*.rake" | head -10Repository: expertiza/reimplementation-back-end
Length of output: 90
Unsafe data migration: removes assignment_id without backfilling parent_id first.
This migration removes the assignment_id column and adds parent_id with null: false, but contains no logic to copy existing relationship data. On production databases with data, existing assignment relationships are lost and the migration fails due to the null constraint.
The seeds confirm parent_id should replace assignment_id relationships, but there is no separate backfill migration and no external mechanism to preserve existing data.
Suggested migration pattern (backfill before drop)
-class ChangeToPolymorphicAssociationInTeams < ActiveRecord::Migration[8.0]
- def change
- # Remove old assignment reference if it still exists
- if column_exists?(:teams, :assignment_id)
- if foreign_key_exists?(:teams, :assignments)
- remove_reference :teams, :assignment, foreign_key: true
- else
- remove_reference :teams, :assignment, foreign_key: false
- end
- end
-
- # Add polymorphic association fields (type column already exists)
- add_column :teams, :parent_id, :integer, null: false unless column_exists?(:teams, :parent_id)
- end
-end
+class ChangeToPolymorphicAssociationInTeams < ActiveRecord::Migration[8.0]
+ def up
+ add_column :teams, :parent_id, :integer unless column_exists?(:teams, :parent_id)
+
+ if column_exists?(:teams, :assignment_id)
+ execute <<~SQL
+ UPDATE teams
+ SET parent_id = assignment_id
+ WHERE parent_id IS NULL
+ SQL
+
+ change_column_null :teams, :parent_id, false
+ remove_reference :teams, :assignment, foreign_key: foreign_key_exists?(:teams, :assignments)
+ end
+ end
+
+ def down
+ add_reference :teams, :assignment, foreign_key: true unless column_exists?(:teams, :assignment_id)
+ execute <<~SQL
+ UPDATE teams
+ SET assignment_id = parent_id
+ WHERE assignment_id IS NULL
+ SQL
+ remove_column :teams, :parent_id if column_exists?(:teams, :parent_id)
+ end
+end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrate/20250418004442_change_to_polymorphic_association_in_teams.rb`
around lines 6 - 15, The migration currently removes assignment_id then adds
parent_id with null: false without preserving data; update the migration to (1)
add parent_id as nullable (add_column :teams, :parent_id, :integer, null: true
unless column_exists?(:teams, :parent_id)), (2) backfill parent_id from
assignment_id for existing rows (copy assignment_id -> parent_id), (3) run a
safe verification step if desired, (4) change parent_id to null: false or add a
NOT NULL constraint, and only then remove the assignment_id reference (use
foreign_key_exists? and remove_reference :teams, :assignment as currently used);
ensure you reference the existing helpers column_exists?, foreign_key_exists?,
remove_reference, assignment_id and parent_id in the same migration or split
into separate backfill and cleanup migrations to avoid losing data.
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
New Features
Database Updates