Improve spec coverage for api/web/push_subscriptions controller (#27858)

Co-authored-by: Claire <claire.github-309c@sitedethib.com>

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?
+    render json: @push_subscription, serializer: REST::WebPushSubscriptionSerializer
-      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
   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

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])
+    context 'with a user who has a session with a prior subscription' do
-      expect(push_subscription['key_p256dh']).to eq(create_payload[:subscription][:keys][:p256dh])
+      let!(:prior_subscription) { Fabricate(:web_push_subscription, session_activation: user.session_activations.last) }
-      expect(push_subscription['key_auth']).to eq(create_payload[:subscription][:keys][:auth])
+
+      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(created_push_subscription.data['policy']).to eq 'all'
-
-      expect(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