CSC/ECE 517 Spring 2017/oss E1713

From Expertiza_Wiki
Jump to navigation Jump to search

E1713. Refactor penalty_helper.rb and late_policies_controller.rb
This project is intended to improve the code quality and reduce technical debt within the Expertiza system by refactoring the penalty_helper.rb module, refactoring the late_policies_controller.rb controller, and adding test coverage for both files.

Introduction

Expertiza

Expertiza is an open source web application which allows an instructor to manage assignments. It has various features viz. creating new assignments, customizing existing assignments, automatically allocating submissions for peer review etc. It has been developed using the Ruby on Rails framework, and the code is available on Github.

Project Goals

In this project, the files late_policies_controller.rb and penalty_helper.rb were to be refactored. The methods contained in these files were too long, and needed to be either shortened, or used polymorphically, or deleted altogether if they were not being used. They also were to be refactored semantically if the code did not follow good Ruby coding practices.

Functionality

Late policies and penalties functionality

After an instructor creates an assignment in Expertiza they will have the ability to edit the details of the assignment by clicking the Edit action icon beside the assignment. From the "Editing Assignment" screen instructors have the ability to set due dates for each round of an assignment on the "Due dates" tab. On this tab, the instructor can elect to apply a late policy to the assignment by checking the "Apply penalty policy" checkbox and selecting a late policy from the adjacent drop down menu. If no late policies exist, or if the instructor wishes to define a new late policy, then they can click the "New late policy" link to define a new late policy.

An instructor can define a late policy with the following fields:

  • Late policy name - This name will show up in the drop down menu on the "Due dates" tab when editing the assignment. It is not enforced to be unique.
  • Penalty Unit - With the options of 'Minute', 'Hour', and 'Day' this field will define the frequency with which penalty points are deducted if an assignment is submitted after a due date.
  • Penalty Point Per Unit - This is the amount of points which will be deducted from the student's score for the assignment round every time the Penalty Unit time has elapsed between the due date and the submission date. [Range: > 50]
  • Maximum Penalty - Points will continue to be deducted from the assignment round score for each Penalty Unit time that has elapsed, but the number of points deducted will not exceed the Maximum Penalty [Range: 1..50]

Once an assignment has a late policy applied to an assignment, students' submissions will be checked against deadlines to determine if the late policy should be enforced. The penalty is applied to the grades under two circumstances. When a student is viewing an assignment task and they click on the "Your scores" link, the applicable penalties will be applied before displaying. Also, when an instructor is viewing the list of assignments on the "Manage content" page, if they click the icon for "View scores" the penalties will be applied to all submissions within the list which were submitted past the due date.


Change specifics

The penalty_helper.rb and late_policies_controller.rb files are central to the creation and maintenance of late policies. Late policies are created by instructors and associated with assignments. The late_policies_controller.rb file is primarily used in the communication with view where late policies are displayed, created, or modified. The penalty_helper.rb file includes many useful methods related to late policies and the penalties which result from their use. For instance, if an assignment is designated as having penalties calculated then when it is graded the PenaltyHelper module will allow the penalties for each assignment to be calculated and applied to the final grade for the assignment.

This effort was initiated with the intent of refactoring some longer methods with redundant subprocedures so that the overall structure of the code is cleaner, concise, and DRYer. This was achieved by performing analysis on the files to identify methods which are not used within the Expertiza code base, methods which are too long, and redundant code which can be extracted from other methods and replaced with a call to a single method.

Late Policies Controller

This is controller class for late policies. It handles all the basic operations on late policies like create/edit/delete policy. The project goal was to refactor the code of create and update methods to make them smaller and more readable. Three helper methods were added in the PenaltyHelper module to make the code shorter. There was a common input check in both create and update methods which was separated to write two new methods "check_penalty_points_validity" and "check_policy_with_same_name", which helped to reuse the code. 

 #checking that penalty_per_unit is not exceeding max_penalty
 def self.check_penalty_points_validity(max_penalty, penalty_per_unit)
   if max_penalty < penalty_per_unit
     flash[:error] = "The maximum penalty cannot be less than penalty per unit."
     invalid_penalty_per_unit = true
   else
     invalid_penalty_per_unit = false
   end
 end

 #method to check whether the policy name given as a parameter already exists under the current instructor id
 #it return true if there's another policy with the same name under current instructor else false
 def self.check_policy_with_same_name(late_policy_name)
   @policy = LatePolicy.where(policy_name: late_policy_name)
   if !@policy.nil? && !@policy.empty?
     @policy.each do |p|
       next unless p.instructor_id == instructor_id
       return true
     end
   end
   return false
 end

In case of update action call, there was a part of code which was updating already calculated penalty objects based on the updated late policy. This part was written as a separate method in the helper class with name "update_calculated_penalty_objects". 

 #this method updates all the penalty objects which uses the penalty policy which is passed as a parameter
 #whenever a policy is updated, all the existing penalty objects needs to be updated according to new policy
 def self.update_calculated_penalty_objects(penalty_policy)
   @penaltyObjs = CalculatedPenalty.all
   @penaltyObjs.each do |pen|
     @participant = AssignmentParticipant.find(pen.participant_id)
     @assignment = @participant.assignment
     next unless @assignment.late_policy_id == penalty_policy.id
     @penalties = calculate_penalty(pen.participant_id)
     @total_penalty = (@penalties[:submission] + @penalties[:review] + @penalties[:meta_review])
     if pen.deadline_type_id.to_i == 1
       {penalty_points: @penalties[:submission]}
       pen.update_attribute(:penalty_points, @penalties[:submission])
     elsif pen.deadline_type_id.to_i == 2
       {penalty_points: @penalties[:review]}
       pen.update_attribute(:penalty_points, @penalties[:review])
     elsif pen.deadline_type_id.to_i == 5
       {penalty_points: @penalties[:meta_review]}
       pen.update_attribute(:penalty_points, @penalties[:meta_review])
     end
   end
 end

Some variable names were changed in order to make it easier for reader to understand the meaning of code. There was some unused commented code which was removed.

Penalty Helper

This is a helper class which contains methods which calculate the different penalties, and are then brought together in one Hash object. The aim was to keep the methods below 25 lines of code if possible. Out of the numerous methods in the file, there were 3 substantial methods, namely calculate_penalty, calculate_submission_penalty, and compute_penalty_on_reviews crossed the 25 line limit.

calculate_penalty is the main method of the PenaltyHelper module. It is called from the GradesController module. The other methods are called from within the calculate_penalty module. The method mainly contains a few assignment statements, and some method calls. There was no scope for it to be shortened further without risking non-operation of some functionalities.

A particular segment of code converted a time difference into respective unit. This code was inserted into a method with the parameter 'time_difference'. The conversion segment was called in calculate_submission_penalty, as well as compute_penalty_on_reviews. Inserting the conversion logic into a separate method allowed the simplification of both methods. 


 def self.calculate_penalty_units(time_difference, penalty_unit)
   if penalty_unit == 'Minute'
     penalty_units = time_difference / 60
   elsif penalty_unit == 'Hour'
     penalty_units = time_difference / 3600
   elsif penalty_unit == 'Day'
     penalty_units = time_difference / 86_400
   end
 end

The method was called in two places. 

 # Line 44
 time_difference = last_submission_time - submission_due_date
 penalty_units = calculate_penalty_units(time_difference, @penalty_unit)

 # Line 109
 time_difference = review_map_created_at_list.at(i) - review_due_date
 penalty_units = calculate_penalty_units(time_difference, @penalty_unit)

Apart from the method which was defined, some other methods which were not being called were removed, extraneous statements which were written and then commented were also removed. Also, minor modifications in places where code did not follow Ruby grammar were made, for eg: a space was added between 'if' and the condition statement.

Test plan

The project formerly had no tests associated with these source files. Our refactoring added moved functionality out of the controller and into the helper module, therefore our testing focused on validating the helper methods in penalty_helper.rb.

Unit tests

Unit Test Summary
Method Parameters Purpose Tested Scenarios
check_policy_with_same_name late_policy_name; instructor_id Check whether a given late policy name exists for the current instructor. 1) Returns true when passed an existing policy name; 2) Returns false when passed a non-existant policy name
check_penalty_points_validity max_penalty; penalty_per_unit Validate that the max penalty and penalty points per unit provided by the user allow at least one unit to pass before reaching the max. 1) Returns true (indicating invalid values) when the penalty points per unit > max penalty; 2) Returns false (indicating valid values) when the penalty points per unit < max penalty; 3) Returns false (indicating valid values) when the penalty points per unit == max penalty
calculate_penalty_units time_difference; penalty_unit Convert a time difference, given in seconds, into the requested penalty unit (i.e. Minutes, Hours, or Days). 1) Returns 1 minute when passed a time difference of 1 minute; 2) Returns 1 hour when passed a time difference of 1 hour; 3) Returns 1 day when passed a time difference of 1 day;

Peer review validation

Validating application functionality

Instructions on how our reviewers should validate the impacted functionality to ensure that it still works.

Validating code via RSpec tests

Set up Expertiza VCL environment

First, set up your Expertiza environment. Navigate to https://vcl.ncsu.edu and click the "Make a Reservation" button. Sign in with your Unity ID. Click the "Reservations" button and then click "New Reservation". In the environment drop down box select the ' [CSC517, F15] Ruby on Rails / Expertiza' environment, set your session duration, and click the "Create Reservation" button.

Once your reservation is ready you should see a Connect! button. Click it and copy the IP address. If you don't have PuTTY then download it. Open PuTTY and paste the IP address in the hostname field and click "Open." Confirm connection at the security dialog. Log into the server using your Unity ID and password. Once logged in, execute the following commands to setup the Expertiza environment. 

  git clone https://github.com/michaelamoran/expertiza.git
  cd expertiza
  cp ./config/database.yml.example ./config/database.yml
  cp ./config/secrets.yml.example ./config/secrets.yml
  vi ./Gemfile
  # Search for 'pg' and delete the line and save
  bundle install
  rake db:migrate
  curl https://raw.githubusercontent.com/creationix/nvm/v0.13.1/install.sh | bash
  source ~/.bash_profile
  nvm install v7.7.3
  npm install -g bower
  bower install
  
  # The following commands will prepare the test database so the tests don't fail.
  rake db:create RAILS_ENV=test
  rake db:migrate RAILS_ENV=test

Executing tests

Now, to run the tests, make sure you are in the project's root directory (i.e. ~/expertiza/). Run the following command: 

  rspec spec/helpers/penalty_helper_spec.rb

Once it is complete, the output should contain the number of test cases and how man of them passed or failed.

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2017/oss_E1713&oldid=107899"