CSC/ECE 517 Fall 2013/oss cmh

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

The theme that we went with for refactoring the accept method is “Skin Controller, Fat Model.” We wanted to move much of the logistics out of the accept method letting models handle this while keeping the controller strictly there to handle communication between the view and model. The accept method within the controller had too much functionality. We first moved much of the functionality within the accept method to a model accept method which purpose is to make sure that all items that need to be taken care of upon a method accept are in fact handled. Just moving this code to the model did not solve the issue of complexity. We created smaller methods to handle individual changes that needed to be taken care of for example upon an accept the users topic must be updated, the topics they are waitlisted for must be dropped, amongst many other things. We took these smaller changes that needed to be made and moved them into separate methods amongst various models like signed_up_user.rb and teams_user.rb. We also noticed that some variable names did not do a good job of describing their focus so we changed these variable names to increase readability. After our refactoring the accept method is much smaller and much more readable.<ref> Coding Conventions "Ruby On Rails coding conventions, standards and best practices" </ref><ref> Best Practices "10 Ruby on Rails Best Practices" </ref>

Before Refactor

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

After Refactor

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

References

<references/>