fix: Add catch for additional webpush errors (#9662)

Webpush gem throws errors such as `WebPush::ExpiredSubscription`,
`WebPush::InvalidSubscription`, `WebPush::Unauthorized`. We handled only
ExpiredSubscription.

If the SDK threw any other errors, it would pause sending the
notification to all other devices for that user. This change would
update the logic to remove the expired subscription and handle the rest
of the errors gracefully.

Fixes
https://linear.app/chatwoot/issue/CW-3399/webpushinvalidsubscription-host-fcmgoogleapiscom-nethttpnotfound-404
This commit is contained in:
Pranav
2024-06-21 14:58:36 -07:00
committed by GitHub
parent 66c6b8cd4f
commit ee2844877c
3 changed files with 74 additions and 28 deletions

View File

@@ -6,5 +6,13 @@ FactoryBot.define do
identifier { 'test' }
subscription_type { 'browser_push' }
subscription_attributes { { endpoint: 'test', auth: 'test' } }
trait :browser_push do
subscription_type { 'browser_push' }
end
trait :fcm do
subscription_type { 'fcm' }
end
end
end

View File

@@ -7,33 +7,58 @@ describe Notification::PushNotificationService do
let(:fcm_double) { instance_double(FCM) }
let(:fcm_service_double) { instance_double(Notification::FcmService, fcm_client: fcm_double) }
before do
allow(WebPush).to receive(:payload_send).and_return(true)
allow(Notification::FcmService).to receive(:new).and_return(fcm_service_double)
allow(fcm_double).to receive(:send_v1).and_return({ body: { 'results': [] }.to_json })
allow(GlobalConfigService).to receive(:load).with('FIREBASE_PROJECT_ID', nil).and_return('test_project_id')
allow(GlobalConfigService).to receive(:load).with('FIREBASE_CREDENTIALS', nil).and_return('test_credentials')
end
describe '#perform' do
it 'sends webpush notifications for webpush subscription' do
with_modified_env VAPID_PUBLIC_KEY: 'test' do
create(:notification_subscription, user: notification.user)
context 'when the push server returns success' do
before do
allow(WebPush).to receive(:payload_send).and_return(true)
allow(Rails.logger).to receive(:info)
allow(Notification::FcmService).to receive(:new).and_return(fcm_service_double)
allow(fcm_double).to receive(:send_v1).and_return({ body: { 'results': [] }.to_json })
allow(GlobalConfigService).to receive(:load).with('FIREBASE_PROJECT_ID', nil).and_return('test_project_id')
allow(GlobalConfigService).to receive(:load).with('FIREBASE_CREDENTIALS', nil).and_return('test_credentials')
end
described_class.new(notification: notification).perform
expect(WebPush).to have_received(:payload_send)
expect(Notification::FcmService).not_to have_received(:new)
it 'sends webpush notifications for webpush subscription' do
with_modified_env VAPID_PUBLIC_KEY: 'test' do
create(:notification_subscription, user: notification.user)
described_class.new(notification: notification).perform
expect(WebPush).to have_received(:payload_send)
expect(Notification::FcmService).not_to have_received(:new)
expect(Rails.logger).to have_received(:info).with("Browser push sent to #{user.email} with title #{notification.push_message_title}")
end
end
it 'sends a fcm notification for firebase subscription' do
with_modified_env ENABLE_PUSH_RELAY_SERVER: 'false' do
create(:notification_subscription, user: notification.user, subscription_type: 'fcm')
described_class.new(notification: notification).perform
expect(Notification::FcmService).to have_received(:new)
expect(fcm_double).to have_received(:send_v1)
expect(WebPush).not_to have_received(:payload_send)
expect(Rails.logger).to have_received(:info).with("FCM push sent to #{user.email} with title #{notification.push_message_title}")
end
end
end
end
it 'sends a fcm notification for firebase subscription' do
with_modified_env ENABLE_PUSH_RELAY_SERVER: 'false' do
create(:notification_subscription, user: notification.user, subscription_type: 'fcm')
context 'when the push server returns error' do
it 'sends webpush notifications for webpush subscription' do
with_modified_env VAPID_PUBLIC_KEY: 'test' do
mock_response = instance_double(Net::HTTPResponse, body: 'Subscription is invalid')
mock_host = 'fcm.googleapis.com'
allow(WebPush).to receive(:payload_send).and_raise(WebPush::InvalidSubscription.new(mock_response, mock_host))
allow(Rails.logger).to receive(:info)
create(:notification_subscription, :browser_push, user: notification.user)
expect(Rails.logger).to receive(:info) do |message|
expect(message).to include('WebPush subscription expired:')
end
described_class.new(notification: notification).perform
expect(Notification::FcmService).to have_received(:new)
expect(fcm_double).to have_received(:send_v1)
expect(WebPush).not_to have_received(:payload_send)
end
end
end