CSC/ECE 517 Spring 2024 - E2447. Reimplement Advice Controller
About Expertiza
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 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?
# 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
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
# 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
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 [Commit]
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
edit_advice
Here is the current implementation of edit_advice
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
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 [Commit]
# 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
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 [Commit]
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
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.
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
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.
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
Here are the tests after refactoring with the test skeleton and this is the [Commit] with the changes.
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
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.
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
Here are the tests after refactoring with the test skeleton
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
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 [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