From f770462525493d2c9a5ea88d8930de5a8cce1652 Mon Sep 17 00:00:00 2001 From: Brian Austin Date: Fri, 27 Dec 2024 19:24:43 -0500 Subject: [PATCH 1/5] AO3-6450 Exclude hidden/unrevealed tags from bins --- app/models/tag.rb | 6 ++--- lib/tasks/after_tasks.rake | 21 +++++++++++++++ spec/lib/tasks/after_tasks.rake_spec.rb | 36 +++++++++++++++++++++++++ spec/models/tag_spec.rb | 27 ++++++++++--------- 4 files changed, 75 insertions(+), 15 deletions(-) diff --git a/app/models/tag.rb b/app/models/tag.rb index b8b694a5438..951afa2f875 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -630,9 +630,9 @@ def unfilterable? !(self.canonical? || self.unwrangleable? || self.merger_id.present? || self.mergers.any?) end - # Returns true if a tag has been used in posted works - def has_posted_works? - self.works.posted.any? + # Returns true if a tag has been used in posted works that are revealed and not hidden + def has_posted_works? # rubocop:disable Naming/PredicateName + self.works.posted.revealed.unhidden.any? end # sort tags by name diff --git a/lib/tasks/after_tasks.rake b/lib/tasks/after_tasks.rake index 97340982c25..02283682d1d 100644 --- a/lib/tasks/after_tasks.rake +++ b/lib/tasks/after_tasks.rake @@ -358,5 +358,26 @@ namespace :After do puts("Admin not found.") end end + + desc "Reindex tags associated with works that are hidden or unrevealed" + task(reindex_hidden_unrevealed_tags: :environment) do + hidden_count = Work.hidden.count + hidden_batches = (hidden_count + 999) / 1_000 + puts "Inspecting #{hidden_count} hidden works in #{hidden_batches} batches" + Work.hidden.find_in_batches.with_index do |batch, index| + batch.each { |work| work.tags.reindex_all } + puts "Finished batch #{index + 1} of #{hidden_batches}" + end + + unrevealed_count = Work.unrevealed.count + unrevealed_batches = (unrevealed_count + 999) / 1_000 + puts "Inspecting #{unrevealed_count} unrevealed works in #{unrevealed_batches} batches" + Work.unrevealed.find_in_batches.with_index do |batch, index| + batch.each { |work| work.tags.reindex_all } + puts "Finished batch #{index + 1} of #{unrevealed_batches}" + end + + puts "Finished reindexing tags on hidden and unrevealed works" + end # This is the end that you have to put new tasks above. end diff --git a/spec/lib/tasks/after_tasks.rake_spec.rb b/spec/lib/tasks/after_tasks.rake_spec.rb index d897ace40e2..13c787046e5 100644 --- a/spec/lib/tasks/after_tasks.rake_spec.rb +++ b/spec/lib/tasks/after_tasks.rake_spec.rb @@ -503,3 +503,39 @@ end end end + +describe "rake After:reindex_hidden_unrevealed_tags" do + context "with a posted work" do + let!(:work) { create(:work) } + + it "does not reindex the work's tags" do + expect do + subject.invoke + end.not_to add_to_reindex_queue(work.tags.first, :background) + end + end + + context "with a hidden work" do + let!(:work) { create(:work, hidden_by_admin: true) } + + it "reindexes the work's tags" do + expect do + subject.invoke + end.to add_to_reindex_queue(work.tags.first, :background) + end + end + + context "with an unrevealed work" do + let(:work) { create(:work) } + + before do + work.update!(in_unrevealed_collection: true) + end + + it "reindexes the work's tags" do + expect do + subject.invoke + end.to add_to_reindex_queue(work.tags.first, :background) + end + end +end diff --git a/spec/models/tag_spec.rb b/spec/models/tag_spec.rb index 340c2d87991..4d891a5599d 100644 --- a/spec/models/tag_spec.rb +++ b/spec/models/tag_spec.rb @@ -1,5 +1,4 @@ -# encoding: UTF-8 -require 'spec_helper' +require "spec_helper" describe Tag do after(:each) do @@ -66,7 +65,7 @@ end it "Writes to the database do not happen immediately" do - (1..40 * ArchiveConfig.TAGGINGS_COUNT_CACHE_DIVISOR - 1).each do |try| + (1..(40 * ArchiveConfig.TAGGINGS_COUNT_CACHE_DIVISOR) - 1).each do |try| @fandom_tag.taggings_count = try @fandom_tag.reload expect(@fandom_tag.taggings_count_cache).to eq 0 @@ -343,15 +342,19 @@ def expect_tag_update_flag_in_redis_to_be(flag) end describe "has_posted_works?" do - before do - create(:work, fandom_string: "love live,jjba") - create(:draft, fandom_string: "zombie land saga,jjba") - end - - it "is true if used in posted works" do - expect(Tag.find_by(name: "zombie land saga").has_posted_works?).to be_falsey - expect(Tag.find_by(name: "love live").has_posted_works?).to be_truthy - expect(Tag.find_by(name: "jjba").has_posted_works?).to be_truthy + { + "draft" => { posted: false }, + "unrevealed" => { in_unrevealed_collection: true }, + "hidden" => { hidden_by_admin: true } + }.each do |description, attributes| + it "is false if only used on #{description} works" do + create(:work, fandom_string: "love live,jjba") + non_visible_work = create(:work, fandom_string: "zombie land saga,jjba") + non_visible_work.update!(**attributes) + expect(Tag.find_by(name: "zombie land saga").has_posted_works?).to be_falsey + expect(Tag.find_by(name: "love live").has_posted_works?).to be_truthy + expect(Tag.find_by(name: "jjba").has_posted_works?).to be_truthy + end end end From ca56120b00873571def6c4ce45fe08ce9523ef01 Mon Sep 17 00:00:00 2001 From: Brian Austin Date: Fri, 27 Dec 2024 20:25:51 -0500 Subject: [PATCH 2/5] Reindex on hidden/revealed status change --- app/models/work.rb | 7 +++-- spec/models/work_spec.rb | 67 ++++++++++++++++++++++++++++++---------- 2 files changed, 54 insertions(+), 20 deletions(-) diff --git a/app/models/work.rb b/app/models/work.rb index 0881823f956..d028d622287 100755 --- a/app/models/work.rb +++ b/app/models/work.rb @@ -310,10 +310,11 @@ def should_reindex_pseuds? destroyed? || (saved_changes.keys & pertinent_attributes).present? end - # If the work gets posted, we should (potentially) reindex the tags, - # so they get the correct draft-only status. + # If the work gets posted, hidden, or (un)revealed, we should (potentially) reindex the tags, + # so they get the correct visibility status. def update_tag_index - return unless saved_change_to_posted? + return unless saved_change_to_posted? || saved_change_to_hidden_by_admin? || saved_change_to_in_unrevealed_collection? + taggings.each(&:update_search) end diff --git a/spec/models/work_spec.rb b/spec/models/work_spec.rb index 51d989108e8..ad54b4ea7f7 100644 --- a/spec/models/work_spec.rb +++ b/spec/models/work_spec.rb @@ -1,4 +1,4 @@ -require 'spec_helper' +require "spec_helper" describe Work do # see lib/collectible_spec for collection-related tests @@ -10,16 +10,17 @@ context "when posted" do it "posts the first chapter" do work = create(:work) - work.first_chapter.posted.should == true + expect(work.first_chapter.posted).to eq true end end context "create_stat_counter" do it "creates a stat counter for that work id" do - expect { + expect do @work = build(:work) @work.save! - }.to change{ StatCounter.all.count }.by(1) + end.to change { StatCounter.all.count } + .by(1) expect(StatCounter.where(work_id: @work.id)).to exist end end @@ -48,10 +49,10 @@ let(:too_short) { ArchiveConfig.TITLE_MIN - 1 } it "errors if the title without leading spaces is shorter than #{ArchiveConfig.TITLE_MIN}" do - expect { + expect do @work = create(:work, title: " #{too_short}") @work.reload - }.to raise_error(ActiveRecord::RecordInvalid,"Validation failed: Title must be at least #{ArchiveConfig.TITLE_MIN} characters long without leading spaces.") + end.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: Title must be at least #{ArchiveConfig.TITLE_MIN} characters long without leading spaces.") end # Reset the min characters in the title, so that the factory is valid @@ -121,6 +122,34 @@ end end + describe "reindexing" do + let!(:work) { create(:work) } + + context "when draft status is changed" do + it "enqueues tags for reindex" do + expect do + work.update!(posted: false) + end.to add_to_reindex_queue(work.fandoms.last, :main) + end + end + + context "when hidden by an admin" do + it "enqueues tags for reindex" do + expect do + work.update!(hidden_by_admin: true) + end.to add_to_reindex_queue(work.fandoms.last, :main) + end + end + + context "when put in an unrevealed collection" do + it "enqueues tags for reindex" do + expect do + work.update!(in_unrevealed_collection: true) + end.to add_to_reindex_queue(work.fandoms.last, :main) + end + end + end + describe "#crossover" do it "is not crossover with one fandom" do fandom = create(:canonical_fandom, name: "nge") @@ -392,7 +421,7 @@ let(:work) { build(:work) } before do - work.recipients = recipient1 + "," + recipient2 + work.recipients = "#{recipient1},#{recipient2}" end it "contains gifts for the same recipients when they are first added" do @@ -410,7 +439,7 @@ end it "only contains one gift if the same recipient is entered twice" do - work.recipients = recipient2 + "," + recipient2 + work.recipients = "#{recipient2},#{recipient2}" expect(work.new_gifts.collect(&:recipient)).to eq([recipient2]) end end @@ -511,8 +540,9 @@ @admin_setting.update_attribute(:hide_spam, true) end it "automatically hides spam works and sends an email" do - expect { @work.update!(spam: true) }. - to change { ActionMailer::Base.deliveries.count }.by(1) + expect { @work.update!(spam: true) } + .to change { ActionMailer::Base.deliveries.count } + .by(1) expect(@work.reload.hidden_by_admin).to be_truthy expect(ActionMailer::Base.deliveries.last.subject).to eq("[AO3] Your work was hidden as spam") end @@ -522,8 +552,8 @@ @admin_setting.update_attribute(:hide_spam, false) end it "does not automatically hide spam works and does not send an email" do - expect { @work.update!(spam: true) }. - not_to change { ActionMailer::Base.deliveries.count } + expect { @work.update!(spam: true) } + .not_to change { ActionMailer::Base.deliveries.count } expect(@work.reload.hidden_by_admin).to be_falsey end end @@ -580,9 +610,10 @@ before { work.reload } it "raises an error" do - expect { work.remove_author(to_remove) }.to raise_exception( - "Sorry, we can't remove all creators of a work." - ) + expect { work.remove_author(to_remove) } + .to raise_exception( + "Sorry, we can't remove all creators of a work." + ) end end @@ -610,7 +641,8 @@ let(:work) { create(:work) } it "does not save an original creator record" do - expect { work.destroy }.not_to change { WorkOriginalCreator.count } + expect { work.destroy } + .not_to change { WorkOriginalCreator.count } end context "when an original creator exists" do @@ -618,7 +650,8 @@ it "deletes the original creator" do work.destroy - expect { original_creator.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { original_creator.reload } + .to raise_error(ActiveRecord::RecordNotFound) end end end From 9344d519da82c299a3bccc6e8e1a21239b4d78dd Mon Sep 17 00:00:00 2001 From: Brian Austin Date: Fri, 27 Dec 2024 20:28:57 -0500 Subject: [PATCH 3/5] Reindex task using update_search instead of reindex_all --- lib/tasks/after_tasks.rake | 4 ++-- spec/lib/tasks/after_tasks.rake_spec.rb | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/tasks/after_tasks.rake b/lib/tasks/after_tasks.rake index 02283682d1d..38b1cf4f7f2 100644 --- a/lib/tasks/after_tasks.rake +++ b/lib/tasks/after_tasks.rake @@ -365,7 +365,7 @@ namespace :After do hidden_batches = (hidden_count + 999) / 1_000 puts "Inspecting #{hidden_count} hidden works in #{hidden_batches} batches" Work.hidden.find_in_batches.with_index do |batch, index| - batch.each { |work| work.tags.reindex_all } + batch.each { |work| work.taggings.each(&:update_search) } puts "Finished batch #{index + 1} of #{hidden_batches}" end @@ -373,7 +373,7 @@ namespace :After do unrevealed_batches = (unrevealed_count + 999) / 1_000 puts "Inspecting #{unrevealed_count} unrevealed works in #{unrevealed_batches} batches" Work.unrevealed.find_in_batches.with_index do |batch, index| - batch.each { |work| work.tags.reindex_all } + batch.each { |work| work.taggings.each(&:update_search) } puts "Finished batch #{index + 1} of #{unrevealed_batches}" end diff --git a/spec/lib/tasks/after_tasks.rake_spec.rb b/spec/lib/tasks/after_tasks.rake_spec.rb index 13c787046e5..e36778598be 100644 --- a/spec/lib/tasks/after_tasks.rake_spec.rb +++ b/spec/lib/tasks/after_tasks.rake_spec.rb @@ -511,7 +511,7 @@ it "does not reindex the work's tags" do expect do subject.invoke - end.not_to add_to_reindex_queue(work.tags.first, :background) + end.not_to add_to_reindex_queue(work.tags.first, :main) end end @@ -521,7 +521,7 @@ it "reindexes the work's tags" do expect do subject.invoke - end.to add_to_reindex_queue(work.tags.first, :background) + end.to add_to_reindex_queue(work.tags.first, :main) end end @@ -535,7 +535,7 @@ it "reindexes the work's tags" do expect do subject.invoke - end.to add_to_reindex_queue(work.tags.first, :background) + end.to add_to_reindex_queue(work.tags.first, :main) end end end From ea03d30e3fc6626dc02e3c249cd826cd03fef4fa Mon Sep 17 00:00:00 2001 From: Brian Austin Date: Fri, 3 Jan 2025 19:27:14 -0500 Subject: [PATCH 4/5] Update comment --- app/models/work.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/work.rb b/app/models/work.rb index d028d622287..d6a87ed6301 100755 --- a/app/models/work.rb +++ b/app/models/work.rb @@ -310,7 +310,7 @@ def should_reindex_pseuds? destroyed? || (saved_changes.keys & pertinent_attributes).present? end - # If the work gets posted, hidden, or (un)revealed, we should (potentially) reindex the tags, + # If the work gets posted, (un)hidden, or (un)revealed, we should (potentially) reindex the tags, # so they get the correct visibility status. def update_tag_index return unless saved_change_to_posted? || saved_change_to_hidden_by_admin? || saved_change_to_in_unrevealed_collection? From e9bb7b58247f54f6229d0ed836f02937d14b2c2a Mon Sep 17 00:00:00 2001 From: Brian Austin Date: Thu, 9 Jan 2025 15:18:54 +0000 Subject: [PATCH 5/5] Add feature tests --- .../tag_wrangling_more.feature | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/features/tags_and_wrangling/tag_wrangling_more.feature b/features/tags_and_wrangling/tag_wrangling_more.feature index 95dbaca5136..424d488461f 100644 --- a/features/tags_and_wrangling/tag_wrangling_more.feature +++ b/features/tags_and_wrangling/tag_wrangling_more.feature @@ -336,3 +336,67 @@ Feature: Tag wrangling: assigning wranglers, using the filters on the Wranglers Then I should not see "bookmark char tag" When I follow "Relationships by fandom (0)" Then I should not see "bookmark rel tag" + + Scenario: Tags from unrevealed works don't show in unwrangled bins + Given a canonical fandom "Testing" + And I have the hidden collection "Unrevealed Tags" + And I am logged in as a tag wrangler + When I post the work "Hello There" with fandom "Testing" with character "unrevealed char" in the collection "Unrevealed Tags" + Given the periodic tag count task is run + And all indexing jobs have been run + And I flush the wrangling sidebar caches + When I view the unwrangled character bin for "Testing" + Then I should not see "unrevealed char" + + Scenario: Tags from unrevealed works appear in unwrangled bins when the work is revealed + Given a canonical fandom "Testing" + And I have the hidden collection "Unrevealed Tags" + And I am logged in as a tag wrangler + When I post the work "Hello There" with fandom "Testing" with character "unrevealed char" in the collection "Unrevealed Tags" + Given the periodic tag count task is run + And all indexing jobs have been run + And I flush the wrangling sidebar caches + When I view the unwrangled character bin for "Testing" + Then I should not see "unrevealed char" + When I reveal works for "Unrevealed Tags" + Given the periodic tag count task is run + And all indexing jobs have been run + And I flush the wrangling sidebar caches + And I am logged in as a tag wrangler + When I view the unwrangled character bin for "Testing" + Then I should see "unrevealed char" + + Scenario: Tags from hidden works don't appear in unwrangled bins + Given a canonical fandom "Testing" + And I am logged in as a tag wrangler + When I post the work "Hello There" with fandom "Testing" with character "hidden char" + When I am logged in as a super admin + And I hide the work "Hello There" + Given the periodic tag count task is run + And all indexing jobs have been run + And I flush the wrangling sidebar caches + And I am logged in as a tag wrangler + When I view the unwrangled character bin for "Testing" + Then I should not see "hidden char" + + Scenario: Tags from hidden works appear in unwrangled bins when the work is un-hidden + Given a canonical fandom "Testing" + And I am logged in as a tag wrangler + When I post the work "Hello There" with fandom "Testing" with character "hidden char" + When I am logged in as a super admin + And I hide the work "Hello There" + Given the periodic tag count task is run + And all indexing jobs have been run + And I flush the wrangling sidebar caches + And I am logged in as a tag wrangler + When I view the unwrangled character bin for "Testing" + Then I should not see "hidden char" + When I am logged in as a super admin + And I view the work "Hello There" + And I follow "Make Work Visible" + Given the periodic tag count task is run + And all indexing jobs have been run + And I flush the wrangling sidebar caches + And I am logged in as a tag wrangler + When I view the unwrangled character bin for "Testing" + Then I should see "hidden char"