CSC/ECE 517 Fall 2015/oss E1551 RGS: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
 
(9 intermediate revisions by 2 users not shown)
Line 9: Line 9:
Responses come in versions.  Any time an author revises work, and the reviewer comes back to review it, a new Response object is generated.  This Response object points to a particular ReponseMap, which tells who the reviewer is, which team is the reviewee, and what the reviewed entity is.
Responses come in versions.  Any time an author revises work, and the reviewer comes back to review it, a new Response object is generated.  This Response object points to a particular ReponseMap, which tells who the reviewer is, which team is the reviewee, and what the reviewed entity is.


== Project Description ==
*To test the responsecontroller login as user:instructor6 with password:password
'''What needs to be done:'''
*Go to assignments tab and choose any assignment from the assignments listed.
*From there click on others work and choose any review among the reviews listed
*Click on view or begin to start reviewing.
*You can also test response controller in similar way by logging as student student2064 and password:password.


== Changes ==
== Changes ==
Line 16: Line 19:
;Problem definition
;Problem definition
:<code>latestResponseVersion</code> is misnamed.  It returns all versions of a response, and anyway, the method name should use underscores, not camelcase.
:<code>latestResponseVersion</code> is misnamed.  It returns all versions of a response, and anyway, the method name should use underscores, not camelcase.
;Solution
;Solution summary
:
:Renamed <code>latestResponseVersion</code> to <code>set_all_responses</code>.
==== Solution ====
Examining <code>latestResponseVersion</code> we were able to determine that it did the following two things:
# Set the <code>@prev</code> variable to a relation of responses that correspond to the map id of the current response being created or edited.
# Set the <code>@review_scores</code> variable to an Array version if the contents of <code>@prev</code>.
Since the purpose of the code seemed to be to set variables, we renamed it to <code>set_all_responses</code>.
We also simplified the method by using ActiveRecord::Relation's <code>to_a</code> to convert the relation to an array.
There was some duplicate code in the <code>scores</code> function which we replaced with a call the <code>set_all_responses</code>.


=== Rename get_scores ===
=== Rename get_scores ===
Line 36: Line 46:
;Solution summary
;Solution summary
:There was not much actual overlap in these two methods. Minimal changes.
:There was not much actual overlap in these two methods. Minimal changes.
The ResponseController controller has an overall problem in that it knows too much about the subclasses of <code>ResponseMap</code>. There is some major refactoring that needs to occur to clean up this controller's coupling with <code>ResponseMap</code>'s subclasses. We made an attempt to do this, but it was just to large a change to accomplish in the time allotted.
=== Consistent authorization ===
=== Consistent authorization ===
;Problem definition
;Problem definition
Line 92: Line 104:
;Solution summary
;Solution summary
:The <code>get_content</code> method was renamed <code>set_content</code>, simplified, and documented with comments.
:The <code>get_content</code> method was renamed <code>set_content</code>, simplified, and documented with comments.
A review of the <code>get_content</code> method showed that it's purpose was to '''set''' various variables for use by the response views. As such, we renamed this method <code>set_content</code> to help clarify it's intent.
We also extracted methods for the more complicated variables into their own methods, making the <code>set_content</code> method easy to understand at a glance.
=== Remove SQL queries ===
=== Remove SQL queries ===
;Problem definition
;Problem definition
Line 120: Line 135:


== Tests ==
== Tests ==
Some functional tests are written for this class using Rspec and Factorygirls.
Some functional tests are written for this class using Rspec. The test coverage has increased from 19% to 24.1%
;To check these tests.
;To check these tests.
*clone the github repository link provided above  
*clone the github repository link provided above  

Latest revision as of 03:29, 7 November 2015

E1551. Refactoring response_controller.rb

Expertiza

About Expertiza Expertiza is an open source project accomplished by NCState students. It was developed in Ruby on Rails. It has general features like forming teams, registering for topics in a project, uploading the final deliveries. After completing the project there is an option for submitting reviews as well and updating the reviews. There is an option for submitting new quizzes as well.

ResponseController

What it does: response_controller.rb manages Responses. A Response is what is generated any time a user fills out a rubric--any rubric. This includes review rubrics, author-feedback rubrics, teammate-review rubrics, quizzes, and surveys. Responses come in versions. Any time an author revises work, and the reviewer comes back to review it, a new Response object is generated. This Response object points to a particular ReponseMap, which tells who the reviewer is, which team is the reviewee, and what the reviewed entity is.

  • To test the responsecontroller login as user:instructor6 with password:password
  • Go to assignments tab and choose any assignment from the assignments listed.
  • From there click on others work and choose any review among the reviews listed
  • Click on view or begin to start reviewing.
  • You can also test response controller in similar way by logging as student student2064 and password:password.

Changes

Refactor latestResponseVersion method

Problem definition
latestResponseVersion is misnamed. It returns all versions of a response, and anyway, the method name should use underscores, not camelcase.
Solution summary
Renamed latestResponseVersion to set_all_responses.

Solution

Examining latestResponseVersion we were able to determine that it did the following two things:

  1. Set the @prev variable to a relation of responses that correspond to the map id of the current response being created or edited.
  2. Set the @review_scores variable to an Array version if the contents of @prev.

Since the purpose of the code seemed to be to set variables, we renamed it to set_all_responses. We also simplified the method by using ActiveRecord::Relation's to_a to convert the relation to an array. There was some duplicate code in the scores function which we replaced with a call the set_all_responses.

Rename get_scores

Problem definition
get_scores has a Java-like name. It should just be scores.

Solution summary

The method get_scores was renamed to scores.

Delete rereview method

Problem definition
The 100+-line method rereview does not seem to be used anymore. The second-round review can be invoked in the same way as the first-round review. Remove it.
Solution summary
The method rereview was deleted.

DRY create and update

Problem definition
create and update use completely different code. Factor the common parts out into a partial, thereby simplifying the methods.
Solution summary
There was not much actual overlap in these two methods. Minimal changes.

The ResponseController controller has an overall problem in that it knows too much about the subclasses of ResponseMap. There is some major refactoring that needs to occur to clean up this controller's coupling with ResponseMap's subclasses. We made an attempt to do this, but it was just to large a change to accomplish in the time allotted.

Consistent authorization

Problem definition
Authorization needs to be checked in the action_allowed method instead of in redirect_when_disallowed at the bottom of the file.
Solution summary
So the functionality is moved from redirect_when_disallowed to action_allowed. And also it is made sure that no one other than the author of a review (or another team member, in the case of author feedback) can edit it and then removed the redirect_when_disallowed method.
Code before changing

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

Code after changing

def action_allowed?
   case params[:action]
   when 'view','edit','delete','update'
     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)
   else
     current_user
   end
 end

Refactor get_content

Problem definition
get_content is a complex method. It should be renamed to content and simplified. Comments should be added explaining what it does.
Solution summary
The get_content method was renamed set_content, simplified, and documented with comments.

A review of the get_content method showed that it's purpose was to set various variables for use by the response views. As such, we renamed this method set_content to help clarify it's intent. We also extracted methods for the more complicated variables into their own methods, making the set_content method easy to understand at a glance.

Remove SQL queries

Problem definition
This class contains SQL queries. Please change them to Active Record commands.
Solution summary
No SQL queries were identified. No changes.


Comparison of Original and Refactored Code

Major refactoring revolved around changing method names according to rails convention, using helper methods for avoiding duplication of code in controller methods.

Duplications in Code
Original duplications : 172
Post Refactoring : 21

Code Complexity (Compared on Code Climate)

Original ResponseController<ref name="originalresponsecontroller>Original responseController https://github.com/expertiza/expertiza</ref>

Refactored Responsecontroller <ref name="Refactoredresponsecontroller>Refactored responseController https://github.com/viswaraavi/expertiza</ref>

Tests

Some functional tests are written for this class using Rspec. The test coverage has increased from 19% to 24.1%

To check these tests.
  • clone the github repository link provided above
  • run the command rspec spec/controllers/response_controller_spec.rb

References

<references>