From d5a7fafca1f77dc7745a6e12c74793fb7793dae5 Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 27 Apr 2020 17:02:40 -0500 Subject: [PATCH] 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 | 4 --- app/logical/alias_and_implication_importer.rb | 24 ++++--------- app/logical/post_sets/post.rb | 5 +++ app/logical/tag_change_notice_service.rb | 13 ------- app/models/bulk_update_request.rb | 18 +++++----- app/models/tag_relationship.rb | 8 ----- .../bulk_update_requests/_search.html.erb | 2 +- .../posts/partials/index/_posts.html.erb | 15 +++++--- ...190519_add_tags_to_bulk_update_requests.rb | 6 ++++ db/structure.sql | 13 +++++-- ...064_initialize_bulk_update_request_tags.rb | 11 ++++++ test/functional/posts_controller_test.rb | 6 ++++ test/unit/bulk_update_request_test.rb | 36 ++++++++----------- 13 files changed, 80 insertions(+), 81 deletions(-) delete mode 100644 app/logical/tag_change_notice_service.rb create mode 100644 db/migrate/20200427190519_add_tags_to_bulk_update_requests.rb create mode 100755 script/fixes/064_initialize_bulk_update_request_tags.rb diff --git a/app/helpers/posts_helper.rb b/app/helpers/posts_helper.rb index 932e8a184..31da14cbc 100644 --- a/app/helpers/posts_helper.rb +++ b/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) diff --git a/app/logical/alias_and_implication_importer.rb b/app/logical/alias_and_implication_importer.rb index f4034c04d..660c365c5 100644 --- a/app/logical/alias_and_implication_importer.rb +++ b/app/logical/alias_and_implication_importer.rb @@ -79,27 +79,17 @@ class AliasAndImplicationImporter end def affected_tags - tokens = self.class.tokenize(text) - tokens.inject([]) do |all, token| - case token[0] + AliasAndImplicationImporter.tokenize(text).flat_map do |type, *args| + case type when :create_alias, :remove_alias, :create_implication, :remove_implication - all << token[1] - all << token[2] - all - + [args[0], args[1]] when :mass_update - all += PostQueryBuilder.new(token[1]).split_query - all += PostQueryBuilder.new(token[2]).parse_tag_edit - all - + tags = PostQueryBuilder.new(args[0]).tags + PostQueryBuilder.new(args[1]).tags + tags.reject(&:negated).reject(&:optional).reject(&:wildcard).map(&:name) when :change_category - all << token[1] - all - - else - all + args[0] end - end + end.sort.uniq end private diff --git a/app/logical/post_sets/post.rb b/app/logical/post_sets/post.rb index fe36c4867..ea67abc4c 100644 --- a/app/logical/post_sets/post.rb +++ b/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 diff --git a/app/logical/tag_change_notice_service.rb b/app/logical/tag_change_notice_service.rb deleted file mode 100644 index 4d840e637..000000000 --- a/app/logical/tag_change_notice_service.rb +++ /dev/null @@ -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 diff --git a/app/models/bulk_update_request.rb b/app/models/bulk_update_request.rb index 961b22488..3b664100b 100644 --- a/app/models/bulk_update_request.rb +++ b/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 diff --git a/app/models/tag_relationship.rb b/app/models/tag_relationship.rb index a463deee5..1ed46ad98 100644 --- a/app/models/tag_relationship.rb +++ b/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 diff --git a/app/views/bulk_update_requests/_search.html.erb b/app/views/bulk_update_requests/_search.html.erb index f00126150..11c3d26f8 100644 --- a/app/views/bulk_update_requests/_search.html.erb +++ b/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" %> diff --git a/app/views/posts/partials/index/_posts.html.erb b/app/views/posts/partials/index/_posts.html.erb index 36f2bbecf..cbb347c54 100644 --- a/app/views/posts/partials/index/_posts.html.erb +++ b/app/views/posts/partials/index/_posts.html.erb @@ -19,10 +19,17 @@ <% end %> - <% if show_tag_change_notice? %> -
-

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) %>.

-
+ <% if CurrentUser.user.is_member? && post_set.query.is_simple_tag? %> + <% cache("tag-change-notice:#{post_set.query.normalize_query}", expires_in: 4.hours) do %> + <% if post_set.pending_bulk_update_requests.present? %> +
+

+ 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 } %>. +

+
+ <% end %> + <% end %> <% end %> <% unless post_set.is_random? %> diff --git a/db/migrate/20200427190519_add_tags_to_bulk_update_requests.rb b/db/migrate/20200427190519_add_tags_to_bulk_update_requests.rb new file mode 100644 index 000000000..3ce966618 --- /dev/null +++ b/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 diff --git a/db/structure.sql b/db/structure.sql index 78b0a5d5d..c8e7ff934 100644 --- a/db/structure.sql +++ b/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'); diff --git a/script/fixes/064_initialize_bulk_update_request_tags.rb b/script/fixes/064_initialize_bulk_update_request_tags.rb new file mode 100755 index 000000000..c68e9fef7 --- /dev/null +++ b/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 diff --git a/test/functional/posts_controller_test.rb b/test/functional/posts_controller_test.rb index c4ac38dcd..1a0e28668 100644 --- a/test/functional/posts_controller_test.rb +++ b/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 diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index 9c6d780b9..c3d3f44f1 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/test/unit/bulk_update_request_test.rb @@ -13,30 +13,22 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase CurrentUser.ip_addr = nil end - context "#update_notice" do - setup do - @forum_topic = create(:forum_topic, creator: @admin) - end + should "parse the tags inside the script" do + @bur = create(:bulk_update_request, script: %{ + create alias aaa -> 000 + 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 - @script = "create alias aaa -> 000\n" + - "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(%w[000 111 222 333 444 aaa bbb ccc ddd eee iii], @bur.tags) + end - assert_equal(@forum_topic.id, Cache.get("tcn:aaa")) - assert_equal(@forum_topic.id, Cache.get("tcn:000")) - assert_equal(@forum_topic.id, Cache.get("tcn:bbb")) - 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 + should_eventually "parse tags with tag type prefixes inside the script" do + @bur = create(:bulk_update_request, script: "mass update aaa -> artist:bbb") + assert_equal(%w[aaa bbb], @bur.tags) end context "on approval" do