From 58a5065e5253ed969f88cc68a18a39bb5676e955 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Mon, 27 Dec 2021 17:44:42 +0100 Subject: [PATCH 1/7] Catch common `ShareWorker` exceptions to avoid unnecessary retries --- app/workers/share_worker.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/app/workers/share_worker.rb b/app/workers/share_worker.rb index 6d6067cf..be70d022 100644 --- a/app/workers/share_worker.rb +++ b/app/workers/share_worker.rb @@ -11,6 +11,15 @@ class ShareWorker user_service = User.find(user_id).services.find_by(type: service_type) user_service.post(Answer.find(answer_id)) + rescue ActiveRecord::RecordNotFound + # The question to be posted was deleted + return + rescue Twitter::Error::DuplicateStatus + return + rescue Twitter::Error::Unauthorized + # User's Twitter token has expired or been revoked + # TODO: Notify user if this happens (https://github.com/Retrospring/retrospring/issues/123) + return rescue => e logger.info "failed to post answer #{answer_id} to #{service} for user #{user_id}: #{e.message}" NewRelic::Agent.notice_error(e) From 9c599db3a782d3b19c2a052c276429687f41f678 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Mon, 27 Dec 2021 18:17:47 +0100 Subject: [PATCH 2/7] Log message if `ShareWorker` tries to post something deleted --- app/workers/share_worker.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/workers/share_worker.rb b/app/workers/share_worker.rb index be70d022..9b552cd0 100644 --- a/app/workers/share_worker.rb +++ b/app/workers/share_worker.rb @@ -12,6 +12,7 @@ class ShareWorker user_service.post(Answer.find(answer_id)) rescue ActiveRecord::RecordNotFound + logger.info "Tried to post answer ##{answer_id} for user ##{user_id} to #{service.titleize} but the user/answer/service did not exist (likely deleted), will not retry." # The question to be posted was deleted return rescue Twitter::Error::DuplicateStatus From 0b926a43e4355909687e67846d43a4eb8c7161dc Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Mon, 27 Dec 2021 18:53:59 +0100 Subject: [PATCH 3/7] Test `ShareWorker`'s handling of deleted records --- spec/workers/share_worker_spec.rb | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 spec/workers/share_worker_spec.rb diff --git a/spec/workers/share_worker_spec.rb b/spec/workers/share_worker_spec.rb new file mode 100644 index 00000000..5c5c3a3f --- /dev/null +++ b/spec/workers/share_worker_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe ShareWorker do + let(:user) { FactoryBot.create(:user) } + let(:answer) { FactoryBot.create(:answer, user: user) } + + describe "#perform" do + subject { ShareWorker.new.perform(user.id, answer.id, 'twitter') } + + before do + Service.create!(type: 'Services::Twitter', + user: user) + end + + context 'when answer doesn\'t exist' do + it 'prevents the job from retrying and logs a message' do + answer.destroy! + Sidekiq.logger.should_receive(:info) + subject + expect(ShareWorker.jobs.size).to eq(0) + end + end + end +end From d73e269d85e26a2b750406cd8b2384ee24ae5d72 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Mon, 27 Dec 2021 23:02:32 +0100 Subject: [PATCH 4/7] Test `ShareWorker`'s handling of unhandled exceptions --- spec/workers/share_worker_spec.rb | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/spec/workers/share_worker_spec.rb b/spec/workers/share_worker_spec.rb index 5c5c3a3f..988d5300 100644 --- a/spec/workers/share_worker_spec.rb +++ b/spec/workers/share_worker_spec.rb @@ -5,21 +5,29 @@ require 'rails_helper' describe ShareWorker do let(:user) { FactoryBot.create(:user) } let(:answer) { FactoryBot.create(:answer, user: user) } + let!(:service) { Services::Twitter.create!(type: 'Services::Twitter', + user: user) } describe "#perform" do - subject { ShareWorker.new.perform(user.id, answer.id, 'twitter') } - - before do - Service.create!(type: 'Services::Twitter', - user: user) - end + subject { + Sidekiq::Testing.fake! do + ShareWorker.perform_async(user.id, answer.id, 'twitter') + end + } context 'when answer doesn\'t exist' do it 'prevents the job from retrying and logs a message' do answer.destroy! Sidekiq.logger.should_receive(:info) - subject - expect(ShareWorker.jobs.size).to eq(0) + expect { subject }.to change(ShareWorker.jobs, :size).by(1) + expect { ShareWorker.drain }.to change(ShareWorker.jobs, :size).by(-1) + end + end + + context 'when answer exists' do + it 'retries on unhandled exceptions' do + expect { subject }.to change(ShareWorker.jobs, :size).by(1) + expect { ShareWorker.drain }.to raise_error(Twitter::Error::Forbidden) end end end From 9dafa675d126762335098734e14f37b8a0187829 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Mon, 27 Dec 2021 23:03:51 +0100 Subject: [PATCH 5/7] Log on `ShareWorker` Twitter exceptions --- app/workers/share_worker.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/workers/share_worker.rb b/app/workers/share_worker.rb index 9b552cd0..af83aa86 100644 --- a/app/workers/share_worker.rb +++ b/app/workers/share_worker.rb @@ -16,10 +16,12 @@ class ShareWorker # The question to be posted was deleted return rescue Twitter::Error::DuplicateStatus + logger.info "Tried to post answer ##{answer_id} from user ##{user_id} to Twitter but the status was already posted." return rescue Twitter::Error::Unauthorized # User's Twitter token has expired or been revoked # TODO: Notify user if this happens (https://github.com/Retrospring/retrospring/issues/123) + logger.info "Tried to post answer ##{answer_id} from user ##{user_id} to Twitter but the token has exired or been revoked." return rescue => e logger.info "failed to post answer #{answer_id} to #{service} for user #{user_id}: #{e.message}" From 45dae78ed01a8e3bed2dc67246900128f27c02a7 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Tue, 28 Dec 2021 03:03:59 +0100 Subject: [PATCH 6/7] Adjust `ShareWorker` test to not be dependent on config options --- spec/workers/share_worker_spec.rb | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/spec/workers/share_worker_spec.rb b/spec/workers/share_worker_spec.rb index 988d5300..1100f5a3 100644 --- a/spec/workers/share_worker_spec.rb +++ b/spec/workers/share_worker_spec.rb @@ -8,6 +8,20 @@ describe ShareWorker do let!(:service) { Services::Twitter.create!(type: 'Services::Twitter', user: user) } + before do + stub_const("APP_CONFIG", { + 'hostname' => 'example.com', + 'anonymous_name' => 'Anonymous', + 'https' => true, + 'items_per_page' => 5, + 'sharing' => { + 'twitter' => { + 'consumer_key' => '', + } + } + }) + end + describe "#perform" do subject { Sidekiq::Testing.fake! do @@ -27,7 +41,7 @@ describe ShareWorker do context 'when answer exists' do it 'retries on unhandled exceptions' do expect { subject }.to change(ShareWorker.jobs, :size).by(1) - expect { ShareWorker.drain }.to raise_error(Twitter::Error::Forbidden) + expect { ShareWorker.drain }.to raise_error(Twitter::Error::BadRequest) end end end From a534dd04d55f2412b3106d2924b00782ddc55ad6 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Tue, 28 Dec 2021 03:59:45 +0100 Subject: [PATCH 7/7] Test all cases of error handling for `ShareWorker` --- spec/workers/share_worker_spec.rb | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/spec/workers/share_worker_spec.rb b/spec/workers/share_worker_spec.rb index 1100f5a3..df41350d 100644 --- a/spec/workers/share_worker_spec.rb +++ b/spec/workers/share_worker_spec.rb @@ -32,15 +32,30 @@ describe ShareWorker do context 'when answer doesn\'t exist' do it 'prevents the job from retrying and logs a message' do answer.destroy! - Sidekiq.logger.should_receive(:info) + Sidekiq.logger.should_receive(:info).with("Tried to post answer ##{answer.id} for user ##{user.id} to Twitter but the user/answer/service did not exist (likely deleted), will not retry.") expect { subject }.to change(ShareWorker.jobs, :size).by(1) expect { ShareWorker.drain }.to change(ShareWorker.jobs, :size).by(-1) end end context 'when answer exists' do + it 'handles Twitter::Error::DuplicateStatus' do + allow_any_instance_of(Services::Twitter).to receive(:post).with(answer).and_raise(Twitter::Error::DuplicateStatus) + Sidekiq.logger.should_receive(:info).with("Tried to post answer ##{answer.id} from user ##{user.id} to Twitter but the status was already posted.") + subject + ShareWorker.drain + end + + it 'handles Twitter::Error::Unauthorized' do + allow_any_instance_of(Services::Twitter).to receive(:post).with(answer).and_raise(Twitter::Error::Unauthorized) + Sidekiq.logger.should_receive(:info).with("Tried to post answer ##{answer.id} from user ##{user.id} to Twitter but the token has exired or been revoked.") + subject + ShareWorker.drain + end + it 'retries on unhandled exceptions' do expect { subject }.to change(ShareWorker.jobs, :size).by(1) + Sidekiq.logger.should_receive(:info) expect { ShareWorker.drain }.to raise_error(Twitter::Error::BadRequest) end end