CampaignSubscription #end! when already ended

[jon-sully]

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]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

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

❗ 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

@@           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	Coverage Δ
app/models/caffeinate/campaign_subscription.rb	100.00% <100.00%> (ø)
...ec/models/caffeinate/campaign_subscription_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.

[joshmn]

speccccssss; also could check ended?

maybe we should update_columns to avoid the callbacks.

wdyt

[jon-sully]

😅 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 😆