E1837 refactor review mapping controller - Revision history

Dwgoslen2 at 18:54, 9 November 2018

2018-11-09T18:54:52Z

← Older revision		Revision as of 18:54, 9 November 2018
Line 47:		Line 47:

	It was migrated to be a scope inside of the Team model (<code>find_team_for_assignment_and_user</code>).		It was migrated to be a scope inside of the Team model (<code>find_team_for_assignment_and_user</code>).
	~~== Tests Added ==~~

			== Test Plan ==

			The majority of our changes were refactors, and thusly required a few new tests, but we did need to adjust existing tests to expect proper mocked/seamed methods.

			=== Tests Added ===
	{\| class="wikitable"		{\| class="wikitable"
	\|-		\|-

Dwgoslen2 at 19:39, 2 November 2018

2018-11-02T19:39:29Z

← Older revision		Revision as of 19:39, 2 November 2018
Line 27:		Line 27:

	Each of these strategies determines the number of reviews per team, the number of reviews per student, and the total number of reviews needed. After implementing this pattern, we were able to simply use the methods provided on the ReviewStrategy class in the algorithm that assigns reviews with minimal refactoring.		Each of these strategies determines the number of reviews per team, the number of reviews per student, and the total number of reviews needed. After implementing this pattern, we were able to simply use the methods provided on the ReviewStrategy class in the algorithm that assigns reviews with minimal refactoring.


	Below is a simple UML diagram of these new classes:		Below is a simple UML diagram of these new classes:

	[[File:ReviewStrategyDiagram.png]]		[[File:ReviewStrategyDiagram.png]]


	== Removed Methods ==		== Removed Methods ==
	We were able to remove the following methods that were either obsolete or had functionality that could be refactored into other methods.		We were able to remove the following methods that were either obsolete or had functionality that could be refactored into other methods.
	:- <code>execute_peer_review_strategy</code>: This functionality simply performed a check that is now in <code>automatic_review_mapping</code>		:- <code>execute_peer_review_strategy</code>: This functionality simply performed a check that is now in <code>automatic_review_mapping</code>

	~~== Group Like Methods ==~~
	~~All methods that were not directly called from an incoming HTTP request we made and grouped into private methods to provide clarity~~

	== Removed SQL ==		== Removed SQL ==
Line 67:		Line 62:

	== Suggestions for Additional Improvements ==		== Suggestions for Additional Improvements ==
	There are several reports that look to have either been moved to a different location or are simply not used anymore. All code and views for these reports should be removed if confirmed they are not used		The report generation should be refactored to allow each report to specify how it should be rendered. One idea was setting each report to supply a collection of "report lines" that are rendered one at a time in a table. This would allow for each report to specify its data and its rendering functions separatly.
	~~:- Calibration Report~~

	~~Additionally, and as mentioned above, the~~ report generation should be refactored to allow each report to specify how it should be rendered. One idea was setting each report to supply a collection of "report lines" that are rendered one at a time in a table. This would allow for each report to specify its data and its rendering functions separatly.

Dwgoslen2 at 00:38, 29 October 2018

2018-10-29T00:38:15Z

← Older revision		Revision as of 00:38, 29 October 2018
Line 65:		Line 65:
	\|-		\|-
	\|}		\|}

			== Suggestions for Additional Improvements ==
			There are several reports that look to have either been moved to a different location or are simply not used anymore. All code and views for these reports should be removed if confirmed they are not used
			:- Calibration Report

			Additionally, and as mentioned above, the report generation should be refactored to allow each report to specify how it should be rendered. One idea was setting each report to supply a collection of "report lines" that are rendered one at a time in a table. This would allow for each report to specify its data and its rendering functions separatly.

Dwgoslen2 at 23:33, 28 October 2018

2018-10-28T23:33:35Z

← Older revision		Revision as of 23:33, 28 October 2018
Line 8:		Line 8:
	:- remove unused methods		:- remove unused methods
	:- properly group like methods		:- properly group like methods
	~~:- properly name like methods~~
	:- remove SQL statements from the controller into models/helpers where applicable		:- remove SQL statements from the controller into models/helpers where applicable

Line 37:		Line 36:
	== Removed Methods ==		== Removed Methods ==
	We were able to remove the following methods that were either obsolete or had functionality that could be refactored into other methods.		We were able to remove the following methods that were either obsolete or had functionality that could be refactored into other methods.
	:- <code>execute_peer_review_strategy</code>: This functionality simply performed a check that is now in automatic_review_mapping		:- <code>execute_peer_review_strategy</code>: This functionality simply performed a check that is now in <code>automatic_review_mapping</code>

			== Group Like Methods ==
			All methods that were not directly called from an incoming HTTP request we made and grouped into private methods to provide clarity

	== Removed SQL ==		== Removed SQL ==

Dwgoslen2 at 23:30, 28 October 2018

2018-10-28T23:30:02Z

← Older revision		Revision as of 23:30, 28 October 2018
Line 37:		Line 37:
	== Removed Methods ==		== Removed Methods ==
	We were able to remove the following methods that were either obsolete or had functionality that could be refactored into other methods.		We were able to remove the following methods that were either obsolete or had functionality that could be refactored into other methods.
	:- execute_peer_review_strategy: This functionality simply performed a check that is now in automatic_review_mapping		:- <code>execute_peer_review_strategy</code>: This functionality simply performed a check that is now in automatic_review_mapping

	== Removed SQL ==		== Removed SQL ==

Dwgoslen2 at 23:28, 28 October 2018

2018-10-28T23:28:57Z

← Older revision		Revision as of 23:28, 28 October 2018
Line 6:		Line 6:
	== Goals ==		== Goals ==
	:- separation of concerns		:- separation of concerns
	:- ~~removed~~ unused methods		:- remove unused methods
	:- properly group like methods		:- properly group like methods
	:- ~~remove unused controller~~ methods		:- properly name like methods
	:- remove SQL statements from the controller into models/helpers where applicable		:- remove SQL statements from the controller into models/helpers where applicable

			== Separation of Concerns ==
	== ~~New Helpers~~ ==
	As part of refactoring, we have introduced several new Helper classes and methods.		As part of refactoring, we have introduced several new Helper classes and methods.

Line 34:		Line 33:

	[[File:ReviewStrategyDiagram.png]]		[[File:ReviewStrategyDiagram.png]]


			== Removed Methods ==
			We were able to remove the following methods that were either obsolete or had functionality that could be refactored into other methods.
			:- execute_peer_review_strategy: This functionality simply performed a check that is now in automatic_review_mapping

	== Removed SQL ==		== Removed SQL ==
Line 46:		Line 50:

	It was migrated to be a scope inside of the Team model (<code>find_team_for_assignment_and_user</code>).		It was migrated to be a scope inside of the Team model (<code>find_team_for_assignment_and_user</code>).

	~~== Removed Methods ==~~
	~~We were able to remove the following methods that were either obsolete or had functionality that could be refactored into other methods.~~
	~~:- execute_peer_review_strategy: This functionality simply performed a check that is now in automatic_review_mapping~~

	== Tests Added ==		== Tests Added ==

Dwgoslen2 at 23:26, 28 October 2018

2018-10-28T23:26:09Z

@@ Line 52: / Line 52: @@
 == Tests Added ==
-A test was added to validate that the instructor cannot request more reviews than the team size for an assignment. This test was added in review_mapping_controller_spec.rb and is
-"when student review num is greater than or equal to team size throws an error stating that student review number cannot be greater than or equal to team size"
+{| class="wikitable"

Dwgoslen2 at 23:23, 28 October 2018

2018-10-28T23:23:02Z

← Older revision		Revision as of 23:23, 28 October 2018
Line 22:		Line 22:

	=== Strategy Pattern for Automatic Review Mapping ===		=== Strategy Pattern for Automatic Review Mapping ===
	The previous implementation for the Automatic Review mapping method (automatic_review_mapping) was lengthy and overrode specific arguments within the Automatic Review Mapping Strategy method (automatic_review_mapping_strategy) before actually assigning reviews. We decided to implement an actual Strategy pattern here.		The previous implementation for the Automatic Review mapping method (<code>automatic_review_mapping</code>) was lengthy and overrode specific arguments within the Automatic Review Mapping Strategy method (<code>automatic_review_mapping_strategy</code>) before actually assigning reviews. We decided to implement an actual Strategy pattern here.

	We created a simple ReviewStrategy class with the following subclasses:		We created a simple ReviewStrategy class with the following subclasses:
Line 45:		Line 45:
	</code>		</code>

	It was migrated to be a scope inside of the Team model.		It was migrated to be a scope inside of the Team model (<code>find_team_for_assignment_and_user</code>).

	== Removed Methods ==		== Removed Methods ==

Dwgoslen2: /* Report Formatter Helper */

2018-10-28T23:20:32Z

Report Formatter Helper

← Older revision		Revision as of 23:20, 28 October 2018
Line 18:		Line 18:
	This helper now contains isolated methods for each report type that needs to be built from the controller. This helper is implemented as a module, so any/all instance variables are added to the class including it. This allows us to keep the controller logic very small but still populate all needed values for the report views.		This helper now contains isolated methods for each report type that needs to be built from the controller. This helper is implemented as a module, so any/all instance variables are added to the class including it. This allows us to keep the controller logic very small but still populate all needed values for the report views.

			Each report type is represented by a separate method in the Helper and the Helper exposes a single public method to render the report. This allows us to hide implementation details to be changed later as needed from within the module and the controller can generally stay the same. The only caveat is that all of the correct instance variables will need to be defined and populated per report. We considered refactoring the reports section, but after discussion with our
			mentor, we decided it would be a large enough effort in of itself to change how reports are generated and rendered.

	=== Strategy Pattern for Automatic Review Mapping ===		=== Strategy Pattern for Automatic Review Mapping ===

Dwgoslen2 at 22:38, 28 October 2018

2018-10-28T22:38:47Z

← Older revision		Revision as of 22:38, 28 October 2018
Line 49:		Line 49:
	We were able to remove the following methods that were either obsolete or had functionality that could be refactored into other methods.		We were able to remove the following methods that were either obsolete or had functionality that could be refactored into other methods.
	:- execute_peer_review_strategy: This functionality simply performed a check that is now in automatic_review_mapping		:- execute_peer_review_strategy: This functionality simply performed a check that is now in automatic_review_mapping

			== Tests Added ==
			A test was added to validate that the instructor cannot request more reviews than the team size for an assignment. This test was added in review_mapping_controller_spec.rb and is
			"when student review num is greater than or equal to team size throws an error stating that student review number cannot be greater than or equal to team size"