E2615 -Testing User model and Users controller #346
Open
AtharvaW195 wants to merge 6 commits into
Open
Conversation
… defaults
- Add presence, uniqueness, format, and length validation tests for
name, email, and full_name fields
- Add association tests for all belongs_to and has_many declarations
using shoulda-matchers
- Fix has_many :assignments to use through: :participants
- Add has_many :instructed_assignments with correct foreign key
- Remove incorrect has_many :invitations (no valid foreign key path)
- Fix scopes to use joins(:role).where(roles: { name: }) instead of
hardcoded Role::*_ID constants that do not exist
- Add scope tests with two-sided inclusion/exclusion assertions
- Add delegated role predicate tests for student?, ta?, instructor?,
administrator?, super_administrator?
- Add defaults tests verifying after_initialize sets all flags correctly
📝 WalkthroughWalkthroughRemoves automatic default passwords in user creation, tightens User model password validation and role/STI handling, adjusts TA model associations and instructor resolution, adds a migration enforcing a composite unique index, and introduces extensive model and request specs for users. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as UsersController
participant Model as User Model
participant Validator as Validator
participant DB as Database
Client->>Controller: POST /users (create) with user_params
Controller->>Model: User.new(user_params)
Model->>Validator: Evaluate password_required?(context)
alt password_required? = true
Validator->>Model: enforce presence + length
else password_required? = false
Validator->>Model: skip presence, enforce length only
end
Model->>Model: has_secure_password sets password_digest (if password present)
Model->>DB: Save user
DB-->>Model: Persist result
Model-->>Controller: user or errors
Controller-->>Client: 201 or 422 response
sequenceDiagram
participant Client
participant Controller as UsersController
participant Model as User.instantiate
participant RoleService as Role Resolution
participant Subclass as Role Subclass/Object
Client->>Controller: Request returning user records
Controller->>Model: User.instantiate(record)
Model->>RoleService: Inspect record.role.name
alt matches Student regex
RoleService->>Subclass: instantiate Student
Subclass-->>Model: Student instance
else matches TA regex
RoleService->>Subclass: instantiate TA
Subclass-->>Model: TA instance
else matches Instructor regex
RoleService->>Subclass: instantiate Instructor
Subclass-->>Model: Instructor instance
else no match
RoleService-->>Model: return original record
end
Model-->>Controller: typed object
Controller-->>Client: Respond with object representation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Generated by 🚫 Danger |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
app/models/user.rb (2)
19-28: Fix comment formatting and indentation.Static analysis flags inconsistent comment indentation and missing spaces after
#. While functional, cleaning these up improves readability.♻️ Proposed formatting fix
- `#Looking` at invitation.rb, invitations relate to users only through participants - #(from_id and to_id both point to AssignmentParticipant, not User). - `#There` is no user foreign key on the invitations table at all. - # has_many :invitations + # Looking at invitation.rb, invitations relate to users only through participants + # (from_id and to_id both point to AssignmentParticipant, not User). + # There is no user foreign key on the invitations table at all. + # has_many :invitations has_many :participants - # A user participates in assignments via the participants join table - has_many :assignments, through: :participants -# A user can also be the instructor of many assignments directly -# via instructor_id on the assignments table + # A user participates in assignments via the participants join table + has_many :assignments, through: :participants + # A user can also be the instructor of many assignments directly + # via instructor_id on the assignments table has_many :instructed_assignments, class_name: 'Assignment', foreign_key: 'instructor_id'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/user.rb` around lines 19 - 28, Fix the inconsistent comment formatting in user.rb by normalizing comment indentation and adding a single space after each leading hash; update the block above the associations (the lines mentioning invitation.rb, invitations relation, and notes about assignments/instructor) so each comment starts with "# " and aligns with the surrounding code, and ensure commented-out association "# has_many :invitations" uses the same style; leave the association lines (has_many :participants, has_many :assignments, has_many :instructed_assignments) unchanged except for aligning them vertically with the cleaned comments.
126-128: Simplify redundant ternary.The ternary
role.ta? ? true : falseis redundant sincerole.ta?already returns a boolean.♻️ Proposed simplification
def teaching_assistant? - role.ta? ? true : false + role.ta? end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/user.rb` around lines 126 - 128, The method teaching_assistant? contains a redundant ternary; replace its body with a direct boolean return of role.ta? (i.e., change def teaching_assistant? ... end to simply return role.ta?) so the method delegates the boolean check to role.ta? without the unnecessary ? true : false. Ensure you update the method definition for teaching_assistant? accordingly.spec/requests/api/v1/users_controller_spec.rb (1)
157-168: Fix inconsistent indentation.Line 157 has an extra leading space causing indentation inconsistency with the surrounding code.
♻️ Proposed fix
- let(:payload) do + let(:payload) do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/api/v1/users_controller_spec.rb` around lines 157 - 168, The let(:payload) block has an extra leading space on its declaration causing inconsistent indentation; remove the stray leading space before "let(:payload) do" so the "let(:payload)" line aligns with the surrounding examples and ensure the nested user hash remains indented consistently (check the let(:payload) block and its user: { ... } contents).app/models/ta.rb (1)
14-17: Consider eager loading to avoid N+1 queries.
courses_assisted_withperforms a separateCourse.findfor each mapping, causing N+1 queries. This could be optimized:♻️ Proposed optimization
def courses_assisted_with - courses = TaMapping.where(user_id: id) - courses.map { |c| Course.find(c.course_id) } + Course.joins(:ta_mappings).where(ta_mappings: { user_id: id }) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/ta.rb` around lines 14 - 17, The current courses_assisted_with method issues N+1 queries by calling Course.find for each TaMapping; change it to fetch all courses in a single query by using the TaMapping -> Course relationship (e.g., query Course where id IN the set of TaMapping.course_id for this user or use an ActiveRecord association such as has_many :ta_mappings and has_many :courses, through: :ta_mappings) so that courses_assisted_with performs one DB query instead of a find per mapping.spec/models/user_spec.rb (1)
396-406: Consider adding test for TA without assigned instructor.The test covers the happy path where TA has a parent, but there's no test for when a TA has
nilparent_id. Given thatmy_instructornow returnsparent_iddirectly, testing this edge case would help document expected behavior.📝 Suggested additional test
it 'handles teaching assistant without parent' do ta = create(:user, role: ta_role, institution: institution, parent: nil) # Document expected behavior - either raise error or return nil expect(ta.instructor_id).to be_nil # or expect { ta.instructor_id }.to raise_error end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/user_spec.rb` around lines 396 - 406, Add a test for the TA-without-parent edge case: create a TA user with role ta_role, institution: institution, and parent: nil, then assert the expected behavior of instructor_id (since my_instructor now returns parent_id directly) — e.g., expect(ta.instructor_id).to be_nil (or if the intended behavior is to raise, use expect { ta.instructor_id }.to raise_error(...)); place this new example alongside the existing instructor/TA/student specs and reference the instructor_id (and my_instructor if relevant) methods in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/ta.rb`:
- Around line 10-12: my_instructor currently returns parent_id which can be nil
because belongs_to :parent is optional; add a presence validation to Ta
(validates :parent_id, presence: true) to ensure a TA always has a parent, and
update/or keep the my_instructor method (Ta#my_instructor) as the canonical
getter for the instructor id so callers like User#instructor_id (which calls
Ta.find(id).my_instructor) always receive a non-nil value; if you prefer a
defensive approach instead, change Ta#my_instructor to return parent&.id or
raise a clear error when parent_id is nil so failures are explicit.
In `@db/schema.rb`:
- Line 411: The schema has an unconditional unique DB index named
index_teams_participants_on_participant_id_unique that contradicts the
TeamsParticipant model validation (validates :participant_id, uniqueness: {
scope: :team_id }); update the DB to match the model by writing a migration that
removes the existing unique index
(index_teams_participants_on_participant_id_unique) and adds a composite unique
index on [:team_id, :participant_id] (e.g., name it
index_teams_participants_on_team_participant_unique) so a participant can belong
to multiple teams while preserving per-team uniqueness.
In `@spec/requests/api/v1/users_controller_spec.rb`:
- Around line 668-674: In role_users, after calling Role.find_by(name:) add a
nil check that immediately renders a 404 JSON error (e.g. render json: { error:
"Role '#{name}' not found" }, status: :not_found) and returns before calling
role.users; keep the existing rescue for ActiveRecord::RecordNotFound as-is.
Then update the spec for the invalid role case to expect a 404 response instead
of 500 (adjust the example that uses let(:name) { 'role_that_does_not_exist' }
and the response assertion).
---
Nitpick comments:
In `@app/models/ta.rb`:
- Around line 14-17: The current courses_assisted_with method issues N+1 queries
by calling Course.find for each TaMapping; change it to fetch all courses in a
single query by using the TaMapping -> Course relationship (e.g., query Course
where id IN the set of TaMapping.course_id for this user or use an ActiveRecord
association such as has_many :ta_mappings and has_many :courses, through:
:ta_mappings) so that courses_assisted_with performs one DB query instead of a
find per mapping.
In `@app/models/user.rb`:
- Around line 19-28: Fix the inconsistent comment formatting in user.rb by
normalizing comment indentation and adding a single space after each leading
hash; update the block above the associations (the lines mentioning
invitation.rb, invitations relation, and notes about assignments/instructor) so
each comment starts with "# " and aligns with the surrounding code, and ensure
commented-out association "# has_many :invitations" uses the same style; leave
the association lines (has_many :participants, has_many :assignments, has_many
:instructed_assignments) unchanged except for aligning them vertically with the
cleaned comments.
- Around line 126-128: The method teaching_assistant? contains a redundant
ternary; replace its body with a direct boolean return of role.ta? (i.e., change
def teaching_assistant? ... end to simply return role.ta?) so the method
delegates the boolean check to role.ta? without the unnecessary ? true : false.
Ensure you update the method definition for teaching_assistant? accordingly.
In `@spec/models/user_spec.rb`:
- Around line 396-406: Add a test for the TA-without-parent edge case: create a
TA user with role ta_role, institution: institution, and parent: nil, then
assert the expected behavior of instructor_id (since my_instructor now returns
parent_id directly) — e.g., expect(ta.instructor_id).to be_nil (or if the
intended behavior is to raise, use expect { ta.instructor_id }.to
raise_error(...)); place this new example alongside the existing
instructor/TA/student specs and reference the instructor_id (and my_instructor
if relevant) methods in the test.
In `@spec/requests/api/v1/users_controller_spec.rb`:
- Around line 157-168: The let(:payload) block has an extra leading space on its
declaration causing inconsistent indentation; remove the stray leading space
before "let(:payload) do" so the "let(:payload)" line aligns with the
surrounding examples and ensure the nested user hash remains indented
consistently (check the let(:payload) block and its user: { ... } contents).
🪄 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: 9fa0fae0-2dee-4e7f-92ff-aa04f3725c24
📒 Files selected for processing (6)
app/controllers/users_controller.rbapp/models/ta.rbapp/models/user.rbdb/schema.rbspec/models/user_spec.rbspec/requests/api/v1/users_controller_spec.rb
💤 Files with no reviewable changes (1)
- app/controllers/users_controller.rb
Comment on lines
+668
to
+674
| # Scenario: invalid role name currently triggers server error in controller | ||
| # (graceful handling would be 404 after controller fix) | ||
| response '500', 'Returns server error for invalid role name (current behavior)' do | ||
| let(:name) { 'role_that_does_not_exist' } | ||
|
|
||
| run_test! | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the controller implementation
rg -n 'def role_users' --type ruby -A10 app/controllers/Repository: expertiza/reimplementation-back-end
Length of output: 798
Test documents a controller bug that should be fixed.
The role_users action calls role.users on a potentially nil role when the role name doesn't exist, causing a NoMethodError that isn't caught by the existing rescue clause, resulting in the 500 response. The controller should handle missing roles with a 404 response instead.
Add a nil check after Role.find_by(name:):
Suggested controller fix
def role_users
name = params[:name].split('_').map(&:capitalize).join(' ')
role = Role.find_by(name:)
return render json: { error: "Role '#{name}' not found" }, status: :not_found if role.nil?
users = role.users
render json: users, status: :ok
rescue ActiveRecord::RecordNotFound => e
render json: { error: e.message }, status: :not_found
endThen update the test to expect 404:
-response '500', 'Returns server error for invalid role name (current behavior)' do
+response '404', 'Returns not found for invalid role name' do📝 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
| # Scenario: invalid role name currently triggers server error in controller | |
| # (graceful handling would be 404 after controller fix) | |
| response '500', 'Returns server error for invalid role name (current behavior)' do | |
| let(:name) { 'role_that_does_not_exist' } | |
| run_test! | |
| end | |
| # Scenario: invalid role name currently triggers server error in controller | |
| # (graceful handling would be 404 after controller fix) | |
| response '404', 'Returns not found for invalid role name' do | |
| let(:name) { 'role_that_does_not_exist' } | |
| run_test! | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/requests/api/v1/users_controller_spec.rb` around lines 668 - 674, In
role_users, after calling Role.find_by(name:) add a nil check that immediately
renders a 404 JSON error (e.g. render json: { error: "Role '#{name}' not found"
}, status: :not_found) and returns before calling role.users; keep the existing
rescue for ActiveRecord::RecordNotFound as-is. Then update the spec for the
invalid role case to expect a 404 response instead of 500 (adjust the example
that uses let(:name) { 'role_that_does_not_exist' } and the response assertion).
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/user.rb (1)
110-116:⚠️ Potential issue | 🟠 MajorGuard
recursively_parent_ofagainst cycles.A parent loop like
A -> B -> Anever hitsnil,self, or the super-admin stop case here, so impersonation checks can recurse untilSystemStackError.Suggested fix
def recursively_parent_of(user) - p = user.parent - return false if p.nil? - return true if p == self - return false if p.role.super_administrator? - - recursively_parent_of(p) + seen_ids = [] + current = user.parent + + until current.nil? + return true if current == self + return false if current.role.super_administrator? || seen_ids.include?(current.id) + + seen_ids << current.id + current = current.parent + end + + false end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/user.rb` around lines 110 - 116, recursively_parent_of can infinite-loop on cyclic parent links; fix by tracking visited nodes and returning false on cycles: in recursively_parent_of(user) initialize a seen set (e.g., seen ||= Set.new or an array of ids), check and return false if p.id is already in seen, add p.id to seen before recursing, and pass seen into the recursive call (change signature to accept an optional seen parameter or convert to an iterative loop using a visited set); reference the method name recursively_parent_of and the local variable p when adding these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/user.rb`:
- Around line 167-168: User#generate_jwt is creating HS256 tokens with
secret_key_base which no longer matches JsonWebToken.decode (which expects RS256
with RSA keys). Update generate_jwt to sign with the same RSA private key and
algorithm used by lib/json_web_token.rb/JsonWebToken.encode: load the app's JWT
RSA private key from credentials (the same credential used by JsonWebToken),
build the same payload (id and exp) and encode using RS256 so tokens generated
by User#generate_jwt are accepted by JsonWebToken.decode. Ensure you reference
the method name generate_jwt and the JsonWebToken implementation so the key
source and alg match exactly.
- Around line 48-62: The custom class method self.instantiate is shadowing
ActiveRecord.instantiate (which expects attributes hash)—rename this method to
something like self.cast_for_role(record) to avoid breaking Rails' internal
materialization; update all call sites (e.g., the managed_users action in
UsersController that currently calls User.instantiate) to call
User.cast_for_role(record) instead, and search the codebase for any other
references to instantiate to replace them so Rails' instantiate signature
remains untouched.
In `@spec/models/user_spec.rb`:
- Around line 314-337: The tests call User.instantiate with an already-built
model which doesn't mirror Rails' instantiate(attributes, column_types = {})
usage and can mask real loading bugs; update the specs to persist users (e.g.,
create/save) and use the actual ActiveRecord load path (e.g., User.find or
equivalent finder) to load the record and assert the returned object's class
(Instructor, Ta, Administrator, SuperAdministrator, or User), or if instantiate
is intended as an explicit caster, rename the method from instantiate to
something clearer and adjust tests to call that new API instead.
- Around line 564-593: The tests currently call JWT.decode(token, nil, false)
which disables verification and hides an algorithm/key mismatch between
User#generate_jwt (HS256) and production decoding (JsonWebToken.decode expecting
RS256); update the specs to call JsonWebToken.decode(token) (or the same public
method authenticate_request! uses) when decoding tokens so the test exercises
the real verification path and fails if algorithm/key are mismatched, and keep
references to User#generate_jwt, JsonWebToken.decode, and authenticate_request!
so it's clear which code must stay consistent.
---
Outside diff comments:
In `@app/models/user.rb`:
- Around line 110-116: recursively_parent_of can infinite-loop on cyclic parent
links; fix by tracking visited nodes and returning false on cycles: in
recursively_parent_of(user) initialize a seen set (e.g., seen ||= Set.new or an
array of ids), check and return false if p.id is already in seen, add p.id to
seen before recursing, and pass seen into the recursive call (change signature
to accept an optional seen parameter or convert to an iterative loop using a
visited set); reference the method name recursively_parent_of and the local
variable p when adding these checks.
🪄 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: 8d84597e-f861-4131-88a3-53ebbd7c556f
📒 Files selected for processing (7)
app/controllers/users_controller.rbapp/models/ta.rbapp/models/user.rbdb/migrate/20260428000000_update_teams_participants_unique_index.rbdb/schema.rbspec/models/user_spec.rbspec/requests/api/v1/users_controller_spec.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/controllers/users_controller.rb
- app/models/ta.rb
Comment on lines
48
to
62
| def self.instantiate(record) | ||
| case record.role | ||
| when Role::TEACHING_ASSISTANT | ||
| case record.role.name | ||
| when /Super Administrator/ | ||
| record.becomes(SuperAdministrator) | ||
| when /Teaching Assistant/ | ||
| record.becomes(Ta) | ||
| when Role::INSTRUCTOR | ||
| when /Instructor/ | ||
| record.becomes(Instructor) | ||
| when Role::ADMINISTRATOR | ||
| when /Administrator/ | ||
| record.becomes(Administrator) | ||
| when Role::SUPER_ADMINISTRATOR | ||
| record.becomes(SuperAdministrator) | ||
| else | ||
| super | ||
| # Student or other roles remain as User | ||
| record | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.rb" | grep -E "(user|spec|test)" | head -20Repository: expertiza/reimplementation-back-end
Length of output: 948
🏁 Script executed:
cat -n app/models/user.rb | head -100Repository: expertiza/reimplementation-back-end
Length of output: 4388
🏁 Script executed:
rg "User\.find|User\.all|\.instantiate" --type rb -A 2 -B 2Repository: expertiza/reimplementation-back-end
Length of output: 106
🏁 Script executed:
rg "instantiate" --type rb -A 3 -B 3Repository: expertiza/reimplementation-back-end
Length of output: 106
🏁 Script executed:
rg "instantiate" -A 3 -B 3Repository: expertiza/reimplementation-back-end
Length of output: 3014
🏁 Script executed:
rg "\.find|\.all" --max-count 20 -B 2 -A 2Repository: expertiza/reimplementation-back-end
Length of output: 50392
🏁 Script executed:
rg "class.*Administrator|class.*Ta|class.*Instructor" --max-count 10Repository: expertiza/reimplementation-back-end
Length of output: 1306
🏁 Script executed:
find . -type f -name "*_spec.rb" -o -name "*_test.rb" | xargs grep -l "instantiate\|User\.find\|User\.all" 2>/dev/null | head -5Repository: expertiza/reimplementation-back-end
Length of output: 161
🏁 Script executed:
cat -n spec/models/user_spec.rb | grep -A 30 "describe '\.instantiate'"Repository: expertiza/reimplementation-back-end
Length of output: 1602
🏁 Script executed:
rg "User\.find\(|User\.all" app/controllers/users_controller.rb -B 2 -A 5Repository: expertiza/reimplementation-back-end
Length of output: 1040
🏁 Script executed:
rg "def test_\|it '" spec/models/user_spec.rb | head -30Repository: expertiza/reimplementation-back-end
Length of output: 61
Rename method to avoid colliding with Rails's instantiate signature.
The custom method overrides Active Record's instantiate(attributes, column_types = {}) with an incompatible signature expecting a model instance instead of an attributes hash. This breaks the contract with Rails's finder methods (User.find, User.all, etc.), which internally call instantiate(attributes_hash, column_types_dict) during record materialization.
Rename to cast_for_role or similar to make the intent explicit and avoid shadowing Rails internals.
Suggested fix
- def self.instantiate(record)
+ def self.cast_for_role(record)
case record.role.name
when /Super Administrator/
record.becomes(SuperAdministrator)
when /Teaching Assistant/
record.becomes(Ta)
when /Instructor/
record.becomes(Instructor)
when /Administrator/
record.becomes(Administrator)
else
# Student or other roles remain as User
record
end
endUpdate the controller call at app/controllers/users_controller.rb:managed_users to use the new method name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/user.rb` around lines 48 - 62, The custom class method
self.instantiate is shadowing ActiveRecord.instantiate (which expects attributes
hash)—rename this method to something like self.cast_for_role(record) to avoid
breaking Rails' internal materialization; update all call sites (e.g., the
managed_users action in UsersController that currently calls User.instantiate)
to call User.cast_for_role(record) instead, and search the codebase for any
other references to instantiate to replace them so Rails' instantiate signature
remains untouched.
Comment on lines
167
to
+168
| def generate_jwt | ||
| JWT.encode({ id: id, exp: 60.days.from_now.to_i }, Rails.application.credentials.secret_key_base) | ||
| JWT.encode({ id: id, exp: 60.days.from_now.to_i }, Rails.application.credentials.secret_key_base, 'HS256') |
There was a problem hiding this comment.
User#generate_jwt no longer matches the app's decoder.
app/controllers/concerns/jwt_token.rb authenticates with JsonWebToken.decode, and lib/json_web_token.rb signs tokens with RS256 RSA keys. This method now emits HS256 tokens with secret_key_base, so tokens generated here will be rejected by authenticated endpoints.
Suggested fix
def generate_jwt
- JWT.encode({ id: id, exp: 60.days.from_now.to_i }, Rails.application.credentials.secret_key_base, 'HS256')
+ JsonWebToken.encode({ id: id }, 60.days.from_now)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def generate_jwt | |
| JWT.encode({ id: id, exp: 60.days.from_now.to_i }, Rails.application.credentials.secret_key_base) | |
| JWT.encode({ id: id, exp: 60.days.from_now.to_i }, Rails.application.credentials.secret_key_base, 'HS256') | |
| def generate_jwt | |
| JsonWebToken.encode({ id: id }, 60.days.from_now) | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/user.rb` around lines 167 - 168, User#generate_jwt is creating
HS256 tokens with secret_key_base which no longer matches JsonWebToken.decode
(which expects RS256 with RSA keys). Update generate_jwt to sign with the same
RSA private key and algorithm used by lib/json_web_token.rb/JsonWebToken.encode:
load the app's JWT RSA private key from credentials (the same credential used by
JsonWebToken), build the same payload (id and exp) and encode using RS256 so
tokens generated by User#generate_jwt are accepted by JsonWebToken.decode.
Ensure you reference the method name generate_jwt and the JsonWebToken
implementation so the key source and alg match exactly.
Comment on lines
+314
to
+337
| it 'returns Instructor for instructor role' do | ||
| user = create(:user, role: instructor_role, institution: institution) | ||
| expect(User.instantiate(user)).to be_a(Instructor) | ||
| end | ||
|
|
||
| it 'returns Ta for teaching assistant role' do | ||
| user = create(:user, role: ta_role, institution: institution) | ||
| expect(User.instantiate(user)).to be_a(Ta) | ||
| end | ||
|
|
||
| it 'returns Administrator for administrator role' do | ||
| user = create(:user, role: admin_role, institution: institution) | ||
| expect(User.instantiate(user)).to be_a(Administrator) | ||
| end | ||
|
|
||
| it 'returns SuperAdministrator for super administrator role' do | ||
| user = create(:user, role: super_admin_role, institution: institution) | ||
| expect(User.instantiate(user)).to be_a(SuperAdministrator) | ||
| end | ||
|
|
||
| it 'returns User as-is for student role' do | ||
| user = create(:user, role: student_role, institution: institution) | ||
| expect(User.instantiate(user)).to be_a(User) | ||
| expect(User.instantiate(user)).not_to be_a(Instructor) |
There was a problem hiding this comment.
These examples are exercising a helper shape that Active Record never uses.
User.instantiate(user) passes an already-built model, but Rails calls instantiate(attributes, column_types = {}) with raw attribute hashes during finder loading. These specs can stay green while real User.find paths still blow up. (api.rubyonrails.org)
A better regression here is to persist a row and assert that the actual load path returns the right subclass (or rename the method if it is meant to be an explicit caster).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/models/user_spec.rb` around lines 314 - 337, The tests call
User.instantiate with an already-built model which doesn't mirror Rails'
instantiate(attributes, column_types = {}) usage and can mask real loading bugs;
update the specs to persist users (e.g., create/save) and use the actual
ActiveRecord load path (e.g., User.find or equivalent finder) to load the record
and assert the returned object's class (Instructor, Ta, Administrator,
SuperAdministrator, or User), or if instantiate is intended as an explicit
caster, rename the method from instantiate to something clearer and adjust tests
to call that new API instead.
Comment on lines
+564
to
+593
| it 'generates a valid JWT token' do | ||
| token = user.generate_jwt | ||
|
|
||
| expect(token).to be_a(String) | ||
| expect(token.split('.').length).to eq(3) # JWT has 3 parts | ||
| end | ||
|
|
||
| it 'encodes user id in token' do | ||
| token = user.generate_jwt | ||
| decoded = JWT.decode(token, nil, false) | ||
| expect(decoded[0]['id']).to eq(user.id) | ||
| end | ||
|
|
||
| it 'sets expiration to 60 days from now' do | ||
| token = user.generate_jwt | ||
| decoded = JWT.decode(token, nil, false) | ||
| exp_time = Time.at(decoded[0]['exp']) | ||
| expect(exp_time).to be_between(59.days.from_now, 61.days.from_now) | ||
| end | ||
|
|
||
| it 'generates different tokens on each call' do | ||
| token1 = user.generate_jwt | ||
| decoded1 = JWT.decode(token1, nil, false) | ||
| token2 = user.generate_jwt | ||
| decoded2 = JWT.decode(token2, nil, false) | ||
| # Both encode the same user id; tokens may be identical within same second — just verify structure | ||
| expect(token1).to be_a(String) | ||
| expect(decoded1[0]['id']).to eq(user.id) | ||
| expect(decoded2[0]['id']).to eq(user.id) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the JsonWebToken class and User model
find . -type f -name "*.rb" | grep -E "(json_web_token|user\.rb)" | head -20Repository: expertiza/reimplementation-back-end
Length of output: 222
🏁 Script executed:
# Search for JsonWebToken class definition
rg "class JsonWebToken" -A 5Repository: expertiza/reimplementation-back-end
Length of output: 467
🏁 Script executed:
# Search for the User#generate_jwt method
rg "def generate_jwt" -B 2 -A 10Repository: expertiza/reimplementation-back-end
Length of output: 360
🏁 Script executed:
# Look for authenticate_request! method
rg "def authenticate_request!" -B 2 -A 10Repository: expertiza/reimplementation-back-end
Length of output: 921
🏁 Script executed:
# Check JWT.decode usage patterns in the codebase
rg "JWT\.decode" -B 2 -A 2Repository: expertiza/reimplementation-back-end
Length of output: 1925
🏁 Script executed:
# Check the full jwt_token.rb file to see what auth_token method does
rg "def auth_token|def http_token|def user_id_in_token" -A 5Repository: expertiza/reimplementation-back-end
Length of output: 964
🏁 Script executed:
# Check the entire jwt_token.rb concern file
cat app/controllers/concerns/jwt_token.rbRepository: expertiza/reimplementation-back-end
Length of output: 960
🏁 Script executed:
# Look for where JsonWebToken.decode is actually called
rg "JsonWebToken\.decode" -B 2 -A 2Repository: expertiza/reimplementation-back-end
Length of output: 680
🏁 Script executed:
# Let's see the full JsonWebToken implementation
cat lib/json_web_token.rbRepository: expertiza/reimplementation-back-end
Length of output: 1182
Use JsonWebToken.decode in tests to catch the algorithm/key mismatch in production code.
The spec's use of JWT.decode(token, nil, false) skips verification and masks a critical bug: User#generate_jwt creates tokens with HS256, but authenticate_request! (which calls JsonWebToken.decode) expects RS256 with RSA keys. These tokens will fail in production.
Example fix
- decoded = JWT.decode(token, nil, false)
- expect(decoded[0]['id']).to eq(user.id)
+ decoded = JsonWebToken.decode(token)
+ expect(decoded[:id]).to eq(user.id)🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 566-566: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 568-568: Unnecessary spacing detected.
(Layout/ExtraSpacing)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/models/user_spec.rb` around lines 564 - 593, The tests currently call
JWT.decode(token, nil, false) which disables verification and hides an
algorithm/key mismatch between User#generate_jwt (HS256) and production decoding
(JsonWebToken.decode expecting RS256); update the specs to call
JsonWebToken.decode(token) (or the same public method authenticate_request!
uses) when decoding tokens so the test exercises the real verification path and
fails if algorithm/key are mismatched, and keep references to User#generate_jwt,
JsonWebToken.decode, and authenticate_request! so it's clear which code must
stay consistent.
aanand-1706
reviewed
May 5, 2026
| def role_users | ||
| name = params[:name].split('_').map(&:capitalize).join(' ') | ||
| role = Role.find_by(name:) | ||
| unless role |
Contributor
There was a problem hiding this comment.
role = Role.find_by!(name:)
Isn't this better?
aanand-1706
reviewed
May 5, 2026
Comment on lines
3
to
+4
| class Ta < User | ||
| validates :parent_id, presence: true |
Contributor
There was a problem hiding this comment.
Could this break existing entries?
aanand-1706
reviewed
May 5, 2026
|
|
||
| class UpdateTeamsParticipantsUniqueIndex < ActiveRecord::Migration[8.0] | ||
| def change | ||
| remove_index :teams_participants, name: :index_teams_participants_on_participant_id_unique |
Contributor
There was a problem hiding this comment.
How was index_teams_participants_on_participant_id_unique being used earlier?
aanand-1706
reviewed
May 5, 2026
| def courses_assisted_with | ||
| courses = TaMapping.where(ta_id: id) | ||
| courses.map { |c| Course.find(c.course_id) } | ||
| Course.joins(:ta_mappings).where(ta_mappings: { user_id: id }).distinct.to_a |
Contributor
There was a problem hiding this comment.
Is user_id present in ta_mappings?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests
Chores