Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Spring 2024 - E2447. Reimplement Advice Controller

2024-04-24T08:35:20Z

Bdevine2: /* Pull Request */

== '''About Expertiza''' ==
[https://expertiza.ncsu.edu/ Expertiza] is a software which benefits both instructors and students by providing platform for various types of submissions and providing reusable objects for peer review. Expertiza is an open-source project developed on [https://rubyonrails.org/ Ruby on Rails] framework. In Expertiza an instructors can not only create and customize new or existing assignments, but he/she can also create a list of topics and subject in which the students can sign up. Along with that, Students can form their teams and groups to work with on various projects and assignments. Students can also peer-review other students' submissions. This enables students to work together to improve each other’s learning experiences by providing feedbacks.

== '''Project Description''' ==
This project involves implementing the AdviceController into the reimplementation repository. The controller has limited functionality where it performs validation checks on advice. We will also be making a few refactoring changes and ensuring full test coverage of the methods on the controller.

Advice_controller first checks whether current user has TA privileges or not by implementing action_allowed? method. Secondly it sets the number of advices based on score and sort it in descending order. Then it checks four conditions for the advices.

Methods
* '''action_allowed?'''
* '''invalid_advice?'''
* '''edit_advice'''
* '''save_advice'''

== '''Methods''' ==

==== '''invalid_advice?''' ====
Here is the current implementation of invalid_advice?
<pre>
# checks whether the advices for a question in questionnaire have valid attributes
# return true if the number of advices and their scores are invalid, else returns false
def invalid_advice?(sorted_advice, num_advices, question)
return ((question.question_advices.length != num_advices) ||
sorted_advice.empty? ||
(sorted_advice[0].score != @questionnaire.max_question_score) ||
(sorted_advice[sorted_advice.length - 1].score != @questionnaire.min_question_score))
end
</pre>

'''Plans to Refactor'''
# First, the comments are a bit hard to read, but those can easily be switched.
# We want to split the checks up into different methods as per the Single Responsibliity Principle.
# We do not believe there needs to be any logic changes and all the checks are valuable, and we could not come up with any other

Here is the new implementation of invalid_advice
<pre>
## This method will return true if the advice and its scores is invalid.
# Validates by utilizing the private methods invalid_advice_length? and invalid_advice_scores?
def invalid_advice?(sorted_advice, num_advices, question)
invalid_advice_length?(num_advices, question, sorted_advice) ||
invalid_advice_scores?(sorted_advice)
end

## Checks to see if the advice is the correct length.
# Checks to see if the number of advices is different than the question_advices or advice is empty
def invalid_advice_length?(num_advices, question, sorted_advice)
question.question_advices.length != num_advices ||
sorted_advice.empty?
end

## Checks to see if the scores are valid
# Checks to see if the first and last index of the sorted_advice array are different than expected.
def invalid_advice_scores?(sorted_advice)
sorted_advice[0].score != @questionnaire.max_question_score ||
sorted_advice[sorted_advice.length - 1].score != @questionnaire.min_question_score
end

</pre>

Note that the two new methods invalid_advice_length? and invalid_advice_scores? are in the private field as no other methods need to access them. Additionally, this will not affect any testing which makes refactoring the tests later down the line easier.

==== '''save_advice''' ====
Here is the current implementation of save_advice
<pre>
# save the advice for a questionnaire
def save_advice
# Stores the questionnaire with given id in URL
@questionnaire = Questionnaire.find(params[:id])
begin
# checks if advice is present or not
unless params[:advice].nil?
params[:advice].keys.each do |advice_key|
# Updates the advice corresponding to the key
QuestionAdvice.update(advice_key, advice: params[:advice][advice_key.to_sym][:advice])
end
flash[:notice] = 'The advice was successfully saved!'
end
rescue ActiveRecord::RecordNotFound
# If record not found, redirects to edit_advice
render action: 'edit_advice', id: params[:id]
end
redirect_to action: 'edit_advice', id: params[:id]
end
</pre>

'''Plans to refactor'''
# Simplify the flow since all paths lead to redirect_to 'edit_advice' we can remove the rescue call and replace that with a failed flash message
# Refactor the .each to iterate over params and more clearly handle the update of advice

'''Refactored code''' - Refactored code [https://github.com/AdrYne01/reimplementation-back-end/commit/8b5db0cd60a23c3b032eda025bec863aa059efaa [Commit]]
<pre>
def save_advice
begin
# checks if advice is present or not
unless params[:advice].nil?
params[:advice].each do |advice_key, advice_params|
# get existing advice to update by key with the passed in advice param
QuestionAdvice.update(advice_key, advice: advice_params.slice(:advice)[:advice])
end
# we made it here so it was saved
flash[:notice] = 'The advice was successfully saved!'
end
rescue ActiveRecord::RecordNotFound
# If record not found, redirects to edit_advice and sends flash
flash[:notice] = 'The advice record was not found and saved!'
end
# regardless of action above redirect to edit and show flash message if one exists
redirect_to action: 'edit_advice', id: params[:id]
end
</pre>

==== '''edit_advice''' ====
Here is the current implementation of edit_advice
<pre>
def edit_advice
# Stores the questionnaire with given id in URL
@questionnaire = Questionnaire.find(params[:id])

# For each question in a quentionnaire, this method adjusts the advice size if the advice size is <,> number of advices or
# the max or min score of the advices does not correspond to the max or min score of questionnaire respectively.
@questionnaire.questions.each do |question|
# if the question is a scored question, store the number of advices corresponding to that question (max_score - min_score), else 0
num_advices = if question.is_a?(ScoredQuestion)
@questionnaire.max_question_score - @questionnaire.min_question_score + 1
else
0
end

# sorting question advices in descending order by score
sorted_advice = question.question_advices.sort_by { |x| x.score }.reverse

# Checks the condition for adjusting the advice size
if invalid_advice?(sorted_advice, num_advices, question)
# The number of advices for this question has changed.
QuestionnaireHelper.adjust_advice_size(@questionnaire, question)
end
end
end
</pre>

'''Plans to refactor'''
# We can have separate functions to do all the different operations in the edit_advice method.
# Have separate functions for calculating num_advices and sorting the questions by score
# This is done to make sure the edit_advice method follows the Single Responsibility Principle.
# While sorting questions, we can use sort_by(&:score) instead of using a block. It is a shorthand notation and avoids creating a new Proc object for every element in the collection of the questions.
# Refactor the 'if' statement into a ternary operator while calculating the number of advice. This accomplishes the same logic more concisely.

'''Refactored code''' Refactored code [https://github.com/AdrYne01/reimplementation-back-end/commit/a1e26c0679f71df65b18961ba7e26da1deb04fe1 [Commit]]
<pre>
# Modify the advice associated with a questionnaire
# Separate methods were introduced to calculate the number of advices and sort the advices related to the current question attribute
# This is done to adhere to Single Responsibility Principle
def edit_advice
# For each question in a questionnaire, this method adjusts the advice size if the advice size is <,> number of advices or
# the max or min score of the advices does not correspond to the max or min score of questionnaire respectively.
@questionnaire.questions.each do |question|
# if the question is scored, store the number of advices corresponding to that question (max_score - min_score), else 0
# # Call to a separate method to adhere to Single Responsibility Principle
num_advices = calculate_num_advices(question)

# sorting question advices in descending order by score
# Call to a separate method to adhere to Single Responsibility Principle
sorted_advice = sort_question_advices(question)

# Checks the condition for adjusting the advice size
if invalid_advice?(sorted_advice, num_advices, question)
# The number of advices for this question has changed.
QuestionnaireHelper.adjust_advice_size(@questionnaire, question)
end
end
end

# Function to calculate number of advices for the current question attribute based on max and min question score.
# Method name is consistent with the functionality
# Refactored the 'if' statement into a ternary operator. This accomplishes the same logic more concisely.
def calculate_num_advices(question)
question.is_a?(ScoredQuestion) ? @questionnaire.max_question_score - @questionnaire.min_question_score + 1 : 0
end

# Function to sort question advices related to the current question attribute
# While sorting questions, sort_by(&:score) is used instead of using a block. It is a shorthand notation and avoids creating a new Proc object for every element in the collection of the questions.
def sort_question_advices(question)
question.question_advices.sort_by(&:score).reverse
end
</pre>

====== Note 1 ======
After completing the initial refactoring there was an issue with one item that was clearly not '''DRY'''. The line of code @questionnaire = Questionnaire.find(params[:id]) which was in both the save and edit methods.

A private method set_questionnaire was created and assigned in a before_action at the top of the controller. The following is the refactored pieces and the code [https://github.com/AdrYne01/reimplementation-back-end/commit/e882f7b49c9edad41705d5c6b7582936069fa279 [Commit]]
<pre>
before_action :set_questionnaire, only: %i[ edit_advice save_advice ]

def set_questionnaire
# Stores the questionnaire with given id in URL
@questionnaire = Questionnaire.find(params[:id])
end
</pre>

== '''Testing (rspec)''' ==

==== '''invalid_advice''' ====
Here are the current tests for invalid_advice? method. As mentioned before, if this method is changed to valid_advice?, the tests will be changed and flipped accordingly. There are some edge cases that we will add like what question advice score = max score of questionnaire or advice score = min score of questionnaire.

<pre>
context "when invalid_advice? is called with question advice score > max score of questionnaire" do
# max score of advice = 3 (!=2)
let(:questionAdvice1) {build(:question_advice, id:1, score: 1, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 3, question_id: 1, advice: "Advice2")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2])], max_question_score: 2)
end

it "invalid_advice? returns true when called with incorrect maximum score for a question advice" do
sorted_advice = questionnaire.questions[0].question_advices.sort_by { |x| x.score }.reverse
num_advices = questionnaire.max_question_score - questionnaire.min_question_score + 1
controller.instance_variable_set(:@questionnaire,questionnaire)
expect(controller.invalid_advice?(sorted_advice,num_advices,questionnaire.questions[0])).to eq(true)
end
end

context "when invalid_advice? is called with question advice score < min score of questionnaire" do
# min score of advice = 0 (!=1)
let(:questionAdvice1) {build(:question_advice, id:1, score: 0, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 2, question_id: 1, advice: "Advice2")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2])], max_question_score: 2)
end

it "invalid_advice? returns true when called with incorrect minimum score for a question advice" do
sorted_advice = questionnaire.questions[0].question_advices.sort_by { |x| x.score }.reverse
num_advices = questionnaire.max_question_score - questionnaire.min_question_score + 1
controller.instance_variable_set(:@questionnaire,questionnaire)
expect(controller.invalid_advice?(sorted_advice,num_advices,questionnaire.questions[0])).to eq(true)
end
end

context "when invalid_advice? is called with number of advices > (max-min) score of questionnaire" do
# number of advices > 2
let(:questionAdvice1) {build(:question_advice, id:1, score: 1, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 2, question_id: 1, advice: "Advice2")}
let(:questionAdvice3) {build(:question_advice, id:3, score: 2, question_id: 1, advice: "Advice3")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2,questionAdvice3])], max_question_score: 2)
end

it "invalid_advice? returns true when called with incorrect number of question advices" do
sorted_advice = questionnaire.questions[0].question_advices.sort_by { |x| x.score }.reverse
num_advices = questionnaire.max_question_score - questionnaire.min_question_score + 1
controller.instance_variable_set(:@questionnaire,questionnaire)
expect(controller.invalid_advice?(sorted_advice,num_advices,questionnaire.questions[0])).to eq(true)
end
end
</pre>

While these test are efficient, we wanted to more closely align the tests with the auto generated test skeleton. Additionally, some of the tests are repetitive and would make refactoring more challenging as duplicate tests create an unnecessary headache.

Here are the tests after refactoring:

<pre>
describe "#invalid_advice?" do
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1) }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 3, question_id: 1) }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end
context "when the sorted advice is empty" do
it "returns true" do
## Set sorted advice to empty.
sorted_advice = []
num_advices = 5
expect(controller.invalid_advice?(sorted_advice, num_advices, questionnaire.questions[0])).to be_truthy
end
end

context "when the number of advices does not match the expected number" do
it "returns true" do
## Set sorted advice to be different length the num_advices.
sorted_advice = [questionAdvice1, questionAdvice2]
num_advices = 1
expect(controller.invalid_advice?(sorted_advice, num_advices, questionnaire.questions[0])).to be_truthy
end
end

context "when the highest scoring advice does not have the maximum question score" do
it "returns true" do
# Create a question advice with a score lower than the maximum question score
questionAdvice1 = build(:question_advice, id: 1, score: 3, question_id: 1)
questionnaire = build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1])], max_question_score: 5)

num_advices = questionnaire.max_question_score - questionnaire.min_question_score + 1
sorted_advice = questionnaire.questions[0].question_advices.sort_by(&:score).reverse

expect(controller.invalid_advice?(sorted_advice, num_advices, questionnaire.questions[0])).to be_truthy
end
end
context "when the lowest scoring advice does not have the minimum question score" do
it "returns true" do
# Create a question advice with a score higher than the minimum question score
questionAdvice1 = build(:question_advice, id: 1, score: 1, question_id: 1) # Assuming minimum question score is 2
questionnaire = build(:questionnaire, id: 1, min_question_score: 2,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1])], max_question_score: 5)

num_advices = questionnaire.max_question_score - questionnaire.min_question_score + 1
sorted_advice = questionnaire.questions[0].question_advices.sort_by(&:score).reverse

expect(controller.invalid_advice?(sorted_advice, num_advices, questionnaire.questions[0])).to be_truthy
end
end

context 'when invalid_advice? is called with all conditions satisfied' do
# Question Advices passing all conditions
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end

it 'invalid_advice? returns false when called with all correct pre-conditions ' do
sorted_advice = questionnaire.questions[0].question_advices.sort_by { |x| x.score }.reverse
num_advices = questionnaire.max_question_score - questionnaire.min_question_score + 1
controller.instance_variable_set(:@questionnaire, questionnaire)
expect(controller.invalid_advice?(sorted_advice, num_advices, questionnaire.questions[0])).to eq(false)
end
end

end
</pre>

==== '''save_advice''' ====
Here are the current tests for save_advice. They do have complete coverage for the testing save_advice which we will test before and after the refactoring. We will be modifying the does not save piece to ensure the new flash message on failure shows.

<pre>
describe '#save_advice' do
context "when save_advice is called" do
# When ad advice is saved
let(:questionAdvice1) {build(:question_advice, id:1, score: 1, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 2, question_id: 1, advice: "Advice2")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2])], max_question_score: 2)
end

it "saves advice successfully" do
# When an advice is saved successfully
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with('1',{:advice => "Hello"}).and_return("Ok")
params = {advice: {"1" => {:advice => "Hello"}}, id: 1}
session = {user: instructor1}
result = get :save_advice, params: params, session: session
expect(flash[:notice]).to eq('The advice was successfully saved!')
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end

it "does not save the advice" do
# When an advice is not saved
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with(any_args).and_return("Ok")
params = {id: 1}
session = {user: instructor1}
result = get :save_advice, params: params, session: session
expect(flash[:notice]).not_to be_present
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end
end
end
</pre>

Here are the tests after refactoring with the test skeleton and this is the [https://github.com/AdrYne01/reimplementation-back-end/commit/3be59d5a6edd020e00bf5bba3c97fffae4538aa0 [Commit]] with the changes.

<pre>
describe '#save_advice' do
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end
context 'when the advice is present' do
it 'updates the advice for each key' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with('1', { advice: 'Hello' }).and_return('Ok')
allow(QuestionAdvice).to receive(:update).with('2', { advice: 'Goodbye' }).and_return('Ok')
# Add some advice parameters that will allow for update success
advice_params = {
'1' => { advice: 'Hello' },
'2' => { advice: 'Goodbye' }
}
params = { advice: advice_params, id: 1 }
session = { user: instructor1 }

# Act
result = get(:save_advice, params:, session:)

# Assert
# check each key to see if it received update
# Always expect redirect
advice_params.keys.each do |advice_key|
expect(QuestionAdvice).to have_received(:update).with(advice_key, advice: advice_params[advice_key][:advice])
end
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end

it 'sets a success flash notice' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with('1', { advice: 'Hello' }).and_return('Ok')
allow(QuestionAdvice).to receive(:update).with('2', { advice: 'Goodbye' }).and_return('Ok')
# Add some advice parameters that will allow for update success
params = { advice: {
'1' => { advice: 'Hello' },
'2' => { advice: 'Goodbye' }
}, id: 1 }
session = { user: instructor1 }

# Act
get(:save_advice, params:, session:)

# Assert
expect(flash[:notice]).to eq('The advice was successfully saved!')
end
end

context 'when the advice is not present' do
it 'does not update any advice' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with(any_args).and_return('Ok')
# no advice parameter
params = { id: 1 }
session = { user: instructor1 }

# Act
result = get(:save_advice, params:, session:)

# Assert
# Expect no update to be called with nil params for advice
# Expect no flash
# Always expect redirect
expect(QuestionAdvice).not_to have_received(:update)
expect(flash[:notice]).not_to be_present
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end
end

context 'when the questionnaire is not found' do
it 'renders the edit_advice view' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with(any_args).and_raise(ActiveRecord::RecordNotFound)

#Act
# call get on edit_advice with params that will hit the record not found path
get :edit_advice, params: { advice: {
'1' => { advice: 'Hello' },
'2' => { advice: 'Goodbye' }
}, id: 1 }

# Assert: Verify that the controller renders the correct view
expect(response).to render_template('edit_advice')
end
end

it 'redirects to the edit_advice action' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with(any_args).and_return('Ok')
params = { advice: {
'1' => { advice: 'Hello' },
'2' => { advice: 'Goodbye' }
}, id: 1 }
session = { user: instructor1 }

# Act
result = get(:save_advice, params:, session:)

# Assert
# expect 302 redirect and for it to redirect to edit_advice
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end
end
</pre>

==== '''edit_advice''' ====
Here are the current tests for edit_advice. The current tests cover most of the functionalities of the edit_advice method. Tests can be added to evaluate the impact of invalid_advice? method on the edit_advice method and checking if the number of advice is correctly calculated based on the questionnaire's min and max scores. We can also add tests to check the actions of the controller when the invalid_advice? method returns false.

<pre>
describe '#edit_advice' do

context "when edit_advice is called and invalid_advice? evaluates to true" do
# edit advice called
let(:questionAdvice1) {build(:question_advice, id:1, score: 1, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 2, question_id: 1, advice: "Advice2")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2])], max_question_score: 2)
end

it "edit advice redirects correctly when called" do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
params = {id: 1}
session = {user: instructor1}
result = get :edit_advice, params: params, session: session
expect(result.status).to eq 200
expect(result).to render_template(:edit_advice)
end
end
end
</pre>

Here are the tests after refactoring with the test skeleton
<pre>
describe '#edit_advice' do

context 'when edit_advice is called and invalid_advice? evaluates to true' do
# edit advice called
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end

it 'edit advice redirects correctly when called' do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
params = { id: 1 }
session = { user: instructor1 }
result = get(:edit_advice, params:, session:)
expect(result.status).to eq 200
expect(result).to render_template(:edit_advice)
end
end

context 'when advice adjustment is not necessary' do
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end

it 'does not adjust advice size when called' do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(controller).to receive(:invalid_advice?).and_return(false)
expect(QuestionnaireHelper).not_to receive(:adjust_advice_size)
get :edit_advice, params: { id: 1 }
end
end
context "when the advice size needs adjustment" do
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end

before do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(controller).to receive(:invalid_advice?).and_return(true)
end

it "calculates the number of advices for each question" do
expect(controller).to receive(:calculate_num_advices).once # Assuming there are two questions in the questionnaire
get :edit_advice, params: { id: 1 }
end

it "sorts question advices in descending order by score" do
expect(controller).to receive(:sort_question_advices).once # Assuming there are two questions in the questionnaire
get :edit_advice, params: { id: 1 }
end

it "adjusts the advice size if the number of advices is less than the max score of the questionnaire" do
allow(controller).to receive(:calculate_num_advices).and_return(1) # Assuming only one advice calculated
expect(QuestionnaireHelper).to receive(:adjust_advice_size).with(questionnaire, questionnaire.questions.first)
get :edit_advice, params: { id: 1 }
end

it "adjusts the advice size if the number of advices is greater than the max score of the questionnaire" do
allow(controller).to receive(:calculate_num_advices).and_return(3) # Assuming three advices calculated
expect(QuestionnaireHelper).to receive(:adjust_advice_size).with(questionnaire, questionnaire.questions.first)
get :edit_advice, params: { id: 1 }
end

it "adjusts the advice size if the max score of the advices does not correspond to the max score of the questionnaire" do
allow(controller).to receive(:sort_question_advices).and_return([questionAdvice2, questionAdvice1]) # Assuming advices not sorted correctly
expect(QuestionnaireHelper).to receive(:adjust_advice_size).with(questionnaire, questionnaire.questions.first)
get :edit_advice, params: { id: 1 }
end

it "adjusts the advice size if the min score of the advices does not correspond to the min score of the questionnaire" do
allow(questionnaire).to receive(:min_question_score).and_return(0) # Assuming min score not matching
expect(QuestionnaireHelper).to receive(:adjust_advice_size).with(questionnaire, questionnaire.questions.first)
get :edit_advice, params: { id: 1 }
end
end

context "when the advice size does not need adjustment" do
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end

before do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(controller).to receive(:invalid_advice?).and_return(false)
end

it "does not adjust the advice size if the number of advices is equal to the max score of the questionnaire" do
allow(controller).to receive(:calculate_num_advices).and_return(2) # Assuming two advices calculated
expect(QuestionnaireHelper).not_to receive(:adjust_advice_size)
get :edit_advice, params: { id: 1 }
end

it "does not adjust the advice size if the max score of the advices corresponds to the max score of the questionnaire" do
expect(QuestionnaireHelper).not_to receive(:adjust_advice_size)
get :edit_advice, params: { id: 1 }
end

it "does not adjust the advice size if the min score of the advices corresponds to the min score of the questionnaire" do
expect(QuestionnaireHelper).not_to receive(:adjust_advice_size)
get :edit_advice, params: { id: 1 }
end
end
end
</pre>

====== Testing Note 1 ======
During our initial run of tests with no altered code we were getting a few failure items. The two major ones were related to a missing edit erb file and an issue with the tests finding the method flash.
Upon researching I noticed that the ApplicationController inherited from '''ActionController::API''' which does not contain the method for flash. The refactor fork contained quite a few api based controllers which this makes sense for. This does not make sense for non API related controllers like AdviceController. In my PR I have modified ApplicationController to inherit from '''ActionController::Base''' to allow for our testing to complete.

'''Recommendation''' - Future work that includes api based controllers should have an APIController they inherit from that is '''APIController < ActionController::API'''. The changes made based on this discovery are here in this [https://github.com/AdrYne01/reimplementation-back-end/commit/bebf4a15bbb9d8b5d1d0c6f1ac77c664c261be1b [Commit]]

=== SimpleCov Coverage Report ===
----
The SimpleCov report shows 93.55% relevant LoC for our advice_controller.rb model file.
----
*'''https://adryne01.github.io/Prog4-SimpleCov/Code%20coverage%20for%20Reimplementation-back-end.html'''
[[File:SimpleCovReport.png]] 

== Authors ==
This project was completed by Aiden Bartlett, Brandon Devine, and Aditya Karthikeyan.

== Pull Request ==
https://github.com/expertiza/reimplementation-back-end/pull/106

== Problem Document ==
https://docs.google.com/document/d/1nmJ9ce_Or6hRucsVpq4m_IFN4aKH6N1NepEpMGfGqbM/edit#heading=h.531sshwnqkr

== Video (methods and testing) ==
https://www.youtube.com/watch?v=AhPnEzIC5s4

CSC/ECE 517 Spring 2024 - E2447. Reimplement Advice Controller

2024-04-21T21:22:26Z

Bdevine2: /* Video */

== '''About Expertiza''' ==
[https://expertiza.ncsu.edu/ Expertiza] is a software which benefits both instructors and students by providing platform for various types of submissions and providing reusable objects for peer review. Expertiza is an open-source project developed on [https://rubyonrails.org/ Ruby on Rails] framework. In Expertiza an instructors can not only create and customize new or existing assignments, but he/she can also create a list of topics and subject in which the students can sign up. Along with that, Students can form their teams and groups to work with on various projects and assignments. Students can also peer-review other students' submissions. This enables students to work together to improve each other’s learning experiences by providing feedbacks.

== '''Project Description''' ==
This project involves implementing the AdviceController into the reimplementation repository. The controller has limited functionality where it performs validation checks on advice. We will also be making a few refactoring changes and ensuring full test coverage of the methods on the controller.

Advice_controller first checks whether current user has TA privileges or not by implementing action_allowed? method. Secondly it sets the number of advices based on score and sort it in descending order. Then it checks four conditions for the advices.

Methods
* '''action_allowed?'''
* '''invalid_advice?'''
* '''edit_advice'''
* '''save_advice'''

== '''Methods''' ==

==== '''invalid_advice?''' ====
Here is the current implementation of invalid_advice?
<pre>
# checks whether the advices for a question in questionnaire have valid attributes
# return true if the number of advices and their scores are invalid, else returns false
def invalid_advice?(sorted_advice, num_advices, question)
return ((question.question_advices.length != num_advices) ||
sorted_advice.empty? ||
(sorted_advice[0].score != @questionnaire.max_question_score) ||
(sorted_advice[sorted_advice.length - 1].score != @questionnaire.min_question_score))
end
</pre>

'''Plans to Refactor'''
# First, the comments are a bit hard to read, but those can easily be switched.
# Rather than checking if advice is not invalid, (question.question_advices.length != num_advices) we want to implement this method to be valid_advice where it returns if the length is valid.
# We want to split the checks up into different methods as per the Single Responsibliity Principle.
# We do not believe there needs to be any logic changes and all the checks are valuable, and we could not come up with any other

==== '''save_advice''' ====
Here is the current implementation of save_advice
<pre>
# save the advice for a questionnaire
def save_advice
# Stores the questionnaire with given id in URL
@questionnaire = Questionnaire.find(params[:id])
begin
# checks if advice is present or not
unless params[:advice].nil?
params[:advice].keys.each do |advice_key|
# Updates the advice corresponding to the key
QuestionAdvice.update(advice_key, advice: params[:advice][advice_key.to_sym][:advice])
end
flash[:notice] = 'The advice was successfully saved!'
end
rescue ActiveRecord::RecordNotFound
# If record not found, redirects to edit_advice
render action: 'edit_advice', id: params[:id]
end
redirect_to action: 'edit_advice', id: params[:id]
end
</pre>

'''Plans to refactor'''
# Simplify the flow since all paths lead to redirect_to 'edit_advice' we can remove the rescue call and replace that with a failed flash message
# Refactor the .each to iterate over params and more clearly handle the update of advice

'''Refactored code''' - Refactored code [https://github.com/AdrYne01/reimplementation-back-end/commit/8b5db0cd60a23c3b032eda025bec863aa059efaa [Commit]]
<pre>
def save_advice
begin
# checks if advice is present or not
unless params[:advice].nil?
params[:advice].each do |advice_key, advice_params|
# get existing advice to update by key with the passed in advice param
QuestionAdvice.update(advice_key, advice: advice_params.slice(:advice)[:advice])
end
# we made it here so it was saved
flash[:notice] = 'The advice was successfully saved!'
end
rescue ActiveRecord::RecordNotFound
# If record not found, redirects to edit_advice and sends flash
flash[:notice] = 'The advice record was not found and saved!'
end
# regardless of action above redirect to edit and show flash message if one exists
redirect_to action: 'edit_advice', id: params[:id]
end
</pre>

==== '''edit_advice''' ====
Here is the current implementation of edit_advice
<pre>
def edit_advice
# Stores the questionnaire with given id in URL
@questionnaire = Questionnaire.find(params[:id])

# For each question in a quentionnaire, this method adjusts the advice size if the advice size is <,> number of advices or
# the max or min score of the advices does not correspond to the max or min score of questionnaire respectively.
@questionnaire.questions.each do |question|
# if the question is a scored question, store the number of advices corresponding to that question (max_score - min_score), else 0
num_advices = if question.is_a?(ScoredQuestion)
@questionnaire.max_question_score - @questionnaire.min_question_score + 1
else
0
end

# sorting question advices in descending order by score
sorted_advice = question.question_advices.sort_by { |x| x.score }.reverse

# Checks the condition for adjusting the advice size
if invalid_advice?(sorted_advice, num_advices, question)
# The number of advices for this question has changed.
QuestionnaireHelper.adjust_advice_size(@questionnaire, question)
end
end
end
</pre>

'''Plans to refactor'''
# We can have separate functions to do all the different operations in the edit_advice method.
# Have separate functions for calculating num_advices and sorting the questions by score
# This is done to make sure the edit_advice method follows the Single Responsibility Principle.
# While sorting questions, we can use sort_by(&:score) instead of using a block. It is a shorthand notation and avoids creating a new Proc object for every element in the collection of the questions.
# Refactor the 'if' statement into a ternary operator while calculating the number of advice. This accomplishes the same logic more concisely.

'''Refactored code''' Refactored code [https://github.com/AdrYne01/reimplementation-back-end/commit/a1e26c0679f71df65b18961ba7e26da1deb04fe1 [Commit]]
<pre>
# Modify the advice associated with a questionnaire
# Separate methods were introduced to calculate the number of advices and sort the advices related to the current question attribute
# This is done to adhere to Single Responsibility Principle
def edit_advice
# For each question in a questionnaire, this method adjusts the advice size if the advice size is <,> number of advices or
# the max or min score of the advices does not correspond to the max or min score of questionnaire respectively.
@questionnaire.questions.each do |question|
# if the question is scored, store the number of advices corresponding to that question (max_score - min_score), else 0
# # Call to a separate method to adhere to Single Responsibility Principle
num_advices = calculate_num_advices(question)

# sorting question advices in descending order by score
# Call to a separate method to adhere to Single Responsibility Principle
sorted_advice = sort_question_advices(question)

# Checks the condition for adjusting the advice size
if invalid_advice?(sorted_advice, num_advices, question)
# The number of advices for this question has changed.
QuestionnaireHelper.adjust_advice_size(@questionnaire, question)
end
end
end

# Function to calculate number of advices for the current question attribute based on max and min question score.
# Method name is consistent with the functionality
# Refactored the 'if' statement into a ternary operator. This accomplishes the same logic more concisely.
def calculate_num_advices(question)
question.is_a?(ScoredQuestion) ? @questionnaire.max_question_score - @questionnaire.min_question_score + 1 : 0
end

# Function to sort question advices related to the current question attribute
# While sorting questions, sort_by(&:score) is used instead of using a block. It is a shorthand notation and avoids creating a new Proc object for every element in the collection of the questions.
def sort_question_advices(question)
question.question_advices.sort_by(&:score).reverse
end
</pre>

====== Note 1 ======
After completing the initial refactoring there was an issue with one item that was clearly not '''DRY'''. The line of code @questionnaire = Questionnaire.find(params[:id]) which was in both the save and edit methods.

A private method set_questionnaire was created and assigned in a before_action at the top of the controller. The following is the refactored pieces and the code [https://github.com/AdrYne01/reimplementation-back-end/commit/e882f7b49c9edad41705d5c6b7582936069fa279 [Commit]]
<pre>
before_action :set_questionnaire, only: %i[ edit_advice save_advice ]

def set_questionnaire
# Stores the questionnaire with given id in URL
@questionnaire = Questionnaire.find(params[:id])
end
</pre>

== '''Testing (rspec)''' ==

==== '''invalid_advice''' ====
Here are the current tests for invalid_advice? method. As mentioned before, if this method is changed to valid_advice?, the tests will be changed and flipped accordingly. There are some edge cases that we will add like what question advice score = max score of questionnaire or advice score = min score of questionnaire.

<pre>
context "when invalid_advice? is called with question advice score > max score of questionnaire" do
# max score of advice = 3 (!=2)
let(:questionAdvice1) {build(:question_advice, id:1, score: 1, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 3, question_id: 1, advice: "Advice2")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2])], max_question_score: 2)
end

it "invalid_advice? returns true when called with incorrect maximum score for a question advice" do
sorted_advice = questionnaire.questions[0].question_advices.sort_by { |x| x.score }.reverse
num_advices = questionnaire.max_question_score - questionnaire.min_question_score + 1
controller.instance_variable_set(:@questionnaire,questionnaire)
expect(controller.invalid_advice?(sorted_advice,num_advices,questionnaire.questions[0])).to eq(true)
end
end

context "when invalid_advice? is called with question advice score < min score of questionnaire" do
# min score of advice = 0 (!=1)
let(:questionAdvice1) {build(:question_advice, id:1, score: 0, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 2, question_id: 1, advice: "Advice2")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2])], max_question_score: 2)
end

it "invalid_advice? returns true when called with incorrect minimum score for a question advice" do
sorted_advice = questionnaire.questions[0].question_advices.sort_by { |x| x.score }.reverse
num_advices = questionnaire.max_question_score - questionnaire.min_question_score + 1
controller.instance_variable_set(:@questionnaire,questionnaire)
expect(controller.invalid_advice?(sorted_advice,num_advices,questionnaire.questions[0])).to eq(true)
end
end

context "when invalid_advice? is called with number of advices > (max-min) score of questionnaire" do
# number of advices > 2
let(:questionAdvice1) {build(:question_advice, id:1, score: 1, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 2, question_id: 1, advice: "Advice2")}
let(:questionAdvice3) {build(:question_advice, id:3, score: 2, question_id: 1, advice: "Advice3")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2,questionAdvice3])], max_question_score: 2)
end

it "invalid_advice? returns true when called with incorrect number of question advices" do
sorted_advice = questionnaire.questions[0].question_advices.sort_by { |x| x.score }.reverse
num_advices = questionnaire.max_question_score - questionnaire.min_question_score + 1
controller.instance_variable_set(:@questionnaire,questionnaire)
expect(controller.invalid_advice?(sorted_advice,num_advices,questionnaire.questions[0])).to eq(true)
end
end
</pre>

==== '''save_advice''' ====
Here are the current tests for save_advice. They do have complete coverage for the testing save_advice which we will test before and after the refactoring. We will be modifying the does not save piece to ensure the new flash message on failure shows.

<pre>
describe '#save_advice' do
context "when save_advice is called" do
# When ad advice is saved
let(:questionAdvice1) {build(:question_advice, id:1, score: 1, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 2, question_id: 1, advice: "Advice2")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2])], max_question_score: 2)
end

it "saves advice successfully" do
# When an advice is saved successfully
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with('1',{:advice => "Hello"}).and_return("Ok")
params = {advice: {"1" => {:advice => "Hello"}}, id: 1}
session = {user: instructor1}
result = get :save_advice, params: params, session: session
expect(flash[:notice]).to eq('The advice was successfully saved!')
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end

it "does not save the advice" do
# When an advice is not saved
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with(any_args).and_return("Ok")
params = {id: 1}
session = {user: instructor1}
result = get :save_advice, params: params, session: session
expect(flash[:notice]).not_to be_present
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end
end
end
</pre>

Here are the tests after refactoring with the test skeleton and this is the [https://github.com/AdrYne01/reimplementation-back-end/commit/3be59d5a6edd020e00bf5bba3c97fffae4538aa0 [Commit]] with the changes.

<pre>
describe '#save_advice' do
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end
context 'when the advice is present' do
it 'updates the advice for each key' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with('1', { advice: 'Hello' }).and_return('Ok')
allow(QuestionAdvice).to receive(:update).with('2', { advice: 'Goodbye' }).and_return('Ok')
# Add some advice parameters that will allow for update success
advice_params = {
'1' => { advice: 'Hello' },
'2' => { advice: 'Goodbye' }
}
params = { advice: advice_params, id: 1 }
session = { user: instructor1 }

# Act
result = get(:save_advice, params:, session:)

# Assert
# check each key to see if it received update
# Always expect redirect
advice_params.keys.each do |advice_key|
expect(QuestionAdvice).to have_received(:update).with(advice_key, advice: advice_params[advice_key][:advice])
end
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end

it 'sets a success flash notice' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with('1', { advice: 'Hello' }).and_return('Ok')
allow(QuestionAdvice).to receive(:update).with('2', { advice: 'Goodbye' }).and_return('Ok')
# Add some advice parameters that will allow for update success
params = { advice: {
'1' => { advice: 'Hello' },
'2' => { advice: 'Goodbye' }
}, id: 1 }
session = { user: instructor1 }

# Act
get(:save_advice, params:, session:)

# Assert
expect(flash[:notice]).to eq('The advice was successfully saved!')
end
end

context 'when the advice is not present' do
it 'does not update any advice' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with(any_args).and_return('Ok')
# no advice parameter
params = { id: 1 }
session = { user: instructor1 }

# Act
result = get(:save_advice, params:, session:)

# Assert
# Expect no update to be called with nil params for advice
# Expect no flash
# Always expect redirect
expect(QuestionAdvice).not_to have_received(:update)
expect(flash[:notice]).not_to be_present
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end
end

context 'when the questionnaire is not found' do
it 'renders the edit_advice view' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with(any_args).and_raise(ActiveRecord::RecordNotFound)

#Act
# call get on edit_advice with params that will hit the record not found path
get :edit_advice, params: { advice: {
'1' => { advice: 'Hello' },
'2' => { advice: 'Goodbye' }
}, id: 1 }

# Assert: Verify that the controller renders the correct view
expect(response).to render_template('edit_advice')
end
end

it 'redirects to the edit_advice action' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with(any_args).and_return('Ok')
params = { advice: {
'1' => { advice: 'Hello' },
'2' => { advice: 'Goodbye' }
}, id: 1 }
session = { user: instructor1 }

# Act
result = get(:save_advice, params:, session:)

# Assert
# expect 302 redirect and for it to redirect to edit_advice
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end
end
</pre>

==== '''edit_advice''' ====
Here are the current tests for edit_advice. The current tests cover most of the functionalities of the edit_advice method. Tests can be added to evaluate the impact of invalid_advice? method on the edit_advice method and checking if the number of advice is correctly calculated based on the questionnaire's min and max scores. We can also add tests to check the actions of the controller when the invalid_advice? method returns false.

<pre>
describe '#edit_advice' do

context "when edit_advice is called and invalid_advice? evaluates to true" do
# edit advice called
let(:questionAdvice1) {build(:question_advice, id:1, score: 1, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 2, question_id: 1, advice: "Advice2")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2])], max_question_score: 2)
end

it "edit advice redirects correctly when called" do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
params = {id: 1}
session = {user: instructor1}
result = get :edit_advice, params: params, session: session
expect(result.status).to eq 200
expect(result).to render_template(:edit_advice)
end
end
end
</pre>

Here are the tests after refactoring with the test skeleton
<pre>
describe '#edit_advice' do

context 'when edit_advice is called and invalid_advice? evaluates to true' do
# edit advice called
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end

it 'edit advice redirects correctly when called' do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
params = { id: 1 }
session = { user: instructor1 }
result = get(:edit_advice, params:, session:)
expect(result.status).to eq 200
expect(result).to render_template(:edit_advice)
end
end

context 'when advice adjustment is not necessary' do
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end

it 'does not adjust advice size when called' do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(controller).to receive(:invalid_advice?).and_return(false)
expect(QuestionnaireHelper).not_to receive(:adjust_advice_size)
get :edit_advice, params: { id: 1 }
end
end
context "when the advice size needs adjustment" do
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end

before do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(controller).to receive(:invalid_advice?).and_return(true)
end

it "calculates the number of advices for each question" do
expect(controller).to receive(:calculate_num_advices).once # Assuming there are two questions in the questionnaire
get :edit_advice, params: { id: 1 }
end

it "sorts question advices in descending order by score" do
expect(controller).to receive(:sort_question_advices).once # Assuming there are two questions in the questionnaire
get :edit_advice, params: { id: 1 }
end

it "adjusts the advice size if the number of advices is less than the max score of the questionnaire" do
allow(controller).to receive(:calculate_num_advices).and_return(1) # Assuming only one advice calculated
expect(QuestionnaireHelper).to receive(:adjust_advice_size).with(questionnaire, questionnaire.questions.first)
get :edit_advice, params: { id: 1 }
end

it "adjusts the advice size if the number of advices is greater than the max score of the questionnaire" do
allow(controller).to receive(:calculate_num_advices).and_return(3) # Assuming three advices calculated
expect(QuestionnaireHelper).to receive(:adjust_advice_size).with(questionnaire, questionnaire.questions.first)
get :edit_advice, params: { id: 1 }
end

it "adjusts the advice size if the max score of the advices does not correspond to the max score of the questionnaire" do
allow(controller).to receive(:sort_question_advices).and_return([questionAdvice2, questionAdvice1]) # Assuming advices not sorted correctly
expect(QuestionnaireHelper).to receive(:adjust_advice_size).with(questionnaire, questionnaire.questions.first)
get :edit_advice, params: { id: 1 }
end

it "adjusts the advice size if the min score of the advices does not correspond to the min score of the questionnaire" do
allow(questionnaire).to receive(:min_question_score).and_return(0) # Assuming min score not matching
expect(QuestionnaireHelper).to receive(:adjust_advice_size).with(questionnaire, questionnaire.questions.first)
get :edit_advice, params: { id: 1 }
end
end

context "when the advice size does not need adjustment" do
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end

before do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(controller).to receive(:invalid_advice?).and_return(false)
end

it "does not adjust the advice size if the number of advices is equal to the max score of the questionnaire" do
allow(controller).to receive(:calculate_num_advices).and_return(2) # Assuming two advices calculated
expect(QuestionnaireHelper).not_to receive(:adjust_advice_size)
get :edit_advice, params: { id: 1 }
end

it "does not adjust the advice size if the max score of the advices corresponds to the max score of the questionnaire" do
expect(QuestionnaireHelper).not_to receive(:adjust_advice_size)
get :edit_advice, params: { id: 1 }
end

it "does not adjust the advice size if the min score of the advices corresponds to the min score of the questionnaire" do
expect(QuestionnaireHelper).not_to receive(:adjust_advice_size)
get :edit_advice, params: { id: 1 }
end
end
end
</pre>

====== Testing Note 1 ======
During our initial run of tests with no altered code we were getting a few failure items. The two major ones were related to a missing edit erb file and an issue with the tests finding the method flash.
Upon researching I noticed that the ApplicationController inherited from '''ActionController::API''' which does not contain the method for flash. The refactor fork contained quite a few api based controllers which this makes sense for. This does not make sense for non API related controllers like AdviceController. In my PR I have modified ApplicationController to inherit from '''ActionController::Base''' to allow for our testing to complete.

'''Recommendation''' - Future work that includes api based controllers should have an APIController they inherit from that is '''APIController < ActionController::API'''. The changes made based on this discovery are here in this [https://github.com/AdrYne01/reimplementation-back-end/commit/bebf4a15bbb9d8b5d1d0c6f1ac77c664c261be1b [Commit]]

== Authors ==
This project was completed by Aiden Bartlett, Brandon Devine, and Aditya Karthikeyan.

== Pull Request ==
https://github.com/expertiza/reimplementation-back-end/pull/99

== Problem Document ==
https://docs.google.com/document/d/1nmJ9ce_Or6hRucsVpq4m_IFN4aKH6N1NepEpMGfGqbM/edit#heading=h.531sshwnqkr

== Video (methods and testing) ==
https://www.youtube.com/watch?v=AhPnEzIC5s4

CSC/ECE 517 Spring 2024 - E2447. Reimplement Advice Controller

2024-04-21T21:21:55Z

Bdevine2: /* Video */

== '''About Expertiza''' ==
[https://expertiza.ncsu.edu/ Expertiza] is a software which benefits both instructors and students by providing platform for various types of submissions and providing reusable objects for peer review. Expertiza is an open-source project developed on [https://rubyonrails.org/ Ruby on Rails] framework. In Expertiza an instructors can not only create and customize new or existing assignments, but he/she can also create a list of topics and subject in which the students can sign up. Along with that, Students can form their teams and groups to work with on various projects and assignments. Students can also peer-review other students' submissions. This enables students to work together to improve each other’s learning experiences by providing feedbacks.

== '''Project Description''' ==
This project involves implementing the AdviceController into the reimplementation repository. The controller has limited functionality where it performs validation checks on advice. We will also be making a few refactoring changes and ensuring full test coverage of the methods on the controller.

Advice_controller first checks whether current user has TA privileges or not by implementing action_allowed? method. Secondly it sets the number of advices based on score and sort it in descending order. Then it checks four conditions for the advices.

Methods
* '''action_allowed?'''
* '''invalid_advice?'''
* '''edit_advice'''
* '''save_advice'''

== '''Methods''' ==

==== '''invalid_advice?''' ====
Here is the current implementation of invalid_advice?
<pre>
# checks whether the advices for a question in questionnaire have valid attributes
# return true if the number of advices and their scores are invalid, else returns false
def invalid_advice?(sorted_advice, num_advices, question)
return ((question.question_advices.length != num_advices) ||
sorted_advice.empty? ||
(sorted_advice[0].score != @questionnaire.max_question_score) ||
(sorted_advice[sorted_advice.length - 1].score != @questionnaire.min_question_score))
end
</pre>

'''Plans to Refactor'''
# First, the comments are a bit hard to read, but those can easily be switched.
# Rather than checking if advice is not invalid, (question.question_advices.length != num_advices) we want to implement this method to be valid_advice where it returns if the length is valid.
# We want to split the checks up into different methods as per the Single Responsibliity Principle.
# We do not believe there needs to be any logic changes and all the checks are valuable, and we could not come up with any other

==== '''save_advice''' ====
Here is the current implementation of save_advice
<pre>
# save the advice for a questionnaire
def save_advice
# Stores the questionnaire with given id in URL
@questionnaire = Questionnaire.find(params[:id])
begin
# checks if advice is present or not
unless params[:advice].nil?
params[:advice].keys.each do |advice_key|
# Updates the advice corresponding to the key
QuestionAdvice.update(advice_key, advice: params[:advice][advice_key.to_sym][:advice])
end
flash[:notice] = 'The advice was successfully saved!'
end
rescue ActiveRecord::RecordNotFound
# If record not found, redirects to edit_advice
render action: 'edit_advice', id: params[:id]
end
redirect_to action: 'edit_advice', id: params[:id]
end
</pre>

'''Plans to refactor'''
# Simplify the flow since all paths lead to redirect_to 'edit_advice' we can remove the rescue call and replace that with a failed flash message
# Refactor the .each to iterate over params and more clearly handle the update of advice

'''Refactored code''' - Refactored code [https://github.com/AdrYne01/reimplementation-back-end/commit/8b5db0cd60a23c3b032eda025bec863aa059efaa [Commit]]
<pre>
def save_advice
begin
# checks if advice is present or not
unless params[:advice].nil?
params[:advice].each do |advice_key, advice_params|
# get existing advice to update by key with the passed in advice param
QuestionAdvice.update(advice_key, advice: advice_params.slice(:advice)[:advice])
end
# we made it here so it was saved
flash[:notice] = 'The advice was successfully saved!'
end
rescue ActiveRecord::RecordNotFound
# If record not found, redirects to edit_advice and sends flash
flash[:notice] = 'The advice record was not found and saved!'
end
# regardless of action above redirect to edit and show flash message if one exists
redirect_to action: 'edit_advice', id: params[:id]
end
</pre>

==== '''edit_advice''' ====
Here is the current implementation of edit_advice
<pre>
def edit_advice
# Stores the questionnaire with given id in URL
@questionnaire = Questionnaire.find(params[:id])

# For each question in a quentionnaire, this method adjusts the advice size if the advice size is <,> number of advices or
# the max or min score of the advices does not correspond to the max or min score of questionnaire respectively.
@questionnaire.questions.each do |question|
# if the question is a scored question, store the number of advices corresponding to that question (max_score - min_score), else 0
num_advices = if question.is_a?(ScoredQuestion)
@questionnaire.max_question_score - @questionnaire.min_question_score + 1
else
0
end

# sorting question advices in descending order by score
sorted_advice = question.question_advices.sort_by { |x| x.score }.reverse

# Checks the condition for adjusting the advice size
if invalid_advice?(sorted_advice, num_advices, question)
# The number of advices for this question has changed.
QuestionnaireHelper.adjust_advice_size(@questionnaire, question)
end
end
end
</pre>

'''Plans to refactor'''
# We can have separate functions to do all the different operations in the edit_advice method.
# Have separate functions for calculating num_advices and sorting the questions by score
# This is done to make sure the edit_advice method follows the Single Responsibility Principle.
# While sorting questions, we can use sort_by(&:score) instead of using a block. It is a shorthand notation and avoids creating a new Proc object for every element in the collection of the questions.
# Refactor the 'if' statement into a ternary operator while calculating the number of advice. This accomplishes the same logic more concisely.

'''Refactored code''' Refactored code [https://github.com/AdrYne01/reimplementation-back-end/commit/a1e26c0679f71df65b18961ba7e26da1deb04fe1 [Commit]]
<pre>
# Modify the advice associated with a questionnaire
# Separate methods were introduced to calculate the number of advices and sort the advices related to the current question attribute
# This is done to adhere to Single Responsibility Principle
def edit_advice
# For each question in a questionnaire, this method adjusts the advice size if the advice size is <,> number of advices or
# the max or min score of the advices does not correspond to the max or min score of questionnaire respectively.
@questionnaire.questions.each do |question|
# if the question is scored, store the number of advices corresponding to that question (max_score - min_score), else 0
# # Call to a separate method to adhere to Single Responsibility Principle
num_advices = calculate_num_advices(question)

# sorting question advices in descending order by score
# Call to a separate method to adhere to Single Responsibility Principle
sorted_advice = sort_question_advices(question)

# Checks the condition for adjusting the advice size
if invalid_advice?(sorted_advice, num_advices, question)
# The number of advices for this question has changed.
QuestionnaireHelper.adjust_advice_size(@questionnaire, question)
end
end
end

# Function to calculate number of advices for the current question attribute based on max and min question score.
# Method name is consistent with the functionality
# Refactored the 'if' statement into a ternary operator. This accomplishes the same logic more concisely.
def calculate_num_advices(question)
question.is_a?(ScoredQuestion) ? @questionnaire.max_question_score - @questionnaire.min_question_score + 1 : 0
end

# Function to sort question advices related to the current question attribute
# While sorting questions, sort_by(&:score) is used instead of using a block. It is a shorthand notation and avoids creating a new Proc object for every element in the collection of the questions.
def sort_question_advices(question)
question.question_advices.sort_by(&:score).reverse
end
</pre>

====== Note 1 ======
After completing the initial refactoring there was an issue with one item that was clearly not '''DRY'''. The line of code @questionnaire = Questionnaire.find(params[:id]) which was in both the save and edit methods.

A private method set_questionnaire was created and assigned in a before_action at the top of the controller. The following is the refactored pieces and the code [https://github.com/AdrYne01/reimplementation-back-end/commit/e882f7b49c9edad41705d5c6b7582936069fa279 [Commit]]
<pre>
before_action :set_questionnaire, only: %i[ edit_advice save_advice ]

def set_questionnaire
# Stores the questionnaire with given id in URL
@questionnaire = Questionnaire.find(params[:id])
end
</pre>

== '''Testing (rspec)''' ==

==== '''invalid_advice''' ====
Here are the current tests for invalid_advice? method. As mentioned before, if this method is changed to valid_advice?, the tests will be changed and flipped accordingly. There are some edge cases that we will add like what question advice score = max score of questionnaire or advice score = min score of questionnaire.

<pre>
context "when invalid_advice? is called with question advice score > max score of questionnaire" do
# max score of advice = 3 (!=2)
let(:questionAdvice1) {build(:question_advice, id:1, score: 1, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 3, question_id: 1, advice: "Advice2")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2])], max_question_score: 2)
end

it "invalid_advice? returns true when called with incorrect maximum score for a question advice" do
sorted_advice = questionnaire.questions[0].question_advices.sort_by { |x| x.score }.reverse
num_advices = questionnaire.max_question_score - questionnaire.min_question_score + 1
controller.instance_variable_set(:@questionnaire,questionnaire)
expect(controller.invalid_advice?(sorted_advice,num_advices,questionnaire.questions[0])).to eq(true)
end
end

context "when invalid_advice? is called with question advice score < min score of questionnaire" do
# min score of advice = 0 (!=1)
let(:questionAdvice1) {build(:question_advice, id:1, score: 0, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 2, question_id: 1, advice: "Advice2")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2])], max_question_score: 2)
end

it "invalid_advice? returns true when called with incorrect minimum score for a question advice" do
sorted_advice = questionnaire.questions[0].question_advices.sort_by { |x| x.score }.reverse
num_advices = questionnaire.max_question_score - questionnaire.min_question_score + 1
controller.instance_variable_set(:@questionnaire,questionnaire)
expect(controller.invalid_advice?(sorted_advice,num_advices,questionnaire.questions[0])).to eq(true)
end
end

context "when invalid_advice? is called with number of advices > (max-min) score of questionnaire" do
# number of advices > 2
let(:questionAdvice1) {build(:question_advice, id:1, score: 1, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 2, question_id: 1, advice: "Advice2")}
let(:questionAdvice3) {build(:question_advice, id:3, score: 2, question_id: 1, advice: "Advice3")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2,questionAdvice3])], max_question_score: 2)
end

it "invalid_advice? returns true when called with incorrect number of question advices" do
sorted_advice = questionnaire.questions[0].question_advices.sort_by { |x| x.score }.reverse
num_advices = questionnaire.max_question_score - questionnaire.min_question_score + 1
controller.instance_variable_set(:@questionnaire,questionnaire)
expect(controller.invalid_advice?(sorted_advice,num_advices,questionnaire.questions[0])).to eq(true)
end
end
</pre>

==== '''save_advice''' ====
Here are the current tests for save_advice. They do have complete coverage for the testing save_advice which we will test before and after the refactoring. We will be modifying the does not save piece to ensure the new flash message on failure shows.

<pre>
describe '#save_advice' do
context "when save_advice is called" do
# When ad advice is saved
let(:questionAdvice1) {build(:question_advice, id:1, score: 1, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 2, question_id: 1, advice: "Advice2")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2])], max_question_score: 2)
end

it "saves advice successfully" do
# When an advice is saved successfully
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with('1',{:advice => "Hello"}).and_return("Ok")
params = {advice: {"1" => {:advice => "Hello"}}, id: 1}
session = {user: instructor1}
result = get :save_advice, params: params, session: session
expect(flash[:notice]).to eq('The advice was successfully saved!')
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end

it "does not save the advice" do
# When an advice is not saved
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with(any_args).and_return("Ok")
params = {id: 1}
session = {user: instructor1}
result = get :save_advice, params: params, session: session
expect(flash[:notice]).not_to be_present
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end
end
end
</pre>

Here are the tests after refactoring with the test skeleton and this is the [https://github.com/AdrYne01/reimplementation-back-end/commit/3be59d5a6edd020e00bf5bba3c97fffae4538aa0 [Commit]] with the changes.

<pre>
describe '#save_advice' do
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end
context 'when the advice is present' do
it 'updates the advice for each key' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with('1', { advice: 'Hello' }).and_return('Ok')
allow(QuestionAdvice).to receive(:update).with('2', { advice: 'Goodbye' }).and_return('Ok')
# Add some advice parameters that will allow for update success
advice_params = {
'1' => { advice: 'Hello' },
'2' => { advice: 'Goodbye' }
}
params = { advice: advice_params, id: 1 }
session = { user: instructor1 }

# Act
result = get(:save_advice, params:, session:)

# Assert
# check each key to see if it received update
# Always expect redirect
advice_params.keys.each do |advice_key|
expect(QuestionAdvice).to have_received(:update).with(advice_key, advice: advice_params[advice_key][:advice])
end
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end

it 'sets a success flash notice' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with('1', { advice: 'Hello' }).and_return('Ok')
allow(QuestionAdvice).to receive(:update).with('2', { advice: 'Goodbye' }).and_return('Ok')
# Add some advice parameters that will allow for update success
params = { advice: {
'1' => { advice: 'Hello' },
'2' => { advice: 'Goodbye' }
}, id: 1 }
session = { user: instructor1 }

# Act
get(:save_advice, params:, session:)

# Assert
expect(flash[:notice]).to eq('The advice was successfully saved!')
end
end

context 'when the advice is not present' do
it 'does not update any advice' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with(any_args).and_return('Ok')
# no advice parameter
params = { id: 1 }
session = { user: instructor1 }

# Act
result = get(:save_advice, params:, session:)

# Assert
# Expect no update to be called with nil params for advice
# Expect no flash
# Always expect redirect
expect(QuestionAdvice).not_to have_received(:update)
expect(flash[:notice]).not_to be_present
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end
end

context 'when the questionnaire is not found' do
it 'renders the edit_advice view' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with(any_args).and_raise(ActiveRecord::RecordNotFound)

#Act
# call get on edit_advice with params that will hit the record not found path
get :edit_advice, params: { advice: {
'1' => { advice: 'Hello' },
'2' => { advice: 'Goodbye' }
}, id: 1 }

# Assert: Verify that the controller renders the correct view
expect(response).to render_template('edit_advice')
end
end

it 'redirects to the edit_advice action' do
# Arrange
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(QuestionAdvice).to receive(:update).with(any_args).and_return('Ok')
params = { advice: {
'1' => { advice: 'Hello' },
'2' => { advice: 'Goodbye' }
}, id: 1 }
session = { user: instructor1 }

# Act
result = get(:save_advice, params:, session:)

# Assert
# expect 302 redirect and for it to redirect to edit_advice
expect(result.status).to eq 302
expect(result).to redirect_to('/advice/edit_advice?id=1')
end
end
</pre>

==== '''edit_advice''' ====
Here are the current tests for edit_advice. The current tests cover most of the functionalities of the edit_advice method. Tests can be added to evaluate the impact of invalid_advice? method on the edit_advice method and checking if the number of advice is correctly calculated based on the questionnaire's min and max scores. We can also add tests to check the actions of the controller when the invalid_advice? method returns false.

<pre>
describe '#edit_advice' do

context "when edit_advice is called and invalid_advice? evaluates to true" do
# edit advice called
let(:questionAdvice1) {build(:question_advice, id:1, score: 1, question_id: 1, advice: "Advice1")}
let(:questionAdvice2) {build(:question_advice, id:2, score: 2, question_id: 1, advice: "Advice2")}
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1,questionAdvice2])], max_question_score: 2)
end

it "edit advice redirects correctly when called" do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
params = {id: 1}
session = {user: instructor1}
result = get :edit_advice, params: params, session: session
expect(result.status).to eq 200
expect(result).to render_template(:edit_advice)
end
end
end
</pre>

Here are the tests after refactoring with the test skeleton
<pre>
describe '#edit_advice' do

context 'when edit_advice is called and invalid_advice? evaluates to true' do
# edit advice called
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end

it 'edit advice redirects correctly when called' do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
params = { id: 1 }
session = { user: instructor1 }
result = get(:edit_advice, params:, session:)
expect(result.status).to eq 200
expect(result).to render_template(:edit_advice)
end
end

context 'when advice adjustment is not necessary' do
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end

it 'does not adjust advice size when called' do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(controller).to receive(:invalid_advice?).and_return(false)
expect(QuestionnaireHelper).not_to receive(:adjust_advice_size)
get :edit_advice, params: { id: 1 }
end
end
context "when the advice size needs adjustment" do
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end

before do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(controller).to receive(:invalid_advice?).and_return(true)
end

it "calculates the number of advices for each question" do
expect(controller).to receive(:calculate_num_advices).once # Assuming there are two questions in the questionnaire
get :edit_advice, params: { id: 1 }
end

it "sorts question advices in descending order by score" do
expect(controller).to receive(:sort_question_advices).once # Assuming there are two questions in the questionnaire
get :edit_advice, params: { id: 1 }
end

it "adjusts the advice size if the number of advices is less than the max score of the questionnaire" do
allow(controller).to receive(:calculate_num_advices).and_return(1) # Assuming only one advice calculated
expect(QuestionnaireHelper).to receive(:adjust_advice_size).with(questionnaire, questionnaire.questions.first)
get :edit_advice, params: { id: 1 }
end

it "adjusts the advice size if the number of advices is greater than the max score of the questionnaire" do
allow(controller).to receive(:calculate_num_advices).and_return(3) # Assuming three advices calculated
expect(QuestionnaireHelper).to receive(:adjust_advice_size).with(questionnaire, questionnaire.questions.first)
get :edit_advice, params: { id: 1 }
end

it "adjusts the advice size if the max score of the advices does not correspond to the max score of the questionnaire" do
allow(controller).to receive(:sort_question_advices).and_return([questionAdvice2, questionAdvice1]) # Assuming advices not sorted correctly
expect(QuestionnaireHelper).to receive(:adjust_advice_size).with(questionnaire, questionnaire.questions.first)
get :edit_advice, params: { id: 1 }
end

it "adjusts the advice size if the min score of the advices does not correspond to the min score of the questionnaire" do
allow(questionnaire).to receive(:min_question_score).and_return(0) # Assuming min score not matching
expect(QuestionnaireHelper).to receive(:adjust_advice_size).with(questionnaire, questionnaire.questions.first)
get :edit_advice, params: { id: 1 }
end
end

context "when the advice size does not need adjustment" do
let(:questionAdvice1) { build(:question_advice, id: 1, score: 1, question_id: 1, advice: 'Advice1') }
let(:questionAdvice2) { build(:question_advice, id: 2, score: 2, question_id: 1, advice: 'Advice2') }
let(:questionnaire) do
build(:questionnaire, id: 1, min_question_score: 1,
questions: [build(:question, id: 1, weight: 2, question_advices: [questionAdvice1, questionAdvice2])], max_question_score: 2)
end

before do
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(controller).to receive(:invalid_advice?).and_return(false)
end

it "does not adjust the advice size if the number of advices is equal to the max score of the questionnaire" do
allow(controller).to receive(:calculate_num_advices).and_return(2) # Assuming two advices calculated
expect(QuestionnaireHelper).not_to receive(:adjust_advice_size)
get :edit_advice, params: { id: 1 }
end

it "does not adjust the advice size if the max score of the advices corresponds to the max score of the questionnaire" do
expect(QuestionnaireHelper).not_to receive(:adjust_advice_size)
get :edit_advice, params: { id: 1 }
end

it "does not adjust the advice size if the min score of the advices corresponds to the min score of the questionnaire" do
expect(QuestionnaireHelper).not_to receive(:adjust_advice_size)
get :edit_advice, params: { id: 1 }
end
end
end
</pre>

====== Testing Note 1 ======
During our initial run of tests with no altered code we were getting a few failure items. The two major ones were related to a missing edit erb file and an issue with the tests finding the method flash.
Upon researching I noticed that the ApplicationController inherited from '''ActionController::API''' which does not contain the method for flash. The refactor fork contained quite a few api based controllers which this makes sense for. This does not make sense for non API related controllers like AdviceController. In my PR I have modified ApplicationController to inherit from '''ActionController::Base''' to allow for our testing to complete.

'''Recommendation''' - Future work that includes api based controllers should have an APIController they inherit from that is '''APIController < ActionController::API'''. The changes made based on this discovery are here in this [https://github.com/AdrYne01/reimplementation-back-end/commit/bebf4a15bbb9d8b5d1d0c6f1ac77c664c261be1b [Commit]]

== Authors ==
This project was completed by Aiden Bartlett, Brandon Devine, and Aditya Karthikeyan.

== Pull Request ==
https://github.com/expertiza/reimplementation-back-end/pull/99

== Problem Document ==
https://docs.google.com/document/d/1nmJ9ce_Or6hRucsVpq4m_IFN4aKH6N1NepEpMGfGqbM/edit#heading=h.531sshwnqkr

== Video ==
https://www.youtube.com/watch?v=AhPnEzIC5s4

CSC/ECE 517 Spring 2024 - E2447. Reimplement Advice Controller

CSC/ECE 517 Spring 2024 - E2422 Refactor Advice Controller

#REDIRECT [[CSC/ECE 517 Spring 2024 - E2422 Refactor Advice Controller]]

CSC/ECE 517 Spring 2024 - E2447. Reimplement Advice Controller

2024-04-02T21:38:14Z

Bdevine2: Bdevine2 moved page CSC/ECE 517 Spring 2024 - Refactor Advice Controller to CSC/ECE 517 Spring 2024 - E2422 Refactor Advice Controller: moving to rename

2024-03-23T14:38:53Z

Bdevine2: /* delete */

== 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 [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2023_-_E2319._Reimplement_questionnaire.rb 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.

[[File:Rspec-header.png]]

=== 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.

[[File:Validation 1.png]]

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.

[[File:Validation 2.png]]

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.

[[File:Validation 3.png]]

=== 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.

[[File:True-false-rspec.png]]

=== delete ===
----

[[File:Delete-rspec-1.png]]
[[File:Delete-rspec-2.png]]
[[File:Delete-rspec-3.png]]

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

=== 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

File:Delete-rspec-3.png

2024-03-23T14:37:54Z

Bdevine2:

File:Delete-rspec-2.png

2024-03-23T14:37:29Z

Bdevine2:

File:Delete-rspec-1.png

2024-03-23T14:37:06Z

Bdevine2: first section of delete tests

== Summary ==
first section of delete tests

E2422. Reimplement questionnaire.rb

2024-03-23T14:33:58Z

Bdevine2: /* Questionnaire model */

== 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 [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2023_-_E2319._Reimplement_questionnaire.rb 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.

[[File:Rspec-header.png]]

=== 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.

[[File:Validation 1.png]]

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.

[[File:Validation 2.png]]

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.

[[File:Validation 3.png]]

=== 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.

[[File:True-false-rspec.png]]

=== 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

=== 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

E2422. Reimplement questionnaire.rb

2024-03-23T14:33:41Z

Bdevine2: /* Questionnaire model */

== 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 [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2023_-_E2319._Reimplement_questionnaire.rb 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

[[File:Rspec-header.png]]

=== 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.

[[File:Validation 1.png]]

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.

[[File:Validation 2.png]]

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.

[[File:Validation 3.png]]

=== 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.

[[File:True-false-rspec.png]]

=== 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

=== 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

E2422. Reimplement questionnaire.rb

2024-03-23T14:32:28Z

Bdevine2: /* Rspec Testing */

== 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 [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2023_-_E2319._Reimplement_questionnaire.rb 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 ===

[[File:Rspec-header.png]]

=== 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.

[[File:Validation 1.png]]

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.

[[File:Validation 2.png]]

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.

[[File:Validation 3.png]]

=== 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.

[[File:True-false-rspec.png]]

=== 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

=== 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

File:Rspec-header.png

2024-03-23T14:29:38Z

Bdevine2: the initial let's at the top test class level

== Summary ==
the initial let's at the top test class level

E2422. Reimplement questionnaire.rb

2024-03-23T14:26:22Z

Bdevine2: /* true_false_questions? */

== 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 [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2023_-_E2319._Reimplement_questionnaire.rb 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

=== 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.

[[File:Validation 1.png]]

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.

[[File:Validation 2.png]]

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.

[[File:Validation 3.png]]

=== 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.

[[File:True-false-rspec.png]]

=== 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

=== 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

File:True-false-rspec.png

2024-03-23T14:19:18Z

Bdevine2: screenshot of tests for true false method

== Summary ==
screenshot of tests for true false method

E2422. Reimplement questionnaire.rb

2024-03-23T14:16:02Z

Bdevine2: /* Validate(s) */

== 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 [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2023_-_E2319._Reimplement_questionnaire.rb 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

=== 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.

[[File:Validation 1.png]]

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.

[[File:Validation 2.png]]

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.

[[File:Validation 3.png]]

=== 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

=== 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