diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index efe5d7cdb..a234870f6 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -10,6 +10,7 @@ class TagsController < ApplicationController def index @tags = Tag.search(search_params).paginate(params[:page], :limit => params[:limit], :search_count => params[:search]) + respond_with(@tags) do |format| format.xml do render :xml => @tags.to_xml(:root => "tags") @@ -18,7 +19,13 @@ class TagsController < ApplicationController end def autocomplete - @tags = Tag.names_matches_with_aliases(params[:search][:name_matches]) + if CurrentUser.is_builder? + # limit rollout + @tags = TagAutocomplete.search(params[:search][:name_matches]) + else + @tags = Tag.names_matches_with_aliases(params[:search][:name_matches]) + end + expires_in params[:expiry].to_i.days if params[:expiry] respond_with(@tags) do |format| diff --git a/app/javascript/src/javascripts/autocomplete.js.erb b/app/javascript/src/javascripts/autocomplete.js.erb index cb7ef0c3d..97df81127 100644 --- a/app/javascript/src/javascripts/autocomplete.js.erb +++ b/app/javascript/src/javascripts/autocomplete.js.erb @@ -307,6 +307,11 @@ Autocomplete.insert_completion = function(input, completion) { var regexp = new RegExp("(" + Autocomplete.TAG_PREFIXES + ")?\\S+$", "g"); before_caret_text = before_caret_text.replace(regexp, "$1") + completion + " "; + if (Utility.meta('current-user-id') === '1') { + // is this actually better? + after_caret_text = after_caret_text.replace(/^\S+/, ""); + } + input.value = before_caret_text + after_caret_text; input.selectionStart = input.selectionEnd = before_caret_text.length; }; diff --git a/app/logical/tag_autocomplete.rb b/app/logical/tag_autocomplete.rb new file mode 100644 index 000000000..cf1e4d51c --- /dev/null +++ b/app/logical/tag_autocomplete.rb @@ -0,0 +1,99 @@ +module TagAutocomplete + extend self + + PREFIX_BOUNDARIES = "(_/:;-" + + class Result < Struct.new(:name, :post_count, :category, :antecedent_name) + def to_xml(options = {}) + to_h.to_xml(options) + end + end + + def search(query) + candidates = count_sort( + query, + search_prefix(query, 3) + + search_fuzzy(query, 5) + + search_exact(query, 3) + + search_aliases(query, 3) + ) + end + + def count_sort(query, words) + words.uniq.sort_by do |x| + x.post_count + end.reverse + end + + def search_exact(query, n=3) + Tag + .where("name like ? escape e'\\\\'", query.to_escaped_for_sql_like + "%") + .where("post_count > 0") + .order("post_count desc") + .limit(n) + .pluck(:name, :post_count, :category) + .map {|row| Result.new(*row)} + end + + def search_fuzzy(query, n=5) + if query.size <= 3 + return [] + end + + Tag + .where("name % ?", query) + .where("name like ? escape E'\\\\'", query[0].to_escaped_for_sql_like + '%') + .where("post_count > 0") + .order(Arel.sql("similarity(name, #{Tag.connection.quote(query)}) * log(10, post_count + 1) DESC")) + .limit(n) + .pluck(:name, :post_count, :category) + .map {|row| Result.new(*row)} + end + + def search_prefix(query, n=3) + if query.size >= 5 + return [] + end + + if query.size <= 1 + return [] + end + + if query =~ /[-_()]/ + return [] + end + + if query.size >= 3 + min_post_count = 0 + else + min_post_count = 5_000 + n += 2 + end + + anchors = "^" + query.split("").map {|x| Regexp.escape(x)}.join(".*[#{PREFIX_BOUNDARIES}]") + Tag + .where("name ~ ?", anchors) + .where("post_count > ?", min_post_count) + .where("post_count > 0") + .order("post_count desc") + .limit(n) + .pluck(:name, :post_count, :category) + .map {|row| Result.new(*row)} + end + + def search_aliases(query, n=20) + wildcard_name = query + "*" + TagAlias + .select("tags.name, tags.post_count, tags.category, tag_aliases.antecedent_name") + .joins("INNER JOIN tags ON tags.name = tag_aliases.consequent_name") + .where("tag_aliases.antecedent_name LIKE ? ESCAPE E'\\\\'", wildcard_name.to_escaped_for_sql_like) + .active + .where("tags.name NOT LIKE ? ESCAPE E'\\\\'", wildcard_name.to_escaped_for_sql_like) + .where("tag_aliases.post_count > 0") + .order("tag_aliases.post_count desc") + .limit(n) + .pluck(:name, :post_count, :category, :antecedent_name) + .map {|row| Result.new(*row)} + end +end + diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index 97a906aab..2425a89fd 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -1,8 +1,9 @@ class TagImplication < TagRelationship + extend Memoist + before_save :update_descendant_names after_save :update_descendant_names_for_parents after_destroy :update_descendant_names_for_parents - after_save :update_descendant_names_for_parents, if: ->(rec) { rec.is_retired? } after_save :create_mod_action validates_uniqueness_of :antecedent_name, :scope => :consequent_name validate :absence_of_circular_relation @@ -17,6 +18,7 @@ class TagImplication < TagRelationship module DescendantMethods extend ActiveSupport::Concern + extend Memoist module ClassMethods # assumes names are normalized @@ -32,17 +34,16 @@ class TagImplication < TagRelationship end def descendants - @descendants ||= begin - [].tap do |all| - children = [consequent_name] + [].tap do |all| + children = [consequent_name] - until children.empty? - all.concat(children) - children = TagImplication.active.where(antecedent_name: children).pluck(:consequent_name) - end - end.sort.uniq - end + until children.empty? + all.concat(children) + children = TagImplication.active.where(antecedent_name: children).pluck(:consequent_name) + end + end.sort.uniq end + memoize :descendants def descendant_names_array descendant_names.split(/ /) @@ -53,7 +54,7 @@ class TagImplication < TagRelationship end def update_descendant_names! - clear_descendants_cache + flush_cache update_descendant_names update_attribute(:descendant_names, descendant_names) end @@ -64,20 +65,15 @@ class TagImplication < TagRelationship parent.update_descendant_names_for_parents end end - - def clear_descendants_cache - @descendants = nil - end end module ParentMethods - def parents - @parents ||= self.class.where(["consequent_name = ?", antecedent_name]) - end + extend Memoist - def clear_parents_cache - @parents = nil + def parents + self.class.where("consequent_name = ?", antecedent_name) end + memoize :parents end module ValidationMethods @@ -139,6 +135,8 @@ class TagImplication < TagRelationship end module ApprovalMethods + extend Memoist + def process!(update_topic: true) unless valid? raise errors.full_messages.join("; ") @@ -215,19 +213,18 @@ class TagImplication < TagRelationship end def forum_updater - @forum_updater ||= begin - post = if forum_topic - forum_post || forum_topic.posts.where("body like ?", TagImplicationRequest.command_string(antecedent_name, consequent_name) + "%").last - else - nil - end - ForumUpdater.new( - forum_topic, - forum_post: post, - expected_title: TagImplicationRequest.topic_title(antecedent_name, consequent_name) - ) + post = if forum_topic + forum_post || forum_topic.posts.where("body like ?", TagImplicationRequest.command_string(antecedent_name, consequent_name) + "%").last + else + nil end + ForumUpdater.new( + forum_topic, + forum_post: post, + expected_title: TagImplicationRequest.topic_title(antecedent_name, consequent_name) + ) end + memoize :forum_updater end include DescendantMethods @@ -236,8 +233,7 @@ class TagImplication < TagRelationship include ApprovalMethods def reload(options = {}) + flush_cache super - clear_parents_cache - clear_descendants_cache end end diff --git a/test/models/tag_autocomplete_test.rb b/test/models/tag_autocomplete_test.rb new file mode 100644 index 000000000..e3fe371a0 --- /dev/null +++ b/test/models/tag_autocomplete_test.rb @@ -0,0 +1,101 @@ +require 'test_helper' + +class TagAutocompleteTest < ActiveSupport::TestCase + subject { TagAutocomplete } + + context "#search_exact" do + setup do + @tags = [ + create(:tag, name: "abcdef", post_count: 1), + create(:tag, name: "abczzz", post_count: 2), + create(:tag, name: "abcyyy", post_count: 0), + create(:tag, name: "bbbbbb") + ] + end + + should "find the tags" do + expected = [ + @tags[1], + @tags[0] + ].map(&:name) + assert_equal(expected, subject.search_exact("abc", 3).map(&:name)) + end + end + + context "#search_fuzzy" do + setup do + @tags = [ + create(:tag, name: "abcdef", post_count: 1), + create(:tag, name: "abcdzz", post_count: 2), + + # one char mismatch + create(:tag, name: "abcezz", post_count: 2), + + # too long + create(:tag, name: "abcdefghijk", post_count: 2), + + # wrong prefix + create(:tag, name: "bbcdef", post_count: 2), + + # zero post count + create(:tag, name: "abcdyy", post_count: 0), + + # completely different + create(:tag, name: "bbbbbb") + ] + end + + should "find the tags" do + expected = [ + @tags[1], + @tags[2], + @tags[0] + ].map(&:name) + assert_equal(expected, subject.search_fuzzy("abcd", 3).map(&:name)) + end + end + + context "#search_prefix" do + setup do + @tags = [ + create(:tag, name: "abcdef", post_count: 1), + create(:tag, name: "alpha_beta_cat", post_count: 2), + create(:tag, name: "alpha_beta_dat", post_count: 0), + create(:tag, name: "alpha_beta_(cane)", post_count: 2), + create(:tag, name: "alpha_beta/cane", post_count: 2) + ] + end + + should "find the tags" do + expected = [ + @tags[1], + @tags[3], + @tags[4] + ].map(&:name) + assert_equal(expected, subject.search_prefix("abc", 3).map(&:name)) + end + end + + context "#search_aliases" do + setup do + @user = create(:user) + @tags = [ + create(:tag, name: "/abc", post_count: 0), + create(:tag, name: "abcdef", post_count: 1), + create(:tag, name: "zzzzzz", post_count: 1), + ] + as_user do + @aliases = [ + create(:tag_alias, antecedent_name: "/abc", consequent_name: "abcdef", status: "active", post_count: 1) + ] + end + end + + should "find the tags" do + results = subject.search_aliases("/abc", 3) + assert_equal(1, results.size) + assert_equal("abcdef", results[0].name) + assert_equal("/abc", results[0].antecedent_name) + end + end +end diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index 5060bfccf..b8ad95380 100644 --- a/test/unit/tag_implication_test.rb +++ b/test/unit/tag_implication_test.rb @@ -100,11 +100,11 @@ class TagImplicationTest < ActiveSupport::TestCase end should "update its descendants on save" do - ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") - ti2 = FactoryBot.create(:tag_implication, :antecedent_name => "ccc", :consequent_name => "ddd") + ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb", :status => "active") + ti2 = FactoryBot.create(:tag_implication, :antecedent_name => "ccc", :consequent_name => "ddd", :status => "active") ti1.reload ti2.reload - ti2.update_attributes( + ti2.update( :antecedent_name => "bbb" ) ti1.reload