DEV: adds support for bannered until (PR #13417)

ATM it only implements server side of it, as my need is for automation purposes. However it should probably be added in the UI too as it’s unexpected to have pinned_until and no bannered_until.

GitHub

Should we at least check that bannered_until is in the future?

I dont know we dont do it for pinned ubtil, I guess it would be enqueued instantly, no big deal.

Could also be a two-liner.

      raise Discourse::InvalidParameters.new(:topic_id) unless args[:topic_id].present?
      Topic.find_by(id: args[:topic_id])&.remove_banner!(Discourse.system_user)

Yes so far I ve tried to mimmic the codepath of pinned until, but yes I can simplify this.

Are there plans to call this method in core? Otherwise, I don’t think this code should live in core at all and should belong in the plugin that requires it.

We need to add an ensure consistency job like how we did for pinned_until. Sidekiq can be wiped and jobs can fail so we need some way to recovering here.

1 Like

I think it should that’s also the goal of this PR

Are there plans to call this method in core? Otherwise, I don’t think this code should live in core at all and should belong in the plugin that requires it.

I created a RFC so we can discuss this

Yes I know about the risk but didn’t know we had this for pinned, cool :+1: will add it

I will need to add a bannered_until field on topics table

I think we need a partial index here. Otherwise, the ensure consistency query is going to be very slow since we have to query against the entire topics table.

@SamSaffron Are we still OK with this pattern or do you prefer having a recurring job that runs regularly like how we do for bookmarks?

I don’t think we should be testing the Jobs::RemoveBanner job in this spec here. I would create a spec file specifically to test the job for better organisation.

to me both pinned until and bannered until are fancy topic timers. I would be delighted if these two edge cases just fit into the topic timer system instead of living in a special codepath. … @martin-brennan any thoughts here.

Added a few items and added to the discussion around topic timers

For the pinning functionality we cancel the existing job first before scheduling a new one, I think we should do that here too:

Jobs.cancel_scheduled_job(:remove_banner, topic_id: self.id)

I agree with this…in theory. However topic timers still suffer from the fact that you can only have one at a time, our backend and our UI is built around this. For example we move bannered_until into a topic timer and then you want an auto-close or a temporarily close on the same topic. You can’t do it. I think we need to do this in future work, an overhaul of the timer system to allow for multiple timers.

Until then this functionality, while not perfect, is probably the best way to go (copying how pinned until works).

We don’t have a partial index on pinned_until either

Agree, we should always test jobs with an independent spec

Is there any point in raising an error here? It will just make errors in the logs we can do nothing about. Better to just do an early return.