CSC/ECE 517 Fall 2015/oss E1570 avr: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
No edit summary
Line 253: Line 253:


==Removing duplicate methods==
==Removing duplicate methods==
The ResponseController allows the user to create and edit responses to questionnaires, and the functionality changed over time, causing two new_feedback and view methods to be created; the oldest of which doesn't get called anymore.
Anush For u to write here...
 
{| class="wikitable"
{| class="wikitable"
|-
|-
Line 342: Line 341:
</pre>
</pre>
|}
|}
==Moving code to the proper place==
The ResponseController allows the user to create and edit responses to questionnaires, in doing so, there needs to be some sort of authentication, and the proper place for that is in the action_allowed? method, however the un-refactored code did in the redirect_when_disallowed method. Moreover, once this code was moved to the action_allowed? method, all of the references to redirect_when_disallowed were removed (action_allowed? gets called automatically).
{| class="wikitable"
|-
! |Before Changes
! |After Changes
|- style="vertical-align:top;"
|<pre>
def action_allowed?
    current_user
end
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
</pre>
|<pre>
  def action_allowed?
    # 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.where(id: params[:id]).empty?
      true
    elsif
    response = Response.find(params[:id])
    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
  end
</pre>
|}
==Moving get_scores to the Response Model==
The get_scores method was being implemented in the controller. Its function fits better in the model.
As such, the method was moved and changed to retain the functionality of code that uses it. Since the variables it requires were no longer in scope, they're passed in as arguments. Also due to scope, the result has to be explicitly returned.
{| class="wikitable"
|-
! |Before Changes
! |After Changes
|- style="vertical-align:top;"
|<pre>
In response_controller.rb
def get_scores
    @review_scores = []
    @question_type = []
    @questions.each do |question|
      @review_scores << Score
        .where(
          response_id: @response.id,
          question_id:  question.id
        ).first
      @question_type << QuestionType.find_by_question_id(question.id)
    end
  end
</pre>
|<pre>
In response.rb
def get_scores(response, questions)
    review_scores = []
    question_type = []
    questions.each do |question|
      review_scores << Score
                            .where(
                                response_id: response.id,
                                question_id:  question.id
                            ).first
      question_type << QuestionType.find_by_question_id(question.id)
    end
      question_type
  end
</pre>
<pre>
In response_controller.rb
def view
    @response = Response.find(params[:id])
    @map = @response.map
    get_content
    @question_type = @response.get_scores(@response, @questions)
  end
</pre>
|}
==Moving the sorting of response versions logic out of rereview method==
The code to sort response versions has been moved from rereview method to a separate method. This was done because the rereview method was becoming very long and confusing.
{| class="wikitable"
|-
! |Before Changes
! |After Changes
|- style="vertical-align:top;"
|<pre>
In method rereview
      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
</pre>
|<pre>
In method rereview
  def rereview
    @map=ResponseMap.find(params[:id])
    # store response content in map
    get_content
    previous_responses
    #sort all the available versions in descending order.
    if @prev.present?
      sortResponseVersion
    end
</pre>
<pre>
def sortResponseVersion
    @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
</pre>
|}
==Removing special code in rereview method==
The rereview method had some special code to check if the current user is jace_smith and uses a custom hard-coded rubric instead of a rubric from the system. This was a code segment that was written when multipart rubrics were first tested. Removing this code will break the assignment and so the code was moved to a separate method and commented as a kludge.
{| class="wikitable"
|-
! |Before Changes
! |After Changes
|- style="vertical-align:top;"
|<pre>
In method rereview
      #**********************
      # 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
</pre>
|<pre>
In method rereview
  # 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"
        handle_jens_assignment
      else
        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"
        handle_jens_assignment
      else
        render :action => 'response'
      end
    end
  end
</pre>
<pre>
#kludge for checking if assignment is jen's assignment and using her rubric if it is
  def handle_jens_assignment
    if @assignment.id < 469
      @next_action = "update"
      render :action => 'custom_response'
    else
      @next_action = "update"
      render :action => 'custom_response_2011'
    end
  end
</pre>
|}
=Steps to verify changes=
=Steps to verify changes=


Revision as of 20:43, 28 October 2015

E1570. Refactoring TreeDisplayController

This page provides a description of the Expertiza based OSS project. This project aimed at refactoring the TreeDisplayController.

Introduction to Expertiza

Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). The Expertiza project is supported by the National Science Foundation.<ref>Expertiza</ref><ref>Wiki</ref>

Problem Statement

Classes involved: 

tree_display_controller.rb

What they do: This controller deals with displaying “trees” of objects to the instructor. When an instructor logs in, (s)he is greeted with a homepage that lists questionnaires (rubrics, quizzes, surveys), courses, and assignments. These objects are displayed in a “tree,” allowing the user to click on the top-level object and see the objects beneath it.

What's wrong with it:

  • Many Duplicate Methods.
  • Very Long Methods combining many Functionalities.
  • Redundant methods not being used anywhere.

What needs to be done:

  1. Split get_children_node_ng and get_children_node_2_ng into smaller methods and give reasonable names to them and make sure implement common code in a single method.


Changes Made

TreeDisplayController

Method Name Changes Made Reason For Change
Put ur txt here put ur text here
get_children_node_ng Moved part of the code from these methods to newly created methods populate_rows,display_row,page_render methods. These Methods had many functionalities being implemented which made the method look lengthy and moreover these methods were performing almost similar functions which could be grouped together and reduce the overall code length.
get_children_node_2_ng
------- Put ur txt here put ur text here

Re-factored Code

Refactoring get_children_node_ng method

Before Changes After Changes 
  def get_children_node_ng
    childNodes = {}
    if params[:reactParams][:child_nodes].is_a? String
      childNodes = JSON.parse(params[:reactParams][:child_nodes])
    else
      childNodes = params[:reactParams][:child_nodes]
    end
    tmpRes = {}
    res = {}
    for node in childNodes
      fnode = eval(params[:reactParams][:nodeType]).new

      for a in node
        fnode[a[0]] = a[1]
      end

      # fnode is the parent node
      # ch_nodes are childrens
      ch_nodes = fnode.get_children(nil, nil, session[:user].id, nil, nil)
      tmpRes[fnode.get_name] = ch_nodes

      # cnode = fnode.get_children("created_at", "desc", 2, nil, nil)

    end

    for nodeType in tmpRes.keys
      res[nodeType] =  Array.new

      for node in tmpRes[nodeType]
        tmpObject = {}
        tmpObject["nodeinfo"] = node
        tmpObject["name"] = node.get_name
        tmpObject["type"] = node.type

        if nodeType == 'Courses' || nodeType == "Assignments"
          tmpObject["directory"] = node.get_directory
          tmpObject["creation_date"] = node.get_creation_date
          tmpObject["updated_date"] = node.get_modified_date
          tmpObject["private"] = node.get_private
          instructor_id = node.get_instructor_id
          tmpObject["instructor_id"] = instructor_id
          unless (instructor_id.nil?)
            tmpObject["instructor"] = User.find(instructor_id).name
          else
            tmpObject["instructor"] = nil
          end

          tmpObject["is_available"] = is_available(session[:user], instructor_id) || (session[:user].role_id == 6 && 

Ta.get_my_instructors(session[:user].id).include?(instructor_id) && ta_for_current_course?(node))
          if nodeType == "Assignments"
            tmpObject["course_id"] = node.get_course_id
            tmpObject["max_team_size"] = node.get_max_team_size
            tmpObject["is_intelligent"] = node.get_is_intelligent
            tmpObject["require_quiz"] = node.get_require_quiz
            tmpObject["allow_suggestions"] = node.get_allow_suggestions
            tmpObject["has_topic"] = SignUpTopic.where(['assignment_id = ?', node.node_object_id]).first ? true : false
          end
        end
        res[nodeType] << tmpObject
      end

    end

    respond_to do |format|
      format.html {render json: res}
    end
  end

 def get_children_node_ng
    # First Level Of Tree Display with Folder Nodes(Courses, Assisgnments,Questionnaires )
    childNodes = {}
    if params[:reactParams][:child_nodes].is_a? String
      childNodes = JSON.parse(params[:reactParams][:child_nodes])
    else
      childNodes = params[:reactParams][:child_nodes]
    end
    tmpRes = {}
    res = {}
    for node in childNodes
      # Declaring Foldernode Object as New
      fnode = eval(params[:reactParams][:nodeType]).new
      for a in node
        fnode[a[0]] = a[1]
      end
      # fnode is the parent node
      # ch_nodes are childrens
      ch_nodes = fnode.get_children(nil, nil, session[:user].id, nil, nil)
      tmpRes[fnode.get_name] = ch_nodes

    end
      call_function ="get_children_node_ng"
      # Row Populating
      populate_rows(tmpRes,call_function)
  end

Refactoring get_children_node_2_ng method

Before Changes After Changes 
  def get_children_node_2_ng
    childNodes = {}
    if params[:reactParams2][:child_nodes].is_a? String
      childNodes = JSON.parse(params[:reactParams2][:child_nodes])
    else
      childNodes = params[:reactParams2][:child_nodes]
    end
    tmpRes = {}
    res = []
    fnode = eval(params[:reactParams2][:nodeType]).new
    childNodes.each do |key, value|
      fnode[key] = value
    end

    ch_nodes = fnode.get_children(nil, nil, session[:user].id, nil, nil)
    tmpRes = ch_nodes
    if tmpRes
      for child in tmpRes
        nodeType = child.type
        res2 = {}
        res2["nodeinfo"] = child
        res2["name"] = child.get_name
        res2["key"] = params[:reactParams2][:key]
        res2["type"] = nodeType

        res2["private"] = child.get_private
        res2["creation_date"] = child.get_creation_date
        res2["updated_date"] = child.get_modified_date
        if nodeType == 'CourseNode' || nodeType == "AssignmentNode"
          res2["directory"] = child.get_directory
          instructor_id = child.get_instructor_id
          res2["instructor_id"] = instructor_id
          unless (instructor_id.nil?)
            res2["instructor"] = User.find(instructor_id).name
          else
            res2["instructor"] = nil
          end

          res2["is_available"] = is_available(session[:user], instructor_id) || (session[:user].role_id == 6 && 
Ta.get_my_instructors(session[:user].id).include?(instructor_id) && ta_for_current_course?(child))
          if nodeType == "AssignmentNode"
            res2["course_id"] = child.get_course_id
            res2["max_team_size"] = child.get_max_team_size
            res2["is_intelligent"] = child.get_is_intelligent
            res2["require_quiz"] = child.get_require_quiz
            res2["allow_suggestions"] = child.get_allow_suggestions
            res2["has_topic"] = SignUpTopic.where(['assignment_id = ?', child.node_object_id]).first ? true : false
          end
        end
        res << res2
      end
    end

    respond_to do |format|
      format.html {render json: res}
    end
  end

 def get_children_node_2_ng
    # Second Level of Tree Display with Children Nodes
    childNodes = {}
    if params[:reactParams2][:child_nodes].is_a? String
      childNodes = JSON.parse(params[:reactParams2][:child_nodes])
    else
      childNodes = params[:reactParams2][:child_nodes]
    end

    res = []
    fnode = eval(params[:reactParams2][:nodeType]).new
    childNodes.each do |key, value|
      fnode[key] = value
    end

    ch_nodes = fnode.get_children(nil, nil, session[:user].id, nil, nil)

    call_function = "get_children_node_2_ng"
    populate_rows(ch_nodes, call_function)
  end

Removing duplicate methods

Anush For u to write here...

Before Changes After Changes 
def new_feedback
    review = Response.find(params[:id])
    if review
      reviewer = AssignmentParticipant.where(user_id: session[:user].id, parent_id:  review.map.assignment.id).first
      map = FeedbackResponseMap.where(reviewed_object_id: review.id, reviewer_id:  reviewer.id).first
      if map.nil?
        map = FeedbackResponseMap.create(:reviewed_object_id => review.id, :reviewer_id => reviewer.id, 
                                         :reviewee_id => review.map.reviewer.id)
      end
      redirect_to :action => 'new', :id => map.map_id, :return => "feedback"
    else
      redirect_to :back
    end
  end

def new_feedback
    review = Response.find(params[:id])
    if review
      reviewer = AssignmentParticipant.where(user_id: session[:user].id, parent_id:  review.map.assignment.id).first
      map = FeedbackResponseMap.where(reviewed_object_id: review.id, reviewer_id:  reviewer.id).first
      if map.nil?
        #if no feedback exists by dat user den only create for dat particular response/review
        map = FeedbackResponseMap.create(:reviewed_object_id => review.id, :reviewer_id => reviewer.id, 
                                         :reviewee_id => review.map.reviewer.id)
      end
      redirect_to :action => 'new', :id => map.id, :return => "feedback"
    else
      redirect_to :back
    end
  end

def new_feedback
    review = Response.find(params[:id])
    if review
      reviewer = AssignmentParticipant.where(user_id: session[:user].id, parent_id:  review.map.assignment.id).first
      map = FeedbackResponseMap.where(reviewed_object_id: review.id, reviewer_id:  reviewer.id).first
      if map.nil?
        #if no feedback exists by dat user den only create for dat particular response/review
        map = FeedbackResponseMap.create(:reviewed_object_id => review.id, :reviewer_id => reviewer.id, 
                                         :reviewee_id => review.map.reviewer.id)
      end
      redirect_to :action => 'new', :id => map.id, :return => "feedback"
    else
      redirect_to :back
    end
  end

def view
    @response = Response.find(params[:id])
    return if redirect_when_disallowed(@response)
    @map = @response.map
    get_content
    @review_scores = Array.new
    @question_type = Array.new
    @questions.each do |question|
      @review_scores << Score.where(response_id: @map.response_id, question_id:  question.id).first
      @question_type << QuestionType.find_by_question_id(question.id)
    end
  end

def view
    @response = Response.find(params[:id])
    return if action_allowed?(@response)
    @map = @response.map
    get_content
    get_scores(@response, @questions)
  end

def view
    @response = Response.find(params[:id])
    return if action_allowed?(@response)
    @map = @response.map
    get_content
    get_scores(@response, @questions)
  end

Steps to verify changes

Action Allowed

Action allowed is called before every method call to confirm whether or not the currently logged in user has the required privileges to complete the requested action. To test the functionality of this method one has to attempt to complete an action that they have privileges to complete, and confirm that they're allowed to complete it. One also has to attempt to complete an action that they don't have the privileges to complete, and confirm that they're not allowed to complete it.

Allowed

Log into the application with the user having a student's role (User Id: user5072, Password: password)

  1. Click on Assignments, and you should see the following:

Not Allowed

Log into the application with the user having an instructor's role (User Id: user6, Password: password)

  1. Click on Assignments, and you should see the following:

get_scores

Get scores is called anytime the scores for reviews are shown. Given that, the way to test its correct functionality is to navigate to a completed review and make sure the scores are shown and are correct.

Log into the application with the user having a student's role (User Id: user5072, Password: password)

  1. Click on Assignments
  2. Click on Writing assignment 1b, Spring 2013
  3. Click on Others' Work
  4. Click on Review done at --2013-03-01 23:57:55 UTC

Successful loading of this page confirms the get_scores method.

rereview

Rereview manages the versioning of multiple reviews for the same assignment. To test the correct functionality of review, one should attempt to update a review (i.e. 'rereview').

Log into the application with the user having a student's role (User Id: user5072, Password: password)

  1. Click on Assignments
  2. Click on Writing assignment 1b, Spring 2013
  3. Click on Others' Work
  4. Click on Update (beside Metaprogramming in dynamically typed languages)

Successful loading of this page confirms the rereview method.

References

<references></references>

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2015/oss_E1570_avr&oldid=97248"