E1837 refactor review mapping controller

Background

At the heart of Expertiza is the ability to review the work of other teams and review your own teammates.The Review Mapping Controller is at the heart of coordinating all of the necessary steps to assign reviews in various ways to specific students or teams.

As such, this controller has become rather large and unmaintainable over time as new types of reviews and review assignment strategies are created. The purpose of this work is to improve the maintainability of this controller based on the goals listed below.

Goals

- separation of concerns

- remove unused methods

- properly group like methods

- remove SQL statements from the controller into models/helpers where applicable

Separation of Concerns

As part of refactoring, we have introduced several new Helper classes and methods.

Report Formatter Helper

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

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.

We created a simple ReviewStrategy class with the following subclasses:

- StudentReviewStrategy

- TeamReviewStrategy

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:

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

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

Several methods had raw SQL, or at the very least SQL-like statements. Where possible, these calls were pushed down into their respective models.

Start Self Review

This method had the following SQL

SELECT t.id as t_id FROM teams_users u, teams t WHERE u.team_id = t.id and t.parent_id = ? and user_id = ?

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

Tests Added

Test Name	Class	Description
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	/spec/review_mapping_controller_spec.rb	Validates that the instructor cannot request more reviews than the team size for an assignment

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.