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

1. First, the comments are a bit hard to read, but those can easily be switched.
2. 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.
3. We want to split the checks up into different methods as per the Single Responsibliity Principle.
4. 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

1. 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
2. Refactor the .each to iterate over params and more clearly handle the update of advice

Refactored code

 
..

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

1. We can have separate functions to do all the different operations in the edit_advice method.
2. Have separate functions for calculating num_advices and sorting the questions by score
3. This is done to make sure the edit_advice method follows the Single Responsibility Principle.
4. 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.

Refactored code

 
..

Testing (rspec)

invalid_advice

Here are the current tests for invalid_advice? method.

  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

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.

  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