From 4289ed1d13b9c3b91663581c44635105c4bc0412 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Tue, 23 May 2017 12:11:39 -0400 Subject: [PATCH] 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 --- .../api/v1/timelines/base_controller.rb | 30 +++++++++ .../api/v1/timelines/home_controller.rb | 44 +++++++++++++ .../api/v1/timelines/public_controller.rb | 41 +++++++++++++ .../api/v1/timelines/tag_controller.rb | 51 ++++++++++++++++ .../api/v1/timelines_controller.rb | 61 ------------------- .../timelines/{index.rabl => base/show.rabl} | 0 config/routes.rb | 8 ++- .../api/v1/timelines/home_controller_spec.rb | 44 +++++++++++++ .../public_controller_spec.rb} | 43 ++++++------- .../api/v1/timelines/tag_controller_spec.rb | 41 +++++++++++++ spec/routing/api_timelines_spec.rb | 18 ++++++ 11 files changed, 291 insertions(+), 90 deletions(-) create mode 100644 app/controllers/api/v1/timelines/base_controller.rb create mode 100644 app/controllers/api/v1/timelines/home_controller.rb create mode 100644 app/controllers/api/v1/timelines/public_controller.rb create mode 100644 app/controllers/api/v1/timelines/tag_controller.rb delete mode 100644 app/controllers/api/v1/timelines_controller.rb rename app/views/api/v1/timelines/{index.rabl => base/show.rabl} (100%) create mode 100644 spec/controllers/api/v1/timelines/home_controller_spec.rb rename spec/controllers/api/v1/{timelines_controller_spec.rb => timelines/public_controller_spec.rb} (50%) create mode 100644 spec/controllers/api/v1/timelines/tag_controller_spec.rb create mode 100644 spec/routing/api_timelines_spec.rb diff --git a/app/controllers/api/v1/timelines/base_controller.rb b/app/controllers/api/v1/timelines/base_controller.rb new file mode 100644 index 000000000..4eb29e74a --- /dev/null +++ b/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 diff --git a/app/controllers/api/v1/timelines/home_controller.rb b/app/controllers/api/v1/timelines/home_controller.rb new file mode 100644 index 000000000..33ff48b39 --- /dev/null +++ b/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 diff --git a/app/controllers/api/v1/timelines/public_controller.rb b/app/controllers/api/v1/timelines/public_controller.rb new file mode 100644 index 000000000..644c7a6f1 --- /dev/null +++ b/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 diff --git a/app/controllers/api/v1/timelines/tag_controller.rb b/app/controllers/api/v1/timelines/tag_controller.rb new file mode 100644 index 000000000..818f49d3d --- /dev/null +++ b/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 diff --git a/app/controllers/api/v1/timelines_controller.rb b/app/controllers/api/v1/timelines_controller.rb deleted file mode 100644 index 27dc3d0ef..000000000 --- a/app/controllers/api/v1/timelines_controller.rb +++ /dev/null @@ -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 diff --git a/app/views/api/v1/timelines/index.rabl b/app/views/api/v1/timelines/base/show.rabl similarity index 100% rename from app/views/api/v1/timelines/index.rabl rename to app/views/api/v1/timelines/base/show.rabl diff --git a/config/routes.rb b/config/routes.rb index e3c6ce156..81c205daa 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -136,9 +136,11 @@ Rails.application.routes.draw do end end - get '/timelines/home', to: 'timelines#home', as: :home_timeline - get '/timelines/public', to: 'timelines#public', as: :public_timeline - get '/timelines/tag/:id', to: 'timelines#tag', as: :hashtag_timeline + namespace :timelines do + resource :home, only: :show, controller: :home + resource :public, only: :show, controller: :public + resources :tag, only: :show + end get '/search', to: 'search#index', as: :search diff --git a/spec/controllers/api/v1/timelines/home_controller_spec.rb b/spec/controllers/api/v1/timelines/home_controller_spec.rb new file mode 100644 index 000000000..faa6c60ce --- /dev/null +++ b/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 diff --git a/spec/controllers/api/v1/timelines_controller_spec.rb b/spec/controllers/api/v1/timelines/public_controller_spec.rb similarity index 50% rename from spec/controllers/api/v1/timelines_controller_spec.rb rename to spec/controllers/api/v1/timelines/public_controller_spec.rb index 72eed1e5a..353ab9bc2 100644 --- a/spec/controllers/api/v1/timelines_controller_spec.rb +++ b/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 - it 'returns http success' do - get :home - expect(response).to have_http_status(:success) + describe 'GET #show' do + before do + PostStatusService.new.call(user.account, 'New status from user for federated public timeline.') end - end - describe 'GET #public' do it 'returns http success' do - get :public + get :show + expect(response).to have_http_status(:success) + expect(response.headers['Link'].links.size).to eq(2) end end - describe 'GET #tag' do + describe 'GET #show with local only' do before do - PostStatusService.new.call(user.account, 'It is a #test') + PostStatusService.new.call(user.account, 'New status from user for local public timeline.') end it 'returns http success' do - get :tag, params: { id: 'test' } + 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 - it 'returns http unprocessable entity' do - get :home - expect(response).to have_http_status(:unprocessable_entity) - end - end - - describe 'GET #public' do + describe 'GET #show' do it 'returns http success' do - get :public - expect(response).to have_http_status(:success) - end - end + get :show - 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 diff --git a/spec/controllers/api/v1/timelines/tag_controller_spec.rb b/spec/controllers/api/v1/timelines/tag_controller_spec.rb new file mode 100644 index 000000000..f743f0cde --- /dev/null +++ b/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 diff --git a/spec/routing/api_timelines_spec.rb b/spec/routing/api_timelines_spec.rb new file mode 100644 index 000000000..31717209f --- /dev/null +++ b/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