CSC/ECE 517 Fall 2017/E1746 Refactor user.rb

E1746 [Test First Development] Refactor user.rb

This page provides a description of the Expertiza based OSS project.

About Expertiza

Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

Test Plan

The following tasks were accomplished in this project:

• Complete the pending tests in user_spec.rb

There are many pending tests needed to be finished. And for the methods which are needed to be refactored or renamed, this is also can been seen as writing failing tests first.

• Refactor get_user_list method.

The get_user_list methods contain many conditions and each of them has a lot of code which makes the method very long. The factor is to transfer the statements to the corresponding subclasses ,and writing method calls instead.

• Refactor self.search_users method

The method has many repetitive codes which are used in the conditionals. We refactor it by writing a single method call and let the method call to choose the conditional.

• Rename methods and change all other places it is used.
• Use find_by instead of dynamic method.

The find_by_name or find_by_email method which are used in the user.rb is like old style code. And this kind of code can be replaced with new dynamic ones like, find_by.

Missions and Solutions

Mission 1:Complete the pending tests in user_spec.rb

There are many pending tests in the file 'user.rb'. What we have done is finishing these pending tests and makes them pass.The user_spec.rb file on github is below.

https://github.com/AnranZhou/expertiza/blob/master/spec/models/user_spec.rb

Mission 2:Refactor get_user_list method

The task is to write failing test first and ,move if conditions to corresponding subclasses (eg. ta.rb, instructor.rb) with same method name,then replace the conditional with the relevant method calls.

First writing the failing test first according to the requirements of the refactor.

#describe '#get_user_list' do
    context 'when current user is super admin' do
      it 'fetches all users' do
        allow(user).to receive_message_chain("role.super_admin?"){ true }
        allow(user).to receive_message_chain("role.instructor?"){ false }
        allow(user).to receive_message_chain("role.ta?"){false}
        allow(SuperAdministrator).to receive(:get_user_list).and_return([user1,user2])
        expect(user.get_user_list).to eq ([user1,user2])
      end
  end
    context 'when current user is an instructor' do
      it 'fetches all users in his/her course/assignment' do
        allow(user).to receive_message_chain("role.super_admin?"){ false }
        allow(user).to receive_message_chain("role.instructor?"){ true }
        allow(user).to receive_message_chain("role.ta?"){false}
        allow(Instructor).to receive(:get_user_list).and_return([user1,user2])
        expect(user.get_user_list).to eq ([user1,user2])
      end
    end

    context 'when current user is a TA' do
      it 'fetches all users in his/her courses'do
        allow(user).to receive_message_chain("role.super_admin?"){ false }
        allow(user).to receive_message_chain("role.ta?"){ true }
        allow(user).to receive_message_chain("role.instructor?"){ false }
        allow(Ta).to receive(:get_user_list).and_return([user1,user2])
        expect(user.get_user_list).to eq ([user1,user2])
    end
    end
  end

The next step is to move if conditionals to its relatively subclasses. In this case,the superadministrator.rb, instructor.rb and ta.rb.

#class SuperAdministrator < User
  def self.get_user_list
    user_list=[]
    User.all.find_each do |user|
    user_list << user
    end
  end
end

  #def self.get_user_list
    user_list=[]
    participants = []
    Course.where(instructor_id: self.id).find_each do |course|
      participants << course.get_participants
    end
    Assignment.where(instructor_id: self.id).find_each do |assignment|
      participants << assignment.participants
    end
    participants.each do |p_s|
      next if p_s.empty?
      p_s.each do |p|
        user_list << p.user if self.role.hasAllPrivilegesOf(p.user.role)
      end
    end
end

#def self.get_user_list
  get_user_list=[]
  courses.each do |course_id|
      course = Course.find(course_id)
      participants << course.get_participants
    end
    participants.each do |p_s|
      next if p_s.empty?
      p_s.each do |p|
        user_list << p.user if self.role.hasAllPrivilegesOf(p.user.role)
      end
    end
end

Then replace the conditional with the relevant method calls.

#def get_user_list
    user_list = []
    # If the user is a super admin, fetch all users
    if self.role.super_admin?
       return SuperAdministrator.get_user_list
    end


    # If the user is an instructor, fetch all users in his course/assignment
    if self.role.instructor?
      return Instructor.get_user_list
    end

    # If the user is a TA, fetch all users in his courses
    if self.role.ta?
      return Ta.get_user_list
    end

    # Add the children to the list
    unless self.role.super_admin?
      User.all.find_each do |u|
        if is_recursively_parent_of(u)
          user_list << u unless user_list.include?(u)
        end
      end
    end

    user_list.uniq
  end

So the refactor of get_user_list method has been done.

Mission 3:Refactor self.search_users method.

This task is required to write the failing test first and extract duplicated code to a new method.

First, write the failing test.

 #descirbe '.search_users' do

    let(:role) { Role.new }
    before(:each) do
        allow(User).to receive_message_chain(:order,:where).and_return(user)
    end

    it 'when the search_by is 1' do
      search_by = "1"
      user_id = double
      letter = 'name'
      search_filter = '%' + letter + '%'
      expect(User).to receive_message_chain(:order,:where).with("(role_id in (?) or id = ?) and name like ?", role.get_available_roles, user_id, search_filter)
      expect(User.search_users(role,user_id,letter,search_by)).to eq user
    end

    it 'when the search_by is 2' do
      search_by = "2"
      user_id = double
      letter = 'fullname'
      search_filter = '%' + letter + '%'
      expect(User).to receive_message_chain(:order,:where).with("(role_id in (?) or id = ?) and fullname like ?", role.get_available_roles, user_id, search_filter)
      expect(User.search_users(role,user_id,letter,search_by)).to eq user
    end

    it 'when the search_by is 3' do
      search_by = "3"
      user_id = double
      letter = 'email'
      search_filter = '%' + letter + '%'
      expect(User).to receive_message_chain(:order,:where).with("(role_id in (?) or id = ?) and email like ?", role.get_available_roles, user_id, search_filter)
      expect(User.search_users(role,user_id,letter,search_by)).to eq user
    end

    it 'when the search_by is default value' do
      search_by = nil
      user_id = double
      letter = ''
      search_filter = letter + '%'
      expect(User).to receive_message_chain(:order,:where).with("(role_id in (?) or id = ?) and name like ?", role.get_available_roles, user_id, search_filter)
      expect(User.search_users(role,user_id,letter,search_by)).to eq user
    end
  end

Then,extract duplicated code to a new method.Since this is kind of changing original method.So below is the changing on github.

#   def self.search_users(role, user_id, letter, search_by)
 -    if search_by == '1' # search by user name
 +    key_word = {'1'=>'name','2'=>'fullname','3'=>'email'}
 +    if key_word.include? search_by
        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)
 +      users = User.order('name').where("(role_id in (?) or id = ?) and #{key_word[search_by]} 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)

Mission 4:Rename method and change all other places it is used

First, is to write failing test of the renamed method which has been finished in mission one.So just skip to the second part to rename them.

is_recursively_parent_of → recursively_parent_of?

  -       expect(user1.is_recursively_parent_of(user)).to eq false
  +       expect(user1.recursively_parent_of(user)).to eq false

is_creator_of? → creator_of?

 -  describe '#is_creator_of?' do
 +  describe '#creator_of?' do
      it 'returns true of current user (user) is the creator of target user (user1)' do
        allow(user1).to receive(:creator).and_return(user)
 -      expect(user.is_creator_of?(user1)).to be true
 +      expect(user.creator_of?(user1)).to be true
      end
  
      it 'returns false of current user (user) is not the creator of target user (user1)' do
        allow(user1).to receive(:creator).and_return(user2)
 -      expect(user.is_creator_of?(user1)).to be false
 -      expect(user2.is_creator_of?(user1)).to be true
 +      expect(user.creator_of?(user1)).to be false
 +      expect(user2.creator_of?(user1)).to be true
  
      end
    end
 @@ -382,16 +382,16 @@
      end
    end

is_teaching_assistant_for? → teaching_assistant_for?

 -  describe '#is_teaching_assistant_for?' do
 +  describe '#teaching_assistant_for?' do
      it 'returns false if current user is not a TA' do
        allow(user1).to receive_message_chain("role.ta?"){ false }
 -      expect(user1.is_teaching_assistant_for?(user)).to be_falsey
 +      expect(user1.teaching_assistant_for?(user)).to be_falsey
      end
  
      it 'returns false if current user is a TA, but target user is not a student'do
        allow(user1).to receive_message_chain("role.ta?"){true }
        allow(user).to receive_message_chain("role.name").and_return('teacher')
 -      expect(user1.is_teaching_assistant_for?(user)).to be_falsey
 +      expect(user1.teaching_assistant_for?(user)).to be_falsey
      end
  
      it 'returns true if current user is a TA of target user'do
 @@ -401,23 +401,23 @@
      c1=Course.new
      allow(user1).to receive_message_chain(:courses_assisted_with,:any?).and_yield(c1)
      allow_any_instance_of(Course).to receive_message_chain(:assignments,:map,:flatten,:map,:include?,:user).and_return(true)
 -    expect(user1.is_teaching_assistant_for?(user)).to be true
 +    expect(user1.teaching_assistant_for?(user)).to be true
      end
    end
  
 

is_teaching_assistant? → teaching_assistant?

  -  describe '#is_teaching_assistant?' do
  +  describe '#teaching_assistant?' do
      it 'returns true if current user is a TA' do
        allow(user).to receive_message_chain("role.ta?"){ true }
 -      expect(user.is_teaching_assistant?).to be true
 +      expect(user.teaching_assistant?).to be true
      end
  
      it 'returns false if current user is not a TA' do
        allow(user).to receive_message_chain("role.ta?"){ false }
 -      expect(user.is_teaching_assistant?).to be_falsey
 +      expect(user.teaching_assistant?).to be_falsey
      end
    end

After writing the test, it is going to rename the method in the user.rb. Below is the changed part of the code.

 -    return true if self.is_teaching_assistant_for?(user)
 -    return true if self.is_recursively_parent_of(user)
 +    return true if self.teaching_assistant_for?(user)
 +    return true if self.recursively_parent_of(user)
 -    def is_recursively_parent_of(user)
 +    def recursively_parent_of(user)
 -    self.is_recursively_parent_of(p)
 +    self.recursively_parent_of(p)
 -    if is_recursively_parent_of(u)
 +    if recursively_parent_of(u)
 -    def is_creator_of?(user)
 +    def creator_of?(user)
 -    def is_teaching_assistant_for?(student)
 -    return false unless is_teaching_assistant?
 +    def teaching_assistant_for?(student)
 +    return false unless teaching_assistant?
 -    def is_teaching_assistant?
 +    def teaching_assistant?

Mission 5:Use find_by instead of dynamic method

：：There are three lines using find_by_name and find_by_email which are now be instead of find_by[params].The task is to replace the old one to refactor the method.

First,writing the failing tests,here is the original test code including find_by method.

  allow(User).to receive(:find_by_name).and_return(user)

And below is the new failing test

  allow(User).to receive(:find_by).and_return(user)

And so as the other codes contain find_by method

 -      allow(User).to receive(:find_by).with(email).and_return(user)
 +      allow(User).to receive(:find_by).and_return(user)
 -      allow(User).to receive(:find_by_email).and_return(nil)
 +      allow(User).to receive(:find_by).and_return(nil)
 -      allow(User).to receive(:find_by_name).and_return(user)
 +      allow(User).to receive(:find_by).and_return(user)

And then change the find_by_name and find_by_name in the user.rb file in to dynamic one.

 -      user = User.find_by_name(row[0])
 +      user = User.find_by name: row[0]
 -      user = User.find_by_email(login)
 +      user = User.find_by email: login
 -      User.find_by_name(params[:user][:name])
 +      User.find_by name: params[:user][:name]

References

1. Expertiza on GitHub
2. GitHub Project Repository Fork
3. The live Expertiza website
4. Demo link
5. Expertiza project documentation wiki
6. Rspec Documentation
7. Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin