posts/index: fix several "This tag is under discussion" issues.

Several fixes for the "This tag is under discussion" notice on the post
index page:

* Fix the notice appearing for BURs that aren't pending.
* Fix the notice never going away because of the cache never expiring.
* List all topics when a tag is involved in multiple BURs.
* Link to the forum post instead of the forum topic (fix #4421).
* Optimization: don't check for BURs when the search isn't a simple
  single tag search.
* Add a `tags` field to the bulk update requests table for tracking all
  tags involved in the request (excluding tags in mass updates that are
  negated/optional/wildcards). Known issue: doesn't handle tag type
  prefixes in mass updates correctly (e.g. `mass update foo -> artist:bar`
  doesn't detect the tag `bar`).
* Allow searching the /bulk_update_requests page by tags.

We don't really need to cache the notice here, but we do it anyway to
reduce queries on the post index page.

app/helpers/posts_helper.rb

@@ -62,10 +62,6 @@ module PostsHelper
     return params[:pool_id].to_i == pool.id
   end
 
-  def show_tag_change_notice?
-    CurrentUser.user.is_member? && PostQueryBuilder.new(params[:tags]).split_query.size == 1 && TagChangeNoticeService.get_forum_topic_id(params[:tags])
-  end
-
   private
 
   def nav_params_for(page)

app/logical/alias_and_implication_importer.rb

@@ -79,27 +79,17 @@ class AliasAndImplicationImporter
   end
 
   def affected_tags
-    tokens = self.class.tokenize(text)
+    AliasAndImplicationImporter.tokenize(text).flat_map do |type, *args|
-    tokens.inject([]) do |all, token|
+      case type
-      case token[0]
       when :create_alias, :remove_alias, :create_implication, :remove_implication
-        all << token[1]
+        [args[0], args[1]]
-        all << token[2]
-        all
-
       when :mass_update
-        all += PostQueryBuilder.new(token[1]).split_query
+        tags = PostQueryBuilder.new(args[0]).tags + PostQueryBuilder.new(args[1]).tags
-        all += PostQueryBuilder.new(token[2]).parse_tag_edit
+        tags.reject(&:negated).reject(&:optional).reject(&:wildcard).map(&:name)
-        all
-
       when :change_category
-        all << token[1]
+        args[0]
-        all
-
-      else
-        all
       end
-    end
+    end.sort.uniq
   end
 
   private

app/logical/post_sets/post.rb

@@ -131,5 +131,10 @@ module PostSets
       # be smarter about this in the future
       posts.reject(&:is_deleted).select(&:visible?).max_by(&:fav_count)
     end
+
+    def pending_bulk_update_requests
+      return BulkUpdateRequest.none unless query.is_simple_tag?
+      @pending_bulk_update_requests ||= BulkUpdateRequest.pending.where_array_includes_any(:tags, query.tags.first.name)
+    end
   end
 end

app/logical/tag_change_notice_service.rb

@@ -1,13 +0,0 @@
-module TagChangeNoticeService
-  module_function
-
-  def get_forum_topic_id(tag)
-    Cache.get("tcn:#{tag}")
-  end
-
-  def update_cache(affected_tags, forum_topic_id)
-    affected_tags.each do |tag|
-      Cache.put("tcn:#{tag}", forum_topic_id, 1.week)
-    end
-  end
-end

app/models/bulk_update_request.rb

@@ -8,15 +8,16 @@ class BulkUpdateRequest < ApplicationRecord
   belongs_to :forum_post, optional: true
   belongs_to :approver, optional: true, class_name: "User"
 
+  before_validation :normalize_text
+  before_validation :update_tags
   validates_presence_of :script
   validates_presence_of :title, if: ->(rec) {rec.forum_topic_id.blank?}
   validates_presence_of :forum_topic, if: ->(rec) {rec.forum_topic_id.present?}
   validates_inclusion_of :status, :in => %w(pending approved rejected)
   validate :script_formatted_correctly
   validate :validate_script, :on => :create
-  before_validation :normalize_text
+
   after_create :create_forum_topic
-  after_save :update_notice
 
   scope :pending_first, -> { order(Arel.sql("(case status when 'pending' then 0 when 'approved' then 1 else 2 end)")) }
   scope :pending, -> {where(status: "pending")}
@@ -34,7 +35,7 @@ class BulkUpdateRequest < ApplicationRecord
     def search(params = {})
       q = super
 
-      q = q.search_attributes(params, :user, :approver, :forum_topic_id, :forum_post_id, :script)
+      q = q.search_attributes(params, :user, :approver, :forum_topic_id, :forum_post_id, :script, :tags)
       q = q.text_attribute_matches(:script, params[:script_matches])
 
       if params[:status].present?
@@ -152,6 +153,10 @@ class BulkUpdateRequest < ApplicationRecord
     self.script = script.downcase
   end
 
+  def update_tags
+    self.tags = AliasAndImplicationImporter.new(script, nil).affected_tags
+  end
+
   def skip_secondary_validations=(v)
     @skip_secondary_validations = v.to_s.truthy?
   end
@@ -168,13 +173,6 @@ class BulkUpdateRequest < ApplicationRecord
     status == "rejected"
   end
 
-  def update_notice
-    TagChangeNoticeService.update_cache(
-      AliasAndImplicationImporter.new(script, nil).affected_tags,
-      forum_topic_id
-    )
-  end
-
   def self.available_includes
     [:user, :forum_topic, :forum_post, :approver]
   end

app/models/tag_relationship.rb

@@ -29,7 +29,6 @@ class TagRelationship < ApplicationRecord
   validates :approver, presence: { message: "must exist" }, if: -> { approver_id.present? }
   validates :forum_topic, presence: { message: "must exist" }, if: -> { forum_topic_id.present? }
   validate :antecedent_and_consequent_are_different
-  after_save :update_notice
 
   def normalize_names
     self.antecedent_name = antecedent_name.mb_chars.downcase.tr(" ", "_")
@@ -158,13 +157,6 @@ class TagRelationship < ApplicationRecord
     end
   end
 
-  def update_notice
-    TagChangeNoticeService.update_cache(
-      [antecedent_name, consequent_name],
-      forum_topic_id
-    )
-  end
-
   def self.available_includes
     [:creator, :approver, :forum_post, :forum_topic, :antecedent_tag, :consequent_tag, :antecedent_wiki, :consequent_wiki]
   end

app/views/bulk_update_requests/_search.html.erb

@@ -1,7 +1,7 @@
 <%= search_form_for(bulk_update_requests_path) do |f| %>
   <%= f.input :user_name, label: "Creator", input_html: { value: params[:search][:user_name], data: { autocomplete: "user" } } %>
   <%= f.input :approver_name, label: "Approver", input_html: { value: params[:search][:approver_name], data: { autocomplete: "user" } } %>
-  <%= f.input :script_matches, label: "Script", input_html: { value: params[:search][:script_matches] } %>
+  <%= f.input :tags_include_any, label: "Tags", input_html: { value: params[:search][:tags_include_any], data: { autocomplete: "tag-query" } } %>
   <%= f.input :status, label: "Status", collection: %w[pending approved rejected], include_blank: true, selected: params[:search][:status] %>
   <%= f.input :order, collection: [%w[Status status_desc], %w[Last\ updated updated_at_desc], %w[Newest id_desc], %w[Oldest id_asc]], include_blank: false, selected: params[:search][:order] %>
   <%= f.submit "Search" %>

app/views/posts/partials/index/_posts.html.erb

@@ -19,10 +19,17 @@
     </div>
   <% end %>
 
-  <% if show_tag_change_notice? %>
+  <% if CurrentUser.user.is_member? && post_set.query.is_simple_tag? %>
-    <div class="fineprint tag-change-notice">
+    <% cache("tag-change-notice:#{post_set.query.normalize_query}", expires_in: 4.hours) do %>
-      <p>This tag is the subject of an ongoing discussion. If you have any relevant information, please join the <%= link_to "discussion", forum_topic_path(TagChangeNoticeService.get_forum_topic_id(params[:tags]) || 1) %>.</p>
+      <% if post_set.pending_bulk_update_requests.present? %>
-    </div>
+        <div class="fineprint tag-change-notice">
+          <p>
+            This tag is being discussed in
+            <%= to_sentence post_set.pending_bulk_update_requests.map { |bur| link_to "Topic ##{bur.forum_topic_id}: #{bur.forum_topic.title}", bur.forum_post } %>.
+          </p>
+        </div>
+      <% end %>
+    <% end %>
   <% end %>
 
   <% unless post_set.is_random? %>

db/migrate/20200427190519_add_tags_to_bulk_update_requests.rb

@@ -0,0 +1,6 @@
+class AddTagsToBulkUpdateRequests < ActiveRecord::Migration[6.0]
+  def change
+    add_column :bulk_update_requests, :tags, "text[]", default: "{}", null: false
+    add_index :bulk_update_requests, :tags, using: :gin
+  end
+end

db/structure.sql

@@ -669,7 +669,8 @@ CREATE TABLE public.bulk_update_requests (
     created_at timestamp without time zone NOT NULL,
     updated_at timestamp without time zone NOT NULL,
     approver_id integer,
-    forum_post_id integer
+    forum_post_id integer,
+    tags text[] DEFAULT '{}'::text[] NOT NULL
 );
 
 
@@ -4732,6 +4733,13 @@ CREATE INDEX index_bans_on_user_id ON public.bans USING btree (user_id);
 CREATE INDEX index_bulk_update_requests_on_forum_post_id ON public.bulk_update_requests USING btree (forum_post_id);
 
 
+--
+-- Name: index_bulk_update_requests_on_tags; Type: INDEX; Schema: public; Owner: -
+--
+
+CREATE INDEX index_bulk_update_requests_on_tags ON public.bulk_update_requests USING gin (tags);
+
+
 --
 -- Name: index_comment_votes_on_comment_id; Type: INDEX; Schema: public; Owner: -
 --
@@ -7384,6 +7392,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20200325073456'),
 ('20200325074859'),
 ('20200403210353'),
-('20200406054838');
+('20200406054838'),
+('20200427190519');
 
 

script/fixes/064_initialize_bulk_update_request_tags.rb

@@ -0,0 +1,11 @@
+#!/usr/bin/env ruby
+
+require_relative "../../config/environment"
+
+BulkUpdateRequest.transaction do
+  BulkUpdateRequest.find_each do |request|
+    request.tags = AliasAndImplicationImporter.new(request.script, nil).affected_tags
+    request.save!(validate: false)
+    puts "bur id=#{request.id} tags=#{request.tags}"
+  end
+end

test/functional/posts_controller_test.rb

@@ -70,6 +70,12 @@ class PostsControllerTest < ActionDispatch::IntegrationTest
           get posts_path(tags: "search:all")
           assert_response :success
         end
+
+        should "show a notice for a single tag search with a pending BUR" do
+          create(:bulk_update_request, script: "create alias foo -> bar")
+          get_auth posts_path(tags: "foo"), @user
+          assert_select ".tag-change-notice"
+        end
       end
 
       context "with a multi-tag search" do

test/unit/bulk_update_request_test.rb

@@ -13,30 +13,22 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
       CurrentUser.ip_addr = nil
     end
 
-    context "#update_notice" do
+    should "parse the tags inside the script" do
-      setup do
+      @bur = create(:bulk_update_request, script: %{
-        @forum_topic = create(:forum_topic, creator: @admin)
+        create alias aaa -> 000
-      end
+        create implication bbb -> 111
+        remove alias ccc -> 222
+        remove implication ddd -> 333
+        mass update eee id:1 -fff ~ggg hhh* -> 444 -555
+        category iii -> meta
+      })
 
-      should "update the cache" do
+      assert_equal(%w[000 111 222 333 444 aaa bbb ccc ddd eee iii], @bur.tags)
-        @script = "create alias aaa -> 000\n" +
+    end
-          "create implication bbb -> 111\n" +
-          "remove alias ccc -> 222\n" +
-          "remove implication ddd -> 333\n" +
-          "mass update eee -> 444\n"
-        FactoryBot.create(:bulk_update_request, script: @script, forum_topic: @forum_topic)
 
-        assert_equal(@forum_topic.id, Cache.get("tcn:aaa"))
+    should_eventually "parse tags with tag type prefixes inside the script" do
-        assert_equal(@forum_topic.id, Cache.get("tcn:000"))
+      @bur = create(:bulk_update_request, script: "mass update aaa -> artist:bbb")
-        assert_equal(@forum_topic.id, Cache.get("tcn:bbb"))
+      assert_equal(%w[aaa bbb], @bur.tags)
-        assert_equal(@forum_topic.id, Cache.get("tcn:111"))
-        assert_equal(@forum_topic.id, Cache.get("tcn:ccc"))
-        assert_equal(@forum_topic.id, Cache.get("tcn:222"))
-        assert_equal(@forum_topic.id, Cache.get("tcn:ddd"))
-        assert_equal(@forum_topic.id, Cache.get("tcn:333"))
-        assert_equal(@forum_topic.id, Cache.get("tcn:eee"))
-        assert_equal(@forum_topic.id, Cache.get("tcn:444"))
-      end
     end
 
     context "on approval" do