E1868 remove reports from review mapping controller: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
No edit summary
Line 11: Line 11:


== Separation of Concerns ==
== Separation of Concerns ==
As part of refactoring, we have introduced several new Helper classes and methods.
As part of this refactoring, we will separate the 2 controller functions of review_mapping_controller:
:- Manage various reviewers
:- Display and manage different reports


== generalize code to display reports ==
== generalize code to display reports ==
The reports loop through either participants or teams. This loop can be generalized so that the headers, footers, etc. are all the same for all the reports.


== modify the UI to list the types of reports ==
== modify the UI to list the types of reports ==
In the current implementation, the reports are accessed by clicking on the “view review report” button in the buttons tray of an assignment, which leads into the review report page. This page contains a drop-down menu near the top which has the list of reports to navigate to.


== write tests for the new controller ==
The drop-down menu can be replaced by a new ‘...’ icon in the buttons tray, which on hovering on it, would show the menu of various reports.
[edit] 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.


[edit] Strategy Pattern for Automatic Review Mapping
== write tests for the new controller ==
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.
Since the logic for report generation has been abstracted into a new controller, existing tests for report generation must be modified and new tests must be written for the new controller.


We created a simple ReviewStrategy class with the following subclasses:
= Implementation specifics =


- StudentReviewStrategy
= Test Plan =
- TeamReviewStrategy
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.
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:


ReviewStrategyDiagram.png
= Results =
 
[edit] 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
[edit] 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.
 
[edit] 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).
 
[edit] 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.


[edit] Tests Added
= Conclusion =
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
[edit] Suggestions for Additional Improvements
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.

Revision as of 03:13, 10 November 2018

Background

As part of the project E1837[1], the review_mapping_controller.rb file has been significantly modified to improve maintainability, code quality and code readability. To summarize, the review_mapping_controller was modified by separating the report generation functionality into a new helper named - report_formatter_helper. This helper contains the logic of rendering different reports as per the user's request thus simplifying the code inside the controller.

This refactoring has a good impact on the overall design but can be further modified to made the code more Object-Oriented. The aim of our project is to extrapolate the report_formatter_helper code into a reports_controller.

Goals

- separation of concerns
- generalize code to display reports
- modify the UI to list the types of reports
- write tests for the new controller

Separation of Concerns

As part of this refactoring, we will separate the 2 controller functions of review_mapping_controller:

- Manage various reviewers
- Display and manage different reports

generalize code to display reports

The reports loop through either participants or teams. This loop can be generalized so that the headers, footers, etc. are all the same for all the reports.

modify the UI to list the types of reports

In the current implementation, the reports are accessed by clicking on the “view review report” button in the buttons tray of an assignment, which leads into the review report page. This page contains a drop-down menu near the top which has the list of reports to navigate to.

The drop-down menu can be replaced by a new ‘...’ icon in the buttons tray, which on hovering on it, would show the menu of various reports.


write tests for the new controller

Since the logic for report generation has been abstracted into a new controller, existing tests for report generation must be modified and new tests must be written for the new controller.

Implementation specifics

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.

Results

Conclusion

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=E1868_remove_reports_from_review_mapping_controller&oldid=119251"