From 4bc56b8ac0f0ea7802cf54d72f42b345b5180259 Mon Sep 17 00:00:00 2001 From: rhymes Date: Wed, 9 Jun 2021 14:28:19 +0200 Subject: [PATCH] Search results: prioritize articles titles (#13918) * Add HairTrigger spec * Add weights to columns tsvectors * Add correct sorting for search results * Remove Timecop.travel * Changed order of columns in trigger based on weights --- app/models/article.rb | 14 ++-- app/services/search/article.rb | 7 +- ..._create_trigger_articles_insert_update1.rb | 69 +++++++++++++++++++ db/schema.rb | 16 ++--- spec/models/hair_trigger_spec.rb | 11 +++ spec/services/search/article_spec.rb | 32 +++++++-- 6 files changed, 125 insertions(+), 24 deletions(-) create mode 100644 db/migrate/20210609103939_create_trigger_articles_insert_update1.rb create mode 100644 spec/models/hair_trigger_spec.rb diff --git a/app/models/article.rb b/app/models/article.rb index 38b791275..ef41dc3a9 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -123,12 +123,12 @@ class Article < ApplicationRecord .declare("l_org_vector tsvector; l_user_vector tsvector") do <<~SQL NEW.reading_list_document := - to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.body_markdown, ''))) || - to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_tag_list, ''))) || - to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_name, ''))) || - to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_username, ''))) || - to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.title, ''))) || - to_tsvector('simple'::regconfig, + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.title, ''))), 'A') || + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_tag_list, ''))), 'B') || + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.body_markdown, ''))), 'C') || + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_name, ''))), 'D') || + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_username, ''))), 'D') || + setweight(to_tsvector('simple'::regconfig, unaccent( coalesce( array_to_string( @@ -139,7 +139,7 @@ class Article < ApplicationRecord '' ) ) - ); + ), 'D'); SQL end diff --git a/app/services/search/article.rb b/app/services/search/article.rb index f89154511..25e13a7ef 100644 --- a/app/services/search/article.rb +++ b/app/services/search/article.rb @@ -14,7 +14,7 @@ module Search relation = relation.search_articles(term) if term.present? - relation = sort(relation, sort_by, sort_direction) + relation = sort(relation, term, sort_by, sort_direction) tag_flares = Homepage::FetchTagFlares.call(relation) @@ -25,7 +25,10 @@ module Search serialize(relation, tag_flares) end - def self.sort(relation, sort_by, sort_direction) + def self.sort(relation, term, sort_by, sort_direction) + # By skipping ordering, we rely on the custom ranking defined in the article's tsvector document + return relation if term.present? && sort_by.blank? + return relation.reorder(sort_by => sort_direction) if sort_by&.to_sym == :published_at relation.reorder(DEFAULT_SORT_BY) diff --git a/db/migrate/20210609103939_create_trigger_articles_insert_update1.rb b/db/migrate/20210609103939_create_trigger_articles_insert_update1.rb new file mode 100644 index 000000000..77c3fbab6 --- /dev/null +++ b/db/migrate/20210609103939_create_trigger_articles_insert_update1.rb @@ -0,0 +1,69 @@ +# This migration was auto-generated via `rake db:generate_trigger_migration'. +# While you can edit this file, any changes you make to the definitions here +# will be undone by the next auto-generated trigger migration. + +class CreateTriggerArticlesInsertUpdate1 < ActiveRecord::Migration[6.1] + def up + drop_trigger("update_reading_list_document", "articles", :generated => true) + + create_trigger("update_reading_list_document", :generated => true, :compatibility => 1). + on("articles"). + name("update_reading_list_document"). + before(:insert, :update). + for_each(:row). + declare("l_org_vector tsvector; l_user_vector tsvector") do + <<-SQL_ACTIONS +NEW.reading_list_document := + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.title, ''))), 'A') || + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_tag_list, ''))), 'B') || + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.body_markdown, ''))), 'C') || + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_name, ''))), 'D') || + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_username, ''))), 'D') || + setweight(to_tsvector('simple'::regconfig, + unaccent( + coalesce( + array_to_string( + -- cached_organization is serialized to the DB as a YAML string, we extract only the name attribute + regexp_match(NEW.cached_organization, 'name: (.*)$', 'n'), + ' ' + ), + '' + ) + ) + ), 'D'); + SQL_ACTIONS + end + end + + def down + drop_trigger("update_reading_list_document", "articles", :generated => true) + + create_trigger("update_reading_list_document", :generated => true, :compatibility => 1). + on("articles"). + name("update_reading_list_document"). + before(:insert, :update). + for_each(:row). + declare("l_org_vector tsvector; l_user_vector tsvector") do + <<-SQL_ACTIONS +NEW.reading_list_document := + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.body_markdown, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_tag_list, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_name, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_username, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.title, ''))) || + to_tsvector('simple'::regconfig, + unaccent( + coalesce( + array_to_string( + -- cached_organization is serialized to the DB as a YAML string, we extract only the name attribute + regexp_match(NEW.cached_organization, 'name: (.*)$', 'n'), + ' ' + ), + '' + ) + ) + ); + SQL_ACTIONS + end + end +end diff --git a/db/schema.rb b/db/schema.rb index f6fee2394..72ab68d9e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_05_12_025422) do +ActiveRecord::Schema.define(version: 2021_06_09_103939) do # These are extensions that must be enabled in order to support this database enable_extension "citext" @@ -1606,12 +1606,12 @@ ActiveRecord::Schema.define(version: 2021_05_12_025422) do declare("l_org_vector tsvector; l_user_vector tsvector") do <<-SQL_ACTIONS NEW.reading_list_document := - to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.body_markdown, ''))) || - to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_tag_list, ''))) || - to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_name, ''))) || - to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_username, ''))) || - to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.title, ''))) || - to_tsvector('simple'::regconfig, + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.title, ''))), 'A') || + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_tag_list, ''))), 'B') || + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.body_markdown, ''))), 'C') || + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_name, ''))), 'D') || + setweight(to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_username, ''))), 'D') || + setweight(to_tsvector('simple'::regconfig, unaccent( coalesce( array_to_string( @@ -1622,7 +1622,7 @@ NEW.reading_list_document := '' ) ) - ); + ), 'D'); SQL_ACTIONS end diff --git a/spec/models/hair_trigger_spec.rb b/spec/models/hair_trigger_spec.rb new file mode 100644 index 000000000..215ee156c --- /dev/null +++ b/spec/models/hair_trigger_spec.rb @@ -0,0 +1,11 @@ +require "rails_helper" + +# HairTrigger suggests adding this test to make sure the schema and triggers are aligned +# See https://github.com/jenseng/hair_trigger#testing +RSpec.describe HairTrigger, type: :model do + describe ".migrations_current?" do + it "is always true" do + expect(described_class.migrations_current?).to be(true) + end + end +end diff --git a/spec/services/search/article_spec.rb b/spec/services/search/article_spec.rb index f4a43afe9..c6ef94c02 100644 --- a/spec/services/search/article_spec.rb +++ b/spec/services/search/article_spec.rb @@ -102,7 +102,17 @@ RSpec.describe Search::Article, type: :service do end context "when sorting" do - it "sorts by 'hotness_score' and 'comments_count' in descending order by default" do + it "sorts by title, tags, body ranking by default with a search term", :aggregate_failures do + article_body = create(:article) + article_body.update(body_markdown: "The Other Side of Silence Impedit consequatur") + article_tag = create(:article, tags: "consequatur") + article_title = create(:article, title: "The Other Side of Silence Impedit consequatur") + + results = described_class.search_documents(term: "consequatur") + expect(results.pluck(:id)).to eq([article_title.id, article_tag.id, article_body.id]) + end + + it "sorts by 'hotness_score' and 'comments_count' in descending order without a search term" do article1, article2, article3 = create_list(:article, 3) article1.update_columns(hotness_score: 10, comments_count: 10) @@ -113,13 +123,21 @@ RSpec.describe Search::Article, type: :service do expect(results.pluck(:id)).to eq([article2.id, article1.id, article3.id]) end - it "supports sorting by published_at in ascending and descending order", :aggregate_failures do - article1 = create(:article) + it "supports sorting by published_at in ascending and descending order with a search term", :aggregate_failures do + article1 = create(:article, tags: "ruby") + article2 = create(:article, tags: "ruby", published_at: 1.week.ago) - article2 = nil - Timecop.travel(1.week.ago) do - article2 = create(:article) - end + results = described_class.search_documents(term: "ruby", sort_by: :published_at, sort_direction: :asc) + expect(results.pluck(:id)).to eq([article2.id, article1.id]) + + results = described_class.search_documents(term: "ruby", sort_by: :published_at, sort_direction: :desc) + expect(results.pluck(:id)).to eq([article1.id, article2.id]) + end + + it "supports sorting by published_at in ascending and descending order without a search term", + :aggregate_failures do + article1 = create(:article) + article2 = create(:article, published_at: 1.week.ago) results = described_class.search_documents(sort_by: :published_at, sort_direction: :asc) expect(results.pluck(:id)).to eq([article2.id, article1.id])