Move Webhook::DispatchEventJob to Sidekiq

[atsmith813]

What type of PR is this? (check all applicable)

• Refactor
• Optimization

Description

This creates a new Webhook::DispatchEventWorker (spec included) and starts sending jobs to it. Once this is merged and deployed, I will open a second PR to remove the old job code and spec.

Related Tickets & Documents

• Epic: Moving Jobs From DelayedJob to Sidekiq #5305

Added to documentation?

• no documentation needed

[rhymes]

Looks good! I just restarted the build because it was failing in a weird way, I'll keep an eye on the result and approve later if all goes well

[rhymes]

I thought I liked the old payload better but not I'm unsure hahaha

[mstruve]

I'm personally a fan of using a hash as arguments bc then it is clear what the params are without having to go into the job in the code to figure it out. But I'm good either way.

[rhymes]

I thought we had abandoned the idea in an old PR (not to use hash arguments) because they are not well supported by Sidekiq:

The arguments you pass to perform_async must be composed of simple JSON datatypes: string, integer, float, boolean, null(nil), array and hash. This means you must not use ruby symbols as arguments.

from https://github.com/mperham/sidekiq/wiki/Best-Practices

[atsmith813]

I could go either way here.

I know we've discussed not using hash arguments and using hash arguments.

My thinking for doing it this way was based on a few thoughts:

1. Since it's only 2 arguments, it's not quite as confusing as to what's being passed in.
2. When the job is called, you can see what's being passed in since both arguments are built beforehand just above it:

def call
  endpoint_urls = Endpoint.for_events([event_type]).where(user_id: record.user_id).pluck(:target_url)
  return if endpoint_urls.empty?

  event_json = Event.new(event_type: event_type, payload: PayloadAdapter.new(record).hash).to_json
  endpoint_urls.each do |url|
    DispatchEventWorker.perform_async(url, event_json)
  end
end

3. Since event_json is a Hash of its own, if we changed it to Hash arguments we'd have a nested Hash that we would probably want to do deep_symbolize_keys! on to keep everything consistent in the job. Not a big deal, but felt a little "dirtier" than necessary.

On the other hand, I'm all for the idea that using a Hash argument does improve clarity. I can just about always agree with that point!

[mstruve]

@rhymes we don't want to attempt to pass symbols, but a hash with string keys when you push past 3 arguments I think is a good idea. Like @atsmith813 mentioned, in this case we only have a couple, it's not a big deal, but the more you get a mapped hash is nice visibility I think. It makes it really easy to see in the dashboard what arguments are what.

[mstruve]

Probably should check out those spec failures, feels like we were possibly stubbing the old worker or something before.

[mstruve]

I'm personally a fan of using a hash as arguments bc then it is clear what the params are without having to go into the job in the code to figure it out. But I'm good either way.