CSC/ECE 517 Fall 2015/ossE1568BZHXJS

Introduction

Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). It is used in select courses at NC State and by professors at several other colleges and universities. This page provides a description of the Expertiza based on OSS project. As the project, our team members successfully removed the answer controller, split confusing methods and provided all unit test in the Answer model. Changes can be found in the Answer.rb file which locates in expertiza/app/models/.

Problem Statement

There is a answer.rb file which handle all methods related to answer calculations. But there is no related view file related to AnswerController. All methods in it are show,action_allowed? and calculate_all_penalties. It is obvious that calculate_all_penalties method has already been deployed in GradesController. So we can get rid of calculate_all_penalties in AnswersController. Action_allowed? method is used to set the access authorization for different users.

Changes

Remove AnswersController.rb

Because there is not show view page related to show method. So we can remove AnswerController. This can be checked in the controllers folder.

Refactored self.compute_scores method

As described in OSS project, the self.compute_scores method is complex. Followed the principle that one method only do one thing, we split the the self.compute_scores by adding a method called self.compute_stat to take on some responsibilities from the self.compute_scores method, namely, computing the current score. Specific changes before and after are shown below:

Before changes

 class Answer < ActiveRecord::Base
 belongs_to :question
 # Computes the total score for a *list of assessments*
 # parameters
 #  assessments - a list of assessments of some type (e.g., author feedback, teammate review)
 #  questions - the list of questions that was filled out in the process of doing those assessments
 def self.compute_scores(assessments, questions)
   scores = Hash.new
   if !assessments.nil?&&assessments.length > 0
     scores[:max] = -999999999
     scores[:min] = 999999999
     total_score = 0
     length_of_assessments=assessments.length.to_f
     assessments.each { |assessment|
       curr_score = get_total_score(:response => [assessment], :questions => questions)
       if curr_score > scores[:max]
         scores[:max] = curr_score
       end
       if curr_score < scores[:min]
         scores[:min] = curr_score
       end
       # Check if the review is invalid. If is not valid do not include in score calculation
       if  @invalid==1
         length_of_assessments=length_of_assessments-1
         curr_score=0
       end
       total_score += curr_score
     }
     if (length_of_assessments!=0)
       scores[:avg] = total_score.to_f / length_of_assessments
     else
       scores[:avg]=0
     end
   else
     scores[:max] = nil
     scores[:min] = nil
     scores[:avg] = nil
   end
   return scores
 end

---

After changes

Method "compute_scores"

 # Computes the total score for a *list of assessments*
 # parameters
 #  assessments - a list of assessments of some type (e.g., author feedback, teammate review)
 #  questions - the list of questions that was filled out in the process of doing those assessments
  def self.compute_scores(assessments, questions)
   scores = Hash.new
   if !assessments.nil? && assessments.length > 0
     scores[:max] = -999999999
     scores[:min] = 999999999
     total_score = 0
     length_of_assessments = assessments.length.to_f
     assessments.each do |assessment|
       current_score, scores = compute_stat(assessment, questions, scores, length_of_assessments)
       total_score += current_score
     end
     if length_of_assessments != 0
       scores[:avg] = total_score.to_f / length_of_assessments
     else
       scores[:avg] = 0
     end
   else
     scores[:max] = nil
     scores[:min] = nil
     scores[:avg] = nil
   end
   return scores
 end

Method "compute_stat"

 def self.compute_stat(assessment, questions, scores, length_of_assessments)
   curr_score = get_total_score(:response => [assessment], :questions => questions)
   if curr_score > scores[:max]
     scores[:max] = curr_score
   end
   if curr_score < scores[:min]
     scores[:min] = curr_score
   end
   # Check if the review is invalid. If is not valid do not include in score calculation
   if @invalid == 1
     length_of_assessments = length_of_assessments - 1
     curr_score = 0
   end
   return curr_score, scores
 end

Refactored "submission_valid?" method

We found some parts of codes in the 'self.submission_valid?' method can be taken out as a individual method. To fulfill the function which calculates the latest review phase start time, we named this method as 'latest_review_deadline". See the changes below:

Before changes

   def self.submission_valid?(response)
     if response
       map=ResponseMap.find(response.map_id)
       #assignment_participant = Participant.where(["id = ?", map.reviewee_id])
       @sorted_deadlines = nil
       @sorted_deadlines = DueDate.where(["assignment_id = ?", map.reviewed_object_id]).order('due_at DESC')
       # to check the validity of the response
       if @sorted_deadlines.nil?
         #find the latest review deadline
         #less than current time
         flag = 0
         latest_review_phase_start_time = nil
         current_time = Time.new
         for deadline in @sorted_deadlines
           # if flag is set then we saw a review deadline in the
           # previous iteration - check if this deadline is a past
           # deadline
           if ((flag == 1) && (deadline.due_at <= current_time))
             latest_review_phase_start_time = deadline.due_at
             break
           else
             flag = 0
           end
           # we found a review or re-review deadline - examine the next deadline
           # to check if it is past
           if (deadline.deadline_type_id == 4 ||deadline.deadline_type_id == 2)
             flag = 1
           end
         end
         resubmission_times =   ResubmissionTime.where(participant_id: map.reviewee_id).order('resubmitted_at DESC')
         if response .is_valid_for_score_calculation?(resubmission_times, latest_review_phase_start_time)
           @invalid = 0
         else
           @invalid = 1
         end
         return @invalid
       end
     end
   end

---

After changes

Method "latest_review_deadline"

 def self.latest_review_deadline(sorted_deadlines)
   flag = 0
   latest_review_phase_start_time = nil
   current_time = Time.new
   for deadline in sorted_deadlines
     # if flag is set then we saw a review deadline in the
     # previous iteration - check if this deadline is a past
     # deadline
     if flag == 1 && deadline.due_at <= current_time
       latest_review_phase_start_time = deadline.due_at
       break
     else
       flag = 0
     end
     # we found a review or re-review deadline - examine the next deadline
     # to check if it is past
     if deadline.deadline_type_id == 4 || deadline.deadline_type_id == 2
       flag = 1
     end
   end
   return latest_review_phase_start_time
 end

Method "submission_valid?"

 def self.submission_valid?(response)
   if response
     map=ResponseMap.find(response.map_id)
     #assignment_participant = Participant.where(["id = ?", map.reviewee_id])
     @sorted_deadlines = nil
     @sorted_deadlines = DueDate.where(["assignment_id = ?", map.reviewed_object_id]).order('due_at DESC')
     # to check the validity of the response
     if !@sorted_deadlines.nil?
       latest_review_phase_start_time = latest_review_deadline(@sorted_deadlines)
       resubmission_times = ResubmissionTime.where(participant_id: map.reviewee_id).order('resubmitted_at DESC')
       if response.is_valid_for_score_calculation?(resubmission_times, latest_review_phase_start_time)
         @invalid = 0
       else
         @invalid = 1
       end
       return @invalid
     end
   end
 end

Changed code to Rails 4 format

Line 89 SQL query uses 'find_by_sql' which is not in accord with Rails 4 format and we substituted it with 'where'; which can be reflected in Line 93 in our case.

Rewrote Line 89 long SQL

Since Line 89 SQL is too long, which violates the rule that code should be readable; thus we take the following measures:

    questionnaire_data = ScoreView.where(type: 'Scale', q1_id: @questions[0].questionnaire_id, s_response_id: @response.id)
    questionnaire_data += ScoreView.where(type: 'Criterion', q1_id: @questions[0].questionnaire_id, s_response_id: @response.id)

Unit tests using Rspec

There are six methods in the new version of answer.rb; so we performed six unit tests. The test file for Answer model is answer_spec.rb which can be found in the directory: expertiza/spec/models/ . Below is the unit test code:

Command to run the test: rspec spec/models/answer_spec.rb

   require 'rspec'
   require_relative '../rails_helper'
   
   #Unit test for 'compute_scores'
   describe 'compute_scores' do
     context 'when assessment is not nil' do
       it 'should return valid scores' do
         question=Question.new(
             txt: "qusetionaaaaa",
             weight: 1,
             questionnaire_id: 200,
             type: "Criterion",
             break_before: true)
         assessment = 'Assessment'
         scores1 = {max: 100, min: 100, avg: 100}
         allow(Answer).to receive(:compute_stat).and_return([100, scores1])
         expect(Answer.compute_scores([assessment], [question])).to eq scores1
       end
     end
   
     context 'when assessment is nil' do
       it 'should return nil for score hash' do
         scores2 = {max: nil, min: nil, avg: nil}
         expect(Answer.compute_scores(nil, nil)).to eq scores2
       end
     end
   end
   
   
   
   #Unit test for 'computer_quiz_scores'
   describe 'compute_quiz_scores' do
     before(:each) {
       allow_message_expectations_on_nil
     }
   
     context 'when responses is not nil' do
       it 'should return valid scores' do
         responses=Response.new
         responses.id=1000
         responses.created_at = DateTime.current
         responses.updated_at = DateTime.current
         responses.map_id=1
         responses.additional_comment="additional_comment"
         responses.version_num=1
               
         allow(QuizQuestionnaire).to receive(:find)
         allow(nil).to receive(:questions)
         allow(nil).to receive(:reviewed_object_id)
         allow(Answer).to receive(:get_total_score).and_return(100)
         scores1 = {max: 100, min: 100, avg: 100}
         expect(Answer.compute_quiz_scores([responses])).to eq scores1
       end
     end
   
     context 'when responses is empty' do
       it 'should return nil for score hash' do
         responses = []
         scores2 = {max: nil, min: nil, avg: nil}
         expect(Answer.compute_quiz_scores(responses)).to eq scores2
       end
     end
   end
   
   
   
   #Unit test for 'get_total_score'
   describe 'get_total_score' do
     before(:each) {
       allow(Answer).to receive(:submission_valid?)
       @responses=Response.new
       @responses.id=1000
       @responses.created_at = DateTime.current
       @responses.updated_at = DateTime.current
       @responses.map_id=1
       @responses.additional_comment="additional_comment"
       @responses.version_num=1
       @question=Question.new(
           txt: "qusetionaaaaa",
           weight: 1,
           questionnaire_id: 200,
           type: "Criterion",
           break_before: true)
     }
   
     it 'should return weighted total score when sum_of_weights > 0 && max_question_score' do
       score = ScoreView.new(:type => 'Criterion',
                             :q1_id => @question.questionnaire_id,
                             :s_response_id => @responses.id,
                             :question_weight => 1,
                             :s_score => 5,
                             :q1_max_question_score => 5)
       allow(ScoreView).to receive(:where).and_return([score])
       expect(Answer.get_total_score(:response => [@responses], :questions => [@question])).to eq 100
     end
   
     it 'should return -1 when sum_of_weights <= 0 or max_question_score does not exist' do
       expect(Answer.get_total_score(:response => [@responses], :questions => [@question])).to eq -1
     end
   
   end
   
   #Unit test for 'compute_stat'
   describe 'compute_stat' do
     before(:each) {
       @scores = {max: -999999999, min: 999999999}
       allow(Answer).to receive(:get_total_score).and_return(100)
     }
   
     context "when invalid is 1" do
       it 'should return current score and scores' do
         Answer.instance_variable_set(:@invalid, 1)
         expect(Answer.compute_stat(nil, nil, @scores, 5)).to eq [0, @scores]
       end
     end
   
     context "when invalid is 0" do
       it 'should return current score and scores' do
         Answer.instance_variable_set(:@invalid, 0)
         expect(Answer.compute_stat(nil, nil, @scores, 5)).to eq [100, @scores]
       end
     end
   end
   
   #Unit test for 'submission valid'
   describe 'submission valid' do
     before(:each) {
       allow_message_expectations_on_nil
       late_due = DueDate.new(due_at: Time.parse("2020-10-30"), deadline_type_id: 2)
       early_due = DueDate.new(due_at: Time.parse("2010-10-30"), deadline_type_id: 2)
       sorted_deadlines = [late_due, early_due]
   
       @responses=Response.new
       @responses.id=1000
       @responses.created_at = DateTime.current
       @responses.updated_at = DateTime.current
       @responses.map_id=1
       @responses.additional_comment="additional_comment"
       @responses.version_num=1
   
       map=double(:ResponseMap)
       allow(ResponseMap).to receive(:find).and_return(map)
       allow(map).to receive(:reviewed_object_id)
       allow(map).to receive(:reviewee_id)
       allow(DueDate).to receive(:where).and_return(sorted_deadlines)
       allow(sorted_deadlines).to receive(:order).and_return(sorted_deadlines)
       allow(ResubmissionTime).to receive(:where)
       allow(nil).to receive(:order)
       allow(Answer).to receive(:latest_review_deadline)
     }
   
     it 'invalid should be 1' do
       allow(@responses).to receive(:is_valid_for_score_calculation?).and_return(false)
       expect(Answer.submission_valid?(@responses)).to eq 1
     end
   
     it 'invalid should be 0' do
       allow(@responses).to receive(:is_valid_for_score_calculation?).and_return(true)
       expect(Answer.submission_valid?(@responses)).to eq 0
     end
   end
   
   #Unit test for 'latest review deadline'
   describe 'latest review deadline' do
     late_due = DueDate.new(due_at: Time.parse("2020-10-30"), deadline_type_id: 2)
     early_due = DueDate.new(due_at: Time.parse("2010-10-30"), deadline_type_id: 2)
     sorted_deadlines = [late_due, early_due]
   
     it 'should return early due date' do
       expect(Answer.latest_review_deadline(sorted_deadlines)).to eq early_due.due_at
     end
   end

Test from UI

To verify that our code did not break the original functions. Sign in as instructor6 with password of 'password' and go to 'Assignments' then pick an assignment; and check scores. The result remains same.

Relative Links

1. Expertiza on GitHub
2. GitHub Project Repository
3. Deployment
4. Pull Request on GitHub