diff --git a/app/controllers/api/v0/videos_controller.rb b/app/controllers/api/v0/videos_controller.rb index 5ffb67041..d51c89714 100644 --- a/app/controllers/api/v0/videos_controller.rb +++ b/app/controllers/api/v0/videos_controller.rb @@ -1,21 +1,25 @@ module Api module V0 class VideosController < ApiController - caches_action :index, - cache_path: proc { |c| c.params.permit! }, - expires_in: 10.minutes respond_to :json before_action :cors_preflight_check after_action :cors_set_access_control_headers + before_action :set_cache_control_headers, only: %i[index] + def index - @page = params[:page] - @video_articles = Article.published. - where.not(video: [nil, ""], video_thumbnail_url: [nil, ""]). - where("score > ?", -4). + page = params[:page] + per_page = (params[:per_page] || 24).to_i + num = [per_page, 1000].min + + @video_articles = Article.with_video. + includes([:user]). + select(:id, :video, :path, :title, :video_thumbnail_url, :user_id, :video_duration_in_seconds). order("hotness_score DESC"). - page(params[:page].to_i).per(24) + page(page).per(num) + + set_surrogate_key_header "videos", Article.table_key, @video_articles.map(&:record_key) end end end diff --git a/app/controllers/videos_controller.rb b/app/controllers/videos_controller.rb index cfb09f0b2..9e76e68f1 100644 --- a/app/controllers/videos_controller.rb +++ b/app/controllers/videos_controller.rb @@ -6,12 +6,13 @@ class VideosController < ApplicationController def new; end def index - @video_articles = Article.published. - where.not(video: [nil, ""], video_thumbnail_url: [nil, ""]). - where("score > ?", -4). + @video_articles = Article.with_video. + includes([:user]). + select(:id, :video, :path, :title, :video_thumbnail_url, :user_id, :video_duration_in_seconds). order("hotness_score DESC"). page(params[:page].to_i).per(24) - set_surrogate_key_header "videos_landing_page" + + set_surrogate_key_header "videos", Article.table_key, @video_articles.map(&:record_key) end def create diff --git a/app/models/article.rb b/app/models/article.rb index 956f6b965..60eddfe8c 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -138,6 +138,8 @@ class Article < ApplicationRecord scope :feed, -> { published.select(:id, :published_at, :processed_html, :user_id, :organization_id, :title, :path) } + scope :with_video, -> { published.where.not(video: [nil, ""], video_thumbnail_url: [nil, ""]).where("score > ?", -4) } + algoliasearch per_environment: true, auto_remove: false, enqueue: :trigger_index do attribute :title add_index "searchables", id: :index_id, per_environment: true, enqueue: :trigger_index do diff --git a/app/views/videos/index.html.erb b/app/views/videos/index.html.erb index a4bf0c74d..553b8f90f 100644 --- a/app/views/videos/index.html.erb +++ b/app/views/videos/index.html.erb @@ -30,7 +30,7 @@ <%= video_article.video_duration_in_minutes %>

<%= video_article.title %>

-

<%= User.find(video_article.user_id).name %>

+

<%= video_article.user.name %>

<% end %> diff --git a/spec/requests/api/v0/videos_spec.rb b/spec/requests/api/v0/videos_spec.rb new file mode 100644 index 000000000..9c9350da9 --- /dev/null +++ b/spec/requests/api/v0/videos_spec.rb @@ -0,0 +1,96 @@ +require "rails_helper" + +RSpec.describe "Api::V0::Videos", type: :request do + let_it_be_readonly(:user) { create(:user, :video_permission) } + + def create_article(article_params = {}) + default_params = { + user: user, video: "https://example.com", video_thumbnail_url: "https://example.com", title: "video" + } + params = default_params.merge(article_params) + create(:article, params) + end + + describe "GET /api/videos" do + it "returns articles with videos" do + create_article + + get api_videos_path + + expect(response.parsed_body.size).to eq(1) + end + + it "does not return unpublished video articles" do + article = create_article + article.update(published: false) + + get api_videos_path + + expect(response.parsed_body.size).to eq(1) + end + + it "does not return regular articles without videos" do + create(:article) + + get api_videos_path + + expect(response.parsed_body.size).to eq(0) + end + + it "does not return video articles with a score that is too low" do + create_article(score: -4) + + get api_videos_path + + expect(response.parsed_body.size).to eq(0) + end + + it "returns video articles with the correct json representation", :aggregate_failures do + video_article = create_article + + get api_videos_path + + response_video = response.parsed_body.first + expected_keys = %w[type_of id path cloudinary_video_url title user_id video_duration_in_minutes user] + expect(response_video.keys).to match_array(expected_keys) + + %w[id path cloudinary_video_url title user_id video_duration_in_minutes].each do |attr| + expect(response_video[attr]).to eq(video_article.public_send(attr)) + end + + expect(response_video["user"]["name"]).to eq(video_article.user.name) + end + + it "orders video articles by descending hotness score" do + video_article = create_article(hotness_score: 10) + other_video_article = create_article(hotness_score: 9) + + get api_videos_path + + expected_result = [video_article.id, other_video_article.id] + expect(response.parsed_body.map { |a| a["id"] }).to eq(expected_result) + end + + it "supports pagination" do + create_list( + :article, 3, + user: user, video: "https://example.com", video_thumbnail_url: "https://example.com", title: "video" + ) + + get api_videos_path, params: { page: 1, per_page: 2 } + expect(response.parsed_body.length).to eq(2) + + get api_videos_path, params: { page: 2, per_page: 2 } + expect(response.parsed_body.length).to eq(1) + end + + it "sets the correct edge caching surrogate key for all video articles" do + video_article = create_article + + get api_videos_path + + expected_key = ["videos", "articles", video_article.record_key].to_set + expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key) + end + end +end diff --git a/spec/requests/videos_api_spec.rb b/spec/requests/videos_api_spec.rb deleted file mode 100644 index 1a8674a8c..000000000 --- a/spec/requests/videos_api_spec.rb +++ /dev/null @@ -1,17 +0,0 @@ -require "rails_helper" - -RSpec.describe "Videos", type: :request do - let(:unauthorized_user) { create(:user) } - let(:authorized_user) { create(:user, :video_permission) } - - describe "GET /api/videos" do - it "shows articles with video" do - not_video_article = create(:article) - video_article = create(:article) - video_article.update_columns(video: "video", video_thumbnail_url: "video", title: "this video") - get "/api/videos" - expect(response.body).to include(video_article.title) - expect(response.body).not_to include(not_video_article.title) - end - end -end