CSC/ECE 517 Spring 2015/oss E1506 SYZ

E1506: Refactoring, testing and new features related to “users”

Overview

Code refactoring

Refactoring is a disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behavior<ref>Refactoring</ref>. Refactoring adds to the value of any program that has at least one of the following shortcomings<ref>Benefits of Code Refactoring Michael Hunger. Oct. 25, 2000</ref>:

• Programs that are hard to read are hard to modify;
• Programs that have duplicate logic are hard to modify;
• Programs that require additional behavior that requires you to change running code are hard to modify;
• Programs with complex conditional logic are hard to modify.
  

Rspec

RSpec is a behavior-driven development (BDD) framework for the Ruby programming language, inspired by JBehave. It contains its own mocking framework that is fully integrated into the framework based upon JMock. The framework can be considered a domain-specific language (DSL) and resembles a natural language specification<ref>Rspec</ref>.

Object-oriented Design Principles

Object-oriented programming (OOP) is a programming language model organized around objects rather than "actions" and data rather than logic<ref>Margaret Rouse. object-oriented programming (OOP) definition. Aug. 3, 2008</ref>. Historically, a program has been viewed as a logical procedure that takes input data, processes it, and produces output data. Though an Object oriented language provides us with highly useful and important programming concepts like Inheritance, Polymorphism, Abstraction and Encapsulation which definitely makes the code more efficient, it is equally important to have the knowledge of using them in the code. Object Oriented Design Principles are core of OOPS programming<ref>Javin Paul. Blogspot. March 3, 2012</ref>. It is important to know these design principles, to create clean and modular design. There are many design principles that help us to create clean and efficient code.

Project Resources

• GitHub Repository
• AWS (Username: user6, no password)
• YouTube Video Demo
• GitHub Pull Request Link
• Travis CI build passed

Todo List

Related files:

* users_controller.rb
* participants_controller.rb
* user.rb
* users/show.html.erb

• Find the un-called methods if any and delete them. [done]
• Change the Rails 2 syntax to Rails 4 style.
• Refactor users_controller.rb
  • Change the white space for the second harf of this file, starts at “def edit”. [done]
  • Separate the “paginate_list” method into two methods. The search method should be in model and the paginating method should be in the controller. [done]
• New feature: delete users
  • A user can be deleted if (s)he has not participated in an assignment. [done]
  • If the user is participating in an assignment, the system will ask, “User is participating in k assignments. Delete as a participant in these assignment(s)?” [done]
  • If the user has submitted or reviewed in any of these assignments, the system will say the user cannot be deleted, but offer to rename the user account to <current_account_name>_hidden.
    • rename (javascript calling update method in users_controller.rb) [done]
    • different users have different delete methods. [done]
  • If the person trying to delete does not want to rename the account, the system will just say that the user can’t be deleted. [done]
  • Write tests (with Rspec) for this feature.[done]
• Testing feature: search for users
  • In rails 4 branch, admins can search for the users with 1) users’ login names 2) users’ last or first names and 3) users’ emails. Please write tests (with Rspec) for this feature [done]

Delete Uncalled Methods

Method

• The primary way we find unused method is right-click on each method's name, then choose "Find Usages", as the following picture shows:

Find Usages

• Then the finding results will be displayed in the bottom, as the following picture shows:

Find Results

Implementation

• In this way we identified the following method from UserController.rb:

def self.participants_in(assignment_id)
 users = Array.new
 participants = AssignmentParticipant.find_by_parent_id(assignment_id)
 participants.each{
   |participant|
   users << User.find(participant.user_id)
 }
end

• And the following methods from User.rb

def assign_random_password
 if self.password.blank?
   self.password = self.random_password
 end
end

def self.random_password(size=8)
 random_pronouncable_password((size/2).round) + rand.to_s[2,3]
end

def get_author_name
 return self.fullname
end

• There's one method salt_first in User.rb cannot be deleted. Salt is a small chunk of random data to the password before it's hashed. The salt is then stored along with the hash in the database, and used to check potentially valid passwords<ref>Salt</ref>. Here's the function:

  def salt_first?
    true
  end

Change Rails 2 syntax to Rails 4

• One main problem(bug, related to user) in Expertiza is that you can create one new user from the UI, but the second time you try to create one, you get “undefined ‘with_scope’” (see Issue 504 in Github), as the following picture shows:

"With_Scope" Bug

• When calling @user.save method, it will implicitly call the ActiveRecord::Base#with_scope method. However, this method is no longer supported in current Rails 4.1.5 version<ref>Rails. GitHub. Jun. 29, 2010</ref>, which is used by Expertiza Rails 2 version. We had searched a lot through the Internet, but find very limited useful information on this topic. It doesn't seem reasonable for @user.save still calling the with_scope method, since all versions if Rails, as well as ActiveRecord, are marked as the latest in Gemfile.lock.

Refactor users_controller.rb

Refactor the “paginate_list” method

The "paginate_list" method is a function in "users_controller.rb". It will be called if you do search in Manage->Users page. It has two components: search for users, paginate the results.

However, the controller should not know how the information is retrieved. So we would like to refactor this method by seperating it into search method and paginating method. The search method is in model and the paginating method is still in controller.

The code below is the "paginate_list" method in official Expertiza:

# For filtering the users list with proper search and pagination.
def paginate_list(role, user_id, letter)
  paginate_options = {"1" => 25, "2" => 50, "3" => 100}

  # If the above hash does not have a value for the key,
  # it means that we need to show all the users on the page
  #
  # Just a point to remember, when we use pagination, the
  # 'users' variable should be an object, not an array

  #The type of condition for the search depends on what the user has selected from the search_by dropdown
  condition = "(role_id in (?) or id = ?) and name like ?" #default used when clicking on letters
  search_filter = letter + '%'
  @search_by = params[:search_by]
  if @search_by == '1'  #search by user name
    condition = "(role_id in (?) or id = ?) and name like ?"
    search_filter = '%' + letter + '%'
  elsif @search_by == '2' # search by full name
    condition = "(role_id in (?) or id = ?) and fullname like ?"
    search_filter = '%' + letter + '%'
  elsif @search_by == '3' # search by email
     condition = "(role_id in (?) or id = ?) and email like ?"
     search_filter = '%' + letter + '%'
  end

  if (paginate_options["#{@per_page}"].nil?) #displaying all - no pagination
    users = User.order('name').where( [condition, role.get_available_roles, user_id, search_filter]).paginate(:page => params[:page], :per_page => User.count)
  else #some pagination is active - use the per_page
    users = User.page(params[:page]).order('name').per_page(paginate_options["#{@per_page}"]).where([condition, role.get_available_roles, user_id, search_filter])
  end
  users
end

From the code we can see that the search method needs four parameters:

• role: user can only search users below his role
• user_id: current user id
• letter: keyword in search
• search_by: search by user name, full name or email

Base on this, we implemented the search method in "user.rb":

def self.search_users(role, user_id, letter, search_by)
  if search_by == '1'  #search by user name
    search_filter = '%' + letter + '%'
    users = User.order('name').where( "(role_id in (?) or id = ?) and name like ?", role.get_available_roles, user_id, search_filter )
  elsif search_by == '2' # search by full name
    search_filter = '%' + letter + '%'
    users = User.order('name').where( "(role_id in (?) or id = ?) and fullname like ?", role.get_available_roles, user_id, search_filter )
  elsif search_by == '3' # search by email
    search_filter = '%' + letter + '%'
    users = User.order('name').where( "(role_id in (?) or id = ?) and email like ?", role.get_available_roles, user_id, search_filter )
  else #default used when clicking on letters
    search_filter = letter + '%'
    users = User.order('name').where( "(role_id in (?) or id = ?) and name like ?", role.get_available_roles, user_id, search_filter )
  end
  users
end

And the original "paginate_list" method is modified as:

# For filtering the users list with proper search and pagination.
def paginate_list(role, user_id, letter)
  paginate_options = {"1" => 25, "2" => 50, "3" => 100}

  # If the above hash does not have a value for the key,
  # it means that we need to show all the users on the page
  #
  # Just a point to remember, when we use pagination, the
  # 'users' variable should be an object, not an array

  #The type of condition for the search depends on what the user has selected from the search_by dropdown
  @search_by = params[:search_by]

  # search for corresponding users
  users = User.search_users(role, user_id, letter, @search_by)

  # paginate
  if (paginate_options["#{@per_page}"].nil?) #displaying all - no pagination
    users = users.paginate(:page => params[:page], :per_page => User.count)
  else #some pagination is active - use the per_page
    users = users.page(params[:page]).per_page(paginate_options["#{@per_page}"])
  end
  users
end

New feature: delete users

Design

At the very beginning, we decide to use "Cascading Delete" to delete users. Because there are many relationship between different users. They can be reviewer, reviewee, teaching assistant, and so on.If we have to delete an user, we have to not only delete the record in user table, but also other related tables.

So, we divide all users into two set, one is new user without any relationship, the other is the old user with some relationships. And we find that it is quite easy to achieve the functionality of deleting new users. When deleting old users, we find some problems. Because old users may be a reviewer before and score some assignments. If we delete some old users, the assignments' review scores will be a mess.

After discussing with professor, we decide to deprecate"Cascading Delete". And we use below algorithm to handle user deletion.

Confirm box flow diagram

• A user can be deleted if (s)he has not participated in an assignment;
• If the user is participating in an assignment, the system will ask, “User is participating in k assignments. Delete as a participant in these assignment(s)?”;

Delete confirm box

• If the user has submitted or reviewed in any of these assignments, the system will say the user cannot be deleted, but offer to rename the user account to <current_account_name>_hidden;

Rename confirm box

• Rename (javascript calling update method in users_controller.rb);

Rename success

• If the person trying to delete does not want to rename the account, the system will just say that the user can’t be deleted.

Cannot delete

Implementation

We add a userDeleteConfirmBox.js file to implement this unique confirm box algorithm.

• First, we overwrite the Rails default confirm function.

$.rails.allowAction = function(link) {
  console.log(link.attr('data-relationship'))
  console.log(link.attr('data-username'))
  if ((!link.attr('data-confirm')) || (link.attr('data-relationship') && link.attr('data-relationship') == 'false')) {
   return true;
  }
  $.rails.showConfirmDialog(link);
  return false;
};
$.rails.confirmed = function(link) {
  link.removeAttr('data-confirm');
  return link.trigger('click.rails');
};

• Then, we write code to achieve customed confirm box using our own algorithm.

$.rails.showConfirmDialog = function(link) {
 var message = link.attr('data-confirm');
 //confirmation box style
 $(function() {
   $(html1).modal();
   $("#dialog-confirm1").dialog({
     width: 340,
     buttons: {
       "Cancel": function() {
         $( this ).dialog( "close" );
         location.reload();
       },
       "Yes": function() {
         //return $.rails.confirmed(link);
         $( this ).dialog( "close" );
         $(html2).modal();
         $("#dialog-confirm2").dialog({
           width: 340,
           buttons: {
             "Cancel": function() {
               $( this ).dialog( "close" );
               location.reload();
             },
             "Yes": function() {                 
                   $('#rename').click() 
                 $( this ).dialog( "close" );
                 $(html3).modal();
                 $("#dialog-confirm3").dialog({
                   width: 340,
                 });
             },
             "No, delete any way!": function() {
                 $( this ).dialog( "close" );
                 $(html4).modal();
                 $("#dialog-confirm4").dialog({
                     width: 340,
                     buttons: {
                       "Close": function() {
                         $( this ).dialog( "close" );
                         location.reload();
                       }
                     }
                 });
               }
             }
          });
        }
      }
    });
 });
 return $('#dialog-confirm .confirm').on('click', function() {
   return $.rails.confirmed(link);
 });
};

• After that, we all the if condition in /users/show.html.erb file. So when deleting the old users, Expertiza uses the functionality and customed confirm box we define; when deleting the new users, Expertiza will use the default confirm box.

 <% if @assignment_participant_num != 0 and (!@maps.nil? or @maps.length != 0)%> 
     <%= link_to 'Delete', {:action => 'destroy', :id => @user}, data:{:confirm => "User is participating in #{@assignment_participant_num} assignments.
Delete as a participant in these assignment(s)?", :relationship => 'true', :username => @user.name}, :method => :delete, remote: true%>   
 <% else %>
      <%= link_to 'Delete', {:action => 'destroy', :id => @user}, data:{:confirm => "User joins in #{@assignment_participant_num} assignment(s), but without participates in
any assignments. Delete as a participant in these assignment(s)?", :relationship => 'false'}, :method => :delete, remote: true%>
  <% end %>

Testing feature: search for users

Design

We write some features test to test "search for users" functionality.

• Mock two users in spec/features/helpers/login_helper.rb file, one is instructor, the other is student.

module LogInHelper
 def log_in(name, password)
   visit '/'
   expect(page).to have_content 'Login'
   fill_in 'User Name', with: name
   fill_in 'Password', with: password
   click_button 'Login'
   expect(page).to have_content "User: #{name}"
 end

 def student
   User.where(name: 'student').first || User.new({
     "name"=>"student",
     "crypted_password"=>"bd08fb03e2e3115964b1b39ea40625292a776a86",
     "role_id"=>1,
     "password_salt"=>"tQ6OGFiyL9dIlwxeSJf",
     "fullname"=>"Student, Perfect",
     "email"=>"pstudent@dev.null",
     "parent_id"=>1,
     "private_by_default"=>false,
     "mru_directory_path"=>nil,
     "email_on_review"=>true,
     "email_on_submission"=>true,
     "email_on_review_of_review"=>true,
     "is_new_user"=>false,
     "master_permission_granted"=>0,
     "handle"=>"",
     "leaderboard_privacy"=>false,
     "digital_certificate"=>nil,
     "timezonepref"=>"Eastern Time (US & Canada)",
     "copy_of_emails"=>false,
   })
 end

 def instructor
   User.where(name: 'instructor').first || User.new({
     name: "instructor",
     password: "password",
     password_confirmation: "password",
     role_id: 2,
     fullname: "Dole, Bob",
     email: "bdole@dev.null",
     parent_id: 2,
     private_by_default: false,
     mru_directory_path: nil,
     email_on_review: true,
     email_on_submission: true,
     email_on_review_of_review: true,
     is_new_user: false,
     master_permission_granted: 0,
     handle: "",
     leaderboard_privacy: false,
     digital_certificate: nil,
     public_key: nil,
     copy_of_emails: false,
   })
 end
end

• Instructor searches users by login names.

feature 'Method 1: Instructor search a user' do
 before(:all) do
   instructor.save
   student.save
   log_in instructor.name, "password"
 end
 scenario 'by login name' do
   visit '/users/list'
   fill_in 'letter', with: 'student'
   find('#search_by').select 'Username'
   click_button 'Search'
   expect(page).to have_content("Student, Perfect")
   expect(page).to have_content("pstudent@dev.null")
 end
end

• Instructor searches users by first name or last name.

feature 'Method2: Instructor search a user' do
 before(:all) do
   instructor.save
   student.save
   log_in instructor.name, "password"
 end
 scenario 'by last or first name' do
   visit '/users/list'
   fill_in 'letter', with: 'Bob'
   find('#search_by').select 'Full name'
   click_button 'Search'
   expect(page).to have_content("Dole, Bob")
   expect(page).to have_content("bdole@dev.null")
 end
end

• Instructor searches users by email.

feature 'Method3: Instructor search a user' do
 before(:all) do
   instructor.save
   student.save
   log_in instructor.name, "password"
 end
 scenario 'by email' do
   visit '/users/list'
   fill_in 'letter', with: 'bdole@dev.null'
   find('#search_by').select 'Email'
   click_button 'Search'
   expect(page).to have_content("Dole, Bob")
   expect(page).to have_content("bdole@dev.null")
 end
end

• Instructor deletes new users.

feature 'Instructor delete a user' do
 before(:all) do
   instructor.save
   student.save
   log_in instructor.name, "password"
 end
 scenario 'which has no relationship' do
   visit '/users/list'
   #in order to show whole user list
   fill_in 'letter', with: 
   find('#search_by').select 'Username'
   click_button 'Search'
   click_link 'student'
   click_link 'Delete'
   expect(page).to_not have_content("student")
   expect(page).to have_content("instructor")
 end
end

Implementation

rspec spec/features/users_spec.rb

Rspec feature test pass

References

<references/>