CSC/ECE 517 Fall 2022 - E2257: Refactor questionnaires controller.rb

Introduction

In Expertiza, Questionnaire is the superclass for all kinds of questionnaires and rubrics—rubrics for evaluating submissions and teammate contributions, and taking quizzes and surveys. All of these are subclasses of Questionnaire. questionnaires_controller.rb is charged with creating (adding questions), displaying, and managing Questionnaires. Because it is used so widely, malfunctions could cause bugs in many parts of the system; hence it is especially important that the code be clear.

Issues Fixed

1. Removed create_questionnaire
2. Split create method into smaller self-documenting methods.
3. QuizQuestionnaire check
4. Refactor add_new_questions method
5. Remove redundant variables in QuestionnairesController

Files Changed

• app/controllers/questionnaires_controller.rb
• spec/controllers/questionnaires_controller_spec.rb
• app/models/question.rb

Test Plan

No new functionality was added, and all changes have been either refactoring or removal of unnecessary code. Thus no new test cases were added. Only making sure the existing test cases do not fail for now.

Changes

Fix #1

The create_questionnnaires method was present but its related functionality had been removed. Some kind of link to it is still present in one of the initial migrations which were populating a database to show values for a drop-down that is no longer in use. Thus ended up removing the following function from the controller.

  def create_questionnaire
    @questionnaire = Object.const_get(params[:questionnaire][:type]).new(questionnaire_params)
    # Create Quiz content has been moved to Quiz Questionnaire Controller
    if @questionnaire.type != 'QuizQuestionnaire' # checking if it is a quiz questionnaire
      @questionnaire.instructor_id = Ta.get_my_instructor(session[:user].id) if session[:user].role.name == 'Teaching Assistant'
      save

      redirect_to controller: 'tree_display', action: 'list'
    end
  end

The following test corresponding to this create_questionnaire function above was also removed from questionnaire_controller_spec.rb.

  describe '#create_questionnaire and #save' do
    context 'when quiz is valid' do
      before(:each) do
        # create_quiz_questionnaire
        allow_any_instance_of(QuestionnairesController).to receive(:validate_quiz).and_return('valid')
      end

      context 'when questionnaire type is not QuizQuestionnaire' do
        it 'redirects to submitted_content#edit page' do
          request_params = { aid: 1,
                             pid: 1,
                             questionnaire: { name: review_questionnaire.name,
                                              type: review_questionnaire.type } }
          # create_questionnaire
          allow(ReviewQuestionnaire).to receive(:new).with(any_args).and_return(review_questionnaire)
          user_session = { user: build(:teaching_assistant, id: 1) }
          allow(Ta).to receive(:get_my_instructor).with(1).and_return(6)
          # save
          allow(TreeFolder).to receive(:find_by).with(name: 'Review').and_return(double('TreeFolder', id: 1))
          allow(FolderNode).to receive(:find_by).with(node_object_id: 1).and_return(double('FolderNode'))
          allow_any_instance_of(QuestionnairesController).to receive(:undo_link).with(any_args).and_return('')
          post :create_questionnaire, params: request_params, session: user_session
          expect(flash[:note]).to be nil
          expect(response).to redirect_to('/tree_display/list')
          expect(controller.instance_variable_get(:@questionnaire).private).to eq false
          expect(controller.instance_variable_get(:@questionnaire).name).to eq review_questionnaire.name
          expect(controller.instance_variable_get(:@questionnaire).min_question_score).to eq 0
          expect(controller.instance_variable_get(:@questionnaire).max_question_score).to eq 5
          expect(controller.instance_variable_get(:@questionnaire).type).to eq review_questionnaire.type
          expect(controller.instance_variable_get(:@questionnaire).instructor_id).to eq 6
        end
      end
    end
  end

Fix #2

The create method itself is 49 lines long. This is much too long to be viewable at a glance. Thus the method was broken into adding_question_variables and create_node method so that they are clear enough to be self-documenting.

   def create
    if params[:questionnaire][:name].blank?
      flash[:error] = 'A rubric or survey must have a title.'
      redirect_to controller: 'questionnaires', action: 'new', model: params[:questionnaire][:type], private: params[:questionnaire][:private]
    else
      questionnaire_private = params[:questionnaire][:private] == 'true'
      display_type = params[:questionnaire][:type].split('Questionnaire')[0]
      begin
        @questionnaire = Object.const_get(params[:questionnaire][:type]).new if Questionnaire::QUESTIONNAIRE_TYPES.include? params[:questionnaire][:type]
      rescue StandardError
        flash[:error] = $ERROR_INFO
      end
      begin
        # Zhewei: Right now, the display_type in 'questionnaires' table and name in 'tree_folders' table are not consistent.
        # In the future, we need to write migration files to make them consistency.
        # E1903 : We are not sure of other type of cases, so have added a if statement. If there are only 5 cases, remove the if statement
        if %w[AuthorFeedback CourseSurvey TeammateReview GlobalSurvey AssignmentSurvey BookmarkRating].include?(display_type)
          display_type = display_type.split(/(?=[A-Z])/).join('%')
        end
        # setting the object variables
        adding_question_variables(questionnaire_private, display_type)
        # Create node
        create_node()
      rescue StandardError
        flash[:error] = $ERROR_INFO
      end
      redirect_to controller: 'questionnaires', action: 'edit', id: @questionnaire.id
    end
  end

---

  # Assigns corrresponding variables to questionnaire object.
  def adding_question_variables(prv,display)
    @questionnaire.private = prv
    @questionnaire.name = params[:questionnaire][:name]
    @questionnaire.instructor_id = session[:user].id
    @questionnaire.min_question_score = params[:questionnaire][:min_question_score]
    @questionnaire.max_question_score = params[:questionnaire][:max_question_score]
    @questionnaire.type = params[:questionnaire][:type]
    @questionnaire.display_type = display
    @questionnaire.instruction_loc = Questionnaire::DEFAULT_QUESTIONNAIRE_URL
    @questionnaire.save
  end
 

---

  # Creates tree node
  def create_node()
    tree_folder = TreeFolder.where(['name like ?', @questionnaire.display_type]).first
    parent = FolderNode.find_by(node_object_id: tree_folder.id)
    QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id, type: 'QuestionnaireNode')
    flash[:success] = 'You have successfully created a questionnaire!'
  end
 

Fix #3

There were three checks for whether a questionnaire is a QuizQuestionnaire. Testing the class of an object is always suspect, though, in a controller, which has to control a whole hierarchy of objects, it is not necessarily wrong. However, they probably aren’t necessary. 2 of them seem to have been removed already and the only remaining check was in create_questionnaire, and it also got removed while applying Fix #1.

Fix #4

According to the code climate, it was suggested that usage of each_key is preferable instead of keys.each . Rubocop suggests this due to performance. Using large sets of data is where this becomes noticeable. Chaining methods is going to be slower than using the built-in method (in this case) which accomplishes the task with a single special enumerator.

    params[:question].keys.each do |question_key|
 

    params[:question].each_key do |question_key|
 

Fix #5

According to the code climate, it was suggested that the use of parentheses around a method call should be avoided.

    display_type = (display_type.split(/(?=[A-Z])/)).join('%') 
 

    display_type = display_type.split(/(?=[A-Z])/).join('%') 
 

Fix #6

The add_new_questions method has string literal values like 'Strongly agree', 'Strongly disagree', '50, 3', '0|1|2|3|4|5' for max_label, min_label, size and alternatives. The literals are removed by introducing constants in question.rb.

      if question.is_a? ScoredQuestion
        question.weight = params[:question][:weight]
        question.max_label = 'Strongly agree'
        question.min_label = 'Strongly disagree'
      end

      question.size = '50, 3' if question.is_a? Criterion
      question.size = '50, 3' if question.is_a? Cake
      question.alternatives = '0|1|2|3|4|5' if question.is_a? Dropdown
      question.size = '60, 5' if question.is_a? TextArea
      question.size = '30' if question.is_a? TextField

 

      if question.is_a? ScoredQuestion
        question.weight = params[:question][:weight]
        question.max_label = Question::MAX_LABEL
        question.min_label = Question::MIN_LABEL
      end

      if Question::SIZES.key?(question.class.name)
        question.size = Question::SIZES[question.class.name]
      end
      if Question::ALTERNATIVES.key?(question.class.name)
        question.alternatives = Question::ALTERNATIVES[question.class.name]
      end
 

Fix #7

The add_new_questions and save_all_questions methods have reduntant variable defined called questionnaire_id. The variable declaration is redundant as we can access it using params[:id].

     questionnaire_id = params[:id] unless params[:id].nil?
     num_of_existed_questions = Questionnaire.find(questionnaire_id).questions.size
 

     num_of_existed_questions = Questionnaire.find(params[:id]).questions.size
 

UI Testing

Log into the expertiza hosted link provided and go to the questionnaires tab. The following can be done to test the code:

• Try creating any type of questionnaire and adding questions to it. The Create button on the questionnaire creation page and the "Add", "Save {questionnaire_type} questionnaire" buttons on the edit questionnaire page should work without any errors. The "Edit/View advice" on the edit questionnaire page does not work for some reason in the originally provided code itself (Yet figured out the reason).
• Try editing the created questionnaire.

Future scope

• Have a closer look at the following issue: Assignment Branch Condition size for save_all_questions is too high.
  

save_all_questions has params[:view_advice] as another if statement not sure why this is here or what it corresponds to, the application seems to have a ‘view advice’ button on the questionnaire edit page, but clicking it throws the following error: No route matches [GET] "/advice/edit_advice". Hence not sure how to proceed here.

• Bad if statement. Line 190: if question.is_a? ScoredQuestion.

Need to have a closer look at the class higher archy and understand how this can be refactored.

Project Mentor

Edward Gehringer (efg@ncsu.edu)

Team Member

Ashwin Shankar Umasankar (aumasan@ncsu.edu)

Kailash Singaravelu (ksingar2@ncsu.edu)

Pujitha Enamandala (penaman@ncsu.edu)

Reference

1. VCL link
2. Github Repo
3. Github Pull Request
4. Expertiza on GitHub
5. Expertiza Project Documentation Wiki
6. CodeClimate Documentation