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

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.

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.

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:

Removed was_submitted. Changed to previously_submitted in the create method.

  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:

Introduced temporary explaining variables in the redirect method. Changed the if-else ladder to a switch case.

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

(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 assignment can be viewed at: https://github.com/expertiza/expertiza/pull/1414

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

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

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

Team Members

1. Anusha Godavarithi

2. Michael Lewallen

3. Akhil Pabba