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

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)

save_advice

Here are the current tests for save_advice. They do have complete coverage for the testing save_advice which I will test before and after the refactoring. I 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