Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T01:06:10Z

Njclimer: /* References */

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==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
<pre>
#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])

</pre>
===Refactored Where Method Calls===
<pre>
#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

</pre>
===Use good array checking===
<pre>
(check.length == 0)
(check[0].name <=> @team.name) == 0)
</pre>
They were changed to:
<pre>
matching_teams.length.zero?
(matching_teams[0].name <=> team.name).zero?
</pre>
if !first_waitlisted_user.nil?
...
end
<pre>

With
</pre>
if first_waitlisted_user
...
end
<pre>
==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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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:

<pre>comparison of Fixnum with nil failed</pre>
on
<pre>TeamNode.create parent_id: parent.id, node_object_id: team.id</pre>

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=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]

[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Style Guide]

[https://github.com/expertiza/expertiza/ Expertiza Github Repository]

[https://github.com/expertiza/expertiza/pull/431 Pull Request for E1461]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T01:05:59Z

Njclimer: /* References */

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==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
<pre>
#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])

</pre>
===Refactored Where Method Calls===
<pre>
#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

</pre>
===Use good array checking===
<pre>
(check.length == 0)
(check[0].name <=> @team.name) == 0)
</pre>
They were changed to:
<pre>
matching_teams.length.zero?
(matching_teams[0].name <=> team.name).zero?
</pre>
if !first_waitlisted_user.nil?
...
end
<pre>

With
</pre>
if first_waitlisted_user
...
end
<pre>
==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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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:

<pre>comparison of Fixnum with nil failed</pre>
on
<pre>TeamNode.create parent_id: parent.id, node_object_id: team.id</pre>

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=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Style Guide]
[https://github.com/expertiza/expertiza/ Expertiza Github Repository]
[https://github.com/expertiza/expertiza/pull/431 Pull Request for E1461]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T01:04:15Z

Njclimer: /* References */

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T01:03:31Z

Njclimer: /* Bug in Create */

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==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
<pre>
#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])

</pre>
===Refactored Where Method Calls===
<pre>
#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

</pre>
===Use good array checking===
<pre>
(check.length == 0)
(check[0].name <=> @team.name) == 0)
</pre>
They were changed to:
<pre>
matching_teams.length.zero?
(matching_teams[0].name <=> team.name).zero?
</pre>
if !first_waitlisted_user.nil?
...
end
<pre>

With
</pre>
if first_waitlisted_user
...
end
<pre>
==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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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:

<pre>comparison of Fixnum with nil failed</pre>
on
<pre>TeamNode.create parent_id: parent.id, node_object_id: team.id</pre>

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=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T01:01:57Z

Njclimer: /* Clean up Permissions */

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==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
<pre>
#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])

</pre>
===Refactored Where Method Calls===
<pre>
#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

</pre>
===Use good array checking===
<pre>
(check.length == 0)
(check[0].name <=> @team.name) == 0)
</pre>
They were changed to:
<pre>
matching_teams.length.zero?
(matching_teams[0].name <=> team.name).zero?
</pre>
if !first_waitlisted_user.nil?
...
end
<pre>

With
</pre>
if first_waitlisted_user
...
end
<pre>
==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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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:

<pre>comparison of Fixnum with nil failed</pre>
on
<pre>TeamNode.create parent_id: parent.id, node_object_id: team.id</pre>

This hapens with the unrefactored codebase 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=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T01:01:30Z

Njclimer: /* Clean up Permissions */

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==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
<pre>
#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])

</pre>
===Refactored Where Method Calls===
<pre>
#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

</pre>
===Use good array checking===
<pre>
(check.length == 0)
(check[0].name <=> @team.name) == 0)
</pre>
They were changed to:
<pre>
matching_teams.length.zero?
(matching_teams[0].name <=> team.name).zero?
</pre>
if !first_waitlisted_user.nil?
...
end
<pre>

With
</pre>
if first_waitlisted_user
...
end
<pre>
==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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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 <pre>action_allowed?</pre> to happen before any other before actions. This causes any prerequisite or dependent conditions in action_allowed? to have to be set inside <pre>action_allowed?</pre>. 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:

<pre>comparison of Fixnum with nil failed</pre>
on
<pre>TeamNode.create parent_id: parent.id, node_object_id: team.id</pre>

This hapens with the unrefactored codebase 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=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T01:00:13Z

Njclimer: /* Testing */

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==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
<pre>
#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])

</pre>
===Refactored Where Method Calls===
<pre>
#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

</pre>
===Use good array checking===
<pre>
(check.length == 0)
(check[0].name <=> @team.name) == 0)
</pre>
They were changed to:
<pre>
matching_teams.length.zero?
(matching_teams[0].name <=> team.name).zero?
</pre>
if !first_waitlisted_user.nil?
...
end
<pre>

With
</pre>
if first_waitlisted_user
...
end
<pre>
==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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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 <pre>action_allowed?</pre> to happen before any other before actions. This causes any prerequisite or dependent conditions in action_allowed? to have to be set inside <pre>action_allowed?</pre>. 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

Permissions are checked AGAIN in this call (outside action_allowed?) and are defined here independent 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:

<pre>comparison of Fixnum with nil failed</pre>
on
<pre>TeamNode.create parent_id: parent.id, node_object_id: team.id</pre>

This hapens with the unrefactored codebase 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=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T00:59:37Z

Njclimer: /* Use good array checking */

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==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
<pre>
#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])

</pre>
===Refactored Where Method Calls===
<pre>
#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

</pre>
===Use good array checking===
<pre>
(check.length == 0)
(check[0].name <=> @team.name) == 0)
</pre>
They were changed to:
<pre>
matching_teams.length.zero?
(matching_teams[0].name <=> team.name).zero?
</pre>
if !first_waitlisted_user.nil?
...
end
<pre>

With
</pre>
if first_waitlisted_user
...
end
<pre>
==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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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.
==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 <pre>action_allowed?</pre> to happen before any other before actions. This causes any prerequisite or dependent conditions in action_allowed? to have to be set inside <pre>action_allowed?</pre>. 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

Permissions are checked AGAIN in this call (outside action_allowed?) and are defined here independent 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:

<pre>comparison of Fixnum with nil failed</pre>
on
<pre>TeamNode.create parent_id: parent.id, node_object_id: team.id</pre>

This hapens with the unrefactored codebase 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=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T00:57:15Z

Njclimer:

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==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
<pre>
#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])

</pre>
===Refactored Where Method Calls===
<pre>
#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

</pre>
===Use good array checking===
<pre>
(check.length == 0)
(check[0].name <=> @team.name) == 0)
</pre>
They were changed to:
<pre>
matching_teams.length.zero?
(matching_teams[0].name <=> team.name).zero?
</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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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.
==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 <pre>action_allowed?</pre> to happen before any other before actions. This causes any prerequisite or dependent conditions in action_allowed? to have to be set inside <pre>action_allowed?</pre>. 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

Permissions are checked AGAIN in this call (outside action_allowed?) and are defined here independent 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:

<pre>comparison of Fixnum with nil failed</pre>
on
<pre>TeamNode.create parent_id: parent.id, node_object_id: team.id</pre>

This hapens with the unrefactored codebase 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=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T00:51:39Z

Njclimer: /* Style Changes Based on Current Conventions */

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==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
<pre>
#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])

</pre>
===Refactored Where Method Calls===
<pre>
#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

</pre>
===Use good array checking===
<pre>
(check.length == 0)
(check[0].name <=> @team.name) == 0)
</pre>
They were changed to:
<pre>
matching_teams.length.zero?
(matching_teams[0].name <=> team.name).zero?
</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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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.
==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 <pre>action_allowed?</pre> to happen before any other before actions. This causes any prerequisite or dependent conditions in action_allowed? to have to be set inside <pre>action_allowed?</pre>. 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

Permissions are checked AGAIN in this call (outside action_allowed?) and are defined here independent 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:

<pre>comparison of Fixnum with nil failed</pre>
on
<pre>TeamNode.create parent_id: parent.id, node_object_id: team.id</pre>

This hapens with the unrefactored codebase as well.

=References=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T00:36:03Z

Njclimer: /* "Where" Method Calls and Change to More Descriptive Variable Names */

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==Style Changes Based on Current Conventions==
==="Where" Method Calls and Change to More Descriptive Variable Names===
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:
Old Where Method Calls
<pre>
#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])

</pre>
Refactored Where Method Calls
<pre>
#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

</pre>
Use good array checking
<pre>
(check.length == 0)
(check[0].name <=> @team.name) == 0)
</pre>
They were changed to:
<pre>
matching_teams.length.zero?
(matching_teams[0].name <=> team.name).zero?
</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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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.
==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 <pre>action_allowed?</pre> to happen before any other before actions. This causes any prerequisite or dependent conditions in action_allowed? to have to be set inside <pre>action_allowed?</pre>. 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

Permissions are checked AGAIN in this call (outside action_allowed?) and are defined here independent 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:

<pre>comparison of Fixnum with nil failed</pre>
on
<pre>TeamNode.create parent_id: parent.id, node_object_id: team.id</pre>

This hapens with the unrefactored codebase as well.

=References=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T00:28:35Z

Njclimer: /* Clean up Permissions */

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==Style Changes Based on Current Conventions==
==="Where" Method Calls and Change to More Descriptive Variable Names===
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:
Old Where Method Calls
<pre>
#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])

</pre>
Refactored Where Method Calls
<pre>
#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

</pre>
Some more name and styling changes:
<pre>
(check.length == 0)
(check[0].name <=> @team.name) == 0)
</pre>
They were changed to:
<pre>
matching_teams.length.zero?
(matching_teams[0].name <=> team.name).zero?
</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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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.
==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 <pre>action_allowed?</pre> to happen before any other before actions. This causes any prerequisite or dependent conditions in action_allowed? to have to be set inside <pre>action_allowed?</pre>. 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

Permissions are checked AGAIN in this call (outside action_allowed?) and are defined here independent 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:

<pre>comparison of Fixnum with nil failed</pre>
on
<pre>TeamNode.create parent_id: parent.id, node_object_id: team.id</pre>

This hapens with the unrefactored codebase as well.

=References=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T00:17:56Z

Njclimer: /* Clean up Permissions */

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==Style Changes Based on Current Conventions==
==="Where" Method Calls and Change to More Descriptive Variable Names===
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:
Old Where Method Calls
<pre>
#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])
</pre>
Refactored Where Method Calls
<pre>
#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
</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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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.
==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 <pre>action_allowed?</pre> to happen before any other before actions. This causes any prerequisite or dependent conditions in action_allowed? to have to be set inside <pre>action_allowed?</pre>. 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

Permissions are checked AGAIN in this call (outside action_allowed?) and are defined here independent 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.

=References=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T00:17:22Z

Njclimer: /* Clean up Permissions */

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==Style Changes Based on Current Conventions==
==="Where" Method Calls and Change to More Descriptive Variable Names===
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:
Old Where Method Calls
<pre>
#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])
</pre>
Refactored Where Method Calls
<pre>
#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
</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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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.
==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 <pre>action_allowed?</pre> to happen before any other before actions. This causes any prerequisite or
dependent conditions in action_allowed? to have to be set inside <pre>action_allowed?</pre>. 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

Permissions are checked AGAIN in this call (outside action_allowed?) and are defined here independent 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.

=References=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T00:16:07Z

Njclimer: /* Testing */

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==Style Changes Based on Current Conventions==
==="Where" Method Calls and Change to More Descriptive Variable Names===
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:
Old Where Method Calls
<pre>
#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])
</pre>
Refactored Where Method Calls
<pre>
#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
</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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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.
==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 <pre>action_allowed?</pre> to happen before any other before actions. This causes any prerequisite or
dependent conditions in action_allowed? to have to be set inside <pre>action_allowed?</pre>. 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

Permissions are checked AGAIN in this call (outside action_allowed?) and are defined here independent of the actual permissions set
in the InitiationsController.

=References=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T00:15:41Z

Njclimer:

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

=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!

<pre>
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
</pre>

=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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
==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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

==Style Changes Based on Current Conventions==
==="Where" Method Calls and Change to More Descriptive Variable Names===
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:
Old Where Method Calls
<pre>
#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])
</pre>
Refactored Where Method Calls
<pre>
#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
</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
</pre>
<pre>
def leave

def remove_participant
</pre>

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.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
==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
<pre>
#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]
</pre>
=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.
<pre>
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
</pre>
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.
=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 <pre>action_allowed?</pre> to happen before any other before actions. This causes any prerequisite or
dependent conditions in action_allowed? to have to be set inside <pre>action_allowed?</pre>. 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

Permissions are checked AGAIN in this call (outside action_allowed?) and are defined here independent of the actual permissions set
in the InitiationsController.
=References=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-29T00:12:34Z

Njclimer:

'''Expertiza - Refactoring StudentTeamController'''

__TOC__

==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!

<pre>
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
</pre>

==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:
<pre>
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]
</pre>
After Refactoring:
<pre>
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]
</pre>
===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
<pre>
#Redirect Routes

redirect_to view_student_team_path student_id: student.id

#Routes.rb

resources :student_team do
</pre>
to
<pre>
#Redirect Routes

redirect_to view_student_teams_path student_id: student.id

#Routes.rb

resources :student_teams do
</pre>

===Style Changes Based on Current Conventions===
===="Where" Method Calls and Change to More Descriptive Variable Names====
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:
Old Where Method Calls
<pre>
#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])
</pre>
Refactored Where Method Calls
<pre>
#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
</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
</pre>
<pre>
def leave

def remove_participant
</pre>

Pull up method refactoring technique was used to DRY up the code and avoid duplicating data. 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 make it easy to read code.

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

==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
<pre>
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
</pre>
Refactored Code
<pre>
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

</pre>
=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
<pre>
#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]
</pre>
==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.
<pre>
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
</pre>
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.
=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 <pre>action_allowed?</pre> to happen before any other before actions. This causes any prerequisite or
dependent conditions in action_allowed? to have to be set inside <pre>action_allowed?</pre>. 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

Permissions are checked AGAIN in this call (outside action_allowed?) and are defined here independent of the actual permissions set
in the InitiationsController.
=References=
[http://www.gotealeaf.com/blog/how-to-install-ruby-on-rails-development-environment-for-linux Setting up Ruby on Rails Environment on Linux]

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-28T23:50:55Z

Njclimer:

CSC/ECE 517 Fall 2014/OSS E1461 knn

2014-10-28T23:50:15Z

Njclimer:

CSC/ECE 517 Fall 2014/ch1a 16 av

2014-09-25T22:16:45Z

Njclimer:

[https://docs.google.com/document/d/1eq3XHiUUBIrEBx-R2Mgolaw7HttSgzPniwDLEF5oVk0/edit?pli=1 Writeup Page]
<b>Continuous integration pipelines</b> (CIP) are software packages that help groups of developers manage, maintain, merge, build and test software continuously and automatically throughout the day. These packages provide the infrastructure to support the practice of <b>Continuous Integration</b> (CI), a method of continuously combining everyone changes in order to prevent integration issues.

=Continuous Integration=

=Background=
==Introduction to the Pactice of Continuous Integration==
Before we talk about the pipelines that support it, we must define continuose integration itself. Continuous integration has gained a lot of popularity with agile development, called out specifically in the flavor of agile called Extreme Programming (XP).<ref name="Stahl">D. Ståhl Modeling continuous integration practice differences in industry software development J. Syst. Software 87pp. 48 <last_page> 59. 2014. . DOI: 10.1016/j.jss.2013.08.032.</ref><ref name="Meyer">M. Meyer. Continuous integration and its tools IEEE Software 31(3), pp. 14 <last_page> 16. 2014. . DOI: 10.1109/MS.2014.58.</ref> As agile developers began to shy away from the traditional waterfall development cycle and move towards smaller cycles, the need to limit the global gates that software needed to go through arose.

The core idea is that the longer code sits with a developer, the more bugs they can introduce to the code. The older bugs are, the more they cost to fix. To combat this, continuous integration is a push to test, merge, combine, and package builds every day.

One of the core tenets of agile program that enable continuous integration is the idea of test driven development. Test driven development is the idea that you should design the tests that a piece of code should pass before developing the code, then writing the minimal amount of code required to pass all tests. If there is a function or feature that is not captured by the tests, it should not be coded. These tests were first developed as personal unit tests. Later, this was expanded into automated unit testing with unit testing frameworks, such as CUnit, JUnit, etc.
By running the test on each commit, it becomes immediately apparent on which commit the bug was introduced or exposed. This allows for less backtracking when dealing with show-stopping bugs.<ref name="P.Duvall"> P. Duvall, S. Matyas and A. Glover. Continuous Integration: Improving Software Quality and Reducing Risk 2007Available: Table of contents only http://www.loc.gov/catdir/toc/ecip0714/2007012001.html</ref><ref name="Meyer"/>

Later developments in this process adopted the use of a “build server”-a separate, clean development environment that is independent of the developers work environment. By integrating the source control, build server, and unit testing into one interface, the whole process can be managed from one single pipeline.

===Continuous Integration History===
The idea for continuous integration has been around for a lot longer than there has been a name for it or tools to run it. Nightly or daily builds have been toted as a valued for decades.<ref name="M.Fowler"/>
Many institutions began with in-house processes that supported some form continuous development. These systems were primarily put in place to deal with the resource heavy requirements of building and maintaining software. This induced daily cycles of commit, merge, test, and report. As the popularity of this system arose, it created a market for continuous integration pipeline software. These in house tools used build automation tools like build servers to create their software.

One of the oldest of these software packages was CruiseControll. Release in March, 2001, this software has been iterated upon for many years.

Another early riser in the field was Hudson. Created at Sun Micro-systems, it was released under the MIT License, so it was free (as in speech). It became popular as an open source alternative to Cruise Control. When Sun Microsystems was sold to Oracle, the creates forked the project to keep it “free”, creating the Jenkins.

Since that time, the number of products have become widespread, touting such features as cloud integration, integration with different source control, and a myriad of language support
===Benefits and Drawbacks===
Proponents state that the bugs are caught sooner, following the “Fail faster” paradigm in software engineering. Because it is strongly built on automation, proponents state that continuous integration will reduce repetitive processes that will slow down the code. <ref name="P.Duvall"/> <ref name="M.Fowler">M. Fowler. (01 May 2006). Continuous Integration. Available: http://martinfowler.com/articles/continuousIntegration.html</ref> However, the most powerful result of the process is that it maintains an up-to-date “clean” node of the software. This software can usually be packaged into a release with little additional effort. This provides risk mitigation by having a partially complete product, as well as aids in bug tracking. <ref name="P.Duvall"/><ref name="M.Fowler"/>

Skeptics state that continuous integration requires costly frameworks and framework maintenance, and gives little benefit over frequent integration. There is no unified standard on how to implement CI and thus it is hard to measure its affects. It is such a huge process and time consuming process that it is difficult to tell which parts improve and which parts do nothing to help the software development process.<ref name="Stahl"/>

=Introduction to Continuose Integration Software (Pipelines)=

There are many suggested features for continous integration software pipeline. These software piplenes could be adhoc and built in house, or externally provided. This section will cover best practices and procedures to ensure functional system builds that report no unexpected errors to the developers. Features include version control, build and test automation, as well as developer housekeeping rules that streamline the process into an powerful software development tool. Below are essential principles of continuous integration:
===Maintain a Source Repository===
Source code can become very complex in larger systems. Continuous integration mitigates untracked branching that developers wish to avoid. The entire code for a functional build should be on the central repository, including test cases, configuration files, and database schemas. For proper functionality, the baseline suggestion is to host the information in a source code management system like Subversion, where all developers push towards the current build.<ref name="M.Fowler"/>

===Automate the Build===
Across the numerous development platforms, there are many build scripts that are used to configure a project and in some cases using very uncommon commands. Sometimes changes to the source code file structure can cause the build scripts to change. Therefore automation can serve to reduce the possibility of incorrect configurations and simplify the entire build process.<ref name="P.Duvall"/> Automated build systems are standard in a number of systems . Java developers created Ant, Unix users configure with make, .NET developers prominently use MSBuild, and Ruby has the rake command that automates configuration, build, and database migration.

It is imperative that these builds include all source code committed to the repository.<ref name="P.Duvall"/> This means that introducing a new machine should only require downloading the project source and instructing a single command to have a fully functional application or executable.

===Make Your Build Self-Testing===
Extreme Programming(XP) and Test Driven Development(TDD) show that self-testing code is very valuable in catching the bugs that sometimes slip through regression test coverage. In continuous integration pipelines, these development methodologies aren't paramount to an effective system. The significance is in having having the ability to build test code that covers a majority of the project by issuing issuing a single command.<ref name="M.Fowler"/> Since this procedure is called after a clean commit, a resulting failure in the test code build should result in a failed project build as well. Essentially, this automatic testing procedure gives developers confidence in functional project releases.
===All Contributors Commit To the Mainline Every Day===
Frequent communication is a major facet of continuous integration. Each day developers follow a series of operations to submit successful repository changes:
*Retrieve and update the trunk or mainline code
*Fix any conflicts as they merge their commit to the trunk
*Pass automatic build in testing and release
Once the commit is established on the trunk, other developers can follow the same procedure to validate their contributions while the version control system provided another form of error checking in file inconsistencies. Continuous integration requires this quick and constant communication so that any code conflicts can be found and removed in a fast manner.<ref name="P.Duvall"/> When an error is detected in running code rather than compilation, the system of continuous commits reduces the search time for repository changes that caused the system to break.
===Every Commit Should Build the Mainline on an Integration Machine===
After every commit from a developer merging changes to the mainline, the system should execute some automated build script or a daemon to automatically configure and run the build process.<ref name="M.Fowler"/> Each commit is only successful if the build of the main source code passes. Of course this does give each developer the responsibility of maintaining source code in good state, as their changes may introduce error to the system.

===Keep the Build Fast===
Commits to the system should be frequent. The system uses quick iterations as a way to reduce the amount of bugs introduced to the code base. Continuous integration utilizes the deployment pipeline as a two staged process for committing a correct build.<ref name="M.Fowler"/> In the first stage, the build process consists of unit testing and merging with the mainline on a localized copy of the source and a mock or nonexistent database. After the commit build stage, the developer will have a good commit sent to the server for testing on the production database and source filesystem. Building on both the production level and local versions is imperative, as it allows developers more code coverage with the ability to utilize performance testing in the build process.
===Test in a Clone of the Production Environment===
As previously noted, the mainline should be tested in a local copy before the server automated build test.<ref name="P.Duvall"/> Since developers may operate in varied computing environments, resulting errors from updating to the mainline can be machine dependent, thus only fixable by the developer with that specific code conflict. Setting up an environment cloned as closely to the source environment as possible, promotes the ability to set up a testing environment that can relay performance testing of the build without affecting the production database. Therefore if a test fails in the mock environment, then the commit will not take effect on the mainline of the version control system.
===Make it Easy for Anyone to Get the Latest Executable===
It seems trivial to note, but all contributors, meaning developers and testers, should have access to the current build.<ref name="P.Duvall"/><ref name="M.Fowler"/> Keeping multiple eyes on the development of the code base ultimately mitigates any bugs or defects that could go untracked in the build process. When a contributor sees a issue in the code, they are instinctively prompted to call attention to this mistake. Again, taking us back to the recurring moniker that the earlier conflict is found, the quicker it can be handled.
===Everyone can see what's happening===
Visibility is truly important in developing projects that have a varied number of developers, each committing numerous times daily towards a complex code base. In some cases, those developing the executable may be operating in distant locations. Continuous integration promotes accountability in version changes. Everyone working on the project should know the state of the current build as well as the author of each commit to the system.<ref name="P.Duvall"/> With a full awareness, contributors know who to contact for relevant issues and get a deeper understanding of the code base.
===Automate Deployment===
Expounding the principle of automated build, a system operating a continuous integration platform should be structured to configure and build in various environments. Notably, the system needs environments for commit testing, secondary testing, and a production application environment. Constant file transfers and updates relies on the use of simple command scripts to establish the previously denoted build procedure in a single operation. Other than providing this time savings, the automated build process ensures that the code mainline is in a healthy functioning state before any release.<ref name="M.Fowler"/>
=Platforms=
===Jenkins<ref name="Jenkins">Jenkins CI. (). Welcome to Jenkins CI! | Jenkins CI . Available: http://jenkins-ci.org/.</ref>===
Jenkins is an open source project that split from one of the earliest CI pipelines, Hudson, in 2010. As one of the longest running pipelines, it has been continuously extended, supporting many different languages, including C, C++, Java, and Python.
===Bamboo<ref name="bamboo">(). Continuous Integration & Build Server - Bamboo https://www.atlassian.com/software/bamboo </ref>===
Bamboo is proprietary build server. Built by the same company that made One of its unique feature is that is can expose the service on the cloud rather than having the developers push it themselves. It also professesses being able to be integrated with Jira, an issue tracking software.
===ThoughtWorks Go<ref name="thought">(2014, September 18). Continuous Integration | ThoughtWorks [Online]. Available: http://www.thoughtworks.com/continuous-integration.</ref>===

Thoughtworks Go highlights continuous integration as a key component of an expanded production mechanism that the creators Go envision as concept called Continuous Delivery. These CD platforms are extremely useful for managing code releases in large organizations. The service of helps promote lean and dynamic low-risk releases that streamline the ability to automate development and test that always ensures completely functional deployments.

Software is available on multiple development operating systems including Windows, Mac OS X, popular Linux distributions, Solaris, and Package OS.

===Travis-ci<ref name="travis">(2014, September 17). Travis CI: Getting started [Online]. Available: http://docs.travis-ci.com/user/getting-started/.</ref>===
Travis is a hosted and distributed continuous integration service. As it’s mainly suggested for closed projects, the service is integrated with GitHub to utilizes the version control framework for determining builds.

=See Also=
===References to Continuous Integration===
*http://en.wikipedia.org/wiki/Continuous_integration
*http://www.methodsandtools.com/archive/archive.php?id=121
*http://en.wikipedia.org/wiki/Comparison_of_continuous_integration_software
*http://www.martinfowler.com/articles/continuousIntegration.html
*http://ieeexplore.ieee.org.prox.lib.ncsu.edu/stamp/stamp.jsp?tp=&arnumber=6802994

===References to Specific Pipelines===
*http://hudson-ci.org/
*https://travis-ci.com/
*http://jenkins-ci.org/
*http://www.thoughtworks.com/products/go-continuous-delivery
*https://www.atlassian.com/software/bamboo
*http://integrity.github.io/
*http://stridercd.com/
*http://en.wikipedia.org/wiki/Team_Foundation_Server

=References=
<references/>