thoughtbot / griddler

Simplify receiving email in Rails (deprecated)
http://griddler.io/
MIT License
1.38k stars 199 forks source link

ActiveJob support #273

Closed fabn closed 4 months ago

fabn commented 7 years ago

Would you accept a PR that (conditionally) introduces support for ActiveJob? Something like:

  def create
    normalized_params.each do |p|
      process_email(p)
    end

    head :ok
  end

  # ...

  def process_email(params)
    if griddler_configuration.use_active_job?
      processor_class.perform_later(email) # In this case processor class must extend ActiveJob::Base
    else
      processor_class.new(Griddler::Email.new(params)).public_send(processor_method)
    end
  end
fabn commented 7 years ago

I'm asking this because currently Griddler::Email is not serializable as AJ parameter, a very quick alternative would be to make Griddler::Email#params public, in this way a user may do this

# in processor
MyJob.perform_later(email.params)

# in Job class
def perform(params)
  email = Griddler::Email.new(params)
end

But in any case it would be useless to process the email twice, once in controller and later in the job.

wingrunr21 commented 7 years ago

I think this is a good idea but if implemented I would not want to limit it to ActiveJob. We'd also need to support setting up the queue, priority, etc.

fabn commented 7 years ago

I agree about other settings, mainly queue name, but I'm reluctant to explicitly support other queue BG job processors, that's ActiveJob's job 😄

Meanwhile I've implemented this in my project using a monkey patch in this way

# in config/initializers/griddler.rb
module Griddler

  # Reopen configuration class to allow active job support
  module ConfigurationExtension

    # Enable active job support, interpret processor job class as an active job subclass
    def use_active_job!
      @active_job = true
    end

    def use_active_job?
      defined?(@active_job) ? !!@active_job : false
    end
  end

  # Prepend module to override behavior
  Configuration.prepend(ConfigurationExtension)

  # Reopen controller to change its behavior to use active job
  module EmailsControllerExtension

    def self.prepended(base)
      base.delegate :use_active_job?, to: :griddler_configuration
    end

    def create
      normalized_params.each do |p|
        process_email(p)
      end

      head :ok
    end

    def process_email(params)
      if use_active_job?
        processor_class.perform_later(params) # In this case processor class must extend ActiveJob::Base
      else
        processor_class.new(Griddler::Email.new(params)).public_send(processor_method)
      end
    end
  end

  # Prepend the module to override behavior
  EmailsController.prepend(EmailsControllerExtension)

end

# Configure griddler
Griddler.configure do |config|
  config.processor_class = EmailCommandJob
  config.use_active_job!
  config.reply_delimiter = '-- REPLY ABOVE THIS LINE --'
  config.email_service = :sendgrid
end
wingrunr21 commented 7 years ago

ActiveJob's purpose it to provide opinionated background job support for Rails. By necessity, that means its support is sort of "lowest common denominator". Sidekiq, for instance, has a much better retry feature than that built into ActiveJob. By choosing to only support ActiveJob, downstream consumers are then forced to use that interface if they need background job support with Griddler. I'd rather not ask people to use ActiveJob just to get that feature (especially if they aren't loading it along with the rest of Rails since they are using a different job strategy).

The monkeypatch looks fine (although a refinement may be a better implementation here?). I'm not a huge fan of use_active_job? for every iteration of that loop but in the grand scheme of things oh well.

mull commented 7 years ago

So how do people actually do this now? I definitely want to pawn off processing emails to the background

wingrunr21 commented 7 years ago

Off the top of my head:

class ActiveJobEmail < Griddler::Email
  # implement GlobalID so this class can be serialized
end

class GriddlerJob < ActiveJob::Base
  def perform(email)
    # do something with email
  end
end

Griddler.configure do |config|
  config.processor_class = GriddlerJob
  config.email_class = ActiveJobEmail
  config.processor_method = :enqueue
end

Alternatively, instead of subclassing Griddler::Email, you could override initialize in the job and convert the email to a hash prior to enqueuing it.

Note that I haven't actually tried this approach.

MikeRogers0 commented 6 years ago

Depending on how much data in coming from your Email Service, it could be a little risky to pass your griddler object class straight ActiveJob. For example, if you receive 5 emails with 100mb worth of attachments you're potentially passing a lot of data around (Or losing references to temporary files).

I think a better approach is to have something like:

class GriddlerEmail < ApplicationRecord
  # Some validations, saving just the data you need.
  # You could use ActiveStorage for saving attachments.
end
class GriddlerReceiver
  def initialize(email)
    @email = email
  end

  def perform(email)
    # Pass your Griddle email attributes somewhere into your database.
    griddler_email = GriddlerEmail.new
    griddler_email.attributes = email.to_h

    # If you've got your bare minimum, go off and do whatever you need to do to that emails data.
    griddler_email.save && GriddlerJob.perform_later(griddler_email)
  end
end