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.

Sign up for GitHub

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

Jump to bottom

Fixed faulty nil checks #2139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixed faulty nil checks #2139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion app/controllers/assessment/grading.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,13 @@ def submission_popover
end

def score_grader_info
score = Score.find(params[:score_id])
score = Score.find_by(id: params[:score_id])
if score.nil?
flash[:error] = "Could not find score #{params[:score_id]}."
redirect_to action: :show
return
end

grader = (if score then score.grader else nil end)
grader_info = ""
if grader
Expand Down
10 changes: 8 additions & 2 deletions app/controllers/assessments_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,8 +455,14 @@ def raw_score(scores)
end

def grade
@problem = @assessment.problems.find(params[:problem])
@submission = @assessment.submissions.find(params[:submission])
@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
Comment on lines +458 to +465
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Comment on lines +460 to +465
Copy link
Contributor

Choose a reason for hiding this comment

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

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

# Shows a form which has the submission on top, and feedback on bottom
begin
subFile = Rails.root.join("courses", @course.name, @assessment.name,
Expand Down
18 changes: 10 additions & 8 deletions app/controllers/course_user_data_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,23 +88,25 @@ def create

action_auth_level :show, :student
def show
@requestedUser = @cud.course.course_user_data.find(params[:id])
@requestedUser = @cud.course.course_user_data.find_by(id: params[:id])
if @requestedUser.nil?
flash[:error] = "Could not find user #{params[:id]}"
redirect_to([:users, @course]) && return
end
respond_to do |format|
if @requestedUser
format.html
format.json { render json: @requestedUser.to_json }
else
format.json { head :bad_request }
end
end
end

action_auth_level :edit, :student
def edit
@editCUD = @course.course_user_data.find(params[:id])
@editCUD = @course.course_user_data.find_by(id: params[:id])
if @editCUD.nil?
flash[:error] = "Can't find user in the course"
redirect_to(action: "index") && return
redirect_to(action: "show") && return
end

if (@editCUD.id != @cud.id) && !@cud.instructor? &&
Expand All @@ -124,7 +126,7 @@ def update
# ensure presence of nickname
# isn't a User model validation since users can start off without nicknames
# application_controller's authenticate_user redirects here if nickname isn't set
@editCUD = @course.course_user_data.find(params[:id])
@editCUD = @course.course_user_data.find_by(id: params[:id])
redirect_to(action: "index") && return if @editCUD.nil?

if @cud.student?
Expand Down Expand Up @@ -174,7 +176,7 @@ def update

action_auth_level :destroy, :instructor
def destroy
Copy link
Contributor

Choose a reason for hiding this comment

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

destroy / destroyConfirm don't actually seem to be routes that are currently used, might be worth removing.

@destroyCUD = @course.course_user_data.find(params[:id])
@destroyCUD = @course.course_user_data.find_by(id: params[:id])
if @destroyCUD && @destroyCUD != @cud && params[:yes1] && params[:yes2] && params[:yes3]
@destroyCUD.destroy # awwww!!!
end
Expand All @@ -187,7 +189,7 @@ def destroy
# this GET page confirms that the instructor wants to destroy the user
action_auth_level :destroyConfirm, :instructor
def destroyConfirm
@destroyCUD = @course.course_user_data.find(params[:id])
@destroyCUD = @course.course_user_data.find_by(id: params[:id])
return unless @destroyCUD.nil?

flash[:error] = "The user to be deleted is not in the course"
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/courses_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,7 @@ def run_moss

# First, validate access on each of the requested assessments
assessmentIDs&.keys&.each do |aid|
assessment = Assessment.find(aid)
assessment = Assessment.find_by(id: aid)
unless assessment
flash[:error] = "Invalid Assessment ID: #{aid}"
redirect_to(action: :moss) && return
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/groups_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ def destroy
#
action_auth_level :import, :instructor
def import
ass = @course.assessments.find(params[:ass])
ass = @course.assessments.find_by(id: params[:ass])
if !ass
flash[:error] = "Assessment not found."
redirect_to(action: :index) && return
Expand Down Expand Up @@ -360,7 +360,7 @@ def group_params
#
def get_member_cud
cud = if params[:member_id]
@course.course_user_data.find(params[:member_id].to_i)
@course.course_user_data.find_by(id: params[:member_id].to_i)
elsif params[:member_email]
@course.course_user_data.joins(:user).find_by(users: { email: params[:member_email] })
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/lti_nrps_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ def request_access_token
# NRPS endpoint for Autolab to send an NRPS request to LTI Advantage Platform
action_auth_level :sync_roster, :instructor
def sync_roster
lcd = LtiCourseDatum.find(params[:lcd_id])
lcd = LtiCourseDatum.find_by(id: params[:lcd_id])
if lcd.nil? || lcd.membership_url.nil? || lcd.course_id.nil?
raise LtiLaunchController::LtiError.new("Unable to update roster", :bad_request)
end
Expand Down
8 changes: 4 additions & 4 deletions app/controllers/schedulers_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ def visual_run

action_auth_level :update, :instructor
def update
@scheduler = Scheduler.find(params[:id])
if @scheduler.update(scheduler_params)
@scheduler = Scheduler.find_by(id: params[:id])
if @scheduler&.update(scheduler_params)
flash[:success] = "Scheduler updated."
redirect_to(course_schedulers_path(@course))
else
Expand All @@ -98,8 +98,8 @@ def update

action_auth_level :destroy, :instructor
def destroy
@scheduler = Scheduler.find(params[:id])
if @scheduler.destroy
@scheduler = Scheduler.find_by(id: params[:id])
if @scheduler&.destroy
flash[:success] = "Scheduler destroyed."
redirect_to(course_schedulers_path(@course))
else
Expand Down
10 changes: 5 additions & 5 deletions app/controllers/users_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ def create
# GET users/:id/edit
action_auth_level :edit, :student
def edit
user = User.find(params[:id])
user = User.find_by(id: params[:id])
if user.nil?
flash[:error] = "Failed to edit user: user does not exist."
redirect_to(users_path) && return
Expand All @@ -151,7 +151,7 @@ def edit
# PATCH users/:id/
action_auth_level :update, :student
def update
user = User.find(params[:id])
user = User.find_by(id: params[:id])
if user.nil?
flash[:error] = "Failed to update user: user does not exist."
redirect_to(users_path) && return
Expand Down Expand Up @@ -194,7 +194,7 @@ def destroy
redirect_to(users_path) && return
end

user = User.find(params[:id])
user = User.find_by(id: params[:id])
if user.nil?
flash[:error] = "Failed to delete user: user doesn't exist."
redirect_to(users_path) && return
Expand Down Expand Up @@ -361,7 +361,7 @@ def change_password_for_user
end

def update_password_for_user
@user = User.find(params[:id])
@user = User.find_by(id: params[:id])
return if params[:user].nil? || params[:user].is_a?(String) || @user.nil?

if params[:user][:password] != params[:user][:password_confirmation]
Expand Down Expand Up @@ -418,7 +418,7 @@ def set_gh_oauth_client
end

def set_user
@user = User.find(params[:id])
@user = User.find_by(id: params[:id])
return unless @user.nil?

flash[:error] = "User doesn't exist."
Expand Down
2 changes: 1 addition & 1 deletion script/updateGradebook.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
require File.expand_path(File.dirname(__FILE__)+'/../config/environment')

course_id = ARGV[0]
@course = Course.find(course_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

this script is unused, should delete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I delete the whole file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

@course = Course.find_by(id: course_id)
exit if !@course

#require(File.expand_path("app/models/gradebook_cache.rb"))
Expand Down