CSC/ECE 517 Fall 2013/oss cmh: Difference between revisions
Line 22: | Line 22: | ||
=== Create Method === | === Create Method === | ||
'''Before Refactor''' | |||
<pre> | <pre> | ||
def create | def create | ||
Line 58: | Line 58: | ||
end | end | ||
end | end | ||
redirect_to :controller => 'student_team', :action => 'view', :id=> student.id | |||
end | |||
</pre> | |||
'''After Refactor''' | |||
<pre> | |||
def create | |||
user = User.find_by_name(params[:user][:name].strip) | |||
team = AssignmentTeam.find_by_id(params[:team_id]) | |||
student = AssignmentParticipant.find(params[:student_id]) | |||
return unless current_user_id?(student.user_id) | |||
#check if the invited user is valid | |||
if !user | |||
flash[:note] = "\"#{params[:user][:name].strip}\" does not exist. Please make sure the name entered is correct." | |||
else | |||
participant= AssignmentParticipant.first( :conditions => ['user_id =? and parent_id =?', user.id, student.parent_id]) | |||
#check if the user is a participant of the assignment | |||
if !participant | |||
flash[:note] = "\"#{params[:user][:name].strip}\" is not a participant of this assignment." | |||
else | |||
team_member = TeamsUser.all(:conditions => ['team_id =? and user_id =?', team.id, user.id]) | |||
#check if invited user is already in the team | |||
if (team_member.size > 0) | |||
flash[:note] = "\"#{user.name}\" is already a member of team." | |||
else | |||
#check if the invited user is already invited (i.e. awaiting reply) | |||
if Invitation.is_invited?(student.user_id, user.id, student.parent_id) | |||
@invitation = Invitation.new | |||
@invitation.to_id = user.id | |||
@invitation.from_id = student.user_id | |||
@invitation.assignment_id = student.parent_id | |||
@invitation.reply_status = 'W' | |||
@invitation.save | |||
else | |||
flash[:note] = "You have already sent an invitation to \"#{user.name}\"." | |||
end | |||
end | |||
end | |||
end | |||
update_join_team_request | |||
redirect_to :controller => 'student_team', :action => 'view', :id=> student.id | redirect_to :controller => 'student_team', :action => 'view', :id=> student.id | ||
end | end |
Revision as of 21:19, 29 October 2013
Introduction
What the invitation controller does and the process advertising, creating invites and accepting invites.
Project Description
Classes: invitation_controller.rb (104 lines) invitation.rb (5 lines)
What it does: Handles invitations to join teams.
What needs to be done: The code is deeply nested and quite confusing. There should be a single method responsible for finding whether a user is already on a team, adding someone onto a waitlist, dropping someone off of a waitlist, changing/updating the topic that the user is assigned to, etc. Some of these methods should be in model classes, such as invitation.rb and signed_up_user.rb. Break the complicated methods into shorter methods with clear names, and place these methods in the most appropriate class, moving a lot of functionality to the model classes.
Currently, after someone joins a team, pending invitations are not removed. There should be a method handling deletion of invitations (including declined invitations) to a user after that user joins a team. This should be a model method.
Though this class is not long, the code looks and reads like it is very complicated. Breaking it down into multiple methods with clear names and proper division of functionality between classes will be a challenge.
Motivation
Briefly talk about why invitation controller needed refactoring.
Design Changes
Create Method
Before Refactor
def create user = User.find_by_name(params[:user][:name].strip) team = AssignmentTeam.find_by_id(params[:team_id]) student = AssignmentParticipant.find(params[:student_id]) return unless current_user_id?(student.user_id) #check if the invited user is valid if !user flash[:notice] = "\"#{params[:user][:name].strip}\" does not exist. Please make sure the name entered is correct." else participant = AssignmentParticipant.find(:first, :conditions => ['user_id =? and parent_id =?', user.id, student.parent_id]) if !participant flash[:notice] = "\"#{params[:user][:name].strip}\" is not a participant of this assignment." else team_member = TeamsUser.find(:all, :conditions => ['team_id =? and user_id =?', team.id, user.id]) #check if invited user is already in the team if (team_member.size > 0) flash[:notice] = "\"#{user.name}\" is already a member of team." else sent_invitation = Invitation.find(:all, :conditions => ['from_id = ? and to_id = ? and assignment_id = ? and reply_status = "W"', student.user_id, user.id, student.parent_id]) #check if the invited user is already invited (i.e. awaiting reply) if sent_invitation.length == 0 @invitation = Invitation.new @invitation.to_id = user.id @invitation.from_id = student.user_id @invitation.assignment_id = student.parent_id @invitation.reply_status = 'W' @invitation.save else flash[:notice] = "You have already sent an invitation to \"#{user.name}\"." end end end end redirect_to :controller => 'student_team', :action => 'view', :id=> student.id end
After Refactor
def create user = User.find_by_name(params[:user][:name].strip) team = AssignmentTeam.find_by_id(params[:team_id]) student = AssignmentParticipant.find(params[:student_id]) return unless current_user_id?(student.user_id) #check if the invited user is valid if !user flash[:note] = "\"#{params[:user][:name].strip}\" does not exist. Please make sure the name entered is correct." else participant= AssignmentParticipant.first( :conditions => ['user_id =? and parent_id =?', user.id, student.parent_id]) #check if the user is a participant of the assignment if !participant flash[:note] = "\"#{params[:user][:name].strip}\" is not a participant of this assignment." else team_member = TeamsUser.all(:conditions => ['team_id =? and user_id =?', team.id, user.id]) #check if invited user is already in the team if (team_member.size > 0) flash[:note] = "\"#{user.name}\" is already a member of team." else #check if the invited user is already invited (i.e. awaiting reply) if Invitation.is_invited?(student.user_id, user.id, student.parent_id) @invitation = Invitation.new @invitation.to_id = user.id @invitation.from_id = student.user_id @invitation.assignment_id = student.parent_id @invitation.reply_status = 'W' @invitation.save else flash[:note] = "You have already sent an invitation to \"#{user.name}\"." end end end end update_join_team_request redirect_to :controller => 'student_team', :action => 'view', :id=> student.id end
Accept Method
def accept @inv = Invitation.find(params[:inv_id]) @inv.reply_status = 'A' @inv.save student = Participant.find(params[:student_id]) #if you are on a team and you accept another invitation, remove your previous entry in the teams_users table. old_entry = TeamsUser.find(:first, :conditions => ['user_id = ? and team_id = ?', student.user_id, params[:team_id]]) if old_entry != nil old_entry.destroy end #if you are on a team and you accept another invitation and if your old team does not have any members, delete the entry for the team other_members = TeamsUser.find(:all, :conditions => ['team_id = ?', params[:team_id]]) if other_members.nil? || other_members.length == 0 old_team = AssignmentTeam.find(:first, :conditions => ['id = ?', params[:team_id]]) if old_team != nil old_team.destroy end #if a signup sheet exists then release all the topics selected by this team into the pool. old_teams_signups = SignedUpUser.find_all_by_creator_id(params[:team_id]) if !old_teams_signups.nil? for old_teams_signup in old_teams_signups if old_teams_signup.is_waitlisted == false # i.e., if the old team was occupying a slot, & thus is releasing a slot ... first_waitlisted_signup = SignedUpUser.find_by_topic_id_and_is_waitlisted(old_teams_signup.topic_id, true) if !first_waitlisted_signup.nil? #As this user is going to be allocated a confirmed topic, all of his waitlisted topic signups should be purged first_waitlisted_signup.is_waitlisted = false first_waitlisted_signup.save #Also update the participant table. But first_waitlisted_signup.creator_id is the team id #so find one of the users on the team because the update_topic_id function in participant #will take care of updating all the participants on the team user_id = TeamsUser.find(:first, :conditions => {:team_id => first_waitlisted_signup.creator_id}).user_id participant = Participant.find_by_user_id_and_parent_id(user_id,old_team.assignment.id) participant.update_topic_id(old_teams_signup.topic_id) SignUpTopic.cancel_all_waitlists(first_waitlisted_signup.creator_id, SignUpTopic.find(old_teams_signup.topic_id)['assignment_id']) end # if !first_waitlisted_signup.nil # Remove the now-empty team from the slot it is occupying. end # if old_teams_signup.is_waitlisted == false old_teams_signup.destroy end end end
def accept @inv = Invitation.find(params[:inv_id]) @inv.reply_status = 'A' @inv.save invited_user_id = Participant.find(params[:student_id]).user_id #Remove the users previous team since they are accepting an invite for possibly a new team. TeamsUser.remove_team(invited_user_id, params[:team_id]) #Accept the invite and return boolean on whether the add was successful add_successful = Invitation.accept_invite(params[:team_id], @inv.from_id, @inv.to_id) #If add wasn't successful because team was full display message unless add_successful flash[:error]= "The team already has the maximum number of members." end redirect_to :controller => 'student_team', :action => 'view', :id => Participant.find(params[:student_id]).id end