AO3-6450 Exclude hidden/unrevealed tags from bins (#5006)

* AO3-6450 Exclude hidden/unrevealed tags from bins

* Reindex on hidden/revealed status change

* Reindex task using update_search instead of reindex_all

* Update comment

* Add feature tests

app/models/tag.rb

@@ -633,9 +633,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

app/models/work.rb

@@ -313,10 +313,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, (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?
+    return unless saved_change_to_posted? || saved_change_to_hidden_by_admin? || saved_change_to_in_unrevealed_collection?
+
     taggings.each(&:update_search)
   end
 

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"

lib/tasks/after_tasks.rake

@@ -496,5 +496,26 @@ namespace :After do
     count = ChallengeAssignment.where("pinch_request_signup_id IS NOT NULL AND request_signup_id IS NULL").update_all("request_signup_id = pinch_request_signup_id")
     puts("Migrated pinch_request_signup for #{count} challenge assignments.")
   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.taggings.each(&:update_search) }
+      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.taggings.each(&:update_search) }
+      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

spec/lib/tasks/after_tasks.rake_spec.rb

@@ -472,3 +472,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, :main)
+    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, :main)
+    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, :main)
+    end
+  end
+end

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
@@ -376,15 +375,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
 

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,15 +641,17 @@
     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
       let!(:original_creator) { create(:work_original_creator, work: work) }
 
       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