CSC/ECE 517 Fall 2020/oss E2072 RLM
This wiki page is for the description of changes made under E2072, Refactor assignment_participant.rb,assignment for Fall 2020, CSC/ECE 517.
Peer Review Information
For users intending to view the deployed Expertiza associated with this assignment, the credentials are below:
- Instructor login: username -> instructor6, password -> password
- Student login: username -> student4340, password -> password
- Student login: username -> student4405, password -> password
Expertiza Background
Expertiza is an educational web application created and maintained by the joint efforts of the students and the faculty at NCSU and supported by MIT and the National Science Foundation.
It’s an open source project developed on Ruby on Rails platform and it’s code is available on Github. It allows students to review each other’s work and improve their work upon this feedback.
Description of Project
The following is an Expertiza based OSS project which deals primarily with the AssignmentParticipant model and Review Mapping helper. It focuses on refactoring unkept, unused and misleading methods within the aforementioned files. The project’s goal is to enhance some of the methods in use and clean up others that were either outdated or in need of improvement. Some other parts of the application were modified as well to work with our changes in the Assignment Participant model.
Team Members
- Luke Burgess (laburges@ncsu.edu)
- Micah Gafford (mbgaffor@ncsu.edu)
- Robert Casano (rjcasano@ncsu.edu)
Files Modified
Our project focused primarily on refactoring the assignment_partipant.rb Model file. The files modified for this project are:
1. assignment_participant.rb
2. review_mapping_helper.rb
3. assignment_participant_spec.rb
4. publishing_controller.rb
5. user.rb
6. quiz_assignment.rb
7. response.rb
8. response_spec.rb
Assignment Participant Model
This is a controller that helps students and instructors view grades and reviews, update scores, check for grading conflicts and calculate penalties. A couple of long and complex methods were refactored from this controller along with removal of some non-functional code and a few language changes to make it Ruby style. Three methods in particular, namely conflict_notification ,calculate_all_penalties and edit were found to be too long and were in need of refactoring into smaller, easier to manage methods. Few more compact methods were created for this purpose.
There were no existing test cases for the controller. We have added a spec file named 'grades_spec.rb' under the spec folder. As no changes were done for the model, no tests for the model were included.
ReviewMappingHelper
This is a helper class that contained methods to help calculate and display information regarding participant reviews and the volume of each review. One non-functional method was removed from the file and two other methods were updated to support sorting the volume of an arbitrary number of review rounds.
List of Changes
We worked on the following work items(WIs)
WI1: Refactor self.grant_publishing_rights to assign_copyright
WI2: Determine whether reviews_by_reviewer method is still needed.
WI3: Refactor copy method name
WI4: Refactor assign_quiz
WI5: Refactor Review Score
WI6: Rewrite Score calculation within the scores method
WI7: Refactor Volume - # Distinct Words in a Review
WI8: Provide documentation for Compute_Assignment_Score
Solutions Implemented and Delivered
Refactoring Review Volume
This involved three methods in review_mapping_helper.rb, sort_reviewer_by_review_volume_desc, initialize_chart_elements and display_volume_metric. The first two methods worked together to sort the reviews by the average volume of reviews in each round of an assignment and then move the data of reviews in each round from a current round to prepare to display the information in chart form. The last method was found to not be used within the entirety of the project.
The following changes were made:
1. The methods were originally implemented in a way that only allowed for the information from three rounds of an assignment to be gathered. The functions had to be re-implemented using a loop that could gather and initialize the data from any given number of rounds.
2. The display_volume_metric was no longer being used within the rest of the project and so was removed to clean up the file.
sort_review_by_review_volume_desc loop re-implementation:
Initialize_chart_elements loop re-implementation:
Display_volume_metric removal:
Rewrite Scores Method
The scores method returns the scores that a specific assignment_participant has received Originally, the scores method contained several convoluted method calls with some unused and commented out code. Our team was tasked with rewriting the scores method in assignment_participant.rb to make it more readable and transparent.
The following changes were made to accomplish this task:
1. Removed topic_total_scores (score calculations for a microtask) and moved it into the scores method, since this method was only called once and wasn't used anywhere else. I also removed the associated tests.
2. Removed calculate_scores method and refactored the code into the scores method, since this method was only called once and wasn't used anywhere else. I also removed the associated tests.
3. Refactored the merge_scores method which is called by Scores.
- DRYed out the code by adding a new update_min_or_max method that dynamically sets the min or max
- Refactored merge_scores to use update_min_or_max method.
- Added 6 RSPEC tests for this new update_min_or_max method
4. Added comments for the scores method and merge_scores method
<Image Here>
Refactor assign_quiz Method
The assign_quiz method was originally located in the assignment_participant.rb file and was requested to be moved into quiz_questionnaire.rb. However, upon further investigation, it was discovered that there existed two assign_quiz_dynamically methods, one which called the aforementioned assign_quiz method. Our investigation concluded that a newly constructed assign_quiz_dynamically method, located in review_mapping_controller.rb, had become the primary method of performing the tasks associated with assign_quiz and assign_quiz dynamically. Hence, following the consultation of our mentor, we elected to remove the definition and all instances of assign_quiz (assignment_participant.rb) and assign_quiz_dynamically (quiz_assignment.rb).
The following changes were made:
1. Removed definition of assign_quiz method (assignment_participant.rb)
2, Removed all unused instances assign_quiz_dynamically (defined in quiz_assignment.rb)
- Alternative assign_quiz_dynamically method located in review_mapping_controller.rb became the primary method used in recent years
3. Ensured all instances of assign_quiz_dynamically (review_mapping_controller.rb) were untouched and operational as expected
- This definition does not implement assign_quiz method from assignment_participant
4. Removed #assign_quiz spec test from assignment_participant_spec.rb (see "Testing Details")
Investigate reviews_by_reviewer Method
The reviews_by_reviewer method returns the reviews of a certain assignment_participant. However, for the past 5 years or so all reviews have been of assignment_teams, not assignment_participant. Our team was tasked with investigating the reviews_by_reviewer method in assignment_participant.rb to determine whether it is still used, or would be useful to continue maintaining.
The following determinations were made: 1. Determined that all reviews are of assignment_teams and not of assignment_participant 2. The only calls to reviews_by_reviewer method happen in CollusionCycle. 3. After verifying with Yang and Dr. Gehringer, we determined that Collusion Cycle is no longer used in the application. CollusionCycle was originally a piece of code that helped detect whether bias existed when teammates gave reviews of other teammates' work. 4. Because CollusionCycle is unused, we deemed it appropriate to make the following changes:
- Removed the method definition from assignment_participant.rb
- Removed the rspec test for this method
Refactor copy Method Name
The copy method was determined to have a misleading name for a method that copies a participant to a course. As a result, the copy method name was refactored into 'copy_participant'.
The resulting changes were made to correspond to the name change:
1. Refactored copy method in assignment_participant.rb into 'copy_participant'
2. Refactored all instances of copy_participant associated with a assignment_participant
- Spec test in assignment_participant_spec.rb was only instance found (see "Testing Details")
Refactor self.grant_publishing_rights to assign_copyright
Aside from simply refactoring the name of self.grant_publishing_rights into assign_copyright, upon further investigation, we noted that the definition of the method in assignment_participant.rb, did not require the participant parameter and participant loop, but was simplified by moving this functionality to wherever the method is called in the application.
The following changes were made:
1. Refactored self.grant_publishing_rights into 'assign_copyright'
- Refactored definition of the method in assignment_participant.rb, removing the participant parameter and participant loop; moving this functionality to wherever the method is called in the application
2. Altered functionality of the method call in def generate_keys (user.rb) to handle the 'participants.each do' loop
3. Altered functionality of the method call in grant_with_private_key (publishing_controller.rb) to handle the 'participants.each do' loop
4. Built spec test for modified assign_copyright method in assignment_participant_spec.rb (see "Testing Details")
* Creates new RSA key-pair for a participant * Tests if key matches using assign_copyright
Refactor review_score method
The review_score method returns the review score for a particular questionnaire. The issue originally stated that the method should be able to determine the review score for any round of review. The original method returned the first round (index position 0) review score.
We made the following determinations:
1. The review_score method only returns the first round review score which is from index position 0:
2. We determined that review_score method is only being called in collusion_cycle.rb, which is no longer used in the Expertiza application.
3. We determined there was a reference to a “review_score” model in question.rb but there is no model that this refers to:
Changes Made:
1. Since review_score was only being called in collusion_cycle.rb, and collusion_cycle.rb is no longer used in the application, we deemed it correct to remove the review_score method from assignment_participant.rb:
2. Removed the reference to “review_score” model which appeared to be made in error since there is no review_score model. This change was made in question.rb file.
Additional Changes to Files
Response.rb
Additionally, in making some of the changes to the way expertiza calculates volumetric scores, we noticed incompatibilities because the methods in assignment_participant.rb call other methods to retrieve score information and calculations, which were hard coded for a maximum of three rounds of review.
We identified two methods in response.rb which dealt with the comments for different rounds of reviews, but would only calculate up to three rounds. These two methods were *self.concatenate_all_review_comments* and *self.get_volume_of_review_comments*. Although these methods are in response.rb which is not the primary focus of our project, our mentor instructed us to change the related methods to be able to run their calculations for any number of rounds of reviews.
The original methods (before our changes) were as follows:
As you can see, self.get_volume_of_review_comments relies and directly calls self.concantenate_all_review_comments. Thus, we needed to rewrite both of these methods to use the same convention as in sort_review_by_review_volume_desc where the calculations should be for any number of rounds, not just three.
We rewrote these two methods as follows to work with any number of rounds of reviews and comments.
In the rewritten code displayed above, our team has changed both methods to function to concatenate all review comments and then get the average word volume for each of those review comments for any number of rounds. This makes the code far more functional and universal to our changes elsewhere in the application.
We also made changes to the RSpec tests for these methods, which will be discussed in the Rspec Testing Section below.
Testing Details
Test Plan
For testing our changes, we focused on updating and designing new automated RSpec tests for every method. We also made sure to design a UI test to display that more than three rounds of reviews can be displayed on a page. See the #UI_Testing section.
For RSpec testing, we focused on designing an extensive suite of rspec tests for the assignment_participant model that we primarily dealt with. However, whenever we made any sort of change, we immediately went and updated/created the test cases regarding our changes. All RSPEC test changes are listed below in the #RSpec_Testing section.
RSpec
Refactor assign_quiz Method
Removed assign_quiz spec test from assignment_participant_spec.rb to correlate with the deletion of the assign_quiz method itself.
Refactor copy Method Name
Refactored copy to copy_participant in all spec test instances (assignment_participant_spec.rb).
Refactor self.grant_publishing_rights to assign_copyright
Following refactor, a new spec test needed to be built for the modified assign_copyright method in assignment_participant_spec.rb.
- Creates new RSA key-pair for a participant
- Assigns the public key of this key-pair to the user.public_key attribute.
- Tests if key matches using assign_copyright
In this test, we created a new RSA key pair in order to effectively test the functionality of the assign_copyright method. We then call the assign_copyright and pass the private key to this method. In order to test whether the function succeeded, we test to make sure that the permission_granted field is set to true for this participant.
Rewrite Scores Method
As described in the previous section, during the rewrite of the scores method our team cleaned up the merge scores method as well, by creating a new update_max_or_min method and abstracting/DRYing out the code. We wanted to make sure to thoroughly test this new method by adding the following rspec tests:
1. Removed topic_total_scores tests
2. Removed calculate_scores tests
We verified that the current scores tests were testing the functionality well and no changes were made to these tests.
3. We added 6 new tests to the rspec file to test the new methods for update_max_or_min. As displayed below, we test updating the min, and the max, under three different conditions.
Reviews_by_reviewer Method
Since we decided to remove this method because it is unused in the application (besides Collusion_cycle.rb), we removed the associated rspec tests as well:
Review Score
We also removed the Rspec tests for review_score method which we removed as stated in the previous section.
Collusion Cycle
As discussed in previous section, we also removed some failing tests in collusion_cycle_spec.rb, as per request from Dr. Gehringer, which began failing after we removed methods Collusion Cycle called from assignment_participant.rb. These test removals are displayed below:
Response
As discussed in a previous section, the response.rb file had to be edited so that the methods in this file are able to perform the comment volumetric calculations for any given number of rounds. Because of the changes discussed in the previous section, we had to update the corresponding Rspec tests to work with the new return format.
UI Testing
Following steps needs to be performed to test the volume refactoring in the UI:
1. Login as instructor and enter anonymized view using the manage tag.
2. Go to the manage tab and enter the assignments page
3. Choose any of the listed assignments, under the action section, click “view reports”
- If the course comes up with an error that means that the chosen assignment does not have any reviews. Please choose another assignment.
- (This particular error is out of scope of our current project)
- A good choice for an assignment would be 'Final Project (and design doc)
4. Select the view button
5. You should see the volume of reviews by round for each reviewer.
Scope for future improvement
1. We found that scores are calculated in many different places and in many different ways throughout the code. An interesting future project would be to identify where these different calculations are made, and refactor so they are all made in one place for the same type of scores. This will make the code more universal and easier to follow and understand.
2. Currently, if there are no reviews for an assignment you are thrown an error when trying to review the report. We did not fix this, due to it being out of scope of our project, but this error could be caught and the user could be redirected which would make the project work much better.