AO3-5825 Cache work and bookmark counts on dashboard sidebar (#3695)

* AO3-5825 Cache work and bookmark counts on dashboard sidebar

This cuts down on the number of Elasticsearch count queries.

Unlike cached search results in WorksController, cached keys
for the counts are not timestamped and cannot be invalidated by
e.g. WorksOwner#update_works_index_timestamp. The counts will lag
behind the actual search results.

* AO3-5825 Remove print_ prefix in related sidebar helpers

* AO3-5825 Add race_condition_ttl

* AO3-5825 Fix style

* AO3-5825 Add comment on alternative cache key implementation

app/helpers/users_helper.rb

@@ -73,33 +73,36 @@ def print_coauthors(user)
 
   # Prints link to bookmarks page with user-appropriate number of bookmarks
   # (The total should reflect the number of bookmarks the user can actually see.)
-  def print_bookmarks_link(user, pseud = nil)
-    return print_pseud_bookmarks_link(pseud) if pseud.present? && !pseud.new_record?
+  def bookmarks_link(user, pseud = nil)
+    return pseud_bookmarks_link(pseud) if pseud.present? && !pseud.new_record?
+
     total = BookmarkSearchForm.count_for_user(user)
     span_if_current ts('Bookmarks (%{bookmark_number})', bookmark_number: total.to_s), user_bookmarks_path(@user)
   end
 
-  def print_pseud_bookmarks_link(pseud)
-    total = BookmarkSearchForm.count_for_pseuds([pseud])
+  def pseud_bookmarks_link(pseud)
+    total = BookmarkSearchForm.count_for_pseud(pseud)
     span_if_current ts('Bookmarks (%{bookmark_number})', bookmark_number: total.to_s), user_pseud_bookmarks_path(@user, pseud)
   end
 
   # Prints link to works page with user-appropriate number of works
   # (The total should reflect the number of works the user can actually see.)
-  def print_works_link(user, pseud = nil)
-    return print_pseud_works_link(pseud) if pseud.present? && !pseud.new_record?
-    total = WorkSearchForm.user_count(user)
+  def works_link(user, pseud = nil)
+    return pseud_works_link(pseud) if pseud.present? && !pseud.new_record?
+
+    total = WorkSearchForm.count_for_user(user)
     span_if_current ts('Works (%{works_number})', works_number: total.to_s), user_works_path(@user)
   end
 
-  def print_pseud_works_link(pseud)
-    total = WorkSearchForm.pseud_count(pseud)
+  def pseud_works_link(pseud)
+    total = WorkSearchForm.count_for_pseud(pseud)
     span_if_current ts('Works (%{works_number})', works_number: total.to_s), user_pseud_works_path(@user, pseud)
   end
 
   # Prints link to series page with user-appropriate number of series
-  def print_series_link(user, pseud = nil)
-    return print_pseud_series_link(pseud) if pseud.present? && !pseud.new_record?
+  def series_link(user, pseud = nil)
+    return pseud_series_link(pseud) if pseud.present? && !pseud.new_record?
+
     if current_user.nil?
       total = Series.visible_to_all.exclude_anonymous.for_pseuds(user.pseuds).length
     else
@@ -108,7 +111,7 @@ def print_series_link(user, pseud = nil)
     span_if_current ts('Series (%{series_number})', series_number: total.to_s), user_series_index_path(@user)
   end
 
-  def print_pseud_series_link(pseud)
+  def pseud_series_link(pseud)
     if current_user.nil?
       total = Series.visible_to_all.exclude_anonymous.for_pseuds([pseud]).length
     else
@@ -117,7 +120,7 @@ def print_pseud_series_link(pseud)
     span_if_current ts('Series (%{series_number})', series_number: total.to_s), user_pseud_series_index_path(@user, pseud)
   end
 
-  def print_gifts_link(user)
+  def gifts_link(user)
     if current_user.nil?
       gift_number = user.gift_works.visible_to_all.distinct.count
     else
@@ -137,11 +140,6 @@ def authored_items(pseud, work_counts = {}, rec_counts = {})
     items.html_safe
   end
 
-  #  def print_pseud_drafts_link(pseud)
-  #    total = pseud.unposted_works.size
-  #    link_to_unless_current t('my_drafts', default:"Drafts") + " (#{total})", drafts_user_pseud_works_path(@user, pseud)
-  #  end
-
   def authors_header(collection, what = 'People')
     if collection.total_pages < 2
       case collection.size

app/models/search/bookmark_search_form.rb

@@ -48,21 +48,6 @@ class BookmarkSearchForm
 
   attr_accessor :options
 
-  def self.count_for_user(user)
-    BookmarkQuery.new(
-      user_ids: [user.id],
-      show_private: User.current_user.is_a?(Admin) || user == User.current_user
-    ).count
-  end
-
-  def self.count_for_pseuds(pseuds)
-    BookmarkQuery.new(
-      pseud_ids: pseuds.map(&:id),
-      show_private: User.current_user.is_a?(Admin) ||
-                    pseuds.map(&:user).uniq == [User.current_user]
-    ).count
-  end
-
   ATTRIBUTES.each do |filterable|
     define_method(filterable) { options[filterable] }
   end
@@ -169,6 +154,39 @@ def sort_direction(sort_column)
     'desc'
   end
 
+  ###############
+  # COUNTING
+  ###############
+
+  def self.count_for_user(user)
+    show_private = User.current_user.is_a?(Admin) || user == User.current_user
+
+    Rails.cache.fetch(count_cache_key(user, show_private), count_cache_options) do
+      BookmarkQuery.new(user_ids: [user.id], show_private: show_private).count
+    end
+  end
+
+  def self.count_for_pseud(pseud)
+    show_private = User.current_user.is_a?(Admin) || pseud.user == User.current_user
+
+    Rails.cache.fetch(count_cache_key(pseud, show_private), count_cache_options) do
+      BookmarkQuery.new(pseud_ids: [pseud.id], show_private: show_private).count
+    end
+  end
+
+  def self.count_cache_key(owner, show_private)
+    status = User.current_user ? "logged_in" : "logged_out"
+    status << "_private" if show_private
+    "bookmark_count_#{owner.class.name.underscore}_#{owner.id}_#{status}"
+  end
+
+  def self.count_cache_options
+    {
+      expires_in: ArchiveConfig.SECONDS_UNTIL_DASHBOARD_COUNTS_EXPIRE.seconds,
+      race_condition_ttl: 10.seconds
+    }
+  end
+
   private
 
   def processed_options(opts = {})

app/models/search/work_search_form.rb

@@ -49,32 +49,6 @@ class WorkSearchForm
 
   attr_accessor :options
 
-  # Make a direct request to the elasticsearch count api
-  def self.count_for_user(user)
-    WorkQuery.new(user_ids: [user.id]).count
-  end
-
-  def self.count_for_pseuds(pseuds)
-    WorkQuery.new(pseud_ids: pseuds.map(&:id)).count
-  end
-
-  def self.user_count(user)
-    cached_count(user) || count_for_user(user)
-  end
-
-  def self.pseud_count(pseud)
-    cached_count(pseud) || count_for_pseuds([pseud])
-  end
-
-  def self.cached_count(owner)
-    status = User.current_user ? 'logged_in' : 'logged_out'
-    key = "#{owner.works_index_cache_key}_#{status}_page"
-    works = Rails.cache.read(key)
-    if works.present?
-      works.total_entries
-    end
-  end
-
   ATTRIBUTES.each do |filterable|
     define_method(filterable) { options[filterable] }
   end
@@ -247,4 +221,35 @@ def default_sort_direction
     end
   end
 
+  ###############
+  # COUNTING
+  ###############
+
+  def self.count_for_user(user)
+    Rails.cache.fetch(count_cache_key(user), count_cache_options) do
+      WorkQuery.new(user_ids: [user.id]).count
+    end
+  end
+
+  def self.count_for_pseud(pseud)
+    Rails.cache.fetch(count_cache_key(pseud), count_cache_options) do
+      WorkQuery.new(pseud_ids: [pseud.id]).count
+    end
+  end
+
+  # If we want to invalidate cached work counts whenever the owner (which for
+  # this method can only be a user or a pseud) has a new work, we can use
+  # "#{owner.works_index_cache_key}" instead of "#{owner.class.name.underscore}_#{owner.id}".
+  # See lib/works_owner.rb.
+  def self.count_cache_key(owner)
+    status = User.current_user ? 'logged_in' : 'logged_out'
+    "work_count_#{owner.class.name.underscore}_#{owner.id}_#{status}"
+  end
+
+  def self.count_cache_options
+    {
+      expires_in: ArchiveConfig.SECONDS_UNTIL_DASHBOARD_COUNTS_EXPIRE.seconds,
+      race_condition_ttl: 10.seconds
+    }
+  end
 end

app/views/users/_contents.html.erb

@@ -42,11 +42,11 @@
     </ul>
     <% if @pseud %>
       <% if @pseud.works.count > ArchiveConfig.NUMBER_OF_ITEMS_VISIBLE_IN_DASHBOARD %>
-        <ul class="actions" role="navigation"><li><%= print_pseud_works_link(@pseud) %></li>
+        <ul class="actions" role="navigation"><li><%= pseud_works_link(@pseud) %></li>
       <% end %>
     <% else %>   
       <% if @user.works.size > ArchiveConfig.NUMBER_OF_ITEMS_VISIBLE_IN_DASHBOARD %>
-        <ul class="actions" role="navigation"><li><%= print_works_link(@user) %></li></ul>
+        <ul class="actions" role="navigation"><li><%= works_link(@user) %></li></ul>
       <% end %>
     <% end %>
   </div>
@@ -60,11 +60,11 @@
     </ol>
     <% if @pseud %>
       <% if @pseud.series.count > ArchiveConfig.NUMBER_OF_ITEMS_VISIBLE_IN_DASHBOARD %>
-        <ul class="actions" role="navigation"><li><%= print_pseud_series_link(@pseud) %></li></ul>
+        <ul class="actions" role="navigation"><li><%= pseud_series_link(@pseud) %></li></ul>
       <% end %>
     <% else %>
       <% if @user.series.size > ArchiveConfig.NUMBER_OF_ITEMS_VISIBLE_IN_DASHBOARD %>
-        <ul class="actions" role="navigation"><li><%= print_series_link(@user) %></li></ul>
+        <ul class="actions" role="navigation"><li><%= series_link(@user) %></li></ul>
       <% end %>
     <% end %> 
    </div>
@@ -79,11 +79,11 @@
     <% unless @user == User.orphan_account %>
       <% if @pseud %>
         <% if @pseud.bookmarks.visible.size > ArchiveConfig.NUMBER_OF_ITEMS_VISIBLE_IN_DASHBOARD %>
-          <ul class="actions" role="navigation"><li><%= print_pseud_bookmarks_link(@pseud) %></li></ul>
+          <ul class="actions" role="navigation"><li><%= pseud_bookmarks_link(@pseud) %></li></ul>
         <% end %>
       <% else %>
         <% if @user.bookmarks.visible.size > ArchiveConfig.NUMBER_OF_ITEMS_VISIBLE_IN_DASHBOARD %>
-          <ul class="actions" role="navigation"><li><%= print_bookmarks_link(@user) %></li></ul>
+          <ul class="actions" role="navigation"><li><%= bookmarks_link(@user) %></li></ul>
         <% end %>
       <% end %> 
     <% end %>

app/views/users/_sidebar.html.erb

@@ -19,23 +19,23 @@
     <li><%= span_if_current ts("Skins"), user_skins_path(@user) %></li>
   <% end %>
 </ul>
-	
-	
+
+
 <h4 class="landmark heading"><%= ts("Pitch")%></h4>
 <ul class="navigation actions">
-	<li><%= print_works_link(@user, @pseud) %></li>
+	<li><%= works_link(@user, @pseud) %></li>
 	<% if @user == current_user || logged_in_as_admin? %>
 	  <li><%= span_if_current ts("Drafts") + " (#{@user.unposted_works.size})", drafts_user_works_path(@user) %></li>
   <% end %>
-	<li><%= print_series_link(@user, @pseud) %></li>
+	<li><%= series_link(@user, @pseud) %></li>
 
 	<% unless @user == User.orphan_account %>
-	  <li><%= print_bookmarks_link(@user, @pseud) %></li>
+	  <li><%= bookmarks_link(@user, @pseud) %></li>
 	<% end %>
 
   <li><%= span_if_current ts("Collections (%{coll_number})", :coll_number => @user.maintained_collections.count), user_collections_path(@user) %></li>
 </ul>
-	
+
 <% if @user == current_user %>
   <h4 class="landmark heading"><%= ts("Catch")%></h4>
   <ul class="navigation actions">
@@ -46,8 +46,8 @@
     <% end %>
     <li><%= span_if_current ts("Subscriptions"), user_subscriptions_path(@user) %></li>
   </ul>
-<% end %>	
-	
+<% end %>
+
 <h4 class="landmark heading"><%= ts("Switch")%></h4>
 <ul class="navigation actions">
   <% if @user == current_user %>
@@ -59,8 +59,7 @@
     <li><%= span_if_current ts("Claims (%{claim_number})", :claim_number => (@user.request_claims.unposted.count)), user_claims_path(@user) %></li>
     <li><%= span_if_current ts("Related Works (%{related_works_number})", :related_works_number => (@user.related_works.posted.count + @user.parent_work_relationships.count)), user_related_works_path(@user) %></li>
   <% end %>
-  <li><%= print_gifts_link(@user) %></li> 
+  <li><%= gifts_link(@user) %></li>
 </ul>
 
 </div>
-

config/config.yml

@@ -386,8 +386,9 @@ HELP_DIRECTORY: '/help'
 #PRODUCTION_CACHE: "memory"
 PRODUCTION_CACHE: "memcache"
 
-# how long we cache autocomplete search results in minutes
-AUTOCOMPLETE_EXPIRATION_TIME: 10
+# TTL in seconds for cached work and bookmark counts on user/pseud dashboards.
+# Default to 20 minutes.
+SECONDS_UNTIL_DASHBOARD_COUNTS_EXPIRE: 1200
 
 # how many cache pages to expire for a tag/collection/pseud when a work is updated or deleted
 PAGES_TO_CACHE: 5

features/bookmarks/bookmark_privacy.feature

@@ -7,7 +7,8 @@ Feature: Private bookmarks
   @disable_caching
   Scenario: private bookmarks on public and restricted works
 
-    Given a canonical fandom "Stargate SG-1"
+    Given dashboard counts expire after 10 seconds
+      And a canonical fandom "Stargate SG-1"
       And I am logged in as "workauthor"
       And I post the locked work "Secret Masterpiece"
       And I post the work "Public Masterpiece"
@@ -51,10 +52,13 @@ Feature: Private bookmarks
       And I should not see "Another Masterpiece"
     When I am on avid_bookmarker's bookmarks page
     Then I should see "3 Bookmarks by avid_bookmarker"
-      And I should see "Bookmarks (3)"
+      And I should see "Bookmarks (0)"
       And I should see "Public Masterpiece"
       And I should see "Secret Masterpiece"
       And I should see "Another Masterpiece"
+    When I wait 11 seconds
+      And I reload the page
+    Then I should see "Bookmarks (3)"
     When I go to the bookmarks page for user "avid_bookmarker" with pseud "infrequent_bookmarker"
     Then I should see "1 Bookmark by infrequent_bookmarker (avid_bookmarker)"
       And I should see "Bookmarks (1)"

features/step_definitions/search_steps.rb

@@ -43,3 +43,8 @@
   stub_const("ArchiveConfig", OpenStruct.new(ArchiveConfig))
   ArchiveConfig.TAGS_PER_SEARCH_PAGE = per_page.to_i
 end
+
+Given /^dashboard counts expire after (\d+) seconds?$/ do |seconds|
+  stub_const("ArchiveConfig", OpenStruct.new(ArchiveConfig))
+  ArchiveConfig.SECONDS_UNTIL_DASHBOARD_COUNTS_EXPIRE = seconds.to_i
+end