CSC/ECE 517 Fall 2015/ossE1568BZHXJS
Jump to navigation
Jump to search
E1568. Remove AnswersController
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:
RUN TEST with command in terminal: 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.