Spring2019 E1907 refactor response controller

Introduction

Expertiza is an open source webapp that was 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 maintained by students and faculty at NCSU.

Project Summary

The file response_controller.rb handles the operations on responses based on user permissions. The user is redirected to the appropriate place on Expertiza after the action is complete. The responses are from the completion of peer reviews and questionnaires. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

Project Tasks

Task 1

Problem:

edit_allowed function The function name is misleading - it checks if the response can be viewed by a person or not. Refactor to a more appropriate name.

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.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
      current_user_id?(user_id)
    end
  end

Solution:

Renamed edit_allowed function to view_allowed in the response_controller.rb file

def view_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.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
      current_user_id?(user_id)
    end
  end

Task 2

Problem:

Move the pending_surveys function to survey_deployment_contoller.rb. Ensure that moving the function does not break the code functionalities.

Solution:

Moved the pending_surveys function to survey_deployment_controller.rb

Made changes in the following files and folders:

• app/controllers/response_controller.rb
• app/controllers/survey_deployment_controller.rb
• config/routes.rb
• spec/controllers/respose_controller_spec.rb
• Moved pending_surveys.html.erb from views/response to views/survey_deployment

Task 3

Problem:

The assign_instance_variables method violates several principles - The method is doing two things, and the method is not really needed at all. It is called only in two places and does entirely different things based on where it is called from.

Solution:

Removed assign_instance_vars section as it was not needed and combined in Edit and New sections

Original assign_instance_vars method :-

def assign_instance_vars
    case params[:action]
    when 'edit'
      @header = 'Edit'
      @next_action = 'update'
      @response = Response.find(params[:id])
      @map = @response.map
      @contributor = @map.contributor
    when 'new'
      @header = 'New'
      @next_action = 'create'
      @feedback = params[:feedback]
      @map = ResponseMap.find(params[:id])
      @modified_object = @map.id
    end
    @return = params[:return]
  end

Moved the assign_instance_vars for the Edit action to the edit method.

  def edit
    # instance variables for Edit action
    @header = 'Edit'
    @next_action = 'update'
    @response = Response.find(params[:id])
    @map = @response.map
    @contributor = @map.contributor
    @prev = Response.where(map_id: @map.id)
    @review_scores = @prev.to_a
    if @prev.present?
      @sorted = @review_scores.sort {|m1, m2| m1.version_num.to_i && m2.version_num.to_i ? m2.version_num.to_i <=>  
m1.version_num.to_i : (m1.version_num ? -1 : 1) }
      @largest_version_num = @sorted[0]
    end
    @modified_object = @response.response_id
    # set more handy variables for the view
    set_content
    @review_scores = []
    @questions.each do |question|
      @review_scores << Answer.where(response_id: @response.response_id, <br> question_id: question.id).first
    end
    @questionnaire = set_questionnaire
    render action: 'response'
  end

Moved assign_instance_vars for the New action inside the new method : -

def new
    # instance variable for New action
    @header = 'New'
    @next_action = 'create'
    @feedback = params[:feedback]
    @map = ResponseMap.find(params[:id])
    @modified_object = @map.id
    set_content(true)
    @stage = @assignment.get_current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id)) if @assignment
      team = AssignmentTeam.find(@map.reviewee_id)
    @response = Response.where(map_id: @map.id, round: @current_round.to_i).order(updated_at: :desc).first
    if @response.nil? || team.most_recent_submission.updated_at > @response.updated_at
      @response = Response.create(map_id: @map.id, additional_comment: '', round: @current_round, is_submitted: 0)
    end
    questions = sort_questions(@questionnaire.questions)
    init_answers(questions)
    render action: 'response'
  end

Task 4

Problem:

In the create() code in create method, especially the was_submitted and is_submitted part, is hard to understand. It can be cleaned up.

Solution:

Removed was_submitted. Changed to previously_submitted in the create method.

PASTE CODE HERE

Task 5

Problem:

The private methods have few to no comments.

Solution:

Comments were added to the private methods as shown below.

PASTE CODE WITH COMMENTS HERE

Task 6

Problem:

Redirect method has a if-else ladder. See if it can be refactored to something more understandable.

Solution:

Introduced temporary explaining variables in the redirect method

PASTE CODE HERE (SWITCH STATEMENT)

Test Plan

(Please re-word and expand this thought.)

Because of moving the pending_surveys method, some of the original spec tests failed. So we fixed it.

context 'when params[:return] is survey' do
     it 'redirects to survey_deployment#pending_surveys page' do
       @params[:return] = 'survey'
       get :redirect, @params
       expect(response).to redirect_to('/survey_deployment/pending_surveys')
     end
   end

   context 'when params[:return] is other content' do
     it 'redirects to student_review#list page' do
       @params[:return] = 'other'
       get :redirect, @params
       expect(response).to redirect_to('/student_review/list?id=1')
     end
   end

Code Climate and Travis CI

There are only cyclomatic, cognitive errors with code climate whereas there are no integration errors with Travis CI

References

1. The pull request for this task can be viewed at: https://github.com/expertiza/expertiza/pull/1383

2. GitHub Repo: https://github.com/Anusha-Godavarthi/expertiza

3. Team Branches: https://github.com/Anusha-Godavarthi/expertiza/branches

4. Expertiza Main Repo https://github.com/expertiza/expertiza

5. Expertiza Documentation http://wiki.expertiza.ncsu.edu/index.php/Main_Page

Team Members

1. Anusha Godavarithi

2. Michael Lewallen

3. Akhil Pabba