CSC/ECE 517 Fall 2023 - E2357. Refactor sign up sheet controller.rb
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
- response_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)