E2617. Testing Questionnaire and Course Models#336
Open
z0brooks wants to merge 28 commits into
Open
Conversation
Djhanse2/questionnaire unit tests
Co-authored-by: Darrin Hansen <darrinjhansen@gmail.com>
Co-authored-by: Darrin Hansen <darrinjhansen@gmail.com>
Co-authored-by: Darrin Hansen <darrinjhansen@gmail.com>
Co-authored-by: Darrin Hansen <darrinjhansen@gmail.com>
Co-authored-by: Darrin Hansen <darrinjhansen@gmail.com>
Co-authored-by: Darrin Hansen <darrinjhansen@gmail.com>
Co-authored-by: Darrin Hansen <darrinjhansen@gmail.com>
added unit testing for question advice
…ts_bug fixed bug in course model and added testing for it
Generated by 🚫 Danger |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors instructor and course associations by removing an ChangesModel Associations & Factory Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. 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: 5
🧹 Nitpick comments (5)
spec/models/question_advice_spec.rb (1)
64-68: Useless variable assignment and formatting issues.Line 66 has a useless assignment
options = {}. The value is passed directly and never used. Consider passing{}directly. Additionally, RuboCop flagged multiple formatting issues throughout the file (4-space indentation, trailing whitespace).♻️ Proposed fix
- describe '#test export_fields' do - it 'make sure that the columns of QuestionAdvice are properly being mapped' do - output = QuestionAdvice.export_fields(options = {}) - expect(output).to eq(["id", "question_id", "score", "advice", "created_at", "updated_at"]) - end - end + describe '#test export_fields' do + it 'make sure that the columns of QuestionAdvice are properly being mapped' do + output = QuestionAdvice.export_fields({}) + expect(output).to eq(%w[id question_id score advice created_at updated_at]) end + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/question_advice_spec.rb` around lines 64 - 68, The spec for QuestionAdvice.export_fields includes an unnecessary assignment "options = {}" — change the call to pass an empty hash directly (QuestionAdvice.export_fields({})) and remove the redundant local assignment; also fix RuboCop formatting issues in this file (use 2-space Ruby indentation, remove trailing whitespace, and align block indentation) so the spec conforms to style rules.spec/factories/question_advice.rb (1)
5-9: Fix formatting to follow Ruby conventions.RuboCop flagged several style issues: 4-space indentation instead of 2-space, missing spaces inside braces, extra empty line, and missing final newline.
♻️ Proposed formatting fix
FactoryBot.define do - factory :question_advice do - score {5} - association :item - advice {'default advice'} - end - + factory :question_advice do + score { 5 } + association :item + advice { 'default advice' } + end end +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/factories/question_advice.rb` around lines 5 - 9, Update the factory :question_advice formatting to match Ruby conventions: use 2-space indentation for the block, add spaces inside the attribute blocks so score and advice use braces with spaces (e.g. score { 5 } and advice { 'default advice' }), ensure association :item stays properly indented, remove the extra blank line and add a final newline at EOF; locate these changes around the factory :question_advice definition to apply them.spec/factories.rb (1)
48-55: Consider inheriting from the:userfactory to reduce duplication.The
:instructorfactory duplicates all attribute definitions from the:userfactory. SinceInstructoris a subclass ofUser, you could inherit from the user factory to keep things DRY:♻️ Optional refactor to reduce duplication
- factory :instructor, class: 'Instructor' do - sequence(:name) { |_n| Faker::Name.name.to_s.delete(" \t\r\n").downcase } - sequence(:email) { |_n| Faker::Internet.email.to_s } - password { 'password' } - sequence(:full_name) { |_n| "#{Faker::Name.name}#{Faker::Name.name}".downcase } - role { create(:role, :instructor) } - institution { create(:institution) } - end + factory :instructor, parent: :user, class: 'Instructor' do + role { create(:role, :instructor) } + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/factories.rb` around lines 48 - 55, The :instructor factory duplicates attributes from the :user factory; change it to inherit from the :user factory (e.g., declare factory :instructor, parent: :user, class: 'Instructor') and remove duplicated attribute definitions (name, email, password, full_name) so only instructor-specific overrides remain (keep role { create(:role, :instructor) } and institution { create(:institution) } or any other Instructor-specific traits); ensure the factory still sets class: 'Instructor' so instances are the correct subclass.spec/models/questionnaire_spec.rb (2)
119-122: This example is a no-op.It only installs stubs, so it still passes if
copy_questionnaire_detailsnever runs or stops calling either dependency. I'd either remove it or actually invoke the method and assert the interaction/result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/questionnaire_spec.rb` around lines 119 - 122, The example currently only installs stubs for Questionnaire.find and Item.where without ever exercising copy_questionnaire_details, so it’s a no-op; update the spec to actually call the method under test (copy_questionnaire_details) and then assert the expected interactions or results (for example, expect(Questionnaire).to have_received(:find).with('1') and expect(Item).to have_received(:where).with(questionnaire_id: '1'), or assert the side‑effects/return value of copy_questionnaire_details), or if coverage is unnecessary simply remove the example.
149-157: Usecreate!instead ofinsertfor cleaner, more maintainable test setup.While the
QuestionAdvicemodel currently has no validations or callbacks, usingcreate!is more aligned with how the application actually creates records and will ensure tests remain valid if the model evolves. It also avoids manually managingcreated_atandupdated_attimestamps.Suggested change
- QuestionAdvice.insert( - { - question_id: question1.id, - score: 3, - advice: 'Good rationale', - created_at: Time.zone.now, - updated_at: Time.zone.now - } - ) + QuestionAdvice.create!( + question_id: question1.id, + score: 3, + advice: 'Good rationale' + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/questionnaire_spec.rb` around lines 149 - 157, Replace the direct use of QuestionAdvice.insert with an ActiveRecord creation that mirrors application behavior: call QuestionAdvice.create! with question_id: question1.id, score: 3, and advice: 'Good rationale' (omit manual created_at/updated_at handling) so the test uses model callbacks/validations and will fail loudly if creation is invalid.
🤖 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/course.rb`:
- Around line 22-24: In add_ta, the error message uses #{user&.id} which prints
an empty space when user is nil; change the nil-branch to return a clear message
that does not interpolate user.id (e.g., "The user does not exist" or "User not
found") so the response is readable; update the nil check inside the add_ta
method to use that static message instead of user&.id.
In `@spec/models/course_spec.rb`:
- Around line 113-124: The test exposes a bug in Course#remove_ta where the
query uses the literal symbol course_id: :id instead of the actual course id;
update the method (remove_ta in app/models/course.rb) to build the lookup with
course_id: id (or self.id) so that calls like ta_mappings.find_by(user_id:
user_id, course_id: id) and TaMapping.where(user_id: user_id, course_id: id)
return real records; remove the test mocks so the spec exercises the real code
and then run the tests to verify the mapping is found and role downgrades
correctly.
In `@spec/models/question_advice_spec.rb`:
- Line 3: The describe block currently references the wrong model: replace the
top-level RSpec describe target from Questionnaire to QuestionAdvice so the spec
correctly reads describe QuestionAdvice, type: :model (update any stray
references to Questionnaire within the example group if present) to ensure tests
are organized and reported under the correct model.
- Around line 55-56: The example test description is incorrect: inside the
describe '#advice' block the it string reads "returns the score of
QuestionAdvice" but the assertion actually checks the advice attribute; update
the test description in the example test (the it block under describe '#advice')
to accurately say something like "returns the advice of QuestionAdvice" so the
description matches the behavior being tested.
In `@spec/models/questionnaire_spec.rb`:
- Around line 141-143: The test relies on unordered association access (using
copied_questionnaire.items.first/second) which can be flaky; update the
assertion to be deterministic by either ensuring the copy logic queries Items
ordered by seq (e.g., modify copy_questionnaire_details to use
Item.where(...).order(:seq)) or change the spec to assert contents without order
(e.g., use match_array on copied_questionnaire.items.map(&:txt) or assert
ordered results after calling .order(:seq) on copied_questionnaire.items); refer
to copied_questionnaire.items, copy_questionnaire_details, and Item.where(...)
when making the change.
---
Nitpick comments:
In `@spec/factories.rb`:
- Around line 48-55: The :instructor factory duplicates attributes from the
:user factory; change it to inherit from the :user factory (e.g., declare
factory :instructor, parent: :user, class: 'Instructor') and remove duplicated
attribute definitions (name, email, password, full_name) so only
instructor-specific overrides remain (keep role { create(:role, :instructor) }
and institution { create(:institution) } or any other Instructor-specific
traits); ensure the factory still sets class: 'Instructor' so instances are the
correct subclass.
In `@spec/factories/question_advice.rb`:
- Around line 5-9: Update the factory :question_advice formatting to match Ruby
conventions: use 2-space indentation for the block, add spaces inside the
attribute blocks so score and advice use braces with spaces (e.g. score { 5 }
and advice { 'default advice' }), ensure association :item stays properly
indented, remove the extra blank line and add a final newline at EOF; locate
these changes around the factory :question_advice definition to apply them.
In `@spec/models/question_advice_spec.rb`:
- Around line 64-68: The spec for QuestionAdvice.export_fields includes an
unnecessary assignment "options = {}" — change the call to pass an empty hash
directly (QuestionAdvice.export_fields({})) and remove the redundant local
assignment; also fix RuboCop formatting issues in this file (use 2-space Ruby
indentation, remove trailing whitespace, and align block indentation) so the
spec conforms to style rules.
In `@spec/models/questionnaire_spec.rb`:
- Around line 119-122: The example currently only installs stubs for
Questionnaire.find and Item.where without ever exercising
copy_questionnaire_details, so it’s a no-op; update the spec to actually call
the method under test (copy_questionnaire_details) and then assert the expected
interactions or results (for example, expect(Questionnaire).to
have_received(:find).with('1') and expect(Item).to
have_received(:where).with(questionnaire_id: '1'), or assert the
side‑effects/return value of copy_questionnaire_details), or if coverage is
unnecessary simply remove the example.
- Around line 149-157: Replace the direct use of QuestionAdvice.insert with an
ActiveRecord creation that mirrors application behavior: call
QuestionAdvice.create! with question_id: question1.id, score: 3, and advice:
'Good rationale' (omit manual created_at/updated_at handling) so the test uses
model callbacks/validations and will fail loudly if creation is invalid.
🪄 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: ba08191e-1457-4165-94f3-7a1030159f9a
📒 Files selected for processing (10)
app/models/course.rbapp/models/question_advice.rbdb/schema.rbspec/factories.rbspec/factories/courses.rbspec/factories/question_advice.rbspec/factories/questionnaires.rbspec/models/course_spec.rbspec/models/question_advice_spec.rbspec/models/questionnaire_spec.rb
Comment on lines
141
to
143
| expect(copied_questionnaire.items.count).to eq(2) | ||
| expect(copied_questionnaire.items.first.txt).to eq(question1.txt) | ||
| expect(copied_questionnaire.items.second.txt).to eq(question2.txt) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether Questionnaire#items or Item defines an ordering that makes
# `.first` / `.second` deterministic.
fd -i '^questionnaire\.rb$|^item\.rb$' app/models \
| xargs -r rg -n -C3 'has_many\s+:items|default_scope|order\('Repository: expertiza/reimplementation-back-end
Length of output: 594
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1. Check Item model for default_scope or ordering
echo "=== Checking Item model for default_scope or ordering ==="
cat app/models/item.rb | head -50
echo ""
echo "=== Searching for 'seq' in Item model ==="
rg -n 'seq|default_scope|order' app/models/item.rb -A2 -B1 || echo "No matches found"
# 2. Check the actual test code context (lines 130-150 to understand item creation)
echo ""
echo "=== Test code context ==="
sed -n '130,150p' spec/models/questionnaire_spec.rb
# 3. Check if the questionnaire copy/clone logic specifies any ordering
echo ""
echo "=== Checking Questionnaire model for copy/clone logic ==="
rg -n 'copy|clone|items\s*=' app/models/questionnaire.rb -A3 -B1 || echo "No copy/clone logic found in questionnaire.rb"Repository: expertiza/reimplementation-back-end
Length of output: 206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Item-related model files
echo "=== Searching for Item model files ==="
fd -i 'item' app/models --type f
echo ""
echo "=== Searching for classes named Item ==="
rg 'class\s+Item' app/models -nRepository: expertiza/reimplementation-back-end
Length of output: 335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1. Check Item model for default_scope or ordering
echo "=== Item model (first 60 lines) ==="
head -60 app/models/Item.rb
echo ""
echo "=== Searching for default_scope or order in Item model ==="
rg -n 'default_scope|order|seq' app/models/Item.rb -A2 -B1 || echo "No matches found"
# 2. Check the actual test code context (lines 130-150)
echo ""
echo "=== Test code context (lines 130-150) ==="
sed -n '130,150p' spec/models/questionnaire_spec.rb
# 3. Check for copy/clone logic in Questionnaire
echo ""
echo "=== Checking for copy/clone logic in Questionnaire ==="
rg -n 'def.*copy|def.*clone|\.items\s*=' app/models/Questionnaire.rb -A5 || echo "No copy/clone logic found"Repository: expertiza/reimplementation-back-end
Length of output: 3989
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Questionnaire model with correct casing
echo "=== Looking for Questionnaire model ==="
fd 'questionnaire' app/models --type f -i
echo ""
echo "=== Checking Questionnaire model for copy logic ==="
cat app/models/questionnaire.rb | head -100Repository: expertiza/reimplementation-back-end
Length of output: 4505
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how question1 and question2 are created in the test
echo "=== Test setup (searching for question1 and question2 definition) ==="
rg -n 'question1|question2' spec/models/questionnaire_spec.rb -B5 | head -50
echo ""
echo "=== Verifying Item.where call returns unordered results ==="
grep -n 'Item.where' app/models/questionnaire.rb -A2 -B2Repository: expertiza/reimplementation-back-end
Length of output: 2669
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the complete definition of question2
echo "=== Complete question2 definition ==="
sed -n '27,29p' spec/models/questionnaire_spec.rb
echo ""
echo "=== Verify that Item.where has no order in copy_questionnaire_details ==="
sed -n '42,50p' app/models/questionnaire.rbRepository: expertiza/reimplementation-back-end
Length of output: 752
Avoid relying on .first / .second for item ordering.
The items association has no explicit order: clause, and Item.where(questionnaire_id: ...) in copy_questionnaire_details is unordered. This can flap across database adapters. Use .order(:seq) or match_array to make the assertion deterministic.
Suggested change
- expect(copied_questionnaire.items.first.txt).to eq(question1.txt)
- expect(copied_questionnaire.items.second.txt).to eq(question2.txt)
+ expect(copied_questionnaire.items.order(:seq).pluck(:txt))
+ .to eq([question1.txt, question2.txt])📝 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
| expect(copied_questionnaire.items.count).to eq(2) | |
| expect(copied_questionnaire.items.first.txt).to eq(question1.txt) | |
| expect(copied_questionnaire.items.second.txt).to eq(question2.txt) | |
| expect(copied_questionnaire.items.count).to eq(2) | |
| expect(copied_questionnaire.items.order(:seq).pluck(:txt)) | |
| .to eq([question1.txt, question2.txt]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/models/questionnaire_spec.rb` around lines 141 - 143, The test relies on
unordered association access (using copied_questionnaire.items.first/second)
which can be flaky; update the assertion to be deterministic by either ensuring
the copy logic queries Items ordered by seq (e.g., modify
copy_questionnaire_details to use Item.where(...).order(:seq)) or change the
spec to assert contents without order (e.g., use match_array on
copied_questionnaire.items.map(&:txt) or assert ordered results after calling
.order(:seq) on copied_questionnaire.items); refer to
copied_questionnaire.items, copy_questionnaire_details, and Item.where(...) when
making the change.
Few line changes regarding to a few minor bugs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/models/course.rb (1)
22-24:⚠️ Potential issue | 🟡 MinorUse a static nil-user error message in
add_ta.Line 24 still interpolates
user&.id, which yields an awkward empty ID whenuseris nil. Also, RuboCop is right that thisreturnis redundant.💡 Suggested adjustment
def add_ta(user) if user.nil? - return { success: false, message: "The user with id #{user&.id} does not exist" } + { success: false, message: 'User does not exist' } elsif TaMapping.exists?(user_id: user.id, course_id: id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/course.rb` around lines 22 - 24, The add_ta method currently interpolates user&.id in the nil-user error message which produces an empty/awkward ID and uses a redundant return; change the error branch in add_ta to return a static message like "The user does not exist" (or similar fixed string) instead of "#{user&.id}" and remove the unnecessary explicit return so the method simply yields the hash { success: false, message: "The user does not exist" } when user is nil.spec/models/question_advice_spec.rb (1)
55-57:⚠️ Potential issue | 🟡 MinorFix the example description in
#adviceblock.The spec description says it returns the score, but the assertions verify
advice.🛠️ Proposed fix
describe '#advice' do - it 'returns the score of QuestionAdvice' do + it 'returns the advice of QuestionAdvice' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/question_advice_spec.rb` around lines 55 - 57, The example description in the describe '#advice' block is wrong: update the it description string from saying it "returns the score of QuestionAdvice" to reflect the assertion (e.g., "returns the advice of QuestionAdvice" or "returns the advice"); edit the it block containing question_advice1.advice to use the corrected human-readable description so the spec text matches the expectation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/models/question_advice_spec.rb`:
- Line 66: The test unnecessarily assigns options = {} when calling
QuestionAdvice.export_fields; remove the useless assignment and call the method
with a literal hash instead by replacing "output =
QuestionAdvice.export_fields(options = {})" with "output =
QuestionAdvice.export_fields({})" (target the usage of export_fields in the spec
to make this change).
---
Duplicate comments:
In `@app/models/course.rb`:
- Around line 22-24: The add_ta method currently interpolates user&.id in the
nil-user error message which produces an empty/awkward ID and uses a redundant
return; change the error branch in add_ta to return a static message like "The
user does not exist" (or similar fixed string) instead of "#{user&.id}" and
remove the unnecessary explicit return so the method simply yields the hash {
success: false, message: "The user does not exist" } when user is nil.
In `@spec/models/question_advice_spec.rb`:
- Around line 55-57: The example description in the describe '#advice' block is
wrong: update the it description string from saying it "returns the score of
QuestionAdvice" to reflect the assertion (e.g., "returns the advice of
QuestionAdvice" or "returns the advice"); edit the it block containing
question_advice1.advice to use the corrected human-readable description so the
spec text matches the expectation.
🪄 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: afafb2b3-9ec2-4a3d-addb-1eaff2ba8156
📒 Files selected for processing (2)
app/models/course.rbspec/models/question_advice_spec.rb
|
|
||
| describe '#test export_fields' do | ||
| it 'make sure that the columns of QuestionAdvice are properly being mapped' do | ||
| output = QuestionAdvice.export_fields(options = {}) |
There was a problem hiding this comment.
Remove useless assignment in export_fields test call.
options = {} is assigned but never used after the call; pass the hash directly.
🛠️ Proposed fix
- output = QuestionAdvice.export_fields(options = {})
+ output = QuestionAdvice.export_fields({})📝 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
| output = QuestionAdvice.export_fields(options = {}) | |
| output = QuestionAdvice.export_fields({}) |
🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 66-66: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[warning] 66-66: Useless assignment to variable - options.
(Lint/UselessAssignment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/models/question_advice_spec.rb` at line 66, The test unnecessarily
assigns options = {} when calling QuestionAdvice.export_fields; remove the
useless assignment and call the method with a literal hash instead by replacing
"output = QuestionAdvice.export_fields(options = {})" with "output =
QuestionAdvice.export_fields({})" (target the usage of export_fields in the spec
to make this change).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 9-40: Add Ruby language identifiers to the three fenced code
blocks that contain the SimpleCov snippets: the block with require
'simplecov-html', the block setting SimpleCov.formatter =
SimpleCov::Formatter::MultiFormatter.new([...]) and the block starting with if
!ENV['COVERAGE_STARTED'] (the SimpleCov.start 'rails' ... end block); update
each opening fence from ``` to ```ruby so markdownlint MD040 warnings are
resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 44-46: The fenced code block containing the RSpec command (```
bundle exec rspec spec/models/questionnaire_spec.rb spec/models/course_spec.rb
spec/models/question_advice_spec.rb ```) needs a shell language identifier to
satisfy MD040; update the opening fence to use ```shell (or ```bash) so the
block becomes a shell code fence and preserves the same command content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Just a small test description with QuestionAdvice fix
There was a problem hiding this comment.
♻️ Duplicate comments (1)
spec/models/question_advice_spec.rb (1)
66-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the useless assignment in
export_fieldscall.At Line 66,
options = {}is assigned only as a method argument and never used, which adds noise to the test.🛠️ Proposed fix
- output = QuestionAdvice.export_fields(options = {}) + output = QuestionAdvice.export_fields({})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/models/question_advice_spec.rb` at line 66, The test calls QuestionAdvice.export_fields with a redundant local assignment "options = {}"; remove the useless assignment and call the method directly as QuestionAdvice.export_fields({}) or QuestionAdvice.export_fields depending on desired default behavior, updating the spec line where output is set (the reference is the call to QuestionAdvice.export_fields in spec/models/question_advice_spec.rb).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@spec/models/question_advice_spec.rb`:
- Line 66: The test calls QuestionAdvice.export_fields with a redundant local
assignment "options = {}"; remove the useless assignment and call the method
directly as QuestionAdvice.export_fields({}) or QuestionAdvice.export_fields
depending on desired default behavior, updating the spec line where output is
set (the reference is the call to QuestionAdvice.export_fields in
spec/models/question_advice_spec.rb).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 186b2546-b9c3-4727-96ae-a39998a5baf8
📒 Files selected for processing (1)
spec/models/question_advice_spec.rb
Fixed an error message in Course.rb
…ctory Removed duplicate Instructor factory
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
This PR adds comprehensive RSpec unit tests and bug fixes for three core models: Questionnaire, QuestionAdvice, and Course. The goal is to improve reliability, validate expected behavior, and catch edge cases in the peer review workflow.
Changes
Model Coverage
Questionnaire
copy) including associated questions and adviceQuestionAdvice
Course
TaMapping(including role updates)copy_courseWhy This Matters
These models are central to assignment reviews and course management. Strengthening test coverage and fixing bugs improves system stability and reduces the risk of regressions.
Testing
bundle exec rspecNotes
Wiki Page: https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2026_-_E2617._Testing_Questionnaire_and_Course_Models#Key_Relationships
Summary by CodeRabbit
Bug Fixes
Tests