AO3-6328 Allow tags to be sorted by number of uses (#4632)

* AO3-6328 Allow tags to be sorted by number of uses

* AO3-6328 Added tests and fixed comparison bug

* AO3-6328 Added sort by uses code and tests

* AO3-6328 Fixed uses sort test and added default direction

* AO3-6328 Speed up test, fix test format, Hound recs

* AO3-6328 Hound fixes for tests, revert unrelated

* AO3-6328 Hound fix for instance variables, fix comment

* AO3-6328 Cleaned up rspec and added step to ensure reindex

* AO3-6328 Hound fix, removed unnecessary comments

* Made spacing more consistent with spec tests

* AO3-6328 Add tests for reindexing, fix IndexQueue key computing

* AO3-6328 Hound fixes

* AO3-6328 Use base_class in IndexQueue

* AO3-6328 Feedback fixes

* AO3-6328 Fix incorrect merge conflict resolution

* AO3-6328 Houndilocks

* AO3-6328 Fix outdated code and tests

* AO3-6328 Feedback

* AO3-6328 Fix broken step

* AO3-6328 hound

---------

Co-authored-by: tararosenthal <[email protected]>

app/controllers/application_controller.rb

@@ -485,7 +485,7 @@ def valid_sort_column(param, model='work')
     if model.to_s.downcase == 'work'
       allowed = %w(author title date created_at word_count hit_count)
     elsif model.to_s.downcase == 'tag'
-      allowed = %w(name created_at taggings_count_cache)
+      allowed = %w[name created_at taggings_count_cache uses]
     elsif model.to_s.downcase == 'collection'
       allowed = %w(collections.title collections.created_at)
     elsif model.to_s.downcase == 'prompt'

app/controllers/tags_controller.rb

@@ -409,6 +409,7 @@ def tag_search_params
       :type,
       :canonical,
       :created_at,
+      :uses,
       :sort_column,
       :sort_direction
     )

app/jobs/tag_count_update_job.rb

@@ -17,7 +17,7 @@ def perform_on_batch(tag_ids)
     Tag.transaction do
       tag_ids.each do |id|
         value = REDIS_GENERAL.get("tag_update_#{id}_value")
-        Tag.where(id: id).update_all(taggings_count_cache: value) if value.present?
+        Tag.where(id: id).update(taggings_count_cache: value) if value.present?
       end
     end
   end

app/models/indexing/index_queue.rb

@@ -12,6 +12,7 @@ def self.all
   end
 
   def self.get_key(klass, label)
+    klass = klass.is_a?(Class) ? klass.base_class : klass
     "index:#{klass.to_s.underscore}:#{label}"
   end
 

app/models/search/tag_query.rb

@@ -44,7 +44,7 @@ def per_page
   def sort
     direction = options[:sort_direction]&.downcase
     case options[:sort_column]
-    when "taggings_count_cache"
+    when "taggings_count_cache", "uses"
       column = "uses"
       direction ||= "desc"
     when "created_at"

app/models/search/tag_search_form.rb

@@ -11,6 +11,7 @@ class TagSearchForm
     :fandoms,
     :type,
     :created_at,
+    :uses,
     :sort_column,
     :sort_direction
   ]
@@ -53,11 +54,12 @@ def sort_direction
   def sort_options
     [
       %w[Name name],
-      ["Date Created", "created_at"]
+      ["Date Created", "created_at"],
+      %w[Uses uses]
     ]
   end
 
   def default_sort_direction
-    %w[created_at].include?(sort_column) ? "desc" : "asc"
+    %w[created_at uses].include?(sort_column) ? "desc" : "asc"
   end
 end

app/models/tag.rb

@@ -50,8 +50,17 @@ def taggings_count_cache_key
   end
 
   def write_taggings_to_redis(value)
+    # Atomically set the value while extracting the old value.
+    old_redis_value = REDIS_GENERAL.getset("tag_update_#{id}_value", value).to_i
+
+    # If the value hasn't changed from the saved version or the REDIS version,
+    # there's no need to write an update to the database, so let's just bail
+    # out.
+    return value if value == old_redis_value && value == taggings_count_cache
+
+    # If we've reached here, then the value has changed, and we need to make
+    # sure that the new value is written to the database.
     REDIS_GENERAL.sadd("tag_update", id)
-    REDIS_GENERAL.set("tag_update_#{id}_value", value)
     value
   end
 

features/step_definitions/tag_steps.rb

@@ -59,6 +59,23 @@
   end
 end
 
+Given "a set of tags for tag sort by use exists" do
+  {
+    "10 uses" => 10,
+    "8 uses" => 8,
+    "also 8 uses" => 8,
+    "5 uses" => 5,
+    "2 uses" => 2,
+    "0 uses" => 0
+  }.each do |freeform, uses|
+    tag = Freeform.find_or_create_by_name(freeform.dup)
+    tag.taggings_count = uses
+  end
+
+  step "all indexing jobs have been run"
+  step "the periodic tag count task is run"
+end
+
 Given /^I have a canonical "([^\"]*)" fandom tag named "([^\"]*)"$/ do |media, fandom|
   fandom = Fandom.find_or_create_by_name(fandom)
   fandom.update(canonical: true)

features/tags_and_wrangling/tag_search.feature

@@ -196,3 +196,27 @@ Feature: Search Tags
       And the 2nd tag result should contain "created second"
       And the 3rd tag result should contain "created third"
       And the 4th tag result should contain "created fourth"
+
+  Scenario: Search and sort by Uses in descending and ascending order
+    Given a set of tags for tag sort by use exists
+    When I am on the search tags page
+      And I fill in "Tag name" with "uses"
+      And I select "Uses" from "Sort by"
+      And I select "Descending" from "Sort direction"
+      And I press "Search Tags"
+    Then I should see "6 Found"
+      And the 1st tag result should contain "10 uses"
+      And the 2nd tag result should contain "8 uses"
+      And the 3rd tag result should contain "8 uses"
+      And the 4th tag result should contain "5 uses"
+      And the 5th tag result should contain "2 uses"
+      And the 6th tag result should contain "0 uses"
+    When I select "Ascending" from "Sort direction"
+      And I press "Search Tags"
+    Then I should see "6 Found"
+      And the 1st tag result should contain "0 uses"
+      And the 2nd tag result should contain "2 uses"
+      And the 3rd tag result should contain "5 uses"
+      And the 4th tag result should contain "8 uses"
+      And the 5th tag result should contain "8 uses"
+      And the 6th tag result should contain "10 uses"

spec/models/search/tag_query_spec.rb

@@ -191,5 +191,15 @@
       q = TagQuery.new(sort_column: "created_at", sort_direction: "asc")
       expect(q.generated_query[:sort]).to eq([{ "created_at" => { order: "asc", unmapped_type: "date" } }, { id: { order: "asc" } }])
     end
+
+    it "allows you to sort by Uses" do
+      q = TagQuery.new(sort_column: "uses")
+      expect(q.generated_query[:sort]).to eq([{ "uses" => { order: "desc" } }, { id: { order: "desc" } }])
+    end
+
+    it "allows you to sort by Uses in ascending order" do
+      q = TagQuery.new(sort_column: "uses", sort_direction: "asc")
+      expect(q.generated_query[:sort]).to eq([{ "uses" => { order: "asc" } }, { id: { order: "asc" } }])
+    end
   end
 end

spec/models/tag_spec.rb

@@ -77,6 +77,80 @@
         expect(@fandom_tag.taggings_count_cache).to eq 40 * ArchiveConfig.TAGGINGS_COUNT_CACHE_DIVISOR
       end
     end
+
+    describe "write_redis_to_database" do
+      let(:tag) { create(:fandom) }
+      let!(:work) { create(:work, fandom_string: tag.name) }
+
+      before do
+        RedisJobSpawner.perform_now("TagCountUpdateJob")
+        tag.reload
+      end
+
+      def expect_tag_update_flag_in_redis_to_be(flag)
+        expect(REDIS_GENERAL.sismember("tag_update", tag.id)).to eq flag
+      end
+
+      it "does not write to the database when reading the count" do
+        tag.taggings_count
+        expect_tag_update_flag_in_redis_to_be(false)
+      end
+
+      it "does not write to the database when assigning the same count" do
+        tag.taggings_count = 1
+        expect_tag_update_flag_in_redis_to_be(false)
+      end
+
+      it "writes to the database when assigning a new count" do
+        tag.taggings_count = 2
+        expect_tag_update_flag_in_redis_to_be(true)
+
+        RedisJobSpawner.perform_now("TagCountUpdateJob")
+        tag.reload
+
+        # Actual number of taggings has not changed though count cache has.
+        expect(tag.taggings_count_cache).to eq 2
+        expect(tag.taggings_count).to eq 1
+      end
+
+      it "writes to the database when adding a new work with the same tag" do
+        create(:work, fandom_string: tag.name)
+        expect_tag_update_flag_in_redis_to_be(true)
+
+        RedisJobSpawner.perform_now("TagCountUpdateJob")
+        tag.reload
+
+        expect(tag.taggings_count_cache).to eq 2
+        expect(tag.taggings_count).to eq 2
+      end
+
+      it "does not write to the database with a blank value" do
+        # Blank values will cause errors if assigned earlier due to division
+        # in taggings_count_expiry.
+        REDIS_GENERAL.set("tag_update_#{tag.id}_value", "")
+        REDIS_GENERAL.sadd("tag_update", tag.id)
+
+        RedisJobSpawner.perform_now("TagCountUpdateJob")
+
+        expect(tag.reload.taggings_count_cache).to eq 1
+      end
+
+      it "triggers reindexing of tags which aren't used much" do
+        create(:work, fandom_string: tag.name)
+
+        expect { RedisJobSpawner.perform_now("TagCountUpdateJob") }
+          .to add_to_reindex_queue(tag.reload, :main)
+      end
+
+      it "triggers reindexing of tags which are used significantly" do
+        ArchiveConfig.TAGGINGS_COUNT_MIN_CACHE_COUNT.times do
+          create(:work, fandom_string: tag.name)
+        end
+
+        expect { RedisJobSpawner.perform_now("TagCountUpdateJob") }
+          .to add_to_reindex_queue(tag.reload, :main)
+      end
+    end
   end
 
   it "should not be valid without a name" do