Clean up api/subscriptions controller (#3448)

app/controllers/api/subscriptions_controller.rb

@@ -5,18 +5,15 @@ class Api::SubscriptionsController < ApiController
   respond_to :txt
 
   def show
-    if @account.subscription(api_subscription_url(@account.id)).valid?(params['hub.topic'])
+    if subscription.valid?(params['hub.topic'])
-      @account.update(subscription_expires_at: Time.now.utc + (params['hub.lease_seconds'] || 86_400).to_i.seconds)
+      @account.update(subscription_expires_at: future_expires)
-      render plain: HTMLEntities.new.encode(params['hub.challenge']), status: 200
+      render plain: encoded_challenge, status: 200
     else
       head 404
     end
   end
 
   def update
-    body = request.body.read
-    subscription = @account.subscription(api_subscription_url(@account.id))
-
     if subscription.verify(body, request.headers['HTTP_X_HUB_SIGNATURE'])
       ProcessingWorker.perform_async(@account.id, body.force_encoding('UTF-8'))
     end
@@ -26,6 +23,28 @@ class Api::SubscriptionsController < ApiController
 
   private
 
+  def subscription
+    @_subscription ||= @account.subscription(
+      api_subscription_url(@account.id)
+    )
+  end
+
+  def body
+    @_body ||= request.body.read
+  end
+
+  def encoded_challenge
+    HTMLEntities.new.encode(params['hub.challenge'])
+  end
+
+  def future_expires
+    Time.now.utc + lease_seconds_or_default
+  end
+
+  def lease_seconds_or_default
+    (params['hub.lease_seconds'] || 86_400).to_i.seconds
+  end
+
   def set_account
     @account = Account.find(params[:id])
   end

spec/controllers/api/subscriptions_controller_spec.rb

@@ -6,16 +6,29 @@ RSpec.describe Api::SubscriptionsController, type: :controller do
   let(:account) { Fabricate(:account, username: 'gargron', domain: 'quitter.no', remote_url: 'topic_url', secret: 'abc') }
 
   describe 'GET #show' do
-    before do
+    context 'with valid subscription' do
-      get :show, params: { :id => account.id, 'hub.topic' => 'topic_url', 'hub.challenge' => '456', 'hub.lease_seconds' => "#{86400 * 30}" }
+      before do
-    end
+        get :show, params: { :id => account.id, 'hub.topic' => 'topic_url', 'hub.challenge' => '456', 'hub.lease_seconds' => "#{86400 * 30}" }
+      end
 
-    it 'returns http success' do
+      it 'returns http success' do
-      expect(response).to have_http_status(:success)
+        expect(response).to have_http_status(:success)
+      end
+
+      it 'echoes back the challenge' do
+        expect(response.body).to match '456'
+      end
     end
 
-    it 'echoes back the challenge' do
+    context 'with invalid subscription' do
-      expect(response.body).to match '456'
+      before do
+        expect_any_instance_of(Account).to receive_message_chain(:subscription, :valid?).and_return(false)
+        get :show, params: { :id => account.id }
+      end
+
+      it 'returns http success' do
+        expect(response).to have_http_status(:missing)
+      end
     end
   end