Various optimizations (#6249) [deploy]

* Pluck over map
* Explain why map makes sense there
* Refactor reactions controller a bit and iterate only once
* Use group by instead of N counting queries
* More positive
* Simplify BufferedArticlesController
* Less queries for MailchimpBot
* Use Rails instead of SQL
* Build comment_ids only when needed

app/controllers/buffered_articles_controller.rb

@@ -1,20 +1,21 @@
 class BufferedArticlesController < ApplicationController
   # No authorization required for entirely public controller
+
   def index
-    @article_urls = buffered_article_urls
-    render json: {
-      urls: @article_urls
-    }.to_json
+    render json: { urls: buffered_articles_urls }.to_json
   end
 
-  def buffered_article_urls
-    if Rails.env.production?
-      Article.
-        where("last_buffered > ? OR published_at > ?", 24.hours.ago, 20.minutes.ago).
-        select(:path).
-        map { |a| "https://#{ApplicationConfig['APP_DOMAIN']}#{a.path}" }
-    else
-      Article.all.map { |a| "https://#{ApplicationConfig['APP_DOMAIN']}#{a.path}" }
-    end
+  private
+
+  def buffered_articles_urls
+    relation = if Rails.env.production?
+                 Article.where("last_buffered > ?", 24.hours.ago).
+                   or(Article.where("published_at > ?", 20.minutes.ago))
+               else
+                 Article.all
+               end
+
+    paths = relation.pluck(:path)
+    paths.map { |path| "https://#{ApplicationConfig['APP_DOMAIN']}#{path}" }
   end
 end

app/controllers/reactions_controller.rb

@@ -4,37 +4,46 @@ class ReactionsController < ApplicationController
 
   def index
     skip_authorization
+
     if params[:article_id]
       id = params[:article_id]
-      reactions = if session_current_user_id.present?
-                    Reaction.where(reactable_id: id,
-                                   reactable_type: "Article",
-                                   user_id: session_current_user_id).
-                      where("points > ?", 0)
+
+      reactions = if session_current_user_id
+                    Reaction.positive.
+                      where(
+                        reactable_id: id,
+                        reactable_type: "Article",
+                        user_id: session_current_user_id,
+                      )
                   else
-                    []
+                    Reaction.none
                   end
-      render json:
-      {
-        current_user: { id: session_current_user_id },
-        article_reaction_counts: Reaction.count_for_article(id),
-        reactions: reactions
-      }.to_json
+
+      result = { article_reaction_counts: Reaction.count_for_article(id) }
     else
-      comments = Comment.where(
-        commentable_id: params[:commentable_id],
-        commentable_type: params[:commentable_type],
-      ).select(%i[id positive_reactions_count])
-      comment_ids = comments.map(&:id)
-      reaction_counts = comments.map { |c| { id: c.id, count: c.positive_reactions_count } }
-      reactions = session_current_user_id ? cached_user_positive_reactions(current_user).where(reactable_id: comment_ids) : []
-      render json:
-        {
-          current_user: { id: session_current_user_id },
-          positive_reaction_counts: reaction_counts,
-          reactions: reactions
-        }.to_json
+      comments = Comment.
+        where(commentable_id: params[:commentable_id], commentable_type: params[:commentable_type]).
+        select(%i[id positive_reactions_count])
+
+      reaction_counts = comments.map do |comment|
+        { id: comment.id, count: comment.positive_reactions_count }
+      end
+
+      reactions = if session_current_user_id
+                    comment_ids = reaction_counts.map { |rc| rc[:id] }
+                    cached_user_positive_reactions(current_user).where(reactable_id: comment_ids)
+                  else
+                    Reaction.none
+                  end
+
+      result = { positive_reaction_counts: reaction_counts }
     end
+
+    render json: {
+      current_user: { id: session_current_user_id },
+      reactions: reactions
+    }.merge(result).to_json
+
     set_surrogate_key_header params.to_s unless session_current_user_id
   end
 
@@ -79,8 +88,7 @@ class ReactionsController < ApplicationController
 
   def cached_user_positive_reactions(user)
     Rails.cache.fetch("cached_user_reactions-#{user.id}-#{user.updated_at}", expires_in: 24.hours) do
-      Reaction.where(user_id: user.id).
-        where("points > ?", 0)
+      user.reactions.positive
     end
   end
 

app/labor/mailchimp_bot.rb

@@ -76,8 +76,12 @@ class MailchimpBot
     return false unless user.tag_moderator?
 
     success = false
-    tags = user.roles.where(name: "tag_moderator").map { |tag| Tag.find(tag.resource_id).name }
+
+    tag_ids = user.roles.where(name: "tag_moderator").pluck(:resource_id)
+    tag_names = Tag.where(id: tag_ids).pluck(:name)
+
     status = user.email_tag_mod_newsletter ? "subscribed" : "unsubscribed"
+
     begin
       gibbon.lists(SiteConfig.mailchimp_tag_moderators_id).members(target_md5_email).upsert(
         body: {
@@ -89,7 +93,7 @@ class MailchimpBot
             TWITTER: user.twitter_username.to_s,
             GITHUB: user.github_username.to_s,
             IMAGE_URL: user.profile_image_url.to_s,
-            TAGS: tags.join(", ")
+            TAGS: tag_names.join(", ")
           }
         },
       )

app/models/chat_channel.rb

@@ -55,7 +55,7 @@ class ChatChannel < ApplicationRecord
     raise "Invalid direct channel" if users.size != 2 && channel_type == "direct"
 
     if channel_type == "direct"
-      usernames = users.map(&:username).sort
+      usernames = users.map(&:username).sort # .map as `users` is an array
       contrived_name = "Direct chat between " + usernames.join(" and ")
       slug = usernames.join("/")
     else

app/models/concerns/reactable.rb

@@ -6,6 +6,6 @@ module Reactable
   end
 
   def sync_reactions_count
-    update_column(:positive_reactions_count, reactions.where("points > ?", 0).size)
+    update_column(:positive_reactions_count, reactions.positive.size)
   end
 end

app/models/reaction.rb

@@ -14,6 +14,8 @@ class Reaction < ApplicationRecord
                   }
   counter_culture :user
 
+  scope :positive, -> { where("points > ?", 0) }
+
   validates :category, inclusion: { in: CATEGORIES }
   validates :reactable_type, inclusion: { in: REACTABLE_TYPES }
   validates :status, inclusion: { in: STATUSES }
@@ -48,8 +50,10 @@ class Reaction < ApplicationRecord
     def count_for_article(id)
       Rails.cache.fetch("count_for_reactable-Article-#{id}", expires_in: 1.hour) do
         reactions = Reaction.where(reactable_id: id, reactable_type: "Article")
+        counts = reactions.group(:category).count
+
         %w[like readinglist unicorn].map do |type|
-          { category: type, count: reactions.where(category: type).size }
+          { category: type, count: counts.fetch(type, 0) }
         end
       end
     end

app/services/analytics_service.rb

@@ -85,9 +85,8 @@ class AnalyticsService
       where("score > 0")
     @follow_data = Follow.
       where(followable_type: user_or_org.class.name, followable_id: user_or_org.id)
-    @reaction_data = Reaction.
-      where(reactable_id: article_ids, reactable_type: "Article").
-      where("points > 0")
+    @reaction_data = Reaction.positive.
+      where(reactable_id: article_ids, reactable_type: "Article")
     @page_view_data = PageView.where(article_id: article_ids)
 
     # filter data by date if needed

db/seeds.rb

@@ -92,7 +92,7 @@ Article.clear_index!
 num_articles.times do |i|
   tags = []
   tags << "discuss" if (i % 3).zero?
-  tags.concat Tag.order(Arel.sql("RANDOM()")).select("name").first(3).map(&:name)
+  tags.concat Tag.order(Arel.sql("RANDOM()")).limit(3).pluck(:name)
 
   markdown = <<~MARKDOWN
     ---

spec/models/reaction_spec.rb

@@ -111,6 +111,20 @@ RSpec.describe Reaction, type: :model do
     end
   end
 
+  describe ".count_for_article" do
+    it "counts the reactions an article has grouped by category" do
+      create(:reaction, reactable: article, user: user, category: "like")
+      create(:reaction, reactable: article, user: user, category: "unicorn")
+
+      expected_result = [
+        { category: "like", count: 1 },
+        { category: "readinglist", count: 0 },
+        { category: "unicorn", count: 1 },
+      ]
+      expect(described_class.count_for_article(article.id)).to eq(expected_result)
+    end
+  end
+
   context "when callbacks are called after save" do
     let!(:reaction) { build(:reaction, category: "like", reactable: article, user: user) }
 

spec/models/shared_examples/sync_reactions_count.rb

@@ -11,7 +11,7 @@ RSpec.shared_examples "#sync_reactions_count" do |reactable_type|
       expect(reactable.positive_reactions_count).to eq(0)
       reactable.sync_reactions_count
       reactable.reload
-      expected_count = reactable.reactions.where("points > ?", 0).size
+      expected_count = reactable.reactions.positive.size
       expect(reactable.positive_reactions_count).to eq(expected_count)
     end
   end

spec/requests/buffered_articles_spec.rb

@@ -2,20 +2,23 @@ require "rails_helper"
 
 RSpec.describe "BufferedArticles", type: :request do
   describe "GET /buffered_articles" do
-    it "works! (now write some real specs)" do
-      get "/buffered_articles"
+    it "works successfully" do
+      get buffered_articles_path
+
       expect(response).to have_http_status(:ok)
     end
 
     it "responds with json" do
-      get "/buffered_articles"
+      get buffered_articles_path
+
       expect(response.content_type).to eq("application/json")
     end
 
     it "responds with at least one url" do
-      create(:article)
-      get "/buffered_articles"
-      expect(response.body).to include(ApplicationConfig["APP_DOMAIN"])
+      article = create(:article)
+
+      get buffered_articles_path
+      expect(response.parsed_body["urls"].first).to eq(article.decorate.url)
     end
   end
 end

spec/requests/reactions_spec.rb

@@ -2,20 +2,35 @@ require "rails_helper"
 
 RSpec.describe "Reactions", type: :request do
   let(:user)    { create(:user) }
-  let(:article) { create(:article, user_id: user.id) }
+  let(:article) { create(:article, user: user) }
   let(:comment) { create(:comment, commentable: article) }
-  let(:max_age) { FastlyRails.configuration.max_age }
-  let(:stale_if_error) { FastlyRails.configuration.stale_if_error }
+
+  let_it_be(:max_age) { FastlyRails.configuration.max_age }
+  let_it_be(:stale_if_error) { FastlyRails.configuration.stale_if_error }
 
   describe "GET /reactions?article_id=:article_id" do
+    before do
+      create(:reaction, reactable: article, user: user, points: 1)
+    end
+
     context "when signed in" do
       before do
         sign_in user
-        get "/reactions?article_id=#{article.id}"
+
+        get reactions_path(article_id: article.id)
       end
 
-      it "returns reactions count for article" do
-        expect(response.body).to include("article_reaction_counts")
+      it "returns the correct json response" do
+        result = response.parsed_body
+
+        expect(result["current_user"]).to eq("id" => user.id)
+        expected_reactions_counts = [
+          { "category" => "like", "count" => 1 },
+          { "category" => "readinglist", "count" => 0 },
+          { "category" => "unicorn", "count" => 0 },
+        ]
+        expect(result["article_reaction_counts"]).to eq(expected_reactions_counts)
+        expect(result["reactions"].to_json).to eq(user.reactions.where(reactable: article).to_json)
       end
 
       it "does not set cache control headers" do
@@ -31,10 +46,19 @@ RSpec.describe "Reactions", type: :request do
     end
 
     context "when signed out" do
-      before { get "/reactions?article_id=#{article.id}" }
+      before { get reactions_path(article_id: article.id) }
 
-      it "returns reactions count for article" do
-        expect(response.body).to include("article_reaction_counts")
+      it "returns the correct json response" do
+        result = response.parsed_body
+
+        expect(result["current_user"]).to eq("id" => nil)
+        expected_reactions = [
+          { "category" => "like", "count" => 1 },
+          { "category" => "readinglist", "count" => 0 },
+          { "category" => "unicorn", "count" => 0 },
+        ]
+        expect(result["article_reaction_counts"]).to eq(expected_reactions)
+        expect(result["reactions"]).to be_empty
       end
 
       it "sets the surrogate key header equal to params for article" do
@@ -50,19 +74,28 @@ RSpec.describe "Reactions", type: :request do
     end
   end
 
-  describe "GET /reactions?commentable_id=:article.id&commentable_type=Comment" do
+  describe "GET /reactions?commentable_id=:article.id&commentable_type=Article" do
+    before do
+      create(:reaction, reactable: comment, user: user, points: 1)
+    end
+
     context "when signed in" do
       before do
         sign_in user
-        get "/reactions?commentable_id=#{article.id}&commentable_type=Comment"
+
+        get reactions_path(commentable_id: article.id, commentable_type: "Article")
       end
 
-      it "returns positive reaction counts" do
-        expect(response.body).to include("positive_reaction_counts")
+      it "returns the correct json response" do
+        result = response.parsed_body
+
+        expect(result["current_user"]).to eq("id" => user.id)
+        expect(result["positive_reaction_counts"]).to eq([{ "id" => article.comments.last.id, "count" => 1 }])
+        expect(result["reactions"].to_json).to eq(user.reactions.where(reactable: comment).to_json)
       end
 
       it "does not set surrogate key headers" do
-        expect(response.headers["Surrogate-Key"]).to eq(nil)
+        expect(response.headers["surrogate-key"]).to be_nil
       end
 
       it "does not set Fastly cache control and surrogate control headers" do
@@ -74,10 +107,14 @@ RSpec.describe "Reactions", type: :request do
     end
 
     context "when signed out" do
-      before { get "/reactions?commentable_id=#{article.id}&commentable_type=Comment" }
+      before { get reactions_path(commentable_id: article.id, commentable_type: "Article") }
 
-      it "returns positive reaction counts" do
-        expect(response.body).to include("positive_reaction_counts")
+      it "returns the correct json response" do
+        result = response.parsed_body
+
+        expect(result["current_user"]).to eq("id" => nil)
+        expect(result["positive_reaction_counts"]).to eq([{ "id" => article.comments.last.id, "count" => 1 }])
+        expect(result["reactions"]).to be_empty
       end
 
       it "sets the surrogate key header equal to params" do