Improve video controllers caching and retrieving (#5709) [deploy]

* Add tests and improve pagination and caching

* Add two more specs

* Add .with_video scope

* Get rid of N+1

* Only select needed columns

* Improve efficiency of the video page as well

* Add missing set_cache_control_headers to API videos#index

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

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

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

app/views/videos/index.html.erb

@@ -30,7 +30,7 @@
             <span class="video-timestamp"><%= video_article.video_duration_in_minutes %></span>
           </div>
           <p><strong><%= video_article.title %></strong></p>
-          <p class="video-username"><%= User.find(video_article.user_id).name %></p>
+          <p class="video-username"><%= video_article.user.name %></p>
       </a>
     <% end %>
   </div>

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

spec/requests/videos_api_spec.rb

@@ -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