FIX: Use FinalDestination::HTTP to push notifications (#29858)

Sometimes `Jobs::PushNotification` gets stuck, probably because of the
network call. This commit replaces `Excon` with `FinalDestination::HTTP`
which is safer.
This commit is contained in:
Bianca Nenciu 2024-11-21 05:11:51 +02:00 committed by GitHub
parent a0cf8f64f9
commit 429cf656e7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 43 additions and 18 deletions

View File

@ -28,19 +28,22 @@ module Jobs
next if push_url.blank? next if push_url.blank?
begin uri = URI.parse(push_url)
result =
Excon.post(
push_url,
body: payload.merge(notifications: notifications).to_json,
headers: {
"Content-Type" => "application/json",
"Accept" => "application/json",
},
)
if result.status != 200 http = FinalDestination::HTTP.new(uri.host, uri.port)
# we failed to push a notification ... log it http.use_ssl = true
request =
FinalDestination::HTTP::Post.new(
uri.request_uri,
{ "Content-Type" => "application/json" },
)
request.body = payload.merge(notifications: notifications).to_json
begin
response = http.request(request)
if response.code.to_i != 200
Rails.logger.warn( Rails.logger.warn(
"Failed to push a notification to #{push_url} Status: #{result.status}: #{result.status_line}", "Failed to push a notification to #{push_url} Status: #{result.status}: #{result.status_line}",
) )

View File

@ -17,23 +17,45 @@ RSpec.describe Jobs::PushNotification do
} }
end end
let!(:request) do
stub_request(:post, "https://test.localhost:80/").with(
body: {
"secret_key" => SiteSetting.push_api_secret_key,
"url" => "http://test.localhost",
"title" => "Discourse",
"description" => "",
"notifications" => [
{
"notification_type" => 1,
"excerpt" => "Hello you",
"url" => "http://test.localhost/t/#{post.topic_id}/#{post.post_number}",
"client_id" => user.id,
},
],
}.to_json,
headers: {
"Content-Type" => "application/json",
},
).to_return(status: 200, body: "", headers: {})
end
before { SiteSetting.push_notification_time_window_mins = 5 } before { SiteSetting.push_notification_time_window_mins = 5 }
context "with valid user" do context "with valid user" do
it "does not send push notification when user is online" do it "does not send push notification when user is online" do
user.update!(last_seen_at: 1.minute.ago) user.update!(last_seen_at: 1.minute.ago)
Excon.expects(:post).never
Jobs::PushNotification.new.execute(data) Jobs::PushNotification.new.execute(data)
expect(request).not_to have_been_requested
end end
it "sends push notification when user is offline" do it "sends push notification when user is offline" do
user.update!(last_seen_at: 10.minutes.ago) user.update!(last_seen_at: 10.minutes.ago)
Excon.expects(:post).once
Jobs::PushNotification.new.execute(data) Jobs::PushNotification.new.execute(data)
expect(request).to have_been_requested.once
end end
end end
@ -41,9 +63,9 @@ RSpec.describe Jobs::PushNotification do
it "does not send push notification" do it "does not send push notification" do
data["user_id"] = -999 data["user_id"] = -999
Excon.expects(:post).never
Jobs::PushNotification.new.execute(data) Jobs::PushNotification.new.execute(data)
expect(request).not_to have_been_requested
end end
end end
end end