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:

Metrics for perform method

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:

1. Refactor the single Sidekiq worker into multiple workers
2. Remove all code related to the drop_one_member_topics deadline type
3. Update and add unit tests according to the above changes
4. Test all new worker classes to ensure Sidekiq jobs were being dequeued and run as expected
5. 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_types 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.

Remove all code related to the drop_one_member_topics deadline type

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
end

The 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):

1. 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:

1. Edit an assignment.
2. Select the Use simicheck? box.
3. Set Simicheck Delay to 0 (or to a nonzero value to observe a delay).
4. Set the SimiCheck Similarity Threshold to a nonzero value.
5. 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).
6. Click Save.
7. 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:

1. Log into Expertiza as an instructor.
2. Navigate to Manage>Assignments in the navigation.
3. Click the plus button in the top right corner on the Manage Assignments and create an assignment with default settings.
4. Go to the Due Dates tab and assign due dates for both the assignment and the review.
5. 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.
6. 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.
7. 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.
8. To ensure that the assignments are now queued on Sidekiq, visit the Sidekiq UI by going to [Expertiza VCL URL]/sidekiq.
9. 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