Latest revision as of 04:05, 31 March 2015

E1508 Refactoring Response Controller

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

In order to run our code vist: 152.46.20.14:1 (VNC Viewer) or 152.46.20.14:5801 (browser), and use the password: password to log in. Please leave the system like you found it (with the browser open, and set to localhost:3000).

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:

response_controller.rb
response.rb

What they do: Allows the user to create and edit responses to questionnaires … such as performing a review, rating a teammate, or giving feedback to a reviewer.

What's wrong with it:

It doesn’t do authorization properly.
It contains duplicated methods.
Functionality that should be in models is incorporated into the controller.

What needs to be done:

latestresponseversion seems misnamed. It fetches all previous versions of a response (which is to say, all previous review versions by the current reviewer of the current author for the current assignment).
get_scores gets all the scores assigned in a particular response. A “response” is created when someone submits a review, a partner evaluation, a survey, etc.
The rereview method is 98 lines long. Several parts of it should be turned into methods. Sorting review versions is really 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.
rereview contains special code to check whether an assignment is “Jen’s assignment”; this was the first assignment we 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 probably impossible to remove this code without breaking that assignment, but it should be done in a separate method, and commented appropriately as a kludge.
Again in rereview, creating a new version of a review is a model responsibility, and should be moved out of the controller.
There are two (consecutive) copies of the edit method. The second appears to be the newer one, and, according to the rules for method definition, is the one that is currently in use. The first should be removed. Ditto for new_feedback and view--the 2nd version of each appears to be newer & should be kept.
Authorization to perform actions is not checked correctly. It is supposed to be done through the action_allowed? method at the beginning of the class definition. Different authorization should be 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. Currently, authorization is denied by the redirect_when_disallowed method, which was an earlier, more error-prone way of controlling access. It should go away, now that the class has an action_allowed? method.

Changes Made

Response Controller

Method Name	Changes Made	Reason For Change
latestResponseVersion	Changed name to previous_responses	The name wasn't indicative of what the method did
new_feedback	Deleted the old new_feedback method	The old new_feedback method was not getting called due to ruby rules
view	Deleted the old view method	The old view method was not getting called due to ruby rules
redirect_when_disallowed	Moved all of the code from redirect_when_disallowed to action_allowed and deleted all of the previous references (action_allowed? gets called automatically)	Authorization to perform actions wasn't being performed correctly, it is supposed to be done through action_allowed?
action_allowed?
rereview	Moved logic for sorting response versions, and for getting the latest response to their own methods. The method had some special code that was moved out. Could not move the logic for creating new reviews out of rereview since the new reviews were not created by rereview but instead was using the create method in response_controller.rb	The rereview method was becoming too large and confusing

Response Model

Method Name	Changes Made	Reason For Change
get_scores	Moved from controller (response_controller.rb) to model (response.rb)	The functionality in get_scores makes more sense to be implemented in the Model rather than in the controller

Re-factored Code

Renaming latestResponseVersion method

The latestResponseVersion method is misnamed. It fetches all the previous versions of a response and its current name does not reflect its actual functionality. The method was renamed as previous_responses since it better suited the actual functionality.

Before Changes

After Changes

 def latestResponseVersion
    #get all previous versions of responses for the response map.
    @review_scores=Array.new
    @prev=Response.where(map_id: @map.id)
    for element in @prev
      @review_scores << element
    end
  end

In method rereview

     for element in @prev
      if (element.map.id==@map.map.id)
        array_not_empty=1
        @review_scores << element
      end
    end
    latestResponseVersion

In method create

    @map = ResponseMap.find(params[:id]) 
    @res = 0
    msg = ""
    error_msg = ""
    latestResponseVersion

 def previous_responses
    #get all previous versions of responses for the response map.
    @review_scores=Array.new
    @prev=Response.where(map_id: @map.id)
    for element in @prev
      @review_scores << element
    end
  end

In method rereview

    @map=ResponseMap.find(params[:id])
    # store response content in map
    get_content
    previous_responses

In method create

    @map = ResponseMap.find(params[:id]) 
    @res = 0
    msg = ""
    error_msg = ""
    previous_responses

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.

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

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

Before Changes

After Changes

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

  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

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.

Before Changes

After Changes

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

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

In response_controller.rb

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

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.

Before Changes

After Changes

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

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

 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

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.

Before Changes

After Changes

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

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

 #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

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)

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)

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)

Click on Assignments
Click on Writing assignment 1b, Spring 2013
Click on Others' Work
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)

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

Successful loading of this page confirms the rereview method.

References

CSC/ECE 517 Fall 2014/oss E1508 MRS: Difference between revisions

Latest revision as of 04:05, 31 March 2015

Contents

Introduction to Expertiza

Problem Statement

Changes Made

Response Controller

Response Model

Re-factored Code

Renaming latestResponseVersion method

Removing duplicate methods

Moving code to the proper place

Moving get_scores to the Response Model

Moving the sorting of response versions logic out of rereview method

Removing special code in rereview method

Steps to verify changes

Action Allowed

Allowed

Not Allowed

get_scores

rereview

References

Navigation menu

@@ Line 2: / Line 2: @@
 This page provides a description of the Expertiza based OSS project. This project aimed at refactoring the ResponseController.
+<b>In order to run our code vist: 152.46.20.14:1 (VNC Viewer) or 152.46.20.14:5801 (browser), and use the password: password to log in. Please leave the system like you found it (with the browser open, and set to localhost:3000).</b>
 __TOC__
+=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>[http://expertiza.ncsu.edu/ Expertiza]</ref><ref>[http://wikis.lib.ncsu.edu/index.php/Expertiza Wiki]</ref>
 = Problem Statement =
@@ Line 9: / Line 14: @@
   response_controller.rb
   response.rb
- '''???????'''
 '''What they do:'''
@@ Line 41: / Line 45: @@
 | Changed name to previous_responses
 | The name wasn't indicative of what the method did
-|-
-| edit
-| Deleted the old edit method
-| The old edit method was not getting called due to ruby rules
 |-
 | new_feedback
@@ Line 54: / Line 54: @@
 | The old view method was not getting called due to ruby rules
 |-
-| redirect_when_disallowed & action_allowed?
+| redirect_when_disallowed
-| Moved all of the code from redirect_when_disallowed to action_allowed and changed all of the references
+| rowspan="2" | Moved all of the code from redirect_when_disallowed to action_allowed and deleted all of the previous references (action_allowed? gets called automatically)
-| Authorization to perform actions wasn't being performed correctly, it is supposed to be done through action_allowed?
+| rowspan="2" | Authorization to perform actions wasn't being performed correctly, it is supposed to be done through action_allowed?
+|-
+| action_allowed?
+|-
+| rereview
+| Moved logic for sorting response versions, and for getting the latest response to their own methods. The method had some special code that was moved out. Could not move the logic for creating new reviews out of rereview since the new reviews were not created by rereview but instead was using the create method in response_controller.rb
+| The rereview method was becoming too large and confusing
+|-
 |}
-==Views==
+==Response Model==
 {| class="wikitable"
 |-
-! style="width:18%;"|View Name
+! style="width:13%;"|Method Name
 ! style="width:33%;"|Changes Made
 ! style="width:43%;"|Reason For Change
-|- style="vertical-align:top;"
-| rowspan="5" valign="middle" |
-sign_up_sheet/_add_signup_topcis
-| Fixed title
-| The page title displayed for an instructor was "Sign-up sheet for ... "
 |-
-| Commented out View/Manage bookmark links
+| get_scores
-| These links were broken, and according to existing code were not meant for the instructor view.
+| Moved from controller (response_controller.rb) to model (response.rb)
+| The functionality in get_scores makes more sense to be implemented in the Model rather than in the controller
 |-
-| Added variable initialization for @sign_up_topics
+|}
-| The variable was referenced further on in the view, which was causing an 'nil value' error
+= Re-factored Code =
+==Renaming latestResponseVersion method==
+The latestResponseVersion method is misnamed. It fetches all the previous versions of a response and its current name does not reflect its actual functionality. The method was renamed as previous_responses since it better suited the actual functionality.
+{| class="wikitable"
 |-
-| Fixed flash message rendering for new topic creation
+! |Before Changes
-| The flash message was rendering incorrectly with HTML tags visible on the web page
+! |After Changes
+|- style="vertical-align:top;"
+|<pre>
+ def latestResponseVersion
+    #get all previous versions of responses for the response map.
+    @review_scores=Array.new
+    @prev=Response.where(map_id: @map.id)
+    for element in @prev
+      @review_scores << element
+    end
+  end
+</pre>
+<pre>
+In method rereview
+     for element in @prev
+      if (element.map.id==@map.map.id)
+        array_not_empty=1
+        @review_scores << element
+      end
+    end
+    latestResponseVersion
+</pre>
+<pre>
+In method create
+    @map = ResponseMap.find(params[:id])
+    @res = 0
+    msg = ""
+    error_msg = ""
+    latestResponseVersion
+</pre>
+|<pre>
+ def previous_responses
+    #get all previous versions of responses for the response map.
+    @review_scores=Array.new
+    @prev=Response.where(map_id: @map.id)
+    for element in @prev
+      @review_scores << element
+    end
+  end
+</pre>
+<pre>
+In method rereview
+    @map=ResponseMap.find(params[:id])
+    # store response content in map
+    get_content
+    previous_responses
+</pre>
+<pre>
+In method create
+    @map = ResponseMap.find(params[:id])
+    @res = 0
+    msg = ""
+    error_msg = ""
+    previous_responses
+</pre>
+|}
+==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.
+{| class="wikitable"
 |-
-| Added full path names to partial names
+! |Before Changes
-| The shared partials were also being used by views in the Assignments controller, so full paths were needed
+! |After Changes
+|- style="vertical-align:bottom;"
+|<pre>
+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
+</pre>
+|<pre>
+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
+</pre>
+|- style="vertical-align:bottom;"
+|<pre>
+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
+</pre>
+|<pre>
+def view
+    @response = Response.find(params[:id])
+    return if action_allowed?(@response)
+    @map = @response.map
+    get_content
+    get_scores(@response, @questions)
+  end
+</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"
 |-
-| sign_up_sheet/_add_topics
+! |Before Changes
-| Added separation between the Import Topics and Manage Bookmarks links
+! |After Changes
-| The two links were clumped together and it was difficult to distinguish between them
+|- 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"
 |-
-| sign_up_sheet/_actions
+! |Before Changes
-| Replaced rendering of the reserve_topic partial with the _all_actions partial
+! |After Changes
-| The reserve_topic partial is no longer being used and is replaced by the newly created _all_actions partial
+|- 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"
 |-
-| sign_up_sheet/_all_actions
+! |Before Changes
-| Created this new partial
+! |After Changes
-| We created the _all_actions partial to conditionally render the "Actions" table field for both instructors and students. This now also includes working links through which instructors or administrators can edit or delete topics.
+|- 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"
 |-
-| sign_up_sheet/_table_line
+! |Before Changes
-| Gave edit/delete rights to Super Admin
+! |After Changes
-| The Super-Administrator user wasn't included in the list of users with these permissions
+|- style="vertical-align:top;"
-|-
+|<pre>
-| sign_up_sheet/edit
+In method rereview
-| Modified title and included @assignment_id variable to be passed as a parameter variable to the update action
-| The view was initially trying to access the params[:assignment_id] variable which was not getting initialized
+       #**********************
-|-
+      # Check whether this is Jen's assgt. & if so, use her rubric
-| assignments/edit
+      if (@assignment.instructor_id == User.find_by_name("jace_smith").id) && @title == "Review"
-| Changed the partial rendered to /sign_up_sheet/add_signup_topcis.html
+        if @assignment.id < 469
-| The assignments/edit/add_signup_topics partial (with duplicated code) was otherwise being rendered
+          @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=
+==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 (<b>User Id:</b> user5072, <b>Password:</b> password)
+# Click on Assignments, and you should see the following:
+[[File:actionallowedPass.png]]
+===Not Allowed===
+Log into the application with the user having an instructor's role (<b>User Id:</b> user6, <b>Password:</b> password)
+# Click on Assignments, and you should see the following:
+[[File:actionallowedFail.png]]
+== 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 (<b>User Id:</b> user5072, <b>Password:</b> password)
+# Click on Assignments
+# Click on Writing assignment 1b, Spring 2013
+# Click on Others' Work
+# 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 (<b>User Id:</b> user5072, <b>Password:</b> password)
+# Click on Assignments
+# Click on Writing assignment 1b, Spring 2013
+# Click on Others' Work
+# Click on Update (beside Metaprogramming in dynamically typed languages)
+Successful loading of this page confirms the rereview method.
+=References=
+<references></references>

CSC/ECE 517 Fall 2014/oss E1508 MRS: Difference between revisions

Latest revision as of 04:05, 31 March 2015

Introduction to Expertiza

Problem Statement

Changes Made

Response Controller

Response Model

Re-factored Code

Renaming latestResponseVersion method

Removing duplicate methods

Moving code to the proper place

Moving get_scores to the Response Model

Moving the sorting of response versions logic out of rereview method

Removing special code in rereview method

Steps to verify changes

Action Allowed

Allowed

Not Allowed

get_scores

rereview

References

Navigation menu

Search