CSC/ECE 517 Fall 2017/E1749 TFD questionnaire controller.rb refactor

From Expertiza_Wiki
Jump to navigation Jump to search

About

Expertiza is a website that is used and developed by NCSU students and faculty. Coded using Ruby on Rails the source code is readily available on Github -> https://github.com/expertiza/expertiza. The website is used by students for organizing teams, signing up for topics, reviewing other teams and another bunch of tasks. The faculty i.e. Instructors and TAs use this website to set new tasks, add new questions and a few other tasks.

Team Members

  • Shreyas Zagade
  • Mohit Satarkar
  • Darshan Bhandari

Setting Up The Working Environment

One of several ways to set up the environment and the one we adopted is:-
Ubuntu-Expertiza image (.OVA) [Recommended]

This is the link for the image. (https://drive.google.com/a/ncsu.edu/file/d/0B2vDvVjH76uEUmNKVncxRUhUVVE/view?usp=sharing)
And you can install VirtualBox (free) and import this image into VirtualBox.

Some machine may require you to enable virtualization and then run the following commands.

cd expertiza 
bash ./setup.sh 
bundle install 
rake db:migrate
  • For logging in as an instructor:-
Username: instructor6
Password: password

Files involved

  1. questionnaire_controller.rb
  2. questionnaire_controller_spec.rb

Issues

  • questionnaires_controller.rb is a fairly complex file.
  • It contains a lot of methods that are long and hard to understand, these methods need to be broken down into simpler and more specific methods that are easier to read/understand.
  • Also, the few instances of code duplication that exist should be removed.

Tasks assigned

  1. Complete pending tests in questionnaires_controller_spec.rb, and write integration tests for newly-created methods. Please finish one set of pending tests first before refactoring corresponding methods.
  2. Refactor create method
    • Write failing tests first
    • Split into several simpler methods and assign reasonable names
    • Extract duplicated code into separate methods
  3. Refactor update_quiz, save_choices method
    • Write failing tests first.
    • Use polymorphism to replace the long switch statements
      • Move if conditions to corresponding subclasses (eg. MultipleChoiceCheckbox.rb, MultipleChoiceRadio.rb) with same method name
      • Replace the conditional with the relevant method calls
      • Remove duplicated code
  4. Use find_by instead of dynamic method
    • Write failing tests first
    • L68, L385, L386, L559, L560

Tasks Completed

  • Testing
  • Refactoring


Testing


1) Completed test cases for create

When test case was pending
   describe '#create' do
     it 'redirects to questionnaires#edit page after create a new questionnaire'
   end
After writing test case
  describe '#create' do
   it 'redirects to questionnaires#edit page after create a new questionnaire' do
     tree_folder1 = double('TreeFolder')
     allow(tree_folder1).to receive(:id).and_return(1)
     allow(tree_folder1).to receive(:node_object_id).and_return(1)
     tree_folder2 = double('TreeFolder')
     allow(tree_folder2).to receive(:id).and_return(1)
     allow(tree_folder2).to receive(:node_object_id).and_return(1)
     allow(TreeFolder).to receive(:where).with(["name like ?", "Review"]).and_return([tree_folder1, tree_folder2])
     folder_node2 = double('FolderNode')
     allow(folder_node2).to receive(:id).and_return(1)
     allow(FolderNode).to receive(:find_by_node_object_id).and_return(folder_node2)
     user = double("User")
     allow(user).to receive(:id).and_return(1)
     params = {
       questionnaire: {
         private: "true",
         type: "ReviewQuestionnaire",
         name: "Random Name",
         min_question_score: "0",
         max_question_score: "5"
       }
     }
     session = {user: user}
     get :create, params, session
     expect(flash[:success]).to eq("You have successfully created a questionnaire!")
     response.should redirect_to(edit_questionnaire_path(id: 1))
   end
 end

2) Completed test cases for update_quiz

When test case was pending
  describe '#update_quiz' do
   context 'when @questionnaire is nil' do
     it 'redirects to submitted_content#view page'
   end

   context 'when @questionnaire is not nil' do
     it 'updates all quiz questions and redirects to submitted_content#view page'
       # params = {
       #   id: 1,
       #   pid: 1,
       #   save: true,
       #   questionnaire: {
       #     name: 'test questionnaire',
       #     instructor_id: 6,
       #     private: 0,
       #     min_question_score: 0,
       #     max_question_score: 5,
       #     type: 'ReviewQuestionnaire',
       #     display_type: 'Review',
       #     instructor_loc: 
       #   },
       #   question: {
       #     '1' => {txt: 'Q1'},
       #     '2' => {txt: 'Q2'},
       #     '3' => {txt: 'Q3'}
       #   },
       #   quiz_question_choices: {
       #     '1' => {MultipleChoiceRadio:
       #             {:correctindex => 1, '1' => {txt: 'a11'}, '2' => {txt: 'a12'}, '3' => {txt: 'a13'}, '4' => {txt: 'a14'}}},
       #     '2' => {TrueFalse: {'1' => {iscorrect: 'True'}}},
       #     '3' => {MultipleChoiceCheckbox:
       #             {'1' => {iscorrect: '1', txt: 'a31'}, '2' => {iscorrect: '0', txt: 'a32'},
       #              '3' => {iscorrect: '1', txt: 'a33'}, '4' => {iscorrect: '0', txt: 'a34'}}}
       #   }
       # }
   end
 end
After writing test case
 describe '#update_quiz' do
   context 'when @questionnaire is nil' do
     it 'redirects to submitted_content#view page' do
       params = {
         id: 1,
         pid: 1
       }
       get :update_quiz, params
       response.should redirect_to(view_submitted_content_index_path(id: params[:pid]))
     end
   end

   context 'when @questionnaire is not nil' do
     it 'updates all quiz questions and redirects to submitted_content#view page' do
       params = {
         id: 3,
         pid: 1,
         save: true,
         questionnaire: {
           name: 'test questionnaire',
           instructor_id: 6,
           private: 0,
           min_question_score: 0,
           max_question_score: 5,
           type: 'ReviewQuestionnaire',
           display_type: 'Review',
           instructor_loc: 
         },
         question: {
           '1' => {txt: 'Q1'},
           '2' => {txt: 'Q2'},
           '3' => {txt: 'Q3'}
         },
         quiz_question_choices: {
           '1' => {MultipleChoiceRadio:
                   {:correctindex => 1, '1' => {txt: 'a11'}, '2' => {txt: 'a12'}, '3' => {txt: 'a13'}, '4' => {txt: 'a14'}}},
           '2' => {TrueFalse: {'1' => {iscorrect: 'True'}}},
           '3' => {MultipleChoiceCheckbox:
                   {'1' => {iscorrect: '1', txt: 'a31'}, '2' => {iscorrect: '0', txt: 'a32'},
                    '3' => {iscorrect: '1', txt: 'a33'}, '4' => {iscorrect: '0', txt: 'a34'}}}
         }
       }
       get :update_quiz, params
       response.should redirect_to(view_submitted_content_index_path(id: params[:pid]))
     end
   end
 end


Refactoring


  • Create method has been split into two methods create and save_questionnaire_create

Before Modification: 

 def create
   questionnaire_private = params[:questionnaire][:private] == "true" ? true : false
   display_type = params[:questionnaire][:type].split('Questionnaire')[0]
   if Questionnaire::QUESTIONNAIRE_TYPES.include? params[:questionnaire][:type]
     @questionnaire = Object.const_get(params[:questionnaire][:type]).new
   end
   begin
     @questionnaire.private = questionnaire_private
     @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]
     # 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.
     case display_type
     when 'AuthorFeedback'
       display_type = 'Author%Feedback'
     when 'CourseSurvey'
       display_type = 'Course%Survey'
     when 'TeammateReview'
       display_type = 'Teammate%Review'
     when 'GlobalSurvey'
       display_type = 'Global%Survey'
     when 'AssignmentSurvey'
       display_type = 'Assignment%Survey'
     end
     @questionnaire.display_type = display_type
     @questionnaire.instruction_loc = Questionnaire::DEFAULT_QUESTIONNAIRE_URL
     @questionnaire.save
     # 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!'
   rescue
     flash[:error] = $ERROR_INFO
   end
   redirect_to controller: 'questionnaires', action: 'edit', id: @questionnaire.id
 end

After Modification: 

 def create
   begin
     save_questionnaire_create
     # 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!'
   rescue
     flash[:error] = $ERROR_INFO
   end
   redirect_to controller: 'questionnaires', action: 'edit', id: @questionnaire.id
 end



 def save_questionnaire_create
   questionnaire_private = params[:questionnaire][:private] == "true" ? true : false
   display_type = params[:questionnaire][:type].split('Questionnaire')[0]
   if Questionnaire::QUESTIONNAIRE_TYPES.include? params[:questionnaire][:type]
     @questionnaire = Object.const_get(params[:questionnaire][:type]).new
   end
   @questionnaire.private = questionnaire_private
   @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]
   # 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.
   case display_type
   when 'AuthorFeedback'
     display_type = 'Author%Feedback'
   when 'CourseSurvey'
     display_type = 'Course%Survey'
   when 'TeammateReview'
     display_type = 'Teammate%Review'
   when 'GlobalSurvey'
     display_type = 'Global%Survey'
   when 'AssignmentSurvey'
     display_type = 'Assignment%Survey'
   end
   @questionnaire.display_type = display_type
   @questionnaire.instruction_loc = Questionnaire::DEFAULT_QUESTIONNAIRE_URL
   @questionnaire.save
 end

We have also refactored the update_quiz and save_choices method, by implementing the instructions assigned to us for changing their ideology. Both of these had very long switch statements, which could be much shorter and keep optimum functionality. But the use of Polymorphism to change the way the method worked turned out to be much better.

The use of find_by instead of dynamic use was helpful. First we needed to complete the test cases at multiple mentioned line numbers. After hat, we were able to properly employ the methods as and when they were required.

We have displayed examples wherever appropriate, and the code we have uploaded on github is more than sufficient to enlighten about the changes that have been carried out.

Test Plan

For testing methods, we don't have to check the preconditions, as we have been assigned with refactoring the methods and testing those refactored methods properly.

We check the file questionnaires_controller_spec.rb, and test the methods required to be refactored based on our modifications we have to carry out in the spec file for the testing methods.

Methods Contexts
view Properly checks the rendering of questionnaires
new Looks for system reaction to presence of whitespace
new Tests the system reaction to absence of whitespace
create Checks if newly created page can be edited for values.
update_quiz Checks if the content in the questionnaire is nil
update_quiz Also tests if the content in the questionnaire is not nil
copy, copy_questionnaire_details, assign_instructor_id Tested the redirection to the view page of the copied questionnaire
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2017/E1749_TFD_questionnaire_controller.rb_refactor&oldid=112634"