E1908 signupsheet
Jump to navigation
Jump to search
E1908. Refactoring the Sign-up sheet Controller
This page provides a description of the Expertiza based OSS project.
About Expertiza
[1] is an open source project dependent on on Rails structure. Expertiza enables the teacher to make new assignments and alter new or existing assignments. It additionally enables the educator to make a rundown of subjects the students can agree to accept. Students can shape groups in Expertiza to chip away at different undertakings and projects. Students can likewise peer audit other students' entries. Expertiza underpins accommodation crosswise over different record types, including the URLs and wiki pages.
Problem Statement
The following tasks were accomplished in this project:
- Improved the clarity of code by improving the variable and parameter names.
- Followed naming conventions throughout and renamed methods with inconsistent names including the calling methods.
- Rectified several unwanted if-else conditions in methods and optimized the code.
- Refactored all instance variables and removed unnecessarily defined variables.
- Removed certain unwanted flash messages that occur for some user actions.
- Included comments for functionalities throughout for better understanding.
About Sign-up sheet Controller
Sign-up sheet controller contains all functions related to management of the signup sheet for an assignment function to add new topics to an assignment, edit properties of a particular topic, delete a topic, etc are included here.
Issues and Fixes
- Problem 1: Create method has an if-else condition determining if create or update should be called. Create method should not be responsible for calling update. Identify why the if-else condition exists. The if-else condition exists because the current implementation calls update if a signup sheet with the same name already exists.
- Solution: Rectified this method by removing the call to update and flashing an error instead.
if topic.nil?
setup_new_topic
else
# update_existing_topic topic
# Create must not be used for calling update. So replaced it with an error message
flash[:error] = "The topic already exists."
end
end
- Problem 2: Update method has a plethora of instance variables defined before updating. These are not necessary (For e.g., look at update method of bookmarks_controller).
- Solution: Refactored the variables not needed out.
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
# Replaced all the above unnecessary variables and save with a single update call for all the parameters
@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])
- Problem 3: Destroy has a misleading else flash message.
- Solution: Refactored the mislleading flash messages not needed out.
@topic.destroy
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully deleted. ")
else
# Replaced this flash[:error] = "The topic could not be deleted." with
flash[:error] = "This topic could not be found." # error message mage more specific
end
# changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + "#tabs-5"
- Problem 4: Add_signup_topics_staggered does not do anything.
- Solution: Renamed participants variable to 'teams'.
# add_signup_topics_staggered calls add_signup_topics and does nothing else. So removed the following function.
def add_signup_topics_staggered
add_signup_topics
end
- Problem 5: Several method names are renamed to be more intuitive.
- Solution: load_add_signup_topics is renamed to get_assignment_data and ad_info is renamed to get_ad.
# Contains links that let an admin or Instructor edit, delete, view enrolled/waitlisted members for each topic
# Also contains links to delete topics and modify the deadlines for individual topics. Staggered means that different topics can have different deadlines.
# def load_add_signup_topics(assignment_id) previously
def get_assignment_data(assignment_id)
@id = assignment_id
@sign_up_topics = SignUpTopic.where('assignment_id = ?', assignment_id)
@slots_filled = SignUpTopic.find_slots_filled(assignment_id)
- Problem 6: The list method is too long and is sparsely commented.
- Solution: Added comments.
# function to list all topics and bids to a participant
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)
# If the assignment supports bidding, add all the bids of an
# individual or team to the list of signed topics
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)
# Show selected topics only if the assignment's deadline hasn't passed
unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? and @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
# TODO: fix this; cant use 0
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team[0].t_id)
end
end
render 'sign_up_sheet/intelligent_topic_selection' and return if @assignment.is_intelligent
end
- Problem 7: What are the 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.
- Solution: signup_as_instructor specifies the student and displays a new page called via a get request whereas signup_as_instructor_action is an action called via post request which aims to signup a student.
- Problem 8: Participants variable in load_add_signup_topics actually means teams that signed up for a topic.
- Solution: Renamed participants variable to 'teams'.
# Though called participants, @participants are actually records in signed_up_teams table, which
# is a mapping table between teams and topics (waitlisted recored are also counted)
@participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
@teams = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
- Problem 9: Signup_as_instructor_action has if-else ladder.
- Solution: It has been made more elegant using a helper function.
def signup_student user
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
end
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!"
end
if !user.nil? and AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
signup_student(user);
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
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
- Problem 10: Delete_signup and delete_signup_as_instructor have much in common and violates the DRY principle.
- Solution: Refactored them by moving the duplicate code to a helper function.
def can_delete_topic? is_instructor, participant, assignment, drop_topic_deadline
submission_error_message = ""
deadline_error_message = ""
if is_instructor?
submission_error_message = "The student has already submitted their work, so you are not allowed to remove them"
deadline_error_message = "You cannot drop a student after the drop topic deadline!"
else
submission_error_message = "You have already submitted your work, so you are not allowed to drop your topic."
deadline_error_message = "You cannot drop your topic after the drop topic deadline!"
end
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = submission_error_message
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
return false
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = deadline_error_message
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
return false
end
return true
end
# this function is used to delete a previous signup
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 can_delete_topic?(false, participant, assignment, drop_topic_deadline)
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 can_delete_topic?(true, participant, assignment, drop_topic_deadline)
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
Testing
As the project involved only refactoring variables and method names, only build tests and already existing unit tests were performed.
References
- https://github.com/expertiza/expertiza
- http://expertiza.ncsu.edu/ The live Expertiza website