diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index a8f838e77..51bdaf6a9 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -159,8 +159,17 @@ class SearchController < ApplicationController term: feed_params[:search_fields], ) elsif class_name.User? - # No need to check for articles or podcast episodes if we know we only want users - user_search + if FeatureFlag.enabled?(:search_2_users) + Search::Postgres::User.search_documents( + term: feed_params[:search_fields], + page: feed_params[:page], + per_page: feed_params[:per_page], + sort_by: feed_params[:sort_by] == "published_at" ? :created_at : nil, + sort_direction: feed_params[:sort_direction], + ) + else + user_search + end else # search page feed_content_search end diff --git a/app/models/user.rb b/app/models/user.rb index 843975196..c7dd4945d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -266,19 +266,25 @@ class User < ApplicationRecord # used in expression indexes as it's a mutable function and depends on server settings # => https://stackoverflow.com/a/11007216/4186181 # - # rubocop:disable Layout/LineLength scope :search_by_name_and_username, lambda { |term| where( - %{to_tsvector('simple', coalesce(name::text, '')) @@ to_tsquery('simple', ''' ' || unaccent('simple', ?) || ' ''' || ':*')}, - term, + sanitize_sql_array( + [ + "to_tsvector('simple', coalesce(name::text, '')) @@ to_tsquery('simple', ? || ':*')", + connection.quote(term), + ], + ), ).or( where( - %{to_tsvector('simple', coalesce(username::text, '')) @@ to_tsquery('simple', ''' ' || unaccent('simple', ?) || ' ''' || ':*')}, - term, + sanitize_sql_array( + [ + "to_tsvector('simple', coalesce(username::text, '')) @@ to_tsquery('simple', ? || ':*')", + connection.quote(term), + ], + ), ), ) } - # rubocop:enable Layout/LineLength scope :with_feed, -> { where.not(feed_url: [nil, ""]) } before_validation :check_for_username_change @@ -650,6 +656,7 @@ class User < ApplicationRecord "#{employer_name}#{mostly_work_with}#{available_for}" end + # TODO: this can be removed once we migrate away from ES def search_score counts_score = (articles_count + comments_count + reactions_count + badge_achievements_count) * 10 score = (counts_score + tag_keywords_for_search.size) * reputation_modifier diff --git a/app/models/user_role.rb b/app/models/user_role.rb new file mode 100644 index 000000000..b976bb8a0 --- /dev/null +++ b/app/models/user_role.rb @@ -0,0 +1,7 @@ +# Exposes the `rolify` gem's `users_roles` table via ActiveRecord +class UserRole < ApplicationRecord + self.table_name = "users_roles" + + belongs_to :user + belongs_to :role +end diff --git a/app/serializers/search/postgres_user_serializer.rb b/app/serializers/search/postgres_user_serializer.rb new file mode 100644 index 000000000..58499eeb9 --- /dev/null +++ b/app/serializers/search/postgres_user_serializer.rb @@ -0,0 +1,17 @@ +module Search + # to be renamed when ES is gone + class PostgresUserSerializer < ApplicationSerializer + attribute :class_name, -> { "User" } + attributes :id, :path + attribute :title, &:name + + attribute :user do |user| + { + # currently the frontend expects this to be the username, despite the attribute name + name: user.username, + profile_image_90: user.profile_image_90, + username: user.username + } + end + end +end diff --git a/app/services/search/postgres/user.rb b/app/services/search/postgres/user.rb new file mode 100644 index 000000000..ee519c0b3 --- /dev/null +++ b/app/services/search/postgres/user.rb @@ -0,0 +1,74 @@ +module Search + module Postgres + class User + ATTRIBUTES = %i[ + id + name + profile_image + username + ].freeze + + DEFAULT_PER_PAGE = 60 + MAX_PER_PAGE = 100 + + # User.search_score used to take employer related fields into account, but they have since been moved to profile + # and removed from fields that are searched against. + HOTNESS_SCORE_ORDER = Arel.sql(%{ + (((articles_count + comments_count + reactions_count + badge_achievements_count) * 10) * reputation_modifier) + DESC + }.squish).freeze + + def self.search_documents(term: nil, sort_by: :nil, sort_direction: :desc, page: 0, per_page: DEFAULT_PER_PAGE) + # NOTE: we should eventually update the frontend + # to start from page 1 + page = page.to_i + 1 + per_page = [(per_page || DEFAULT_PER_PAGE).to_i, MAX_PER_PAGE].min + + relation = ::User + + relation = filter_suspended_users(relation) + + relation = relation.search_by_name_and_username(term) if term.present? + + relation = relation.select(*ATTRIBUTES) + + relation = sort(relation, sort_by, sort_direction) + + relation = relation.page(page).per(per_page) + + serialize(relation) + end + + # `User.without_role` generates a subquery + 2 inner joins. + # Given that the number of suspended users will, hopefully, be a tiny percentage + # of regular users, and the `rolify`'s gem approach is not particularly efficient, + # we simplified the subquery and added a precondition to skip that query entirely, + # when a community has no suspended users. + # NOTE: An alternative approach that could be explored is to + # preload the user ids of all suspended users and use those with `.where.not(id: ...)` + def self.filter_suspended_users(relation) + suspended = UserRole.joins(:role).where(roles: { name: :suspended }) + + return relation unless suspended.exists? + + relation.where.not(id: suspended.select(:user_id)) + end + private_class_method :filter_suspended_users + + def self.sort(relation, sort_by, sort_direction) + return relation.reorder(sort_by => sort_direction) if sort_by&.to_sym == :created_at + + relation.reorder(HOTNESS_SCORE_ORDER) + end + private_class_method :sort + + def self.serialize(users) + Search::PostgresUserSerializer + .new(users, is_collection: true) + .serializable_hash[:data] + .pluck(:attributes) + end + private_class_method :serialize + end + end +end diff --git a/spec/models/user_role_spec.rb b/spec/models/user_role_spec.rb new file mode 100644 index 000000000..7dd097666 --- /dev/null +++ b/spec/models/user_role_spec.rb @@ -0,0 +1,6 @@ +require "rails_helper" + +RSpec.describe UserRole, type: :model do + it { is_expected.to belong_to(:user) } + it { is_expected.to belong_to(:role) } +end diff --git a/spec/requests/search_spec.rb b/spec/requests/search_spec.rb index 2c4d299df..3d6c68280 100644 --- a/spec/requests/search_spec.rb +++ b/spec/requests/search_spec.rb @@ -301,6 +301,31 @@ RSpec.describe "Search", type: :request, proper_status: true do expect(response.parsed_body["result"].first).to include("body_text" => comment.body_markdown) end end + + context "when using PostgreSQL for users" do + before do + allow(FeatureFlag).to receive(:enabled?).with(:search_2_users).and_return(true) + end + + it "returns the correct keys", :aggregate_failures do + create(:user) + + get search_feed_content_path(class_name: "User") + + expect(response.parsed_body["result"]).to be_present + end + + it "supports the search params" do + user = create(:user) + + get search_feed_content_path( + class_name: "User", page: 0, per_page: 1, search_fields: user.name, + sort_by: :created_at, sort_direction: :desc + ) + + expect(response.parsed_body["result"].first["id"]).to eq(user.id) + end + end end describe "GET /search/reactions" do diff --git a/spec/services/search/postgres/user_spec.rb b/spec/services/search/postgres/user_spec.rb new file mode 100644 index 000000000..e395e9070 --- /dev/null +++ b/spec/services/search/postgres/user_spec.rb @@ -0,0 +1,118 @@ +require "rails_helper" + +# rubocop:disable Rails/PluckId +# This spec uses `pluck` on an array of hashes, but Rubocop can't tell the difference. +RSpec.describe Search::Postgres::User, type: :service do + describe "::search_documents" do + it "returns an empty result if there are no users" do + expect(described_class.search_documents).to be_empty + end + + it "does not return suspended users" do + user = create(:user, :suspended) + + expect(described_class.search_documents.pluck(:id)).not_to include(user.id) + end + + it "returns regular users" do + user = create(:user) + + expect(described_class.search_documents.pluck(:id)).to include(user.id) + end + + it "returns admins" do + user = create(:user, :super_admin) + + expect(described_class.search_documents.pluck(:id)).to include(user.id) + end + + context "when describing the result format" do + let(:results) { described_class.search_documents } + + it "returns the correct attributes for a single result", :aggregate_failures do + user = create(:user) + + result = results.first + + expect(result.keys).to match_array(%i[class_name id path title user]) + expect(result[:class_name]).to eq("User") + expect(result[:id]).to eq(user.id) + expect(result[:path]).to eq(user.path) + expect(result[:title]).to eq(user.name) + + expect(result[:user][:name]).to eq(user.username) + expect(result[:user][:profile_image_90]).to eq(user.profile_image_90) + expect(result[:user][:username]).to eq(user.username) + end + end + + context "when searching for a term" do + let(:user) { create(:user) } + + it "matches against the user's name", :aggregate_failures do + user.update_columns(name: "Langston Hughes") + + result = described_class.search_documents(term: "lang") + expect(result.first[:id]).to eq(user.id) + + result = described_class.search_documents(term: "fiesta") + expect(result).to be_empty + end + + it "matches against the user's username", :aggregate_failures do + result = described_class.search_documents(term: user.username.first(3)) + expect(result.first[:id]).to eq(user.id) + + result = described_class.search_documents(term: "fiesta") + expect(result).to be_empty + end + end + + context "when sorting" do + it "sorts by 'hotness_score' in descending order by default" do + user1, user2 = create_list(:user, 2) + + user1.update_columns(articles_count: 10, reputation_modifier: 1.0) + user2.update_columns(articles_count: 10, reputation_modifier: 2.2) + + results = described_class.search_documents + expect(results.pluck(:id)).to eq([user2.id, user1.id]) + end + + it "supports sorting by created_at in ascending and descending order", :aggregate_failures do + user1 = create(:user) + + user2 = nil + Timecop.travel(1.week.ago) do + user2 = create(:user) + end + + results = described_class.search_documents(sort_by: :created_at, sort_direction: :asc) + expect(results.pluck(:id)).to eq([user2.id, user1.id]) + + results = described_class.search_documents(sort_by: :created_at, sort_direction: :desc) + expect(results.pluck(:id)).to eq([user1.id, user2.id]) + end + end + + context "when paginating" do + it "returns no items when out of pagination bounds" do + create_list(:user, 2) + + result = described_class.search_documents(page: 99) + expect(result).to be_empty + end + + it "returns paginated items", :aggregate_failures do + create_list(:user, 2) + + result = described_class.search_documents(page: 0, per_page: 1) + expect(result.length).to eq(1) + + result = described_class.search_documents(page: 1, per_page: 1) + expect(result.length).to eq(1) + end + end + end +end +# rubocop:enable Rails/PluckId diff --git a/spec/services/search/postgres/username_spec.rb b/spec/services/search/postgres/username_spec.rb index bc225b330..443c81d00 100644 --- a/spec/services/search/postgres/username_spec.rb +++ b/spec/services/search/postgres/username_spec.rb @@ -47,6 +47,14 @@ RSpec.describe Search::Postgres::Username, type: :service do expect(described_class.search_documents(user.name.first(3))).to be_present end + it "finds a user if their name contains quotes", :aggregate_failures do + user = create(:user, name: "McNamara O'Hara") + expect(described_class.search_documents(user.name)).to be_present + + user = create(:user, name: 'McNamara O"Hara') + expect(described_class.search_documents(user.name)).to be_present + end + it "finds multiple users whose names have common parts", :aggregate_failures do alex = create(:user, username: "alex") alexsmith = create(:user, name: "alexsmith")