From fe58ac8d9f1b0c4347fde451f1caedac2ac605bc Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Wed, 15 Nov 2023 08:14:51 -0500 Subject: [PATCH] Improve spec coverage for `api/web/push_subscriptions` controller (#27858) Co-authored-by: Claire --- .../api/web/push_subscriptions_controller.rb | 78 ++++++++++++------- .../web/push_subscriptions_controller_spec.rb | 60 ++++++++------ 2 files changed, 86 insertions(+), 52 deletions(-) diff --git a/app/controllers/api/web/push_subscriptions_controller.rb b/app/controllers/api/web/push_subscriptions_controller.rb index 5167928e93..167d16fc4d 100644 --- a/app/controllers/api/web/push_subscriptions_controller.rb +++ b/app/controllers/api/web/push_subscriptions_controller.rb @@ -3,37 +3,13 @@ class Api::Web::PushSubscriptionsController < Api::Web::BaseController before_action :require_user! before_action :set_push_subscription, only: :update + before_action :destroy_previous_subscriptions, only: :create, if: :prior_subscriptions? + after_action :update_session_with_subscription, only: :create def create - active_session = current_session + @push_subscription = ::Web::PushSubscription.create!(web_push_subscription_params) - unless active_session.web_push_subscription.nil? - active_session.web_push_subscription.destroy! - active_session.update!(web_push_subscription: nil) - end - - # Mobile devices do not support regular notifications, so we enable push notifications by default - alerts_enabled = active_session.detection.device.mobile? || active_session.detection.device.tablet? - - data = { - policy: 'all', - alerts: Notification::TYPES.index_with { alerts_enabled }, - } - - data.deep_merge!(data_params) if params[:data] - - push_subscription = ::Web::PushSubscription.create!( - endpoint: subscription_params[:endpoint], - key_p256dh: subscription_params[:keys][:p256dh], - key_auth: subscription_params[:keys][:auth], - data: data, - user_id: active_session.user_id, - access_token_id: active_session.access_token_id - ) - - active_session.update!(web_push_subscription: push_subscription) - - render json: push_subscription, serializer: REST::WebPushSubscriptionSerializer + render json: @push_subscription, serializer: REST::WebPushSubscriptionSerializer end def update @@ -43,6 +19,41 @@ class Api::Web::PushSubscriptionsController < Api::Web::BaseController private + def active_session + @active_session ||= current_session + end + + def destroy_previous_subscriptions + active_session.web_push_subscription.destroy! + active_session.update!(web_push_subscription: nil) + end + + def prior_subscriptions? + active_session.web_push_subscription.present? + end + + def subscription_data + default_subscription_data.tap do |data| + data.deep_merge!(data_params) if params[:data] + end + end + + def default_subscription_data + { + policy: 'all', + alerts: Notification::TYPES.index_with { alerts_enabled }, + } + end + + def alerts_enabled + # Mobile devices do not support regular notifications, so we enable push notifications by default + active_session.detection.device.mobile? || active_session.detection.device.tablet? + end + + def update_session_with_subscription + active_session.update!(web_push_subscription: @push_subscription) + end + def set_push_subscription @push_subscription = ::Web::PushSubscription.find(params[:id]) end @@ -51,6 +62,17 @@ class Api::Web::PushSubscriptionsController < Api::Web::BaseController @subscription_params ||= params.require(:subscription).permit(:endpoint, keys: [:auth, :p256dh]) end + def web_push_subscription_params + { + access_token_id: active_session.access_token_id, + data: subscription_data, + endpoint: subscription_params[:endpoint], + key_auth: subscription_params[:keys][:auth], + key_p256dh: subscription_params[:keys][:p256dh], + user_id: active_session.user_id, + } + end + def data_params @data_params ||= params.require(:data).permit(:policy, alerts: Notification::TYPES) end diff --git a/spec/controllers/api/web/push_subscriptions_controller_spec.rb b/spec/controllers/api/web/push_subscriptions_controller_spec.rb index 9f027ede90..58677841ca 100644 --- a/spec/controllers/api/web/push_subscriptions_controller_spec.rb +++ b/spec/controllers/api/web/push_subscriptions_controller_spec.rb @@ -37,37 +37,49 @@ describe Api::Web::PushSubscriptionsController do } end + before do + sign_in(user) + + stub_request(:post, create_payload[:subscription][:endpoint]).to_return(status: 200) + end + describe 'POST #create' do it 'saves push subscriptions' do - sign_in(user) - - stub_request(:post, create_payload[:subscription][:endpoint]).to_return(status: 200) - post :create, format: :json, params: create_payload + expect(response).to have_http_status(200) + user.reload - push_subscription = Web::PushSubscription.find_by(endpoint: create_payload[:subscription][:endpoint]) + expect(created_push_subscription).to have_attributes( + endpoint: eq(create_payload[:subscription][:endpoint]), + key_p256dh: eq(create_payload[:subscription][:keys][:p256dh]), + key_auth: eq(create_payload[:subscription][:keys][:auth]) + ) + expect(user.session_activations.first.web_push_subscription).to eq(created_push_subscription) + end - expect(push_subscription['endpoint']).to eq(create_payload[:subscription][:endpoint]) - expect(push_subscription['key_p256dh']).to eq(create_payload[:subscription][:keys][:p256dh]) - expect(push_subscription['key_auth']).to eq(create_payload[:subscription][:keys][:auth]) + context 'with a user who has a session with a prior subscription' do + let!(:prior_subscription) { Fabricate(:web_push_subscription, session_activation: user.session_activations.last) } + + it 'destroys prior subscription when creating new one' do + post :create, format: :json, params: create_payload + + expect(response).to have_http_status(200) + expect { prior_subscription.reload }.to raise_error(ActiveRecord::RecordNotFound) + end end context 'with initial data' do it 'saves alert settings' do - sign_in(user) - - stub_request(:post, create_payload[:subscription][:endpoint]).to_return(status: 200) - post :create, format: :json, params: create_payload.merge(alerts_payload) - push_subscription = Web::PushSubscription.find_by(endpoint: create_payload[:subscription][:endpoint]) + expect(response).to have_http_status(200) - expect(push_subscription.data['policy']).to eq 'all' + expect(created_push_subscription.data['policy']).to eq 'all' %w(follow follow_request favourite reblog mention poll status).each do |type| - expect(push_subscription.data['alerts'][type]).to eq(alerts_payload[:data][:alerts][type.to_sym].to_s) + expect(created_push_subscription.data['alerts'][type]).to eq(alerts_payload[:data][:alerts][type.to_sym].to_s) end end end @@ -75,23 +87,23 @@ describe Api::Web::PushSubscriptionsController do describe 'PUT #update' do it 'changes alert settings' do - sign_in(user) - - stub_request(:post, create_payload[:subscription][:endpoint]).to_return(status: 200) - post :create, format: :json, params: create_payload - alerts_payload[:id] = Web::PushSubscription.find_by(endpoint: create_payload[:subscription][:endpoint]).id + expect(response).to have_http_status(200) + + alerts_payload[:id] = created_push_subscription.id put :update, format: :json, params: alerts_payload - push_subscription = Web::PushSubscription.find_by(endpoint: create_payload[:subscription][:endpoint]) - - expect(push_subscription.data['policy']).to eq 'all' + expect(created_push_subscription.data['policy']).to eq 'all' %w(follow follow_request favourite reblog mention poll status).each do |type| - expect(push_subscription.data['alerts'][type]).to eq(alerts_payload[:data][:alerts][type.to_sym].to_s) + expect(created_push_subscription.data['alerts'][type]).to eq(alerts_payload[:data][:alerts][type.to_sym].to_s) end end end + + def created_push_subscription + Web::PushSubscription.find_by(endpoint: create_payload[:subscription][:endpoint]) + end end