CSC/ECE 517 Fall 2023 - E2357. Refactor sign up sheet controller.rb: Difference between revisions
Line 374: | Line 374: | ||
'''Before Refactoring''' | '''Before Refactoring''' | ||
def new_feedback | def new_feedback | ||
review = Response.find(params[:id]) unless params[:id].nil? | review = Response.find(params[:id]) unless params[:id].nil? | ||
Line 389: | Line 390: | ||
end | end | ||
'''After Refactoring''' | '''After Refactoring''' | ||
def new_feedback | def new_feedback | ||
# Replaced unless params[:id].nil? with if params[:id].present? for a more concise condition | # Replaced unless params[:id].nil? with if params[:id].present? for a more concise condition |
Revision as of 20:58, 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
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)