Spring2019 E1907 refactor response controller - Revision history

Mblewall: /* Code Climate and Travis CI */

2019-04-02T03:39:07Z

Code Climate and Travis CI

← Older revision		Revision as of 03:39, 2 April 2019
Line 454:		Line 454:
	There were no observed failures after performing the modified test.		There were no observed failures after performing the modified test.

	='''Code Climate and Travis CI'''=		='''Code Climate and Travis CI Results'''=
	~~There are only cyclomatic~~, ~~cognitive~~ errors with code ~~climate whereas there are~~ no ~~integration errors~~ with ~~Travis CI~~		After completing the changes to the code, the code modifications were committed to GitHub. Then, a pull request was performed on the committed code. During this process, two automated bots were used to analyze the code. These bots were from Code Climate and from Travis CI.

			The Code Climate bot did not detect any major errors with the code. However, it did comment that the code complexity was too high for some methods. Since our team did not make any changes to these sections of code, no additional changes were made to the code to resolve the complexity concerns. The logic for the code appears to be working and it was uncertain how the logic could be further reduced to lower the code complexity score.

			The second bot, Travis Ci, performed four different analysis cycles on the committed code. It reported that the code passed its inspection with no issues.

	='''References'''=		='''References'''=

Mblewall: /* Test Plan */

2019-04-02T03:25:37Z

Test Plan

← Older revision		Revision as of 03:25, 2 April 2019
Line 428:		Line 428:
	='''Test Plan'''=		='''Test Plan'''=

			The test file used for the response_controller.rb was located in the */spec/controllers/response_controller_spec.rb. This test was previously created from a different project. However, the test file did have to be modified since the pending_surveys method was removed. The following snippets of code show the modifications to the test file.

	~~Because of moving the pending_surveys method, some of the original spec tests failed. So we fixed it.~~		Original content:
			<pre>
	context 'when params[:return] is survey' do		context 'when params[:return] is survey' do
	it 'redirects to survey_deployment#pending_surveys page' do		it 'redirects to survey_deployment#pending_surveys page' do
Line 438:		Line 439:
	end		end
	end		end
			</pre>

			Modified content:
			<pre>
	context 'when params[:return] is other content' do		context 'when params[:return] is other content' do
	it 'redirects to student_review#list page' do		it 'redirects to student_review#list page' do
Line 446:		Line 450:
	end		end
	end		end
			</pre>

			There were no observed failures after performing the modified test.

	='''Code Climate and Travis CI'''=		='''Code Climate and Travis CI'''=

Mblewall: /* Task 6 */

2019-04-02T03:15:45Z

Task 6

← Older revision		Revision as of 03:15, 2 April 2019
Line 396:		Line 396:
	'''Solution:'''		'''Solution:'''

	~~Introduced temporary explaining variables in~~ the redirect method. ~~Changed the~~ if-else ladder to a switch case.		A local variable was created to hold the description of the action to be taken by the redirect method. The if-else ladder was transformed into a switch (case) statement to make the code more legible.

			The modifications to the redirect method are shown below.

	<pre>		<pre>

Mblewall: /* Task 4 */

2019-04-02T03:03:28Z

Task 4

← Older revision		Revision as of 03:03, 2 April 2019
Line 272:		Line 272:
	'''Solution:'''		'''Solution:'''

	~~Removed~~ was_submitted. ~~Changed~~ to previously_submitted in the create method.		The first instance of was_submitted was removed. This variable was renamed to previously_submitted in the other lines of the create method. The purpose of this variable is to determine if the response was previously submitted or the submitted response is a new response. These changes are shown in the code below.

	<pre>		<pre>
Line 298:		Line 298:
	end		end
	previously_submitted = @response.is_submitted		previously_submitted = @response.is_submitted
	@response.update(additional_comment: params[:review][:comments], is_submitted: is_submitted) # ignore if autoupdate try to save when the response object is not yet created.		@response.update(additional_comment: params[:review][:comments], is_submitted: is_submitted)
			# ignore if autoupdate try to save when the response object is not yet created.

	# ,:version_num=>@version)		# ,:version_num=>@version)

Mblewall: /* Task 4 */

2019-04-02T03:00:03Z

Task 4

← Older revision		Revision as of 03:00, 2 April 2019
Line 252:		Line 252:
	end		end
	was_submitted = @response.is_submitted		was_submitted = @response.is_submitted
	@response.update(additional_comment: params[:review][:comments], is_submitted: is_submitted) # ignore if autoupdate try to save when the response object is not yet created.		@response.update(additional_comment: params[:review][:comments], is_submitted: is_submitted)
			# ignore if autoupdate try to save when the response object is not yet created.

	# ,:version_num=>@version)		# ,:version_num=>@version)

Mblewall: /* Task 4 */

2019-04-02T02:59:34Z

Task 4

← Older revision		Revision as of 02:59, 2 April 2019
Line 225:		Line 225:

	In the create() code in create method, especially the was_submitted and is_submitted part, is hard to understand. It can be cleaned up.		In the create() code in create method, especially the was_submitted and is_submitted part, is hard to understand. It can be cleaned up.

			The original code of the create method is shown below.
	<pre>		<pre>
	def create		def create

Mblewall: /* Task 3 */

2019-04-02T02:58:32Z

Task 3

← Older revision		Revision as of 02:58, 2 April 2019
Line 139:		Line 139:
	The assign_instance_variables method violates several principles - The method is doing two things, and the method is not really needed at all. It is called only in two places and does entirely different things based on where it is called from.		The assign_instance_variables method violates several principles - The method is doing two things, and the method is not really needed at all. It is called only in two places and does entirely different things based on where it is called from.

	The original ~~assign_instance_variables~~ method is shown below:		The original assign_instance_vars method is shown below:
	<pre>		<pre>
	# assigning the instance variables for Edit and New actions		# assigning the instance variables for Edit and New actions

Mblewall at 02:56, 2 April 2019

2019-04-02T02:56:49Z

← Older revision		Revision as of 02:56, 2 April 2019
Line 19:		Line 19:
	<pre>		<pre>
	def edit_allowed?(map, user_id)		def edit_allowed?(map, user_id)
	~~assignment = map~~.~~reviewer~~.~~assignment~~		...
	~~# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)~~
	~~if map~~.~~is_a? ReviewResponseMap~~
	~~reviewee_team = AssignmentTeam.find(map.reviewee_id)~~
	~~return current_user_id?(user_id) \|\| reviewee_team.user?(current_user) \|\| current_user.role.name == 'Administrator' \|\|~~
	~~(current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) \|\|~~
	~~(current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))~~
	~~else~~
	~~current_user_id?(user_id)~~
	~~end~~
	end		end
	</pre>		</pre>
Line 34:		Line 25:
	'''Solution:'''		'''Solution:'''

	~~Renamed~~ edit_allowed function to view_allowed in the response_controller.rb file		The edit_allowed? function was renamed to view_allowed? in the response_controller.rb file.
	<pre>		<pre>
	def view_allowed?(map, user_id)		def view_allowed?(map, user_id)
	~~assignment = map~~.~~reviewer~~.~~assignment~~		...
	~~# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)~~
	~~if map~~.~~is_a? ReviewResponseMap~~
	~~reviewee_team = AssignmentTeam.find(map.reviewee_id)~~
	~~return current_user_id?(user_id) \|\| reviewee_team.user?(current_user) \|\| current_user.role.name == 'Administrator' \|\|~~
	~~(current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) \|\|~~
	~~(current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))~~
	~~else~~
	~~current_user_id?(user_id)~~
	~~end~~
	end		end
	</pre>		</pre>
Line 55:		Line 37:
	Move the pending_surveys function to survey_deployment_contoller.rb. Ensure that moving the function does not break the code functionalities.		Move the pending_surveys function to survey_deployment_contoller.rb. Ensure that moving the function does not break the code functionalities.

			The original code for the pending_surveys method in the response_controller.rb is shown below.
	<pre>		<pre>
	def pending_surveys		def pending_surveys
Line 123:		Line 106:
	end		end
	</pre>		</pre>

	* spec/controllers/respose_controller_spec.rb		* spec/controllers/respose_controller_spec.rb

	Original -		Original -
	<pre>		<pre>
Line 134:		Line 119:
	end		end
	</pre>		</pre>

	New -		New -

	<pre>		<pre>
	context 'when params[:return] is survey' do		context 'when params[:return] is survey' do
Line 144:		Line 131:
	end		end
	</pre>		</pre>

	* Moved pending_surveys.html.erb from views/response to views/survey_deployment		* Moved pending_surveys.html.erb from views/response to views/survey_deployment

Line 151:		Line 139:
	The assign_instance_variables method violates several principles - The method is doing two things, and the method is not really needed at all. It is called only in two places and does entirely different things based on where it is called from.		The assign_instance_variables method violates several principles - The method is doing two things, and the method is not really needed at all. It is called only in two places and does entirely different things based on where it is called from.

			The original assign_instance_variables method is shown below:
	~~'''Solution:'''~~

	~~Removed assign_instance_vars section as it was not needed and combined in Edit and New sections~~

	~~Original assign_instance_vars~~ method :-
	<pre>		<pre>
			# assigning the instance variables for Edit and New actions
	def assign_instance_vars		def assign_instance_vars
	case params[:action]		case params[:action]
	when 'edit'		when 'edit'
Line 176:		Line 159:
	@return = params[:return]		@return = params[:return]
	end		end

	</pre>		</pre>


	~~Moved the assign_instance_vars for the Edit action to the edit method.~~

			'''Solution:'''

			The assign_instance_vars method was removed as it was not needed. The logic from this method was combined with the Edit and New sections as shown below.

			The edit method with modifications.
	<pre>		<pre>
	def edit		def edit
Line 460:		Line 446:
	='''References'''=		='''References'''=

	1. The pull request for this assignment can be viewed at: https://github.com/expertiza/expertiza/pull/1414		1. The problem statement for E 1907 can be viewed here https://docs.google.com/document/d/1SeIPiccoujhJZgdb3WzWBkg2v8TKSn5SwcHBeX9kzJQ/edit?usp=sharing

			2. The pull request for this assignment can be viewed at: https://github.com/expertiza/expertiza/pull/1414

	2. Team Branches:		3. Team Branches:
	All the three team members have contributed towards the project and their branches can be viewed here		All the three team members have contributed towards the project and their branches can be viewed here
	https://github.com/Anusha-Godavarthi/expertiza/branches		https://github.com/Anusha-Godavarthi/expertiza/branches

	3. Screencast: https://docs.google.com/presentation/d/1AAc95U7vq-zJ8mgD4lVvoQzn5Dui75cMKCoAyjcRm8c/edit?usp=sharing		4. Screencast: https://docs.google.com/presentation/d/1AAc95U7vq-zJ8mgD4lVvoQzn5Dui75cMKCoAyjcRm8c/edit?usp=sharing

	4. Expertiza Main Repo		5. Expertiza Main Repo
	https://github.com/expertiza/expertiza		https://github.com/expertiza/expertiza

	5. Expertiza Documentation http://wiki.expertiza.ncsu.edu/index.php/Main_Page		6. Expertiza Documentation http://wiki.expertiza.ncsu.edu/index.php/Main_Page

	='''Team Members'''=		='''Team Members'''=

Agodava: /* Test Plan */

2019-04-02T01:26:52Z

Test Plan

← Older revision		Revision as of 01:26, 2 April 2019
Line 436:		Line 436:
	='''Test Plan'''=		='''Test Plan'''=

	~~(Please re-word and expand this thought.)~~

	Because of moving the pending_surveys method, some of the original spec tests failed. So we fixed it.		Because of moving the pending_surveys method, some of the original spec tests failed. So we fixed it.

Agodava: /* References */

2019-04-02T01:24:34Z

References

← Older revision		Revision as of 01:24, 2 April 2019
Line 467:		Line 467:
	https://github.com/Anusha-Godavarthi/expertiza/branches		https://github.com/Anusha-Godavarthi/expertiza/branches

	3. Expertiza Main Repo		3. Screencast: https://docs.google.com/presentation/d/1AAc95U7vq-zJ8mgD4lVvoQzn5Dui75cMKCoAyjcRm8c/edit?usp=sharing

			4. Expertiza Main Repo
	https://github.com/expertiza/expertiza		https://github.com/expertiza/expertiza

	4. Expertiza Documentation http://wiki.expertiza.ncsu.edu/index.php/Main_Page		5. Expertiza Documentation http://wiki.expertiza.ncsu.edu/index.php/Main_Page

	='''Team Members'''=		='''Team Members'''=