Refactor of API timeline actions (#3263)

- Increase coverage to exercise all parts of each action
- Move into namespace to share common code
- Misc refactor of each action for smaller methods, simpler code

app/controllers/api/v1/timelines/base_controller.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+module Api::V1::Timelines
+  class BaseController < ApiController
+    respond_to :json
+    after_action :insert_pagination_headers, unless: -> { @statuses.empty? }
+
+    private
+
+    def cache_collection(raw)
+      super(raw, Status)
+    end
+
+    def pagination_params(core_params)
+      params.permit(:local, :limit).merge(core_params)
+    end
+
+    def insert_pagination_headers
+      set_pagination_headers(next_path, prev_path)
+    end
+
+    def next_path
+      raise 'Override in child controllers'
+    end
+
+    def prev_path
+      raise 'Override in child controllers'
+    end
+  end
+end

app/controllers/api/v1/timelines/home_controller.rb

@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+module Api::V1::Timelines
+  class HomeController < BaseController
+    before_action -> { doorkeeper_authorize! :read }, only: [:show]
+    before_action :require_user!, only: [:show]
+
+    def show
+      @statuses = load_statuses
+    end
+
+    private
+
+    def load_statuses
+      cached_home_statuses.tap do |statuses|
+        set_maps(statuses)
+      end
+    end
+
+    def cached_home_statuses
+      cache_collection home_statuses
+    end
+
+    def home_statuses
+      account_home_feed.get(
+        limit_param(DEFAULT_STATUSES_LIMIT),
+        params[:max_id],
+        params[:since_id]
+      )
+    end
+
+    def account_home_feed
+      Feed.new(:home, current_account)
+    end
+
+    def next_path
+      api_v1_timelines_home_url pagination_params(max_id: @statuses.last.id)
+    end
+
+    def prev_path
+      api_v1_timelines_home_url pagination_params(since_id: @statuses.first.id)
+    end
+  end
+end

app/controllers/api/v1/timelines/public_controller.rb

@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+module Api::V1::Timelines
+  class PublicController < BaseController
+    def show
+      @statuses = load_statuses
+    end
+
+    private
+
+    def load_statuses
+      cached_public_statuses.tap do |statuses|
+        set_maps(statuses)
+      end
+    end
+
+    def cached_public_statuses
+      cache_collection public_statuses
+    end
+
+    def public_statuses
+      public_timeline_statuses.paginate_by_max_id(
+        limit_param(DEFAULT_STATUSES_LIMIT),
+        params[:max_id],
+        params[:since_id]
+      )
+    end
+
+    def public_timeline_statuses
+      Status.as_public_timeline(current_account, params[:local])
+    end
+
+    def next_path
+      api_v1_timelines_public_url pagination_params(max_id: @statuses.last.id)
+    end
+
+    def prev_path
+      api_v1_timelines_public_url pagination_params(since_id: @statuses.first.id)
+    end
+  end
+end

app/controllers/api/v1/timelines/tag_controller.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+module Api::V1::Timelines
+  class TagController < BaseController
+    before_action :load_tag
+
+    def show
+      @statuses = load_statuses
+    end
+
+    private
+
+    def load_tag
+      @tag = Tag.find_by(name: params[:id].downcase)
+    end
+
+    def load_statuses
+      cached_tagged_statuses.tap do |statuses|
+        set_maps(statuses)
+      end
+    end
+
+    def cached_tagged_statuses
+      cache_collection tagged_statuses
+    end
+
+    def tagged_statuses
+      if @tag.nil?
+        []
+      else
+        tag_timeline_statuses.paginate_by_max_id(
+          limit_param(DEFAULT_STATUSES_LIMIT),
+          params[:max_id],
+          params[:since_id]
+        )
+      end
+    end
+
+    def tag_timeline_statuses
+      Status.as_tag_timeline(@tag, current_account, params[:local])
+    end
+
+    def next_path
+      api_v1_timelines_tag_url params[:id], pagination_params(max_id: @statuses.last.id)
+    end
+
+    def prev_path
+      api_v1_timelines_tag_url params[:id], pagination_params(since_id: @statuses.first.id)
+    end
+  end
+end

app/controllers/api/v1/timelines_controller.rb

@@ -1,61 +0,0 @@
-# frozen_string_literal: true
-
-class Api::V1::TimelinesController < ApiController
-  before_action -> { doorkeeper_authorize! :read }, only: [:home]
-  before_action :require_user!, only: [:home]
-
-  respond_to :json
-
-  def home
-    @statuses = Feed.new(:home, current_account).get(limit_param(DEFAULT_STATUSES_LIMIT), params[:max_id], params[:since_id])
-    @statuses = cache_collection(@statuses)
-
-    set_maps(@statuses)
-
-    next_path = api_v1_home_timeline_url(pagination_params(max_id: @statuses.last.id))    unless @statuses.empty?
-    prev_path = api_v1_home_timeline_url(pagination_params(since_id: @statuses.first.id)) unless @statuses.empty?
-
-    set_pagination_headers(next_path, prev_path)
-
-    render :index
-  end
-
-  def public
-    @statuses = Status.as_public_timeline(current_account, params[:local]).paginate_by_max_id(limit_param(DEFAULT_STATUSES_LIMIT), params[:max_id], params[:since_id])
-    @statuses = cache_collection(@statuses)
-
-    set_maps(@statuses)
-
-    next_path = api_v1_public_timeline_url(pagination_params(max_id: @statuses.last.id))    unless @statuses.empty?
-    prev_path = api_v1_public_timeline_url(pagination_params(since_id: @statuses.first.id)) unless @statuses.empty?
-
-    set_pagination_headers(next_path, prev_path)
-
-    render :index
-  end
-
-  def tag
-    @tag      = Tag.find_by(name: params[:id].downcase)
-    @statuses = @tag.nil? ? [] : Status.as_tag_timeline(@tag, current_account, params[:local]).paginate_by_max_id(limit_param(DEFAULT_STATUSES_LIMIT), params[:max_id], params[:since_id])
-    @statuses = cache_collection(@statuses)
-
-    set_maps(@statuses)
-
-    next_path = api_v1_hashtag_timeline_url(params[:id], pagination_params(max_id: @statuses.last.id))    unless @statuses.empty?
-    prev_path = api_v1_hashtag_timeline_url(params[:id], pagination_params(since_id: @statuses.first.id)) unless @statuses.empty?
-
-    set_pagination_headers(next_path, prev_path)
-
-    render :index
-  end
-
-  private
-
-  def cache_collection(raw)
-    super(raw, Status)
-  end
-
-  def pagination_params(core_params)
-    params.permit(:local, :limit).merge(core_params)
-  end
-end

config/routes.rb

@@ -136,9 +136,11 @@ Rails.application.routes.draw do
         end
       end
 
-      get '/timelines/home',     to: 'timelines#home', as: :home_timeline
+      namespace :timelines do
-      get '/timelines/public',   to: 'timelines#public', as: :public_timeline
+        resource :home, only: :show, controller: :home
-      get '/timelines/tag/:id',  to: 'timelines#tag', as: :hashtag_timeline
+        resource :public, only: :show, controller: :public
+        resources :tag, only: :show
+      end
 
       get '/search', to: 'search#index', as: :search
 

spec/controllers/api/v1/timelines/home_controller_spec.rb

@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe Api::V1::Timelines::HomeController do
+  render_views
+
+  let(:user)  { Fabricate(:user, account: Fabricate(:account, username: 'alice'), current_sign_in_at: 1.day.ago) }
+
+  before do
+    allow(controller).to receive(:doorkeeper_token) { token }
+  end
+
+  context 'with a user context' do
+    let(:token) { double acceptable?: true, resource_owner_id: user.id }
+
+    describe 'GET #show' do
+      before do
+        follow = Fabricate(:follow, account: user.account)
+        PostStatusService.new.call(follow.target_account, 'New status for user home timeline.')
+      end
+
+      it 'returns http success' do
+        get :show
+
+        expect(response).to have_http_status(:success)
+        expect(response.headers['Link'].links.size).to eq(2)
+      end
+    end
+  end
+
+  context 'without a user context' do
+    let(:token) { double acceptable?: true, resource_owner_id: nil }
+
+    describe 'GET #show' do
+      it 'returns http unprocessable entity' do
+        get :show
+
+        expect(response).to have_http_status(:unprocessable_entity)
+        expect(response.headers['Link']).to be_nil
+      end
+    end
+  end
+end

spec/controllers/api/v1/timelines/public_controller_spec.rb

@@ -1,6 +1,8 @@
+# frozen_string_literal: true
+
 require 'rails_helper'
 
-RSpec.describe Api::V1::TimelinesController, type: :controller do
+describe Api::V1::Timelines::PublicController do
   render_views
 
   let(:user)  { Fabricate(:user, account: Fabricate(:account, username: 'alice')) }
@@ -12,28 +14,29 @@ RSpec.describe Api::V1::TimelinesController, type: :controller do
   context 'with a user context' do
     let(:token) { double acceptable?: true, resource_owner_id: user.id }
 
-    describe 'GET #home' do
+    describe 'GET #show' do
-      it 'returns http success' do
-        get :home
-        expect(response).to have_http_status(:success)
-      end
-    end
-
-    describe 'GET #public' do
-      it 'returns http success' do
-        get :public
-        expect(response).to have_http_status(:success)
-      end
-    end
-
-    describe 'GET #tag' do
       before do
-        PostStatusService.new.call(user.account, 'It is a #test')
+        PostStatusService.new.call(user.account, 'New status from user for federated public timeline.')
       end
 
       it 'returns http success' do
-        get :tag, params: { id: 'test' }
+        get :show
+
         expect(response).to have_http_status(:success)
+        expect(response.headers['Link'].links.size).to eq(2)
+      end
+    end
+
+    describe 'GET #show with local only' do
+      before do
+        PostStatusService.new.call(user.account, 'New status from user for local public timeline.')
+      end
+
+      it 'returns http success' do
+        get :show, params: { local: true }
+
+        expect(response).to have_http_status(:success)
+        expect(response.headers['Link'].links.size).to eq(2)
       end
     end
   end
@@ -41,24 +44,12 @@ RSpec.describe Api::V1::TimelinesController, type: :controller do
   context 'without a user context' do
     let(:token) { double acceptable?: true, resource_owner_id: nil }
 
-    describe 'GET #home' do
+    describe 'GET #show' do
-      it 'returns http unprocessable entity' do
-        get :home
-        expect(response).to have_http_status(:unprocessable_entity)
-      end
-    end
-
-    describe 'GET #public' do
       it 'returns http success' do
-        get :public
+        get :show
-        expect(response).to have_http_status(:success)
-      end
-    end
 
-    describe 'GET #tag' do
-      it 'returns http success' do
-        get :tag, params: { id: 'test' }
         expect(response).to have_http_status(:success)
+        expect(response.headers['Link']).to be_nil
       end
     end
   end

spec/controllers/api/v1/timelines/tag_controller_spec.rb

@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe Api::V1::Timelines::TagController do
+  render_views
+
+  let(:user)  { Fabricate(:user, account: Fabricate(:account, username: 'alice')) }
+
+  before do
+    allow(controller).to receive(:doorkeeper_token) { token }
+  end
+
+  context 'with a user context' do
+    let(:token) { double acceptable?: true, resource_owner_id: user.id }
+
+    describe 'GET #show' do
+      before do
+        PostStatusService.new.call(user.account, 'It is a #test')
+      end
+
+      it 'returns http success' do
+        get :show, params: { id: 'test' }
+        expect(response).to have_http_status(:success)
+        expect(response.headers['Link'].links.size).to eq(2)
+      end
+    end
+  end
+
+  context 'without a user context' do
+    let(:token) { double acceptable?: true, resource_owner_id: nil }
+
+    describe 'GET #show' do
+      it 'returns http success' do
+        get :show, params: { id: 'test' }
+        expect(response).to have_http_status(:success)
+        expect(response.headers['Link']).to be_nil
+      end
+    end
+  end
+end

spec/routing/api_timelines_spec.rb

@@ -0,0 +1,18 @@
+require 'rails_helper'
+
+describe 'API timeline routes' do
+  it 'routes to home timeline' do
+    expect(get('/api/v1/timelines/home')).
+      to route_to('api/v1/timelines/home#show')
+  end
+
+  it 'routes to public timeline' do
+    expect(get('/api/v1/timelines/public')).
+      to route_to('api/v1/timelines/public#show')
+  end
+
+  it 'routes to tag timeline' do
+    expect(get('/api/v1/timelines/tag/test')).
+      to route_to('api/v1/timelines/tag#show', id: 'test')
+  end
+end