From e611623379925c72c5d73bf8fd024ac549f20675 Mon Sep 17 00:00:00 2001 From: weeklies <80141759+weeklies@users.noreply.github.com> Date: Mon, 2 Oct 2023 16:45:42 +0800 Subject: [PATCH] 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 --- app/controllers/application_controller.rb | 2 +- app/controllers/tags_controller.rb | 1 + app/jobs/tag_count_update_job.rb | 2 +- app/models/indexing/index_queue.rb | 1 + app/models/search/tag_query.rb | 2 +- app/models/search/tag_search_form.rb | 6 +- app/models/tag.rb | 11 ++- features/step_definitions/tag_steps.rb | 17 +++++ .../tags_and_wrangling/tag_search.feature | 24 ++++++ spec/models/search/tag_query_spec.rb | 10 +++ spec/models/tag_spec.rb | 74 +++++++++++++++++++ 11 files changed, 144 insertions(+), 6 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index ea852588005..59c49d4ef59 100644 --- a/app/controllers/application_controller.rb +++ b/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' diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 1fac4822833..f7ae2d2b6e9 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -409,6 +409,7 @@ def tag_search_params :type, :canonical, :created_at, + :uses, :sort_column, :sort_direction ) diff --git a/app/jobs/tag_count_update_job.rb b/app/jobs/tag_count_update_job.rb index 87f8e1e139b..15007d2e8d2 100644 --- a/app/jobs/tag_count_update_job.rb +++ b/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 diff --git a/app/models/indexing/index_queue.rb b/app/models/indexing/index_queue.rb index e9ffe3684e3..c1d61cbf913 100644 --- a/app/models/indexing/index_queue.rb +++ b/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 diff --git a/app/models/search/tag_query.rb b/app/models/search/tag_query.rb index 8b90ee7af8a..1e83dab1f7c 100644 --- a/app/models/search/tag_query.rb +++ b/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" diff --git a/app/models/search/tag_search_form.rb b/app/models/search/tag_search_form.rb index df04dc1832c..c3bb4dae569 100644 --- a/app/models/search/tag_search_form.rb +++ b/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 diff --git a/app/models/tag.rb b/app/models/tag.rb index 24e979d29cb..7f7e5edca3c 100644 --- a/app/models/tag.rb +++ b/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 diff --git a/features/step_definitions/tag_steps.rb b/features/step_definitions/tag_steps.rb index 3e3b3d247cc..99704c48311 100644 --- a/features/step_definitions/tag_steps.rb +++ b/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) diff --git a/features/tags_and_wrangling/tag_search.feature b/features/tags_and_wrangling/tag_search.feature index a8952d47291..a265a293626 100644 --- a/features/tags_and_wrangling/tag_search.feature +++ b/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" diff --git a/spec/models/search/tag_query_spec.rb b/spec/models/search/tag_query_spec.rb index 7ef558b0255..b0e6f4c1b48 100644 --- a/spec/models/search/tag_query_spec.rb +++ b/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 diff --git a/spec/models/tag_spec.rb b/spec/models/tag_spec.rb index 7270549f734..16daf57b9bb 100644 --- a/spec/models/tag_spec.rb +++ b/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