E2422. Reimplement questionnaire.rb
Problem Description
The project includes re-implementation of the questionnaire.rb found in Expertiza to the Reimplementation-back-end . The current implementation includes various functionalities such as validation checks and methods. The goal is to implement these ensuring the code follows SOLID principles and is DRY in nature.
Important points
- The new model should use rails in built active record validation and eliminate the Validate_questionnaire method.
- The model should check for any overlapping functionalities from the controller and ensure all business logic is present in the model only.
- Eliminate methods and fields from this model that are exclusively utilized in test files so that only the required changes to the model are merged into the main branch.
- Ruby naming conventions should be followed for each method.
- Tests should be performed using Rspec covering all necessary functionalities.
- Comments should be present for every new functionality that is added and ensure to avoid all mistakes mentioned in the checklist document.
- Simplification of code is possible in various functions such as get_weighted_score and compute_weighted_score. For more details check the PR which has changes and ideas used for the implementation by the last team working on it.
- There is already an existing model in the Reimplementation back end . However this is essentially a copy paste from expertiza. This was done by the team working on the controller last spring in 2023. All changes should be made to the same file. Starting from scratch in your fork will be the quickest way to ensure no mix up from the existing implementation.
Methods Reimplemented
Validate(s)
Before refactoring
def validate_questionnaire errors.add(:max_question_score, 'The maximum question score must be a positive integer.') if max_question_score < 1 errors.add(:min_question_score, 'The minimum question score must be a positive integer.') if min_question_score < 0 errors.add(:min_question_score, 'The minimum question score must be less than the maximum.') if min_question_score >= max_question_score
results = Questionnaire.where('id <> ? and name = ? and instructor_id = ?', id, name, instructor_id)
errors.add(:name, 'Questionnaire names must be unique.') if results.present?
end
end
After refactoring
validates :name, presence: true
validate :name_is_unique
validates :max_question_score, numericality: { only_integer: true, greater_than: 0 }, presence: true
validates :min_question_score, numericality: { only_integer: true, greater_than_or_equal_to: 0 }, presence: true
validate :min_less_than_max
def name_is_unique # return if we do not have all the values to check return unless id && name && instructor_id
# check for existing named questionnaire for this instructor that is not this questionnaire
existing = Questionnaire.where('name = ? and instructor_id = ? and id <> ?', name, instructor_id, id)
errors.add(:name, 'must be unique') if existing.present?
end
def min_less_than_max # do we have values if not then do not attempt to validate this return unless min_question_score && max_question_score
errors.add(:min_question_score, 'must be less than max question score') if min_question_score >= max_question_score end
I changed the validate_questionnaire from a method that needs to be called for validation into individual validates in the order I expected so they could each have single responsibility.
get_weighted_score
Before refactoring
def get_weighted_score(assignment, scores)
# create symbol for "varying rubrics" feature -Yang
round = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: id).used_in_round
questionnaire_symbol = if round.nil?
symbol
else
(symbol.to_s + round.to_s).to_sym
end
compute_weighted_score(questionnaire_symbol, assignment, scores)
end
def compute_weighted_score(symbol, assignment, scores)
aq = assignment_questionnaires.find_by(assignment_id: assignment.id)
if scores[symbol][:scores][:avg].nil?
0
else
scores[symbol][:scores][:avg] * aq.questionnaire_weight / 100.0
end
end
After refactoring
def get_weighted_score(assignment, scores) compute_weighted_score(questionnaire_symbol(assignment), assignment, scores) end
def compute_weighted_score(symbol, assignment, scores) aq = assignment_questionnaires.find_by(assignment_id: assignment.id) scores[symbol][:scores][:avg].nil? ? 0 : scores[symbol][:scores][:avg] * aq.questionnaire_weight / 100.0 end
def questionnaire_symbol(assignment)
# create symbol for "varying rubrics" feature
# Credit ChatGPT to help me get from the inline below to the used inline, the yield self allowed me the work I wanted to do
# AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: id)&.used_in_round ? "#{symbol}#{round}".to_sym : symbol
AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: id)&.used_in_round&.yield_self { |round| "#{symbol}#{round}".to_sym } || symbol
end
I changed the two methods into two public methods and one private method. Since the original get_weighted_score got the score and looked up the symbol to use I felt it violated the single use principle. I created a private method to do the lookup of the symbol to use on its own. I created an elegant line of code to attempt to find an assignment questionnaire and if not null and used in a round it yields the result to be used in the return value. If not then it will just use this symbol. I had this line but without the yield as I was attempting to get it doing what I wanted it to do. I asked chatGPT and it presented me with the yield statement which I then used. Now each method only does what it should, gets the results, computes the result, or evaluates the symbol.
true_false_questions?
def true_false_questions?
questions.each { |question| return true if question.type == 'Checkbox' }
false
end
This method was doing the job very succinctly already. I no direct changes to this method and left it as is.
delete
Before refactoring
def delete
assignments.each do |assignment|
raise "The assignment #{assignment.name} uses this questionnaire.
Do you want to <A href='../assignment/delete/#{assignment.id}'>delete</A> the assignment?"
end
questions.each(&:delete)
node = QuestionnaireNode.find_by(node_object_id: id) node.destroy if node
destroy end
After refactoring
def delete
# Check to see if we go further? we cannot proceed if there are any assignments
assignment = assignments.first
if assignment
raise "The assignment #{assignment.name} uses this questionnaire. Do you want to <A href='../assignment/delete/#{assignment.id}'>delete</A> the assignment?"
end
questions.each(&:delete)
node = QuestionnaireNode.find_by(node_object_id: id) node&.destroy
destroy end
I only changed a few pieces of this method. First of all there was no reason to do an each on the list if the first one was going to throw an exception already, so I simplified that. Also just doing a null check on the dereference to the destroy on questionnaire node was more succinct than doing an if.
Required migrations
The following migrations were required to bring the current reimplementation fork up to par with what was needed to match the Expertiza class.
class AddUsedInRoundToAssignmentQuestionnaire < ActiveRecord::Migration[7.0]
def self.up add_column 'assignment_questionnaires', 'used_in_round', :integer end def self.down remove_column 'assignment_questionnaires', 'used_in_round' end
end
class AddQuestionnarieWeightToAssignmentQuestionnaire < ActiveRecord::Migration[7.0]
def self.up add_column 'assignment_questionnaires', 'questionnaire_weight', :integer, default: 0, null: false end def self.down remove_column 'assignment_questionnaires', 'questionnaire_weight' end
end
class CreateNodes < ActiveRecord::Migration[7.0]
def self.up
create_table :nodes do |t|
t.column :parent_id, :integer
t.column :node_object_id, :integer
t.column :type, :string
end
end
def self.down drop_table :nodes end
end
Rspec Testing
Suggested test skeleton
# Here are the skeleton rspec tests to be implemented as well, or to replace existing duplicate tests
describe "#get_weighted_score" do
context "when the assignment has a round" do
it "computes the weighted score using the questionnaire symbol with the round appended" do
# Test case 1
# Given an assignment with an ID and a questionnaire with a symbol
# And the questionnaire is used in a round
# And a set of scores
# When the get_weighted_score method is called with the assignment and scores
# Then it should compute the weighted score using the questionnaire symbol with the round appended
# Test case 2
# Given an assignment with an ID and a questionnaire with a symbol
# And the questionnaire is used in a round
# And a different set of scores
# When the get_weighted_score method is called with the assignment and scores
# Then it should compute the weighted score using the questionnaire symbol with the round appended
end
end
context "when the assignment does not have a round" do
it "computes the weighted score using the questionnaire symbol" do
# Test case 3
# Given an assignment with an ID and a questionnaire with a symbol
# And the questionnaire is not used in a round
# And a set of scores
# When the get_weighted_score method is called with the assignment and scores
# Then it should compute the weighted score using the questionnaire symbol
# Test case 4
# Given an assignment with an ID and a questionnaire with a symbol
# And the questionnaire is not used in a round
# And a different set of scores
# When the get_weighted_score method is called with the assignment and scores
# Then it should compute the weighted score using the questionnaire symbol
end
end
end
describe "#compute_weighted_score" do
context "when the average score is nil" do
it "returns 0" do
# Test scenario
end
end
context "when the average score is not nil" do
it "calculates the weighted score based on the questionnaire weight" do
# Test scenario
end
end
end
describe "#true_false_questions?" do
context "when there are true/false questions" do
it "returns true" do
# Test scenario 1: Multiple questions with type 'Checkbox'
# Test scenario 2: Single question with type 'Checkbox'
end
end
context "when there are no true/false questions" do
it "returns false" do
# Test scenario 1: Multiple questions with no 'Checkbox' type
# Test scenario 2: Single question with no 'Checkbox' type
end
end
end
describe "#delete" do
context "when there are assignments using the questionnaire" do
it "raises an error with a message asking if the user wants to delete the assignment" do
# Test scenario 1
# Given: There are assignments using the questionnaire
# When: The delete method is called
# Then: An error is raised with a message asking if the user wants to delete the assignment
# Test scenario 2
# Given: There are multiple assignments using the questionnaire
# When: The delete method is called
# Then: An error is raised for each assignment with a message asking if the user wants to delete the assignment
end
end
context "when there are no assignments using the questionnaire" do
it "deletes all the questions associated with the questionnaire" do
# Test scenario 1
# Given: There are no assignments using the questionnaire
# When: The delete method is called
# Then: All the questions associated with the questionnaire are deleted
# Test scenario 2
# Given: There are no assignments using the questionnaire and there are multiple questions
# When: The delete method is called
# Then: All the questions associated with the questionnaire are deleted
end
it "deletes the questionnaire node if it exists" do
# Test scenario 1
# Given: There are no assignments using the questionnaire and the questionnaire node exists
# When: The delete method is called
# Then: The questionnaire node is deleted
# Test scenario 2
# Given: There are no assignments using the questionnaire and the questionnaire node does not exist
# When: The delete method is called
# Then: No error is raised and the method completes successfully
end
it "deletes the questionnaire" do
# Test scenario 1
# Given: There are no assignments using the questionnaire
# When: The delete method is called
# Then: The questionnaire is deleted
# Test scenario 2
# Given: There are no assignments using the questionnaire and there are multiple questionnaires
# When: The delete method is called
# Then: The questionnaire is deleted
end
end
end
describe "#max_possible_score" do
context "when the questionnaire has no questions" do
it "returns 0 as the maximum possible score" do
# test code
end
end
context "when the questionnaire has questions with different weights" do
it "returns the correct maximum possible score based on the weights and max_question_score" do
# test code
end
end
context "when the questionnaire has questions with the same weight" do
it "returns the correct maximum possible score based on the weight and max_question_score" do
# test code
end
end
context "when the questionnaire ID does not exist" do
it "returns nil as the maximum possible score" do
# test code
end
end
end
describe '.copy_questionnaire_details' do
context 'when given valid parameters' do
it 'creates a copy of the questionnaire with the instructor_id' do
# Test body
end
it 'sets the name of the copied questionnaire as "Copy of [original name]"' do
# Test body
end
it 'sets the created_at timestamp of the copied questionnaire to the current time' do
# Test body
end
it 'saves the copied questionnaire' do
# Test body
end
it 'creates copies of all the questions from the original questionnaire' do
# Test body
end
it 'sets the questionnaire_id of the copied questions to the id of the copied questionnaire' do
# Test body
end
it 'sets the size of the copied criterion and text response questions to "50,3" if size is nil' do
# Test body
end
it 'saves the copied questions' do
# Test body
end
it 'creates copies of all the question advices associated with the original questions' do
# Test body
end
it 'sets the question_id of the copied question advices to the id of the copied question' do
# Test body
end
it 'saves the copied question advices' do
# Test body
end
it 'returns the copied questionnaire' do
# Test body
end
end
end
describe "#validate_questionnaire" do
context "when the maximum question score is less than 1" do
it "should add an error message" do
# test code
end
end
context "when the minimum question score is less than 0" do
it "should add an error message" do
# test code
end
end
context "when the minimum question score is greater than or equal to the maximum question score" do
it "should add an error message" do
# test code
end
end
context "when a questionnaire with the same name and instructor already exists" do
it "should add an error message" do
# test code
end
end
end
Validate(s)
describe '#name' do
# Test validates the name of the questionnaire
it 'returns the name of the Questionnaire' do
# Act Assert
expect(questionnaire.name).to eq('abc')
expect(questionnaire1.name).to eq('xyz')
expect(questionnaire3.name).to eq('pqr')
end
# Test ensures that the name field of the questionnaire is not blank
it 'Validate presence of name which cannot be blank' do
# Arrange
questionnaire.name = ' '
# Act Assert
expect(questionnaire).not_to be_valid
end
# Test ensures that the name field of the questionnaire is unique per instructor
it 'Validate name field must be unique per instructor' do
# Arrange Act
questionnaire.save!
questionnaire1.name = questionnaire.name
questionnaire3.name = questionnaire.name
instructor2 = Instructor.create(name: 'testinstructortwo', email: 'test2@test.com', full_name: 'Test Instructor 2', password: '123456', role: role)
instructor2.save!
questionnaire3.instructor_id = instructor2.id
# Assert
expect(questionnaire).to be_valid
expect(questionnaire1).not_to be_valid
expect(questionnaire3).to be_valid
end
end
describe '#instructor_id' do
# Test validates the instructor id in the questionnaire
it 'returns the instructor id' do
expect(questionnaire.instructor_id).to eq(instructor.id)
end
end
describe '#maximum_score' do
# Test validates the maximum score in the questionnaire
it 'validate maximum score' do
expect(questionnaire.max_question_score).to eq(10)
end
# Test ensures maximum score is an integer
it 'validate maximum score is integer' do
expect(questionnaire.max_question_score).to eq(10)
questionnaire.max_question_score = 'a'
expect(questionnaire).not_to be_valid
questionnaire.max_question_score = 1.1
expect(questionnaire).not_to be_valid
end
# Test ensures maximum score is positive
it 'validate maximum score should be positive' do
expect(questionnaire.max_question_score).to eq(10)
questionnaire.max_question_score = -10
expect(questionnaire).not_to be_valid
questionnaire.max_question_score = 0
expect(questionnaire).not_to be_valid
end
# Test ensures maximum score is greater than the minimum score it 'validate maximum score should be bigger than minimum score' do
expect(questionnaire).to be_valid
questionnaire.max_question_score = 0
expect(questionnaire).not_to be_valid
questionnaire.max_question_score = 2
questionnaire.min_question_score = 3
expect(questionnaire).not_to be_valid
end
end
describe '#minimum_score' do
# Test validates minimum score of a questionnaire
it 'validate minimum score' do
questionnaire.min_question_score = 5
expect(questionnaire.min_question_score).to eq(5)
end
# Test ensures minimum score is smaller than maximum score
it 'validate minimum should be smaller than maximum' do
expect(questionnaire.min_question_score).to eq(0)
questionnaire.min_question_score = 10
expect(questionnaire).not_to be_valid
questionnaire.min_question_score = 0
end
# Test ensures minimum score is an integer
it 'validate minimum score is integer' do
expect(questionnaire.min_question_score).to eq(0)
questionnaire.min_question_score = 'a'
expect(questionnaire).not_to be_valid
end
end
describe 'associations' do
# Test validates the association that a questionnaire comprises of several questions
it 'has many questions' do
expect(questionnaire.questions).to include(question1, question2)
end
end
get_weighted_score
describe '#get_weighted_score' do
before :each do
@questionnaire = FactoryBot.create(:review_questionnaire, { instructor_id: instructor.id })
@assignment = FactoryBot.create(:assignment)
end
context 'when the assignment has a round' do
it 'computes the weighted score using the questionnaire symbol with the round appended' do
# Test case 1
# Arrange
FactoryBot.create(:assignment_questionnaire, { assignment_id: @assignment.id, questionnaire_id: @questionnaire.id, used_in_round: 1 })
scores = { "#{@questionnaire.symbol}#{1}".to_sym => { scores: { avg: 100 } } }
# Act Assert
expect(@questionnaire.get_weighted_score(@assignment, scores)).to eq(100)
# Test case 2
# Arrange
scores = { "#{@questionnaire.symbol}#{1}".to_sym => { scores: { avg: 75 } } }
# Act Assert
expect(@questionnaire.get_weighted_score(@assignment, scores)).to eq(75)
end
end
context 'when the assignment does not have a round' do
it 'computes the weighted score using the questionnaire symbol' do
# Test case 3
# Arrange
FactoryBot.create(:assignment_questionnaire, { assignment_id: @assignment.id, questionnaire_id: @questionnaire.id })
scores = { @questionnaire.symbol => { scores: { avg: 100 } } }
# Act Assert
expect(@questionnaire.get_weighted_score(@assignment, scores)).to eq(100)
# Test case 4
# Arrange
scores = { @questionnaire.symbol => { scores: { avg: 75 } } }
# Act Assert
expect(@questionnaire.get_weighted_score(@assignment, scores)).to eq(75)
end
end
describe "#compute_weighted_score" do
before :each do
@questionnaire = FactoryBot.create(:review_questionnaire, { instructor_id: instructor.id })
@assignment = FactoryBot.create(:assignment)
FactoryBot.create(:assignment_questionnaire, { assignment_id: @assignment.id, questionnaire_id: @questionnaire.id, questionnaire_weight: 50 })
end
context "when the average score is nil" do
it "returns 0" do
# Test scenario
# Arrange
scores = { @questionnaire.symbol => { scores: { avg: nil } } }
# Act Assert
expect(@questionnaire.compute_weighted_score(@questionnaire.symbol, @assignment, scores)).to eq(0)
end
end
context "when the average score is not nil" do
it "calculates the weighted score based on the questionnaire weight" do
# Test scenario
# Arrange
scores = { @questionnaire.symbol => { scores: { avg: 75 } } }
# Act Assert
expect(@questionnaire.compute_weighted_score(@questionnaire.symbol, @assignment, scores)).to eq(75 * 0.5)
end
end
end
end
true_false_questions?
describe "#true_false_questions?" do
before :each do
@questionnaire = FactoryBot.create(:review_questionnaire, { instructor_id: instructor.id })
end
context "when there are true/false questions" do
it "returns true" do
# Test scenario 2: Single question with type 'Checkbox'
# Arrange
@questionnaire.questions.create(weight: 1, id: 1, seq: 1, txt: "que 1", question_type: "Checkbox", break_before: true)
# Act Assert
expect(@questionnaire.true_false_questions?).to eq(true)
# Test scenario 1: Multiple questions with type 'Checkbox'
# Arrange
@questionnaire.questions.create(weight: 1, id: 2, seq: 1, txt: "que 1", question_type: "Checkbox", break_before: true)
@questionnaire.questions.create(weight: 1, id: 3, seq: 1, txt: "que 1", question_type: "Checkbox", break_before: true)
# Act Assert
expect(@questionnaire.true_false_questions?).to eq(true)
end
end
context "when there are no true/false questions" do
it "returns false" do
# Test scenario 2: Single question with no 'Checkbox' type
# Arrange
@questionnaire.questions.create(weight: 1, id: 1, seq: 1, txt: "que 1", question_type: "Scale", break_before: true)
# Act Assert
expect(@questionnaire.true_false_questions?).to eq(false)
# Test scenario 1: Multiple questions with no 'Checkbox' type
# Arrange
@questionnaire.questions.create(weight: 1, id: 2, seq: 1, txt: "que 1", question_type: "Scale", break_before: true)
@questionnaire.questions.create(weight: 1, id: 3, seq: 1, txt: "que 1", question_type: "Scale", break_before: true)
# Act Assert
expect(@questionnaire.true_false_questions?).to eq(false)
end
end
end
delete
describe "#delete" do
context "when there are assignments using the questionnaire" do
it "raises an error with a message asking if the user wants to delete the assignment" do
# Arrange
questionnaire_single_assignment = FactoryBot.create(:review_questionnaire, { instructor_id: instructor.id })
single_assignment = FactoryBot.create(:assignment)
FactoryBot.create(:assignment_questionnaire, { assignment_id: single_assignment.id, questionnaire_id: questionnaire_single_assignment.id})
questionnaire_multi_assignment = FactoryBot.create(:review_questionnaire, { instructor_id: instructor.id })
multi_assignment1 = FactoryBot.create(:assignment)
FactoryBot.create(:assignment_questionnaire, { assignment_id: multi_assignment1.id, questionnaire_id: questionnaire_multi_assignment.id})
multi_assignment2 = FactoryBot.create(:assignment)
FactoryBot.create(:assignment_questionnaire, { assignment_id: multi_assignment2.id, questionnaire_id: questionnaire_multi_assignment.id})
# Test scenario 1
# Given: There are assignments using the questionnaire
# When: The delete method is called
# Then: An error is raised with a message asking if the user wants to delete the assignment
expect { questionnaire_single_assignment.delete }.to raise_exception(RuntimeError) do |error|
expect(error.message).to match(/^The assignment .* uses this questionnaire/)
end
# Test scenario 2
assignment1_pattern = /^The assignment #{Regexp.escape(multi_assignment1.name)} uses this questionnaire/
assignment2_pattern = /^The assignment #{Regexp.escape(multi_assignment2.name)} uses this questionnaire/
# Given: There are multiple assignments using the questionnaire
# When: The delete method is called
# Then: An error is raised for each assignment with a message asking if the user wants to delete the assignment
expect { questionnaire_multi_assignment.delete }.to raise_exception(RuntimeError) do |error|
expect(error.message).to match(assignment1_pattern)
end
multi_assignment1.destroy!
expect { questionnaire_multi_assignment.delete }.to raise_exception(RuntimeError) do |error|
expect(error.message).to match(assignment2_pattern)
end
end
end
context "when there are no assignments using the questionnaire" do
it "deletes all the questions associated with the questionnaire" do
# Test scenario 1
# Given: There are no assignments using the questionnaire
# When: The delete method is called
# Then: All the questions associated with the questionnaire are deleted
# Arrange
questionnaire_one_question = FactoryBot.create(:review_questionnaire, { instructor_id: instructor.id })
question1 = questionnaire_one_question.questions.build(weight: 1, id: 1, seq: 1, txt: "que 1", question_type: "Scale", break_before: true)
question1.save!
# Act
questionnaire_one_question.delete
# Assert
expect(Question.find_by(id: question1.id)).to be_nil
# Test scenario 2
# Given: There are no assignments using the questionnaire and there are multiple questions
# When: The delete method is called
# Then: All the questions associated with the questionnaire are deleted
# Arrange
questionnaire_multi_question = FactoryBot.create(:review_questionnaire, { instructor_id: instructor.id })
question1 = questionnaire_multi_question.questions.build(weight: 1, id: 1, seq: 1, txt: "que 1", question_type: "Scale", break_before: true)
question1.save!
question2 = questionnaire_multi_question.questions.build(weight: 1, id: 2, seq: 1, txt: "que 1", question_type: "Scale", break_before: true)
question2.save!
# Act
questionnaire_multi_question.delete
# Assert
expect(Question.find_by(id: question1.id)).to be_nil
expect(Question.find_by(id: question2.id)).to be_nil
end
it "deletes the questionnaire node if it exists" do
# Test scenario 1
# Given: There are no assignments using the questionnaire and the questionnaire node exists
# When: The delete method is called
# Then: The questionnaire node is deleted
questionnaire = FactoryBot.create(:review_questionnaire, { instructor_id: instructor.id })
q_node = QuestionnaireNode.create(parent_id: 0, node_object_id: questionnaire.id, type: 'QuestionnaireNode')
# Act
questionnaire.delete
# Assert
expect(QuestionnaireNode.find_by(id: q_node.id)).to be_nil
# Test scenario 2
# Given: There are no assignments using the questionnaire and the questionnaire node does not exist
# When: The delete method is called
# Then: No error is raised and the method completes successfully
questionnaire = FactoryBot.create(:review_questionnaire, { instructor_id: instructor.id })
# Act Assert
expect { questionnaire.delete }.not_to raise_error
expect(Questionnaire.find_by(id: questionnaire.id)).to be_nil
end
The below tests were redundant so I did not implement them from the skeleton of tests.
# it "deletes the questionnaire" do
# Test scenario 1
# Given: There are no assignments using the questionnaire
# When: The delete method is called
# Then: The questionnaire is deleted
# Test scenario 2
# Given: There are no assignments using the questionnaire and there are multiple questionnaires
# When: The delete method is called
# Then: The questionnaire is deleted
# end
end
end