Skip to content

E2608. Hierarchical list display#327

Open
veera2508 wants to merge 12 commits into
expertiza:mainfrom
veera2508:main
Open

E2608. Hierarchical list display#327
veera2508 wants to merge 12 commits into
expertiza:mainfrom
veera2508:main

Conversation

@veera2508
Copy link
Copy Markdown

@veera2508 veera2508 commented Mar 31, 2026

All details can be found here: https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2026_-_E2608._Hierarchical_list_display

Hosted at: http://152.7.177.90:3000/

Summary by CodeRabbit

Release Notes

  • New Features

    • Questionnaires now support hierarchical organization by type and dedicated item retrieval endpoints
    • Teams endpoint adds filtering by type and parent context; includes parent identifier in responses
    • Courses endpoint now includes nested assignment data
  • Improvements

    • Questionnaire deletion workflow simplified

@veera2508
Copy link
Copy Markdown
Author

veera2508 commented Mar 31, 2026

There are some files (Gemfile) which should not be modified in the final PR. Once a review is done, we will revert those back. This is because those changes are required to run it on docker on the VCL.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 6, 2026

2 Warnings
⚠️ Pull request is too big (more than 500 LoC).
⚠️ Schema changes detected without a corresponding DB migration.

Generated by 🚫 Danger

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 28, 2026

📝 Walkthrough

Walkthrough

This PR implements a hierarchical questionnaire and team management system, adds API endpoints for questionnaire organization and items retrieval, updates course serialization to include nested assignments, refactors question type sourcing to use a dedicated model, enables team hierarchy filtering with parent_id support, and includes comprehensive request specifications alongside significant database schema updates.

Changes

Cohort / File(s) Summary
Configuration & Environment
.gitignore, .ruby-version, Dockerfile, Gemfile
Minor formatting updates: extend gitignore for Ruby versioning/dependency files, update Ruby version format by removing prefix, add trailing newline, and remove extra blank line.
Questionnaires Controller (Major Refactor)
app/controllers/questionnaires_controller.rb
Adds hierarchical and items endpoints; supports nested items_attributes in create/update with database transactions; implements sync_items! for item-level synchronization with field normalization (canonical question_type, seq, alternatives, break_before casting, size computation); updates error handling to return record.errors.full_messages; uses destroy! with explicit success/error JSON responses.
Other Controllers
app/controllers/courses_controller.rb, app/controllers/questions_controller.rb, app/controllers/teams_controller.rb
Courses: eager-load and serialize nested assignments. Questions: source types from QuestionType model with fallback to default list. Teams: filter by parent_id and type array/comma-separated values, sort by name/id, map legacy assignment_id to parent_id.
Models & Serializers
app/models/Item.rb, app/models/question_type.rb, app/models/questionnaire.rb, app/serializers/team_serializer.rb
Add id to Item JSON serialization; create new QuestionType model; remove before_destroy callback and check_for_question_associations from Questionnaire; include parent_id in TeamSerializer output.
Routing & Database
config/routes.rb, db/schema.rb
Add questionnaires hierarchical collection and items member routes. Remove cakes table; rename reply_status to status in invitations and join_team_requests; remove type column from response_maps; add assignment_id index on project_topics and unique composite index on teams_users (team_id, user_id); drop foreign keys from invitations.from_id and invitations.to_id.
Request Specifications
spec/requests/courses_spec.rb, spec/requests/questionnaires_spec.rb, spec/requests/teams_spec.rb
Add comprehensive request specs with JWT authentication: Courses API validates nested assignment serialization; Questionnaires API exercises listing, hierarchical grouping by display_type, and item retrieval with sequence ordering; Teams API validates parent_id and type filtering.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant QuestionnairesController
    participant Questionnaire Model
    participant Item Model
    participant Database

    Client->>QuestionnairesController: POST/PATCH with items_attributes
    activate QuestionnairesController
    QuestionnairesController->>Database: BEGIN TRANSACTION
    activate Database
    QuestionnairesController->>Questionnaire Model: Update questionnaire fields
    Questionnaire Model->>Database: Save questionnaire
    QuestionnairesController->>Questionnaire Model: sync_items!(items_attributes)
    activate Questionnaire Model
    Questionnaire Model->>Item Model: Create/update/destroy items
    Item Model->>Item Model: Normalize fields<br/>(question_type, seq, size)
    Item Model->>Database: Persist changes
    Questionnaire Model->>Questionnaire Model: Handle _destroy flags
    deactivate Questionnaire Model
    alt Validation Success
        Database->>Database: COMMIT
        QuestionnairesController->>Client: 200 OK + JSON response
    else Validation Error
        Database->>Database: ROLLBACK
        QuestionnairesController->>Client: 422 + errors.full_messages
    end
    deactivate Database
    deactivate QuestionnairesController
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Poem

🐰 With questionnaires now arranged in trees,
And items synced with graceful ease,
Teams find their parents, types aligned,
API endpoints perfectly designed!
The schema dances, tests now pass—
A refactor done with rabbit class! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'E2608. Hierarchical list display' is directly related to the main changes in the PR, which implement hierarchical list display capabilities for questionnaires and teams.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
db/schema.rb (1)

323-330: ⚠️ Potential issue | 🔴 Critical

response_maps STI/type-dependent logic is now broken.

response_maps no longer has a type column, but app/models/response_map.rb (Line 34 in the provided snippet) still branches on map.type == 'ReviewResponseMap'. This can break response classification at runtime.

🐛 Suggested fix path
# migration approach (preferred if STI/type checks are still required)
add_column :response_maps, :type, :string
add_index :response_maps, :type

If removing STI was intentional, update all map.type usages to a non-STI discriminator before shipping.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@db/schema.rb` around lines 323 - 330, The model indicates STI is broken
because the response_maps table no longer has a type column yet
app/models/response_map.rb still branches on map.type == 'ReviewResponseMap';
fix by either restoring the STI discriminator (add a string column named type to
the response_maps table and add an index on :type) or, if STI was intentionally
removed, update all usages of map.type (including the conditional in
app/models/response_map.rb) to use the new discriminator field or polymorphic
logic (e.g. a role/status column or class_name attribute) and remove any
STI-specific behavior.
🧹 Nitpick comments (3)
app/models/Item.rb (1)

26-33: Wrap the only: list to satisfy RuboCop.

Layout/LineLength already flags this line; reflowing the attribute list keeps the serialization change intact and avoids lint noise.

♻️ Suggested reflow
-      super(options.merge({
-                            only: %i[id 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|
+      super(options.merge(
+        only: %i[
+          id 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|
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/models/Item.rb` around lines 26 - 33, The as_json method's only:
attribute list is triggering RuboCop Layout/LineLength; reflow the array so each
symbol is on its own (or grouped) line to reduce line length while preserving
the same symbols and order. Edit the as_json method (the only: key within
super(options.merge({...})) and its include: questionnaire block) to break the
%i[...] list across multiple lines (and align closing brackets) so the
serialization output stays identical but lines are shorter to satisfy RuboCop.
spec/requests/courses_spec.rb (1)

9-24: Extract duplicated request auth header helper into shared spec support.

auth_headers_for is repeated across multiple request specs (courses/teams/questionnaires). Centralizing it in spec/support will reduce drift and simplify future token payload changes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@spec/requests/courses_spec.rb` around lines 9 - 24, Extract the duplicated
auth_headers_for method into a single shared spec helper (e.g., a new helper
module that defines auth_headers_for and uses JsonWebToken.encode with the same
payload), place it under spec/support, and include that module into RSpec (via
RSpec.configure.include) so request specs can call auth_headers_for directly;
then remove the duplicated method definitions from the courses, teams, and
questionnaires request specs to centralize token payload changes.
db/schema.rb (1)

277-278: Redundant duplicate index on project_topics.assignment_id.

Line 277 and Line 278 define two non-unique indexes on the same column. Keeping both adds write/storage overhead without improving read plans in normal cases.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@db/schema.rb` around lines 277 - 278, There are two non-unique indexes on
project_topics.assignment_id ("fk_sign_up_categories_sign_up_topics" and
"index_project_topics_on_assignment_id"); remove the redundant one to avoid
extra write/storage overhead—delete the t.index line for
"fk_sign_up_categories_sign_up_topics" from schema.rb (keep
"index_project_topics_on_assignment_id"), and add a Rails migration that drops
the database index named "fk_sign_up_categories_sign_up_topics" (or the
redundant index you choose to remove) so the schema and DB stay in sync, then
run the migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@app/controllers/questionnaires_controller.rb`:
- Around line 163-170: Remove :instructor_id from the strong params list (the
params.require(:questionnaire).permit(...) call) so clients cannot set
ownership, and in the controller actions that build/update questionnaires
(create and update) explicitly set the instructor on the attributes from the
authenticated user (e.g., assign questionnaire_attributes[:instructor_id] =
current_user.id before creating/updating the Questionnaire). Ensure this is done
in the same controller methods that call the permitted params so instructor_id
is derived server-side rather than accepted from the request.
- Around line 233-241: The update currently overwrites existing item fields when
keys are omitted; change the assignment logic so seq and break_before only
change when the incoming payload includes them. For seq, set it to
item_data[:seq] (or its presence-casted value) if item_data.key?(:seq),
otherwise fall back to existing_item&.seq and only default to index + 1 when
there is no existing_item; for break_before, only cast and use
ActiveModel::Type::Boolean.new.cast(item_data[:break_before]) when
item_data.key?(:break_before), otherwise use existing_item&.break_before (do not
default to true). Keep other fields (normalize_alternatives, build_item_size,
etc.) as-is.
- Around line 75-79: The items action currently uses
Questionnaire.find(params[:id]) which exposes private questionnaires to any
authenticated user; change it to enforce access control by locating the record
through an ownership or visibility check (e.g. use
current_user.questionnaires.find(params[:id]) or Questionnaire.find_by(id:
params[:id]) and then require questionnaire.public? ||
questionnaire.instructor_id == current_user.id || current_user.admin?); if the
check fails, render a 403/404 (e.g. render json: { error: "Not authorized" },
status: :forbidden) and only then render questionnaire.items.order(:seq); keep
the existing rescue for RecordNotFound or switch to a nil-check and return
early.

In `@app/models/question_type.rb`:
- Around line 1-2: Add presence and uniqueness validation to the QuestionType
model and enforce NOT NULL + uniqueness at the DB level: update class
QuestionType to include validates :name, presence: true, uniqueness: true; then
create a new migration (or update the CreateQuestionTables migration) to (1)
clean existing bad data (remove or deduplicate blank/duplicate names), (2)
change the question_types.name column to NOT NULL, and (3) add a unique index on
:name (unique: true) so the database enforces uniqueness; reference the
QuestionType model and the CreateQuestionTables migration when locating the code
to modify.

In `@db/schema.rb`:
- Line 227: The schema still shows t.string "status" for the join_team_requests
table while the codebase expects reply_status; run the pending migration
RenameStatusToReplyStatusInInJoinTeamRequests (e.g. rails db:migrate) to rename
the column from status to reply_status, then re-dump the schema (rails
db:schema:dump or ensure migrations ran in the environment) and verify the
join_team_requests table now lists t.string "reply_status"; if migrations fail,
inspect the migration file RenameStatusToReplyStatusInInJoinTeamRequests and fix
any issues preventing it from running before re-running migrations.

---

Outside diff comments:
In `@db/schema.rb`:
- Around line 323-330: The model indicates STI is broken because the
response_maps table no longer has a type column yet app/models/response_map.rb
still branches on map.type == 'ReviewResponseMap'; fix by either restoring the
STI discriminator (add a string column named type to the response_maps table and
add an index on :type) or, if STI was intentionally removed, update all usages
of map.type (including the conditional in app/models/response_map.rb) to use the
new discriminator field or polymorphic logic (e.g. a role/status column or
class_name attribute) and remove any STI-specific behavior.

---

Nitpick comments:
In `@app/models/Item.rb`:
- Around line 26-33: The as_json method's only: attribute list is triggering
RuboCop Layout/LineLength; reflow the array so each symbol is on its own (or
grouped) line to reduce line length while preserving the same symbols and order.
Edit the as_json method (the only: key within super(options.merge({...})) and
its include: questionnaire block) to break the %i[...] list across multiple
lines (and align closing brackets) so the serialization output stays identical
but lines are shorter to satisfy RuboCop.

In `@db/schema.rb`:
- Around line 277-278: There are two non-unique indexes on
project_topics.assignment_id ("fk_sign_up_categories_sign_up_topics" and
"index_project_topics_on_assignment_id"); remove the redundant one to avoid
extra write/storage overhead—delete the t.index line for
"fk_sign_up_categories_sign_up_topics" from schema.rb (keep
"index_project_topics_on_assignment_id"), and add a Rails migration that drops
the database index named "fk_sign_up_categories_sign_up_topics" (or the
redundant index you choose to remove) so the schema and DB stay in sync, then
run the migration.

In `@spec/requests/courses_spec.rb`:
- Around line 9-24: Extract the duplicated auth_headers_for method into a single
shared spec helper (e.g., a new helper module that defines auth_headers_for and
uses JsonWebToken.encode with the same payload), place it under spec/support,
and include that module into RSpec (via RSpec.configure.include) so request
specs can call auth_headers_for directly; then remove the duplicated method
definitions from the courses, teams, and questionnaires request specs to
centralize token payload changes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 51c7a483-8fbe-41a9-91e8-1cf32023f00c

📥 Commits

Reviewing files that changed from the base of the PR and between cc03ecd and fd90cff.

📒 Files selected for processing (17)
  • .gitignore
  • .ruby-version
  • Dockerfile
  • Gemfile
  • app/controllers/courses_controller.rb
  • app/controllers/questionnaires_controller.rb
  • app/controllers/questions_controller.rb
  • app/controllers/teams_controller.rb
  • app/models/Item.rb
  • app/models/question_type.rb
  • app/models/questionnaire.rb
  • app/serializers/team_serializer.rb
  • config/routes.rb
  • db/schema.rb
  • spec/requests/courses_spec.rb
  • spec/requests/questionnaires_spec.rb
  • spec/requests/teams_spec.rb
💤 Files with no reviewable changes (2)
  • Gemfile
  • app/models/questionnaire.rb

Comment on lines +75 to +79
def items
questionnaire = Questionnaire.find(params[:id])
render json: questionnaire.items.order(:seq), status: :ok and return
rescue ActiveRecord::RecordNotFound
render json: { error: "Questionnaire not found" }, status: :not_found and return
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add access control in items to prevent private questionnaire data exposure.

Line 76 fetches by ID only; any authenticated user can request items for another instructor’s private questionnaire if they know the ID.

🔒 Proposed fix
 def items
-  questionnaire = Questionnaire.find(params[:id])
+  questionnaire = Questionnaire
+                  .where(private: false)
+                  .or(Questionnaire.where(instructor_id: current_user.id))
+                  .find(params[:id])
   render json: questionnaire.items.order(:seq), status: :ok and return
 rescue ActiveRecord::RecordNotFound
   render json: { error: "Questionnaire not found" }, status: :not_found and return
 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 75 - 79, The items
action currently uses Questionnaire.find(params[:id]) which exposes private
questionnaires to any authenticated user; change it to enforce access control by
locating the record through an ownership or visibility check (e.g. use
current_user.questionnaires.find(params[:id]) or Questionnaire.find_by(id:
params[:id]) and then require questionnaire.public? ||
questionnaire.instructor_id == current_user.id || current_user.admin?); if the
check fails, render a 403/404 (e.g. render json: { error: "Not authorized" },
status: :forbidden) and only then render questionnaire.items.order(:seq); keep
the existing rescue for RecordNotFound or switch to a nil-check and return
early.

Comment on lines +163 to +170
params.require(:questionnaire).permit(
:name,
:questionnaire_type,
:private,
:min_question_score,
:max_question_score,
:instructor_id,
items_attributes: [
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Do not permit client-controlled instructor_id in questionnaire params.

Permitting :instructor_id allows ownership spoofing unless tightly gated elsewhere. Ownership should be derived from current_user (or explicitly admin-only).

🛡️ Proposed fix
 def questionnaire_params
   params.require(:questionnaire).permit(
     :name,
     :questionnaire_type,
     :private,
     :min_question_score,
     :max_question_score,
-    :instructor_id,
     items_attributes: [
       :id,
       :txt,
       :question_type,
@@
# additionally in create/update assignment path
questionnaire_attributes[:instructor_id] = current_user.id
🤖 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 163 - 170, Remove
:instructor_id from the strong params list (the
params.require(:questionnaire).permit(...) call) so clients cannot set
ownership, and in the controller actions that build/update questionnaires
(create and update) explicitly set the instructor on the attributes from the
authenticated user (e.g., assign questionnaire_attributes[:instructor_id] =
current_user.id before creating/updating the Questionnaire). Ensure this is done
in the same controller methods that call the permitted params so instructor_id
is derived server-side rather than accepted from the request.

Comment on lines +233 to +241
txt: item_data[:txt].presence || existing_item&.txt,
question_type: question_type,
weight: item_data[:weight].presence || existing_item&.weight,
seq: item_data[:seq].presence || index + 1,
alternatives: normalize_alternatives(item_data[:alternatives]) || existing_item&.alternatives,
min_label: item_data[:min_label].presence || existing_item&.min_label,
max_label: item_data[:max_label].presence || existing_item&.max_label,
break_before: item_data.key?(:break_before) ? ActiveModel::Type::Boolean.new.cast(item_data[:break_before]) : true,
size: build_item_size(question_type, item_data, existing_item)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Update path currently overwrites existing item values when fields are omitted.

Line 236 defaults missing seq to index + 1, and Line 240 defaults missing break_before to true. On partial updates, this can silently mutate existing item state.

🐛 Proposed fix
 def build_item_attributes(item_data, index, existing_item = nil)
   question_type = canonical_question_type(item_data[:question_type])
   {
     txt: item_data[:txt].presence || existing_item&.txt,
     question_type: question_type,
     weight: item_data[:weight].presence || existing_item&.weight,
-    seq: item_data[:seq].presence || index + 1,
+    seq: item_data[:seq].presence || existing_item&.seq || index + 1,
     alternatives: normalize_alternatives(item_data[:alternatives]) || existing_item&.alternatives,
     min_label: item_data[:min_label].presence || existing_item&.min_label,
     max_label: item_data[:max_label].presence || existing_item&.max_label,
-    break_before: item_data.key?(:break_before) ? ActiveModel::Type::Boolean.new.cast(item_data[:break_before]) : true,
+    break_before: if item_data.key?(:break_before)
+                    ActiveModel::Type::Boolean.new.cast(item_data[:break_before])
+                  elsif existing_item
+                    existing_item.break_before
+                  else
+                    true
+                  end,
     size: build_item_size(question_type, item_data, existing_item)
   }.compact
 end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
txt: item_data[:txt].presence || existing_item&.txt,
question_type: question_type,
weight: item_data[:weight].presence || existing_item&.weight,
seq: item_data[:seq].presence || index + 1,
alternatives: normalize_alternatives(item_data[:alternatives]) || existing_item&.alternatives,
min_label: item_data[:min_label].presence || existing_item&.min_label,
max_label: item_data[:max_label].presence || existing_item&.max_label,
break_before: item_data.key?(:break_before) ? ActiveModel::Type::Boolean.new.cast(item_data[:break_before]) : true,
size: build_item_size(question_type, item_data, existing_item)
txt: item_data[:txt].presence || existing_item&.txt,
question_type: question_type,
weight: item_data[:weight].presence || existing_item&.weight,
seq: item_data[:seq].presence || existing_item&.seq || index + 1,
alternatives: normalize_alternatives(item_data[:alternatives]) || existing_item&.alternatives,
min_label: item_data[:min_label].presence || existing_item&.min_label,
max_label: item_data[:max_label].presence || existing_item&.max_label,
break_before: if item_data.key?(:break_before)
ActiveModel::Type::Boolean.new.cast(item_data[:break_before])
elsif existing_item
existing_item.break_before
else
true
end,
size: build_item_size(question_type, item_data, existing_item)
🧰 Tools
🪛 RuboCop (1.86.1)

[convention] 240-240: Line is too long. [121/120]

(Layout/LineLength)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/controllers/questionnaires_controller.rb` around lines 233 - 241, The
update currently overwrites existing item fields when keys are omitted; change
the assignment logic so seq and break_before only change when the incoming
payload includes them. For seq, set it to item_data[:seq] (or its
presence-casted value) if item_data.key?(:seq), otherwise fall back to
existing_item&.seq and only default to index + 1 when there is no existing_item;
for break_before, only cast and use
ActiveModel::Type::Boolean.new.cast(item_data[:break_before]) when
item_data.key?(:break_before), otherwise use existing_item&.break_before (do not
default to true). Keep other fields (normalize_alternatives, build_item_size,
etc.) as-is.

Comment on lines +1 to +2
class QuestionType < ApplicationRecord
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check the current state of the QuestionType model
cat -n app/models/question_type.rb

Repository: expertiza/reimplementation-back-end

Length of output: 136


🏁 Script executed:

# Find the migration file for question_types table
fd -e rb -path "*db/migrate*" | xargs grep -l "QuestionType\|question_type" | head -5

Repository: expertiza/reimplementation-back-end

Length of output: 248


🏁 Script executed:

# Search for QuestionsController and the types method
fd -e rb -path "*controllers*" | xargs grep -l "QuestionsController" | head -3

Repository: expertiza/reimplementation-back-end

Length of output: 248


🏁 Script executed:

# Search for usage of QuestionType in the codebase
rg "QuestionType" --type rb -A 3 -B 3 | head -100

Repository: expertiza/reimplementation-back-end

Length of output: 106


🏁 Script executed:

# Find migration files related to question_type
git ls-files | grep -i "migrate" | xargs grep -l "question_type" 2>/dev/null || echo "Checking db/migrate directly..."
ls -la db/migrate/ | grep -i question

Repository: expertiza/reimplementation-back-end

Length of output: 968


🏁 Script executed:

# Find QuestionsController
git ls-files | grep -i "controller" | xargs grep -l "QuestionsController\|questions_controller" 2>/dev/null | head -5

Repository: expertiza/reimplementation-back-end

Length of output: 119


🏁 Script executed:

# Search for all references to QuestionType and types method
rg "QuestionType" --multiline -A 5 -B 2

Repository: expertiza/reimplementation-back-end

Length of output: 800


🏁 Script executed:

# Look for any validates statements in question_type model
rg "validates" app/models/question_type.rb

Repository: expertiza/reimplementation-back-end

Length of output: 61


🏁 Script executed:

# Read the migration file that creates question_tables
cat -n db/migrate/20250214224716_create_question_tables.rb

Repository: expertiza/reimplementation-back-end

Length of output: 1267


🏁 Script executed:

# Also check the other migration file
cat -n db/migrate/20230401213404_create_questions.rb

Repository: expertiza/reimplementation-back-end

Length of output: 777


Add presence and uniqueness constraints to the question_types table.

QuestionType drives a public endpoint but lacks both model validations and database constraints. The migration creates the :name column without NOT NULL or uniqueness, allowing blank or duplicate entries to corrupt the endpoint response.

Add model validations and update the migration to enforce constraints at the database level:

🛠️ Suggested fixes

Model validation:

 class QuestionType < ApplicationRecord
+  validates :name, presence: true, uniqueness: true
 end

Migration constraint (add to db/migrate/20250214224716_create_question_tables.rb):

 create_table :question_types, options: "CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci" do |t|
-  t.string :name
+  t.string :name, null: false
   t.timestamps
 end
+add_index :question_types, :name, unique: true
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
class QuestionType < ApplicationRecord
end
class QuestionType < ApplicationRecord
validates :name, presence: true, uniqueness: true
end
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/models/question_type.rb` around lines 1 - 2, Add presence and uniqueness
validation to the QuestionType model and enforce NOT NULL + uniqueness at the DB
level: update class QuestionType to include validates :name, presence: true,
uniqueness: true; then create a new migration (or update the
CreateQuestionTables migration) to (1) clean existing bad data (remove or
deduplicate blank/duplicate names), (2) change the question_types.name column to
NOT NULL, and (3) add a unique index on :name (unique: true) so the database
enforces uniqueness; reference the QuestionType model and the
CreateQuestionTables migration when locating the code to modify.

Comment thread db/schema.rb
t.integer "team_id"
t.text "comments"
t.string "reply_status"
t.string "status"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Find JoinTeamRequest-related files =="
fd -i 'join*team*request*.rb' app spec db || true

echo "== Search for stale reply_status references =="
rg -n -C2 '\breply_status\b' app spec db || true

echo "== Search for JoinTeamRequest status field usage =="
rg -n -C2 'JoinTeamRequest|join_team_requests|\.status\b' app spec db || true

Repository: expertiza/reimplementation-back-end

Length of output: 50392


🏁 Script executed:

sed -n '215,235p' db/schema.rb

Repository: expertiza/reimplementation-back-end

Length of output: 875


🏁 Script executed:

rg -n '\.status\b' app/models/join_team_request.rb app/controllers/join_team_requests_controller.rb app/serializers/join_team_request_serializer.rb 2>/dev/null || true

Repository: expertiza/reimplementation-back-end

Length of output: 61


🏁 Script executed:

cat -n app/models/join_team_request.rb

Repository: expertiza/reimplementation-back-end

Length of output: 383


🏁 Script executed:

cat -n app/serializers/join_team_request_serializer.rb

Repository: expertiza/reimplementation-back-end

Length of output: 1025


🏁 Script executed:

cat -n db/migrate/20250805174104_rename_status_to_reply_status_in_in_join_team_requests.rb

Repository: expertiza/reimplementation-back-end

Length of output: 287


Schema migration is unapplied: join_team_requests table still has "status" column instead of "reply_status".

The migration RenameStatusToReplyStatusInInJoinTeamRequests exists to rename the column, and all code (model, serializer, controller, tests) has been updated to use reply_status. However, the schema.rb at line 227 still shows t.string "status". This schema-code mismatch will cause runtime failures when the application tries to access the reply_status attribute.

Ensure the migration has been run: rails db:migrate

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@db/schema.rb` at line 227, The schema still shows t.string "status" for the
join_team_requests table while the codebase expects reply_status; run the
pending migration RenameStatusToReplyStatusInInJoinTeamRequests (e.g. rails
db:migrate) to rename the column from status to reply_status, then re-dump the
schema (rails db:schema:dump or ensure migrations ran in the environment) and
verify the join_team_requests table now lists t.string "reply_status"; if
migrations fail, inspect the migration file
RenameStatusToReplyStatusInInJoinTeamRequests and fix any issues preventing it
from running before re-running migrations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants