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.

Problem Statement

The following tasks were accomplished in this project:

• Complete the pending tests in user_spec.rb
• Refactor get_user_list method
• Refactor self.search_users method
• Rename method and change all other places it is used
• Use find_by instead of dynamic method

Current Implementation

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.