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

The problem statement for E 1907 can be viewed here https://docs.google.com/document/d/1SeIPiccoujhJZgdb3WzWBkg2v8TKSn5SwcHBeX9kzJQ/edit?usp=sharing

The pull request of our project can be viewed here https://github.com/expertiza/expertiza/pull/1414

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)
    ...
  end

Solution:

The edit_allowed? function was renamed to view_allowed? in the response_controller.rb file.

def view_allowed?(map, user_id)
    ...
  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.

The original code for the pending_surveys method in the response_controller.rb is shown below.

def pending_surveys
    unless session[:user] # Check for a valid user
      redirect_to '/'
      return
    end
    @surveys = []  # Get all the course survey deployments for this user
    [CourseParticipant, AssignmentParticipant].each do |participant_type| # Get all the participant(course or assignment) entries for this user
      participants = participant_type.where(user_id: session[:user].id)
      next unless participants
      participants.each do |p|
        survey_deployment_type = (participant_type == CourseParticipant ? CourseSurveyDeployment : AssignmentSurveyDeployment)
        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.zone.now > survey_deployment.start_date && Time.zone.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

Solution:

Moved the pending_surveys function to survey_deployment_controller.rb

Made changes in the following files and folders:

• app/controllers/survey_deployment_controller.rb (the pending_surveys method was moved to this file)

• app/controllers/response_controller.rb

Original -

elsif params[:return] == "survey"
      redirect_to controller: 'response_controller', action: 'pending_surveys'
else

New -

    elsif params[:return] == "survey"
      redirect_to controller: 'survey_deployment', action: 'pending_surveys'
    else

• config/routes.rb

Original -

collection do
      get :list
      get :reminder_thread
    end

New -

collection do
      get :list
      get :reminder_thread
      get :pending_surveys
    end

• spec/controllers/respose_controller_spec.rb

Original -

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

New -

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

• 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.

The original assign_instance_vars method is shown below:

# assigning the instance variables for Edit and New actions
  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

Solution:

The assign_instance_vars method was removed as it was not needed. The logic from this method was combined with the Edit and New sections as shown below.

The edit method with modifications.

  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.

The original code of the create method is shown below.

def create
    map_id = params[:id]
    map_id = params[:map_id] unless params[:map_id].nil? # pass map_id as a hidden field in the review form
    @map = ResponseMap.find(map_id)
    if params[:review][:questionnaire_id]
      @questionnaire = Questionnaire.find(params[:review][:questionnaire_id])
      @round = params[:review][:round]
    else
      @round = nil
    end
    is_submitted = (params[:isSubmit] == 'Yes')
    was_submitted = false
    # There could be multiple responses per round, when re-submission is enabled for that round.
    # Hence we need to pick the latest response.
    @response = Response.where(map_id: @map.id, round: @round.to_i).order(created_at: :desc).first
    if @response.nil?
      @response = Response.create(
        map_id: @map.id,
        additional_comment: params[:review][:comments],
        round: @round.to_i,
        is_submitted: is_submitted
      )
    end
    was_submitted = @response.is_submitted
    @response.update(additional_comment: params[:review][:comments], is_submitted: is_submitted) 
# ignore if autoupdate try to save when the response object is not yet created.

    # ,:version_num=>@version)
    # Change the order for displaying questions for editing response views.
    questions = sort_questions(@questionnaire.questions)
    create_answers(params, questions) if params[:responses]
    msg = "Your response was successfully saved."
    error_msg = ""
    # only notify if is_submitted changes from false to true
    if (@map.is_a? ReviewResponseMap) && (was_submitted == false && @response.is_submitted) && @response.significant_difference?
      @response.notify_instructor_on_difference
      @response.email
    end
    redirect_to controller: 'response', action: 'save', id: @map.map_id,
                return: params[:return], msg: msg, error_msg: error_msg, review: params[:review], save_options: params[:save_options]
  end

Solution:

The first instance of was_submitted was removed. This variable was renamed to previously_submitted in the other lines of the create method. The purpose of this variable is to determine if the response was previously submitted or the submitted response is a new response. These changes are shown in the code below.

  def create
    map_id = params[:id]
    map_id = params[:map_id] unless params[:map_id].nil? # pass map_id as a hidden field in the review form
    @map = ResponseMap.find(map_id)
    if params[:review][:questionnaire_id]
      @questionnaire = Questionnaire.find(params[:review][:questionnaire_id])
      @round = params[:review][:round]
    else
      @round = nil
    end
    is_submitted = (params[:isSubmit] == 'Yes')
    # There could be multiple responses per round, when re-submission is enabled for that round.
    # Hence we need to pick the latest response.
    @response = Response.where(map_id: @map.id, round: @round.to_i).order(created_at: :desc).first
    if @response.nil?
      @response = Response.create(
        map_id: @map.id,
        additional_comment: params[:review][:comments],
        round: @round.to_i,
        is_submitted: is_submitted
      )
    end
    previously_submitted = @response.is_submitted
    @response.update(additional_comment: params[:review][:comments], is_submitted: is_submitted) 
# ignore if autoupdate try to save when the response object is not yet created.

    # ,:version_num=>@version)
    # Change the order for displaying questions for editing response views.
    questions = sort_questions(@questionnaire.questions)
    create_answers(params, questions) if params[:responses]
    msg = "Your response was successfully saved."
    error_msg = ""
    # only notify if is_submitted changes from false to true
    if (@map.is_a? ReviewResponseMap) && (previously_submitted == false && @response.is_submitted) && @response.significant_difference?
      @response.notify_instructor_on_difference
      @response.email
    end
    redirect_to controller: 'response', action: 'save', id: @map.map_id,
                return: params[:return], msg: msg, error_msg: error_msg, review: params[:review], save_options: params[:save_options]
  end

Task 5

Problem:

The private methods have few to no comments.

Solution:

Comments were added to the private methods as shown below.

# assigning the instance variables for Edit and New actions
  def assign_instance_vars

# identifying the questionnaire type
# updating the current round for the reviewer's responses
  def set_questionnaire_for_new_response

# maps questions based on their id to its corresponding to its review score
  def scores

# if user is not filling a new rubric, the @response object should be available.
# we can find the questionnaire from the question_id in answers
  def set_questionnaire

# checks if the questionnaire is nil and opens drop down or rating accordingly
  def set_dropdown_or_scale

# sorts by sequence number
  def sort_questions(questions)

def create_answers(params, questions)
    # create score if it is not found. If it is found update it otherwise update it.

 def init_answers(questions)
    questions.each do |q|
      # it's unlikely that these answers exist, but in case the user refresh the browser some might have been inserted.

Task 6

Problem:

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

Originally, the redirect method was :-

 def redirect
    error_id = params[:error_msg]
    message_id = params[:msg]
    flash[:error] = error_id unless error_id and error_id.empty?
    flash[:note] = message_id unless message_id and message_id.empty?
    @map = Response.find_by(map_id: params[:id])
    if params[:return] == "feedback"
      redirect_to controller: 'grades', action: 'view_my_scores', id: @map.reviewer.id
    elsif params[:return] == "teammate"
      redirect_to view_student_teams_path student_id: @map.reviewer.id
    elsif params[:return] == "instructor"
      redirect_to controller: 'grades', action: 'view', id: @map.response_map.assignment.id
    elsif params[:return] == "assignment_edit"
      redirect_to controller: 'assignments', action: 'edit', id: @map.response_map.assignment.id
    elsif params[:return] == "selfreview"
      redirect_to controller: 'submitted_content', action: 'edit', id: @map.response_map.reviewer_id
    elsif params[:return] == "survey"
      redirect_to controller: 'survey_deployment', action: 'pending_surveys'
    else
      redirect_to controller: 'student_review', action: 'list', id: @map.reviewer.id
    end
  end

Solution:

A local variable was created to hold the description of the action to be taken by the redirect method. The if-else ladder was transformed into a switch (case) statement to make the code more legible.

The modifications to the redirect method are shown below.

def redirect
    error_id = params[:error_msg]
    message_id = params[:msg]
    flash[:error] = error_id unless error_id and error_id.empty?
    flash[:note] = message_id unless message_id and message_id.empty?
    @map = Response.find_by(map_id: params[:id])
    case params[:return]
    when "feedback"
      redirect_to controller: 'grades', action: 'view_my_scores', id: @map.reviewer.id
    when "teammate"
      redirect_to view_student_teams_path student_id: @map.reviewer.id
    when "instructor"
      redirect_to controller: 'grades', action: 'view', id: @map.response_map.assignment.id
    when "assignment_edit"
      redirect_to controller: 'assignments', action: 'edit', id: @map.response_map.assignment.id
    when "selfreview"
      redirect_to controller: 'submitted_content', action: 'edit', id: @map.response_map.reviewer_id
    when "survey"
      redirect_to controller: 'survey_deployment', action: 'pending_surveys'
    else
      redirect_to controller: 'student_review', action: 'list', id: @map.reviewer.id
    end
  end

Test Plan

The test file used for the response_controller.rb was located in the */spec/controllers/response_controller_spec.rb. This test was previously created from a different project. However, the test file did have to be modified since the pending_surveys method was removed. The following snippets of code show the modifications to the test file.

Original content:

 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

Modified content:

    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

There were no observed failures after performing the modified test.

Code Climate and Travis CI Results

After completing the changes to the code, the code modifications were committed to GitHub. Then, a pull request was performed on the committed code. During this process, two automated bots were used to analyze the code. These bots were from Code Climate and from Travis CI.

The Code Climate bot did not detect any major errors with the code. However, it did comment that the code complexity was too high for some methods. Since our team did not make any changes to these sections of code, no additional changes were made to the code to resolve the complexity concerns. The logic for the code appears to be working and it was uncertain how the logic could be further reduced to lower the code complexity score.

The second bot, Travis Ci, performed four different analysis cycles on the committed code. It reported that the code passed its inspection with no issues.

References

1. The problem statement for E 1907 can be viewed here https://docs.google.com/document/d/1SeIPiccoujhJZgdb3WzWBkg2v8TKSn5SwcHBeX9kzJQ/edit?usp=sharing

2. The pull request for this assignment can be viewed at: https://github.com/expertiza/expertiza/pull/1414

3. Team Branches: All the three team members have contributed towards the project and their branches can be viewed here https://github.com/Anusha-Godavarthi/expertiza/branches

4. Screencast: https://docs.google.com/presentation/d/1AAc95U7vq-zJ8mgD4lVvoQzn5Dui75cMKCoAyjcRm8c/edit?usp=sharing

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

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

Team Members

1. Anusha Godavarithi

2. Michael Lewallen

3. Akhil Pabba