From 83852038e46ff2506703b23c34d93ccf4a24a3ad Mon Sep 17 00:00:00 2001 From: rhymes Date: Wed, 24 Mar 2021 15:40:00 +0100 Subject: [PATCH] [Search 2.0] Reading list (#13052) * Step one in populating the reading list with PG This first attempt tries to recycle the `Search::ArticleSerializer` which is only used in input in ES, but we're using it in output in PG. For this reason it's currently 15.55x times slower * Serialize only what is requested by the frontend `Search::ArticleSerializer` which is only used in ES in the indexing step aims to add as much info as possible for broader purposes, in this case (with serialization in output) we should aim to save only what's requested from the frontend. * Optimize selection of articles columns * Select only needed columns for users * Compute total of reading list items * Attach the basic filtering based on PG on the search controller * Restructure in methods * Add tags support * Use LIKE on articles.cached_tag_list * Fix tags as nil * Fix default pagination * Add optional FTS for reading list * Reworded the tags comment explaining why * Add index to reactions.status * Fix total counter in Preact readingList component * Fix total count in reading list backend search * Add GIN index to articles.cached_tag_list * Add service tests * Add search request specs * Added missing early return * Update spec/requests/search_spec.rb Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com> * Extract MAX_PER_PAGE constant and add comments Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com> --- app/controllers/search_controller.rb | 23 +- app/javascript/readingList/readingList.jsx | 3 +- .../searchableItemList/searchableItemList.js | 5 + app/models/article.rb | 11 + app/models/tag.rb | 7 +- .../search/reading_list_article_serializer.rb | 36 ++ app/services/search/postgres/reading_list.rb | 123 +++++++ ...319185315_add_index_to_reactions_status.rb | 7 + ...7_add_index_to_articles_cached_tag_list.rb | 9 + db/schema.rb | 2 + spec/requests/search_spec.rb | 66 +++- .../search/postgres/reading_list_spec.rb | 327 ++++++++++++++++++ 12 files changed, 592 insertions(+), 27 deletions(-) create mode 100644 app/serializers/search/reading_list_article_serializer.rb create mode 100644 app/services/search/postgres/reading_list.rb create mode 100644 db/migrate/20210319185315_add_index_to_reactions_status.rb create mode 100644 db/migrate/20210322152837_add_index_to_articles_cached_tag_list.rb create mode 100644 spec/services/search/postgres/reading_list_spec.rb diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 6814631d2..2a1b2893c 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -119,11 +119,26 @@ class SearchController < ApplicationController end def reactions - result = Search::ReadingList.search_documents( - params: reaction_params.to_h, user: current_user, - ) + if FeatureFlag.enabled?(:search_2_reading_list) + # [@rhymes] we're recyling the existing params as we want to change the frontend as + # little as possible, we might simplify in the future + result = Search::Postgres::ReadingList.search_documents( + current_user, + page: reaction_params[:page], + per_page: reaction_params[:per_page], + statuses: reaction_params[:status], + tags: reaction_params[:tag_names], + term: reaction_params[:search_fields], + ) - render json: { result: result["reactions"], total: result["total"] } + render json: { result: result[:items], total: result[:total] } + else + result = Search::ReadingList.search_documents( + params: reaction_params.to_h, user: current_user, + ) + + render json: { result: result["reactions"], total: result["total"] } + end end private diff --git a/app/javascript/readingList/readingList.jsx b/app/javascript/readingList/readingList.jsx index b60194469..135947f1d 100644 --- a/app/javascript/readingList/readingList.jsx +++ b/app/javascript/readingList/readingList.jsx @@ -158,6 +158,7 @@ export class ReadingList extends Component { render() { const { items = [], + itemsTotal, availableTags, selectedTags, showLoadMoreButton, @@ -190,7 +191,7 @@ export class ReadingList extends Component {

{isStatusViewValid ? 'Reading list' : 'Archive'} - {` (${items.length})`} + {` (${itemsTotal})`}

diff --git a/app/javascript/searchableItemList/searchableItemList.js b/app/javascript/searchableItemList/searchableItemList.js index f8eb1f8b8..5c5b9b80b 100644 --- a/app/javascript/searchableItemList/searchableItemList.js +++ b/app/javascript/searchableItemList/searchableItemList.js @@ -12,6 +12,7 @@ export function defaultState(options) { items: [], itemsLoaded: false, + itemsTotal: 0, availableTags: [], selectedTags: [], @@ -74,6 +75,8 @@ export function performInitialSearch({ searchOptions = {} }) { const responsePromise = fetchSearch('reactions', dataHash); return responsePromise.then((response) => { const reactions = response.result; + // FIXME: [@rhymes] the list of tags in the left column of the reading list + // is populated with only the tags belonging to items in the first page const availableTags = [ ...new Set(reactions.flatMap((rxn) => rxn.reactable.tag_list)), ].sort(); @@ -81,6 +84,7 @@ export function performInitialSearch({ searchOptions = {} }) { page: 0, items: reactions, itemsLoaded: true, + itemsTotal: response.total, showLoadMoreButton: hitsPerPage < response.total, availableTags, }); @@ -128,6 +132,7 @@ export function search(query, { page, tags, statusView, appendItems = false }) { query, page: newPage, items, + itemsTotal: response.total, showLoadMoreButton: items.length < response.total, }); }); diff --git a/app/models/article.rb b/app/models/article.rb index b4e7b1b29..448478ddf 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -7,6 +7,7 @@ class Article < ApplicationRecord include Reactable include Searchable include UserSubscriptionSourceable + include PgSearch::Model SEARCH_SERIALIZER = Search::ArticleSerializer SEARCH_CLASS = Search::FeedContent @@ -111,6 +112,16 @@ class Article < ApplicationRecord serialize :cached_user serialize :cached_organization + # [@rhymes] this is adapted from the `search_fields` property in + # `config/elasticsearch/mappings/feed_content.json` + pg_search_scope :search_reading_list, + against: %i[body_markdown title cached_tag_list], + associated_against: { + organization: %i[name], + user: %i[name username] + }, + using: { tsearch: { prefix: true } } + # [@jgaskins] We use an index on `published`, but since it's a boolean value # the Postgres query planner often skips it due to lack of diversity of the # data in the column. However, since `published_at` is a *very* diverse diff --git a/app/models/tag.rb b/app/models/tag.rb index ef535cbee..2edc59293 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -13,9 +13,6 @@ class Tag < ActsAsTaggableOn::Tag include Searchable include PgSearch::Model - pg_search_scope :search_by_name, - against: :name, - using: { tsearch: { prefix: true } } ALLOWED_CATEGORIES = %w[uncategorized language library tool site_mechanic location subcommunity].freeze HEX_COLOR_REGEXP = /\A#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})\z/.freeze @@ -48,6 +45,10 @@ class Tag < ActsAsTaggableOn::Tag after_commit :sync_related_elasticsearch_docs, on: [:update] after_commit :remove_from_elasticsearch, on: [:destroy] + pg_search_scope :search_by_name, + against: :name, + using: { tsearch: { prefix: true } } + scope :eager_load_serialized_data, -> {} SEARCH_SERIALIZER = Search::TagSerializer diff --git a/app/serializers/search/reading_list_article_serializer.rb b/app/serializers/search/reading_list_article_serializer.rb new file mode 100644 index 000000000..86009a719 --- /dev/null +++ b/app/serializers/search/reading_list_article_serializer.rb @@ -0,0 +1,36 @@ +module Search + class ReadingListArticleSerializer < ApplicationSerializer + attribute :id, &:reaction_id + attribute :user_id, &:reaction_user_id + + # NOTE: the list of attributes for `reactable` comes from two places: + # => the `` Preact component: https://github.com/forem/forem/blob/33d0e03dbd94fc6797693b84fcafb5040ea399d0/app/javascript/readingList/components/ItemListItem.jsx#L72-L85 + # => the `performInitialSearch` function: https://github.com/forem/forem/blob/33d0e03dbd94fc6797693b84fcafb5040ea399d0/app/javascript/searchableItemList/searchableItemList.js#L78 + attribute :reactable do |article, params| + user = params[:users][article.user_id] + tags = article.cached_tag_list.to_s.split(", ") + + { + path: article.path, + readable_publish_date_string: article.readable_publish_date, + reading_time: article.reading_time, + + # TODO: once we switch to PG we should revisit how we handle tags in the + # frontend, we're sending back tags twice in slightly different formats + tag_list: tags, + tags: tags.map { |tag| { name: tag } }, + + title: article.title, + + # NOTE: not using the `NestedUserSerializer` because we don't need the + # the `pro` flag on the frontend, and we also avoid hitting Redis to + # fetch the cached value + user: { + name: user.name, + profile_image_90: user.profile_image_90, + username: user.username + } + } + end + end +end diff --git a/app/services/search/postgres/reading_list.rb b/app/services/search/postgres/reading_list.rb new file mode 100644 index 000000000..dca6aef71 --- /dev/null +++ b/app/services/search/postgres/reading_list.rb @@ -0,0 +1,123 @@ +module Search + module Postgres + class ReadingList + ATTRIBUTES = [ + "articles.cached_tag_list", + "articles.crossposted_at", + "articles.path", + "articles.published_at", + "articles.reading_time", + "articles.title", + "articles.user_id", + "reactions.id AS reaction_id", + "reactions.user_id AS reaction_user_id", + ].freeze + USER_ATTRIBUTES = %i[id name profile_image username].freeze + + DEFAULT_STATUSES = %w[confirmed valid].freeze + + DEFAULT_PER_PAGE = 60 + MAX_PER_PAGE = 100 # to avoid querying too many items, we set a maximum amount for a page + + def self.search_documents(user, term: nil, statuses: [], tags: [], page: 0, per_page: DEFAULT_PER_PAGE) + return {} unless user + + statuses = statuses.presence || DEFAULT_STATUSES + tags = tags.presence || [] + + # NOTE: [@rhymes] 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 + + result = find_articles( + user_id: user.id, + term: term, + statuses: statuses, + tags: tags, + page: page, + per_page: per_page, + ) + + # NOTE: [@rhymes] an earlier version used `Article.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 = result[:items].pluck(:user_id) + users = find_users(user_ids) + + { + items: serialize(result[:items], users), + total: result[:total] + } + end + + def self.find_articles(user_id:, term:, statuses:, tags:, page:, per_page:) + relation = ::Article + .joins(:reactions) + .where("reactions.category": :readinglist) + .where("reactions.user_id": user_id) + .where("reactions.status": statuses) + + relation = relation.search_reading_list(term) if term.present? + + # NOTE: [@rhymes] A previous version was implemented with: + # `.tagged_with(tags, any: false).reselect(*ATTRIBUTES)` + # + # =>`.tagged_with()` merges `articles.*` to the SQL, thus we needed to + # use `reselect()`, see https://github.com/forem/forem/pull/12420 + # => `.tagged_with()` with multiple tags constructs a monster query, + # see https://explain.depesz.com/s/CqQV / https://explain.dalibo.com/plan/1Lm + # This is because the `acts-as-taggable-on` query creates a separate INNER JOIN + # per each tag that is added to the list, each new clause uses the `LIKE` operator on `tags.name`. + # That could have been improved by by adding a GIN index on `tags.name`, see + # https://www.cybertec-postgresql.com/en/postgresql-more-performance-for-like-and-ilike-statements/ + # and a similar discussion https://github.com/forem/forem/pull/12584#discussion_r570756176 + # + # An alternative solution, as we don't need the `Tag` model itself, is to use + # `articles.cached_tag_list` and the `LIKE` operator on it, this could be further + # improved, if needed, by adding a GIN index on `cached_tag_list` + # It seems not to be needed as this approach is roughly 1850 times faster than the previous + # see https://explain.depesz.com/s/ajoP / https://explain.dalibo.com/plan/PZb + tags.each do |tag| + relation = relation.where("articles.cached_tag_list LIKE ?", "%#{tag}%") + end + + # here we issue a COUNT(*) after all the conditions are applied, + # because we need to fetch the total number of articles, pre pagination + total = relation.count + + relation = relation.select(*ATTRIBUTES).order("reactions.created_at": :desc) + relation = relation.page(page).per(per_page) + + { + items: relation, + total: total + } + end + private_class_method :find_articles + + 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(articles, users) + Search::ReadingListArticleSerializer + .new(articles, params: { users: users }, is_collection: true) + .serializable_hash[:data] + .pluck(:attributes) + end + private_class_method :serialize + end + end +end diff --git a/db/migrate/20210319185315_add_index_to_reactions_status.rb b/db/migrate/20210319185315_add_index_to_reactions_status.rb new file mode 100644 index 000000000..1cd745fcb --- /dev/null +++ b/db/migrate/20210319185315_add_index_to_reactions_status.rb @@ -0,0 +1,7 @@ +class AddIndexToReactionsStatus < ActiveRecord::Migration[6.0] + disable_ddl_transaction! + + def change + add_index :reactions, :status, algorithm: :concurrently + end +end diff --git a/db/migrate/20210322152837_add_index_to_articles_cached_tag_list.rb b/db/migrate/20210322152837_add_index_to_articles_cached_tag_list.rb new file mode 100644 index 000000000..07d2a5976 --- /dev/null +++ b/db/migrate/20210322152837_add_index_to_articles_cached_tag_list.rb @@ -0,0 +1,9 @@ +class AddIndexToArticlesCachedTagList < ActiveRecord::Migration[6.0] + disable_ddl_transaction! + + def change + # From + # We need a GIN index on `cached_tag_list` to speed up `LIKE` queries + add_index :articles, :cached_tag_list, using: :gin, opclass: :gin_trgm_ops, algorithm: :concurrently + end +end diff --git a/db/schema.rb b/db/schema.rb index b9de1234b..861e2186f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -156,6 +156,7 @@ ActiveRecord::Schema.define(version: 2021_03_23_190443) do t.string "video_thumbnail_url" t.index "user_id, title, digest(body_markdown, 'sha512'::text)", name: "index_articles_on_user_id_and_title_and_digest_body_markdown", unique: true t.index ["boost_states"], name: "index_articles_on_boost_states", using: :gin + t.index ["cached_tag_list"], name: "index_articles_on_cached_tag_list", opclass: :gin_trgm_ops, using: :gin t.index ["canonical_url"], name: "index_articles_on_canonical_url", unique: true t.index ["collection_id"], name: "index_articles_on_collection_id" t.index ["comment_score"], name: "index_articles_on_comment_score" @@ -1036,6 +1037,7 @@ ActiveRecord::Schema.define(version: 2021_03_23_190443) do t.index ["points"], name: "index_reactions_on_points" t.index ["reactable_id", "reactable_type"], name: "index_reactions_on_reactable_id_and_reactable_type" t.index ["reactable_type"], name: "index_reactions_on_reactable_type" + t.index ["status"], name: "index_reactions_on_status" t.index ["user_id", "reactable_id", "reactable_type", "category"], name: "index_reactions_on_user_id_reactable_id_reactable_type_category", unique: true end diff --git a/spec/requests/search_spec.rb b/spec/requests/search_spec.rb index 11b7b3292..8cedb4bf6 100644 --- a/spec/requests/search_spec.rb +++ b/spec/requests/search_spec.rb @@ -198,29 +198,57 @@ RSpec.describe "Search", type: :request, proper_status: true do end describe "GET /search/reactions" do - let(:authorized_user) { create(:user) } - let(:mock_response) do - { "reactions" => [{ id: 123 }], "total" => 100 } + before do + sign_in authorized_user end - it "returns json with reactions and total" do - sign_in authorized_user - allow(Search::ReadingList).to receive(:search_documents).and_return( - mock_response, - ) - get "/search/reactions" - expect(response.parsed_body).to eq("result" => [{ "id" => 123 }], "total" => 100) + context "when using Elasticsearch" do + let(:mock_response) do + { "reactions" => [{ id: 123 }], "total" => 100 } + end + + before do + allow(Search::ReadingList).to receive(:search_documents).and_return(mock_response) + end + + it "returns json with reactions and total" do + get search_reactions_path + + expect(response.parsed_body).to eq("result" => [{ "id" => 123 }], "total" => 100) + end + + it "accepts array of tag names" do + get search_reactions_path(tag_names: [1, 2]) + + expect(Search::ReadingList).to( + have_received(:search_documents) + .with(params: { "tag_names" => %w[1 2] }, user: authorized_user), + ) + end end - it "accepts array of tag names" do - sign_in authorized_user - allow(Search::ReadingList).to receive(:search_documents).and_return( - mock_response, - ) - get "/search/reactions?tag_names[]=1&tag_names[]=2" - expect(Search::ReadingList).to have_received( - :search_documents, - ).with(params: { "tag_names" => %w[1 2] }, user: authorized_user) + context "when using PostgreSQL" do + let(:article) { create(:article) } + + before do + allow(FeatureFlag).to receive(:enabled?).with(:search_2_reading_list).and_return(true) + create(:reaction, category: :readinglist, reactable: article, user: authorized_user) + end + + it "returns the correct keys", :aggregate_failures do + get search_reactions_path + + expect(response.parsed_body["result"]).to be_present + expect(response.parsed_body["total"]).to eq(1) + end + + it "supports the search params" do + article.update_columns(title: "Title", cached_tag_list: "ruby, python") + + get search_reactions_path(page: 0, per_page: 1, status: %w[valid], tags: %w[ruby], term: "title") + + expect(response.parsed_body["result"].first["reactable"]).to include("title" => "Title") + end end end end diff --git a/spec/services/search/postgres/reading_list_spec.rb b/spec/services/search/postgres/reading_list_spec.rb new file mode 100644 index 000000000..c99576b33 --- /dev/null +++ b/spec/services/search/postgres/reading_list_spec.rb @@ -0,0 +1,327 @@ +require "rails_helper" + +RSpec.describe Search::Postgres::ReadingList, type: :service do + let(:user) { create(:user) } + let(:article) { create(:article) } + let(:article_not_in_reading_list) { create(:article) } + let(:article_1) { create(:article, with_tags: false) } + let(:article_2) { create(:article, with_tags: false) } + + def extract_from_results(result, attribute) + result[:items].pluck(:reactable).pluck(attribute) + end + + describe "::search_documents" do + before do + create(:reaction, user: user, reactable: article, category: :readinglist, status: :valid) + end + + it "returns an empty result without a user" do + expect(described_class.search_documents(nil)).to be_empty + end + + it "does not include an article not in the reading list" do + result = described_class.search_documents(user) + expect(extract_from_results(result, :path)).not_to include(article_not_in_reading_list.path) + end + + it "does not return an article belonging to another user's reading list" do + create( + :reaction, + reactable: article_not_in_reading_list, + category: :readinglist, + user: article_not_in_reading_list.user, + ) + + result = described_class.search_documents(user) + expect(extract_from_results(result, :path)).not_to include(article_not_in_reading_list.path) + end + + it "returns results of articles in the reading list" do + result = described_class.search_documents(user) + expect(extract_from_results(result, :path)).to include(article.path) + end + + it "returns the total count of the articles in the reading list" do + articles = create_list(:article, 2) + articles.each { |article| create(:reaction, category: :readinglist, reactable: article, user: user) } + + result = described_class.search_documents(user) + expect(result[:total]).to eq(user.reactions.readinglist.count) + end + + context "when describing the result format" do + let(:result) { described_class.search_documents(user) } + + it "returns the correct attributes for the result", :aggregate_failures do + expect(result.keys).to match_array(%i[items total]) + expect(result[:total]).to be_present + expect(result[:items]).to be_present + end + + it "returns the correct attributes for a reading list item", :aggregate_failures do + item = result[:items].first + + expect(item.keys).to match_array(%i[id user_id reactable]) + expect(item[:id]).to eq(user.reactions.readinglist.first.id) + expect(item[:user_id]).to eq(user.id) + end + + it "returns the correct attributes for a reading list item's reactable", :aggregate_failures do + item = result[:items].first + reactable = item[:reactable] + + expect(reactable.keys).to match_array( + %i[ + path readable_publish_date_string reading_time + tag_list tags title user + ], + ) + expect(reactable[:path]).to eq(article.path) + expect(reactable[:readable_publish_date_string]).to eq(article.readable_publish_date) + expect(reactable[:reading_time]).to eq(article.reading_time) + tags = article.cached_tag_list.to_s.split(", ") + expect(reactable[:tag_list]).to eq(tags) + expect(reactable[:tags]).to eq(tags.map { |tag| { name: tag } }) + expect(reactable[:title]).to eq(article.title) + end + + it "returns the correct attributes for a reading list item's reactable's user", :aggregate_failures do + item = result[:items].first + reactable = item[:reactable] + + reactable_user = reactable[:user] + expect(reactable_user.keys).to eq(%i[name profile_image_90 username]) + expect(reactable_user[:name]).to eq(article.user.name) + expect(reactable_user[:profile_image_90]).to eq(article.user.profile_image_90) + expect(reactable_user[:username]).to eq(article.user.username) + end + end + + context "when filtering by statuses" do + it "returns confirmed items by default" do + item = user.reactions.readinglist.last + item.update_columns(status: :confirmed) + + expect(described_class.search_documents(user)[:items].first[:id]).to eq(item.id) + end + + it "returns valid items by default" do + item = user.reactions.readinglist.last + item.update_columns(status: :valid) + + expect(described_class.search_documents(user)[:items].first[:id]).to eq(item.id) + end + + it "selects archived items upon request", :aggregate_failures do + valid_item = user.reactions.readinglist.last + archived_item = create(:reaction, user: user, category: :readinglist, status: :archived) + + # rubocop:disable Rails/PluckId + ids = described_class.search_documents(user, statuses: :archived)[:items].pluck(:id) + # rubocop:enable Rails/PluckId + + expect(ids).to include(archived_item.id) + expect(ids).not_to include(valid_item.id) + end + end + + context "when filtering by tags" do + after do + user.reactions.readinglist.delete_all + end + + it "filters out items belonging to an article without the requested tag" do + article_1.tag_list.add(:beginners) + article_1.save! + + create(:reaction, reactable: article_1, user: user, category: :readinglist) + + result = described_class.search_documents(user, tags: [:foobar]) + expect(extract_from_results(result, :path)).to be_empty + end + + it "selects items belonging to an article with the requested tag" do + article_1.tag_list.add(:beginners) + article_1.save! + + create(:reaction, reactable: article_1, user: user, category: :readinglist) + + result = described_class.search_documents(user, tags: [:beginners]) + expect(extract_from_results(result, :path)).to match_array([article_1.path]) + end + + it "selects items belonging to an article with all the requested tags", :aggregate_failures do + article_1.tag_list.add(:beginners) + article_1.tag_list.add(:ruby) + article_1.save! + + article_2.tag_list.add(:beginners) + article_2.save! + + create(:reaction, reactable: article_1, user: user, category: :readinglist) + create(:reaction, reactable: article_2, user: user, category: :readinglist) + + result = described_class.search_documents(user, tags: %i[beginners ruby]) + expect(extract_from_results(result, :path)).to include(article_1.path) + expect(extract_from_results(result, :path)).not_to include(article_2.path) + end + end + + context "when filtering by statuses and tags" do + it "selects items with the requested statuses and articles tags", :aggregate_failures do + article_1.tag_list.add(:beginners) + article_2.tag_list.add(:beginners) + article_1.save! + article_2.save! + + valid_item = create(:reaction, category: :readinglist, user: user, reactable: article_1, status: :valid) + archived_item = create(:reaction, category: :readinglist, user: user, reactable: article_2, status: :archived) + + result = described_class.search_documents(user, statuses: %i[valid], tags: %i[beginners]) + + # rubocop:disable Rails/PluckId + item_ids = result[:items].pluck(:id) + # rubocop:enable Rails/PluckId + expect(item_ids).to include(valid_item.id) + expect(item_ids).not_to include(archived_item.id) + end + end + + context "when searching for a term" do + it "matches against the article's body_markdown", :aggregate_failures do + article.update_columns(body_markdown: "Life of the party") + + result = described_class.search_documents(user, term: "part") + expect(extract_from_results(result, :path)).to include(article.path) + + result = described_class.search_documents(user, term: "fiesta") + expect(extract_from_results(result, :path)).to be_empty + end + + it "matches against the article's title", :aggregate_failures do + article.update_columns(title: "Life of the party") + + result = described_class.search_documents(user, term: "part") + expect(extract_from_results(result, :path)).to include(article.path) + + result = described_class.search_documents(user, term: "fiesta") + expect(extract_from_results(result, :path)).to be_empty + end + + it "matches against the article's tags", :aggregate_failures do + article.update_columns(cached_tag_list: "javascript, beginners, ruby") + + result = described_class.search_documents(user, term: "beginner") + expect(extract_from_results(result, :path)).to include(article.path) + + result = described_class.search_documents(user, term: "newbie") + expect(extract_from_results(result, :path)).to be_empty + end + + it "matches against the article's organization's name", :aggregate_failures do + article.organization = create(:organization, name: "ACME corp") + article.save! + + result = described_class.search_documents(user, term: "ACME") + expect(extract_from_results(result, :path)).to include(article.path) + + result = described_class.search_documents(user, term: "ECMA") + expect(extract_from_results(result, :path)).to be_empty + end + + it "matches against the article's user's name", :aggregate_failures do + article_user = article.user + article_user.update_columns(name: "Friday Sunday") + + result = described_class.search_documents(user, term: "Frida") + expect(extract_from_results(result, :path)).to include(article.path) + + result = described_class.search_documents(user, term: "Sat") + expect(extract_from_results(result, :path)).to be_empty + end + + it "matches against the article's user's username", :aggregate_failures do + article_user = article.user + article_user.update_columns(username: "fridaysunday") + + result = described_class.search_documents(user, term: "Frida") + expect(extract_from_results(result, :path)).to include(article.path) + + result = described_class.search_documents(user, term: "Sat") + expect(extract_from_results(result, :path)).to be_empty + end + end + + context "when searching for a term and filtering by statuses" do + it "selects items with the requested status belonging to articles matching the term", :aggregate_failures do + article.update_columns(body_markdown: "Life of the party") + + result = described_class.search_documents(user, term: "part", statuses: %i[valid]) + expect(extract_from_results(result, :path)).to include(article.path) + + result = described_class.search_documents(user, term: "part", statuses: %i[archived]) + expect(extract_from_results(result, :path)).to be_empty + + result = described_class.search_documents(user, term: "fiesta", statuses: %i[valid]) + expect(extract_from_results(result, :path)).to be_empty + end + end + + context "when searching for a term and filtering by tags" do + it "selects items with the requested status belonging to articles matching the term", :aggregate_failures do + article.update_columns(body_markdown: "Life of the party", cached_tag_list: "javascript, beginners, ruby") + + result = described_class.search_documents(user, term: "part", tags: %i[javascript]) + expect(extract_from_results(result, :path)).to include(article.path) + + result = described_class.search_documents(user, term: "part", tags: %i[python]) + expect(extract_from_results(result, :path)).to be_empty + + result = described_class.search_documents(user, term: "fiesta", tags: %i[javascript]) + expect(extract_from_results(result, :path)).to be_empty + end + end + + context "when searching for a term and filtering by statuses and tags" do + it "selects items with the requested status belonging to articles matching the term", :aggregate_failures do + article.update_columns(body_markdown: "Life of the party", cached_tag_list: "javascript, beginners, ruby") + + result = described_class.search_documents(user, term: "part", statuses: %i[valid], tags: %i[javascript]) + expect(extract_from_results(result, :path)).to include(article.path) + + result = described_class.search_documents(user, term: "part", statuses: %i[archived], tags: %i[javascript]) + expect(extract_from_results(result, :path)).to be_empty + + result = described_class.search_documents(user, term: "part", statuses: %i[valid], tags: %i[python]) + expect(extract_from_results(result, :path)).to be_empty + end + end + + context "when paginating" do + let(:articles) { create_list(:article, 2) } + + before do + articles.each { |article| create(:reaction, category: :readinglist, reactable: article, user: user) } + end + + it "returns the total count of the articles pre-pagination" do + result = described_class.search_documents(user, page: 1, per_page: 1) + expect(result[:total]).to eq(user.reactions.readinglist.count) + end + + it "returns no items when out of pagination bounds" do + result = described_class.search_documents(user, page: 99) + expect(result[:items]).to be_empty + end + + it "returns paginated items", :aggregate_failures do + result = described_class.search_documents(user, page: 1, per_page: 1) + expect(result[:items].length).to eq(1) + + result = described_class.search_documents(user, page: 2, per_page: 1) + expect(result[:items].length).to eq(1) + end + end + end +end