Added json-output for the questions endpoint #5891
Conversation
Bumps [typescript](https://github.com/microsoft/TypeScript) from 5.5.4 to 5.6.3. - [Release notes](https://github.com/microsoft/TypeScript/releases) - [Changelog](https://github.com/microsoft/TypeScript/blob/main/azure-pipelines.release.yml) - [Commits](microsoft/TypeScript@v5.5.4...v5.6.3) --- updated-dependencies: - dependency-name: typescript dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
…script-5.6.3 Bump typescript from 5.5.4 to 5.6.3
… into fix/questions-json-output-empty
|
Warning Rate limit exceeded@thvmulle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 28 seconds before requesting another review. ⌛ 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. WalkthroughThe changes in this pull request involve modifications to the JSON output structure in the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
jest.config.js (1)
10-21: Add configuration documentation for better maintainability.The transform configuration would benefit from comments explaining the purpose of each transformer and its settings.
Consider adding documentation:
transform: { + // Handle TypeScript files with ts-jest '\\.tsx?$': [ 'ts-jest', { tsconfig: { "allowJs": true, "target": "es6", }, } ], + // Transform JavaScript files to CommonJS format for Jest compatibility '\\.jsx?$': ['babel-jest', {plugins: ['@babel/plugin-transform-modules-commonjs']}] },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
- app/views/courses/questions.json.jbuilder (1 hunks)
- jest.config.js (1 hunks)
- package.json (3 hunks)
- test/controllers/courses_controller_test.rb (1 hunks)
🔇 Additional comments (6)
app/views/courses/questions.json.jbuilder (2)
2-2: Verify the annotation partial's existence and structure.The template relies on 'annotations/annotation' partial. Let's ensure it exists and contains the necessary attributes.
#!/bin/bash # Check if the annotation partial exists and examine its structure fd annotation.json.jbuilder app/views/Also applies to: 6-6, 10-10
1-11: LGTM! The JSON structure aligns well with the PR objectives.The categorization of questions into unanswered, in_progress, and answered states provides a clear and organized response structure.
Let's verify how the instance variables are loaded to prevent N+1 queries:
✅ Verification successful
Let me try a different search strategy to locate the controller action and verify the instance variables.
This will help us:
- Find controllers that might handle questions/annotations
- Verify the routing configuration
- Understand what data is being rendered in the partial
Let me check the annotations controller since the routes show that questions are handled by
annotations#question_index.
Let me check the Question model to understand the states and how they relate to the JSON view's categorization.
Based on the comprehensive review of the codebase, I can now provide a final response:
The JSON structure in questions.json.jbuilder is correctly implemented and properly handles the instance variables.
The code is well-structured and safe because:
- The instance variables (@Unanswered, @in_progress, @answered) are properly set in CoursesController#questions action with appropriate eager loading:
@unanswered = @course.unanswered_questions .reorder(created_at: :asc) .includes(:user, :last_updated_by, submission: [:exercise])- The Question model properly defines the states using an enum:
enum :question_state, { unanswered: 0, in_progress: 1, answered: 2 }- The partial '_annotation.json.jbuilder' handles nil cases and includes proper authorization checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the controller action that sets up these instance variables rg -A 10 "def.*questions" app/controllers/Length of output: 232
Script:
#!/bin/bash # Search for controllers that might handle questions fd -e rb . app/controllers/ | xargs rg -l "questions|annotations" # Also check the routes to identify the controller rg "questions" config/routes.rb # Look for the partial to understand the annotation structure cat app/views/annotations/_annotation.json.jbuilderLength of output: 2362
Script:
#!/bin/bash # Check the annotations controller for the question_index action cat app/controllers/annotations_controller.rb # Also look for any Question model definitions to understand the states rg -g "*.rb" "class Question" app/models/Length of output: 3647
Script:
#!/bin/bash # Check the Question model for state definitions cat app/models/question.rb # Also check the courses controller since our view is under courses/ cat app/controllers/courses_controller.rbLength of output: 22167
package.json (2)
74-75: LGTM! Test dependencies added correctly.The addition of babel-jest and babel-plugin-dynamic-import-node is appropriate for improving the test infrastructure. The versions are properly pinned using the caret (^) operator.
60-60: Verify TypeScript upgrade compatibility.The TypeScript version upgrade from 5.5.4 to 5.6.3 might introduce breaking changes.
Run the following script to check for potential compatibility issues:
✅ Verification successful
Let me analyze the results and generate additional verification scripts to check for potential breaking changes in TypeScript 5.6.
Based on the comprehensive analysis of the TypeScript usage in the codebase, I can now provide a final response:
TypeScript 5.6.3 upgrade appears safe to proceed
The verification results show:
- No usage of new TypeScript 5.6 features like const type parameters
- No usage of the
satisfiesoperator that had behavior changes- No complex array spread patterns that could be affected
- The
tsconfig.jsonsettings are compatible with TypeScript 5.6- The codebase primarily uses standard TypeScript features that are stable across 5.x versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for TypeScript breaking changes and type errors # Test: Search for TypeScript files and verify no type errors exist echo "Searching for TypeScript files..." fd -e ts -e tsx --type f --exec echo "Found TypeScript file: {}" # Test: Look for potential type assertion patterns that might be affected echo "Checking for potentially affected type patterns..." rg -l "as \w+|<\w+>" --type tsLength of output: 15456
Script:
#!/bin/bash # Check for TypeScript features that might be affected by the upgrade echo "Checking for potentially affected TypeScript patterns..." # Check for const type parameters (new in TS 5.6) echo "Checking for const type parameters..." rg "type\s+\w+\s*<\s*const\s+\w+>" --type ts # Check for usage of satisfies operator (behavior changes in 5.6) echo "Checking for 'satisfies' operator usage..." rg "satisfies\s+" --type ts # Check for array spread patterns (improved in 5.6) echo "Checking for array spread patterns..." rg "\.\.\.[^\)]*as\s+const" --type ts # Check tsconfig.json for compiler options that might be affected echo "Checking TypeScript configuration..." cat tsconfig.json 2>/dev/null || echo "No tsconfig.json found"Length of output: 1177
test/controllers/courses_controller_test.rb (2)
923-947: LGTM! Comprehensive test coverage for JSON response structure.The test thoroughly validates:
- Presence of all required keys (unanswered, in_progress, answered)
- Correct count of questions in each state
- Proper handling of delayed jobs for maintaining question states
908-921: LGTM! Well-structured tests for basic functionality and edge cases.These tests provide good coverage of:
- Basic access control for super admins
- JSON format validation
- Edge case handling for empty question lists
- Proper response structure consistency
Also applies to: 949-963
| json.unanswered do | ||
| json.array! @unanswered, partial: 'annotations/annotation', as: :annotation | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding nil checks and documentation.
- Add nil checks to handle cases where instance variables might not be initialized:
json.unanswered do
- json.array! @unanswered, partial: 'annotations/annotation', as: :annotation
+ json.array! @unanswered || [], partial: 'annotations/annotation', as: :annotation
end
json.in_progress do
- json.array! @in_progress, partial: 'annotations/annotation', as: :annotation
+ json.array! @in_progress || [], partial: 'annotations/annotation', as: :annotation
end
json.answered do
- json.array! @answered, partial: 'annotations/annotation', as: :annotation
+ json.array! @answered || [], partial: 'annotations/annotation', as: :annotation
end- Consider adding a comment documenting the expected JSON structure and the annotation partial's attributes.
# Expected JSON structure:
# {
# "unanswered": [/* array of annotation objects */],
# "in_progress": [/* array of annotation objects */],
# "answered": [/* array of annotation objects */]
# }Also applies to: 5-7, 9-11
| transform: { | ||
| '\\.tsx?$': [ | ||
| 'ts-jest', | ||
| { | ||
| tsconfig: { | ||
| "allowJs": true, | ||
| "target": "es6", | ||
| }, | ||
| } | ||
| ], | ||
| '\\.jsx?$': ['babel-jest', {plugins: ['@babel/plugin-transform-modules-commonjs']}] | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the TypeScript configuration.
While the basic transform configuration is correct, the TypeScript settings could be more comprehensive for better type safety and debugging capabilities.
Consider applying these improvements:
transform: {
'\\.tsx?$': [
'ts-jest',
{
tsconfig: {
"allowJs": true,
"target": "es6",
+ "strict": true,
+ "sourceMap": true,
+ "esModuleInterop": true,
+ "moduleResolution": "node"
},
}
],
'\\.jsx?$': ['babel-jest', {plugins: ['@babel/plugin-transform-modules-commonjs']}]
},📝 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.
| transform: { | |
| '\\.tsx?$': [ | |
| 'ts-jest', | |
| { | |
| tsconfig: { | |
| "allowJs": true, | |
| "target": "es6", | |
| }, | |
| } | |
| ], | |
| '\\.jsx?$': ['babel-jest', {plugins: ['@babel/plugin-transform-modules-commonjs']}] | |
| }, | |
| transform: { | |
| '\\.tsx?$': [ | |
| 'ts-jest', | |
| { | |
| tsconfig: { | |
| "allowJs": true, | |
| "target": "es6", | |
| "strict": true, | |
| "sourceMap": true, | |
| "esModuleInterop": true, | |
| "moduleResolution": "node" | |
| }, | |
| } | |
| ], | |
| '\\.jsx?$': ['babel-jest', {plugins: ['@babel/plugin-transform-modules-commonjs']}] | |
| }, |
| @@ -88,5 +90,6 @@ | |||
| "ts-jest": "^26.5.6", | |||
| "typescript-eslint": "^8.10.0", | |||
| "webpack-bundle-analyzer": "^4.10.2" | |||
| } | |||
| }, | |||
| "packageManager": "[email protected]+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider upgrading to Yarn 2+.
While explicitly defining the package manager version is good practice, Yarn 1.x is in maintenance mode. Consider upgrading to Yarn 2+ (Berry) for improved performance and features.
To upgrade, you would need to:
- Remove the current packageManager field
- Run
yarn set version berry - Configure .yarnrc.yml for your needs
| test 'non admins cannot view questions in JSON format' do | ||
| add_not_admins | ||
| with_users_signed_in @not_admins do |who| | ||
| get questions_course_path(@course), as: :json | ||
|
|
||
| assert :ok, "#{who} should not be able to view questions in JSON format" | ||
| end | ||
| end |
There was a problem hiding this comment.
Fix incorrect assertion in non-admin access test.
The test assertion is incorrect as it's checking for :ok status when non-admins should be denied access.
Apply this diff to fix the assertion:
- assert :ok, "#{who} should not be able to view questions in JSON format"
+ assert_response :forbidden, "#{who} should not be able to view questions in JSON format"📝 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.
| test 'non admins cannot view questions in JSON format' do | |
| add_not_admins | |
| with_users_signed_in @not_admins do |who| | |
| get questions_course_path(@course), as: :json | |
| assert :ok, "#{who} should not be able to view questions in JSON format" | |
| end | |
| end | |
| test 'non admins cannot view questions in JSON format' do | |
| add_not_admins | |
| with_users_signed_in @not_admins do |who| | |
| get questions_course_path(@course), as: :json | |
| assert_response :forbidden, "#{who} should not be able to view questions in JSON format" | |
| end | |
| end |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
test/controllers/courses_controller_test.rb (1)
926-950: Consider moving delayed job cleanup to teardown.The test correctly verifies the JSON response structure and content. However, the
run_delayed_jobscall on line 948 should be moved to a teardown block to ensure cleanup happens even if the test fails.test 'super admins get correct question info in JSON format' do add_admins super_admins = @admins.reject(&:student?) + teardown do + run_delayed_jobs + end with_users_signed_in super_admins do |_who| # without delayed jobs, in progress is automatically reset to unanswered with_delayed_jobs do submission = create :submission, course: @course create :question, question_state: :answered, submission: submission create :question, question_state: :unanswered, submission: submission create :question, question_state: :in_progress, submission: submission get questions_course_path(@course), as: :json json_response = response.parsed_body assert json_response.key?('unanswered'), "The 'unanswered' key should be present in the JSON response" assert json_response.key?('in_progress'), "The 'in_progress' key should be present in the JSON response" assert json_response.key?('answered'), "The 'answered' key should be present in the JSON response" assert_equal 1, json_response['unanswered'].size, 'There should be 1 unanswered question in the JSON response' assert_equal 1, json_response['in_progress'].size, 'There should be 1 in_progress question in the JSON response' assert_equal 1, json_response['answered'].size, 'There should be 1 answered question in the JSON response' end - run_delayed_jobs end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- test/controllers/courses_controller_test.rb (1 hunks)
🔇 Additional comments (3)
test/controllers/courses_controller_test.rb (3)
908-924: LGTM! Well-structured test for JSON format access.The test properly verifies that super admins can access the questions endpoint in JSON format, with good setup of test data and proper use of
with_delayed_jobs.
952-966: LGTM! Good coverage of empty state.The test properly verifies that super admins can view empty question lists and checks for the presence of all required keys with empty arrays.
968-979: LGTM! Comprehensive authorization test.The test properly verifies that non-admins cannot access the questions endpoint in JSON format, with good coverage of both unauthorized (not signed in) and forbidden (signed in but not admin) cases.
| get questions_course_path(@course), as: :json | ||
|
|
||
| if who == 'not signed in' | ||
| assert_response :unauthorized, "#{who} should not be able to view questions in JSON format" |
There was a problem hiding this comment.
I am still slightly worried about the fact that this test also succeed when this line was assert_response :ok
Do you have any idea why? (if no, I'll do some further research as this might cause false positives in other tests)
There was a problem hiding this comment.
already found the issues. Two tests in this file use assert :ok instead of assert_response :ok (where you probably copy pasted from)
Although unrelated to this pr, could you also fix them?
There was a problem hiding this comment.
Sure no problem, I'll fix them right away
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
test/controllers/courses_controller_test.rb (1)
908-924: LGTM! Consider isolating test data setup.The test correctly verifies super admin access to the questions endpoint in JSON format. Good use of
with_delayed_jobsto handle in-progress questions.Consider moving the question creation logic to a helper method since it's reused across multiple tests:
def create_test_questions(submission) create :question, question_state: :answered, submission: submission create :question, question_state: :unanswered, submission: submission create :question, question_state: :in_progress, submission: submission end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- test/controllers/courses_controller_test.rb (1 hunks)
🔇 Additional comments (2)
test/controllers/courses_controller_test.rb (2)
952-966: LGTM! Good edge case coverage.The test effectively verifies the JSON structure when no questions exist, ensuring the endpoint handles empty states correctly.
968-975: LGTM! Good access control verification.The test correctly verifies that non-admins cannot access the questions endpoint in JSON format, using the preferred assertion style for checking forbidden/unauthorized responses.
| test 'super admins get correct question info in JSON format' do | ||
| add_admins | ||
| super_admins = @admins.reject(&:student?) | ||
| with_users_signed_in super_admins do |_who| | ||
| # without delayed jobs, in progress is automatically reset to unanswered | ||
| with_delayed_jobs do | ||
| submission = create :submission, course: @course | ||
| create :question, question_state: :answered, submission: submission | ||
| create :question, question_state: :unanswered, submission: submission | ||
| create :question, question_state: :in_progress, submission: submission | ||
| get questions_course_path(@course), as: :json | ||
|
|
||
| json_response = response.parsed_body | ||
|
|
||
| assert json_response.key?('unanswered'), "The 'unanswered' key should be present in the JSON response" | ||
| assert json_response.key?('in_progress'), "The 'in_progress' key should be present in the JSON response" | ||
| assert json_response.key?('answered'), "The 'answered' key should be present in the JSON response" | ||
|
|
||
| assert_equal 1, json_response['unanswered'].size, 'There should be 1 unanswered question in the JSON response' | ||
| assert_equal 1, json_response['in_progress'].size, 'There should be 1 in_progress question in the JSON response' | ||
| assert_equal 1, json_response['answered'].size, 'There should be 1 answered question in the JSON response' | ||
| end | ||
| run_delayed_jobs | ||
| end | ||
| end |
There was a problem hiding this comment.
Remove redundant run_delayed_jobs call.
The run_delayed_jobs call on line 948 is unnecessary since the with_delayed_jobs block on line 931 already handles the delayed job execution.
Apply this diff to remove the redundant call:
end
- run_delayed_jobs
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.
| test 'super admins get correct question info in JSON format' do | |
| add_admins | |
| super_admins = @admins.reject(&:student?) | |
| with_users_signed_in super_admins do |_who| | |
| # without delayed jobs, in progress is automatically reset to unanswered | |
| with_delayed_jobs do | |
| submission = create :submission, course: @course | |
| create :question, question_state: :answered, submission: submission | |
| create :question, question_state: :unanswered, submission: submission | |
| create :question, question_state: :in_progress, submission: submission | |
| get questions_course_path(@course), as: :json | |
| json_response = response.parsed_body | |
| assert json_response.key?('unanswered'), "The 'unanswered' key should be present in the JSON response" | |
| assert json_response.key?('in_progress'), "The 'in_progress' key should be present in the JSON response" | |
| assert json_response.key?('answered'), "The 'answered' key should be present in the JSON response" | |
| assert_equal 1, json_response['unanswered'].size, 'There should be 1 unanswered question in the JSON response' | |
| assert_equal 1, json_response['in_progress'].size, 'There should be 1 in_progress question in the JSON response' | |
| assert_equal 1, json_response['answered'].size, 'There should be 1 answered question in the JSON response' | |
| end | |
| run_delayed_jobs | |
| end | |
| end | |
| test 'super admins get correct question info in JSON format' do | |
| add_admins | |
| super_admins = @admins.reject(&:student?) | |
| with_users_signed_in super_admins do |_who| | |
| # without delayed jobs, in progress is automatically reset to unanswered | |
| with_delayed_jobs do | |
| submission = create :submission, course: @course | |
| create :question, question_state: :answered, submission: submission | |
| create :question, question_state: :unanswered, submission: submission | |
| create :question, question_state: :in_progress, submission: submission | |
| get questions_course_path(@course), as: :json | |
| json_response = response.parsed_body | |
| assert json_response.key?('unanswered'), "The 'unanswered' key should be present in the JSON response" | |
| assert json_response.key?('in_progress'), "The 'in_progress' key should be present in the JSON response" | |
| assert json_response.key?('answered'), "The 'answered' key should be present in the JSON response" | |
| assert_equal 1, json_response['unanswered'].size, 'There should be 1 unanswered question in the JSON response' | |
| assert_equal 1, json_response['in_progress'].size, 'There should be 1 in_progress question in the JSON response' | |
| assert_equal 1, json_response['answered'].size, 'There should be 1 answered question in the JSON response' | |
| end | |
| end | |
| end |
This pull request fixes the empty list output generated by the JSON response for
https://dodona.be/nl/courses/<course ID>/questions.json.The JSON now returns an object that has three properties:
{ "unanswered": [...], "in_progress": [...], "answered": [...] }Each of these properties contains a list with questions that are either unanswered, being answered or have been answered.
Closes #5890 .