CSC/ECE 517 Fall 2015/ossE1558BGJ

Introduction

This page provides a description of the modifications and improvements made to the Expertiza project’s source code as part of an Expertiza based Open Source Software project. Expertiza is a web based application which allows students to submit and peer-review classmates’ work, including written articles and development projects. The specific changes made during the course of this project were to the ReviewResponseMap.rb file, with additional changes made to other related files in support of refactoring ReviewResponseMap.rb.

Problem Statement

ReviewResponseMap is the model class used to manage the relationship between contributors, reviewers, and assignments. The intent of the changes were to refactor the code for better readability and adherence to Ruby coding standards and best practices. Primarily these changes involved the refactoring of overly complex methods, renaming methods for better readability, and the addition of missing unit tests.

Project Requirements:

1. [[ https://codeclimate.com/ |Code Climate]] shows import method is complex, because of lots of checks. This method can be fixed by adding private methods for raising import error.
2. Get_assessments_round_for method can be renamed to get_team_responses_for_round. Team_id private variable is not needed.
3. metareview_response_maps rename, refactoring can be done. No need to do second iteration.
4. write missing unit tests for existing methods.

Design Patterns

As part of our project, we refactored the model to follow the Single Responsibility and Don't Repeat Yourself (DRY) Principles. Both of the principles focus on reducing code repetition and increasing the cohesion of the individual methods in the class. The Single Responsibility Principle states that each class and method should have sole responsibility over one single part of the functionality of the system<ref>https://en.wikipedia.org/wiki/Single_responsibility_principle</ref>. The DRY principle states that whenever the same types of logic and functionality are being performed in a class the duplicated logic should be extracted into a new method that can be called in place of the repeated lines of code <ref>https://en.wikipedia.org/wiki/Don%27t_repeat_yourself</ref>. A prime example of a method that initially violated these principles was the import method, which in addition to performing the core import processes also performed a number of checks that would be more properly extracted into private methods.

Code Changes

Refactoring of import method

[[ https://codeclimate.com/ |Code Climate]] reports showed the import method to be overly complex, with a number of checks being included within the import method itself. The resolution for this problem was to refactor the import method, creating private methods to perform the null checks and throw import errors.

Prior to refactoring, all null checks were performed in line:

After refactoring the import method adheres to the single responsibility principle, performing only key import tasks while making calls to private methods for any necessary validation:

 #Imports new reponse maps if they do not already exist
  def self.import(row, _session, id)
      if row.length < 2
          raise ArgumentError, 'Not enough items'
      end
      assignment = find_assignment(id)
      index = 1
      reviewee_name = row[0]
      while index < row.length
          reviewee_id = nil
          reviewer_name = row[index]
          reviewer = get_assignment_participant(reviewer_name,  assignment.id, "reviewer")
          participant_nil?(reviewer, reviewer_name , "reviewer")
         
         #Find reviewee if assignment is a team assignment
         if assignment.team_assignment
             reviewee = AssignmentTeam.where(name: reviewee_name .to_s.strip, parent_id: assignment.id).first
             participant_nil?(reviewee, reviewee_name, "author")
             reviewee_id = reviewee.id
         #Find reviewee if assignment is not a team assignment
         else
	     reviewee = get_assignment_participant(reviewee_name, assignment.id, "reviewee")
             participant_nil?(reviewee, reviewee_name, "author")
             reviewee_id  = TeamsUser.team_id(reviewee.parent_id, reviewee.user_id)
         end
         create_response_map(reviewer, reviewee_id,  assignment)
         index += 1
      end
  end

Private methods added:

 # Check for if assignment value is null
 def self.find_assignment(id)
     begin
         assignment = Assignment.find(id)
     rescue ActiveRecord::RecordNotFound
         raise ImportError, "The assignment with id \"#{id}\" was not found.<a href='/assignment/new'>Create</a> this assignment?"
     end
 end

 # Check for if participant is null
 def self.participant_nil?(participant, user_name, participant_type) 
     error_message = nil
     if participant_type == "author"
         error_message = "The author \"#{user_name.to_s.strip}\" was not found.<a href='/users/new'>Create</a> this user?"
     else
         error_message =  "The reviewer \"#{user_name}\" is not a participant in this assignment.<a href='/users/new'>Register</a> this user as a participant?"
     end
     check_nil?(participant, error_message)
 end

 # Check for if user is null
 def self.user_nil?(user, user_name, user_type)
     check_nil?( user, "The user account for the \"#{user_type}\" \"#{user_name}\" was not found.<a href='/users/new'>Create</a> this user?")
 end

#Throws import error for nil objects
 def self.check_nil?(object, error_message)
     if object.nil?
         raise ImportError, error_message
     end
 end

Refactoring of get_assessments_round_for method

The get_assessments_round_for method’s name failed to provide a clear idea of the method’s purpose and functionality. Additionally, there was a private variable, team_id, introduced in the method that was unnecessary and could be replaced by using the id property of the team parameter directly.

Following the refactoring, the name of the method provides a clearer idea of its purpose:

 def self.get_team_responses_for_round(team, round)
   responses = []
   if team.id
     maps = ResponseMap.where(reviewee_id: team.id, type: 'ReviewResponseMap')
     maps.each do |map|
       unless map.response.empty? && map.response.reject { |r| r.round != round }.empty?
         responses << map.response.reject { |r| r.round != round }.last
       end
     end
     responses.sort! { |a, b| a.map.reviewer.fullname <=> b.map.reviewer.fullname }
   end
   responses
 end

The change in method name also required changes to the following files that referenced it:

/app/models/assignment.rb:436:

./app/models/assignment_participant.rb:268:

Refactor metareview_response_maps

The metareview_response_maps method was unnecessarily complex, introducing unneeded private variables and an additional iteration inside the main loop.

After refactoring:

 def get_metareview_response_maps
   responses = Response.where(map_id: id)
   metareview_response_maps = []
   responses.each do |response|
     metareview_response_maps << MetareviewResponseMap.where(reviewed_object_id: response.id)
   end
   metareview_response_maps
 end

Addition of code comments and unit tests

In addition to the refactoring performed to DRY up the ReviewResponseMap.rb methods we added missing tests for the existing methods and restructured the pre-existing tests to use fixtures. The following fixture files were added during the process:

• assignment_questionnaires.yml
• assignments.yml
• participants.yml
• questionnaired.yml
• response_maps.yml
• responses.yml
• teams.yml
• users.yml

Unit Tests

To run the unit tests, follow these steps:

1. Download the master branch of the repo.
2. Setup the Databases for the test environment (we have used Zhewei's scrubbed expertiza DB )
   • Run the " rake db:create RAILS_ENV=test " command
   • Run the " rake db:reset RAILS_ENV=test " command
   • Scrub the DB using " mysql -u root expertiza_development < expertiza-scrubbed.sql"
   • Run the " rake db:migrate "
3. Run " rake test test/unit/review_response_map_test.rb " in the "expertiza/" directory.
   • If you run into a mysql log in error go into config/database.yml and edit the mysql credentials for the test section so that it matches the local environment's mysql configuration
4. Check if tests passed or failed.

Manual Test Case

In order to test the changes made to the ReviewResponseMap, bring up Expertiza and log in as an administrator. Once logged in, proceed with the following steps:

1. Create at least three users to perform the tests.
2. Create an assignment for only the users you just created. Ensure that the assignment is only available for your new users. It’s important to follow this step exactly so that you will not confuse your new users with the preexisting users.
3. Sign out.
4. Log in to Expertiza as User1 and submit the assignment.
5. Open two new Chrome Incognito window and log in as the other 2 users and select that submission for review. It is best to keep these in multiple incognito tabs so that you will not have to log out and log back in each time.
6. Check from User1’s “Your scores” page whether the page is loading correctly prior to the reviews being performed.
7. Review the assignment from User2’s login.
8. Ensure that reviews show up on User1’s page.
9. Repeat steps 6 and 7 for User3.
10. While logged in as User1, give feedback to User2 and User3
11. Change the deadline so that you are able to switch into the “Metareview Phase”.
12. Repeat the above steps.
13. Perform the review from User2(or User3) and ensure that the metareviews are correctly displayed on User1’s page

References

<references/>       3. Expertiza Main Repo
4. Refactored ReviewResponseMap.rb
5. Test Fixtures
6. Unit Tests