Allow `Caffeinate::Action` to be usable beyond Caffeinate

Okay so I finally got some time to play with the Action stuff locally and got a pretty good sense of how to fix what I'd thought might've been a problem — essentially the presumption that "if you inherit from Caffeinate::Action, all of your actions must only be kicked off by Caffeinate drips." But I don't think that's a fair assumption.

If we look at the ActionMailer side of things, it'd be normal to have a UserMailer which has an action for welcome_email (non-Caffeinate) and thirty_day_followup (Caffeinate):

class UserMailer < ApplicationMailer
  layout "standard_email"

  def welcome(user)
    mail(
      from: Rails.config.generic_from_email,
      to: user.full_email,

      subject: "Welcome to XYZ Service!"
    )
  end
  # Fired off somewhere in your app by running:
  # UserMailer.welcome(@user).deliver

  def thirty_day_follow(mailing)
    user = mailing.subscriber

    mail(
      from: Rails.config.generic_from_email,
      to: user.full_email,

      subject: "How are you liking things?"
    )
  end
  # Fired off by Caffeinate Dripper
end

And both of those ought to work fine in their respective use-cases.

The trouble is that if we setup something similar for a Caffeinate::Action-inheritable, we run into trouble. Here's an example (and I'm going to use a two-layered approach here because I think that's the normal / encouraged setup, following ActionMailer encouraging having an ApplicationMailer then UserMailer, AgentMailer, etc):

class ApplicationTexter < Caffeinate::Action
  class Envelope
    def initialize(sms)
      @sms = sms
    end

    def deliver(texter)
      # do other stuff to prep / save @sms here
      @sms.send!
    end
    alias_method :deliver!, :deliver
  end

  def message(kwargs)
    Envelope.new(Sms.new(kwargs))
  end
end

class UserTexter < ApplicationTexter
  def welcome(user)
    message(
      from: Rails.config.generic_from_phone_number,
      to: user.phone_number,
      body: "Welcome to XYZ Service! We're so excited to have you..."
    )
  end
  # Fired off somewhere in your app by running:
  # UserTexter.welcome(@user).deliver

  def thirty_day_followup(mailing)
    user = mailing.subscriber

    message(
      from: Rails.config.generic_from_phone_number,
      to: user.phone_number,
      body: "Long time no see... everything okay?"
    )
  end
end

If you try this setup, using the welcome method somewhere else in the app will fail. Any Texter that inherits from Caffeinate::Action (directly or in a second layer etc.) cannot behave normally outside of the Caffeinate context (where there's only one arg passed in and it's a Caffeinate::Mailing). That's no good! I think Caffeinate::Action-inheritables ought to behave like ActionMailer — do the normal flow of things and run the normal callbacks and let everything run fine, only adding extra functionality if it detects that this particular flow is from a Drip; not preventing non-Drip-based flows from running normally.

This PR, which doesn't yet fix any specs, aims to fix that by passing through the action args plainly and only setting the magic self.caffeinate_mailing if the argument is indeed a single Caffeinate::Mailing. I probably also didn't do this correctly considering that args could actually be *args, **kwargs, so I need to fix that — a Mailer action should be able to accept any / all forms of parameters! Anyway, as long as the self.caffeinate_mailing doesn't get set, all of the other machinery operates totally normally and doesn't assume Caffeinate-level features (which is great!) so I think this is all we have to do! :thumbsup:

Random Thoughts:

Are you open to thinking about the name of Caffeinate::Action more? I think anything that inherits from this class should end in an "'er" like ActionMailer and my "Texter" and a hypothetical "WebPusher" — for that reason, what would you think about Caffeinate::Deliverer? Or honestly maybe even something a little more on-the-nose like Caffeinate::ActionMailerSpoofer?
Not a 'now' thing, but we should probably allow folks to opt in to registering other interceptors and observers for Caffeinate::Actions at some point — I think at the moment it's hard-coded to just be ::Caffeinate::ActionMailer::Interceptor and ::Caffeinate::ActionMailer::Observer. They can obviously override the inform_interceptors and inform_observers methods but it'd be nice if we had a class method for registering like ActionMailer itself: ActionMailer::Base.register_interceptor() / ActionMailer::Base.register_observer()

Still To-do:

[ ] Fix the args pass-through to support *args, **kwargs, possibly even &block in case someone needs that
[ ] Fix the specs!

Codecov Report

Patch coverage: 97.87% and project coverage change: +0.01 :tada:

Comparison is base (925b939) 98.62% compared to head (6aa25de) 98.64%.

:exclamation: Current head 6aa25de differs from pull request most recent head cc988bd. Consider uploading reports for the commit cc988bd to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #32 +/- ## ========================================== + Coverage 98.62% 98.64% +0.01% ========================================== Files 98 98 Lines 2479 2513 +34 ========================================== + Hits 2445 2479 +34 Misses 34 34 ``` | [Impacted Files](https://codecov.io/gh/joshmn/caffeinate/pull/32?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [lib/caffeinate/action\_proxy.rb](https://codecov.io/gh/joshmn/caffeinate/pull/32?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-bGliL2NhZmZlaW5hdGUvYWN0aW9uX3Byb3h5LnJi) | `85.96% <87.50%> (ø)` | | | [app/models/caffeinate/campaign.rb](https://codecov.io/gh/joshmn/caffeinate/pull/32?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-YXBwL21vZGVscy9jYWZmZWluYXRlL2NhbXBhaWduLnJi) | `100.00% <100.00%> (ø)` | | | [app/models/caffeinate/campaign\_subscription.rb](https://codecov.io/gh/joshmn/caffeinate/pull/32?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-YXBwL21vZGVscy9jYWZmZWluYXRlL2NhbXBhaWduX3N1YnNjcmlwdGlvbi5yYg==) | `100.00% <100.00%> (ø)` | | | [lib/caffeinate.rb](https://codecov.io/gh/joshmn/caffeinate/pull/32?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-bGliL2NhZmZlaW5hdGUucmI=) | `95.23% <100.00%> (ø)` | | | [spec/caffeinate/action\_proxy\_spec.rb](https://codecov.io/gh/joshmn/caffeinate/pull/32?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3BlYy9jYWZmZWluYXRlL2FjdGlvbl9wcm94eV9zcGVjLnJi) | `100.00% <100.00%> (ø)` | | | [spec/caffeinate/dripper/cases/perform\_spec.rb](https://codecov.io/gh/joshmn/caffeinate/pull/32?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3BlYy9jYWZmZWluYXRlL2RyaXBwZXIvY2FzZXMvcGVyZm9ybV9zcGVjLnJi) | `100.00% <100.00%> (ø)` | | | [...ec/models/caffeinate/campaign\_subscription\_spec.rb](https://codecov.io/gh/joshmn/caffeinate/pull/32?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3BlYy9tb2RlbHMvY2FmZmVpbmF0ZS9jYW1wYWlnbl9zdWJzY3JpcHRpb25fc3BlYy5yYg==) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

This PR, which doesn't yet fix any specs, aims to fix that by passing through the action args plainly and only setting the magic self.caffeinate_mailing if the argument is indeed a single Caffeinate::Mailing. I probably also didn't do this correctly considering that args could actually be *args, **kwargs, so I need to fix that — a Mailer action should be able to accept any / all forms of parameters! Anyway, as long as the self.caffeinate_mailing doesn't get set, all of the other machinery operates totally normally and doesn't assume Caffeinate-level features (which is great!) so I think this is all we have to do! 👍

This kind of breaks the convention between what Caffeinate interfaces with and what it doesn't interface with. For example, I can expect that all mailer actions that are specific to Caffeinate accept a mailing object. I think that everything that's setup for Caffeinate should have that expectation, don't you?

I think it's fine to assume that arguments are going to be a mailing to an Action (no, not married to the name). I don't think Caffeinate::Action should be used outside of the context of Caffeinate (thus the namespace). Because ActionMailer handles things that aren't Caffeinate-related, in addition to mail (messages) that are Caffeinate-related, we do need to check in the observer. But in a normal context — for example, if Caffeinate was a custom mailer method — we wouldn't, because we'd assume that everything that flows through it (mailing#process!) is a mailing.

Making Caffeinate::Action difficult to use outside of Caffeinate is a design choice. It's meant to be in isolation which is why its design is so strict: returning an object that responds to deliver! allows for the setup of another layer if it needs to handle something from a mailer.

If you can provide an example app where this flow makes sense perhaps that would help me wrap my head around what you're trying to do.

Edit: In your example, I don't think ApplicationTexter should be an Caffeinate::Action, but instead Caffeinate::Action methods should invoke (or wrap around) a separate class that handles the logic. I think in that sense Caffeinate::Action is trying to do too much.

In my original design of Caffeinate, I encouraged every Mailer to be separate (and I still encourage it, but I do break convention myself); Users::OnboardingDripper should map to Users::OnboardingDripperMailer, not Users::OnboardingMailer. I'm a firm believer in each mailer action should reasonably expect the same set of arguments, and if they don't, you're bordering on feature envy.

I think it's totally fair to want to have a strict API at the action level — if a given Mailer or Texter action is going to be used by Caffeinate, it shouldn't be used directly / outside of Caffeinate. But by offering the Action class as a general purpose "we'll handle some of the ActionMailer spoofing for you" class, it needs to be able to support both (1) methods that are for Caffeinate and (2) methods that are meant to be used somewhere else (directly, for example).

I see where you're coming from, I think we may just believe different things from the API front here.

I think in that sense Caffeinate::Action is trying to do too much.

There's probably some validity to that — the implementation I originally wrote up that inspired you to write Caffeinate::Action is indeed pretty generic. I wrote that to allow a Texter object to quack like, and behave like, an ActionMailer object. Not strictly for Caffeinate's sake (although it does leverage that spoofing), but for usage outside of Caffeinate too.

If the goal of Caffeinate::Action is to take on some of the responsibility of spoofing, I think it should do it in the general context, not the specific context of just Caffeinate actions. If it were the latter, I'd have to write my own spoofing logic again anyway, since I use my Texters in both Caffeinate-driven and non-Caffeinate-driven ways.

I'm a firm believer in each mailer action should reasonably expect the same set of arguments, and if they don't, you're bordering on feature envy

🤔 I'm not sure I agree with that one myself; I tend to take the approach that each Mailer class should be separated based on the target of who's being messaged. To me it makes sense to group that way, even if some actions in that class receive slightly different inputs. And, in practice, even those different inputs generally have to get down to the specific person anyway — so the input difference is more-so just a matter of unwrapping?

To that end, here's three actions within a single class that would all be sent to a User. Personally I don't think I'd want three different classes as a result of this. They're all doing the same thing and resolving down to a user variable, it's just a matter of how it's passed in. That being grounds for splitting to a whole separate class just feels like having a lot of extra files :stuck_out_tongue::

class UserMailer
  # For non-param usage:
  # UserMailer.welcome(@user).deliver
  def welcome(user)
    mail(to: user.email)
  end

  # For param-usage:
  # UserMailer.with(user: @user).welcome.deliver
  def welcome
    user = params[:user]

    mail(to: user.email)
  end

  # For Caffeinate-usage:
  # drip :welcome, mailer: :UserMailer, etc.
  def welcome(mailing)
    user = mailing.subscriber

    mail(to: user.email)
  end
end

But if you feel strongly that Caffeinate::Action should only be used in Mailer/[ETC]'er classes that implement only Caffeinate actions, that's okay. I'll just not use Caffeinate::Action and have my own spoofing logic in my Texter class, and that's cool! My Texter classes are used both inside and outside of the Caffeinate context and I need them to quack like an ActionMailer in both.

I napped on it and I think that a better description for Action would be some sort of proxy layer. At least that better accurately describes its intention of existing.

Would you be able to do something likeeeeee:

def welcome_for_caffeinate(mailing) 
  welcome(mailing.user)
end

I do feel strongly that Caffeinate::Action (or ActionProxy, or whatever), should be considered siloed. I think that its design is reasonable enough to accommodate what you're trying to do. I think I'm just not seeing it completely. If a light example app with some sort of "here's the problem we have internally, and here's how I'm trying to make Caffeinate work with it" where I can fire off some commands, I can probably figure out the missing link.

If a light example app with some sort of "here's the problem we have internally, and here's how I'm trying to make Caffeinate work with it" where I can fire off some commands, I can probably figure out the missing link.

That's totally fair. I'll see if I can spin something up for you later today 👍 the crux of it is that I'm using Noticed for notifications and Caffeinate for drips and I'm wanting both frameworks to call the same Texters/Mailers/etc'er's

see https://github.com/joshmn/caffeinate-noticed-example

bin/setup to get you all setup. am i still missing something? I feel like something just isn't clicking for me out of understanding your case here, which happens a lot because I am notoriously dense. 😂

Haha you're on it! Here's the missing link for your repo — the plugin for Noticed to add a delivery method: deliver by Texter.

# NOTE: Custom delivery method LARGELY modeled after the Email delivery method,
# since the Texter construct is largely modeled after ActionMailer!
# See: https://github.com/excid3/noticed/blob/master/lib/noticed/delivery_methods/email.rb
# NOTE: Doesn't support :enqueue option / 'deliver later' functionality. May add later.
class DeliveryMethods::Sms < Noticed::DeliveryMethods::Base
  def deliver
    texter.send(method.to_sym, format).deliver
  end

  private

  # texter: "UserTexter"
  # texter: UserTexter
  def texter
    option = options.fetch(:texter)
    case option
    when String
      option.constantize
    else
      option
    end
  end

  # Method should be a symbol
  #
  # If notification responds to symbol, call that method and use return value
  # If notification does not respond to symbol, use the symbol for the texter method
  # Otherwise, use the underscored notification class name as the texter method
  def method
    method_name = options[:method]&.to_sym
    if method_name.present?
      notification.respond_to?(method_name) ? notification.send(method_name) : method_name
    else
      notification.class.name.underscore
    end
  end

  def format
    params = if (method = options[:format])
      notification.send(method)
    else
      notification.params
    end
    params.merge(recipient: recipient, record: record)
  end
end

That allows your Notification to do:

deliver_by :text, class: "DeliveryMethods::Sms", texter: "UserTexter"

And thus your UserTexter class can have an action called user_notification (since that's the name of your notification)

joshmn / caffeinate

Allow `Caffeinate::Action` to be usable beyond Caffeinate #32

Codecov Report