Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added json-output for the questions endpoint #5891

Merged
merged 14 commits into from
Oct 28, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion app/views/courses/questions.json.jbuilder
Original file line number Diff line number Diff line change
@@ -1 +1,11 @@
json.array! @questions
json.unanswered do
json.array! @unanswered, partial: 'annotations/annotation', as: :annotation
end
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding nil checks and documentation.

  1. 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
  1. 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


json.in_progress do
json.array! @in_progress, partial: 'annotations/annotation', as: :annotation
end

json.answered do
json.array! @answered, partial: 'annotations/annotation', as: :annotation
end
20 changes: 12 additions & 8 deletions jest.config.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,24 @@
module.exports = {
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
preset: "ts-jest/presets/js-with-ts",
reporters: ["default", "jest-junit"],
globals: {
"ts-jest": {
tsconfig: {
"allowJs": true,
"target": "es6",
},
},
},
setupFiles: ["<rootDir>/test/javascript/setup-jest.ts"],
roots: ["test/javascript/", "app/assets/"],
moduleDirectories: [
"node_modules",
"app/assets/javascripts"
],
transform: {
'\\.tsx?$': [
'ts-jest',
{
tsconfig: {
"allowJs": true,
"target": "es6",
},
}
],
'\\.jsx?$': ['babel-jest', {plugins: ['@babel/plugin-transform-modules-commonjs']}]
},
Comment on lines +10 to +21
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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']}]
},

transformIgnorePatterns: ["node_modules/?!(d3)"],
collectCoverageFrom: ["app/assets/javascripts/**/*.{js,ts}"],
};
7 changes: 5 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
"sass": "^1.80.3",
"tippy.js": "^6.3.7",
"ts-loader": "^9.5.1",
"typescript": "^5.5.4",
"typescript": "^5.6.3",
"webpack": "^5.95.0",
"webpack-cli": "^5.1.4"
},
Expand All @@ -71,6 +71,8 @@
"@typescript-eslint/eslint-plugin": "^5.62.0",
"@typescript-eslint/parser": "^5.62.0",
"@webcomponents/scoped-custom-element-registry": "^0.0.9",
"babel-jest": "^29.7.0",
"babel-plugin-dynamic-import-node": "^2.3.3",
"babel-plugin-istanbul": "^7.0.0",
"eslint": "^9.13.0",
"eslint-config-google": "^0.14.0",
Expand All @@ -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"
Copy link

Choose a reason for hiding this comment

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

🛠️ 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:

  1. Remove the current packageManager field
  2. Run yarn set version berry
  3. Configure .yarnrc.yml for your needs

}
69 changes: 69 additions & 0 deletions test/controllers/courses_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,75 @@ def with_users_signed_in(users)
end
end

test 'super admins are able to view questions in JSON format' do
add_admins
super_admins = @admins.reject(&:student?)
with_users_signed_in super_admins do |who|
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
# without delayed jobs, in progress is automatically reset to unanswered
with_delayed_jobs do
# Create some questions so we actually render something
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

assert_response :ok, "#{who} should be able to view questions in JSON format"
end
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
run_delayed_jobs
end
end
Comment on lines +926 to +950
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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


test 'super admins can view empty question lists' do
add_admins
super_admins = @admins.reject(&:student?)
with_users_signed_in super_admins do |_who|
get questions_course_path(@course), as: :json

assert_response :ok

json_response = response.parsed_body

assert_empty json_response['unanswered'], 'There should be 0 unanswered questions in the JSON response'
assert_empty json_response['in_progress'], 'There should be 0 in_progress questions in the JSON response'
assert_empty json_response['answered'], 'There should be 0 answered questions in the JSON response'
end
end

test 'non admins cannot view questions in JSON format' do
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
add_not_admins
with_users_signed_in @not_admins do |who|
get questions_course_path(@course), as: :json

assert (response.forbidden? || response.unauthorized?), "#{who} should not be able to view questions in JSON format"
end
end

test 'Icalendar link exports valid and correct ics file' do
time1 = DateTime.now
time2 = DateTime.now + 1.day + 1.hour + 1.second
Expand Down
Loading