maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

[Search 2.0] Comments on search page (#13401)

Browse source 
* Working MVP

* Add comment to COMMENTABLE_TYPES

* Add specs to search_spec

* Add service specs and fix ordering

* Fixed queries and specs

* Clarify COMMENTABLE_TYPES comment

* Move FORCED_EAGER_LOAD_QUERY to Comment

* Add highlighting to Comment search

* Add explicit .to_s on class_name params

* Remove Comment#forced_eager_load_serialized_data

* Revert CommentSerializer changes

* Hardcode class_name attribute

* Add PR feedback

* Use one liner syntax in serializer

* Update user comment

* Update path attribute logic

* Fix user_ids

* Remove PodcastEpisode skipped spec

* Add sort order to Search::Postgres::Comment

* Fix search_specs

* Move pagination TODO comment
This commit is contained in:
Alex 2021-04-20 12:34:21 -04:00 • committed by GitHub
parent 589f3066ff
commit cf0baf05f9
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 403 additions and 61 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

35
app/controllers/search_controller.rb
View file

@ -120,20 +120,31 @@ class SearchController < ApplicationController
end
def feed_content
feed_docs = if params[:class_name].blank?
# If we are in the main feed and not filtering by type return
# all articles, podcast episodes, and users
feed_content_search.concat(user_search)
elsif params[:class_name] == "User"
# No need to check for articles or podcast episodes if we know we only want users
user_search
else
# if params[:class_name] == PodcastEpisode, Article, or Comment then skip user lookup
feed_content_search
end
class_name = feed_params[:class_name].to_s.inquiry
result =
if class_name.blank?
# If we are in the main feed and not filtering by type return
# all articles, podcast episodes, and users
feed_content_search.concat(user_search)
elsif class_name.Comment? && FeatureFlag.enabled?(:search_2_comments)
Search::Postgres::Comment.search_documents(
page: feed_params[:page],
per_page: feed_params[:per_page],
sort_by: feed_params[:sort_by],
sort_direction: feed_params[:sort_direction],
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
else
# if params[:class_name] == PodcastEpisode, Article, or Comment then skip user lookup
feed_content_search
end
render json: {
result: feed_docs,
result: result,
display_jobs_banner: SiteConfig.display_jobs_banner,
jobs_url: SiteConfig.jobs_url
}

23
app/models/comment.rb
View file

@ -2,6 +2,7 @@ class Comment < ApplicationRecord
has_ancestry
resourcify
include PgSearch::Model
include Reactable
include Searchable
@ -9,7 +10,9 @@ class Comment < ApplicationRecord
SEARCH_CLASS = Search::FeedContent
BODY_MARKDOWN_SIZE_RANGE = (1..25_000).freeze
COMMENTABLE_TYPES = %w[Article PodcastEpisode].freeze
TITLE_DELETED = "[deleted]".freeze
TITLE_HIDDEN = "[hidden by post author]".freeze
@ -28,7 +31,6 @@ class Comment < ApplicationRecord
has_many :mentions, as: :mentionable, inverse_of: :mentionable, dependent: :destroy
has_many :notifications, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all
has_many :notification_subscriptions, as: :notifiable, inverse_of: :notifiable, dependent: :destroy
before_validation :evaluate_markdown, if: -> { body_markdown }
before_save :set_markdown_character_count, if: :body_markdown
before_save :synchronous_spam_score_check
@ -67,6 +69,25 @@ class Comment < ApplicationRecord
after_commit :remove_from_elasticsearch, on: [:destroy]
# [@atsmith813] this is adapted from the `search_field` property in
# `config/elasticsearch/mappings/feed_content.json` and
# `app/serializers/search/comment_serializer.rb`
#
# highlighter settings are taken from
# Search::QueryBuildersFeedContent#add_highlight_fields
pg_search_scope :search_comments,
against: %i[body_markdown],
using: {
tsearch: {
prefix: true,
highlight: {
StartSel: "<mark>",
StopSel: "</mark>",
MaxFragments: 2
}
}
}
scope :eager_load_serialized_data, -> { includes(:user, :commentable) }
alias touch_by_reaction save

48
app/serializers/search/postgres_comment_serializer.rb Normal file
View file

@ -0,0 +1,48 @@
module Search
# TODO[@atsmith813]: Rename this to CommentSerializer once Elasticsearch is removed
class PostgresCommentSerializer < ApplicationSerializer
attribute :id, &:search_id
attributes :path do |comment, params|
user = params[:users][comment.user_id]
if user
"/#{user.username}/comment/#{comment.id_code_generated}"
else
"/404.html"
end
end
attributes :public_reactions_count
attribute :body_text, &:body_markdown
attribute :class_name, -> { "Comment" }
attribute :highlight do |comment, _params|
{
body_text: [comment.pg_search_highlight]
}
rescue PgSearch::PgSearchHighlightNotSelected
# This is needed because in Search::Postgres::Comment we only call the
# search if a term is provided. This means if a user searches with a
# blank term, we skip the line that executes .with_pg_search_highlight.
# Skipping this AND trying to call .pg_search_highlight raises an error
# which we ignore here - basically filling in highlights if they're
# there.
end
attribute :hotness_score, &:score
attribute :published, -> { true }
attribute :published_at, &:created_at
attribute :readable_publish_date_string, &:readable_publish_date
attribute :title, &:commentable_title
# NOTE: not using the `NestedUserSerializer` to avoid hitting Redis to
# fetch the cached value
attribute :user do |comment, params|
user = params[:users][comment.user_id]
user.slice(:name, :profile_image_90, :username).symbolize_keys
end
end
end

118
app/services/search/postgres/comment.rb Normal file
View file

@ -0,0 +1,118 @@
module Search
module Postgres
class Comment
ATTRIBUTES = [
"COALESCE(articles.published, false) AS commentable_published",
"COALESCE(articles.title, '') AS commentable_title",
"comments.body_markdown",
"comments.commentable_id",
"comments.commentable_type",
"comments.created_at",
"comments.id AS id",
"comments.public_reactions_count",
"comments.score",
"comments.user_id",
].freeze
private_constant :ATTRIBUTES
USER_ATTRIBUTES = %i[
id
name
profile_image
username
].freeze
private_constant :USER_ATTRIBUTES
ARTICLE_COMMENTABLE_QUERY = <<-SQL.freeze
LEFT JOIN articles
ON comments.commentable_id = articles.id
AND comments.commentable_type = 'Article'
SQL
private_constant :ARTICLE_COMMENTABLE_QUERY
DEFAULT_PER_PAGE = 60
private_constant :DEFAULT_PER_PAGE
DEFAULT_SORT_BY = "comments.score".freeze
private_constant :DEFAULT_SORT_BY
DEFAULT_SORT_DIRECTION = :desc
private_constant :DEFAULT_SORT_DIRECTION
MAX_PER_PAGE = 120 # to avoid querying too many items, we set a maximum amount for a page
private_constant :MAX_PER_PAGE
# We filter comments for those that are:
# 1. On Articles
# 2. Not deleted
# 3. Not hidden by commentable user (i.e. an Article author didn't hide the comment)
# 4. Are attached to published articles
QUERY_FILTER = <<-SQL.freeze
comments.commentable_type = 'Article' AND
comments.deleted = false AND
comments.hidden_by_commentable_user = false AND
articles.published = true
SQL
private_constant :QUERY_FILTER
def self.search_documents(
page: 0,
per_page: DEFAULT_PER_PAGE,
sort_by: DEFAULT_SORT_BY,
sort_direction: DEFAULT_SORT_DIRECTION,
term: nil
)
sort_by ||= DEFAULT_SORT_BY
# The UI and serializer rename created_at (the actual DB column name) to
# published_at
sort_by = "comments.created_at" if sort_by == "published_at"
sort_direction ||= DEFAULT_SORT_DIRECTION
# NOTE: [@rhymes/atsmith813] 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 = ::Comment.joins(ARTICLE_COMMENTABLE_QUERY).where(QUERY_FILTER)
relation = relation.search_comments(term).with_pg_search_highlight if term.present?
relation = relation.select(*ATTRIBUTES).reorder("#{sort_by}": sort_direction)
results = relation.page(page).per(per_page)
# NOTE: [@rhymes/atsmith813] an earlier version used `.includes(:user)`
# to preload users, unfortunately it's not possible in Rails to specify
# which fields of the included relation's table to select ahead of time.
# The `users` table is massive (115 columns on March 2021) and thus we
# shouldn't load it all in memory just to select a few fields.
# For these reasons I decided to avoid preloading altogether and issue
# an additional SQL query to load User objects
# (see https://github.com/forem/forem/pull/4744#discussion_r345698674
# and https://github.com/rails/rails/issues/15185#issuecomment-351868335
# for additional context)
user_ids = results.pluck(:user_id)
users = find_users(user_ids)
serialize(results, users)
end
def self.find_users(user_ids)
::User
.where(id: user_ids)
.select(*USER_ATTRIBUTES)
.index_by(&:id)
end
private_class_method :find_users
def self.serialize(results, users)
Search::PostgresCommentSerializer
.new(results, params: { users: users }, is_collection: true)
.serializable_hash[:data]
.pluck(:attributes)
end
private_class_method :serialize
end
end
end

122
spec/requests/search_spec.rb
View file

@ -166,61 +166,87 @@ RSpec.describe "Search", type: :request, proper_status: true do
end
describe "GET /search/feed_content" do
let(:mock_documents) { [{ "title" => "article1" }] }
context "when using Elasticsearch" do
let(:mock_documents) { [{ "title" => "article1" }] }
it "returns json" do
allow(Search::FeedContent).to receive(:search_documents).and_return(
mock_documents,
)
it "returns json" do
allow(Search::FeedContent).to receive(:search_documents).and_return(
mock_documents,
)
get "/search/feed_content"
expect(response.parsed_body["result"]).to eq(mock_documents)
get "/search/feed_content"
expect(response.parsed_body["result"]).to eq(mock_documents)
end
it "queries only the user index if class_name=User" do
allow(Search::FeedContent).to receive(:search_documents)
allow(Search::User).to receive(:search_documents).and_return(
mock_documents,
)
get "/search/feed_content?class_name=User"
expect(Search::User).to have_received(:search_documents)
expect(Search::FeedContent).not_to have_received(:search_documents)
end
it "queries for Articles, Podcast Episodes and Users if no class_name filter is present" do
allow(Search::FeedContent).to receive(:search_documents).and_return(
mock_documents,
)
allow(Search::User).to receive(:search_documents).and_return(
mock_documents,
)
get "/search/feed_content"
expect(Search::User).to have_received(:search_documents)
expect(Search::FeedContent).to have_received(:search_documents)
end
it "queries for only Articles and Podcast Episodes if class_name!=User" do
allow(Search::FeedContent).to receive(:search_documents).and_return(
mock_documents,
)
allow(Search::User).to receive(:search_documents)
get "/search/feed_content?class_name=Article"
expect(Search::User).not_to have_received(:search_documents)
expect(Search::FeedContent).to have_received(:search_documents)
end
it "queries for approved" do
allow(Search::FeedContent).to receive(:search_documents).and_return(
mock_documents,
)
get "/search/feed_content?class_name=Article&approved=true"
expect(Search::FeedContent).to have_received(:search_documents).with(
params: { "approved" => "true", "class_name" => "Article" },
)
end
end
it "queries only the user index if class_name=User" do
allow(Search::FeedContent).to receive(:search_documents)
allow(Search::User).to receive(:search_documents).and_return(
mock_documents,
)
context "when using PostgreSQL" do
before do
allow(FeatureFlag).to receive(:enabled?).with(:search_2_comments).and_return(true)
end
get "/search/feed_content?class_name=User"
expect(Search::User).to have_received(:search_documents)
expect(Search::FeedContent).not_to have_received(:search_documents)
end
it "returns the correct keys for comments" do
create(:comment, body_markdown: "Ruby on Rails rocks!")
get search_feed_content_path(search_fields: "rails", class_name: "Comment")
expect(response.parsed_body["result"]).to be_present
end
it "queries for Articles, Podcast Episodes and Users if no class_name filter is present" do
allow(Search::FeedContent).to receive(:search_documents).and_return(
mock_documents,
)
allow(Search::User).to receive(:search_documents).and_return(
mock_documents,
)
it "supports the search params for comments" do
comment = create(:comment, body_markdown: "Ruby on Rails rocks!")
get search_feed_content_path(
search_fields: "rails",
class_name: "Comment",
page: 0,
per_page: 1,
)
get "/search/feed_content"
expect(Search::User).to have_received(:search_documents)
expect(Search::FeedContent).to have_received(:search_documents)
end
it "queries for only Articles and Podcast Episodes if class_name!=User" do
allow(Search::FeedContent).to receive(:search_documents).and_return(
mock_documents,
)
allow(Search::User).to receive(:search_documents)
get "/search/feed_content?class_name=Article"
expect(Search::User).not_to have_received(:search_documents)
expect(Search::FeedContent).to have_received(:search_documents)
end
it "queries for approved" do
allow(Search::FeedContent).to receive(:search_documents).and_return(
mock_documents,
)
get "/search/feed_content?class_name=Article&approved=true"
expect(Search::FeedContent).to have_received(:search_documents).with(
params: { "approved" => "true", "class_name" => "Article" },
)
expect(response.parsed_body["result"].first).to include("body_text" => comment.body_markdown)
end
end
end

118
spec/services/search/postgres/comment_spec.rb Normal file
View file

@ -0,0 +1,118 @@
require "rails_helper"
RSpec.describe Search::Postgres::Comment, type: :service do
let(:comment) { create(:comment) }
describe "::search_documents" do
context "when filtering Commentables" do
it "does not include comments from Articles that are unpublished", :aggregate_failures do
comment_text = "Ruby on Rails rocks!"
published_article = create(:article, title: "Published Article", published: true)
unpublished_article = create(:article, title: "Unpublished Article", published: true)
comment_on_published_article = create(:comment, body_markdown: comment_text, commentable: published_article)
comment_on_unpublished_article = create(:comment, body_markdown: comment_text, commentable: unpublished_article)
unpublished_article.update_columns(published: false)
result = described_class.search_documents(term: "rails")
# rubocop:disable Rails/PluckId
ids = result.pluck(:id)
# rubocop:enable Rails/PluckId
expect(ids).not_to include(comment_on_unpublished_article.search_id)
expect(ids).to include(comment_on_published_article.search_id)
end
end
context "when describing the result format" do
let(:result) { described_class.search_documents(term: comment.body_markdown) }
it "returns the correct attributes for the result" do
expected_keys = %i[
id path public_reactions_count body_text class_name highlight
hotness_score published published_at readable_publish_date_string
title user
]
expect(result.first.keys).to match_array(expected_keys)
end
it "returns the correct attributes for the user" do
expected_keys = %i[username name profile_image_90]
expect(result.first[:user].keys).to match_array(expected_keys)
end
it "returns highlights" do
expected_keys = %i[body_text]
expect(result.first[:highlight].keys).to match_array(expected_keys)
highlights = result.first[:highlight][:body_text].first
expect(highlights).to include("<mark>", "</mark>")
end
it "orders the results by score (hotness_score) in descending order by default" do
comment_text = "Ruby on Rails rocks!"
comment = create(:comment, body_markdown: comment_text, score: 0)
hotter_comment = create(:comment, body_markdown: comment_text, score: 99)
result = described_class.search_documents(term: "rails")
scores = result.pluck(:hotness_score)
expect(scores).to eq([hotter_comment.score, comment.score])
end
it "orders the results by published_at (created_at) in descending order" do
comment_text = "Ruby on Rails rocks!"
comment = create(:comment, body_markdown: comment_text, score: 0)
older_comment = create(:comment, body_markdown: comment_text, score: 99, created_at: 1.day.ago)
result = described_class.search_documents(sort_by: "published_at", sort_direction: "desc", term: "rails")
# rubocop:disable Rails/PluckId
ids = result.pluck(:id)
# rubocop:enable Rails/PluckId
expect(ids).to eq([comment.search_id, older_comment.search_id])
end
it "orders the results by published_at (created_at) in ascending order" do
comment_text = "Ruby on Rails rocks!"
comment = create(:comment, body_markdown: comment_text, score: 0)
older_comment = create(:comment, body_markdown: comment_text, score: 99, created_at: 1.day.ago)
result = described_class.search_documents(sort_by: "published_at", sort_direction: "asc", term: "rails")
# rubocop:disable Rails/PluckId
ids = result.pluck(:id)
# rubocop:enable Rails/PluckId
expect(ids).to eq([older_comment.search_id, comment.search_id])
end
end
context "when searching for a term" do
it "matches against the comment's body_markdown (body_text)", :aggregate_failures do
comment.update_columns(body_markdown: "Ruby on Rails rocks!")
result = described_class.search_documents(term: "rails")
expect(result.first[:body_text]).to eq comment.body_markdown
result = described_class.search_documents(term: "javascript")
expect(result).to be_empty
end
end
context "when paginating" do
before { create_list(:comment, 2) }
it "returns no results when out of pagination bounds" do
result = described_class.search_documents(page: 99)
expect(result).to be_empty
end
it "returns paginated results", :aggregate_failures do
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