From eb38e9df3129c2bc5ec3ecd1656336bf813747ce Mon Sep 17 00:00:00 2001 From: Christian Schmidt Date: Wed, 29 Mar 2023 10:52:40 +0200 Subject: [PATCH] Requeue expiration notification (#24311) --- app/services/update_status_service.rb | 4 +- app/workers/poll_expiration_notify_worker.rb | 2 +- spec/services/update_status_service_spec.rb | 9 ++- .../poll_expiration_notify_worker_spec.rb | 61 ++++++++++++++++++- 4 files changed, 71 insertions(+), 5 deletions(-) diff --git a/app/services/update_status_service.rb b/app/services/update_status_service.rb index f75fdf55d..d1c2b990f 100644 --- a/app/services/update_status_service.rb +++ b/app/services/update_status_service.rb @@ -141,9 +141,9 @@ class UpdateStatusService < BaseService poll = @status.preloadable_poll # If the poll had no expiration date set but now has, or now has a sooner - # expiration date, and people have voted, schedule a notification + # expiration date, schedule a notification - return unless poll.present? && poll.expires_at.present? && poll.votes.exists? + return unless poll.present? && poll.expires_at.present? PollExpirationNotifyWorker.remove_from_scheduled(poll.id) if @previous_expires_at.present? && @previous_expires_at > poll.expires_at PollExpirationNotifyWorker.perform_at(poll.expires_at + 5.minutes, poll.id) diff --git a/app/workers/poll_expiration_notify_worker.rb b/app/workers/poll_expiration_notify_worker.rb index 0e29a5f60..b7a60fab8 100644 --- a/app/workers/poll_expiration_notify_worker.rb +++ b/app/workers/poll_expiration_notify_worker.rb @@ -3,7 +3,7 @@ class PollExpirationNotifyWorker include Sidekiq::Worker - sidekiq_options lock: :until_executed + sidekiq_options lock: :until_executing def perform(poll_id) @poll = Poll.find(poll_id) diff --git a/spec/services/update_status_service_spec.rb b/spec/services/update_status_service_spec.rb index e52a0e52b..d6923722a 100644 --- a/spec/services/update_status_service_spec.rb +++ b/spec/services/update_status_service_spec.rb @@ -120,7 +120,9 @@ RSpec.describe UpdateStatusService, type: :service do before do status.update(poll: poll) VoteService.new.call(voter, poll, [0]) - subject.call(status, status.account_id, text: 'Foo', poll: { options: %w(Bar Baz Foo), expires_in: 5.days.to_i }) + Sidekiq::Testing.fake! do + subject.call(status, status.account_id, text: 'Foo', poll: { options: %w(Bar Baz Foo), expires_in: 5.days.to_i }) + end end it 'updates poll' do @@ -138,6 +140,11 @@ RSpec.describe UpdateStatusService, type: :service do it 'saves edit history' do expect(status.edits.pluck(:poll_options)).to eq [%w(Foo Bar), %w(Bar Baz Foo)] end + + it 'requeues expiration notification' do + poll = status.poll.reload + expect(PollExpirationNotifyWorker).to have_enqueued_sidekiq_job(poll.id).at(poll.expires_at + 5.minutes) + end end context 'when mentions in text change' do diff --git a/spec/workers/poll_expiration_notify_worker_spec.rb b/spec/workers/poll_expiration_notify_worker_spec.rb index 8229db815..78cbc1ee4 100644 --- a/spec/workers/poll_expiration_notify_worker_spec.rb +++ b/spec/workers/poll_expiration_notify_worker_spec.rb @@ -4,10 +4,69 @@ require 'rails_helper' describe PollExpirationNotifyWorker do let(:worker) { described_class.new } + let(:account) { Fabricate(:account, domain: remote? ? 'example.com' : nil) } + let(:status) { Fabricate(:status, account: account) } + let(:poll) { Fabricate(:poll, status: status, account: account) } + let(:remote?) { false } + let(:poll_vote) { Fabricate(:poll_vote, poll: poll) } + + describe '#perform' do + around do |example| + Sidekiq::Testing.fake! do + example.run + end + end - describe 'perform' do it 'runs without error for missing record' do expect { worker.perform(nil) }.to_not raise_error end + + context 'when poll is not expired' do + it 'requeues job' do + worker.perform(poll.id) + expect(described_class.sidekiq_options_hash['lock']).to be :until_executing + expect(described_class).to have_enqueued_sidekiq_job(poll.id).at(poll.expires_at + 5.minutes) + end + end + + context 'when poll is expired' do + before do + poll_vote + + travel_to poll.expires_at + 5.minutes + + worker.perform(poll.id) + end + + context 'when poll is local' do + it 'notifies voters' do + expect(ActivityPub::DistributePollUpdateWorker).to have_enqueued_sidekiq_job(poll.status.id) + end + + it 'notifies owner' do + expect(LocalNotificationWorker).to have_enqueued_sidekiq_job(poll.account.id, poll.id, 'Poll', 'poll') + end + + it 'notifies local voters' do + expect(LocalNotificationWorker).to have_enqueued_sidekiq_job(poll_vote.account.id, poll.id, 'Poll', 'poll') + end + end + + context 'when poll is remote' do + let(:remote?) { true } + + it 'does not notify remote voters' do + expect(ActivityPub::DistributePollUpdateWorker).to_not have_enqueued_sidekiq_job(poll.status.id) + end + + it 'does not notify owner' do + expect(LocalNotificationWorker).to_not have_enqueued_sidekiq_job(poll.account.id, poll.id, 'Poll', 'poll') + end + + it 'notifies local voters' do + expect(LocalNotificationWorker).to have_enqueued_sidekiq_job(poll_vote.account.id, poll.id, 'Poll', 'poll') + end + end + end end end