Fixed faulty nil checks #2139
Fixed faulty nil checks #2139
Conversation
WalkthroughWalkthroughThis update addresses a common issue across several controllers and a script within the application, focusing on enhancing error handling and data retrieval methods. The changes involve modifying how the application searches for and verifies the existence of database records, likely to improve reliability and user feedback when records are not found. Changes
Possibly related 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
coder6583
requested review from
a team and
20wildmanj
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @problem = @assessment.problems.find_by(params[:problem]) | ||
| if @problem.nil? | ||
| flash[:error] = "Could not find problem #{params[:problem]}" | ||
| end | ||
| @submission = @assessment.submissions.find_by(params[:submission]) | ||
| if @submission.nil? | ||
| flash[:error] = "Could not find submission #{params[:submission]}" | ||
| end |
There was a problem hiding this comment.
Using find_by improves nil handling, but the way params are passed to it might not work as expected. find_by expects key-value pairs, but params[:problem] and params[:submission] are likely to be single values (IDs). Consider specifying the column names explicitly, for example, find_by(id: params[:problem]). This ensures clarity and prevents potential bugs if the parameters structure changes.
- @problem = @assessment.problems.find_by(params[:problem])
+ @problem = @assessment.problems.find_by(id: params[:problem])
- @submission = @assessment.submissions.find_by(params[:submission])
+ @submission = @assessment.submissions.find_by(id: params[:submission])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.
| @problem = @assessment.problems.find_by(params[:problem]) | |
| if @problem.nil? | |
| flash[:error] = "Could not find problem #{params[:problem]}" | |
| end | |
| @submission = @assessment.submissions.find_by(params[:submission]) | |
| if @submission.nil? | |
| flash[:error] = "Could not find submission #{params[:submission]}" | |
| end | |
| @problem = @assessment.problems.find_by(id: params[:problem]) | |
| if @problem.nil? | |
| flash[:error] = "Could not find problem #{params[:problem]}" | |
| end | |
| @submission = @assessment.submissions.find_by(id: params[:submission]) | |
| if @submission.nil? | |
| flash[:error] = "Could not find submission #{params[:submission]}" | |
| end |
| flash[:error] = "Could not find problem #{params[:problem]}" | ||
| end | ||
| @submission = @assessment.submissions.find_by(params[:submission]) | ||
| if @submission.nil? | ||
| flash[:error] = "Could not find submission #{params[:submission]}" | ||
| end |
There was a problem hiding this comment.
Setting error messages in the flash is a good practice when handling nil values. However, after setting the error message, consider redirecting the user to a relevant page (e.g., the assessments index or a custom error page). This prevents the execution of further code that assumes @problem and @submission are not nil and enhances the user experience by not leaving them on a potentially broken page.
if @problem.nil?
flash[:error] = "Could not find problem #{params[:problem]}"
redirect_to assessments_path and return
end
if @submission.nil?
flash[:error] = "Could not find submission #{params[:submission]}"
redirect_to assessments_path and return
end
| @@ -174,7 +176,7 @@ def update | |||
|
|
|||
| action_auth_level :destroy, :instructor | |||
| def destroy | |||
There was a problem hiding this comment.
destroy / destroyConfirm don't actually seem to be routes that are currently used, might be worth removing.
| @@ -3,7 +3,7 @@ | |||
| require File.expand_path(File.dirname(__FILE__)+'/../config/environment') | |||
|
|
|||
| course_id = ARGV[0] | |||
| @course = Course.find(course_id) | |||
There was a problem hiding this comment.
this script is unused, should delete
There was a problem hiding this comment.
Can I delete the whole file?
Description
.findinstead of.find_byMotivation and Context
It prevents the website from responding with ruby errors, and redirects users to valid website and shows them their error.
Issue #2128
How Has This Been Tested?
grading.rb/courses/:course_name/assessments/:assessment_name/score_grader_info/:idwith an invalid id and made sure it redirects the user back to the assessment pagecourse_user_data_controller.rb/course/:course_name/course_user_data/:idwith a valid id and made sure that it showed the user/course/:course_name/course_user_data/:idwith an invalid id and made sure that it went back to the user list with the correct errorcourses_controller.rbgroups_controller.rbusers_controller.rb/usersand checked there are users/users/:idwith valid user id and checked that it shows the user/users/:idwith an invalid user id and check that it goes back to /usersschedulers_controller.rb,lti_nrps_controller.rb,updateGradebook.rbTypes of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for lintingOther issues / help required
I need some help on testing
schedulers_controller.rb,lti_nrps_controller.rb,updateGradebook.rb.