DEV: Change Topic Timer from enqueue_at scheduled jobs to incrementally executed jobs (PR #11698)

Moves the topic timer jobs from being scheduled ahead of time with enqueue_at to a 5 minute scheduled run like bookmark reminders.

GitHub

The title of this pull request changed from “DEV: Topic timer enqueuer” to "DEV: Change Topic Timer from enqueue_at scheduled jobs to incrementally executed jobs

I can’t remember but do we still allow topic timers to specify a “task” by the minute? If so, 2 minutes might be too wide.

      timers.find_each do |timer|

Just to be safer on larger sites.

topic&.closed? Will topic ever be nil? We clean up topic timers whenever a topic is destroyed.

I’m not sure if replacing :close_topic with TopicTimer.type_job_map[:close] is better here since it makes the code longer and adds a layer of symbol over another symbol.

  fab!(:deleted_timer) do
    Fabricate(:topic_timer, execute_at: 1.minute.ago, created_at: 1.hour.ago, status_type: TopicTimer.types[:close]).tap(&:trash!)
  end

I think this will work?

Instead of mocking here, are we able to test the expected outcome of the subject.execute method call instead? In this case, I think it just means a Sidekiq job has not been scheduled?

      end

      it "does not return timers in the future of the provided before time" do
        topic_timer.update!(execute_at: 3.days.from_now)
        topic_timer.update!(execute_at: 1.minute.ago, created_at: 10.minutes.ago)
      it "returns false if the job is not scheduled" do

minor suggestion

Yeah you can…I guess there won’t be too many getting sent off each minute. FWIW you can set bookmark reminders to the minute too but the job runs every 5 mins, I don’t think granularity is needed there.

I guess not, this was just legacy code

Does not work, it cannot find the timer inside the it block later. IMO the current way is clearer anyway

@tgxworld review changes made except for one, thanks for the review!

The new changes are duplicated across the jobs. I think we can create a parent job and have ‘TopicTimer’ jobs subclass the parent class.

    def execute(_)

I think we need to change the use of TopicTimer.type_job_map across the other methods as well :slight_smile: