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

Integration Tests

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. Writing the test including writing stubs and mocks to ensure that the tests are fed what is required.

The following are the stubs and mocks written for the testing:

  success_response = Net::HTTPResponse.new(1.0, 200, "OK")
  current_round = 1
  stage = nil
  let(:assignment) { build(:assignment, instructor_id: 6) }
  let(:instructor) { build(:instructor, id: 6) }
  let(:participant) { build(:participant, id: 1, user_id: 6, assignment: assignment) }
  let(:review_response) { build(:response, id: 1, map_id: 1) }
  let(:review_response_map) { build(:review_response_map, id: 1, reviewer: participant) }
  let(:questionnaire) { build(:questionnaire, id: 1, questions: [question]) }
  let(:question) { Criterion.new(id: 1, weight: 2, break_before: true) }
  let(:assignment_questionnaire) { build(:assignment_questionnaire) }
  let(:answer) { double('Answer') }
  let(:assignment_due_date) { build(:assignment_due_date) }

  before(:each) do
    stub_current_user(instructor, instructor.role.name, instructor.role)
    allow(Assignment).to receive(:find).with('1').and_return(assignment)
    allow(Assignment).to receive(:find).and_return(assignment)
    allow(Response).to receive(:find).with('1').and_return(review_response)
    allow(Response).to receive(:find).and_return(review_response)
    allow(Response).to receive(:find_by).and_return(review_response)
    allow(ResponseMap).to receive(:find).with('1').and_return(review_response_map)
    allow(ResponseMap).to receive(:find).with(1).and_return(review_response_map)
    allow(Participant).to receive(:find).with(1).and_return(participant)
    allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
    allow(assignment).to receive(:number_of_current_round).and_return(current_round)
    allow(assignment).to receive(:get_current_stage).and_return(stage)
    allow(review_response).to receive(:delete).and_return(success_response)
    allow(review_response).to receive(:map).and_return(review_response_map)
    allow(review_response).to receive(:questionnaire_by_answer).and_return(questionnaire)
    allow(review_response_map).to receive(:assignment).and_return(assignment)
    allow(review_response_map).to receive(:save).and_return(true)
    allow(review_response_map).to receive(:questionnaire).with(current_round).and_return(questionnaire)
    request.env['HTTP_REFERER'] = 'www.google.com'
  end

The following are a few test cases written by the team:

describe '#edit' do
    it 'renders response#response page' do
      params = {id: review_response.id, return: ''}
      post :edit, params: params
      expect(response).to render_template("response")
    end
  end

describe '#pending_surveys' do
    context 'when session[:user] is nil' do
      it 'redirects to root path (/)' do
        get "pending_surveys"
        expect(response).to redirect_to('/')
      end
    end

    context 'when session[:user] is not nil' do
      it 'renders pending_surveys page' do
        session[:user] = participant
        get "pending_surveys"
        expect(response).to have_http_status(200)
        expect(response).to render_template("pending_surveys")
      end
    end
  end