CSC/ECE 517 Fall 2023 - E2357. Refactor sign up sheet controller.rb: Difference between revisions
Jump to navigation
Jump to search
| Line 376: | Line 376: | ||
def new_feedback | def new_feedback | ||
review = Response.find(params[:id]) unless params[:id].nil? | review = Response.find(params[:id]) unless params[:id].nil? | ||
if review | if review | ||
Revision as of 21:18, 6 November 2023
This wiki page is for the information regarding the changes made for the E2257 OSS assignment for Fall 2023, CSC/ECE 517.
Introduction
Expertiza is an open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well-developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.
About Controller
The sign up sheet controller contains all functions related to the management of the signup sheet for an assignment:
- Allows an instructor to add/remove topics to an assignment.
- Allows an instructor to edit properties of topics.
- Allows an instructor to assign/remove students to topics
- Allows a student to see the list of available topics which they can bid on for a given OSS assignment.
Issues
- Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
- Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out. [This may have already been fixed, but save_topic_deadline also has these problems.]
- Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
- Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
- What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
- The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
- Refactor participants variable in load_add_signup_topics
- [In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
- Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
- Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. Refactor.
- Code Climate issues
Previous Version of Project
- CSC/ECE_517_Spring_2022_-_E2223._Refactor_sign_up_sheet_controller.rb
- https://github.com/palvitgarg99/expertiza
- https://github.com/expertiza/expertiza/pull/2345
Solutions
- The naming inconsistency issue has already been fixed by the previous version of the project. It has been verified by us.
- Refactoring the Update method
Before refactoring
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end
After refactoring
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], category: params[:topic][:category],
topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description],link:params[:topic][:link] )
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
flash[:success] = 'The topic has been updated.'
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end
- Improved Method names as follows:
- signup_as_instructor_action is used to sign up a student for a particular topic for an assignment as an instructor. signup_as_instructor function does nothing. It is an empty function. The function is not being called anywhere in the code. The method is not needed and hence removed.
signup_as_instructor_action
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = 'That student does not exist!'
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = 'You have successfully signed up the student for the topic!'
ExpertizaLogger.info LoggerMessage.new(controller_name, , 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = 'The student has already signed up for a topic!'
ExpertizaLogger.info LoggerMessage.new(controller_name, , 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = 'The student is not registered for the assignment!'
ExpertizaLogger.info LoggerMessage.new(controller_name, , 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
signup_as_instructor function
def signup_as_instructor; end
- The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods. This issue is fixed by refactoring the List Method using helper methods. Here the 'list' method is doing a lot of different things, making it quite lengthy and potentially harder to maintain. So by refactoring and restructuring we can improve readability and maintainability.
Before Refactoring
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
@sign_up_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@max_team_size = @assignment.max_team_size
team_id = @participant.team.try(:id)
@use_bookmark = @assignment.use_bookmark
if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
signed_up_topics = []
@bids.each do |bid|
sign_up_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << sign_up_topic if sign_up_topic
end
signed_up_topics &= @sign_up_topics
@sign_up_topics -= signed_up_topics
@bids = signed_up_topics
end
@num_of_topics = @sign_up_topics.size
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)
unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)
# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end
After Refactoring
#Controller method
def display_topics_and_signup_info
find_participant_and_assignment
load_signup_topic_details
process_intelligent_topics if @assignment.is_intelligent
load_deadlines
check_user_signup_topics
render_intelligent_topic_selection if @assignment.is_intelligent
end
private
# Helper method: Find participant and assignment
def find_participant_and_assignment
@participant = find_participant
@assignment = @participant.assignment
end
# Helper method: Find participant based on the given ID
def find_participant
AssignmentParticipant.find(params[:id].to_i)
end
# Helper method: Find participant based on the given ID
def find_participant
AssignmentParticipant.find(params[:id].to_i)
end
# Helper method: Load sign-up topic details
def load_signup_topic_details
@slots_filled = SignUpTopic.slots_filled_for(@assignment.id)
@slots_waitlisted = SignUpTopic.slots_waitlisted_for(@assignment.id)
@signup_topics = SignUpTopic.public_topics_for_assignment(@assignment.id)
@max_team_size = @assignment.max_team_size
@use_bookmark = @assignment.use_bookmark
end
# Helper method: Process intelligent topics
def process_intelligent_topics
@bids, signed_up_topics = retrieve_bids_and_signed_topics
@signup_topics -= signed_up_topics
@bids = signed_up_topics
@num_of_topics = @signup_topics.size
end
# Helper method: Retrieve bids and signed topics
def retrieve_bids_and_signed_topics
team_id = @participant.team.try(:id)
bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
signed_up_topics = bids.each_with_object([]) do |bid, topics|
topic = SignUpTopic.find_by(id: bid.topic_id)
topics << topic if topic
end
[bids, signed_up_topics & @signup_topics]
end
# Helper method: Load deadlines
def load_deadlines
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
@student_bids = @participant.team.nil? ? [] : Bid.where(team_id: @participant.team.id)
end
# Helper method: Check user signup topics
def check_user_signup_topics
return unless (due_date = @assignment.due_dates.find_by(deadline_type_id: 1))
@show_actions = !@assignment.staggered_deadline? && (due_date.due_at < Time.now)
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = users_team.empty? ? nil : SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
# Helper method: Render intelligent topic selection
def render_intelligent_topic_selection
render('sign_up_sheet/intelligent_topic_selection') && return
end
- Signup_as_instructor_action has if-else ladder. Made this function more elegant. The `assignment_id` and `topic_id` are assigned to variables at the beginning of the method. This makes the code more concise and improves clarity. The refactored code reduces nesting by breaking down the conditions into separate if statements. This avoids deep nesting and makes the code more linear.
Before refactoring Signup_as_instructor_action
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = 'That student does not exist!'
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = 'You have successfully signed up the student for the topic!'
ExpertizaLogger.info LoggerMessage.new(controller_name, , 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = 'The student has already signed up for a topic!'
ExpertizaLogger.info LoggerMessage.new(controller_name, , 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = 'The student is not registered for the assignment!'
ExpertizaLogger.info LoggerMessage.new(controller_name, , 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
After refactoring Signup_as_instructor_action
# Signs up a student for a specific topic in an assignment by an instructor
def signup_as_instructor_action
user = User.find_by(name: params[:username])
# validate invalid user
if user.nil?
flash[:error] = 'That student does not exist!'
else
#assign params[:assignment_id] and params[:topic_id] to variables and use these variables to avoid repetition
assignment_id = params[:assignment_id]
topic_id = params[:topic_id]
if AssignmentParticipant.exists?(user_id: user.id, parent_id: assignment_id)
if SignUpSheet.signup_team(assignment_id, user.id, topic_id)
flash[:success] = 'You have successfully signed up the student for the topic!'
ExpertizaLogger.info LoggerMessage.new(controller_name, , "Instructor signed up student for topic: #{topic_id}"))
else
flash[:error] = 'The student has already signed up for a topic!'
ExpertizaLogger.info LoggerMessage.new(controller_name, , 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = 'The student is not registered for the assignment!'
ExpertizaLogger.info LoggerMessage.new(controller_name, , "The student is not registered for the assignment: #{user.id}")
end
end
redirect_to(controller: 'assignments', action: 'edit', id: assignment_id)
end
- Delete_signup and delete_signup_as_instructor violate the DRY principle. Refactored this. Created a new private function handle_signup_deletion to handle deletion for both instructor as well as student. A new function, check_signup_eligibility, generates the error message that the topic cannot be dropped. The redirect_path function is used to redirect the user.
Before Refactoring Delete_signup and delete_signup_as_instructor
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? || !participant.team.hyperlinks.empty?
flash[:error] = 'You have already submitted your work, so you are not allowed to drop your topic.'
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? && (Time.now > drop_topic_deadline.due_at)
flash[:error] = 'You cannot drop your topic after the drop topic deadline!'
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = 'You have successfully dropped your topic!'
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
def delete_signup_as_instructor
# find participant using assignment using team and topic ids
team = Team.find(params[:id])
assignment = Assignment.find(team.parent_id)
user = TeamsUser.find_by(team_id: team.id).user
participant = AssignmentParticipant.find_by(user_id: user.id, parent_id: assignment.id)
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
if !participant.team.submitted_files.empty? || !participant.team.hyperlinks.empty?
flash[:error] = 'The student has already submitted their work, so you are not allowed to remove them.'
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? && (Time.now > drop_topic_deadline.due_at)
flash[:error] = 'You cannot drop a student after the drop topic deadline!'
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = 'You have successfully dropped the student from the topic!'
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
After Refactoring Delete_signup and delete_signup_as_instructor
def delete_signup_common(participant, assignment, drop_topic_deadline, topic_id)
if !participant.team.submitted_files.empty? || !participant.team.hyperlinks.empty?
flash[:error] = 'You have already submitted your work, so you are not allowed to drop your topic.'
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, "Dropping topic for already submitted work: #{topic_id}")
elsif !drop_topic_deadline.nil? && (Time.now > drop_topic_deadline.due_at)
flash[:error] = 'You cannot drop your topic after the drop topic deadline!'
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, "Dropping topic for ended work: #{topic_id}")
else
delete_signup_for_topic(assignment.id, topic_id, participant.user_id)
flash[:success] = 'You have successfully dropped your topic!'
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, "Student has dropped the topic: #{topic_id}")
end
end
def delete_signup # delete_signup method participant = AssignmentParticipant.find(params[:id]) assignment = participant.assignment drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6) topic_id = params[:topic_id] delete_signup_common(participant, assignment, drop_topic_deadline, topic_id) redirect_to action: 'list', id: params[:id] end
def delete_signup_as_instructor # delete_signup_as_instructor method team = Team.find(params[:id]) assignment = Assignment.find(team.parent_id) user = TeamsUser.find_by(team_id: team.id).user participant = AssignmentParticipant.find_by(user_id: user.id, parent_id: assignment.id) drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6) topic_id = params[:topic_id] delete_signup_common(participant, assignment, drop_topic_deadline, topic_id) redirect_to controller: 'assignments', action: 'edit', id: assignment.id end
- Code Climate Issues:
- unexpected token error (Using Ruby 2.1 parser; configure using TargetRubyVersion parameter, under AllCops): Safe navigation operator &. is available from Ruby 2.3. Currently replaced it with nil check conditional statement
Before Refactoring
flash[:error] = error_id unless error_id&.empty? flash[:note] = message_id unless message_id&.empty?
After Refactoring
flash[:error] = error_id unless error_id.nil? || error_id.empty? flash[:note] = message_id unless message_id.nil? || message_id.empty?
- Method new_feedback has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring
Before Refactoring
def new_feedback
review = Response.find(params[:id]) unless params[:id].nil?
if review
reviewer = AssignmentParticipant.where(user_id: session[:user].id, parent_id: review.map.assignment.id).first
map = FeedbackResponseMap.where(reviewed_object_id: review.id, reviewer_id: reviewer.id).first
if map.nil?
# if no feedback exists by dat user den only create for dat particular response/review
map = FeedbackResponseMap.create(reviewed_object_id: review.id, reviewer_id: reviewer.id, reviewee_id: review.map.reviewer.id)
end
redirect_to action: 'new', id: map.id, return: 'feedback'
else
redirect_back fallback_location: root_path
end
end
After Refactoring
def new_feedback
# Replaced unless params[:id].nil? with if params[:id].present? for a more concise condition
review = Response.find(params[:id]) if params[:id].present?
if review
#Assigned session[:user] and review.map.assignment to variables for clarity
current_user = session[:user]
assignment = review.map.assignment
reviewer = AssignmentParticipant.where(user_id: current_user.id, parent_id: assignment.id).first
map = FeedbackResponseMap.where(reviewed_object_id: review.id, reviewer_id: reviewer.id).first
# if no feedback exists by dat user den only create for dat particular response/review
if map.nil?
map = FeedbackResponseMap.create(
reviewed_object_id: review.id,
reviewer_id: reviewer.id,
reviewee_id: review.map.reviewer.id
)
end
redirect_to action: 'new', id: map.id, return: 'feedback'
else
redirect_back fallback_location: root_path
end
end
- Method edit has 29 lines of code (exceeds 25 allowed). Method edit has a Cognitive Complexity of 10 (exceeds 5 allowed). Consider refactoring
Before Refactoring
def edit
assign_action_parameters
@prev = Response.where(map_id: @map.id)
@review_scores = @prev.to_a
if @prev.present?
@sorted = @review_scores.sort do |m1, m2|
if m1.version_num.to_i && m2.version_num.to_i
m2.version_num.to_i <=> m1.version_num.to_i
else
m1.version_num ? -1 : 1
end
end
@largest_version_num = @sorted[0]
end
# Added for E1973, team-based reviewing
@map = @response.map
if @map.team_reviewing_enabled
@response = Lock.get_lock(@response, current_user, Lock::DEFAULT_TIMEOUT)
if @response.nil?
response_lock_action
return
end
end
@modified_object = @response.response_id
# set more handy variables for the view
set_content
@review_scores = []
@review_questions.each do |question|
@review_scores << Answer.where(response_id: @response.response_id, question_id: question.id).first
end
@questionnaire = questionnaire_from_response
render action: 'response'
end
After Refactoring
def edit
assign_action_parameters
load_response_data
render action: 'response'
end
def handle_team_reviewing
return unless @map.team_reviewing_enabled
@response = Lock.get_lock(@response, current_user, Lock::DEFAULT_TIMEOUT)
response_lock_action if @response.nil?
end
def load_response_data
@prev = Response.where(map_id: @map.id)
@review_scores = @prev.to_a
if @prev.present?
@sorted = @review_scores.sort do |m1, m2|
if m1.version_num.to_i && m2.version_num.to_i
m2.version_num.to_i <=> m1.version_num.to_i
else
m1.version_num ? -1 : 1
end
end
@largest_version_num = @sorted[0]
end
@map = @response.map
handle_team_reviewing
@modified_object = @response.response_id
set_content
@review_scores = []
@review_questions.each do |question|
@review_scores << Answer.where(response_id: @response.response_id, question_id: question.id).first
end
@questionnaire = questionnaire_from_response
end
- Method redirect has 27 lines of code (exceeds 25 allowed). Consider refactoring.
Before Refactoring
def redirect
error_id = params[:error_msg]
message_id = params[:msg]
flash[:error] = error_id unless error_id&.empty?
flash[:note] = message_id unless message_id&.empty?
@map = Response.find_by(map_id: params[:id])
case params[:return]
when 'feedback'
redirect_to controller: 'grades', action: 'view_my_scores', id: @map.reviewer.id
when 'teammate'
redirect_to view_student_teams_path student_id: @map.reviewer.id
when 'instructor'
redirect_to controller: 'grades', action: 'view', id: @map.response_map.assignment.id
when 'assignment_edit'
redirect_to controller: 'assignments', action: 'edit', id: @map.response_map.assignment.id
when 'selfreview'
redirect_to controller: 'submitted_content', action: 'edit', id: @map.response_map.reviewer_id
when 'survey'
redirect_to controller: 'survey_deployment', action: 'pending_surveys'
when 'bookmark'
bookmark = Bookmark.find(@map.response_map.reviewee_id)
redirect_to controller: 'bookmarks', action: 'list', id: bookmark.topic_id
when 'ta_review' # Page should be directed to list_submissions if TA/instructor performs the review
redirect_to controller: 'assignments', action: 'list_submissions', id: @map.response_map.assignment.id
else
# if reviewer is team, then we have to get the id of the participant from the team
# the id in reviewer_id is of an AssignmentTeam
reviewer_id = @map.response_map.reviewer.get_logged_in_reviewer_id(current_user.try(:id))
redirect_to controller: 'student_review', action: 'list', id: reviewer_id
end
end
After Refactoring
def redirect
error_id = params[:error_msg]
message_id = params[:msg]
flash[:error] = error_id unless error_id.nil? || error_id.empty?
# Safe navigation operator &. is available from Ruby 2.3. Currently replaced it with nil check conditional statement
flash[:note] = message_id unless message_id.nil? || message_id.empty?
@map = Response.find_by(map_id: params[:id])
redirect_based_on_return
end
def redirect_based_on_return
case params[:return]
when 'feedback'
redirect_to controller: 'grades', action: 'view_my_scores', id: @map.reviewer.id
when 'teammate'
redirect_to view_student_teams_path student_id: @map.reviewer.id
when 'instructor'
redirect_to controller: 'grades', action: 'view', id: @map.response_map.assignment.id
when 'assignment_edit'
redirect_to controller: 'assignments', action: 'edit', id: @map.response_map.assignment.id
when 'selfreview'
redirect_to controller: 'submitted_content', action: 'edit', id: @map.response_map.reviewer_id
when 'survey'
redirect_to controller: 'survey_deployment', action: 'pending_surveys'
when 'bookmark'
bookmark = Bookmark.find(@map.response_map.reviewee_id)
redirect_to controller: 'bookmarks', action: 'list', id: bookmark.topic_id
when 'ta_review' # Page should be directed to list_submissions if TA/instructor performs the review
redirect_to controller: 'assignments', action: 'list_submissions', id: @map.response_map.assignment.id
else
# if reviewer is team, then we have to get the id of the participant from the team. the id in reviewer_id is of an AssignmentTeam
reviewer_id = @map.response_map.reviewer.get_logged_in_reviewer_id(current_user.try(:id))
redirect_to controller: 'student_review', action: 'list', id: reviewer_id
end
end
- Method create has a Cognitive Complexity of 8 (exceeds 5 allowed). Method create has 29 lines of code (exceeds 25 allowed). Consider refactoring:
Before Refactoring
def create
map_id = params[:id]
unless params[:map_id].nil?
map_id = params[:map_id]
end # pass map_id as a hidden field in the review form
@map = ResponseMap.find(map_id)
if params[:review][:questionnaire_id]
@questionnaire = Questionnaire.find(params[:review][:questionnaire_id])
@round = params[:review][:round]
else
@round = nil
end
is_submitted = (params[:isSubmit] == 'Yes')
# There could be multiple responses per round, when re-submission is enabled for that round.
# Hence we need to pick the latest response.
@response = Response.where(map_id: @map.id, round: @round.to_i).order(created_at: :desc).first
if @response.nil?
@response = Response.create(map_id: @map.id, additional_comment: params[:review][:comments],
round: @round.to_i, is_submitted: is_submitted)
end
was_submitted = @response.is_submitted
# ignore if autoupdate try to save when the response object is not yet created.s
@response.update(additional_comment: params[:review][:comments], is_submitted: is_submitted)
# :version_num=>@version)
# Change the order for displaying questions for editing response views.
questions = sort_questions(@questionnaire.questions)
create_answers(params, questions) if params[:responses]
msg = 'Your response was successfully saved.'
error_msg =
# only notify if is_submitted changes from false to true
if (@map.is_a? ReviewResponseMap) && (!was_submitted && @response.is_submitted) && @response.significant_difference?
@response.notify_instructor_on_difference
@response.email
end
redirect_to controller: 'response', action: 'save', id: @map.map_id,
return: params.permit(:return)[:return], msg: msg, error_msg: error_msg, review: params.permit(:review)[:review], save_options: params.permit(:save_options)[:save_options]
end
After refactoring
def create setup_response process_response finalize_response end
def setup_response map_id = params[:map_id].presence || params[:id] @map = ResponseMap.find(map_id) if params[:review][:questionnaire_id] @questionnaire = Questionnaire.find(params[:review][:questionnaire_id]) @round = params[:review][:round] else @round = nil end end
def process_response is_submitted = (params[:isSubmit] == 'Yes') @response = find_or_create_response was_submitted = @response.is_submitted # ignore if autoupdate try to save when the response object is not yet created.s @response.update(additional_comment: params[:review][:comments], is_submitted: is_submitted) # :version_num=>@version) end
def finalize_response
# Change the order for displaying questions for editing response views.
questions = sort_questions(@questionnaire.questions)
create_answers(params, questions) if params[:responses]
msg = 'Your response was successfully saved.'
error_msg =
# only notify if is_submitted changes from false to true
if should_notify_instructor_on_difference?(was_submitted)
@response.notify_instructor_on_difference
@response.email
end
redirect_to controller: 'response', action: 'save', id: @map.map_id,
return: params.permit(:return)[:return], msg: msg, error_msg: error_msg, review: params.permit(:review)[:review], save_options: params.permit(:save_options)[:save_options]
end
def find_or_create_response round = @round.to_i is_submitted = params[:isSubmit] == 'Yes' # There could be multiple responses per round, when re-submission is enabled for that round. # Hence we need to pick the latest response. @response = Response.where(map_id: @map.id, round: round).order(created_at: :desc).first @response ||= Response.create(map_id: @map.id, additional_comment: params[:review][:comments], round: round, is_submitted: is_submitted) end
def should_notify_instructor_on_difference?(was_submitted) @map.is_a?(ReviewResponseMap) && (!was_submitted && @response.is_submitted) && @response.significant_difference? end
- Method action_allowed? has a Cognitive Complexity of 6 (exceeds 5 allowed).
Before Refactoring
def action_allowed?
response = user_id = nil
action = params[:action]
# Initialize response and user id if action is edit or delete or update or view.
if %w[edit delete update view].include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if response.map.reviewer
end
case action
when 'edit'
# If response has been submitted, no further editing allowed.
return false if response.is_submitted
# Else, return true if the user is a reviewer for that response.
current_user_is_reviewer?(response.map, user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
current_user_is_reviewer?(response.map, user_id)
when 'view'
response_edit_allowed?(response.map, user_id)
else
user_logged_in?
end
end
After Refactoring
def action_allowed?
response = user_id = nil
action = params[:action]
response = find_response_for_actions(action)
case action
when 'edit'
# If response has been submitted, no further editing allowed.
return false if response&.is_submitted
# Else, return true if the user is a reviewer for that response.
current_user_is_reviewer?(response.map, response_reviewer_user_id(response))
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
current_user_is_reviewer?(response.map, response_reviewer_user_id(response))
when 'view'
response_edit_allowed?(response.map, response_reviewer_user_id(response))
else
user_logged_in?
end
end
def find_response_for_actions(action)
# Initialize response and user id if action is edit or delete or update or view.
if %w[edit delete update view].include?(action)
Response.find(params[:id])
end
end
def response_reviewer_user_id(response)
response.map.reviewer&.user_id
end
- Method update has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.
Before Refactoring
def update
render nothing: true unless action_allowed?
msg =
begin
# the response to be updated
# Locking functionality added for E1973, team-based reviewing
if @map.team_reviewing_enabled && !Lock.lock_between?(@response, current_user)
response_lock_action
return
end
@response.update_attribute('additional_comment', params[:review][:comments])
@questionnaire = questionnaire_from_response
questions = sort_questions(@questionnaire.questions)
# for some rubrics, there might be no questions but only file submission (Dr. Ayala's rubric)
create_answers(params, questions) unless params[:responses].nil?
if params['isSubmit'] && params['isSubmit'] == 'Yes'
@response.update_attribute('is_submitted', true)
end
if (@map.is_a? ReviewResponseMap) && @response.is_submitted && @response.significant_difference?
@response.notify_instructor_on_difference
end
rescue StandardError
msg = "Your response was not saved. Cause:189 #{$ERROR_INFO}"
end
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].name, "Your response was submitted: #{@response.is_submitted}", request)
redirect_to controller: 'response', action: 'save', id: @map.map_id,
return: params.permit(:return)[:return], msg: msg, review: params.permit(:review)[:review],
save_options: params.permit(:save_options)[:save_options]
end
After Refactoring
def update
render nothing: true unless action_allowed?
msg =
# the response to be updated
# Locking functionality added for E1973, team-based reviewing
begin
if @map.team_reviewing_enabled && !Lock.lock_between?(@response, current_user)
response_lock_action
return
end
update_response_attributes
process_questionnaire
create_answers_if_needed
submit_response_if_requested
rescue StandardError
msg = "Your response was not saved. Cause:189 #{$ERROR_INFO}"
end
log_and_redirect_response_save(msg)
end
def update_response_attributes
@response.update_attribute('additional_comment', params[:review][:comments])
@response.update_attribute('is_submitted', true) if params['isSubmit'] && params['isSubmit'] == 'Yes'
end
def process_questionnaire
@questionnaire = questionnaire_from_response
@questions = sort_questions(@questionnaire.questions)
end
# for some rubrics, there might be no questions but only file submission (Dr. Ayala's rubric)
def create_answers_if_needed
create_answers(params, @questions) unless params[:responses].nil?
end
def submit_response_if_requested
@response.notify_instructor_on_difference if @map.is_a?(ReviewResponseMap) && @response.is_submitted && @response.significant_difference?
end
def log_and_redirect_response_save(msg)
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].name, "Your response was submitted: #{@response.is_submitted}", request)
redirect_to controller: 'response', action: 'save', id: @map.map_id,
return: params.permit(:return)[:return], msg: msg, review: params.permit(:review)[:review],
save_options: params.permit(:save_options)[:save_options]
end
Modified Files
- sign_up_sheet_controller.rb
Tests
describe '#update' do
context 'when topic cannot be found' do
it 'shows an error flash message and redirects to assignment#edit page' do
allow(SignUpTopic).to receive(:find).with('1').and_return(nil)
request_params = { id: 1, assignment_id: 1 }
post :update, params: request_params
expect(flash[:error]).to eq('The topic could not be updated.')
expect(response).to redirect_to('/assignments/1/edit#tabs-2')
end
end
describe '#delete_signup_as_instructor' do
before(:each) do
allow(Team).to receive(:find).with('1').and_return(team)
allow(TeamsUser).to receive(:find_by).with(team_id: 1).and_return(double('TeamsUser', user: student))
allow(AssignmentParticipant).to receive(:find_by).with(user_id: 8, parent_id: 1).and_return(participant)
allow(participant).to receive(:team).and_return(team)
end
context 'when either submitted files or hyperlinks of current team are not empty' do
it 'shows a flash error message and redirects to assignment#edit page' do
allow(assignment).to receive(:instructor).and_return(instructor)
request_params = { id: 1 }
user_session = { user: instructor }
get :delete_signup_as_instructor, params: request_params, session: user_session
expect(flash[:error]).to eq('The student has already submitted their work, so you are not allowed to remove them.')
expect(response).to redirect_to('/assignments/1/edit')
end
end
context 'when both submitted files and hyperlinks of current team are empty and drop topic deadline is not nil and its due date has already passed' do
it 'shows a flash error message and redirects to assignment#edit page' do
due_date.due_at = DateTime.now.in_time_zone - 1.day
allow(assignment).to receive(:due_dates).and_return(due_date)
allow(due_date).to receive(:find_by).with(deadline_type_id: 6).and_return(due_date)
allow(team).to receive(:submitted_files).and_return([])
allow(team).to receive(:hyperlinks).and_return([])
request_params = {
id: 1,
due_date: {
'1_submission_1_due_date' => nil,
'1_review_1_due_date' => nil
}
}
user_session = { user: instructor }
get :delete_signup_as_instructor, params: request_params, session: user_session
expect(flash[:error]).to eq('You cannot drop a student after the drop topic deadline!')
expect(response).to redirect_to('/assignments/1/edit')
end
end
context 'when both submitted files and hyperlinks of current team are empty and drop topic deadline is nil' do
it 'shows a flash success message and redirects to assignment#edit page' do
allow(team).to receive(:submitted_files).and_return([])
allow(team).to receive(:hyperlinks).and_return([])
allow(SignedUpTeam).to receive(:find_team_users).with(1, 6).and_return([team])
allow(team).to receive(:t_id).and_return(1)
request_params = { id: 1, topic_id: 1 }
user_session = { user: instructor }
get :delete_signup_as_instructor, params: request_params, session: user_session
expect(flash[:success]).to eq('You have successfully dropped the student from the topic!')
expect(response).to redirect_to('/assignments/1/edit')
end
end
end
Team
Mentor
- Ameya Vaichalikar (agvaicha)
Team Members
- Sravya Karanam (skarana)
- Sucharita Nadendla (snadend3)
- Abhishek Desai (adesai7)