CSC/ECE 517 Fall 2014/OSS E1461 knn

Expertiza - Refactoring StudentTeamController

Introduction

Expertiza is an open source web application created by NCSU give a interface for team learning and peer review. Expertiza allows students to create and communicate with teams as well as an easy platform for online assignment submission. Another feature of Expertiza is its framework for allowing a peer review system on assignments, creating an easy way to give and receive feedback on assignments, allowing for improvement and re-submission based on classmates constructive feedback.

One part of the OSS project for Fall 2014 was refactoring of different sections of Expertiza. Our team was tasked with refactoring of the StudentTeamController.

StudentTeamController

StudentTeamController is responsible for the methods creation and joining of teams for group projects on Expertiza. Specifically, this controller allows the user to:

• Create, edit or delete a team (name)
• Add a teamate to the team
• Leave a team if they want to move to a new team
• View received and sent invitations

This controller also provides the student and team instance variables to the corresponding student_team views.

We were tasked with refactoring this controller using DRY principles and more RESTful actions as well as updating our sections using the most current excepted ruby and rails style guidelines. In addition, 6 specific tasks were given:

• Change the list method to index (and others in the RESTful style)
• Use Ruby 1.9 style key: value pairs
• Remove commented-out code
• Use routing helpers (http://guides.rubyonrails.org/routing.html#path-and-url-helpers)
• Pluralize the class (StudentTeamsController)
• Use `.zero?` instead of `== 0`

---

Setting Up Expertiza on Your Computer to Work on it

Most people who are new to working on Linux and Github can have issues working, so these links and instructions can help you setting it up on your computer for final project or other purposes. Setup was done first on Windows and because of lot of gem conflicts with windows, Linux 14.04 was used instead.

-Update Linux package
-Install Git, Setup username , password
-Suggest Installing Ruby with Rbenv Manager
-Install ruby version 2+ and look for available versions using -l command
-Install rails 4+
-Install and setup Java Environment
-Check the Ruby, Rails and Java Environment Versions Before cloning Expertiza
-Setup for Nokogiri Gem so it doesn't give error
-Install Mysql and setup password and keep it safe as it will be required for importing database
-Fork and Clone Expertiza from https://github.com/expertiza/expertiza.git
-Import Database
-Bundle Install should work and GOOD LUCK!

sudo apt-get update
sudo apt-get install git
git config --global user.name "Name Surname"
git config --global user.email aaa@ncsu.edu

git clone git://github.com/sstephenson/rbenv.git .rbenv
echo 'export PATH="$HOME/.rbenv/bin:$PATH"' >> ~/.bashrc
echo 'eval "$(rbenv init -)"' >> ~/.bashrc

git clone git://github.com/sstephenson/ruby-build.git ~/.rbenv/plugins/ruby-build
echo 'export PATH="$HOME/.rbenv/plugins/ruby-build/bin:$PATH"' >> ~/.bashrc
git clone https://github.com/sstephenson/rbenv-gem-rehash.git ~/.rbenv/plugins/rbenv-gem-rehash
git clone https://github.com/ianheggie/rbenv-binstubs.git ~/.rbenv/plugins/rbenv-binstubs
bundle install --binstubs .bundle/bin

rbenv install -l
rbenv install 2.1.2
rbenv global 2.1.2
gem install rails

sudo apt-get install java7-jdk
export JAVA_HOME=/usr/lib/jvm/java-7-openjdk-amd64/
sudo apt-get install mysql-server libmysqlclient-dev

ruby -v
rails -v
java -version

bundle config build.nokogiri --use-system-libraries
gem install mysql
mysql -u root -p Your_Database < Database-You-Importing.sql
gem install rjb
bundle install

Refactoring of Old Code

Using Routing Helpers

Modifications were made to routes pointing to student_teams views by adding helpers as well as changing to new bracket initializers and removed unnecessary parens. Changes were made inside StudentTeamsController.rb as well as anywhere that used routes pointing to StudentTeamsController which consisted of:

• advertise_for_partner_controller.rb
• invitation_controller.rb
• join_team_requests_controller.rb
• reports_controller.rb
• response_controller.rb
• app\views\advertise_for_partner\show.html.erb
• app\views\student_task\view.html.erb
• app\views\student_teams\view.html.erb

The routes changed were as follows:

Before Refactoring:

redirect_to :controller => 'student_team', :action => 'view' , :id=> @student.id
redirect_to :controller =>'student_team', :action => 'edit', :team_id =>params[:team_id], :student_id => params[:student_id]

After Refactoring:

redirect_to view_student_teams_path student_id: student.id
redirect_to edit_student_teams_path team_id: params[:team_id], student_id: params[:student_id]

Pluralize StudentTeamController Class Name

StudentTeamController was changed to StudentTeamsController in accordance to current Rails convention. This required the pluralizing of any references to the class such as in routes directing to StudentTeamsController and in the routes.rb file itself

#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do

to

#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do

Style Changes Based on Current Conventions

All where method calls in the controller were refactored to reflect current style guidelines. These calls were changed from the older style more closely resembling a sql WHERE statement to the more conventional style. In addition, the check variables used in the create and update methods were changed to existing_assignment and matching_teams to better reflect what they represent. Finally, in places where ".where" was being used to find just one instance, it was instead changed to "find_by". The changes made are shown below:

Non-SQL Style Where Method Calls

Old Where Method Calls

#view

@send_invs = Invitation.where( ['from_id = ? and assignment_id = ?', @student.user.id, @student.assignment.id])
@received_invs = Invitation.where( ['to_id = ? and assignment_id = ? and reply_status = "W"', @student.user.id, @student.assignment.id])

#create

check = AssignmentTeam.where( ["name =? and parent_id =?", params[:team][:name], @student.parent_id])

#update

check = AssignmentTeam.where( ["name =? and parent_id =?", params[:team][:name], @team.parent_id])

#leave

user = TeamsUser.where(["team_id =? and user_id =?", params[:team_id], @student.user_id]).first
other_members = TeamsUser.where( ['team_id = ?', params[:team_id]])
old_team = AssignmentTeam.where( ['id = ?', params[:team_id]])
signups = SignedUpUser.where( {:creator_id => params[:team_id]})
non_waitlisted_users = SignedUpUser.where( {:topic_id => signup_topic_id, :is_waitlisted => false})
max_choosers = SignUpTopic.where( {:id => signup_topic_id}).first.max_choosers
first_waitlisted_user = SignedUpUser.where( {:topic_id => signup_topic_id, :is_waitlisted => true}).first
waitlisted_team_user = TeamsUser.where( {:team_id => first_waitlisted_user.creator_id}).first
old_invs = Invitation.where( ['from_id = ? and assignment_id = ?', @student.user_id, @student.parent_id])

Refactored Where Method Calls

#view

@send_invs = Invitation.where from_id: student.user.id, assignment_id: student.assignment.id
@received_invs = Invitation.where to_id: student.user.id, assignment_id: student.assignment.id, reply_status: 'W'

#create (change of variable names)

existing_assignments = AssignmentTeam.where name: params[:team][:name], parent_id: student.parent_id

#update (change of variable names)

matching_teams = AssignmentTeam.where name: params[:team][:name], parent_id: team.parent_id

#remove_participant (renamed method - was #leave; .find and .find_by refactoring)

team_user = TeamsUser.find_by team_id: params[:team_id], user_id: student.user_id
old_team = AssignmentTeam.find params[:team_id]
sign_ups = SignedUpUser.where creator_id: params[:team_id]
non_waitlisted_users = SignedUpUser.where topic_id: sign_up_topic_id, is_waitlisted: false
max_choosers = SignUpTopic.find(sign_up_topic_id).max_choosers
first_waitlisted_user = SignedUpUser.find_by topic_id: sign_up_topic_id, is_waitlisted: true#<order?
waitlisted_team_user = TeamsUser.find_by team_id: first_waitlisted_user.creator_id 
old_invites = Invitation.where from_id: student.user_id, assignment_id: student.parent_id

Use good array checking

(check.length == 0)
(check[0].name <=> @team.name) == 0)

They were changed to:

matching_teams.length.zero?
(matching_teams[0].name <=> team.name).zero?

if !first_waitlisted_user.nil?

 ...

end

With

if first_waitlisted_user

 ...

end

==Use boolean-like objects in conditionals==
Replace
<pre>

==Method Refactoring==
Different method refactoring techniques were used to make clarity and understanding of code very easy and simple.
For example,
Rename method technique was used for "remove" method which didn't mean anything to more understandable name "remove_advertisement".
"Leave" was changed to "remove_participant" which is more helpful for reader.

<pre>
  def remove

  def remove_advertisement

  def leave

  def remove_participant

Pull up method refactoring technique was used to DRY up the code and avoid duplication. For example, statement in code below was used at 3 different places in original code so pulling up that command in one method and calling "team_created_successfully" at all places made it easy to read code.

  def team_created_successfully
    undo_link "Team \"#{team.name}\" has been updated successfully. "
  end

Before Actions

Two lines were repeated in multiple methods in the StudentTeamsController, one to set the @student instance variable and one to set the @team instance variable. The setting of the @student variable is used in the view, update, edit, create, and leave methods while @team is set in the edit and update methods. The setting of these variables was moved to methods which cache the values to set the local variables lazily. These methods had to be used as before_actions in order to be usable by the view and edit views. Finally, in order to standardize the setting of the instance variable, the name of the :id params value was changed to :student_id in all redirect calls. Old Code

Setting of @student and @team instance variable

#view, create

@student = AssignmentParticipant.find(params[:id])

#edit, leave

@student = AssignmentParticipant.find(params[:student_id])

#edit,update

@team = AssignmentTeam.find(params[:team_id])

Changing of Routes

redirect_to :controller => 'student_team', :action => 'view' , :id=> @student.id

Refactored Code

Before Actions

 def team
    @team ||= AssignmentTeam.find params[:team_id]
  end
  def team=(value)
    @team = value
  end

  def student
    @student ||= AssignmentParticipant.find(params[:student_id])
  end

  def student=(value)
    @student = value
  end

  before_action :team, only: [:edit, :update]
  before_action :student, only: [:view, :update, :edit, :create, :remove_participant]

Changing of Routes (Changed all instances of redirects to view to reflect student_id)

redirect_to :controller => 'student_team', :action => 'view' , :student_id=> @student.id

Removed Comment out Code

Commented out code can be restored from the repository. It only serves to clutter the class. Blocks like the one shown below were removed

   #respond_to do |format|
   # format.html # index.html.erb
   #format.xml { render :xml => @log_entries }
   #end
   #redirect_to :controller => 'student_team', :action => 'advertise_for_partners' , :id => params[:team_id]

Work left to be Done

There is much work left to be done with this controller. Some of the changes have wider scope than just this class.

Change View to Show

The correct name for the view method is show. This was noticed at the last minue, and because of the use of "view" throughout rails simple refactoring tools were not enough to fix this.

Do Bookeeping in Models not in the Controllers

The largest function by far in this controller is the remove_participant (formally 'leave') function.

    def remove_participant
      #remove the topic_id from participants
      student.update_topic_id nil
  
      #remove the entry from teams_users
  
      team_user = TeamsUser.find_by team_id: params[:team_id], user_id: student.user_id
  
      if team_user
        team_user.destroy
  
        undo_link "User \"#{team_user.name}\" has been removed from the team successfully. "
      end
  
      #if your old team does not have any members, delete the entry for the team
      if TeamsUser.where(team_id: params[:team_id]).empty?
        old_team = AssignmentTeam.find params[:team_id]
        if old_team
          old_team.destroy
          #if assignment has signup sheet then the topic selected by the team has to go back to the pool
          #or to the first team in the waitlist
  
          sign_ups = SignedUpUser.where creator_id: params[:team_id]
          sign_ups.each {|sign_up|
            #get the topic_id
            sign_up_topic_id = sign_up.topic_id
            #destroy the sign_up
            sign_up.destroy
  
            #get the number of non-waitlisted users signed up for this topic
            non_waitlisted_users = SignedUpUser.where topic_id: sign_up_topic_id, is_waitlisted: false
            #get the number of max-choosers for the topic
            max_choosers = SignUpTopic.find(sign_up_topic_id).max_choosers
  
            #check if this number is less than the max choosers
            if non_waitlisted_users.length < max_choosers
              first_waitlisted_user = SignedUpUser.find_by topic_id: sign_up_topic_id, is_waitlisted: true#<order?
  
              #moving the waitlisted user into the confirmed signed up users list
              if first_waitlisted_user
                first_waitlisted_user.is_waitlisted = false
                first_waitlisted_user.save
  
                waitlisted_team_user = TeamsUser.find_by team_id: first_waitlisted_user.creator_id #<this relationship is weird
                #waitlisted_team_user could be nil since the team the student left could have been the one waitlisted on the topic
                #and teams_users for the team has been deleted in one of the earlier lines of code
  
                if waitlisted_team_user
                  user_id = waitlisted_team_user.user_id
                  if user_id
                    waitlisted_participant = Participant.find_by_user_id user_id
                    waitlisted_participant.update_topic_id nil
  
                  end
                end
              end
            end
  
          }
        end
      end

Everything past the 14th line of code is bookeeping that should be handled by the model itself. In large part, relationships could solve all of the checks if members exist or are empty and destroy them with dependent_destroy. In addition, the current TeamsUser model should be removed entirely and replaced with the had_many_and_belongs_to relationship.

RSpec Functional Testing

The system currently only has limited RSpec tests. The team was only able to implement rudementary tests. Additional tests are required to ensure good code coverage.

Additional Changes to Name

There is no such construct as a "Student Team". In fact, "Student Teams" can currently have members who are not students (instructors), though the permissions for the instructors actually doing anything in the controller are tied off. A more accurate name of the class, and more accurate roles for the class, could be defined by "AssignmentTeamsController" which is the actual focus of this class.

Unessisary Member Variables

Member variables are exposed outside the scope of the specific action. They should be used in as few situations as possible. A minimal set of target classes (Such as @team, and possibly @student) should be all that is used.

Clean up Permissions

The current permissions scheme forces the permissions to be checked at the beginning of the controller. This makes it so that the controller has the dule role of interfaceing the views with the model AND managing its permissions.

The inheritance structure currently forces `action_allowed?` to happen before any other before actions. This causes any prerequisite or dependent conditions in action_allowed? to have to be set inside `action_allowed?`. However, this method is not called in the case of a Super User login, and therefore these types of members need to be set both in the action_allowed? and before the view is shown.

Finally, the current permissions scheme causes the odd structures we see in the "View" call.

  def view
    #View will check if send_invs and recieved_invs are set before showing
    #only the owner should be able to see those.
    return unless current_user_id? student.user_id
    @send_invs = Invitation.where from_id: student.user.id, assignment_id: student.assignment.id
    @received_invs = Invitation.where to_id: student.user.id, assignment_id: student.assignment.id, reply_status: 'W'
  end

As you can see, the view method checks permissions again in this call (outside action_allowed?). In addition, this is not very dry, as the are defined here copies of the actual permissions set in the InitiationsController.

This could be addressed to clean up the beginning of the controller. However, this is a sweeping change across the whole codebase.

Bug in Create

There exists a bug in the create call. Currently, Ruby throws:

comparison of Fixnum with nil failed

on

TeamNode.create parent_id: parent.id, node_object_id: team.id

This hapens with the unrefactored codebase (Git Rev d2144d13a6fd26203e464a90beaaaceb69506c6f) as well.

Remove Dynamic Finders

Dynamic Finders (find_by_xxx_yyyy) are depreciated in Rails 4. They should be replaced by `where(xxx: ,yyy:).first` or by `find_by` (non-dynamic find_by is not depreciated).

References

Setting up Ruby on Rails Environment on Linux

Global Style Guide

Expertiza Github Repository

Pull Request for E1461