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

Mmsatar2 at 03:50, 8 November 2017

2017-11-08T03:50:06Z

← Older revision		Revision as of 03:50, 8 November 2017
Line 284:		Line 284:
	@questionnaire.save		@questionnaire.save
	end		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==		==Test Plan==

Mmsatar2: /* Test Plan */

2017-11-02T05:52:36Z

Test Plan

← Older revision		Revision as of 05:52, 2 November 2017
Line 299:		Line 299:
	\|-		\|-
	\| new		\| new
	\| Looks for system reaction to presence ~~and~~ absence of whitespace		\| Looks for system reaction to presence of whitespace
			\|-
			\| new
			\| Tests the system reaction to absence of whitespace
	\|-		\|-
	\| create		\| create
Line 305:		Line 308:
	\|-		\|-
	\| update_quiz		\| update_quiz
	\| Checks if the content in the questionnaire is nil or not		\| 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		\| copy, copy_questionnaire_details, assign_instructor_id
	\| Tested the redirection to the view page of the copied questionnaire		\| Tested the redirection to the view page of the copied questionnaire
	\|-		\|-

Mmsatar2: /* Test Plan */

2017-11-02T05:49:58Z

Test Plan

← Older revision		Revision as of 05:49, 2 November 2017
Line 299:		Line 299:
	\|-		\|-
	\| new		\| new
	\| Looks for presence and absence of whitespace~~, and how the system~~		\| Looks for system reaction to presence and absence of whitespace
	~~\| reacts to both contexts~~
	\|-		\|-
	\| create		\| create
Line 306:		Line 305:
	\|-		\|-
	\| update_quiz		\| update_quiz
	\| Checks ~~both the contexts~~ if the content in the questionnaire is nil		\| Checks if the content in the questionnaire is nil or not
	\| or not
	\|-		\|-
	\| copy, copy_questionnaire_details, assign_instructor_id		\| copy, copy_questionnaire_details, assign_instructor_id
	\| Tested the redirection to the view page of the copied questionnaire		\| Tested the redirection to the view page of the copied questionnaire
	\|-		\|-

Mmsatar2: /* Test Plan */

2017-11-02T05:48:10Z

Test Plan

← Older revision		Revision as of 05:48, 2 November 2017
Line 300:		Line 300:
	\| new		\| new
	\| Looks for presence and absence of whitespace, and how the system		\| Looks for presence and absence of whitespace, and how the system
	\|reacts to both contexts		\| reacts to both contexts
	\|-		\|-
	\| create		\| create
Line 307:		Line 307:
	\| update_quiz		\| update_quiz
	\| Checks both the contexts if the content in the questionnaire is nil		\| Checks both the contexts if the content in the questionnaire is nil
	\|or not		\| or not
	\|-		\|-
	\| copy, copy_questionnaire_details, assign_instructor_id		\| copy, copy_questionnaire_details, assign_instructor_id
	\| Tested the redirection to the view page of the copied questionnaire		\| Tested the redirection to the view page of the copied questionnaire
	\|-		\|-

Mmsatar2: /* Test Plan */

2017-11-02T05:47:02Z

Test Plan

← Older revision		Revision as of 05:47, 2 November 2017
Line 300:		Line 300:
	\| new		\| new
	\| Looks for presence and absence of whitespace, and how the system		\| Looks for presence and absence of whitespace, and how the system
	reacts to both contexts		\|reacts to both contexts
	\|-		\|-
	\| create		\| create
Line 307:		Line 307:
	\| update_quiz		\| update_quiz
	\| Checks both the contexts if the content in the questionnaire is nil		\| Checks both the contexts if the content in the questionnaire is nil
	or not		\|or not
	\|-		\|-
	\| copy, copy_questionnaire_details, assign_instructor_id		\| copy, copy_questionnaire_details, assign_instructor_id
	\| Tested the redirection to the view page of the copied questionnaire		\| Tested the redirection to the view page of the copied questionnaire
	\|-		\|-

Mmsatar2: /* Test Plan */

2017-11-02T05:46:16Z

Test Plan

← Older revision		Revision as of 05:46, 2 November 2017
Line 286:		Line 286:

	==Test Plan==		==Test Plan==
	~~The pre conditions which are required~~ to ~~test~~ the methods ~~are not applicable because the project includes the process of refactoring~~ and testing ~~the~~ refactored methods.		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.

	~~The response_spec~~.rb file ~~does not include any edge cases. The edge cases are out of scope because~~ the ~~project includes refactoring and writing test cases~~.		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.

	{\| class="wikitable"		{\| class="wikitable"
Line 295:		Line 295:
	! Contexts		! Contexts
	\|-		\|-
	\| ~~response_id~~		\| view
	\| ~~tests if~~ the ~~response id is returned~~		\| Properly checks the rendering of questionnaires
	\|-		\|-
	\| ~~display_as_html~~		\| new
	\| ~~tests if corresponding html page is rendered based on~~ the ~~id passed~~		\| Looks for presence and absence of whitespace, and how the system
			reacts to both contexts
	\|-		\|-
	\| ~~construct_instructor_html~~		\| create
	\| ~~not tested~~		\| Checks if newly created page can be edited for values.
	\|-		\|-
	\| ~~construct_student_html~~		\| update_quiz
	\| not ~~tested~~		\| Checks both the contexts if the content in the questionnaire is nil
			or not
	\|-		\|-
	\| ~~construct_review_response~~		\| copy, copy_questionnaire_details, assign_instructor_id
	\| ~~not tested~~		\| Tested the redirection to the view page of the copied questionnaire
	\|-		\|-
	~~\| add_table_rows~~
	\|
	\|-
	~~\| additional_comment~~
	\|
	\|-
	~~\| total_score~~
	~~\| tests if the method computes the total score of the review~~
	\|-
	~~\| delete~~
	\|
	\|-
	~~\| average_score~~
	~~\| tests if the method computes the average score when the other function computes the maximum score~~
	\|-
	~~\| maximum_score~~
	~~\| tests if the method returns the maximum score of the current review~~
	\|-
	~~\| email~~
	~~\| tests if there is a call to email method in corresponding response maps~~
	\|-
	~~\| questionnaire_by_answer~~
	~~\| tests if method returns the questionnaire of the question of current answer or review questionnaire of current assignment based on the value of the answer~~
	\|-
	~~\| concatenate_all_review_comments~~
	~~\| tests if the method returns concatenated review comments~~
	\|-
	~~\| get_volume_of_review_comments~~
	~~\| tests if the method returns volumes of review comments in each round~~
	\|-
	~~\| significant_difference?~~
	~~\| tests if the method returns true when the difference between average score on same artifact from others and current score is bigger than allowed percentage otherwise false~~
	\|-
	~~\| avg_scores_and_count_for_prev_reviews~~
	~~\| tests if the method returns the average score and count of previous reviews when current response is not in current response array~~
	\|-
	~~\| notify_instructor_on_difference~~
	\|
	\|}

Mmsatar2 at 04:36, 2 November 2017

2017-11-02T04:36:42Z

← Older revision		Revision as of 04:36, 2 November 2017
Line 284:		Line 284:
	@questionnaire.save		@questionnaire.save
	end		end

			==Test Plan==
			The pre conditions which are required to test the methods are not applicable because the project includes the process of refactoring and testing the refactored methods.

			The response_spec.rb file does not include any edge cases. The edge cases are out of scope because the project includes refactoring and writing test cases.

			{\| class="wikitable"
			\|-
			! Methods
			! Contexts
			\|-
			\| response_id
			\| tests if the response id is returned
			\|-
			\| display_as_html
			\| tests if corresponding html page is rendered based on the id passed
			\|-
			\| construct_instructor_html
			\| not tested
			\|-
			\| construct_student_html
			\| not tested
			\|-
			\| construct_review_response
			\| not tested
			\|-
			\| add_table_rows
			\|
			\|-
			\| additional_comment
			\|
			\|-
			\| total_score
			\| tests if the method computes the total score of the review
			\|-
			\| delete
			\|
			\|-
			\| average_score
			\| tests if the method computes the average score when the other function computes the maximum score
			\|-
			\| maximum_score
			\| tests if the method returns the maximum score of the current review
			\|-
			\| email
			\| tests if there is a call to email method in corresponding response maps
			\|-
			\| questionnaire_by_answer
			\| tests if method returns the questionnaire of the question of current answer or review questionnaire of current assignment based on the value of the answer
			\|-
			\| concatenate_all_review_comments
			\| tests if the method returns concatenated review comments
			\|-
			\| get_volume_of_review_comments
			\| tests if the method returns volumes of review comments in each round
			\|-
			\| significant_difference?
			\| tests if the method returns true when the difference between average score on same artifact from others and current score is bigger than allowed percentage otherwise false
			\|-
			\| avg_scores_and_count_for_prev_reviews
			\| tests if the method returns the average score and count of previous reviews when current response is not in current response array
			\|-
			\| notify_instructor_on_difference
			\|
			\|}

Dbhanda: Created page with "== 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://gi..."

2017-10-28T02:15:24Z

Created page with "== 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://gi..."

New page

== 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.''' 
<pre>cd expertiza </pre>
<pre>bash ./setup.sh </pre>
<pre>bundle install </pre>
<pre>rake db:migrate </pre>
*For logging in as an instructor:- 
:Username: instructor6 
:Password: password 

== Files involved==

# questionnaire_controller.rb
# 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 ==
# 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.
# Refactor create method
#* Write failing tests first
#* Split into several simpler methods and assign reasonable names
#* Extract duplicated code into separate methods
# 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
# 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