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

1. The new model should use rails in built active record validation and eliminate the Validate_questionnaire method.
2. The model should check for any overlapping functionalities from the controller and ensure all business logic is present in the model only.
3. 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.
4. Ruby naming conventions should be followed for each method.
5. Tests should be performed using Rspec covering all necessary functionalities.
6. Comments should be present for every new functionality that is added and ensure to avoid all mistakes mentioned in the checklist document.
7. 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.
8. 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

I have removed the unnecessary assignment of questions variable since we can directly access questions associated with the original questionnaire. I used string interpolation, instead of concatenation, for setting the name of the copied questionnaire. I wrapped the copying process in a transaction to ensure data consistency. Transactions are protective blocks where SQL statements are only permanent if they can all succeed as one atomic action.

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

---

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