CSC/ECE 517 Fall 2014/oss E1458 sst

Expertiza - Refactoring ResponseController

Project Description

The response controller allows the user to create and edit responses to questionnaires such as performing a review, rating a teammate or giving feedback to a reviewer. Our project requirement was to perform the following changes :

• Perform authorization properly.
• Remove the duplicated methods.
• Reduce the complexity of the rereview method.
• Move the functionality incorporated in the controller, to the model, as it is the models responsibility to implement this functionality.

Refactoring carried out

The following changes have been made in the project, as described in the requirements document.

Perform Authorization correctly

• Authorization to perform actions was not checked correctly. It is supposed to be done through the action_allowed? method at the beginning of the class definition. Different authorizations are required for different operations. For example, someone should be allowed to view a response if they wrote the response, or they are the person or on the team whose work the response applied to, or if they are an instructor or TA for the class. The person who wrote a response should be allowed to edit it, but not the person/team who was being reviewed, nor the instructor or TA for the class.
• Earlier, the authorization was denied by the redirect_when_disallowed method, which was a more error-prone way of controlling access. This method has now been removed, and now the class has an action_allowed? Method which does the authorization check and allows the user to perform the action if it is allowed.

Before Refactoring:

redirect_when_disallowed Method was used for authorization purposes.

def redirect_when_disallowed(response)
    # For author feedback, participants need to be able to read feedback submitted by other teammates.
    # If response is anything but author feedback, only the person who wrote feedback should be able to see it.
    if response.map.read_attribute(:type) == 'FeedbackResponseMap' && response.map.assignment.team_assignment?
      team = response.map.reviewer.team
      unless team.has_user session[:user]
        redirect_to '/denied?reason=You are not on the team that wrote this feedback'
      else
        return false
      end
      response.map.read_attribute(:type)
    end
    !current_user_id?(response.map.reviewer.user_id)
end

After Refactoring:

  def action_allowed?
    case params[:action]
      when 'view', 'edit', 'delete', 'rereview', 'update'
        if response.map.read_attribute(:type) == 'FeedbackResponseMap'
          team = response.map.reviewer.team
          if team.has_user session[:user]
           flag= true
          else
            flag=false
          end
        end
      else
        flag=true
    end
    return flag
  end

Removal of redundant methods

• There were two copies of the edit, new_feedback and view methods. The second being the newer one, and, according to the rules for method definition, is the one that is currently in use because the latest version overrides the previous versions. We refactored the code by removing the redundant methods for edit, new_feedback and view.

Cleaning up of the rereview moethod

• The rereview method was 98 lines long. We refactored the code by turning several parts of it into methods. Now the code is 81 lines long.

Before Refactoring:

def rereview
    @map=ResponseMap.find(params[:id])
    get_content
    array_not_empty=0
    @review_scores=Array.new
    @prev=Response.all
    #get all versions and find the latest version
    for element in @prev
      if (element.map.id==@map.map.id)
        array_not_empty=1
        @review_scores << element
      end
    end

    latestResponseVersion
    #sort all the available versions in descending order.
    if @prev.present?
      @sorted=@review_scores.sort { |m1, m2| (m1.version_num and m2.version_num) ? m2.version_num <=> m1.version_num : (m1.version_num ? -1 : 1) }
      @largest_version_num=@sorted[0]
      @latest_phase=@largest_version_num.created_at
      due_dates = DueDate.where(["assignment_id = ?", @assignment.id])
      @sorted_deadlines=Array.new
      @sorted_deadlines=due_dates.sort { |m1, m2| (m1.due_at and m2.due_at) ? m1.due_at <=> m2.due_at : (m1.due_at ? -1 : 1) }
      current_time=Time.new.getutc
      #get the highest version numbered review
      next_due_date=@sorted_deadlines[0]
      #check in which phase the latest review was done.
      for deadline_version in @sorted_deadlines
        if (@largest_version_num.created_at < deadline_version.due_at)
          break
        end
      end
      for deadline_time in @sorted_deadlines
        if (current_time < deadline_time.due_at)
          break
        end
      end
    end
    #check if the latest review is done in the current phase.
    #if latest review is in current phase then edit the latest one.
    #else create a new version and update it.
    # editing the latest review
    if (deadline_version.due_at== deadline_time.due_at)
      #send it to edit here
      @header = "Edit"
      @next_action = "update"
      @return = params[:return]
      @response = Response.find_by_map_id_and_version_num(params[:id], @largest_version_num.version_num)
      return if redirect_when_disallowed(@response)
      @modified_object = @response.response_id
      @map = @response.map
      get_content
      @review_scores = Array.new
      @questions.each {
          |question|
        @review_scores << Score.find_by_response_id_and_question_id(@response.response_id, question.id)
      }
      #**********************
      # Check whether this is Jen's assgt. & if so, use her rubric
      if (@assignment.instructor_id == User.find_by_name("jace_smith").id) && @title == "Review"
        if @assignment.id < 469
          @next_action = "update"
          render :action => 'custom_response'
        else
          @next_action = "update"
          render :action => 'custom_response_2011'
        end
      else
        # end of special code (except for the end below, to match the if above)
        #**********************
        render :action => 'response'
      end
    else
      #else create a new version and update it.
      @header = "New"
      @next_action = "create"
      @feedback = params[:feedback]
      @map = ResponseMap.find(params[:id])
      @return = params[:return]
      @modified_object = @map.map_id
      get_content
      #**********************
      # Check whether this is Jen's assgt. & if so, use her rubric
      if (@assignment.instructor_id == User.find_by_name("jace_smith").id) && @title == "Review"
        if @assignment.id < 469
          @next_action = "create"
          render :action => 'custom_response'
        else
          @next_action = "create"
          render :action => 'custom_response_2011'
        end
      else
        # end of special code (except for the end below, to match the if above)
        #**********************
        render :action => 'response'
      end
    end
  end

After Refactoring:

def rereview
   @map=ResponseMap.find(params[:id])
    get_content
    array_not_empty=0
    @review_scores=Array.new
    @prev=Response.all
    #get all versions and find the latest version
    for element in @prev
      if (element.map.id==@map.map.id)
        array_not_empty=1
        @review_scores << element
      end
    end

    @review_scores=response.getAllResponseVersions
    #sort all the available versions in descending order.
    if @prev.present?
      @largest_version_num = Response.get_largest_version_number(@review_scores)
      @latest_phase=@largest_version_num.created_at
      due_dates = DueDate.where(["assignment_id = ?", @assignment.id])

      @sorted_deadlines = DueDate.sort_deadlines(due_dates)
      current_time=Time.new.getutc
      #get the highest version numbered review
      next_due_date=@sorted_deadlines[0]
      #check in which phase the latest review was done.
      for deadline_version in @sorted_deadlines
        if (@largest_version_num.created_at < deadline_version.due_at)
          break
        end
      end
      for deadline_time in @sorted_deadlines
        if (current_time < deadline_time.due_at)
          break
        end
      end
    end
    #check if the latest review is done in the current phase.
    #if latest review is in current phase then edit the latest one.
    #else create a new version and update it.
    # editing the latest review
    if (deadline_version.due_at== deadline_time.due_at)
      #send it to edit here
      @header = "Edit"
      @next_action = "update"
      @return = params[:return]
      @response = Response.where(map_id: params[:id], version_num:  @largest_version_num.version_num).first
      @modified_object = @response.response_id
      @map = @response.map
      get_content
      @review_scores = Array.new
      @questions.each {
          |question|
          @review_scores << Score.where(response_id: @response.response_id, question_id:  question.id).first
      }
      #**********************


      # Check whether this is Jace's assgt. & if so, use her rubric
      if (check_user_name_jace? && @title == "Review")
          handle_jace_kludge
      else
        # end of special code (except for the end below, to match the if above)
        #**********************
        render :action => 'response'
      end

    else
      #else create a new version and update it.
      @header = "New"
      @next_action = "create"
      @feedback = params[:feedback]
      @map = ResponseMap.find(params[:id])
      @return = params[:return]
      @modified_object = @map.map_id
      get_content
      #**********************
      # Check whether this is Jen's assgt. & if so, use her rubric
      if (check_user_name_jace? && @title == "Review")
        handle_jace_kludge
      else
        # end of special code (except for the end below, to match the if above)
        #**********************
        render :action => 'response'
      end
    end
  end

Creation of a Kludge

• The rereview method contained a special code to check whether an assignment is “Jen’s assignment”; this was the first assignment that was ever created with a multipart rubric. It was hard-coded into the system, rather than working on a rubric that was created in the normal way. It is impossible to remove this code without breaking that assignment. It is now implemented as a separate method named handle_jace_kludge.

Before Refactoring:

The following code was present in the rereview method.

 #**********************
 # Check whether this is Jen's assgt. & if so, use her rubric
      if (@assignment.instructor_id == User.find_by_name("jace_smith").id) && @title == "Review"
        if @assignment.id < 469
          @next_action = "update"
          render :action => 'custom_response'
        else
          @next_action = "update"
          render :action => 'custom_response_2011'
        end
      else
        # end of special code (except for the end below, to match the if above)
        #**********************
        render :action => 'response'
      end     

After Refactoring:

We added a method named 'handle_jace_kludge'.

  def handle_jace_kludge
    # ** if assignment belongs to Jace handle it depending on the assignment id **
    if @assignment.id < 469
      @next_action = "update"
      render :action => 'custom_response'
    else
      @next_action = "update"
      render :action => 'custom_response_2011'
    end
  end

Moved methods from Response Controller to appropriate models

• /*TODO for wiki Sorting review versions is not a controller responsibility; it would be better to do this in a model class (which class?) Ditto for determining whether a review is current (i.e., was done during the current assignment phase). This is a query that is made about a review (actually, about a response, which may be a review, author feedback, etc.). It should be placed in the appropriate model class.*/