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 decided to change this 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 made 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 did not have to alter this method very much. 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.
max_possible_score
Before refactoring
def max_possible_score
results = Questionnaire.joins('INNER JOIN questions ON questions.questionnaire_id = questionnaires.id')
.select('SUM(questions.weight) * questionnaires.max_question_score as max_score')
.where('questionnaires.id = ?', id)
results[0].max_score
end
After refactoring
def max_possible_score
## Just return 0 if there are no questions; don't throw an error.
return 0 if questions.empty?
## Sums up the weight of all the questions. This is not necessarily 1.
sum_of_weights = questions.sum{ | question| quesitons.weight}
max_possible_score = sum_of_weights * max_possible_score
end
This refactoring is similar to E2319. Reimplement questionnaire.rb. Instead of using the complicated SQL query, this should be easier to understand and is rather straightforward. Additionally, we added comments to make it very clear what the logic was doing. Adding the error check at the beginning should make the code slightly more robust in the case of questionnaires with no questions, and the variable names have been changed for clarity.
copy_questionnaire_details
Before refactoring
def self.copy_questionnaire_details(params)
orig_questionnaire = Questionnaire.find(params[:id])
questions = Question.where(questionnaire_id: params[:id])
questionnaire = orig_questionnaire.dup
questionnaire.name = 'Copy of ' + orig_questionnaire.name
questionnaire.created_at = Time.zone.now
questionnaire.updated_at = Time.zone.now
questionnaire.save!
questions.each do |question|
new_question = question.dup
new_question.questionnaire_id = questionnaire.id
new_question.save!
end
questionnaire
end
After refactoring
def self.copy_questionnaire_details(params)
orig_questionnaire = find(params[:id])
questionnaire = orig_questionnaire.dup
questionnaire.name = "Copy of #{orig_questionnaire.name}"
questionnaire.created_at = Time.zone.now
questionnaire.updated_at = Time.zone.now
ActiveRecord::Base.transaction do
questionnaire.save!
orig_questionnaire.questions.each do |question|
new_question = question.dup
new_question.questionnaire_id = questionnaire.id
new_question.save!
end
end
questionnaire end
This method handles copying a questionnaire and its attributes and dependencies in the model structure. We find the questionnaire to copy from the database and assign it to orig_questionnaire. In the original code, they used a questions variable to access different questions associated with the original questionnaire. However, we can directly access the questions associated with a questionnaire with how the models are set up. So, it has been eliminated when reimplementing the method. String interpolation was used instead of string concatenation to improve readability and simplicity. It is also less error-prone when compared to concatenation. A big change in the implementation of this method is wrapping the copying process in a transaction. This is done to ensure data consistency. Active Record Transactions are protective blocks where SQL statements are only permanent if they can all succeed as one atomic action. (Ref. https://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html) We started an ActiveRecord transaction to save this new questionnaire and to duplicate the questions associated with the original questionnaire.
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
Questionnaire model
Some of the variables that we will need in all or nearly all tests we have declared at the top within the whole describe for the Questionnaire class tests.
Validate(s)
The following list of tests are to assert all the validations for the class. Using the predefined factory variables we test validations. Are all names what we expect. Validate we cannot have a valid questionnaire without a name. Create two questionnaires with the same name and instructor to ensure we cannot save the second one. Also validate that the questionnaire returns the assigned instructors id.
There are a few tests to run which can help us test the maximum_score method. We can make sure it matches what is expected, we can assert that alpha characters and floats are not valid. We can follow that up with tests to ensure it is a positive integer, and that the max is larger than the minimum.
The final validations are minimum score and associations. The few tests for minimum score are to check that it is what it should be, that it is positive, and cannot be an alpha character.
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?
We decided to use before each to setup the questionnaire that we can use for the duration of this describe. For these tests I did use exclusively the test skeleton provided to us and the scenarios felt like complete tests. Testing both a single and multiple valid checkbox questions assert to true. We also want to validate that single or multiple non checkbox questions assert to false.
delete
It is important to test that deleting a questionnaire also deletes all the questions associated with that questionnaire. In this case we test two situations, first we need to test just one question on the questionnaire, and then multiple questions on the questionnaire.
For the last parts of delete testing I used 2 more of the test skeletons. There are two scenarios to test in relation to the question node associated to a questionnaire. We should assert that questions are deleted with and without an associated node, and that the questionnaire node does not exist any longer in the case where there was an associated node.
copy_questionnaire_details
describe '.copy_questionnaire_details' do
# Test ensures calls from the method copy_questionnaire_details
it 'allowing calls from copy_questionnaire_details' do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(Question).to receive(:where).with(questionnaire_id: '1').and_return([Question])
end
# Test ensures creation of a copy of given questionnaire
# Combined two tests into one to avoid performing the same functionalities again to test another feature.
it 'creates a copy of the questionnaire' do
instructor.save!
questionnaire.save!
question1.save!
question2.save!
# Stub the where method to return the questions associated with the original questionnaire
allow(questionnaire).to receive_message_chain(:questions, :each).and_yield(question1).and_yield(question2)
# Call the copy_questionnaire_details method
copied_questionnaire = Questionnaire.copy_questionnaire_details({ id: questionnaire.id })
# Assertions
expect(copied_questionnaire.instructor_id).to eq(questionnaire.instructor_id)
expect(copied_questionnaire.name).to eq("Copy of #{questionnaire.name}")
expect(copied_questionnaire.created_at).to be_within(1.second).of(Time.zone.now)
# Verify that the copied questionnaire has associated questions
expect(copied_questionnaire.questions.count).to eq(2)
expect(copied_questionnaire.questions.first.txt).to eq(question1.txt)
expect(copied_questionnaire.questions.second.txt).to eq(question2.txt)
end
end
The below tests were redundant
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