CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

Introduction

Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

About response_controller.rb

The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

Tasks accomplished

1. Refactoring response_controller.rb

Included:

• Breaking down large methods into smaller chunks of readable reusable code.
• Changing code to adhere to latest Ruby conventions
• Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

Pull request

The pull request for this task can be viewed at [1]

Refactoring Explained

Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-

  def action_allowed?
    case params[:action]
    when 'edit' # If response has been submitted, no further editing allowed
      response = Response.find(params[:id])
      return false if response.is_submitted
      return current_user_id?(response.map.reviewer.user_id)
      # Deny access to anyone except reviewer & author's team
    when 'delete', 'update'
      response = Response.find(params[:id])
      return current_user_id?(response.map.reviewer.user_id)
    when 'view'
      response = Response.find(params[:id])
      map = response.map
      assignment = response.map.reviewer.assignment
      # if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
      if map.is_a? ReviewResponseMap
        reviewee_team = AssignmentTeam.find(map.reviewee_id)
        return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
      else
        return current_user_id?(response.map.reviewer.user_id)
      end
    else
      current_user
    end
  end

The view case is extracted into a separate method and some common variables have been extracted out of the cases

def action_allowed?
    response = user_id = nil
    action = params[:action]
    if %w(edit delete update view).include?(action)
      response = Response.find(params[:id])
      user_id = response.map.reviewer.user_id if (response.map.reviewer)
    end
    case action
    when 'edit' # If response has been submitted, no further editing allowed
      return false if response.is_submitted
      return current_user_id?(user_id)
      # Deny access to anyone except reviewer & author's team
    when 'delete', 'update'
      return current_user_id?(user_id)
    when 'view'
      return edit_allowed?(response.map, user_id)
    else
      current_user
    end
  end

  def edit_allowed?(map, user_id)
    assignment = map.reviewer.assignment
    # if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
    if map.is_a? ReviewResponseMap
      reviewee_team = AssignmentTeam.find(map.reviewee_id)
      return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
    else
      return current_user_id?(user_id)
    end
  end

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

@map = Response.find_by_map_id(params[:id])

Moving on with latest Rails conventions, function is being modified as

@map = Response.find_by(map_id: params[:id])

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

questions.sort {|a, b| a.seq <=> b.seq }

has been replaced with

questions.sort_by(&:seq)

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

  def pending_surveys
    unless session[:user] # Check for a valid user
      redirect_to '/'
      return
    end

    # Get all the participant(course or assignment) entries for this user
    course_participants = CourseParticipant.where(user_id: session[:user].id)
    assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

    # Get all the course survey deployments for this user
    @surveys = []
    if course_participants
      course_participants.each do |cp|
        survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
        next unless survey_deployments
        survey_deployments.each do |survey_deployment|
          next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
          @surveys <<
          [
            'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
            'survey_deployment_id' => survey_deployment.id,
            'start_date' => survey_deployment.start_date,
            'end_date' => survey_deployment.end_date,
            'parent_id' => cp.parent_id,
            'participant_id' => cp.id,
            'global_survey_id' => survey_deployment.global_survey_id
          ]
        end
      end
    end

    # Get all the assignment survey deployments for this user
    if assignment_participants
      assignment_participants.each do |ap|
        survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
        next unless survey_deployments
        survey_deployments.each do |survey_deployment|
          next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
          @surveys <<
          [
            'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
            'survey_deployment_id' => survey_deployment.id,
            'start_date' => survey_deployment.start_date,
            'end_date' => survey_deployment.end_date,
            'parent_id' => ap.parent_id,
            'participant_id' => ap.id,
            'global_survey_id' => survey_deployment.global_survey_id
          ]
        end
      end
    end
  end

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32. The refactored method is given below:-

  def pending_surveys
    unless session[:user] # Check for a valid user
      redirect_to '/'
      return
    end

    # Get all the course survey deployments for this user
    @surveys = []
    [CourseParticipant, AssignmentParticipant].each do |item|
      # Get all the participant(course or assignment) entries for this user
      participant_type = item.where(user_id: session[:user].id)
      next unless participant_type
      participant_type.each do |p|
        survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
        survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
        next unless survey_deployments
        survey_deployments.each do |survey_deployment|
          next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
          @surveys <<
              [
                'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
                'survey_deployment_id' => survey_deployment.id,
                'start_date' => survey_deployment.start_date,
                'end_date' => survey_deployment.end_date,
                'parent_id' => p.parent_id,
                'participant_id' => p.id,
                'global_survey_id' => survey_deployment.global_survey_id
              ]
        end
      end
    end
  end

Test Plan and Execution

23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality. These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

context 'when params action is view' do
      context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
        it 'allows certain action' do
          params = {action: "view", id: review_response.id}
          controller.params = params
          expect(controller.action_allowed?).to eq(true)
        end
      end
    end
<pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting <pre> params = {action: "view", id: review_response.id} 

.

To do so, we set these params and expect the function action_allowed to return true. The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

'What all was tested'

We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc and filled out the preexisting test cases present, which were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are either an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

'Stubs and mocks'

We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.