Expand Periodical API

[jon-sully]

periodical currently supports every: and start: when it comes to parameters that control its time sequences. I think of these as

# :every object
periodical :remind_users, every: 5.minutes
# :every proc
periodical :drip_our_customers, every: -> { 2.months + rand(-1.day..1.day) }

# :start proc
periodical :check_in_with_user, start: -> { 2.days.from_now }

But sometimes these aren't the most ergonomic. Particularly in that both want a relative amount of time, not an absolute timestamp (which makes sense given their verbiage, 'start' and 'every'). But often times in models and systems it's easier to get to absolute timestamps.

In addition, I think it'd be a nice ergonomics win to add :while as a before-any-time-computations check as to whether or not the next periodical should even fire. I suppose this is no different than a before_drip check that .end!'s the subscription, but I feel like the ergonomics of having a :while parameter that essentially only continues the sequence if it's given a proc that resolves to truthy is a good representation of most systems. I can imagine a lot of folks will use something like

periodical :win_back_attempt, while: -> { subscriber.active? }, every: -> { 4.weeks }

Sure, something like this is essentially equivalent:

before_drip do |_, mailing|
  if !subscriber.active?
    mailing.subscription.end!
    mailing.skip!
    throw(:abort)
  end
end

But the ergonomics and expressiveness of the :while key directly on the periodical feels a lot cleaner... and I suppose it does allow you to have different :while conditions for different periodicals within a single dripper. The before_drip approach would have to differentiate between multiple different periodicals manually in its logic (which would be icky!)

In terms of the time APIs, I think we could add a :next_at keyword that expects to resolve to an absolute time:

periodical :check_in_with_user, while: -> { subscriber.needs_check_in? }, next_at: -> { subscriber.next_morning_delivery_slot }

---

Now, all of that said, I just wanted to get my thoughts on paper here. I think we may already support some of this functionality and I've just missed it along the way. I'm going to begin this process by really looking over the options that are supported already with periodical and see if / how much would need to change to accomplish the above ideas 👍

[jon-sully]

And, just worth referencing, I wrote this PR some time ago:

https://github.com/joshmn/caffeinate/pull/25

Which was based on this comment — explaining that :every supporting a Proc would be nice, and that the :start has a strange behavior where its offset is actually evaluated / added for every run of a periodical. Which is true, and I still consider to be a bug? I have several periodicals in production that have weird start: blocks that look like:

    start: -> do
      if send_at.nil? # this is the first message in the sequence
        (((subscriber.next_textable_time_after(Time.current) - Time.current)) / 1.minute).minutes # next afternoon delivery
      else
        0.minutes # don't offset on other messages; :every does that
      end
    end

... they're split because they have to figure out if they're running for the first mailing vs. all subsequent mailings. This just illustrating the issue that :start is being logically applied to all mailings, not just the first one (which I believe was the intention).

Second, and separate from the concept of "start should only be on the first message", we can see the hooks one must jump through in order to return a relative time rather than an absolute time when my helpers are more geared toward absolute times. That said, I've now realized that I can return absolute times in my :start proc... as of #26.

There were some subtle changes in https://github.com/joshmn/caffeinate/pull/26 that changed some expectations. I'd previously written my start: proc the way I did above because I knew that whatever was returned from that proc would have .from_now called on it — so it had to be a relative date. That logic changed here in #26 such that start: can resolve to an absolute time and it'll work (but it's backwards compatible and will call .from_now if the value given responds to :from_now... which is why my current dripper code still works on 2.5 👍)

So... let me revise the above to a simpler version:

    start: -> do
      if send_at.nil? # this is the first message in the sequence
        subscriber.next_textable_time_after Time.current
      else
        Time.current # don't offset on other messages; :every does that
      end
    end

This recognizes that :start can now return an absolute time (nice!) but also still has to include the "is this the first message?" logic because, although we say that the :start is only used for the first message in the sequence, the logic for computing the next-message's send_at always checks/uses :start in that math:

    def call
      if periodical?
        start = Caffeinate.config.now.call
        if options[:start]                             # <== Doesn't check to see if this is the first message
          start = OptionEvaluator.new(options[:start], self, mailing).call  # <== so it always overrides `start`
        end
        start += OptionEvaluator.new(options[:every], self, mailing).call if mailing.caffeinate_campaign_subscription.caffeinate_mailings.size.positive?
        date = start
      elsif options[:on]
        # ...
      else
        # ...
      end

      if date.respond_to?(:from_now)
        date = date.from_now
      end

      # ...

      date
    end

(Explanation of above) even on subsequent messages, the if options[:start] will always be true (the :start key is still in the source code) so the local variable start will always be overridden to the evaluation of that start key

So wrapping this tangent up, :start still isn't first-message aware. That's an issue.

BUT, now that :start is absolute-time-capable, I wonder if we could sort of just hack it to be an absolute-time version of :every? :every is a required key, but if we make its offset zero minutes and use a proc that resolves to an absolute time with :start, we technically now have a periodical that sets the next message in the sequence to an absolute time the way I wanted:

periodical :unresponsive_perpetual_email, every: 0.minutes, start: -> { subscriber.next_textable_time_after Time.current }

Since :start runs on all messages and it's a proc that returns an absolute time, that'll set that local date variable in the code snippet above, the :every offset of 0.minutes will get added to it, and that'll become the new message send_at. It's kind of gross and definitely not how :start and :every were meant to work (it would be better if there was a dedicated keyword for this, like the :next_at I initially proposed...), but this does actually get the job done. ✅

I also realized while digging here that we did add a boolean gate to the periodical, I just forgot about it! It's not while, it's if. Also in #26, but not yet documented, the if will essentially act the same as while would:

periodical :unresponsive_perpetual_email, every: 1.hour, if: -> { subscriber.still_hasnt_paid? }

...With one careful caveat. if won't prevent an already-scheduled mailing from going out — we need to use a before_drip halt for that. if only determines if a follow-up mailing should be created, right after one has just been sent. So I guess that makes my example here ^ not great. Something like "only send the email if they haven't paid yet" is logic better suited for a before_drip halt. Maybe this is a better example:

periodical :unresponsive_perpetual_email, every: 1.hour, if: -> { subscriber.nag_count < 10 }

I haven't played with it enough to have opinions yet but it feels like, since if isn't for cancelling a currently-sending mailing, if is better suited for logical gates that pertain to the lifecycle / lifetime of the subscription, not necessarily individual mailings (which we'd use before_drip gates for). Idk. I'll feel that out over time. Nonetheless, this satisfies the "let's add while:" idea from OP here too ✅

So.... yeah actually I think I'm good and maybe don't need to expand the time APIs for periodical? Probably will if we fix :start to actually only run on the first message and/or if we change logic around if (not sure we should though), but for now I think this works...

[jon-sully]

So, while I think I'm good for now, the simplest (and likely cleanest / best) path forward is probably to make :start only actually do anything on the first mailing (as intended) then actually open up :on to work on periodicals. That would change the API to "use :every for relative time splits between messages (even if they're dynamic) and use :on for absolute time splits between messages (even if they're dynamic)" which is nice 😎

[jon-sully]

Ah, one note for a future PR — if someone is using a periodical with the if: key and the if condition goes false, should we should probably call .end! on the subscription? I'm mostly thinking we should make sure the on_complete callbacks run. Maybe we just call those directly instead of ending the subscription (since that has implications about future .subscribe calls!) 🤔