CSC/ECE 517 Spring 2017 E1711

E1711. Refactor delayed_mailer.rb and scheduled_task.rb

Overview

Introduction to Expertiza

Expertiza is a web based tool that allows instructors to create collaborative assignments where students can provide peer feedback on each others work. It provides instructors a system to manage assignments and different courses.

Scope of project E1711

The goal of E1711 was to refactor code found in the files delayed_mailer.rb and scheduled_task.rb.

The following issues were identified as the scope of this project:

• scheduled_task.rb duplicated most of its code from delayed_mailer.rb. Our objective is to reduce/remove the duplication in keeping with the DRY principle.
• The perform method uses a giant case statement, instead we were to incorporate the use of polymorphism
• The mail_signed_up_users is long and should be broken into smaller and better named methods
• Add/modify test cases as we added/removed/modified areas of the code

Information about the assignment can be found on this document

NOTE: All our changes and updates on the project is committed on the "oss-branch" in our remote git repository, not the "master" branch. [1]

Test Plan

As this was a purely refactoring effort, our testing consisted of confirming existing tests did not break, modifying existing tests to match our changes, or adding tests as needed. The below sections will cover each objective in detail and will include information on how testing was done for each of those changes. See each of the Our Testing sections.

The following flowchart shows how to arrive at the tested scenarios. The charts refer to Scenarios by number. Below Summary table delves into details of the test cases.

Instead the below table summarizes the test cases for delayed_mailer.rb Much of our work consisted of modifying the existing tests so they were more thorough and adding some tests. The selection of the added tests were to verify the functions that were restructured as a part of this effort. Functions that were not touched are out of the scope of this assignment so no tests were added there. Adding tests for other functions can be a future assignment.

You can find the test cases in delayed_mailer_spec.rb

Also you will note at the start of the tests, a lot of information is created in the DB. All these things need to be in the DB in order to verify that the mailer is successful. Also note that for each of the groups getting emails, the test cases only deliver one email. That is because the DB has a minimum amount of test data where each group to send the emails to only has one member and one email address.

Scenario Number	Scenario Description	Test Type	Test Description	Test Tool	Test Added/Modified
1	Email submission deadline reminders to signed up users	Unit	A new delayed mailing task is created with deadline type submission deadline. The number of delayed jobs pending is increased by 1. One second later, the job launches. Verify that the deadline type of the job is submission deadline. Verify the count of mail deliveries increase by 1.	Rspec	Modified
2	Email review deadline reminders to reviewers	Unit	A new delayed mailing task is created with deadline type review deadline . The number of delayed jobs pending is increased by 1. One second later, the job launches. Verify that the deadline type of the job is review deadline. Verify the count of mail deliveries increase by 1.	Rspec	Modified
3	Email metareview deadlines to metareviews and team members of the assignment	Unit	A new delayed mailing task is created with deadline type metareview deadline. The number of delayed jobs pending is increased by 1. One second later, the job launches. Verify that the deadline type of the job is metareview deadline. Verify the count of mail deliveries increase by 2. The increase is by 2 instead of 1 like the other tests since both a metareviewer and a team member were emailed.	Rspec	Modified
4	Email drop topic deadline reminders to reviewers	Unit	A new delayed mailing task is created with deadline type drop topic deadline. The number of delayed jobs pending is increased by 1. One second later, the job launches. Verify that the deadline type of the job is drop topic deadline. Verify the count of mail deliveries increase by 1.	Rspec	Modified
5	Email sign up deadline reminders to all assignment participants	Unit	A new delayed mailing task is created with deadline type sign up deadline. The number of delayed jobs pending is increased by 1. One second later, the job launches. Verify that the deadline type of the job is drop topic deadline. Verify the count of mail deliveries increase by 1.	Rspec	Modified
6	Email team formation deadline reminders to reviewers	Unit	A new delayed mailing task is created. with deadline type team formation deadline The number of delayed jobs pending is increased by 1. One second later, the job launches. Verify that the deadline type of the job is team formation deadline. Verify the count of mail deliveries increase by 1.	Rspec	Modified
7	Verify find_team_members_email_for_all_topics works.	Functional	New data is inserted into the DB such that there is a single team member on a single assignment with a single sign up topic whose email address is expertiza@malinator.com. Then a new DelayedMailer object is created. The method is then called and we verify that the found email address is the expertiza@mailinator.com, as expected.	Rspec	Added
8	Verify mail_signed_up_users works when there are no sign up topics.	Unit	New data is inserted into the DB such that there is a single team member on a single assignment with no sign up topics. Then a new DelayedMailer object is created. The method is then called and we verify that there was a call to find_signed_up_team_members_email, as expected.	Rspec	Added
9	Verify mail_signed_up_users works when there are sign up topics.	Unit	New data is inserted into the DB such that there is a single team member on a single assignment with a single sign up topic. Then a new DelayedMailer object is created. The method is then called and we verify that there was a call to find_signed_up_team_members_email_for_all_topics, as expected.	Rspec	Added
10	Verify find_signed_up_team_members_email works.	Unit	New data is inserted into the DB such that there is a single team member on a single assignment with a no sign up topic whose email address is expertiza@malinator.com. Then a new DelayedMailer object is created. The method is then called and we verify that the found email address is the expertiza@mailinator.com, as expected.	Rspec	Added

Our team tried to improve the test file. If you see the delayed_mailer_spec.rb history, you notice the test cases were more like just creating a DelayedMailer object and putting that into DelayedJob queue, and then checking if the queue size has increased or not. Well..that was a very poor quality test from our perspective, because the actual functionality of sending emails per different deadline type was not tested and besides that, there were no assignment, no topic, no user, and etc in the test scenario at all. Now in our test cases we have created all the necessary objects and set their relations to create a fair test environment with an assignment, topic, team and user, then added test cases which actually test the "perform" method functionality (perform method is the major method of DelayedMailer which eventually sends emails to certain recipients per deadline type). Therefore now we claim our tests cases provide a higher confidence rather than before when they pass.

Remove duplicated code between delayed_mailer.rb and scheduled_task.rb

Background of Objective

The main objective of this task was to reduce duplicated code as recommended by DRY principle.

After a deep investigation into understanding why the same code would appear in two separate files, we concluded that the authors of scheduled_task.rb found that delayed_mailer.rb already integrated with a Ruby gem called delayed_job. delayed_job is used to run tasks asynchronously in the background. It was used in delayed_mailer to add an outgoing email to a queue to send out in the future. Instead of recreating similar infrastructure, the authors copied over the file to a different location, added on the new desired functionality, and renamed it. The authors also pointed all existing code to use scheduled_task.rb instead of delayed_mailer.rb

Our Changes

We found that the vast majority of the code in both the files applied to sending emails so we decided to merge the scheduled_task.rb into delayed_mailer.rb by porting the enhancements made for the scheduled tasks into delayed_mailer.rb. Doing so allowed us to meet the objective of DRYing out the code. However, we still have a situation where scheduled tasks feature has code in a section devoted to mailer code. We recommend that a future project take up the needed changes to pull out the scheduled task code out of the mailer file and make it a standalone feature as it should have been originally. See details in the Future Refactoring Opportunities below.

Existing Tests

We did not find any existing automated testing targeting this area of the code. All existing tests in the spec file associated with this feature were copies of the tests in the delayed mailer feature and did not exercise any of the new code that was introduced with adding the new types of scheduled tasks.

Our Testing

We went back to the demonstration of this feature and manually tested all the same scenarios. We verified that no functionality presented in the scheduled tasks demonstration was broken. Tests were added for the delayed mailer feature (covered in more details in the following sections). We recommend tests focused towards scheduled tasks are added once the scheduled tasks code is removed from the delayed mailer file back into an independent file.

Refactor the perform mehtod

Background of Objective

The objective of this task was to clean up this method. It was identified as being badly named, long, and not taking advantage of good design. It is essentially a giant case statement where one of the variables passed into the DelayedMailer constructor determines the people who receives the email.. Some suggested ways to refactor this function was first rename the perform to better describe what it does and to use polymorphism in place of the case statement. The original thought was that polymorphism could be used to determine what was sent out in each email based on the type of the email.

Our Changes

The first item we looked at was to rename the method. A generic name such as perform usually means that the method is doing too much or does not have a clear objective. Unfortunately, we found we could not rename the method because the delayed_job gem requires a method named perform in order to work on a custom job. See documentation here

The second part of the planned refactoring was to use polymorphism to determine the content of the email. After simplifying much of the code in the file, we discovered that the content of the email was independent of the variables used to initialize the object. At that point, we weighed the complexity of new code to use polymorphism versus a significantly more simplified case statement, and because of the lack of existing tests, decided that the simplified case statement was not only short, it is also very simple to follow the flow through the code. We instead focused on DRYing out this code as a better return on immediate investment as opposed to adding polymorphism.

Existing Tests

There were six existing Rspec tests for delayed_mailer.rb, but the existing tests only verify that a new delayed job was added to the queue based on the type of deadline specified when creating a new DelayedMalier. We made the existing tests more complete by adding in sections that properly created the data objects. We also added verification that once the job is executed, the deadline type for the job is what we expected. We also added verification to make sure that the number of jobs were changing and the mail delivery counts were increasing as expected.

The six existing tests that we modified were scenarios 1 to 6 listed above.

Our Testing

We modified existing test cases for the perform method to be more thorough. We continuously would run these tests during development to make sure they continue to pass.

Refactor mail_signed_up_users method

Background of Objective

The primary objective of this task was that the method is long and could be broken into smaller appropriately named methods.

Our Changes

We replaced the existing single method mail_signed_up_users with a shorter version and a new method find_team_members_email_for_all_topics. We also heavily modified (including renaming) the existing method getTeamMembersMail, and it is now called find_team_members_email. As the naming of the methods suggest, there are still opportunities for DRYing the two functions to get team member emails, but exiting code makes it extremely difficult to combine the two functions exactly though they are extremely similar at first glance. Merging the two would require refactoring areas outside the scope of this assignment and a whole new set of test cases to verify a lot of basic functionality is not broken in the process. We recommend this is taken up as a part of a future assignment. See Future Refactoring Opportunities below.

Existing Tests

We did not find any existing automated testing targeting this area of the code. None of the six existing tests verified if the emails are being retrieved from the database so changes in the database schema would break the feature.

Our Testing

We added test cases for each of the methods we changed. There are now 10 test cases that are covering this file. The four new test cases focus on verifying that the functions can access the database to protect against schema changes and making sure the right methods are called. Tests were not added for code that was not within the scope of this assignment but we recommend the remainder of the functions also get similar test cases. See details in Future Refactoring Opportunities below.

Future Refactoring Opportunities

Once we started to refactor the code, we found many additional opportunities to refactor the files that was beyond the planned scope. We note down some of the opportunities here and leave it to future projects to address.

1. Scheduled Task feature does not belong in the mailers. It needs to be a standalone file with new tests added.

2. There are still methods that look like they should be collapsed into one function and opportunities for DRYing out code once other tightly coupled methods are also refactored.

3. There were practically no tests for this area of the code. We added tests for what we modified but much code still remains that is not being tested.

4. There are many calls to the database for the same information in different methods. These repeated calls could probably be reduced to one call and save the result from the database.

5. We found out that all assignments in Expertiza are team assignments and querying if the assignment is a team assignment is hardcoded to return true. perform does have some business logic where the recipient of the email depends on whether or not the assignment is a team assignment. This business logic needs to be revisited by the Expertiza team before the method can be further simplified to ensure the correct mail goes out in the correct scenario. We kept the logic the same as it was out of our scope to change business logic.

Extra Credit

During our testing, we found that the "create new late policy" function was broken. It was throwing a ActiveModel::ForbiddenAttributesError on LatePoliciesController#create, as seen below:

To fix this, we had to add the following code to the private section of late_policies_controller.rb:

   def late_policy_params
   params.require(:late_policy).permit(:policy_name, :penalty_per_unit, :penalty_unit, :max_penalty)
   end

and change the argument in the LatePolicy.new() to just

   @late_policy = LatePolicy.new(late_policy_params)

After these two small changes, we were able to create new late policies from the UI.