Separate drip types #26
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #26 +/- ##
==========================================
- Coverage 98.64% 98.58% -0.06%
==========================================
Files 98 101 +3
Lines 2502 2612 +110
==========================================
+ Hits 2468 2575 +107
- Misses 34 37 +3
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. |
| @@ -97,7 +97,7 @@ def user | |||
| # Assigns attributes to the Mailing from the Drip | |||
| def from_drip(drip) | |||
| self.send_at = drip.send_at(self) | |||
| self.mailer_class = drip.options[:mailer_class] | |||
| self.mailer_class = drip.options[:mailer_class] || drip.options[:action_class] | |||
There was a problem hiding this comment.
hate this. move to just assuming action_class
There was a problem hiding this comment.
Might be too generic here, but what about just deliverer?
There was a problem hiding this comment.
same idea. just need to get away from mailer_class in v3
| @@ -97,7 +97,7 @@ def user | |||
| # Assigns attributes to the Mailing from the Drip | |||
| def from_drip(drip) | |||
| self.send_at = drip.send_at(self) | |||
| self.mailer_class = drip.options[:mailer_class] | |||
| self.mailer_class = drip.options[:mailer_class] || drip.options[:action_class] | |||
There was a problem hiding this comment.
Might be too generic here, but what about just deliverer?
| # @option options [Symbol|Proc|ActiveSupport::Duration] :every How often the mailing should be created | ||
| # @option options [Symbol|Proc] :if If the periodical should create another mailing | ||
| # @option options [Symbol|Proc] :start The offset time to start the clock (only used on the first mailing creation) |
There was a problem hiding this comment.
I like this API 👍
:everya static value or proc that yields an amount of time that ought to be used for the 'next' message in the sequence- I wonder if this should also support using a specific time or date rather than an amount of time — we can probably just call
.respond_to? :from_nowto see if it's a specific date or just anActiveSupport::Duration
- I wonder if this should also support using a specific time or date rather than an amount of time — we can probably just call
:ifa proc that, if yields false, ends the periodical altogether — probably should run before the actual send so that it doesn't send the message at hand if false:starta static value or proc that yields an amount of time that will be added to 'right now' to set the firing date for the first message only.- Same note here to consider adding support for specific times/dates as well as durations
I wonder if we should add the until logic to this PR?
:untila proc that, if yields true, ends the periodical altogether (probably also before_drip)- Thinking that true == end because the usage will probably be like
until: -> { subscriber.is_opted_out? }oruntil: -> { Date.current.year > 2023 }
- Thinking that true == end because the usage will probably be like
There was a problem hiding this comment.
isn't until the same as if?
There was a problem hiding this comment.
re: deliver_class — yeah, something like that.
There was a problem hiding this comment.
isn't until the same as if?
Ah, I suppose it is, just the ! of one another. The implementation under the hood here could be as simple as that too, but I think it'd be worth exposing both for the consumer; depending on their method names, one will read better than the other.
if :user_still_wants_emails
unless :user_opted_out_of_emails
lib/caffeinate/dripper/periodical.rb
Outdated
| if (mailing.drip.action == action_name) && (thing = mailing.drip.options[:if]) | ||
| if OptionEvaluator.new(thing, mailing.drip, mailing).call | ||
| if mailing.drip.action == action_name | ||
| next_mailing = mailing.dup | ||
| next_mailing.send_at = mailing.drip.send_at(mailing) | ||
| next_mailing.save! | ||
| end | ||
| end | ||
| else | ||
| if mailing.drip.action == action_name | ||
| next_mailing = mailing.dup | ||
| next_mailing.send_at = mailing.drip.send_at(mailing) | ||
| next_mailing.save! | ||
| end |
There was a problem hiding this comment.
| if (mailing.drip.action == action_name) && (thing = mailing.drip.options[:if]) | |
| if OptionEvaluator.new(thing, mailing.drip, mailing).call | |
| if mailing.drip.action == action_name | |
| next_mailing = mailing.dup | |
| next_mailing.send_at = mailing.drip.send_at(mailing) | |
| next_mailing.save! | |
| end | |
| end | |
| else | |
| if mailing.drip.action == action_name | |
| next_mailing = mailing.dup | |
| next_mailing.send_at = mailing.drip.send_at(mailing) | |
| next_mailing.save! | |
| end | |
| if (mailing.drip.action == action_name) | |
| return unless (thing = mailing.drip.options[:if]) && OptionEvaluator.new(thing, mailing.drip, mailing).call | |
| next_mailing = mailing.dup | |
| next_mailing.send_at = mailing.drip.send_at(mailing) | |
| next_mailing.save! | |
| end |
There was a problem hiding this comment.
if isn't required and we still want to queue things up, no?
There was a problem hiding this comment.
Ah sorry, yeah, that
return unless (thing = mailing.drip.options[:if]) && OptionEvaluator.new(thing, mailing.drip, mailing).callshould be
return if (thing = mailing.drip.options[:if]) && !OptionEvaluator.new(thing, mailing.drip, mailing).callThere was a problem hiding this comment.
Ah; I always forget about that. next works for those situations, but having the more verbose code is also fine 😛 either way!
There was a problem hiding this comment.
Ah; I always forget about that. next works for those situations, but having the more verbose code is also fine 😛 either way!
PeriodicalDrip needs some love — I don't have the bandwidth right now to document it fully (I also don't use it!) but tests should pass. Feel free to pick up where I left off.