E2614 Role Based Reviewing#347
Conversation
Instructor Api - Review strategy, assigning roles
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR adds role-based duty support: moves duty member limits from global duties to assignment-specific join records, adds duty associations and validations, new endpoints to update participant duties and assignment-duty limits, schema migrations, serializer updates, dependency tweaks, and new request specs. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as AssignmentsDutiesController
participant AuthZ as Authorization
participant Loader as BeforeAction Loader
participant Model as AssignmentsDuty Model
participant DB as Database
Client->>Controller: PATCH /assignments/:a_id/duties/:id/limit
Controller->>AuthZ: action_allowed!
AuthZ-->>Controller: allow / deny
Controller->>Loader: set_assignment
Loader->>DB: fetch Assignment
DB-->>Loader: Assignment or nil
Loader-->>Controller: `@assignment` or 404
Controller->>Loader: set_assignment_duty
Loader->>DB: fetch AssignmentsDuty by assignment_id & id
DB-->>Loader: AssignmentsDuty or nil
Loader-->>Controller: `@assignment_duty` or 404
Controller->>Model: assign max_members_for_duty, validate & save
Model->>DB: persist
alt saved
DB-->>Model: success
Controller-->>Client: 200 + serialized duty info
else validation fails
DB-->>Model: errors
Controller-->>Client: 422 + errors
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~28 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docker-compose.yml (1)
21-23:⚠️ Potential issue | 🟠 MajorMove hard-coded credentials to environment variables.
The
docker-compose.ymlfile hard-codesMYSQL_PASSWORDandMYSQL_ROOT_PASSWORDat lines 22-23, and theappservice includes the plaintext password inDATABASE_URL. While these are development credentials (documented in the README), this pattern should still be changed to use environment variables to prevent accidental exposure of real credentials if the setup is repurposed or forked.Suggested fix
db: image: mysql:8.0 restart: always environment: MYSQL_USER: ${MYSQL_USER:-dev} - MYSQL_PASSWORD: expertiza - MYSQL_ROOT_PASSWORD: expertiza + MYSQL_PASSWORD: ${MYSQL_PASSWORD} + MYSQL_ROOT_PASSWORD: ${MYSQL_ROOT_PASSWORD}Create a
.envfile at the repository root (add.envto.gitignore):MYSQL_USER=dev MYSQL_PASSWORD=expertiza MYSQL_ROOT_PASSWORD=expertizaAlso update the
appserviceDATABASE_URLto use environment variables.Note: The CI/CD workflows in
.github/workflows/use the same hard-coded credentials for test environments, which is acceptable for ephemeral test containers. However,.envshould still be excluded from version control for local development to follow security best practices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 21 - 23, Replace the hard-coded DB creds in docker-compose.yml by reading MYSQL_USER, MYSQL_PASSWORD and MYSQL_ROOT_PASSWORD from environment variables and update the app service's DATABASE_URL to interpolate those env vars (replace literal values for MYSQL_USER / MYSQL_PASSWORD / MYSQL_ROOT_PASSWORD and the plaintext password inside DATABASE_URL); add instructions to create a local .env (and ensure .env is listed in .gitignore) with keys MYSQL_USER, MYSQL_PASSWORD and MYSQL_ROOT_PASSWORD so developers can set dev credentials without committing them.spec/requests/api/v1/teams_participants_controller_spec.rb (1)
160-182:⚠️ Potential issue | 🟠 MajorCreate the duty mapping in the 200-path setup.
update_dutynow rejects duty IDs that are not present inassignments_duties, but this example still posts a hard-codedduty_id: 2without creating theDuty/AssignmentsDutyrow. That makes the spec fail with422, or worse, pass only if some unrelated test data happens to create ID2.Make the example self-contained
response(200, 'duty updated successfully') do + let!(:duty) do + Duty.create!(name: 'Leader', instructor: instructor) + end + let!(:assignment_duty) do + AssignmentsDuty.create!(assignment: assignment, duty: duty, max_members_for_duty: 2) + end let!(:participant_for_update) do AssignmentParticipant.create!( parent_id: assignment.id, user: student_user, handle: student_user.name ) end let(:teams_participant) do TeamsParticipant.create!( team_id: team_with_assignment.id, participant_id: participant_for_update.id, user_id: participant_for_update.user_id ) end let(:token) { JsonWebToken.encode({ id: student_user.id }) } let(:Authorization) { "Bearer #{token}" } - let(:payload) { { teams_participant_id: teams_participant.id, teams_participant: { duty_id: 2 } } } + let(:payload) { { teams_participant_id: teams_participant.id, teams_participant: { duty_id: duty.id } } } run_test! do |response| json = JSON.parse(response.body) expect(json['message']).to eq("Duty updated successfully") - expect(participant_for_update.reload.duty_id).to eq(2) + expect(participant_for_update.reload.duty_id).to eq(duty.id) end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/api/v1/teams_participants_controller_spec.rb` around lines 160 - 182, The spec posts a hard-coded duty_id (2) but doesn't create the corresponding Duty/AssignmentsDuty, causing a 422; in the 200-path setup create a Duty (e.g., via Duty.create!) and then an AssignmentsDuty (or AssignmentsDuties) linking that Duty to the existing assignment (use the existing assignment variable), and change the payload to use that duty's id (teams_participant: { duty_id: duty.id } or set let(:payload) to reference the created duty) so the update_duty validation passes; locate the setup around AssignmentParticipant/TeamsParticipant (participant_for_update, teams_participant, payload) and add the Duty and AssignmentsDuty creation there.app/models/participant.rb (1)
13-30:⚠️ Potential issue | 🟠 MajorScope this duty-limit validation to assignment participants.
For
CourseParticipant,parent_idis a course id, soAssignmentsDuty.find_by(assignment_id: parent_id, ...)misses and the|| 1fallback starts enforcing a one-member duty cap on course teams. This should only run when the participant actually belongs to an assignment.Minimal fix
- validate :duty_limit_within_team, if: -> { duty_id.present? && team.present? } + validate :duty_limit_within_team, if: -> { duty_id.present? && team.present? && assignment.present? }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/participant.rb` around lines 13 - 30, The duty_limit_within_team validation is running for CourseParticipant too and uses parent_id as an assignment id; restrict it so it only runs for assignment participants—either add the guard to the validator (e.g. change the validation predicate to include a check that this is an Assignment participant or that parent_type == 'Assignment' / instance_of? AssignmentParticipant) or add an early return in duty_limit_within_team (return unless self is an assignment participant) before calling AssignmentsDuty.find_by; reference duty_limit_within_team, CourseParticipant and AssignmentParticipant (or parent_type) when making the change.config/application.rb (1)
20-35:⚠️ Potential issue | 🟠 MajorSplit the Rails 8 defaults and Redis cache configuration out of this feature PR.
config.load_defaults 8.0changes framework-wide behavior and may introduce unintended breaking changes across the application. The Redis cache store configuration is similarly orthogonal to the duty/role feature—neither the Duty model, AssignmentsDuty, nor RolesController useRails.cache, and the configured cache store is not utilized anywhere in the codebase. Bundling these changes with a narrowly-scoped feature makes regressions much harder to isolate and complicates reviewing this PR's actual impact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/application.rb` around lines 20 - 35, Remove the unrelated framework/defaults and global cache changes from this feature PR: revert the addition of config.load_defaults 8.0 and the config.cache_store = :redis_cache_store, { url: ENV['CACHE_STORE'], expires_in: 3.days } entry in application.rb so the feature only contains duty/role-specific changes; leave any API-only setting (config.api_only) intact if it was preexisting, and move the Rails 8 defaults and Redis cache configuration into a separate follow-up PR or config change commit after the feature is merged.
🧹 Nitpick comments (3)
docker-compose.yml (2)
13-16: Consider DB/Redis readiness (healthchecks) to avoid startup races.
depends_onis present (Lines 13-16), but there are no healthchecks. Without readiness gating, the app can attempt connections before MySQL/Redis are ready, causing flaky startup in local/dev environments.Add healthchecks for
dbandredisand conditionally gateappstartup on healthy state where supported by your Compose version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 13 - 16, Add container healthchecks for the services named "db" and "redis" and update the "app" service's depends_on to wait for service health where supported; specifically, add a healthcheck block for db (use a mysql-ready check such as running "mysqladmin ping" or an equivalent probe that returns success when MySQL is accepting connections) and a healthcheck block for redis (use "redis-cli ping" or equivalent), and change the "app" service depends_on entry to use service_healthy/condition: service_healthy so Compose will wait for those healthchecks to pass before starting the app; adjust timeouts/retries to reasonable values to avoid false negatives.
31-31: Pinredisimage version instead ofredis:latest(repro/security).Using
redis:latest(Line 31) can introduce unexpected behavior changes and makes deployments less reproducible. Prefer a fixed tag (e.g.,redis:7.2.x) consistent with your target compatibility.Check what Redis version the app expects (if any) and whether other compose/services already rely on a specific Redis major/minor version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` at line 31, The docker-compose.yml currently uses an unpinned Redis image ("image: redis:latest"); change that to a specific, tested tag (e.g., "redis:7.2.x" or the exact patch like "redis:7.2.5") to ensure reproducible deployments and security; update the image line under the Redis service and commit. Before choosing the tag, search the codebase and configs (environment variables, connection settings, Dockerfiles, CI, and any redis client usage) for expected Redis features or version constraints and check other compose/services for already used Redis major/minor versions to pick a compatible fixed tag.app/controllers/assignments_duties_controller.rb (1)
59-71: Extract this duty payload shape into one shared serializer/presenter.The same
{ duty_id, duty_name, max_members_for_duty }mapping now exists here and inapp/serializers/assignment_serializer.rb. Keeping it in one place will make future response changes less error-prone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/assignments_duties_controller.rb` around lines 59 - 71, Extract the duplicate duty payload ({ duty_id, duty_name, max_members_for_duty }) into a single reusable serializer/presenter class (e.g., AssignmentDutyPresenter or AssignmentDutySerializer) and replace the inline mapping in serialized_assignment_duties/serialize_assignment_duty with a call to this new presenter; update app/serializers/assignment_serializer.rb to use the same presenter so both places delegate to the new class (expose a method like as_json or to_hash) and remove the duplicated mapping from the controller.
🤖 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/teams_participants_controller.rb`:
- Around line 29-35: The duty lookup currently uses participant.parent_id which
is incorrect for team participants; instead, first ensure the participant
belongs to an assignment team (gate the action to assignment teams) by checking
team_participant.team is an assignment-type team, then derive the assignment
from team_participant.team (e.g. assignment = team_participant.team.parent or
team_participant.team.assignment) and use that assignment.id in
AssignmentsDuty.find_by(assignment_id: assignment.id, duty_id: duty_id); if the
team is not an assignment team, render the appropriate error response.
In `@app/serializers/assignment_serializer.rb`:
- Around line 4-15: The serializer methods has_role_based_review and
assignment_duties are causing extra DB queries by calling
object.assignments_duties and accessing assignment_duty.duty for each serialized
Assignment; fix it by preloading assignments_duties and their duty association
in the controller (e.g. include :assignments_duties and assignments_duties:
:duty) and change AssignmentSerializer to accept the already-loaded association
data (use object.association(:assignments_duties).loaded? or accept a
precomputed payload) so has_role_based_review checks the in-memory collection
and assignment_duties iterates the preloaded objects without triggering queries.
In `@db/migrate/20260423202332_adjust_duty_limits.rb`:
- Around line 2-5: The migration currently adds
assignments_duties.max_members_for_duty with default 1 and then drops
duties.max_members_for_duty, losing existing per-duty limits; change it to a
safe, reversible migration by: add the new column first
(assignments_duties.max_members_for_duty), backfill values from
duties.max_members_for_duty into assignments_duties via a SQL UPDATE or
ActiveRecord join using the duties and assignments_duties tables, then only
after backfill remove the original duties.max_members_for_duty; implement this
in an explicit up/down or reversible migrate method in adjust_duty_limits.rb and
reference the table names (:duties, :assignments_duties) and column symbol
(:max_members_for_duty) when making the changes.
In `@db/migrate/20260423210000_add_duty_id_to_assignment_questionnaires.rb`:
- Around line 2-4: The migration currently uses add_column
:assignment_questionnaires, :duty_id, :integer which creates an unindexed,
non-foreign-key integer column and mismatches other duty_id columns; replace
this with add_reference :assignment_questionnaires, :duty, type: :bigint, index:
true, foreign_key: true (and set null: false if the association is required) so
the DB gets a bigint-typed FK plus an index and constraint; update any
tests/fixtures if they relied on the old integer column.
In `@db/schema.rb`:
- Line 13: The schema.rb is missing the migration that adds duty_id to
assignment_questionnaires (migration
20260423210000_add_duty_id_to_assignment_questionnaires.rb), so update the
schema by running the pending migration(s) and regenerating the schema dump: run
the migration(s) (e.g., rails db:migrate) in your development environment (or
run the specific migration file), then run rails db:schema:dump (or rails
db:structure:dump if your project uses structure) to produce an updated
db/schema.rb that includes the assignment_questionnaires.duty_id column so the
model’s belongs_to :duty matches the schema.
In `@docker-compose.yml`:
- Line 7: The DATABASE_URL in docker-compose.yml is missing the trailing '?'
expected by your config/database.yml URL rewriting (the .gsub('?',
'_development?') / .gsub('?', '_test?') / .gsub('?', '_production?') logic);
update the DATABASE_URL value to include a trailing '?' (so the value ends with
'?') so the .gsub replacements produce distinct environment-specific database
names and restore test isolation.
---
Outside diff comments:
In `@app/models/participant.rb`:
- Around line 13-30: The duty_limit_within_team validation is running for
CourseParticipant too and uses parent_id as an assignment id; restrict it so it
only runs for assignment participants—either add the guard to the validator
(e.g. change the validation predicate to include a check that this is an
Assignment participant or that parent_type == 'Assignment' / instance_of?
AssignmentParticipant) or add an early return in duty_limit_within_team (return
unless self is an assignment participant) before calling
AssignmentsDuty.find_by; reference duty_limit_within_team, CourseParticipant and
AssignmentParticipant (or parent_type) when making the change.
In `@config/application.rb`:
- Around line 20-35: Remove the unrelated framework/defaults and global cache
changes from this feature PR: revert the addition of config.load_defaults 8.0
and the config.cache_store = :redis_cache_store, { url: ENV['CACHE_STORE'],
expires_in: 3.days } entry in application.rb so the feature only contains
duty/role-specific changes; leave any API-only setting (config.api_only) intact
if it was preexisting, and move the Rails 8 defaults and Redis cache
configuration into a separate follow-up PR or config change commit after the
feature is merged.
In `@docker-compose.yml`:
- Around line 21-23: Replace the hard-coded DB creds in docker-compose.yml by
reading MYSQL_USER, MYSQL_PASSWORD and MYSQL_ROOT_PASSWORD from environment
variables and update the app service's DATABASE_URL to interpolate those env
vars (replace literal values for MYSQL_USER / MYSQL_PASSWORD /
MYSQL_ROOT_PASSWORD and the plaintext password inside DATABASE_URL); add
instructions to create a local .env (and ensure .env is listed in .gitignore)
with keys MYSQL_USER, MYSQL_PASSWORD and MYSQL_ROOT_PASSWORD so developers can
set dev credentials without committing them.
In `@spec/requests/api/v1/teams_participants_controller_spec.rb`:
- Around line 160-182: The spec posts a hard-coded duty_id (2) but doesn't
create the corresponding Duty/AssignmentsDuty, causing a 422; in the 200-path
setup create a Duty (e.g., via Duty.create!) and then an AssignmentsDuty (or
AssignmentsDuties) linking that Duty to the existing assignment (use the
existing assignment variable), and change the payload to use that duty's id
(teams_participant: { duty_id: duty.id } or set let(:payload) to reference the
created duty) so the update_duty validation passes; locate the setup around
AssignmentParticipant/TeamsParticipant (participant_for_update,
teams_participant, payload) and add the Duty and AssignmentsDuty creation there.
---
Nitpick comments:
In `@app/controllers/assignments_duties_controller.rb`:
- Around line 59-71: Extract the duplicate duty payload ({ duty_id, duty_name,
max_members_for_duty }) into a single reusable serializer/presenter class (e.g.,
AssignmentDutyPresenter or AssignmentDutySerializer) and replace the inline
mapping in serialized_assignment_duties/serialize_assignment_duty with a call to
this new presenter; update app/serializers/assignment_serializer.rb to use the
same presenter so both places delegate to the new class (expose a method like
as_json or to_hash) and remove the duplicated mapping from the controller.
In `@docker-compose.yml`:
- Around line 13-16: Add container healthchecks for the services named "db" and
"redis" and update the "app" service's depends_on to wait for service health
where supported; specifically, add a healthcheck block for db (use a mysql-ready
check such as running "mysqladmin ping" or an equivalent probe that returns
success when MySQL is accepting connections) and a healthcheck block for redis
(use "redis-cli ping" or equivalent), and change the "app" service depends_on
entry to use service_healthy/condition: service_healthy so Compose will wait for
those healthchecks to pass before starting the app; adjust timeouts/retries to
reasonable values to avoid false negatives.
- Line 31: The docker-compose.yml currently uses an unpinned Redis image
("image: redis:latest"); change that to a specific, tested tag (e.g.,
"redis:7.2.x" or the exact patch like "redis:7.2.5") to ensure reproducible
deployments and security; update the image line under the Redis service and
commit. Before choosing the tag, search the codebase and configs (environment
variables, connection settings, Dockerfiles, CI, and any redis client usage) for
expected Redis features or version constraints and check other compose/services
for already used Redis major/minor versions to pick a compatible fixed tag.
🪄 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: 8836a2d7-5a18-4aa6-ab83-99bd573c1390
📒 Files selected for processing (15)
Gemfileapp/controllers/assignments_duties_controller.rbapp/controllers/teams_participants_controller.rbapp/models/assignment_questionnaire.rbapp/models/assignments_duty.rbapp/models/participant.rbapp/serializers/assignment_serializer.rbconfig/application.rbconfig/routes.rbdb/migrate/20260423202332_adjust_duty_limits.rbdb/migrate/20260423210000_add_duty_id_to_assignment_questionnaires.rbdb/schema.rbdocker-compose.ymlspec/requests/api/v1/assignments_duties_controller_spec.rbspec/requests/api/v1/teams_participants_controller_spec.rb
| def has_role_based_review | ||
| object.assignments_duties.exists? | ||
| end | ||
|
|
||
| def assignment_duties | ||
| object.assignments_duties.includes(:duty).map do |assignment_duty| | ||
| { | ||
| duty_id: assignment_duty.duty_id, | ||
| duty_name: assignment_duty.duty&.name, | ||
| max_members_for_duty: assignment_duty.max_members_for_duty | ||
| } | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect where AssignmentSerializer is used and whether those paths preload duties.
# Expected result: collection render paths should preload assignments_duties: :duty.
echo "== Assignment render sites =="
rg -nP --type ruby 'AssignmentSerializer|each_serializer:\s*AssignmentSerializer|serializer:\s*AssignmentSerializer|render\s+json:\s+@assignments|render\s+json:\s+@assignment' app/controllers app/serializers
echo
echo "== Duty preloading in controllers =="
rg -nP --type ruby 'includes\([^)]*assignments_duties[^)]*duty|preload\([^)]*assignments_duties[^)]*duty|eager_load\([^)]*assignments_duties[^)]*duty' app/controllersRepository: expertiza/reimplementation-back-end
Length of output: 756
🏁 Script executed:
#!/bin/bash
# Check for other potential uses of AssignmentSerializer or assignment rendering
echo "== All AssignmentSerializer references =="
rg -n 'AssignmentSerializer' --type ruby
echo
echo "== Render json with assignment variable (broader search) =="
rg -nB2 -A2 'render.*json.*assignment' --type ruby app/controllers
echo
echo "== Check for respond_with patterns =="
rg -n 'respond_with.*assignment' --type ruby app/controllers
echo
echo "== Check what student.assignment returns =="
rg -n 'def assignment' --type ruby app/models/user.rb app/models/student.rbRepository: expertiza/reimplementation-back-end
Length of output: 13995
🏁 Script executed:
#!/bin/bash
# Check if Student model exists or if it's part of User
echo "== Student model or relationship =="
fd -e rb . app/models | xargs grep -l "class.*Student" | head -5
echo
echo "== Check AssignmentSerializer attributes =="
cat -n app/serializers/assignment_serializer.rb | head -30
echo
echo "== Check if assignment_duties is accessed in controllers =="
rg -n 'assignment_duties|assignments_duties' app/controllers/student_teams_controller.rbRepository: expertiza/reimplementation-back-end
Length of output: 962
🏁 Script executed:
#!/bin/bash
# Verify the Assignment model associations
echo "== Assignment model class definition and associations =="
cat -n app/models/assignment.rb | head -50
echo
echo "== Check for has_many :assignments_duties in Assignment =="
rg -A5 'has_many.*assignments_duties|has_many.*:assignments_duties' app/models/assignment.rbRepository: expertiza/reimplementation-back-end
Length of output: 3536
These serializer methods trigger unnecessary database queries per assignment render.
Both has_role_based_review and assignment_duties issue separate database queries. Since AssignmentSerializer is currently used in student_teams_controller.rb to serialize single assignments, each render causes 2 extra queries that could be avoided by preloading assignments_duties and assignments_duties.duty in the controller and passing the data directly to the serializer.
🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 4-4: Use 2 (not 5) spaces for indentation.
(Layout/IndentationWidth)
[convention] 5-5: Use 2 (not 5) spaces for indentation.
(Layout/IndentationWidth)
[convention] 8-8: Use 2 (not 5) spaces for indentation.
(Layout/IndentationWidth)
[convention] 9-9: Use 2 (not 5) spaces for indentation.
(Layout/IndentationWidth)
[convention] 10-10: Use 2 (not 5) spaces for indentation.
(Layout/IndentationWidth)
[convention] 11-11: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
(Layout/FirstHashElementIndentation)
🤖 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 4 - 15, The serializer
methods has_role_based_review and assignment_duties are causing extra DB queries
by calling object.assignments_duties and accessing assignment_duty.duty for each
serialized Assignment; fix it by preloading assignments_duties and their duty
association in the controller (e.g. include :assignments_duties and
assignments_duties: :duty) and change AssignmentSerializer to accept the
already-loaded association data (use
object.association(:assignments_duties).loaded? or accept a precomputed payload)
so has_role_based_review checks the in-memory collection and assignment_duties
iterates the preloaded objects without triggering queries.
| def change | ||
| remove_column :duties, :max_members_for_duty, :integer | ||
| add_column :assignments_duties, :max_members_for_duty, :integer, default: 1 | ||
| end |
There was a problem hiding this comment.
Backfill existing limits before dropping the source column.
This migration resets every existing duty limit to 1. Any non-default duties.max_members_for_duty values are lost on migrate, and change is misleading here because the split becomes irreversible once per-assignment values diverge.
Safer migration outline
class AdjustDutyLimits < ActiveRecord::Migration[8.0]
- def change
- remove_column :duties, :max_members_for_duty, :integer
- add_column :assignments_duties, :max_members_for_duty, :integer, default: 1
+ def up
+ add_column :assignments_duties, :max_members_for_duty, :integer, default: 1, null: false
+
+ execute <<~SQL
+ UPDATE assignments_duties ad
+ INNER JOIN duties d ON d.id = ad.duty_id
+ SET ad.max_members_for_duty = COALESCE(d.max_members_for_duty, 1)
+ SQL
+
+ remove_column :duties, :max_members_for_duty, :integer
+ end
+
+ def down
+ raise ActiveRecord::IrreversibleMigration, 'duty limits become assignment-specific after this migration'
end
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrate/20260423202332_adjust_duty_limits.rb` around lines 2 - 5, The
migration currently adds assignments_duties.max_members_for_duty with default 1
and then drops duties.max_members_for_duty, losing existing per-duty limits;
change it to a safe, reversible migration by: add the new column first
(assignments_duties.max_members_for_duty), backfill values from
duties.max_members_for_duty into assignments_duties via a SQL UPDATE or
ActiveRecord join using the duties and assignments_duties tables, then only
after backfill remove the original duties.max_members_for_duty; implement this
in an explicit up/down or reversible migrate method in adjust_duty_limits.rb and
reference the table names (:duties, :assignments_duties) and column symbol
(:max_members_for_duty) when making the changes.
| def change | ||
| add_column :assignment_questionnaires, :duty_id, :integer | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how assignment_questionnaires.duty_id is declared and whether schema includes an index/FK.
rg -n 'add_(column|reference).*assignment_questionnaires.*duty' db/migrate
rg -n 'create_table "assignment_questionnaires"|t\.(integer|bigint) "duty_id"|t\.index \["duty_id"\]|add_foreign_key "assignment_questionnaires", "duties"' db/schema.rbRepository: expertiza/reimplementation-back-end
Length of output: 571
🏁 Script executed:
#!/bin/bash
# Check for other migrations that might add the index
rg -n 'add_index.*assignment_questionnaires.*duty|duty_id' db/migrate/*.rb | head -20
# Check if the migration file matches what's shown - read full content
echo "=== Full migration file content ==="
cat db/migrate/20260423210000_add_duty_id_to_assignment_questionnaires.rb
# Search for foreign key constraint in schema or migrations
echo "=== Foreign key search ==="
rg -n 'add_foreign_key.*assignment_questionnaires|foreign_key.*duty' db/Repository: expertiza/reimplementation-back-end
Length of output: 848
🏁 Script executed:
#!/bin/bash
# Check the AssignmentQuestionnaire model
echo "=== AssignmentQuestionnaire model ==="
find app -name "*assignment_questionnaire*" -type f | head -5
# Search for the model definition
rg -n 'class AssignmentQuestionnaire' app/
# Check for migrations AFTER 20260423210000 that touch assignment_questionnaires
echo "=== Migrations after 20260423210000 ==="
ls -1 db/migrate/*.rb | grep -A 10 '20260423210000' | head -20
# Check if there are any later migrations
rg -n 'assignment_questionnaires' db/migrate/*.rb | grep -v '20260423210000'Repository: expertiza/reimplementation-back-end
Length of output: 789
🏁 Script executed:
#!/bin/bash
# Read the AssignmentQuestionnaire model
echo "=== AssignmentQuestionnaire model content ==="
cat app/models/assignment_questionnaire.rb
# Get full schema entry for assignment_questionnaires including foreign keys
echo "=== Schema for assignment_questionnaires (full) ==="
rg -A 30 'create_table "assignment_questionnaires"' db/schema.rbRepository: expertiza/reimplementation-back-end
Length of output: 1580
🏁 Script executed:
#!/bin/bash
# Verify duties table exists
echo "=== Duties table in schema ==="
rg -A 5 'create_table "duties"' db/schema.rb | head -15
# Check all duty_id column types in schema for consistency
echo "=== All duty_id columns in schema ==="
rg 'bigint "duty_id"|integer "duty_id"' db/schema.rbRepository: expertiza/reimplementation-back-end
Length of output: 516
Use add_reference instead of add_column to create the foreign key and index.
This adds duty_id as a bare integer column without an index or foreign key constraint, allowing orphaned records and degrading lookup/filter performance on this association. Additionally, other duty_id columns in the schema use :bigint, but this migration uses :integer (type mismatch).
Suggested migration fix
class AddDutyIdToAssignmentQuestionnaires < ActiveRecord::Migration[8.0]
def change
- add_column :assignment_questionnaires, :duty_id, :integer
+ add_reference :assignment_questionnaires, :duty, null: true, foreign_key: true, index: true
end
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrate/20260423210000_add_duty_id_to_assignment_questionnaires.rb` around
lines 2 - 4, The migration currently uses add_column :assignment_questionnaires,
:duty_id, :integer which creates an unindexed, non-foreign-key integer column
and mismatches other duty_id columns; replace this with add_reference
:assignment_questionnaires, :duty, type: :bigint, index: true, foreign_key: true
(and set null: false if the association is required) so the DB gets a
bigint-typed FK plus an index and constraint; update any tests/fixtures if they
relied on the old integer column.
| environment: | ||
| RAILS_ENV: development | ||
| DATABASE_URL: mysql2://root:expertiza@db:3306/reimplementation? | ||
| DATABASE_URL: mysql2://root:expertiza@db:3306/reimplementation |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find database.yml file
find . -name "database.yml" -o -name "database.yml.*" | head -20Repository: expertiza/reimplementation-back-end
Length of output: 127
🏁 Script executed:
# Also check for any database configuration files
fd -e yml -e yaml | grep -i databaseRepository: expertiza/reimplementation-back-end
Length of output: 99
🏁 Script executed:
# Read the database.yml file to check for the gsub pattern
cat -n ./config/database.ymlRepository: expertiza/reimplementation-back-end
Length of output: 617
🏁 Script executed:
# Also check if there's a .old version with different content
cat -n ./config/database.yml.oldRepository: expertiza/reimplementation-back-end
Length of output: 2381
🏁 Script executed:
# Verify the exact state of docker-compose.yml to see the full context around line 7
cat -n ./docker-compose.ymlRepository: expertiza/reimplementation-back-end
Length of output: 1124
Add trailing ? to DATABASE_URL in docker-compose.yml.
The current DATABASE_URL (line 7) is missing the trailing ? required by your config/database.yml URL rewriting logic. Lines 10, 14, and 18 of database.yml use .gsub('?', '_development?'), .gsub('?', '_test?'), and .gsub('?', '_production?') to differentiate environment-specific databases. Without the ?, all three environments will connect to the same database (reimplementation), breaking test isolation.
Suggested fix
- DATABASE_URL: mysql2://root:expertiza@db:3306/reimplementation
+ DATABASE_URL: mysql2://root:expertiza@db:3306/reimplementation?📝 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.
| DATABASE_URL: mysql2://root:expertiza@db:3306/reimplementation | |
| DATABASE_URL: mysql2://root:expertiza@db:3306/reimplementation? |
🧰 Tools
🪛 Checkov (3.2.525)
[medium] 7-8: Basic Auth Credentials
(CKV_SECRET_4)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yml` at line 7, The DATABASE_URL in docker-compose.yml is
missing the trailing '?' expected by your config/database.yml URL rewriting (the
.gsub('?', '_development?') / .gsub('?', '_test?') / .gsub('?', '_production?')
logic); update the DATABASE_URL value to include a trailing '?' (so the value
ends with '?') so the .gsub replacements produce distinct environment-specific
database names and restore test isolation.
Add patch for updating duty_id
Generated by 🚫 Danger |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/participants_controller.rb`:
- Around line 103-115: The update_duty action incorrectly returns HTTP 201
(:created) for a successful update; change the render in update_duty (method
update_duty in ParticipantsController) to use status: :ok to match REST
conventions and other controllers (UsersController#update,
RolesController#update, DutiesController#update); update the success render call
from render json: participant, status: :created to render json: participant,
status: :ok and run related controller specs to ensure expected status
assertions are updated.
In `@spec/requests/api/v1/participants_controller_spec.rb`:
- Line 360: The failing spec creates a Duty without the required instructor
reference; update the `let!(:role_duty)` declaration to supply an instructor
(e.g. create or use the spec's existing instructor fixture) by calling
`Duty.create!(name: 'Developer', instructor: instructor)` or `Duty.create!(name:
'Developer', instructor_id: instructor.id)` so the `Duty.create!` call (assigned
to `role_duty`) satisfies the non-null `instructor_id` constraint.
🪄 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: 973d6461-7eba-4ce1-97e8-1a9f7436e74a
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.gitignoreapp/controllers/participants_controller.rbapp/serializers/participant_serializer.rbconfig/routes.rbdb/schema.rbsetup.shspec/requests/api/v1/participants_controller_spec.rb
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- setup.sh
| # Update the specified participant duty | ||
| # PATCH /participants/:id/duty | ||
| def update_duty | ||
| participant = find_participant | ||
| return unless participant | ||
|
|
||
| participant.duty_id = params[:duty_id].presence | ||
| if participant.save | ||
| render json: participant, status: :created | ||
| else | ||
| render json: participant.errors, status: :unprocessable_entity | ||
| end | ||
| end |
There was a problem hiding this comment.
Use :ok instead of :created for update operations.
The update_duty action returns HTTP 201 (:created) on line 111, but this endpoint updates an existing participant resource. Per REST conventions and codebase patterns (see UsersController#update, RolesController#update, DutiesController#update which all return :ok), successful updates should return HTTP 200.
Proposed fix
if participant.save
- render json: participant, status: :created
+ render json: participant, status: :ok
else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/participants_controller.rb` around lines 103 - 115, The
update_duty action incorrectly returns HTTP 201 (:created) for a successful
update; change the render in update_duty (method update_duty in
ParticipantsController) to use status: :ok to match REST conventions and other
controllers (UsersController#update, RolesController#update,
DutiesController#update); update the success render call from render json:
participant, status: :created to render json: participant, status: :ok and run
related controller specs to ensure expected status assertions are updated.
| } | ||
| } | ||
|
|
||
| let!(:role_duty) { Duty.create!(name: 'Developer') } |
There was a problem hiding this comment.
Missing required instructor_id when creating Duty.
The duties table has instructor_id defined as null: false (see db/schema.rb line 181). This Duty.create! call will raise an ActiveRecord::NotNullViolation because instructor_id is not provided.
🐛 Proposed fix
- let!(:role_duty) { Duty.create!(name: 'Developer') }
+ let!(:role_duty) { Duty.create!(name: 'Developer', instructor_id: instructor.id) }📝 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.
| let!(:role_duty) { Duty.create!(name: 'Developer') } | |
| let!(:role_duty) { Duty.create!(name: 'Developer', instructor_id: instructor.id) } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/requests/api/v1/participants_controller_spec.rb` at line 360, The
failing spec creates a Duty without the required instructor reference; update
the `let!(:role_duty)` declaration to supply an instructor (e.g. create or use
the spec's existing instructor fixture) by calling `Duty.create!(name:
'Developer', instructor: instructor)` or `Duty.create!(name: 'Developer',
instructor_id: instructor.id)` so the `Duty.create!` call (assigned to
`role_duty`) satisfies the non-null `instructor_id` constraint.
| head :no_content | ||
| end | ||
|
|
||
| # PATCH /assignments/:assignment_id/duties/:id/limit |
There was a problem hiding this comment.
Please add a comment saying that the limit is the max # of team members who can have this duty.
| team_participant.update(duty_id: duty_id) | ||
| render json: { message: "Duty updated successfully" }, status: :ok | ||
| participant = team_participant.participant | ||
|
|
|
|
||
| def action_allowed! | ||
| unless current_user_has_instructor_privileges? | ||
| render json: { error: 'Not authorized' }, status: :forbidden |
There was a problem hiding this comment.
action_allowed! renders 403 but does not return, so the action may continue after unauthorized access.
6 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests