add support for Actions

[joshmn]

This will allow for PORO support, in addition to integrating with ActionMailer.

In the future, we should rename mailings to messages and change the inference that Caffeinate only handles ActionMailer-based stuff to any-based stuff.

Resolves #14

cc @jon-sully

[codecov]

Codecov Report

Patch coverage: 94.80% and project coverage change: -0.26 ⚠️

Comparison is base (9b6efaa) 98.88% compared to head (abff332) 98.62%.

❗ Current head abff332 differs from pull request most recent head 0c08b9a. Consider uploading reports for the commit 0c08b9a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
- Coverage   98.88%   98.62%   -0.26%     
==========================================
  Files          95       98       +3     
  Lines        2330     2479     +149     
==========================================
+ Hits         2304     2445     +141     
- Misses         26       34       +8     

Impacted Files	Coverage Δ
spec/caffeinate/drip_spec.rb	96.36% <ø> (ø)
lib/caffeinate/action.rb	85.45% <85.45%> (ø)
lib/caffeinate.rb	95.23% <100.00%> (+0.23%)	⬆️
lib/caffeinate/dripper/defaults.rb	100.00% <100.00%> (ø)
lib/caffeinate/dripper/drip_collection.rb	100.00% <100.00%> (ø)
lib/caffeinate/message_handler.rb	100.00% <100.00%> (ø)
spec/caffeinate/action_spec.rb	100.00% <100.00%> (ø)
spec/caffeinate/dripper/cases/drip_spec.rb	100.00% <100.00%> (ø)
spec/caffeinate/dripper/cases/perform_spec.rb	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

[jon-sully]

Thanks for being so on this! I might need a little time to fully grok the idea of the Action ("built your on ___'er" like my Text'er) and feel which parts of the abstraction it covers. Will review though!

[joshmn]

Thanks for being so on this! I might need a little time to fully grok the idea of the Action ("built your on ___'er" like my Text'er) and feel which parts of the abstraction it covers. Will review though!

Sure! The idea here is just to create something that works just like ActionMailer::Base (with the delegation to instance methods):

class UserMailer < ActionMailer::Base
  def welcome(mailing)
  end
end

to

class UserAction < Caffeinate::Action
  def welcome(mailing)
  end
end

Can be invoked the same.

This means that internal changes remain minimal. If, from an Action, a developer needs to break out, they can:

class UserAction < Caffeinate::Action
  def welcome(mailing)
    SomeThing.new # whatever 
  end
end

It's kind of hacky, kind of gross, but I didn't want to make a ton of internal changes. Right now, as is, Action-based drips won't support Caffeinate's deliver_later support but that's just because I'm kind of lazy right now.

[jon-sully]

Haha I support not implementing deliver_later for actions. I think a two-layer approach may end up being the route folks choose — and that's mirrored with ActionMailer: having an ApplicationMailer. I think that helps cement the idea that a Caffeinate::Action is a blueprint for a delivery mechanism, of sorts:

class ApplicationTexter < Caffeinate::Action
  def deliver
    # should this be here? Shared delivery logic? or pushed down?
    # should Caffeinate expect this method to exist since that matches Action Mailer?
  end
end

# ...

class UserTexter < ApplicationTexter
  def welcome(texting) # for Texters I call them "texting"s even though they're technically "mailing"s
    @user = :foo
    # set up other stuff here?
    # save off a DB record here if that's your thing?

    # return nothing; just setup local state so that when Caffeinate calls #deliver everything's ready?
  end 
end

Just spitballing but trying to figure out how the implementation API will look 🤔

I like the idea of exposing an ::Action wrapper in the first place, I think that's neat. Could help clarify some layers of abstraction. Just want to make sure it feels like it slots in well? 🤔

Ah, one other note that may be interesting for you is the sense of what a "PORO" is in the first place. This assumes that my Texter can inherit from Caffeinate::Action — but in our app a Texter is used both by Drippers (time-based messages coordinated by Caffeinate) and by Notifications (real-time alerts coordinated by the Noticed gem). Both Drippers and Notifications invoke UserTexter, for instance. The only difference is what gets passed to them (a mailing vs. a params hash).

So it may be useful to think about inheritance vs. mix-ins or other means of sharing behavior? And/or what we actually need out of a PORO for it to correctly implement / behave like a Caffeinate Actionable

[jon-sully]

class ApplicationTexter
  acts_like_action_mailer

  def deliver
    # Developer should implement logic here for how to synchronously do their delivery in whatever medium this is
  end
end

class UserTexter < ApplicationTexter
  def welcome(texting)
    @user = texting.subscriber
    @external_number = @user.phone
    @internal_number = Rails.config.company_texting_number
  end
end

# dripper setup
class RandomDripper
  drip :welcome, sender: :user_texter, delay: 30.minutes
end

# Caffeinate effectively calls this under the hood?
message = UserTexter.welcome(mailing)
message.deliver
# pretty similar to ActionMailer?

Just some API ideas coming to mind 🤔 trying to think about how all of this would come together for a third delivery mechanism. Mailers, Texters, maybe WebPushers?

[joshmn]

@jon-sully Ahhh I see your thinking now! Pushing up a change that reflects that use-case. Watch this.

[joshmn]

So here's the situation now:

There's the Action which will implement #deliver (following pattern of Mail::Message). Then we'll fall into #do_delivery (again like Mail::Message) and use its delivery_method logic to handle deliver.

I imagine:

class FancyClient
  def initialize(phone_number, message)
    @phone_number = phone_number
    @message = message 
  end

  def deliver!
    HTTParty.post ... 
  end
end

class UserAction < Caffeinate::Action
  def welcome(mailing)
    FancyClient.new(mailing.subscriber.phone_number, "welcome")
  end
end

And then under the hood, all should be well.

[jon-sully]

🤔 I've given this a good read-over and mulled it in my head but I think I just feel like of lukewarm about it, personally — trying to pinpoint why, exactly. I think what I like about my project's current Texter setup is that Texters are technically independent of Caffeinate altogether; their sole goal is to implement the ActionMailer API on the outside but send texts instead. In that way, Caffeinate doesn't need to know anything special about them at all.

The entire footprint for spoofed integration is that a new ActionMailer 'spoofer' must:

• Receive an unknown class method and convert it to an instance method on a new instance
• Return something from that instance method call which can receive:
  • #deliver to 'do the delivering process'
  • #perform_deliveries= as a flag for whether or not the delivery should go ahead
  • #caffeinate_mailing= as a temporary pointer to what prompted the message
• And should call Caffeinate::ActionMailer::Interceptor.delivering_email self at the beginning of the #deliver method
• And should update the @caffeinate_mailing after the delivery work is done

And in practice, I don't think that's actually that much code:

class ApplicationTexter
  # NOTE: Delegates unknown class methods to a new instance
  class << self
    delegate_missing_to :new
  end

  attr_accessor :caffeinate_mailing
  attr_writer :perform_deliveries

  def deliver
    @perform_deliveries ||= true
    Caffeinate::ActionMailer::Interceptor.delivering_email self
    return unless @perform_deliveries

    @sms.save!
    @sms.send!

    if caffeinate_mailing
      caffeinate_mailing.update!(sent_at: Caffeinate.config.time_now, skipped_at: nil)
      caffeinate_mailing.caffeinate_campaign.to_dripper.run_callbacks(:after_send, caffeinate_mailing, self)
    end
  end

  def render(template_name = nil)
    # helper to render SMS body from view html.erb files
  end

  def message(kwargs)
    tap do
      @sms = Sms.new(kwargs)
    end
  end
end

And this particular setup means that subclasses (in parity with ActionMailer) are pretty simple:

class UserTexter < ApplicationTexter
  def long_term_followup(texting)
    @user = texting.subscriber.decorate

    message(
      sender: Rails.robot,
      recipient: @user,
      internal_number: Rails.config.our_phone_number,
      external_number: @user.phone,
    )
  end
end

So these follow the ActionMailer-style API without any Caffeinate classes / inheritance:

# how you would use one directly
UserTexter.long_term_followup(params).deliver

# how you'd use one in a dripper, it wouldn't know any better!
class WelcomeDripper
  drip :long_term_followup, mailer: :UserTexter, delay: 2.days
end

So the question (to me) becomes, okay what can Caffeinate offer that simples / removes some of this ^ code and makes my life as the developer simpler? And that's challenging because I still want to uphold the ActionMailer-style API of Class.method(args).deliver so that these Texters can be used elsewhere too.

I like the idea that Caffeinate::Action automatically covers the "pre-send" and "after-send" logic so that I wouldn't have to call Caffeinate::ActionMailer::Interceptor.delivering_email self and caffeinate_mailing.update!... myself; that's a win!

I also like that Caffeinate::Action smooths over the "class method but it's an instance" funkiness that ActionMailer does for me too; so I can remove the class << self; delegate_missing_to :new stuff from my class.

But beyond those two things, I'm pretty sure I'd still need the rest of my code in place, right? And ApplicationTexter would be inheriting from Caffeinate::Action (which is fine since it currently is a PORO) but I'd still have two layers — the ApplicationTexter then individual Texters by recipient (per ActionMailer's norms). So still the same number of files.

That's kind of where I'm at — trying to weigh the balance of having to inherit from another class (which wouldn't work if my Texters were already inheriting from something) for the sake of saving some code (which is valuable for sure) at the cost of more complexity and machinery.

And, I should totally note, I don't mean any of this negatively and I love the stuff you built in this PR! Just hope to iterate toward a solid API that feels natural to Texters (or WebPushers, or etc.) in their own stand-alone context

[joshmn]

Sorry for the delayed reply. Was doing interviews all day.

@jon-sully these are great points.

And in practice, I don't think that's actually that much code:

I don't think it is either. :) But, I don't want to make the user (developer) to have to think — having one way of doing things means it's less brittle, and keeping that footprint as small as possible is one way of doing this. :)

But beyond those two things, I'm pretty sure I'd still need the rest of my code in place, right?

I probably should have shown what your stuff would look like if this was merged. I'd like to think it's much cleaner and has even less surface area. You'd remove your ApplicationTexter, too. :)

If you tell yourself a Caffeinate::Action is the equivalent to ActionMailer::Base, and the subsequent Envelope is a Mail::Message, I think it might be more palatable. :)

class TexterAction < Caffeinate::Action
  class Envelope
    def initialize(user)
      @sms = Sms.new(
        sender: Rails.robot,
        recipient: user,
        internal_number: Rails.config.our_phone_number,
        external_number: user.phone,
        )
    end

    def deliver!(action)
      @action.action_name # :long_term_followup
      @sms.save!
      @sms.send!
    end
  end

  def long_term_followup(texting)
    user = texting.subscriber.decorate

    Envelope.new(user)
  end
end

I am personally a huge fan of this because of how contained it is and how predictable it is, and how testable it is. I can make sure that Envelope is handling everything correctly, and I can let Caffeinate do the work about whether or not it should be delivered, if the mailing (texting) should be marked as sent, etc. :)

Let me know if this helps clarify things or if you have further thoughts. Again, I haven't had this use-case yet myself (and that's ultimately how personally find things I need to build — usually after many iterations) so I have the obvious blindspots. The goal of Caffeinate is to provide a way of scheduling and executing some action on/at/after a specific time, and hiding the logic surrounding that is key so that the user doesn't end up shooting themselves in the foot. :)

[jon-sully]

If you tell yourself a Caffeinate::Action is the equivalent to ActionMailer::Base, and the subsequent Envelope is a Mail::Message, I think it might be more palatable. :)

Ah! 💡 There is is. Yep, bringing in Envelope made it much more clear and yeah, this does actually. help to get more in line with ActionMailer's "mailer vs. Mail object" paradigm.

Probably would still need a top-level Texter and subclasses (the same way that we typically have ApplicationMailer < ActionMailer::Base and UserMailer < ApplicationMailer) since those subclasses are typically subclassed per-recipient (e.g. we have Users and Agents, so we'd want a UserTexter and AgentTexter) and the Envelope definition would probably live up in the root Texter, but I think that's actually fine.

Thanks for circling back on that example! Since examples make the whole thing more visible, here's how I'd envision this whole thing working from my eyes (still the same Caffeinate::Action code):

class ApplicationTexter < Caffeinate::Action
  class Envelope
    def initialize(kwargs)
      @sms = Sms.new(kwargs.reverse_merge(
        sender: Rails.robot,
        direction: :outbound
      ))
    end

    def deliver!(action)
      @action.action_name # :long_term_followup
      @sms.save!
      @sms.send!
    end
  end

  def message(kwargs)
    Envelope.new kwargs.reverse_merge(
      body: render
    )
  end

  def render
    # finds the appropriate view file w/ application renderer
  end
end

# -----

class UserTexter < ApplicationMailer
  def long_term_followup(texting)
    @user = texting.subscriber.decorate

    message(
      external_number: @user.mobile,
      internal_number: Rails.config.our_phone_number,
      recipient: @user
    )
  end
  # view at: app/views/user_texter/long_term_followup.text.erb
end

class AgentTexter < ApplicationMailer
  def getting_started_followup(texting)
    @agent = texting.subscriber.decorate

    message(
      external_number: @agent.mobile,
      internal_number: Rails.config.our_phone_number,
      recipient: @agent
    )
  end
  # view at: app/views/agent_texter/getting_started_followup.text.erb
end

[jon-sully]

So I think only a couple of things stand out to me as remaining for this PR / feature, but overall I like it!

• For parity with ActionMailer I think we want to use #deliver not #deliver! (with the exclamation mark)
• We should make sure that this will work for actions that inherit from Caffeinate::Action but are invoked outside of the Caffeinate context. E.g. if I just plainly call UserTexter.welcome_message.deliver, that ought to work. The Action callbacks for the pre-hooks, do_delivery, and after-hooks should be able to know that the Caffeinate stuff is nil and that should be fine (perform_deliveries defaults to true?)
• I think we should bolster the docs a little more with this one. It's a bit confusing (to me) to walk through a scenario where we want to convert a was-ActionMailer to a now-Custom-Action; I think we might just want an example where we set up a custom action from scratch (Texters or WebPushers are a good example). I think that may be less confusing that converting what was already an ActionMailer action

Oh, last, and just a thought here; I wonder if we might convert the mailer:, mailing_classs:, action_class: keys for drip to a unified performer: key — what do you think about the verbiage of "Performer"?

[joshmn]

For parity with ActionMailer I think we want to use #deliver not #deliver! (with the exclamation mark)

They delegate to the Mail::Message (action_mailer/message_delivery.rb). I added the bang though. These won't work with async delivery as it sits. Need to document that and improve upon it.

We should make sure that this will work for actions that inherit from Caffeinate::Action but are invoked outside of the Caffeinate context.

I'd need to see a use case for that to agree; how would that work when you're not passing it a mailing? They're expected to take a mailing object and a mailing object only. Kind of funky thing to check side the method definition, no?

I think we should bolster the docs a little more with this one.

Same.

RE: converting names — v3.0 :)

[joshmn]

If you tell yourself a Caffeinate::Action is the equivalent to ActionMailer::Base, and the subsequent Envelope is a Mail::Message, I think it might be more palatable. :)

Ah! 💡 There is is. Yep, bringing in Envelope made it much more clear and yeah, this does actually. help to get more in line with ActionMailer's "mailer vs. Mail object" paradigm.

Probably would still need a top-level Texter and subclasses (the same way that we typically have ApplicationMailer < ActionMailer::Base and UserMailer < ApplicationMailer) since those subclasses are typically subclassed per-recipient (e.g. we have Users and Agents, so we'd want a UserTexter and AgentTexter) and the Envelope definition would probably live up in the root Texter, but I think that's actually fine.

Thanks for circling back on that example! Since examples make the whole thing more visible, here's how I'd envision this whole thing working from my eyes (still the same Caffeinate::Action code):

class ApplicationTexter < Caffeinate::Action
  class Envelope
    def initialize(kwargs)
      @sms = Sms.new(kwargs.reverse_merge(
        sender: Rails.robot,
        direction: :outbound
      ))
    end

    def deliver!(action)
      @action.action_name # :long_term_followup
      @sms.save!
      @sms.send!
    end
  end

  def message(kwargs)
    Envelope.new kwargs.reverse_merge(
      body: render
    )
  end

  def render
    # finds the appropriate view file w/ application renderer
  end
end

# -----

class UserTexter < ApplicationMailer
  def long_term_followup(texting)
    @user = texting.subscriber.decorate

    message(
      external_number: @user.mobile,
      internal_number: Rails.config.our_phone_number,
      recipient: @user
    )
  end
  # view at: app/views/user_texter/long_term_followup.text.erb
end

class AgentTexter < ApplicationMailer
  def getting_started_followup(texting)
    @agent = texting.subscriber.decorate

    message(
      external_number: @agent.mobile,
      internal_number: Rails.config.our_phone_number,
      recipient: @agent
    )
  end
  # view at: app/views/agent_texter/getting_started_followup.text.erb
end

I think you're close with this example. I think the Envelope's #deliver! should handle the render body. Something like:

class ApplicationTexter < Caffeinate::Action
  class Envelope
    def initialize(kwargs)
      @sms = Sms.new(kwargs.reverse_merge(
        sender: Rails.robot,
        direction: :outbound
      ))
    end

    def deliver!(action)
      @sms.body = render(action)
      @sms.save!
      @sms.send!
    end
  
    private
    def render(action)
      # action.action_name 
    end 
  end
end

[jon-sully]

I added the bang though

We could probably support both #deliver and #deliver! Just checking for either/or and calling as such. I suppose just as well, Envelope could also just alias too.

I think you're close with this example. I think the Envelope's #deliver! should handle the render body. Something like:

I agree! I realized that after finishing my last comment since action_name being available makes the hard part of rendering much easier (knowing which template to render!).

I'd need to see a use case for that to agree; how would that work when you're not passing it a mailing? They're expected to take a mailing object and a mailing object only. Kind of funky thing to check side the method definition, no?

Sure; I have a real example for you. We have a UserTexter that is responsible for firing off texts from a few different contexts:

• When called directly somewhere in the app, passing params directly (such as a 2FA code)
• When called by Caffeinate for running a drip callback
• When sending a Notification message via Noticed

In each of these cases the method in question receives slightly different arguments, but that's okay:

class UserTexter < ApplicationMailer
  # From a Caffeinate drip
  def long_term_followup(texting)
    @user = texting.subscriber.decorate

    message(
      external_number: @user.mobile,
      internal_number: Rails.config.our_phone_number,
      recipient: @user
    )
  end

  # Called directly somewhere in the app:
  # UserTexter.two_factor_code(@user).deliver
  def two_factor_code(user)
    @code = user.two_factor_code
    
    message(external_number: user.mobile, internal_number: "123")
  end
  
  # Invoked by Noticed
  # (Which calls under the hood: UserTexter.new_comment_notification(comment: SomeComment).deliver)
  def new_comment_notification(params)
    @comment = params[:comment]
    
    message(external_number: user.mobile, internal_number: "123", recipient: user)
  end
end

So that's 3 different ways that Texter (or mailer) methods can work slightly differently for different context. They do receive different arguments, but I'm just saying that we'll want to make sure that direct-invocation (like two_factor_code) doesn't blow up when you try to run UserTexter.two_factor_code(@user).deliver because the Caffeinate middleware is trying to run pre- or post-hooks looking for a mailing that doesn't exist

[joshmn]

Sure; I have a real example for you. We have a UserTexter that is responsible for firing off texts from a few different contexts:

The design here makes it so that we don't care about the underlying return of the object (from the Action) since it's processed independently. :) Normally, Caffeinate looks at the returning Mail::Message object and checks for a caffeinate_mailing attr. Here, we achieve the same since we're going through a Caffeinate::Action and handling the funky method_missing which allows us to intercept and instantiate something that we do care about (and apply the necessary mailing).

[joshmn]

Just to recap:

I think we're good on this and its design? I'm going to spend today rewriting documentation for everything.