CSC/ECE 517 Fall 2021 - E2144. Refactor delayed mailer and scheduled task
Background
This project revolves around the Sidekiq gem for asynchronous processing of email tasks. It has a queue system to hold and then process jobs. Sidekiq’s queue replaces DelayedMailer’s queue. The previous team that worked on this also created a method perform()
to gather email IDs of all participants in an assignment and send them an email reminder. Some test cases exist for this work.
Previous Setup
Initial work can be found here.
Code in the pull request was found to be reasonable and working. The method perform()
has:
Metric | Current Value | Limit |
---|---|---|
Cognitive Complexity | 15 | 5 |
Cyclomatic Complexity | 8 | 6 |
Assignment Branch Condition size | 22.93 | 15 |
Perceived complexity | 10 | 7 |
Previous pull request also has duplicated commit messages and the commits need to be squashed.
Files involved (some changed should not have been changed):
- app/controllers/assignments_controller.rb
- app/mailers/mail_worker.rb
- app/models/assignment_form.rb
- config/sidekiq.yml
- spec/models/assignment_form_spec.rb
- spec/sidekiq_mail_worker_spec.rb
- spec/spec_helper.rb
There are minor styling issues such as missing or trailing whitespaces, lines that are too long, extra empty line, unused arguments.
Problem Statement
This project revolves around the Sidekiq gem for asynchronous processing of email tasks. It has a queue system to hold and then process jobs. Sidekiq's queue replaces DelayedMailer’s queue. The previous team that worked on this also created a method perform() to gather email IDs of all participants in an assignment and send them an email reminder. Some test cases exist for this work. The project was partially merged.
What needs to be done: Feedback from the previous team (E1835) is: Code in pull request seems pretty reasonable. Some concerns wrt. naming functions, but otherwise reviewers seem to agree that the code is good.
Your pull request has many duplicated commit messages. Please try to squash
similar commits.
And using meaningful commit messages later.
You should commit changes to the DB schema (db/schema.rb
) only if you have created new DB migrations.
Please double check your code. If you did not aim to change the DB, please revert the DB schema changes. ***** Important, try not to change the DB.
You should not change rails_helper.rb
or spec_helper.rb
file; please revert these changes.
The previous team’s code needs a refactor in some places, though the actual function of it seems solid. It is very possible that in the three years since this code has been worked on, more mailer functionality has been added. There are other teams this semester working on mailer code as well. Try to keep your code as contained and simple as possible, so that merging these projects is possible.
Summary of Work Needed
After discussing with our project mentor and Dr. Gehringer, we determined the main tasks to be the following:
- Refactor the single Sidekiq worker into multiple workers
- Remove all code related to the
drop_one_member_topics
deadline type - Update and add unit tests according to the above changes
- Test all new worker classes to ensure Sidekiq jobs were being dequeued and run as expected
- Code cleanup
Refactor the single Sidekiq worker into multiple workers
Previously there was only a single worker class that handled all types of jobs. Its top-level method perform
looked like the following.
def perform(assignment_id, deadline_type, due_at) self.assignment_id = assignment_id self.deadline_type = deadline_type self.due_at = due_at assignment = Assignment.find(self.assignment_id) participant_mails = find_participant_emails if %w[drop_one_member_topics drop_outstanding_reviews compare_files_with_simicheck].include?(self.deadline_type) drop_one_member_topics if self.deadline_type == "drop_outstanding_reviews" && assignment.team_assignment drop_outstanding_reviews if self.deadline_type == "drop_outstanding_reviews" perform_simicheck_comparisons(self.assignment_id) if self.deadline_type == "compare_files_with_simicheck" else # Can we rename deadline_type(metareview) to "teammate review". If, yes then we donot need this if clause below! deadlineText = if self.deadline_type == "metareview" "teammate review" else self.deadline_type end email_reminder(participant_mails, deadlineText) unless participant_mails.empty? end end
The processing was different depending on the deadline_type
. The deadline_type
s in
%w[drop_one_member_topics drop_outstanding_reviews compare_files_with_simicheck]
did not lead to an email reminder being sent, but they were still handled in this MailWorker
class.
To make the code more manageable and easier to follow, it made sense to break up the responsibilities of this single MailWorker
class into multiple workers:
SimicheckWorker
DropOutstandingReviewsWorker
MailWorker
All three workers extended an "abstract class" called Worker
that contained the Sidekiq-related include
and the queue name to which all jobs would be sent.
For simplicity, we still had all new workers continue to use a single queue, which we renamed from mailers
to jobs
. A future improvement could divide these jobs to go to multiple different queues if needed.
It was decided that code used for dropping one member or two member teams from topics would never need to be implemented in the Expertiza website. Thus, we removed all code related to the drop_one_member_topics method within the Expertiza beta. This involved removing the drop_one_member_topics
function from drop_outstanding_reviews_mail_worker.rb
, now renamed drop_outstanding_reviews_worker.rb
. The drop_one_member_topics
method was called from the prepare_data
method, which was then called by the Sidekiq method perform
so we removed this call. The drop_one_member_topics
was also referenced in assignment_form.rb
using add_delayed_job
in the add_to_delayed_queue
method and in delayed_mailer.html.erb
as an option for the handler so both of these code snippets were removed as well.
Code Cleanup
Smaller improvements done while implementing the main tasks included:
- Using ActiveSupport::Duration objects in order to calculate the minutes and seconds related to the dequeue times
- Refactoring the time calculations for the dequeue times to the DueDate class
- Correcting the parameters passed to the workers, which were previously incorrect. As an example:
def add_delayed_job(_assignment, deadline_type, due_date, min_left) delayed_job_id = MailWorker.perform_in(min_left * 60, due_date.parent_id, deadline_type, due_date.due_at) delayed_job_id endThe first parameter after the time delay must be assignment ID, and
due_date.parent_id
is not an assignment ID.
- Removing whitespace issues reported by codeclimate
- Refactoring all variable names to be in snake_case
- Removing identical blocks of code
- Adding method comments to make methods more understandable
Test Plan
Unit Tests
In order to properly test that our refactoring had worked properly, we found that we would have to add in multiple tests to account for our new methods as well as refactor old tests to work with the changes that we had made.
For every worker, we have added a test case in the spec/workers/sidekiq_mail_worker_spec.rb
file: one each for MailWorker, SimicheckWorker, DropOutstandingReviewsWorker.
Further, we moved methods find_min_from_now_duration
from assignment_form.rb
to due_date.rb
. Along with that we introduced some methods to make it easier to understand the conversions and values that are being set.
Functional Tests
We decided that functional testing would need to be done by manually checking to see if Sidekiq tasks were queued as expected when changes to assignments were made. The three important functional tests that we decided needed to be verified were that the Simicheck Worker was queuing, the Mail Worker was queueing, and the Drop Outstanding Reviews Worker was queuing. These could be considered verified under the condition that the When field, Job field, and Arguments field in the Sidekiq view all came out as expected.
Considering Edge Cases and Other Possibilities
If we try to set the due date to be a date that has passed, no new jobs are queued as expected.
Implementing Test Plan
Update and add unit tests according to the refactoring changes made
The following tests were added according to the methods we moved and created:
describe "#find_min_from_now_duration" do it "returns time difference between due_date and now" do allow(DateTime).to receive(:now).and_return(DateTime.new(2021, 10, 20, 11, 11, 11).in_time_zone) due_at = Time.parse(DateTime.new(2021, 10, 20, 12, 12, 12).in_time_zone.to_s(:db)) expect(DueDate.find_min_from_now_duration(due_at)).to eq(61) end end describe "#get_dequeue_time_as_seconds_duration_from_now" do it "returns time difference between now and dequeue of job in seconds" do allow(DateTime).to receive(:now).and_return(DateTime.new(2021, 10, 20, 11, 00, 00).in_time_zone) due_at = DateTime.new(2021, 10, 20, 12, 00, 00) delay_duration = 1.hour assignment_id = create(:assignment, staggered_deadline: true, name: "testassignment").id due_date = create(:topic_due_date, deadline_type: @deadline_type, submission_allowed_id: @deadline_right, review_allowed_id: @deadline_right, review_of_review_allowed_id: @deadline_right, due_at: due_at, parent_id: assignment_id) expect(DueDate.get_dequeue_time_as_seconds_duration_from_now(due_date, delay_duration)).to eq(7200) end end
Test all new worker classes to ensure Sidekiq jobs were being dequeued and run as expected
Sidekiq setup
In order to test the Sidekiq workers, we needed to run Sidekiq (steps were obtained from this site):
- In a terminal separate from the one in which the main rails application is running, run
bundle exec sidekiq
This Sidekiq-specific terminal will show logs and printed statements from the workers.
NB: To create a generic sidekiq worker to test with, you could use
rails g sidekiq:worker TestWorker
to create a shell worker class.
Sidekiq UI
To see which messages were being enqueued and dequeued in Sidekiq, we use the Sidekiq user interface, which can be accessed via the /sidekiq
route of the application.
SimicheckWorker Test
To enqueue a Simicheck job:
- Edit an assignment.
- Select the
Use simicheck?
box. - Set
Simicheck Delay
to0
(or to a nonzero value to observe a delay). - Set the SimiCheck Similarity Threshold to a nonzero value.
- Ensure the Late Policy is set to
--None--
under the Due Dates tab. If there is a late policy set, per pre-existing code, the enqueued SimicheckWorker job will be deleted and will not appear in the Sidekiq UI (unless it is dequeued quickly, before the code can delete the jobs in the queue). - Click
Save
. - Observe a Simicheck Worker job enqueued and dequeued in the Sidekiq UI.
MailWorker and DropOustandingReviews Test
The Mail Worker sends out notifications to students if the due date of an assignment is changed. Drop Outstanding Reviews Worker drops all reviews that had not been worked on before the due date was changed. These both can be tested together, as when an assignment with a review is updated, Sidekiq should queue both the jobs for Mail Worker and Drop Outstanding Reviews Worker. To test these:
- Log into Expertiza as an instructor.
- Navigate to Manage>Assignments in the navigation.
- Click the plus button in the top right corner on the Manage Assignments and create an assignment with default settings.
- Go to the Due Dates tab and assign due dates for both the assignment and the review.
- Select a late policy next to Apply penalty policy:. If a late policy has not been created, press New Late Policy and follow the steps for creating a late policy.
- Assign student7613 to the assignment you created by clicking the person with the plus icon in the icons next to the assignment on the Manage Assignments page.
- Edit the assignment, go to the due dates tab, and change the due date on the review to be a day ahead of its current due date, and press save. The assignment due date must be changed to a time at least an hour ahead of the current time for the job to be queued.
- To ensure that the assignments are now queued on Sidekiq, visit the Sidekiq UI by going to [Expertiza VCL URL]/sidekiq.
- Upon arriving on this page, verify that the jobs added to the queue were correct by clicking to Scheduled. After arriving on this page, you should see something like the following:
The job MailWorker that had been queued successfully shows that our refactoring of Mail Worker had worked properly and the Mail Worker was operational. The job DropOutstandingReviewsWorker showed that our refactoring of Drop Outstanding Reviews Worker had also been done correctly.
Automated Tests
The following tests were added to test the workers:
describe "test different workers" do it "should increase the size of queue by 1 when MailWorker is used" do Sidekiq::Testing.fake! MailWorker.perform_in(42.minutes, 1, "review", "2021-10-27 00:00:01") queue = Sidekiq::Queues["jobs"] expect(queue.size).to eq(1) queue.clear end it "should increase the size of queue by 1 when SimicheckWorker is used" do Sidekiq::Testing.fake! SimicheckWorker.perform_in(42.minutes, 1) queue = Sidekiq::Queues["jobs"] expect(queue.size).to eq(1) queue.clear end it "should increase the size of queue by 1 when DropOutstandingReviewsWorker is used" do Sidekiq::Testing.fake! DropOutstandingReviewsWorker.perform_in(42.minutes, 1, "review", "2021-10-27 00:00:01") queue = Sidekiq::Queues["jobs"] expect(queue.size).to eq(1) queue.clear end end