joshmn / caffeinate

A Rails engine for drip campaigns/scheduled sequences and periodical support. Works with ActionMailer, and other things.
https://caffeinate.email
MIT License
345 stars 13 forks source link

CampaignSubscription `#end!` when already ended #35

Closed jon-sully closed 1 year ago

jon-sully commented 1 year ago

Ran into a little infinite loop when calling cs.end! from inside an on_complete block:

  on_complete do |campaign_subscription|
    # Marking ended instead of just letting it be complete so subscriber
    # can be subscribed again in the future
    campaign_subscription.end!
  end

Since calling .end! calls .update! inside of it, which will then trigger the after_commit and thus back into the on_complete block ^ and the cycle executes again ad infinitum!

I figured this was a graceful way to avoid the after_commit callback if the thing is already ended! WDYT?

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (30bf0ca) 98.64% compared to head (5a4c030) 98.64%.

:exclamation: Current head 5a4c030 differs from pull request most recent head b501bb9. Consider uploading reports for the commit b501bb9 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #35 +/- ## ======================================= Coverage 98.64% 98.64% ======================================= Files 98 98 Lines 2502 2511 +9 ======================================= + Hits 2468 2477 +9 Misses 34 34 ``` | [Impacted Files](https://codecov.io/gh/joshmn/caffeinate/pull/35?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Ξ” | | |---|---|---| | [app/models/caffeinate/campaign\_subscription.rb](https://codecov.io/gh/joshmn/caffeinate/pull/35?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%> (ΓΈ)` | | | [...ec/models/caffeinate/campaign\_subscription\_spec.rb](https://codecov.io/gh/joshmn/caffeinate/pull/35?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.

joshmn commented 1 year ago

speccccssss; also could check ended?

maybe we should update_columns to avoid the callbacks.

wdyt

jon-sully commented 1 year ago

πŸ˜… sorry for the specs lacking! Felt like this one was tiny!

update_columns could work but I guess I'd ask the question "if someone ends a campaign and then calls end! again later, would they want the latest ended_at date? Or the date it was first (and really) 'ended'?" I'd probably guess that subsequent calls to .end! on an already-ended subscription should be no-ops more than re-set the ended_at πŸ€”

Checking ended? is better than just doing !!ended_at β€” good call πŸ‘

EDIT: I also write these micro-PRs directly from GH so I'll write specs hoping they pass in CI πŸ˜†