CSC/ECE 517 Fall 2013/oss cmh

Introduction

The invitation controller within Expertiza handles the invite functionality which allows user to send and receives invitations. Users can create teams and invite users to join there team. The invitation controller handles the creation of the invite and the accept or decline of an invite. The invitation controller also handles requests to join teams via advertisements. After advertisements are sent a user can request to receive and invite to the team and subsequent functionality is handled by the invitation controller once an invite is sent. Our project was to refactor the code around invitations mainly dealing with the invitation controller and invitation model.

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

As is noted in the project description section our project was centered around refactoring the invitation controller. Much of the functionality that was implemented within the invitation controller actually should have been in model classes. There was only about 5 lines of code in the invitation model while there were 104 lines in the invitation controller. A lot of the code in the invitation controller needed to be dispersed amongst the invitation model and various other models within the application. The code within the invitation controller was also very confusing and difficult to understand for a number of reasons. Some of the problems causing the code to be complicated included deeply nested if statements, dense methods and variable names that did not accurately reflect the values they held. The goal of our project was to make the overall design of the invitation controller simpler thus making the code easier to understand and ensuring that distribution of functionality in the application honored the MVC framework.

Design Changes

Create Method

Though the create method is relatively small and fairly understandable, it still asked for a few changes. There were a few method calls to the deprecated method "find" which had to be replaced with calls to the "first" and the "all" methods.

The call to

 AssignmentParticipant.find(:first, :conditions => ['user_id =? and parent_id =?', user.id, student.parent_id])

was replaced with

AssignmentParticipant.first( :conditions => ['user_id =? and parent_id =?', user.id, student.parent_id])

Also, the call to

 TeamsUser.find(:all, :conditions => ['team_id =? and user_id =?', team.id, user.id])

was replaced with

TeamsUser.all(:conditions => ['team_id =? and user_id =?', team.id, user.id])

The above changes removed some of the older conventions for querying the database.

Also, to promote code readability and attempting to honor the MVC framework we moved some logic out of the create method of the invitation controller. There was a check to indicate whether a user has already been invited to a team when an invite is sent. If the invited user has already been invited the invitation won't be created but if the user hasn't already been invited then the invitation can be created. We moved the check which queried the database to the invitation model. Here's that portion of code before our refactor

sent_invitation=Invitation.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
end

and here's that portion of the code after our refactor.

sent_invitation=Invitation.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 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
end

These changes even more so helped to simply the create method and help to achieve better readability.

Below are our starting and ending versions of the create method before and after refactoring.

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 user,student

    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 “Skinny Controller, Fat Model.” <ref> Best Practices "10 Ruby on Rails Best Practices" </ref> We wanted to move much of the logistics out of the accept method letting models handle this while keeping the controller limited to being the interface between the view and model. Before accept method was refactored it was very dense and contained too much functionality. The accept method was in the invitation controller was performing much of the invitation logic that should have been handled in the invitation model and various other models throughout the application. The accept method also played many different roles in that not only was it handling items that the model was responsible for but it was performing many different operations. There are many actions that need to take place when a user accepts an invitation and the controller method was taking care of all of these things. We began to eliminate these issues by extracting smaller operations that the accept method was handling into various methods and moving this code into various models such as the invitation.rb, signed_up_user.rb and team_user.rb.

Skinny Controller, Fat Model

Before: invitation_controller.rb - accept method
The following code served the purpose of moving a user out of their current team when they accept an invite to join another team. The code actually belonged in the teams_user model because it was affecting the teams_user table in the DB. The code was extracted into the subsequent method displayed below and called from the invitation controller.

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

After: invitation_controller.rb - accept method

TeamsUser.remove_previous_team(student.user_id, params[:team_id])

After: teams-user.rb
The two methods displayed below are the sample method. The first iteration of refactoring produced the first version of the method. After reviewing method names and variable names we found that our first solution could be improved and this came up with the second and final version of the method. <ref> Coding Conventions "Ruby On Rails coding conventions, standards and best practices" </ref>

def self.remove_previous_team(user_id, team_id)
    old_entry = TeamsUser.find(:first, :conditions => ['user_id = ? and team_id = ?', user_id, team_id])
    if old_entry != nil
      old_entry.destroy
    end
end

def self.remove_team(user_id, team_id)
    team_user = TeamsUser.find(:first, :conditions => ['user_id = ? and team_id = ?', user_id, team_id])
    if team_user != nil
      team_user.destroy
    end
end

Before: invitation_controller.rb - accept method
As with the previous example we found many more examples where a few lines of code serving one purpose could be extracted into its own method. The below code serves the purpose of removing a users team from the assignment team table in the DB. This would take place if a user joins a new team and they had previously created a team and was the only member of that team.

old_team = AssignmentTeam.find(:first, :conditions => ['id = ?', params[:team_id]])
if old_team != nil
   old_team.destroy
end

After: invitation_controller.rb - accept method
This is the subsequent method call for the extracted code which was placed in the invitation controller accept method.

AssignmentTeam.remove_team_by_id(params[:team_id]) 

After: assignment_team.rb
The method created in assignment_team.rb can be seen below.

#Remove a team given the team id
def self.remove_team_by_id(id)
  old_team = AssignmentTeam.find(id)
  if old_team != nil
    old_team.destroy
  end
end

As in the two previous examples there were many method extractions from code within the accept method.

Code Readability

Scopes
Scopes are a useful feature of rails that for creations of reusable constraints for database interactions. Scopes help create reusable code and more readable code. Where one would typically have to write a whole query within their code a scope and be used to minimize the need to write the same query over and over again and decrease the amount of code.

Query within accept method:

other_members = TeamsUser.find(:all, :conditions => ['team_id = ?', params[:team_id]])

Query within accept method with scope:

other_members = TeamsUser.by_team_id(params[:team_id])

Scope Added in teams_user.rb:

scope :by_team_id, ->(team_id) { where("team_id = ?", team_id) }

The help promote better code readability we added a few more scopes during the course of refactoring the accept method.

The below example shows the before and after version of a query. The subsequent version makes the query more readable.
Before:

SignUpTopic.find(old_teams_signup.topic_id)['assignment_id']

After:

SignUpTopic.find(old_teams_signup.topic_id).assignment_id

Variable Names
The following variable was used to capture where a member could be added to a team (i.e. is there enough space on the team for the user to join). The boolean value was returned from a method call and stored in the following variable but we felt the variable name was not an accurate representation of it's purpose

add_member_return

so we renamed it to the following.

can_add_member

After the above design changes the accept method still contained a quite a few calls to the various methods. This cause us to create a model method called accept_invite which served the purpose of encapsulating these function call into a single method that was not in the controller. This method was called from the accept method thus making the accept method considerably smaller. The purpose of the accept_invite method is to handle all of the changes that take place once a user accepts an invite. Below the starting and final version of the accept method can be seen. The accept method is much smaller and more readable since our refactoring.

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

Bug Fixes

Invite Requests

Problem Description

When a particular user responds to a advertisement sent out by another user (advertiser) a new Request invitation is created which is displayed in the advertiser's received request section of the team page as shown below:

When the advertiser clicks on the invite the corresponding received request should be removed from the received requests list and the request sender should see the status as accepted when he logs in again as shown below:

Complete steps to reproduce the bug fix

1. Login as admin with password"pass"
2. Create new course and an assignment within the course
    - Set all the properties appropriately
    - Set the assignment as a team assignment
    - Set the deadlines properly.
3. Add 2 participants(user1 and user2) to the assignment(users need to be created who act as participants)
4. Create a topic within the assignment
5. Login as user1 and select the topic in the signup sheet
6. Create an advertisement in the your team link of the assignments
7. Now login as user2 and click on signup sheet. You should see an advertisement sent out by user1.
8. Click on the advertise link and request an invitation.
9. Login as user1 again and go to the your team link.
10. User1 should see the the invitation request in the received requests with the invite and decline button next to it.
11. When user1 clicks on the invite button the request invitation should be removed and when user2 logs in again he should see the sent request as accepted.

Code changes done to fix the bug

A new method was introduced to update the "status" field of the join_table_requests .The method is a follows:

def update_join_team_request
    old_entry = JoinTeamRequest.first(:conditions => ['participant_id =? and team_id =?', params[:participant_id],params[:team_id]])
    if old_entry
       old_entry.update_attribute("status",'A')
    end
end

Error Messages

Problem Description

We working on bug fixes and refactoring we noticed that some of the error messages which were not bring displayed. The issue was that the create method was setting these error messages using notice which was not handled by the flash messages partial used throughout the application. We fixed this so that error message are now being displayed properly as shown below:

Code Changes

1. The flash[:notice] is changed to flash[:note] in the invitation_controller.rb file
   
2. The _flash_messages.html.erb file is updated to include :notice which may be used in some other parts of the application.

Future Work

• For the current project we only focused on refactoring the invitation_controller.rb and moving functionality to associated models. Invitations work with many different models within the application so in order to better the overall design these associated models also need to be refactored along with the invitation controller. Being constrained to refactoring the invitation model and controller limited the ability to eliminate duplicate code and some inelegant existing designs within the code. In the future a more thorough and a wholesome approach which involves refactoring various components of Expertiza together can lead to even better code reuse.
  
• Since this project was about refactoring an existing code, we could not strictly follow the TDD model where RSpecs are written before the methods are designed. This may be considered for future assignments.
  
• Detailed documentation of the invitation and the team advertisement process can be produced which will help the users to better understand this functionality.
  

References

<references/>