diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 2a1b2893c..d9b742e24 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -75,11 +75,19 @@ class SearchController < ApplicationController end def listings - cl_docs = Search::Listing.search_documents( - params: listing_params.to_h, - ) + result = + if FeatureFlag.enabled?(:search_2_listings) + Search::Postgres::Listing.search_documents( + category: listing_params[:category], + page: listing_params[:page], + per_page: listing_params[:per_page], + term: listing_params[:listing_search], + ) + else + Search::Listing.search_documents(params: listing_params.to_h) + end - render json: { result: cl_docs } + render json: { result: result } end def users diff --git a/app/models/listing.rb b/app/models/listing.rb index 4b832cd08..eee54211c 100644 --- a/app/models/listing.rb +++ b/app/models/listing.rb @@ -4,6 +4,7 @@ class Listing < ApplicationRecord self.table_name = "classified_listings" include Searchable + include PgSearch::Model SEARCH_SERIALIZER = Search::ListingSerializer SEARCH_CLASS = Search::Listing @@ -32,6 +33,12 @@ class Listing < ApplicationRecord validate :restrict_markdown_input validate :validate_tags + # [@atsmith813] this is adapted from the `listing_search` property in + # `config/elasticsearch/mappings/listings.json` + pg_search_scope :search_listings, + against: %i[body_markdown cached_tag_list location slug title], + using: { tsearch: { prefix: true } } + scope :published, -> { where(published: true) } # NOTE: we still need to use the old column name for the join query diff --git a/app/serializers/search/listing_result_serializer.rb b/app/serializers/search/listing_result_serializer.rb new file mode 100644 index 000000000..53bdcce0e --- /dev/null +++ b/app/serializers/search/listing_result_serializer.rb @@ -0,0 +1,29 @@ +module Search + # TODO: [@atsmith813] Rename this to just ListingSerializer once Elasticsearch + # is fully removed + class ListingResultSerializer < ApplicationSerializer + attributes :id, + :body_markdown, + :bumped_at, + :category, + :contact_via_connect, + :expires_at, + :originally_published_at, + :location, + :processed_html, + :published, + :slug, + :title, + :user_id + + attribute :tags do |listing| + listing.cached_tag_list.to_s.split(", ") + end + + attribute :author do |listing| + ListingAuthorSerializer.new(listing.author) + .serializable_hash + .dig(:data, :attributes) + end + end +end diff --git a/app/services/search/postgres/listing.rb b/app/services/search/postgres/listing.rb new file mode 100644 index 000000000..74d3e1cd3 --- /dev/null +++ b/app/services/search/postgres/listing.rb @@ -0,0 +1,59 @@ +module Search + module Postgres + class Listing + ATTRIBUTES = %i[ + id + body_markdown + bumped_at + cached_tag_list + classified_listing_category_id + contact_via_connect + expires_at + organization_id + originally_published_at + location + processed_html + published + slug + title + user_id + ].freeze + private_constant :ATTRIBUTES + + DEFAULT_PER_PAGE = 75 + private_constant :DEFAULT_PER_PAGE + + MAX_PER_PAGE = 150 # to avoid querying too many items, we set a maximum amount for a page + private_constant :MAX_PER_PAGE + + def self.search_documents(category: nil, page: 0, per_page: DEFAULT_PER_PAGE, term: nil) + # 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 = ::Listing + .includes(:user, :organization, :listing_category) + .where(published: true) + + relation = relation.where("classified_listing_categories.slug": category) if category.present? + + relation = relation.search_listings(term) if term.present? + + relation = relation.select(*ATTRIBUTES).order(bumped_at: :desc) + + results = relation.page(page).per(per_page) + + serialize(results) + end + + def self.serialize(results) + Search::ListingResultSerializer + .new(results, is_collection: true) + .serializable_hash[:data] + .pluck(:attributes) + end + private_class_method :serialize + end + end +end diff --git a/db/migrate/20210326172406_add_published_index_to_listings.rb b/db/migrate/20210326172406_add_published_index_to_listings.rb new file mode 100644 index 000000000..7b70b32d4 --- /dev/null +++ b/db/migrate/20210326172406_add_published_index_to_listings.rb @@ -0,0 +1,15 @@ +class AddPublishedIndexToListings < ActiveRecord::Migration[6.0] + disable_ddl_transaction! + + def up + unless index_exists?(:classified_listings, :published) + add_index :classified_listings, :published, algorithm: :concurrently + end + end + + def down + if index_exists?(:classified_listings, :published) + remove_index :classified_listings, column: :published, algorithm: :concurrently + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 68caafc0e..d5c4717a3 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_03_25_183834) do +ActiveRecord::Schema.define(version: 2021_03_26_172406) do # These are extensions that must be enabled in order to support this database enable_extension "citext" @@ -344,6 +344,7 @@ ActiveRecord::Schema.define(version: 2021_03_25_183834) do t.bigint "user_id" t.index ["classified_listing_category_id"], name: "index_classified_listings_on_classified_listing_category_id" t.index ["organization_id"], name: "index_classified_listings_on_organization_id" + t.index ["published"], name: "index_classified_listings_on_published" t.index ["user_id"], name: "index_classified_listings_on_user_id" end diff --git a/spec/requests/search_spec.rb b/spec/requests/search_spec.rb index 8cedb4bf6..c829947ad 100644 --- a/spec/requests/search_spec.rb +++ b/spec/requests/search_spec.rb @@ -70,16 +70,43 @@ RSpec.describe "Search", type: :request, proper_status: true do end describe "GET /search/listings" do - let(:mock_documents) do - [{ "title" => "listing1" }] + context "when using Elasticsearch" do + let(:mock_documents) do + [{ "title" => "listing1" }] + end + + it "returns json" do + allow(Search::Listing).to receive(:search_documents).and_return( + mock_documents, + ) + get "/search/listings" + expect(response.parsed_body).to eq("result" => mock_documents) + end end - it "returns json" do - allow(Search::Listing).to receive(:search_documents).and_return( - mock_documents, - ) - get "/search/listings" - expect(response.parsed_body).to eq("result" => mock_documents) + context "when using PostgreSQL" do + before do + allow(FeatureFlag).to receive(:enabled?).with(:search_2_listings).and_return(true) + end + + it "returns the correct keys" do + create(:listing) + get search_listings_path + expect(response.parsed_body["result"]).to be_present + end + + it "supports the search params" do + listing = create(:listing) + + get search_listings_path( + category: listing.category, + page: 0, + per_page: 1, + term: listing.title.downcase, + ) + + expect(response.parsed_body["result"].first).to include("title" => listing.title) + end end end diff --git a/spec/services/search/postgres/listing_spec.rb b/spec/services/search/postgres/listing_spec.rb new file mode 100644 index 000000000..9c711705d --- /dev/null +++ b/spec/services/search/postgres/listing_spec.rb @@ -0,0 +1,134 @@ +require "rails_helper" + +RSpec.describe Search::Postgres::Listing, type: :service do + let(:listing) { create(:listing) } + + describe "::search_documents" do + it "does not include a listing that is unpublished", :aggregate_failures do + published_listing = create(:listing, title: "Published Listing", published: true) + unpublished_listing = create(:listing, title: "Unpublished Listing", published: false) + result = described_class.search_documents(term: "Listing") + titles = result.pluck(:title) + + expect(titles).not_to include(unpublished_listing.title) + expect(titles).to include(published_listing.title) + end + + context "when describing the result format" do + let(:result) { described_class.search_documents(term: listing.title) } + + it "returns the correct attributes for the result" do + expected_keys = %i[ + id body_markdown bumped_at category contact_via_connect expires_at + originally_published_at location processed_html published slug title + user_id tags author + ] + + expect(result.first.keys).to match_array(expected_keys) + end + + it "returns the correct attributes for the author" do + expected_keys = %i[username name profile_image_90] + expect(result.first[:author].keys).to match_array(expected_keys) + end + + it "returns tag as an Array" do + expect(result.first[:tags]).to be_an_instance_of(Array) + end + end + + context "when searching for a term" do + it "matches against the listing's body_markdown", :aggregate_failures do + listing.update_columns(body_markdown: "A Sweet New Opportunity") + result = described_class.search_documents(term: "new") + + expect(result.first[:body_markdown]).to eq listing.body_markdown + + result = described_class.search_documents(term: "old") + expect(result).to be_empty + end + + it "matches against the listing's cached_tag_list", :aggregate_failures do + listing.update_columns(cached_tag_list: "javascript, beginners, ruby") + result = described_class.search_documents(term: "beginner") + + expect(result.first[:tags].join(", ")).to eq listing.cached_tag_list + + result = described_class.search_documents(term: "newbie") + expect(result).to be_empty + end + + it "matches against the listing's location", :aggregate_failures do + listing.update_columns(location: "Tampa") + result = described_class.search_documents(term: "tampa") + + expect(result.first[:location]).to eq listing.location + + result = described_class.search_documents(term: "milan") + expect(result).to be_empty + end + + it "matches against the listing's slug", :aggregate_failures do + listing.update_columns(slug: "some-cool-slug") + result = described_class.search_documents(term: "cool") + + expect(result.first[:slug]).to eq listing.slug + + result = described_class.search_documents(term: "lame") + expect(result).to be_empty + end + + it "matches against the listing's title", :aggregate_failures do + listing.update_columns(title: "Awesome New Listing") + result = described_class.search_documents(term: "new") + + expect(result.first[:title]).to eq listing.title + + result = described_class.search_documents(term: "old") + expect(result).to be_empty + end + end + + context "when searching for a term and filtering by category" do + it "selects results with the requested category" do + job_listings_category = create(:listing_category, name: "Job Listings", slug: "jobs") + + job_listing = create(:listing, + title: "Looking for a Ruby on Rails Developer!", + listing_category: job_listings_category) + + education_category = create(:listing_category, name: "Education/Courses", slug: "education") + + listing.update_columns( + title: "New Ruby on Rails for Beginners Course!", + classified_listing_category_id: education_category.id, + ) + + result = described_class.search_documents(term: "Ruby on Rails", category: job_listing.category) + # rubocop:disable Rails/PluckId + ids = result.pluck(:id) + # rubocop:enable Rails/PluckId + + expect(ids).to include(job_listing.id) + expect(ids).not_to include(listing.id) + end + end + + context "when paginating" do + before { create_list(:listing, 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