Add skip_delivery_if config option to skip enqueuing unnecessary Delivery jobs

[justwiebe]

Pull Request

Summary:

Updated the EventJob to check a new DeliverBy skip_delivery_if conditional so that no unnecessary jobs are queued

Related Issue:

Description:

My project needs to send out notifications to 7000 users at a time. We are adding mobile app notifications, but very few users will have the mobile app, and even less should need iOS and Android notifications. Currently the if/unless conditionals are run after the job for that delivery method is queued up, which lets the job no-op, but with 14,000 jobs created, it can take a long time to run through them, even if they aren't doing anything.

To save time and allow the real notifications to be processed sooner, we shouldn't create jobs if we don't have to.

Testing:

I created unit tests on the EventJob to make sure that no extra jobs are queued up.

Screenshots (if applicable):

Checklist:

• Code follows the project's coding standards
• Tests have been added or updated to cover the changes
• Documentation has been updated (if applicable)
• All existing tests pass
• Conforms to the contributing guidelines

Additional Notes:

[excid3]

The jobs must run the conditions at execution time.

The reason being that a notification may be delivered several different ways:

Send websocket notification immediately
If unread after 2 minutes, send push notification
If unread after 10 minutes, send email notification

These need to evaluate during the execution of the job, not before.

We would need to introduce a separate configuration for this case that is clear in it's different usage.

[justwiebe]

Okay, I'll update it. What do you think of queue_delivery_job? as a method name @excid3?

[excid3]

What do you think of queue_delivery_job? as a method name @excid3?

I don't think it clearly describes it. Since it will always delivery by default, what if it was something like config.skip_delivery_if

[excid3]

I am still finding it hard to differentiate between if after sleeping on this. I was just reading Andy Croll's blog post and debating on naming still. https://andycroll.com/ruby/a-job-should-know-whether-to-run-itself/

skip_enqueue_if might be a better name?

class MessageNotifier
  deliver_by :action_cable

  deliver_by :email do |config|
    config.unless = ->{ read_at? }
    config.skip_enqueue_if = ->{ !recipient.email_notifications? }
  end
end

config.if/unless could also be renamed if we had a good way to differentiate between them.

Or maybe it's config.before_enqueue like in Andy's blog post and raising throwing an exception in the block would abort.

Just not sure. 🤔

[justwiebe]

What would your thoughts be on using skip_enqueue_if and notify_if instead of if and notify_unless instead of unless? Obviously those last two would need to be additional, with if/unless deprecated, unless you want to go to version 3 already

[excid3]

Noodling on it, I'm thinking before_enqueue may be good at providing additional functionality as a callback along with the ability to abort enqueues. I took a stab at refactoring that last night and it seems good.

config.before_enqueue = ->{ throw(:abort) unless recipient.email_notifications? }

This also keeps a nice name separation between the two.

[justwiebe]

Great, thanks for doing that!

[excid3]

Thanks for this PR @justwiebe! 🙏