Fix (some) n+1/eager loading issues (#5294) [deploy]

* Remove mock from specs to test the actual Suggester

* Fix articles API top articles n+1

* Fix n+1 in articles API with state parameter

* Remove eager loading for organization dashboard

* Eager load notifiables in notifications only when needed

* Algolia does not like this

app/controllers/additional_content_boxes_controller.rb

@@ -8,16 +8,18 @@ class AdditionalContentBoxesController < ApplicationController
 
     article_ids = params[:article_id].split(",")
     @article = Article.find(article_ids[0])
-    @suggested_articles = Suggester::Articles::Classic.
-      new(@article, not_ids: article_ids).get(2)
+
+    @suggested_articles = Suggester::Articles::Classic.new(@article, not_ids: article_ids).get(2)
+
     if (!user_signed_in? || params[:state] == "include_sponsors") &&
         @article.user.permit_adjacent_sponsors &&
         randomize
-      @boosted_article = Suggester::Articles::Boosted.new(
-        (@article.decorate.cached_tag_list_array + @article.boosted_additional_tags.split).sample,
-        not_ids: (article_ids + @suggested_articles.pluck(:id)), area: "additional_articles",
-      ).suggest
+
+      tag = (@article.decorate.cached_tag_list_array + @article.boosted_additional_tags.split).sample
+      not_ids = article_ids + @suggested_articles.pluck(:id)
+      @boosted_article = Suggester::Articles::Boosted.new(tag, not_ids: not_ids, area: "additional_articles").suggest
     end
+
     set_surrogate_key_header "additional_content_boxes_" + params.to_s
     render "boxes", layout: false
   end

app/controllers/dashboards_controller.rb

@@ -16,12 +16,17 @@ class DashboardsController < ApplicationController
     if params[:which] == "organization" && params[:org_id] && (@user.org_admin?(params[:org_id]) || @user.any_admin?)
       target = @organizations.find_by(id: params[:org_id])
       @organization = target
+      @articles = target.articles
+    else
+      # if the target is a user, we need to eager load the organization
+      @articles = target.articles.includes(:organization)
     end
 
-    @articles = target.articles.includes(:organization).sorting(params[:sort]).decorate
+    @articles = @articles.sorting(params[:sort]).decorate
 
     # Updates analytics in background if appropriate
-    Articles::UpdateAnalyticsWorker.perform_async(current_user.id) if @articles && SiteConfig.ga_fetch_rate < 50 # Rate limit concerned, sometimes we throttle down.
+    update_analytics = @articles && SiteConfig.ga_fetch_rate < 50 # Rate limited, sometimes we throttle down
+    Articles::UpdateAnalyticsWorker.perform_async(current_user.id) if update_analytics
   end
 
   def following_tags

app/controllers/notifications_controller.rb

@@ -26,12 +26,20 @@ class NotificationsController < ApplicationController
                        @user.notifications
                      end
 
-    @notifications = @notifications.includes(:notifiable).without_past_aggregations.order(notified_at: :desc)
+    @notifications = @notifications.without_past_aggregations.order(notified_at: :desc)
 
     # if offset based pagination is invoked by the frontend code, we filter out all earlier ones
     @notifications = @notifications.where("notified_at < ?", notified_at_offset) if notified_at_offset
 
-    @notifications = NotificationDecorator.decorate_collection(@notifications.limit(num))
+    @notifications = @notifications.limit(num)
+
+    # after notifications are paginated/limited, we check if any of the notifiables is "Comment" with no action
+    # because its respective view is the only one that actually uses ".notifiable" to render,
+    # this way we save useless eager loadings
+    notifiable_eager_loading_params = { notifiable_type: %w[Comment], action: [nil, ""] }
+    @notifications = @notifications.includes(:notifiable) if @notifications.exists?(notifiable_eager_loading_params)
+
+    @notifications = NotificationDecorator.decorate_collection(@notifications)
 
     @last_user_reaction = @user.reactions.last&.id
     @last_user_comment = @user.comments.last&.id

app/services/article_api_index_service.rb

@@ -74,18 +74,29 @@ class ArticleApiIndexService
   end
 
   def top_articles
-    Article.published.order("positive_reactions_count DESC").where("published_at > ?", top.to_i.days.ago).
+    Article.published.includes(:user, :organization).
+      where("published_at > ?", top.to_i.days.ago).
+      order("positive_reactions_count DESC").
       page(page).per(per_page || DEFAULT_PER_PAGE)
   end
 
   def state_articles(state)
-    if state == "fresh"
-      Article.published.
-        where("positive_reactions_count < ? AND featured_number > ? AND score > ?", 2, 7.hours.ago.to_i, -2)
-    elsif state == "rising"
-      Article.published.
-        where("positive_reactions_count > ? AND positive_reactions_count < ? AND featured_number > ?", 19, 33, 3.days.ago.to_i)
-    end
+    articles = Article.published.includes(:user, :organization)
+
+    articles = if state == "fresh"
+                 articles.where(
+                   "positive_reactions_count < ? AND featured_number > ? AND score > ?", 2, 7.hours.ago.to_i, -2
+                 )
+               elsif state == "rising"
+                 articles.where(
+                   "positive_reactions_count > ? AND positive_reactions_count < ? AND featured_number > ?",
+                   19, 33, 3.days.ago.to_i
+                 )
+               else
+                 Article.none
+               end
+
+    articles.page(page).per(per_page || DEFAULT_PER_PAGE)
   end
 
   def collection_articles(collection_id)

app/views/additional_content_boxes/boxes.html.erb

@@ -5,6 +5,7 @@
              classification_text: "From one of our Community Sponsors",
              follow_cue: @boosted_article.organization&.tag_line || @boosted_article.organization&.tag_line %>
 <% end %>
+
 <% if @suggested_articles.any? %>
   <% @suggested_articles.each do |article| %>
     <%= render "additional_content_boxes/article_box",

spec/requests/additional_content_boxes_spec.rb

@@ -7,29 +7,29 @@ RSpec.describe "AdditionalContentBoxes", type: :request do
 
   describe "GET /additional_content_boxes" do
     it "returns an article if there is a published/featured one" do
-      suggestion = build_stubbed(:article, published: true, featured: true, path: "blah")
-      suggestion.title = "Title of the article"
-
-      fake = instance_double(Suggester::Articles::Classic)
-      allow(Suggester::Articles::Classic).to receive(:new).and_return(fake)
-      allow(fake).to receive(:get).and_return([suggestion])
+      suggestion = create(:article, published: true, published_at: 12.months.ago, featured: true, path: "blah")
+      suggestion.update(body_markdown: "foobar", title: "Title of the article", positive_reactions_count: 100)
 
       get "/additional_content_boxes?article_id=#{regular_article.id}&state=include_sponsors"
-      expect(response.body).to include CGI.escapeHTML(suggestion.title)
+      expect(response.body).to include(CGI.escapeHTML(suggestion.title))
     end
 
     it "returns no article if not published/featured" do
       suggestion = create(:article)
+
       get "/additional_content_boxes?article_id=#{regular_article.id}&state=include_sponsors"
-      expect(response.body).not_to include CGI.escapeHTML(suggestion.title)
+      expect(response.body).not_to include(CGI.escapeHTML(suggestion.title))
     end
 
     it "returns boosted article if available" do
       organization = create(:organization)
       create(:article, published: true, featured: true)
-      boosted_sugg = create(:article, tags: [tag.name], featured: true, boosted_additional_articles: true, organization_id: organization.id)
+      boosted_sugg = create(
+        :article, tags: [tag.name], featured: true, boosted_additional_articles: true, organization_id: organization.id
+      )
+
       get "/additional_content_boxes?article_id=#{regular_article.id}&state=include_sponsors"
-      expect(response.body).to include CGI.escapeHTML(boosted_sugg.title)
+      expect(response.body).to include(CGI.escapeHTML(boosted_sugg.title))
     end
 
     it "returns 422 status if article_id is missing" do

spec/requests/api/v0/articles_spec.rb

@@ -167,6 +167,37 @@ RSpec.describe "Api::V0::Articles", type: :request do
         expect(json_response.length).to eq(1)
       end
     end
+
+    context "with state param" do
+      it "returns fresh articles" do
+        article.update_columns(positive_reactions_count: 1, score: 1)
+
+        get api_articles_path(state: "fresh")
+        expect(response.parsed_body.size).to eq(1)
+      end
+
+      it "returns rising articles" do
+        article.update_columns(positive_reactions_count: 32, score: 1, featured_number: 2.days.ago.to_i)
+
+        get api_articles_path(state: "rising")
+        expect(response.parsed_body.size).to eq(1)
+      end
+
+      it "returns nothing if the state is unknown" do
+        get api_articles_path(state: "foobar")
+
+        expect(response.parsed_body).to be_empty
+      end
+
+      it "supports pagination" do
+        create_list(:article, 2, tags: "discuss", positive_reactions_count: 1, score: 1)
+
+        get api_articles_path(state: "fresh"), params: { page: 1, per_page: 2 }
+        expect(json_response.length).to eq(2)
+        get api_articles_path(state: "fresh"), params: { page: 2, per_page: 2 }
+        expect(json_response.length).to eq(1)
+      end
+    end
   end
 
   describe "GET /api/articles/:id" do