Closed joshmn closed 1 year ago
Patch coverage: 94.80
% and project coverage change: -0.26
:warning:
Comparison is base (
9b6efaa
) 98.88% compared to head (abff332
) 98.62%.:exclamation: Current head abff332 differs from pull request most recent head 0c08b9a. Consider uploading reports for the commit 0c08b9a to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
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!
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.
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
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?
@jon-sully Ahhh I see your thinking now! Pushing up a change that reflects that use-case. Watch this.
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.
🤔 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:
new
instance#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 messageCaffeinate::ActionMailer::Interceptor.delivering_email self
at the beginning of the #deliver
method@caffeinate_mailing
after the delivery work is doneAnd 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 Texter
s 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
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. :)
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
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 :)
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
andUserMailer < ApplicationMailer
) since those subclasses are typically subclassed per-recipient (e.g. we haveUsers
andAgents
, so we'd want aUserTexter
andAgentTexter
) 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
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:
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
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).
Just to recap:
I think we're good on this and its design? I'm going to spend today rewriting documentation for everything.
This will allow for PORO support, in addition to integrating with ActionMailer.
In the future, we should rename
mailings
tomessages
and change the inference that Caffeinate only handles ActionMailer-based stuff to any-based stuff.Resolves https://github.com/joshmn/caffeinate/issues/14
cc @jon-sully