CSC/ECE 517 Fall 2015/oss E1559 rrz: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
No edit summary
Line 361: Line 361:
   end
   end
|}
|}
== References ==
[https://github.com/mandgerohit/expertiza Our Repository]

Revision as of 21:21, 31 October 2015

E1559: Refactoring JoinTeamRequestsController and InvitationController classes

This page gives a breif description on the expertiza based oss project that aimed at refactoring join_team_request_controller.rb and invitation_controller.rb files so that they follow the DRY principle and Ruby on Rails coding practices.

Code Refactoring

Code refactoring is process of changing the code to make it more maintainable, without changing the functionality of the code. Some of the reasons for performing refactoring are:

  • To remove duplicate code.
  • To make the code more maintainable.
  • To divide functionality of the class

Introduction to Expertiza

Expertiza is a web application where students can submit and review learning objects like code, writings, etc. It gives scope for creation of reusable learning objects. Students submit assignments, which can than graded through peer reviews. The Expertiza project is supported by the National Science Foundation.

Problem Statement

Classes involved

JoinTeamRequestsController and InvitationController

What the controller does

invitation_controller.rb is used by a user to invite other users to join his/her team. It performs validation before creating a request. Following chunk of code checks whether the user is allowed to get the invite or not.

   def action_allowed? # user specified only have access to the functionality of the controller
   ['Student', 'Instructor', 'Teaching Assistant'].include?(current_role_name)
   end

Now, invited user can accept or reject the request. Once the user has accepted the request, he can be seen as a part of the team.

join_team_requests_controller.rb is used when user decides to join a team. This is achieved by creating an advertisement for the team. Once the advertisement is created, it is shown in the Topic Selection section. The user who wants to join the team can send a "Request" to the members of the team. The members can then decide whether to send him/her an invite or decline the request.

What's wrong with the code

The invitation_controller.rb is doing the required task but it is difficult to understand the code, hence it becomes difficult to maintain the code. And the functions in the accept and create method can be broken down into separate methods.

In join_team_request_controller.rb has duplicate code in various methods which can be removed by creating a separate method for this common code.

What needs to be done

invitation_controller.rb

  • Rename to Invitations_Controller.rb, as is not in accordance with current naming convention.
  • Add comments explaining what each method does, and comments on how important variables are used as currently there are no comments.
  • Refactor create and accept methods. Shorten and clarify them by adding private methods, as create and accept methods currently have a lot of code.
  • Change the find_by_sql call(s) to Rails (Active Record) statements.
  • Make sure that it can be used by a user with a TA or instructor account, if they are participating in this assignment.
  • Change grammatically wrong or awkward flash messages.

join_team_requests_controller.rb

  • Add comments to the code.
  • Remove duplicate code, from create and accept methods.
  • Decline and destroy method should check for successful operation before returning.
  • Change grammatically wrong or awkward flash messages.

Implementation

Renamed invitation_controller.rb to invitations_controller.rb

The controller renaming had to be incorporated in different files of the project. The table contain the file names along with the lines before and after the changes.

File Name Line no. Before After
config/routes.rb 160 resources :invitation do resources :invitations do
config/routes.rb 170 resources :join_team_requests do
   collection do
     get :edit
   end
 end
resources :join_team_requests do
   collection do
     post :decline
     get :edit
   end
 end
app/views/join_team_requests/_list_received.html.erb 33 <%= form_tag :controller => 'invitation', :action => 'create' do %> <%= form_tag :controller => 'invitations', :action => 'create' do %>
app/views/student_teams/view.html.erb 151 {:controller => 'invitation', :action => 'cancel', :inv_id => inv.id, :student_id => @student.id} %> {:controller => 'invitations', :action => 'cancel', :inv_id => inv.id, :student_id => @student.id} %>
app/views/student_teams/view.html.erb 193 {:controller => 'invitation', :action => 'accept', :inv_id => inv.id, :student_id => @student.id, :team_id => @team_id}, {:controller => 'invitations', :action => 'accept', :inv_id => inv.id, :student_id => @student.id, :team_id => @team_id},
app/views/student_teams/view.html.erb 198 {:controller => 'invitation', :action => 'decline', :inv_id => inv.id, :student_id => @student.id} %> {:controller => 'invitations', :action => 'decline', :inv_id => inv.id, :student_id => @student.id} %>
app/views/student_teams/view.html.erb 217 <%= form_tag :controller => 'invitation', :action => 'create' do %> <%= form_tag :controller => 'invitations', :action => 'create' do %>
app/controllers/invitations_controller.rb 1 class InvitationController < ApplicationController class InvitationsController < ApplicationController

InvitationController.rb create and accept methods refactored

Before After
def create
   user = User.find_by_name(params[:user][:name].strip)
   team = AssignmentTeam.find(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.where('user_id =? and parent_id =?', user.id, student.parent_id).first
     #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."
     elsif team.full?
        flash[:error] = "Your team already has max members."
     else
       team_member = TeamsUser.where(['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

def accept

   @inv = Invitation.find(params[:inv_id])
   student = Participant.find(params[:student_id])
   assignment_id=@inv.assignment_id
   inviter_user_id=@inv.from_id
   inviter_participant = AssignmentParticipant.find_by_user_id_and_assignment_id(inviter_user_id,assignment_id)
   ready_to_join=false
   #check if the inviter's team is still existing, and have available slot to add the invitee
   inviter_assignment_team = AssignmentTeam.team(inviter_participant)
   if inviter_assignment_team.nil?
     flash[:error]= "The team which invited you does not exist any more."
   else
     if inviter_assignment_team.full?
       flash[:error]= "The team which invited you is full now."
     else
       ready_to_join=true
     end
   end

def create #adds the new invitation to the table

         if Invitation.is_invited?(@student.user_id, @user.id, @student.parent_id)
           set_invitation(@user.id,@student.user_id,@student.parent_id,'W') #'W states the invitation is waitlisted, still not accepted'
         else
           flash[:note] = @@messages[:already_invited]
         end
   update_join_team_request @user,@student
   redirect_to view_student_teams_path student_id: @student.id
 end

def update_join_team_request(user,student) #update the status in the join_team_request to A

   if user && student
     participant= AssignmentParticipant.where(['user_id =? and parent_id =?', user.id, student.parent_id]).first
     if participant
       old_entry = JoinTeamRequest.where(['participant_id =? and team_id =?', participant.id,params[:team_id]]).first
       if old_entry
         old_entry.update_attribute("status",'A') # 'A' states that the invitation has been accepted
       end
     end
   end
 end
 def auto_complete_for_user_name # searches the user name in the user table for auto complete
   search = params[:user][:name].to_s
   User.where(User.arel_table[:name].matches("%#{search}%"))
   #@users = User.find_by_sql("select * from users where LOWER(name) LIKE '%"+search+"%'") unless search.blank?
 end
 def accept # allows accepting of the invitation
   @inv = Invitation.find(params[:inv_id])
   student = Participant.find(params[:student_id])
   assignment_id=@inv.assignment_id
   inviter_user_id=@inv.from_id
   inviter_participant = AssignmentParticipant.find_by_user_id_and_assignment_id(inviter_user_id,assignment_id)
   #check if the inviter's team is still existing, and have available slot to add the invitee
   inviter_assignment_team = AssignmentTeam.team(inviter_participant)
   if inviter_assignment_team.nil?
     flash[:error]= @@messages[:invitation_not_exist]
   elsif inviter_assignment_team.full?
       flash[:error]= @@messages[:full_team]
   else
     @inv.reply_status = 'A'
     @inv.save
     #Remove the users previous team since they are accepting an invite for possibly a new team.
     TeamsUser.remove_team(student.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, student.parent_id)
     unless add_successful
       flash[:error] = @@messages[:fail_to_add]
     end
   end
   redirect_to view_student_teams_path student_id: params[:student_id]
 end

InvitationController.rb Change the find_by_sql call(s) to Rails (Active Record) statements.

Before After
@users = User.find_by_sql("select * from users where LOWER(name) LIKE '%"+search+"%'") unless search.blank? User.where(User.arel_table[:name].matches("%#{search}%"))

InvitationController.rb changed to allow TA and instructor to be a part of a team

Before After
def action_allowed?
   current_role_name.eql?("Student")
 end

def action_allowed? # user specified only have access to the functionality of the controller

   ['Student', 'Instructor', 'Teaching Assistant'].include?(current_role_name)
 end

InvitationController.rb added method for messages and invitation

private def set_messages(name)
   @@messages[:user_not_found] = "\"#{name}\" does not exist. Please make sure entered name is correct."
   @@messages[:user_not_participant] = "\"#{name}\" is not a participant of this assignment."
   @@messages[:max_members] = "Maximum team limit has been reached."
   @@messages[:already_member] = "\"#{name}\" is already a team member."
   @@messages[:already_invited] = "You have already sent an invitation to \"#{name}\"."
   @@messages[:full_team] = "The team which invited you is full now."
   @@messages[:invitation_not_exist]= "The team which invited you does not exist any more."
   @@messages[:fail_to_add] = "Something went wrong in the system. Hence failed to add you to the team which invited you. Please try again."
 end

private def set_invitation(to_id,from_id,assignment_id,reply_status)

   @invitation = Invitation.new
   @invitation.to_id = to_id
   @invitation.from_id = from_id
   @invitation.assignment_id = assignment_id
   @invitation.reply_status = reply_status
   @invitation.save
 end

JoinTeamRequestController.rb removed duplicate code

Before After
def index
   @join_team_requests = JoinTeamRequest.all                     
   respond_to do |format|                                       
     format.html # index.html.erb                                
     format.xml  { render :xml => @join_team_requests }       
end 
def index
   @join_team_request = JoinTeamRequest.all
   render_request # index.html.erb
end
def show # searches the join team requests for a particular id
    @join_team_request = JoinTeamRequest.find(params[:id]) 
    respond_to do |format|
      format.html # show.html.erb
     format.xml  { render :xml => @join_team_request }
   end
 end

def new # create a new join team request entry instance

    @join_team_request = JoinTeamRequest.new
   respond_to do |format|
     format.html # new.html.erb
     format.xml  { render :xml => @join_team_request }
   end

end

private def render_request
     respond_to do |format|
     format.html 
     format.xml  { render :xml => @join_team_request } 
    end
  end

def show # searches the join team requests for a particular id

    @join_team_request = JoinTeamRequest.find(params[:id]) 
     render_request # show.html.erb
  end

def new # create a new join team request entry instance

    @join_team_request = JoinTeamRequest.new
    render_request # new.html.erb
  end

joinTeamRequestsController.rb - Decline and destroy method should check for successful operation before returning.

Before After
def decline
   @join_team_request = JoinTeamRequest.find(params[:id])
   @join_team_request.status = 'D'
   @join_team_request.save
   redirect_to view_student_teams_path student_id: params[:teams_user_id]
 end

def decline

   @join_team_request = JoinTeamRequest.find(params[:id])
   @join_team_request.status = 'D' #'D' stands for decline
   if @join_team_request.save
     redirect_to view_student_teams_path student_id: params[:teams_user_id], notice: "JoinTeamRequest was successfully declined."
   else
     redirect_to root_path, notice: "Decline request could not be performed."
 end

end

def destroy
   @join_team_request = JoinTeamRequest.find(params[:id])
   @join_team_request.destroy
   respond_to do |format|
     format.html { redirect_to(join_team_requests_url) }
     format.xml  { head :ok }
   end
 end

def destroy # destroy a join_team_request entry of a particular id

   @join_team_request = JoinTeamRequest.find(params[:id])
   if @join_team_request.destroy
     respond_to do |format|
       format.html { redirect_to(join_team_requests_url) }
       format.xml  { head :ok }
     end
   else
     redirect_to root_path, notice: "JoinTeamRequest could not deleted."
   end
 end

References

Our Repository